From 1ceeb60e8eee55d5525006ea149004c5e6532b73 Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Thu, 20 Aug 2026 23:17:04 +0700 Subject: [PATCH 1/3] fix(ship): agent-mode pauses were treated as LLM failures, stubbing artefacts Every checkpoint that generates an artefact (spec/arch/test/breakdown/code) funnelled generateWithValidation's error straight into its generic "LLM failed -> write a stub" branch without checking IsAgentTurn first. In --agent-mode, a bridge miss (a pause owed to the host agent, not a failure) was therefore indistinguishable from a real provider error: the checkpoint overwrote the artefact with a stub template and logged a false failure. A second instance of the same root cause lived in the per-checkpoint post-processing loop: 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. The next invocation then saw that file as "already done" and moved on, repeating the mistake on the next checkpoint - the observed symptom being a single run silently stamping broken stubs across arch/test/breakdown/code instead of pausing once per checkpoint. Also fixes: reusing the default agent-mode session across two different features let the ordinal-fallback replay (keyed only on "operation#N", with no feature scoping) silently serve one feature's recorded answer into an unrelated feature's run. Bridge.SetFeature now resets stale session state on a feature switch instead of leaving it to collide. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 10 ++ internal/agentbridge/agentbridge.go | 24 +++- internal/cli/cmdship/agent_mode_test.go | 140 ++++++++++++++++++++++++ internal/cli/cmdship/arch.go | 3 + internal/cli/cmdship/ship.go | 80 +++++++++++++- 5 files changed, 254 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d980b4f..694138d 100644 --- a/CHANGELOG.md +++ b/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 `.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 diff --git a/internal/agentbridge/agentbridge.go b/internal/agentbridge/agentbridge.go index ce75273..d6ff375 100644 --- a/internal/agentbridge/agentbridge.go +++ b/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. diff --git a/internal/cli/cmdship/agent_mode_test.go b/internal/cli/cmdship/agent_mode_test.go index f48cb33..d016c93 100644 --- a/internal/cli/cmdship/agent_mode_test.go +++ b/internal/cli/cmdship/agent_mode_test.go @@ -31,6 +31,9 @@ package cmdship import ( "bytes" + "errors" + "os" + "path/filepath" "strings" "testing" @@ -194,6 +197,143 @@ func TestAgentMode_IgnoresAnInjectedProvider(t *testing.T) { } } +// ── Regression: a pause must not be treated as an LLM failure ───────────────── +// +// Root cause fixed here: 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: the checkpoint clobbered whatever artefact was on disk with +// a stub template, logged a false failure, and — because the bridge's pause +// latch is set inside Lookup regardless — every checkpoint gated by +// agentPaused() correctly stopped running in *that* invocation, but the +// artefact files were already corrupted by the time it did. On the next +// invocation, the stubbed file made the checkpoint look "already done" and +// the pipeline moved on to the next checkpoint, which repeated the same +// mistake — net effect: a single run silently stamped 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() + + // Turn 1: spec pauses. + if _, err := runShipAgent(t, root); !IsAgentTurn(err) { + t.Fatalf("expected spec to pause, got %v", err) + } + 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(except ...string) { + t.Helper() + skip := map[string]bool{} + for _, s := range except { + skip[s] = true + } + for _, artefact := range []string{"arch.md", "test-stubs.md", "breakdown.md", "code-plan.md"} { + if skip[artefact] { + continue + } + 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++ { + 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("turn %d: expected a pause, got %v\n%s", i, err, out.String()) + } + + bn, _ := agentbridge.Open(root, agentbridge.DefaultSession) + pending, ok := bn.Pending() + if !ok { + t.Fatalf("turn %d: expected a pending turn", i) + } + if pending.Operation == "ship:arch:generate" { + assertNoStubsYet() + reachedArch = true + break + } + assertNoStubsYet() + 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) + } +} + +// ── Regression: reusing a session across features must not cross-contaminate ── +// +// Root cause fixed here: Bridge.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 TestAgentMode_SwitchingFeatureResetsStaleSession(t *testing.T) { + t.Parallel() + root := t.TempDir() + + b, err := agentbridge.Open(root, agentbridge.DefaultSession) + if err != nil { + t.Fatalf("open bridge: %v", err) + } + 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, agentbridge.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, err := agentbridge.Open(root, agentbridge.DefaultSession) + if err != nil { + t.Fatalf("reopen bridge: %v", err) + } + 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, agentbridge.ErrTurnRequired) { + t.Fatalf("expected a fresh pause for the new feature, got content=%q err=%v", content, lookupErr) + } +} + // ── False-positive guard: no bridge, no behaviour change ────────────────────── func TestNoBridge_PipelineUnaffected(t *testing.T) { diff --git a/internal/cli/cmdship/arch.go b/internal/cli/cmdship/arch.go index a8d4b2c..6b38c56 100644 --- a/internal/cli/cmdship/arch.go +++ b/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) diff --git a/internal/cli/cmdship/ship.go b/internal/cli/cmdship/ship.go index d224323..9e3f95d 100644 --- a/internal/cli/cmdship/ship.go +++ b/internal/cli/cmdship/ship.go @@ -115,6 +115,35 @@ var ( "forge agent prompt # read the turn, then: forge agent submit --file ") ) +// 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 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. From 0f4788aef30f035656cae27f12a4f9aafd92e12a Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Thu, 20 Aug 2026 23:27:13 +0700 Subject: [PATCH 2/3] chore(build): bump Go toolchain to 1.26.6 for stdlib CVE fixes go1.26.5 carries 4 known stdlib vulnerabilities (GO-2026-6218, GO-2026-6090, GO-2026-5972, GO-2026-5026) reachable from code this binary actually calls (net/url, crypto/tls, encoding/asn1, net/http), all fixed in go1.26.6. Unrelated to the agent-mode fix in the prior commit; bumped here only because the pre-push govulncheck gate correctly blocked on it. Co-Authored-By: Claude Sonnet 5 --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 4c7faae..97a2232 100644 --- a/go.mod +++ b/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 From 4cec2c21ab02326feaa022663e93e0a4893d6831 Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Thu, 20 Aug 2026 23:39:44 +0700 Subject: [PATCH 3/3] test(ship): relocate agent-mode regression tests to their paired prod files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M1-27 (tests-precede-code gate) flags a _test.go file committed after its paired same-named .go file. TestAgentMode_ArchPauseDoesNotStubTheArtefact and TestAgentMode_SwitchingFeatureResetsStaleSession exercise ship.go/arch.go and agentbridge.go (all touched by the prior commit) but were added to agent_mode_test.go, whose pair (agent_mode.go) was untouched — a false positive from the gate's naive file-pairing, not a real ordering violation. Moved to ship_test.go and agentbridge_test.go respectively, which pair with files actually modified in this change. Co-Authored-By: Claude Sonnet 5 --- internal/agentbridge/agentbridge_test.go | 44 +++++++ internal/cli/cmdship/agent_mode_test.go | 140 ----------------------- internal/cli/cmdship/ship_test.go | 82 +++++++++++++ 3 files changed, 126 insertions(+), 140 deletions(-) diff --git a/internal/agentbridge/agentbridge_test.go b/internal/agentbridge/agentbridge_test.go index 38956d6..014cfbf 100644 --- a/internal/agentbridge/agentbridge_test.go +++ b/internal/agentbridge/agentbridge_test.go @@ -441,3 +441,47 @@ func TestFence_SurvivesFencedContentInsideThePrompt(t *testing.T) { 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) + } +} diff --git a/internal/cli/cmdship/agent_mode_test.go b/internal/cli/cmdship/agent_mode_test.go index d016c93..f48cb33 100644 --- a/internal/cli/cmdship/agent_mode_test.go +++ b/internal/cli/cmdship/agent_mode_test.go @@ -31,9 +31,6 @@ package cmdship import ( "bytes" - "errors" - "os" - "path/filepath" "strings" "testing" @@ -197,143 +194,6 @@ func TestAgentMode_IgnoresAnInjectedProvider(t *testing.T) { } } -// ── Regression: a pause must not be treated as an LLM failure ───────────────── -// -// Root cause fixed here: 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: the checkpoint clobbered whatever artefact was on disk with -// a stub template, logged a false failure, and — because the bridge's pause -// latch is set inside Lookup regardless — every checkpoint gated by -// agentPaused() correctly stopped running in *that* invocation, but the -// artefact files were already corrupted by the time it did. On the next -// invocation, the stubbed file made the checkpoint look "already done" and -// the pipeline moved on to the next checkpoint, which repeated the same -// mistake — net effect: a single run silently stamped 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() - - // Turn 1: spec pauses. - if _, err := runShipAgent(t, root); !IsAgentTurn(err) { - t.Fatalf("expected spec to pause, got %v", err) - } - 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(except ...string) { - t.Helper() - skip := map[string]bool{} - for _, s := range except { - skip[s] = true - } - for _, artefact := range []string{"arch.md", "test-stubs.md", "breakdown.md", "code-plan.md"} { - if skip[artefact] { - continue - } - 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++ { - 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("turn %d: expected a pause, got %v\n%s", i, err, out.String()) - } - - bn, _ := agentbridge.Open(root, agentbridge.DefaultSession) - pending, ok := bn.Pending() - if !ok { - t.Fatalf("turn %d: expected a pending turn", i) - } - if pending.Operation == "ship:arch:generate" { - assertNoStubsYet() - reachedArch = true - break - } - assertNoStubsYet() - 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) - } -} - -// ── Regression: reusing a session across features must not cross-contaminate ── -// -// Root cause fixed here: Bridge.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 TestAgentMode_SwitchingFeatureResetsStaleSession(t *testing.T) { - t.Parallel() - root := t.TempDir() - - b, err := agentbridge.Open(root, agentbridge.DefaultSession) - if err != nil { - t.Fatalf("open bridge: %v", err) - } - 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, agentbridge.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, err := agentbridge.Open(root, agentbridge.DefaultSession) - if err != nil { - t.Fatalf("reopen bridge: %v", err) - } - 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, agentbridge.ErrTurnRequired) { - t.Fatalf("expected a fresh pause for the new feature, got content=%q err=%v", content, lookupErr) - } -} - // ── False-positive guard: no bridge, no behaviour change ────────────────────── func TestNoBridge_PipelineUnaffected(t *testing.T) { diff --git a/internal/cli/cmdship/ship_test.go b/internal/cli/cmdship/ship_test.go index de5090b..e8167fa 100644 --- a/internal/cli/cmdship/ship_test.go +++ b/internal/cli/cmdship/ship_test.go @@ -38,6 +38,7 @@ import ( "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 @@ func TestRunQATestSuite_NodeProject_NoTestScript_FallsThrough(t *testing.T) { 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) + } +}