fix: bound the six remaining retained-state leaks (#820 wave 2) - #892
Open
lidge-jun wants to merge 92 commits into
Open
fix: bound the six remaining retained-state leaks (#820 wave 2)#892lidge-jun wants to merge 92 commits into
lidge-jun wants to merge 92 commits into
Conversation
…d mechanisms wp-a: modelWireDefaults on the github-copilot registry entry (mechanism already on tree: hard pin > modelAdapters > registry defaults > provider adapter); conservative 6-model set, nano/sol lead-only via modelAdapters. wp-b: adopt PR #860's capability file map + #875 root cause found — sanitizeReasoningInputContent blanks plaintext reasoning for every Responses provider (openai-responses.ts:35, called :1027); scope it. wp-c: registry.ts:217 map + authoritative-window [1m] predicate.
1: docs target corrected to reference/configuration/providers.md + locales 2: exact seed before/after array, defaultModel unchanged, nano/sol excluded 3: evidence table moved to 000 (LEXICO-SPLIT-01) 4: per-model pi.dev provenance + two-leg selection rule (sol/nano out)
…ve seed policy 1: gpt-5.6-sol meets the two-leg rule identically to luna/terra (#748 field report + pi.dev Responses declaration + JetBrains conditional chat failure); excluding it was an inconsistent rule application. nano stays out (no field report leg). 2: seed policy made explicitly additive (no removals); gpt-5-mini added as a verified chat model so the chat regression fixture is honest; scenario 4 corrected.
…ecedence fixture claim ledger and selection reference now say seven built-in / sol verified; scenario 3 uses gpt-5.4-nano or gpt-5-mini for the opt-in direction since sol is itself a default and cannot prove override precedence.
github-copilot fronts a mixed-wire catalog: gpt-5.3-codex, gpt-5.4, gpt-5.4-mini, gpt-5.5, gpt-5.6-luna, gpt-5.6-sol, gpt-5.6-terra reject /chat/completions for real Codex-agent traffic (function tools + reasoning). Declare them as registry modelWireDefaults so every inbound wire rides Responses while the provider-wide adapter stays openai-chat for the chat-served catalog. Explicit modelAdapters still win in both directions; gpt-5.4-nano stays out (no field report) as a documented override example. Evidence: issue #748 field runs, pi.dev wire declarations, litellm#23332 (gpt-5.4), JetBrains LLM-29711 (gpt-5.6-sol). Tests: tests/github-copilot-wire-defaults.test.ts — 22 cases incl. captured-upstream-URL replay proof on all three inbound wires. Consolidates the routing half of PR #746 (its sampling/credential-replay half remains a separate parity/security unit). Closes #748.
… bounds Three explorer passes found wave-1 landings already on dev (77243d9 framework, d1408b9 continuation cap+spill, 034d320 cache caps, a616078 translator budgets), so the campaign narrows to refinements: #841 admission boundary (direct-spill oversized, bounded snapshot read, bounded replay), #847 collector per-call scope + mandatory budget + 502 normalization, #844 incremental frames + typed partial-EOF, #845 NOOP (superseded), #843 fixed-size SHA-256 key identities, #840 ACL timeout-memo release + destination keying.
All 7 blockers accepted: blob-ID key channel unbounded (045 becomes a real fix), antigravity caps exclude key bytes, NUL-separator collision ambiguity (length-prefix instead), cursor backlog/parser-cursor state machine + frameWork-deferred EOF, collector test math + dual bridge budget sites + exact 413 sites, ACL temp-only release API, state typed-observability seams + failure/replacement scenarios.
001 synced with amended designs; 020 same-ID replacement split 8a/8b with pruning qualification; 030 scenario-7 asserts lease release after full serialized owner charge; 040 explicit rawBacklog byte cap incl. headers + drain-to-quiescence EOF; 045 validate/digest before hex expansion with separate key-bytes counter (64 MiB payload cap intact); 050 incremental length-prefixed hashing + replay_signature_oversized skip-replay overflow contract.
…l replay Oversized candidates (> 64 MiB resident cap) now write directly to durable spill and install only the stub — they never become resident and never demote unrelated chains; candidates above the new 256 MiB single-spill payload ceiling are tombstoned at admission instead of being retained unreadably. Snapshot loading refuses to parse files over 32 MiB and applies the same admission boundary to oversized rows. Spill replay refuses payloads above the ceiling before read/parse with a typed too_large reason (surfaced as spill_too_large; wire shape unchanged). Snapshot selection now measures UTF-8 bytes, not UTF-16 length. Refines #841 on top of the wave-1 spill design.
…s escape hatch reference/configuration/providers.md (EN + ko/ja/zh-cn/ru): the modelAdapters row now names the built-in Copilot Responses-only defaults and the opt-in path for models without one (gpt-5.4-nano example). guides/providers.md gains the routing-precedence note (pin > modelAdapters > registry default > provider adapter).
Envelope truth: the payload ceiling is enforced against the real
{version,...} spill envelope after publication (a candidate within the
wrapper's size of the cap was previously retained unreadably). Snapshot
size gate uses statSync so symlinked oversized snapshots cannot bypass
it. Over-ceiling same-ID tombstones now defer the old generation's
unlink until the tombstone is durable (same crash rule as
replaceSpillEntryAtomically, whose catch path gets the same fix).
directSpills only counts actual stub installs. Tests: exact-envelope
boundary, no-read proof (too_large beats missing), deferred-unlink
ordering for tombstone and spill replacement, symlinked snapshot
refusal, tighter same-ID accounting bound.
…ntics adopts #860's reviewed fail-closed semantics (undefined strips; explicit config is the escape hatch); names preserveResponsesReasoningContent as the reasoning-replay flag following the statelessResponses flow; maps every edit point on the current tree.
A snapshot symlinked to a FIFO or device (isFile=false, size=0, e.g. /dev/zero) fell through to readFileSync and could block or stream unbounded input; only regular files within the size ceiling are parsed now.
1: research survey moved to 000 (LEXICO-SPLIT-01) 2: guides/codex-app-models.md x5 locales added to docs file map 3: options.serviceTier clearing required + tested 4: ocxr1-strip-under-preservation negative case added 5: comment wording calibrated (accepted replay = verified; required = inference)
…to 502 The non-stream Chat collector charged streamed tool arguments to a generic retained_collectors scope, so one call could consume nearly the whole 32 MiB turn budget instead of the 2 MiB per-call limit; arguments now open a per-call scope (kind tool_args, wire-index key) and close it only after the final serialized owner is charged, with scopes released on every error path. Provider-controlled overflow in the collector is now 502 upstream_error on all three mapping sites (was 413 invalid_request_error on two), matching the adapter/bridge contract. buildResponseJSON/bridgeToResponsesSSE no longer have an unbounded no-budget path: omission creates a default turn budget (disposed with the call/stream) instead of skipping accounting.
…epSeek reasoning replay Two DeepSeek wire fixes, one capability flow: 1. service_tier is an OpenAI-only Responses parameter, but fast mode injected it for every Responses provider. A provider-level supportsServiceTier capability now gates it after the final route is settled: canonical openai/openai-apikey keep fast-mode inject/remove (unset fast mode preserves a caller value); deepseek and volcengine-agent-plan strip it; unclassified providers fail closed unless explicitly opted in. Stripping also clears options.serviceTier so logging never mislabels a removed tier. Adopts PR #860's reviewed fail-closed semantics. 2. sanitizeReasoningInputContent blanked reasoning content for EVERY Responses provider — a rule only the ChatGPT native backend needs. DeepSeek's Responses API accepts plaintext reasoning replay, so providers flagged preserveResponsesReasoningContent keep it (ocxr1 envelopes are still stripped). Fixes the local half of #875: continuations after tool calls no longer reach DeepSeek with emptied reasoning items. Both fields flow registry -> providerConfigSeed -> enrichProviderFromRegistry -> router backfill without overriding explicit config. Tests: tests/service-tier-capability.test.ts + tests/deepseek-reasoning-replay.test.ts (18 cases incl. live handleResponses payload capture).
…ent reference rows, capability-gated fast-tier guides reference/configuration/providers.md (EN/ko/ja/zh-cn/ru): rows for the two new provider fields. guides/codex-app-models.md (all five locales): the blanket 'routed non-OpenAI models strip service-tier metadata' wording is now the capability-gated fail-closed behavior with the explicit opt-in — closing #860's open docs review issue.
Final owner transfer is wrapped: a copy-charge overflow now releases the copies already charged, closes every open call scope, and surfaces the same typed 502 upstream_error instead of a raw budget exception. The translation_buffer_limit classification is 502 upstream_error on the remaining streaming sites (fail() override, response.failed handler, processing catch) and the defensive JSON replay path, so streaming, collector, and non-stream clients agree; genuine client-request 413s are untouched. Owned default SSE budgets are disposed at every stream-death path (terminal close, incomplete terminal, torn-down controller, heartbeat failure, cancel) after the final charges, never inside reportTerminal. New test seam translatorLiveBudgetCountForTests proves disposal; collector tests now cover final-copy overflow cleanup and exact surviving charge.
A client cancel during a pending upstream next() disposed the owned budget while the in-flight step still processed the late event, reanimating charges outside liveBudgets; step() now bails immediately after the await when closed/cancelled (emit() was already guarded). The defensive JSON replay status ternary for translation_buffer_limit is 502 (was 413) — every path now agrees. The final-copy overflow test threshold is corrected so the overflow actually activates in the final owner transfer (250, not 150). New seams: translatorAggregateCurrentBytesForTests; cancel-during-pending-next regression asserts live count and aggregate bytes return to baseline; endpoint regression pins the replay 502/upstream_error shape.
Seeding them via providerConfigSeed broke the management API's canonical openai seed comparison (exact key set) — 4 management-provider-validation failures. Follow the modelWireDefaults philosophy instead: the registry holds the defaults, providerConfigSeed stays free of them so an explicit user value stays distinguishable, and enrichProviderFromRegistry + the router backfill supply them from the entry directly.
Breaking to upstreamDone still ran the EOF path, whose closeCurrentMessage()/retainFinishedItem() charges the disposed budget directly (the emit() closed-guard does not cover retention); a cancelled step now returns immediately like the gated early-return path. The regression now synchronizes on the second upstream next() having actually started before cancelling.
…ts-inject + generator override desktop-profile and model-info guards already landed; the open halves are the registry map, #854's withSubagentContextMarker port, and the stale jawcode generator override contradicting its own committed output.
… EOF
The live transport concatenated every incoming chunk with the entire
pending remainder (O(n^2) on a large incomplete frame) and settled
stream end immediately: complete frames plus a trailing partial settled
as silent success. The backlog is now a {buf,start,end} cursor with
amortized growth (per-chunk O(chunk)) and lazy prefix compaction; a new
framing export consumeConnectFrames reports consumed raw bytes without
materializing a remainder copy. Accounting moves to raw used bytes
(headers included), closing the tiny-frame/header-flood gap in the
payload-only counter. Stream end now drains frameWork to quiescence,
fails typed frame_incomplete on any unconsumed remainder (unless the
close was expected), and keeps the zero-frame unexpected-EOF
classification.
…ts + full regression set my stale check misread the bedrock rows: the anthropic section of the generated metadata still pins sonnet-4-6 at 200k with a catalog test blessing it; file map now includes the generated rows, codex-catalog test, registry-parity assertions, and #854's complete test set (modified 372k test + five additions incl incomplete-metadata branch).
…tative [1m] in generated profiles Two halves, consolidated from PRs #839 and #854: 1. ANTHROPIC_MODEL_CONTEXT_WINDOWS omitted claude-opus-4-7, claude-opus-4-6, and claude-sonnet-4-6 although all three ship in ANTHROPIC_MODELS — they advertised max_input_tokens null, emitted no [1m] picker row, and Claude Code accounted them at its 200k default. All three are documented at 1M by Anthropic (Opus 4.6 2026-02-05, Opus 4.7 2026-04-16, Sonnet 4.6 2026-02-17). The generated jawcode metadata and its 200k-pinned catalog test move with the registry; the stale CONTEXT_WINDOW_OVERRIDES generator pin is removed. 2. Generated subagent defs marked [1m] with the MAIN-SESSION auto-context predicate, so a 372K route was written [1m] into generated profiles — accounted at 1M with no compaction pairing in the subagent. Generated defs now mark only authoritative >=1M windows, strip inherited unsafe markers to bare, preserve genuine routed [1m] ids (kimi/k3[1m]) and provider caps, and keep selectors with unknown windows as-was. Main-session env-slot marking is unchanged. Tests: 372k roster test corrected + five new regressions (catalog-derived 1M markers, routed [1m] preservation, 350K cap unmarking, marker-case precedence, incomplete-metadata preservation), catalog 1M contract, registry parity assertions for all three windows.
consumeConnectFrames now returns payload VIEWS into the backlog (safe: append-only at end, compaction/growth replace the buffer), so the charge transfers to the frame lifecycle instead of reserving a second copy — an exact 16 MiB effective payload peaks at 16 MiB + 5 and completes, matching the documented boundary. One idempotent releaseBacklogLease owns the raw remainder on every terminal path (failAndClear, EOF classifications, first-frame timeout); the data-handler catch no longer releases the chunk charge after a successful append, which understated the retained backlog. Tests: exact-boundary live probe, lease-to-zero after frame_incomplete, 1/3/7/64 KiB chunk sweep (RED on both counts before this repair).
When reserveTransportBytes rejected an over-cap chunk, the data-handler catch released the chunk size anyway, debiting unrelated live ownership (clamps masked the imbalance). The catch now releases only when the reservation succeeded but the append never ran; after an append the terminal backlog cleanup owns the bytes. Regression: two transports on one shared budget — A parks an incomplete frame, B's own incomplete frame overflows; A's lease is byte-exact throughout and the budget returns to zero.
…CTOU, canonical depth, frame pinning, abandoned budgets, docs) - prompt-journal: the journal temp hardens required (fail closed like the token/tray writers — it carries full config.toml bytes); durableDelete propagates non-ENOENT failures; recovery revalidates each target immediately before its own restore and maps delete failures to recovery_required instead of lying committed/rolled-back. - antigravity canonicalization caps recursion depth at 128 (byte/keys budgets did not stop a nesting bomb). - Cursor framing: payloads <= 64 KiB are copied, not viewed, so small frames never pin the 32 MiB backlog buffer in the work queue. - bridge: an abandoned stream (never read, never cancelled) gets an unref'd watchdog that disposes its owned default budget instead of holding it in liveBudgets for the process lifetime. - README: weekly re-arm bound documented on the star surface too. - chore: untrack go/internal/cli/config_parity.go again (re-added by a broad add); move the completed wt2 unit to devlog/_fin.
Owner
Author
|
CodeRabbit round folded (a86ee03):
Notes on the remaining comments: the devlog pre-disclosure flags (wt1/wt4 execution notes) pass the repo's own test — every defect they describe already has a public diff (the original PRs and this one), so they disclose nothing new and stay as implementation records. The other devlog plan-doc nits belong to sibling workstreams outside this PR's scope. The abandoned-budget alternative you suggested (defer to first |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
devlog/_fin/260802_wt2_zero_leak_bounds/050_fix_antigravity_key_identities.md (1)
8-10: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse UTF-16 code units in the replay-key contract.
Line 8 says the keys use length-prefixed UTF-8 components, but Line 35 and the implemented contract use length-prefixed UTF-16 code units. These encodings differ for lone surrogates. Keep the documentation aligned with the implementation to avoid reintroducing cross-session key collisions.
Proposed documentation fix
- - `replayKey` (:57): SHA-256 hex over LENGTH-PREFIXED UTF-8 components + - `replayKey` (:57): SHA-256 hex over LENGTH-PREFIXED UTF-16 code-unit components🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260802_wt2_zero_leak_bounds/050_fix_antigravity_key_identities.md` around lines 8 - 10, Update the replayKey and functionCallKey documentation to describe length-prefixed UTF-16 code-unit components instead of UTF-8, matching the implemented contract and existing documentation. Preserve the stated fixed-length hashing and canonicalization behavior.devlog/_fin/260802_wt2_zero_leak_bounds/045_fix_blob_id_keys.md (1)
11-14: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRecord the implemented blob-ID key contract.
Line 12 still leaves a reject-or-digest decision and typed errors unresolved. The supplied
070_campaign_close.mdrecord states that IDs up to 64 bytes useh:plus hex, while larger IDs used:plus SHA-256 hex. Update this final record to describe that contract and its shared store/lookup derivation.Proposed documentation fix
- - Admission boundary ... either (a) rejected typed (`blob_id_invalid`/`blob_id_too_large`) or (b) stored under a fixed-size derived key `sha256(id)` ... + - Admission boundary ... IDs up to 64 bytes use `h:` plus hex; larger IDs use `d:` plus SHA-256 hex. Apply the same derivation to storage and lookup ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@devlog/_fin/260802_wt2_zero_leak_bounds/045_fix_blob_id_keys.md` around lines 11 - 14, Update the final record to document the established blob-ID key contract: IDs up to 64 bytes use the h: prefix plus hex encoding, while larger IDs use the d: prefix plus SHA-256 hex. State that store admission and all lookup paths, including getBlobArgs, hydration, and scope pins, use this same derivation, with derived fixed-size key bytes included in byte accounting.src/codex/prompt-journal.ts (1)
220-306: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
recoverIfNeededstill restores/deletes whatever absolute paths the journal names, with no binding to caller-expected targets.Line 220 keeps the signature
recoverIfNeeded(journalPath: string): RecoveryOutcome. Lines 233-234 readrecord.configPathandrecord.storePathstraight from the decoded journal, and lines 269 and 287 pass them directly torestore(), which callsdurableWriteordurableDeleteon those paths. The checksum validated bydecodeJournalis an unkeyed SHA-256 over the journal body: it proves internal consistency, not that this process wrote the journal or that the named paths belong to this store.The revalidation added at lines 260-267 and 278-285 (re-classifying the target immediately before
restore) is a solid TOCTOU narrowing for content drift, but it does not address path authenticity: a crafted or corrupted journal naming an out-of-treeconfigPath/storePath, with apostConfig/postStorehash matching that file's current bytes, still causesrestore()to overwrite or delete an arbitrary file at the service's privilege level. This is the same gap raised in a prior review round and remains unresolved in the shown code.Bind recovery to caller-supplied targets: pass
{ configPath, storePath }from the caller (per the prior review,commitinsrc/codex/prompt-layers.tsalready computes both before invoking recovery) and reject the journal outright when its recorded paths differ from the expected ones, before anyclassify,restore, ordurableDeletecall.🛡️ Proposed fix: bind recovery to caller-supplied targets
-export function recoverIfNeeded(journalPath: string): RecoveryOutcome { +export function recoverIfNeeded( + journalPath: string, + expected: { configPath: string; storePath: string }, +): RecoveryOutcome { const raw = readOrNull(journalPath); if (raw === null) return { ok: true, action: "none" }; const record = decodeJournal(raw); if (record === null) { return { ok: false, error: "recovery_required", detail: `journal at ${journalPath} failed its checksum or is truncated; nothing was read from it`, }; } + // The envelope checksum is not a MAC: it proves internal consistency, not + // authorship. Never write or delete a path the caller did not name. + if (record.configPath !== expected.configPath || record.storePath !== expected.storePath) { + return { + ok: false, + error: "recovery_required", + detail: `journal at ${journalPath} names targets this store does not own`, + }; + } + const config = classify(readOrNull(record.configPath), record.preConfig, record.postConfig);Update the call site in
src/codex/prompt-layers.tsto pass{ configPath, storePath }intorecoverJournal/recoverIfNeeded.As per path instructions for
src/**: "Watch for credential handling: tokens and OAuth material must never be logged or serialized into responses."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex/prompt-journal.ts` around lines 220 - 306, Update recoverIfNeeded to accept caller-supplied expected configPath and storePath targets, and update the prompt-layers commit call site to pass { configPath, storePath }. Immediately after decoding the journal and before any classify, restore, or durableDelete operation, reject recovery when either recorded path differs from its expected target, returning recovery_required without touching either path.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@devlog/_fin/260802_wt2_zero_leak_bounds/040_fix_cursor_incremental_frames.md`:
- Line 19: Update the framing record around consumeConnectFrames to match the
implementation: document the returned consumedBytes field instead of nextOffset,
and describe the 64 KiB payload copy threshold with subarray views for larger
payloads rather than claiming all frames are slice copies.
In `@devlog/_fin/260802_wt2_zero_leak_bounds/070_campaign_close.md`:
- Line 32: Reconcile the test count in the final validation entry: update the
recorded total to 7,261 to match the supplied PR validation, or explicitly label
7,162 as the historical worktree result if that count must be retained. Keep
only one authoritative current count.
In `@src/bridge.ts`:
- Around line 227-237: The owned-budget watchdog in bridgeToResponsesSSE must
track stream inactivity rather than elapsed stream lifetime, so active streams
cannot be disposed while step() is processing events. Consolidate stream-death
cleanup to clear ownedWatchdog and the heartbeat interval, cancel upstream work,
and dispose the budget on terminal, cancellation, and enqueue-failure paths
including the handlers near lines 318 and 1230. Add a focused regression test
covering an active stream that remains alive beyond the configured watchdog
delay.
In `@tests/bridge.test.ts`:
- Around line 1099-1110: Strengthen the watchdog test around
bridgeToResponsesSSE by asserting translatorLiveBudgetCountForTests() equals
before + 1 immediately after creating the stream, before waiting for
abandonment. Keep the existing post-watchdog assertion that the count returns to
before, and preserve the current cleanup behavior.
---
Duplicate comments:
In `@devlog/_fin/260802_wt2_zero_leak_bounds/045_fix_blob_id_keys.md`:
- Around line 11-14: Update the final record to document the established blob-ID
key contract: IDs up to 64 bytes use the h: prefix plus hex encoding, while
larger IDs use the d: prefix plus SHA-256 hex. State that store admission and
all lookup paths, including getBlobArgs, hydration, and scope pins, use this
same derivation, with derived fixed-size key bytes included in byte accounting.
In
`@devlog/_fin/260802_wt2_zero_leak_bounds/050_fix_antigravity_key_identities.md`:
- Around line 8-10: Update the replayKey and functionCallKey documentation to
describe length-prefixed UTF-16 code-unit components instead of UTF-8, matching
the implemented contract and existing documentation. Preserve the stated
fixed-length hashing and canonicalization behavior.
In `@src/codex/prompt-journal.ts`:
- Around line 220-306: Update recoverIfNeeded to accept caller-supplied expected
configPath and storePath targets, and update the prompt-layers commit call site
to pass { configPath, storePath }. Immediately after decoding the journal and
before any classify, restore, or durableDelete operation, reject recovery when
either recorded path differs from its expected target, returning
recovery_required without touching either path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e377b6d5-1509-4bf6-9e4c-8b798185f6b3
📒 Files selected for processing (16)
README.mddevlog/_fin/260802_wt2_zero_leak_bounds/000_plan.mddevlog/_fin/260802_wt2_zero_leak_bounds/001_root_cause_delta.mddevlog/_fin/260802_wt2_zero_leak_bounds/010_implementation.mddevlog/_fin/260802_wt2_zero_leak_bounds/020_fix_responses_state_admission.mddevlog/_fin/260802_wt2_zero_leak_bounds/030_fix_tool_arg_collector_scope.mddevlog/_fin/260802_wt2_zero_leak_bounds/040_fix_cursor_incremental_frames.mddevlog/_fin/260802_wt2_zero_leak_bounds/045_fix_blob_id_keys.mddevlog/_fin/260802_wt2_zero_leak_bounds/050_fix_antigravity_key_identities.mddevlog/_fin/260802_wt2_zero_leak_bounds/060_fix_acl_memo_release.mddevlog/_fin/260802_wt2_zero_leak_bounds/070_campaign_close.mdsrc/adapters/cursor/framing.tssrc/adapters/google-antigravity-replay.tssrc/bridge.tssrc/codex/prompt-journal.tstests/bridge.test.ts
|
|
||
| - `bun x tsc --noEmit`: PASS (repeated across all cycles). | ||
| - `bun run privacy:scan`: PASS (wp1; re-verified wp7 by reviewer). | ||
| - Full `bun run test`: **PASS** — 7162 pass, 8 skip, 0 fail, 34,386 expect() calls across 487 files (256.21s), recorded 2026-08-02 on this branch. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the final test count.
Line 32 records 7,162 passing tests, while the supplied PR validation reports 7,261 passing tests. Keep one authoritative count. If 7,162 is the historical worktree result, label it as historical; otherwise update the line to 7,261.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_fin/260802_wt2_zero_leak_bounds/070_campaign_close.md` at line 32,
Reconcile the test count in the final validation entry: update the recorded
total to 7,261 to match the supplied PR validation, or explicitly label 7,162 as
the historical worktree result if that count must be retained. Keep only one
authoritative current count.
Comment on lines
+1099
to
+1110
| test("an abandoned stream's owned budget is disposed by the watchdog", async () => { | ||
| resetTranslatorAggregateForTests(); | ||
| setOwnedBudgetAbandonedMsForTests(10); | ||
| try { | ||
| const before = translatorLiveBudgetCountForTests(); | ||
| bridgeToResponsesSSE(replay([{ type: "text_delta", text: "never read" }]), "mock/test-model"); | ||
| await new Promise(resolve => setTimeout(resolve, 40)); | ||
| expect(translatorLiveBudgetCountForTests()).toBe(before); | ||
| } finally { | ||
| setOwnedBudgetAbandonedMsForTests(null); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The watchdog test does not prove a budget was ever created before checking it is disposed.
The test at line 1103 captures before = translatorLiveBudgetCountForTests(), then at line 1104 calls bridgeToResponsesSSE(...) without options, so ownsBudget is true and createTranslatorBudget() runs synchronously inside bridgeToResponsesSSE (per src/bridge.ts, the budget assignment happens at function-body scope, before any stream is read). After the 40 ms wait, line 1106 asserts translatorLiveBudgetCountForTests() equals before.
The gap: this assertion also passes if the owned budget is never created at all, for example a future regression in the ownsBudget = !options?.translatorBudget computation, or in the watchdog wiring itself. Since before and the post-wait count would then trivially match, this specific regression — the exact one the watchdog feature and this PR round target — would slip through undetected.
Add an intermediate assertion right after creating the stream, confirming the count rose to before + 1, before waiting for the watchdog to bring it back down.
🐛 Proposed strengthening of the regression assertion
const before = translatorLiveBudgetCountForTests();
bridgeToResponsesSSE(replay([{ type: "text_delta", text: "never read" }]), "mock/test-model");
+ // Prove the owned budget was actually created before checking it is disposed;
+ // otherwise a regression that skips creation would pass this test vacuously.
+ expect(translatorLiveBudgetCountForTests()).toBe(before + 1);
await new Promise(resolve => setTimeout(resolve, 40));
expect(translatorLiveBudgetCountForTests()).toBe(before);Separately, the 10 ms abandonment timeout (line 1101) paired with a 40 ms real-timer wait (line 1105) gives only a 4x margin; on a loaded CI runner this class of real-timer test can flake. Not a blocker given the existing margin, but worth keeping in mind if this test becomes flaky.
As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("an abandoned stream's owned budget is disposed by the watchdog", async () => { | |
| resetTranslatorAggregateForTests(); | |
| setOwnedBudgetAbandonedMsForTests(10); | |
| try { | |
| const before = translatorLiveBudgetCountForTests(); | |
| bridgeToResponsesSSE(replay([{ type: "text_delta", text: "never read" }]), "mock/test-model"); | |
| await new Promise(resolve => setTimeout(resolve, 40)); | |
| expect(translatorLiveBudgetCountForTests()).toBe(before); | |
| } finally { | |
| setOwnedBudgetAbandonedMsForTests(null); | |
| } | |
| }); | |
| test("an abandoned stream's owned budget is disposed by the watchdog", async () => { | |
| resetTranslatorAggregateForTests(); | |
| setOwnedBudgetAbandonedMsForTests(10); | |
| try { | |
| const before = translatorLiveBudgetCountForTests(); | |
| bridgeToResponsesSSE(replay([{ type: "text_delta", text: "never read" }]), "mock/test-model"); | |
| // Prove the owned budget was actually created before checking it is disposed; | |
| // otherwise a regression that skips creation would pass this test vacuously. | |
| expect(translatorLiveBudgetCountForTests()).toBe(before + 1); | |
| await new Promise(resolve => setTimeout(resolve, 40)); | |
| expect(translatorLiveBudgetCountForTests()).toBe(before); | |
| } finally { | |
| setOwnedBudgetAbandonedMsForTests(null); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/bridge.test.ts` around lines 1099 - 1110, Strengthen the watchdog test
around bridgeToResponsesSSE by asserting translatorLiveBudgetCountForTests()
equals before + 1 immediately after creating the stream, before waiting for
abandonment. Keep the existing post-watchdog assertion that the count returns to
before, and preserve the current cleanup behavior.
Source: Path instructions
…en dist is absent The integrations CSRF test fetches the real served page, which only exists when gui/dist is built. CI runs bun test BEFORE GUI build, so the page fell back to the JSON root payload and the meta assertion failed on every fresh checkout (ubuntu + macos on the merge ref). Build once in-file when the artifact is missing.
boundedInjectedInstruction sliced UTF-16 code units at the character budget, which could leave a lone high surrogate at the cut; encoding that substitutes U+FFFD into the injected instruction. The slice now drops the dangling half pair instead. Regression pins the astral char at the exact budget boundary. Found while hunting #904 (Korean U+FFFD reports) — kiro-specific, not the reporter's kimi/opus path.
Three more lone-surrogate U+FFFD producers found by the #904 coverage audit: the v1 compaction retained tail could begin on a lone low surrogate when the budget cut landed mid-pair; the kiro reasoning carry could emit a delta ending on a lone high surrogate; and kiro tool-description truncation could end on one. Each cut now shifts by one code unit to keep pairs whole, with boundary-pinned regressions (the 80,001-code-unit astral repro for compaction included).
…d-scan repair, strict EOF predicates) Review findings on the stacked contributor fixes, all red-green verified: - core.ts upstream JSON branch read the whole body with an unbounded .text(); every non-streaming upstream — now including WebSocket turns deliberately answered with bounded JSON — could grow proxy memory without limit. The read goes through relay.readBoundedResponseText (32 MiB ceiling, body cancelled on overflow) and fails closed with a 502 instead of parsing a partial body. - anthropic lastValidJsonObject collected every brace offset into two arrays before its candidate cap, so brace-dense hostile input cost O(n) index storage. It now scans backwards from the end with lastIndexOf and never materializes an index; inputs above 1 MiB are not repaired at all. - an empty text_delta marked sawVisibleText, letting a cut-off tolerant stream complete as a successful empty answer; only non-empty text authorizes tolerant completion now. - a translator-budget overflow could be followed by tool_call_end/done when the generator was fully drained; the budget error now returns immediately as the single terminal event.
… UTF-8 cap) Re-audit findings on 9794e24: - readBoundedResponseText awaited reader.cancel(), so a broken stream whose cancellation never settles would hang the overflow path instead of returning the documented 502. The custom helper is replaced by the house primitive readBoundedResponseBody, which cancels fire-and-forget with synchronous-throw protection and adds total (180s) and inactivity (30s) transfer deadlines on top of the byte ceiling; oversize and stalls both fail closed, and a partial body is never parsed. - bounded-body.ts gains a maxBytes option (default unchanged at 64 KiB) and accumulates into a geometrically growing single buffer, so per-chunk metadata cannot amplify beyond the payload budget on large ceilings. - the repair byte cap measured UTF-16 code units; astral text could enter the parse attempts at 4x the intended bytes. utf8BytesExceed measures the exact UTF-8 length with early exit and no allocation; regression test covers a 600k-code-unit, 1.2 MB input that a length check would have admitted.
…in the repair cap The 4-byte branch of utf8BytesExceed skipped the next code unit whenever it existed, without checking it is a low surrogate — so a run of high surrogates was counted at 2 bytes per unit while TextEncoder emits 3. The pair path now requires the next unit in 0xDC00..0xDFFF; anything else counts 3 bytes without skipping. Regression: 200k high-surrogate pairs (400k code units, 1.2 MB on the wire) decline repair instead of being admitted at 800k counted bytes.
This was referenced Aug 2, 2026
Each GET /api/system/memory read costs ~600 ms on the shared ubuntu runner (the route samples the live process), and the test performs eight reads — 5.2 s against bun's 5 s default timeout, which is how it flaked red on the wp6 head while passing locally and on macos. Bump the per-test timeout to 20 s; the assertions and read count are unchanged.
…weep test The symlinked-directory sweep test drives the real load path, whose stale-temp sweep checks liveness with kill(pid, 0). The hardcoded dead pid (4242/4243) collided with a live process on the macos CI runner, so the temp survived and the assertion flaked red while the same commit passed locally and on ubuntu. Probe upward for a pid that returns ESRCH instead; the injected-liveness tests are untouched.
…nners Both execute real SQLite writes and filesystem moves, the same intrinsic-cost class as the five neighbors that already carry STORE_BUDGET_MS — they simply never got the budget, and flaked at ~5.6s against bun's 5s default on the windows-latest runner. Ablation check: both assert real quarantine/permanent behavior (locally green in <0.5s), so the budget is headroom, not cover.
…ion test The live PowerShell enumeration test already tolerated one transient empty result, but on a sufficiently contended windows-latest runner execFileSync's 8s production deadline fires instead and the error propagates by design — that throw was not tolerated, flaking the job. Catch it the same way and add a third attempt with a longer settle; production behavior is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Unified bug-fix campaign: the wt2 retained-state hardening plus all four planned workstreams (wt1 update path, wt4 server/config, wt5 Windows service, wt3 provider wire), reviewed per workstream by independent adversarial reviewers and verified end-to-end. Supersedes #840 #841 #843 #844 #845 #847 (closed earlier) and #871 #886 #887 #889 (closed as each workstream landed here).
wt2 — retained-state bounds (tracker #820 wave 2)
upstream_erroreverywhere (was 413 ×4); no unbounded no-budget path; cancel-race fixes.frame_incomplete; terminal backlog lease owner.h:/d:), key bytes counted + classified; zero-payload blobs stay evictable.wt1 — update path
major.minor.patchcore on the latest channel (registry target side stays strict).wt4 — server/config
wt5 — Windows service
wt3 — provider wire
modelAdapterspreserved.service_tiertri-state capability gate (truepermits injection,falsestrips,undefinedpreserves caller values; docs aligned in EN + ja/ko/ru/zh-cn) and reasoning_content replay for tool-call continuations.[1m]picker rows only for authoritative ≥1M windows (no 372K route marked).wp6 — contributor bug-fix stack (5 PRs stacked, 2 triaged closed)
grok-4.5-fast: sent as canonicalgrok-4.5+requested_model.parameters(effort,fast=true); legacymodel_detailsomitted only for parameterized external selections; flattened ids remain live-discovery evidence only.x-codex-turn-metadatarequest_kind: "turn"preserves foreground Luna turns; headerless/malformed/future-unknown metadata keeps legacy prefix interception.stream: false), reframed by the WS bridge; HTTP/SSE stays streaming. Folded review: the upstream body read is now bounded (32 MiB + 180s/30s transfer deadlines via the house bounded-body primitive, fail-closed 502).anthropicEofTolerancecapability (default off); EOF completes only with non-empty visible text or complete tool-call JSON; backward-scan repair with an exact UTF-8 byte cap (surrogate-safe); budget overflow is a single terminal event.identifyRoutedModelnames the concrete wire model id under a strict ASCII allowlist (≤128 chars,._/@:+-[]~) with safe fallback, across catalog sync + anthropic/google/kiro/openai-chat.maxBytesoption and geometric buffer accumulation.Validation
bun run test: 7,452 pass, 8 skip, 0 fail locally (full suite on daf7069, 499 files); every workstream's focused suites re-run after each stackbun x tsc --noEmit: passbun run privacy:scan: passNotes
Summary by CodeRabbit
New Features
ocx configcommands for viewing, editing, validating, importing, and exporting configuration.Bug Fixes