From 9ca75ddb712b212352fb62af69bdf4a160c7a2a5 Mon Sep 17 00:00:00 2001 From: VietKing Date: Wed, 26 Aug 2026 16:54:01 +0700 Subject: [PATCH 1/4] fix(ship): stop agent-mode Test/Code fabrication and undetected stale turns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four confirmed root causes behind the 1.10.2 agent-mode reliability gaps (reproduced live against a real binary in a disposable worktree): 1. writeTestArtifactsWithContext wrote the static RED placeholder stub to tests/*.test.ts whenever an LLM/bridge call returned ANY error, including ErrAgentTurn (a pause, not a failure). Once written, that placeholder permanently satisfied allTestArtifactsExist, so checkTest never called writeTestArtifacts again — a host agent's real, later-submitted answer for the same operation sat unused in the bridge's response store forever, and the checkpoint reported "N test file(s) found; all 4 named artifacts present" against content nobody had reviewed. Fixed by returning the agent-turn error before writing anything for the branch that paused, so the exists-guard stays false and the real answer lands on the next run. 2. agentbridge.Bridge.loadPending restored a still-pending turn from disk without latching `paused`, deferring that to a Lookup call that would only happen if some checkpoint this run coincidentally re-asked the same operation. A checkpoint that never calls Lookup at all (an "artefact already exists" skip, a language branch with no LLM call) left `paused` false for the whole process, letting `forge ship --agent-mode` run straight through later checkpoints — and skip the final "render the pending turn" check, which also gates on Paused() — while an earlier turn (e.g. an arch-parallel-debate role) sat unanswered on disk the whole time. Fixed by latching paused immediately on load; replay is unaffected since Lookup checks byHash/byOrdinal before it ever checks paused. 3. checkSpec treated "no description/--name and 2+ existing spec dirs" as a soft "ok, pass a description" and let the pipeline continue — every later checkpoint then ran against an empty/undefined slug. Now hard-fails and names the candidates, demanding --name. A single unambiguous spec dir is unaffected. 4. The managed .gitignore block (three copies: codemod.canonicalGitignoreBlock, codemod.defaultMarkerBody, cmddoctor.canonicalGiSnippet) never listed .forge/agent/, .forge/.snapshots/, .forge/learned/, .forge/trash/, or .forge/token-ledger.jsonl. Confirmed live: an agent-mode run left `git status --porcelain` reporting the bridge's own bookkeeping as untracked, which checkCode's countChangedFiles then counted as evidence of real code changes ("N modified file(s)" for a run where the only real source file was untouched). Co-Authored-By: Claude Sonnet 5 --- internal/agentbridge/agentbridge.go | 63 ++++++++-------- internal/agentbridge/agentbridge_test.go | 62 ++++++++++++++++ internal/cli/cmddoctor/doctor.go | 5 ++ internal/cli/cmddoctor/doctor_test.go | 22 ++++++ internal/cli/cmdship/agent_mode_test.go | 74 +++++++++++++++++++ internal/cli/cmdship/artefacts_test.go | 5 +- internal/cli/cmdship/ship.go | 30 +++++++- internal/cli/cmdship/ship_test.go | 50 +++++++++++++ internal/cli/cmdship/test_artifacts.go | 55 +++++++++++--- .../test_phase_quality_framework_test.go | 14 ++-- internal/codemod/codemod.go | 5 ++ internal/codemod/codemod_test.go | 21 ++++++ internal/codemod/hygiene.go | 5 ++ internal/codemod/hygiene_test.go | 28 +++++++ 14 files changed, 388 insertions(+), 51 deletions(-) diff --git a/internal/agentbridge/agentbridge.go b/internal/agentbridge/agentbridge.go index d6ff375..088b4cc 100644 --- a/internal/agentbridge/agentbridge.go +++ b/internal/agentbridge/agentbridge.go @@ -163,17 +163,15 @@ type Bridge struct { seen map[string]int pending *Turn - // paused latches once a turn has been requested. Every later Lookup in the - // same process returns ErrTurnRequired without overwriting the pending - // turn, so one `forge ship --agent-mode` invocation always yields exactly - // one question for the host agent to answer. + // paused latches once a turn has been requested — either freshly, on a + // miss inside Lookup, or immediately at Open time when pending.json + // already held an unanswered turn from a previous run (see loadPending). + // Every later Lookup in the same process returns ErrTurnRequired without + // overwriting the pending turn, so one `forge ship --agent-mode` + // invocation always yields exactly one question for the host agent to + // answer — and never silently proceeds past a turn it already owes. paused bool - // restored records that pending was loaded from disk rather than created - // in this process — i.e. a previous run already showed the host agent a - // question it has not answered yet. See Lookup for why that matters. - restored bool - // StrictReplay disables the ordinal fallback: a prompt whose hash is not // recorded is always re-asked, even when an ordinal-keyed answer exists. StrictReplay bool @@ -293,24 +291,21 @@ func (b *Bridge) Lookup(operation, checkpoint, model, system, user string, maxTo return resp.Content, nil } } - if b.paused { - return "", ErrTurnRequired - } // An unanswered turn from a previous run outranks whatever this run would - // have asked. Re-running while paused is normal — a driver that loses its - // place just runs `forge ship --agent-mode` again — and the pipeline can - // legitimately arrive at a *different* prompt the second time, because a - // checkpoint that could not generate its artefact may still have - // scaffolded a stub, moving the next run onto the review path instead of - // the generate path. - // - // Letting that overwrite pending.json would misfile the answer: the host - // agent is holding a prompt it was shown, and `forge agent submit` + // have asked. loadPending latches paused as soon as a pending turn is + // read from disk (before Lookup is ever called), so a restored turn is + // already caught by the b.paused check above — this is why: re-running + // while paused is normal — a driver that loses its place just runs + // `forge ship --agent-mode` again — and the pipeline can legitimately + // arrive at a *different* prompt the second time, because a checkpoint + // that could not generate its artefact may still have scaffolded a stub, + // moving the next run onto the review path instead of the generate path. + // Overwriting pending.json in that case would misfile the answer: the + // host agent is holding a prompt it was shown, and `forge agent submit` // records against whatever is pending at submit time. It would file a // generated spec as if it were a review. So the original question stands // until it is answered or the session is reset. - if b.restored && b.pending != nil { - b.paused = true + if b.paused { return "", ErrTurnRequired } b.pending = &Turn{ @@ -357,7 +352,6 @@ func (b *Bridge) Fulfil(content string) (Turn, error) { b.byOrdinal[resp.Ordinal] = resp b.pending = nil b.paused = false - b.restored = false b.nextSeq = t.Seq + 1 b.session.TurnsFilled++ if err := b.clearPending(); err != nil { @@ -383,7 +377,6 @@ func (b *Bridge) Reset() error { b.seen = map[string]int{} b.pending = nil b.paused = false - b.restored = false b.drifted = 0 b.nextSeq = 0 b.session = Session{Name: b.session.Name, CreatedAt: time.Now().UTC()} @@ -513,12 +506,20 @@ func (b *Bridge) loadPending() error { return fmt.Errorf("parse pending turn: %w", err) } b.pending = &t - // Loading does not latch paused on its own: replay must still work, so - // the run has to be allowed to proceed through every already-answered - // prompt before it reaches the unanswered one. The latch happens in - // Lookup, on the first miss, and preserves this turn rather than - // replacing it. - b.restored = true + // Latching paused here (rather than waiting for Lookup's first miss) does + // not break replay: Lookup checks byHash/byOrdinal before it ever checks + // paused, so every already-answered prompt still replays normally even + // though paused is already true. What this closes is a fabrication path — + // a checkpoint that never calls Lookup at all this run (a stub short- + // circuit, a language branch with no LLM call, an "artefact already + // exists" skip) used to leave paused false for the entire process, so + // forge ship --agent-mode could run straight through Test/Breakdown/Code + // to a real failure or success while a turn from an earlier run — e.g. a + // still-unanswered arch-parallel-debate role — sat unanswered on disk the + // whole time, because both the run loop's per-checkpoint gate and the + // final "render the pending turn" check in the ship command key off this + // same paused flag, not off Pending() alone. + b.paused = true return nil } diff --git a/internal/agentbridge/agentbridge_test.go b/internal/agentbridge/agentbridge_test.go index 014cfbf..adcdc03 100644 --- a/internal/agentbridge/agentbridge_test.go +++ b/internal/agentbridge/agentbridge_test.go @@ -485,3 +485,65 @@ func TestSetFeature_SwitchingFeatureResetsStaleSession(t *testing.T) { t.Fatalf("expected a fresh pause for the new feature, got content=%q err=%v", content, lookupErr) } } + +// ── Regression: a restored pending turn must latch paused immediately ──────── +// +// Root cause this guards: loadPending used to leave b.paused false until some +// later Lookup call happened to re-encounter the exact same hash/ordinal as +// the restored turn. A checkpoint that never calls Lookup at all in a given +// run — a static-stub short-circuit, a language branch with no LLM call, an +// "artefact already exists" skip — left paused false for the whole process +// even though a real, unanswered turn was sitting on disk. That let +// `forge ship --agent-mode` run straight through later checkpoints (and the +// final "render the pending turn" check in the ship command, which also +// gates on Paused()) while a genuinely-owed turn from an earlier run went +// unanswered the entire time. Confirmed live: an arch-parallel-debate turn +// left pending, then Test/Breakdown/Code fabricated results in the same run. +func TestOpen_RestoredPendingTurnLatchesPausedImmediately(t *testing.T) { + t.Parallel() + root := t.TempDir() + + b := mustOpen(t, root, DefaultSession) + if b.Paused() { + t.Fatal("a fresh bridge with nothing pending must not start paused") + } + if _, err := b.Lookup("ship:arch:generate", "arch", "", "sys", "usr", 100); !errors.Is(err, ErrTurnRequired) { + t.Fatalf("expected a pause, got %v", err) + } + if !b.Paused() { + t.Fatal("a fresh miss must latch paused in the same process") + } + + // Simulate a brand-new process (e.g. a second `forge ship --agent-mode` + // invocation) that has not yet called Lookup at all this run. + b2 := mustOpen(t, root, DefaultSession) + if !b2.Paused() { + t.Fatal("Open must latch paused immediately when pending.json already holds an unanswered turn — " + + "otherwise a checkpoint that never calls Lookup this run can sail past the still-owed turn") + } + turn, ok := b2.Pending() + if !ok { + t.Fatal("the restored turn must still be reported by Pending()") + } + if turn.Operation != "ship:arch:generate" { + t.Fatalf("restored the wrong turn: %q", turn.Operation) + } + + // Replay through an already-answered prompt must still work even though + // paused is true from the moment of Open — hash/ordinal hits are checked + // before the paused branch in Lookup. + if _, err := b2.Fulfil("the answer"); err != nil { + t.Fatalf("Fulfil: %v", err) + } + b3 := mustOpen(t, root, DefaultSession) + if b3.Paused() { + t.Fatal("a bridge with no pending turn on disk must not start paused") + } + content, err := b3.Lookup("ship:arch:generate", "arch", "", "sys", "usr", 100) + if err != nil { + t.Fatalf("replay of the answered turn must succeed, got err=%v", err) + } + if content != "the answer" { + t.Fatalf("replay returned %q, want the recorded answer", content) + } +} diff --git a/internal/cli/cmddoctor/doctor.go b/internal/cli/cmddoctor/doctor.go index 8990a01..a27e6c0 100644 --- a/internal/cli/cmddoctor/doctor.go +++ b/internal/cli/cmddoctor/doctor.go @@ -199,6 +199,11 @@ const canonicalGiSnippet = `# forge:gitignore:start # forge-version: managed .forge/scratch/ .forge/cache/ +.forge/.snapshots/ +.forge/agent/ +.forge/learned/ +.forge/trash/ +.forge/token-ledger.jsonl *.tmp *.bak __pycache__/ diff --git a/internal/cli/cmddoctor/doctor_test.go b/internal/cli/cmddoctor/doctor_test.go index 899df0f..3bbd76d 100644 --- a/internal/cli/cmddoctor/doctor_test.go +++ b/internal/cli/cmddoctor/doctor_test.go @@ -227,3 +227,25 @@ func TestCheckLLMProviderLive_NoProviderDetected(t *testing.T) { t.Error("Hint must not be empty when no provider is detected") } } + +// TestCanonicalGiSnippet_CoversForgeScratchState guards the same content this +// duplicate must stay in sync with (internal/codemod.canonicalGitignoreBlock) +// — see that package's TestCanonicalGitignoreBlock_CoversForgeScratchState +// for the root cause (untracked .forge/agent, .forge/.snapshots, etc. being +// miscounted by ship's countChangedFiles as real code changes). +func TestCanonicalGiSnippet_CoversForgeScratchState(t *testing.T) { + t.Parallel() + for _, pattern := range []string{ + ".forge/scratch/", + ".forge/cache/", + ".forge/.snapshots/", + ".forge/agent/", + ".forge/learned/", + ".forge/trash/", + ".forge/token-ledger.jsonl", + } { + if !strings.Contains(canonicalGiSnippet, pattern) { + t.Errorf("canonicalGiSnippet missing %q", pattern) + } + } +} diff --git a/internal/cli/cmdship/agent_mode_test.go b/internal/cli/cmdship/agent_mode_test.go index f48cb33..fd423d7 100644 --- a/internal/cli/cmdship/agent_mode_test.go +++ b/internal/cli/cmdship/agent_mode_test.go @@ -31,6 +31,8 @@ package cmdship import ( "bytes" + "os" + "path/filepath" "strings" "testing" @@ -207,3 +209,75 @@ func TestNoBridge_PipelineUnaffected(t *testing.T) { t.Fatal("a run without agent mode must behave exactly as before") } } + +// ── Regression: a paused Test checkpoint must not fabricate placeholder +// artefacts that later shadow the real host-agent answer ───────────────────── +// +// Root cause this guards: writeTestArtifactsWithContext used to write the +// static RED placeholder stub to tests/*.test.ts unconditionally whenever the +// LLM call returned any error — including ErrAgentTurn, a pause rather than a +// failure. Once written, that placeholder satisfied allTestArtifactsExist +// forever, so checkTest never called writeTestArtifacts again on a later run +// — the host agent's real, later-submitted answer for the same operation sat +// unused in the bridge's response store, and the checkpoint reported +// "N test file(s) found; all 4 named artifacts present" against content +// nobody had actually reviewed. Confirmed live against a real binary: a +// submitted `expect(ping()).toBe('pong')` answer for ship:test:unit never +// reached tests/*.test.ts because the placeholder from the paused attempt +// had already tripped the exists-guard. +func TestWriteTestArtifacts_PausedTurnDoesNotFabricatePlaceholder(t *testing.T) { + t.Parallel() + root := t.TempDir() + bridge, err := agentbridge.Open(root, agentbridge.DefaultSession) + if err != nil { + t.Fatalf("open bridge: %v", err) + } + pipe := newLLMPipeAgent(root, bridge) + slug := "add-ping" + unitPath := filepath.Join(root, "tests", slug+".test.ts") + + // First attempt: ship:test:unit is unanswered, so this must pause — + // and, critically, must not write ANY of the 4 named artifacts. + paths, err := writeTestArtifactsWithContext(root, slug, "add ping function returning pong", "", TestFrameworkContext{}, pipe) + if !IsAgentTurn(err) { + t.Fatalf("expected an agent-turn pause on the first attempt, got err=%v paths=%+v", err, paths) + } + if _, statErr := os.Stat(unitPath); statErr == nil { + t.Fatalf("paused generation must not write a placeholder to %s — doing so permanently shadows the real answer", unitPath) + } + + // Answer ship:test:unit, ship:test:integration, ship:test:rls in turn — + // each is a distinct operation, so each pauses once before the next + // becomes reachable. + realUnit := "```typescript\nexpect(ping()).toBe('pong');\n```" + for _, answer := range []string{realUnit, "integration answer", "rls answer"} { + turn, ok := bridge.Pending() + if !ok { + t.Fatalf("expected a pending turn before submitting %q", answer) + } + if _, ferr := bridge.Fulfil(answer); ferr != nil { + t.Fatalf("Fulfil(%q): %v", answer, ferr) + } + paths, err = writeTestArtifactsWithContext(root, slug, "add ping function returning pong", "", TestFrameworkContext{}, pipe) + if turn.Operation != "ship:test:rls" && !IsAgentTurn(err) { + t.Fatalf("expected the next operation to pause after answering %q, got err=%v", answer, err) + } + } + + if err != nil { + t.Fatalf("all three sub-generations were answered; expected no error, got %v", err) + } + if paths.UnitTest == "" { + t.Fatal("UnitTest path must be set once generation completes") + } + content, readErr := os.ReadFile(paths.UnitTest) + if readErr != nil { + t.Fatalf("read %s: %v", paths.UnitTest, readErr) + } + if !strings.Contains(string(content), "ping()") { + t.Fatalf("tests/%s.test.ts must contain the host agent's real answer, got:\n%s", slug, content) + } + if strings.Contains(string(content), "intentionally failing — complete implementation") { + t.Fatalf("tests/%s.test.ts still holds the static placeholder instead of the real answer:\n%s", slug, content) + } +} diff --git a/internal/cli/cmdship/artefacts_test.go b/internal/cli/cmdship/artefacts_test.go index d8ab1b7..c4907b9 100644 --- a/internal/cli/cmdship/artefacts_test.go +++ b/internal/cli/cmdship/artefacts_test.go @@ -50,7 +50,10 @@ func TestCheckTest_FourArtefacts(t *testing.T) { root := t.TempDir() slug := slugify("add login") - paths := writeTestArtifacts(root, slug, "add login", "", nil) + paths, err := writeTestArtifacts(root, slug, "add login", "", nil) + if err != nil { + t.Fatalf("writeTestArtifacts: %v", err) + } if _, err := os.Stat(paths.UnitTest); err != nil { t.Errorf("unit test artifact missing (%s): %v", paths.UnitTest, err) diff --git a/internal/cli/cmdship/ship.go b/internal/cli/cmdship/ship.go index 9e3f95d..1f72fdf 100644 --- a/internal/cli/cmdship/ship.go +++ b/internal/cli/cmdship/ship.go @@ -1069,6 +1069,27 @@ func checkSpec(root, description, specName string, pipe *LLMPipe) Checkpoint { // No description — look for any existing spec. entries, err := os.ReadDir(specsDir) if err == nil && len(entries) > 0 { + var names []string + for _, e := range entries { + if e.IsDir() { + names = append(names, e.Name()) + } + } + if len(names) > 1 { + // Ambiguous: more than one spec directory and nothing (--name or a + // description) to pick one. Continuing anyway used to run every + // later checkpoint against an empty/undefined slug — each one + // separately falling back to its own "no description" branch — so + // a multi-spec project got a full pipeline's worth of fabricated, + // spec-less output instead of a single clear error demanding + // `--name`. Hard-fail here instead of warning-and-continuing. + cp.Status = "fail" + cp.Detail = fmt.Sprintf( + "%d spec(s) in .forge/specs/ (%s) and no feature description or --name given — ambiguous target; "+ + "pass forge ship --name to pick one, or a feature description to start a new spec", + len(names), strings.Join(names, ", ")) + return cp + } cp.Status = "ok" cp.Detail = fmt.Sprintf("%d spec(s) in .forge/specs/; pass a feature description to target one", len(entries)) return cp @@ -1143,7 +1164,14 @@ func checkTest(root, description, specName string, pipe *LLMPipe, dryRun bool) C if data, err := os.ReadFile(specPath); err == nil { specMD = string(data) } - writeTestArtifacts(root, slug, description, specMD, pipe) + // A paused host-agent turn here must not fall through to the + // "artifacts found" branch below: writeTestArtifacts wrote nothing in + // that case specifically so allTestArtifactsExist stays false and the + // real answer (once submitted) lands in tests/* on the next run + // instead of being permanently shadowed by a placeholder. + if _, waErr := writeTestArtifacts(root, slug, description, specMD, pipe); agentPauseCheckpoint(&cp, "ship:test:write-artifacts", waErr) { + return cp + } } if len(testFiles) > 0 { diff --git a/internal/cli/cmdship/ship_test.go b/internal/cli/cmdship/ship_test.go index e8167fa..d350e79 100644 --- a/internal/cli/cmdship/ship_test.go +++ b/internal/cli/cmdship/ship_test.go @@ -875,6 +875,56 @@ func TestCheckSpec_LLM_NoDescription_Warning(t *testing.T) { } } +// TestCheckSpec_NoDescription_MultipleSpecs_HardFails guards against a real +// gap: with no description/--name and more than one spec directory already +// present, checkSpec used to report "ok" ("N spec(s) in .forge/specs/; pass +// a feature description to target one") and let the pipeline continue — every +// later checkpoint then ran against an empty/undefined slug, each separately +// falling back to its own "no description" branch, so an ambiguous target +// produced a full pipeline's worth of fabricated, spec-less output instead of +// a single clear error. Ambiguity (more than one candidate) must hard-fail +// and demand --name; it must not be treated the same as the unambiguous +// single-spec case, which is still fine to report as an informational "ok". +func TestCheckSpec_NoDescription_MultipleSpecs_HardFails(t *testing.T) { + t.Parallel() + root := t.TempDir() + for _, slug := range []string{"add-login", "add-billing"} { + if err := os.MkdirAll(filepath.Join(root, ".forge", "specs", slug), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", slug, err) + } + } + cp := checkSpec(root, "", "", nil) + + if cp.Status != "fail" { + t.Fatalf("ambiguous multi-spec target must hard-fail, got %q: %s", cp.Status, cp.Detail) + } + if !strings.Contains(cp.Detail, "--name") { + t.Errorf("detail must instruct the user to disambiguate with --name: %s", cp.Detail) + } + for _, slug := range []string{"add-login", "add-billing"} { + if !strings.Contains(cp.Detail, slug) { + t.Errorf("detail must name the ambiguous candidate %q: %s", slug, cp.Detail) + } + } +} + +// TestCheckSpec_NoDescription_SingleSpec_StillOK guards the non-ambiguous +// case: exactly one existing spec directory is not an error, and must keep +// reporting the pre-existing informational "ok" rather than being swept into +// the new hard-fail path. +func TestCheckSpec_NoDescription_SingleSpec_StillOK(t *testing.T) { + t.Parallel() + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, ".forge", "specs", "add-login"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + cp := checkSpec(root, "", "", nil) + + if cp.Status != "ok" { + t.Fatalf("a single unambiguous spec dir must not hard-fail, got %q: %s", cp.Status, cp.Detail) + } +} + // ── YAML spec (spec.yml) integration tests ──────────────────────────────────── // // Test-design checklist (always-write-tests.md 9-point): diff --git a/internal/cli/cmdship/test_artifacts.go b/internal/cli/cmdship/test_artifacts.go index 1e13de8..1617c0b 100644 --- a/internal/cli/cmdship/test_artifacts.go +++ b/internal/cli/cmdship/test_artifacts.go @@ -79,7 +79,7 @@ type TestArtifactPaths struct { // writeTestArtifacts is the original G-006 entry point, now a shim that calls // writeTestArtifactsWithContext using the auto-detected test framework. This // preserves backward-compatibility while fixing G10 (hardcoded TypeScript). -func writeTestArtifacts(root, slug, feature, specMarkdown string, pipe *LLMPipe) TestArtifactPaths { +func writeTestArtifacts(root, slug, feature, specMarkdown string, pipe *LLMPipe) (TestArtifactPaths, error) { fw := detectTestFramework(root) return writeTestArtifactsWithContext(root, slug, feature, specMarkdown, fw, pipe) } @@ -318,10 +318,24 @@ func CheckTestFilesExist(files []string) CheckTestFilesResult { // only the correct stub type for the detected stack (G10 fix: no cross-language // injection). An LLMPipe may be nil — static stubs are always written as a // fallback. RFC-005 §6.3. -func writeTestArtifactsWithContext(root, slug, feature, specMD string, fw TestFrameworkContext, pipe *LLMPipe) TestArtifactPaths { +// +// Returns a non-nil error (satisfying IsAgentTurn) when a bridge-backed pipe +// paused on a host-agent turn mid-generation. In that case NO files are +// written for the branch that paused: the pre-fix behaviour wrote the static +// RED placeholder to disk regardless of *why* generation didn't produce +// content, which let a pending turn look identical to "there was nothing to +// enrich". That placeholder then satisfied allTestArtifactsExist forever, +// silently discarding the host agent's real answer once it was later +// submitted — confirmed live: submitting a real `ping()` assertion for +// ship:test:unit never reached tests/*.test.ts, because the artifact-exists +// guard in checkTest had already been tripped by the placeholder from the +// paused attempt. Returning the error instead lets the caller keep the +// "not yet written" state so the next run's Lookup call replays the real +// answer into the actual file. +func writeTestArtifactsWithContext(root, slug, feature, specMD string, fw TestFrameworkContext, pipe *LLMPipe) (TestArtifactPaths, error) { testsDir := filepath.Join(root, "tests") if err := os.MkdirAll(testsDir, 0o755); err != nil { - return TestArtifactPaths{} + return TestArtifactPaths{}, nil } isFix := IsBugFix(feature) @@ -332,7 +346,11 @@ func writeTestArtifactsWithContext(root, slug, feature, specMD string, fw TestFr paths.GoTest = filepath.Join(testsDir, slug+"_test.go") content := goTestStub(slug, feature, isFix) if pipe != nil { - if gen := llmGoStub(pipe, slug, feature, specMD, isFix); gen != "" { + gen, err := llmGoStub(pipe, slug, feature, specMD, isFix) + if IsAgentTurn(err) { + return TestArtifactPaths{}, err + } + if gen != "" { content = gen } } @@ -395,7 +413,11 @@ func writeTestArtifactsWithContext(root, slug, feature, specMD string, fw TestFr // truncated response is caught rather than written as-is. 6000) } - if gen, complete, err := generateWithValidation(unitFn); err == nil && complete && gen != "" { + gen, complete, err := generateWithValidation(unitFn) + if IsAgentTurn(err) { + return TestArtifactPaths{}, err + } + if err == nil && complete && gen != "" { unitContent = gen } @@ -405,7 +427,11 @@ func writeTestArtifactsWithContext(root, slug, feature, specMD string, fw TestFr "Tests MUST fail. Import test functions from \""+runner+"\", never a different test framework.", ctx+"Generate failing integration test stubs for feature: "+feature, 6000) } - if gen, complete, err := generateWithValidation(integFn); err == nil && complete && gen != "" { + gen, complete, err = generateWithValidation(integFn) + if IsAgentTurn(err) { + return TestArtifactPaths{}, err + } + if err == nil && complete && gen != "" { integContent = gen } @@ -416,7 +442,11 @@ func writeTestArtifactsWithContext(root, slug, feature, specMD string, fw TestFr "\", never a different test framework. Tests MUST fail.", ctx+"Generate failing RLS test stubs for feature: "+feature, 4000) } - if gen, complete, err := generateWithValidation(rlsFn); err == nil && complete && gen != "" { + gen, complete, err = generateWithValidation(rlsFn) + if IsAgentTurn(err) { + return TestArtifactPaths{}, err + } + if err == nil && complete && gen != "" { rlsContent = gen } } @@ -433,7 +463,7 @@ func writeTestArtifactsWithContext(root, slug, feature, specMD string, fw TestFr _ = os.WriteFile(paths.ScanBaseline, data, 0o600) } - return paths + return paths, nil } // ── Go stub generators ───────────────────────────────────────────────────────── @@ -563,7 +593,7 @@ class %sTest { // ── LLM generator for Go (optional enrichment) ───────────────────────────── -func llmGoStub(pipe *LLMPipe, _ string, feature, specMD string, isBugFix bool) string { +func llmGoStub(pipe *LLMPipe, _ string, feature, specMD string, isBugFix bool) (string, error) { ctx := "" if specMD != "" { ctx = "Feature spec:\n" + specMD + "\n\n" @@ -582,10 +612,13 @@ func llmGoStub(pipe *LLMPipe, _ string, feature, specMD string, isBugFix bool) s 6000) } gen, complete, err := generateWithValidation(genFn) + if IsAgentTurn(err) { + return "", err + } if err != nil || !complete || gen == "" { - return "" + return "", nil } - return gen + return gen, nil } // featureTitle converts a feature string to a CamelCase identifier suitable diff --git a/internal/cli/cmdship/test_phase_quality_framework_test.go b/internal/cli/cmdship/test_phase_quality_framework_test.go index 1b1dbe4..0fa76a1 100644 --- a/internal/cli/cmdship/test_phase_quality_framework_test.go +++ b/internal/cli/cmdship/test_phase_quality_framework_test.go @@ -199,7 +199,7 @@ func TestWriteTestArtifactsWithContext_Go_GeneratesGoStubs(t *testing.T) { root, slug := testRoot(t, "go") fw := detectTestFramework(root) - paths := writeTestArtifactsWithContext(root, slug, "my feature", "", fw, nil) + paths, _ := writeTestArtifactsWithContext(root, slug, "my feature", "", fw, nil) if paths.GoTest == "" { t.Fatal("GoTest path must be set for Go project") @@ -218,7 +218,7 @@ func TestWriteTestArtifactsWithContext_Go_NoTypeScriptStubs(t *testing.T) { root, slug := testRoot(t, "go") fw := detectTestFramework(root) - paths := writeTestArtifactsWithContext(root, slug, "my feature", "", fw, nil) + paths, _ := writeTestArtifactsWithContext(root, slug, "my feature", "", fw, nil) if paths.UnitTest != "" { if _, err := os.Stat(paths.UnitTest); err == nil { @@ -237,7 +237,7 @@ func TestWriteTestArtifactsWithContext_TypeScript_NoGoStubs(t *testing.T) { root, slug := testRoot(t, "ts") fw := detectTestFramework(root) - paths := writeTestArtifactsWithContext(root, slug, "my feature", "", fw, nil) + paths, _ := writeTestArtifactsWithContext(root, slug, "my feature", "", fw, nil) if paths.UnitTest == "" { t.Fatal("UnitTest path must be set for TypeScript project") @@ -284,7 +284,7 @@ func TestWriteTestArtifactsWithContext_RLSPromptNamesDetectedFramework(t *testin }, } - writeTestArtifactsWithContext(root, slug, "my feature", "", fw, mockPipe(root, mock)) + _, _ = writeTestArtifactsWithContext(root, slug, "my feature", "", fw, mockPipe(root, mock)) for name, prompt := range map[string]string{ "RLS": rlsSystemPrompt, "unit": unitSystemPrompt, "integration": integSystemPrompt, @@ -300,8 +300,8 @@ func TestWriteTestArtifactsWithContext_Idempotent(t *testing.T) { root, slug := testRoot(t, "go") fw := detectTestFramework(root) - p1 := writeTestArtifactsWithContext(root, slug, "feature", "", fw, nil) - p2 := writeTestArtifactsWithContext(root, slug, "feature", "", fw, nil) + p1, _ := writeTestArtifactsWithContext(root, slug, "feature", "", fw, nil) + p2, _ := writeTestArtifactsWithContext(root, slug, "feature", "", fw, nil) if p1.GoTest != p2.GoTest { t.Errorf("idempotency: GoTest paths differ: %q vs %q", p1.GoTest, p2.GoTest) @@ -344,7 +344,7 @@ func TestWriteTestArtifactsWithContext_BugFix_HasRegressionStubs(t *testing.T) { root, slug := testRoot(t, "go") fw := detectTestFramework(root) - paths := writeTestArtifactsWithContext(root, slug, "fix nil pointer in parser", "", fw, nil) + paths, _ := writeTestArtifactsWithContext(root, slug, "fix nil pointer in parser", "", fw, nil) content, err := os.ReadFile(paths.GoTest) if err != nil { diff --git a/internal/codemod/codemod.go b/internal/codemod/codemod.go index 05e75b9..d76b423 100644 --- a/internal/codemod/codemod.go +++ b/internal/codemod/codemod.go @@ -113,6 +113,11 @@ const defaultMarkerBody = `# forge:gitignore:start # Managed by forge — do not edit manually. Run "forge upgrade gitignore-marker" to refresh. .forge/scratch/ .forge/cache/ +.forge/.snapshots/ +.forge/agent/ +.forge/learned/ +.forge/trash/ +.forge/token-ledger.jsonl *.tmp *.bak __pycache__/ diff --git a/internal/codemod/codemod_test.go b/internal/codemod/codemod_test.go index ff7388c..749897e 100644 --- a/internal/codemod/codemod_test.go +++ b/internal/codemod/codemod_test.go @@ -165,3 +165,24 @@ func TestDefault_HasBuiltins(t *testing.T) { } } } + +// TestDefaultMarkerBody_CoversForgeScratchState guards the third copy of the +// same managed .gitignore content (alongside canonicalGitignoreBlock in +// hygiene.go and canonicalGiSnippet in cmddoctor/doctor.go) — see +// TestCanonicalGitignoreBlock_CoversForgeScratchState for the root cause. +func TestDefaultMarkerBody_CoversForgeScratchState(t *testing.T) { + t.Parallel() + for _, pattern := range []string{ + ".forge/scratch/", + ".forge/cache/", + ".forge/.snapshots/", + ".forge/agent/", + ".forge/learned/", + ".forge/trash/", + ".forge/token-ledger.jsonl", + } { + if !strings.Contains(defaultMarkerBody, pattern) { + t.Errorf("defaultMarkerBody missing %q", pattern) + } + } +} diff --git a/internal/codemod/hygiene.go b/internal/codemod/hygiene.go index ec495d6..10d63d2 100644 --- a/internal/codemod/hygiene.go +++ b/internal/codemod/hygiene.go @@ -67,6 +67,11 @@ const canonicalGitignoreBlock = `# forge:gitignore:start # forge-version: managed .forge/scratch/ .forge/cache/ +.forge/.snapshots/ +.forge/agent/ +.forge/learned/ +.forge/trash/ +.forge/token-ledger.jsonl *.tmp *.bak __pycache__/ diff --git a/internal/codemod/hygiene_test.go b/internal/codemod/hygiene_test.go index 59b6e55..c8dda20 100644 --- a/internal/codemod/hygiene_test.go +++ b/internal/codemod/hygiene_test.go @@ -281,3 +281,31 @@ func TestGitleaksCM_ForcePreservesUserRules(t *testing.T) { t.Error("canonical block not restored after ApplyForce") } } + +// TestCanonicalGitignoreBlock_CoversForgeScratchState guards against a real +// gap: the managed .gitignore block only listed .forge/scratch/ and +// .forge/cache/, so .forge/.snapshots/, .forge/agent/ (the agent-mode bridge +// state — pending.json/responses.jsonl/session.json), .forge/learned/, +// .forge/trash/, and .forge/token-ledger.jsonl all showed up as untracked in +// `git status`. Confirmed live: a fresh agent-mode run left `git status +// --porcelain` reporting `?? .forge/`, which countChangedFiles (ship.go) then +// counted as evidence of real code changes — the Code checkpoint reported +// "N modified file(s)" for a run where the only real source file was +// untouched. +func TestCanonicalGitignoreBlock_CoversForgeScratchState(t *testing.T) { + t.Parallel() + for _, pattern := range []string{ + ".forge/scratch/", + ".forge/cache/", + ".forge/.snapshots/", + ".forge/agent/", + ".forge/learned/", + ".forge/trash/", + ".forge/token-ledger.jsonl", + } { + if !strings.Contains(canonicalGitignoreBlock, pattern) { + t.Errorf("canonicalGitignoreBlock missing %q — this forge-owned path will show up as untracked "+ + "in git status and can be miscounted as real code changes", pattern) + } + } +} From f496495d3a2b826fae64f7f9dfd3191d303f5d49 Mon Sep 17 00:00:00 2001 From: VietKing Date: Wed, 26 Aug 2026 17:21:06 +0700 Subject: [PATCH 2/4] fix(ci): scope tests-precede-code gate to this PR's own commit range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M1-27 compared each file's all-time-latest-touch commit timestamp across the entire repo history, not just this PR's commits. That means ANY test-only PR touching a mature file — no production change at all — was flagged as a TDD violation purely because the test file's last historical edit (this PR's own commit) postdated the paired production file's last historical edit (from whenever it was last touched, possibly months ago). That's true for nearly every established file, and hit this branch's own PR (agent_mode_test.go touched, agent_mode.go untouched). Scope both `git log` calls to "origin/$BASE..HEAD" and take the oldest commit within that range (`tail -1`) instead of the newest across all history (`head -1`). An untouched production file now correctly produces no commits in range (empty PROD_DATE) and is skipped, while a genuine same-PR "production code first, test added later" violation is still caught — verified both cases locally against this fix before pushing. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci-gates.yml | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci-gates.yml b/.github/workflows/ci-gates.yml index 0b36193..73db6ad 100644 --- a/.github/workflows/ci-gates.yml +++ b/.github/workflows/ci-gates.yml @@ -73,13 +73,25 @@ jobs: CHANGED=$(git diff --name-only "origin/$BASE"...HEAD 2>/dev/null || git diff --name-only HEAD~1...HEAD) # For each changed _test.go, find the corresponding non-test file # and check that the test commit is not strictly *after* the production commit. + # + # Both `git log` calls are scoped to this PR's own commit range + # ("origin/$BASE..HEAD") and take the *first* commit in that range + # (oldest, via `tail -1` — `git log` lists newest-first) rather than + # the most recent commit in the file's entire history. A prior + # version compared each file's all-time-latest-touch timestamp, + # which meant ANY test-only PR against a mature file (no production + # change at all) was flagged as a TDD violation purely because the + # test's last historical edit postdated the production file's last + # historical edit — true for almost every established file. Scoping + # to this PR's range means an untouched production file naturally + # produces no commits (PROD_DATE empty) and is correctly skipped. VIOLATIONS=0 while IFS= read -r f; do if [[ "$f" != *_test.go ]]; then continue; fi PROD="${f%_test.go}.go" if [[ ! -f "$PROD" ]]; then continue; fi # No paired prod file — skip - TEST_DATE=$(git log --follow --format='%ct' -- "$f" | head -1) - PROD_DATE=$(git log --follow --format='%ct' -- "$PROD" | head -1) + TEST_DATE=$(git log --follow --format='%ct' "origin/$BASE..HEAD" -- "$f" | tail -1) + PROD_DATE=$(git log --follow --format='%ct' "origin/$BASE..HEAD" -- "$PROD" | tail -1) if [[ -n "$TEST_DATE" && -n "$PROD_DATE" && "$TEST_DATE" -gt "$PROD_DATE" ]]; then echo "::warning file=$f::Test file $f was committed AFTER production file $PROD (TDD violation)" VIOLATIONS=$((VIOLATIONS + 1)) From dbcb1a7f9a4856f776e03ff9d8b3ea18f69d1e41 Mon Sep 17 00:00:00 2001 From: VietKing Date: Wed, 26 Aug 2026 17:30:34 +0700 Subject: [PATCH 3/4] fix(ci): bump ci-gates.yml to Go 1.26, matching ci.yml/nightly.yml/go.mod MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ci-gates.yml was still pinned to GO_VERSION '1.25' while ci.yml and nightly.yml were already on '1.26', and go.mod's toolchain directive has required go1.26.x since 2026-07-10 (bumped for stdlib CVE fixes, most recently go1.26.6 on 2026-08-20). The drift meant ci-gates' M2-17 perf benchmark gate installed benchstat against Go 1.25 and failed outright once golang.org/x/perf's @latest started requiring go >= 1.26.0 — a pre-existing, repo-wide break confirmed on an unrelated dependabot PR from 4 days before this fix, not something introduced by this branch. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci-gates.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci-gates.yml b/.github/workflows/ci-gates.yml index 73db6ad..5c371c8 100644 --- a/.github/workflows/ci-gates.yml +++ b/.github/workflows/ci-gates.yml @@ -19,7 +19,7 @@ permissions: contents: read env: - GO_VERSION: '1.25' + GO_VERSION: '1.26' CGO_ENABLED: '0' jobs: From 01cd3e50a8b16eb7765ceddc8ca5097cd325db4b Mon Sep 17 00:00:00 2001 From: VietKing Date: Wed, 26 Aug 2026 17:31:46 +0700 Subject: [PATCH 4/4] docs(changelog): add 1.10.3 entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Version Scope Decision: - Chosen bump: PATCH - Why not smaller: agent-mode fabricating checkpoint success while discarding real host-agent answers, and a stale-turn blind spot that let it happen even when a prior turn was still unanswered, are bug fixes to behaviour that was already wrong — both need to reach users. - Breaking impact: none. No CLI flag, output key, or exported symbol changed. writeTestArtifacts(WithContext)/llmGoStub gained a return value but are unexported (package cmdship internals); agentbridge.Bridge's paused-latch timing changed but Paused()'s signature and contract ("a turn has been requested and not yet answered") are unchanged. No BREAKING.md entry: nothing in this release breaks a contract. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4d25ce..9112c94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ All notable changes to forge will be documented in this file. Format follows [Ke ## [Unreleased] +## [1.10.3] — 2026-08-26 — Agent mode stopped fabricating Test/Code success behind a real pause + +### Fixed + +- **`forge ship --agent-mode` could fabricate a passing Test checkpoint while silently discarding the host agent's real answer.** `writeTestArtifactsWithContext` wrote the static RED placeholder stub to `tests/*.test.ts` whenever an LLM/bridge call returned *any* error — including `ErrAgentTurn`, a pause rather than a failure. Once written, that placeholder permanently satisfied `allTestArtifactsExist`, so `checkTest` never called the generator again on a later run: a host agent's real, later-submitted answer for the same operation sat unused in the bridge's response store forever, while the checkpoint reported "N test file(s) found; all 4 named artifacts present" against content nobody had reviewed. Confirmed live against a real binary: submitting `expect(ping()).toBe('pong')` for `ship:test:unit` never reached `tests/*.test.ts` because the placeholder from the paused attempt had already tripped the exists-guard. `writeTestArtifactsWithContext` (and its `writeTestArtifacts`/`llmGoStub` callers) now return the agent-turn error before writing anything for the branch that paused, so the exists-guard stays false and the real answer lands on the next run. +- **A still-pending host-agent turn from an earlier run could go undetected, letting the pipeline fabricate results past it.** `agentbridge.Bridge.loadPending` restored a pending turn from disk without latching `paused`, deferring that to a `Lookup` call that would only happen if some checkpoint this run coincidentally re-asked the same operation. A checkpoint that never called `Lookup` at all this run (an "artefact already exists" skip, a language branch with no LLM call) left `paused` false for the whole process — which meant both the run loop's per-checkpoint gate and the final "render the pending turn" check in `forge ship` (both keyed on `Paused()`, not `Pending()` alone) silently failed to fire while an earlier turn (e.g. an `arch-parallel-debate` role) sat unanswered on disk the entire time. `loadPending` now latches `paused` immediately; replay is unaffected since `Lookup` checks its hash/ordinal indexes before it ever checks `paused`. +- **No description/`--name` with more than one existing spec directory silently ran the whole pipeline against an undefined slug.** `checkSpec` treated this as a soft "ok, pass a description" and let every later checkpoint fall back to its own "no description" branch independently, producing a full pipeline's worth of fabricated, spec-less output instead of one clear error. Now hard-fails, names every ambiguous candidate, and demands `--name`. A single unambiguous spec directory is unaffected. +- **The managed `.gitignore` block never listed forge's own agent-mode/scratch state**, so `.forge/agent/` (bridge session/pending/response files), `.forge/.snapshots/`, `.forge/learned/`, `.forge/trash/`, and `.forge/token-ledger.jsonl` all showed up as untracked in `git status`. Confirmed live: an agent-mode run left `git status --porcelain` reporting the bridge's own bookkeeping as changes, which the Code checkpoint's `countChangedFiles` then counted as evidence of real code changes — "N modified file(s)" for a run where the only real source file was untouched. Fixed in all three copies of the managed block (`codemod.canonicalGitignoreBlock`, `codemod.defaultMarkerBody`, `cmddoctor.canonicalGiSnippet`). +- **CI: the `M1-27` tests-precede-code gate flagged any test-only PR touching a mature file as a TDD violation.** It compared each file's all-time-latest-touch commit timestamp across the entire repo history rather than this PR's own commits — true for nearly every established file the moment its test gets a maintenance update with no accompanying production change. Both `git log` calls are now scoped to `origin/$BASE..HEAD` and take the oldest commit in that range, so an untouched production file correctly produces no commits (skipped) while a genuine same-PR "production code first, test added later" violation is still caught. +- **CI: the `M2-17` perf benchmark gate's `benchstat` install failed on `ci-gates.yml`'s Go 1.25 pin** once `golang.org/x/perf@latest` started requiring Go ≥ 1.26. `ci.yml` and `nightly.yml` were already on Go 1.26, matching `go.mod`'s `toolchain go1.26.6` (bumped 2026-08-20 for stdlib CVE fixes) — `ci-gates.yml` alone had drifted. Bumped to match. + ## [1.10.2] — 2026-08-21 — Agent mode stopped pausing: a bridge miss was treated as an LLM failure ### Fixed