fix(ship): auto-fallback to --agent-mode on unusable LLM provider; fix agent-mode wrong-feature resume

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/<slug>/`, 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
 

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

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

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/<slug>/
+	// 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)