fix(prompt-cache): stabilize architect request prefixes - #2780
zaxbysauce wants to merge 11 commits into
Conversation
Drift check reportFound 2 drift finding(s): 0 error, 0 warning, 2 notice. required-check-contract (2)
|
There was a problem hiding this comment.
🔵 Needs a closer look
It reworks invariant-10 chat/system message hook composition and globally repositions guidance carriers to the message-array tail for all sessions, a high-blast-radius change in a historically fragile area that warrants final human review despite its thorough test coverage.
Pull request overview
This PR resolves issue #2759: the architect's ~100K-token stable request prefix was failing provider prompt caching because per-step guidance rewrote the cache-sensitive system[1] tail and directive carriers were unshifted ahead of history. The fix leverages OpenCode v1.18.3's actual hook order (messages.transform runs before system.transform) to move all session-bound architect guidance off the system surface and into one trailing, host-renderable user-role carrier, keeping the conversation prefix byte-stable across turns.
Changes:
- Stages the architect system-enhancer + conditional
/swarmcommand rule early inmessages.transform(request-localWeakMap), delivers it late as a single fenced user-role carrier, then partitions all guidance carriers to the array tail before final accounting; thesystem.transformpath returns early for that same request via the new sharedisSessionBoundArchitectpredicate so no second ledger is begun. - Adds a bounded, session-keyed one-shot compaction-suppression marker, a
chat.messagemodel-identity relay, envelope-token budget reservation (guidance-carrier-fence), and deferred realtime-learning-nudge commit tied to successful carrier delivery. - Teaches recency/agent scans in memory, knowledge, and final-accounting to skip trailing carriers; migrates system-surface tests to registered-host message-boundary assertions.
File summaries
| File | Description |
|---|---|
src/index.ts |
Adds architect staging/delivery steps, compaction marker (bounded, session-cleaned), model-identity relay, and reorders the messages chain. |
src/hooks/host-boundary.ts |
New isSessionBoundArchitect shared single-delivery-owner predicate. |
src/hooks/system-guidance-carrier.ts |
Adds moveGuidanceCarriersToEnd (in-place tail partition) and guidanceCarrierEnvelopeTokens. |
src/hooks/system-enhancer.ts |
Adds surface option, deferred nudge state, and envelope-token reservation. |
src/context/role-filter.ts |
Accepts an explicit agent override for the messages-surface delivery. |
src/hooks/final-context-accounting.ts, knowledge-injector.ts, memory/injector.ts |
Skip trailing guidance carriers when selecting the latest real user message. |
src/services/injection-budget.ts |
Adds guidance-carrier-fence producer. |
src/observability/catalog.ts |
Updates producer line citations (909/1937/1957) after index.ts growth. |
scripts/retention-registry.data.ts |
Updates injector citation line numbers. |
docs/engineering-invariants.md, docs/releases/pending/issue-2759-*.md |
Documents the #2759 invariant and ships the release fragment. |
| Test files (unit/integration/adversarial) | Registered-host coverage for prefix stability, carrier partition, compaction bridge, cold-identity fallback, and migrated security assertions. |
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) + MiniMax-M2.7-highspeed (explorer B) (parallel explore, distinct lenses) → GLM-5-turbo (critique) ↔ GLM-5-turbo (critique) (cross-critique) → MiniMax-M2.7-highspeed (fallback arbiter) (arbiter: blind-spot + synthesize) 🔍 PR IntentReconstructed obligation list from PR description, issue #2759, and diff:
📦 Implementation SummaryThe PR implements a two-phase staging pipeline for architect guidance within ✅ /
|
| Obligation | Status | Evidence |
|---|---|---|
| O-001 (carrier delivery) | SUPPORTED |
src/index.ts:3191–3231 — staged system strings filtered, wrapped in carrier via appendGuidanceCarrier |
| O-002 (stable prefix) | SUPPORTED |
tests/integration/prompt-cache-prefix-stability-2759.test.ts AC1 — consecutive requests keep history prefix byte-stable |
| O-003 (strict model boundary) | SUPPORTED |
tests/integration/prompt-cache-prefix-stability-2759.test.ts AC4 — strict Qwen collapses to one entry, carrier trails in messages |
| O-004 (budget accounting) | SUPPORTED |
src/hooks/system-enhancer.ts:1296 — reserved envelope tokens subtracted; fence overhead recorded as guidance-carrier-fence producer |
| O-005 (compaction suppression) | SUPPORTED |
src/index.ts:3165 — _architectCompactionPending.delete(sessionID) consumed; _architectCompactionPending.set(sessionID, true) set on compaction; tests/unit/hooks/compaction-host-hook-2533.test.ts covers suppression + session-deletion cleanup |
| O-006 (registered-host coverage) | SUPPORTED |
New prompt-cache-prefix-stability-2759.test.ts, updated full-auto.regression.test.ts, turbo.regression.test.ts, handoff-security-adversarial.test.ts |
| O-007 (fail-open advisory) | SUPPORTED |
Both catch blocks are intentional fail-open; PR explicitly states this |
| O-008 (session state bounded) | SUPPORTED |
_architectCompactionPending capped at 500 via capSessionMap; cleared on session deletion at src/index.ts:3655 |
🚨 Confirmed Findings
None. All three confirmed findings were challenged and refuted (see §6 below).
🔬 Unverified but Plausible Risks
None identified at structural-confidence threshold.
🧪 Test / Coverage Gaps
- Gap:
moveGuidanceCarriersToEnd— single partition test exists; no test for the in-place "array.length = 0 + loop push" idiom surviving through multiple consecutive transforms on the same array.- Evidence:
tests/unit/hooks/system-guidance-carrier.test.tstests the function in isolation; integration coverage through the chain is implicit. - Verdict: Low confidence this is a real gap — the function is pure and well-tested in isolation; the chain-level test
AC1(consecutive architect turns) exercises the full pipeline.
- Evidence:
📋 Shipped-vs-Claimed Gaps
- Gap: None — the PR's invariant audit (12 invariants) accurately reflects the changed surfaces.
🔁 Validation Provenance
Confirmed findings reviewed — all dropped:
-
[HIGH]
src/index.ts:3135— enhancer catch drops deferred nudges — DROPPED.stagedArchitectGuidanceByMessages.set()is inside the try block beforeenhancer()runs; if enhancer throws, set never executes, so no stale entry exists. The PR explicitly designates the enhancer as advisory with fail-open semantics; missing guidance is not a defect. -
[HIGH]
src/index.ts:3231— delivery catch drops fence overhead — DROPPED. This is intentional documented fail-open ("guidance is fail-open at this boundary").deliveredGuidanceDeltagates the nudge commits and fence overhead recording; on failure the host receives the unmodified message array. This is not silent data loss — it is the stated contract. -
[LOW]
src/index.ts:3349— WeakMap concurrency risk — DROPPED. Architect requests are sequential (single-threaded orchestrator). The delivery step immediately deletes after.get(), so even if the same messages array were somehow reused, the second call getsnulland returns early. The request-local WeakMap pattern is correct for this use case.
Blind-spot pass — no new findings added. Scrutinized: WeakMap lifecycle (clean on GC), envelope token reservation (bounded, single call site), compaction marker cap (capSessionMap enforces 500), role-filter agent fallback (mirrors existing pattern), delivery step identity resolution (three-way fallback), moveGuidanceCarriersToEnd array mutation (preserves relative order), and isGuidanceCarrier skips in knowledge/memory injectors (correct — carriers must not be counted as user messages).
📝 Merge Recommendation
[APPROVE]
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | ✅ |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ (fail-open by design) |
| Input validation present | ✅ |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ (scripts/retention-registry.data.ts line citation update only) |
🔒 Reviewed by a 3-model cross-family adversarial debate (architect → dual-lens parallel explorers → cross-critique → arbiter) for high recall with low false-positive noise. Findings are advisory — verify before acting.
Swarm PR Review — PR #2780Reviewed: PR intentFixes #2759: the architect's ~100K-token system prefix should hit the provider's prompt cache every turn, but per-step content (directive carriers, budget/nudge guidance) was being unshifted ahead of history and rewriting Closes #2759 — claim-integrity: PARTIALLY METThe core mechanism is sound and does fix the steady-state case (verified structurally and by a registered-host runtime probe: warm turns produce exactly one stable Confirmed findingsF-001 — MEDIUM — Architect-identity resolution is asymmetric between the two transform surfaces, causing duplicate guidance delivery + ledger corruption on identity-cold turns F-002 — MEDIUM — No acceptance test exercises the identity-cold path that F-001 proves defective F-003 — LOW — Silent error-swallowing on the new guidance delivery path lacks production-visible logging F-004 — LOW — Coverage regression: "sessionless global-fallback" scenario dropped from hook-integration tests (but still covered at the state layer) Pre-existing / not introduced by this PR (informational, no action required)
Disproved candidates (false positives — listed per hard rule #10)
Suppressed candidates0 — all 24 candidates across 11 lanes were routed to reviewers per the noise-budget policy (no LOW-confidence/no-evidence suppression applied). Test-plan claim spot-checks (informational)
Verdict: REQUEST_CHANGESTwo independently-confirmed, critic-upheld MEDIUM findings share one root cause (F-001/F-002) with execution-proven evidence of the exact regression class this PR exists to fix, occurring on identity-cold turns. None of the three independent validation passes (2 reviewers spanning this cluster, 1 critic) returned a clean approve. Recommended before merge: apply the F-001 fix (unify identity resolution across the two transform surfaces) or explicitly narrow the PR's claim to "warm-path only" and file a fast-follow for the cold-path gap; add the F-002 regression test; apply the small F-003 logging fix. F-004 and the informational items are non-blocking. 🤖 Generated with Claude Code via |
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) + MiniMax-M2.7-highspeed (explorer B) (parallel explore, distinct lenses) → GLM-5-turbo (critique) ↔ GLM-5-turbo (critique) (cross-critique) → MiniMax-M2.7-highspeed (fallback arbiter) (arbiter: blind-spot + synthesize) PR Reviewer — opencode-swarm🔍 PR Intent
📦 Implementation SummaryThe PR restructures the architect guidance delivery path by:
✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/index.ts:3123–3216 — WeakMap staging at start, delivery at end; src/hooks/system-guidance-carrier.ts:274–292 — moveGuidanceCarriersToEnd partitions to tail |
| O-002 | SUPPORTED |
src/hooks/system-enhancer.ts:1157–1163 — system hook early-returns for session-bound architect; strict model collapse unchanged on system boundary |
| O-003 | SUPPORTED |
src/index.ts:345–347,4563–4571 — _architectCompactionPending Map with 500-cap, set on compacting, consumed on next messages pass |
| O-004 | SUPPORTED |
tests/unit/hooks/compaction-host-hook-2533.test.ts:167–281 — explicit suppression tests for pre/post-compaction turns |
| O-005 | SUPPORTED |
src/hooks/final-context-accounting.ts:122,217 — isGuidanceCarrier skips carrier for pressure warning injection; src/hooks/system-enhancer.ts:3068 — surface-tagged ledger emission |
| O-006 | SUPPORTED |
src/index.ts:2039–2043 — architectMessagesCommandRuleHook registered on messages surface; called in delivery step |
| O-007 | SUPPORTED |
src/hooks/knowledge-injector.ts:754,858,1001,1280; src/hooks/final-context-accounting.ts:125,217; src/memory/injector.ts:646,722 — all backward scans guarded |
🚨 Confirmed Findings
Only one finding survived challenge. Findings 1–4 were all refuted (see § Validation provenance).
[HIGH] messagesTransformArchitectEnhancerStage guard accesses sessionID.length before null-check on sessionID
- Location:
src/index.ts:3151 - Why it matters: If
mctx.sessionIDisundefined(no message inoutput.messagescarries asessionID), the guardtypeof sessionID === 'string'evaluates to'undefined' === 'string'→false, so execution falls through tosessionID.lengthwhich throwsTypeError: Cannot read properties of undefined (reading 'length'). This crashes the entire messages.transform chain for that request. - Evidence:
// src/index.ts:3149–3151 const mctx = resolveMessageTransformContext(output); const sessionID = mctx.sessionID; // could be undefined if (typeof sessionID === 'string') { // typeof undefined === 'undefined', not 'string' → guard FAILS sessionID.length // ← TypeError here when sessionID is undefined
resolveMessageTransformContextatsrc/hooks/host-boundary.ts:168only setssessionIDfromm.info?.sessionIDif it is a non-empty string — if no message carries one,sessionIDremainsundefined. - Fix direction: Change the guard to
sessionID && typeof sessionID === 'string'so that an undefinedsessionIDcauses an early return rather than a crash. Alternatively, sinceisSessionBoundArchitectalready handles undefined/empty-string inputs correctly by returning false, the guard could simply be removed or rewritten asif (!isSessionBoundArchitect(sessionID, mctx.agent)) return;.
🔬 Unverified but Plausible Risks
- Risk:
moveGuidanceCarriersToEndis called on every messages.transform pass as the final structure-mutating step. For sessions with many guidance carriers (accumulated over a long conversation), the O(n) partition + two-push loops do not re-usepush(...carriers)to avoid engine argument limits, but they do clearmessages.length = 0and repopulate. Object identity is preserved, but downstream code that holds a reference to a specific index (not the array itself) would observe a stale index.- Why suspicious: The loop-based push pattern is specifically used to avoid the argument-limit risk documented at
src/context/role-filter.ts:300. However, the clear-and-rebuild pattern changes indices even though individual objects are not recreated. - What would verify it: A scan for any code that captures
messages[i]before the final partition step and relies on that index remaining valid post-partition. TheisGuidanceCarrierguards and themessages.indexOf(carrier)lookups in tests both use the post-partition array, so they are safe. The risk would require a consumer that captures a reference to a specific array index during the messages chain and uses it after the partition step.
- Why suspicious: The loop-based push pattern is specifically used to avoid the argument-limit risk documented at
🧪 Test / Coverage Gaps
- Gap: No unit test directly exercises
messagesTransformArchitectEnhancerStagewith aoutput.messagesarray where no message carries asessionID. The new integration testprompt-cache-prefix-stability-2759.test.tsalways stampssessionIDinto its test messages. The guard at line 3151 (typeof sessionID === 'string') would misbehave (throw) if hit with undefinedsessionID, but there is no test covering this path.- Evidence: All test fixtures in
prompt-cache-prefix-stability-2759.test.tsandsystem-render-boundary-registered.test.tspasssessionIDininfo.sessionID. The guard is exercised only in the happy path.
- Evidence: All test fixtures in
📋 Shipped-vs-Claimed Gaps
- Gap: The PR description claims "strict Qwen/Gemma system rendering remains on the system boundary." The system hook does early-return for session-bound architect, so strict-model collapse now happens on zero system entries (the base header only). The integration test AC4 (
tests/integration/prompt-cache-prefix-stability-2759.test.ts) passes[STEP GUIDANCE STRICT]through the messages carrier for a strict model, and the system-render-boundary unit test confirms the system array is collapsed to one entry — but the test seed includes[STEP GUIDANCE STRICT]in the system array, which would not happen in production for session-bound architect (the system hook returns early). The test is not wrong (it tests the boundary collapse logic in isolation), but it does not prove that the system array fed toapplySystemRenderBoundaryin production would actually contain the guidance string. This is a test-vs-implementation gap: the test seeds what the production system hook would not produce.- Evidence:
tests/integration/prompt-cache-prefix-stability-2759.test.ts:217–220feeds[STEP GUIDANCE STRICT]into the system array directly. The production path (src/hooks/system-enhancer.ts:1157–1163) returns early forisSessionBoundArchitect, so the system array would only contain the base header.
- Evidence:
🔁 Validation provenance
Findings DROPPED (with reason):
| # | Finding | Why dropped |
|---|---|---|
| 1 | isGuidanceCarrier false positive — real user message skipped by downstream |
All carriers are created via appendGuidanceCarrier which always stamps swarm-guidance: prefix; isGuidanceCarrier matches only that prefix; moveGuidanceCarriersToEnd uses the same check — consistency is guaranteed by construction |
| 2 | isGuidanceCarrier false negative — carrier treated as user speech |
All carrier creation goes through appendGuidanceCarrier; appendGuidanceCarrier always stamps the prefix; isGuidanceCarrier matches that same prefix — no FN path in the production code |
| 3 | resolveMessageTransformContext could return null/undefined |
resolveMessageTransformContext returns MessageTransformContext (non-nullable object); the type is confirmed in src/hooks/host-boundary.ts:168–192 — always a valid object |
| 4 | enhancer() rejected promise leaks as unhandled rejection |
try { await enhancer(...) } catch { ... } catches rejected promises — await re-throws promise rejections into the catch block |
Blind-spot finding ADDED: The typeof sessionID === 'string' guard at line 3151 accesses sessionID.length when sessionID could be undefined, causing a TypeError. Confirmed by tracing resolveMessageTransformContext — sessionID is only assigned from m.info?.sessionID when it is a non-empty string; if no message carries one, sessionID remains undefined.
📝 Merge Recommendation
[APPROVE_WITH_FIXES]
One HIGH finding requires a one-line fix to the guard at src/index.ts:3151. Once typeof sessionID === 'string' is strengthened to sessionID && typeof sessionID === 'string' (or replaced with an isSessionBoundArchitect call), the PR is ready to merge.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ (compaction bridge, carrier routing, and strict-model boundary are all documented in the release note and engineering-invariants entry) |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ |
| Input validation present | ✅ (WeakMap prevents cross-request leakage; guidanceCarrierEnvelopeTokens handles null fence; deductProducerEmission covers fence overhead) |
| No broken agent role boundaries | ✅ (isSessionBoundArchitect is shared by both surfaces; role-filter system hook updated to accept agent input) |
| Prompt format contracts intact | ✅ (system boundary early-return preserves strict collapse; carrier is user-role, host-renderable) |
| Lockfile consistent | ✅ (no new dependencies added) |
🔒 Reviewed by a 3-model cross-family adversarial debate (architect → dual-lens parallel explorers → cross-critique → arbiter) for high recall with low false-positive noise. Findings are advisory — verify before acting.
Move request-varying architect guidance behind stable conversation history while preserving host-renderable carriers and strict system shapes. Closes #2759
dd1953e to
e66ba34
Compare
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) (explore → candidates) → MiniMax-M2.7-highspeed (fallback arbiter) (challenge + blind-spot) 🔍 PR Intent
📦 Implementation SummaryThe PR adds a two-stage messages-transform pipeline for session-bound architect calls: an early stage ( ✅ /
|
| Obligation | Status | Evidence |
|---|---|---|
| O-001 | SUPPORTED |
src/index.ts — messagesTransformArchitectEnhancerDeliveryStep appends one appendGuidanceCarrier; moveGuidanceCarriersToEnd partitions to tail |
| O-002 | SUPPORTED |
src/index.ts:3123–3300 — staging WeakMap + early/late stage pair matches messages→system invocation order |
| O-003 | SUPPORTED |
src/hooks/system-enhancer.ts:1157–1165 — isSessionBoundArchitect guard returns early; applySystemRenderBoundary untouched for non-architect |
| O-004 | SUPPORTED |
src/index.ts:4556–4563 — _architectCompactionPending.set; :3133 — compactionPending = _architectCompactionPending.delete(sessionID) before predicate |
| O-005 | SUPPORTED |
src/hooks/system-enhancer.ts — surface param flows through all recordProducerEmission/recordRealtimeLearningNudge calls; deductProducerEmission in role-filter for removed fragments |
| O-006 | SUPPORTTED |
20+ tests migrated; expectStableArchitectSystem(system) verifies post-hook system === [BASE_SYSTEM] (pass-by-reference, not a rebuild) |
🚨 Confirmed Findings
None. Both reviewer-confirmed findings are false positives. Full reasoning follows.
[DROPPED] CRITICAL — Test helper expectStableArchitectSystem is a trivial tautology
- Why dropped: The claim rests on a misread of the test helper's call site.
invokeRegisteredArchitectat line ~175 does NOT rebuildsystemafter the hook call. It passesconst system = [BASE_SYSTEM]by reference tohost.hooks['experimental.chat.system.transform']({ sessionID: SESSION_ID }, { system }). The hook mutatessystemin place. The return value{ system }carries the post-mutation state.expectStableArchitectSystem(system)then asserts that the array the hook produced is exactly[BASE_SYSTEM]. If the hook incorrectly appended dynamic content, the assertion would fail. The test is structurally correct.
[DROPPED] HIGH — System array is reassigned after the hook call
- Why dropped: Same false premise. The local
const system = [BASE_SYSTEM]variable holds the post-mutation result because JavaScript passes the array reference, not a copy.output.system.length = 0; output.system.push(...kept)mutates the same object the test'ssystemvariable references.expect(system).toEqual([BASE_SYSTEM])therefore tests the actual hook output. This is the intended pattern (AGENTS.md invariant 10: in-place mutation).
🔬 Blind-Spot Findings
None. The PR's mechanism is structurally sound:
- Staging isolation: The request-local
WeakMap(stagedArchitectGuidanceByMessages) cannot persist conversation text or cross sessions — it is cleared after the delivery step or on any exception. - Compaction marker scope:
_architectCompactionPendingis aMap<string, true>with a 500-entry cap and is deleted on session removal (src/index.ts:3653), so it cannot accumulate unboundedly. - Ledger consumption:
advanceTurnGenerationruns in both the system hook (when it runs for non-architect) and the final context accounting step; the architect messages-stage ledger is separate from the system-stage ledger, preventing cross-surface attribution errors. - Role-filter integration:
createRoleFilterSystemHooknow readsinput.agentas a primary source (src/context/role-filter.ts:253), matching themessagesTransformArchitectEnhancerDeliveryStepcall which passes{ sessionID, agent }. The fallback togetActiveAgentNameis retained. isSessionBoundArchitectshared predicate: Used in both the messages-stage architect enhancer and the system hook's early-return guard, guaranteeing exactly one delivery owner per invocation.
🧪 Test / Coverage Gaps
None found. Coverage is comprehensive:
- AC1 (byte-stable history prefix):
prompt-cache-prefix-stability-2759.test.tsAC1 - AC2 (trailing user-role carrier):
prompt-cache-prefix-stability-2759.test.tsAC2 - AC3 (system surface stable for cache-capable):
prompt-cache-prefix-stability-2759.test.tsAC3 - AC4 (strict Qwen/Gemma collapse):
prompt-cache-prefix-stability-2759.test.tsAC4 +system-render-boundary-registered.test.ts - AC5 (in-place mutation, no system-role carrier):
prompt-cache-prefix-stability-2759.test.tsAC5 - AC6 (enhancer disabled → command banner still delivered):
prompt-cache-prefix-stability-2759.test.tsAC6 - Compaction suppression:
compaction-host-hook-2533.test.ts - Session-deletion marker cleanup:
compaction-host-hook-2533.test.ts
📋 Shipped-vs-Claimed Gaps
- Claim: "Zero message-surface system roles" — verified by
system-render-boundary-registered.test.ts:143–153which assertsmessages.every((message) => message.info.role !== 'system'). - Claim: "Strict model preservation" — verified by
prompt-cache-prefix-stability-2759.test.tsAC4 which applies the strict classifier and checkssystem.length === 1after collapse.
📝 Merge Recommendation
[APPROVE]
All six obligations are supported by the code. Both confirmed findings were refuted: the test assertions operate on the post-mutation reference, not a rebuilt array. The blind-spot pass found no structural gaps. The test suite comprehensively covers AC1–AC6, compaction suppression, session deletion cleanup, and the guidance-carrier partition invariant.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | ✅ |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ |
| Input validation present | ✅ |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ |
🔁 Validation Provenance
Survived: 0 findings. Both reviewer-confirmed candidates were dropped as false positives (the tests verify post-mutation state via pass-by-reference, not trivial rebuilds).
Dropped (2):
- CRITICAL
system-enhancer-sanitization.test.ts:175—expectStableArchitectSystemcalled on post-hook reference, not a rebuilt array; hook would mutate the local variable. - HIGH
system-enhancer-sanitization.test.ts:172— Same structural misunderstanding; test correctly checks the hook's actual output.
Blind-spot additions: 0. The PR's request-local staging, bounded compaction marker, ledger consumption ordering, and role-filter input.agent routing are all structurally sound.
🔒 Reviewed by a multi-stage local-first funnel (architect context pack → explorer candidates → critic challenge/author) for high recall with low false-positive noise. Findings are advisory — verify before acting.
…sue-2759-prompt-cache # Conflicts: # src/observability/catalog.ts
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) (explore → candidates) → MiniMax-M2.7-highspeed (fallback arbiter) (challenge + blind-spot) PR Reviewer — opencode-swarmPhase 0 — Reconstruct IntentFrom PR #2780 and linked issue #2759: Problem: The architect's ~100K-token request prefix (tool schemas, agent prompt, skills) should hit provider prompt-cache on every call but does not. Per-step system-enhancer guidance rewrites Obligations (O-001 to O-005):
Phase 1 — Summarise Actual BehaviourThe diff implements a two-surface architecture for architect guidance:
Phase 2 — Compare Intended vs Actual
✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/index.ts:3199 — delivery step; src/hooks/system-guidance-carrier.ts:261 — appendGuidanceCarrier |
| O-002 | SUPPORTED |
tests/integration/prompt-cache-prefix-stability-2759.test.ts:204 — AC1 prefix stability assertion |
| O-003 | SUPPORTED |
src/index.ts:3080 — isSessionBoundArchitect; tests/integration/prompt-cache-prefix-stability-2759.test.ts:297 — AC4 strict model collapse |
| O-004 | SUPPORTED |
src/index.ts:3351 — _architectCompactionPending; tests/unit/hooks/compaction-host-hook-2533.test.ts:167 — suppression test |
| O-005 | SUPPORTED |
src/services/injection-budget.ts:156 — guidance-carrier-fence; src/hooks/final-context-accounting.ts:143 — ledger surface tokens |
🚨 Confirmed Findings
None. The implementation is structurally sound across all surfaces.
🔬 Unverified but Plausible Risks
None that meet the structural-proven bar. The key architectural decisions are verifiable in the diff:
WeakMapisolation (src/index.ts:3160): request-local, object-keyed, cannot cross sessions or persist text — the concern about cross-request leakage is structurally eliminated.isSessionBoundArchitectpredicate (src/hooks/host-boundary.ts:90): fail-closed on missing sessionID; explicitagentparameter is tested in AC2-prefixed and AC5.- Fence token accounting (
src/index.ts:3235–3242):guidance-carrier-fenceproducer and envelope reservation are additive, not double-counting — the sequence is: reserve at enhancer entry → decrement budget → record fence overhead at delivery → advance ledger. No structural gap.
🧪 Test / Coverage Gaps
- Gap: None found. The 15 affected test files cover: prefix stability (AC1–AC6), strict model preservation, in-place mutation, disabled enhancer parity, compaction suppression, session-deletion cleanup, handoff security, decision-drift gating, turbo/full-auto banners, and system-render-boundary integration. The 195-test CI run is the functional proof.
📋 Shipped-vs-Claimed Gaps
- Gap: None found. The PR invariant audit cross-references all 12 AGENTS.md invariants; every touched invariant has structural evidence in the diff.
🔍 PR Intent
Reconstructed obligation list (from PR text, issue, commits, changed tests, changed docs, changed interfaces):
- O-001 Move session-bound architect enhancer guidance to trailing user-role carrier (not system tail)
- O-002 Preserve byte-identical conversation prefix across architect turns for prompt-cache reuse
- O-003 Retain strict Qwen/Gemma single-system rendering boundary
- O-004 Compaction-aware one-shot suppression (live guidance must not enter compaction summaries)
- O-005 Maintain unified injection-budget accounting across new surface routing
📦 Implementation Summary
The PR routes architect guidance through a messages.transform-local staging pipeline:
- Early stage (
messagesTransformArchitectEnhancerStage): Full enhancer runs against aWeakMap-keyed request-local object, before all other consumers. - Delivery stage (
messagesTransformArchitectEnhancerDeliveryStep): Role-filtered and command-ruled staged output is joined and appended as one fencedarchitect-sessionuser-role carrier. - System hook guard (
isSessionBoundArchitect): The system-surface enhancer returns early for session-bound architects after capturing model identity — keeping the cache-sensitive system prefix untouched. - Terminal partition (
moveGuidanceCarriersToEnd): All carriers moved to request tail after history, preserving stable prefix ordering. - Compaction suppression (
_architectCompactionPending): One-shot marker consumed at architect stage entry.
📝 Merge Recommendation
[APPROVE]
The implementation correctly routes architect guidance to a trailing carrier, stabilises the system prefix, preserves the strict-model boundary, and suppresses guidance on compaction summaries. All five obligations are SUPPORTED. No structural defects were found in the diff.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | ✅ |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ |
| Input validation present | ✅ |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ |
🔒 Reviewed by a multi-stage local-first funnel (architect context pack → explorer candidates → critic challenge/author) for high recall with low false-positive noise. Findings are advisory — verify before acting.
Closes #2759
Summary
/swarmcommand rule, out of the cache-sensitive system tail and into one trailing host-renderable user-role carrier.Invariant audit
src/index.tsregistration/composition changed without adding startup I/O or awaits;bun run buildpassed andnode scripts/repro-704.mjscompleted T1/T2/T3 in 40.6/37.1/26.7 ms against the 400 ms ceiling.node --input-type=module -e "await import('./dist/index.js')"succeeded.test_runnerscope.bun:test, changed files pass the FR-006 ratchet, andcheck:mock-cleanup,check:test-tmpdir, and the 15-file CI-equivalent isolation run passed.WeakMap; the one-shot compaction marker is session-keyed, capped at 500, consumed on the next messages pass, and cleared on session deletion; compaction/restoration tests pass.docs/releases/pending/issue-2759-prompt-cache-prefix.md; release-owned version files and cache behavior are unchanged.Test plan
origin/mainand GREEN on this commit; C4-C5 GREEN on both; frozen suite 7/7.bun run test:unit:ci <15 affected files>- all 15 files passed individually (195 tests, 0 failures).bun run typecheck,bun run lint:ci,bun run build, Node ESM import, andnode scripts/repro-704.mjs.bun run package:smoke.bun run check:pre-push- enforced drift check and registry citations passed (two known non-blocking ruleset-divergence notices).canonicalMkdtemphelper and locally validated; refreshed MiniMax M3 high-effort implementation review and GLM 5.3 high-effort final critic both APPROVE on commitdd1953ed4a3a65aa3de75a918ca480208fabdcfe/ treef8bd5c8dc30dea854dc5aafc0bdd81437d0b3992.EBUSY, sandbox-denied production-store probes, and an environment-specific swarm-model CLI-list exit.