Zero-leak state stores: hard caps, durable spill, and generation-fenced sweeping - #829
Conversation
…ores Exhaustive 36-store inventory (continuation, blobs, rings, caches, registries, translator-layer accumulators) with per-store bounds, eviction contracts, worst-case math, and blast-radius notes; audited implementation roadmap across five review rounds (ownership matrix, spill-failure tombstone ladder, blob provenance admission, three-way eviction mechanism lock, ACL trust-boundary contract); decade docs 010-070 written to diff level, one PABCD cycle each. The design goal: the strongest theoretical no-leak posture among production TS LLM proxies — held as a hypothesis until the 060 benchmark proves it with a source ledger.
…eview rounds Generation-unique spill basenames (content digest + monotonic counter), atomic no-replace publication (link/exclusive-copy, EEXIST retry — plain rename replaces and is unsuitable), and exactly-once supersededSpill ownership across stub-to-resident and resident-to-resident replacements. The 005 locked decision and phase map now match 010 end to end.
A-gate rounds (Banach): trusted-ID spill reads, digest+generation basenames, caller-driven 400 recovery, failure-safe UTF-8 measurement with reject+tombstone, cap-override test triggers, consumer ledger, and the final residual — the replacement transaction is now a single old-stub→new-stub swap with the old file unlinked last.
The continuation store kept its newest entry exempt from the 64 MiB cap — one oversized response (images, giant tool results) could pin hundreds of MiB for an hour. The cap is now unconditional: oversized entries demote to a disk stub only after an atomic no-replace publication (write-fsync-link, EEXIST advances a generation counter), replay reads through the stub with digest verification, and a spill failure leaves a tombstone that fails continuation explicitly instead of silently dropping context. Startup GC reclaims orphaned generations; the legacy debounced snapshot still serves small entries. /api/system/memory responseState now reports resident/stub/tombstone counts, spill payload bytes, and write/read failure counters. Plan: devlog/_plan/260801_zero_leak_state_stores/010 (8 audit rounds).
wp3 A-gate: request-scope pins own in-flight blob liveness (provenance only orders post-request eviction), the admission ladder precomputes victims against both limits and commits once, the SetBlobResult typed error replaces a false schema premise, all four stores get exact 040 snapshot/eviction exports, and the unreachable vision oversized test becomes a configurable-limit seam.
… rounds Cross-provenance refresh merges to the stronger class, expired unpinned remote rows join the evictable set, oldestAt names the exact next eviction victim, and the vision clamp now covers the first-use outcome.
Cursor blobs gain per-blob 16 MiB / aggregate 64 MiB caps with two-phase atomic admission, request-scope pins that keep a blob alive from construction through hydration, provenance-aware eviction (remote rows stay pinned within TTL; refresh merges to the stronger class), and a typed capacity error before an unstored hash can reach the wire. Antigravity replay sessions cap inner calls at 256/2 MiB with 64 KiB signatures; vision descriptions clamp before insertion (first use and cache hit byte-identical) under a byte-weighted LRU; image-normalize pass/miss sentinels get key-byte accounting and a count cap. Every store exports snapshot/eviction hooks for the 040 budget. Plan: devlog/_plan/260801_zero_leak_state_stores/020 (5 audit rounds).
…arget wp4 A-gate (Faraday): GenerationContext gains canonical comboTargets so partial-target removal is prunable, every affected store write path carries a generation fence so late completions cannot resurrect deleted keys, and the PID memo moves to liveness-proof cleanup with a bounded probe budget.
…bject triggers Six audit rounds on the eviction design: candidate generations come from a never-reused attempt counter so a failed attempt cannot strand an owner on an obsolete live set; triggers are the three real lifecycle shapes (startup capture, in-place management commits, OAuth live adoption) and saveConfig alone never reconciles; ACL memo coverage now spans all five hardened-temp producers.
One sweeper now owns eviction for the expired-key and dead-key store classes: TTL sweeps only where getters already treat expired rows as absent; config-generation reconciliation (unique attempt counters, wholesale owner replacement, deletion-only callbacks) drops keys unreachable from the live server config; late-completion writer fences stop an old flight from resurrecting a deleted key; PID rows are liveness-proven, never generation-swept. Windows ACL success memos are forgotten only after their hardened temp is confirmed gone, across all five producers. Security review: ACL changes affect success-memo lifecycle only; required ACL failures remain fail-closed and destination-keyed timeout memoization is unchanged. Plan: devlog/_plan/260801_zero_leak_state_stores/030 (6 audit rounds).
The per-combo topology-generation map duplicated what the global reconciled-generation plus the live-target set already guarantee, so the commit fence keeps the same admissions with one less keyed store. The tier-backup scrub now forgets the hardened secret path in exactly one place, after removal is proven, instead of four racy call sites.
The audit synthesis described the reauth fence and dead-PID probe as they stood before repair; the table now states the per-write generation fence and the serialized non-positive-PID discard that actually shipped. The rotation test also learns that a resolved auth context carries its writer generation.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change set bounds continuation state, caches, configuration-owned stores, diagnostics, active resources, ACL memo state, translator/stream buffers, background-shell lifecycle, and usage-log reads. It adds durable response spilling, generation-aware reconciliation, admission leases, process-wide memory accounting, GitHub-star consent gating, truncation metadata, and regression coverage. ChangesZero-leak state-store hardening
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Freshness audit against the rebased tree moved four caps to their real owners: the turn lease now lives at the fetch boundary with boundary-owned release, the WebSocket reservation precedes the upgrade, the pool-quota cap counts total flight objects across the per-account sets, and the dead 16-live-worker cap gave way to bounding the spawn-closure queue itself. Each quota-probe caller gets a defined busy behavior instead of an imagined shared 409 surface.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ed4ca0ae2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | OAuth mutation tail (`oauth/store.ts:292-305`) | One promise points to an uncapped FIFO of load-modify-persist closures. No timeout or queue-depth admission; settled work collapses. Worst is every pending mutation closure and captured credential/config values. | **CONDITIONALLY-UNBOUNDED while disk mutation stalls** | Removing serialization reintroduces lost updates between refresh and account switching. Add admission/timeout without reordering accepted writes. | | ||
| | Grok apply tail (`server/management/agent-settings-routes.ts:62-70,545`) | One promise points to an uncapped FIFO of management apply closures; settled work collapses. Worst pending applies times captured apply arguments/config references. | **CONDITIONALLY-UNBOUNDED while apply stalls** | Concurrent read-modify-write cycles can overwrite each other. Coalesce superseded settings or return busy, preserving accepted order. | | ||
| | Codex credential refresh (`codex/account-store.ts:20-22,268-270,349-360,383-458`) | Active promise per distinct refresh-grant fingerprint; same grant deduplicates. Rows delete in `finally`; fetch is 30 s, file-lock wait is 65 s, but distinct admissions have no cap. Worst concurrent expired grants times credential/fetch state. | **CONDITIONALLY-UNBOUNDED by active account concurrency** | Evicting a live flight duplicates token rotation and can cause generation conflicts. Bound admission; never detach an accepted grant owner. | | ||
| | MiMo bootstrap (`adapters/mimo-free.ts:27-35,47-74,92-133`) | One JWT, expiry number, one in-flight bootstrap, and one 36-char client id. Bootstrap has a 15 s timeout and flight clears in `finally`; JWT refresh is expiry-triggered. Count is fixed, but upstream `data.jwt` has no response/value byte cap. | **CONDITIONALLY-UNBOUNDED value bytes** | Keep same-flight bootstrap, expiry skew, and stable anonymous client id; reject an oversized bootstrap response/JWT before caching. | | ||
| | Management usage read (`usage/log.ts:389-403,470-511`) | One active revision-keyed promise; same revision deduplicates and it clears in `finally`. The promise retains the fully parsed append-only usage log and byte/text intermediates until completion; no file-size cap. | **CONDITIONALLY-UNBOUNDED active value bytes** | Dedup prevents duplicate full parses. Bound/read incrementally or aggregate, but do not cache parsed rows after completion. | |
There was a problem hiding this comment.
This inventory publishes concrete, still-unfixed resource-exhaustion paths—including uncapped OAuth/Grok queues, credential-refresh flights, oversized JWTs, and unbounded log parsing—with precise source locations and remedies; 005_impl_roadmap.md lines 44-47 confirms that these fixes remain assigned to later work phases. Move this pre-fix triage to ignored scratch space and commit only the shipped fixes or a public advisory, because tracking it under _plan discloses the weaknesses immediately.
AGENTS.md reference: AGENTS.md:L61-L69
Useful? React with 👍 / 👎.
| await atomicWriteFileAsync(path, JSON.stringify({ version: 2, states: entries })); | ||
| if (revision === stateRevision) break; |
There was a problem hiding this comment.
On a busy proxy or slow filesystem, response completions can increment stateRevision during every awaited atomic write, so this loop rewrites a potentially 24 MiB snapshot indefinitely until it observes a completely quiet write window. The persistence gate then never releases, flushResponseState() can block graceful shutdown, and the process continuously consumes disk I/O; cap the attempts or persist one captured revision and schedule a later flush while preserving spill-unlink safety.
Useful? React with 👍 / 👎.
| export function encodeCursorRunRequest(request: CursorRunRequest): Uint8Array { | ||
| return prepareCursorRunRequest(request).bytes; |
There was a problem hiding this comment.
When a caller uses this exported compatibility wrapper instead of LiveCursorTransport, prepareCursorRunRequest() seals a scope containing every generated blob, but the wrapper discards the only token that can release those pins. These blobs remain permanently request-pinned and cannot be evicted by TTL or budget logic, so repeated encodes eventually exhaust the 64 MiB/4,096-entry store and make later encodes throw CursorBlobAdmissionError; release the scope before returning the bytes or otherwise preserve the old wrapper's evictable-blob behavior.
AGENTS.md reference: src/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| if (entry.kind !== "resident") { | ||
| deleteEntry(oldestId); | ||
| continue; |
There was a problem hiding this comment.
When the store exceeds the RAM cap and its oldest row is already a spill stub, this branch deletes that continuation even while later resident payloads are what consume nearly all the bytes. For example, with the 4,000-byte test cap, adding several roughly 1,500-byte chains spills the first chain, but the next insertion deletes that first stub before spilling the next resident; tests/responses-state.test.ts's “byte cap spills over-cap rows instead of evicting prior continuation ids” scenario therefore loses ids[0]. Search for an oldest resident to demote first, and delete stubs or tombstones only if bounded metadata alone still exceeds the cap.
Useful? React with 👍 / 👎.
| lastReconciledGeneration = generation; | ||
| return removedProviders.size; |
There was a problem hiding this comment.
When a provider is deleted while its asynchronous /models request is still awaiting the network, this reconciliation removes its four cache rows and advances the generation, but the late fetchProviderModels() completion still calls markProviderDiscoveryOk() and setCached() without checking that generation. The deleted provider therefore immediately reappears in the retained maps, and if a different provider is recreated under the same name before completion, its catalog can serve the old provider's models for the cache TTL; capture the generation at fetch admission and reject late writes unless that exact provider owner is still authoritative.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 37
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/server/responses/core.ts (1)
1490-1497: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueSimplify the redundant
writerGenerationnarrowing insrc/server/responses/compact.ts:263-272.usesCodexForwardPoolAuthis a type predicate that narrowsauthCtxto"pool"or"main-pool". PassauthCtx.writerGenerationdirectly. The guarded reads insrc/server/responses/core.ts:1490-1497and1588-1597are safe.🤖 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/server/responses/core.ts` around lines 1490 - 1497, In the Codex forward-pool outcome recording flow, remove the redundant writerGeneration narrowing and pass authCtx.writerGeneration directly to recordCodexUpstreamOutcome. Apply this in the corresponding compact.ts path guarded by usesCodexForwardPoolAuth, while preserving the existing guarded reads in core.ts.src/codex/auth-api.ts (1)
1211-1223: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCapture
writerGenerationbefore the async OAuth wait, for consistency with the rest of this file.
fetchMainAccountInfoAttempt(Line 352) andfetchFreshPoolAccountQuota(Line 515) both capturewriterGeneration = captureConfigGeneration()before starting async work, then pass it explicitly tomarkAccountNeedsReauth/setAccountQuotaFromParsedafter the fetch completes. This protects against a config change happening mid-flight.
setAccountQuotaFromParsed(accountId, quota)at Line 1317 does not follow this pattern. It sits inside a background IIFE that polls for up to 150 iterations at 2 seconds each (5 minutes) waiting for OAuth completion, and thequotavalue is derived from a WHAM fetch issued near Line 1213. NowriterGenerationis captured before that fetch. Relying on the default parameter meanscaptureConfigGeneration()is evaluated fresh at Line 1317, right before the write, so it always reflects the current generation rather than the generation active when the WHAM data was retrieved. Any generation check performed downstream insetAccountQuotaFromParsedbecomes a no-op for this call site.The
codexAccountPersistenceConflictre-check at Line 1290-1303 mitigates the immediate risk since noawaitseparates it from Line 1317, but capturing the generation earlier (e.g. right before the WHAM fetch at Line 1213) and passing it explicitly keeps this call site consistent with the defensive pattern used elsewhere in the file, and protects against future refactors that touch the ordering between the conflict check and the quota write.🛡️ Proposed fix
- let email = cred.email || accountId; + const writerGeneration = captureConfigGeneration(); + let email = cred.email || accountId; let plan: string | undefined; let quota: Omit<StoredAccountQuota, "updatedAt"> | null = null; try { @@ if (quota) { - setAccountQuotaFromParsed(accountId, quota); + setAccountQuotaFromParsed(accountId, quota, writerGeneration); }Also applies to: 1287-1335
🤖 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/auth-api.ts` around lines 1211 - 1223, Capture writerGeneration via captureConfigGeneration() before the WHAM fetch in the surrounding OAuth flow, then pass that captured value explicitly to setAccountQuotaFromParsed(accountId, quota) in the background IIFE. Preserve the existing polling and conflict re-check behavior while ensuring the quota write uses the generation active when the WHAM data was retrieved.src/vision/index.ts (1)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional
snapshot/evictOldestlet an injected cache silently disable byte budgeting.
snapshot?()andevictOldest?()are optional onVisionDescriptionCache. When a cache without them is installed throughsetVisionDescriptionCache,visionDescriptionRetainedStoreSnapshot()reports{ count: 0, bytes: 0, … }(line 127) andevictOldestVisionDescriptionForBudget()returns 0 (line 132). A byte-budget sweeper then reads "this store holds nothing" and can never reclaim from it, while the injected cache grows unbounded.
tests/vision-cache.test.tsline 273 installs exactly such a cache (get/set/clearonly), so the degraded shape is reachable today.Make both members required so every implementation must report and release bytes. A test double then needs three extra lines, and the observability contract becomes non-optional.
♻️ Proposed refactor
export interface VisionDescriptionCache { get(key: string): string | undefined; set(key: string, value: string): void; clear(): void; - snapshot?(): { count: number; bytes: number; oldestAt: number | null }; - evictOldest?(): number; + /** Retained-byte accounting is part of the contract: budget sweeps depend on it. */ + snapshot(): { count: number; bytes: number; oldestAt: number | null }; + /** Releases the least-recently-used entry and returns the bytes reclaimed. */ + evictOldest(): number; }Also applies to: 119-133
🤖 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/vision/index.ts` around lines 28 - 34, Make snapshot and evictOldest required members of the VisionDescriptionCache interface, then update all implementations and test doubles passed to setVisionDescriptionCache to provide both methods. Preserve visionDescriptionRetainedStoreSnapshot and evictOldestVisionDescriptionForBudget behavior while ensuring every injected cache reports usage and supports eviction.
🤖 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/_plan/260801_zero_leak_state_stores/010_continuation_hard_cap.md`:
- Around line 196-201: Update the synchronous durable spill path used by
rememberResponseState() to move serialization, file writes, and fsync work off
the request event loop through a bounded asynchronous or isolated I/O mechanism
with at most one in-flight operation. Add explicit admission and latency limits
as needed, while preserving durable-before-stub publication and preventing
unbounded pending-write chains or post-response stub races.
- Around line 314-320: Revise the pendingSpillUnlinks overflow behavior so
deferred spill files are never unlinked before the replacement snapshot is
durable. When the 128-entry cap is reached, synchronously flush the snapshot
before unlinking, or reject/preserve the replacement; remove the documented
“overflow unlinks oldest-first immediately” path while keeping persistNow()’s
post-success queue draining.
In `@devlog/_plan/260801_zero_leak_state_stores/030_eviction_mechanisms.md`:
- Around line 141-149: The documented failure semantics are not executable
because sweepExpiredOnWrite() only performs TTL eviction. Update the
reconciliation/sweeper design so a failed owner callback is retried exactly once
on the next write using a freshly built context from
setGenerationContextBuilder, or revise the write-trigger contract to invoke that
fresh reconciliation path. Add a regression covering one callback failure
followed by a next-write retry.
In `@devlog/_plan/260801_zero_leak_state_stores/035_registry_admission_caps.md`:
- Around line 172-192: Update the usage reader described by
MANAGEMENT_USAGE_MAX_READ_BYTES and ManagementUsageSnapshot to enforce a maximum
per-line size while consuming bounded chunks. Detect overlong lines
incrementally, discard their contents without retaining them, and continue
parsing subsequent lines; account for discarded bytes through
truncatedPrefixBytes or the existing historyTruncated signal so capped history
is reported accurately.
- Around line 120-134: Separate admission from resource registration by adding
tryAcquireCodexWebSocket() and tryAcquireStorageWorker() functions that return
leases before WebSocket upgrade or Worker creation. Update the lease API to
attach each created resource afterward, releasing the lease if attachment fails;
preserve tryRegisterCodexWebSocket and tryRegisterStorageWorker only for
post-creation tracking as needed. Ensure rejected requests never create
untracked sockets or workers.
In `@devlog/_plan/260801_zero_leak_state_stores/040_app_bytes_observability.md`:
- Around line 80-84: Align the translator observability contract across all
referenced sites: in
devlog/_plan/260801_zero_leak_state_stores/040_app_bytes_observability.md:80-84,
either extend ObservedBufferRegistration with the 050 metrics or constrain 050
to currentBytes, highWaterBytes, and active; in
devlog/_plan/260801_zero_leak_state_stores/050_translator_stream_bounds.md:70-73,
define the function that publishes the final high-water snapshot before
dispose(); and in 050_translator_stream_bounds.md:200-201, use the exact 040
schema and explicitly define whether peak and rejected are retained separately.
- Around line 109-111: Update snapshot collection and
enforceAppOwnedMemoryBudget so an owner whose snapshot throws is marked unknown
or retains its last known byte accounting instead of being reported as zero;
incomplete snapshots must not be treated as successful budget enforcement or
permit completion claims. Add and expose a scalar counter for snapshot failures
while preserving the existing evictOldest() behavior.
In `@devlog/_plan/260801_zero_leak_state_stores/050_translator_stream_bounds.md`:
- Around line 61-68: Make translatorBudget required in the production
IncomingMeta and ProviderAdapter.parseStream contracts, ensuring
request-admitted adapter calls always receive and forward a budget. Audit all
adapter call sites and update them to pass the admitted budget; if tests or
non-stream callers require omission, introduce separate non-production types
rather than weakening the production interfaces.
- Around line 183-188: Define Bun-compatible backpressure in the live transport
around the HTTP/2 stream data handling: do not rely on pause() or resume(), and
instead close/cancel the stream before queued protobuf bytes exceed the hard
cap. Emit the established coherent adapter error when rejecting additional data,
while preserving FIFO ordering for terminal and tool messages and tracking
queued byte/count limits.
In
`@devlog/_plan/260801_zero_leak_state_stores/055_background_shell_lifecycle.md`:
- Around line 108-117: The lifecycle sequence must retain ownership when the
child process has not emitted close. Update the termination logic around
entry.terminating and backgroundShells deletion so entries are removed only
after confirmed process exit; otherwise keep a bounded termination-pending entry
or place it in a bounded orphan registry with liveness checks, allowing later
cleanup retries while preserving the existing timeout and privacy-safe
accounting.
- Around line 141-147: Add and export an idempotent terminateAllBackgroundShells
function alongside terminateBackgroundShellsForSession, ensuring it terminates
and awaits every remaining background shell. Update the shared drainAndShutdown
lifecycle to await this global cleanup before final process termination, while
preserving existing transport-owned cleanup behavior.
- Around line 64-92: Replace the undefined BackgroundShellSpawnError and unnamed
typed errors with one exported background-shell error/response union, including
stable codes for capacity exhaustion, unknown shells, missing sessions, and
session-owner mismatches. Update CursorBackgroundShellBusyError and the
implementations of backgroundShellSpawnExec() and writeShellStdinExec() to use
this contract, then apply the same codes and shape in native-exec.ts,
live-transport.ts, and related tests.
In `@devlog/_plan/260801_zero_leak_state_stores/070_close_and_push.md`:
- Around line 17-20: Replace the hardcoded checkout path in the “Preflight:
freeze the checkout” procedure with a repository-root lookup using git rev-parse
--show-toplevel, or define and document a reusable REPO_ROOT variable before
subsequent commands.
In `@src/adapters/cursor/native-exec.ts`:
- Around line 146-184: Reduce blob mutation overhead by updating blobLocalBytes,
blobPinnedBytes, and blobEvictableBytes incrementally within setBlob and
deleteBlob, while retaining recomputeBlobClassAccounting only for expiry
transitions and test resets. Track the minimum unpinned remote expiration in
shared state and update the timer only when that minimum changes, avoiding
repeated full scans and clearTimeout/setTimeout churn across release paths.
- Around line 196-209: In src/adapters/cursor/native-exec.ts lines 196-209,
update releaseHydratedBlob to return immediately when requestScope is undefined
and only remove the named scope’s pin. In src/adapters/cursor/live-transport.ts
line 850, avoid forwarding an undefined blobRequestScope to handleCursorNativeKv
after the scope is released. Add a regression test near
tests/cursor-blob.test.ts lines 769-782 confirming unscoped hydration preserves
pins from two request scopes.
In `@src/adapters/google-antigravity-replay.ts`:
- Around line 189-194: Add a concise comment immediately before the
delete/re-insert sequence in the call-handling block around entry.byCall,
documenting that it moves the call to the newest Map position for
evictInnerCalls and intentionally bypasses deleteReplayCall because sizeBytes is
unchanged; warn that routing this through the helper would corrupt byte
accounting.
In `@src/config.ts`:
- Around line 2159-2182: Normalize maxProbes to a finite, non-negative integer
at the start of sweepDeadOcxStartProcessCache before any cursor or probe-count
calculations, ensuring NaN and invalid values result in zero probes without
storing NaN in ocxStartProcessSweepCursor. Add a regression test covering NaN
and confirming valid cached PIDs remain sweepable on later calls.
In `@src/lib/config-ownership.ts`:
- Around line 86-111: Remove the redundant hasLiveOwnedPath filesystem check
from reconcileConfigOwnershipRoots. After confirming a root is absent from
context.configRoots, delete it directly and preserve the existing removed count
and generation tracking behavior; leave listLiveConfigOwnershipRoots unchanged.
In `@src/lib/gcp-adc.ts`:
- Around line 71-94: Wire the existing sweepExpiredGcpAdcTokens handler into the
gcp-adc registration so shared sweepExpiredOnWrite dispatch invokes it. Keep
src/lib/gcp-adc.ts lines 71-94 unchanged; in
src/lib/state-store-registrations.ts lines 73-101, import
sweepExpiredGcpAdcTokens alongside reconcileGcpAdcTokens and add it as the
registration’s sweepExpired handler.
In `@src/lib/state-store-registrations.ts`:
- Around line 59-71: Register sweepExpiredGcpAdcTokens alongside
reconcileGcpAdcTokens in the "gcp-adc" state registration, and add the
corresponding import. Ensure the state sweeper invokes it so expired tokenCache
entries are removed during periodic sweeps.
In `@src/lib/state-store-sweeper.ts`:
- Around line 76-87: ||||Add an explicit module-level reconciling guard around
the pending-reconciliation retry in sweepExpiredOnWrite: only invoke
reconcileStateGeneration when no reconciliation is already active, set the guard
before entering the try block, and clear it in a finally block. Also reset
reconciling in resetStateStoreSweeperForTests so test state is isolated.
- Around line 111-128: Update the reconciliation flow around reconcileGeneration
and the failed branch so configGeneration advances to candidateGeneration
whenever the succeeded stores have already adopted that generation, while
reconciliationRetryPending remains true when any callback fails. Add a focused
regression test beside the existing partial-failure coverage in the sweeper
tests, verifying that a write for an owner added after a partially failed pass
is committed.
In `@src/oauth/index.ts`:
- Around line 843-848: Update the provider cleanup loop around loginState to
handle removed providers by aborting any in-flight login, then unconditionally
deleting the corresponding loginState, loginManual, and loginAbort entries and
counting successful removals. Remove the unreachable loginAbort delete guard,
and verify the login flow ordering before aborting so a legitimately
initializing provider is not incorrectly canceled; if that window exists, apply
the required state and age-bound check.
In `@src/oauth/store.ts`:
- Around line 415-424: Update listLiveOAuthAccountKeys to call the existing
oauthAccountKey(provider, account.id) helper when adding each composite key,
replacing the inline provider/account encoding. Keep the surrounding provider
filtering and account iteration unchanged; do not expand the refactor to
decoders or other consumers.
In `@src/oauth/token-guardian.ts`:
- Around line 240-258: Update reconcileGuardianBackoff so the valid codex keys
always include codex:${MAIN_CODEX_ACCOUNT_ID}, even when context.codexAccountIds
omits the sentinel. Preserve the existing pool-account reconciliation and
OAuth-key handling, ensuring the main-account entry remains in backoff and
liveBackoffKeys.
In `@src/providers/quota.ts`:
- Around line 437-458: Extract the repeated commit-and-sweep logic in the
surrounding quota probe into a local helper that accepts an
AccountQuotaCacheEntry, applies mayCommitAccountQuotaKey, updates
accountQuotaCache, and calls sweepExpiredOnWrite. Replace all three inline
blocks in the success, normal, and catch branches with this helper while
preserving each branch’s entry construction and return behavior.
- Around line 363-367: Replace the in-place report filtering in
reconcileLiveStateStores with clearProviderQuotaCache, preserving the cache
invalidationEpoch behavior and avoiding any mutation of cache.response or
cache.key. Extend the existing state-store sweeper regression test to restore
the identical provider configuration after reconciliation and verify
fetchProviderQuotaReports returns that provider rather than the previously
pruned response.
In `@src/responses/state.ts`:
- Around line 476-479: Update the persisted-entry size calculation in the
snapshot loop to use UTF-8 byte length rather than JSON string character count,
matching the existing serializedBytes accounting. Ensure the byte-based size is
used for both SNAPSHOT_ENTRY_MAX_BYTES and SNAPSHOT_TOTAL_MAX_BYTES checks while
preserving the current continue, break, and total accumulation behavior.
- Around line 256-279: Update isSpillRef to validate fileName with the same
OWNED_SPILL_NAME/validSpillRef rule used by the spill store, preferably by
reusing an exported predicate from spill-store.ts. Ensure loadSnapshotEntry
rejects invalid spill references through its existing tombstone path so the
failure is recorded as spill_failed rather than admitting an unreplayable spill
stub.
- Around line 461-493: Bound the retry loop in persistNow around the
stateRevision comparison so it performs only a small fixed number of snapshot
attempts, then accepts the latest completed snapshot even if stateRevision
changed during the write. Preserve the existing serialization and atomic write
behavior, and ensure control exits the loop so pendingSpillUnlinks is drained on
every call while the debounce mechanism can retry later.
- Around line 539-558: The pruneResponses eviction loop performs unbounded
synchronous durable spills and can block the request path on many fsyncs.
Replace the loop’s spill handling with a bounded admission barrier around
serialized writeResponseSpillDurably work, preserving synchronous durable
publication and immediate enforcement of the RAM cap; ensure rows not admitted
are handled without retaining resident data. Add coverage for many resident rows
already exceeding byteCap().
In `@src/tray/windows.ts`:
- Around line 221-259: Move the io.write call in replaceWindowsTrayOwnedFile
into the existing try block so failures during writing enter the finally cleanup
path. Preserve the renamed-success behavior and ensure any partial temporary
file is unlinked and its hardened-path state forgotten when writing or later
replacement steps fail.
In `@src/vision/index.ts`:
- Around line 51-64: Update the cache set method to compute the replacement size
and reject values exceeding maxBytes or invalid maxEntries before deleting or
modifying the existing entry. Preserve the current entry and byte accounting
when admission fails, while retaining normal eviction and replacement behavior
for admissible values. Add a regression test near the existing maxBytes coverage
in tests/vision-cache.test.ts that caches a small value, lowers maxBytes,
attempts an oversized replacement for the same key, and verifies the original
entry remains.
In `@tests/anthropic-image-normalize.test.ts`:
- Around line 147-166: Correct the first test title to describe its configured
2,500-byte cache limit rather than 64 MiB, and tighten the assertion in the “040
snapshot…” test around evictOldestAnthropicImageNormalizeForBudget so it
validates the evicted entry’s own released bytes using stable pre-eviction
metadata or entry information; remove the misleading comparison against
post-eviction metadataBytes divided by before.count while preserving the exact
bytes accounting assertion.
In `@tests/cursor-live-transport.test.ts`:
- Around line 64-85: Move the default setCursorBlobLimitsForTests cleanup into
an afterEach hook in the test file, ensuring limits are restored even when
assertions fail. Remove the now-redundant resetCursorBlobStateForTests call from
this test, confirm afterEach is imported, and retain the test’s existing setup
and assertions.
In `@tests/responses-state.test.ts`:
- Around line 973-989: Correct the test description and setup in the “pending
spill-unlink deferral” test to state that the synchronous body prevents the
debounced persistTimer from firing, rather than claiming spill I/O snapshot
writes fail; keep the existing assertions and replacement loop unchanged. Remove
the unused dir assignment and its corresponding void dir statement, and remove
the no-op setSpillIoForTest({ write: undefined }) call unless it serves another
verified purpose.
In `@tests/state-store-sweeper.test.ts`:
- Around line 175-194: Replace the local reconcileOwner closure in the “stale or
duplicate generation cannot delete current keys” test with the production
reconciliation path: register the owner, invoke the real
sweeper/reconcileStateGeneration flow as in the nearby test, and assert the
generated candidate removes stale rows. Separately invoke the exported owner
function with a hand-built stale or duplicate GenerationContext to exercise the
generation guard directly, preserving assertions that current keys remain
intact.
---
Outside diff comments:
In `@src/codex/auth-api.ts`:
- Around line 1211-1223: Capture writerGeneration via captureConfigGeneration()
before the WHAM fetch in the surrounding OAuth flow, then pass that captured
value explicitly to setAccountQuotaFromParsed(accountId, quota) in the
background IIFE. Preserve the existing polling and conflict re-check behavior
while ensuring the quota write uses the generation active when the WHAM data was
retrieved.
In `@src/server/responses/core.ts`:
- Around line 1490-1497: In the Codex forward-pool outcome recording flow,
remove the redundant writerGeneration narrowing and pass
authCtx.writerGeneration directly to recordCodexUpstreamOutcome. Apply this in
the corresponding compact.ts path guarded by usesCodexForwardPoolAuth, while
preserving the existing guarded reads in core.ts.
In `@src/vision/index.ts`:
- Around line 28-34: Make snapshot and evictOldest required members of the
VisionDescriptionCache interface, then update all implementations and test
doubles passed to setVisionDescriptionCache to provide both methods. Preserve
visionDescriptionRetainedStoreSnapshot and evictOldestVisionDescriptionForBudget
behavior while ensuring every injected cache reports usage and supports
eviction.
🪄 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: b67e67a9-0cc5-4313-9df3-c82fcc493abd
📒 Files selected for processing (79)
devlog/_plan/260801_zero_leak_state_stores/000_state_store_inventory.mddevlog/_plan/260801_zero_leak_state_stores/005_impl_roadmap.mddevlog/_plan/260801_zero_leak_state_stores/006_roadmap_audit_synthesis.mddevlog/_plan/260801_zero_leak_state_stores/010_continuation_hard_cap.mddevlog/_plan/260801_zero_leak_state_stores/020_blob_and_replay_caps.mddevlog/_plan/260801_zero_leak_state_stores/030_eviction_mechanisms.mddevlog/_plan/260801_zero_leak_state_stores/035_registry_admission_caps.mddevlog/_plan/260801_zero_leak_state_stores/040_app_bytes_observability.mddevlog/_plan/260801_zero_leak_state_stores/050_translator_stream_bounds.mddevlog/_plan/260801_zero_leak_state_stores/055_background_shell_lifecycle.mddevlog/_plan/260801_zero_leak_state_stores/060_proxy_benchmark.mddevlog/_plan/260801_zero_leak_state_stores/070_close_and_push.mdgui/src/components/MemoryObservabilityCard.tsxsrc/adapters/anthropic-image-normalize.tssrc/adapters/cursor/live-transport.tssrc/adapters/cursor/native-exec.tssrc/adapters/cursor/protobuf-request.tssrc/adapters/google-antigravity-replay.tssrc/codex/account-runtime-state.tssrc/codex/auth-api.tssrc/codex/auth-context.tssrc/codex/catalog/aggregation.tssrc/codex/catalog/provider-fetch.tssrc/codex/model-cache.tssrc/codex/pool-rotation.tssrc/codex/quota.tssrc/codex/routing.tssrc/codex/subagent-model-fallback.tssrc/combos/failover.tssrc/combos/request.tssrc/combos/resolve.tssrc/combos/types.tssrc/config.tssrc/lib/config-ownership.tssrc/lib/errors.tssrc/lib/gcp-adc.tssrc/lib/state-store-registrations.tssrc/lib/state-store-sweeper.tssrc/lib/windows-secret-acl.tssrc/oauth/anthropic-routing.tssrc/oauth/index.tssrc/oauth/store.tssrc/oauth/token-guardian.tssrc/providers/key-failover.tssrc/providers/openai-sidecar.tssrc/providers/quota.tssrc/responses/spill-store.tssrc/responses/state.tssrc/router.tssrc/server/index.tssrc/server/lifecycle.tssrc/server/management-auth.tssrc/server/management/combo-routes.tssrc/server/management/oauth-account-routes.tssrc/server/management/provider-routes.tssrc/server/management/system-routes.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/tray/windows.tssrc/vision/index.tstests/anthropic-image-normalize.test.tstests/codex-auth-api.test.tstests/codex-main-rotation.test.tstests/combos.test.tstests/config.test.tstests/cursor-blob.test.tstests/cursor-live-transport.test.tstests/google-antigravity-replay.test.tstests/issue-702-expired-replay-state.test.tstests/memory-watchdog.test.tstests/oauth-store-multi.test.tstests/openai-provider-option-startup.test.tstests/provider-account-quota.test.tstests/responses-state.test.tstests/server-management-auth.test.tstests/state-store-sweeper.test.tstests/vision-cache.test.tstests/windows-secret-acl.test.tstests/windows-tray.test.ts
| This phase deliberately uses a synchronous transaction. `rememberResponseState()` is | ||
| called from synchronous bridge completion callbacks (`src/bridge.ts:143,811-817` and | ||
| `src/server/relay.ts:498,680-718`); an un-awaited asynchronous spill would permit an | ||
| unbounded pending-write closure chain and a post-response stub race. The synchronous | ||
| path is the bounded in-flight window: at most one transaction exists on the JS thread, | ||
| with no queue. Spills are exceptional, not the small-entry hot path. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Move durable spill I/O off the request event loop.
rememberResponseState() performs synchronous serialization, file writes, and fsyncSync() for potentially large entries. Repeated oversized replacements can block the Bun JavaScript thread and stall unrelated requests, streams, and management calls. Calling this path “exceptional” does not provide a production bound.
Use a bounded asynchronous or isolated I/O path with one in-flight operation, or define an explicit admission limit and latency budget. Preserve durable-before-stub publication.
🤖 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/_plan/260801_zero_leak_state_stores/010_continuation_hard_cap.md`
around lines 196 - 201, Update the synchronous durable spill path used by
rememberResponseState() to move serialization, file writes, and fsync work off
the request event loop through a bounded asynchronous or isolated I/O mechanism
with at most one in-flight operation. Add explicit admission and latency limits
as needed, while preserving durable-before-stub publication and preventing
unbounded pending-write chains or post-response stub races.
| `pendingSpillUnlinks` (hard cap 128 entries — small refs, not payloads; overflow | ||
| unlinks oldest-first immediately, accepting the narrow crash-window regression only | ||
| under pathological replacement churn), and `persistNow()` drains the queue strictly | ||
| AFTER the snapshot write succeeds. The queue holds no payload bytes, never counts | ||
| toward `storedResponseBytes`, and is not ownership: files it references are already | ||
| unreferenced by the post-flush snapshot and would be reclaimed by orphan GC anyway — | ||
| the queue only accelerates that reclamation. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not unlink deferred spill files before the snapshot is durable.
Line 316 permits immediate unlink when pendingSpillUnlinks overflows. This violates Lines 297-303, which require the old file to survive until the snapshot referencing the new stub is durable. A crash after the early unlink can reload the old stub with a missing spill file and return previous_response_not_found.
When the queue is full, synchronously flush the snapshot before unlinking, or preserve the old file and reject the replacement. Do not accept an early-unlink path that knowingly weakens replay durability.
🤖 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/_plan/260801_zero_leak_state_stores/010_continuation_hard_cap.md`
around lines 314 - 320, Revise the pendingSpillUnlinks overflow behavior so
deferred spill files are never unlinked before the replacement snapshot is
durable. When the 128-entry cap is reached, synchronously flush the snapshot
before unlinking, or reject/preserve the replacement; remove the documented
“overflow unlinks oldest-first immediately” path while keeping persistNow()’s
post-success queue draining.
| - **Failure semantics:** each successful owner atomically records its current | ||
| live-key set/topology and `lastReconciledGeneration`. If a callback throws, | ||
| log the static registration name, leave that owner on its prior generation, | ||
| and schedule ONE retry on the next `sweepExpiredOnWrite` tick (retry uses a | ||
| freshly built context — stale contexts are never reused). | ||
| To make that retry executable, the sweeper stores the CONTEXT BUILDER, not | ||
| a context: `setGenerationContextBuilder(build: () => GenerationContext)` is | ||
| called once from `state-store-registrations.ts`; the retry invokes it | ||
| fresh (round-2 blocker 4). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make failed reconciliation retries executable.
sweepExpiredOnWrite() is defined as TTL-only and does not run reconciliation callbacks. However, this section says a failed reconciliation callback retries on the next sweepExpiredOnWrite() tick. No code path performs that retry.
Add a separate pending-reconciliation retry path, or change the write-trigger contract to run a fresh reconciliation context. Add a regression that throws one owner callback and verifies that the next write retries it.
🤖 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/_plan/260801_zero_leak_state_stores/030_eviction_mechanisms.md` around
lines 141 - 149, The documented failure semantics are not executable because
sweepExpiredOnWrite() only performs TTL eviction. Update the
reconciliation/sweeper design so a failed owner callback is retried exactly once
on the next write using a freshly built context from
setGenerationContextBuilder, or revise the write-trigger contract to invoke that
fresh reconciliation path. Add a regression covering one callback failure
followed by a next-write retry.
| set(key: string, value: string): void { | ||
| this.entries.delete(key); | ||
| this.entries.set(key, value); | ||
| while (this.entries.size > this.maxEntries) { | ||
| const oldest = this.entries.keys().next().value; | ||
| if (oldest === undefined) break; | ||
| this.entries.delete(oldest); | ||
| const existing = this.entries.get(key); | ||
| if (existing) { | ||
| this.entries.delete(key); | ||
| this.bytes -= existing.sizeBytes; | ||
| } | ||
| const sizeBytes = descriptionEncoder.encode(key).byteLength + descriptionEncoder.encode(value).byteLength; | ||
| if (sizeBytes > this.maxBytes || this.maxEntries <= 0) return; | ||
| while (this.entries.size + 1 > this.maxEntries || this.bytes + sizeBytes > this.maxBytes) { | ||
| if (this.evictOldest() === 0) return; | ||
| } | ||
| this.entries.set(key, { value, sizeBytes, storedAt: Date.now() }); | ||
| this.bytes += sizeBytes; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
An oversized replacement value evicts the still-valid cached predecessor.
Lines 52-56 delete the existing entry and subtract its bytes. Line 58 then returns early when sizeBytes > this.maxBytes. The predecessor is already gone at that point, so a replacement that fails admission silently removes a description that was still valid and still within budget.
Byte accounting stays correct, so this is not a leak. The effect is a lost cache entry and one extra vision sidecar request on the next turn. tests/vision-cache.test.ts line 297 exercises maxBytes: 1 against an empty cache, so the replacement variant is uncovered.
Compute the size and run the admission check before touching the existing entry.
🐛 Proposed fix
set(key: string, value: string): void {
+ const sizeBytes = descriptionEncoder.encode(key).byteLength + descriptionEncoder.encode(value).byteLength;
+ // Check admission BEFORE removing the predecessor: a rejected replacement
+ // must not drop a still-valid cached description.
+ if (sizeBytes > this.maxBytes || this.maxEntries <= 0) return;
const existing = this.entries.get(key);
if (existing) {
this.entries.delete(key);
this.bytes -= existing.sizeBytes;
}
- const sizeBytes = descriptionEncoder.encode(key).byteLength + descriptionEncoder.encode(value).byteLength;
- if (sizeBytes > this.maxBytes || this.maxEntries <= 0) return;
while (this.entries.size + 1 > this.maxEntries || this.bytes + sizeBytes > this.maxBytes) {
if (this.evictOldest() === 0) return;
}Add a regression test near tests/vision-cache.test.ts line 297: cache a small description, lower maxBytes, then re-set the same key with an oversized value and assert the original entry survives.
📝 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.
| set(key: string, value: string): void { | |
| this.entries.delete(key); | |
| this.entries.set(key, value); | |
| while (this.entries.size > this.maxEntries) { | |
| const oldest = this.entries.keys().next().value; | |
| if (oldest === undefined) break; | |
| this.entries.delete(oldest); | |
| const existing = this.entries.get(key); | |
| if (existing) { | |
| this.entries.delete(key); | |
| this.bytes -= existing.sizeBytes; | |
| } | |
| const sizeBytes = descriptionEncoder.encode(key).byteLength + descriptionEncoder.encode(value).byteLength; | |
| if (sizeBytes > this.maxBytes || this.maxEntries <= 0) return; | |
| while (this.entries.size + 1 > this.maxEntries || this.bytes + sizeBytes > this.maxBytes) { | |
| if (this.evictOldest() === 0) return; | |
| } | |
| this.entries.set(key, { value, sizeBytes, storedAt: Date.now() }); | |
| this.bytes += sizeBytes; | |
| } | |
| set(key: string, value: string): void { | |
| const sizeBytes = descriptionEncoder.encode(key).byteLength + descriptionEncoder.encode(value).byteLength; | |
| // Check admission BEFORE removing the predecessor: a rejected replacement | |
| // must not drop a still-valid cached description. | |
| if (sizeBytes > this.maxBytes || this.maxEntries <= 0) return; | |
| const existing = this.entries.get(key); | |
| if (existing) { | |
| this.entries.delete(key); | |
| this.bytes -= existing.sizeBytes; | |
| } | |
| while (this.entries.size + 1 > this.maxEntries || this.bytes + sizeBytes > this.maxBytes) { | |
| if (this.evictOldest() === 0) return; | |
| } | |
| this.entries.set(key, { value, sizeBytes, storedAt: Date.now() }); | |
| this.bytes += sizeBytes; | |
| } |
🤖 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/vision/index.ts` around lines 51 - 64, Update the cache set method to
compute the replacement size and reject values exceeding maxBytes or invalid
maxEntries before deleting or modifying the existing entry. Preserve the current
entry and byte accounting when admission fails, while retaining normal eviction
and replacement behavior for admissible values. Add a regression test near the
existing maxBytes coverage in tests/vision-cache.test.ts that caches a small
value, lowers maxBytes, attempts an oversized replacement for the same key, and
verifies the original entry remains.
| test("cache eviction occurs before insertion and never exceeds 64 MiB", async () => { | ||
| setNormalizeCacheLimitsForTests({ maxBytes: 2_500, maxEntryBytes: 2_000 }); | ||
| const encode: EncodeFn = async () => ({ data: "e".repeat(1_100), mediaType: "image/jpeg" }); | ||
| for (let i = 0; i < 3; i++) { | ||
| await normalizeImageTargets([target(fakePngBase64(3000 + i, 2000 + i, 256 + i))], { encode }); | ||
| expect(getNormalizeStatsForTests().cacheBytes).toBeLessThanOrEqual(2_500); | ||
| } | ||
| expect(getNormalizeStatsForTests().cacheEntries).toBeLessThanOrEqual(2); | ||
| }); | ||
|
|
||
| test("040 snapshot is observe-only and oldest-row eviction returns full metadata-inclusive released bytes", async () => { | ||
| await normalizeImageTargets([target(fakePngBase64(100, 100, 128))], { validate }); | ||
| await normalizeImageTargets([target(fakePngBase64(101, 101, 129))], { validate }); | ||
| const before = anthropicImageNormalizeRetainedStoreSnapshot(); | ||
| expect(anthropicImageNormalizeRetainedStoreSnapshot()).toEqual(before); | ||
| const released = evictOldestAnthropicImageNormalizeForBudget(); | ||
| expect(released).toBeGreaterThan(0); | ||
| expect(released).toBeGreaterThanOrEqual(getNormalizeStatsForTests().metadataBytes / Math.max(1, before.count)); | ||
| expect(anthropicImageNormalizeRetainedStoreSnapshot().bytes).toBe(before.bytes - released); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Tighten the eviction assertion and correct the test title.
Two separate problems in this block.
Line 147: the title claims "never exceeds 64 MiB", but line 148 configures maxBytes: 2_500 and line 152 asserts <= 2_500. The 64 MiB figure is the production default (CACHE_BYTE_CAP), which this test never exercises. A reader investigating a 64 MiB regression would be sent to a test that cannot detect one.
Line 164: this compares the evicted entry's released bytes against the POST-eviction metadataBytes divided by the PRE-eviction before.count. The two sides describe different populations, so the inequality holds for almost any value and cannot fail on a real accounting regression. Line 165 already asserts the exact invariant (after.bytes === before.bytes - released). Replace line 164 with a claim about the evicted entry itself.
💚 Proposed changes
- test("cache eviction occurs before insertion and never exceeds 64 MiB", async () => {
+ test("cache eviction occurs before insertion and never exceeds the configured byte cap", async () => {
setNormalizeCacheLimitsForTests({ maxBytes: 2_500, maxEntryBytes: 2_000 }); const released = evictOldestAnthropicImageNormalizeForBudget();
expect(released).toBeGreaterThan(0);
- expect(released).toBeGreaterThanOrEqual(getNormalizeStatsForTests().metadataBytes / Math.max(1, before.count));
+ // The released total is metadata-inclusive: it covers the key bytes plus the
+ // value bytes of the evicted row, and it drops the entry count by exactly one.
+ const after = anthropicImageNormalizeRetainedStoreSnapshot();
+ expect(after.count).toBe(before.count - 1);
expect(anthropicImageNormalizeRetainedStoreSnapshot().bytes).toBe(before.bytes - released);📝 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.
| test("cache eviction occurs before insertion and never exceeds 64 MiB", async () => { | |
| setNormalizeCacheLimitsForTests({ maxBytes: 2_500, maxEntryBytes: 2_000 }); | |
| const encode: EncodeFn = async () => ({ data: "e".repeat(1_100), mediaType: "image/jpeg" }); | |
| for (let i = 0; i < 3; i++) { | |
| await normalizeImageTargets([target(fakePngBase64(3000 + i, 2000 + i, 256 + i))], { encode }); | |
| expect(getNormalizeStatsForTests().cacheBytes).toBeLessThanOrEqual(2_500); | |
| } | |
| expect(getNormalizeStatsForTests().cacheEntries).toBeLessThanOrEqual(2); | |
| }); | |
| test("040 snapshot is observe-only and oldest-row eviction returns full metadata-inclusive released bytes", async () => { | |
| await normalizeImageTargets([target(fakePngBase64(100, 100, 128))], { validate }); | |
| await normalizeImageTargets([target(fakePngBase64(101, 101, 129))], { validate }); | |
| const before = anthropicImageNormalizeRetainedStoreSnapshot(); | |
| expect(anthropicImageNormalizeRetainedStoreSnapshot()).toEqual(before); | |
| const released = evictOldestAnthropicImageNormalizeForBudget(); | |
| expect(released).toBeGreaterThan(0); | |
| expect(released).toBeGreaterThanOrEqual(getNormalizeStatsForTests().metadataBytes / Math.max(1, before.count)); | |
| expect(anthropicImageNormalizeRetainedStoreSnapshot().bytes).toBe(before.bytes - released); | |
| }); | |
| test("cache eviction occurs before insertion and never exceeds the configured byte cap", async () => { | |
| setNormalizeCacheLimitsForTests({ maxBytes: 2_500, maxEntryBytes: 2_000 }); | |
| const encode: EncodeFn = async () => ({ data: "e".repeat(1_100), mediaType: "image/jpeg" }); | |
| for (let i = 0; i < 3; i++) { | |
| await normalizeImageTargets([target(fakePngBase64(3000 + i, 2000 + i, 256 + i))], { encode }); | |
| expect(getNormalizeStatsForTests().cacheBytes).toBeLessThanOrEqual(2_500); | |
| } | |
| expect(getNormalizeStatsForTests().cacheEntries).toBeLessThanOrEqual(2); | |
| }); | |
| test("040 snapshot is observe-only and oldest-row eviction returns full metadata-inclusive released bytes", async () => { | |
| await normalizeImageTargets([target(fakePngBase64(100, 100, 128))], { validate }); | |
| await normalizeImageTargets([target(fakePngBase64(101, 101, 129))], { validate }); | |
| const before = anthropicImageNormalizeRetainedStoreSnapshot(); | |
| expect(anthropicImageNormalizeRetainedStoreSnapshot()).toEqual(before); | |
| const released = evictOldestAnthropicImageNormalizeForBudget(); | |
| expect(released).toBeGreaterThan(0); | |
| // The released total is metadata-inclusive: it covers the key bytes plus the | |
| // value bytes of the evicted row, and it drops the entry count by exactly one. | |
| const after = anthropicImageNormalizeRetainedStoreSnapshot(); | |
| expect(after.count).toBe(before.count - 1); | |
| expect(anthropicImageNormalizeRetainedStoreSnapshot().bytes).toBe(before.bytes - released); | |
| }); |
🤖 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/anthropic-image-normalize.test.ts` around lines 147 - 166, Correct the
first test title to describe its configured 2,500-byte cache limit rather than
64 MiB, and tighten the assertion in the “040 snapshot…” test around
evictOldestAnthropicImageNormalizeForBudget so it validates the evicted entry’s
own released bytes using stable pre-eviction metadata or entry information;
remove the misleading comparison against post-eviction metadataBytes divided by
before.count while preserving the exact bytes accounting assertion.
…late windows-latest measured 18s and 34s for the two spill tests whose hundreds of fsync'd writes are the assertion itself, against Bun's 5s default — the wait is intrinsic, so they get a dedicated bulk-durable-IO budget with the measurement recorded. The ACL memo-lifecycle test also inherited success memos from earlier win32-override cases and asserted absolute counts, so it now resets the harden state first.
Every operational registry now answers admission before work begins: turns at the fetch boundary, WebSockets before the upgrade, storage workers and mutation homes before spawning, credential-refresh and pool-quota flights before their requests, catalog gathers before providers are contacted. Diagnostic rings and fixed slots account their bytes and truncate on a UTF-8 boundary, discovery and usage reads are byte-bounded with truncation made visible through the management API down to the GUI, and the Cursor MCP manager stages its catalog transactionally under typed limits. The shared gate lives in src/lib/admission.ts; the 035 plan doc carries the four audit rounds that moved each cap onto its real owner.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Windows CI has branch-relevant failures that block this large state-store change. Two new previous_response_id cap/cleanup tests have a 5s test budget but took about 17.7s and 34.2s on Windows, so their current implementation/test shape is not cross-platform bounded. Separately, ephemeral harden success memo lifecycle expected one memo entry but observed five, showing the test does not isolate/reset shared ACL memo state. Please make the bounded tests deterministic or give them a justified Windows-safe budget, reset the ACL memo around the lifecycle assertion, and rerun the full Windows matrix. Given the 79-file state/persistence scope, I will not override a failing platform gate.
…d shutdown lease-exact The debug ring unsubscribe now verifies registration identity so a stale disposer cannot evict a replacement listener while its admission lease stays live. An aborted Anthropic refresh owner records durable stale evidence, so the replacement flight surfaces a retryable stale error instead of escalating to reauthentication. Forced shutdown releases every admitted turn exactly once, including leases that never bound a controller, and late binding after that release stays idempotent. The guardian, quota, login, MCP decode-boundary, and GUI qualification tests now prove behavior instead of grepping source.
…undaries Three audit rounds against the post-035 tree rewrote the stale halves of the observability plan: registrations become named adapters over the hook shapes that actually shipped, the config field lands on /api/settings because /api/config PUT is disabled, and the usage 'other' bucket must bound the per-day model maps as well as the top-level one. The phase also absorbs three verified external findings as pre-work — the unbounded snapshot retry, stub-before-resident demotion, and the unwired GCP expiry sweep — with the persistence follow-up contract locked to one bounded same-path pass at shutdown.
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/server/responses/core.ts (1)
1109-1115: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve replay errors for combo requests.
The child
handleResponsescall detects corrupt replay state, so combo requests do not bypass replay handling. However,handleComboResponsesselects a target first, thenconsumeComboFailurereformats the child response beforecomboFailureDecisionstops processing. Return the standardprevious_response_not_foundresponse before combo target selection. Do not reject unresolved IDs for providers that support native continuation. Add a regression test intests/server-combo-failover-e2e.test.tsthat asserts the 400 error code and no upstream request.🤖 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/server/responses/core.ts` around lines 1109 - 1115, Handle previousResponseReplayFailure(body) before combo target selection so corrupt replay state returns the standard 400 previous_response_not_found response without invoking an upstream request. Preserve native continuation behavior by only applying this check to replay failures that require local handling, not unresolved IDs supported by the provider. Add regression coverage in the combo failover end-to-end test suite asserting the error code and zero upstream requests.Source: Path instructions
src/storage/restore-job.ts (1)
145-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated worker-reservation/spawn-gate pattern into a shared helper.
runInWorkerinsrc/storage/restore-job.tsandrunInWorkerinsrc/storage/policy-job.tsimplement the identical admission lifecycle: acquire a reservation viatryReserveStorageWorker(), reject withStorageWorkerAdmissionBusyErrorif none is available, run insidewithStorageWorkerSpawnGate, bind the reservation to the constructedWorker, release it on construction failure, and release it again in an outer.catch(). Only the worker script URL and the posted message payload differ between the two.
src/storage/restore-job.ts#L145-L223: extract the reservation/spawn-gate/cleanup scaffolding (everything except thepostMessagepayload and theWorkerscript URL) into a shared helper insrc/storage/worker-lifecycle.ts, parameterized by script URL and message payload.src/storage/policy-job.ts#L286-L360: switch thisrunInWorkerto call the same shared helper instead of re-implementing the reservation/spawn-gate/cleanup logic.Centralizing this logic means a future fix to the admission lifecycle (for example, restoring release-miss visibility the way
storage-mutation-coordinator.tsdoes) only needs to land once instead of being kept in sync across two files.🤖 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/storage/restore-job.ts` around lines 145 - 223, Extract the shared reservation, spawn-gate, worker binding, construction-failure cleanup, and outer-catch release lifecycle from runInWorker in src/storage/restore-job.ts (lines 145-223) into a helper in src/storage/worker-lifecycle.ts, parameterized by worker script URL and message payload while preserving the existing admission and cleanup behavior. Update runInWorker in src/storage/policy-job.ts (lines 286-360) to use this helper, leaving each job responsible only for its worker-specific URL and posted payload.src/server/lifecycle.ts (1)
157-163: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThe drain loop watches the wrong set, so non-streaming turns are not drained.
activeTurnsreceives an entry only when a lease binds anAbortController(line 54).tryAdmitTurnputs the lease intoadmittedTurnsunconditionally (line 68), andbindAbortControlleris optional. Every admitted turn that never binds a controller is therefore invisible here.
runAdmittedHttpTurninsrc/server/index.ts(lines 360-374) wraps buffered paths that never bind:/v1/messages/count_tokens(line 694),/v1/images/*(line 573),/v1/alpha/search(line 627), and/v1/responses/compact(line 539). During shutdown the loop at line 157 seesactiveTurns.size === 0, skips the grace period, and reachess?.stop(true)at line 197 while those requests are still executing. The client gets a truncated connection instead of a completed response, and the warning at line 161 undercounts in-flight work.
admittedTurnsis the accurate in-flight set. Use it for the wait predicate and the log line.🛠️ Proposed fix: drain on the admitted-lease set
const deadline = Date.now() + timeoutMs; - while (activeTurns.size > 0 && Date.now() < deadline) { + while (admittedTurns.size > 0 && Date.now() < deadline) { await Bun.sleep(100); } - if (activeTurns.size > 0) { - console.warn(`⚠️ Aborting ${activeTurns.size} in-flight turn(s) after ${timeoutMs}ms deadline`); + if (admittedTurns.size > 0) { + console.warn(`⚠️ Aborting ${admittedTurns.size} in-flight turn(s) after ${timeoutMs}ms deadline`); abortAndReleaseAllTurns(new Error("server shutdown")); }🤖 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/server/lifecycle.ts` around lines 157 - 163, Update the shutdown drain loop in the lifecycle handler to use admittedTurns instead of activeTurns for both the wait predicate and the abort warning count. Preserve the existing deadline, sleep, and abortAndReleaseAllTurns behavior so all admitted turns, including those without bound abort controllers, are given the shutdown grace period.src/adapters/cursor/mcp-manager.ts (1)
119-134: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the "Never throws" doc comment;
ensureConnectednow rejects on limit violations.Line 119 still states that
ensureConnectednever throws. That was true whenconnectOneswallowed every failure. It is no longer true.connectAllthrowsMcpCatalogLimitErrorat Line 134, rethrows at Line 146, andconnectOnerethrows both limit error types at Line 163.ensureConnectedreturns that promise unchanged, so callers now receive a rejection.The rejection is handled everywhere I traced.
prepareMcpinsrc/adapters/cursor/live-transport.tscatches it and fails the turn, which its own comment describes as deliberate. Each executor insrc/adapters/cursor/native-exec-mcp.tscatches and converts to a protocol error result. Note also that Line 121 memoizes the rejected promise, so every laterresolveTool,callTool,listResources, andreadResourcecall rejects with the same error for the life of the manager. For a catalog-limit violation that fail-closed behavior is right, because a retry would hit the same limit.The code is correct. The comment now tells the next reader the opposite, and this is exactly the kind of comment someone relies on when deciding whether to add a try/catch.
📝 Proposed fix: state the actual contract
- /** Idempotent, lazy connect + tool discovery across all servers. Never throws. */ + /** + * Idempotent, lazy connect + tool discovery across all servers. + * + * A single server that fails to connect is isolated and skipped. A catalog or payload limit + * violation rejects instead, because admitting a partial catalog would hide the breach. The + * rejected promise is memoized, so every later call rejects identically: retrying cannot get + * under a limit that the configuration already exceeds. + */ ensureConnected(): Promise<void> {🤖 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/adapters/cursor/mcp-manager.ts` around lines 119 - 134, Update the doc comment above ensureConnected() to remove the “Never throws” claim and accurately state that it lazily connects and discovers tools, memoizing and returning the connection promise, which may reject on connection or catalog-limit failures.
🤖 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 `@gui/src/pages/Usage.tsx`:
- Line 817: Update the Notice rendering for data?.historyTruncated in Usage to
use a warning or informational tone rather than tone="ok"; if those tones are
unavailable, use the existing alert tone while preserving the current
translation and conditional rendering.
In `@src/adapters/cursor/mcp-manager.ts`:
- Line 105: Delete the unused toolCatalogBytes field and remove all assignments
to it in the commit, rollback, and dispose paths of the cursor manager. Keep the
existing staged.reduce aggregate calculation and per-server catalogBytes
admission checks unchanged.
- Around line 114-116: Clamp each override assigned in the CursorMcpManager
constructor to its corresponding built-in ceiling, so options.maxTools,
options.maxSchemaBytes, and options.maxResultBytes can only tighten the limits
rather than raise them. Preserve the existing defaults when options are unset,
and ensure the effective maxResultBytes remains bounded for callTool,
readResource, and assertDecodedResultBudget.
- Around line 284-286: Update assertDecodedResultBudget and the
callTool/toContentItems flow to reuse the result byte count already computed by
callTool instead of invoking payloadBytes(value) again; alternatively, simplify
payloadBytes to serialize without stableJson key sorting while preserving
stableJson for indexTools. Ensure the existing maxResultBytes enforcement
remains unchanged.
In `@src/claude/inbound-debug.ts`:
- Around line 74-85: Replace repeated whole-entry JSON.stringify sizing in the
metadata truncation loop with incremental byte accounting for the entry and each
removed metadata element. Update the running size whenever metadata keys are
removed, and ensure the resulting size remains consistent with ringBytes
initialization and the entryBytes/removeOldest accounting paths.
In `@src/codex/auth-api.ts`:
- Line 666: Update the /api/codex-auth/accounts/pause-exhausted route handler to
catch PoolQuotaProbeBusyError propagated by pauseExhaustedCodexAccounts and
return the established structured 503 server_busy response with Retry-After: 1,
while preserving existing handling for other errors. Add a regression test
covering this route when quota admission is saturated at MAX_POOL_QUOTA_FLIGHTS.
In `@src/lib/admission.ts`:
- Around line 12-58: Update src/lib/admission.ts lines 12-58 around
createAdmissionGate so release reports whether it performed the first release
and enables redundant-release accounting through the gate’s releaseMisses
metric. In src/lib/debug-log-buffer.ts lines 47-59, update both
subscribeDebugLogEntries branches to release only after successful listener
deletion and record a release miss through the admission API otherwise. In
tests/debug.test.ts lines 159-166, replace the duplicate call on one stop
closure with a foreign-owner release scenario and assert that releaseMisses
increases.
In `@src/lib/injection-debug-log.ts`:
- Around line 9-32: Update injectionDebugLog to pass retainedLine to console.log
instead of the original line, ensuring both the ring buffer and console echo use
the MAX_DEBUG_LINE_BYTES-bounded value.
In `@src/oauth/anthropic-routing.ts`:
- Around line 68-72: Add focused tests for normalizeAffinityComponent and
affinity admission covering exactly 512 UTF-8 bytes as accepted, 513 bytes as
rejected, and multibyte characters with byte-based sizing. In the anthropic
account-pool tests, use explicit timestamps to verify
rotateAnthropicAccountOn429 triggers the registered write-time sweep and removes
expired health entries.
In `@src/oauth/index.ts`:
- Around line 528-535: Update the shared refresh dispatcher around the provider
branches so it marks flight.dispatched when dispatching any provider refresh,
rather than relying on refreshAnthropicAccountWithLock. Ensure the xai,
Anthropic, and generic paths all receive the same dispatched state, and remove
or avoid duplicate Anthropic-only marking so the dispatcher remains the single
owner of this invariant.
In `@src/server/responses/core.ts`:
- Line 503: Update HandleResponsesOptions.turnAdmissionLease and the matching
logIds.turnAdmissionLease fields in the chat completions and Claude messages
handlers to use ActiveTurnLease instead of AdmissionLease. Ensure the value
passed by handleResponses() to registerTurn() preserves the active lease
contract, including bindAbortController(), so ownership remains until the
response stream ends.
In `@src/usage/log.ts`:
- Around line 472-487: Update the usage parsing loop around parseUsageLines to
enforce MANAGEMENT_USAGE_MAX_ENTRIES after each batch, retaining only the newest
entries while tracking the total dropped count for the final result. Replace the
lines.filter(...).length allocation with a reduce-based non-blank line count,
and preserve the existing entriesDropped value and returned ordering.
In `@tests/active-registry-admission.test.ts`:
- Around line 78-83: Replace the ineffective
getTrackedCodexWebSocketCountForAccount("not-admitted") assertion in the
active-registry admission test with assertions on
codexWebSocketAdmissionMetrics(). Verify rejected equals one for the 129th
attempt and active remains at the configured cap, preserving the existing 503
response and server_busy checks.
In `@tests/api-usage.test.ts`:
- Around line 109-127: Update the test around the usage route cache to cover
both behaviors promised by its name: after the initial cached reads, save a
different managementUsageMaxReadBytes value, stop the current server, restart it
so the new configuration is captured, and fetch the usage route again to assert
the result is recomputed under the new limit. If restart-based invalidation is
not added, rename the test to describe only preservation of truncation metadata
across repeated reads.
In `@tests/codex-account-store.test.ts`:
- Around line 400-408: Replace microtask-only waits with bounded Bun.sleep(1)
polling: in tests/codex-account-store.test.ts lines 400-408, wait for fetchCalls
with a bounded loop and assert it equals 1; at lines 364-370, remove the
Promise.resolve wait, assert fetchCalls is at most 32 before release, and
exactly 32 after Promise.all(admitted); in tests/codex-auth-api.test.ts lines
289-292, use a bounded Bun.sleep(1) loop before asserting requestCount equals 1.
In `@tests/codex-auth-api.test.ts`:
- Around line 254-267: Remove the duplicated login-capacity setup and assertions
from the test named “pool-quota flight 17 total across accounts rejects before
request creation while compatible generation joins,” including its associated
try/finally cleanup. Keep the test focused on pool-quota flight capacity and
compatible-generation joining, leaving the dedicated login-capacity test
unchanged.
- Around line 289-292: Replace the 20-iteration Promise.resolve microtask loop
in the concurrent request test around requestCount with an event-loop yield
using Bun.sleep(1), while preserving the existing bounded wait and requestCount
assertion.
In `@tests/config.test.ts`:
- Around line 99-102: Extend the invalid-value loop in the config validation
tests to cover mcpMaxTools, mcpMaxSchemaBytes, and mcpMaxResultBytes using the
same provider override and rejection assertion. Preserve the existing invalid
values and avoid adding separate test cases.
In `@tests/cursor-mcp-manager.test.ts`:
- Around line 23-27: Replace the test-local canonicalJson helper with the
production stableJson serializer or a focused exported byte-count helper from
the MCP manager module, and use that shared implementation for the boundary
calculation around the test’s indexTools assertion. Remove canonicalJson so the
computed 4 MiB boundary remains aligned with indexTools.
In `@tests/debug.test.ts`:
- Around line 159-166: Update the test titled “subscriber unsubscribe is
idempotent and only a foreign owner records release miss” to create and exercise
a release attempt from a different subscription owner, rather than invoking only
the same stop closure twice. Retain the idempotent-unsubscribe assertion, and
verify that the foreign-owner attempt produces the expected release-miss
behavior once the releaseMisses wiring is fixed.
In `@tests/model-visibility-management-api.test.ts`:
- Around line 65-77: The existing test only inspects source text and must be
replaced or supplemented with a behavior-level regression test for the
management route. In the management API test suite, saturate all eight
catalog-gather slots, invoke the affected endpoint, and assert an HTTP 503
response with Retry-After set to 1 and an error body containing code
"catalog_busy"; release the slots during cleanup.
In `@tests/oauth-manual-code.test.ts`:
- Around line 95-98: Add a distinct test expectation in the manual login code
boundary coverage to verify a 4096-byte input is accepted by the byte-size
check, using a value that does not rely on being a parseable authorization code.
Keep the existing 4097-byte rejection assertion and ensure the new assertion
distinguishes size validation from later code parsing behavior.
In `@tests/oauth-refresh.test.ts`:
- Around line 136-158: Split the combined test into separate tests for the
33rd-flight capacity rejection and stale same-key replacement behavior. Preserve
the existing 32 synthetic seeds, no-network assertion, and Date.now() - 120_001
stale boundary; keep each test’s setup and cleanup scoped to its own invariant.
- Around line 599-613: Add a companion test near “legacy OAuth refresh intent
without flightId remains valid” that writes an OAuth refresh intent with a
non-string flightId, such as a number, then asserts readOAuthRefreshIntent
rejects it according to the parser’s invalid-input behavior rather than
returning a usable intent. Keep the existing missing-flightId compatibility
assertions unchanged.
In `@tests/storage-worker-lifecycle.test.ts`:
- Around line 96-108: Update the test around withStorageWorkerSpawnGate and the
Promise.all call so all non-null reservations are released in a finally block,
including when the spawn gate rejects before invoking its callback; preserve the
existing per-callback release behavior while preventing reservations from
leaking into later tests.
---
Outside diff comments:
In `@src/adapters/cursor/mcp-manager.ts`:
- Around line 119-134: Update the doc comment above ensureConnected() to remove
the “Never throws” claim and accurately state that it lazily connects and
discovers tools, memoizing and returning the connection promise, which may
reject on connection or catalog-limit failures.
In `@src/server/lifecycle.ts`:
- Around line 157-163: Update the shutdown drain loop in the lifecycle handler
to use admittedTurns instead of activeTurns for both the wait predicate and the
abort warning count. Preserve the existing deadline, sleep, and
abortAndReleaseAllTurns behavior so all admitted turns, including those without
bound abort controllers, are given the shutdown grace period.
In `@src/server/responses/core.ts`:
- Around line 1109-1115: Handle previousResponseReplayFailure(body) before combo
target selection so corrupt replay state returns the standard 400
previous_response_not_found response without invoking an upstream request.
Preserve native continuation behavior by only applying this check to replay
failures that require local handling, not unresolved IDs supported by the
provider. Add regression coverage in the combo failover end-to-end test suite
asserting the error code and zero upstream requests.
In `@src/storage/restore-job.ts`:
- Around line 145-223: Extract the shared reservation, spawn-gate, worker
binding, construction-failure cleanup, and outer-catch release lifecycle from
runInWorker in src/storage/restore-job.ts (lines 145-223) into a helper in
src/storage/worker-lifecycle.ts, parameterized by worker script URL and message
payload while preserving the existing admission and cleanup behavior. Update
runInWorker in src/storage/policy-job.ts (lines 286-360) to use this helper,
leaving each job responsible only for its worker-specific URL and posted
payload.
🪄 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: 0a55a7d4-af3c-455d-8329-d44f06f0e743
📒 Files selected for processing (79)
devlog/_plan/260801_zero_leak_state_stores/035_registry_admission_caps.mdgui/src/components/apikeys-workspace/ApiKeysWorkspace.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/ApiKeys.tsxgui/src/pages/Usage.tsxgui/tests/usage-layout.test.tssrc/adapters/cursor/live-models.tssrc/adapters/cursor/live-transport.tssrc/adapters/cursor/mcp-manager.tssrc/adapters/cursor/native-exec-mcp.tssrc/adapters/mimo-free.tssrc/claude/inbound-debug.tssrc/cli/catalog-prewarm.tssrc/codex/account-store.tssrc/codex/auth-api.tssrc/codex/auth-context.tssrc/codex/catalog/provider-fetch.tssrc/codex/main-account-cache.tssrc/codex/project-config-warnings.tssrc/codex/routing.tssrc/codex/shim.tssrc/codex/websocket-registry.tssrc/config.tssrc/lib/admission.tssrc/lib/crash-guard.tssrc/lib/debug-log-buffer.tssrc/lib/injection-debug-log.tssrc/lib/sidecar-tracker.tssrc/oauth/anthropic-routing.tssrc/oauth/index.tssrc/oauth/store.tssrc/oauth/token-guardian.tssrc/server/chat-completions.tssrc/server/claude-messages.tssrc/server/index.tssrc/server/lifecycle.tssrc/server/management-api.tssrc/server/management/api-key-usage.tssrc/server/management/logs-usage-routes.tssrc/server/management/oauth-account-routes.tssrc/server/responses/core.tssrc/server/startup-health-cache.tssrc/server/system-env.tssrc/server/ws-bridge.tssrc/storage/policy-job.tssrc/storage/restore-job.tssrc/storage/storage-mutation-coordinator.tssrc/storage/worker-lifecycle.tssrc/types.tssrc/usage/log.tstests/active-registry-admission.test.tstests/api-debug.test.tstests/api-usage.test.tstests/claude-inbound-debug.test.tstests/cli-catalog-prewarm.test.tstests/codex-account-store.test.tstests/codex-auth-api.test.tstests/codex-auth-context.test.tstests/config.test.tstests/cursor-hardening.test.tstests/cursor-mcp-manager.test.tstests/debug.test.tstests/gather-routed-models-single-flight.test.tstests/helpers/test-budget.tstests/mimo-free-provider.test.tstests/model-visibility-management-api.test.tstests/oauth-manual-code.test.tstests/oauth-refresh.test.tstests/responses-state.test.tstests/shutdown-drain.test.tstests/storage-mutation-race.test.tstests/storage-worker-lifecycle.test.tstests/usage-log.test.tstests/windows-secret-acl.test.ts
| ) : ( | ||
| <> | ||
| {state.showError && <Notice tone="err">{t("usage.loadError")}</Notice>} | ||
| {data?.historyTruncated && <Notice tone="ok">{t("usage.historyTruncated")}</Notice>} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not render truncated history with the success tone.
Notice renders tone="ok" with success styling and a check icon. Truncated history means that the report is incomplete. Add a warning or informational tone to Notice and use it here. If that is not possible in this change, use the existing alert tone instead of ok.
🤖 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 `@gui/src/pages/Usage.tsx` at line 817, Update the Notice rendering for
data?.historyTruncated in Usage to use a warning or informational tone rather
than tone="ok"; if those tones are unavailable, use the existing alert tone
while preserving the current translation and conditional rendering.
| private readonly servers = new Map<string, ConnectedServer>(); | ||
| /** advertisedName -> { serverName, original toolName } */ | ||
| private readonly toolIndex = new Map<string, { serverName: string; toolName: string; handle: McpToolHandle }>(); | ||
| private toolCatalogBytes = 0; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
toolCatalogBytes is written in three places and never read.
Line 105 initializes it, Line 139 accumulates into it on commit, Line 145 zeroes it on rollback, and Line 292 zeroes it on dispose. No expression ever reads it. Every admission decision uses a freshly computed value instead: Line 133 recomputes the cross-server total with staged.reduce, and Line 199 uses the per-server local catalogBytes.
Two consequences. First, the field is dead state that looks load-bearing, so a future reader may assume the aggregate check consults it. Second, if someone does wire it into a check, it will be wrong on duplicate advertised names: Line 138 overwrites the colliding toolIndex entry while Line 139 still adds both servers' bytes, so the counter overstates the retained catalog.
Pick one direction. Either make it authoritative and replace the O(n²) reduce at Lines 132-133, or delete it.
♻️ Option A: make the counter authoritative and drop the repeated reduce
private async connectAll(): Promise<void> {
const staged: Array<{ connection: ConnectedServer; tools: Array<[string, { serverName: string; toolName: string; handle: McpToolHandle }]>; bytes: number }> = [];
+ let stagedTools = 0;
+ let stagedBytes = 0;
try {
for (const server of this.resolved) {
const connected = await this.connectOne(server);
if (!connected) continue;
staged.push(connected);
- const toolCount = staged.reduce((sum, row) => sum + row.tools.length, 0);
- const catalogBytes = staged.reduce((sum, row) => sum + row.bytes, 0);
- if (toolCount > this.maxTools || catalogBytes > CURSOR_MCP_MAX_CATALOG_BYTES) throw new McpCatalogLimitError();
+ stagedTools += connected.tools.length;
+ stagedBytes += connected.bytes;
+ if (stagedTools > this.maxTools || stagedBytes > CURSOR_MCP_MAX_CATALOG_BYTES) throw new McpCatalogLimitError();
}Option B is to delete the field and its three assignments. With at most CURSOR_MCP_MAX_SERVERS (32) staged rows the current reduce costs nothing measurable, so Option B is the smaller change if no future check needs the counter.
Also applies to: 139-139, 292-292
🤖 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/adapters/cursor/mcp-manager.ts` at line 105, Delete the unused
toolCatalogBytes field and remove all assignments to it in the commit, rollback,
and dispose paths of the cursor manager. Keep the existing staged.reduce
aggregate calculation and per-server catalogBytes admission checks unchanged.
| this.maxTools = options.maxTools ?? CURSOR_MCP_MAX_TOOLS; | ||
| this.maxSchemaBytes = options.maxSchemaBytes ?? CURSOR_MCP_MAX_SCHEMA_BYTES; | ||
| this.maxResultBytes = options.maxResultBytes ?? CURSOR_MCP_MAX_RESULT_BYTES; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Clamp the operator overrides to the built-in ceilings so a config edit cannot remove the result cap.
Lines 114-116 use options.X ?? DEFAULT , so the constants act as defaults, not as maxima. src/config.ts Line 473 and Line 475 validate mcpMaxTools, mcpMaxSchemaBytes, and mcpMaxResultBytes only as z.number().int().positive(). Any positive integer is accepted, including one far above the built-in value.
For maxTools this is contained. The aggregate ceiling CURSOR_MCP_MAX_CATALOG_BYTES is not overridable and is enforced at Line 134 and Line 199, so tool-catalog memory stays capped at 4 MiB no matter how high mcpMaxTools goes.
maxResultBytes has no such backstop. It is the only bound at Line 239 (callTool), Line 271 and Line 275 (readResource), and Line 285 (assertDecodedResultBudget). A config carrying mcpMaxResultBytes: 2_000_000_000 removes the per-result cap outright, and the decoded-blob accounting added in this PR then guards nothing. That contradicts the "hard caps" goal of the change set.
This needs an operator edit, so it is a hardening gap and not an exploitable path. Treat the constants as maxima and let the config only tighten them.
🛡️ Proposed fix: overrides may only lower the ceiling
- this.maxTools = options.maxTools ?? CURSOR_MCP_MAX_TOOLS;
- this.maxSchemaBytes = options.maxSchemaBytes ?? CURSOR_MCP_MAX_SCHEMA_BYTES;
- this.maxResultBytes = options.maxResultBytes ?? CURSOR_MCP_MAX_RESULT_BYTES;
+ // The constants are ceilings, not defaults. A provider config may tighten a budget for a
+ // constrained deployment; it must not raise one, or the hard cap stops being hard.
+ this.maxTools = Math.min(options.maxTools ?? CURSOR_MCP_MAX_TOOLS, CURSOR_MCP_MAX_TOOLS);
+ this.maxSchemaBytes = Math.min(options.maxSchemaBytes ?? CURSOR_MCP_MAX_SCHEMA_BYTES, CURSOR_MCP_MAX_SCHEMA_BYTES);
+ this.maxResultBytes = Math.min(options.maxResultBytes ?? CURSOR_MCP_MAX_RESULT_BYTES, CURSOR_MCP_MAX_RESULT_BYTES);If raising a budget is an intended deployment option, say so in the CursorMcpManagerOptions doc comment at Lines 75-77 and add a documented upper bound in the Zod schema instead, so the reachable maximum is stated in one place.
📝 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.
| this.maxTools = options.maxTools ?? CURSOR_MCP_MAX_TOOLS; | |
| this.maxSchemaBytes = options.maxSchemaBytes ?? CURSOR_MCP_MAX_SCHEMA_BYTES; | |
| this.maxResultBytes = options.maxResultBytes ?? CURSOR_MCP_MAX_RESULT_BYTES; | |
| // The constants are ceilings, not defaults. A provider config may tighten a budget for a | |
| // constrained deployment; it must not raise one, or the hard cap stops being hard. | |
| this.maxTools = Math.min(options.maxTools ?? CURSOR_MCP_MAX_TOOLS, CURSOR_MCP_MAX_TOOLS); | |
| this.maxSchemaBytes = Math.min(options.maxSchemaBytes ?? CURSOR_MCP_MAX_SCHEMA_BYTES, CURSOR_MCP_MAX_SCHEMA_BYTES); | |
| this.maxResultBytes = Math.min(options.maxResultBytes ?? CURSOR_MCP_MAX_RESULT_BYTES, CURSOR_MCP_MAX_RESULT_BYTES); |
🤖 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/adapters/cursor/mcp-manager.ts` around lines 114 - 116, Clamp each
override assigned in the CursorMcpManager constructor to its corresponding
built-in ceiling, so options.maxTools, options.maxSchemaBytes, and
options.maxResultBytes can only tighten the limits rather than raise them.
Preserve the existing defaults when options are unset, and ensure the effective
maxResultBytes remains bounded for callTool, readResource, and
assertDecodedResultBudget.
| assertDecodedResultBudget(value: unknown, decodedBytes: number): void { | ||
| if (payloadBytes(value) + decodedBytes > this.maxResultBytes) throw new McpPayloadTooLargeError("result"); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
assertDecodedResultBudget re-serializes a payload that callTool already measured.
Line 285 calls payloadBytes(value), which runs stableJson and a UTF-8 encode over the whole value. For the tool-call path that work already happened. callTool measured the same result at Line 239, then toContentItems in src/adapters/cursor/native-exec-mcp.ts Line 117 passes the result back here and Line 285 measures it again.
So every MCP tool result that carries image content is fully JSON-serialized and UTF-8 encoded twice. The budget is 8 MiB by default (CURSOR_MCP_MAX_RESULT_BYTES), so the duplicated work scales to megabytes per tool call, and stableJson additionally sorts every object's keys with localeCompare on each pass.
Note that stableJson sorting is pure overhead for this purpose: payloadBytes only reads .byteLength, and key order does not change the serialized length. Two options, either is fine.
♻️ Option A: accept a precomputed size so the caller measures once
- assertDecodedResultBudget(value: unknown, decodedBytes: number): void {
- if (payloadBytes(value) + decodedBytes > this.maxResultBytes) throw new McpPayloadTooLargeError("result");
- }
+ /** `serializedBytes` lets a caller that already measured the payload avoid a second pass. */
+ assertDecodedResultBudget(value: unknown, decodedBytes: number, serializedBytes?: number): void {
+ const raw = serializedBytes ?? payloadBytes(value);
+ if (raw + decodedBytes > this.maxResultBytes) throw new McpPayloadTooLargeError("result");
+ }Option B is to have payloadBytes skip the key sort, since no caller depends on the exact string. That removes the localeCompare cost from both passes and is a smaller diff:
function payloadBytes(value: unknown): number {
- return utf8.encode(stableJson(value)).byteLength;
+ // Only the length is read, and key order does not change it — skip the sort.
+ return utf8.encode(JSON.stringify(value) ?? "").byteLength;
}Keep stableJson for indexTools at Line 194 if a stable schema string is wanted there for any other reason.
🤖 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/adapters/cursor/mcp-manager.ts` around lines 284 - 286, Update
assertDecodedResultBudget and the callTool/toContentItems flow to reuse the
result byte count already computed by callTool instead of invoking
payloadBytes(value) again; alternatively, simplify payloadBytes to serialize
without stableJson key sorting while preserving stableJson for indexTools.
Ensure the existing maxResultBytes enforcement remains unchanged.
| function entryBytes(entry: ClaudeInboundDebugEntry): number { | ||
| return retainedUtf8Bytes(JSON.stringify(entry)); | ||
| } | ||
|
|
||
| function removeOldest(): number { | ||
| const removed = ring.shift(); | ||
| if (!removed) return 0; | ||
| const bytes = entryBytes(removed); | ||
| ringBytes -= bytes; | ||
| return bytes; | ||
| } | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Repeated full-object JSON.stringify in the truncation loop is expensive for large metadata.
entryBytes (Lines 74-76) computes size by calling JSON.stringify(entry) on the whole entry and measuring the UTF-8 length of the result. The while-loop at Lines 126-130 calls entryBytes(entry) once per popped metadataKeys element, and the loop can run up to MAX_CLAUDE_INBOUND_METADATA_KEYS (64) times for a single captured request. Each of those 64 calls re-serializes the entire entry object with JSON.stringify, even though only one array element changed between iterations.
In the worst case — a client sending metadata with 64 keys, each truncated to the 8 KiB per-value cap — the entry starts near 512 KiB and is re-stringified up to 64 times while shrinking. This is materially more expensive than the additive byte accounting used elsewhere in this PR (compare fetchTraceBytes in src/lib/crash-guard.ts, which sums retainedUtf8Bytes() over individual fields instead of serializing the whole object).
Since Claude inbound debug capture is opt-in but client-triggerable once enabled (the metadata object shape is controlled by the request body), this repeated serialization is worth avoiding. Track the entry's byte size incrementally instead of re-deriving it from JSON.stringify on every loop iteration:
♻️ Proposed refactor sketch
-function entryBytes(entry: ClaudeInboundDebugEntry): number {
- return retainedUtf8Bytes(JSON.stringify(entry));
-}
+function metadataKeysBytes(keys: string[] | undefined): number {
+ return (keys ?? []).reduce((sum, key) => sum + retainedUtf8Bytes(key), 0);
+}
+
+function entryBytes(entry: ClaudeInboundDebugEntry): number {
+ // Sum the variable-length fields directly instead of JSON.stringify-ing the
+ // whole entry on every call inside the truncation loop.
+ return retainedUtf8Bytes(entry.model)
+ + retainedUtf8Bytes(entry.resolvedModel ?? "")
+ + retainedUtf8Bytes(entry.thinkingType ?? "")
+ + retainedUtf8Bytes(entry.outputConfigEffort ?? "")
+ + retainedUtf8Bytes(entry.anthropicBeta ?? "")
+ + metadataKeysBytes(entry.metadataKeys)
+ + 128; // fixed overhead for scalar fields (id/at/tags/flags)
+}Verify this against the entryBytes-based accounting used for ringBytes at Line 139 and removeOldest at Lines 78-84, so the running total stays consistent with whatever computation method you pick.
Also applies to: 106-140
🤖 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/claude/inbound-debug.ts` around lines 74 - 85, Replace repeated
whole-entry JSON.stringify sizing in the metadata truncation loop with
incremental byte accounting for the entry and each removed metadata element.
Update the running size whenever metadata keys are removed, and ensure the
resulting size remains consistent with ringBytes initialization and the
entryBytes/removeOldest accounting paths.
| test("catalog busy maps management and v1 models to 503 startup to warn-skip and system-env to skip", async () => { | ||
| const management = await Bun.file(new URL("../src/server/management-api.ts", import.meta.url)).text(); | ||
| const server = await Bun.file(new URL("../src/server/index.ts", import.meta.url)).text(); | ||
| const prewarm = await Bun.file(new URL("../src/cli/catalog-prewarm.ts", import.meta.url)).text(); | ||
| const systemEnv = await Bun.file(new URL("../src/server/system-env.ts", import.meta.url)).text(); | ||
| for (const source of [management, server]) { | ||
| expect(source).toContain("CatalogGatherBusyError"); | ||
| expect(source).toContain('"catalog_busy"'); | ||
| expect(source).toContain('"Retry-After": "1"'); | ||
| } | ||
| expect(prewarm).toContain("startup discovery skipped"); | ||
| expect(systemEnv).toContain('(error as { code?: unknown }).code === "catalog_busy"'); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test the catalog-busy response through the route.
This test only checks source text. It does not prove that a saturated catalog gather returns HTTP 503, Retry-After: 1, and error.code = "catalog_busy" from the management route. Add a behavior-level test that fills the eight catalog-gather slots and invokes the affected endpoint.
As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
🤖 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/model-visibility-management-api.test.ts` around lines 65 - 77, The
existing test only inspects source text and must be replaced or supplemented
with a behavior-level regression test for the management route. In the
management API test suite, saturate all eight catalog-gather slots, invoke the
affected endpoint, and assert an HTTP 503 response with Retry-After set to 1 and
an error body containing code "catalog_busy"; release the slots during cleanup.
Source: Path instructions
| test("OAuth token-refresh flight 33 rejects and a stale same-key owner cannot delete replacement", async () => { | ||
| await saveCredential("kiro", { access: "old", refresh: "rt-old", expires: 1, accountId: "flight-owner" }); | ||
| const accountId = getAccountSet("kiro")!.activeAccountId; | ||
| const full = seedOAuthTokenRefreshFlightsForTests(Array.from({ length: 32 }, (_, index) => ({ key: `synthetic-${index}` }))); | ||
| const fetchBefore = mockRefreshFetch([]); | ||
| try { | ||
| await expect(getValidAccessTokenForAccount("kiro", accountId)).rejects.toBeInstanceOf(OAuthTokenRefreshBusyError); | ||
| expect(fetchBefore.count()).toBe(0); | ||
| } finally { | ||
| for (const promise of full.promises) promise.catch(() => {}); | ||
| full.cleanup(); | ||
| } | ||
|
|
||
| const stale = seedOAuthTokenRefreshFlightsForTests([{ key: `kiro\0${accountId}`, startedAt: Date.now() - 120_001 }]); | ||
| const refresh = mockRefreshFetch([ | ||
| new Response(JSON.stringify({ accessToken: "fresh", refreshToken: "rt-fresh", expiresIn: 3600 }), { status: 200 }), | ||
| ]); | ||
| const replacement = getValidAccessTokenForAccount("kiro", accountId); | ||
| await expect(stale.promises[0]!).rejects.toBeInstanceOf(OAuthTokenRefreshStaleError); | ||
| await expect(replacement).resolves.toBe("fresh"); | ||
| expect(refresh.count()).toBe(1); | ||
| stale.cleanup(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider splitting the two behaviors; the assertions themselves are well targeted.
The capacity half is precise. The cap is 32 (MAX_OAUTH_TOKEN_REFRESH_FLIGHTS, src/oauth/index.ts Line 57), Line 139 seeds exactly 32, and the request at Line 142 is the 33rd. The seeded keys are synthetic-N, not kiro\0<accountId>, so neither the join branch at Line 295 nor the stale-replacement branch at Lines 299-304 can fire, and the capacity check at Line 305 is provably what rejects. Line 143 then confirms rejection precedes any network call, which is the assertion that matters for an admission cap. Line 145 attaches .catch before cleanup() aborts, so the seeded rejections stay handled.
The stale half is also correct: Date.now() - 120_001 is one millisecond past OAUTH_TOKEN_REFRESH_FLIGHT_STALE_MS, which is the right boundary to pin.
Two notes.
First, the test name claims a stale owner "cannot delete replacement", but that outcome is structural rather than observed. cleanup() deletes only entries whose flight object is in its own inserted array, and the replacement is a different object, so it could never be removed by this call. The real protection is the identity guard at Line 302, if (tokenRefreshes.get(key) === existing), which the replacement resolving at Line 155 does exercise.
Second, the two behaviors share one kiro credential and one test body. When this test fails, the report does not say which invariant broke, and the first half's finally block adds cleanup that the second half does not need. Two tests would localize the failure.
No change is required. If you split them, keep the 32-seed count and the Date.now() - 120_001 offset exactly as they are, since both sit one unit past their limits on purpose.
🤖 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/oauth-refresh.test.ts` around lines 136 - 158, Split the combined test
into separate tests for the 33rd-flight capacity rejection and stale same-key
replacement behavior. Preserve the existing 32 synthetic seeds, no-network
assertion, and Date.now() - 120_001 stale boundary; keep each test’s setup and
cleanup scoped to its own invariant.
| test("legacy OAuth refresh intent without flightId remains valid", async () => { | ||
| await saveCredential("anthropic", { access: "old", refresh: "rt-old", expires: 1, accountId: "acct" }); | ||
| const id = getAccountSet("anthropic")!.activeAccountId; | ||
| const legacy = { | ||
| version: 1, | ||
| provider: "anthropic", | ||
| accountId: id, | ||
| generation: credentialGeneration(getAccountCredential("anthropic", id)!), | ||
| createdAt: Date.now() - 120_001, | ||
| } as const; | ||
| writeFileSync(getAuthRefreshIntentPath("anthropic", id), `${JSON.stringify(legacy)}\n`); | ||
|
|
||
| expect(readOAuthRefreshIntent("anthropic", id)).toEqual(legacy); | ||
| expect(readOAuthRefreshIntent("anthropic", id)?.uncertain).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add the rejecting case for a non-string flightId.
This test pins the valuable direction. src/oauth/store.ts Line 74 guards value.flightId !== undefined && typeof value.flightId !== "string", and Lines 611-612 confirm that a pre-upgrade intent with no flightId reads back unchanged and is not marked uncertain. That guard direction matters a great deal on upgrade: if undefined tripped it, readOAuthRefreshIntent would return uncertain: true for every account with an in-flight refresh, and refreshAnthropicAccountWithLock Lines 441-443 would mark each one needsReauth. Every mid-refresh user would be forced to log in again after upgrading. Good test to have.
The guard has a second direction that no test covers. Nothing writes a non-string flightId and asserts the parser rejects it. A change that relaxed Line 74 to accept any type would keep this test green, and a numeric flightId from a corrupted or hand-edited file would then reach the identity comparison at src/oauth/index.ts Line 436, where 42 === "some-uuid" is silently false. The intent would be preserved when it should have been treated as unparseable.
Both directions are one assertion each.
💚 Proposed fix: pin the reject direction too
expect(readOAuthRefreshIntent("anthropic", id)).toEqual(legacy);
expect(readOAuthRefreshIntent("anthropic", id)?.uncertain).toBeUndefined();
+
+ // The other direction of the same guard: a present-but-non-string flightId is
+ // unparseable and must degrade to the uncertain sentinel, not flow through to the
+ // flight-identity comparison in refreshAnthropicAccountWithLock.
+ writeFileSync(getAuthRefreshIntentPath("anthropic", id), `${JSON.stringify({ ...legacy, flightId: 42 })}\n`);
+ expect(readOAuthRefreshIntent("anthropic", id)).toMatchObject({ uncertain: true, generation: "", createdAt: 0 });
});📝 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.
| test("legacy OAuth refresh intent without flightId remains valid", async () => { | |
| await saveCredential("anthropic", { access: "old", refresh: "rt-old", expires: 1, accountId: "acct" }); | |
| const id = getAccountSet("anthropic")!.activeAccountId; | |
| const legacy = { | |
| version: 1, | |
| provider: "anthropic", | |
| accountId: id, | |
| generation: credentialGeneration(getAccountCredential("anthropic", id)!), | |
| createdAt: Date.now() - 120_001, | |
| } as const; | |
| writeFileSync(getAuthRefreshIntentPath("anthropic", id), `${JSON.stringify(legacy)}\n`); | |
| expect(readOAuthRefreshIntent("anthropic", id)).toEqual(legacy); | |
| expect(readOAuthRefreshIntent("anthropic", id)?.uncertain).toBeUndefined(); | |
| }); | |
| test("legacy OAuth refresh intent without flightId remains valid", async () => { | |
| await saveCredential("anthropic", { access: "old", refresh: "rt-old", expires: 1, accountId: "acct" }); | |
| const id = getAccountSet("anthropic")!.activeAccountId; | |
| const legacy = { | |
| version: 1, | |
| provider: "anthropic", | |
| accountId: id, | |
| generation: credentialGeneration(getAccountCredential("anthropic", id)!), | |
| createdAt: Date.now() - 120_001, | |
| } as const; | |
| writeFileSync(getAuthRefreshIntentPath("anthropic", id), `${JSON.stringify(legacy)}\n`); | |
| expect(readOAuthRefreshIntent("anthropic", id)).toEqual(legacy); | |
| expect(readOAuthRefreshIntent("anthropic", id)?.uncertain).toBeUndefined(); | |
| // The other direction of the same guard: a present-but-non-string flightId is | |
| // unparseable and must degrade to the uncertain sentinel, not flow through to the | |
| // flight-identity comparison in refreshAnthropicAccountWithLock. | |
| writeFileSync(getAuthRefreshIntentPath("anthropic", id), `${JSON.stringify({ ...legacy, flightId: 42 })}\n`); | |
| expect(readOAuthRefreshIntent("anthropic", id)).toMatchObject({ uncertain: true, generation: "", createdAt: 0 }); | |
| }); |
🤖 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/oauth-refresh.test.ts` around lines 599 - 613, Add a companion test
near “legacy OAuth refresh intent without flightId remains valid” that writes an
OAuth refresh intent with a non-string flightId, such as a number, then asserts
readOAuthRefreshIntent rejects it according to the parser’s invalid-input
behavior rather than returning a usable intent. Keep the existing
missing-flightId compatibility assertions unchanged.
| test("storage worker reservation 17 rejects before enqueue while the first 16 spawn serially and drain", async () => { | ||
| const reservations = Array.from({ length: 16 }, () => tryReserveStorageWorker()); | ||
| expect(reservations.every(Boolean)).toBe(true); | ||
| expect(tryReserveStorageWorker()).toBeNull(); | ||
| let active = 0; | ||
| let peak = 0; | ||
| await Promise.all(reservations.map((reservation, index) => withStorageWorkerSpawnGate(async () => { | ||
| active += 1; | ||
| peak = Math.max(peak, active); | ||
| await Bun.sleep(index === 0 ? 2 : 0); | ||
| active -= 1; | ||
| reservation?.release(); | ||
| }))); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Release reservations when the spawn gate rejects.
If withStorageWorkerSpawnGate() rejects, its callback does not run. The test then leaves its acquired reservations active. Later tests can fail because the worker gate remains full.
Wrap the Promise.all() call in try/finally. Release every non-null reservation in the finally block.
🤖 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/storage-worker-lifecycle.test.ts` around lines 96 - 108, Update the
test around withStorageWorkerSpawnGate and the Promise.all call so all non-null
reservations are released in a finally block, including when the spawn gate
rejects before invoking its callback; preserve the existing per-callback release
behavior while preventing reservations from leaking into later tests.
One process-wide budget now sits over every evictable retained store — diagnostic rings, caches, Cursor blobs, and continuation state — demoting oldest entries in a fixed category order and reporting the whole picture as a privacy-safe appOwnedBytes block on the system memory endpoint. The budget is a user-visible setting on /api/settings (256 MiB default), documented across all five locales as a cap on app-owned state, not RSS. Three defects found by external review land as pre-work: the snapshot writer's unbounded retry is capped with stable-only spill unlinking, the RAM-cap loop demotes residents before touching older spill stubs, and the GCP ADC expiry sweep finally joins the periodic registration table. The usage-log entry-cap test also gains the store budget windows-latest measured it needing.
…ement single-flight The usage-summary cache now records when its revision read actually completed and evicts by that order, so an older-started slow read that finishes last is not mistaken for the oldest retained entry. The budget enforcement pass gains an explicit single-flight refinement, the raw config boundary a direct rejection test, and the sweeper-tick fallback a TTL-expiry regression. The 040 plan doc is re-anchored to the delivered implementation with only phase-050 observed-buffer work left open.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adapters/cursor/native-exec.ts (1)
302-316: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTwo store mutators call the process-wide enforcer from inside their own insert path and then report success without re-reading the store.
enforceAppOwnedMemoryBudget(src/lib/app-owned-memory.tslines 217-241) loops until the process total fits the budget and callsevictOldest()on whichever registered store has the oldest entry. A freshly inserted, unpinned entry is a legal victim. Both call sites return an "admitted" result afterwards, so the return value means "was admitted", not "is resident". Establish one contract for mutators: either re-read the key after enforcement and downgrade the result, or exclude the key under admission from budget eviction for the duration of the call.
src/adapters/cursor/native-exec.ts#L302-L316: after line 315, checkblobs.has(k)and return{ admitted: false, reason: "pinned_saturation" }when the enforcer removed the entry. This site is the material one, because the blob key is embedded in the outgoing protobuf request and hydrated later, so a dangling key becomes a hydration miss mid-turn.src/adapters/anthropic-image-normalize.ts#L168-L173: confirm no caller treats thetrueat line 173 as proof of residency, and apply the same re-read guard if any caller re-reads the cache instead of using the value it already holds.
src/vision/index.tslines 406-412 has the same call shape but is already safe, because line 411 builds the resolved outcome from the localsuccessfulTextrather than from a cache read. That is the pattern the other two sites should match.🤖 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/adapters/cursor/native-exec.ts` around lines 302 - 316, The store mutators can report admission after enforcement has evicted the newly inserted entry. In src/adapters/cursor/native-exec.ts lines 302-316, re-check blobs.has(k) after reconcileBlobClassAccountingAndEnforce() and return admitted false with reason "pinned_saturation" when absent; otherwise preserve the successful result. In src/adapters/anthropic-image-normalize.ts lines 168-173, verify callers do not treat true as residency and add the same post-enforcement re-read guard if callers re-read the cache; no direct change is needed if the existing value is used directly. The already-safe src/vision/index.ts lines 406-412 requires no change.
🤖 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 `@src/adapters/cursor/native-exec.ts`:
- Around line 170-173: Decouple budget enforcement from the per-mutation
reconcile path: keep recomputeBlobClassAccounting in
reconcileBlobClassAccountingAndEnforce and the mutation flows setBlob,
releaseHydratedBlob, and releaseCursorBlobRequestScope, but schedule or invoke
enforceAppOwnedMemoryBudget once at a request boundary or coalesce bursts
through a microtask/short timer. Ensure repeated blob mutations trigger only one
retainedSnapshots pass while preserving process-wide budget enforcement.
In `@src/lib/app-owned-memory.ts`:
- Around line 164-189: Bound the synchronous eviction loop in
enforceAppOwnedMemoryBudget with a MAX_ENFORCEMENT_STEPS_PER_CALL limit,
stopping once the per-call cap is reached even if current.total remains above
budgetBytes. Preserve existing candidate selection and eviction behavior, and
rely on the existing registerStateSweepAfterTick periodic sweep to continue
enforcement until the budget converges.
In `@src/lib/debug-log-buffer.ts`:
- Around line 14-15: Move the createAdmissionGate import from "./admission" and
the enforceAppOwnedMemoryBudget import from "./app-owned-memory" into the
existing top-of-file import block, removing their duplicate trailing
declarations after resetDebugLogBufferForTests.
In `@src/responses/state.ts`:
- Around line 598-600: Update the byte-cap pruning loop in pruneResponses to use
the tracked oldestResidentId instead of materializing states with
[...states].find(...). Resolve the victim directly from that pointer, preserve
the existing fallback when no tracked resident exists, and keep the loop’s
eviction behavior unchanged.
In `@tests/codex-auth-context.test.ts`:
- Around line 603-628: Update the test “busy and stale Codex refresh failures do
not mark the auth-context account for reauth” to reuse
occupyCodexRefreshCapacity for creating and gating the 32 refreshes instead of
duplicating that setup. Extend the helper with an optional account-removal step
if needed, or invoke it and then save the test-specific account credential
separately; preserve the test’s existing assertions and scenario.
In `@tests/debug.test.ts`:
- Around line 168-185: Update the test “stale subscriber disposer cannot remove
a replacement registration” so two disposer closures share one registration by
subscribing the same listener twice before either disposer runs; invoke only one
disposer before creating the replacement, then invoke the other after
replacement creation to reach the registration-mismatch guard and retain the
existing assertions.
In `@tests/google-antigravity-replay.test.ts`:
- Line 217: Update the snapshot assertion in the replay test to require a
positive, non-zero bytes value in addition to count 512 and the existing fully
evictable relationship. Keep the evictableBytes comparison against
snapshot.bytes while adding an independent lower-bound assertion for
snapshot.bytes so zero byte accounting cannot pass.
In `@tests/state-store-sweeper.test.ts`:
- Around line 176-214: Wrap the timer-driven test body after the setInterval and
clearInterval spies are created in a finally block, moving
stopStateStoreSweeper() and both mockRestore() calls into that block. Preserve
the existing assertions and timer behavior while ensuring cleanup runs when any
assertion or setup step fails, matching the teardown pattern used by the
neighboring test.
---
Outside diff comments:
In `@src/adapters/cursor/native-exec.ts`:
- Around line 302-316: The store mutators can report admission after enforcement
has evicted the newly inserted entry. In src/adapters/cursor/native-exec.ts
lines 302-316, re-check blobs.has(k) after
reconcileBlobClassAccountingAndEnforce() and return admitted false with reason
"pinned_saturation" when absent; otherwise preserve the successful result. In
src/adapters/anthropic-image-normalize.ts lines 168-173, verify callers do not
treat true as residency and add the same post-enforcement re-read guard if
callers re-read the cache; no direct change is needed if the existing value is
used directly. The already-safe src/vision/index.ts lines 406-412 requires no
change.
🪄 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: b98bc42b-6585-42da-8e56-fc5dfb1fbe8d
📒 Files selected for processing (59)
devlog/_plan/260801_zero_leak_state_stores/006_roadmap_audit_synthesis.mddevlog/_plan/260801_zero_leak_state_stores/040_app_bytes_observability.mddocs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdgui/tests/apikeys-workspace.test.tsxgui/tests/usage-layout.test.tssrc/adapters/anthropic-image-normalize.tssrc/adapters/cursor/native-exec.tssrc/adapters/google-antigravity-replay.tssrc/claude/inbound-debug.tssrc/codex/model-cache.tssrc/config.tssrc/lib/app-owned-memory-stores.tssrc/lib/app-owned-memory.tssrc/lib/crash-guard.tssrc/lib/debug-log-buffer.tssrc/lib/gcp-adc.tssrc/lib/injection-debug-log.tssrc/lib/state-store-registrations.tssrc/lib/state-store-sweeper.tssrc/lib/windows-secret-acl.tssrc/oauth/index.tssrc/oauth/store.tssrc/responses/state.tssrc/server/index.tssrc/server/lifecycle.tssrc/server/management-auth.tssrc/server/management/config-routes.tssrc/server/management/logs-usage-routes.tssrc/server/management/system-routes.tssrc/server/management/usage-summary-cache.tssrc/server/request-log.tssrc/types.tssrc/usage/summary.tssrc/vision/index.tsstructure/02_config-and-codex-home.mdstructure/05_gui-and-management-api.mdtests/app-owned-memory.test.tstests/cli-headless-parity.test.tstests/codex-auth-context.test.tstests/config.test.tstests/cursor-blob.test.tstests/cursor-mcp-manager.test.tstests/debug.test.tstests/gcp-adc.test.tstests/google-antigravity-replay.test.tstests/memory-watchdog.test.tstests/oauth-refresh.test.tstests/responses-state.test.tstests/server-management-auth.test.tstests/settings-stream-mode.test.tstests/shutdown-drain.test.tstests/state-store-sweeper.test.tstests/usage-log.test.tstests/usage-summary.test.tstests/windows-secret-acl.test.ts
| function reconcileBlobClassAccountingAndEnforce(): void { | ||
| recomputeBlobClassAccounting(); | ||
| enforceAppOwnedMemoryBudget(); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Every blob mutation now snapshots all twelve registered retained stores, which compounds the per-mutation scan cost already flagged here.
reconcileBlobClassAccountingAndEnforce pairs recomputeBlobClassAccounting with enforceAppOwnedMemoryBudget. The second call is not cheap. enforceAppOwnedMemoryBudget calls retainedSnapshots(), which invokes the snapshot() callback of every entry in APP_OWNED_RETAINED_STORE_REGISTRATIONS (src/lib/app-owned-memory-stores.ts lines 71-144): request log, four debug rings, the crash ring, five caches, cursor blobs, and continuation state.
This helper runs on four paths, three of which are request-scoped: setBlob (line 315), releaseHydratedBlob (line 214), and releaseCursorBlobRequestScope (line 370). As noted in the earlier review of lines 147-190, src/adapters/cursor/protobuf-request.ts calls storeCursorBlob once per root prompt, per conversation turn, per user message, and per assistant or tool step, so a history-heavy request produces hundreds of mutations. Each one now performs two full blob-store scans, timer churn, and a twelve-store snapshot pass.
The budget is a process-wide invariant, not a per-insert one. Decouple the two concerns:
- Keep
recomputeBlobClassAccountingon mutation paths, ideally incremental as suggested previously. - Call
enforceAppOwnedMemoryBudget()once per request boundary, or coalesce it behind a microtask or short timer so a burst of mutations triggers one enforcement pass instead of hundreds.
The work is bounded, so this is not a stability risk. It is avoidable latency on request preparation.
Also applies to: 187-187, 214-214, 370-370
🤖 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/adapters/cursor/native-exec.ts` around lines 170 - 173, Decouple budget
enforcement from the per-mutation reconcile path: keep
recomputeBlobClassAccounting in reconcileBlobClassAccountingAndEnforce and the
mutation flows setBlob, releaseHydratedBlob, and releaseCursorBlobRequestScope,
but schedule or invoke enforceAppOwnedMemoryBudget once at a request boundary or
coalesce bursts through a microtask/short timer. Ensure repeated blob mutations
trigger only one retainedSnapshots pass while preserving process-wide budget
enforcement.
| function nextCandidate( | ||
| snapshots: ReadonlyMap<string, RetainedStoreSnapshot>, | ||
| ineligible: Set<string>, | ||
| ): RetainedStoreRegistration | undefined { | ||
| for (const category of CATEGORY_ORDER) { | ||
| let candidate: RetainedStoreRegistration | undefined; | ||
| let candidateAt = Number.POSITIVE_INFINITY; | ||
| for (const registration of retainedStores.values()) { | ||
| if (registration.category !== category || ineligible.has(registration.id)) continue; | ||
| const snapshot = snapshots.get(registration.id); | ||
| if (!snapshot || snapshot.evictableBytes <= 0) continue; | ||
| const oldestAt = snapshot.oldestAt; | ||
| if (oldestAt === null || !Number.isFinite(oldestAt)) { | ||
| enforcementCounters.oldestAtContractViolations += 1; | ||
| ineligible.add(registration.id); | ||
| continue; | ||
| } | ||
| if (!candidate || oldestAt < candidateAt) { | ||
| candidate = registration; | ||
| candidateAt = oldestAt; | ||
| } | ||
| } | ||
| if (candidate) return candidate; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Unbounded eviction loop can synchronously stall the event loop under large overage.
enforceAppOwnedMemoryBudget's while (current.total > budgetBytes) loop (Lines 219-247) has no cap on iterations. Each iteration:
- Calls
nextCandidate, which scans every registered store per category (Lines 168-185). - Calls
candidate.evictOldest(), which — perevictOldestCursorBlobForBudgetinsrc/adapters/cursor/native-exec.ts(shown in this batch's graph context) — does its own O(n) linear scan over the store to find the single oldest entry, then removes exactly one entry.
Driving a large overage down to budget therefore costs O(k × n) work, where k is the number of entries that must be evicted and n is average store size. This function is called synchronously and inline from many hot paths reviewed in this batch: injectionDebugLog, appendDebugLogLine, captureClaudeInbound, instrumentFetch, and retainRequestLogEntry. A runtime budget decrease via configureAppOwnedMemoryBudget() (for example, through the /api/settings limit mentioned in the PR description), or a burst of retained state accumulated before the first enforcement pass, can trigger tens of thousands of these O(n) scans in a single synchronous call, stalling the single-threaded Bun event loop for every in-flight request.
Consider bounding the number of evictions performed per call (for example, a MAX_ENFORCEMENT_STEPS_PER_CALL constant) and relying on the periodic sweep (registerStateSweepAfterTick, already wired in src/lib/app-owned-memory-stores.ts) to finish the remainder across subsequent ticks, while still guaranteeing eventual convergence to budget.
Also applies to: 209-253
🤖 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/lib/app-owned-memory.ts` around lines 164 - 189, Bound the synchronous
eviction loop in enforceAppOwnedMemoryBudget with a
MAX_ENFORCEMENT_STEPS_PER_CALL limit, stopping once the per-call cap is reached
even if current.total remains above budgetBytes. Preserve existing candidate
selection and eviction behavior, and rely on the existing
registerStateSweepAfterTick periodic sweep to continue enforcement until the
budget converges.
| interface DebugSubscriberRegistration { lease: AdmissionLease } | ||
| const listeners = new Map<(entry: DebugLogEntry) => void, DebugSubscriberRegistration>(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
New imports appear at the end of the file instead of the top.
The import { createAdmissionGate, ... } from "./admission"; and import { enforceAppOwnedMemoryBudget } from "./app-owned-memory"; statements land after resetDebugLogBufferForTests, at the end of the file rather than alongside the existing top-of-file imports. ES module import hoisting means this does not change behavior, but it is unconventional and likely an artifact of how the patch was applied. Move both statements into the existing import block at the top of the file for readability.
Also applies to: 82-83
🤖 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/lib/debug-log-buffer.ts` around lines 14 - 15, Move the
createAdmissionGate import from "./admission" and the
enforceAppOwnedMemoryBudget import from "./app-owned-memory" into the existing
top-of-file import block, removing their duplicate trailing declarations after
resetDebugLogBufferForTests.
| while (storedResponseBytes > byteCap() && states.size > 0) { | ||
| const oldestResident = [...states].find(([, entry]) => entry.kind === "resident"); | ||
| const oldestId = oldestResident?.[0] ?? states.keys().next().value as string | undefined; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
The byte-cap victim scan rebuilds the whole store into an array on every iteration, and it ignores the oldestResidentId this PR just added.
Line 599 evaluates [...states].find(...) inside the while loop at line 598. Each iteration allocates a fresh array of [key, value] pairs for every entry in states, so a prune that must demote k resident rows performs k full O(states) materializations. MAX_STORED_RESPONSES is 1,000, and pruneResponses runs on the request path from setResidentEntry, so a lowered cap or a burst turns this into avoidable O(n·k) allocation churn during request preparation.
There is also a semantic mismatch. Lines 115-124 and 141-146 now maintain oldestResidentId / oldestResidentAt, keyed on createdAt, and evictOldestResponseContinuationForBudget at line 631 uses exactly that pointer to choose its victim. This loop instead picks the first resident row in Map insertion order. The two orders usually agree, because replaceMapEntry deletes and re-inserts with a refreshed createdAt, but they are not guaranteed to agree, and two code paths in the same module should not disagree about which entry is "oldest".
Reuse the tracked pointer. It removes the per-iteration allocation and makes both eviction paths select the same victim.
♻️ Proposed refactor: select the victim from the tracked oldest-resident pointer
while (storedResponseBytes > byteCap() && states.size > 0) {
- const oldestResident = [...states].find(([, entry]) => entry.kind === "resident");
- const oldestId = oldestResident?.[0] ?? states.keys().next().value as string | undefined;
+ // Prefer the tracked oldest resident so this path and
+ // evictOldestResponseContinuationForBudget agree on the victim, and so the
+ // loop does not materialize the whole store on every iteration.
+ const oldestId = oldestResidentId ?? states.keys().next().value as string | undefined;
if (!oldestId) break;Note that the synchronous writeResponseSpillDurably fsync cost inside this same loop was raised in an earlier review and is unchanged; this comment is only about the victim selection.
📝 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.
| while (storedResponseBytes > byteCap() && states.size > 0) { | |
| const oldestResident = [...states].find(([, entry]) => entry.kind === "resident"); | |
| const oldestId = oldestResident?.[0] ?? states.keys().next().value as string | undefined; | |
| while (storedResponseBytes > byteCap() && states.size > 0) { | |
| // Prefer the tracked oldest resident so this path and | |
| // evictOldestResponseContinuationForBudget agree on the victim, and so the | |
| // loop does not materialize the whole store on every iteration. | |
| const oldestId = oldestResidentId ?? states.keys().next().value as string | undefined; | |
| if (!oldestId) break; |
🤖 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/responses/state.ts` around lines 598 - 600, Update the byte-cap pruning
loop in pruneResponses to use the tracked oldestResidentId instead of
materializing states with [...states].find(...). Resolve the victim directly
from that pointer, preserve the existing fallback when no tracked resident
exists, and keep the loop’s eviction behavior unchanged.
| test("busy and stale Codex refresh failures do not mark the auth-context account for reauth", async () => { | ||
| const originalFetch = globalThis.fetch; | ||
| let releaseBusy!: () => void; | ||
| const busyGate = new Promise<void>(resolve => { releaseBusy = resolve; }); | ||
| let busyFetches = 0; | ||
| globalThis.fetch = (async () => { | ||
| busyFetches += 1; | ||
| await busyGate; | ||
| return Response.json({ access_token: "busy-fresh", expires_in: 3600 }); | ||
| }) as typeof fetch; | ||
|
|
||
| const admitted: Promise<unknown>[] = []; | ||
| let clock: ReturnType<typeof spyOn> | undefined; | ||
| try { | ||
| for (let index = 0; index < 32; index += 1) { | ||
| const id = `busy-${index}`; | ||
| saveCodexAccountCredential(id, { | ||
| accessToken: `old-${index}`, | ||
| refreshToken: `refresh-${index}`, | ||
| expiresAt: 0, | ||
| chatgptAccountId: `account-${index}`, | ||
| }); | ||
| admitted.push(getValidCodexToken(id)); | ||
| } | ||
| await Promise.resolve(); | ||
| expect(busyFetches).toBe(32); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reuse occupyCodexRefreshCapacity instead of duplicating the 32-account saturation setup.
Lines 614-628 rebuild the same pattern that occupyCodexRefreshCapacity (Lines 131-159) already implements: create 32 accounts, gate their fetches, wait for all 32 to fire. The later test at Line 682 calls the shared helper directly for an equivalent scenario. Keeping two copies of this setup means a future change to the capacity constant or the gating mechanism must be updated in two places.
Extract the account-removal step as an optional helper parameter, or call occupyCodexRefreshCapacity here too and keep the pool-a-specific credential save as a follow-up step.
🤖 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/codex-auth-context.test.ts` around lines 603 - 628, Update the test
“busy and stale Codex refresh failures do not mark the auth-context account for
reauth” to reuse occupyCodexRefreshCapacity for creating and gating the 32
refreshes instead of duplicating that setup. Extend the helper with an optional
account-removal step if needed, or invoke it and then save the test-specific
account credential separately; preserve the test’s existing assertions and
scenario.
| test("stale subscriber disposer cannot remove a replacement registration", () => { | ||
| let seen = 0; | ||
| const listener = () => { seen += 1; }; | ||
| const staleStop = subscribeDebugLogEntries(listener); | ||
| staleStop(); | ||
| const replacementStop = subscribeDebugLogEntries(listener); | ||
| const before = debugBufferMetrics().subscribers; | ||
|
|
||
| staleStop(); | ||
| appendDebugLogLine("replacement remains active"); | ||
|
|
||
| expect(seen).toBe(1); | ||
| expect(debugBufferMetrics().subscribers).toMatchObject({ | ||
| active: before.active, | ||
| releaseMisses: before.releaseMisses, | ||
| }); | ||
| replacementStop(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test does not exercise the stale-vs-replacement guard it claims to cover.
Trace subscribeDebugLogEntries (src/lib/debug-log-buffer.ts:48-64): each disposer closure has its own disposed flag, checked before the listeners.get(listener) !== registration mismatch guard.
In this test, staleStop() runs at Line 172 (first call): disposed becomes true and registration1 is removed. replacementStop (Line 173) then creates registration2. When staleStop() runs again at Line 176, if (disposed) return; fires immediately, since disposed was already true from Line 172. The mismatch-guard line (listeners.get(listener) !== registration) is never reached. The releaseMisses assertion at Lines 180-183 therefore passes for a reason unrelated to the guard it is meant to validate.
To exercise the actual guard, get two disposers that share ONE registration (call subscribeDebugLogEntries(listener) twice before either unsubscribes — the second call reuses the existing registration), then unsubscribe through only one of them before creating the replacement:
🐛 Proposed fix for genuine stale-disposer coverage
test("stale subscriber disposer cannot remove a replacement registration", () => {
let seen = 0;
const listener = () => { seen += 1; };
- const staleStop = subscribeDebugLogEntries(listener);
- staleStop();
+ const firstStop = subscribeDebugLogEntries(listener);
+ const staleStop = subscribeDebugLogEntries(listener); // shares firstStop's registration
+ firstStop();
const replacementStop = subscribeDebugLogEntries(listener);
const before = debugBufferMetrics().subscribers;
- staleStop();
+ staleStop(); // never called before; must be rejected by the registration-mismatch guard
appendDebugLogLine("replacement remains active");This is the same coverage gap flagged in the previous review round (existing_comments lines 159-166) for the earlier version of this test: the assertion passes for reasons unrelated to the mechanism under test.
🤖 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/debug.test.ts` around lines 168 - 185, Update the test “stale
subscriber disposer cannot remove a replacement registration” so two disposer
closures share one registration by subscribing the same listener twice before
either disposer runs; invoke only one disposer before creating the replacement,
then invoke the other after replacement creation to reach the
registration-mismatch guard and retain the existing assertions.
| } finally { | ||
| values.mockRestore(); | ||
| } | ||
| expect(snapshot!).toMatchObject({ count: 512, evictableBytes: snapshot!.bytes }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The evictableBytes assertion compares the snapshot against its own field, so it cannot detect broken byte accounting.
toMatchObject({ count: 512, evictableBytes: snapshot!.bytes }) reads snapshot.bytes from the same object it asserts on. If replayBytes were mis-accounted and both bytes and evictableBytes came back as 0, the expectation would still pass, because 0 === 0 and count is independent. The test that this file's other cases rely on is exactly this accounting, so the strongest assertion here should not be self-referential.
Add a positive lower bound on bytes. This keeps the "fully evictable" relation while also pinning that 512 observed sessions produced real accounted bytes.
💚 Proposed fix: assert the byte total is non-zero
- expect(snapshot!).toMatchObject({ count: 512, evictableBytes: snapshot!.bytes });
+ expect(snapshot!.count).toBe(512);
+ expect(snapshot!.bytes).toBeGreaterThan(0);
+ expect(snapshot!.evictableBytes).toBe(snapshot!.bytes);
+ expect(snapshot!.pinnedBytes).toBe(0);📝 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.
| expect(snapshot!).toMatchObject({ count: 512, evictableBytes: snapshot!.bytes }); | |
| expect(snapshot!.count).toBe(512); | |
| expect(snapshot!.bytes).toBeGreaterThan(0); | |
| expect(snapshot!.evictableBytes).toBe(snapshot!.bytes); | |
| expect(snapshot!.pinnedBytes).toBe(0); |
🤖 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/google-antigravity-replay.test.ts` at line 217, Update the snapshot
assertion in the replay test to require a positive, non-zero bytes value in
addition to count 512 and the existing fully evictable relationship. Keep the
evictableBytes comparison against snapshot.bytes while adding an independent
lower-bound assertion for snapshot.bytes so zero byte accounting cannot pass.
| const timers: Array<() => void> = []; | ||
| const setSpy = spyOn(globalThis, "setInterval").mockImplementation(((callback: () => void) => { | ||
| timers.push(callback); | ||
| return { unref() {} }; | ||
| }) as typeof setInterval); | ||
| const clearSpy = spyOn(globalThis, "clearInterval").mockImplementation(() => {}); | ||
| let pinned = true; | ||
| let retained = 4; | ||
| registerStateStore({ | ||
| name: "class-transition", | ||
| sweepExpired: () => { pinned = false; return 0; }, | ||
| }); | ||
| registerRetainedStore({ | ||
| id: "sweep-transition", | ||
| category: "caches", | ||
| snapshot: () => ({ | ||
| count: retained > 0 ? 1 : 0, | ||
| bytes: retained, | ||
| evictableBytes: pinned ? 0 : retained, | ||
| pinnedBytes: pinned ? retained : 0, | ||
| oldestAt: !pinned && retained > 0 ? 1 : null, | ||
| }), | ||
| evictOldest: () => { | ||
| const released = retained; | ||
| retained = 0; | ||
| return released; | ||
| }, | ||
| }); | ||
| configureAppOwnedMemoryBudget(0); | ||
| registerAppOwnedMemorySweepFallback(); | ||
|
|
||
| startStateStoreSweeper({ intervalMs: 10, now: () => 42 }); | ||
| timers[0]!(); | ||
|
|
||
| expect(retained).toBe(0); | ||
| stopStateStoreSweeper(); | ||
| setSpy.mockRestore(); | ||
| clearSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Restore the timer spies in a finally block so one failed assertion cannot corrupt the rest of the file.
Lines 177-181 replace the global setInterval and clearInterval. Lines 211-213 undo that only on the success path. If expect(retained).toBe(0) at Line 210 fails, Bun throws before stopStateStoreSweeper() and both mockRestore() calls. The mocks then stay installed for every later test in this file, including the timer lifecycle tests described for Lines 140-214. Those tests would report unrelated failures, and the real cause would be hidden by the first failure.
The neighbouring test at Lines 287-331 already uses this pattern for its environment and cache teardown. Mirror it here.
♻️ Proposed fix: scope teardown to a finally block
configureAppOwnedMemoryBudget(0);
registerAppOwnedMemorySweepFallback();
- startStateStoreSweeper({ intervalMs: 10, now: () => 42 });
- timers[0]!();
-
- expect(retained).toBe(0);
- stopStateStoreSweeper();
- setSpy.mockRestore();
- clearSpy.mockRestore();
+ try {
+ startStateStoreSweeper({ intervalMs: 10, now: () => 42 });
+ timers[0]!();
+
+ expect(retained).toBe(0);
+ } finally {
+ stopStateStoreSweeper();
+ setSpy.mockRestore();
+ clearSpy.mockRestore();
+ }
});📝 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.
| const timers: Array<() => void> = []; | |
| const setSpy = spyOn(globalThis, "setInterval").mockImplementation(((callback: () => void) => { | |
| timers.push(callback); | |
| return { unref() {} }; | |
| }) as typeof setInterval); | |
| const clearSpy = spyOn(globalThis, "clearInterval").mockImplementation(() => {}); | |
| let pinned = true; | |
| let retained = 4; | |
| registerStateStore({ | |
| name: "class-transition", | |
| sweepExpired: () => { pinned = false; return 0; }, | |
| }); | |
| registerRetainedStore({ | |
| id: "sweep-transition", | |
| category: "caches", | |
| snapshot: () => ({ | |
| count: retained > 0 ? 1 : 0, | |
| bytes: retained, | |
| evictableBytes: pinned ? 0 : retained, | |
| pinnedBytes: pinned ? retained : 0, | |
| oldestAt: !pinned && retained > 0 ? 1 : null, | |
| }), | |
| evictOldest: () => { | |
| const released = retained; | |
| retained = 0; | |
| return released; | |
| }, | |
| }); | |
| configureAppOwnedMemoryBudget(0); | |
| registerAppOwnedMemorySweepFallback(); | |
| startStateStoreSweeper({ intervalMs: 10, now: () => 42 }); | |
| timers[0]!(); | |
| expect(retained).toBe(0); | |
| stopStateStoreSweeper(); | |
| setSpy.mockRestore(); | |
| clearSpy.mockRestore(); | |
| }); | |
| const timers: Array<() => void> = []; | |
| const setSpy = spyOn(globalThis, "setInterval").mockImplementation(((callback: () => void) => { | |
| timers.push(callback); | |
| return { unref() {} }; | |
| }) as typeof setInterval); | |
| const clearSpy = spyOn(globalThis, "clearInterval").mockImplementation(() => {}); | |
| let pinned = true; | |
| let retained = 4; | |
| registerStateStore({ | |
| name: "class-transition", | |
| sweepExpired: () => { pinned = false; return 0; }, | |
| }); | |
| registerRetainedStore({ | |
| id: "sweep-transition", | |
| category: "caches", | |
| snapshot: () => ({ | |
| count: retained > 0 ? 1 : 0, | |
| bytes: retained, | |
| evictableBytes: pinned ? 0 : retained, | |
| pinnedBytes: pinned ? retained : 0, | |
| oldestAt: !pinned && retained > 0 ? 1 : null, | |
| }), | |
| evictOldest: () => { | |
| const released = retained; | |
| retained = 0; | |
| return released; | |
| }, | |
| }); | |
| configureAppOwnedMemoryBudget(0); | |
| registerAppOwnedMemorySweepFallback(); | |
| try { | |
| startStateStoreSweeper({ intervalMs: 10, now: () => 42 }); | |
| timers[0]!(); | |
| expect(retained).toBe(0); | |
| } finally { | |
| stopStateStoreSweeper(); | |
| setSpy.mockRestore(); | |
| clearSpy.mockRestore(); | |
| } | |
| }); |
🤖 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/state-store-sweeper.test.ts` around lines 176 - 214, Wrap the
timer-driven test body after the setInterval and clearInterval spies are created
in a finally block, moving stopStateStoreSweeper() and both mockRestore() calls
into that block. Preserve the existing assertions and timer behavior while
ensuring cleanup runs when any assertion or setup step fails, matching the
teardown pattern used by the neighboring test.
|
I extracted the confirmed Windows ACL temp-path memo leak into focused PR #840. Please exclude/rebase out the overlapping ACL memo lifecycle changes if this branch is updated. One important difference: successful temp memos are not the only retained state. Timeout memos also need a stable destination |
…s bytes The freshness audit against the post-040 tree overturned half the plan's premises: streaming Chat/Claude output is emitted, not retained, so the budget now separates live-transient deltas from retained collector copies; Cursor args arrive cumulatively, so replacement-delta accounting replaces per-fragment charging; the backpressure story targets the real message queue and partial-frame buffer without touching the stricter 1024-event cap. Budgets become mandatory at the type level after the optional-param omission CodeRabbit flagged, the passthrough serialization and MCP payloads live on a new observation-only lease so neither acquires a second hard cap, and the 040 observed-buffer contract lands with its four static ids. Five audit rounds locked the amendments.
|
The confirmed Responses single-entry RAM-cap escape is now isolated in #841. Please exclude the overlapping state-admission portion when rebasing this branch. #841 intentionally uses atomic admission and a normal continuation miss for an individually >64 MiB entry. It does not adopt the spill-store/app-memory framework here, keeping this review limited to the reproduced hard-cap violation. |
|
The confirmed Antigravity replay slice is now isolated in #843. Please exclude the overlapping replay-cache changes when rebasing this branch. #843 avoids an app-wide dependency and avoids a full 10,240-session expiration scan on every observe/apply. It uses fixed-size identity digests, per-call/session/global limits, LRU eviction, exact-session expiry, and at-most-once-per-minute background sweeps unless under budget pressure. |
|
The confirmed Cursor blob-store slice is isolated in #845. Please exclude overlapping blob accounting/store changes on rebase. The focused implementation uses request leases: advertised blob ids stay pinned through every terminal path (including async-generator cancellation), while only unpinned entries are TTL/LRU evictable. It also stores root candidates only after pruning and returns a typed |
|
The confirmed tool-argument/SSE overlap has now been extracted into focused draft PR #847: 4 MiB SSE records, 8 MiB per call, 32 MiB per turn, truthful overflow failure, and no canonical event-contract redesign. It passes 206 related tests plus typecheck and privacy scan. Please treat #847 as the reviewable implementation for this slice rather than carrying the overlapping giant-PR version forward. This does not clear the existing CHANGES_REQUESTED decision on #829; its unrelated review findings and broad state-store architecture still need separate resolution. |
…n budget Every bidirectional translation path now draws from one required budget: 2 MiB per assembled tool call, 32 MiB per client turn across all charged copies, threaded from the ingress boundary through adapters, retries, image and web-search loops, and the outbound folds. Streaming deltas that are emitted rather than retained stay transient; genuine Responses passthrough and MCP payloads sit on an observation-only lease so neither acquires a second hard cap. The Cursor transport gains byte-based backpressure with frame-size admission while keeping the stricter 1024-event queue cap, and the image, OAuth, and Grok serialized tails get admission gates that reject before enqueue. The four translator observed ids now appear in the app-owned memory payload, completing the 040 integration contract.
…he CLI
The CLI already deferred the one-time star prompt to the user when an agent was
driving `ocx start`. That deferral was bypassable: `POST /api/github/star` had no
such guard, and a coding agent runs on the user's machine, where it can read
`~/.opencodex/admin-api-token` off disk. One curl starred the repository with the
user's GitHub identity, having asked nobody.
Holding the admin token proves the caller reached a secret, not that a person
chose to spend their own account. Gate the write on consent instead:
- The star POST now returns 403 `agent_consent_required` for agent-driven callers
and, decisively, never spawns `gh` — a refusal that still performed the write
would be theatre. The response names the command to run after a real yes.
- A dashboard click keeps working even when an agent started the proxy, which is
the common case: the person is at the browser, not at the spawning shell. The
click is recognized by its browser-session evidence (same-origin Origin plus the
minted CSRF/GUI-origin headers the auth gate already verified), not by the
proxy's own environment.
- Agent detection widens from 11 vars to 23 (Cline, Roo, Windsurf, opencode,
Gemini CLI, GitLab/Buildkite/Jenkins/TeamCity/Codespaces). A false positive only
postpones a prompt; a false negative lets an agent answer for the user.
`CODEX_HOME` is deliberately excluded — users export it from their own shell
profile, so matching it would silence the prompt for a person at the keyboard.
- The CLI deferral now states the rule an agent tends to rationalize past: do not
infer consent from the surrounding task.
Known edge, failing closed: a non-loopback operator dashboard authenticates with
the raw admin token and sends no CSRF header, so if that proxy was also started
from an agent shell the button is refused and points at the `gh` one-liner. A
service-run proxy carries OCX_SERVICE and is not agent-driven, so the usual
remote setup is unaffected.
Documented in AGENTS.md as a general class ("user-consent actions") rather than a
one-off, plus README and the English/Korean dashboard guides.
Coverage: the route guard was driven red by neutralizing its condition, which
failed exactly the refusal test and nothing else. typecheck, privacy:scan,
lint:gui, and the full suite (6749 pass / 0 fail) are green.
…s them The freshness audit rebuilt the shell-lifecycle plan on what the tree now provides: admission comes from the shared 035 gate with its lease held in the registry entry, rejection speaks the protobuf spawn-error schema the wire already uses, and ownership survives a failed kill instead of vanishing with it. The decisive correction is at shutdown — draining turns cannot prevent a late spawn because queued Cursor frames and fresh WebSocket leases outlive the abort, so a synchronous spawn fence now precedes the global drain snapshot by construction. Three audit rounds locked the amendments.
There was a problem hiding this comment.
Actionable comments posted: 40
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/adapters/cursor/live-transport.ts (1)
709-727: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the finalize timer against a budget throw from
push.
pushnow callsthis.reserveTransportBytes(bytes)at Line 522, which throwsTranslatorBudgetExceededErrorwhen the buffered total would exceedCURSOR_TRANSPORT_MAX_BUFFERED_BYTES. Thedatahandler wraps itspushpath in try/catch at Lines 885-898, and the frame chain catches at Line 868. ThesetTimeoutcallback at Lines 714-726 callspush(event)at Line 719 with no guard. A throw inside a timer callback has no caller to catch it and terminates the Bun process instead of failing the turn.Wrap the terminal emission and route the error through
failAndClear-equivalent handling.🛡️ Suggested guard
this.pendingFinalize = setTimeout(() => { this.pendingFinalize = undefined; if (this.expectedClose) return; - const terminal = finalizeAfterDrain(state); - if (terminal.length === 0) return; - for (const event of terminal) push(event); + try { + const terminal = finalizeAfterDrain(state); + if (terminal.length === 0) return; + for (const event of terminal) push(event); + } catch { + this.cancelCursorRun(); + return; + }🤖 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/adapters/cursor/live-transport.ts` around lines 709 - 727, Guard the terminal event emission in scheduleClientToolFinalize against errors from push, including TranslatorBudgetExceededError. Wrap the terminal loop and subsequent finalize work in try/catch, and route any failure through the same fail-and-clear handling used by the data and frame paths, ensuring the timer callback never allows an exception to escape and crash the process.tests/cursor-tool-finalize-race.test.ts (1)
67-87: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPass the same budget into
createCursorProtobufEventStateso the harness matches production.Line 70 supplies a test budget to
createLiveCursorTransport, but Line 87 creates the event state withouttranslatorBudget.state.translatorBudgetis thereforeundefined, and everyopenCall,reserveTransient, andcloseCallcall added insrc/adapters/cursor/protobuf-events.tsis skipped through optional chaining. The finalize-race path is exactly where budget call lifecycles can leak, so this harness now exercises less than production does. Share one budget between the transport and the state.🧪 Proposed fix
function makeHarness(graceMs: number, clientToolNames: string[]): Harness { + const translatorBudget = createTestTranslatorBudget(); const transport = createLiveCursorTransport({ provider: { adapter: "cursor", baseUrl: "https://api2.cursor.sh", apiKey: "test-token" }, - translatorBudget: createTestTranslatorBudget(), + translatorBudget, headers: new Headers(), clientToolFinalizeGraceMs: graceMs, }) as unknown as { @@ - const state = createCursorProtobufEventState({ clientToolNames }); + const state = createCursorProtobufEventState({ clientToolNames, translatorBudget });🤖 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/cursor-tool-finalize-race.test.ts` around lines 67 - 87, Update makeHarness to create one shared translator budget and pass it to both createLiveCursorTransport and createCursorProtobufEventState via translatorBudget. Ensure the state receives the same budget instance used by the transport so finalize-race tests exercise the production budget lifecycle.src/adapters/openai-responses.ts (1)
1049-1108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the collector charges if the SSE loop throws.
The
for awaitloop at Line 1059 can throwTranslatorBudgetExceededError, both fromdecodeServerSentEventsand fromreserveTransientat Line 1068, Line 1078, and Line 1095. The single release at Line 1108 sits after the loop, so a throw skips it and leaves the currentdeltas,doneText, andsnapshotbytes charged. Wrap the loop intry { ... } finally { ... }and perform the release in thefinallyblock. That also removes the duplicated release expression.src/bridge.ts (1)
1309-1320: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRelease the charged
tool_search_sourcesbytes when batch annotations are built.Line 1525 charges each unique web-search source with
budget?.chargeRetained(bytesOf(JSON.stringify(s)), { kind: "tool_search_sources" }).flushTextconvertspendingWebSourcesintoannotationsat Lines 1309-1311 and clears the array at Line 1312, but it never releases those bytes.pushOutput(item, currentTextBytes)at Line 1318 releases onlycurrentTextBytesunder"retained_collectors".The streaming counterpart does perform the release:
takeWebAnnotationsat Lines 420-425 callsbudget?.releaseRetained(sourceBytes, { kind: "tool_search_sources" }). The batch path is missing that step, sobuildResponseJSONover-reports retained bytes for every web-search turn. On a turn with several searches the inflated total can throwTranslatorBudgetExceededErroron a later buffer and fail a healthy non-streaming response.Mirror the streaming release.
🐛 Proposed fix for the batch source-byte release
const annotations = pendingWebSources.map(s => ({ type: "url_citation", url: s.url, ...(s.title ? { title: s.title } : {}), start_index: 0, end_index: 0, })); + const sourceBytes = pendingWebSources.reduce((sum, source) => sum + bytesOf(JSON.stringify(source)), 0); pendingWebSources = []; + if (sourceBytes > 0) budget?.releaseRetained(sourceBytes, { kind: "tool_search_sources" });🤖 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/bridge.ts` around lines 1309 - 1320, Update the batch annotation construction in flushText to release each pendingWebSources entry’s charged bytes with budget?.releaseRetained(..., { kind: "tool_search_sources" }) before clearing the array. Mirror the existing takeWebAnnotations release behavior and preserve the current annotation creation and pushOutput flow.src/adapters/google.ts (2)
539-560: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winFull-buffer re-encoding on every chunk is O(n²) and hits the pathological case for large inline images.
At Line 540,
budgetEncoder.encode(nextBuffer).byteLengthre-encodes the ENTIRE accumulatedbuffer(not just the newly-decoded chunk) every timereader.read()returns. When no newline has been seen yet,bufferkeeps growing across successive small chunk reads (up toMAX_SSE_FRAME_BYTES= 100 MiB), so total encode work becomes quadratic in the final buffer size. At Line 556,budgetEncoder.encode(buffer).byteLengthrepeats essentially the same encode of the whole buffer again when no newline was found (lines.pop()returns the unmodified full buffer in that case), doubling the redundant work per chunk.This is not just theoretical: Gemini responses can carry large base64
inlineDatapayloads inside a singledata:SSE line (see theinline.datahandling a few lines below), so a multi-megabyte single-line frame is a realistic, not contrived, scenario. Accumulating a 100 MiB frame in 64 KiB reads would perform on the order of gigabytes of redundant UTF-8 encoding work before theMAX_SSE_FRAME_BYTEScap even trips.Track
bufferBytesincrementally instead of re-encoding the whole buffer each time.⚡ Proposed fix: track bytes incrementally instead of re-encoding the full buffer
- const nextBuffer = buffer + decoder.decode(value, { stream: true }); - const nextBufferBytes = budgetEncoder.encode(nextBuffer).byteLength; + const decoded = decoder.decode(value, { stream: true }); + const decodedBytes = budgetEncoder.encode(decoded).byteLength; + const nextBuffer = buffer + decoded; + const nextBufferBytes = bufferBytes + decodedBytes; const appendReservation = budget.reserveTransient(nextBufferBytes, { kind: "live_transient" }); buffer = nextBuffer; appendReservation.commitRetained(); budget.releaseRetained(bufferBytes, { kind: "live_transient" }); bufferBytes = nextBufferBytes; ... const lines = buffer.split("\n"); - buffer = lines.pop() ?? ""; - const residualBytes = budgetEncoder.encode(buffer).byteLength; + const previousBuffer = buffer; + buffer = lines.pop() ?? ""; + const residualBytes = buffer === previousBuffer + ? bufferBytes + : budgetEncoder.encode(buffer).byteLength; const residualReservation = budget.reserveTransient(residualBytes, { kind: "live_transient" }); residualReservation.commitRetained(); budget.releaseRetained(bufferBytes, { kind: "live_transient" }); bufferBytes = residualBytes;🤖 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/adapters/google.ts` around lines 539 - 560, Update the SSE buffering logic around the chunk-append and residual-buffer handling to track UTF-8 byte counts incrementally rather than calling budgetEncoder.encode on the entire accumulated buffer. Add the decoded chunk’s byte length to bufferBytes when appending, then adjust bufferBytes only for bytes removed when complete lines are split out; preserve the existing transient reservation, commit, release, MAX_SSE_FRAME_BYTES limit, and parsing behavior.
636-684: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
parseResponseleakschargeRetainedbytes on the size-limit abort path, and does not map budget-exceeded errors to the same typed error asparseStream.Two related gaps compared to
parseStream(Lines 608-621), which handles both correctly:
- Lines 645-651: on
total > MAX_RESPONSE_BYTES, the function returns immediately, but every prior loop iteration already calledbudget.chargeRetained(value.byteLength, { kind: "retained_collectors" })for the chunks pushed so far (Line 649). None of that charge is ever released before the earlyreturn. If thisbudgetinstance is reused across retry attempts (this file hasfetchVertexWithRetry/fetchAntigravityWithRetry), a leaked charge from a failed attempt can wrongly count against a subsequent retry's budget check.- Lines 670-672: the generic
catch (err)does not checkisTranslatorBudgetExceededError(err). Ifbudget.reserveTransient(...)throws (Lines 656, 667, 678) because the retained cap was hit, this path returns a generic error message instead of the502/translation_buffer_limittyped error thatparseStreamproduces for the identical condition (Lines 611-617). Callers that branch oncode === "translation_buffer_limit"for HTTP 413 mapping will not see it here.Track the running charged-bytes total and release it on every exit path (including the size-limit return and the catch), and add an explicit
isTranslatorBudgetExceededErrorbranch that mirrorsparseStream's typed error.🩹 Proposed fix: track and release charged bytes, and add the typed-error branch
const chunks: Uint8Array[] = []; let total = 0; + let chargedBytes = 0; try { for (;;) { const { done, value } = await reader.read(); if (done) break; total += value.byteLength; if (total > MAX_RESPONSE_BYTES) { await reader.cancel().catch(() => {}); + budget.releaseRetained(chargedBytes, { kind: "retained_collectors" }); return [{ type: "error", message: `google response too large (exceeded ${MAX_RESPONSE_BYTES} bytes)` }]; } budget.chargeRetained(value.byteLength, { kind: "retained_collectors" }); + chargedBytes += value.byteLength; chunks.push(value); } } finally { try { await reader.cancel(); } catch { /* ignore */ } reader.releaseLock(); } ... } catch (err) { + if (isTranslatorBudgetExceededError(err)) { + return [{ + type: "error", + status: 502, + errorType: "upstream_error", + code: "translation_buffer_limit", + message: "upstream translation buffer exceeded the safe limit", + }]; + } return [{ type: "error", message: err instanceof Error ? err.message : "failed to read google response body" }]; }🤖 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/adapters/google.ts` around lines 636 - 684, Update parseResponse to track the cumulative bytes passed to budget.chargeRetained and release that exact amount on the MAX_RESPONSE_BYTES early return and all body-reading error paths, avoiding leaks across retries. In parseResponse’s catch around body collection and reservation, check isTranslatorBudgetExceededError(err) first and return the same typed 502/translation_buffer_limit error shape used by parseStream; retain the existing generic error handling for other failures.src/adapters/kiro.ts (1)
900-946: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease
retained_collectorsbytes when Kiro flushes buffered events.defer()chargesdeferredevents, butdeferred.splice(0)only removes their array references.fallbackEventshas the same issue when it is yielded. The shared budget is reused for the fallback attempt, so these charges remain inhardChargedBytesand can triggertranslation_buffer_limitduring the same turn. Track each collector's bytes and release them when events leave the collector, including transformed fallback events, or transfer ownership through the shared event-batch utility.🤖 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/adapters/kiro.ts` around lines 900 - 946, Release retained_collectors budget bytes when buffered events leave the Kiro collectors: update defer and fallbackEvents handling around stage so deferred.splice(0) and fallback batches also remove their associated charges, including transformed phased events. Track charged byte totals per collector or use the shared event-batch ownership utility, ensuring all yielded or flushed events transfer or release ownership before the fallback attempt reuses the budget.
🤖 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/_plan/260801_zero_leak_state_stores/050_translator_stream_bounds.md`:
- Around line 144-159: Update TranslatorBudget.openCall and its call sites to
admit bytes for each retained tool-call identifier and call-map entry before
inserting a new call, including zero-argument calls. Track the charged amount
per call and release it in closeCall and all terminal cleanup paths, preserving
existing argument-fragment accounting. Add a regression covering many distinct
zero-argument calls and verifying the call/turn caps are enforced.
- Around line 520-524: Update the Grok apply table cell containing
grokApplyFlight so the union pipe in “{ startedAt, promise } | null” is escaped
or rendered inside an HTML code element, preserving the full acceptance contract
as a single Markdown table cell.
In `@README.md`:
- Around line 373-374: Update src/cli/star-prompt.ts so the MARKER is written
only after interactiveConfirm() returns an affirmative result, and add a
regression test verifying declined prompts leave no marker. Preserve or align
the documented no-persistence behavior in README.md lines 373-374,
docs-site/src/content/docs/guides/web-dashboard.md lines 135-136, and
docs-site/src/content/docs/ko/guides/web-dashboard.md lines 113-114; these
documentation sites require no wording change if the implementation matches
them.
In `@src/adapters/anthropic.ts`:
- Around line 928-939: Update the budget-overflow branch in the parseStream
catch block to mark the generator as terminal before yielding the
translation_buffer_limit error, then return immediately after yielding it.
Preserve the existing rethrow behavior for non-budget errors and ensure the
finally cleanup still executes without allowing the subsequent terminal-event
logic to run.
In `@src/adapters/cursor/kv-store.ts`:
- Around line 27-34: Update get in the cursor key-value store to stop charging
and asynchronously releasing retained bytes for reads. Return the cloned value
directly, relying on the existing retention charge applied when the value is
seeded or set, so reading an existing entry cannot throw from chargeRetained or
leave transient accounting for the caller’s copy.
In `@src/adapters/cursor/protobuf-events.ts`:
- Line 370: Update finalizeTurnEvents and failAndClear to close every open
translator-budget call via closeCall before clearing state.openToolCalls,
covering normal completion, truncation, stream errors, and abort/cancellation
paths. Add regression tests for truncation and cancellation to verify no
translator-budget calls remain open.
In `@src/adapters/kiro.ts`:
- Around line 1096-1107: Update the open tool-call tracking record used by the
streaming logic to maintain an optional callBytes counter. In the delta-handling
path around open.chunks, compute each ev.input byte length once, derive
nextCallBytes from the running counter, and update callBytes after committing
the reservation; remove the repeated open.chunks.join("") encoding while
preserving the existing reservation and release behavior.
In `@src/adapters/mimo-free.ts`:
- Around line 205-206: Update buildRequest to pass incoming.abortSignal into
getMimoJwt when starting JWT bootstrap, preserving the existing retry-path
signal handling and ensuring the bootstrap request observes client cancellation.
- Line 210: Update the base request assignment in the adapter’s buildRequest
flow to await base.buildRequest(parsed, incoming) instead of using the as
AdapterRequest cast, preserving the existing JSON parsing and request
construction behavior.
In `@src/adapters/openai-chat.ts`:
- Around line 814-818: Reorder the new-call handling around the
`pendingToolCalls` registration: create the call, invoke
`budget.openCall(call.key)` first, and only push it into `pendingToolCalls`
after that succeeds. Preserve the existing key generation and call
initialization, ensuring failed budget admission leaves no stale entry for
cleanup or `flushToolCalls`.
- Around line 870-876: Update the post-split accounting in the stream buffering
logic around buffer and bufferBytes to release only the consumed prefix instead
of reserving and committing residualBytes. Remove the residualReservation flow,
calculate the retained-byte decrease from bufferBytes and residualBytes, release
that consumed amount as live_transient, and set bufferBytes to residualBytes
without introducing a transient peak or requiring an additional reservation.
- Around line 948-952: Update parseResponse to read the response body as text
once, calculate its byte size from that text, and call budget.chargeRetained
before parsing it. Move JSON.parse into the existing try block so malformed
bodies still trigger the finally cleanup, while preserving the current
AdapterEvent processing.
In `@src/adapters/openai-responses.ts`:
- Around line 1112-1117: Update parseResponse to release the retained payload
charge on every success and error path by wrapping the payload-processing logic
in a finally block, matching the cleanup pattern used by the Anthropic adapter.
Keep the malformed JSON response behavior unchanged and ensure the charge
created by budget.chargeRetained is always released before parseResponse
returns.
In `@src/bridge.ts`:
- Around line 348-372: Update takeReasoningEnvelope to release
hiddenThinkingBytes through budget.releaseRetained with kind "reasoning" before
returning when no signature or redacted reasoning exists. Preserve the existing
envelope construction and reset behavior, ensuring the empty-envelope path
releases charged hidden thinking bytes even when flushHiddenReasoningEnvelope
receives no encrypted value.
In `@src/chat/outbound.ts`:
- Around line 170-183: Drain queuedLiveFrameBytes via releaseDeliveredFrame on
every terminal path in src/chat/outbound.ts, including the fail and successful
close paths, while preserving the existing cancel drain; also drain it in the
finally block of src/claude/outbound.ts alongside buffer cleanup before
controller.close() can complete. If each request explicitly disposes its
TranslatorBudget instead, document that contract in both enqueue helpers rather
than adding the drains.
In `@src/claude/outbound.ts`:
- Around line 474-488: Remove the redundant translation_buffer_limit ternary
from the status calculation in the outbound error-handling flow, since that case
already exits by throwing TranslatorBudgetExceededError. Set status directly
from numeric error.status with the existing 500 fallback, preserving the
subsequent fail behavior.
In `@src/lib/app-owned-memory-stores.ts`:
- Around line 45-48: Move the Grok apply-flight state and
grokApplyFlightSnapshot implementation out of the route module into a low-level
module such as grok/apply-flight.ts. Update agent-settings-routes to import and
use that shared module, and update app-owned-memory-stores to import the
snapshot from it instead of src/server/management/agent-settings-routes,
preserving the existing registry behavior.
In `@src/lib/sse-decoder.ts`:
- Around line 170-177: Update acceptLine’s event and data handling branches so
retained line bytes are released even when reserveTransient throws. Use the
local retainedLineBytes value to perform release in a catch/finally path, or
restore lineRetainedBytes before propagating the error, while preserving
successful reservation and commit behavior.
In `@src/lib/translator-budget.ts`:
- Around line 101-105: Replace the strong Budget registry in liveBudgets with
weak references so undisposed budgets can be garbage-collected. Update
createTranslatorBudget to register weak references, and update dispose and
resetTranslatorAggregateForTests to dereference entries, remove stale
references, and preserve cleanup/reset behavior for live budgets.
In `@src/server/chat-completions.ts`:
- Line 86: Remove the eager retained-body charge and discarded JSON
serialization near translatorBudget.chargeRetained, then charge the translated
request body exactly once at the point its retained serialization is created
near the later internalBody handling. Measure its size with Buffer.byteLength
rather than TextEncoder and preserve the existing request_copies accounting
without retaining a duplicate charge.
- Around line 61-72: Update the response lifecycle in runAdmittedHttpTurn and
withCors so every response branch, including the branches around lines 263-265,
295-301, and 310, disposes the associated translator budget when the body is
abandoned rather than drained or canceled. Reuse the existing
finalizeTranslatorBudgetResponse lifecycle behavior where possible, or add a
safe bounded reaper, while preserving normal pull/cancel cleanup and the current
error-path disposal in the shown handler.
In `@src/server/claude-messages.ts`:
- Line 605: Remove the duplicate request_copies charge around the translated
request construction: ensure internalBody is charged exactly once for the
serialization retained by the Request created near internalBodyJson, using
Buffer.byteLength rather than allocating a TextEncoder Uint8Array. Update the
charge logic at translatorBudget.chargeRetained and the internalBodyJson/request
creation flow, preserving any required pre-header budget bound without leaving
both charges on hardChargedBytes.
- Around line 874-880: Update the error handling around readAnthropicBody in the
count_tokens flow to catch TranslatorBudgetExceededError before the generic
fallback and return HTTP 413 with the same translation_buffer_limit response
semantics used by handleClaudeMessages. Preserve the existing
AnthropicRequestError 400 handling and generic 500 handling for all other
errors.
In `@src/server/management/agent-settings-routes.ts`:
- Around line 578-581: Update the GrokApplyBusyError branch in the catch block
to return HTTP 503 instead of 409 and include the Retry-After header with a
one-second value, matching the existing busy-response behavior. Leave the
generic error response unchanged.
In `@src/server/management/sidebar-routes.ts`:
- Around line 34-37: Bind the sidebar consent flow to validated GUI session
state instead of trusting non-empty headers: update
requireManagementAuth/handleManagementAPI and hasBrowserSessionEvidence so raw
admin-token requests with forged headers are rejected with 403 before
starRepository or gh execution, while valid minted sessions continue. In
tests/sidebar-routes.test.ts lines 186-192, replace the direct handler-only
coverage with real-server tests covering forged headers (403 and no gh
invocation) and a valid minted session.
In `@src/server/request-decompress.ts`:
- Around line 96-99: In the request decompression flow, update the byte
measurements around decoded text and parsed JSON to avoid allocating full-size
temporary buffers: use decoded.byteLength for the text charge and
Buffer.byteLength for the serialized parsed value instead of
TextEncoder().encode(...). Preserve the existing budget accounting and add a
brief comment explaining why the second returned release closure is
intentionally retained until dispose().
In `@src/server/responses-item-id-repair.ts`:
- Around line 176-182: Update the reservation handling in repairEventPayload so
the rewritten payload’s transient budget is released synchronously after commit,
rather than through queueMicrotask, ensuring each rewritten string is charged
only while it is live. Also remove the redundant item_ids reservation/accounting
if relaySseWithPayloadRewrite.enqueueText already covers the same emitted block
with live_transient; preserve the rewritten payload return behavior.
In `@src/server/responses/collaboration.ts`:
- Around line 112-124: Replace the per-iteration TextEncoder allocations in the
tool-processing loop around toolNsMap, freeformToolNames, and
toolSearchToolNames with Buffer.byteLength, matching the existing approach used
by bridge.ts. Preserve the current measured strings and retained_collectors
budget charges while eliminating intermediate Uint8Array allocations.
In `@src/server/responses/core.ts`:
- Around line 1101-1161: Update finalizeOwnedTranslatorBudget and the
handleResponses flow to preserve the direct native win32 passthrough body
instead of calling response.body.getReader() and wrapping it in a JavaScript
ReadableStream. Detect the native no-rewrite path using the existing native
passthrough symbols, and dispose the owned translator budget through its
existing inspection or relay completion/cancellation hooks while retaining the
wrapper for JavaScript relay responses. Add a focused regression test covering
the owned-budget win32 passthrough path.
In `@src/server/sse-payload-rewrite.ts`:
- Around line 103-118: Update enqueueText to encode text once, store the
resulting Uint8Array, and use its byteLength for the transient reservation
before enqueueing it. Also replace the encoder.encode(...).byteLength
measurements at the nearby line-81 and line-95 call sites with
Buffer.byteLength(...) so sizing does not allocate discarded buffers.
In `@src/vision/index.ts`:
- Around line 425-437: Update the vision request transformation around the loop
over parts and its surrounding OcxParsedRequest mutation to build all
replacement parts and total encoded bytes before changing any message content.
Perform one atomic budget admission for the complete rebuild, abort without
mutation when admission is rejected, and commit the reservation only after the
full rebuild succeeds. Add a focused regression in tests/vision-cache.test.ts
covering rejection of a later admission and asserting both input messages and
retained-byte totals remain unchanged.
In `@tests/anthropic-stream-hardening.test.ts`:
- Around line 2-7: Add a focused overflow regression test alongside the existing
Anthropic stream-hardening tests, using createTestTranslatorBudget({
maxTurnBytes }) with a small limit to drive parseStream through an oversized
tool-argument stream. Assert that exactly one terminal event is emitted and that
its code is "translation_buffer_limit", reusing the existing
createAnthropicAdapter and stream-test helpers.
In `@tests/cursor-framing.test.ts`:
- Around line 154-171: Strengthen the test around decodeAvailableConnectFrames
by recording whether its reserveTransient callback is invoked and with what byte
count, then assert it was called once with CURSOR_MAX_CONNECT_FRAME_BYTES.
Remove the chargeRetained/releaseRetained setup that pre-seeds highWaterBytes,
so the high-water assertion reflects the decode reservation rather than test
setup.
In `@tests/cursor-message-mapper.test.ts`:
- Around line 5-8: The Cursor tests leak budgets created by
createTranslatorBudget; replace it with createTestTranslatorBudget from
tests/helpers/translator-budget.ts. In tests/cursor-message-mapper.test.ts lines
5-8, update the import and kvStore helper so all call sites use the test
factory; make the same import, store-helper, and direct-call updates in
tests/cursor-kv-store.test.ts lines 3-6, 10, and 20.
In `@tests/images/z-fulfill.test.ts`:
- Around line 253-280: Strengthen the test around imageFulfillmentTailSnapshot
so it verifies retained-path byte accounting rather than only per-call bytes.
After fulfillment reaches the retention phase, assert that currentBytes includes
the materialized artifact path bytes; otherwise rename the test to describe only
call-byte accounting.
In `@tests/openai-chat-eof.test.ts`:
- Around line 2-8: Centralize budget-wrapped adapter factories in
tests/helpers/translator-budget.ts by adding a generic budgetedFactory helper,
including the per-call createTestTranslatorBudget behavior needed by
responsesSseToChatCompletionsSse. In tests/openai-chat-eof.test.ts#L2-L8,
tests/openai-chat-hardening.test.ts#L2-L8, and
tests/openai-chat-parallel-stream.test.ts#L2-L7, replace local
createOpenAIChatAdapter aliases with the shared helper import. Apply the same
change to createOpenAIChatAdapter and responsesSseToChatCompletionsSse in
tests/cyber-policy-error-fidelity.test.ts#L19-L31, and to
createOpenAIChatAdapter and createResponsesPassthroughAdapter in
tests/volcengine-providers.test.ts#L15-L20.
In `@tests/responses-item-id-repair.test.ts`:
- Around line 9-18: Add a focused regression test near the existing tests that
retains the translator budget returned by the test setup, sends one stream chunk
containing several rewritable events through relaySseWithResponsesItemIdRepair,
and asserts budget.snapshot().currentBytes returns to its initial value with
snapshot().overflows equal to 0. Preserve the existing wrapper behavior while
exposing the budget only as needed for this accounting assertion.
In `@tests/sse-decoder.test.ts`:
- Around line 114-119: Update the async assertion around decodeServerSentEvents
in the translation buffer limit test: invoke the async callback and assert its
returned promise with rejects.toMatchObject({ code: "translation_buffer_limit"
}) instead of toThrow, while preserving the existing record collection and error
expectation.
In `@tests/translator-budget.test.ts`:
- Around line 83-104: Make the cumulative tool-argument overflow test isolate
the overlap path instead of potentially failing during SSE live-buffer
accounting. In the test around createOpenAIChatAdapter, derive fragment sizes
and the pre-charge from the exported translator limit, leaving headroom for both
live-buffer reservations so the second tool-argument update crosses the cap;
alternatively assert the budget snapshot’s overflow scope if that attribution is
available. Remove the hardcoded 512 KiB and 31 MiB values while preserving the
translation_buffer_limit expectation.
- Around line 246-256: The fixture compiler check in the test should use a
dedicated test tsconfig extending the project configuration instead of passing
fixture files directly. Add that configuration, update the command in the test
around the invalid and valid fixture checks to invoke the pinned local
TypeScript 5.9.3 binary via bun x with --project, and preserve the existing
exit-code and diagnostic assertions.
---
Outside diff comments:
In `@src/adapters/cursor/live-transport.ts`:
- Around line 709-727: Guard the terminal event emission in
scheduleClientToolFinalize against errors from push, including
TranslatorBudgetExceededError. Wrap the terminal loop and subsequent finalize
work in try/catch, and route any failure through the same fail-and-clear
handling used by the data and frame paths, ensuring the timer callback never
allows an exception to escape and crash the process.
In `@src/adapters/google.ts`:
- Around line 539-560: Update the SSE buffering logic around the chunk-append
and residual-buffer handling to track UTF-8 byte counts incrementally rather
than calling budgetEncoder.encode on the entire accumulated buffer. Add the
decoded chunk’s byte length to bufferBytes when appending, then adjust
bufferBytes only for bytes removed when complete lines are split out; preserve
the existing transient reservation, commit, release, MAX_SSE_FRAME_BYTES limit,
and parsing behavior.
- Around line 636-684: Update parseResponse to track the cumulative bytes passed
to budget.chargeRetained and release that exact amount on the MAX_RESPONSE_BYTES
early return and all body-reading error paths, avoiding leaks across retries. In
parseResponse’s catch around body collection and reservation, check
isTranslatorBudgetExceededError(err) first and return the same typed
502/translation_buffer_limit error shape used by parseStream; retain the
existing generic error handling for other failures.
In `@src/adapters/kiro.ts`:
- Around line 900-946: Release retained_collectors budget bytes when buffered
events leave the Kiro collectors: update defer and fallbackEvents handling
around stage so deferred.splice(0) and fallback batches also remove their
associated charges, including transformed phased events. Track charged byte
totals per collector or use the shared event-batch ownership utility, ensuring
all yielded or flushed events transfer or release ownership before the fallback
attempt reuses the budget.
In `@src/bridge.ts`:
- Around line 1309-1320: Update the batch annotation construction in flushText
to release each pendingWebSources entry’s charged bytes with
budget?.releaseRetained(..., { kind: "tool_search_sources" }) before clearing
the array. Mirror the existing takeWebAnnotations release behavior and preserve
the current annotation creation and pushOutput flow.
In `@tests/cursor-tool-finalize-race.test.ts`:
- Around line 67-87: Update makeHarness to create one shared translator budget
and pass it to both createLiveCursorTransport and createCursorProtobufEventState
via translatorBudget. Ensure the state receives the same budget instance used by
the transport so finalize-race tests exercise the production budget lifecycle.
🪄 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: 5f137234-a116-4175-b00a-b958a0c54521
📒 Files selected for processing (115)
AGENTS.mdREADME.mddevlog/_plan/260801_zero_leak_state_stores/006_roadmap_audit_synthesis.mddevlog/_plan/260801_zero_leak_state_stores/040_app_bytes_observability.mddevlog/_plan/260801_zero_leak_state_stores/050_translator_stream_bounds.mddocs-site/src/content/docs/guides/web-dashboard.mddocs-site/src/content/docs/ko/guides/web-dashboard.mdgui/src/components/sidebar-github-row.tsxsrc/adapters/anthropic.tssrc/adapters/azure.tssrc/adapters/base.tssrc/adapters/cursor.tssrc/adapters/cursor/framing.tssrc/adapters/cursor/kv-store.tssrc/adapters/cursor/live-transport.tssrc/adapters/cursor/protobuf-events.tssrc/adapters/cursor/transport.tssrc/adapters/google.tssrc/adapters/kiro-thinking.tssrc/adapters/kiro.tssrc/adapters/mimo-free.tssrc/adapters/openai-chat.tssrc/adapters/openai-responses.tssrc/bridge.tssrc/chat/outbound.tssrc/claude/outbound.tssrc/cli/agent-driven.tssrc/cli/star-prompt.tssrc/codex/quota.tssrc/images/fulfill.tssrc/images/loop.tssrc/lib/app-owned-memory-stores.tssrc/lib/app-owned-memory.tssrc/lib/sse-decoder.tssrc/lib/translator-budget.tssrc/oauth/index.tssrc/oauth/store.tssrc/router.tssrc/server/chat-completions.tssrc/server/claude-messages.tssrc/server/index.tssrc/server/management-api.tssrc/server/management/agent-settings-routes.tssrc/server/management/logs-usage-routes.tssrc/server/management/oauth-account-routes.tssrc/server/management/sidebar-routes.tssrc/server/management/usage-summary-cache.tssrc/server/relay.tssrc/server/request-decompress.tssrc/server/responses-image-gen-repair.tssrc/server/responses-item-id-repair.tssrc/server/responses/collaboration.tssrc/server/responses/core.tssrc/server/sse-payload-rewrite.tssrc/vision/index.tssrc/web-search/loop.tssrc/web-search/progress-stream.tstests/adapter-error-inline.test.tstests/adapter-usage.test.tstests/agent-driven.test.tstests/anthropic-compatible-stream.test.tstests/anthropic-reasoning.test.tstests/anthropic-stream-hardening.test.tstests/anthropic-thinking-signature.test.tstests/app-owned-memory.test.tstests/azure-adapter.test.tstests/bridge-live-delivery.test.tstests/chat-completions-endpoint.test.tstests/claude-messages-endpoint.test.tstests/claude-outbound.test.tstests/codex-metadata-integrity.test.tstests/config.test.tstests/cursor-adapter.test.tstests/cursor-framing.test.tstests/cursor-hardening.test.tstests/cursor-kv-store.test.tstests/cursor-live-transport.test.tstests/cursor-message-mapper.test.tstests/cursor-tool-finalize-race.test.tstests/cyber-policy-error-fidelity.test.tstests/deepseek-inbound-wire.test.tstests/fixtures/translator-budget-required.invalid.tstests/fixtures/translator-budget-required.valid.tstests/google-antigravity-wire.test.tstests/google-hardening.test.tstests/google-vertex-stream.test.tstests/google-wire-compiler.test.tstests/helpers/translator-budget.tstests/images/gemini-inline.test.tstests/images/loop.test.tstests/images/z-fulfill.test.tstests/kiro-stream.test.tstests/oauth-store-multi.test.tstests/openai-chat-eof.test.tstests/openai-chat-hardening.test.tstests/openai-chat-parallel-stream.test.tstests/openai-responses-passthrough.test.tstests/passthrough-override.test.tstests/responses-compaction.test.tstests/responses-image-gen-repair.test.tstests/responses-item-id-repair.test.tstests/run-turn-queue.test.tstests/settings-stream-mode.test.tstests/sidebar-routes.test.tstests/sidecar-abort.test.tstests/sse-decoder.test.tstests/sse-failed-tail.test.tstests/sse-payload-rewrite.test.tstests/startup-prompt.test.tstests/state-store-sweeper.test.tstests/translator-budget.test.tstests/umans-provider.test.tstests/volcengine-providers.test.tstests/web-search-timeout-contract.test.tstests/web-search.test.ts
| | Tail | Verified current anchor | Bound, accounting, and acceptance contract | | ||
| |---|---|---| | ||
| | Image fulfillment | `src/images/fulfill.ts:12-24,89-106` | Reserve one of 64 slots before any provider or artifact work in `fulfillImageCall()`. This honestly bounds the whole accepted fulfillment population, not merely the short retention-chain section. The 65th returns ordinary tool failure `image_fulfillment_busy`. `image_fulfillment_tail.currentBytes` is the exact UTF-8 bytes of closure-retained call payload/path strings and the `paths` entries waiting in `retentionTail`; charge each physical retained copy, transfer charge when provider payload becomes queued paths, and release after filter/error. Never report count as bytes. `active` is accepted fulfillments; high-water is aggregate retained bytes. Preserve write→prune→filter. | | ||
| | OAuth mutation | `src/oauth/store.ts:317-330` | Introduce a 128-lease admission gate before appending work to the mutation chain. Full admission rejects with typed exported `OAuthMutationBusyError`; rejected work never enters `mutationTail`. A reserved waiter may wait at most 30 s for the current chain head. At wake/timeout, atomically check whether execution started: if not started at timeout, release its lease and reject without enqueue; if the head won, recheck/swap the current tail synchronously, mark started, and guarantee accepted work executes exactly once. Timeout never interrupts running work. Release lease in execution `finally`. Snapshot bytes are exact retained closure/provider/account string bytes, `active` is reserved+running leases, and rejected waiters contribute neither bytes nor active count. | | ||
| | Grok apply | `src/server/management/agent-settings-routes.ts:62-70,534-559` | Replace the FIFO chain with one `grokApplyFlight: { startedAt, promise } | null`. Concurrent no-body applies join that exact promise only while it is younger than 120 s. Resolve dynamic imports, `loadConfig()`, `readRuntimePort(process.pid)`, host, port, and sync options inside the newly created flight closure—not before queue/join—so joined callers truly share identical persisted/runtime state. If an active flight is 120 s old, do not join or start another; return 409 `grok_apply_busy`. Clear only by identity in `finally`. Snapshot exact retained host/config serialization bytes while active and `active` 0/1. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the union pipe in the Grok table cell.
At Line 524, the raw | in `grokApplyFlight: { startedAt, promise } | null` creates an extra Markdown table column. Renderers can truncate the acceptance contract.
Use an HTML <code> element or escape the pipe.
Proposed fix
- `grokApplyFlight: { startedAt, promise } | null`
+ <code>grokApplyFlight: { startedAt, promise } | null</code>🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 524-524: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 524-524: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 524-524: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 524-524: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 524-524: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 524-524: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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/_plan/260801_zero_leak_state_stores/050_translator_stream_bounds.md`
around lines 520 - 524, Update the Grok apply table cell containing
grokApplyFlight so the union pipe in “{ startedAt, promise } | null” is escaped
or rendered inside an HTML code element, preserving the full acceptance contract
as a single Markdown table cell.
Source: Linters/SAST tools
| - **Declining ends it.** No decline state is persisted, and nothing is injected into any model | ||
| prompt to nudge you later. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make decline persistence match the documented behavior.
src/cli/star-prompt.ts writes MARKER before interactiveConfirm(). A declined prompt therefore leaves durable state and suppresses future prompts. This conflicts with all three statements that no decline state is persisted.
README.md#L373-L374: Keep this statement only if a decline does not write the marker.docs-site/src/content/docs/guides/web-dashboard.md#L135-L136: Keep the English documentation aligned with the final marker behavior.docs-site/src/content/docs/ko/guides/web-dashboard.md#L113-L114: Update the Korean translation with the same final behavior.
Move the marker write until after an affirmative confirmation, then add a regression test that a declined prompt leaves no marker. If one-time suppression after decline is intentional, document that behavior instead.
📍 Affects 3 files
README.md#L373-L374(this comment)docs-site/src/content/docs/guides/web-dashboard.md#L135-L136docs-site/src/content/docs/ko/guides/web-dashboard.md#L113-L114
🤖 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 `@README.md` around lines 373 - 374, Update src/cli/star-prompt.ts so the
MARKER is written only after interactiveConfirm() returns an affirmative result,
and add a regression test verifying declined prompts leave no marker. Preserve
or align the documented no-persistence behavior in README.md lines 373-374,
docs-site/src/content/docs/guides/web-dashboard.md lines 135-136, and
docs-site/src/content/docs/ko/guides/web-dashboard.md lines 113-114; these
documentation sites require no wording change if the implementation matches
them.
Source: Path instructions
| } catch (error) { | ||
| if (!isTranslatorBudgetExceededError(error)) throw error; | ||
| yield { | ||
| type: "error", | ||
| status: 502, | ||
| errorType: "upstream_error", | ||
| code: "translation_buffer_limit", | ||
| message: "upstream translation buffer exceeded the safe limit", | ||
| }; | ||
| } finally { | ||
| if (currentToolCallId) budget.closeCall(currentToolCallId); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Return after the overflow error, or parseStream emits a second terminal event.
The catch block at Line 928 yields the translation_buffer_limit error but does not return. After the finally block runs, execution continues to Line 940. emittedDone is still false, so the generator yields one more event: a done event when pendingStopReason is set, or the error "upstream stream ended before message_stop — possible truncation" when it is not. The turn then carries two terminal events, and the trailing event can mask the real 502/translation_buffer_limit cause downstream. Every other terminal path in this generator returns immediately (Line 900, Line 924).
Set the terminal flag and return inside the catch block.
🐛 Proposed fix
} catch (error) {
if (!isTranslatorBudgetExceededError(error)) throw error;
+ emittedDone = true;
yield {
type: "error",
status: 502,
errorType: "upstream_error",
code: "translation_buffer_limit",
message: "upstream translation buffer exceeded the safe limit",
};
+ return;
} finally {
if (currentToolCallId) budget.closeCall(currentToolCallId);
}📝 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.
| } catch (error) { | |
| if (!isTranslatorBudgetExceededError(error)) throw error; | |
| yield { | |
| type: "error", | |
| status: 502, | |
| errorType: "upstream_error", | |
| code: "translation_buffer_limit", | |
| message: "upstream translation buffer exceeded the safe limit", | |
| }; | |
| } finally { | |
| if (currentToolCallId) budget.closeCall(currentToolCallId); | |
| } | |
| } catch (error) { | |
| if (!isTranslatorBudgetExceededError(error)) throw error; | |
| emittedDone = true; | |
| yield { | |
| type: "error", | |
| status: 502, | |
| errorType: "upstream_error", | |
| code: "translation_buffer_limit", | |
| message: "upstream translation buffer exceeded the safe limit", | |
| }; | |
| return; | |
| } finally { | |
| if (currentToolCallId) budget.closeCall(currentToolCallId); | |
| } |
🤖 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/adapters/anthropic.ts` around lines 928 - 939, Update the budget-overflow
branch in the parseStream catch block to mark the generator as terminal before
yielding the translation_buffer_limit error, then return immediately after
yielding it. Preserve the existing rethrow behavior for non-budget errors and
ensure the finally cleanup still executes without allowing the subsequent
terminal-event logic to run.
| get(key) { | ||
| const value = values.get(key); | ||
| return value ? cloneBytes(value) : undefined; | ||
| if (!value) return undefined; | ||
| budget.chargeRetained(value.byteLength, { kind: "cursor_kv" }); | ||
| const cloned = cloneBytes(value); | ||
| queueMicrotask(() => budget.releaseRetained(value.byteLength, { kind: "cursor_kv" })); | ||
| return cloned; | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
get releases the read charge on a microtask, so the returned clone becomes untracked memory.
Line 30 charges value.byteLength as retained. Line 31 creates a clone that is returned to the caller. Line 32 releases the same bytes on the next microtask. The clone can live for the whole turn, and the accounting stops after one microtask. The budget therefore records a transient spike but never accounts for the copy the caller keeps, which defeats the bounded-retention goal for cursor_kv reads.
A second failure mode: chargeRetained throws when the aggregate cap is exceeded, so a plain read can now abort the turn even though the store already holds the bytes. Reads that only hand out an existing, already-charged buffer should not be able to fail the turn.
Two concrete options:
- Transfer ownership explicitly: return the clone with a release callback, and let the caller release when it drops the copy.
- Do not charge reads at all. The value is already charged as retained by the seed loop or by
set, so the clone is the caller's allocation and belongs to the caller's own scope.
♻️ Proposed fix: stop double-accounting reads
get(key) {
const value = values.get(key);
if (!value) return undefined;
- budget.chargeRetained(value.byteLength, { kind: "cursor_kv" });
- const cloned = cloneBytes(value);
- queueMicrotask(() => budget.releaseRetained(value.byteLength, { kind: "cursor_kv" }));
- return cloned;
+ // The stored value is already charged by the seed loop / set(); the clone is the
+ // caller's allocation and must be accounted in the caller's scope.
+ const reservation = budget.reserveTransient(value.byteLength, { kind: "cursor_kv" });
+ try {
+ const cloned = cloneBytes(value);
+ reservation.release();
+ return cloned;
+ } catch (error) {
+ reservation.release();
+ throw error;
+ }
},🤖 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/adapters/cursor/kv-store.ts` around lines 27 - 34, Update get in the
cursor key-value store to stop charging and asynchronously releasing retained
bytes for reads. Return the cloned value directly, relying on the existing
retention charge applied when the value is seeded or set, so reading an existing
entry cannot throw from chargeRetained or leave transient accounting for the
caller’s copy.
| import { createOpenAIChatAdapter as createOpenAIChatAdapterProduction } from "../src/adapters/openai-chat"; | ||
| import { bridgeToResponsesSSE } from "../src/bridge"; | ||
| import type { AdapterEvent } from "../src/types"; | ||
| import { withTestTranslatorBudget } from "./helpers/translator-budget"; | ||
|
|
||
| const createOpenAIChatAdapter = (...args: Parameters<typeof createOpenAIChatAdapterProduction>) => | ||
| withTestTranslatorBudget(createOpenAIChatAdapterProduction(...args)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The same budget-wrapping alias is re-declared in five test files. tests/helpers/translator-budget.ts exports withTestTranslatorBudget for adapter instances but no budget-wrapped factory, so every consumer repeats an identical const createX = (...args: Parameters<typeof createXProduction>) => withTestTranslatorBudget(createXProduction(...args)) block. Each copy is correct today, but a change to the wrapping contract requires editing five files, and new adapter tests will copy the block again. Export a single generic factory wrapper from the helper, for example budgetedFactory(createOpenAIChatAdapter), then replace each local alias with one import.
tests/openai-chat-eof.test.ts#L2-L8: replace the localcreateOpenAIChatAdapteralias with the shared factory wrapper import.tests/openai-chat-hardening.test.ts#L2-L8: replace the localcreateOpenAIChatAdapteralias with the shared factory wrapper import.tests/openai-chat-parallel-stream.test.ts#L2-L7: replace the localcreateOpenAIChatAdapteralias with the shared factory wrapper import.tests/cyber-policy-error-fidelity.test.ts#L19-L31: replace both thecreateOpenAIChatAdapteralias and theresponsesSseToChatCompletionsSsebudget-injecting wrapper with shared helpers, so the per-callcreateTestTranslatorBudget()behaviour is defined once.tests/volcengine-providers.test.ts#L15-L20: replace both thecreateOpenAIChatAdapterandcreateResponsesPassthroughAdapteraliases with the shared factory wrapper import.
📍 Affects 5 files
tests/openai-chat-eof.test.ts#L2-L8(this comment)tests/openai-chat-hardening.test.ts#L2-L8tests/openai-chat-parallel-stream.test.ts#L2-L7tests/cyber-policy-error-fidelity.test.ts#L19-L31tests/volcengine-providers.test.ts#L15-L20
🤖 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/openai-chat-eof.test.ts` around lines 2 - 8, Centralize budget-wrapped
adapter factories in tests/helpers/translator-budget.ts by adding a generic
budgetedFactory helper, including the per-call createTestTranslatorBudget
behavior needed by responsesSseToChatCompletionsSse. In
tests/openai-chat-eof.test.ts#L2-L8, tests/openai-chat-hardening.test.ts#L2-L8,
and tests/openai-chat-parallel-stream.test.ts#L2-L7, replace local
createOpenAIChatAdapter aliases with the shared helper import. Apply the same
change to createOpenAIChatAdapter and responsesSseToChatCompletionsSse in
tests/cyber-policy-error-fidelity.test.ts#L19-L31, and to
createOpenAIChatAdapter and createResponsesPassthroughAdapter in
tests/volcengine-providers.test.ts#L15-L20.
| function relaySseWithResponsesItemIdRepair( | ||
| body: ReadableStream<Uint8Array>, | ||
| config: Parameters<typeof relaySseWithResponsesItemIdRepairProduction>[1], | ||
| ): ReadableStream<Uint8Array> { | ||
| const budget = createTestTranslatorBudget(); | ||
| return finalizeTranslatorBudgetResponse( | ||
| new Response(relaySseWithResponsesItemIdRepairProduction(body, config, budget)), | ||
| budget, | ||
| ).body!; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the new budget accounting in repairEventPayload.
The wrapper hides the budget from every assertion in this file. No test observes budget.snapshot() or the charges taken at src/server/responses-item-id-repair.ts:92, :179, and :181. A regression in that accounting — for example the concurrent-charge accumulation described in my comment on src/server/responses-item-id-repair.ts lines 176-182 — passes this suite unchanged.
Add one test that keeps a reference to the budget, feeds a stream whose single chunk contains several rewritable events, and asserts that snapshot().currentBytes returns to its starting value and that snapshot().overflows stays at 0. That test also pins the fix for the deferred-release defect.
Repository guidelines require a focused regression test near the existing tests for a behavior change in src/.
🤖 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/responses-item-id-repair.test.ts` around lines 9 - 18, Add a focused
regression test near the existing tests that retains the translator budget
returned by the test setup, sends one stream chunk containing several rewritable
events through relaySseWithResponsesItemIdRepair, and asserts
budget.snapshot().currentBytes returns to its initial value with
snapshot().overflows equal to 0. Preserve the existing wrapper behavior while
exposing the budget only as needed for this accounting assertion.
Source: Coding guidelines
| await expect(async () => { | ||
| for await (const record of decodeServerSentEvents( | ||
| chunkedStream([`data: ${"x".repeat(retained)}\n\n`]), | ||
| { translatorBudget }, | ||
| )) records.push(record); | ||
| }).toThrow(expect.objectContaining({ code: "translation_buffer_limit" })); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Bun test expect toThrow async function rejects matcher
💡 Result:
In Bun, there are two primary patterns for testing asynchronous errors, both of which are supported [1][2]. 1. Using.rejects.toThrow This is the standard Jest-compatible approach [3]. You pass a promise to the expect function and chain.rejects.toThrow [1][2]. Note that you must await the assertion [1][2]. test("should handle async errors", async => { await expect(fetchUser("invalid-id")).rejects.toThrow("User not found"); }); 2. Using an async function wrapper with.toThrow You can also pass an async function directly into expect and use.toThrow [1][2]. This is often used for clearer readability when defining the logic inside the test [1][4]. test("should handle async errors", async => { await expect(async => { await fetchUser("invalid-id"); }).toThrow("User not found"); }); Both methods are officially supported and documented in the Bun test framework [1][2]. Earlier versions of Bun had limitations regarding.rejects.toThrow, but these have been addressed in later updates to ensure compatibility with Jest-like testing patterns [3][5].
Citations:
- 1: https://bun.com/docs/test/writing-tests
- 2: https://bun.sh/docs/test/writing-tests
- 3: https://bun.com/blog/release-notes/bun-v1.0.18
- 4: fix: expect().resolves and expect().rejects now return promises oven-sh/bun#23425
- 5: Promise.reject() expects a callback instead of an error (for testing
.rejects) oven-sh/bun#4755
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant test section ---'
sed -n '80,135p' tests/sse-decoder.test.ts
printf '%s\n' '--- Bun configuration and version references ---'
rg -n --glob 'package.json' --glob 'bun.lock' --glob 'bunfig.toml' --glob '*.yml' --glob '*.yaml' '"bun"|bun@|packageManager' .
printf '%s\n' '--- matcher usage in tests ---'
rg -n 'toThrow|rejects\.|async \(\)' tests/sse-decoder.test.ts tests | head -120Repository: lidge-jun/opencodex
Length of output: 17006
🏁 Script executed:
#!/bin/bash
set -eu
if command -v bun >/dev/null 2>&1; then
printf 'bun=%s\n' "$(bun --version)"
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/async-to-throw.test.ts" <<'TS'
import { expect, test } from "bun:test";
test("Bun async callback toThrow behavior", async () => {
await expect(async () => {
throw Object.assign(new Error("limit"), { code: "translation_buffer_limit" });
}).toThrow(expect.objectContaining({ code: "translation_buffer_limit" }));
});
test("Bun promise rejects matcher behavior", async () => {
await expect((async () => {
throw Object.assign(new Error("limit"), { code: "translation_buffer_limit" });
})()).rejects.toMatchObject({ code: "translation_buffer_limit" });
});
TS
bun test "$tmpdir/async-to-throw.test.ts"
else
echo "bun is not available"
fiRepository: lidge-jun/opencodex
Length of output: 178
🌐 Web query:
site:bun.com/docs/test toThrow async function Bun expect rejects asynchronous errors
💡 Result:
To test for asynchronous errors in Bun, use the.rejects matcher in combination with the expect function [1]. You must await the expectation to ensure the test runner waits for the promise to settle [1]. The standard pattern is: await expect(async => { await yourAsyncFunction; }).rejects.toThrow("Error message or pattern"); This structure is used when passing a function that returns a promise to expect [1]. If you are working with a promise directly, you can also use: await expect(promise).rejects.toThrow; The.toThrow matcher supports strings, regular expressions, or constructor types to verify the error content [1]. This approach is consistent with the Jest-compatible API provided by the bun:test module [1][2].
Citations:
🌐 Web query:
site:github.com/oven-sh/bun "async function" "toThrow" expect test
💡 Result:
To test that an asynchronous function throws an error using Jest or Vitest, you should not use.toThrow directly on the function execution [1][2][3]. Instead, you must use the.rejects matcher [1][2][3]. Because testing frameworks must wait for the promise to settle before checking if it rejected, you must await the expectation [1][2][3]. Correct pattern for Jest and Vitest: await expect(asyncFunction).rejects.toThrow; If you are using Bun's native test runner (bun:test), it supports a more concise syntax by wrapping the async code in a function [1][4]: await expect(async => { await asyncFunction; }).toThrow; Note that in Jest and Vitest, passing an async function directly to expect without using.rejects (e.g., expect(async => await func).toThrow) will generally not work as expected because.toThrow is designed to catch synchronous errors, not promise rejections [3]. Always ensure you are awaiting the promise chain [1][2].
Citations:
- 1: https://github.com/oven-sh/bun/blob/1498d7b7/docs/test/writing-tests.mdx
- 2: https://github.com/oven-sh/bun/blob/a0e221e0/docs/test/writing-tests.mdx
- 3: Fix
expect().rejects.toThrow()oven-sh/bun#5602 - 4: fix: expect().resolves and expect().rejects now return promises oven-sh/bun#23425
🏁 Script executed:
#!/bin/bash
set -eu
for url in \
'https://raw.githubusercontent.com/oven-sh/bun/1498d7b7/docs/test/writing-tests.mdx' \
'https://raw.githubusercontent.com/oven-sh/bun/a0e221e0/docs/test/writing-tests.mdx'
do
echo "--- $url ---"
curl -L --fail --silent "$url" | rg -n -C 4 'rejects|toThrow|async'
done
echo '--- cited pull request metadata and changed lines ---'
curl -L --fail --silent \
-H 'Accept: application/vnd.github+json' \
'https://api.github.com/repos/oven-sh/bun/pulls/23425' |
jq '{state, merged_at, title, body, merge_commit_sha}'
curl -L --fail --silent \
-H 'Accept: application/vnd.github+json' \
'https://api.github.com/repos/oven-sh/bun/pulls/23425/files?per_page=100' |
jq -r '.[] | .filename, .patch' |
rg -n -C 6 'toThrow|rejects|async|promise'Repository: lidge-jun/opencodex
Length of output: 7199
🏁 Script executed:
#!/bin/bash
set -eu
curl -L --fail --silent \
-H 'Accept: application/vnd.github+json' \
'https://api.github.com/repos/oven-sh/bun/pulls/23425' |
jq '{number, title, state, merged_at, created_at, updated_at, merge_commit_sha, html_url}'
echo '--- pull request body ---'
curl -L --fail --silent \
-H 'Accept: application/vnd.github+json' \
'https://api.github.com/repos/oven-sh/bun/pulls/23425' |
jq -r '.body // ""' | sed -n '1,120p'
echo '--- pull request files mentioning async error matchers ---'
curl -L --fail --silent \
-H 'Accept: application/vnd.github+json' \
'https://api.github.com/repos/oven-sh/bun/pulls/23425/files?per_page=100' |
jq -r '.[] | "FILE: \(.filename)\n\(.patch // "")"' |
rg -n -C 8 'toThrow|rejects|async|promise' || trueRepository: lidge-jun/opencodex
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -eu
response="$(mktemp)"
trap 'rm -f "$response"' EXIT
status="$(curl -L --silent --show-error -o "$response" -w '%{http_code}' \
-H 'Accept: application/vnd.github+json' \
'https://api.github.com/repos/oven-sh/bun/pulls/23425')"
printf 'HTTP status: %s\n' "$status"
jq '{keys: (keys), message, documentation_url, number, title, state, merged_at, merge_commit_sha}' "$response"
echo '--- Bun source references ---'
for query in \
'repo:oven-sh/bun "toThrow" "Promise"' \
'repo:oven-sh/bun "rejects" "toThrow"' \
'repo:oven-sh/bun "isAsync" "toThrow"'
do
echo "--- $query ---"
curl -L --silent --show-error --get \
-H 'Accept: application/vnd.github+json' \
--data-urlencode "q=$query" \
'https://api.github.com/search/code' |
jq -r '.message // empty, (.items[]? | "\(.repository.full_name):\(.path)")'
doneRepository: lidge-jun/opencodex
Length of output: 1283
🏁 Script executed:
#!/bin/bash
set -eu
for url in \
'https://github.com/oven-sh/bun/pull/23425.patch' \
'https://github.com/oven-sh/bun/pull/23425.diff'
do
echo "--- $url ---"
curl -L --fail --silent --show-error "$url" |
rg -n -C 8 'toThrow|rejects|async|promise|expect' || true
doneRepository: lidge-jun/opencodex
Length of output: 50376
Assert the promise rejection at tests/sse-decoder.test.ts:114.
The async callback returns a rejected promise. .toThrow(...) does not await that promise. Invoke the callback and use .rejects.toMatchObject({ code: "translation_buffer_limit" }).
🤖 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/sse-decoder.test.ts` around lines 114 - 119, Update the async assertion
around decodeServerSentEvents in the translation buffer limit test: invoke the
async callback and assert its returned promise with rejects.toMatchObject({
code: "translation_buffer_limit" }) instead of toThrow, while preserving the
existing record collection and error expectation.
| test("OpenAI cumulative arguments reject when the old and replacement strings overlap past the turn cap", async () => { | ||
| const adapter = createOpenAIChatAdapter({ | ||
| adapter: "openai-chat", | ||
| baseUrl: "https://example.test/v1", | ||
| apiKey: "key", | ||
| }); | ||
| const firstFragment = "x".repeat(512 * 1024); | ||
| const frames = [ | ||
| { choices: [{ delta: { tool_calls: [{ index: 0, id: "call_1", function: { name: "test_tool", arguments: firstFragment } }] } }] }, | ||
| { choices: [{ delta: { tool_calls: [{ index: 0, function: { arguments: "x" } }] } }] }, | ||
| { choices: [{ delta: {}, finish_reason: "tool_calls" }] }, | ||
| ].map(value => `data: ${JSON.stringify(value)}\n\n`).join("") + "data: [DONE]\n\n"; | ||
| const budget = createTranslatorBudget(); | ||
| budget.chargeRetained(31 * 1024 * 1024, { kind: "retained_collectors" }); | ||
| const events: AdapterEvent[] = []; | ||
| try { | ||
| for await (const event of adapter.parseStream(new Response(frames), budget)) events.push(event); | ||
| expect(events.at(-1)).toMatchObject({ type: "error", code: "translation_buffer_limit" }); | ||
| } finally { | ||
| budget.dispose(); | ||
| } | ||
| }, 60_000); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This test cannot prove the overflow came from cumulative tool arguments.
The name states the failure comes from the old and replacement argument strings overlapping. The assertion at Line 100 only checks code: "translation_buffer_limit", which every translator overflow produces.
With 31 MiB pre-charged at Line 96 and a 512 KiB body, the adapter's own SSE live-buffer accounting (kind: "live_transient", added in this PR) also reserves roughly 512 KiB, and again for the post-split residual. The turn can therefore exhaust its 32 MiB budget inside the live-buffer path before the second tool-argument fragment is ever charged. The test then passes while the cumulative-overlap logic is untested, and a regression in reserveTransient(nextBytes)/releaseRetained(previousBytes) at Lines 825-830 of src/adapters/openai-chat.ts would go undetected.
Pin the cause. Either assert the failing scope, or reduce the pre-charge so only the tool-argument overlap can cross the cap. For example, pre-charge TRANSLATOR_MAX_TURN_BYTES - (firstFragment bytes * 2) - headroom and derive the fragment sizes from the exported constant instead of hardcoding 31 * 1024 * 1024.
💚 Sketch of a cause-pinning assertion
for await (const event of adapter.parseStream(new Response(frames), budget)) events.push(event);
expect(events.at(-1)).toMatchObject({ type: "error", code: "translation_buffer_limit" });
+ // The overflow must be attributed to the tool-argument scope, not the live SSE buffer.
+ expect(budget.snapshot()).toMatchObject({ overflowKind: "tool_args" });Adjust the snapshot field name to whatever TranslatorBudgetSnapshot exposes; if it exposes no per-kind overflow attribution, consider adding it, since it is the only way to make this assertion meaningful.
🤖 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/translator-budget.test.ts` around lines 83 - 104, Make the cumulative
tool-argument overflow test isolate the overlap path instead of potentially
failing during SSE live-buffer accounting. In the test around
createOpenAIChatAdapter, derive fragment sizes and the pre-charge from the
exported translator limit, leaving headroom for both live-buffer reservations so
the second tool-argument update crosses the cap; alternatively assert the budget
snapshot’s overflow scope if that attribution is available. Remove the hardcoded
512 KiB and 31 MiB values while preserving the translation_buffer_limit
expectation.
| test("production adapter contract rejects omitted translator budgets at typecheck", async () => { | ||
| const base = [ | ||
| "x", "tsc", "--noEmit", "--target", "ESNext", "--module", "ESNext", | ||
| "--moduleResolution", "bundler", "--types", "bun-types", "--strict", "--skipLibCheck", | ||
| ]; | ||
| const invalid = Bun.spawnSync(["bun", ...base, "tests/fixtures/translator-budget-required.invalid.ts"]); | ||
| expect(invalid.exitCode).not.toBe(0); | ||
| expect(invalid.stdout.toString() + invalid.stderr.toString()).toContain("TS2554"); | ||
| const valid = Bun.spawnSync(["bun", ...base, "tests/fixtures/translator-budget-required.valid.ts"]); | ||
| expect(valid.exitCode).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the locally declared TypeScript version and the typecheck script this test should mirror.
set -euo pipefail
fd -t f 'package.json' -d 2 --exec sh -c 'echo "== $1"; jq "{scripts: .scripts, ts: (.devDependencies.typescript // .dependencies.typescript)}" "$1"' _ {}
fd -t f 'tsconfig*.json' -d 2 --exec sh -c 'echo "== $1"; cat "$1"' _ {}
fd -t f 'translator-budget-required' tests --exec sh -c 'echo "== $1"; cat -n "$1"' _ {}Repository: lidge-jun/opencodex
Length of output: 4730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== root package metadata =='
cat -n package.json | sed -n '1,180p'
printf '%s\n' '== lockfiles =='
git ls-files '*lock*' '*package-lock.json' '*yarn.lock' '*pnpm-lock.yaml' | sort
for f in bun.lock bun.lockb package-lock.json yarn.lock pnpm-lock.yaml; do
if [ -f "$f" ]; then
printf '\n== %s ==\n' "$f"
rg -n -C 3 '"?(typescript|tsc)"?' "$f" | head -120 || true
fi
done
printf '%s\n' '== target test and nearby fixtures =='
cat -n tests/translator-budget.test.ts | sed -n '230,265p'
git ls-files tests | rg 'translator|tsconfig|package' | sort
printf '%s\n' '== installed compiler files, if present =='
for f in node_modules/typescript/package.json node_modules/.bin/tsc; do
if [ -e "$f" ]; then
ls -l "$f"
[ "$f" = "node_modules/typescript/package.json" ] && jq '{name,version}' "$f"
fi
doneRepository: lidge-jun/opencodex
Length of output: 7642
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '== Bun-selected compiler =='
bun x tsc --version
printf '%s\n' '== local compiler =='
./node_modules/.bin/tsc --version
printf '%s\n' '== explicit-file showConfig with test flags =='
bun x tsc --showConfig --target ESNext --module ESNext --moduleResolution bundler \
--types bun-types --strict --skipLibCheck \
tests/fixtures/translator-budget-required.valid.ts 2>&1 | head -100
printf '%s\n' '== explicit-file showConfig without test flags =='
bun x tsc --showConfig tests/fixtures/translator-budget-required.valid.ts 2>&1 | head -100
printf '%s\n' '== project showConfig =='
bun x tsc --showConfig -p tsconfig.json 2>&1 | head -140Repository: lidge-jun/opencodex
Length of output: 493
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '== explicit-file showConfig with test flags =='
./node_modules/.bin/tsc --showConfig --target ESNext --module ESNext --moduleResolution bundler \
--types bun-types --strict --skipLibCheck \
tests/fixtures/translator-budget-required.valid.ts 2>&1 | head -120
printf '%s\n' '== explicit-file showConfig without test flags =='
./node_modules/.bin/tsc --showConfig tests/fixtures/translator-budget-required.valid.ts 2>&1 | head -120
printf '%s\n' '== project showConfig =='
./node_modules/.bin/tsc --showConfig -p tsconfig.json 2>&1 | head -160Repository: lidge-jun/opencodex
Length of output: 7520
🌐 Web query:
Bun documentation bun x local package binary installs package if not found resolution node_modules
💡 Result:
The command bun x (also available as bunx) is designed to run package executables (binaries) from npm [1][2]. Its resolution behavior follows a specific order: it first checks for the package in the local node_modules directory and, if not found, it automatically downloads and installs the package into a global shared cache to execute it [1][3]. Key details regarding this resolution and execution process include: 1. Local Resolution: bun x prioritizes packages that are already installed locally in the project's node_modules directory [1][3]. 2. Auto-Installation: If the package is not found locally, bun x falls back to auto-installing it from the npm registry into a global cache [1]. This enables you to run tools (such as scaffolding utilities) without requiring a manual install step beforehand [3]. 3. Execution of Binaries: bun x looks for the binary defined in the bin field of the package's package.json [1][4]. If a package has multiple binaries or the binary name does not match the package name, you may need to use the --package (or -p) flag to specify the exact package to install [1]. 4. Flags and Controls: - --package <package>: Specifies the package to install/run, which is useful when the binary name differs from the package name [1]. - --no-install: A flag intended to skip installation if the package is not already found locally, though users have reported inconsistent behavior with this flag in some contexts [5]. - --bun: Forces the command to execute using the Bun runtime instead of Node.js, even if the package has a Node.js shebang [1][6]. While bun x is intended to behave similarly to npx, there have been historical reports of edge-case discrepancies, such as difficulties with certain scoped package names or variations in how executables are determined compared to npx [7][8][9]. For critical project dependencies, it is generally recommended to explicitly include them in your package.json to ensure consistency and reproducibility [5].
Citations:
- 1: https://bun.com/docs/pm/bunx
- 2: https://bun.sh/docs/pm/bunx
- 3: https://linuxcommandlibrary.com/man/bun-x
- 4: Fix build on arch and fix
bun xto execute local binaries from root package.json oven-sh/bun#6913 - 5:
--no-installflag isn't working forbun xoven-sh/bun#23673 - 6: https://bun.com/docs/runtime
- 7:
bun x"could not determine executable" whennpxdoes. oven-sh/bun#9178 - 8: bunx behaviour, cli inconsistencies with bun x oven-sh/bun#5030
- 9: bunx ignores globally installed packages when using scoped package names (e.g.
bunx @scope/pkg) oven-sh/bun#32019
Use a dedicated project configuration for the fixture compiler check. Lines 248–250 pass the fixture as an explicit input, so TypeScript ignores tsconfig.json and the test can diverge from bun run typecheck. Add a test tsconfig that extends the project configuration, then invoke the local TypeScript binary with --project. typescript is pinned to 5.9.3, and bun x uses the local installation first.
🤖 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/translator-budget.test.ts` around lines 246 - 256, The fixture compiler
check in the test should use a dedicated test tsconfig extending the project
configuration instead of passing fixture files directly. Add that configuration,
update the command in the test around the invalid and valid fixture checks to
invoke the pinned local TypeScript 5.9.3 binary via bun x with --project, and
preserve the existing exit-code and diagnostic assertions.
Source: Coding guidelines
… fenced exit Cursor background shells now belong to the session that spawned them: the transport threads its stable id through both dispatch paths, stdin and termination check ownership, and a cross-session write is refused without naming the owner. Admission runs through the shared gate with the lease held in the registry entry until the child's close is actually confirmed — a failed kill quarantines the entry instead of forgetting the process. Idle and absolute deadlines bound every shell's life, the transport's async close awaits its session's cleanup, and shutdown installs a synchronous spawn fence before draining so a queued frame can never start a shell the drain will miss.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/adapters/cursor/live-transport.ts (2)
661-675: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFence queued native execution before shell cleanup.
At
src/adapters/cursor/live-transport.tsLine 662,terminateBackgroundShellsForSession()snapshots the shells that already exist. A queuedbackgroundShellSpawnArgsframe can start later throughframeWorkandhandleServerMessage(). That shell is created after the snapshot, so cleanup does not terminate it.Set a closing flag before cleanup starts. Ignore queued
execServerMessageframes when that flag is set. Set the same flag incancelCursorRun(). Add a regression test that queues a shell spawn behind another frame, closes the transport, then confirms that no child process starts.Proposed fix
private shellCleanup?: Promise<BackgroundShellTerminationReport>; +private closing = false; async close(): Promise<void> { + this.closing = true; if (this.heartbeat) clearInterval(this.heartbeat); // ... } private cancelCursorRun(): void { + this.closing = true; this.expectedClose = true; // ... } private async handleServerMessage(/* ... */): Promise<void> { + if (this.closing) return; // ... }As per coding guidelines, “Behavior changes in
src/require a focused regression test near the existing tests for that subsystem.”Also applies to: 693-693
🤖 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/adapters/cursor/live-transport.ts` around lines 661 - 675, Update the live transport close/cancellation flow around startShellCleanup() and cancelCursorRun() to set a shared closing flag before cleanup begins, and have frameWork/handleServerMessage() ignore queued execServerMessage frames while closing. Add a focused regression test that queues a backgroundShellSpawnArgs frame behind another frame, closes the transport, and verifies no child process starts.Sources: Coding guidelines, Path instructions
869-891: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease retained partial-frame bytes on every terminal path.
At
src/adapters/cursor/live-transport.tsLines 902-918, a partial Connect frame becomes the retainedpendingbuffer. EOF, abort, and decode failures can then settle the transport without callingreleaseTransportBytes(connectBufferedPayloadBytes(pending)).The shared
translatorBudgetkeeps that charge after the transport ends. Repeated interrupted streams can exhaust the Cursor transport budget and reject later requests.Add one idempotent pending-buffer cleanup helper. Call it from both terminal settler callbacks and from close/cancellation paths. Add a regression test that sends a partial payload, terminates the stream, and verifies that retained Cursor transport bytes return to zero.
As per coding guidelines, “Behavior changes in
src/require a focused regression test near the existing tests for that subsystem.”Also applies to: 902-921
🤖 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/adapters/cursor/live-transport.ts` around lines 869 - 891, Ensure retained partial-frame bytes are released on every terminal, close, and cancellation path. Add an idempotent pending-buffer cleanup helper near the pending Connect-frame handling, invoke it from both terminal settler callbacks and close/cancellation flows, and preserve safe repeated invocation. Add a focused regression test alongside the existing live-transport tests that sends a partial payload, terminates the stream, and verifies retained Cursor transport bytes return to zero.Sources: Coding guidelines, 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/_plan/260801_zero_leak_state_stores/055_background_shell_lifecycle.md`:
- Around line 308-315: Update resetBackgroundShellStateForTests() to restore
backgroundShellShutdownStarted to false along with the other global state, and
assert after reset that the shutdown fence is inactive so subsequent spawn tests
start unfenced.
- Around line 287-304: Sanitize the tracked plan in
`055_background_shell_lifecycle.md` by removing unreleased security details,
including native-execution bypass reasoning, failure modes, and regression
cases. Retain only a high-level roadmap, and move the detailed plan to an
untracked `.tmp/` location or a `mktemp -d` artifact; do not leave those details
under `devlog/**/*`.
- Around line 241-256: Update startShellCleanup() so a settled report with
unresolved entries does not remain cached; retain the in-flight promise for
concurrent callers, then clear shellCleanup after completion when
BackgroundShellTerminationReport.unresolved is greater than zero so later
close() or cancelCursorRun() calls retry cleanup.
In `@src/adapters/cursor/native-exec-shell.ts`:
- Around line 495-500: Update the stdout and stderr handlers in the background
shell flow around noteBackgroundShellActivity to use the chunk’s direct byte
length, preserving accurate offsets without decoding buffers; retain a
Buffer.byteLength fallback only for string chunks produced by setEncoding.
- Around line 536-538: Update writeShellStdinExec around the
shell.child.stdin.write call to reject writes when the child has exited or
termination has begun, including entries marked terminating, and convert write
failures such as EPIPE or ERR_STREAM_WRITE_AFTER_END into the protocol’s typed
WriteShellStdinError rather than allowing them to escape. Preserve normal writes
for active shells, and add a regression test beside the ownership test in
tests/cursor-native-exec-shell.test.ts using a fake child whose stdin has ended
and asserting a typed error result.
In `@src/server/lifecycle.ts`:
- Around line 175-179: Update both allSettled rejection logs in
drainAndShutdown: at src/server/lifecycle.ts lines 175-179, replace the generic
rejected count with shellResult.reason, using only Error.message for Error
instances; at src/server/lifecycle.ts lines 184-186, capture the settled
response-state flush result and append its reason using the same
Error.message-or-raw-reason handling to the failure warning. Do not log whole
Error objects.
In `@tests/cursor-native-exec-shell.test.ts`:
- Around line 369-382: Update the cross-session test around writeShellStdinExec
so it unconditionally asserts that result.value.result is an error before
narrowing to inspect the error text. Preserve the existing exact rejection
message and owner-secret exclusion checks, using the local throw-to-narrow
convention demonstrated by spawnSuccess and spawnErrorText.
---
Outside diff comments:
In `@src/adapters/cursor/live-transport.ts`:
- Around line 661-675: Update the live transport close/cancellation flow around
startShellCleanup() and cancelCursorRun() to set a shared closing flag before
cleanup begins, and have frameWork/handleServerMessage() ignore queued
execServerMessage frames while closing. Add a focused regression test that
queues a backgroundShellSpawnArgs frame behind another frame, closes the
transport, and verifies no child process starts.
- Around line 869-891: Ensure retained partial-frame bytes are released on every
terminal, close, and cancellation path. Add an idempotent pending-buffer cleanup
helper near the pending Connect-frame handling, invoke it from both terminal
settler callbacks and close/cancellation flows, and preserve safe repeated
invocation. Add a focused regression test alongside the existing live-transport
tests that sends a partial payload, terminates the stream, and verifies retained
Cursor transport bytes return to zero.
🪄 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: 97ef88d8-efda-428b-9746-9e87fd8a014e
📒 Files selected for processing (11)
devlog/_plan/260801_zero_leak_state_stores/055_background_shell_lifecycle.mdsrc/adapters/cursor/live-transport.tssrc/adapters/cursor/native-exec-shell.tssrc/adapters/cursor/native-exec.tssrc/server/lifecycle.tstests/cursor-live-transport.test.tstests/cursor-native-exec-policy.test.tstests/cursor-native-exec-shell.test.tstests/cursor-native-exec.test.tstests/cursor-transport-retry.test.tstests/shutdown-drain.test.ts
| private startShellCleanup(): Promise<BackgroundShellTerminationReport> { | ||
| return this.shellCleanup ??= terminateBackgroundShellsForSession(this.sessionId); | ||
| } | ||
|
|
||
| async close(): Promise<void> { | ||
| // existing transport/MCP teardown | ||
| await this.startShellCleanup(); | ||
| } | ||
| ``` | ||
|
|
||
| This is contract-compatible with `CursorTransport.close(): void | Promise<void>` at | ||
| `src/adapters/cursor/transport.ts:5-9`; retry already awaits close at | ||
| `transport-retry.ts:80-91,121-127`. There is no fire-and-forget shell cleanup in | ||
| `close()`. `cancelCursorRun()` at `live-transport.ts:663` may start the same promise | ||
| without awaiting in its synchronous cancellation path, but the eventual `close()` awaits | ||
| that exact promise. Repeated close/cancel calls cannot launch competing cleanup passes. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not cache an unresolved cleanup result permanently.
this.shellCleanup ??= terminateBackgroundShellsForSession(...) caches the settled promise. If the report contains unresolved > 0, later close() or cancelCursorRun() calls reuse that report and do not retry the session cleanup. This conflicts with the retry requirement at Lines 190-192. Cache only in-flight attempts, or clear the cached promise when unresolved entries remain.
🤖 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/_plan/260801_zero_leak_state_stores/055_background_shell_lifecycle.md`
around lines 241 - 256, Update startShellCleanup() so a settled report with
unresolved entries does not remain cached; retain the in-flight promise for
concurrent callers, then clear shellCleanup after completion when
BackgroundShellTerminationReport.unresolved is greater than zero so later
close() or cancelCursorRun() calls retry cleanup.
| ## Disabled-by-default contract | ||
|
|
||
| Behavior remains unchanged at all current policy/config anchors: | ||
|
|
||
| - `cursorUnsafeNativeLocalExecEnabled()` at | ||
| `src/adapters/cursor/native-exec.ts:73-75` remains explicit-true only. | ||
| - `resolveCursorNativeExecMode()` at `src/adapters/cursor/exec-policy.ts:17-20` keeps | ||
| unset/default as `"off"`; `effectiveCursorNativeExecAllow()` at `:41-44` keeps only | ||
| `"on"` enabled. | ||
| - `src/types.ts:1133-1152` keeps the unsafe opt-in warning and | ||
| `"off" | "codex-sandbox" | "on"` contract. | ||
| - `docs-site/src/content/docs/reference/configuration.md:435-446` keeps native tools | ||
| disabled by default and `"codex-sandbox"` fail-closed. | ||
| - Policy dispatch at `src/adapters/cursor/native-exec.ts:476-487` still rejects before | ||
| reaching allowed spawn/stdin dispatch. | ||
|
|
||
| No new config field enables shells. The legacy unsafe boolean remains compatibility only; | ||
| docs keep warning that the opt-in bypasses Codex approvals/sandboxing. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Move unreleased security details out of tracked devlog/.
This tracked plan contains pre-disclosure hardening details, native-execution bypass reasoning, failure modes, and regression cases. The devlog/**/* path instruction prohibits storing this material in tracked directories. Keep a sanitized roadmap here and move the detailed plan to .tmp/ or a mktemp -d artifact.
🤖 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/_plan/260801_zero_leak_state_stores/055_background_shell_lifecycle.md`
around lines 287 - 304, Sanitize the tracked plan in
`055_background_shell_lifecycle.md` by removing unreleased security details,
including native-execution bypass reasoning, failure modes, and regression
cases. Retain only a high-level roadmap, and move the detailed plan to an
untracked `.tmp/` location or a `mktemp -d` artifact; do not leave those details
under `devlog/**/*`.
Source: Path instructions
| child.stdout.on("data", chunk => { | ||
| const shell = backgroundShells.get(shellId); | ||
| if (shell) shell.outputLength += Buffer.byteLength(String(chunk)); | ||
| noteBackgroundShellActivity(entry, Buffer.byteLength(String(chunk))); | ||
| }); | ||
| child.stderr.on("data", chunk => { | ||
| const shell = backgroundShells.get(shellId); | ||
| if (shell) shell.outputLength += Buffer.byteLength(String(chunk)); | ||
| noteBackgroundShellActivity(entry, Buffer.byteLength(String(chunk))); | ||
| }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Use the chunk byte length directly instead of decoding and re-encoding.
Lines 496 and 499 compute Buffer.byteLength(String(chunk)). chunk is already a Buffer, so this decodes the whole chunk to a JS string and then re-encodes it to count bytes. Two problems follow.
First, the count can be wrong. If a multi-byte UTF-8 character straddles a chunk boundary, String(chunk) produces U+FFFD and the re-encoded length no longer equals the bytes that arrived. outputLength is returned to the client as terminalFileLengthBeforeInputWritten, so the reported offset drifts.
Second, it allocates. Every stdout and stderr chunk of every background shell pays a full decode plus a full re-encode on a hot pipe, purely to obtain a number the Buffer already knows.
Use chunk.length, with a Buffer.byteLength fallback for the string case that setEncoding would produce.
♻️ Proposed change
child.stdout.on("data", chunk => {
- noteBackgroundShellActivity(entry, Buffer.byteLength(String(chunk)));
+ noteBackgroundShellActivity(entry, typeof chunk === "string" ? Buffer.byteLength(chunk) : chunk.length);
});
child.stderr.on("data", chunk => {
- noteBackgroundShellActivity(entry, Buffer.byteLength(String(chunk)));
+ noteBackgroundShellActivity(entry, typeof chunk === "string" ? Buffer.byteLength(chunk) : chunk.length);
});📝 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.
| child.stdout.on("data", chunk => { | |
| const shell = backgroundShells.get(shellId); | |
| if (shell) shell.outputLength += Buffer.byteLength(String(chunk)); | |
| noteBackgroundShellActivity(entry, Buffer.byteLength(String(chunk))); | |
| }); | |
| child.stderr.on("data", chunk => { | |
| const shell = backgroundShells.get(shellId); | |
| if (shell) shell.outputLength += Buffer.byteLength(String(chunk)); | |
| noteBackgroundShellActivity(entry, Buffer.byteLength(String(chunk))); | |
| }); | |
| child.stdout.on("data", chunk => { | |
| noteBackgroundShellActivity(entry, typeof chunk === "string" ? Buffer.byteLength(chunk) : chunk.length); | |
| }); | |
| child.stderr.on("data", chunk => { | |
| noteBackgroundShellActivity(entry, typeof chunk === "string" ? Buffer.byteLength(chunk) : chunk.length); | |
| }); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/adapters/cursor/native-exec-shell.ts` around lines 495 - 500, Update the
stdout and stderr handlers in the background shell flow around
noteBackgroundShellActivity to use the chunk’s direct byte length, preserving
accurate offsets without decoding buffers; retain a Buffer.byteLength fallback
only for string chunks produced by setEncoding.
| const before = shell.outputLength; | ||
| shell.child.stdin.write(args.chars); | ||
| noteBackgroundShellActivity(shell); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the stdin write against an already-exited child.
Line 537 calls shell.child.stdin.write(args.chars) with no guard. A background shell stays in backgroundShells until its close event runs confirmBackgroundShellClose. Between the child exiting and that event being delivered, the map still holds the entry, so a writeShellStdinArgs request reaches line 537 and writes to an ended stream. Node then emits EPIPE or ERR_STREAM_WRITE_AFTER_END on the stream. writeShellStdinExec is synchronous and has no try/catch, and the stream has no error listener, so the rejection surfaces as an unhandled stream error rather than the typed WriteShellStdinError the protocol defines.
Termination widens the window: terminateBackgroundShell line 373 calls entry.child.stdin.end(), and a quarantined entry stays registered with terminating set. A write after that point always hits an ended stream.
Wrap the write and return the typed error. Also reject writes once termination has begun.
🛡️ Proposed fix
+ if (shell.terminating || !shell.child.stdin.writable) {
+ return execBytes(execMsg, "writeShellStdinResult", create(WriteShellStdinResultSchema, {
+ result: { case: "error", value: create(WriteShellStdinErrorSchema, { error: "shell is no longer accepting input" }) },
+ }));
+ }
const before = shell.outputLength;
- shell.child.stdin.write(args.chars);
+ try {
+ shell.child.stdin.write(args.chars);
+ } catch (err) {
+ return execBytes(execMsg, "writeShellStdinResult", create(WriteShellStdinResultSchema, {
+ result: { case: "error", value: create(WriteShellStdinErrorSchema, { error: errorText(err) }) },
+ }));
+ }
noteBackgroundShellActivity(shell);Add a regression test next to the ownership test in tests/cursor-native-exec-shell.test.ts that ends the fake child's stdin, then asserts a typed error result instead of a throw.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/adapters/cursor/native-exec-shell.ts` around lines 536 - 538, Update
writeShellStdinExec around the shell.child.stdin.write call to reject writes
when the child has exited or termination has begun, including entries marked
terminating, and convert write failures such as EPIPE or
ERR_STREAM_WRITE_AFTER_END into the protocol’s typed WriteShellStdinError rather
than allowing them to escape. Preserve normal writes for active shells, and add
a regression test beside the ownership test in
tests/cursor-native-exec-shell.test.ts using a fake child whose stdin has ended
and asserting a typed error result.
| if (shellResult.status === "rejected") { | ||
| console.warn("[cursor] background shell drain failed", { rejected: 1 }); | ||
| } else if (shellResult.value.unresolved > 0 || shellResult.value.killFailures > 0) { | ||
| console.warn("[cursor] background shell drain incomplete", shellResult.value); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Shutdown discards Promise.allSettled rejection reasons. Both new allSettled sites in drainAndShutdown capture a rejection and then log a message that omits the captured reason. The storage branch in the same function at lines 200-207 logs result.reason, so the shutdown log is inconsistent: a failed shell drain or a failed state flush is reported as a fact with no cause, and the operator has nothing to act on. Log the message only, never the whole error object, so no future error field can serialize command text or credentials.
src/server/lifecycle.ts#L175-L179: replace{ rejected: 1 }withshellResult.reason instanceof Error ? shellResult.reason.message : shellResult.reason.src/server/lifecycle.ts#L184-L186: capture the settled result in a variable and appendresponseStateFlush[0].reason instanceof Error ? ... .message : ...to the"[responses] state flush during shutdown failed"warning.
📍 Affects 1 file
src/server/lifecycle.ts#L175-L179(this comment)src/server/lifecycle.ts#L184-L186
🤖 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/server/lifecycle.ts` around lines 175 - 179, Update both allSettled
rejection logs in drainAndShutdown: at src/server/lifecycle.ts lines 175-179,
replace the generic rejected count with shellResult.reason, using only
Error.message for Error instances; at src/server/lifecycle.ts lines 184-186,
capture the settled response-state flush result and append its reason using the
same Error.message-or-raw-reason handling to the failure warning. Do not log
whole Error objects.
| test("cross-session stdin write is rejected without revealing owner identity", () => { | ||
| const fake = installFakeShellRuntime(); | ||
| const shellId = spawnSuccess(backgroundShellSpawnExec(spawnArgs(), "owner-secret")); | ||
| const result = decodedExec(writeShellStdinExec(execMessage({ | ||
| case: "writeShellStdinArgs", | ||
| value: create(WriteShellStdinArgsSchema, { shellId, chars: "steal" }), | ||
| }), "other-session")); | ||
| expect(result.case).toBe("writeShellStdinResult"); | ||
| if (result.case === "writeShellStdinResult" && result.value.result.case === "error") { | ||
| expect(result.value.result.value.error).toBe("shell belongs to another session"); | ||
| expect(result.value.result.value.error).not.toContain("owner-secret"); | ||
| } | ||
| fake.children[0]!.emit("close", 0, null); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the cross-session rejection assertion unconditional.
Lines 377-380 place the security-relevant assertions inside if (result.case === "writeShellStdinResult" && result.value.result.case === "error"). If a regression made writeShellStdinExec accept a foreign session and return a success result, the condition would be false, the body would be skipped, and the test would still pass. The one unconditional assertion at line 376 only checks the message case, not that the write was refused.
This test guards session ownership on a stdin channel, so it must fail loudly when ownership stops being enforced. Assert the error case first, then narrow.
💚 Proposed change
expect(result.case).toBe("writeShellStdinResult");
- if (result.case === "writeShellStdinResult" && result.value.result.case === "error") {
- expect(result.value.result.value.error).toBe("shell belongs to another session");
- expect(result.value.result.value.error).not.toContain("owner-secret");
- }
+ if (result.case !== "writeShellStdinResult") throw new Error("expected writeShellStdinResult");
+ expect(result.value.result.case).toBe("error");
+ if (result.value.result.case !== "error") throw new Error("expected a rejected stdin write");
+ expect(result.value.result.value.error).toBe("shell belongs to another session");
+ expect(result.value.result.value.error).not.toContain("owner-secret");
fake.children[0]!.emit("close", 0, null);The file already uses this throw-to-narrow style in spawnSuccess at lines 123-125 and spawnErrorText at lines 132-134, so this matches the local convention.
📝 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.
| test("cross-session stdin write is rejected without revealing owner identity", () => { | |
| const fake = installFakeShellRuntime(); | |
| const shellId = spawnSuccess(backgroundShellSpawnExec(spawnArgs(), "owner-secret")); | |
| const result = decodedExec(writeShellStdinExec(execMessage({ | |
| case: "writeShellStdinArgs", | |
| value: create(WriteShellStdinArgsSchema, { shellId, chars: "steal" }), | |
| }), "other-session")); | |
| expect(result.case).toBe("writeShellStdinResult"); | |
| if (result.case === "writeShellStdinResult" && result.value.result.case === "error") { | |
| expect(result.value.result.value.error).toBe("shell belongs to another session"); | |
| expect(result.value.result.value.error).not.toContain("owner-secret"); | |
| } | |
| fake.children[0]!.emit("close", 0, null); | |
| }); | |
| test("cross-session stdin write is rejected without revealing owner identity", () => { | |
| const fake = installFakeShellRuntime(); | |
| const shellId = spawnSuccess(backgroundShellSpawnExec(spawnArgs(), "owner-secret")); | |
| const result = decodedExec(writeShellStdinExec(execMessage({ | |
| case: "writeShellStdinArgs", | |
| value: create(WriteShellStdinArgsSchema, { shellId, chars: "steal" }), | |
| }), "other-session")); | |
| expect(result.case).toBe("writeShellStdinResult"); | |
| if (result.case !== "writeShellStdinResult") throw new Error("expected writeShellStdinResult"); | |
| expect(result.value.result.case).toBe("error"); | |
| if (result.value.result.case !== "error") throw new Error("expected a rejected stdin write"); | |
| expect(result.value.result.value.error).toBe("shell belongs to another session"); | |
| expect(result.value.result.value.error).not.toContain("owner-secret"); | |
| fake.children[0]!.emit("close", 0, 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/cursor-native-exec-shell.test.ts` around lines 369 - 382, Update the
cross-session test around writeShellStdinExec so it unconditionally asserts that
result.value.result is an error before narrowing to inspect the error text.
Preserve the existing exact rejection message and owner-secret exclusion checks,
using the local throw-to-narrow convention demonstrated by spawnSuccess and
spawnErrorText.
Scoring OpenCodex against its own sixteen benchmark categories exposed holes the phase reviews had each individually accepted: bodies were admitted after allocation, management routes parsed unbounded JSON, preflight heartbeats accumulated without cap, spill publication never fsynced the directory entry, the provider and crash rings lacked boundary tests, the periodic sweeper missed the continuation and replay stores, a stale Grok flight could block applies until it settled, the pinned headroom above the eviction target had no proven ceiling, and a timed-out ACL hardening still published the secret. All eight now have finite contracts with boundary tests, including a 512 MiB worst-case pinned ceiling computed from the per-store caps themselves.
The matrix is now populated from commit-pinned sources on both sides: OpenCodex scores sixteen of sixteen categories PASS after the gap-closure commit, while the surveyed TypeScript proxies — Portkey Gateway, Claude Code Router, and mcp-proxy at their 2026 heads — carry FAIL or UNKNOWN cells in body admission, stream bounds, concurrency caps, and secret handling. Claude Code Router stands closest, with a genuinely strong continuation archive and diagnostic budgets. The superiority sentence is deliberately omitted: the cohort still has UNKNOWN cells, and the rule this document set for itself does not bend for a good-looking column. Open gap count: 0.
…on API The repair rounds that landed the translator budget also refined its contract — the transient-reservation lease for overlap accounting, the 16 MiB effective contiguous-payload admission, and the frame-count bounds beside the byte pool. The decade doc now records the shipped shapes so the unit closes with documentation that matches the code.
2 participants
Summary
This lands the first four work-phases of the zero-leak unit (
devlog/_plan/260801_zero_leak_state_stores/), attacking the unbounded in-memory growth behind the recent RSS climb.33402140flineage): the Responses continuation store gets a byte cap; overflow spills to disk atomically (fsync + link-no-replace, generation-unique basenames) with a spill-failed tombstone and a bounded deferred-unlink queue (128 entries).Each phase went through independent adversarial review (4 rounds each, final PASS) recorded in the devlog unit.
Testing
bun run test: 6570 pass / 3 skip / 0 fail (465 files)bun x tsc --noEmit: cleanbun run privacy:scan: passNotes for reviewers
Remaining phases of the unit (registry admission caps, appOwnedBytes observability, translator stream bounds, shell lifecycle, benchmark) will stack on this PR.
Summary by CodeRabbit
New Features
Bug Fixes