fix: bound the six remaining retained-state leaks (#820 wave 2) - #892
Merged
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.
…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 was referenced Aug 2, 2026
Closed
lidge-jun
added a commit
that referenced
this pull request
Aug 3, 2026
… mechanically go/internal/cli/config_parity.go was committed by a broad `git add` three times during the #820 campaign. Two of those were caught and reverted (2101d50, 58e8718); the third rode a86ee03 into the #892 merge, so 682 lines of the retired Go experiment are now tracked on dev — the one and only tracked file under go/, absent from main, preview, and v2.10.0. Nothing in src/, the build, the typecheck, or the test path reads from go/, so this is dead weight in every clone rather than a functional regression. The commit that introduced it says "untrack ... again" in its own message, which is how it passed review: the intent was right and the index was not. .gitignore alone cannot prevent the repeat, because an already-tracked path stops honoring the ignore rule — so the guard lives in tests/repo-hygiene.test.ts next to the .codexclaw/gitlink invariants, driven red once by re-adding the file.
lidge-jun
added a commit
that referenced
this pull request
Aug 3, 2026
fix(security): close the agent star-consent bypass found auditing the #892 merge
2 tasks
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