Skip to content

fix(agent): preserve compaction calibration and completed summaries - #6656

Merged
M4n5ter merged 2 commits into
rust-previewfrom
fix/compaction-calibration-lifecycle
Nov 6, 2026
Merged

M4n5ter merged 2 commits into
rust-previewfrom
fix/compaction-calibration-lifecycle

Conversation

@M4n5ter

@M4n5ter M4n5ter commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

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

  • Extended the existing background-summary lifecycle test with one case: a completed Summary, a yielded Code Mode cell, and a successful FinishTurn. It verifies successful cleanup and the exact checkpoint content after Session reopening. Confirmed that it catches both discarding the candidate and publishing it before tools settle. No new test functions or standalone fixtures.
  • Automatic-compaction integration tests: 8 passed. Adjacent compaction, overflow, handoff, cancellation, yielded-cell and FinishTurn integration tests: 17 passed.
  • Strict affected-crate Clippy (--all-targets --no-deps -- -D warnings), Rust formatting, ASF source headers and git diff --check passed.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@M4n5ter
M4n5ter merged commit aee0d6f into rust-preview Nov 6, 2026
9 checks passed
@M4n5ter
M4n5ter deleted the fix/compaction-calibration-lifecycle branch November 6, 2026 02:37

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants