fix(ship): agent-mode pauses were treated as LLM failures, stubbing artefacts

CHANGELOG.md

@@ -4,6 +4,16 @@ All notable changes to forge will be documented in this file. Format follows [Ke
 
 ## [Unreleased]
 
+## [1.10.2] — 2026-08-20 — Agent mode stopped pausing: a bridge miss was treated as an LLM failure
+
+### Fixed
+
+- **`forge ship --agent-mode` silently stubbed spec/arch/test/breakdown/code instead of pausing for a real turn.** Every checkpoint that generates an artefact via `LLMPipe` funnelled `generateWithValidation`'s error straight into its generic "LLM failed → write a stub, log a failure" branch, without ever checking whether that error was actually `ErrAgentTurn` — a *pause*, not a failure. In agent mode a bridge miss on, say, the arch checkpoint was therefore treated exactly like a real provider error: the checkpoint overwrote whatever was on disk (or nothing, pre-emptively) with a stub template and recorded a false failure in the learned-failures file used as future prompt context. A second, independent instance of the same root cause lived in the per-checkpoint post-processing loop: because the pause was reported as `cp.Status == "ok"`, the completion-marker writer — which writes `<checkpoint>.md` whenever a checkpoint didn't fail — ran anyway and wrote placeholder "Status: warning / Evidence: none" content into the checkpoint's own primary artefact file (e.g. `arch.md`) before the host agent had answered anything. Once that file existed, the next `forge ship --agent-mode` invocation saw it as "already done" and moved straight to the next checkpoint, repeating the mistake — the net effect being a single run that could stamp broken stubs across several checkpoints in a row while only ever showing the user the first turn.
+
+  Every checkpoint (`spec`, `arch`, `test`, `breakdown`, `code`) now checks `IsAgentTurn` before falling back to a stub, and a paused checkpoint is marked `AgentPaused` so the post-checkpoint hooks, evidence policy, digest, and completion-marker steps are all skipped for it rather than run against an artefact that doesn't exist yet.
+
+- **Reusing the same agent-mode session across unrelated features could replay one feature's answers into another's.** The bridge's ordinal-fallback replay (used when a prompt's hash has drifted but its position in the run has not) is keyed only on `operation#N`, with no feature scoping. Driving a second feature through the same `default` session — the common case, since `--session` is opt-in — meant its Nth call to a given operation (e.g. `ship:qa-verify:generate`) could silently hit the *first* feature's recorded answer for that same position instead of asking a fresh question. `Bridge.SetFeature` now detects when a session already belongs to a different feature/slug and resets the session's recorded responses before adopting the new one; `forge ship --agent-mode` prints a note when this happens so a reused session isn't a silent trap.
+
 ## [1.10.1] — 2026-08-08 — Scanner false positives, and a QA scenario that had been red since 1.9.0
 
 ### Fixed

go.mod

@@ -2,7 +2,7 @@ module github.com/teragrid/forge
 
 go 1.25.0
 
-toolchain go1.26.5
+toolchain go1.26.6
 
 require (
 	github.com/spf13/cobra v1.10.2

internal/agentbridge/agentbridge.go

@@ -224,13 +224,33 @@ func (b *Bridge) SessionName() string { return b.session.Name }
 
 // SetFeature records what feature this session is driving. Best-effort: a
 // persistence failure never blocks the pipeline.
-func (b *Bridge) SetFeature(feature, slug string) {
+//
+// If the session already has recorded responses from a *different* feature
+// (its persisted Slug/Feature does not match), those responses are discarded
+// first. Otherwise the ordinal fallback in Lookup — keyed only on
+// "operation#N", with no feature scoping — would happily replay another
+// feature's answer into this run merely because both features' Nth call to
+// the same operation (e.g. "ship:qa-verify:generate") landed in the same
+// position. That is silent cross-feature state contamination: the host agent
+// never sees a prompt for the new feature, it just gets served the old
+// feature's stale artefact under the new feature's name. Reusing the default
+// session across unrelated features (rather than passing --session per
+// feature) is exactly the case this guards.
+func (b *Bridge) SetFeature(feature, slug string) (switched bool) {
 	if b == nil {
-		return
+		return false
+	}
+	prevSlug, prevFeature := b.session.Slug, b.session.Feature
+	hadPrior := prevSlug != "" || prevFeature != ""
+	newIdentity := slug != "" || feature != ""
+	if hadPrior && newIdentity && prevSlug != slug && prevFeature != feature {
+		_ = b.Reset()
+		switched = true
 	}
 	b.session.Feature = feature
 	b.session.Slug = slug
 	_ = b.saveSession()
+	return switched
 }
 
 // Paused reports whether a turn has been requested during this process run.

internal/agentbridge/agentbridge_test.go

@@ -1,3 +1,3 @@
 // Copyright 2024 The Forge Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
@@ -441,3 +441,47 @@
 			strings.Count(out, Fence), out)
 	}
 }
+
+// TestSetFeature_SwitchingFeatureResetsStaleSession is a regression test for
+// cross-feature state contamination: SetFeature recorded the new
+// feature/slug but never checked whether the session already belonged to a
+// different feature. The ordinal fallback in Lookup is keyed only on
+// "operation#N" with no feature scoping, so replaying default-session state
+// into a second, unrelated feature could silently serve the first feature's
+// recorded answer for the second feature's Nth call to the same operation —
+// e.g. its qa-verify tasks — instead of asking a fresh question.
+func TestSetFeature_SwitchingFeatureResetsStaleSession(t *testing.T) {
+	t.Parallel()
+	root := t.TempDir()
+
+	b := mustOpen(t, root, DefaultSession)
+	if switched := b.SetFeature("agency master-calendar", "agency-master-calendar"); switched {
+		t.Fatal("first SetFeature call on a fresh session must not report a switch")
+	}
+	if _, err := b.Lookup("ship:qa-verify:generate", "qa-verify", "", "sys", "usr", 100); !errors.Is(err, ErrTurnRequired) {
+		t.Fatalf("expected a pause, got %v", err)
+	}
+	if _, err := b.Fulfil("master-calendar tasks answer"); err != nil {
+		t.Fatalf("Fulfil: %v", err)
+	}
+	if got := b.Stats().Responses; got != 1 {
+		t.Fatalf("expected 1 recorded response before the switch, got %d", got)
+	}
+
+	// Reopen as a fresh process would, then point the same default session at
+	// an unrelated feature.
+	b2 := mustOpen(t, root, DefaultSession)
+	if switched := b2.SetFeature("checkout redesign", "checkout-redesign"); !switched {
+		t.Fatal("SetFeature must report a switch when the session already belonged to a different feature")
+	}
+	if got := b2.Stats().Responses; got != 0 {
+		t.Fatalf("switching features must discard the prior feature's recorded answers, %d remain", got)
+	}
+	// The Nth call (N=1) to the same operation for the new feature must ask a
+	// fresh question rather than replaying the old feature's answer via the
+	// ordinal fallback.
+	content, lookupErr := b2.Lookup("ship:qa-verify:generate", "qa-verify", "", "sys", "usr", 100)
+	if !errors.Is(lookupErr, ErrTurnRequired) {
+		t.Fatalf("expected a fresh pause for the new feature, got content=%q err=%v", content, lookupErr)
+	}
+}

internal/cli/cmdship/arch.go

@@ -230,6 +230,9 @@ func checkArch(root, description, specName string, pipe *LLMPipe) Checkpoint {
 		// J8/J9: strip conversational preamble and detect truncation before
 		// writing; retry once on an incomplete response.
 		generated, complete, err := generateWithValidation(generateFn)
+		if agentPauseCheckpoint(&cp, "ship:arch:generate", err) {
+			return cp
+		}
 		switch {
 		case err != nil:
 			archContent = archStub(description)

internal/cli/cmdship/ship.go

@@ -115,6 +115,35 @@ var (
 		"forge agent prompt  # read the turn, then: forge agent submit --file <path>")
 )
 
+// agentPauseCheckpoint reports whether genErr is a paused agent-mode turn
+// (IsAgentTurn) rather than a genuine LLM/provider failure, and if so fills
+// in cp with a neutral "awaiting host-agent turn" status.
+//
+// Every checkpoint that generates an artefact via an LLMPipe must call this
+// before falling back to a stub/appendFailure on error. Without it, a pause
+// — the bridge has recorded a pending turn and is waiting for the host agent
+// to answer — was indistinguishable from a real LLM failure: the checkpoint
+// would overwrite whatever artefact was on disk with a stub template and log
+// a false failure to the learned-failures file, while the bridge's own pause
+// latch (Bridge.Paused) silently prevented later checkpoints in the same run
+// from doing any real work either — so a single miss on, say, arch cascaded
+// into every remaining checkpoint being stubbed out instead of the pipeline
+// stopping to show the real prompt. See the outer caller in the ship command,
+// which renders the pending turn from bridge state after RunWithOptions
+// returns — that render is unaffected by what any individual checkpoint's
+// Status/Detail says, but the stub writes and appendFailure calls are not,
+// which is what made this worth guarding at every call site rather than
+// relying on the outer check alone.
+func agentPauseCheckpoint(cp *Checkpoint, operation string, genErr error) bool {
+	if !IsAgentTurn(genErr) {
+		return false
+	}
+	cp.Status = "ok"
+	cp.Detail = "awaiting host-agent turn for " + operation + " — run: forge agent prompt"
+	cp.AgentPaused = true
+	return true
+}
+
 // ExitAgentTurn is the process exit code for a paused agent-mode run.
 //
 // 78 is chosen from the sysexits.h convention (EX_CONFIG, "configuration
@@ -138,6 +167,17 @@ type Checkpoint struct {
 	Debate            *DebateResult    `json:"debate,omitempty"`             // populated when --yolo self-debate runs
 	GapAudit          *SpecAuditResult `json:"gap_audit,omitempty"`          // TG-39: spec-vs-code audit result
 	RemediationRounds int              `json:"remediation_rounds,omitempty"` // rounds of LLM-driven gap remediation
+	// AgentPaused is true when this checkpoint did not run to completion but
+	// instead deferred to a host-agent turn (agent mode). It is not a failure
+	// and not a success — no artefact was produced, so post-checkpoint
+	// side effects (quality-gate hooks, evidence policy, digests, and the
+	// completion-marker file) must all be skipped: running them against a
+	// nonexistent artefact either errors, reports false "unverified" noise,
+	// or — for the completion marker specifically, whose path is the
+	// checkpoint's own primary artefact file (e.g. arch.md) — writes bogus
+	// placeholder content into the exact file the deferred turn was supposed
+	// to produce, corrupting it before the host agent ever answers.
+	AgentPaused bool `json:"-"`
 	// Evidence records what this checkpoint's status actually rests on. A
 	// status of "ok" requires at least one entry from an independent source —
 	// see evidence.go. Emitted in --json so a reviewer or CI job can audit the
@@ -431,7 +471,12 @@ func New() *cobra.Command {
 				return errcode.New(ErrAgentTurn, "open agent bridge", bErr)
 			}
 			bridge.StrictReplay = strictReplay
-			bridge.SetFeature(description, specName)
+			if bridge.SetFeature(description, specName) {
+				fmt.Fprintf(cmd.ErrOrStderr(),
+					"note: session %q was driving a different feature — recorded answers reset for %q "+
+						"(use --session <name> to run multiple features concurrently without this)\n",
+					agentSession, description)
+			}
 			runOpts.AgentBridge = bridge
 			// Interactive y/N gates would block a chat-driven run between
 			// turns, and the host agent has no stdin to answer them with.
@@ -831,6 +876,9 @@ func checkSpec(root, description, specName string, pipe *LLMPipe) Checkpoint {
 				// J8/J9: strip conversational preamble and detect truncation
 				// before overwriting an existing, presumably-good spec.md.
 				reviewed, complete, reviewErr := generateWithValidation(reviewFn)
+				if agentPauseCheckpoint(&cp, "ship:spec:review", reviewErr) {
+					return cp
+				}
 				if reviewErr != nil {
 					cp.Status = "ok"
 					if ySpec != nil {
@@ -906,6 +954,9 @@ func checkSpec(root, description, specName string, pipe *LLMPipe) Checkpoint {
 					}
 					// J8/J9: strip preamble and detect truncation before writing.
 					generated, genComplete, genErr := generateWithValidation(genFn)
+					if agentPauseCheckpoint(&cp, "ship:spec:generate-from-yaml", genErr) {
+						return cp
+					}
 					switch {
 					case genErr != nil:
 						specContent = specStub(description)
@@ -969,6 +1020,9 @@ func checkSpec(root, description, specName string, pipe *LLMPipe) Checkpoint {
 				}
 				// J8/J9: strip preamble and detect truncation before writing.
 				generated, genComplete, genErr := generateWithValidation(genFn)
+				if agentPauseCheckpoint(&cp, "ship:spec:generate", genErr) {
+					return cp
+				}
 				switch {
 				case genErr != nil:
 					specContent = specStub(description)
@@ -1103,6 +1157,9 @@ func checkTest(root, description, specName string, pipe *LLMPipe, dryRun bool) C
 		applyReachability(root, testFiles, &cp)
 		if pipe != nil {
 			if _, err := generateTestStubs(root, description, slug, pipe); err != nil {
+				if agentPauseCheckpoint(&cp, "ship:test:generate", err) {
+					return cp
+				}
 				cp.Detail += fmt.Sprintf(" [LLM:%s — %s]", pipe.ProviderName(), llmErrNote(err))
 			}
 		}
@@ -1111,6 +1168,9 @@ func checkTest(root, description, specName string, pipe *LLMPipe, dryRun bool) C
 	// No test files — generate 4 named artifacts.
 	if pipe != nil {
 		if _, err := generateTestStubs(root, description, slug, pipe); err != nil {
+			if agentPauseCheckpoint(&cp, "ship:test:generate", err) {
+				return cp
+			}
 			cp.Status = "warning"
 			cp.Detail = fmt.Sprintf("no test files; 4 artifacts written to tests/%s.* [LLM:%s — %s]",
 				slug, pipe.ProviderName(), llmErrNote(err))
@@ -1248,6 +1308,9 @@ func checkBreakdown(root, description, specName string, pipe *LLMPipe) Checkpoin
 		// Breakdown does not exist — attempt LLM generation.
 		if pipe != nil {
 			generated, err := generateBreakdown(root, description, slug, pipe)
+			if agentPauseCheckpoint(&cp, "ship:breakdown:generate", err) {
+				return cp
+			}
 			if err != nil {
 				cp.Status = "warning"
 				cp.Detail = fmt.Sprintf("no breakdown.md [LLM:%s — %s] — run forge ship breakdown to generate",
@@ -1298,6 +1361,9 @@ func checkCode(root, description, specName string, pipe *LLMPipe) Checkpoint {
 
 	if pipe != nil {
 		plan, err := generateCodePlan(root, description, slug, pipe)
+		if agentPauseCheckpoint(&cp, "ship:code:generate", err) {
+			return cp
+		}
 		if err != nil {
 			if changedFiles > 0 {
 				cp.Status = "ok"
@@ -2158,6 +2224,18 @@ func runWithOptions(opts RunOptions) *ShipResult {
 	total := len(selected)
 
 	for i, cp := range selected {
+		// A checkpoint that deferred to a host-agent turn produced no
+		// artefact — none of hooks, evidence policy, digesting, or the
+		// completion marker have anything real to inspect, and running them
+		// anyway is actively harmful (the completion marker in particular
+		// would write placeholder content into the checkpoint's own artefact
+		// path). The outer `forge ship` command renders the pending turn from
+		// bridge state regardless of what res contains, so it is safe to stop
+		// here without evaluating the rest of the loop body for this entry.
+		if cp.AgentPaused {
+			res.Checkpoints = append(res.Checkpoints, cp)
+			return res
+		}
 		// ── Post-checkpoint quality-gate hooks ───────────────────────────────
 		// Hooks run after the check* function and can annotate cp.Detail with
 		// warnings or escalate status to "fail" when HookConfig.Strict is set.

internal/cli/cmdship/ship_test.go

@@ -1,3 +1,3 @@
 // Copyright 2024 The Forge Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
@@ -38,6 +38,7 @@
 
 	"github.com/spf13/cobra"
 
+	"github.com/teragrid/forge/internal/agentbridge"
 	"github.com/teragrid/forge/internal/cli/cmdtest"
 	"github.com/teragrid/forge/internal/llmprovider"
 )
@@ -2473,3 +2474,84 @@
 		t.Fatalf("expected the no-runner-found fallback (warning, \"\"), got status=%q detail=%q", status, detail)
 	}
 }
+
+// ── Regression: an agent-mode pause must not be treated as an LLM failure ─────
+//
+// Root cause: checkSpec/checkArch/checkBreakdown/checkCode/checkTest all
+// funnelled generateWithValidation's error straight into their generic "LLM
+// failed, write a stub" branch. IsAgentTurn(err) was never checked, so a
+// bridge miss (a *pause*, not a failure) was indistinguishable from a real
+// provider error, and the checkpoint clobbered whatever artefact was on disk
+// with a stub template. A second, independent bug in this file's own
+// per-checkpoint post-processing loop compounded it: because the pause
+// reported cp.Status == "ok", the completion-marker writer ran anyway and
+// wrote placeholder content into the checkpoint's own primary artefact file
+// (e.g. arch.md) before the host agent had answered anything — so the next
+// invocation saw that file as "already done" and moved on, repeating the
+// mistake on the next checkpoint. Net effect: a single run could silently
+// stamp broken stubs across spec/arch/test/breakdown/code instead of pausing
+// once per checkpoint for a real answer.
+func TestAgentMode_ArchPauseDoesNotStubTheArtefact(t *testing.T) {
+	t.Parallel()
+	root := t.TempDir()
+
+	runShipAgentOnce := func() error {
+		cmd := New()
+		var out bytes.Buffer
+		cmd.SetOut(&out)
+		cmd.SetErr(&out)
+		cmd.SetArgs([]string{"add rate limiting", "--root", root, "--agent-mode"})
+		err := cmd.Execute()
+		if !IsAgentTurn(err) {
+			t.Fatalf("expected a pause, got %v\n%s", err, out.String())
+		}
+		return err
+	}
+
+	// Turn 1: spec pauses.
+	_ = runShipAgentOnce()
+	b, _ := agentbridge.Open(root, agentbridge.DefaultSession)
+	if _, err := b.Fulfil("# Spec\n\n## Acceptance Criteria\n- [ ] works\n"); err != nil {
+		t.Fatalf("Fulfil spec: %v", err)
+	}
+
+	slug := "add-rate-limiting"
+	assertNoStubsYet := func() {
+		t.Helper()
+		for _, artefact := range []string{"arch.md", "test-stubs.md", "breakdown.md", "code-plan.md"} {
+			p := filepath.Join(root, ".forge", "specs", slug, artefact)
+			if fi, statErr := os.Stat(p); statErr == nil {
+				data, _ := os.ReadFile(p)
+				t.Fatalf("%s must not exist yet (size=%d) — the pipeline should have paused for a real "+
+					"answer instead of writing a stub while a turn was still owed:\n%s", artefact, fi.Size(), string(data))
+			}
+		}
+	}
+
+	// Drive the run forward, answering whatever turn comes up (spec.md
+	// existing now routes through a review turn before arch), until arch's
+	// own generation turn is reached. At every step, no *later* checkpoint's
+	// artefact must have been stubbed out while a prior turn was still owed.
+	const maxTurns = 5
+	reachedArch := false
+	for i := 0; i < maxTurns; i++ {
+		_ = runShipAgentOnce()
+
+		bn, _ := agentbridge.Open(root, agentbridge.DefaultSession)
+		pending, ok := bn.Pending()
+		if !ok {
+			t.Fatalf("turn %d: expected a pending turn", i)
+		}
+		assertNoStubsYet()
+		if pending.Operation == "ship:arch:generate" {
+			reachedArch = true
+			break
+		}
+		if _, err := bn.Fulfil("placeholder host-agent answer for " + pending.Operation); err != nil {
+			t.Fatalf("turn %d: Fulfil %s: %v", i, pending.Operation, err)
+		}
+	}
+	if !reachedArch {
+		t.Fatalf("never reached the arch generation turn within %d turns", maxTurns)
+	}
+}