feat(review): failures and live multi-agent progress more reliable

cmd/entire/cli/review/cmd.go

@@ -403,6 +403,7 @@ func runSingleAgentPath(
 	runCtx, cancelRun := context.WithCancel(ctx)
 	defer cancelRun()
 
+	runCfg.EnrichSummary = reviewSummaryTokenEnricher(worktreeRoot, headSHA)
 	canPrompt := interactive.CanPromptInteractively()
 	sinks := composeSingleAgentSinks(singleAgentSinkInputs{
 		out:       out,
@@ -587,7 +588,14 @@ func runMultiAgentPath(
 		defer tuiSink.Wait()
 	}
 
-	summary, waitErr := RunMulti(runCtx, reviewers, reviewtypes.RunConfig{}, sinks)
+	// Multi-agent only wires EnrichAgentRun. The per-agent enricher emits a
+	// synthetic Tokens event as each agent finishes, which the dispatch loop
+	// overwrites onto st.tokens (run_multi.go:168). That value flows into
+	// agentRuns[i].Tokens in the final summary, so a summary-level pass would
+	// redo the same store.List + token hydration once per run.
+	summary, waitErr := RunMulti(runCtx, reviewers, reviewtypes.RunConfig{
+		EnrichAgentRun: reviewAgentRunTokenEnricher(worktreeRoot, headSHA),
+	}, sinks)
 	writePostReviewManifest(ctx, out, worktreeRoot, headSHA, summary, aggregateOutput)
 	if waitErr != nil && runCtx.Err() == nil && ctx.Err() == nil {
 		return fmt.Errorf("review run: %w", waitErr)
@@ -716,6 +724,28 @@ func warnManifestNotWritten(out io.Writer, reason string) {
 	fmt.Fprintln(out, "  Re-run with `ENTIRE_LOG_LEVEL=debug` for diagnostic detail.")
 }
 
+func reviewSummaryTokenEnricher(worktreeRoot, headSHA string) func(context.Context, reviewtypes.RunSummary) reviewtypes.RunSummary {
+	return func(ctx context.Context, summary reviewtypes.RunSummary) reviewtypes.RunSummary {
+		enriched, err := hydrateReviewSummaryTokensFromCurrentState(ctx, worktreeRoot, headSHA, summary, agent.GetByAgentType)
+		if err != nil {
+			logging.Debug(ctx, "review token hydration skipped", slog.String("error", err.Error()))
+			return summary
+		}
+		return enriched
+	}
+}
+
+func reviewAgentRunTokenEnricher(worktreeRoot, headSHA string) func(context.Context, reviewtypes.AgentRun) reviewtypes.AgentRun {
+	return func(ctx context.Context, run reviewtypes.AgentRun) reviewtypes.AgentRun {
+		enriched, err := hydrateReviewAgentRunTokensFromCurrentState(ctx, worktreeRoot, headSHA, run, agent.GetByAgentType)
+		if err != nil {
+			logging.Debug(ctx, "review agent token hydration skipped", slog.String("error", err.Error()))
+			return run
+		}
+		return enriched
+	}
+}
+
 func composeSingleAgentSinks(in singleAgentSinkInputs) []reviewtypes.Sink {
 	if !in.isTTY || !in.canPrompt {
 		fmt.Fprintf(in.out, "Running review with %s...\n", in.agentName)

cmd/entire/cli/review/dump.go

@@ -12,6 +12,7 @@
 package review
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"strings"
@@ -63,15 +64,16 @@ func (s DumpSink) dumpAgent(run reviewtypes.AgentRun) {
 		// agent-level RunError events the parser emitted (typically a torn
 		// stdout stream — caught at the orchestrator level by classifyStatus
 		// even when the process itself exited 0).
-		if run.Err != nil {
-			fmt.Fprintf(&b, "**Failed:** `%v`\n\n", run.Err)
-		} else {
-			b.WriteString("**Failed**\n\n")
-		}
+		writeFailureHeader(&b, run.Err)
 		for _, ev := range run.Buffer {
-			if re, ok := ev.(reviewtypes.RunError); ok && re.Err != nil {
-				fmt.Fprintf(&b, "> agent error: `%v`\n\n", re.Err)
+			re, ok := ev.(reviewtypes.RunError)
+			if !ok || re.Err == nil {
+				continue
+			}
+			if sameFailureError(re.Err, run.Err) {
+				continue
 			}
+			fmt.Fprintf(&b, "> agent error: `%v`\n\n", re.Err)
 		}
 		// Render any narrative text the agent produced before the failure
 		// surfaced — useful when the parser tore mid-response so reviewers
@@ -97,6 +99,52 @@ func (s DumpSink) dumpAgent(run reviewtypes.AgentRun) {
 	fmt.Fprint(s.W, rendered)
 }
 
+func writeFailureHeader(b *strings.Builder, runErr error) {
+	if runErr == nil {
+		b.WriteString("**Failed**\n\n")
+		return
+	}
+	var pe *reviewtypes.ProcessError
+	if errors.As(runErr, &pe) && pe.Stderr != "" {
+		fmt.Fprintf(b, "**Failed:** `%s` exited (`%v`). Stderr:\n\n", pe.AgentName, pe.Err)
+		fence := codeFenceFor(pe.Stderr)
+		fmt.Fprintf(b, "%s\n%s\n%s\n\n", fence, pe.Stderr, fence)
+		return
+	}
+	fmt.Fprintf(b, "**Failed:** `%v`\n\n", runErr)
+}
+
+// codeFenceFor returns a backtick fence at least 3 long and at least one
+// longer than the longest backtick run in s — per CommonMark §4.5, the
+// closing fence must match or exceed the opening fence length, so this
+// prevents stderr content with embedded ``` lines from terminating the
+// fence early and rendering trailing content raw.
+func codeFenceFor(s string) string {
+	longest, current := 0, 0
+	for _, r := range s {
+		if r == '`' {
+			current++
+			if current > longest {
+				longest = current
+			}
+			continue
+		}
+		current = 0
+	}
+	n := longest + 1
+	if n < 3 {
+		n = 3
+	}
+	return strings.Repeat("`", n)
+}
+
+func sameFailureError(a, b error) bool {
+	if a == nil || b == nil {
+		return false
+	}
+	return errors.Is(a, b) || errors.Is(b, a)
+}
+
 // joinAssistantText extracts AssistantText events from a buffer and joins
 // them with newlines, trimming the result to keep dump output tight.
 func joinAssistantText(buf []reviewtypes.Event) string {

cmd/entire/cli/review/dump_test.go

@@ -91,6 +91,97 @@ func TestDumpSink_FailedAgentNoErr(t *testing.T) {
 	}
 }
 
+func TestDumpSink_FailedAgentWithProcessErrorRendersStderrAsCodeFence(t *testing.T) {
+	t.Parallel()
+	var buf bytes.Buffer
+	sink := DumpSink{W: &buf}
+
+	stderr := "Error: API key invalid\nPlease set ANTHROPIC_API_KEY\nHint: see /docs/auth"
+	pe := &reviewtypes.ProcessError{
+		AgentName: "claude-code",
+		Err:       errors.New("exit status 1"),
+		Stderr:    stderr,
+	}
+	run := reviewtypes.AgentRun{
+		Name:   "claude-code",
+		Status: reviewtypes.AgentStatusFailed,
+		Err:    pe,
+	}
+	sink.RunFinished(makeSummary(run))
+
+	out := buf.String()
+	if !strings.Contains(out, "exit status 1") {
+		t.Errorf("expected exit status mention in failure header, got:\n%s", out)
+	}
+	for _, line := range []string{
+		"Error: API key invalid",
+		"Please set ANTHROPIC_API_KEY",
+		"Hint: see /docs/auth",
+	} {
+		if !strings.Contains(out, line) {
+			t.Errorf("expected stderr line %q in output, got:\n%s", line, out)
+		}
+	}
+	if !strings.Contains(out, "```\n"+stderr+"\n```") {
+		t.Errorf("expected stderr in fenced code block, got:\n%s", out)
+	}
+	if strings.Contains(out, "**Failed:** `claude-code: exit status 1: stderr:") {
+		t.Errorf("stderr must not be jammed into the inline failure header, got:\n%s", out)
+	}
+}
+
+func TestDumpSink_DoesNotDoublePrintSyntheticRunErrorMatchingRunErr(t *testing.T) {
+	t.Parallel()
+	var buf bytes.Buffer
+	sink := DumpSink{W: &buf}
+
+	waitErr := errors.New("exit status 1")
+	run := reviewtypes.AgentRun{
+		Name:   "claude-code",
+		Status: reviewtypes.AgentStatusFailed,
+		Err:    waitErr,
+		Buffer: []reviewtypes.Event{
+			reviewtypes.Started{},
+			reviewtypes.Finished{Success: true},
+			reviewtypes.RunError{Err: waitErr},
+		},
+	}
+	sink.RunFinished(makeSummary(run))
+
+	out := buf.String()
+	if strings.Contains(out, "> agent error:") {
+		t.Errorf("synthetic RunError matching run.Err must not produce a blockquote, got:\n%s", out)
+	}
+	if !strings.Contains(out, "**Failed:**") {
+		t.Errorf("expected failure header, got:\n%s", out)
+	}
+}
+
+func TestDumpSink_DoesNotDoublePrintRunErrorWrappedByRunErr(t *testing.T) {
+	t.Parallel()
+	var buf bytes.Buffer
+	sink := DumpSink{W: &buf}
+
+	streamErr := errors.New("torn stdout stream")
+	run := reviewtypes.AgentRun{
+		Name:   "claude-code",
+		Status: reviewtypes.AgentStatusFailed,
+		Err:    agentRunFailureError("claude-code", streamErr),
+		Buffer: []reviewtypes.Event{
+			reviewtypes.RunError{Err: streamErr},
+		},
+	}
+	sink.RunFinished(makeSummary(run))
+
+	out := buf.String()
+	if strings.Contains(out, "> agent error:") {
+		t.Errorf("RunError wrapped by run.Err must not be printed again, got:\n%s", out)
+	}
+	if !strings.Contains(out, "review agent claude-code reported failure: torn stdout stream") {
+		t.Errorf("expected wrapped failure header, got:\n%s", out)
+	}
+}
+
 func TestDumpSink_FailedAgentRunErrorEvent(t *testing.T) {
 	t.Parallel()
 	var buf bytes.Buffer
@@ -186,3 +277,61 @@ func TestDumpSink_EmptyAgentRuns(t *testing.T) {
 		t.Errorf("expected empty counts line, got:\n%s", out)
 	}
 }
+
+// TestDumpSink_FenceEscapesBackticksInStderr verifies that stderr containing
+// a 3-backtick line does not terminate the surrounding code fence early.
+// Per CommonMark §4.5 the closing fence must be at least as long as the
+// opening fence, so the fence has to widen to one more backtick than the
+// longest run in the content.
+func TestDumpSink_FenceEscapesBackticksInStderr(t *testing.T) {
+	t.Parallel()
+	var buf bytes.Buffer
+	sink := DumpSink{W: &buf}
+
+	stderr := "before\n```\ninner content\n```\nafter"
+	pe := &reviewtypes.ProcessError{
+		AgentName: "claude-code",
+		Err:       errors.New("exit status 1"),
+		Stderr:    stderr,
+	}
+	sink.RunFinished(makeSummary(reviewtypes.AgentRun{
+		Name:   "claude-code",
+		Status: reviewtypes.AgentStatusFailed,
+		Err:    pe,
+	}))
+
+	out := buf.String()
+	// Fence must widen to 4 backticks, with the full stderr (including the
+	// embedded 3-backtick lines) sitting verbatim inside.
+	wantFence := "````\n" + stderr + "\n````"
+	if !strings.Contains(out, wantFence) {
+		t.Errorf("expected widened fence around stderr, got:\n%s", out)
+	}
+	// "after" must still be inside the fence (i.e., immediately followed by
+	// the closing fence), not orphaned outside.
+	if !strings.Contains(out, "after\n````") {
+		t.Errorf("trailing stderr content must remain inside the fence, got:\n%s", out)
+	}
+}
+
+func TestCodeFenceFor_MinimumThreeBackticks(t *testing.T) {
+	t.Parallel()
+	cases := []struct {
+		name, in, want string
+	}{
+		{"empty", "", "```"},
+		{"no backticks", "hello world", "```"},
+		{"single backtick", "use `x` here", "```"},
+		{"two backticks", "matched ``code`` style", "```"},
+		{"three backticks on a line", "```\nfenced\n```", "````"},
+		{"four backticks", "````", "`````"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			if got := codeFenceFor(tc.in); got != tc.want {
+				t.Errorf("codeFenceFor(%q) = %q, want %q", tc.in, got, tc.want)
+			}
+		})
+	}
+}