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/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..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" @@ -177,6 +178,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. @@ -212,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 @@ -222,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 77fd2c9..4fc70e1 100644 --- a/internal/gitservice/gitservice_test.go +++ b/internal/gitservice/gitservice_test.go @@ -295,3 +295,118 @@ 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) + } +} + +// 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) + } +}