From 7f5fcbef394b1837004b55263e3d768a19eb25d4 Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Mon, 21 Sep 2026 02:14:33 +0700 Subject: [PATCH 1/2] fix(ship): agent-mode arch debate, --until, and false-green/noisy gates found dogfooding on a two-repo project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by driving `forge ship --agent-mode` through the spec and arch checkpoints of a real feature on a Next.js/Supabase + Python two-repo system. Fixed - arch: in agent mode checkArch wrote arch.md (with "(no concerns raised)" placeholders for every role) while the first debate turn was still owed. The next run hit the "arch.md exists" shortcut, so the debate never resumed and the submitted answer was discarded. It now pauses without writing and asks each of the six roles in turn. - arch: the generation prompt never asked for alternatives although adr-quality-gate fails an ADR with fewer than two; it now does. - ship: reported status ok, with a "spec-vs-code audit found no blocking gaps" evidence line, on a branch with no source changes. It now warns ("nothing to ship") and withholds the audit claim. Unknown git state keeps the old behaviour. - test: the detail reported the whole repository's test-file count ("671 test file(s) found"). It now reports the feature's own tests and warns when it has none. - agent-mode banner said "[YOLO — approval gates disabled]" although --yolo was never passed; it now describes agent mode. - unverified-file-reference check flagged every cross-repo citation as hallucinated. forge.yml `related_repos` now resolves them and lists the sibling repos in the workspace context. - workspace context reported only "Node.js" / "GitHub Actions CI" for a Next.js + Supabase + TypeScript project; it now detects frameworks from package.json, tsconfig and supabase/. The feature being planned is no longer listed as an existing spec once its directory exists. - failure history injected into a spec prompt included nine-day-old failures of unrelated features and repeated identical lines; it is now scoped to the current feature (recent others only) and de-duplicated. Added - `forge ship --until `: stop after the named checkpoint so spec and arch can be reviewed before test/breakdown/code exist. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Nguyen Quang Trung --- docs/verbs/ship.md | 23 +++ internal/cli/cmdship/arch.go | 20 +- internal/cli/cmdship/arch_test.go | 141 ++++++++++++++ internal/cli/cmdship/artefact_validate.go | 74 ++++++- .../cli/cmdship/artefact_validate_test.go | 72 +++++++ internal/cli/cmdship/artefacts_test.go | 83 +++++++- internal/cli/cmdship/prompts_and_learning.go | 47 ++++- internal/cli/cmdship/ship.go | 183 +++++++++++++++++- .../cli/cmdship/ship_nothing_to_ship_test.go | 114 +++++++++++ internal/cli/cmdship/ship_test.go | 112 +++++++++++ .../cli/cmdship/test_feature_scope_test.go | 110 +++++++++++ internal/cli/cmdship/workspace_context.go | 111 ++++++++++- .../cli/cmdship/workspace_context_test.go | 85 ++++++++ internal/gitservice/gitservice.go | 47 +++++ internal/gitservice/gitservice_test.go | 88 +++++++++ 15 files changed, 1286 insertions(+), 24 deletions(-) create mode 100644 internal/cli/cmdship/ship_nothing_to_ship_test.go create mode 100644 internal/cli/cmdship/test_feature_scope_test.go diff --git a/docs/verbs/ship.md b/docs/verbs/ship.md index 94df69a..1f0d1d2 100644 --- a/docs/verbs/ship.md +++ b/docs/verbs/ship.md @@ -85,6 +85,12 @@ forge ship auth/email arch # Run only the QA agent checkpoint forge ship auth/email qa-verify +# Stop after arch so the spec and ADR can be reviewed before any code exists +forge ship auth/email --until arch + +# Then continue from the next checkpoint +forge ship auth/email --from test + # Skip the QA agent (no test runner configured) forge ship auth/email --skip-checkpoint qa-verify @@ -112,8 +118,25 @@ forge ship --tag v1.2.3 | `--no-branch` | false | Do not create or switch to a feature branch; run on current branch | | `--tag ` | — | After a clean pipeline, tag and push a release | | `--skip-checkpoint ` | — | Skip a named checkpoint (e.g. `qa-verify` when no test runner is configured) | +| `--until ` | — | Stop after the named checkpoint (e.g. `--until arch` runs spec and arch, then stops so you can review before test/breakdown/code). Repeat it on every `--agent-mode` continuation; resume later with `--from ` | | `--strict-testing` | false | Enforce the 4-stage testing pipeline (local → pre-push/CI → staging → production) as a blocking `qa-verify` gate instead of an advisory reminder — see below | +## Multi-repository projects (`related_repos`) + +A spec or ADR for one repository often cites files that live in a sibling +repository (a web app and its agent service, an API and its SDK). List those +siblings in `forge.yml` and forge will (a) resolve cited paths against them +instead of flagging every cross-repo citation as "may be hallucinated", and +(b) tell the model they exist, with their detected stacks, in the workspace +context it is given. + +```yaml +related_repos: + - ../ai-agent-system # relative to the project root, or an absolute path +``` + +Entries that do not exist are ignored. + ## The 4-stage testing pipeline (`--strict-testing`) `forge ship`'s checkpoints prove a feature was specced, coded, and passed diff --git a/internal/cli/cmdship/arch.go b/internal/cli/cmdship/arch.go index 3c3fb7e..327192a 100644 --- a/internal/cli/cmdship/arch.go +++ b/internal/cli/cmdship/arch.go @@ -237,7 +237,10 @@ func checkArch(root, description, specName string, pipe *LLMPipe, dryRun bool) C "3) Data Model & Consistency — data entities, migration strategy, consistency model; " + "4) Non-Functional Requirements — p99 latency, throughput, availability SLOs; " + "5) Security Threat Model — STRIDE threats, mitigations, auth/authz boundaries; " + - "6) Deployment & Observability — topology, health checks, metrics, tracing, DR plan. " + + "6) Deployment & Observability — topology, health checks, metrics, tracing, DR plan; " + + "7) Alternatives Considered — at least two genuinely different options you rejected " + + "(label them Alternative A, Alternative B, ...), each with the reason it lost; the ADR " + + "quality gate fails an ADR that evaluates fewer than two. " + "End with a concise ADR summary: Status, Context, Decision, Consequences. " + "After the ADR, append a fenced ```yaml block containing a valid OpenAPI 3.1.0 contract " + "for all API endpoints introduced by this feature. " + @@ -287,6 +290,21 @@ func checkArch(root, description, specName string, pipe *LLMPipe, dryRun bool) C archContent, openapiContent = extractOpenAPIBlock(generated, description) // P1: run parallel role debate and append reviewer concerns. debateSuffix := runParallelArchDebate(pipe, description, archContent, 300) + // Agent mode: a role's turn is owed. Nothing may be written yet. + // Writing arch.md here with placeholder "(no concerns raised)" + // sections made the next run hit the "arch.md already exists" + // idempotency shortcut above, so the debate never resumed and the + // answer the host agent submitted was silently discarded (seen on + // a real run: all six roles read "(no concerns raised)" although + // one had been answered). Return before the write; on the next + // run the recorded generate answer and the answered roles replay, + // and the next unanswered role is asked. + if b := pipe.Bridge(); b != nil && b.Paused() { + cp.Status = "ok" + cp.Detail = "awaiting host-agent turn for arch-parallel-debate — run: forge agent prompt" + cp.AgentPaused = true + return cp + } if debateSuffix != "" { archContent += debateSuffix } diff --git a/internal/cli/cmdship/arch_test.go b/internal/cli/cmdship/arch_test.go index 297b77c..79980a6 100644 --- a/internal/cli/cmdship/arch_test.go +++ b/internal/cli/cmdship/arch_test.go @@ -36,8 +36,10 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" + "github.com/teragrid/forge/internal/agentbridge" "github.com/teragrid/forge/internal/llmprovider" ) @@ -706,3 +708,142 @@ func TestCheckArch_LLM_SupabaseRPCResponse(t *testing.T) { t.Errorf("expected supabase-rpc style detected from extracted openapi.yaml, got %q", style) } } + +// TestCheckArch_AgentMode_AsksEveryRoleAndKeepsTheAnswers is a regression test +// for the agent-mode arch debate silently collapsing to a single turn. +// +// checkArch used to run the debate, notice nothing, and write arch.md straight +// away even though the bridge had just paused on the first role's turn. The +// file therefore contained "(no concerns raised)" for every role, and the next +// run hit the "arch.md already exists" idempotency shortcut — the debate never +// resumed, and the answer the host agent had just submitted was discarded. +// Seen on a real run: six roles, one answered, all six read "(no concerns +// raised)" in the final document. +// +// Contract pinned here: while any role's turn is owed the checkpoint pauses and +// writes nothing; once every role has answered, arch.md holds every answer and +// no placeholder. +func TestCheckArch_AgentMode_AsksEveryRoleAndKeepsTheAnswers(t *testing.T) { + t.Parallel() + root := t.TempDir() + const slug = "debate-feat" + + specDir := filepath.Join(root, ".forge", "specs", slug) + if err := os.MkdirAll(specDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(specDir, "spec.md"), + []byte("# Spec: debate feat\n\n## What\nSomething.\n"), 0o600); err != nil { + t.Fatal(err) + } + archPath := filepath.Join(specDir, "arch.md") + + bridge, err := agentbridge.Open(root, agentbridge.DefaultSession) + if err != nil { + t.Fatalf("open bridge: %v", err) + } + pipe := newLLMPipeAgent(root, bridge) + + const adr = "# Architecture Decision Record: Debate Feat\n\n" + + "## 1. Component Topology\n\nOne handler.\n\n" + + "## 2. API Contracts\n\nREST.\n\n" + + "## ADR Summary\n\n- Status: Proposed.\n- Decision: build it.\n\n" + + "```yaml\nopenapi: 3.1.0\ninfo:\n title: Debate Feat\n version: 1.0.0\npaths: {}\n```\n" + + roles := defaultArchRoles() + debateTurns := 0 + maxIterations := len(roles) + 6 + done := false + for i := 0; i < maxIterations && !done; i++ { + cp := checkArch(root, slug, slug, pipe, false) + if !cp.AgentPaused { + done = true + break + } + if _, statErr := os.Stat(archPath); statErr == nil { + t.Fatalf("iteration %d: arch.md was written while a host-agent turn was still owed", i) + } + pending, ok := bridge.Pending() + if !ok { + t.Fatalf("iteration %d: checkpoint paused but no turn is pending", i) + } + answer := adr + if pending.Operation == "arch-parallel-debate" { + debateTurns++ + answer = fmt.Sprintf("concern-from-role-%d: check the boundary", debateTurns) + } + if _, ferr := bridge.Fulfil(answer); ferr != nil { + t.Fatalf("iteration %d: fulfil %s: %v", i, pending.Operation, ferr) + } + } + if !done { + t.Fatalf("checkArch never completed within %d iterations", maxIterations) + } + if debateTurns != len(roles) { + t.Fatalf("host agent was asked %d debate turn(s), want one per role (%d)", debateTurns, len(roles)) + } + + data, err := os.ReadFile(archPath) + if err != nil { + t.Fatalf("arch.md missing after completion: %v", err) + } + doc := string(data) + for n := 1; n <= len(roles); n++ { + if want := fmt.Sprintf("concern-from-role-%d", n); !strings.Contains(doc, want) { + t.Errorf("arch.md lost the answer %q", want) + } + } + if strings.Contains(doc, "(no concerns raised)") { + t.Errorf("arch.md contains a placeholder although every role answered:\n%s", doc) + } +} + +// TestCheckArch_PromptAsksForAlternatives pins the agreement between the arch +// generation prompt and the adr-quality-gate. The gate fails an ADR that +// evaluates fewer than two alternatives, but the prompt used to list six +// sections and never mention alternatives — so a model that followed the +// prompt to the letter tripped forge's own gate on the first try. +func TestCheckArch_PromptAsksForAlternatives(t *testing.T) { + t.Parallel() + root := t.TempDir() + slug := slugify("prompt alternatives") + dir := filepath.Join(root, ".forge", "specs", slug) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "spec.md"), []byte("# Spec\n"), 0o600); err != nil { + t.Fatal(err) + } + + var ( + mu sync.Mutex + systems []string + ) + mock := &llmprovider.MockProvider{ + Fn: func(req *llmprovider.Request) (*llmprovider.Response, error) { + mu.Lock() + systems = append(systems, req.SystemPrompt) + mu.Unlock() + return mockResponse("# Architecture\n\n## 1. Component Topology\n\nOne service.\n"), nil + }, + } + _ = checkArch(root, "prompt alternatives", "", mockPipe(root, mock), false) + + mu.Lock() + defer mu.Unlock() + found := false + for _, s := range systems { + if strings.Contains(s, "Structure the document with these sections") { + found = true + if !strings.Contains(s, "Alternatives Considered") { + t.Errorf("arch generation prompt must ask for an Alternatives Considered section; got:\n%s", s) + } + if !strings.Contains(s, "Consequences") { + t.Errorf("arch generation prompt must ask for Consequences; got:\n%s", s) + } + } + } + if !found { + t.Fatal("arch generation system prompt was never sent to the provider") + } +} diff --git a/internal/cli/cmdship/artefact_validate.go b/internal/cli/cmdship/artefact_validate.go index cb465f3..6e20194 100644 --- a/internal/cli/cmdship/artefact_validate.go +++ b/internal/cli/cmdship/artefact_validate.go @@ -31,6 +31,8 @@ import ( "path/filepath" "regexp" "strings" + + "gopkg.in/yaml.v3" ) // validateArtefact strips conversational preamble from raw and reports @@ -240,6 +242,73 @@ func generateWithValidation(invoke func() (string, bool, error)) (content string // ordinary prose sentences don't get flagged. var filePathRefPattern = regexp.MustCompile("`((?:[A-Za-z0-9_.-]+/)+[A-Za-z0-9_.-]+\\.[A-Za-z0-9]{1,6})`") +// loadRelatedRepos returns the absolute roots of the sibling repositories a +// project declares in forge.yml: +// +// related_repos: +// - ../ai-agent-system +// +// Many projects are several repositories (a web app and its agent service, an +// API and its SDK), and a spec or ADR for one legitimately cites files that +// live in another. Without this, the unverified-file-reference check could only +// ever look inside the current worktree and flagged every cross-repo citation +// as "may be hallucinated". Relative entries resolve against root. Entries that +// do not exist are dropped; an unreadable or absent forge.yml yields none. +func loadRelatedRepos(root string) []string { + if root == "" { + return nil + } + data, err := os.ReadFile(filepath.Join(root, "forge.yml")) + if err != nil { + return nil + } + var doc struct { + RelatedRepos []string `yaml:"related_repos"` + } + if err := yaml.Unmarshal(data, &doc); err != nil { + return nil + } + var out []string + for _, r := range doc.RelatedRepos { + r = strings.TrimSpace(r) + if r == "" { + continue + } + if !filepath.IsAbs(r) { + r = filepath.Join(root, filepath.FromSlash(r)) + } + r = filepath.Clean(r) + if fi, statErr := os.Stat(r); statErr == nil && fi.IsDir() { + out = append(out, r) + } + } + return out +} + +// pathExistsInRepos reports whether the repo-relative candidate exists under +// root or any related repo. A candidate may also be prefixed with a related +// repo's directory name ("ai-agent-system/src/x.py"), the way a spec cites a +// file in a sibling repo, in which case the prefix is stripped for that repo. +func pathExistsInRepos(root string, related []string, candidate string) bool { + rel := filepath.FromSlash(candidate) + if _, err := os.Stat(filepath.Join(root, rel)); err == nil { + return true + } + for _, repo := range related { + if _, err := os.Stat(filepath.Join(repo, rel)); err == nil { + return true + } + prefix := filepath.Base(repo) + "/" + if strings.HasPrefix(candidate, prefix) { + stripped := filepath.FromSlash(strings.TrimPrefix(candidate, prefix)) + if _, err := os.Stat(filepath.Join(repo, stripped)); err == nil { + return true + } + } + } + return false +} + // findUnverifiedFileReferences scans generated Markdown for backtick-wrapped // file-path-shaped references and returns the ones that do not exist on disk // under root, deduplicated and in first-seen order. @@ -259,6 +328,7 @@ func findUnverifiedFileReferences(root, content string) []string { } seen := make(map[string]bool) var unverified []string + related := loadRelatedRepos(root) for _, m := range filePathRefPattern.FindAllStringSubmatch(content, -1) { candidate := m[1] if seen[candidate] { @@ -270,7 +340,7 @@ func findUnverifiedFileReferences(root, content string) []string { if strings.HasPrefix(candidate, ".forge/") { continue } - if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(candidate))); os.IsNotExist(err) { + if !pathExistsInRepos(root, related, candidate) { unverified = append(unverified, candidate) } } @@ -292,7 +362,7 @@ func appendUnverifiedPathsWarning(root, content string) string { if !strings.HasSuffix(content, "\n") { b.WriteString("\n") } - b.WriteString("\n---\n\n> **⚠ Unverified file references (automated check):** the following paths mentioned above were not found in this repository — verify before relying on them, they may be hallucinated:\n") + b.WriteString("\n---\n\n> **⚠ Unverified file references (automated check):** the following paths mentioned above were not found in this repository or its forge.yml related_repos — verify before relying on them, they may be hallucinated:\n") for _, p := range unverified { b.WriteString("> - `" + p + "`\n") } diff --git a/internal/cli/cmdship/artefact_validate_test.go b/internal/cli/cmdship/artefact_validate_test.go index f070f5b..d8a99b9 100644 --- a/internal/cli/cmdship/artefact_validate_test.go +++ b/internal/cli/cmdship/artefact_validate_test.go @@ -395,3 +395,75 @@ func mustWriteFile(t *testing.T, path, content string) { t.Fatal(err) } } + +// ── related_repos ──────────────────────────────────────────────────────────── + +// TestFindUnverifiedFileReferences_ResolvesRelatedRepos — a spec for one repo +// of a multi-repo system legitimately cites files that live in a sibling repo. +// Without forge.yml's related_repos those were flagged as hallucinated. +func TestFindUnverifiedFileReferences_ResolvesRelatedRepos(t *testing.T) { + t.Parallel() + parent := t.TempDir() + root := filepath.Join(parent, "web-app") + sibling := filepath.Join(parent, "agent-service") + for _, f := range []string{ + filepath.Join(root, "src", "here.ts"), + filepath.Join(sibling, "src", "queue", "handler.py"), + } { + if err := os.MkdirAll(filepath.Dir(f), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(f, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(root, "forge.yml"), + []byte("llm:\n provider: anthropic\nrelated_repos:\n - ../agent-service\n - ../does-not-exist\n"), 0o600); err != nil { + t.Fatal(err) + } + + content := "See `src/here.ts`, `src/queue/handler.py`, `agent-service/src/queue/handler.py` " + + "and the invented `src/queue/ghost.py`." + got := findUnverifiedFileReferences(root, content) + if len(got) != 1 || got[0] != "src/queue/ghost.py" { + t.Fatalf("only the genuinely missing path may be flagged, got %v", got) + } +} + +// Without related_repos the sibling-repo citation is still flagged — the check +// must not silently become more permissive. +func TestFindUnverifiedFileReferences_NoRelatedReposStillFlags(t *testing.T) { + t.Parallel() + parent := t.TempDir() + root := filepath.Join(parent, "web-app") + sibling := filepath.Join(parent, "agent-service") + for _, d := range []string{root, filepath.Join(sibling, "src")} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(sibling, "src", "handler.py"), []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + got := findUnverifiedFileReferences(root, "`agent-service/src/handler.py`") + if len(got) != 1 { + t.Fatalf("a sibling-repo path with no related_repos must still be flagged, got %v", got) + } +} + +func TestLoadRelatedRepos_BadOrMissingConfig(t *testing.T) { + t.Parallel() + if got := loadRelatedRepos(""); got != nil { + t.Errorf("empty root: %v", got) + } + root := t.TempDir() + if got := loadRelatedRepos(root); got != nil { + t.Errorf("no forge.yml: %v", got) + } + if err := os.WriteFile(filepath.Join(root, "forge.yml"), []byte(":\n - [unbalanced"), 0o600); err != nil { + t.Fatal(err) + } + if got := loadRelatedRepos(root); got != nil { + t.Errorf("malformed forge.yml must yield none, got %v", got) + } +} diff --git a/internal/cli/cmdship/artefacts_test.go b/internal/cli/cmdship/artefacts_test.go index 1ab4d64..7da2b52 100644 --- a/internal/cli/cmdship/artefacts_test.go +++ b/internal/cli/cmdship/artefacts_test.go @@ -15,11 +15,14 @@ package cmdship import ( + "encoding/json" "errors" + "fmt" "os" "path/filepath" "strings" "testing" + "time" "github.com/teragrid/forge/internal/errcode" "github.com/teragrid/forge/internal/llmprovider" @@ -203,7 +206,7 @@ func TestLearningLoop_AppendAndRead(t *testing.T) { appendFailure(root, "spec", "my-feature", "acceptance criteria missing") appendFailure(root, "spec", "my-feature", "second failure detail") - got := loadRecentFailures(root, "spec", 3) + got := loadRecentFailures(root, "spec", "my-feature", 3) if got == "" { t.Fatal("loadRecentFailures returned empty string after appendFailure") } @@ -218,7 +221,7 @@ func TestLearningLoop_AppendAndRead(t *testing.T) { func TestLearningLoop_NoFileReturnsEmpty(t *testing.T) { t.Parallel() root := t.TempDir() - got := loadRecentFailures(root, "spec", 3) + got := loadRecentFailures(root, "spec", "my-feature", 3) if got != "" { t.Errorf("expected empty string when no failure file exists, got %q", got) } @@ -228,14 +231,86 @@ func TestLearningLoop_RespectsLimit(t *testing.T) { t.Parallel() root := t.TempDir() + // Distinct details: identical records are collapsed to one (see + // TestLearningLoop_CollapsesIdenticalRecords). for i := 0; i < 5; i++ { - appendFailure(root, "test", "feature-x", "failure") + appendFailure(root, "test", "feature-x", fmt.Sprintf("failure %d", i)) } // Request only 2 recent failures. - got := loadRecentFailures(root, "test", 2) + got := loadRecentFailures(root, "test", "feature-x", 2) // The header mentions "last 2". if !strings.Contains(got, "last 2") { t.Errorf("expected \"last 2\" in output, got:\n%s", got) } } + +// writeFailureRecords writes raw failure records with explicit timestamps. +func writeFailureRecords(t *testing.T, root, checkpoint string, recs []FailureRecord) { + t.Helper() + dir := filepath.Join(root, ".forge", "learned") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + var b strings.Builder + for _, r := range recs { + line, err := json.Marshal(r) + if err != nil { + t.Fatal(err) + } + b.Write(line) + b.WriteString("\n") + } + if err := os.WriteFile(filepath.Join(dir, checkpoint+"-failures.jsonl"), []byte(b.String()), 0o600); err != nil { + t.Fatal(err) + } +} + +// TestLearningLoop_StaleOtherFeatureFailuresAreDropped — a failure recorded for +// an unrelated feature days ago used to be injected into every spec prompt +// ("Recent spec failures: [2026-09-13] billing-past-due-visibility ..."). +func TestLearningLoop_StaleOtherFeatureFailuresAreDropped(t *testing.T) { + t.Parallel() + root := t.TempDir() + old := time.Now().UTC().Add(-9 * 24 * time.Hour).Format(time.RFC3339) + writeFailureRecords(t, root, "spec", []FailureRecord{ + {TS: old, Checkpoint: "spec", Feature: "billing-past-due-visibility", Detail: "truncated after retry"}, + }) + if got := loadRecentFailures(root, "spec", "prospect-finder", 3); got != "" { + t.Errorf("a 9-day-old failure for an unrelated feature must not reach the prompt, got:\n%s", got) + } +} + +// False-positive guards: the same feature's history is kept however old, and a +// fresh failure for another feature is still shown. +func TestLearningLoop_KeepsSameFeatureAndRecentOthers(t *testing.T) { + t.Parallel() + root := t.TempDir() + old := time.Now().UTC().Add(-30 * 24 * time.Hour).Format(time.RFC3339) + fresh := time.Now().UTC().Add(-1 * time.Hour).Format(time.RFC3339) + writeFailureRecords(t, root, "spec", []FailureRecord{ + {TS: old, Checkpoint: "spec", Feature: "Prospect Finder", Detail: "own old lesson"}, + {TS: fresh, Checkpoint: "spec", Feature: "other-feature", Detail: "fresh other lesson"}, + }) + got := loadRecentFailures(root, "spec", "prospect-finder", 3) + if !strings.Contains(got, "own old lesson") { + t.Errorf("same feature's history must be kept regardless of age (slug vs description forms):\n%s", got) + } + if !strings.Contains(got, "fresh other lesson") { + t.Errorf("a recent failure for another feature is still useful context:\n%s", got) + } +} + +// TestLearningLoop_CollapsesIdenticalRecords — the same failure recorded on +// every retry filled the whole window with copies of one line. +func TestLearningLoop_CollapsesIdenticalRecords(t *testing.T) { + t.Parallel() + root := t.TempDir() + for i := 0; i < 3; i++ { + appendFailure(root, "spec", "feat", "spec review truncated") + } + got := loadRecentFailures(root, "spec", "feat", 3) + if strings.Count(got, "spec review truncated") != 1 || !strings.Contains(got, "(last 1)") { + t.Errorf("identical records should be shown once:\n%s", got) + } +} diff --git a/internal/cli/cmdship/prompts_and_learning.go b/internal/cli/cmdship/prompts_and_learning.go index 5841962..9b2b2ca 100644 --- a/internal/cli/cmdship/prompts_and_learning.go +++ b/internal/cli/cmdship/prompts_and_learning.go @@ -123,9 +123,23 @@ func appendFailure(root, checkpoint, feature, detail string) { _, _ = f.WriteString("\n") } -// loadRecentFailures reads the last n failure records for a checkpoint. -// Returns a human-readable summary suitable for prepending to an LLM prompt. -func loadRecentFailures(root, checkpoint string, n int) string { +// failureContextMaxAge bounds how long a failure recorded for a DIFFERENT +// feature keeps being shown as context. Those records are infrastructure +// noise as often as lessons ("spec review truncated after retry"), and a +// nine-day-old one for an unrelated feature only spends prompt budget and +// invites a model to "avoid" something that has nothing to do with the task. +const failureContextMaxAge = 72 * time.Hour + +// loadRecentFailures reads up to n recent failure records for a checkpoint and +// returns a human-readable summary suitable for prepending to an LLM prompt. +// +// feature is the feature being worked on now (a slug or a description). +// Records for that feature are always eligible, however old — they are the +// directly relevant history. Records for other features are eligible only if +// newer than failureContextMaxAge. Identical records (same feature and +// detail) are shown once: a checkpoint that fails the same way on each retry +// otherwise fills the whole window with copies of one line. +func loadRecentFailures(root, checkpoint, feature string, n int) string { path := filepath.Join(root, ".forge", "learned", checkpoint+"-failures.jsonl") data, err := os.ReadFile(path) if err != nil { @@ -135,21 +149,40 @@ func loadRecentFailures(root, checkpoint string, n int) string { if len(lines) == 0 { return "" } - // Take the last n non-empty lines. + currentSlug := slugify(feature) + now := time.Now().UTC() + seen := make(map[string]bool) + // Take the last n eligible, distinct lines (newest first). var recent []string for i := len(lines) - 1; i >= 0 && len(recent) < n; i-- { if lines[i] == "" { continue } var rec FailureRecord - if err := json.Unmarshal([]byte(lines[i]), &rec); err == nil { - recent = append(recent, fmt.Sprintf(" [%s] %s: %s", rec.TS[:10], rec.Feature, rec.Detail)) + if err := json.Unmarshal([]byte(lines[i]), &rec); err != nil { + continue + } + sameFeature := currentSlug != "" && slugify(rec.Feature) == currentSlug + if !sameFeature { + if ts, tsErr := time.Parse(time.RFC3339, rec.TS); tsErr != nil || now.Sub(ts) > failureContextMaxAge { + continue + } + } + key := rec.Feature + "|" + rec.Detail + if seen[key] { + continue + } + seen[key] = true + day := rec.TS + if len(day) >= 10 { + day = day[:10] } + recent = append(recent, fmt.Sprintf(" [%s] %s: %s", day, rec.Feature, rec.Detail)) } if len(recent) == 0 { return "" } - out := fmt.Sprintf("\u26a0\ufe0f Recent %s failures (last %d):\n", checkpoint, len(recent)) + out := fmt.Sprintf("⚠️ Recent %s failures (last %d):\n", checkpoint, len(recent)) for _, r := range recent { out += r + "\n" } diff --git a/internal/cli/cmdship/ship.go b/internal/cli/cmdship/ship.go index 87026ba..0b2fdc1 100644 --- a/internal/cli/cmdship/ship.go +++ b/internal/cli/cmdship/ship.go @@ -144,6 +144,39 @@ func agentPauseCheckpoint(cp *Checkpoint, operation string, genErr error) bool { return true } +// untilCheckpoints returns the checkpoints to run for `--until order[pos]`. +// When names is empty the full pipeline is meant, so every checkpoint up to and +// including pos runs. When names is already narrowed (--from, --quick) it is +// intersected with that prefix so the flags compose instead of the later one +// silently winning. +func untilCheckpoints(names, order []string, pos int) []string { + allowed := make(map[string]bool, pos+1) + for _, cp := range order[:pos+1] { + allowed[cp] = true + } + src := names + if len(src) == 0 { + src = order + } + out := make([]string, 0, len(src)) + for _, n := range src { + if allowed[n] { + out = append(out, n) + } + } + return out +} + +// nextCheckpointAfter names the checkpoint that follows order[pos], for the +// "continue with" hint. It returns "qa-verify" at the end of the order, which +// is harmless: --until qa-verify has nothing left to continue to. +func nextCheckpointAfter(order []string, pos int) string { + if pos+1 < len(order) { + return order[pos+1] + } + return order[len(order)-1] +} + // ExitAgentTurn is the process exit code for a paused agent-mode run. // // 78 is chosen from the sysexits.h convention (EX_CONFIG, "configuration @@ -196,8 +229,13 @@ type AgentAction struct { } type ShipResult struct { - DryRun bool `json:"dry_run"` - Yolo bool `json:"yolo,omitempty"` + DryRun bool `json:"dry_run"` + Yolo bool `json:"yolo,omitempty"` + // AgentMode is true when the run was driven through the agent bridge. + // Agent mode has no stdin for y/N gates, so it sets Yolo internally; this + // field lets the header say so instead of announcing "YOLO" to a user who + // never passed --yolo. + AgentMode bool `json:"agent_mode,omitempty"` Interactive bool `json:"interactive,omitempty"` DebateEnabled bool `json:"debate_enabled,omitempty"` Checkpoints []Checkpoint `json:"checkpoints"` @@ -242,6 +280,7 @@ func init() { "--skip-checkpoint qa-verify (skip QA agent; useful when no test runner is configured)", "--dry-run (validate checkpoints without executing; default in MVP)", "--description (what this change does; required for full pipeline in M1)", + "--until (stop after the named checkpoint, e.g. --until arch to review the spec and ADR before test/breakdown/code)", "--yolo (skip all approval gates — activates 6-role self-debate for quality polishing)", "--json (machine-readable output; also disables interactive prompts)", "--no-branch (skip automatic feature-branch creation; work on the current branch)", @@ -290,6 +329,7 @@ func New() *cobra.Command { quick bool // --quick: lightweight spec+code only (skip test+breakdown+verify) yes bool // --yes: auto-approve all gates (alias for --yolo for non-YOLO users) from string // --from: resume from a named checkpoint + until string // --until: stop after a named checkpoint skipCheckpoint string // --skip-checkpoint: skip a named checkpoint pr bool // --pr: create a draft GitHub PR after all checkpoints pass resume bool // --resume: resume from first incomplete checkpoint (G-002) @@ -312,6 +352,8 @@ func New() *cobra.Command { c.Flags().BoolVarP(&quick, "quick", "Q", false, "lightweight run: spec+code only (skips test, breakdown, verify)") c.Flags().BoolVarP(&yes, "yes", "y", false, "auto-approve all checkpoint gates (alias for --yolo)") c.Flags().StringVarP(&from, "from", "f", "", "resume pipeline from this checkpoint (e.g. --from=code)") + c.Flags().StringVar(&until, "until", "", + "stop after this checkpoint (e.g. --until=arch runs spec and arch, then stops so you can review before test/breakdown/code)") c.Flags().StringVarP(&skipCheckpoint, "skip-checkpoint", "s", "", "skip a specific checkpoint by name") c.Flags().BoolVarP(&pr, "pr", "p", false, "create a draft GitHub PR after all checkpoints pass (requires gh CLI)") c.Flags().StringVarP(&rootDir, "root", "r", "", "project root (default: cwd)") @@ -395,6 +437,29 @@ func New() *cobra.Command { } } + // --until: keep only the checkpoints up to and including the named one. + // Before this flag a full-pipeline call could not be stopped for review + // between arch and test: every continuation ran on to the end, writing + // stubs for checkpoints nobody had asked for. + if until != "" { + order := []string{"spec", "arch", "test", "breakdown", "code", "ship", "qa-verify"} + pos := -1 + for i, cp := range order { + if cp == until { + pos = i + break + } + } + if pos < 0 { + return errcode.Newf(ErrShipFailed, nil, + "--until: unknown checkpoint %q; one of: spec, arch, test, breakdown, code, ship, qa-verify", until) + } + names = untilCheckpoints(names, order, pos) + fmt.Fprintf(cmd.ErrOrStderr(), + "note: --until %s — stopping after %s (repeat --until on every continuation of this run); to go on: forge ship --agent-mode --from %s\n", + until, until, nextCheckpointAfter(order, pos)) + } + // --skip-checkpoint: remove a named checkpoint from the run list. if skipCheckpoint != "" { if len(names) == 0 { @@ -530,6 +595,7 @@ func New() *cobra.Command { } res.Yolo = yolo + res.AgentMode = bridge != nil res.Interactive = gate != nil res.DebateEnabled = runOpts.DebateOpts != nil if branchRes.Branch != "" && branchRes.Warning == "" { @@ -837,7 +903,11 @@ func specYAMLContext(spec *cmdtest.TestSpec) string { func checkSpec(root, description, specName string, pipe *LLMPipe, dryRun bool) Checkpoint { cp := Checkpoint{Name: "Spec"} // G-011: surface recent spec failures as context for the LLM. - recentSpecFailures := loadRecentFailures(root, "spec", 3) + failureFeature := specName + if failureFeature == "" { + failureFeature = description + } + recentSpecFailures := loadRecentFailures(root, "spec", failureFeature, 3) specsDir := filepath.Join(root, ".forge", "specs") if description != "" || specName != "" { // Determine the spec directory name: --name/-n flag takes priority over the @@ -1234,10 +1304,23 @@ func checkTest(root, description, specName string, pipe *LLMPipe, dryRun bool) C if len(testFiles) > 0 { cp.Status = "ok" missing := missingTestArtifacts(root, slug) + // testFiles is every test in the repository. On a real project that is + // hundreds of files and says nothing about this feature — "671 test + // file(s) found" read like coverage while none of them belonged to it. + // Report the feature's own tests first, the repo total as context. + own := featureTestFiles(root, slug, testFiles) if len(missing) == 0 { - cp.Detail = fmt.Sprintf("%d test file(s) found; all 4 named artifacts present (tests/%s.*)", len(testFiles), slug) + cp.Detail = fmt.Sprintf("%d test file(s) for this feature (%d in repo); all %d named artifacts present", + len(own), len(testFiles), len(expectedTestArtifactNames(root, slug))) } else { - cp.Detail = fmt.Sprintf("%d test file(s) found; missing artifacts: %s", len(testFiles), strings.Join(missing, ", ")) + cp.Detail = fmt.Sprintf("%d test file(s) for this feature (%d in repo); forge-scaffolded artifacts not present: %s "+ + "(if this project keeps its tests elsewhere they may already exist under other names)", + len(own), len(testFiles), strings.Join(missing, ", ")) + // Nothing of the feature's own exists yet: existing tests for other + // features must not earn this checkpoint an unqualified ok. + if len(own) == 0 { + cp.Status = "warning" + } } applyReachability(root, artefactFilesForReachability(root, slug), &cp) if pipe != nil { @@ -1373,6 +1456,42 @@ func findTestFiles(root string) []string { return out } +// featureTestFiles narrows the repo-wide test file list to the ones that belong +// to slug: forge's own scaffolded artefacts, plus any test file whose path +// contains the slug (compared with separators stripped, so "add-rate-limit" +// matches add_rate_limit.test.ts and addRateLimit.spec.ts). +func featureTestFiles(root, slug string, all []string) []string { + norm := func(s string) string { + r := strings.NewReplacer("-", "", "_", "", ".", "", "/", "", "\\", "", " ", "") + return strings.ToLower(r.Replace(s)) + } + want := norm(slug) + seen := make(map[string]bool) + var out []string + add := func(p string) { + if !seen[p] { + seen[p] = true + out = append(out, p) + } + } + for _, p := range artefactFilesForReachability(root, slug) { + add(p) + } + if want == "" { + return out + } + for _, p := range all { + rel, err := filepath.Rel(root, p) + if err != nil { + rel = p + } + if strings.Contains(norm(filepath.ToSlash(rel)), want) { + add(p) + } + } + 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 @@ -1602,6 +1721,34 @@ func countChangedSourceFiles(root string) int { return n } +// branchSourceChangeCount counts source-code files changed on the current +// branch (relative to the default branch) plus uncommitted ones, using the same +// "does this plausibly belong to an implementation" filter as +// countChangedSourceFiles. known is false when git cannot answer — not a repo, +// or no default branch to compare against — and callers must then say nothing +// rather than claim there is nothing to ship. +func branchSourceChangeCount(root string) (n int, known bool) { + svc, err := gitservice.New(root) + if err != nil { + return 0, false + } + files, ok := svc.ChangedFilesOnBranch() + if !ok { + return 0, false + } + for _, f := range files { + p := filepath.ToSlash(f) + 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, true +} + // 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. @@ -1689,6 +1836,18 @@ func checkVerify(root, description, specName string, pipe *LLMPipe) Checkpoint { } cp.Status = "ok" + // Nothing to ship: the branch and working tree carry no source-code change. + // The scan and hygiene checks below still ran and are still reported, but + // "ok" plus a "spec-vs-code audit found no blocking gaps" line on a branch + // with no code reads as a clean bill of health for work that does not + // exist (seen on a real spec-only run: ship ✓ with zero code changes). + // Downgrade to a warning and withhold the audit claim. Unknown (no git, no + // default branch) keeps the old behaviour — silence, not a guess. + srcChanged, srcKnown := branchSourceChangeCount(root) + nothingToShip := srcKnown && srcChanged == 0 + if nothingToShip { + cp.Status = "warning" + } // M1: the ship checkpoint has no post-checkpoint gates to earn its green // from, so it records its own. Unlike most "ok" assignments in this file, // these are real observations — the scanner and the hygiene checker were @@ -1697,7 +1856,7 @@ func checkVerify(root, description, specName string, pipe *LLMPipe) Checkpoint { fmt.Sprintf("%d finding(s) total, %d high-confidence", len(scanRes.Findings), len(highFindings))) cp.AddEvidence(SourceExternalTool, "hygiene check found no unmanaged files", fmt.Sprintf("%d manifest pattern(s)", patternCount)) - if auditRes.SpecFound { + if auditRes.SpecFound && !nothingToShip { cp.AddEvidence(SourceReadBack, "spec-vs-code audit found no blocking gaps", fmt.Sprintf("%d warning-level gap(s)", len(auditRes.Gaps))) } @@ -1712,6 +1871,10 @@ func checkVerify(root, description, specName string, pipe *LLMPipe) Checkpoint { // Warning-only gaps — note them but don't fail. cp.Detail += fmt.Sprintf("; %d spec audit warning(s)", len(auditRes.Gaps)) } + if nothingToShip { + cp.Detail = "nothing to ship: no source-code changes on this branch or in the working tree " + + "(docs/, growth/ and .forge/ do not count) — " + cp.Detail + } return cp } @@ -2715,7 +2878,13 @@ func renderText(cmd *cobra.Command, r *ShipResult) { if r.DryRun { mode = " --dry-run" } - if r.Yolo { + switch { + case r.AgentMode: + // Not "YOLO": the user did not ask to skip review. Agent mode simply + // has no terminal for y/N prompts — the host agent answers each turn. + // The only way to get a review stop is --until . + mode += " [agent-mode — no interactive approval prompts; use --until to stop for review]" + case r.Yolo: mode += " [YOLO — approval gates disabled]" } fmt.Fprintf(w, "forge ship%s\n", mode) diff --git a/internal/cli/cmdship/ship_nothing_to_ship_test.go b/internal/cli/cmdship/ship_nothing_to_ship_test.go new file mode 100644 index 0000000..a60a994 --- /dev/null +++ b/internal/cli/cmdship/ship_nothing_to_ship_test.go @@ -0,0 +1,114 @@ +// Copyright 2024 The Forge Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmdship + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// shipTestRepo builds a throwaway git repo on a feature branch off main. +func shipTestRepo(t *testing.T) string { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not found in PATH") + } + dir := t.TempDir() + git := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + git("init") + git("config", "user.email", "test@forge.local") + git("config", "user.name", "Forge Test") + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("# repo\n"), 0o644); err != nil { + t.Fatal(err) + } + git("add", ".") + git("commit", "-m", "initial") + git("branch", "-M", "main") + git("checkout", "-b", "feature/x") + return dir +} + +// TestCheckVerify_NoSourceChanges_IsWarningNotOK is the regression guard for +// the ship checkpoint reporting "ok" — with a "spec-vs-code audit found no +// blocking gaps" evidence line — on a branch that changes no source code (a +// spec-only run). It must downgrade to a warning, say why, and withhold the +// audit claim. +func TestCheckVerify_NoSourceChanges_IsWarningNotOK(t *testing.T) { + t.Parallel() + root := shipTestRepo(t) + makeSpecDir(t, root, "feat", map[string]string{"spec.md": "# Spec\n"}) + + cp := checkVerify(root, "feat", "", nil) + if cp.Status != "warning" { + t.Fatalf("status = %q, want warning; detail: %s", cp.Status, cp.Detail) + } + if !strings.Contains(cp.Detail, "nothing to ship") { + t.Errorf("detail should say nothing to ship: %s", cp.Detail) + } + for _, ev := range cp.Evidence { + if strings.Contains(ev.Claim, "spec-vs-code audit") { + t.Errorf("must not claim a spec-vs-code audit passed when there is no code: %+v", ev) + } + } +} + +// TestCheckVerify_WithSourceChange_StaysOK is the false-positive guard: a +// branch that does carry source code must not be downgraded. +func TestCheckVerify_WithSourceChange_StaysOK(t *testing.T) { + t.Parallel() + root := shipTestRepo(t) + makeSpecDir(t, root, "feat", map[string]string{"spec.md": "# Spec\n"}) + if err := os.WriteFile(filepath.Join(root, "feature.go"), []byte("package feature\n"), 0o644); err != nil { + t.Fatal(err) + } + cmd := exec.Command("git", "add", "feature.go") + cmd.Dir = root + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git add: %v\n%s", err, out) + } + cmd = exec.Command("git", "commit", "-m", "add feature") + cmd.Dir = root + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git commit: %v\n%s", err, out) + } + + cp := checkVerify(root, "feat", "", nil) + if cp.Status != "ok" { + t.Fatalf("status = %q, want ok; detail: %s", cp.Status, cp.Detail) + } + if strings.Contains(cp.Detail, "nothing to ship") { + t.Errorf("must not claim nothing to ship: %s", cp.Detail) + } +} + +// TestCheckVerify_NotAGitRepo_KeepsOldBehaviour — when git cannot say what +// changed, forge must stay silent rather than guess "nothing to ship". +func TestCheckVerify_NotAGitRepo_KeepsOldBehaviour(t *testing.T) { + t.Parallel() + cp := checkVerify(t.TempDir(), "", "", nil) + if strings.Contains(cp.Detail, "nothing to ship") { + t.Errorf("unknown change set must not be reported as nothing to ship: %s", cp.Detail) + } +} diff --git a/internal/cli/cmdship/ship_test.go b/internal/cli/cmdship/ship_test.go index 4b95239..4b903d5 100644 --- a/internal/cli/cmdship/ship_test.go +++ b/internal/cli/cmdship/ship_test.go @@ -2617,3 +2617,115 @@ func TestAgentMode_ArchPauseDoesNotStubTheArtefact(t *testing.T) { t.Fatalf("never reached the arch generation turn within %d turns", maxTurns) } } + +// ── --until ────────────────────────────────────────────────────────────────── + +// TestUntilCheckpoints covers the prefix selection behind `forge ship --until`. +func TestUntilCheckpoints(t *testing.T) { + t.Parallel() + order := []string{"spec", "arch", "test", "breakdown", "code", "ship", "qa-verify"} + cases := []struct { + name string + names []string + pos int + want []string + }{ + {"full pipeline stops after arch", nil, 1, []string{"spec", "arch"}}, + {"until spec keeps only spec", nil, 0, []string{"spec"}}, + {"until last keeps everything", nil, 6, order}, + {"composes with --from", []string{"test", "breakdown", "code"}, 3, []string{"test", "breakdown"}}, + {"composes with --quick", []string{"spec", "code"}, 1, []string{"spec"}}, + {"from beyond until is empty", []string{"code", "ship"}, 1, []string{}}, + } + for _, tc := range cases { + got := untilCheckpoints(tc.names, order, tc.pos) + if strings.Join(got, ",") != strings.Join(tc.want, ",") { + t.Errorf("%s: got %v, want %v", tc.name, got, tc.want) + } + } + if got := nextCheckpointAfter(order, 1); got != "test" { + t.Errorf("nextCheckpointAfter(arch) = %q, want test", got) + } +} + +// TestShip_UntilStopsAfterNamedCheckpoint pins the user-visible behaviour: a +// full-pipeline call with --until arch reports spec and arch and nothing else, +// so a reviewer can stop between arch and test. It must not run — or write +// artefacts for — the later checkpoints. +func TestShip_UntilStopsAfterNamedCheckpoint(t *testing.T) { + t.Parallel() + root := t.TempDir() + cmd := New() + var out, errOut bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errOut) + cmd.SetArgs([]string{"add rate limiting", "--root", root, "--dry-run", "--json", "--until", "arch"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("execute: %v\n%s\n%s", err, out.String(), errOut.String()) + } + var res ShipResult + if err := json.Unmarshal(out.Bytes(), &res); err != nil { + t.Fatalf("decode result: %v\n%s", err, out.String()) + } + var names []string + for _, cp := range res.Checkpoints { + names = append(names, strings.ToLower(cp.Name)) + } + if strings.Join(names, ",") != "spec,arch" { + t.Errorf("checkpoints = %v, want [spec arch]", names) + } + if !strings.Contains(errOut.String(), "--until arch") { + t.Errorf("expected a note explaining the stop on stderr, got: %q", errOut.String()) + } + slugDir := filepath.Join(root, ".forge", "specs", "add-rate-limiting") + for _, f := range []string{"test.md", "breakdown.md", "code.md", "ship.md"} { + if _, err := os.Stat(filepath.Join(slugDir, f)); err == nil { + t.Errorf("%s exists — a checkpoint past --until ran", f) + } + } +} + +// TestShip_UntilRejectsUnknownCheckpoint — a typo must be an error, not a +// silent full run. +func TestShip_UntilRejectsUnknownCheckpoint(t *testing.T) { + t.Parallel() + cmd := New() + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SetArgs([]string{"add rate limiting", "--root", t.TempDir(), "--dry-run", "--until", "archh"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "--until: unknown checkpoint") { + t.Fatalf("expected an unknown-checkpoint error, got %v", err) + } +} + +// TestRenderText_AgentModeHeaderIsNotYOLO — agent mode sets Yolo internally +// because it has no stdin for y/N prompts. Printing "[YOLO — approval gates +// disabled]" then told a user who never passed --yolo that review had been +// switched off by them. The header must describe agent mode instead, and a +// genuine --yolo run must still say YOLO. +func TestRenderText_AgentModeHeaderIsNotYOLO(t *testing.T) { + t.Parallel() + render := func(r *ShipResult) string { + cmd := New() + var out bytes.Buffer + cmd.SetOut(&out) + renderText(cmd, r) + return out.String() + } + cps := []Checkpoint{{Name: "Spec", Status: "ok"}, {Name: "Arch", Status: "ok"}} + + agent := render(&ShipResult{Yolo: true, AgentMode: true, Checkpoints: cps}) + if strings.Contains(agent, "YOLO") { + t.Errorf("agent-mode header must not claim YOLO:\n%s", agent) + } + if !strings.Contains(agent, "agent-mode") || !strings.Contains(agent, "--until") { + t.Errorf("agent-mode header should say what happens and how to stop for review:\n%s", agent) + } + + yolo := render(&ShipResult{Yolo: true, Checkpoints: cps}) + if !strings.Contains(yolo, "YOLO") { + t.Errorf("a real --yolo run must still be labelled YOLO:\n%s", yolo) + } +} diff --git a/internal/cli/cmdship/test_feature_scope_test.go b/internal/cli/cmdship/test_feature_scope_test.go new file mode 100644 index 0000000..54c98d5 --- /dev/null +++ b/internal/cli/cmdship/test_feature_scope_test.go @@ -0,0 +1,110 @@ +// Copyright 2024 The Forge Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmdship + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func writeEmptyFile(t *testing.T, path string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("test('x', () => {});\n"), 0o600); err != nil { + t.Fatal(err) + } +} + +func TestFeatureTestFiles_MatchesSlugAcrossSeparators(t *testing.T) { + t.Parallel() + root := t.TempDir() + mine := []string{ + filepath.Join(root, "tests", "unit", "add_rate_limit.test.ts"), + filepath.Join(root, "src", "__tests__", "addRateLimit.spec.ts"), + filepath.Join(root, "tests", "add-rate-limit.integration.test.ts"), + } + others := []string{ + filepath.Join(root, "tests", "unit", "billing.test.ts"), + filepath.Join(root, "tests", "e2e", "login.spec.ts"), + } + all := append(append([]string{}, mine...), others...) + for _, p := range all { + writeEmptyFile(t, p) + } + + got := featureTestFiles(root, "add-rate-limit", all) + if len(got) != len(mine) { + t.Fatalf("got %d feature test files %v, want %d %v", len(got), got, len(mine), mine) + } + for _, o := range others { + for _, g := range got { + if g == o { + t.Errorf("unrelated test file leaked into the feature's set: %s", o) + } + } + } +} + +func TestFeatureTestFiles_EmptySlugMatchesNothingByName(t *testing.T) { + t.Parallel() + root := t.TempDir() + p := filepath.Join(root, "tests", "anything.test.ts") + writeEmptyFile(t, p) + if got := featureTestFiles(root, "", []string{p}); len(got) != 0 { + t.Fatalf("an empty slug must not match every file, got %v", got) + } +} + +// TestCheckTest_ReportsFeatureTestsNotRepoTotal is the regression guard for +// "671 test file(s) found; missing artifacts: ..." — the headline number was +// the whole repository's test count and said nothing about the feature, and a +// repo with hundreds of unrelated tests earned the checkpoint an unqualified +// ok while none of them belonged to it. +func TestCheckTest_ReportsFeatureTestsNotRepoTotal(t *testing.T) { + t.Parallel() + root := t.TempDir() + for _, n := range []string{"a", "b", "c", "d", "e"} { + writeEmptyFile(t, filepath.Join(root, "tests", "unit", n+".test.ts")) + } + cp := checkTest(root, "add rate limiting", "", nil, true) + if cp.Status == "ok" && strings.Contains(cp.Detail, "missing") { + t.Fatalf("no feature tests exist — status must not be an unqualified ok: %+v", cp) + } + if !strings.Contains(cp.Detail, "0 test file(s) for this feature (5 in repo)") { + t.Errorf("detail should separate the feature's tests from the repo total: %s", cp.Detail) + } + if cp.Status != "warning" { + t.Errorf("status = %q, want warning when the feature has no tests of its own", cp.Status) + } +} + +// False-positive guard: once the feature has its own tests the checkpoint is ok. +func TestCheckTest_FeatureHasOwnTests_StaysOK(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeEmptyFile(t, filepath.Join(root, "tests", "add-rate-limiting.test.ts")) + writeEmptyFile(t, filepath.Join(root, "tests", "other.test.ts")) + cp := checkTest(root, "add rate limiting", "", nil, true) + if cp.Status != "ok" { + t.Fatalf("status = %q, want ok; detail: %s", cp.Status, cp.Detail) + } + if !strings.Contains(cp.Detail, "1 test file(s) for this feature (2 in repo)") { + t.Errorf("unexpected detail: %s", cp.Detail) + } +} diff --git a/internal/cli/cmdship/workspace_context.go b/internal/cli/cmdship/workspace_context.go index d0dec26..35f2c4e 100644 --- a/internal/cli/cmdship/workspace_context.go +++ b/internal/cli/cmdship/workspace_context.go @@ -134,6 +134,21 @@ func collectWorkspaceContext(root, slug string) WorkspaceContextResult { sb.WriteString("\n") } + // 1b. Related repositories declared in forge.yml (related_repos). A change + // in a multi-repo system routinely spans them; the model should know they + // exist and what they are built with rather than guess. + if related := loadRelatedRepos(root); len(related) > 0 { + sb.WriteString("## Related Repositories\n") + for _, r := range related { + label := strings.Join(detectTechStack(r), ", ") + if label == "" { + label = "stack not detected" + } + sb.WriteString(fmt.Sprintf("- %s — %s\n", filepath.Base(r), label)) + } + sb.WriteString("\n") + } + // 2. Project overview from README.md — orient the LLM to the project's purpose. if overview := readProjectOverview(root); overview != "" { res.ProjectOverview = overview @@ -204,7 +219,7 @@ func collectWorkspaceContext(root, slug string) WorkspaceContextResult { } // 10. Existing feature specs — helps LLM avoid duplicating existing work. - if specs := listExistingSpecs(root); len(specs) > 0 { + if specs := listExistingSpecs(root, slug); len(specs) > 0 { sb.WriteString("## Existing Feature Specs (avoid duplicates)\n") sb.WriteString("- " + strings.Join(specs, ", ") + "\n\n") } @@ -241,10 +256,95 @@ func detectTechStack(root string) []string { if _, err := os.Stat(filepath.Join(root, ".github")); err == nil { found = append(found, "GitHub Actions CI") } + // Frameworks and platforms, not just the language runtime. A bare + // "Node.js" tells a model nothing about whether it is looking at Next.js + // + Supabase or an Express service, and a model told nothing invents + // infrastructure to fill the gap (Redis, DataDog, NextAuth.js on a project + // that uses none of them — the arch hallucination this snapshot exists to + // prevent). + found = append(found, detectFrameworkStack(root)...) + found = dedupeStrings(found) sort.Strings(found) return found } +// nodeFrameworkLabels maps an npm package name to the stack label it implies. +// Deliberately short: only packages whose presence changes what an +// architecture or spec should say. +var nodeFrameworkLabels = map[string]string{ + "next": "Next.js", + "react": "React", + "vue": "Vue", + "nuxt": "Nuxt", + "svelte": "Svelte", + "@angular/core": "Angular", + "express": "Express", + "fastify": "Fastify", + "@nestjs/core": "NestJS", + "typescript": "TypeScript", + "tailwindcss": "Tailwind CSS", + "@supabase/supabase-js": "Supabase client", + "@supabase/ssr": "Supabase client", + "prisma": "Prisma", + "drizzle-orm": "Drizzle ORM", + "stripe": "Stripe", + "jest": "Jest", + "vitest": "Vitest", + "@playwright/test": "Playwright", + "@modelcontextprotocol/sdk": "MCP SDK", +} + +// detectFrameworkStack derives framework/platform labels from package.json +// dependencies and from well-known directories. Deterministic, no LLM. +func detectFrameworkStack(root string) []string { + var out []string + if data, err := os.ReadFile(filepath.Join(root, "package.json")); err == nil { + var pkg struct { + Dependencies map[string]json.RawMessage `json:"dependencies"` + DevDependencies map[string]json.RawMessage `json:"devDependencies"` + } + if json.Unmarshal(data, &pkg) == nil { + for _, deps := range []map[string]json.RawMessage{pkg.Dependencies, pkg.DevDependencies} { + for name := range deps { + if label, ok := nodeFrameworkLabels[name]; ok { + out = append(out, label) + } + } + } + } + } + if fileExists(filepath.Join(root, "tsconfig.json")) { + out = append(out, "TypeScript") + } + if fileExists(filepath.Join(root, "supabase", "config.toml")) || + dirExists(filepath.Join(root, "supabase", "migrations")) { + out = append(out, "Supabase (Postgres migrations, RLS)") + } + if dirExists(filepath.Join(root, "supabase", "functions")) { + out = append(out, "Supabase Edge Functions (Deno)") + } + return out +} + +// dedupeStrings returns in with duplicates removed, first occurrence kept. +func dedupeStrings(in []string) []string { + seen := make(map[string]bool, len(in)) + out := make([]string, 0, len(in)) + for _, v := range in { + if !seen[v] { + seen[v] = true + out = append(out, v) + } + } + return out +} + +// dirExists reports whether path is an existing directory. +func dirExists(path string) bool { + fi, err := os.Stat(path) + return err == nil && fi.IsDir() +} + // readGoModSummary returns a short "module/path go X.Y" string from go.mod, // or empty string when go.mod is absent or unparseable. func readGoModSummary(root string) string { @@ -533,7 +633,7 @@ func recentGitLog(root string, n int) string { // listExistingSpecs returns the slugs of feature specs already in .forge/specs/. // The current slug is excluded (it is the feature being planned). -func listExistingSpecs(root string) []string { +func listExistingSpecs(root, currentSlug string) []string { specsDir := filepath.Join(root, ".forge", "specs") entries, err := os.ReadDir(specsDir) if err != nil { @@ -541,7 +641,12 @@ func listExistingSpecs(root string) []string { } var specs []string for _, e := range entries { - if e.IsDir() { + // The feature being planned is not an "existing" spec to avoid + // duplicating. This used to hold only by accident — while its + // directory did not exist yet — so on the arch and later checkpoints + // (spec.md already written) the feature was listed as a duplicate of + // itself. + if e.IsDir() && e.Name() != currentSlug { specs = append(specs, e.Name()) } } diff --git a/internal/cli/cmdship/workspace_context_test.go b/internal/cli/cmdship/workspace_context_test.go index f99c04a..a24556f 100644 --- a/internal/cli/cmdship/workspace_context_test.go +++ b/internal/cli/cmdship/workspace_context_test.go @@ -531,3 +531,88 @@ func writeFile(t *testing.T, dir, name, content string) { t.Fatalf("write %s: %v", name, err) } } + +// ── framework / platform detection ─────────────────────────────────────────── + +// TestDetectTechStack_NextSupabaseTypeScript pins the regression behind the +// arch hallucinations: a Next.js + Supabase + TypeScript project was described +// to the model as just "GitHub Actions CI, Node.js", so it invented the +// infrastructure it could not see. +func TestDetectTechStack_NextSupabaseTypeScript(t *testing.T) { + root := t.TempDir() + writeFile(t, root, "package.json", + `{"dependencies":{"next":"15.0.0","react":"19.0.0","@supabase/supabase-js":"2.0.0","stripe":"1.0.0"},`+ + `"devDependencies":{"typescript":"5.0.0","jest":"29.0.0"}}`) + writeFile(t, root, "tsconfig.json", "{}") + if err := os.MkdirAll(filepath.Join(root, "supabase", "migrations"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(root, "supabase", "functions"), 0o755); err != nil { + t.Fatal(err) + } + + stack := strings.Join(detectTechStack(root), " | ") + for _, want := range []string{ + "Node.js", "Next.js", "React", "Supabase client", "Stripe", "TypeScript", "Jest", + "Supabase (Postgres migrations, RLS)", "Supabase Edge Functions (Deno)", + } { + if !strings.Contains(stack, want) { + t.Errorf("stack %q missing %q", stack, want) + } + } + // TypeScript comes from both package.json and tsconfig.json — listed once. + if strings.Count(stack, "TypeScript") != 1 { + t.Errorf("TypeScript must be listed once, got: %s", stack) + } +} + +// False-positive guard: a plain Node project must not gain frameworks it does +// not use, and a malformed package.json must not break detection. +func TestDetectTechStack_PlainNodeAndMalformedPackageJSON(t *testing.T) { + root := t.TempDir() + writeFile(t, root, "package.json", `{"dependencies":{"left-pad":"1.0.0"}}`) + stack := strings.Join(detectTechStack(root), " | ") + if strings.Contains(stack, "Next.js") || strings.Contains(stack, "Supabase") { + t.Errorf("frameworks reported for a project that uses none: %s", stack) + } + + bad := t.TempDir() + writeFile(t, bad, "package.json", "{not json") + if got := detectTechStack(bad); len(got) != 1 || got[0] != "Node.js" { + t.Errorf("malformed package.json should still yield just Node.js, got %v", got) + } +} + +func TestCollectWorkspaceContext_ListsRelatedRepos(t *testing.T) { + parent := t.TempDir() + root := filepath.Join(parent, "web") + agent := filepath.Join(parent, "agent") + for _, d := range []string{root, agent} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + writeFile(t, agent, "requirements.txt", "fastapi\n") + writeFile(t, root, "forge.yml", "related_repos:\n - ../agent\n") + + res := collectWorkspaceContext(root, "feat") + if !strings.Contains(res.Content, "## Related Repositories") || + !strings.Contains(res.Content, "- agent — Python (requirements.txt)") { + t.Errorf("related repo and its stack should be in the snapshot:\n%s", res.Content) + } +} + +// The feature being planned is not an existing spec to avoid duplicating — +// including once its own directory exists (arch and later checkpoints). +func TestListExistingSpecs_ExcludesCurrentSlugEvenWhenDirExists(t *testing.T) { + root := t.TempDir() + for _, d := range []string{"old-feature", "this-feature"} { + if err := os.MkdirAll(filepath.Join(root, ".forge", "specs", d), 0o755); err != nil { + t.Fatal(err) + } + } + got := listExistingSpecs(root, "this-feature") + if len(got) != 1 || got[0] != "old-feature" { + t.Fatalf("got %v, want [old-feature]", got) + } +} diff --git a/internal/gitservice/gitservice.go b/internal/gitservice/gitservice.go index 9e304c7..f8d425f 100644 --- a/internal/gitservice/gitservice.go +++ b/internal/gitservice/gitservice.go @@ -177,6 +177,53 @@ func (s *Service) ChangedFilesSince(ref string) ([]string, error) { return files, nil } +// baseRefCandidates are tried in order by ChangedFilesOnBranch. Remote-tracking +// refs come first: a local main that lags origin would understate the branch's +// own work, and one that is ahead of origin would overstate it. +var baseRefCandidates = []string{"origin/main", "origin/master", "main", "master"} + +// ChangedFilesOnBranch returns the files changed on the current branch relative +// to the repository's default branch (merge-base diff, so commits already on +// the default branch are not counted), plus any uncommitted paths. +// +// ok is false when no base ref can be resolved — a repo with no main/master, a +// detached HEAD on the base itself, or git failing. Callers must treat that as +// "unknown", never as "nothing changed": reporting a fact forge could not +// establish is the false-green this exists to prevent. +func (s *Service) ChangedFilesOnBranch() (files []string, ok bool) { + seen := make(map[string]bool) + add := func(f string) { + f = strings.TrimSpace(f) + if f != "" && !seen[f] { + seen[f] = true + files = append(files, f) + } + } + for _, base := range baseRefCandidates { + if _, err := s.run("rev-parse", "--verify", "--quiet", base+"^{commit}"); err != nil { + continue + } + out, err := s.run("diff", "--name-only", base+"...HEAD") + if err != nil { + continue + } + for _, line := range strings.Split(out, "\n") { + add(line) + } + ok = true + break + } + if !ok { + return nil, false + } + if statuses, err := s.Status(); err == nil { + for _, st := range statuses { + add(st.Path) + } + } + return files, true +} + // GoFileCommitTimes returns the last-commit timestamp for each Go source file // that has been committed to this repository. Map keys are repo-relative paths // with forward slashes. Files with no commits (new/untracked) are absent. diff --git a/internal/gitservice/gitservice_test.go b/internal/gitservice/gitservice_test.go index 77fd2c9..e6d711b 100644 --- a/internal/gitservice/gitservice_test.go +++ b/internal/gitservice/gitservice_test.go @@ -295,3 +295,91 @@ func TestGoFileCommitTimes_TracksGoFiles(t *testing.T) { times["main.go"], times["main_test.go"]) } } + +// ── ChangedFilesOnBranch ────────────────────────────────────────────────────── + +// gitIn runs a git command in dir and fails the test on error. +func gitIn(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } +} + +func commitFile(t *testing.T, dir, name, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + gitIn(t, dir, "add", name) + gitIn(t, dir, "commit", "-m", "add "+name) +} + +// A repo with no main/master/origin ref: forge cannot say what "this branch's +// changes" are, and must say so (ok=false) rather than report zero. +func TestChangedFilesOnBranch_NoBaseRefIsUnknown(t *testing.T) { + skipIfNoGit(t) + dir := initRepo(t) + gitIn(t, dir, "branch", "-M", "trunk") + svc, err := gitservice.New(dir) + if err != nil { + t.Fatal(err) + } + files, ok := svc.ChangedFilesOnBranch() + if ok || len(files) != 0 { + t.Fatalf("no base ref must be unknown, got ok=%v files=%v", ok, files) + } +} + +func TestChangedFilesOnBranch_CleanBranchIsKnownAndEmpty(t *testing.T) { + skipIfNoGit(t) + dir := initRepo(t) + gitIn(t, dir, "branch", "-M", "main") + gitIn(t, dir, "checkout", "-b", "feature/x") + svc, _ := gitservice.New(dir) + files, ok := svc.ChangedFilesOnBranch() + if !ok { + t.Fatal("main exists, result must be known") + } + if len(files) != 0 { + t.Fatalf("clean feature branch must report no changes, got %v", files) + } +} + +// Committed work, uncommitted work, and — the false-positive guard — commits +// that landed on main after the branch point must NOT be counted. +func TestChangedFilesOnBranch_CommittedUncommittedAndBaseDrift(t *testing.T) { + skipIfNoGit(t) + dir := initRepo(t) + gitIn(t, dir, "branch", "-M", "main") + gitIn(t, dir, "checkout", "-b", "feature/x") + commitFile(t, dir, "feature.go", "package x\n") + if err := os.WriteFile(filepath.Join(dir, "wip.ts"), []byte("export {}\n"), 0o644); err != nil { + t.Fatal(err) + } + // main moves on after the branch point. + gitIn(t, dir, "checkout", "main") + commitFile(t, dir, "upstream.go", "package up\n") + gitIn(t, dir, "checkout", "feature/x") + + svc, _ := gitservice.New(dir) + files, ok := svc.ChangedFilesOnBranch() + if !ok { + t.Fatal("expected a known result") + } + got := map[string]bool{} + for _, f := range files { + got[f] = true + } + if !got["feature.go"] { + t.Errorf("committed branch file missing: %v", files) + } + if !got["wip.ts"] { + t.Errorf("uncommitted file missing: %v", files) + } + if got["upstream.go"] { + t.Errorf("a commit that landed on main after the branch point was counted as branch work: %v", files) + } +} From dd816413b1ee8b83243d9fa62073dd6f24710fde Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Mon, 21 Sep 2026 06:55:38 +0700 Subject: [PATCH 2/2] fix(hooks): stop pre-push and gitservice inheriting git's exported GIT_DIR git exports GIT_DIR (and related variables) to hooks. The pre-push gate runs `go test ./...`, and every test or tool that shells out to git in a temp dir (`git init`, `commit`, `branch -M`, `checkout -b`) therefore operated on the repository being pushed: it committed fixtures onto the branch, force-renamed branches (overwriting `main`) and rewrote .git/config (core.bare, user.name). Seen on a real push from a linked worktree. - .githooks/pre-push unsets the repo-pointing variables so every stage, and everything it spawns, resolves the repository from the working directory. - gitservice no longer passes them to the git processes it starts: a Service is opened on an explicit root and must answer for that root, not for whichever repository the parent environment names. Regression test verified to fail without the change. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Nguyen Quang Trung --- .githooks/pre-push | 13 +++++++++++ internal/gitservice/gitservice.go | 32 ++++++++++++++++++++++++++ internal/gitservice/gitservice_test.go | 27 ++++++++++++++++++++++ 3 files changed, 72 insertions(+) diff --git a/.githooks/pre-push b/.githooks/pre-push index 1b4aa5f..137c9e5 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -50,6 +50,19 @@ set -euo pipefail +# ── isolate from git's own hook environment ───────────────────────────────── +# git exports GIT_DIR (and, depending on version and worktree layout, +# GIT_WORK_TREE / GIT_INDEX_FILE / GIT_COMMON_DIR / ...) to hooks. Every child +# process below inherits them, so ANY test that shells out to git in a temp dir +# (`git init`, `git commit`, `git branch -M`, `git checkout -b`, ...) silently +# operates on THIS repository instead: it commits test fixtures onto the +# branch being pushed, force-renames branches (clobbering `main`), and rewrites +# .git/config (core.bare, user.name/email). Seen on a real push from a linked +# worktree. Cleared here so every stage — and everything they spawn — resolves +# the repository from the working directory, as it does outside a hook. +unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_PREFIX GIT_COMMON_DIR \ + GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_NAMESPACE + # ── emergency bypass ──────────────────────────────────────────────────────── if [[ "${SKIP_PRE_PUSH:-}" == "1" ]]; then echo "⚠ SKIP_PRE_PUSH=1: all pre-push checks skipped — use only in emergencies" diff --git a/internal/gitservice/gitservice.go b/internal/gitservice/gitservice.go index f8d425f..e128636 100644 --- a/internal/gitservice/gitservice.go +++ b/internal/gitservice/gitservice.go @@ -26,6 +26,7 @@ import ( "bytes" "errors" "fmt" + "os" "os/exec" "path/filepath" "strconv" @@ -259,6 +260,7 @@ func (s *Service) GoFileCommitTimes() map[string]time.Time { func (s *Service) run(args ...string) (string, error) { cmd := exec.Command("git", args...) //nolint:gosec // args are caller-controlled cmd.Dir = s.root + cmd.Env = scrubbedGitEnv(os.Environ()) var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr @@ -269,3 +271,33 @@ func (s *Service) run(args ...string) (string, error) { } return stdout.String(), nil } + +// repoPointingEnv are the variables git exports to hooks (and honours from any +// parent) that redirect it to a specific repository, index or object store. +var repoPointingEnv = []string{ + "GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE", "GIT_PREFIX", "GIT_COMMON_DIR", + "GIT_OBJECT_DIRECTORY", "GIT_ALTERNATE_OBJECT_DIRECTORIES", "GIT_NAMESPACE", +} + +// scrubbedGitEnv returns env without the variables that would make git ignore +// the directory the Service was opened on. A Service is created for an explicit +// root; when forge runs inside a git hook (or under `git rebase --exec`) the +// inherited GIT_DIR would otherwise silently point every command at the hook's +// repository instead — reporting that repository's status and history for the +// wrong directory. +func scrubbedGitEnv(env []string) []string { + out := make([]string, 0, len(env)) + for _, kv := range env { + drop := false + for _, name := range repoPointingEnv { + if strings.HasPrefix(kv, name+"=") { + drop = true + break + } + } + if !drop { + out = append(out, kv) + } + } + return out +} diff --git a/internal/gitservice/gitservice_test.go b/internal/gitservice/gitservice_test.go index e6d711b..4fc70e1 100644 --- a/internal/gitservice/gitservice_test.go +++ b/internal/gitservice/gitservice_test.go @@ -383,3 +383,30 @@ func TestChangedFilesOnBranch_CommittedUncommittedAndBaseDrift(t *testing.T) { t.Errorf("a commit that landed on main after the branch point was counted as branch work: %v", files) } } + +// TestService_IgnoresInheritedGitDir is the regression guard for the incident +// where a test run from git's pre-push hook (which exports GIT_DIR) operated on +// the real repository instead of the directory the Service was opened on: +// commits, a force-renamed main and a rewritten .git/config landed in the wrong +// repo. The Service must answer for its own root whatever GIT_DIR says. +func TestService_IgnoresInheritedGitDir(t *testing.T) { + skipIfNoGit(t) + mine := initRepo(t) + decoy := initRepo(t) + commitFile(t, decoy, "decoy.go", "package decoy\n") + + t.Setenv("GIT_DIR", filepath.Join(decoy, ".git")) + t.Setenv("GIT_WORK_TREE", decoy) + + svc, err := gitservice.New(mine) + if err != nil { + t.Fatal(err) + } + commits, err := svc.Log(5) + if err != nil { + t.Fatal(err) + } + if len(commits) != 1 || !strings.Contains(commits[0].Subject, "initial commit") { + t.Fatalf("Service read the decoy repo via GIT_DIR instead of its own root: %+v", commits) + } +}