feat(ferment): auto-compact session at end of each step and phase#583
feat(ferment): auto-compact session at end of each step and phase#583mikenorgate wants to merge 2 commits into
Conversation
After every successful complete_ferment_step or complete_ferment_phase, automatically compact the session context and append a hidden ferment_stage_handoff entry so each stage receives the ferment plan, active phase details, and a summary of what was just completed. Implementation -------------- - state.ts: PendingCompaction type + transient pendingCompactions Map with setPendingCompaction / getPendingCompaction / clearPendingCompaction / drainPendingCompactions (drains the full map, used at agent_end so compaction fires even after complete_ferment clears getActiveId). - runtime.ts: wires all four state functions into FermentRuntime. - tools/steps.ts: records pending compaction on all three completeStep success paths (no-verify, verify-pass, tactical-pass). - tools/phases.ts: records pending compaction on completePhase success. - auto-compaction.ts (new): maybeTriggerFermentCompaction drains drainPendingCompactions() and calls ctx.compact() per ferment with custom instructions containing name / goal / successCriteria / active phase / completed step or phase summary / next up. On onComplete appends a hidden ferment_stage_handoff message. On onError: expected errors (too small, already compacted, cancelled) are silently skipped; handoff entry is always appended regardless. - index.ts: calls maybeTriggerFermentCompaction in agent_end (catch-all for any residual pending compactions after the run finishes). - events.ts: also calls maybeTriggerFermentCompaction in turn_end so that in automated-continuation mode (whole ferment = one agent run) compaction fires between phases, not just at the very end. Bug fixes applied during development -------------------------------------- 1. getActiveId() race: complete_ferment calls setActive(undefined) before agent_end fires, so the original activeId-based lookup always returned undefined. Fixed by drainPendingCompactions() which uses the fermentId stored inside the PendingCompaction object. 2. Single-run automated mode: agent_end fires only once, after complete_ferment, so no between-phase compaction occurred. Fixed by also triggering in turn_end, which fires after each LLM turn and therefore between every phase in a continuous run. Co-Authored-By: Kimchi <noreply@kimchi.dev>
Kimchi Code Review
Summary📊 Review Score: 68/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Tests cover instruction building, handoff payload shape, and basic trigger/no-op paths. However, they do not cover the scenario where a pending compaction arrives while another is in-flight, and the in-flight guard test passes vacuously because the map is already drained. 📝 Found 5 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
There was a problem hiding this comment.
📊 Review Score: 68/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 3/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Tests cover instruction building, handoff payload shape, and basic trigger/no-op paths. However, they do not cover the scenario where a pending compaction arrives while another is in-flight, and the in-flight guard test passes vacuously because the map is already drained.
📝 Found 5 issue(s). See inline comments for details.
| * Check for a pending compaction request and fire `ctx.compact()` if one exists. | ||
| * | ||
| * Called from the `agent_end` event handler so the compaction does not interrupt | ||
| * the agent loop that just finished. The in-flight guard prevents double-fire |
There was a problem hiding this comment.
🚨🔀 Concurrency
The drainPendingCompactions() call removes pending items from the map before the in-flight guard is checked. If ctx.compact() is still executing for a ferment, a subsequent maybeTriggerFermentCompaction call will drain the new pending item and then silently drop it inside triggerCompactionForPending because compactionInProgress.has(fermentId) returns true. This causes lost compaction/handoff entries for step or phase completions that occur while a previous compaction is running.
💡 Suggestion: Change the logic so that pending items are only removed from the map once triggerCompactionForPending confirms compaction is not already in-flight. For example, iterate over pendingCompactions entries, and only delete the key after passing the guard, or re-insert the item with setPendingCompaction when the guard causes an early return.
| const nextAction = buildNextActionDescription(ferment) | ||
|
|
||
| const lines: string[] = ["Preserve ferment plan details in the summary:"] | ||
|
|
There was a problem hiding this comment.
When pending.kind is 'phase', buildCustomInstructions labels the completed phase as the Active phase. A phase that was just completed is no longer active; the next activated phase is. This misleads the compaction model by describing stale state as current.
💡 Suggestion: For phase-kind pendings, emit a Completed phase: line first, then look up the phase whose status === 'active' for the Active phase: line, similar to how buildHandoffDetails resolves activePhase.
| pending, | ||
| ) | ||
| pi.sendMessage( | ||
| { |
There was a problem hiding this comment.
The onError callback invokes appendHandoffEntry() and ctx.ui?.notify() without a try/catch. If pi.sendMessage throws, the exception propagates as an unhandled error in the async callback.
💡 Suggestion: Wrap the body of onError in a try/catch block so any unexpected failure during handoff entry creation or notification is logged but not thrown.
| { id: "step-1", description: "Step" }, | ||
| ) | ||
| storageMap.set(ferment.id, ferment) | ||
| runtime.setActive(ferment) |
There was a problem hiding this comment.
The in-flight guard test does not actually exercise the guard because drainPendingCompactions already emptied the map on the first call. The second call returns early due to an empty pending list, not because of the compactionInProgress set.
💡 Suggestion: After the first maybeTriggerFermentCompaction call, insert a new pending compaction into the runtime state and invoke the function again without calling onComplete; assert that ctx.compact is still called only once and that the new pending is preserved or processed later.
| nextPhaseGoal?: string | ||
| completedStepSummary?: string | ||
| completedPhaseSummary?: string | ||
| /** Number of tokens in the session before compaction was triggered (from CompactionResult.tokensBefore) */ |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
compactionInProgress is a module-level Set, which leaks state across tests and prevents multiple isolated runtimes in the same process.
💡 Suggestion: Move the in-flight tracking into the FermentRuntime state object so it is scoped to the runtime instance and reset naturally with clearFermentState.
- Move in-flight tracking from module-level Set to FermentRuntime state (state.ts: compactionInFlight Set + markCompactionInFlight / clearCompactionInFlight / isCompactionInFlight / clearAllPendingCompactions). Scoped to runtime instance, resets on session_start, no cross-test leakage. - Fix drainPendingCompactions concurrency: previously drained ALL entries including in-flight ones, causing silent drops when a second compaction arrived while the first was still running. Now skips in-flight ferments and leaves their entry in the map for the next turn_end / agent_end to retry. - Fix active-phase label for phase-kind pending: the completed phase is no longer 'active'. buildCustomInstructions now emits 'Completed phase' for phase boundaries and looks up the next status='active' phase separately. - Wrap onError body in try/catch: pi.sendMessage can throw; the error must not propagate as an unhandled rejection from the compact callback. - Fix vacuous in-flight guard test: old test re-called maybeTriggerFerment- Compaction after the map was already drained, not while compaction was actually in-flight. New test: inserts a second pending entry while onComplete has not fired yet, asserts it is held back, fires onComplete, asserts the held entry processes on the next call. - Fix TS2783 'id is specified more than once' in auto-compaction.test.ts: makeStep/makePhase now spread overrides last (required fields only, no duplicate literals). - Clear pending compactions and in-flight set at session_start. Co-Authored-By: Kimchi <noreply@kimchi.dev>
Summary
After every successful
complete_ferment_steporcomplete_ferment_phase, the ferment pipeline now automatically:ferment_stage_handoffsession entry (whether or not compaction succeeded) containing the full context handoff — ferment name, goal, success criteria, phase details, and next action.Motivation
Long-running ferments accumulate a large session context across many phases and steps. Without compaction, the agent carries the full history of every previous phase into each new one, burning tokens and diluting focus. This feature ensures each phase starts with a fresh, compact summary focused on what matters for the next stage.
Changes
state.tsPendingCompactiontype + transientpendingCompactionsmap withset/get/clear/drainruntime.tsFermentRuntimetools/steps.tscompleteStepsuccess pathstools/phases.tscompletePhasesuccessauto-compaction.ts(new)maybeTriggerFermentCompaction: drains pending entries, callsctx.compact(), appends handoff entry inonCompleteandonErrorindex.tsmaybeTriggerFermentCompactioninagent_end(catch-all)events.tsmaybeTriggerFermentCompactioninturn_endfor between-phase compaction in automated continuation modeauto-compaction.test.ts(new)index.test.tsagent_endcompaction trigger testsDesign notes
Why
turn_end+agent_endIn automated continuation mode the whole ferment executes in a single agent run —
agent_endfires only once at the very end. Triggering inturn_end(which fires after each LLM turn) ensures compaction happens between phases during a continuous run. Theagent_endcall remains as a catch-all.Why
drainPendingCompactions()complete_fermentcallssetActive(undefined)beforeagent_endfires, sogetActiveId()returnsundefinedat that point. Storing thefermentIdinsidePendingCompactionand draining the full map sidesteps this race.Handoff entry always appended
ferment_stage_handoffis written in bothonCompleteandonError. Expected non-errors (session too small, already compacted, cancelled) are silently skipped. Unexpected errors surface viactx.ui.notify. The handoff entry is the primary deliverable; compaction itself is a best-effort optimization.Testing