Conversation
… dedup, single-count, failed-row preservation, integration test) Closes the Codex and independent reviewer punch list against PR #61. C1 runtime refresh after cadence drain. EvolutionEngine now owns an onConfigApplied callback that fires from both the normal apply path and the CycleAborted partial-apply recovery whenever applied.length > 0. Wired into src/index.ts next to setQueueWiring so the AgentRuntime evolved config snapshot refreshes after every drain that applies changes. Pre-fix the queued path rewrote phantom-config/ files but the live agent kept its boot-time snapshot until process restart. C2 failed queue rows now stay in the queue. cadence.runDrain filters the batch result to ok-only ids before markProcessed and emits a warn line per failed row including the queue id and error string. Failed rows retry on the next drain instead of being silently deleted, which was reintroducing the pre-Phase-2 drop-on-floor shape for the exact sessions the safety floor exists to protect. C3 gate Haiku call drops the claude_code preset envelope. New omitPreset option on JudgeQueryOptions, flipped on only at the gate call site, swaps the SDK systemPrompt from the preset envelope to a plain string. The SDK accepts both shapes; the plain-string path skips the full Claude Code base prompt and tool catalog. Live fleet data showed gate cost running 20 to 180x the research target because of preset overhead. Other judges keep the preset envelope unchanged. M1 collapses session_count triple-increment to single-increment per fired session. Removed updateAfterSession from enqueueIfWorthy and from the top of afterSessionInternal. The remaining call site inside runCycle has the real hadCorrections value after observation extraction. The Phase 0 M4 mutex-skip increment is obsolete now that Phase 2 replaced the drop-on-floor path with a persistent queue: every enqueued session eventually reaches runCycle. M2 adds a phase-1-2 integration test that wires the real EvolutionEngine, the real EvolutionQueue over in-memory sqlite, the real EvolutionCadence, and a stub AgentRuntime. Two tests cover queue depth post-drain, single-count metrics, runtime refresh callback firing, and failed-row retention through two drains. Single highest leverage item in the punch list; covers every coordination concern between engine, queue, cadence, and runtime in one fixture. M3 dedups queue rows by session_key. EvolutionQueue.enqueue wraps a DELETE WHERE session_key = ? and the INSERT in a sqlite transaction so the dedup is atomic with respect to a concurrent drainAll. No migration required. A busy multi-turn session that crosses the gate threshold multiple times between drains now collapses to a single row with the latest summary, instead of running the full Sonnet judge pipeline once per turn against progressively shorter snapshots. Test plan - bun test: 1385 pass / 10 skip / 0 fail (+11 over the 1374 baseline) - bun run lint: clean - bun run typecheck: clean
![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: 3917fc1d71
ℹ️ 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".
| console.warn(`[evolution] queue row id=${entry.id} pipeline failed, leaving in queue: ${entry.error}`); | ||
| } | ||
| } | ||
| this.queue.markProcessed(okIds); |
There was a problem hiding this comment.
Keeping failed rows in the queue is correct for durability, but deleting only ok IDs means the same row can be processed multiple times, and runCycle increments session_count/success_count before several later failure points (for example, an exception after observation extraction). With this change, one transiently failing row can inflate session metrics and skew auto-rollback/consolidation decisions on every retry. Consider making metrics idempotent per queue row/session or deferring session-counter updates until the row is successfully finalized.
Useful? React with 👍 / 👎.
| console.log( | ||
| `[evolution] Applied ${applied.length} changes (v${this.getCurrentVersion()}) in ${Date.now() - startTime}ms`, | ||
| ); | ||
| this.notifyConfigApplied(); |
There was a problem hiding this comment.
The new config-refresh callback is invoked immediately after apply, but auto-rollback is evaluated later in the same cycle; if rollback triggers, disk state moves back while runtime has already been refreshed to the pre-rollback version. Since rollback() does not call notifyConfigApplied, queued evolution can leave runtime and on-disk config diverged exactly in the safety-critical rollback case.
Useful? React with 👍 / 👎.
Two follow-up fixes for findings Codex raised against the PR #63 fix pass. - engine.ts: dedup updateAfterSession by session.session_key so the M1 single-count contract holds under the C2 retry path. A failed row that retries on the next drain re-enters runCycle under the same key; the new countedSessionKeys Set guarantees one increment per unique conversation for the engine's lifetime. Memory is bounded by unique session_keys; process restart clears the set, which is an acceptable per-restart blip. - engine.ts: call notifyConfigApplied() from rollback() so the auto-rollback branch in runCycle no longer leaves the runtime serving a snapshot newer than the disk state. getConfig() re-reads from disk on every call, so the callback observes the post-rollback content with no extra in-memory work. Tests: +2 (retry dedup in phase-1-2-integration.test.ts, rollback callback in engine.test.ts). 1387 pass / 10 skip / 0 fail. Lint and typecheck clean.
1 participant
Summary
Follow-up PR that closes the combined Codex + independent reviewer punch list against PR #61 (Phase 1 conditional firing gate + Phase 2 SQLite cadence batching). Three CRITs, three MAJORs, all shipped. Optional MINOR items deferred.
CRITs
C1. Wire runtime refresh after cadence drain
The queued path in
enqueueIfWorthyreturns noinlineResult, so the.thencallback insrc/index.tsnever calledruntime.setEvolvedConfigafter a drain. When the cadence drains a batch and applies changes to disk,AgentRuntime's in-memoryevolvedConfigsnapshot stayed at boot-time state until process restart. Effective result: the queued path silently disabled live evolution.Fix:
EvolutionEnginenow owns anonConfigAppliedcallback, set via a newsetOnConfigAppliedmethod next tosetQueueWiring. It fires from both the normal apply path inrunCycleand theCycleAbortedpartial-apply recovery, wheneverapplied.length > 0. Wired insrc/index.tsto callruntime.setEvolvedConfig(evolution.getConfig())on every fire.notifyConfigAppliedswallows callback errors so a telemetry/refresh failure cannot wedge the pipeline.C2. Preserve failed queue rows for retry
cadence.runDrainwas callingmarkProcessedwith every drained id regardless of per-row success. Transient pipeline failures silently deleted rows fromevolution_queue, defeating the whole durability argument for SQLite backing.Fix: filter to
ok=trueids only beforemarkProcessed, emit a warn log per failed row including id and error string. Failed rows retry on the next drain.C3. Strip the
claude_codepreset overhead from the gateObserved per-call gate cost was 20x to 180x the ~$0.0006 research target across the live fleet. Root cause:
runJudgeQueryinjectssystemPrompt: { type: "preset", preset: "claude_code", append: gatePrompt }, which bundles the full Claude Code base prompt plus the tool catalog into every Haiku call. The gate is a pure evaluation call and does not need any of that overhead.Fix: new
omitPreset?: booleanoption onJudgeQueryOptions. When true, the SDK receives a plain-stringsystemPromptinstead of the preset envelope. Verified againstnode_modules/@anthropic-ai/claude-agent-sdk/sdk.d.ts:1281— the SDK explicitly accepts both shapes, and the plain-string path is documented as "Use a custom system prompt" which skips the preset's base prompt. Flipped on only at the gate call site. Every other judge (observation, constitution, regression, safety, quality) keeps the preset.Extracted
buildSystemPromptas an exported helper so the invariant is unit-testable without mocking the SDK's streamingquery().Predicted cost reduction: 10x to 100x depending on VM and cache state. Post-deploy verification reads
gate_stats.haiku_cost_usd_total / total_decisionson the fleet.MAJORs
M1. Collapse session_count triple-increment to single-increment
Phase 0's M4 fix moved
updateAfterSessionto the top ofafterSessionso the mutex-skip path would still bump the counter. Phase 2 replaced the drop-on-floor model with a persistent queue, so the M4 premise is obsolete: every enqueued session eventually hitsrunCycle. But the M4 call was still there, plus a new one insideenqueueIfWorthy, plus the original one insiderunCycle. Fired sessions were counted 3x.Fix: deleted the calls in
enqueueIfWorthyand at the top ofafterSessionInternal. Kept only the call insiderunCyclewherehadCorrectionsis known. Reverted the engine test expectation from 2 back to 1 and rewrote the Phase 0 mutex-skip test to assertactiveCycleSkipCount === 2instead of the old 3x count.M2. End-to-end integration test across engine + queue + cadence + runtime
Every CRIT and MAJOR in this review is a coordination bug between modules, and the existing unit tests stub the engine so none of them could catch any of it. New test file
src/evolution/__tests__/phase-1-2-integration.test.tswires the realEvolutionEngine, realEvolutionQueueover:memory:, realEvolutionCadence, and a stubAgentRuntime. Two tests cover queue depth post-drain, single-count metrics, runtime refresh callback firing, and failed-row retention through two drains. 283 LOC.M3. Dedup queue rows by session_key
A busy multi-turn session that crosses the gate threshold multiple times between drains was producing multiple queue rows, each triggering the full Sonnet judge pipeline on the drain. Cost amplifier scaling with conversation length.
Fix:
EvolutionQueue.enqueuewrapsDELETE FROM evolution_queue WHERE session_key = ?and theINSERTin adb.transaction(...). No migration required. A repeated enqueue for the samesession_keyreplaces the prior row with the latest summary.MINORs
evolution-gate-log.jsonldatareactionsintoSessionSummary: deferred, out of scopejudge_costs: deferred, Phase 3 cost tracking restructuresession_keyformat: deferred, one-line cleanup outside the in-scope file listTest plan
bun test: 1385 pass / 10 skip / 0 fail (+11 new tests over the PR feat: evolution conditional firing gate and cadence batching #61 baseline of 1374)bun run lint: cleanbun run typecheck: cleangate_stats.haiku_cost_usd_total / total_decisionsdrops into the sub-cent range on at least one VM with active gate trafficmetrics.session_countbumps by exactly 1 per drained session (not 3)Notes
engine.tsis now 607 lines, up from 580. Past the 600-line guideline. Phase 3 split target — intentionally not split here because Phase 3's rewrite deletes most of the 6-judge bulk and splitting first would produce throwaway work.