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/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 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/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/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. 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) + } +}