diff --git a/.github/workflows/ci-gates.yml b/.github/workflows/ci-gates.yml index 0b36193..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: @@ -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)) 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 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) + } + } +}