From 037bef27ec1827b789b68e4c7933b663849a6a22 Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Mon, 7 Sep 2026 20:49:46 +0700 Subject: [PATCH 1/4] fix(ship): agent-mode pipeline blockers found dogfooding on a real repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven issues that stopped `forge ship --agent-mode` from certifying a feature end-to-end on a large Next.js/Supabase repo (2026-09-07): 1. forge clean flagged git-ignored files (~450 .playwright-mcp/*.log, sibling .claude/worktrees) as "unmanaged scratch" and the ship hygiene gate hard-failed on them, telling the user to run `forge clean --apply` which would delete other tools'/sessions' ignored working files. clean now skips git-ignored paths *outside* .forge/ by default (forge still tidies its own ignored scratch); `--include-ignored` restores the old behaviour. 2. self-review-gate / spec-completeness / adr-quality / tdd-gate / security-hygiene resolved the spec dir with slugify(description), ignoring the --name/-n override that agent-mode always uses — so every gate reported "spec.md not found / UNVERIFIED" while the artefacts sat in .forge/specs//. They now use HookContext.SpecName via ctxSpecSlug(). 3. Same gates only looked for the canonical filename; agent-mode writes test.md / code-plan.md / etc. Added readSpecArtefact() aliases (arch.md↔adr.md, test.md↔tests.md, code-plan.md↔impl-notes.md). 4. The Code checkpoint reported "N modified file(s)" + status ok by counting every dirty line in `git status` — unrelated docs, .forge ledgers, another session's screenshots. Added countChangedSourceFiles() (source extensions, excludes .forge/ docs/ growth/); a plan with no source change now reports "warning — implement then rerun", not a green "code written". 5. Test artefacts were always written to /tests, invisible to a runner whose roots are "src"; the reachability check then swept the whole repo and flagged the project's pre-existing e2e/staging suites as "unreachable". pickTestsDir() prefers an existing collected dir (src/test, …); reachability now checks only the artefacts forge wrote for this slug. 6. ship:test:integration hard-coded "Jest + supertest"; on a repo without supertest or an HTTP entrypoint the stub could not compile. Prompt now tells the model to follow the project's own integration-test convention. 7. `forge agent submit --file` failed on a path containing ".." or mixed separators ("cannot find the path specified") even when the file existed; the path is now cleaned and resolved against cwd. Tests: cmdship/cmdclean/cmdagent suites green; two TestCheckCode assertions updated to the corrected (plan != implementation) contract. Co-Authored-By: Claude Sonnet 5 --- internal/cli/cmdagent/agent.go | 17 ++- internal/cli/cmdclean/clean.go | 102 +++++++++++++++++- internal/cli/cmdship/hook.go | 77 +++++++++----- internal/cli/cmdship/prompts_and_learning.go | 3 +- internal/cli/cmdship/ship.go | 106 ++++++++++++++++--- internal/cli/cmdship/ship_test.go | 20 +++- internal/cli/cmdship/test_artifacts.go | 54 ++++++++-- 7 files changed, 323 insertions(+), 56 deletions(-) diff --git a/internal/cli/cmdagent/agent.go b/internal/cli/cmdagent/agent.go index 08166df..a6b9e88 100644 --- a/internal/cli/cmdagent/agent.go +++ b/internal/cli/cmdagent/agent.go @@ -35,6 +35,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" "github.com/spf13/cobra" @@ -330,9 +331,21 @@ func readAnswer(cmd *cobra.Command, args []string, fromFile string) (string, err } return string(data), nil case fromFile != "": - data, err := os.ReadFile(fromFile) + // Normalise the path: collapse "..", unify separators, and resolve a + // relative path against the working directory. Without this, a caller + // passing e.g. "C:\a\b/../c/x.md" (mixed separators + parent refs, as + // shells and scratch-dir helpers routinely produce) hit + // "cannot find the path specified" even though the file exists. + p := filepath.FromSlash(fromFile) + if !filepath.IsAbs(p) { + if wd, werr := os.Getwd(); werr == nil { + p = filepath.Join(wd, p) + } + } + p = filepath.Clean(p) + data, err := os.ReadFile(p) if err != nil { - return "", errcode.New(ErrAgentFailed, "read answer file", err) + return "", errcode.New(ErrAgentFailed, "read answer file "+p, err) } return string(data), nil default: diff --git a/internal/cli/cmdclean/clean.go b/internal/cli/cmdclean/clean.go index 8613e97..e159ac7 100644 --- a/internal/cli/cmdclean/clean.go +++ b/internal/cli/cmdclean/clean.go @@ -77,6 +77,70 @@ func isForgeTrash(rel string) bool { return rel == forgeTrashRel || strings.HasPrefix(rel, forgeTrashRel+"/") } +// IncludeIgnored disables the default behaviour of skipping git-ignored paths +// (outside .forge/) during classification. Set by `forge clean --include-ignored`. +// Left as a package var so Run/RunDryRun/RunWithTrash keep their existing +// signatures (ship.go and the test suite call them directly). +var IncludeIgnored bool + +// gitignoreFilter identifies paths git ignores so `forge clean` leaves other +// tools' ignored working files alone (e.g. .playwright-mcp/ console logs, +// editor caches, sibling .claude/worktrees/). Forge's OWN ignored scratch +// under .forge/ is still classified — tidying that is the point of the command. +type gitignoreFilter struct { + files map[string]bool // exact ignored paths (slash-separated, root-relative) + dirs []string // ignored directory prefixes, each ending in "/" +} + +// newGitignoreFilter builds the ignore set via `git ls-files`. When +// includeIgnored is true, or git is unavailable, it returns a filter that +// matches nothing (pre-1.10.6 behaviour). +func newGitignoreFilter(root string, includeIgnored bool) *gitignoreFilter { + f := &gitignoreFilter{files: map[string]bool{}} + if includeIgnored { + return f + } + // -o -i --exclude-standard --directory: list ignored paths, collapsing a + // fully-ignored directory to a single "dir/" entry instead of every file. + cmd := exec.Command("git", "-C", root, "ls-files", "-z", "-o", "-i", + "--exclude-standard", "--directory") + var out bytes.Buffer + cmd.Stdout = &out + if err := cmd.Run(); err != nil { + return f // git unavailable — degrade to "filter nothing" + } + for _, entry := range strings.Split(out.String(), "\x00") { + if entry == "" { + continue + } + entry = filepath.ToSlash(entry) + if strings.HasSuffix(entry, "/") { + f.dirs = append(f.dirs, entry) + } else { + f.files[entry] = true + } + } + return f +} + +// ignored reports whether rel (slash-separated, root-relative) is git-ignored +// and lives outside forge's own .forge/ tree. +func (f *gitignoreFilter) ignored(rel string) bool { + if rel == ".forge" || strings.HasPrefix(rel, ".forge/") { + return false // forge still cleans its own scratch even when gitignored + } + if f.files[rel] { + return true + } + relDir := rel + "/" + for _, d := range f.dirs { + if strings.HasPrefix(relDir, d) { + return true + } + } + return false +} + // loadMerged loads scratch/managed patterns from both .forge/manifest and // .forge/hygiene.yml (if present), returning the union of both. This ensures // forge clean is consistent with forge hygiene's pattern set (issue #15). @@ -164,17 +228,19 @@ func init() { // New returns the cobra command. func New() *cobra.Command { var ( - root string - check bool - dryRun bool - apply bool - asJSON bool + root string + check bool + dryRun bool + apply bool + asJSON bool + includeIgnored bool ) cmd := &cobra.Command{ Use: "clean", Short: "Find/remove unmanaged scratch files.", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { + IncludeIgnored = includeIgnored modes := 0 if check { modes++ @@ -242,6 +308,8 @@ func New() *cobra.Command { cmd.Flags().BoolVar(&dryRun, "dry-run", false, "show what would be deleted without deleting") cmd.Flags().BoolVar(&apply, "apply", false, "move found candidates to .forge/trash//") cmd.Flags().BoolVar(&asJSON, "json", false, "emit machine-readable JSON") + cmd.Flags().BoolVar(&includeIgnored, "include-ignored", false, + "also classify git-ignored paths outside .forge/ (pre-1.10.6 behaviour)") return cmd } @@ -258,6 +326,7 @@ func Run(root string, apply bool) (*Result, error) { mode = "apply" } res := &Result{Root: root, ManifestPath: mf.Path, Mode: mode} + gi := newGitignoreFilter(root, IncludeIgnored) walkErr := filepath.WalkDir(root, func(p string, d fs.DirEntry, werr error) error { if werr != nil { @@ -281,6 +350,13 @@ func Run(root string, apply bool) (*Result, error) { } return nil } + if gi.ignored(rel) { + // git-ignored working file from another tool — not forge's to remove. + if d.IsDir() { + return filepath.SkipDir + } + return nil + } if mf.IsScratch(rel) { res.Candidates = append(res.Candidates, rel) if d.IsDir() { @@ -322,6 +398,7 @@ func RunDryRun(root string) (*Result, error) { return nil, err } res := &Result{Root: root, ManifestPath: mf.Path, Mode: "dry-run"} + gi := newGitignoreFilter(root, IncludeIgnored) _ = filepath.WalkDir(root, func(p string, d fs.DirEntry, werr error) error { if werr != nil || p == root { return werr @@ -337,6 +414,13 @@ func RunDryRun(root string) (*Result, error) { } return nil } + if gi.ignored(rel) { + // git-ignored working file from another tool — not forge's to remove. + if d.IsDir() { + return filepath.SkipDir + } + return nil + } if mf.IsScratch(rel) { res.Candidates = append(res.Candidates, rel) if d.IsDir() { @@ -357,6 +441,7 @@ func RunWithTrash(root string) (*Result, error) { return nil, err } res := &Result{Root: root, ManifestPath: mf.Path, Mode: "apply"} + gi := newGitignoreFilter(root, IncludeIgnored) _ = filepath.WalkDir(root, func(p string, d fs.DirEntry, werr error) error { if werr != nil || p == root { return werr @@ -372,6 +457,13 @@ func RunWithTrash(root string) (*Result, error) { } return nil } + if gi.ignored(rel) { + // git-ignored working file from another tool — not forge's to remove. + if d.IsDir() { + return filepath.SkipDir + } + return nil + } if mf.IsScratch(rel) { res.Candidates = append(res.Candidates, rel) if d.IsDir() { diff --git a/internal/cli/cmdship/hook.go b/internal/cli/cmdship/hook.go index b50951c..73b0b35 100644 --- a/internal/cli/cmdship/hook.go +++ b/internal/cli/cmdship/hook.go @@ -278,6 +278,36 @@ func isHookDisabled(cfg HookConfig, hookName string) bool { return false } +// ctxSpecSlug resolves the spec-directory name a hook should look in. The +// --name/-n override (HookContext.SpecName) always wins over a slug derived +// from the feature description: agent-mode and `forge ship -n ` +// write every artefact under .forge/specs//, so a gate that only +// slugified the (often paragraph-long) description looked in the wrong +// directory and reported every artefact "not found". +func ctxSpecSlug(ctx HookContext) string { + if s := strings.TrimSpace(ctx.SpecName); s != "" { + return s + } + return slugify(ctx.Description) +} + +// readSpecArtefact reads the first of names[] that exists in the spec dir for +// ctx, returning its bytes and the basename that matched. Lets a gate accept +// both the canonical filename and the aliases agent-mode / older pipelines +// write (arch.md vs adr.md, test.md vs tests.md, code-plan.md vs impl-notes.md). +func readSpecArtefact(ctx HookContext, names ...string) ([]byte, string, error) { + dir := filepath.Join(ctx.Root, ".forge", "specs", ctxSpecSlug(ctx)) + var lastErr error + for _, n := range names { + data, err := os.ReadFile(filepath.Join(dir, n)) + if err == nil { + return data, n, nil + } + lastErr = err + } + return nil, "", lastErr +} + // ── Default hooks ───────────────────────────────────────────────────────────── // selfReviewGate checks for placeholder text and hedging language in the @@ -289,16 +319,21 @@ var selfReviewGate = Hook{ Gate: "", // applies to ALL checkpoints Handler: func(ctx HookContext) HookResult { // Map each checkpoint to the artefact files it produces. - slug := slugify(ctx.Description) + slug := ctxSpecSlug(ctx) base := filepath.Join(ctx.Root, ".forge", "specs", slug) + // Each checkpoint lists every filename it might have produced. The + // canonical name comes first; the rest are the names agent-mode and + // older pipeline versions actually write (test-stubs.md, code-plan.md, + // …). A gate that scanned only the canonical name reported UNVERIFIED + // on every agent-mode run. artifactsByCheckpoint := map[string][]string{ "spec": {filepath.Join(base, "spec.md")}, "arch": {filepath.Join(base, "arch.md"), filepath.Join(base, "adr.md")}, - "test": {filepath.Join(base, "tests.md")}, - "breakdown": {filepath.Join(base, "tasks.md")}, - "code": {filepath.Join(base, "impl-notes.md")}, - "ship": {filepath.Join(base, "ship-checklist.md")}, - "qa-verify": {filepath.Join(base, "qa-report.md")}, + "test": {filepath.Join(base, "tests.md"), filepath.Join(base, "test.md"), filepath.Join(base, "test-stubs.md")}, + "breakdown": {filepath.Join(base, "tasks.md"), filepath.Join(base, "breakdown.md")}, + "code": {filepath.Join(base, "impl-notes.md"), filepath.Join(base, "code.md"), filepath.Join(base, "code-plan.md")}, + "ship": {filepath.Join(base, "ship-checklist.md"), filepath.Join(base, "ship.md")}, + "qa-verify": {filepath.Join(base, "qa-report.md"), filepath.Join(base, "qa-verify.md")}, } filesToScan, ok := artifactsByCheckpoint[ctx.CheckpointName] if !ok { @@ -343,7 +378,7 @@ var specCompletenessGate = Hook{ if ctx.Result == nil || ctx.Result.Status == "fail" { return gateNotApplicable() } - slug := slugify(ctx.Description) + slug := ctxSpecSlug(ctx) specPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "spec.md") data, err := os.ReadFile(specPath) if err != nil { @@ -376,7 +411,7 @@ var taskCompletionGate = Hook{ if ctx.Result == nil || ctx.Result.Status == "fail" { return gateNotApplicable() } - slug := slugify(ctx.Description) + slug := ctxSpecSlug(ctx) tasksPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "tasks.md") data, err := os.ReadFile(tasksPath) if err != nil { @@ -406,11 +441,9 @@ var adrQualityGate = Hook{ if ctx.Result == nil || ctx.Result.Status == "fail" { return gateNotApplicable() } - slug := slugify(ctx.Description) - adrPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "adr.md") - data, err := os.ReadFile(adrPath) + data, _, err := readSpecArtefact(ctx, "adr.md", "arch.md") if err != nil { - return gateUnknown("adr-quality-gate: adr.md not found — architecture decision unverified") + return gateUnknown("adr-quality-gate: adr.md / arch.md not found — architecture decision unverified") } content := strings.ToLower(string(data)) // Look for at least 2 alternative headings or list items. @@ -434,7 +467,7 @@ var archFileLint = Hook{ if ctx.Result == nil || ctx.Result.Status == "fail" { return gateNotApplicable() } - slug := slugify(ctx.Description) + slug := ctxSpecSlug(ctx) archPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "arch.md") data, err := os.ReadFile(archPath) if err != nil { @@ -470,11 +503,9 @@ var tddGate = Hook{ if ctx.Result == nil || ctx.Result.Status == "fail" { return gateNotApplicable() } - slug := slugify(ctx.Description) - testsPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "tests.md") - data, err := os.ReadFile(testsPath) + data, _, err := readSpecArtefact(ctx, "tests.md", "test.md", "test-stubs.md") if err != nil { - return gateUnknown("tdd-gate: tests.md not found — test quality unverified") + return gateUnknown("tdd-gate: tests.md / test.md not found — test quality unverified") } content := string(data) // Detect always-passing anti-patterns. @@ -507,7 +538,7 @@ var breakdownCompletenessGate = Hook{ if ctx.Result == nil || ctx.Result.Status == "fail" { return gateNotApplicable() } - slug := slugify(ctx.Description) + slug := ctxSpecSlug(ctx) tasksPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "tasks.md") data, err := os.ReadFile(tasksPath) if err != nil { @@ -539,11 +570,9 @@ var securityHygieneGate = Hook{ if ctx.Result == nil || ctx.Result.Status == "fail" { return gateNotApplicable() } - slug := slugify(ctx.Description) - implPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "impl-notes.md") - data, err := os.ReadFile(implPath) + data, _, err := readSpecArtefact(ctx, "impl-notes.md", "code.md", "code-plan.md") if err != nil { - return gateUnknown("security-hygiene-gate: impl-notes.md not found — implementation notes unscanned") + return gateUnknown("security-hygiene-gate: impl-notes.md / code-plan.md not found — implementation notes unscanned") } content := string(data) // Secret-like patterns. @@ -577,7 +606,7 @@ var qaCoverageGate = Hook{ if ctx.Result == nil || ctx.Result.Status == "fail" { return gateNotApplicable() } - slug := slugify(ctx.Description) + slug := ctxSpecSlug(ctx) specPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "spec.md") qaPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "qa-report.md") @@ -674,7 +703,7 @@ var manualTestPlanGate = Hook{ if ctx.Result == nil || ctx.Result.Status == "fail" { return gateNotApplicable() } - slug := slugify(ctx.Description) + slug := ctxSpecSlug(ctx) planPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "manual-test-plan.md") data, err := os.ReadFile(planPath) if err != nil { diff --git a/internal/cli/cmdship/prompts_and_learning.go b/internal/cli/cmdship/prompts_and_learning.go index db6e4d1..5841962 100644 --- a/internal/cli/cmdship/prompts_and_learning.go +++ b/internal/cli/cmdship/prompts_and_learning.go @@ -68,7 +68,8 @@ func ensurePromptTemplates(root string) { //nolint:unused // called from ship in "ship-spec": "You are a senior product engineer writing a feature specification.\n" + "Produce a Markdown spec with sections: Goal, Scope, Acceptance Criteria, Non-Goals, Open Questions.\n", "ship-test": "You are a senior QA engineer writing failing test stubs for TDD.\n" + - "Tests MUST compile but MUST fail at runtime. Use Jest + supertest for TypeScript, testing.T for Go.\n", + "Tests MUST compile but MUST fail at runtime. Use the project's own test runner and " + + "conventions (do not import a package the project lacks, e.g. supertest).\n", "ship-breakdown": "You are a delivery lead decomposing a feature spec into atomic tasks.\n" + "Format: numbered list. Each task: title, effort (XS/S/M/L), dependencies, acceptance criteria.\n", "ship-code": "You are a senior engineer writing a step-by-step implementation plan.\n" + diff --git a/internal/cli/cmdship/ship.go b/internal/cli/cmdship/ship.go index 52d614f..87026ba 100644 --- a/internal/cli/cmdship/ship.go +++ b/internal/cli/cmdship/ship.go @@ -1239,7 +1239,7 @@ func checkTest(root, description, specName string, pipe *LLMPipe, dryRun bool) C } else { cp.Detail = fmt.Sprintf("%d test file(s) found; missing artifacts: %s", len(testFiles), strings.Join(missing, ", ")) } - applyReachability(root, testFiles, &cp) + applyReachability(root, artefactFilesForReachability(root, slug), &cp) if pipe != nil { if _, err := generateTestStubs(root, description, slug, pipe); err != nil { if agentPauseCheckpoint(&cp, "ship:test:generate", err) { @@ -1270,7 +1270,7 @@ func checkTest(root, description, specName string, pipe *LLMPipe, dryRun bool) C if pipe == nil { cp.Detail += " (run 'forge config set llm.provider ' or set ANTHROPIC_API_KEY / OPENAI_API_KEY)" } - applyReachability(root, findTestFiles(root), &cp) + applyReachability(root, artefactFilesForReachability(root, slug), &cp) return cp } @@ -1373,6 +1373,34 @@ func findTestFiles(root string) []string { return out } +// slugTestArtefactPaths returns the absolute paths of the test artefacts forge +// itself scaffolds for slug (across every supported layout) that currently +// exist on disk. The Test checkpoint's reachability check must run against +// *these* files only — running it against every *_test/*.spec file in the repo +// (findTestFiles) drags in the project's pre-existing e2e/staging/smoke suites, +// which are deliberately outside the unit runner, and reports them as +// "unreachable" — noise that buried the one signal that mattered. +func artefactFilesForReachability(root, slug string) []string { + cands := []string{ + "tests/" + slug + ".test.ts", + "tests/" + slug + ".integration.test.ts", + "tests/" + slug + ".rls.test.ts", + "src/test/" + slug + ".test.ts", + "src/test/" + slug + ".integration.test.ts", + "src/test/" + slug + ".rls.test.ts", + "src/__tests__/" + slug + ".test.ts", + "test/" + slug + ".test.ts", + } + var out []string + for _, c := range cands { + p := filepath.Join(root, filepath.FromSlash(c)) + if _, err := os.Stat(p); err == nil { + out = append(out, p) + } + } + return out +} + // checkBreakdown looks for a task-breakdown file in .forge/specs// and, // when an LLMPipe is available, generates one if it does not exist. // specName, when non-empty, overrides the slug derived from description. @@ -1437,6 +1465,7 @@ func checkBreakdown(root, description, specName string, pipe *LLMPipe) Checkpoin func checkCode(root, description, specName string, pipe *LLMPipe) Checkpoint { cp := Checkpoint{Name: "Code"} changedFiles := countChangedFiles(root) + changedSource := countChangedSourceFiles(root) // Determine slug: --name/-n takes priority over slug derived from description. slug := specName @@ -1450,10 +1479,10 @@ func checkCode(root, description, specName string, pipe *LLMPipe) Checkpoint { return cp } if err != nil { - if changedFiles > 0 { + if changedSource > 0 { cp.Status = "ok" - cp.Detail = fmt.Sprintf("%d modified file(s) [LLM:%s — %s]", - changedFiles, pipe.ProviderName(), llmErrNote(err)) + cp.Detail = fmt.Sprintf("%d changed source file(s) [LLM:%s — %s]", + changedSource, pipe.ProviderName(), llmErrNote(err)) } else { cp.Status = "warning" cp.Detail = fmt.Sprintf("no code changes detected [LLM:%s — %s]", @@ -1462,14 +1491,25 @@ func checkCode(root, description, specName string, pipe *LLMPipe) Checkpoint { return cp } if plan != "" { - if changedFiles > 0 { + // A plan is not an implementation. Only claim the checkpoint is + // satisfied when actual source files changed this run; otherwise + // report the plan as the deliverable and ask for implementation. + // Ambient dirty files (.forge/, docs/, other sessions' scratch) are + // excluded by countChangedSourceFiles so they can't fake a pass — + // this was forge-expert known-gap #5. + ambient := "" + if changedFiles > changedSource { + ambient = fmt.Sprintf(" (%d other working-tree change(s) not attributed to this checkpoint)", + changedFiles-changedSource) + } + if changedSource > 0 { cp.Status = "ok" - cp.Detail = fmt.Sprintf("%d modified file(s); code plan written by %s (see .forge/specs/%s/code-plan.md)", - changedFiles, pipe.ProviderName(), slug) + cp.Detail = fmt.Sprintf("%d changed source file(s); code plan written by %s (see .forge/specs/%s/code-plan.md)%s", + changedSource, pipe.ProviderName(), slug, ambient) } else { - cp.Status = "ok" - cp.Detail = fmt.Sprintf("code plan written by %s (see .forge/specs/%s/code-plan.md) — implement then rerun", - pipe.ProviderName(), slug) + cp.Status = "warning" + cp.Detail = fmt.Sprintf("code plan written by %s (see .forge/specs/%s/code-plan.md) — no source files changed yet; implement then rerun forge ship code%s", + pipe.ProviderName(), slug, ambient) } return cp } @@ -1484,9 +1524,9 @@ func checkCode(root, description, specName string, pipe *LLMPipe) Checkpoint { } // Structural fallback (no LLM or no spec/breakdown context). - if changedFiles > 0 { + if changedSource > 0 { cp.Status = "ok" - cp.Detail = fmt.Sprintf("%d modified file(s) detected in working tree", changedFiles) + cp.Detail = fmt.Sprintf("%d changed source file(s) detected in working tree", changedSource) return cp } cp.Status = "warning" @@ -1522,6 +1562,46 @@ func countChangedFiles(root string) int { return len(statuses) } +// sourceLikeExts are the extensions countChangedSourceFiles treats as "code +// this checkpoint could have produced". Deliberately excludes docs, images, +// lockfiles and forge's own scratch/ledger files. +var sourceLikeExts = map[string]bool{ + ".go": true, ".ts": true, ".tsx": true, ".js": true, ".jsx": true, + ".py": true, ".sql": true, ".rs": true, ".java": true, ".rb": true, + ".kt": true, ".swift": true, ".c": true, ".cc": true, ".cpp": true, + ".h": true, ".hpp": true, ".cs": true, ".php": true, ".scala": true, + ".vue": true, ".svelte": true, ".sh": true, +} + +// countChangedSourceFiles counts working-tree changes that plausibly belong to +// an implementation: source-code extensions, and never under .forge/, docs/, +// or other ambient locations. This stops the Code checkpoint from reporting +// "N modified file(s)" (and status ok) purely because of unrelated dirty files +// — another session's .forge/token-ledger.jsonl, stray screenshots, growth/ +// notes — when in fact no feature code was written this run. +func countChangedSourceFiles(root string) int { + svc, err := gitservice.New(root) + if err != nil { + return 0 + } + statuses, err := svc.Status() + if err != nil { + return 0 + } + n := 0 + for _, st := range statuses { + p := filepath.ToSlash(st.Path) + if strings.HasPrefix(p, ".forge/") || strings.HasPrefix(p, "docs/") || + strings.HasPrefix(p, "growth/") || strings.Contains(p, "/node_modules/") { + continue + } + if sourceLikeExts[strings.ToLower(filepath.Ext(p))] { + n++ + } + } + return n +} + // checkVerify runs the security scanner, clean check, checks the manifest, // and (TG-39) audits spec artefacts for incomplete tasks and authz gaps. // M1-10: forge clean --check is now wired here. diff --git a/internal/cli/cmdship/ship_test.go b/internal/cli/cmdship/ship_test.go index 4d55218..4b95239 100644 --- a/internal/cli/cmdship/ship_test.go +++ b/internal/cli/cmdship/ship_test.go @@ -1770,8 +1770,15 @@ func TestCheckCode_LLM_GeneratesCodePlan(t *testing.T) { } cp := checkCode(root, "code plan feature", "", mockPipe(root, mock)) - if cp.Status != "ok" { - t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) + // A plan is not an implementation: with no source files changed (this + // TempDir isn't a git repo), the checkpoint reports "warning" and asks for + // implementation rather than a misleading "ok" — countChangedSourceFiles / + // forge-expert gap #5 fix. + if cp.Status != "warning" { + t.Fatalf("expected warning (plan written, no code yet), got %q: %s", cp.Status, cp.Detail) + } + if !strings.Contains(cp.Detail, "code-plan.md") { + t.Fatalf("detail should reference the written plan: %s", cp.Detail) } data, err := os.ReadFile(filepath.Join(dir, "code-plan.md")) if err != nil { @@ -1814,8 +1821,13 @@ func TestCheckCode_LLM_HonorsSpecNameOverride(t *testing.T) { } cp := checkCode(root, description, "custom-slug", mockPipe(root, mock)) - if cp.Status != "ok" { - t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) + // "warning" (plan written, no source changed) — see gap #5 fix. What this + // test guards is that the plan lands under the --name dir, not the status. + if cp.Status != "warning" { + t.Fatalf("expected warning (plan written, no code yet), got %q: %s", cp.Status, cp.Detail) + } + if !strings.Contains(cp.Detail, "custom-slug/code-plan.md") { + t.Fatalf("detail should point at the --name override dir: %s", cp.Detail) } if mock.Calls() == 0 { t.Fatal("MockProvider.Complete was not called — generateCodePlan did not find spec.md/breakdown.md under the --name override directory") diff --git a/internal/cli/cmdship/test_artifacts.go b/internal/cli/cmdship/test_artifacts.go index 1617c0b..f151ff6 100644 --- a/internal/cli/cmdship/test_artifacts.go +++ b/internal/cli/cmdship/test_artifacts.go @@ -116,10 +116,30 @@ func expectedTestArtifactNames(root, slug string) []string { // allTestArtifactsExist returns true when all of this project's expected // artifacts (per its detected stack, J5) are present. +// testArtefactDirs is the set of directories an artefact may live in: the +// preferred one for new writes, plus the legacy "tests/" for repos scaffolded +// before pickTestsDir existed. A file present in ANY of them counts as written. +func testArtefactDirs(root string) []string { + preferred := pickTestsDir(root) + legacy := filepath.Join(root, "tests") + if preferred == legacy { + return []string{preferred} + } + return []string{preferred, legacy} +} + +func artefactExists(root, name string) bool { + for _, d := range testArtefactDirs(root) { + if _, err := os.Stat(filepath.Join(d, name)); err == nil { + return true + } + } + return false +} + func allTestArtifactsExist(root, slug string) bool { //nolint:unused // used in ship dry-run gate - testsDir := filepath.Join(root, "tests") for _, name := range expectedTestArtifactNames(root, slug) { - if _, err := os.Stat(filepath.Join(testsDir, name)); err != nil { + if !artefactExists(root, name) { return false } } @@ -129,10 +149,9 @@ func allTestArtifactsExist(root, slug string) bool { //nolint:unused // used in // missingTestArtifacts lists the expected artifact filenames (per this // project's detected stack, J5) that are absent. func missingTestArtifacts(root, slug string) []string { - testsDir := filepath.Join(root, "tests") var missing []string for _, name := range expectedTestArtifactNames(root, slug) { - if _, err := os.Stat(filepath.Join(testsDir, name)); err != nil { + if !artefactExists(root, name) { missing = append(missing, name) } } @@ -332,8 +351,23 @@ func CheckTestFilesExist(files []string) CheckTestFilesResult { // 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. +// pickTestsDir chooses where to scaffold test artefacts. A hardcoded +// "/tests" lands outside the collected path on the many JS/TS projects +// whose runner roots are "src" (Jest `roots: ["src"]`, CRA, Next.js +// conventions) — the files then show up as "unreachable / will never run". If +// one of the common already-collected test directories exists, prefer it; +// otherwise fall back to "tests". +func pickTestsDir(root string) string { + for _, rel := range []string{"src/test", "src/__tests__", "src/tests", "test", "__tests__"} { + if fi, err := os.Stat(filepath.Join(root, filepath.FromSlash(rel))); err == nil && fi.IsDir() { + return filepath.Join(root, filepath.FromSlash(rel)) + } + } + return filepath.Join(root, "tests") +} + func writeTestArtifactsWithContext(root, slug, feature, specMD string, fw TestFrameworkContext, pipe *LLMPipe) (TestArtifactPaths, error) { - testsDir := filepath.Join(root, "tests") + testsDir := pickTestsDir(root) if err := os.MkdirAll(testsDir, 0o755); err != nil { return TestArtifactPaths{}, nil } @@ -423,8 +457,14 @@ func writeTestArtifactsWithContext(root, slug, feature, specMD string, fw TestFr integFn := func() (string, bool, error) { return pipe.InvokeChecked("ship:test:integration", "", - "You are writing failing integration test stubs ("+runnerTitle+" + supertest) for TDD. "+ - "Tests MUST fail. Import test functions from \""+runner+"\", never a different test framework.", + "You are writing failing integration test stubs for TDD using "+runnerTitle+". "+ + "Import test functions from \""+runner+"\", never a different test framework. "+ + "Exercise the feature end-to-end (API routes, RPCs, DB). Follow the project's "+ + "existing integration-test convention — use supertest ONLY if the project already "+ + "depends on it and exposes a single HTTP entrypoint; otherwise invoke route "+ + "handlers / service functions directly (e.g. a Next.js App Router handler, or a "+ + "Supabase client's .rpc()). Do not import a package the project does not have. "+ + "Tests MUST compile but MUST fail at runtime until implemented.", ctx+"Generate failing integration test stubs for feature: "+feature, 6000) } gen, complete, err = generateWithValidation(integFn) From af0e39450480d20ad13c4f6965ad414f56802c71 Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Mon, 7 Sep 2026 20:52:45 +0700 Subject: [PATCH 2/4] fix(ship): self-review-gate no longer flags legitimate "..." in artefacts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing bare "..." from the contains-scan (it hit prose elisions, "P1-01 … P1-09" ranges and code snippets). Unfilled ellipsis placeholders are still caught by a per-line check — a line that is only dots, or a "key: ..." / "= ..." stub — which skips fenced code blocks and does not fire on real content. Co-Authored-By: Claude Sonnet 5 --- internal/cli/cmdship/hook.go | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/internal/cli/cmdship/hook.go b/internal/cli/cmdship/hook.go index 73b0b35..1d006c2 100644 --- a/internal/cli/cmdship/hook.go +++ b/internal/cli/cmdship/hook.go @@ -341,7 +341,12 @@ var selfReviewGate = Hook{ ctx.CheckpointName) } - badPatterns := []string{"TODO", "TBD", "# ") // list/quote/heading markers + if t == "..." || t == "…" || + strings.HasSuffix(t, ": ...") || strings.HasSuffix(t, "= ...") || + strings.HasSuffix(t, ": …") || strings.HasSuffix(t, "= …") { + return gateFail("self-review-gate: unfilled placeholder line in %s: %q", + filepath.Base(fp), strings.TrimSpace(ln)) + } + } } // Scanning zero files is not a clean bill of health. On a first run the // artefact does not exist yet — the checkpoint is about to write it — From 83664147b4e4dd6eb4e81b4c91dfbe5554cbc87d Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Mon, 7 Sep 2026 20:56:32 +0700 Subject: [PATCH 3/4] fix(ship): four-stage-testing-gate honours the --name/-n spec slug testingPipelineEvidencePath slugified ctx.Description, so on an agent-mode run (which always sets --name/-n) the gate looked for testing-pipeline.md under a truncated description slug and reported it missing even when it existed under .forge/specs//. Now resolves via ctxSpecSlug(ctx); the path helper also accepts an already-resolved slug. Co-Authored-By: Claude Sonnet 5 --- internal/cli/cmdship/testing_pipeline.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/internal/cli/cmdship/testing_pipeline.go b/internal/cli/cmdship/testing_pipeline.go index 8b6b94c..a68f29b 100644 --- a/internal/cli/cmdship/testing_pipeline.go +++ b/internal/cli/cmdship/testing_pipeline.go @@ -63,8 +63,16 @@ var testingPipelineStages = []testingPipelineStage{ // evidence for the current feature. Mirrors manualTestPlanGate's and // qaCoverageGate's own artefact path convention exactly // (.forge/specs//.md). -func testingPipelineEvidencePath(root, description string) string { - return filepath.Join(root, ".forge", "specs", slugify(description), "testing-pipeline.md") +func testingPipelineEvidencePath(root, nameOrDesc string) string { + // Treat an argument that already looks like a slug (no spaces/separators) + // as one; slugify a raw description. This makes a resolved --name/-n slug + // and a feature description land on the same directory the rest of the + // pipeline writes to — agent-mode always passes the former. + slug := nameOrDesc + if strings.ContainsAny(slug, " \t/\\") { + slug = slugify(nameOrDesc) + } + return filepath.Join(root, ".forge", "specs", slug, "testing-pipeline.md") } // missingTestingPipelineStages reports which stage keywords are absent from @@ -93,7 +101,7 @@ var fourStageTestingGate = Hook{ return gateNotApplicable() } - path := testingPipelineEvidencePath(ctx.Root, ctx.Description) + path := testingPipelineEvidencePath(ctx.Root, ctxSpecSlug(ctx)) data, err := os.ReadFile(path) if err != nil { if !ctx.StrictTesting { @@ -143,7 +151,7 @@ var fourStageTestingReminder = Hook{ b.WriteString(" (--strict-testing is ON: qa-verify already enforced this via testing-pipeline.md)\n") } else { b.WriteString(fmt.Sprintf(" Advisory only. Document evidence in %s and re-run with\n --strict-testing to make this a blocking gate.\n", - filepath.Base(testingPipelineEvidencePath(ctx.Root, ctx.Description)))) + filepath.Base(testingPipelineEvidencePath(ctx.Root, ctxSpecSlug(ctx))))) } fmt.Fprint(os.Stderr, b.String()) return gatePass() From d52d87fc205ac03123190f36e05100bef30ba30f Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Mon, 7 Sep 2026 20:59:11 +0700 Subject: [PATCH 4/4] fix(ship): tdd-gate + self-review recognise JS/TS test-scenario syntax - tdd-gate now accepts it()/test()/describe() (Jest/Vitest/Jasmine/Mocha) as a test scenario, not only Gherkin or `func Test*`, and reads test-stubs.md in preference to forge's thin test.md summary for the quality check. Co-Authored-By: Claude Sonnet 5 --- internal/cli/cmdship/hook.go | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/internal/cli/cmdship/hook.go b/internal/cli/cmdship/hook.go index 1d006c2..985ad9d 100644 --- a/internal/cli/cmdship/hook.go +++ b/internal/cli/cmdship/hook.go @@ -526,9 +526,11 @@ var tddGate = Hook{ if ctx.Result == nil || ctx.Result.Status == "fail" { return gateNotApplicable() } - data, _, err := readSpecArtefact(ctx, "tests.md", "test.md", "test-stubs.md") + // Prefer the actual stub file (test-stubs.md) over forge's thin + // test.md summary for a *quality* check. + data, _, err := readSpecArtefact(ctx, "tests.md", "test-stubs.md", "test.md") if err != nil { - return gateUnknown("tdd-gate: tests.md / test.md not found — test quality unverified") + return gateUnknown("tdd-gate: tests.md / test-stubs.md not found — test quality unverified") } content := string(data) // Detect always-passing anti-patterns. @@ -541,10 +543,22 @@ var tddGate = Hook{ return gateFail("tdd-gate: always-passing or skipped test pattern detected: %q", pat) } } - // Must reference at least one Given/When/Then or test scenario. - if !strings.Contains(content, "Given ") && !strings.Contains(content, "Scenario:") && - !strings.Contains(content, "func Test") { - return gateFail("tdd-gate: tests.md must contain at least one test scenario (Given/When/Then or func Test*)") + // Must reference at least one test scenario — Gherkin (Given/When/Then, + // Scenario:), Go (func Test*), or a JS/TS runner block (it(/test(/ + // describe(), which is how Jest/Vitest/Jasmine/Mocha express one. + scenarioMarkers := []string{ + "Given ", "Scenario:", "func Test", + "it(", "it('", "it(\"", "test(", "describe(", + } + hasScenario := false + for _, m := range scenarioMarkers { + if strings.Contains(content, m) { + hasScenario = true + break + } + } + if !hasScenario { + return gateFail("tdd-gate: no test scenario found (Given/When/Then, func Test*, or it()/test()/describe())") } return gatePass() },