diff --git a/CHANGELOG.md b/CHANGELOG.md index e1267df..16b803d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to forge will be documented in this file. Format follows [Ke ## [Unreleased] +### Fixed + +- **`forge ship --agent-mode` could run the host project's entire native test suite itself and hang for many minutes with zero output.** The `qa-verify` checkpoint's Phase 2 called `runQATestSuite`, which shells out to `npm test` / `go test ./...` / `pytest` on the target repo — with no agent-mode guard. On a large host project that is a multi-minute blocking run with its own output stream, and forge's `procspawn` timeout does not reliably reap a killed runner's orphaned worker processes on Windows (jest/vitest workers keep the stdout pipe open, so the read never sees EOF). When the pipeline reached `qa-verify` without pausing first — e.g. after `checkArch` short-circuits on an already-present `arch.md` from an earlier answered turn — the result was `forge ship --agent-mode` silently executing the caller's full test suite and appearing to hang. In agent mode the host agent *is* the QA agent (the checkpoint's own description: "QA agent: probe MCP server tools or run native test suite"), so `qa-verify` now emits an advisory telling the host agent to run and report the suite instead of running it itself. Non-agent-mode runs are unchanged. +- **`checkArch`'s parallel role debate still fanned out six goroutines against the agent bridge even though the bridge can only surface one pending turn per run.** The 1.10.4 mutex stopped the data race but not the pile-up: five of the six goroutines do throwaway work that is redone on the next replay, while serialising behind `Bridge.mu` as the first goroutine holds it across `savePending`'s file I/O — a contributing shape behind the "hangs with zero output mid-arch-debate" reports. In agent mode (`pipe.Bridge() != nil`) the debate now runs sequentially and stops at the first owed turn, matching `RunWithOptions`'s existing `serial` intent; the parallel path is kept for real-provider runs. New regression test `TestRunParallelArchDebate_AgentModeSerialNoFanOut`. + ## [1.10.4] — 2026-09-05 — LLM-pipeline auto-fallback, a critical wrong-feature resume, an arch-debate data race, and dry-run leaks ### Fixed diff --git a/internal/cli/cmdship/arch.go b/internal/cli/cmdship/arch.go index e1cf56b..3c3fb7e 100644 --- a/internal/cli/cmdship/arch.go +++ b/internal/cli/cmdship/arch.go @@ -57,9 +57,21 @@ func defaultArchRoles() []archRoleDebate { } } -// runParallelArchDebate concurrently invokes all arch reviewer roles and -// collects their concerns. Results are appended to the arch document as a -// "## Reviewer Concerns" section. A nil pipe is a no-op. +// runParallelArchDebate invokes all arch reviewer roles and collects their +// concerns. Results are appended to the arch document as a "## Reviewer +// Concerns" section. A nil pipe is a no-op. +// +// Concurrency: the roles run in parallel goroutines only when the reasoning +// plane is a real provider. In agent mode (pipe.Bridge() != nil) they run +// sequentially and stop at the first pause. Fanning out against the bridge +// there is pointless and unsafe: the bridge can only surface one pending turn +// per run, so every goroutine after the first miss does throwaway work that is +// redone on the next replay anyway — and it serialises six goroutines behind +// Bridge.mu while the first holds it across file I/O in savePending(), the +// exact shape behind the "forge ship --agent-mode hangs with zero output +// mid-arch-debate" reports (the 1.10.4 mutex stopped the data race but not the +// pile-up). Sequential + early-exit removes the hazard at the source and +// matches RunWithOptions's own `serial` intent for agent mode. func runParallelArchDebate(pipe *LLMPipe, description, archDoc string, maxTokens int) string { if pipe == nil { return "" @@ -70,27 +82,42 @@ func runParallelArchDebate(pipe *LLMPipe, description, archDoc string, maxTokens concern string } results := make([]result, len(roles)) - var wg sync.WaitGroup - - for i, role := range roles { - wg.Add(1) - go func(idx int, r archRoleDebate) { - defer wg.Done() - concern, err := pipe.InvokeDebateRound( - "arch-parallel-debate", - description, - r.persona, - archDoc, - "", - maxTokens, - ) - if err != nil || strings.TrimSpace(concern) == "" { - concern = "(no concerns raised)" + + askRole := func(idx int, r archRoleDebate) { + concern, err := pipe.InvokeDebateRound( + "arch-parallel-debate", + description, + r.persona, + archDoc, + "", + maxTokens, + ) + if err != nil || strings.TrimSpace(concern) == "" { + concern = "(no concerns raised)" + } + results[idx] = result{name: r.name, concern: concern} + } + + if bridge := pipe.Bridge(); bridge != nil { + // Agent mode: sequential, and stop as soon as a turn is owed. + for i, role := range roles { + if bridge.Paused() { + results[i] = result{name: role.name, concern: "(no concerns raised)"} + continue } - results[idx] = result{name: r.name, concern: concern} - }(i, role) + askRole(i, role) + } + } else { + var wg sync.WaitGroup + for i, role := range roles { + wg.Add(1) + go func(idx int, r archRoleDebate) { + defer wg.Done() + askRole(idx, r) + }(i, role) + } + wg.Wait() } - wg.Wait() var sb strings.Builder sb.WriteString("\n\n## Reviewer Concerns (parallel debate)\n\n") diff --git a/internal/cli/cmdship/rfc005_p1p2_test.go b/internal/cli/cmdship/rfc005_p1p2_test.go index 9fdf62b..83c0428 100644 --- a/internal/cli/cmdship/rfc005_p1p2_test.go +++ b/internal/cli/cmdship/rfc005_p1p2_test.go @@ -19,6 +19,9 @@ import ( "path/filepath" "strings" "testing" + "time" + + "github.com/teragrid/forge/internal/agentbridge" ) // ── snapshot.go ────────────────────────────────────────────────────────────── @@ -593,6 +596,58 @@ func TestRunParallelArchDebate_EmptyDocNoPanic(t *testing.T) { _ = runParallelArchDebate(nil, "feat", "", 100) } +// TestRunParallelArchDebate_AgentModeSerialNoFanOut is a regression test for the +// recurring "forge ship --agent-mode hangs with zero output mid-arch-debate" +// reports (docs/plans/FORGE_SHIP_ISSUES_2026-09-04.md ISSUE 5). The 1.10.4 fix +// added a mutex to Bridge, which stopped the data race but not the underlying +// hazard: six goroutines still fanned out against a bridge that can only ever +// surface one pending turn, piling up behind Bridge.mu while the first holds it +// across savePending()'s file I/O. In agent mode the debate must instead run +// sequentially and stop at the first owed turn. +func TestRunParallelArchDebate_AgentModeSerialNoFanOut(t *testing.T) { + t.Parallel() + root := t.TempDir() + bridge, err := agentbridge.Open(root, agentbridge.DefaultSession) + if err != nil { + t.Fatalf("open bridge: %v", err) + } + pipe := newLLMPipeAgent(root, bridge) + + done := make(chan string, 1) + go func() { done <- runParallelArchDebate(pipe, "feat", "# Arch Doc", 300) }() + + var out string + select { + case out = <-done: + case <-time.After(10 * time.Second): + t.Fatal("runParallelArchDebate hung in agent mode — it must run sequentially and return at the first owed turn") + } + + // Every role still appears in the appended section (unanswered ones as + // placeholders), so the arch document shape is unchanged. + if !strings.Contains(out, "## Reviewer Concerns (parallel debate)") { + t.Fatalf("missing Reviewer Concerns section:\n%s", out) + } + for _, r := range defaultArchRoles() { + if !strings.Contains(out, "### "+r.name) { + t.Errorf("role %q missing from debate output", r.name) + } + } + + // Exactly one turn is owed — the first role's — not six, and the bridge + // state is coherent (not corrupted by concurrent writes). + st := bridge.Stats() + if st.Pending == nil { + t.Fatal("expected one pending turn after the agent-mode debate, got none") + } + if st.Pending.Operation != "arch-parallel-debate" { + t.Fatalf("pending turn operation = %q, want arch-parallel-debate", st.Pending.Operation) + } + if !bridge.Paused() { + t.Fatal("bridge must be paused once the first role's turn is owed") + } +} + // ── DAG parallel pipeline ────────────────────────────────────────────────── // // Test design: diff --git a/internal/cli/cmdship/ship.go b/internal/cli/cmdship/ship.go index ae6af16..52d614f 100644 --- a/internal/cli/cmdship/ship.go +++ b/internal/cli/cmdship/ship.go @@ -1987,6 +1987,26 @@ func checkQAVerify(root, description, specName string, pipe *LLMPipe) Checkpoint } // ── Phase 2: automated test suite ─────────────────────────────────────── + // In agent mode the host agent *is* the QA agent — the checkpoint's own + // description is "QA agent: probe MCP server tools or run native test + // suite". forge must not shell out to `npm test` / `go test ./...` / + // `pytest` itself here: on a large host project that is a multi-minute + // blocking run with its own output stream, and forge's procspawn timeout + // does not reliably reap a killed runner's orphaned worker processes on + // Windows (jest/vitest workers keep the stdout pipe open) — the shape + // behind "forge ship --agent-mode hangs for 15 min with zero output" + // reports where the pipeline reached qa-verify without pausing. Emit an + // advisory instead and let the host agent run and report the suite. + if pipe != nil && pipe.Bridge() != nil { + cp.Status = "warning" + cp.Detail = "QA-Verify (agent mode): native test suite not run by forge — " + + "the host agent runs and reports it (4-stage testing pipeline, stage 1/2)" + if auditRes.SpecFound && len(auditRes.Gaps) > 0 { + cp.Detail += fmt.Sprintf("; %d spec audit warning(s)", len(auditRes.Gaps)) + } + return cp + } + cp.Status, cp.Detail = runQATestSuite(root) // Append spec audit warnings to detail regardless of runner.