fix(run): address gadfly review of the checkpoint PR
executus CI / test (pull_request) Successful in 45s
executus CI / test (pull_request) Successful in 45s
Real findings from the consensus review (44 raw; heavy devstral noise): - finalizeCheckpoint is now fired from the top-of-Run defer, so it runs on EVERY exit: a panic, an early build-error return (before the run loop), AND normal completion. Previously an early return on a recovered run left its durable record unfinalized → boot recovery would retry it forever on a persistent build error. (opus + glm) - Removed the dead ActivePhase field from run.RunCheckpointState + run.ResumeState (and the battery RunCheckpoint) — phase recovery is boundary-granular (skip completed phases; the interrupted phase re-runs from its start), so ActivePhase was never written nor read. Docs across ports/checkpoint/phases now state this plainly (5-model consensus that the field + docs over-promised mid-phase resume). - CheckpointerFactory.Begin error is now logged (WARN) before degrading to non-durable, per the documented contract (was silently swallowed). (4 models) - finalizeCheckpoint logs Complete/Fail errors (was silent). - Resume phase-skip now keys off a SEPARATE resumeSkip set, not the live outputs map — a fresh run with two same-named phases no longer skips the second (the outputs map fills as phases run). (opus:max) + regression test. - Removed the dead checkpoint.factory.now field (never set). (opus + glm) - Fixed the stale phaseDeps doc (the step observer moved out of sharedOpts to per-path). Hoisted the resume guard to a local; dropped the wasted acc allocation on the resume path; documented that Save throttling is the Checkpointer's responsibility and the accumulated transcript is pre-compaction (host size-caps it). Note (carried from the PR): classifyCheckpointOutcome keys shutdown on run.ErrShutdown; mort stamps its own runengine.ErrShutdown — the mort wiring PR aliases them so errors.Is matches. New test: duplicate phase names both run on a fresh run. Full ./... green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -130,6 +130,26 @@ func TestPhases_OptionalDoesNotSwallowCancellation(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestPhases_DuplicateNamesBothRun: a fresh (non-resume) run with two phases
|
||||
// sharing a name must run BOTH — the resume-skip guard keys off a separate
|
||||
// resume set, not the live outputs map (which fills as phases run), so a phase
|
||||
// never skips a same-named sibling on a fresh run.
|
||||
func TestPhases_DuplicateNamesBothRun(t *testing.T) {
|
||||
models, fp := phaseProvider(t, fake.Reply("first"), fake.Reply("second"))
|
||||
ex := New(Config{Registry: tool.NewRegistry(), Models: models})
|
||||
ra := RunnableAgent{
|
||||
Name: "p", ModelTier: "test-model",
|
||||
Phases: []Phase{{Name: "x", SystemPrompt: "P1"}, {Name: "x", SystemPrompt: "P2"}},
|
||||
}
|
||||
res := ex.Run(context.Background(), ra, tool.Invocation{RunID: "r"}, "Q")
|
||||
if res.Err != nil {
|
||||
t.Fatalf("run error: %v", res.Err)
|
||||
}
|
||||
if n := len(fp.Calls()); n != 2 {
|
||||
t.Fatalf("both same-named phases must run on a fresh run; got %d model calls", n)
|
||||
}
|
||||
}
|
||||
|
||||
// TestPhases_HardErrorAborts: a NON-optional phase that hits a hard error (not a
|
||||
// budget/step exhaustion) aborts the pipeline; later phases do not run.
|
||||
func TestPhases_HardErrorAborts(t *testing.T) {
|
||||
|
||||
Reference in New Issue
Block a user