Skip to content

fix(retrieval): honor context tier ceilings and stop cooling unserved recalls - #3746

Open
t0saki wants to merge 2 commits into
volcengine:mainfrom
t0saki:fix/context-tier-and-dedup-contract
Open

fix(retrieval): honor context tier ceilings and stop cooling unserved recalls#3746
t0saki wants to merge 2 commits into
volcengine:mainfrom
t0saki:fix/context-tier-and-dedup-contract

Conversation

@t0saki

@t0saki t0saki commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Description

Follow-up to #3534, from its post-merge review rounds. Seven defects in the context assembly kernel and the plugins that call it, plus doc and example corrections. Three are contract violations where the code and the reference documentation disagree; the rest cost recall quality or latency without failing anything visibly.

The common thread in the tier fixes is the storage asymmetry #3534 already documented: memory_updater.py writes the whole stripped body into a memory row's abstract scalar because that field doubles as embedding text, while a resource's abstract is the 256-char summary semantic processing generates. The first revision generalized one heuristic across both shapes, which is safe in one direction and a disclosure boundary crossing in the other.

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

Follows up on #3534, which implements RFC #3372.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation update
  • Test update

Changes Made

1. resources / skills bodies were read and returned past their tier ceiling

start_tier() returned overview for any candidate whose default or pinned tier was abstract but whose stored abstract was empty, and _tiers_down_from("abstract") inserted overview into the downgrade sequence for every category. Both were written for memory files, where the stored abstract is the whole body and overview therefore discloses strictly less — a downgrade despite ranking higher. resources and skills have the opposite shape, so the same substitution reads a body the caller never asked for.

params.py documents that boundary explicitly: "their bodies are large and may carry credentials, so deepening is opt-in", and DEPTH_CEILING_BY_CATEGORY lists only events. docs/{zh,en}/api/06-retrieval.md documents an explicit detail as "both the requested start and ceiling" with entries that "step down". A resource ingested with processing_mode=vectors_only has no abstract, so both statements were false for it.

Reproduced on a resource with no abstract and a body of # Credentials\n\nTOKEN=secret:

Request Before After
detail="abstract" detail="overview", body returned verbatim detail="uri", no read
default path (no detail) detail="overview", body returned verbatim detail="uri", no read
detail="abstract", abstract over the per-entry cap detail="overview", body read detail="uri", no read

Note the second row: this never needed an explicit pin. The default path was reading resource bodies too, which is the wider half of the bug. doc_overview() on a short file returns the heading tree plus the first paragraph, so for these files the "overview" was the document.

Fix: FULL_BODY_ABSTRACT_CATEGORIES gates the substitution on categories whose abstract is the whole body. abstract_substitute() returns overview for those and uri for everything else; _tiers_down_from() and the read-gating predicate (renamed abstract_over_cap → oversized_abstract_needs_body) both consult it, so a resource's body is never read on its behalf. Memory categories keep the substitute unchanged.

2. A no_relevant digest cooled memories that were never served

pipeline.py blanks rendered when the rewrite returns NO_RELEVANT_MEMORY, and the plugins return "" on that status, so nothing is injected. The ledger write ran anyway, recording every planned entry at its abstract/overview tier and cooling those URIs for dedup_turns turns — five by default. The next turn asking a directly related question could not retrieve them.

This also contradicted the ledger's own rule: cooled_uris() already exempts bare-URI entries because "the entry lost to budget pressure, not to the reader having already seen it". A no_relevant turn delivers even less than a bare URI, yet was treated more strictly. /recall has no server-side rewrite or ledger, so the regression was confined to the new context face.

3. Flat retrieval returned a category outside the response contract

category_for() fell through to a literal "memories" for any hit under /memories/<type>/ whose type is not one of the four in MEMORY_CATEGORIES. core/directories.py defines five more built-in types — cases, patterns, tools, trajectories, and skill-usage memories — all reachable from quota-free retrieval, which searches the user root.

The reference table defines entries[].category as six values, and that undeclared seventh value was in no lookup table:

  • penalties.get("memories", 0.0) returned 0.0, so another peer's trajectory competed at full score while every declared category was penalized
  • normalize_detail() filters keys to CATEGORY_KEYS, so detail={"memories": ...} was silently dropped and callers had no documented way to control the tier
  • mode="list" passes the declared category through untouched, so the new face was strictly worse here than the endpoint it supersedes

Fix: OTHER_MEMORY_CATEGORY is now a declared category with a tier default and an other-peer penalty, accepted in detail and in per-category penalties. It stays out of quotas: category_targets() would resolve it to the memory root, which every other bucket already covers, so validation now splits into quota keys (CATEGORY_KEYS) and reportable keys (REPORTED_CATEGORY_KEYS).

This also fixes a misclassification on the same line. viking://user/{u}/memories/skills/s.md is a memory about skill usage, but the old fall-through matched /skills/ in the URI and returned the skills category, applying the wrong tier ceiling and penalty to it — the same shape as the resource-path bug fixed in #3534.

4. ZCode, OpenCode and pi never enabled dedup or query expansion

buildRecallBlock() sends session_id, and with it dedup_turns and server-side query expansion, only when the caller supplies one. All three harnesses own an OV session id and none forwarded it: ZCode computes sessionId at zcode-hook.mjs:73 and called recallForPrompt() without options; pi has sync.sessionId but constructed RecallManager without it; OpenCode has sessionManager.getMappedSessionId() but never passed the manager into its recall component. Each request reached mode="context" successfully with dedup reported as off, so nothing failed — adjacent turns simply re-injected the same URIs, and short queries like "continue that refactor" retrieved on their literal text.

pi's SyncManager is constructed after its RecallManager, so the id arrives as a getter rather than a value.

5. The context-request deadline covered only the last server stage

SERVER_REWRITE_REQUEST_TIMEOUT_MS was 35s against a 30s rewrite fuse, but the server pipeline is serial and the rewrite is only its final stage: query expansion (recall_intent_timeout_s, 5s) runs first, then retrieval, body reads and budgeting. A request spending 4.8s expanding, 1s retrieving and 29.5s rewriting is inside every server budget and was still aborted client-side at 35s — and since the abort fails the whole request, the plugin fell back to /recall and lost the assembled rendered block the server returns even when a rewrite fails. Now 45s, which covers both fuses plus the work between them and leaves a quarter of the 60s prompt-hook budget.

6. plugin config named harnesses that do not read it

plugin-config.mjs and ovcli.conf.example listed opencode, cursor, trae and pi, but only Claude Code and Codex import loadPluginSettings(); the rest receive the module through sync.mjs without consuming it, and ZCode was in neither the key list nor the example. Setting plugin.cursor.recallLimit=2 was silently ignored. HARNESS_KEYS, the example and both locales of the integration overview now name only the two real consumers, with a note to add a key when its loader starts calling the function rather than ahead of it.

7. The deadline and the expansion opt-out did not reach the harnesses this PR enables

Fix 4 forwards a session id, and a session engages server-side query expansion — expansion.py returns [query], "off" immediately when session is None, so before this PR these two harnesses never paid for it. Two separate gaps meant the newly enabled stage arrived without either the budget or the switch it needs.

contextRequestTimeoutMs() took (cfg, serverRewrite) and returned undefined whenever no digest was requested. OpenCode and pi set no recallRewrite, so they never requested one and never got any extension — while the server could now spend up to recall_intent_timeout_s (5s) on expansion before retrieval even starts. It now derives the deadline from the request body instead, because the body is what states which stages will run: session takes the expansion fuse, rewrite takes the digest fuse, neither keeps the caller's own budget.

Even that would not have reached the wire. Both adapters discarded the value: memory-recall.mjs spread the helper's options and then pinned timeoutMs: 5000 after them, and recall.ts passed a literal 10000 positionally, ignoring options. OpenCode's 5s was shorter than the expansion fuse alone, so a legal request would have been aborted client-side, and since an abort fails the whole request it would have fallen back to /recall — losing the dedup and expansion this PR set out to enable, and paying double latency to do it. Each adapter's budget is now a default rather than a ceiling.

Finally, neither harness read OPENVIKING_RECALL_QUERY_EXPANSION, and buildContextSearchBody() only emits query_expansion when the harness also sets recallQueryExpansionConfigured. Both now read the variable and their own recallQueryExpansion config key, so the cost introduced here has an off switch where it is paid. The default stays auto, matching Claude Code and Codex and the documented behaviour of expansion as a default-on optional stage.

Effective deadline per harness, with each adapter's own budget as the fallback:

Request OpenCode pi
no session (before this PR) 5s 10s
session, expansion on 30s 30s
session, query_expansion=off 5s 10s

8. Docs

  • docs/{zh,en}/api/16-memory.md: the /recall response example still showed "successor": "/api/v1/search/search?mode=context"; the code returns "/api/v1/search/search" plus a separate "successor_body": {"mode": "context"}, so copying the example into a compatibility assertion would not match a real response
  • 06-retrieval.md: the memories category row, the corrected floor rule, the no_relevant ledger behaviour, and the note that the memory overview substitute is the one case where the served detail may outrank the pin — and why that is still a content downgrade
  • agent-integrations/01-overview.md: the deadline described per stage rather than rewrite-only, and an accurate statement of which harness reads which knob — the earlier wording implied every harness took the same environment variables, which was false for query expansion

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 — 144 passed, with /tmp/ov-test.conf containing {}.

Five new Python regression tests: both resource paths that must not read a body (missing abstract under an explicit pin and on the default path, and an oversized abstract), the memory substitute that must survive unchanged, the category_for contract together with the penalty and pin lookups that were missing, and the no_relevant turn that must leave the ledger empty.

OPENVIKING_STATE_DIR="$(mktemp -d)" node --test $(rg --files examples | rg '\.test\.mjs$' | sort) — 251 passed.

On the JS side, pi's recall manager gets a test asserting the session id reaches the request body once a session exists, and a new recall-session-wiring.test.mjs pins all three harness call sites. That file is deliberately source-level: each harness stubs its transport below the point where the session id is added, so no existing unit test could observe the omission — which is how three integrations shipped without dedup. It now also pins the timeout passthrough and the query-expansion opt-out in both adapters, the two gaps that turned the session wiring into a latency regression.

The shared core gets a test asserting the deadline tracks the stages the body asks for, including that an explicit query_expansion: "off" costs no headroom.

cd docs && npm run check:api — passes.

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

The tier fixes trade one behaviour deliberately: a resource ingested with processing_mode=vectors_only now returns as a bare URI plus its score rather than an overview of its body. That is the designed floor — #3534 states that every tier carries its URI so the model can drill down through the MCP read tool, and its bare-URI grace rule keeps such an entry out of the dedup cooldown so it returns on the next turn. The alternative, keeping the substitution, means the default recall path reads resource bodies, which is exactly what the per-category tier table was introduced to stop.

events still deepens to full and every memory category keeps the overview substitute, so this changes nothing on the memory path that the tier-model measurements in #3534 were based on.

@t0saki
fix(retrieval): honor tier ceilings and stop cooling unserved recalls
4f5e97b 
Follow-up to volcengine#3534, from its post-merge review round.

- The abstract-to-overview substitute now applies only to categories whose
  stored abstract is the whole file body. A resource or skill whose abstract is
  missing (`processing_mode=vectors_only`) or over the per-entry cap read its
  body and returned an overview instead, which for a short file is the body
  almost verbatim — crossing the opt-in deepening boundary those categories are
  documented to have, and doing it even under an explicit `detail="abstract"`.
  They now degrade to a bare URI and their body is never read.
- A digest reporting `no_relevant` blanks `rendered`, so the client injects
  nothing, yet those URIs still entered the dedup ledger and were cooled for
  `dedup_turns` turns. That contradicted the ledger's own bare-URI grace rule
  and held memories back from the later turn they were relevant to.
- Flat retrieval reaches built-in memory types outside the four named ones
  (`cases`, `patterns`, `tools`, `trajectories`, skill-usage memories) and
  reported them as an undeclared `memories` category that no tier or penalty
  table covered, so other-peer hits skipped the score penalty and callers could
  not pin their tier. The catch-all is now a declared category with both; it
  stays out of `quotas`, whose buckets it would overlap. Skill-usage memories
  also stop being misread as the `skills` category.
- ZCode, OpenCode and pi own an OV session id but did not forward it, so their
  recalls silently ran without query expansion or cross-turn dedup.
- The context-request deadline covered only the server's 30s rewrite fuse, but
  the pipeline is serial: expansion, retrieval and budgeting all precede it.
  45s covers both fuses and the work between them.
- `plugin` config scope and the `/recall` successor example now match what the
  code actually does.
Copilot AI lite review requested due to automatic review settings August 5, 2026 08:36
Copilot AI reviewed Aug 5, 2026
View reviewed changes

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.

Pull request overview

This PR is a follow-up bugfix sweep for the server-side context assembly (“/search mode=context”) and its harness plugins, tightening the tier/ceiling contract to prevent unintended body reads, fixing dedup ledger behavior on no_relevant rewrites, and ensuring integrations consistently send session IDs so dedup/query-expansion engage.

Changes:

  • Enforce category-specific “abstract substitute” behavior so resources/skills never read or return bodies past their tier ceiling when abstracts are missing/oversized, while keeping the memory-category substitute semantics.
  • Prevent no_relevant digest rewrites from recording planned-but-unserved entries in the dedup ledger; add/extend regression tests around tiering, category reporting, and ledger behavior.
  • Make the “other built-in memory types” report as a declared memories category (pin/penalty-aware) and update API validation accordingly; wire session IDs through ZCode/OpenCode/pi harnesses and adjust context request timeout rationale.

Reviewed changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/retrieve/test_context_assembler_pipeline.py Adds regression tests for resource tier ceilings, memory substitute behavior, reported categories, and no_relevant ledger handling.
openviking/server/routers/search.py Distinguishes quota-key vs reported-category validation (CATEGORY_KEYS vs REPORTED_CATEGORY_KEYS).
openviking/retrieve/context_assembler/tiers.py Introduces abstract_substitute() and uses it to avoid substituting resource/skill bodies.
openviking/retrieve/context_assembler/pipeline.py Stops ledger cooling when rewrite status is no_relevant; renames/read-gating uses oversized_abstract_needs_body.
openviking/retrieve/context_assembler/params.py Adds OTHER_MEMORY_CATEGORY (memories), REPORTED_CATEGORY_KEYS, default tier/penalty entries, and gates “full-body abstract” categories.
openviking/retrieve/context_assembler/gather.py Ensures flat retrieval reports unknown memory subtypes as memories and aligns declared-category checks with REPORTED_CATEGORY_KEYS.
openviking/retrieve/context_assembler/budget.py Makes downgrade planning candidate-aware; replaces abstract_over_cap with oversized_abstract_needs_body and uses abstract_substitute() for safe fallbacks.
openviking/retrieve/context_assembler/init.py Exports OTHER_MEMORY_CATEGORY and REPORTED_CATEGORY_KEYS for API/router consumers.
examples/zcode-memory-plugin/scripts/zcode-hook.mjs Forwards derived sessionId into the recall call site.
examples/zcode-memory-plugin/scripts/shared/recall-core.mjs Updates rewrite-request timeout rationale and increases deadline to 45s.
examples/zcode-memory-plugin/scripts/shared/plugin-config.mjs Restricts documented plugin consumers to actual loaders (Claude Code, Codex).
examples/pi-coding-agent-extension/tests/recall-deferred.test.mjs Adds a test asserting session id propagation enables session_id/dedup_turns.
examples/pi-coding-agent-extension/shared/recall-core.mjs Updates rewrite-request timeout rationale and increases deadline to 45s.
examples/pi-coding-agent-extension/shared/recall-core.d.mts Updates type defs to include sessionId option.
examples/pi-coding-agent-extension/shared/plugin-config.mjs Aligns plugin consumer documentation and HARNESS_KEYS with actual usage.
examples/pi-coding-agent-extension/recall.ts Threads a lazy session-id getter into recall requests so server dedup/expansion engage.
examples/pi-coding-agent-extension/index.ts Wires SyncManager.sessionId into RecallManager construction.
examples/ovcli.conf.example Removes inert per-harness plugin.* stanzas that aren’t consumed.
examples/opencode-plugin/lib/shared/recall-core.mjs Updates rewrite-request timeout rationale and increases deadline to 45s.
examples/opencode-plugin/lib/shared/plugin-config.mjs Aligns plugin consumer documentation and HARNESS_KEYS with actual usage.
examples/opencode-plugin/lib/memory-recall.mjs Forwards mapped OV session id to enable dedup/query expansion on context requests.
examples/opencode-plugin/index.mjs Passes sessionManager into recall component for session-id mapping.
examples/memory-plugin-shared/recall-session-wiring.test.mjs Adds source-level assertions pinning session-id wiring in all three harnesses.
examples/memory-plugin-shared/recall-core.test.mjs Updates timeout expectation to reflect serial server pipeline.
examples/memory-plugin-shared/lib/recall-core.mjs Updates rewrite-request timeout rationale and increases deadline to 45s.
examples/memory-plugin-shared/lib/plugin-config.mjs Aligns plugin consumer documentation and HARNESS_KEYS with actual usage.
examples/codex-memory-plugin/scripts/shared/recall-core.mjs Updates rewrite-request timeout rationale and increases deadline to 45s.
examples/codex-memory-plugin/scripts/shared/plugin-config.mjs Aligns plugin consumer documentation and HARNESS_KEYS with actual usage.
examples/claude-code-memory-plugin/scripts/shared/recall-core.mjs Updates rewrite-request timeout rationale and increases deadline to 45s.
examples/claude-code-memory-plugin/scripts/shared/plugin-config.mjs Aligns plugin consumer documentation and HARNESS_KEYS with actual usage.
docs/zh/api/16-memory.md Fixes /recall successor example to match actual response shape.
docs/zh/api/06-retrieval.md Documents memories category, corrected floor/substitute semantics, and no_relevant ledger behavior.
docs/zh/agent-integrations/01-overview.md Clarifies plugin consumer scope and explains the serial-pipeline timeout reasoning.
docs/en/api/16-memory.md Fixes /recall successor example to match actual response shape.
docs/en/api/06-retrieval.md Documents memories category, corrected floor/substitute semantics, and no_relevant ledger behavior.
docs/en/agent-integrations/01-overview.md Clarifies plugin consumer scope and explains the serial-pipeline timeout reasoning.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@t0saki
fix(retrieval): make the context deadline and expansion opt-out reach…
14ccb62 
…able

Forwarding a session id turns on server-side query expansion, an LLM call with
its own 5s fuse, but neither the deadline that was supposed to cover it nor the
switch that turns it off reached the two harnesses this PR newly enabled it for.

- `contextRequestTimeoutMs()` now derives the deadline from the request body
  rather than from `cfg` plus a rewrite flag. The body is what states which
  server stages will run: a session takes the expansion fuse, `rewrite` takes
  the digest fuse, and a bare retrieval takes neither and keeps the caller's own
  budget. Reading `cfg` alone could not tell those apart.
- OpenCode pinned `timeoutMs: 5000` after spreading the helper's options and pi
  ignored them entirely, so the helper's deadline was dead code in both. Their
  own budgets are now defaults rather than ceilings. OpenCode's 5s in particular
  was shorter than the expansion fuse it had just enabled, so a legal request
  would have been aborted client-side and dropped back to the path with neither
  dedup nor expansion.
- OpenCode and pi read `OPENVIKING_RECALL_QUERY_EXPANSION` (and
  `recallQueryExpansion` in their own config files) and set the `configured`
  flag the shared body builder requires, so the documented opt-out exists where
  the cost was introduced.
- The integration overview no longer implies every harness reads the same
  environment knobs, and describes the deadline as per-stage rather than
  rewrite-only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

OpenViking project
Status: Backlog

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@t0saki