Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ThomasK33
added this pull request to stack #4359
September 23, 2026 05:46
ThomasK33
force-pushed
the
compaction-fp4j-l3-token-budget-context
branch
3 times, most recently
from
September 23, 2026 06:15
6f8ea2d to
83f90d6
Compare
Move token-budget admission, warnings, final memory flush, rollover, and recovery orchestration into the controller-owned TokenBudgetStrategy. Keep synchronous generation/admission receipts and the session-owned publication and stream lifecycle boundaries intact. Migrate characterization tests to controller-private strategy seams and cover stale preparations and synchronous continuation ownership. Validation: 69 isolated suites / 2136 tests, 3 mock IPC suites / 13 tests, and make static-check passed. D3 passed desktop and 375px mobile checks across two explicitly linked captures; mobile added no feature requests. The separate baseline visibility prerequisite remains its own commit. --- _Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$1371.86`_ <!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=1371.86 -->
In the shared Bun unit run, WorkspaceStore.test.ts replaces global.queueMicrotask with a synchronous stub and never restores it. The recovery fence witness queued its "microtask" marker through that global, so when the two suites shared a process the marker was recorded inside the fence check itself, ahead of the protected call, and the test failed for a reason unrelated to the session (expected validate:2, got microtask:2). Queue the marker as a real promise reaction instead. It lands in the same microtask queue, so the no-await invariant is unchanged: inserting an await between the fence check and applyContextResetSideEffects still fails the witness, standalone and alongside WorkspaceStore.test.ts. --- _Generated with `xum` • Model: `coder:anthropic/claude-fable-5-1` • Thinking: `xhigh` • Cost: `$8.82`_ <!-- mux-attribution: model=coder:anthropic/claude-fable-5-1 thinking=xhigh costs=8.82 -->
Call the session-owned controller's token-budget query directly at the existing evaluation points. Document app-factory/controller lifetimes and execution-lease ownership. Keep automatic request construction on the host because it resolves live workspace settings and the shared provider/auth view. Local Phase 5 implementation checkpoint, not completed acceptance. Focused tests and make static-check-full pass on this source tree. The required full-unit gate remains blocked: the last shared run had 27 failures and one error; the new fence witness was subsequently fixed and verified in its owning Phase 4 layer. The untouched Storybook budget also fails identically on an exported af4e317 baseline (109 > 79 files, 555 > 305 snapshots). D4 is prepared but has not run. No gate is waived. --- _Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$1432.32`_ <!-- mux-attribution: model=coder:openai/gpt-6-astra thinking=xhigh costs=1432.32 -->
ThomasK33
force-pushed
the
compaction-fp4j-l3-token-budget-context
branch
from
September 23, 2026 06:35
83f90d6 to
6c64594
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Move Token Budget orchestration (admission, warnings, rollover, legacy flush recovery) out of
AgentSessioninto a controller-ownedTokenBudgetStrategy, then remove the forwarding shim soAgentSessionholds no strategy-owned state. Behavior is unchanged. This finishes the extraction that #4266 started for Summarize and Continuous.Layer 3 of 3. Most of the diff is relocation:
strategies/tokenBudget.tsis +1,081 lines andagentSession.tsis +139/−1,037.Background
AgentSessionowned admission, the stream pipeline, and history, and it also decided which context-management strategy ran and kept each strategy's state. #4266 moved Summarize and Continuous behind a per-sessionSessionContextController(src/node/services/contextManagement/). Token Budget stayed inAgentSessionbehind a transitional forwarding shim.Implementation
contextManagement/strategies/tokenBudget.ts: the Token Budget strategy, moved fromAgentSessionwith its admission, advisory, rollover, and recovery logic. It keeps the synchronous generation/admission receipts:validatePreparation(receipt)still runs immediately before dispatch with no interveningawait.SessionContextControllerowns the strategy per session.AgentSessioncalls the controller's token-budget query at the existing evaluation points. The shim and the strategy-owned fields (isTokenBudgetActive,onContextBudgetStepSettled, budget state) are gone.SessionContextHostgains only the members the strategy needs; the session still owns publication, queue mutation, and the stream lifecycle.di/appRuntime.tsdocuments lifetimes:ContextManagementis app-lifetime, controllers are session-owned, and in-flight work holds leases throughenterExecution.queueMicrotaskcannot fake a failure.Validation
make test-uniton the validated change set (600fd38b3: these three layers on266625dd, before the stack was rebuilt onto currentmain): 20,013 pass, 9 skip, 0 fail across 967 files; Storybook/DOM 42/42.BUN_JSC_useWasmIPInt=0applied to that invocation only (Bun 1.3.5 Wasm crash, Bun #17841).make static-check-fullpassed on the same change set.make static-check, the context-management andagentSession.*suites, and the compaction IPC mock suites pass on this head.make dev-server-sandbox, with screenshots and video. Every row below passed; the stub recorded each request./compact.new_contextor forced rollover withContinue. Manual/compactstill summarizes. With PTC+RLM on, settings show Token Budget as inactive.Risks
Medium surface, low expected behavior change. The strategy governs when a context window rolls over. The main regression risk is an
awaitinserted before a synchronous admission point or reset. Deferred-barrier tests guard that, along with the unchanged characterization suites.Follow-ups, pre-existing and not changed here:
applyDurablycan assert "partial to be committed first" (continuousCompactor.ts:750, since 🤖 feat: add continuous compaction experiment #4086). The assertion is caught and recovery state is preserved.applyJsoncEditsre-parses the document once per edit.📋 Implementation Plan (excerpt: summary and phases 4–5; the full plan exceeds GitHub's body limit)
0. Summary
Extract the three context-management strategies out of
AgentSessioninto one app-provided Effect service,ContextManagement, that opens a session-owned controller perAgentSession. Strategies become internal adapters behind the controller.AgentSessionkeeps admission, the stream pipeline, follow-up dispatch, completion publication and history authority; it stops knowing which automatic strategy is active or how its orchestration works.Six increments: Phase 0 tests · 1 selection · 2 seam/DI/shared collaborators · 3 automatic arbitration (Summarize + Continuous, which are interleaved today) · 4 Token Budget · 5 cleanup. Each leaves a usable system with current behavior preserved; unmigrated code stays exactly where it is until its owning phase. Semantics changes (per-turn pinning, idle-timer relocation) are out of scope.
Baseline
fbbea2b16403ceb6f52f71af33a13a06c9c466fa,effect@4.0.0-rc.112. Research artifact (outside repo):/home/coder/.cache/compaction-fp4j-research/compaction-effect-research.md.Net product LoC (estimate, all phases): ≈ 0 to +200 overall;
agentSession.ts−900 to −1000. Per phase below. Estimates, not gates.Phase 4 — Token Budget behind the seam
Work:
TokenBudgetStrategyowns fields:1038–1049,prepareContextBudgetSend, batch prefix/rollover (:4521–4584) →proceed{prefixRows, assemblySnapshot, receipt};checkContextBudgetBeforeSend→reject;stepSettlement(stream)reproduces:8253predicate (budgetActive(options) || stream.contextBudgetFlushTurn);onContextBudgetStepSettledenqueues flush/continue viahost.enqueueContinuation(same dedupe keys, thinking override, 1-step bound);onSendFailureforcontext_budget_exceeded(:8278–8285), budget retry:8700;degradeFlushEntryToContinuation; budget clear folded into the budget-clearing reasons of §4.1 and to the front ofonUserInterrupt;transitionalCompactionHandlerdeleted. Session callsvalidatePreparation(receipt)synchronously where:4572–4585/:6128–6136check today.Transaction boundary: The strategy prepares rollover rows, snapshots and continuation policy. The session retains full request assembly, candidate disposal/transfer, accepted snapshot-ID remapping and file-baseline association, context-reset side effects, durable publication/rollback, and retry dispatch. Capture receipts at the original generation checkpoints, not during earlier prefix preparation. The final generation/admission check and invocation of its protected operation stay in one synchronous segment; the entire recovery transaction is not atomic. Persisted flush restoration yields only in branches that already awaited admission. Prepared rollover requests always install settlement; ordinary stream starts retain the active-mode-or-admitted-flush predicate, with live stream capture when the callback runs.
Acceptance:
agentSession.tokenBudget.test.ts,streamManager.contextBudget.test.ts,agentSession.memoryContext.test.tsunchanged (agentSession.budgetGate.test.tscovers goal monetary budgets; runs as a regression gate only); flush turn hidden/1-step/restricted tools;new_contextrollover; unknown limit skips tokenization; Deferred-barrier test proving no await betweenvalidatePreparationand dispatch.Gate: suites above + all previous;
make static-check; dogfood D3. LoC: ≈ +430 moved+glue / −420 (net +10 to +60).Phase 5 — Cleanup
Work: remove strategy-specific fields/methods left in
AgentSession(isTokenBudgetActive,getContinuousCompactionContext,onContextBudgetStepSettled,continuousCompactor, budget fields); movebuildAutoCompactionRequestinto the controller if all its inputs are strategy-local (otherwise it stays a host method — decided by inspection, not assumed); doc note indi/appRuntime.ts(ContextManagementapp-lifetime; controllers session-owned; leases viaenterExecution). No Effect-native conversions in this phase (§2, §3.5).Acceptance:
AgentSessiondeclares none of the listed strategy-owned fields or methods; calls to controller queries may retain their names. Notransitional*member remains; all gates green. (The ≈ −900 to −1000 line reduction is an expectation, not a criterion.)Gate:
make test-unit;make static-check-full; dogfood D4. LoC: ≈ −150 to −250.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh• Cost:$3484.72