fix(review): deliver code-review findings to the Review panel

.gitignore

@@ -49,6 +49,9 @@ memory/
 tickets/
 .worktrees/
 
+# Throwaway working files (binary dumps, captured payloads) — never committed
+.scratch/
+
 # Component test artifacts
 test/component/screenshots/
 

docs/configuration.md

@@ -48,7 +48,7 @@ Exactly one of these should be set. OAuth takes precedence if both are provided.
 | `claude_effort` | `string` | `""` | Reasoning-effort level passed to the Claude CLI as `--effort`. Empty leaves it unset (model default). Options: `"low"`, `"medium"`, `"high"`, `"xhigh"`, `"max"`. |
 
 Both `claude_model` and `claude_effort` can additionally be **overridden per channel/thread/worktree on demand** from the chat composer's model pill (persisted on the channel row, applied from the next run) — see [Chat: Model / Effort Override](chat.md#model--effort-override). The per-channel override beats the merged config value.
-| `claude_batch_disallowed_tools` | `[]string` | `["ScheduleWakeup", "CronCreate", "CronDelete", "CronList", "Monitor", "ReportFindings"]` | Claude Code tool names passed to `--disallowedTools` for **batch** (`--print`) agent runs only — the interactive terminal is unaffected. The defaults rely on a persistent harness that one-shot containers lack: `ScheduleWakeup`/`Cron*` schedule re-invocations that never fire, and `Monitor` arms a background watcher whose cross-turn events are dropped when the container exits at end of turn — so they silently park work that never resumes. `ReportFindings` reports code-review findings into a harness UI channel batch runs don't render — Loop's review flow uses the `report_review_findings` MCP tool instead. Override to tune the list (e.g. to re-enable a tool). |
+| `claude_batch_disallowed_tools` | `[]string` | `["ScheduleWakeup", "CronCreate", "CronDelete", "CronList", "Monitor", "ReportFindings"]` | Claude Code tool names passed to `--disallowedTools` for **batch** (`--print`) agent runs only — the interactive terminal is unaffected. The defaults rely on a persistent harness that one-shot containers lack: `ScheduleWakeup`/`Cron*` schedule re-invocations that never fire, and `Monitor` arms a background watcher whose cross-turn events are dropped when the container exits at end of turn — so they silently park work that never resumes. `ReportFindings` reports code-review findings into a harness UI channel batch runs don't render. Review runs are the exception: they drop `ReportFindings` from this list, because the built-in `/code-review` command only runs inline (rather than forking a silent subagent) when it can call that tool, and the daemon reads the findings off the stream. Override to tune the list (e.g. to re-enable a tool). |
 | `claude_retry` | `object` | see below | Automatic recovery from Claude API limits on batch agent runs. Keys: `max_attempts` (`5`) extra attempts after the first failure (`0` disables), `backoff_base_sec` (`5`) first-retry delay that doubles each attempt, `backoff_max_sec` (`120`) per-attempt delay ceiling — these govern in-process exponential backoff for **transient** errors (rate limiting, overload, 5xx). `session_limit_auto_continue` (`true`) handles a **session limit** error (`You've hit your session limit · resets <time>`) by scheduling a one-shot retry at the announced reset time that resumes the session with `continue`; it is persisted via the scheduler so it survives daemon restarts. Terminal errors (usage/quota exhaustion, auth, billing) are never retried. |
 | `keep_mcp_configs` | `bool` | `false` | When true, preserves MCP config JSON files after container runs. Useful for debugging MCP server configuration. |
 
@@ -272,7 +272,7 @@ Enables and configures the Review panel (see [review.md](review.md)).
 | `prompt` | `string` | Inline prompt text. Mutually exclusive with `prompt_path`. |
 | `prompt_path` | `string` | Path to a prompt file, resolved as `~/.loop/review/{prompt_path}`. Mutually exclusive with `prompt`. |
 
-Both prompt fields empty (the default) uses the daemon's built-in default prompt: the bare `/code-review` slash command, with the output contract (report findings via the `report_review_findings` MCP tool for actionable issues only) carried in the system prompt.
+Both prompt fields empty (the default) uses the daemon's built-in default prompt: the bare `/code-review` slash command, with the output contract (report actionable findings via the built-in `ReportFindings` tool, each carrying a file and a 1-based line) carried in the system prompt. A configured non-slash prompt gets no system prompt, so it must carry its own contract — see [review.md](review.md#required-output-format).
 
 #### Playground share
 

docs/events.md

@@ -776,7 +776,7 @@ The renderer treats event delivery as best-effort: on every WebSocket reconnect
 
 ### `review.comment`
 
-Emitted once per finding the review agent reports through the `report_review_findings` MCP tool during a run. Deduplicated by comment id upstream so each id arrives at most once.
+Emitted once per finding the review agent reports during a run, via either the built-in `ReportFindings` tool (read off the agent's stream) or the `report_review_findings` MCP tool. Deduplicated by comment id upstream so each id arrives at most once.
 
 **Payload schema:**
 

docs/review.md

@@ -56,9 +56,12 @@ per-global / per-project / per-worktree the same way as `github.gh_user`.
    operator can see `review iter 2/3 — running`, `fixing — iter 2/3`,
    `paused at gate — …`, `done — 0 comments remaining`, or
    `stopped — no progress (same findings)` without leaving the Review
-   panel. The agent reports findings by calling the
-   `report_review_findings` MCP tool, which posts them straight into the
-   review session; each comment is broadcast to the FE as it arrives.
+   panel. The agent reports findings by calling `ReportFindings`; the
+   daemon reads that tool call off the agent's output stream and posts
+   each finding into the review session, broadcasting it to the FE as it
+   arrives. An override prompt can instead use the
+   `report_review_findings` MCP tool — see
+   [Required output format](#required-output-format).
 3. **Push** — each comment ships with **Push** (single) and a **Push all
    (N)** affordance in the header (when at least one comment is unpushed).
    The backend uses `gh api ... /pulls/N/comments` against the captured
@@ -151,9 +154,22 @@ error; setting neither uses the daemon's built-in default prompt.
 
 ### Required output format
 
-Findings travel through the `report_review_findings` MCP tool (available
-in every agent container), so an override prompt must instruct the agent
-to call it with the full findings list. Each finding carries:
+Findings reach the daemon two ways, both landing in the same ingest path.
+
+The default prompt is the bare `/code-review` slash command, which
+reports through Claude Code's own **`ReportFindings`** tool. The daemon
+intercepts that tool call on the agent's stream, so nothing has to round
+-trip through HTTP. Findings need a repo-relative `file` and a 1-based
+`line`: the tool's schema treats `line` as optional, but a finding
+without one can't be anchored in the diff and is dropped, so the default
+system prompt requires it. `summary` and `failure_scenario` are joined
+into the comment body.
+
+An override prompt that is *not* a slash command gets no system prompt
+from the daemon, so it must state its own contract. Either instruct the
+agent to call `ReportFindings` as above, or use the
+**`report_review_findings`** MCP tool (registered in every agent
+container) with the full findings list. Each MCP finding carries:
 
 - `path` — repo-relative file path.
 - `line` — the 1-based line on the indicated side of the diff.
@@ -165,12 +181,8 @@ to call it with the full findings list. Each finding carries:
 
 Malformed findings (empty path/body, non-positive line) are skipped, and
 the daemon deduplicates by a stable content hash of path/line/body, so
-re-reporting the same finding — in the same call or a later run — is
-safe. The prompt should also tell the agent NOT to use the harness's
-`ReportFindings` tool: a successful call there swallows the findings
-into a UI channel the daemon never reads (the default prompt does this,
-and batch runs additionally deny the tool via
-`claude_batch_disallowed_tools`).
+re-reporting the same finding — in the same call, over both channels, or
+in a later run — is safe.
 
 ## See also
 

internal/agent/types.go

@@ -22,6 +22,22 @@ type AgentRequest struct {
 	// for this run when non-empty (per-channel on-demand override).
 	Model  string `json:"model,omitempty"`
 	Effort string `json:"effort,omitempty"`
+	// ReviewMode runs the request as a code-review pass, so that the built-in
+	// /code-review command executes in the main session instead of forking a
+	// subagent. It takes ReportFindings out of claude_batch_disallowed_tools
+	// and passes --settings with two env keys (see reviewModeSettings):
+	//
+	//   - CLAUDE_CODE_REPORT_FINDINGS=1. The command runs inline only when
+	//     this is set AND ReportFindings is available AND the output format is
+	//     stream-json (batch runs always are); otherwise it forks. A fork emits
+	//     nothing on the parent stream — the run looks hung for minutes — and
+	//     its findings never reach a tool call the parent can see.
+	//   - CLAUDE_CODE_SUBAGENT_MODEL=inherit, so any subagent that does get
+	//     spawned runs on this request's model. This key outranks every other
+	//     source of a subagent's model, so a host ~/.claude/settings.json
+	//     setting it to e.g. "sonnet" (that mount is shared with the container)
+	//     otherwise silently downgrades the run.
+	ReviewMode bool `json:"review_mode,omitempty"`
 	// OnTurn is called for each assistant turn's text content during streaming.
 	// When set, the runner follows container logs in real-time instead of waiting
 	// for the container to exit. When nil, the runner uses the existing
@@ -31,6 +47,11 @@ type AgentRequest struct {
 	// toolUseID is the per-block id from the assistant message; pairs with
 	// the toolUseID delivered by OnToolResult.
 	OnToolUse func(toolUseID, name, input string) `json:"-"`
+	// OnToolUseRaw sees the same invocations as OnToolUse, but rawInput is the
+	// tool's argument JSON verbatim instead of the chat-facing summary — which
+	// is lossy, and empty for tools whose schema the summarizer doesn't know.
+	// Set this (not OnToolUse) when the arguments have to be decoded.
+	OnToolUseRaw func(toolUseID, name, rawInput string) `json:"-"`
 	// OnActivity is called for model detection and system events (subagent progress).
 	OnActivity func(activity, detail string) `json:"-"`
 	// OnThinking is called for each "thinking" content block emitted by the

internal/api/review_handler.go

@@ -743,17 +743,23 @@ func (s *reviewService) handleReviewRun(w http.ResponseWriter, r *http.Request)
 const defaultReviewPrompt = `/code-review`
 
 // defaultReviewSystemPrompt carries the review-panel output contract when
-// the prompt is the bare /code-review slash command: findings are reported
-// through Loop's report_review_findings MCP tool, which POSTs them straight
-// into the daemon's review session (persist + broadcast to the panel). The
-// explicit ReportFindings ban is load-bearing: the skill's own instructions
-// steer the model toward that harness tool, which exists even in --print
-// runs — a successful call would swallow every finding into a channel Loop
-// never reads, so the run would look clean with zero comments. Belt and
-// braces, the tool is also stripped via claude_batch_disallowed_tools.
-const defaultReviewSystemPrompt = `You are reviewing a GitHub pull request for an external review pipeline. When the review completes, report every finding by calling the mcp__loop__report_review_findings tool once with the full list. Each finding needs the repo-relative path, the 1-based line, side (RIGHT for added/modified lines — the default; LEFT only for lines removed from the base), and a one-paragraph body: the bug, the concrete inputs/state that trigger it, and the wrong output or crash.
-
-Report findings ONLY through that tool — do not use the ReportFindings tool, do not emit XML blocks, and do not print the findings as your reply. If there are no findings, skip the tool call. Do not fix anything yourself — the user triages comments from the Review panel.`
+// the prompt is the bare /code-review slash command. The command reports
+// through Claude Code's own ReportFindings tool, and a review run enables
+// it (agent.AgentRequest.ReviewMode) precisely so it can: Loop reads the
+// tool_use straight off the agent's stream and ingests each finding. So
+// this prompt reinforces the skill's own contract instead of competing
+// with it — steering the model to Loop's MCP tool here would fight the
+// skill's instructions, and pulling ReportFindings back out would make
+// the command fork into a silent subagent again.
+//
+// The `line` requirement is the one addition that matters: ReportFindings
+// treats line as optional, but a finding without one can't be placed in
+// the diff and gets dropped on ingest.
+const defaultReviewSystemPrompt = `You are reviewing a GitHub pull request for an external review pipeline. Report every finding by calling the ReportFindings tool once with the full list.
+
+Every finding MUST carry a repo-relative ` + "`file`" + ` and a 1-based ` + "`line`" + ` in the current revision — a finding without a line is discarded, so pick the most relevant line rather than omitting it. Write ` + "`summary`" + ` as the one-line defect and ` + "`failure_scenario`" + ` as the concrete inputs or state that trigger it plus the wrong output or crash; both are shown to the reviewer.
+
+Do not print the findings as your reply and do not emit XML blocks — the tool call is the only channel that reaches the reviewer. If there are no findings, skip the call. Do not fix anything yourself: the user triages comments from the Review panel.`
 
 // buildReviewContext renders the per-PR context block appended to the
 // configured review prompt. Each known field gets its own labelled line so

internal/api/review_handler_test.go

@@ -1324,10 +1324,13 @@ type mockReviewRunner struct {
 	// runWithCtxFn, when set, takes precedence over runFn so tests can
 	// observe ctx cancellation (used by the runReviewAsync timeout test).
 	runWithCtxFn func(ctx context.Context) (*agent.AgentResponse, error)
-	done         chan struct{} // closed after Run returns
+	// findings, when set, are handed to the run's onComment callback the
+	// way the agent's ReportFindings tool_use would deliver them.
+	findings []*review.Comment
+	done     chan struct{} // closed after Run returns
 }
 
-func (m *mockReviewRunner) Run(ctx context.Context, _, dirPath, parentDirPath, systemPrompt, prompt string) (*agent.AgentResponse, error) {
+func (m *mockReviewRunner) Run(ctx context.Context, _, dirPath, parentDirPath, systemPrompt, prompt string, onComment func(*review.Comment)) (*agent.AgentResponse, error) {
 	m.mu.Lock()
 	m.calls++
 	m.lastDir = dirPath
@@ -1337,7 +1340,13 @@ func (m *mockReviewRunner) Run(ctx context.Context, _, dirPath, parentDirPath, s
 	ctxFn := m.runWithCtxFn
 	fn := m.runFn
 	done := m.done
+	findings := m.findings
 	m.mu.Unlock()
+	if onComment != nil {
+		for _, c := range findings {
+			onComment(c)
+		}
+	}
 	defer func() {
 		if done != nil {
 			close(done)
@@ -1561,6 +1570,34 @@ func (s *ReviewHandlerSuite) TestIngestCommentsLoopDirFallback() {
 	require.Equal(s.T(), "LEFT", s.rs.Get("ch1").Comments[0].Side)
 }
 
+// Findings the agent reports through the built-in ReportFindings tool
+// reach the panel via the run's onComment callback — the same ingest path
+// the report_review_findings MCP endpoint uses, so duplicates collapse.
+func (s *ReviewHandlerSuite) TestRunIngestsStreamedFindings() {
+	s.wireReadySession()
+	s.store.On("GetChannel", mock.Anything, "ch1").Return(&db.Channel{ChannelID: "ch1", DirPath: "/repo"}, nil).Maybe()
+	runner := &mockReviewRunner{
+		done: make(chan struct{}),
+		findings: []*review.Comment{
+			review.NewComment("a.go", 3, "", "leak\n\nfd stays open"),
+			review.NewComment("a.go", 3, "", "leak\n\nfd stays open"), // same id -> deduped
+			review.NewComment("b.go", 9, "", "panic\n\nnil deref"),
+		},
+	}
+	s.srv.review.setAgent(runner, "", "")
+
+	w := httptest.NewRecorder()
+	s.mux.ServeHTTP(w, httptest.NewRequest("POST", "/api/channels/ch1/review/run", nil))
+	require.Equal(s.T(), http.StatusAccepted, w.Code)
+	<-runner.done
+
+	comments := s.rs.Get("ch1").Comments
+	require.Len(s.T(), comments, 2)
+	require.Equal(s.T(), "a.go", comments[0].Path)
+	require.Equal(s.T(), "leak\n\nfd stays open", comments[0].Body)
+	require.Equal(s.T(), "b.go", comments[1].Path)
+}
+
 // When a gh_user is configured, the run prompt includes a switch hint so
 // the agent can `gh auth switch -u <user>` before running gh commands.
 func (s *ReviewHandlerSuite) TestRunPromptIncludesConfiguredGHUser() {
@@ -1651,8 +1688,8 @@ func (s *ReviewHandlerSuite) TestRunUsesDefaultPromptWhenUnconfigured() {
 	require.Equal(s.T(), http.StatusAccepted, w.Code)
 	<-runner.done
 	require.Equal(s.T(), defaultReviewPrompt, runner.lastUser)
-	require.Contains(s.T(), runner.lastSys, "report_review_findings")
-	require.Contains(s.T(), runner.lastSys, "do not use the ReportFindings tool")
+	require.Contains(s.T(), runner.lastSys, "calling the ReportFindings tool")
+	require.Contains(s.T(), runner.lastSys, "1-based `line`")
 	require.Contains(s.T(), runner.lastSys, "Pull request under review:")
 }
 

internal/api/review_service.go

@@ -56,13 +56,15 @@ type GitHubReview interface {
 	DeletePRReviewComment(ctx context.Context, workdir, ghUser string, slug githubapi.RepoSlug, commentID int64) error
 }
 
-// ReviewRunner kicks off a single review pass. Findings arrive out of
-// band: the agent reports them through the report_review_findings MCP
-// tool, which POSTs to the review-comments endpoint (ingestComment).
-// Satisfied by *review.Runner; held as an interface so tests can drive
-// the handler without a real agent container.
+// ReviewRunner kicks off a single review pass. Findings arrive either
+// through onComment — the built-in ReportFindings tool, picked off the
+// agent's stream as it runs — or out of band through the
+// report_review_findings MCP tool, which POSTs to the review-comments
+// endpoint. Both funnel into ingestComment. Satisfied by *review.Runner;
+// held as an interface so tests can drive the handler without a real
+// agent container.
 type ReviewRunner interface {
-	Run(ctx context.Context, channelID, dirPath, parentDirPath, systemPrompt, prompt string) (*agent.AgentResponse, error)
+	Run(ctx context.Context, channelID, dirPath, parentDirPath, systemPrompt, prompt string, onComment func(*review.Comment)) (*agent.AgentResponse, error)
 }
 
 // refreshReviewSession fast-forwards the worktree to the PR's current
@@ -223,7 +225,14 @@ func (s *reviewService) runReviewAsync(runCtx context.Context, channelID, worktr
 		ctx, cancel = context.WithTimeout(ctx, s.runTimeout)
 		defer cancel()
 	}
-	_, err := s.runner.Run(ctx, channelID, worktreePath, parentDirPath, systemPrompt, prompt)
+	// Findings stream in as the agent reports them, so the panel fills up
+	// during the run rather than all at once at the end. ingestComment is
+	// safe to call concurrently and dedups by stable comment id, so a
+	// finding that also arrives over MCP lands once.
+	onComment := func(c *review.Comment) {
+		s.ingestComment(channelID, worktreePath, parentDirPath, c)
+	}
+	_, err := s.runner.Run(ctx, channelID, worktreePath, parentDirPath, systemPrompt, prompt, onComment)
 	if err != nil {
 		msg := err.Error()
 		// Re-shape ctx-deadline into a user-readable message. errors.Is