From e4d12068aa3d20d6579b83edce829f25161b03f7 Mon Sep 17 00:00:00 2001 From: Nguyen Quang Trung Date: Sun, 6 Sep 2026 21:20:16 +0700 Subject: [PATCH] fix(ship): agent-mode qa-verify no longer runs the host test suite; arch debate runs serial MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two agent-mode hang contributors, both surfacing as "forge ship --agent-mode hangs for minutes with zero output" (FORGE_SHIP_ISSUES_2026-09-04 ISSUE 5, still reproducible on 1.10.4 after the mutex fix): - qa-verify Phase 2 called runQATestSuite unconditionally, shelling out to `npm test` / `go test ./...` / `pytest` on the host repo with no agent-mode guard. On a large project that is a multi-minute blocking run with its own output stream, and procspawn's timeout does not reliably reap orphaned jest/vitest workers on Windows (they keep the stdout pipe open past the parent kill, so the read never sees EOF). When the pipeline reached qa-verify without pausing — e.g. checkArch short-circuiting on an already-present arch.md from an earlier answered turn — `forge ship --agent-mode` silently ran the caller's whole suite. In agent mode the host agent is the QA agent, so qa-verify now emits an advisory and lets the host run + report the suite. Non-agent-mode behaviour is unchanged. - runParallelArchDebate still fanned out one goroutine per reviewer role against the bridge. The bridge can only surface one pending turn per run, so the other five do throwaway work (redone on replay) while serialising behind Bridge.mu as the first holds it across savePending's file I/O. Agent mode now runs the debate sequentially, stopping at the first owed turn, matching RunWithOptions's existing `serial` intent. Parallel path kept for real-provider runs. New test TestRunParallelArchDebate_AgentModeSerialNoFanOut. Full `go test ./...` green; gofmt + go vet clean. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 5 ++ internal/cli/cmdship/arch.go | 71 ++++++++++++++++-------- internal/cli/cmdship/rfc005_p1p2_test.go | 55 ++++++++++++++++++ internal/cli/cmdship/ship.go | 20 +++++++ 4 files changed, 129 insertions(+), 22 deletions(-) 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.