Skip to content

feat(retrieval): assemble auto-recall context server-side via /search mode="context" - #3534

Open
t0saki wants to merge 18 commits into
volcengine:mainfrom
t0saki:feat/search-context-assembly
Open

feat(retrieval): assemble auto-recall context server-side via /search mode="context"#3534
t0saki wants to merge 18 commits into
volcengine:mainfrom
t0saki:feat/search-context-assembly

Conversation

@t0saki

@t0saki t0saki commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Description

Auto-recall assembly lived in every harness plugin: each one searched per memory type, read hits back one by one, and stitched a context block with its own budget and degradation rules. The implementations drifted apart, and the shared weaknesses were visible in production injections — roughly half of the entries degraded to a bare URI plus a score, character budgets distorted up to 6x on CJK text, and adjacent turns re-injected the same memories.

This PR moves assembly into the server as one round trip. /find stays an unchanged stateless primitive, /search gains mode="context" (mode="list" remains the default and is byte-identical to before), and /recall becomes a thin preset over the same kernel with its v1 field names folded onto the new contract.

Implements RFC #3372.

Human Involvement

  • A human participated in the implementation or review loop
  • This PR was generated entirely by AI agents without human participation in the loop

Related Issue

Implements the contract proposed in discussion #3372.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

The breaking part is scoped to the /recall response body: entries now use category/detail/text instead of type/mode/content/summary, rendered is flat XML instead of three levels of nesting, and rank is gone. Request compatibility is preserved — v1 fields are still accepted as aliases on /recall.

Changes Made

  • New assembly kernel under openviking/retrieve/context_assembler/: candidate gathering, tier resolution, token budgeting, flat rendering, dedup ledger, query expansion, digest rewrite. Replaces openviking/retrieve/type_quota_recall.py.
  • Token budgeting with a CJK-aware estimate replaces the character budget. max_tokens is the single budget parameter.
  • Tiers come from a per-category default table rather than one global strategy. events is served at overview and may deepen to full on leftover budget; every other category is served at abstract. detail pins every entry to one tier instead of only capping it, and additionally accepts a per-category map such as {"events":"overview","preferences":"abstract"}. See the tier model note below for why the table looks the way it does.
  • Filling is breadth-first then depth: every candidate lands on its default tier first, then leftover budget deepens in score order. An oversized tier falls back to the previous one instead of being truncated, bounded by max_tokens / candidates * 2 per entry.
  • Overview extraction dispatches by source type: memory files use their leading Summary section, code files use the current code-skeleton extraction API, and long documents use a heading tree plus first paragraph.
  • Directory hits start at the overview tier and read their .overview.md sidecar, since directories carry no stored abstract; their full tier stays capped at overview. Recall v1 injected that sidecar as if it were a whole file.
  • Quotas generalize beyond memory types to resources and skills; purpose presets supply ratios when quotas are absent.
  • dedup_turns keeps a per-session ledger at {session_uri}/.recall_log.json, so every harness inherits cross-turn dedup. exclude_uris remains as the stateless fallback.
  • Every tier carries its URI, so the model can always drill down through the MCP read tool.
  • Server-side query expansion and digest rewriting are optional and fail closed: both have timeout fuses (5s for expansion, 30s for rewrite), and a failed rewrite still returns the unrewritten block. Retrieval failures are reported in stats.retrieval_errors rather than silently yielding an empty block.
  • /recall folds max_charsmax_tokens, min_scorescore_threshold, and the render tri-state → detail, and signals deprecation through a Deprecation header plus stats.deprecated. The MCP recall tool routes through the same kernel.
  • Plugins send one context request and fall back to /recall, then to raw find, on older deployments, caching that outcome so only the first turn pays for the probe. Claude Code and Codex share OPENVIKING_RECALL_COMPRESS / plugin.recallCompress, defaulting to auto: Claude Code prefers local claude -p (Sonnet, low effort) and falls back to server rewrite, while Codex uses local codex exec with Spark then Luna. off disables compression for latency-sensitive paths. Codex also injects profile context at session start through the shared profile builder.
  • A context request that asks the server for a digest gets a deadline that outlasts the server's rewrite fuse, since aborting inside the fuse discards the whole response rather than just the digest. OPENVIKING_RECALL_CONTEXT_TIMEOUT_MS / plugin.recallContextTimeoutMs pins it; the non-rewrite path keeps the ordinary request timeout.
  • plugin joins the ovcli.conf schema in both Python readers, alongside the fields the Rust CLI already writes that had drifted out of them.
  • Docs: mode="context" reference in docs/{zh,en}/api/06-retrieval.md, /recall deprecation and alias table in 16-memory.md, the two retrieval timeouts in the configuration guide, and centralized low-latency plugin settings in the Agent integration overview — including the context-request deadline — with links from the Claude Code and Codex integration pages and image-doc mirrors.

Tier model: why the defaults are per category

The first revision of the ladder assumed abstract is a cheap summary. For memory files it is not. memory_updater.py writes the whole stripped body into the vector row's abstract scalar, because that same field doubles as the embedding text — embedding the full body is the right call on the write side, but it means the ladder is uri < overview < abstract = full for memory, not the strictly monotonic cost ladder the RFC assumed. Only memory is affected: a resource's abstract is the 256-char summary semantic processing produces, and directories have real .abstract.md / .overview.md sidecars.

Two properties of the first revision fell out of that mismatch:

  • abstract was exempted from the per-entry cap on the grounds that it is "cheap by construction", which let a single memory entry consume several times the per-entry budget.
  • detail only ever set a ceiling, so auto and full produced byte-identical output for a memory-only candidate set, and in the 0.38–0.50 score band the RFC itself reports, auto, overview and full were all indistinguishable.

Measured over a real memory store (~2100 files), events is the only memory type whose # Summary extraction is a real compression (median 259 tok body → 66 tok overview, 75% saved); entities and preferences have a median body of ~76 tok, where an overview costs a file read to return a truncated version of something already in hand. So the defaults exploit the storage shape rather than fight it: events starts at overview, everything else is served from abstract, and only events can deepen. The table carries the note to move events back to abstract once the writer stores a separate summary scalar — that fix belongs in the writer and is deliberately not attempted here, since abstract cannot be changed without changing recall quality.

Two consequences worth calling out: the default path now reads only the events candidates instead of every hit, and since the cap exemption is gone, an oversized abstract falls back to overview before it falls back to a bare URI.

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

OPENVIKING_CONFIG_FILE=/tmp/ov-test.conf uv run pytest tests/retrieve tests/server/test_api_search_context.py tests/server/test_recall_endpoint.py tests/server/test_recall_peer_scope.py tests/server/test_mcp_endpoint.py tests/test_ovcli_config_schema.py — 124 passed, with /tmp/ov-test.conf containing {}. New coverage spans candidate gathering, tier dispatch, budget filling and fall-back, the dedup ledger, expansion and rewrite failure modes, the 400 validation matrix, and /recall alias folding. Two of the new server tests go through real AGFS: one reads file bodies and directory sidecars, the other round-trips the dedup ledger against a real session. tests/test_ovcli_config_schema.py asserts the shipped ovcli.conf.example loads in both Python readers and that an unknown field is still rejected, so the schema cannot drift away from the Rust CLI again unnoticed.

The tier-model tests pin the per-category defaults, the pin semantics of an explicit detail, the per-category map, the read gating (only events candidates are read on the default path), the oversized-abstract fallback, and the degradation of an unknown detail value. The bug fixes below each have a regression test, including one that asserts rewrite_usage is dropped when the shared tracker moved by more than one call — the previous test mocked a planner shape that does not exist in production, which is how the dead path stayed green.

OPENVIKING_STATE_DIR="$(mktemp -d)" node --test $(rg --files examples | rg '\.test\.mjs$' | sort) — 193 passed, covering the context-face request body, the downgrade chain, the legacy-server cache, the unified compression knob and model fallback matrix, the context-request deadline, session-start profile injection, and digest URI repair.

cd docs && npm run check:api — passes, which is what the previous revision broke.

Also verified against a locally running server: the context response contract, all four 400 validation cases, /recall alias folding (max_chars: 6500max_tokens: 1625, defaults of 1600/0.35, render: "compact" → abstract ceiling), the Deprecation and Link headers, and a real Claude Code hook run that reached the context face and degraded gracefully when retrieval was unavailable.

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Additional Notes

Bugs found while reworking the tier model, each fixed with a regression test:

  1. Rewrite timeouts were reported as failed on Python 3.10, where asyncio.TimeoutError is a separate class from the builtin. requires-python is >=3.10 and release wheels are built there; CI only runs 3.11, so the assertion never fired.
  2. stats.rewrite_usage read token_tracker off VLMConfig, which has no such attribute — the AttributeError was swallowed and usage was structurally always null, which quietly disables the cost accounting RFC §3.2 promises. It now reads the model instance's tracker and reports only when the call count moved by exactly one, because that tracker is shared across callers.
  3. A single malformed ledger record ({"turn": "x"}, a null turn, a non-dict value) made every deduped recall in that session fail after the full retrieve-read-budget-render pass, and the file was never rewritten, so it could not heal. Records are coerced on read and dropped on the next write, together with records left ahead of the clock by an archive rotation, which previously could never expire and additionally won the eviction sort.
  4. Entries served as a bare URI no longer enter the dedup cooldown — they lost to budget pressure, not to the reader having already seen them. This is the uri-grace behaviour the RFC lists as a benefit of server-side accounting.
  5. The render envelope only neutralised a literal </memory>, so a body could emit <memory uri="..." score="0.99">…</Memory> and forge a sibling entry with its own provenance. Both ends of the tag are now neutralised, case- and whitespace-tolerantly.
  6. Flat-mode gathering discarded the owning bucket and re-derived the category from the URI, so viking://resources/backup/memories/events/log.md was read as an event and escaped the resource tier ceiling.
  7. Cooled and excluded URIs are filtered after retrieval, so a bucket whose whole top page was cooled came back empty rather than falling through to the next-best hits. Retrieval now asks for compensating rows.
  8. /recall quotas overlay the v1 bucket defaults again. v1's normalize_quotas merged over the defaults; the rewrite started from an empty map, so {"events": 5} silently dropped the other three buckets and {} returned nothing at all.
  9. The MCP recall tool sent its own signature defaults as if the caller had supplied them, so its default profile resolved to 0.1/1625 while POST /recall resolved to 0.35/1600 — RFC §3.1 requires the same profile. An unknown detail value ("summary", the v1 spelling an LLM readily produces) also raised KeyError through the whole call instead of degrading.

Found in the second review round:

  1. Build Docs was failing. The API overview's deprecated-recall row described the successor as `/search`, and docs/scripts/check-api-reference.mjs scans everything after the method cell for backticked paths, so it read that description as a route named POST /search — a route the server does not mount and no reference page documents. Both locales now name the endpoint without backticks.
  2. Adding the documented plugin section to a working ~/.openviking/ovcli.conf broke every Python consumer of that file: load_ovcli_config() raised Unknown field 'ovcli.plugin' and OVCLIConfig raised extra_forbidden, so ov doctor and SDK client construction failed before any request went out. The deeper cause is that ovcli.conf's schema belongs to the Rust CLI, which writes root_api_key, output, echo_command, show_progress and verbose and ignores unknown keys, while the two Python readers had each drifted into a different stricter subset — examples/ovcli.conf.example already failed to load in both on main, before this PR. Both readers now accept the full field set, and a test pins the example against them.
  3. mode="context" answered an invalid request with 200 and an empty block. Retrieval validates query and image_url and raises InvalidArgumentError before searching; the gather fuse caught it alongside genuine per-scope failures, so {"mode":"context"} recorded a stats.retrieval_errors entry and returned success where mode="list" returns 400. That contradicts the documented "L0 parameter behaviour matches list mode" and left callers unable to tell a malformed request from a genuine miss. Request rejections now propagate; runtime failures still degrade.
  4. With recallCompress=server — or auto when no local compressor is available — the plugin sent a context request under the ordinary 15s HTTP timeout while the server's rewrite fuse is 30s. A rewrite finishing at 20s was inside its own budget but aborted client-side, and since the abort fails the whole request the plugin fell back to /recall, losing the uncompressed rendered block the server returns even when a rewrite fails. The deadline now outlasts the fuse, but only when the body actually requests a rewrite, and OPENVIKING_RECALL_CONTEXT_TIMEOUT_MS / plugin.recallContextTimeoutMs pins it for deployments that tune retrieval.recall_rewrite_timeout_s.

… mode="context"

Auto-recall assembly lived in every harness plugin: each one searched per
memory type, read hits back one by one, and stitched a context block with its
own budget and degradation rules. The implementations drifted, and the shared
weaknesses showed up in production injections — roughly half of the entries
degraded to a bare URI plus a score, character budgets distorted up to 6x on
CJK text, and adjacent turns re-injected the same memories.

This moves assembly into the server as one round trip. /find stays an unchanged
stateless primitive. /search gains mode="context" (mode="list" is the default
and byte-identical to before), and /recall becomes a thin preset over the same
kernel with its v1 field names folded onto the new contract.

New assembly kernel under openviking/retrieve/context_assembler/:

- Token budgeting with a CJK-aware estimate replaces the character budget.
- detail="auto" fills breadth-first then deepens: every candidate gets a
  readable floor, then overview, then full for high-scoring entries. An
  oversized tier falls back to the previous one instead of being truncated,
  bounded by max_tokens / candidates * 2 per entry.
- Overview extraction dispatches by source: memory files use their leading
  Summary section, code files reuse code_outline signatures, long documents use
  a heading tree plus first paragraph.
- Directory hits start at overview and read their .overview.md sidecar, since
  directories carry no stored abstract; their full tier stays capped at
  overview. v1 injected the sidecar as if it were a whole file.
- Quotas generalize beyond memory types to resources and skills, with purpose
  presets supplying ratios when quotas are absent.
- dedup_turns keeps a per-session ledger at {session_uri}/.recall_log.json so
  every harness inherits cross-turn dedup; exclude_uris remains as the
  stateless fallback.
- Rendering flattens to one <memory uri=... type=... score=... detail=...>
  element per entry. Every tier carries its URI, so the model can always drill
  down through the MCP read tool.
- Query expansion and digest rewriting are opt-in and fail closed: both have
  timeout fuses, and a failed rewrite still returns the unrewritten block.
  Retrieval failures are counted into stats rather than silently yielding an
  empty block.

Plugins now send one context request, falling back to /recall and then to raw
find on older deployments, and cache that outcome so only the first turn pays
for the probe. The tri-state recallRewrite knob chooses between local host-CLI
compression and the server digest, and client-side settings move to a plugin
section in ovcli.conf.
The tier ladder assumed `abstract` is a cheap summary. For memory files it
is not: the memory writer stores the whole stripped body in that scalar
because it doubles as the embedding text, so `abstract` costs the same as
`full` and the ladder runs `uri < overview < abstract = full`. Two of the
model's properties fell out of that: exempting `abstract` from the per-entry
cap let a single entry eat several times the budget, and `detail` — which
only ever set a ceiling — collapsed to two distinguishable behaviours across
its four values, since `auto` already allowed `full` for memory.

Tiers now come from a per-category constant table that treats the storage
shape as a given: `events` starts at overview (the one memory type whose
`# Summary` extraction is a real compression) and may deepen to full on
leftover budget; every other category is served at `abstract`, which for
memory already is the complete file at zero read cost and for resources and
skills is the generated 256-char summary. The table carries the note to move
`events` back to `abstract` once the writer stores a separate summary scalar.

Falling out of that: prefetch now reads only the candidates whose planned
tier needs a body rather than every candidate, `detail` becomes a real pin
(start and ceiling) and additionally accepts a per-category map, and
`full_score_threshold` is gone — leftover budget is spent in score order
instead of behind an absolute threshold the observed score band cannot
support. `auto` is still accepted on the wire as a synonym for "unset".

Assembly fixes found alongside:

- Removing the abstract cap exemption would turn an oversized abstract into
  a bare URI, so it now falls back to overview first — for memory that is a
  cheaper substitute, not a step up.
- Rewrite timeouts were reported as failures on Python 3.10, where
  `asyncio.TimeoutError` is a separate class from the builtin.
- `stats.rewrite_usage` read `token_tracker` off `VLMConfig`, which has no
  such attribute; usage was structurally always null. It now reads the model
  instance's tracker and reports only when the call count moved by exactly
  one, since that tracker is shared.
- A single malformed ledger record made every deduped recall in that session
  fail, and the file was never rewritten, so it could not heal. Records are
  now coerced on read and dropped on the next write, along with records left
  ahead of the clock by an archive rotation.
- Entries served as a bare URI no longer enter the dedup cooldown: they lost
  to budget pressure, not to the reader having already seen them.
- The render envelope only neutralised a literal `</memory>`, so a body could
  forge a sibling entry with its own uri, type and score.
- Flat-mode gathering re-derived the category from the URI, reading
  `viking://resources/backup/memories/events/log.md` as an event.
- Cooled and excluded URIs are compensated with extra rows, so a fully cooled
  bucket falls through to the next-best hits instead of coming back empty.
- `/recall` quotas overlay the v1 bucket defaults again; `{"events": 5}` had
  started dropping the other three buckets.
- The MCP `recall` signature sent its own defaults as if the caller had, which
  resolved a different profile than `POST /recall`; an unknown `detail` value
  raised `KeyError` through the whole call instead of degrading.
huangruiteng added a commit that referenced this pull request Jul 30, 2026
- events.yaml: remove hardcoded ratio_threshold=0 so summaries are
  preferred over full transcripts when short enough
- memory_updater: skip empty-speaker lines for turns with no text
  content in _format_contiguous_group

Part of #3598 (write-path residuals, does not overlap with #3534)
@t0saki
t0saki marked this pull request as ready for review July 31, 2026 09:24
Copilot AI review requested due to automatic review settings July 31, 2026 09:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

t0saki added 2 commits July 31, 2026 17:37
…ssembly

# Conflicts:
#	openviking/server/mcp_endpoint.py
#	openviking_cli/utils/config/retrieval_config.py
const minChars = Math.max(0, Number(cfg.recallCompressMinInputChars ?? 1500));
if (input.length < minChars) return input;

const key = recallDigestCacheKey(entries, input);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking)

When a plugin locally compresses two recall results that contain the same URI set but answer different questions, the second turn receives the first turn's digest instead of a digest for the current query.

recallDigestCacheKey(entries, input) hashes only the sorted URI list when entries is non-empty. The digest produced below is query-specific (buildRecallCompressionPrompt includes query) and content-specific, but neither the query nor the served content participates in the cache identity.

Concrete example:

  1. A turn asks Which command fixed deployment? and retrieves viking://user/alice/memories/events/release.md; the compressor stores a command-focused digest.
  2. A later turn asks What deadline did we agree? and retrieves the same URI, whose body contains both facts.
  3. The key is identical, so line 149 returns the command digest without running the compressor for the deadline query. The same stale reuse occurs if the content at that URI changes.

I reproduced this against the PR head: two different queries with the same rendered entry invoked the compressor once, and both calls returned the first digest. This injects irrelevant or stale memory into the agent on a normal multi-turn workflow.

Please confirm the intended cache identity given that the digest is query- and content-dependent, and explain any constraints or tradeoffs if cross-query reuse is intentional. This must be resolved before merge; the implementation choice is yours.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cf34f2a. The cache key now covers every effective compressor input: the query, the rendered content actually visible after maxInputChars truncation, the sorted served URI set, maxInputChars, maxBullets, and a key version. Identical requests still reuse the digest, while a query, content, URI-set, or compression-setting change causes recompression.

Regression coverage was added in recall-compress-core.test.mjs, including the original same-URI/different-query case and changed rendered content. The test file is now also included in the PR workflow.

quotas = normalize_quotas(params.quotas, params.purpose)
penalties = normalize_penalties(params.other_peer_penalty)

needs_session = bool(params.session_id) and (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking)

An operator who disables intent analysis still incurs session loading and an LLM intent-expansion call when using the new context mode.

needs_session depends only on session_id, query_expansion, and dedup_turns, and expand_queries is then called directly. This path never checks service.search.is_intent_enabled(). In contrast, the existing list-mode path and SearchService.search() explicitly treat retrieval.enable_intent=false as a contract to skip session context and search the raw query.

Concrete example:

  1. Configure retrieval.enable_intent: false.
  2. Send {"query":"continue","mode":"context","session_id":"s1"} with the default query_expansion:"auto".
  3. assemble_context loads s1; expand_queries calls session.get_context_for_search() and constructs IntentAnalyzer when the session has messages.
  4. The equivalent list-mode request skips both operations and uses continue directly.

This makes the same public configuration mean different things across the two modes and causes model cost and latency that the operator explicitly disabled.

Please confirm whether enable_intent=false is intended to govern context-mode expansion as it governs list mode, and explain the operational tradeoff if not. The contract mismatch must be resolved before merge; the implementation choice is yours.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cf34f2a. Context assembly now consults service.search.is_intent_enabled() before deciding whether automatic query expansion needs a session. With intent disabled, the raw query is used and the session is not loaded for expansion; session loading is retained only when independently required by an explicit dedup ledger.

test_disabled_intent_skips_session_loading_and_query_expansion verifies that the session accessor is not touched and only the raw query is searched.

service,
retrieval_errors,
query=query,
ctx=ctx,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking)

A request using the documented default peer_scope="all" misses other-peer memories whenever bucket quotas are not active.

gather_flat() performs one empty-target find with the actor-scoped ctx. For a request carrying X-OpenViking-Actor-Peer: current, default target resolution includes shared user memory and peers/current, but it hides other peer roots. The explicit open-context scan under {user_root}/peers exists only in gather_bucket().

Concrete example:

  1. Store viking://user/alice/peers/other/memories/events/release.md.
  2. Send {"query":"release","mode":"context"} with actor peer current; peer_scope defaults to all and quotas is absent.
  3. normalize_quotas returns None, so this flat path runs with ctx.actor_peer_id == "current" and cannot return the other memory.
  4. Adding an explicit {"quotas":{"events":10}} selects the bucket path, which does scan other peers and can return the same memory.

A sampling option therefore changes the ownership scope, and the response still reports peer_scope:"all" even when no other-peer scope was searched.

Please confirm the intended meaning of peer_scope="all" for quota-free context assembly and explain any constraints behind making it bucket-only. This must be resolved before merge; the implementation choice is yours.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cf34f2a. Flat gathering now mirrors bucketed gathering for peer_scope="all": it performs an open-context scan under the user's peers root, keeps only other-peer memory hits from that scan, and merges/deduplicates them with the actor-scoped flat results. peer_scope="actor" remains unchanged.

test_flat_mode_searches_other_peers_when_scope_is_all covers the quota-free path and verifies that the peer scan uses a context without actor_peer_id.

async def gather_bucket(bucket: str, quota: int) -> List[Candidate]:
targets = category_targets(bucket, ctx)
searches = [
_safe_find(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking)

A supported image search returns an empty context whenever purpose or explicit quotas selects bucketed retrieval.

The _safe_find calls in gather_bucket() forward the text query but omit the image_url argument. The flat call forwards it correctly.

Concrete example:

  1. Send {"query":"","image_url":"data:image/png;base64,...","mode":"context","purpose":"coding"}.
  2. purpose:"coding" resolves to a quota map, so gather_bucket() runs.
  3. Each bucket calls SearchService.find() with an empty query and no image. The service raises Search query or image_url must not be empty.
  4. _safe_find converts those failures into retrieval errors, so the API returns HTTP 200 with an empty assembled context. If both text and image are supplied, it silently performs text-only retrieval instead.

The API documentation states that image_url behaves as in list mode, so changing the sampling strategy must not discard the image input. This needs to be resolved before merge.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cf34f2a. Flat and bucketed retrieval now share one local find helper that forwards the complete common argument set, including image_url, so the two strategies cannot drift on this parameter again.

test_bucket_mode_forwards_image_url_to_every_search covers image-only bucketed retrieval and verifies that every bucket and peer search receives the image input without producing retrieval errors.

cleaned = re.sub(r"^[-*]\s+", "- ", cleaned)[:MAX_BULLET_CHARS].rstrip()
# Checked after truncation: the prompt puts the citation last, so a
# bullet whose URI was cut off no longer carries one.
if "viking://" not in cleaned:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking)

When the optional server-side rewriter emits a hallucinated source URI, the API returns that URI as valid provenance even though it was not among the served entries.

normalize_digest accepts any bullet containing the substring viking://; it does not receive the rendered entry set and cannot verify that the citation was copied from it. An off-contract model response is an expected failure mode for this LLM boundary, and the client-side compressor in this PR already enforces the corresponding invariant with repairDigestUris.

Concrete example:

  1. The assembled result contains only viking://user/alice/memories/events/release.md.
  2. The planner returns - deploy to prod source: viking://user/alice/memories/fake.md.
  3. The line passes this condition, rewrite_context reports status ok, and the fabricated URI is exposed in digest.

Consumers then receive a dead or unrelated provenance link while the response claims a successful rewrite, breaking the PR's stated guarantee that every digest citation is copied from the retrieved fragments.

Please confirm whether server-side digest citations are required to remain within the current assembled entry set, and explain any constraints if arbitrary viking:// citations are intentionally accepted. This provenance issue must be resolved before merge; the implementation choice is yours.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cf34f2a. The pipeline now passes the exact served entry URI set into the rewrite boundary. Digest normalization rejects any bullet with no citation or with any viking:// citation outside that allowlist, so an off-contract model response cannot expose invented provenance.

Coverage was added both for normalization with mixed valid/invented bullets and for an end-to-end rewrite response containing only a hallucinated URI. The pipeline test also asserts that the served URI list is actually passed to the rewriter.

def deprecation_stats(aliases: Sequence[str]) -> Dict[str, Any]:
return {
"endpoint": "/api/v1/search/recall",
"successor": "/api/v1/search/search?mode=context",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (non-blocking)

A client following this deprecation metadata can migrate to the successor URL and still receive the legacy list response rather than assembled context.

mode is a field of the JSON SearchRequest; the /search router selects context mode from request.mode. A request to /api/v1/search/search?mode=context whose body contains only the old recall fields therefore leaves request.mode at its default list value because the query parameter does not populate the body model.

For example, following this string with body {"query":"release"} executes the list branch, while the advertised successor behavior requires {"query":"release","mode":"context"} in the body. The metadata is therefore misleading for automated or human migration, even though the separate Link header names the correct endpoint.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cf34f2a. The deprecation metadata now advertises the actual endpoint as successor: "/api/v1/search/search" and separately provides successor_body: {"mode": "context"}. This makes it explicit that mode belongs in the JSON request body rather than the query string.

The recall endpoint regression test now asserts the complete migration metadata shape.

@qin-ctx qin-ctx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server-side context assembly is the right ownership move: before this PR, each plugin had to issue multiple retrieval/read calls and assemble its own bounded context; after this PR, /search with mode="context" centralizes retrieval, tiering, budgeting, deduplication, and optional rewrite while keeping /find as the primitive path.

I am requesting changes because five supported paths currently violate the documented retrieval/configuration contracts: local digest reuse is not query-safe, context expansion ignores enable_intent=false, flat retrieval does not honor peer_scope="all", bucketed retrieval drops image_url, and server rewrite can return citations outside the served entry set. I also left one non-blocking inline comment on the /recall successor metadata.

t0saki and others added 3 commits August 3, 2026 12:22
Co-authored-by: TRAE CLI <noreply@bytedance.com>
Resolve retrieval conflicts while preserving the context assembler migration and streamlined test coverage.

Co-authored-by: TRAE CLI <noreply@bytedance.com>

@qin-ctx qin-ctx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

服务端集中组装 Context 的方向合理,之前 review 提出的缓存键、enable_intent、peer scope、图片检索和引用约束等问题也已确认修复。当前仍有两个需要在合并前处理的问题:新增的 ovcli.conf.plugin 与现有 Python SDK/CLI 的严格配置 schema 不兼容,以及中英文 API overview 触发了文档检查失败。另有两条非阻塞意见,分别涉及 context 模式的请求错误分类和 Claude Code 服务端重写的超时边界。

"extra_headers": null
"extra_headers": null,

"plugin": {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking)

用户按照这里的新示例,把 plugin 放进 ~/.openviking/ovcli.conf 后,同一份配置将无法再被 Python SDK 和 CLI 读取。Python SDK 的 load_ovcli_config() 会报 Unknown field 'ovcli.plugin';CLI 的 OVCLIConfig 也会因为 extra="forbid"extra_forbidden

例如,原本可用的 {"url":"http://localhost:1933"} 加上 {"plugin":{"recallCompress":"off"}} 后,插件可以直接读取该字段,但 SDK 会在创建客户端、发出任何网络请求之前失败,ov doctor 也会在加载配置时失败。这里新增了共享配置格式,却没有同步两个严格 schema,因此插件配置与现有 SDK/CLI 无法共存。

这个兼容问题需要在合并前解决;如果你认为 plugin 不应进入这两个 schema,请说明这份共享配置在各消费者之间应如何隔离。具体处理方式由你决定。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已在 de8e1a5 修复。plugin 现在同时进入 OVCLIConfig 和 SDK 的 allowed_keys,两边都作为不透明字段透传、不解释内容——plugin-config.mjs 本来就依赖未知键透传,让各 harness 能自带旋钮而不必回头改 Python schema。

顺带说明一个比本 PR 更早的问题:ovcli.conf 的 schema 实际归 Rust CLI 所有(crates/ov_cli/src/config.rs),它会写 root_api_key / output / echo_command / show_progress / verbose,并且 serde 默认忽略未知键;而两个 Python reader 各自漂移成了不同的更严格子集。所以 examples/ovcli.conf.example 在 main 上就已经两边都加载失败了——CLI 报 root_api_keyoutput,SDK 报 echo_command。这次一并把完整字段集补齐,并加了 tests/test_ovcli_config_schema.py,钉住 example 文件在两个 reader 下都能加载、且未知字段仍然被拒。

Comment thread docs/en/api/01-overview.md Outdated
| POST | `/api/v1/sessions/{session_id}/messages/batch` | Add messages in a batch |
| POST | `/api/v1/sessions/{session_id}/used` | Record context or skills actually used |
| POST | `/api/v1/search/recall` | Recall memory as injection-ready context |
| POST | `/api/v1/search/recall` | Deprecated: thin preset over `/search` `mode="context"` |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (blocking)

这里把简写 /search 放进了反引号。文档检查器会把表格这一行解析成另一个 HTTP 路由 POST /search,而不是说明文字。

当前运行 node docs/scripts/check-api-reference.mjs 时,中英文目录都会报告 overview contains unknown HTTP route POST /searchoverview route has no detailed HTTP reference: POST /search,因此 Build Docs 已经失败。请改成不会被识别为新路由的表述,并同步修改中文 overview。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已在 de8e1a5 修复,中英 overview 都改成不会被识别成路由的表述。

根因确认:check-api-reference.mjs 的 overview 检查用 ^\|\s*(METHOD)\s*\|([^\n]+)$ 抓方法列之后的整行,再对其中所有反引号路径逐个 findRoute,所以描述列里的 `/search` 会被当成一条新路由。cd docs && npm run check:api 现在通过。

"""
try:
return await service.search.find(**kwargs)
except Exception as exc:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (non-blocking)

context 模式会把调用方的非法请求当成可降级的检索故障,最后返回 HTTP 200 和空 Context。比如请求体只有 {"mode":"context"},既没有 query 也没有 image;list 模式会返回 400,但 context 模式会在 stats.retrieval_errors 中记录错误后返回成功。非法的 image_url 也会走同一条路径。

检索服务会先校验 query/image,并抛出 InvalidArgumentError。这里捕获了所有 Exception,所以参数错误和某个检索范围的 embedding/provider 故障被一并转成了 None,随后组装流程继续生成空结果。这与文档中“L0 参数行为与 list 模式一致”的说明不符,也让调用方无法通过状态码区分请求写错和确实没有命中。

请确认 context 模式是否有意改变这类请求错误的 HTTP 语义;是否处理、怎样区分参数错误与可降级的运行故障,由你决定。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不是有意改变语义,已在 de8e1a5 修复。

_safe_find 现在放行 InvalidArgumentError:参数错误在每个检索 scope 上都会同样失败,降级只是把调用方的错误藏进一个 200 空 Context,因此让它冒泡,和 mode="list" 一样返回 400 INVALID_ARGUMENT;embedder / provider 这类可降级的运行期故障仍然计入 stats.retrieval_errors 并返回 200。两条测试分别钉住这两侧:一条断言 {}{"mode":"context"} 同样 400,一条断言 findRuntimeError 时仍然 200 且 retrieval_errors 非空。

// OPENVIKING_RECALL_REWRITE / recallRewrite are legacy aliases. Keep the
// internal field name because the shared core maps this mode to the
// server's `rewrite` request field.
recallRewrite: normalizeRewriteMode(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] (non-blocking)

Claude Code 使用 server 压缩,或者 auto 在本地压缩器不可用时回退服务端,Context 请求仍然沿用普通 HTTP 请求的 15 秒默认超时;服务端 rewrite 的超时保险丝则是 30 秒,UserPromptSubmit 外层 hook 是 60 秒。

例如一次正常 rewrite 在第 20 秒完成:服务端仍在允许的时间内,但客户端会在第 15 秒先中断连接。fetchAssembledContext() 随后把整个 Context 响应视为失败并回落到 /recall,因此连服务端承诺在 rewrite 失败时仍返回的未压缩 rendered 也拿不到,而不只是丢失 digest。

请确认服务端压缩模式期望支持的延迟边界,以及客户端是否应该包住服务端的 rewrite fuse;是否调整、如何调整由你决定。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已在 de8e1a5 修复。context 请求只在 body 真的带 rewrite 时才下发更长的 deadline(默认 35s:高于服务端 30s 的 recall_rewrite_timeout_s,低于 UserPromptSubmit 的 60s),非 rewrite 路径完全保持原有超时不变。OPENVIKING_RECALL_CONTEXT_TIMEOUT_MS / plugin.recallContextTimeoutMs 可以给调过 retrieval.recall_rewrite_timeout_s 的部署固定这个值,文档中英文各补了一段。

补充一点排查结果:Codex 实际不受影响——auto-recall.mjsassembleCfg 写死 recallRewrite: "off",从不请求服务端 digest。不过 CC、Codex、shared agent runtime 三处 fetchJSON 都已支持 per-request 超时,避免以后有人把 Codex 切到 server 模式又踩同一个坑。

t0saki and others added 5 commits August 4, 2026 12:03
- Drop the backticked `/search` from the deprecated-recall row in both API
  overviews. The reference checker scans the whole row after the method cell
  for backticked paths, so it read the description as a route named
  `POST /search` and Build Docs failed on an unknown, undocumented route.
- Accept ovcli.conf's full field set in both Python readers. The file's schema
  belongs to the Rust CLI, which writes `root_api_key`, `output`,
  `echo_command`, `show_progress` and `verbose` and ignores unknown keys; the
  two Python readers had drifted into stricter subsets, so the shipped example
  already failed to load in both. Adding the new `plugin` section to a working
  ovcli.conf would have broken `ov doctor` and every SDK client the same way.
- Return 400 from `mode="context"` for a request `mode="list"` also rejects.
  Retrieval validates query and image_url before searching, and the gather
  fuse swallowed that rejection along with genuine scope failures, so a body
  of `{"mode":"context"}` came back 200 with an empty block instead of the
  documented parameter error. Runtime failures still degrade into
  `stats.retrieval_errors`.
- Let a context request that asks for a server-side digest outlast the
  server's rewrite fuse. The plugin's ordinary 15s request timeout is shorter
  than the 30s fuse, so a rewrite that finished inside its own budget was
  aborted client-side, discarding the whole response — including the
  uncompressed block the server returns when a rewrite fails — and falling
  back to `/recall`. The deadline is only extended when the body actually
  requests a rewrite, and `OPENVIKING_RECALL_CONTEXT_TIMEOUT_MS` /
  `plugin.recallContextTimeoutMs` pins it.
Restore cross-domain coding recall, reuse authoritative actor resource
scopes, and make bucket quotas the sole width control in purpose mode.
Keep plugin defaults server-owned while preserving explicit legacy limit
settings through quota conversion.

Co-authored-by: TRAE CLI <noreply@bytedance.com>
Restore the deprecated recall threshold default, distinguish successful empty rewrites from compressor failures, and document legacy quota floors across coding-agent plugins.

Co-authored-by: TRAE CLI <noreply@bytedance.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants