deliverer: empty NextPhase means stay-in-phase, not task completion

[bborbe]

Diagnosis

StepRunner.Run (agent_runner.go) publishes EVERY step result via deliverer.DeliverResult. Preflight steps in multi-step phases return {Status: Done, ContinueToNext: true} with empty NextPhase. The deliverer's AgentStatusDone case called resolveNextPhase, which mapped empty NextPhase to "done" — writing phase: done, status: completed on a live task. This contradicts the documented Result.NextPhase contract (agent_step.go): "Empty means stay in current phase — used for in-place saves between steps in a multi-step phase."

Real-world impact: github-update-go-agent's planning preflight republish marked a live task done/completed mid-run for ~13 min (self-healed only because a later result overwrote it). Every multi-step agent with Done+ContinueToNext preflights is affected.

Fix

• delivery/result-deliverer.go: AgentStatusDone with empty NextPhase is now an in-place save exactly like the AgentStatusInProgress branch — status: in_progress, phase preserved from incoming frontmatter, no resolveNextPhase call. Terminating a task now requires an explicit NextPhase: "done". resolveNextPhase keeps its normalize+warn path for non-empty values (invalid still falls back to "done").
• delivery/content-generator.go (applyStatusFrontmatter): required companion fix — the generators previously wrote phase: done, status: completed unconditionally on Done, so the Kafka deliverer's "preserve phase" would have preserved the generator-clobbered done. Now: Done+empty NextPhase → status: in_progress, phase untouched; Done+non-empty → phase = normalized NextPhase, status: completed only when resolved phase is done (mirrors the Kafka deliverer; also fixes the file-deliverer path).
• agent_status.go / agent_runner.go: AgentResultInfo gains ContinueToNext, forwarded by StepRunner (informational — Done+empty NextPhase is an in-place save regardless).
• docs/task-flow-and-failure-semantics.md: happy-path scenario updated to require explicit NextPhase: "done".

Consumer sweep (this repo)

Steps that returned Status: Done with empty NextPhase and RELIED on the empty→done fallback to complete tasks — each given an explicit NextPhase: "done":

• healthcheck/healthcheck-claude-step.go (claudeStep.Run)
• healthcheck/healthcheck-gemini-step.go (geminiStep.Run)
• healthcheck/healthcheck-nop-step.go (nopStep.Run)
• healthcheck/healthcheck-pi-step.go (piStep.Run)

Inspected, no change needed (config/LLM-driven NextPhase, empty already documented as in-place save):

• claude/agent-step.go — AgentStepConfig.NextPhase ("Empty means in-place save")
• pi/pi-step.go — StepConfig.NextPhase
• agent_parser.go ParseStep — constructor nextPhase param
• claude/task-runner.go + claude/result-deliverer.go adapter — forwards LLM-emitted next_phase

Semantic change for downstream consumers (minor bump): any out-of-repo step or single-shot LLM agent emitting status: done WITHOUT next_phase previously completed its task; it now performs an in-place save (status: in_progress, phase unchanged). Terminal steps must emit/configure next_phase: "done" explicitly.

Tests

• Deliverer: Done+empty NextPhase (with and without ContinueToNext) → status: in_progress + phase preserved; Done+"done" → completed; Done+"execution" → phase: execution / status: in_progress; InProgress branch unchanged.
• Content generators: Done+empty → in-place save; Done+"done" → completed/done; Done+"execution" → execution/in_progress.
• StepRunner forwards ContinueToNext to the deliverer.
• Healthcheck steps assert explicit NextPhase: "done".

make precommit green (tests, lint, security, formatting).

Follow-up

Controller-side staleness guard as defense-in-depth (reject/flag terminal writes that regress a newer task state) — separate issue.

[ben-s-pull-request-reviewer]

Now I have a thorough understanding of the code. Let me compile my findings.

Correctness review of the core fix:

1. delivery/result-deliverer.go:141-151 — AgentStatusDone with NextPhase == "" now sets frontmatter["status"] = "in_progress" and breaks before any phase write. The phase is preserved from fmMap copied at line 126-131. This is the correct fix — early break is reached before any frontmatter writes. ✓

2. delivery/content-generator.go:54-62 — applyStatusFrontmatter mirrors the same logic. Signature changed from (content, status) to (content, result AgentResultInfo). The fallbackContentGenerator, passthroughContentGenerator, and sectionContentGenerator all call it correctly. ✓

3. agent_runner.go:70-76 — ContinueToNext is forwarded from Result to AgentResultInfo. ✓

4. agent_status.go — ContinueToNext field added with clear documentation that it's informational only. ✓

5. Healthcheck steps — All four (claude, gemini, nop, pi) now explicitly set NextPhase: "done". ✓

6. resolveNextPhase — Empty-string handling removed (correctly, since all callers now check empty before calling). ✓

7. Tests — The old test for sets phase=done when done result has empty NextPhase was correctly removed and replaced with two tests verifying in-place save behavior with and without ContinueToNext. ✓

Minor gap found: fallbackContentGenerator and sectionContentGenerator don't have explicit tests for Done+empty NextPhase → in-place save, relying on the passthroughContentGenerator test and shared applyStatusFrontmatter code path. This is acceptable but weaker than having per-generator coverage.

---

{
  "verdict": "approve",
  "summary": "The fix correctly implements the documented Result.NextPhase contract: empty NextPhase on AgentStatusDone is an in-place save (status: in_progress, phase preserved) rather than task completion. The early-break in result-deliverer:141 is reached before any frontmatter writes. All four healthcheck steps were updated to emit explicit NextPhase: \"done\". The ContinueToNext field was added for deliverer visibility but does not alter the branching logic.",
  "comments": [
    {
      "file": "delivery/content-generator_test.go",
      "line": 45,
      "severity": "nit",
      "message": "fallbackContentGenerator has no explicit test for Done+empty NextPhase (in-place save). Coverage relies on passthroughContentGenerator test + shared applyStatusFrontmatter. Consider adding a direct fallback test for defense in depth."
    },
    {
      "file": "delivery/content-generator_test.go",
      "line": 45,
      "severity": "nit",
      "message": "sectionContentGenerator has no explicit test for Done+empty NextPhase (in-place save). Same coverage gap as fallbackContentGenerator."
    },
    {
      "file": "docs/task-flow-and-failure-semantics.md",
      "line": 111,
      "severity": "minor",
      "message": "The PR description mentions 'execution' as a valid phase value (e.g., NextPhase=execution → phase: execution / status: in_progress), but the docs at line 111 only list 'planning, in_progress, ai_review, human_review, done'. Consider adding 'execution' to the happy-path doc for consistency."
    }
  ],
  "concerns_addressed": [
    "correctness: AgentStatusDone with empty NextPhase now produces status: in_progress (phase preserved) — early-break in result-deliverer.go:141 reached before any frontmatter writes",
    "correctness: applyStatusFrontmatter signature changed to take AgentResultInfo; all three generators (fallback, passthrough, section) updated to pass full result",
    "correctness: Done+non-empty NextPhase resolves phase and only marks completed when resolved phase is terminal",
    "correctness: resolveNextPhase no longer handles empty string — callers check empty before calling",
    "correctness: All four healthcheck steps (claude, gemini, nop, pi) now emit explicit NextPhase: done",
    "correctness: ContinueToNext field added to AgentResultInfo — informational only, does not change deliverer branching",
    "correctness: StepRunner.Run forwards ContinueToNext from Result to AgentResultInfo",
    "tests: Old test 'sets phase=done when done result has empty NextPhase' removed and replaced with two new tests verifying in-place save (with and without ContinueToNext)"
  ]
}

[ben-s-pull-request-reviewer]

Based on my thorough review of the changed files, here is my assessment:

Review Summary

The PR is a correct, well-tested bug fix. The core behavioral change is sound: AgentStatusDone with empty NextPhase now means "in-place save" (preserve phase, status: in_progress) rather than "task completion" (phase: done, status: completed). Both kafkaResultDeliverer and applyStatusFrontmatter (content generators) implement the same semantics. resolveNextPhase is only called for non-empty NextPhase, which is safe. All four healthcheck steps now emit explicit NextPhase: "done".

Must Fix: None

Should Fix (Important): None

Nice to Have (Optional):

1. healthcheck/healthcheck-pi-step.go:53 — NextPhase: "done" is present and correct, but there is no healthcheck-pi-step_test.go. All other healthcheck steps (claude, gemini, nop) have dedicated test files that assert result.NextPhase == "done". The missing test leaves healthcheck-pi unverified despite being part of the same semantic contract.

2. docs/task-flow-and-failure-semantics.md:80–101 — The "Result Routing" table still describes the pre-PR behavior where done unconditionally maps to phase = done / status = completed. It does not distinguish the Done + empty NextPhase in-place-save path from Done + explicit NextPhase: "done" terminal path. The happy-path description on line 111 does mention the new semantics, but the routing table above it is stale.

{
  "verdict": "approve",
  "summary": "The bug fix is correct and well-tested. AgentStatusDone with empty NextPhase now correctly produces status: in_progress with phase preserved (in-place save) instead of phase: done / status: completed. Both Kafka deliverer and content generators implement identical semantics. All four healthcheck steps now emit explicit NextPhase: done. One minor test gap: healthcheck-pi-step lacks a dedicated test file unlike the other three healthcheck steps. Documentation in task-flow-and-failure-semantics.md has a stale Result Routing table that pre-dates the behavioral change.",
  "comments": [
    {
      "file": "healthcheck/healthcheck-pi-step.go",
      "line": 53,
      "severity": "nit",
      "message": "nice-to-have: NextPhase: done is correct but no corresponding _test.go file exists — claude, gemini, and nop steps each have dedicated tests asserting NextPhase == done"
    },
    {
      "file": "docs/task-flow-and-failure-semantics.md",
      "line": 80,
      "severity": "nit",
      "message": "nice-to-have: Result Routing table describes pre-PR behavior (done → phase: done, status: completed unconditionally) and does not reflect the new Done+empty NextPhase in-place-save semantics"
    }
  ],
  "concerns_addressed": [
    "correctness: AgentStatusDone+empty NextPhase writes status: in_progress, phase preserved — confirmed in result-deliverer.go:141-151 and content-generator.go:52-62, both paths identical",
    "correctness: applyStatusFrontmatter signature changed to take AgentResultInfo — confirmed all three call sites (fallbackContentGenerator, passthroughContentGenerator, sectionContentGenerator) pass result correctly",
    "correctness: resolveNextPhase only called for non-empty NextPhase — confirmed at result-deliverer.go:153, early return removed; content-generator.go:64 calls it only when NextPhase != ''",
    "correctness: AgentResultInfo gains ContinueToNext — confirmed in agent_status.go:46-50, StepRunner forwards at agent_runner.go:75",
    "correctness: healthcheck steps emit explicit NextPhase: done — confirmed in all four files (claude-step:56, gemini-step:58, nop-step:33, pi-step:53)",
    "tests: result-deliverer_test.go covers Done+empty NextPhase (with/without ContinueToNext), Done+execution, Done+done, Done+invalid — comprehensive",
    "tests: content-generator_test.go covers Done+done, Done+empty, Done+execution — confirmed for fallback and passthrough generators"
  ]
}