diff --git a/CHANGELOG.md b/CHANGELOG.md index 9112c94..8515f62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to forge will be documented in this file. Format follows [Ke ## [Unreleased] -## [1.10.3] — 2026-08-26 — Agent mode stopped fabricating Test/Code success behind a real pause +## [1.10.3] — 2026-09-05 — Agent mode: fabrication, wrong-feature resume, concurrency, and dry-run leaks fixed ### Fixed @@ -14,6 +14,10 @@ All notable changes to forge will be documented in this file. Format follows [Ke - **The managed `.gitignore` block never listed forge's own agent-mode/scratch state**, so `.forge/agent/` (bridge session/pending/response files), `.forge/.snapshots/`, `.forge/learned/`, `.forge/trash/`, and `.forge/token-ledger.jsonl` all showed up as untracked in `git status`. Confirmed live: an agent-mode run left `git status --porcelain` reporting the bridge's own bookkeeping as changes, which the Code checkpoint's `countChangedFiles` then counted as evidence of real code changes — "N modified file(s)" for a run where the only real source file was untouched. Fixed in all three copies of the managed block (`codemod.canonicalGitignoreBlock`, `codemod.defaultMarkerBody`, `cmddoctor.canonicalGiSnippet`). - **CI: the `M1-27` tests-precede-code gate flagged any test-only PR touching a mature file as a TDD violation.** It compared each file's all-time-latest-touch commit timestamp across the entire repo history rather than this PR's own commits — true for nearly every established file the moment its test gets a maintenance update with no accompanying production change. Both `git log` calls are now scoped to `origin/$BASE..HEAD` and take the oldest commit in that range, so an untouched production file correctly produces no commits (skipped) while a genuine same-PR "production code first, test added later" violation is still caught. - **CI: the `M2-17` perf benchmark gate's `benchstat` install failed on `ci-gates.yml`'s Go 1.25 pin** once `golang.org/x/perf@latest` started requiring Go ≥ 1.26. `ci.yml` and `nightly.yml` were already on Go 1.26, matching `go.mod`'s `toolchain go1.26.6` (bumped 2026-08-20 for stdlib CVE fixes) — `ci-gates.yml` alone had drifted. Bumped to match. +- **`forge ship` hard-failed every LLM checkpoint when the configured provider was unusable (e.g. Anthropic credit balance too low) instead of falling back to `--agent-mode`.** `--agent-mode` was already the documented escape hatch for exactly this — "drive the pipeline from your own AI chat instead of an API key" — but it required the operator to notice the failure and manually re-invoke with the flag. `forge ship` now runs a cheap live probe (mirroring `forge doctor --llm`'s `checkLLMProviderLive`) before starting a non-agent-mode run: when a provider *is* configured but a minimal completion call against it fails for a permanent reason (an invalid/expired key, or a hard `invalid_request_error` — Anthropic's shape for "credit balance too low"), the run automatically switches to agent mode and prints a note explaining why, instead of proceeding to fail every checkpoint one at a time. No provider configured at all is left untouched — that already has its own stub/hint UX and is not this failure. Set `FORGE_NO_AGENT_FALLBACK=1` to keep the old hard-failure behavior (e.g. for CI jobs that should error rather than pause on a host-agent turn). +- **CRITICAL: `forge ship --agent-mode` without `--name`/description could silently resume a different, unrelated feature's pending checkpoint.** `Bridge.SetFeature` unconditionally wrote its `feature`/`slug` arguments into the session on every call, including the bare continuation forge itself tells you to run after a submit (`next: forge ship --agent-mode`, no flags). That call passed `SetFeature("", "")`, which blanked the session's previously-recorded feature identity — so the next resolution of "which spec is this session driving" had nothing to resume against and could fall through to an unrelated feature's incomplete checkpoint instead, corrupting its pipeline state with the wrong artefact if the answer were submitted. `SetFeature("", "")` is now a no-op when the session already has an identity, and the bare-continuation path in `forge ship --agent-mode` resolves the missing `--name`/description from the session's own recorded `Feature()` before doing anything else, so a bare re-run of forge's own printed hint now reliably continues the same feature. +- **CRITICAL: `forge ship --agent-mode` could hang for minutes with zero output mid-architecture-debate, or panic with a "concurrent map" error.** `checkArch`'s parallel role debate (`runParallelArchDebate`) fires one goroutine per reviewer role — six by default — and every one calls `LLMPipe.Invoke`, which in agent mode resolves through `Bridge.Lookup`. `Bridge` was documented "not safe for concurrent use" but nothing enforced that: six goroutines mutating the same `seen`/`byHash`/`byOrdinal` maps and `pending`/`paused` fields with no synchronization is a data race the Go runtime can surface as an outright panic, or — plausibly what was actually observed — as a hang, if the race corrupts a map's internal structure rather than tripping the concurrent-access detector cleanly. `Bridge` now serializes every exported method that touches shared state behind a mutex; a new regression test (`TestLookup_ConcurrentCallsAreSafe`) reproduces the exact six-goroutines-one-operation shape and is checked under `go test -race` in the nightly workflow. +- **`--dry-run` made real LLM calls and wrote files to disk despite its own help text ("preview what would happen without making LLM calls or git operations").** `newLLMPipeInteractive`'s dry-run branch called `newLLMPipe(root)`, which returns a live, billable pipe whenever a provider is configured — its own doc comment already (incorrectly) claimed dry-run "falls back to nil silently" while the code did the opposite. Separately, `checkSpec` and `checkArch` had no `dryRun` parameter at all: a preview of a not-yet-generated feature unconditionally created `.forge/specs//`, wrote `workspace-context.md`, and wrote a `spec.md`/`arch.md` stub (or the real LLM output, if a working key was configured) — exactly the stray-directory behavior observed running exploratory `--dry-run` probes. Dry-run now always gets a nil pipe (no live provider is ever dialled), and `checkSpec`/`checkArch` report what they *would* generate without touching disk when the target artefact doesn't already exist yet. ## [1.10.2] — 2026-08-21 — Agent mode stopped pausing: a bridge miss was treated as an LLM failure diff --git a/internal/agentbridge/agentbridge.go b/internal/agentbridge/agentbridge.go index 088b4cc..bcc4a14 100644 --- a/internal/agentbridge/agentbridge.go +++ b/internal/agentbridge/agentbridge.go @@ -86,6 +86,7 @@ import ( "path/filepath" "sort" "strings" + "sync" "time" ) @@ -147,9 +148,28 @@ type Stats struct { Drifted int `json:"drifted"` } -// Bridge is the plane boundary. It is not safe for concurrent use; forge ship -// drives it from a single goroutine. +// Bridge is the plane boundary. +// +// Its exported methods that touch shared state (Lookup, Fulfil, Reset, +// Stats, Pending, Paused, SetFeature, Feature) are safe for concurrent use — +// each takes mu for its duration. This was not always true: checkArch's +// runParallelArchDebate (arch.go) fires one goroutine per reviewer role and +// every one calls Lookup through LLMPipe.invokeViaBridge in agent mode, all +// against the same Bridge. Before mu existed, that meant unsynchronized +// concurrent writes to the seen/byHash/byOrdinal maps and the +// pending/paused/drifted/nextSeq fields — a data race the Go runtime can +// surface as a panic ("concurrent map writes") or, depending on timing, as +// what ISSUE 5 in docs/plans/FORGE_SHIP_ISSUES_2026-09-04.md +// (ai-marketing-platfrom repo) observed: forge ship --agent-mode hanging +// with zero output for minutes mid-arch-debate — plausibly the runtime map +// implementation spinning on corrupted internal state rather than panicking +// cleanly. A single coarse-grained mutex is sufficient here: none of these +// methods are hot-path, and file I/O inside them (savePending, appendResponse, +// etc.) is not itself reentrant-locked, so serializing at the exported-method +// boundary is both correct and the simplest fix. type Bridge struct { + mu sync.Mutex + root string dir string session Session @@ -220,6 +240,21 @@ func (b *Bridge) Dir() string { return b.dir } // SessionName returns the active session name. func (b *Bridge) SessionName() string { return b.session.Name } +// Feature returns the feature description and slug this session last +// recorded via SetFeature, or ("", "") if none has been set yet. Callers use +// this to resume a bare `forge ship --agent-mode` (no description/--name) +// against the same feature the session was already driving, instead of +// leaving the run to fall back to some other, unrelated resolution of "which +// spec to operate on" — see the ISSUE 4 fix at the ship.go call site. +func (b *Bridge) Feature() (feature, slug string) { + if b == nil { + return "", "" + } + b.mu.Lock() + defer b.mu.Unlock() + return b.session.Feature, b.session.Slug +} + // SetFeature records what feature this session is driving. Best-effort: a // persistence failure never blocks the pipeline. // @@ -234,15 +269,30 @@ func (b *Bridge) SessionName() string { return b.session.Name } // feature's stale artefact under the new feature's name. Reusing the default // session across unrelated features (rather than passing --session per // feature) is exactly the case this guards. +// +// Calling SetFeature("", "") — a bare continuation with no description and no +// --name, e.g. re-running the hint `forge ship --agent-mode` printed after a +// submit — is a no-op when the session already has a recorded identity: it +// neither resets nor overwrites it. Before this guard, the unconditional +// write below blanked session.Feature/Slug on every bare call, so the very +// next lookup of "which feature is this session driving" (ship.go's +// resolution of an empty --name/description, ISSUE 4) had nothing to resume +// against and fell through to picking an unrelated feature instead. func (b *Bridge) SetFeature(feature, slug string) (switched bool) { if b == nil { return false } + b.mu.Lock() + defer b.mu.Unlock() prevSlug, prevFeature := b.session.Slug, b.session.Feature hadPrior := prevSlug != "" || prevFeature != "" newIdentity := slug != "" || feature != "" + if hadPrior && !newIdentity { + // Bare continuation: keep driving whatever feature was already set. + return false + } if hadPrior && newIdentity && prevSlug != slug && prevFeature != feature { - _ = b.Reset() + _ = b.resetLocked() switched = true } b.session.Feature = feature @@ -252,11 +302,23 @@ func (b *Bridge) SetFeature(feature, slug string) (switched bool) { } // Paused reports whether a turn has been requested during this process run. -func (b *Bridge) Paused() bool { return b != nil && b.paused } +func (b *Bridge) Paused() bool { + if b == nil { + return false + } + b.mu.Lock() + defer b.mu.Unlock() + return b.paused +} // Pending returns the turn awaiting a host-agent answer, if any. func (b *Bridge) Pending() (Turn, bool) { - if b == nil || b.pending == nil { + if b == nil { + return Turn{}, false + } + b.mu.Lock() + defer b.mu.Unlock() + if b.pending == nil { return Turn{}, false } return *b.pending, true @@ -272,6 +334,8 @@ func (b *Bridge) Lookup(operation, checkpoint, model, system, user string, maxTo if b == nil { return "", ErrTurnRequired } + b.mu.Lock() + defer b.mu.Unlock() hash := Hash(operation, system, user) // The ordinal is consumed even when the hash hits, so that the Nth call to // an operation keeps the same ordinal across runs regardless of which @@ -330,7 +394,12 @@ func (b *Bridge) Lookup(operation, checkpoint, model, system, user string, maxTo // Fulfil records the host agent's answer to the pending turn and clears it. // The recorded response is indexed under both keys so the next replay hits. func (b *Bridge) Fulfil(content string) (Turn, error) { - if b == nil || b.pending == nil { + if b == nil { + return Turn{}, ErrNoPendingTurn + } + b.mu.Lock() + defer b.mu.Unlock() + if b.pending == nil { return Turn{}, ErrNoPendingTurn } if strings.TrimSpace(content) == "" { @@ -367,6 +436,15 @@ func (b *Bridge) Reset() error { if b == nil { return nil } + b.mu.Lock() + defer b.mu.Unlock() + return b.resetLocked() +} + +// resetLocked is Reset's body, factored out so SetFeature — which already +// holds mu when it decides a feature switch requires a reset — can call it +// directly instead of re-entering the non-reentrant mutex via Reset itself. +func (b *Bridge) resetLocked() error { for _, name := range []string{pendingFile, responsesFile, sessionFile} { if err := os.Remove(filepath.Join(b.dir, name)); err != nil && !os.IsNotExist(err) { return fmt.Errorf("reset %s: %w", name, err) @@ -388,6 +466,8 @@ func (b *Bridge) Stats() Stats { if b == nil { return Stats{} } + b.mu.Lock() + defer b.mu.Unlock() s := Stats{Session: b.session, Responses: len(b.byHash), Drifted: b.drifted} if b.pending != nil { p := *b.pending diff --git a/internal/agentbridge/agentbridge_test.go b/internal/agentbridge/agentbridge_test.go index adcdc03..63f27f3 100644 --- a/internal/agentbridge/agentbridge_test.go +++ b/internal/agentbridge/agentbridge_test.go @@ -32,6 +32,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" ) @@ -486,6 +487,87 @@ func TestSetFeature_SwitchingFeatureResetsStaleSession(t *testing.T) { } } +// TestSetFeature_BareContinuationPreservesIdentity is a regression test for +// the FORGE_SHIP_ISSUES_2026-09-04.md ISSUE 4 root cause: a bare re-run of +// the hint forge itself prints after a submit — `forge ship --agent-mode`, +// with no --name and no description — called SetFeature("", "") on every +// continuation. The unconditional write at the end of SetFeature blanked the +// session's recorded Feature/Slug even though nothing about the call +// intended a switch, so the very next lookup of "what feature is this +// session driving" (the ship.go call site) found nothing to resume against. +func TestSetFeature_BareContinuationPreservesIdentity(t *testing.T) { + t.Parallel() + root := t.TempDir() + + b := mustOpen(t, root, DefaultSession) + if switched := b.SetFeature("blog inbound hub", "blog-inbound-hub"); switched { + t.Fatal("first SetFeature call on a fresh session must not report a switch") + } + + // Simulate the bare continuation call forge ship makes when neither + // --name nor a description was passed. + if switched := b.SetFeature("", ""); switched { + t.Fatal("a bare SetFeature(\"\", \"\") must never report a switch") + } + if feature, slug := b.Feature(); feature != "blog inbound hub" || slug != "blog-inbound-hub" { + t.Fatalf("bare continuation must preserve the session's prior identity, got feature=%q slug=%q", feature, slug) + } + + // A fresh process reopening the session must see the same preserved identity. + b2 := mustOpen(t, root, DefaultSession) + if feature, slug := b2.Feature(); feature != "blog inbound hub" || slug != "blog-inbound-hub" { + t.Fatalf("reopened session must preserve identity across processes, got feature=%q slug=%q", feature, slug) + } +} + +// TestLookup_ConcurrentCallsAreSafe is a regression test for ISSUE 5 in +// docs/plans/FORGE_SHIP_ISSUES_2026-09-04.md (ai-marketing-platfrom repo): +// `forge ship --agent-mode` observed hanging with zero output for minutes +// mid-arch-debate. checkArch's runParallelArchDebate (arch.go) fires one +// goroutine per reviewer role and every one calls Lookup — through +// LLMPipe.invokeViaBridge — against the same operation name +// ("arch-parallel-debate") and the same Bridge, concurrently. Before Bridge +// gained its mutex, this unsynchronized concurrent access to the +// seen/byHash/byOrdinal maps and the pending/paused fields was a data race: +// `go test -race` would have caught it immediately, and in production it +// could surface as a runtime panic or, plausibly, the observed hang (the map +// implementation spinning on state corrupted by a concurrent write). This +// test reproduces the exact shape — N goroutines, one Lookup call each, +// same operation, different prompts — and must complete without panicking +// or deadlocking, and must leave the bridge in a single, consistent paused +// state (exactly one pending turn) rather than a torn one. +func TestLookup_ConcurrentCallsAreSafe(t *testing.T) { + t.Parallel() + root := t.TempDir() + b := mustOpen(t, root, DefaultSession) + + const roles = 6 + var wg sync.WaitGroup + errs := make([]error, roles) + for i := 0; i < roles; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + _, err := b.Lookup("arch-parallel-debate", "arch", + "", "persona system prompt", strings.Repeat("x", idx+1), 300) + errs[idx] = err + }(i) + } + wg.Wait() + + for i, err := range errs { + if !errors.Is(err, ErrTurnRequired) { + t.Fatalf("role %d: expected ErrTurnRequired (a pause, not a real failure), got %v", i, err) + } + } + if !b.Paused() { + t.Fatal("bridge must be paused after any Lookup miss") + } + if _, ok := b.Pending(); !ok { + t.Fatal("exactly one pending turn must be recorded, not zero") + } +} + // ── Regression: a restored pending turn must latch paused immediately ──────── // // Root cause this guards: loadPending used to leave b.paused false until some diff --git a/internal/cli/cmdship/arch.go b/internal/cli/cmdship/arch.go index 6b38c56..e1cf56b 100644 --- a/internal/cli/cmdship/arch.go +++ b/internal/cli/cmdship/arch.go @@ -116,7 +116,11 @@ func runParallelArchDebate(pipe *LLMPipe, description, archDoc string, maxTokens // - If spec.md is missing: returns "warning" with a hint to run spec first. // - With an LLMPipe: generates full ADR then runs parallel role debate. // - Without an LLMPipe: writes a structured stub ADR to arch.md. -func checkArch(root, description, specName string, pipe *LLMPipe) Checkpoint { +// +// dryRun, when true, never writes to disk — see checkSpec's matching doc +// comment and ISSUE 3 in docs/plans/FORGE_SHIP_ISSUES_2026-09-04.md +// (ai-marketing-platfrom repo). +func checkArch(root, description, specName string, pipe *LLMPipe, dryRun bool) Checkpoint { cp := Checkpoint{Name: "Arch"} if description == "" && specName == "" { @@ -167,6 +171,15 @@ func checkArch(root, description, specName string, pipe *LLMPipe) Checkpoint { return cp } + // dry-run: arch.md does not exist (idempotent check above already + // returned otherwise) and generating it would create .forge/specs// + // and write arch.md + openapi.yaml — a preview must not do either. + if dryRun { + cp.Status = "ok" + cp.Detail = fmt.Sprintf("dry-run: would generate arch document + openapi.yaml for %q — no files written", description) + return cp + } + // Ensure the spec directory exists before writing arch.md. _ = os.MkdirAll(filepath.Join(root, ".forge", "specs", slug), 0o755) diff --git a/internal/cli/cmdship/arch_test.go b/internal/cli/cmdship/arch_test.go index 6d22ce0..297b77c 100644 --- a/internal/cli/cmdship/arch_test.go +++ b/internal/cli/cmdship/arch_test.go @@ -61,7 +61,7 @@ func TestCheckArch_LLM_GeneratesArchDoc(t *testing.T) { mock := &llmprovider.MockProvider{ Response: mockResponse("# Architecture: add payment API\n\n## 1. Component Topology\n\nPayment service.\n"), } - cp := checkArch(root, "add payment api", "", mockPipe(root, mock)) + cp := checkArch(root, "add payment api", "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) @@ -106,7 +106,7 @@ func TestCheckArch_ExistingArch_Idempotent(t *testing.T) { } mock := &llmprovider.MockProvider{Response: mockResponse("should not be called")} - cp := checkArch(root, "existing arch feature", "", mockPipe(root, mock)) + cp := checkArch(root, "existing arch feature", "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("expected ok for existing arch, got %q: %s", cp.Status, cp.Detail) @@ -132,7 +132,7 @@ func TestCheckArch_NoSpec_Warning(t *testing.T) { t.Parallel() root := t.TempDir() mock := &llmprovider.MockProvider{Response: mockResponse("should not reach LLM")} - cp := checkArch(root, "missing spec feature", "", mockPipe(root, mock)) + cp := checkArch(root, "missing spec feature", "", mockPipe(root, mock), false) if cp.Status != "warning" { t.Fatalf("expected warning when no spec, got %q: %s", cp.Status, cp.Detail) @@ -149,7 +149,7 @@ func TestCheckArch_NoSpec_Warning(t *testing.T) { func TestCheckArch_NoDescription_NoLLM_Warning(t *testing.T) { t.Parallel() root := t.TempDir() - cp := checkArch(root, "", "", nil) + cp := checkArch(root, "", "", nil, false) if cp.Status != "warning" { t.Fatalf("expected warning with no description, got %q: %s", cp.Status, cp.Detail) @@ -165,7 +165,7 @@ func TestCheckArch_NoDescription_WithLLM_Warning(t *testing.T) { t.Parallel() root := t.TempDir() mock := &llmprovider.MockProvider{Response: mockResponse("ignored")} - cp := checkArch(root, "", "", mockPipe(root, mock)) + cp := checkArch(root, "", "", mockPipe(root, mock), false) if cp.Status != "warning" { t.Fatalf("expected warning with no description + LLM, got %q: %s", cp.Status, cp.Detail) @@ -192,7 +192,7 @@ func TestCheckArch_LLMError_StubWritten(t *testing.T) { } mock := &llmprovider.MockProvider{Err: fmt.Errorf("FORGE-4051 transport not implemented")} - cp := checkArch(root, "payment feature", "", mockPipe(root, mock)) + cp := checkArch(root, "payment feature", "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("LLM error should not fail the arch checkpoint; got %q: %s", cp.Status, cp.Detail) @@ -227,7 +227,7 @@ func TestCheckArch_NoLLM_StubWritten(t *testing.T) { t.Fatal(err) } - cp := checkArch(root, "new feature", "", nil) + cp := checkArch(root, "new feature", "", nil, false) if cp.Status != "ok" { t.Fatalf("expected ok without LLM, got %q: %s", cp.Status, cp.Detail) @@ -550,7 +550,7 @@ func TestCheckArch_ForwardsWorkspaceContext(t *testing.T) { return mockResponse("# Architecture: add payment API\n\n## 1. Component Topology\n\nPayment service.\n"), nil }, } - cp := checkArch(root, "add payment api", "", mockPipe(root, mock)) + cp := checkArch(root, "add payment api", "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) @@ -692,7 +692,7 @@ func TestCheckArch_LLM_SupabaseRPCResponse(t *testing.T) { "components:\n schemas: {}\n```\n" mock := &llmprovider.MockProvider{Response: mockResponse(llmResponse)} - cp := checkArch(root, "supabase user profile", "", mockPipe(root, mock)) + cp := checkArch(root, "supabase user profile", "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) diff --git a/internal/cli/cmdship/artefacts_test.go b/internal/cli/cmdship/artefacts_test.go index c4907b9..1ab4d64 100644 --- a/internal/cli/cmdship/artefacts_test.go +++ b/internal/cli/cmdship/artefacts_test.go @@ -31,7 +31,7 @@ func TestCheckSpec_WritesSpecYML(t *testing.T) { t.Parallel() root := t.TempDir() - cp := checkSpec(root, "add login", "", nil) + cp := checkSpec(root, "add login", "", nil, false) if cp.Status != "ok" { t.Fatalf("expected status ok, got %q (detail: %s)", cp.Status, cp.Detail) } diff --git a/internal/cli/cmdship/llmpipe.go b/internal/cli/cmdship/llmpipe.go index c1a7656..bfe3448 100644 --- a/internal/cli/cmdship/llmpipe.go +++ b/internal/cli/cmdship/llmpipe.go @@ -81,23 +81,22 @@ type LLMPipe struct { checkpoint string } -// newLLMPipe detects the active LLM provider from the environment and returns -// an initialized *LLMPipe. Returns nil (not an error) if no provider is -// configured so callers silently fall back to structural dry-run behavior. -func newLLMPipe(root string) *LLMPipe { - p, err := llmprovider.Detect() - if err != nil { - return nil - } - return newLLMPipeWithProvider(p, root) -} - -// newLLMPipeInteractive is like newLLMPipe but prompts the user for an API key -// when no provider is detected and dryRun is false. In dry-run mode it falls -// back to nil silently, matching the old behaviour. +// newLLMPipeInteractive detects the active LLM provider and returns an +// initialized *LLMPipe, prompting the user for an API key when none is +// detected and dryRun is false. +// +// dryRun always returns nil, never a live pipe. ISSUE 3 +// (docs/plans/FORGE_SHIP_ISSUES_2026-09-04.md, ai-marketing-platfrom repo): +// this used to detect and return a live provider pipe whenever credentials +// were configured — dry-run or not — contradicting the documented --dry-run +// contract in cobra's own help text ("preview what would happen without +// making LLM calls or git operations"). A --dry-run run with a working API +// key could silently dial (and bill) the real provider. Every checkpoint's +// own nil-pipe branch is the long-standing, well-tested "no provider +// configured" preview path, so dry-run doesn't need a parallel one. func newLLMPipeInteractive(root string, dryRun bool) *LLMPipe { if dryRun { - return newLLMPipe(root) + return nil } p, err := llmprovider.DetectOrPrompt(nil, nil) // nil → os.Stdin / os.Stderr if err != nil { @@ -359,6 +358,55 @@ func shipMessage(pipe *LLMPipe) string { return "LLM provider: " + pipe.ProviderName() } +// probeProviderUsable detects the configured LLM provider the same way +// newLLMPipe does, then sends one minimal live completion request to confirm +// it can actually serve the pipeline — not just that credentials are +// present. It mirrors `forge doctor --llm`'s checkLLMProviderLive, which +// exists for the identical reason: credential presence and a working call +// are two different things, and a stale forge.yml model pin or an +// out-of-credit API key otherwise fails silently on every checkpoint. +// +// Returns usable=false only when a provider IS configured but a live call +// against it fails for a permanent reason — an invalid/expired API key, or a +// hard invalid_request_error (this is how Anthropic reports "credit balance +// too low" — see llmprovider/anthropic_errors.go ErrInvalidRequest). No +// provider configured at all (including the test-only FORGE_NO_LLM=1 escape +// hatch) is deliberately reported usable=true: that path already has its own +// long-standing UX — a nil pipe that writes stub artefacts and prints a "set +// ANTHROPIC_API_KEY" hint — and is not the failure this fallback targets +// (ISSUE 1: a configured provider that is silently unusable, e.g. an +// out-of-credit key). A transient failure class (rate limiting, a +// momentarily-dead model id the tier router can already fall back around) is +// also left usable=true so a passing blip does not trip the whole pipeline +// into agent mode. +func probeProviderUsable() (usable bool, reason string) { + p, err := llmprovider.Detect() + if err != nil { + return true, "" + } + + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + _, err = p.Complete(ctx, &llmprovider.Request{ + UserPrompt: "Reply with exactly: ok", + MaxTokens: 8, + Capability: "ship-agent-fallback-probe", + }) + if err == nil { + return true, "" + } + var ecErr *errcode.Error + switch { + case errors.As(err, &ecErr) && (ecErr.Code == llmprovider.ErrAuthFailed || ecErr.Code == llmprovider.ErrInvalidRequest): + return false, fmt.Sprintf("provider=%s: %s", p.Name(), llmErrNote(err)) + default: + // Rate limits, transient 5xx, a dead model id the tier router can + // route around, etc. — let the normal checkpoint retry/error path + // handle it rather than pausing the whole run for agent input. + return true, "" + } +} + // extractAPIErrorMessage pulls the human-readable "message" field out of a // provider error body shaped like {"error":{"message":"..."}} or // {"error":{"type":"...","message":"..."}} (the shape used by both Anthropic diff --git a/internal/cli/cmdship/ship.go b/internal/cli/cmdship/ship.go index 1f72fdf..ae6af16 100644 --- a/internal/cli/cmdship/ship.go +++ b/internal/cli/cmdship/ship.go @@ -460,6 +460,25 @@ func New() *cobra.Command { } } + // Auto-fallback: when the pipeline is not already in agent mode and the + // configured LLM provider is unusable for a permanent reason (no + // provider configured, auth failed, or a hard invalid_request such as + // "credit balance too low" — see llmpipe.go probeProviderUsable), drive + // the run via the host agent instead of hard-failing every LLM + // checkpoint. This is the documented escape hatch (--agent-mode) turned + // on automatically instead of requiring the operator to notice the + // failure and re-invoke with the flag. FORGE_NO_AGENT_FALLBACK=1 opts + // out for callers that want a hard failure instead (e.g. CI jobs that + // should not silently pause on a human/host-agent turn). + if !agentMode && !dryRun && os.Getenv("FORGE_AGENT_MODE") != "1" && os.Getenv("FORGE_NO_AGENT_FALLBACK") != "1" { + if usable, reason := probeProviderUsable(); !usable { + fmt.Fprintf(cmd.ErrOrStderr(), + "note: configured LLM provider is unusable (%s) — falling back to --agent-mode "+ + "automatically (set FORGE_NO_AGENT_FALLBACK=1 to disable)\n", reason) + agentMode = true + } + } + // Agent mode: swap the reasoning plane from a paid provider to the // host agent. The deterministic plane is untouched — same checkpoints, // same gates, same artefact validation. @@ -471,6 +490,19 @@ func New() *cobra.Command { return errcode.New(ErrAgentTurn, "open agent bridge", bErr) } bridge.StrictReplay = strictReplay + // ISSUE 4 fix: a bare continuation (no --name, no description — the + // shape of the hint forge itself prints after a submit: "next: forge + // ship --agent-mode") must resume the same feature this session was + // already driving, not fall through to whatever spec/checkpoint + // resolution does with an empty name and description. Resolve from + // the session's own recorded identity before SetFeature runs, and + // propagate into runOpts so RunWithOptions targets the right spec. + if description == "" && specName == "" { + if priorFeature, priorSlug := bridge.Feature(); priorSlug != "" || priorFeature != "" { + description, specName = priorFeature, priorSlug + runOpts.Description, runOpts.SpecName = description, specName + } + } if bridge.SetFeature(description, specName) { fmt.Fprintf(cmd.ErrOrStderr(), "note: session %q was driving a different feature — recorded answers reset for %q "+ @@ -796,7 +828,13 @@ func specYAMLContext(spec *cmdtest.TestSpec) string { // Without an LLMPipe (no provider configured): a Markdown stub is written. // When a pre-generated spec.yml (from `forge test spec`) exists it is loaded // to enrich the LLM call via InvokeWithKnowledge and surfaced in the detail. -func checkSpec(root, description, specName string, pipe *LLMPipe) Checkpoint { +// dryRun, when true, never writes to disk — matches the documented +// `--dry-run` contract and checkTest's existing precedent. See ISSUE 3 in +// docs/plans/FORGE_SHIP_ISSUES_2026-09-04.md (ai-marketing-platfrom repo): +// before this fix, a --dry-run run still created .forge/specs// and +// wrote workspace-context.md and a spec.md stub for any not-yet-generated +// feature, regardless of the flag. +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) @@ -819,7 +857,15 @@ func checkSpec(root, description, specName string, pipe *LLMPipe) Checkpoint { // G-009 (workspace-context phase): collect deterministic project context // before any LLM call so the spec reflects the actual tech stack, // conventions, recent changes, and existing features. - wsCtx := collectWorkspaceContext(root, slug) + // Skipped during --dry-run: collectWorkspaceContext writes + // workspace-context.md unconditionally, and a preview must not create + // files (ISSUE 3). pipe is already guaranteed nil in dry-run (see + // newLLMPipeInteractive), so no LLM call below ever consumes wsSection + // anyway. + var wsCtx WorkspaceContextResult + if !dryRun { + wsCtx = collectWorkspaceContext(root, slug) + } wsSection := "" if wsCtx.Content != "" { wsSection = "\n\n## Workspace Context\n" + wsCtx.Content @@ -929,6 +975,17 @@ func checkSpec(root, description, specName string, pipe *LLMPipe) Checkpoint { } // spec.md does not exist. + if dryRun { + cp.Status = "ok" + if ySpec != nil { + cp.Detail = fmt.Sprintf( + "dry-run: would generate spec.md from spec.yml (%d cases) for %q — no files written", + len(ySpec.Cases), description) + } else { + cp.Detail = fmt.Sprintf("dry-run: would generate spec.md for %q — no files written", description) + } + return cp + } // If a YAML spec is present, generate spec.md from it (KB-enriched when LLM available). if ySpec != nil { if err := os.MkdirAll(filepath.Join(specsDir, slug), 0o755); err == nil { @@ -2139,7 +2196,7 @@ func runWithOptions(opts RunOptions) *ShipResult { if needs("spec") { beforeCheckpoint("spec") - results["spec"] = checkSpec(root, opts.Description, opts.SpecName, pipe) + results["spec"] = checkSpec(root, opts.Description, opts.SpecName, pipe, opts.DryRun) } // P1 DAG: run arch and test in parallel when both are needed — they are @@ -2153,7 +2210,7 @@ func runWithOptions(opts RunOptions) *ShipResult { case serial: if runArch { beforeCheckpoint("arch") - archCP = checkArch(root, opts.Description, opts.SpecName, pipe) + archCP = checkArch(root, opts.Description, opts.SpecName, pipe, opts.DryRun) } if runTest && !agentPaused() { beforeCheckpoint("test") @@ -2167,7 +2224,7 @@ func runWithOptions(opts RunOptions) *ShipResult { beforeCheckpoint("arch") go func() { defer dagWG.Done() - archCP = checkArch(root, opts.Description, opts.SpecName, pipe) + archCP = checkArch(root, opts.Description, opts.SpecName, pipe, opts.DryRun) }() } if runTest { diff --git a/internal/cli/cmdship/ship_test.go b/internal/cli/cmdship/ship_test.go index d350e79..4d55218 100644 --- a/internal/cli/cmdship/ship_test.go +++ b/internal/cli/cmdship/ship_test.go @@ -793,7 +793,7 @@ func TestCheckSpec_LLM_GeneratesNewSpec(t *testing.T) { mock := &llmprovider.MockProvider{ Response: mockResponse("# Spec: add login\n\n## What\nAdd a login form.\n"), } - cp := checkSpec(root, "add login", "", mockPipe(root, mock)) + cp := checkSpec(root, "add login", "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) @@ -832,7 +832,7 @@ func TestCheckSpec_LLM_ReviewsExistingSpec(t *testing.T) { mock := &llmprovider.MockProvider{ Response: mockResponse("# Enhanced Spec\n\n## What\nImproved content.\n"), } - cp := checkSpec(root, "review feature", "", mockPipe(root, mock)) + cp := checkSpec(root, "review feature", "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) @@ -852,7 +852,7 @@ func TestCheckSpec_LLM_ProviderFails_GracefulDegradation(t *testing.T) { t.Parallel() root := t.TempDir() mock := &llmprovider.MockProvider{Err: fmt.Errorf("FORGE-4051 transport not implemented")} - cp := checkSpec(root, "failing feature", "", mockPipe(root, mock)) + cp := checkSpec(root, "failing feature", "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("provider error must not fail the spec checkpoint; got %q: %s", cp.Status, cp.Detail) @@ -865,7 +865,7 @@ func TestCheckSpec_LLM_NoDescription_Warning(t *testing.T) { t.Parallel() root := t.TempDir() mock := &llmprovider.MockProvider{Response: mockResponse("ignored")} - cp := checkSpec(root, "", "", mockPipe(root, mock)) + cp := checkSpec(root, "", "", mockPipe(root, mock), false) if cp.Status != "warning" { t.Fatalf("expected warning with no description, got %q: %s", cp.Status, cp.Detail) @@ -893,7 +893,7 @@ func TestCheckSpec_NoDescription_MultipleSpecs_HardFails(t *testing.T) { t.Fatalf("mkdir %s: %v", slug, err) } } - cp := checkSpec(root, "", "", nil) + cp := checkSpec(root, "", "", nil, false) if cp.Status != "fail" { t.Fatalf("ambiguous multi-spec target must hard-fail, got %q: %s", cp.Status, cp.Detail) @@ -918,7 +918,7 @@ func TestCheckSpec_NoDescription_SingleSpec_StillOK(t *testing.T) { if err := os.MkdirAll(filepath.Join(root, ".forge", "specs", "add-login"), 0o755); err != nil { t.Fatalf("mkdir: %v", err) } - cp := checkSpec(root, "", "", nil) + cp := checkSpec(root, "", "", nil, false) if cp.Status != "ok" { t.Fatalf("a single unambiguous spec dir must not hard-fail, got %q: %s", cp.Status, cp.Detail) @@ -990,7 +990,7 @@ func TestCheckSpec_YAML_WithLLM_KBEnrichedReview(t *testing.T) { mock := &llmprovider.MockProvider{ Response: mockResponse("# Enhanced Spec\n## What\nKB-enriched.\n"), } - cp := checkSpec(root, feature, "", mockPipe(root, mock)) + cp := checkSpec(root, feature, "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) @@ -1028,7 +1028,7 @@ func TestCheckSpec_YAML_NoLLM_DetailShowsCaseCount(t *testing.T) { t.Fatal(err) } - cp := checkSpec(root, feature, "", nil) + cp := checkSpec(root, feature, "", nil, false) if cp.Status != "ok" { t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) @@ -1055,7 +1055,7 @@ func TestCheckSpec_YAML_ZeroCases_StillOK(t *testing.T) { t.Fatal(err) } - cp := checkSpec(root, feature, "", nil) + cp := checkSpec(root, feature, "", nil, false) if cp.Status != "ok" { t.Fatalf("spec with 0 cases must still be ok; got %q: %s", cp.Status, cp.Detail) @@ -1081,7 +1081,7 @@ func TestCheckSpec_YAML_CorruptYAML_FallsBackToSpecMD(t *testing.T) { t.Fatal(err) } - cp := checkSpec(root, feature, "", nil) + cp := checkSpec(root, feature, "", nil, false) if cp.Status != "ok" { t.Fatalf("corrupt spec.yml must not fail checkpoint; got %q: %s", cp.Status, cp.Detail) @@ -1104,8 +1104,8 @@ func TestCheckSpec_YAML_Idempotency(t *testing.T) { t.Fatal(err) } - cp1 := checkSpec(root, feature, "", nil) - cp2 := checkSpec(root, feature, "", nil) + cp1 := checkSpec(root, feature, "", nil, false) + cp2 := checkSpec(root, feature, "", nil, false) if cp1.Status != "ok" || cp2.Status != "ok" { t.Fatalf("both calls must be ok; got %q, %q", cp1.Status, cp2.Status) @@ -1134,7 +1134,7 @@ func TestCheckSpec_YAML_Regression_SpecMDOnly(t *testing.T) { mock := &llmprovider.MockProvider{ Response: mockResponse("# Enhanced\n"), } - cp := checkSpec(root, feature, "", mockPipe(root, mock)) + cp := checkSpec(root, feature, "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("spec.md-only path must be ok; got %q: %s", cp.Status, cp.Detail) @@ -1160,7 +1160,7 @@ func TestCheckSpec_YAML_DataAccuracy_DetailHasCaseCountAndFamilies(t *testing.T) t.Fatal(err) } - cp := checkSpec(root, feature, "", nil) + cp := checkSpec(root, feature, "", nil, false) if cp.Status != "ok" { t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) @@ -1195,7 +1195,7 @@ func TestCheckSpec_YAML_FalsePositiveGuard_NoYAML_NoFailure(t *testing.T) { t.Fatal("test pre-condition: spec.yml must not exist") } - cp := checkSpec(root, feature, "", nil) + cp := checkSpec(root, feature, "", nil, false) if cp.Status != "ok" { t.Fatalf("absent spec.yml must not fail; got %q: %s", cp.Status, cp.Detail) @@ -1219,7 +1219,7 @@ func TestCheckSpec_YAML_OnlyYAML_GeneratesSpecMD(t *testing.T) { mock := &llmprovider.MockProvider{ Response: mockResponse("# Generated from YAML\n## Acceptance Criteria\n- happy path\n"), } - cp := checkSpec(root, feature, "", mockPipe(root, mock)) + cp := checkSpec(root, feature, "", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("expected ok when generating from spec.yml; got %q: %s", cp.Status, cp.Detail) @@ -1255,7 +1255,7 @@ func TestCheckSpec_SpecName_HappyPath(t *testing.T) { t.Fatal(err) } - cp := checkSpec(root, "add login feature", "login", nil) + cp := checkSpec(root, "add login feature", "login", nil, false) if cp.Status != "ok" { t.Fatalf("expected ok with --name login override, got %q: %s", cp.Status, cp.Detail) @@ -1282,7 +1282,7 @@ func TestCheckSpec_SpecName_WithYAML_KBEnriched(t *testing.T) { Response: mockResponse("# Auth Spec Enhanced\n"), } - cp := checkSpec(root, "authentication flow", "auth", mockPipe(root, mock)) + cp := checkSpec(root, "authentication flow", "auth", mockPipe(root, mock), false) if cp.Status != "ok" { t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) @@ -1308,7 +1308,7 @@ func TestCheckSpec_SpecName_Empty_FallsBackToSlug(t *testing.T) { } // specName="" → must resolve via slugify(feature). - cp := checkSpec(root, feature, "", nil) + cp := checkSpec(root, feature, "", nil, false) if cp.Status != "ok" { t.Fatalf("empty specName must use derived slug %q; got %q: %s", slug, cp.Status, cp.Detail) @@ -1324,7 +1324,7 @@ func TestCheckSpec_SpecName_NoSuchDir_GeneratesStub(t *testing.T) { t.Parallel() root := t.TempDir() - cp := checkSpec(root, "my feature", "unknown-spec", nil) + cp := checkSpec(root, "my feature", "unknown-spec", nil, false) if cp.Status != "ok" { t.Fatalf("missing spec dir should produce stub, not fail; got %q: %s", cp.Status, cp.Detail) @@ -1348,8 +1348,8 @@ func TestCheckSpec_SpecName_Idempotency(t *testing.T) { t.Fatal(err) } - cp1 := checkSpec(root, "some description", "my-feature", nil) - cp2 := checkSpec(root, "some description", "my-feature", nil) + cp1 := checkSpec(root, "some description", "my-feature", nil, false) + cp2 := checkSpec(root, "some description", "my-feature", nil, false) if cp1.Status != "ok" || cp2.Status != "ok" { t.Fatalf("both calls must be ok; got %q, %q", cp1.Status, cp2.Status) @@ -1375,7 +1375,7 @@ func TestCheckSpec_SpecName_Regression_NoFlag_DescriptionSlugWorks(t *testing.T) t.Fatal(err) } - cp := checkSpec(root, desc, "", nil) + cp := checkSpec(root, desc, "", nil, false) if cp.Status != "ok" { t.Fatalf("description-derived slug path must still work; got %q: %s", cp.Status, cp.Detail) @@ -1395,7 +1395,7 @@ func TestCheckSpec_SpecName_DataAccuracy_DetailContainsSpecName(t *testing.T) { t.Fatal(err) } - cp := checkSpec(root, "admin dashboard feature", "dashboard", nil) + cp := checkSpec(root, "admin dashboard feature", "dashboard", nil, false) if cp.Status != "ok" { t.Fatalf("expected ok, got %q: %s", cp.Status, cp.Detail) @@ -1442,7 +1442,7 @@ func TestCheckSpec_SpecName_OnlySpecName_NoDescription(t *testing.T) { t.Fatal(err) } - cp := checkSpec(root, "", "login", nil) + cp := checkSpec(root, "", "login", nil, false) if cp.Status != "ok" { t.Fatalf("spec-name-only (no description) must be ok; got %q: %s", cp.Status, cp.Detail)