Repository navigation
fix(agent): preserve compaction calibration and completed summaries - #6656
Conversation
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I reviewed the complete five-file diff at e66d165e644ba9b9dc1e9f5d4a54a725ee382a4b, plus the surrounding automatic-context worker lifecycle, checkpoint construction, EventLog checkpoint validation and history-tail selection. I found no confirmed P0–P2 issue.
The calibration change is supported by the production query: latest_main.rs computes projection_current from later tool_result_archived events. The prior measured request remains usable as a baseline when the rendered history changes. calibration() still requires matching model, provider-route digest, connection and checkpoint, positive measured input and an estimated breakdown. I extracted its actual old/new Rust implementations and ran 14 assertions covering the reduced and increased rendered input, matching/mismatched identities, missing data and saturating arithmetic. The new implementation passes; the old one fails the archived-projection regression. This check uses typed request/usage and route-digest adapters, not a provider or the EventLog.
For normal completion, steps.rs selects finish() only for StepExit::Terminal(Completed); other exits retain drain(). The shared stop path cancels the worker and joins it through the existing cleanup/error handling. finish() records an available checkpoint through the same append-and-clear-loaded path used by adoption; a cancelled/failed summary with no candidate is not turned into a checkpoint. Checkpoint construction retains the captured covered_through, source digest and predecessor, and EventLog tail selection starts after that captured boundary rather than after the later append. I checked those source paths and the new text/native restart regression's tail assertions.
I did not run the complete engine, the new asynchronous persistence/restart regression, actual provider compaction or cleanup fault injection locally. The author's 9 automatic-compaction and 15 adjacent integration passes, and the unit-suite timeout/isolated rerun, remain the author's results. My independent execution here is limited to the extracted calibration check and its old-code control.
The actual Git merge against rust-preview at a61a85d145e7eae0f381aabb08ed69a7c2084c46 is clean. Exact-head CI 42148361185 is still running at this check; this is not a full-CI-success claim. No new stored shape or migration is introduced in this diff.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Reuse the existing lifecycle regression to cover FinishTurn with a yielded cell; remove the standalone regression scaffolding. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I reviewed the update from e66d165e to df5f6a0f4b42a244d95e0084d0dcb3bc2e9594ad and the current complete five-file diff (+90/−27). I found no additional confirmed P0–P2 issue. This updates the earlier review's lifecycle description and test-coverage boundary.
The completed candidate is now returned by drain() and retained while tools settle. steps.rs appends it only after successful tool cleanup, confirmed Summary cleanup and the existing zero-materialization safety check, and only for normal Completed termination. I checked all four drain callers: handoff, native-approval parking and model-retry parking discard the candidate; the normal completion path is the only new place that records it. Cleanup errors propagate before checkpoint publication.
The calibration implementation is byte-for-byte identical to the version used in my earlier 14-assertion extracted-Rust check and old-code failure control. That evidence remains limited to typed request/usage and route-digest adapters; it does not validate a real provider or EventLog.
The previous standalone text/native restart regression and added calibration unit test were removed. The replacement extends the existing lifecycle test with a ready Summary, a yielded Code Mode cell and FinishTurn, then checks the saved text checkpoint after reopening. I read those assertions; I have not executed this revised asynchronous test or independently established native-checkpoint tail preservation. The author's updated 8 automatic-compaction and 17 adjacent integration passes remain the author's results.
The actual Git merge against a61a85d1 remains clean. The old e66d165 CI was cancelled after the head changed; its native shard fails the already identified baseline instruction-fixture issue from #6655, with the same unchanged fixture and opened/before_instructions_ack mismatch. That is not newly attributed to this PR. Current-head CI 42150991855 is still running at this check.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Summary
Tool-result archiving could make a new turn block for compaction even though its prompt had become smaller: the engine discarded measured provider usage and reverted to a larger raw estimate. Completed background summaries were also discarded when Main finished normally, forcing later turns to summarize the same history again.
Keep the matching measured baseline and estimate the change in rendered input, including archived results. When a turn completes normally, drain Summary, settle tools, run the existing safety check, then save any successfully completed candidate as an ordinary checkpoint. This also supports FinishTurn while an earlier Code Mode cell is still running. Unfinished summaries are cancelled through existing cleanup. During Main, candidates still wait for the adoption threshold or provider overflow. No new persistent state, schema migration, or compatibility branch.
Verification
--all-targets --no-deps -- -D warnings), Rust formatting, ASF source headers andgit diff --checkpassed.AI use
Tool(s) and scope: Codex investigated the reported Session, implemented the fixes and ran the regression checks. GPT-6-Astra independently reviewed correctness, test quality and implementation simplicity; its cleanup-order finding was addressed.
Checklist
Does this PR entail a change in behavior?