From d4e7f49f78cd955488a8b2e2a1ef59b017d16cb3 Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Sat, 5 Sep 2026 02:13:33 +0700 Subject: [PATCH 1/2] fix(ship): auto-fallback to --agent-mode on unusable LLM provider; fix agent-mode bare-continuation resuming wrong feature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - forge ship now probes the configured LLM provider (mirrors `forge doctor --llm`) before a non-agent-mode run. When a provider is configured but a live call fails permanently (invalid/expired key, or Anthropic's invalid_request_error shape for "credit balance too low"), the run automatically switches to --agent-mode instead of hard-failing every checkpoint. FORGE_NO_AGENT_FALLBACK=1 opts out. No provider configured at all is left untouched (existing stub/hint UX, not this failure). - Bridge.SetFeature("", "") — the bare continuation forge itself prints as the next-step hint after a submit — was blanking the session's recorded feature identity on every call, so the next resolution of "which feature is this session driving" had nothing to resume against and could fall through to an unrelated feature's incomplete checkpoint (ISSUE 4 in docs/plans/FORGE_SHIP_ISSUES_2026-09-04.md, ai-marketing-platfrom repo). SetFeature("", "") is now a no-op when an identity already exists, and the agent-mode bare-continuation path resolves the missing --name/description from the session's own Bridge.Feature() first. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 6 +++ internal/agentbridge/agentbridge.go | 26 +++++++++++++ internal/agentbridge/agentbridge_test.go | 33 ++++++++++++++++ internal/cli/cmdship/llmpipe.go | 49 ++++++++++++++++++++++++ internal/cli/cmdship/ship.go | 32 ++++++++++++++++ 5 files changed, 146 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4d25ce..97689a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to forge will be documented in this file. Format follows [Ke ## [Unreleased] +### Fixed + +- **`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. + ## [1.10.2] — 2026-08-21 — Agent mode stopped pausing: a bridge miss was treated as an LLM failure ### Fixed diff --git a/internal/agentbridge/agentbridge.go b/internal/agentbridge/agentbridge.go index d6ff375..868d938 100644 --- a/internal/agentbridge/agentbridge.go +++ b/internal/agentbridge/agentbridge.go @@ -222,6 +222,19 @@ 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 "", "" + } + return b.session.Feature, b.session.Slug +} + // SetFeature records what feature this session is driving. Best-effort: a // persistence failure never blocks the pipeline. // @@ -236,6 +249,15 @@ 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 @@ -243,6 +265,10 @@ func (b *Bridge) SetFeature(feature, slug string) (switched bool) { 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() switched = true diff --git a/internal/agentbridge/agentbridge_test.go b/internal/agentbridge/agentbridge_test.go index 014cfbf..ebc52c3 100644 --- a/internal/agentbridge/agentbridge_test.go +++ b/internal/agentbridge/agentbridge_test.go @@ -485,3 +485,36 @@ func TestSetFeature_SwitchingFeatureResetsStaleSession(t *testing.T) { t.Fatalf("expected a fresh pause for the new feature, got content=%q err=%v", content, lookupErr) } } + +// 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) + } +} diff --git a/internal/cli/cmdship/llmpipe.go b/internal/cli/cmdship/llmpipe.go index c1a7656..b2e9b4a 100644 --- a/internal/cli/cmdship/llmpipe.go +++ b/internal/cli/cmdship/llmpipe.go @@ -359,6 +359,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 9e3f95d..636fccc 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 "+ From d9804a61f479af6dad7c3591af0358b4a366e985 Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Sat, 5 Sep 2026 09:26:10 +0700 Subject: [PATCH 2/2] fix(ship): serialize agent-mode bridge access; never dial a live LLM in --dry-run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses ISSUE 5 and ISSUE 3 in docs/plans/FORGE_SHIP_ISSUES_2026-09-04.md (ai-marketing-platfrom repo), on top of the ISSUE 1/4 fixes already on this branch. - CRITICAL: Bridge was documented "not safe for concurrent use", but nothing enforced it. checkArch's runParallelArchDebate fires one goroutine per reviewer role (6 by default), and every one calls Lookup concurrently against the same Bridge in agent mode — an unsynchronized data race on the seen/byHash/byOrdinal maps and the pending/paused fields, which the Go runtime can surface as a panic or, plausibly, as the multi-minute hang with zero output actually observed mid-arch-debate. Bridge now serializes every exported method touching shared state behind a mutex (SetFeature calls a new unexported resetLocked to avoid re-entering it from Reset). New regression test TestLookup_ConcurrentCallsAreSafe reproduces the exact six-goroutines-one-operation shape; go test -race already runs nightly. - --dry-run made real LLM calls and wrote files to disk despite its own help text ("without making LLM calls or git operations"). newLLMPipeInteractive returned a live, billable pipe whenever a provider was configured, dry-run or not — its own doc comment already (incorrectly) claimed otherwise. checkSpec/checkArch had no dryRun parameter at all, so previewing a not-yet-generated feature unconditionally created .forge/specs// and wrote workspace-context.md plus a spec.md/arch.md stub (or real LLM output, if credentials worked) — the stray-directory behavior the doc observed running exploratory --dry-run probes. Dry-run now always gets a nil pipe, and checkSpec/checkArch report what they would generate without touching disk when the target artefact doesn't exist yet. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 4 ++ internal/agentbridge/agentbridge.go | 66 +++++++++++++++++++++--- internal/agentbridge/agentbridge_test.go | 49 ++++++++++++++++++ internal/cli/cmdship/arch.go | 15 +++++- internal/cli/cmdship/arch_test.go | 18 +++---- internal/cli/cmdship/artefacts_test.go | 2 +- internal/cli/cmdship/llmpipe.go | 29 +++++------ internal/cli/cmdship/ship.go | 35 +++++++++++-- internal/cli/cmdship/ship_test.go | 46 ++++++++--------- 9 files changed, 204 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97689a0..9b2dd27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ All notable changes to forge will be documented in this file. Format follows [Ke - **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 ### Fixed diff --git a/internal/agentbridge/agentbridge.go b/internal/agentbridge/agentbridge.go index 868d938..ae7818d 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 @@ -232,6 +252,8 @@ 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 } @@ -262,6 +284,8 @@ 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 != "" @@ -270,7 +294,7 @@ func (b *Bridge) SetFeature(feature, slug string) (switched bool) { return false } if hadPrior && newIdentity && prevSlug != slug && prevFeature != feature { - _ = b.Reset() + _ = b.resetLocked() switched = true } b.session.Feature = feature @@ -280,11 +304,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 @@ -300,6 +336,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 @@ -361,7 +399,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) == "" { @@ -399,6 +442,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) @@ -421,6 +473,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 ebc52c3..49cc65c 100644 --- a/internal/agentbridge/agentbridge_test.go +++ b/internal/agentbridge/agentbridge_test.go @@ -32,6 +32,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" ) @@ -518,3 +519,51 @@ func TestSetFeature_BareContinuationPreservesIdentity(t *testing.T) { 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") + } +} 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 d8ab1b7..fb52236 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 b2e9b4a..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 { diff --git a/internal/cli/cmdship/ship.go b/internal/cli/cmdship/ship.go index 636fccc..c969f63 100644 --- a/internal/cli/cmdship/ship.go +++ b/internal/cli/cmdship/ship.go @@ -828,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) @@ -851,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 @@ -961,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 { @@ -2143,7 +2168,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 @@ -2157,7 +2182,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") @@ -2171,7 +2196,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 e8167fa..81b8bca 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) @@ -940,7 +940,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) @@ -978,7 +978,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) @@ -1005,7 +1005,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) @@ -1031,7 +1031,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) @@ -1054,8 +1054,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) @@ -1084,7 +1084,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) @@ -1110,7 +1110,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) @@ -1145,7 +1145,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) @@ -1169,7 +1169,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) @@ -1205,7 +1205,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) @@ -1232,7 +1232,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) @@ -1258,7 +1258,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) @@ -1274,7 +1274,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) @@ -1298,8 +1298,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) @@ -1325,7 +1325,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) @@ -1345,7 +1345,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) @@ -1392,7 +1392,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)