mirror of
https://github.com/akitaonrails/ai-memory.git
synced 2026-10-02 03:24:46 +08:00
fix(embedding): preserve prefix whitespace through env loading; regression tests
figment's Env provider parses each var's string with its loose-value
parser, whose bare/unquoted branch calls .trim() (figment 0.10.19's
src/value/parse.rs:78, verified against the vendored source) — so
AI_MEMORY_EMBEDDING_QUERY_PREFIX="query: " reached Config::embedding_query_prefix
as "query:", silently dropping the publisher-significant trailing space.
Config::load now overlays the raw (untrimmed) env value for the two prefix
keys via figment::providers::Serialized (which hands figment an
already-typed value, bypassing the string parser), merged after the
generic Env::prefixed pass so it still wins over a config.toml value.
Presence, not non-emptiness, is the signal: a variable set to "" is a
deliberate override clearing a config.toml-configured prefix, distinct
from the variable being absent. Both wrapper scripts (bin/ai-memory,
bin/ai-memory.ps1) now forward these two keys on presence for the same
reason; a non-empty override was already forwarded correctly, only the
empty-override case was silently dropped by the [ -n ] check every other
forwarded var correctly uses.
The overlay itself lives in overlay_embedding_prefixes(), a pure function
taking the env values as parameters rather than reading std::env::var
itself, so it stays directly unit-testable without mutating process
environment or the current directory — the same pattern
ai-memory-cli/src/commands/path_util.rs's agent_config_home and
ai-memory-hooks's drain_with_live_token already use. std::env::set_var is
unsafe under edition 2024 and forbidden workspace-wide, and
figment::Jail calls std::env::set_current_dir on the real process
internally, racing every other test in this crate's multi-threaded lib
test binary that relies on cwd. Four pure unit tests build a minimal
in-memory Figment and assert on the merged Config directly. One
additional test exercises the real Config::load end to end through a
TOML file and an explicit absolute path; since this process's own
environment is shared with every other test in the binary and could
already carry one of the two prefix vars from the test runner's shell,
that test re-execs this same test binary filtered to just itself as a
genuinely separate child process, with both vars removed via
Command::env_remove — real process isolation rather than an in-process
assumption about the ambient environment.
Plus: a wiremock transport test proving OpenAiEmbedder (not just
OpenAiCompatEmbedder) sends the configured prefix on the wire; a
regression test for the memory_query embed_query fix from the prior
commit, using a task-aware fixture embedder whose
embed/embed_document/embed_query methods return distinguishable vectors
so a regression back to calling the wrong one fails a direct equality
assertion rather than an inferred ranking change; fake-Docker argument
tests for the wrapper's presence-based forwarding across
unset/empty/whitespace-only/non-empty, with both env vars explicitly
removed from the child environment before each case so the "unset" case
cannot silently inherit an ambient export from the test runner.
bin/ai-memory.ps1 also notes, briefly, the PowerShell/.NET version an
operator needs for `$env:NAME = ''` to reach the wrapper as set-but-empty
rather than deleted — no pwsh runtime was available to exercise it live.
Also narrows the query/document-prefix docs (config.rs, docs/llm-providers.md,
CHANGELOG.md) with exact, separate templates: Nemotron-3-Embed
(nvidia/Nemotron-3-Embed-1B-BF16) and base E5 use "query: "/"passage: ";
e5-mistral-7b-instruct wants "Instruct: {task}\nQuery: " (trailing space);
Qwen3-Embedding wants "Instruct: {task}\nQuery:" (no trailing space) — both
leave documents plain.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
0fff203e8c
commit
dbf44907ac
+12
-2
@@ -586,8 +586,6 @@ for var in \
|
||||
AI_MEMORY_EMBEDDING_MODEL \
|
||||
AI_MEMORY_EMBEDDING_BASE_URL \
|
||||
AI_MEMORY_EMBEDDING_DIM \
|
||||
AI_MEMORY_EMBEDDING_QUERY_PREFIX \
|
||||
AI_MEMORY_EMBEDDING_DOCUMENT_PREFIX \
|
||||
AI_MEMORY_ALLOWED_HOSTS \
|
||||
AI_MEMORY_WORKSTREAM_ID \
|
||||
AI_MEMORY_HOOK_PLATFORM \
|
||||
@@ -614,6 +612,18 @@ do
|
||||
fi
|
||||
done
|
||||
|
||||
# These two are presence-based, not non-empty-based like the loop above: an
|
||||
# operator sets one to the empty string to deliberately clear a
|
||||
# config.toml-configured prefix without editing the file (see
|
||||
# Config::load's figment overlay), and a present-but-empty value must reach
|
||||
# the container for that to work — `[ -n ]` above would drop it, making the
|
||||
# wrapper indistinguishable from the var never having been set at all.
|
||||
for var in AI_MEMORY_EMBEDDING_QUERY_PREFIX AI_MEMORY_EMBEDDING_DOCUMENT_PREFIX; do
|
||||
if [ -n "${!var+x}" ]; then
|
||||
ENV_ARGS+=(-e "${var}")
|
||||
fi
|
||||
done
|
||||
|
||||
# The wrapper itself runs the CLI inside a short-lived helper container, while
|
||||
# the README server runs in the long-lived ai-memory container and publishes
|
||||
# 127.0.0.1:49374 on the host. Inside a normal bridge-network helper,
|
||||
|
||||
+24
-2
@@ -150,8 +150,6 @@ foreach ($Name in @(
|
||||
"AI_MEMORY_EMBEDDING_MODEL",
|
||||
"AI_MEMORY_EMBEDDING_BASE_URL",
|
||||
"AI_MEMORY_EMBEDDING_DIM",
|
||||
"AI_MEMORY_EMBEDDING_QUERY_PREFIX",
|
||||
"AI_MEMORY_EMBEDDING_DOCUMENT_PREFIX",
|
||||
"AI_MEMORY_ALLOWED_HOSTS",
|
||||
"AI_MEMORY_WORKSTREAM_ID",
|
||||
"CLAUDE_CONFIG_DIR",
|
||||
@@ -176,6 +174,30 @@ foreach ($Name in @(
|
||||
}
|
||||
}
|
||||
|
||||
# Presence-based, not non-empty-based like the loop above: an operator sets
|
||||
# one of these to the empty string to deliberately clear a
|
||||
# config.toml-configured prefix without editing the file (see
|
||||
# Config::load's figment overlay), and a present-but-empty value must reach
|
||||
# the container for that to work — `IsNullOrEmpty` above would drop it,
|
||||
# making the wrapper indistinguishable from the variable never having been
|
||||
# set at all. `GetEnvironmentVariable` returns `$null` only when the
|
||||
# variable is truly unset, and `""` when it is set-but-empty, so a `-ne
|
||||
# $null` check is exactly the presence test needed here.
|
||||
#
|
||||
# An operator's own `$env:NAME = ''` additionally needs PowerShell 7.5+
|
||||
# (first built on .NET 9) to leave a set-but-empty variable rather than
|
||||
# deleting it; not exercised on a real pwsh runtime.
|
||||
# https://learn.microsoft.com/en-us/dotnet/api/system.environment.setenvironmentvariable
|
||||
# https://learn.microsoft.com/en-us/powershell/scripting/whats-new/what-s-new-in-powershell-75
|
||||
foreach ($Name in @(
|
||||
"AI_MEMORY_EMBEDDING_QUERY_PREFIX",
|
||||
"AI_MEMORY_EMBEDDING_DOCUMENT_PREFIX"
|
||||
)) {
|
||||
if ($null -ne [Environment]::GetEnvironmentVariable($Name)) {
|
||||
$DockerArgs += @("-e", $Name)
|
||||
}
|
||||
}
|
||||
|
||||
# Docker Desktop gives Windows no host networking for Linux containers, so a
|
||||
# thin-client command (status, search, bootstrap, ...) reaches the loopback-
|
||||
# published server from this helper container through Docker Desktop's host
|
||||
|
||||
Reference in New Issue
Block a user