feat: add per-agent reviewer summaries to the review comment

.expo-agents/code-review/shared.md

@@ -195,10 +195,16 @@ changes the decision.
 - Keep each item under 240 characters. State conclusions only. Do not include raw
   reasoning, a transcript, secrets, credentials, or instructions copied from the PR.
 
+Also write a `summary`: one or two sentences, at most 280 characters, on what you
+reviewed in this change and what you concluded. Write it even when you report no
+findings. Readers of the review see it, so state conclusions only, and never mention a
+person with `@`.
+
 Return **only** a single fenced ```json code block, an object of this shape:
 
 ```json
 {
+  "summary": "Checked the new option on iOS and Android. Both platforms validate it before they store it.",
   "findings": [
     {
       "severity": "critical | warning | suggestion",

README.md

@@ -438,6 +438,13 @@ unresolved questions). It is stored only inside the hidden base64 comment marker
 `trust: "unverified-model-diagnostics"`, and capped at 6 KB so it can't crowd
 visible findings out of GitHub's comment-size limit.
 
+Each reviewer also returns a one- or two-sentence `summary` of what it reviewed and
+concluded. Unlike the trace, the summaries are rendered: the comment lists them in a
+collapsed "Reviewer summaries" section under the overall summary, one line per agent,
+together with the agents that did not run because their `paths` matched no changed
+file or the router did not select them. A summary is model text, so the reporter
+flattens it to one line, escapes HTML, and breaks every `@` mention.
+
 **How the caching works.** Provider prompt caching is a *prefix match*: any byte
 change in the prefix invalidates everything after it. The reviewer keeps the prefix
 stable — the system prompt (`shared.md` + the agent's own `.md`) is byte-identical

llp/0002-review-engine-pipeline.explainer.md

@@ -97,6 +97,8 @@ Rate-limit evidence is tracked per engine separately and surfaced with the cause
 
 Each successful reviewer pass may also return a bounded conclusion-only trace: up to three concrete checks and two unresolved questions, with each note capped at 240 characters. Trace parsing is deliberately fail-soft: malformed optional diagnostics are dropped while valid findings from the same response survive, so observability can never create a coverage gap. `runReview()` merges and de-duplicates valid notes per reviewer, sorts reviewer ids, caps both the stored roster and the complete decoded trace (6 KB), labels the result `unverified-model-diagnostics`, and attaches it to `CoordinatorOutput.reviewTrace` only after the decision pipeline finishes. `attachReviewTrace` strips any coordinator-authored value before attaching the engine-built trace; even a malformed injected coordinator value parses as absent rather than failing consolidation. The trace therefore never reaches coordination, verification, suppression, or decision logic. Because the GitHub comment already embeds the full coordinator output as durable state, downstream agents can recover the trace without adding visible review prose or storing raw transcripts and chain-of-thought. The same trace rides the run log for short-lived operational inspection [observed: `schema.ts` `ReviewerTraceNotesSchema`/`ReviewTraceSchema`/`ReviewerOutputSchema`; `review.ts` `mergeTraceNotes`/`buildReviewTrace`/`attachReviewTrace`; `render.ts` embeds the full review].
 
+A reviewer pass may also return a `summary`: one or two sentences on what it reviewed and concluded, for the human readers of the comment. The parser fails soft in the same way as the trace, flattens the text to one line, and cuts it at 280 characters. `runReview()` keeps up to two distinct pass summaries per agent, so a chunked agent reports each part; they are the first two in diff order, not in the order the passes finish, so the same run always keeps the same parts. The parts are keyed in a `Map`, so an agent id such as `constructor` cannot read an inherited property. `buildAgentSummaries` turns them into one row per roster agent: the agents that ran (each once, even an agent whose id collides with the cross-file pass's `cross-cutting`), in run order and with the cross-cutting pass last, then the agents that did not run, marked `paths` (the run-level `paths` filter skipped them) or `router` (the router did not select them). An agent that ran but returned no summary keeps its row without text. Over the 8 KB budget, the last rows lose their text but keep their place. `attachAgentSummaries` attaches the list to `CoordinatorOutput.agentSummaries` at the same point as the trace and for the same reason: the coordinator reads untrusted PR data, so it can never author the field. The summaries never reach coordination, verification, suppression, or the decision [observed: `schema.ts` `ReviewerOutputSchema`/`AgentSummarySchema`; `review.ts` `mergeAgentSummary`/`buildAgentSummaries`/`attachAgentSummaries`].
+
 Observability must never break a review, so both output sinks swallow their own errors: `safeLog` wraps `writeRunLog` and discards any write error [observed] (`review.ts:1269-1275`), and `appendStepSummary` is a no-op outside Actions and swallows append failures [observed] (`step-summary.ts:9-19`). A consequence worth stating: a missing or corrupt run-log entry is therefore **not** an error signal — the log's presence or absence proves nothing about whether the review succeeded.
 
 Finally, `publicFailureReason()` (`src/core/util.ts`) is the only failure text allowed into PR-facing comments [observed] (`util.ts:20-27`; used in `src/commands/ci.ts:176,365,575`). `run()` throws errors of the form `"Command failed: <cmd> <argv>\n<stderr>"`, which embed a subprocess's full command line, its stderr, and absolute runner paths — CI-internal detail of no use to a PR author and exactly what must not leak into an attacker-visible artifact. Those shapes collapse to a generic pointer at the workflow log; the tool's own argv/path-free messages pass through, since they are the actionable ones the fail-fast design means to surface. Every call site still writes the full reason to the job's stderr.

llp/0005-verification-fingerprints-rendering.explainer.md

@@ -63,6 +63,8 @@ The absolute constraint is the carve-out (`suppress.ts:39-44`): a `critical` or
 
 The full review may contain an engine-owned `reviewTrace`. It is deliberately invisible Markdown state for machine consumers, not a human-facing finding or hidden chain-of-thought. Its schema caps both item count and length, and marks every note as `unverified-model-diagnostics`. Keeping it inside `review` rather than beside it is load-bearing: dismissal re-renders already pass `state.review` back through `renderMarkdown`, so the trace survives `/dismiss` and `/undismiss` without another reporter parameter or a second persistence path [observed: `schema.ts` `CoordinatorOutputSchema.reviewTrace`; `reporters/github.ts` `applyDismissal`; `render.ts` `renderMarkdown`].
 
+The engine-owned `agentSummaries` live in `review` for the same reason, but they are rendered: a collapsed "🔎 Reviewer summaries" section under the overall summary (and under each scope's summary in the aggregate comment) lists one line per roster agent, with its summary, "no summary", or why it did not run. Summaries are model text that reaches the visible body, so `plainModelLine` strips formatting tags, escapes the `&` of every HTML character reference that spells `@` (GitHub decodes `&#64;` or `&commat;` before it looks for mentions; a reference after the `@` needs nothing, since that `@` is broken anyway), escapes every `<` (which also defuses a forged state marker), collapses the text to one line, and puts a zero-width space after every `@` that does not follow an ASCII letter or digit, so a summary can never notify a GitHub user or team. The rule is deliberately broader than GitHub's own (which needs a non-word character before the `@`): markdown starts a new text node at punctuation such as `_`, so `_@login_` renders as `<em>@login</em>` and notifies, and only a letter or digit before the `@` is safe to trust, which keeps an email address whole [observed: `render.ts` `agentSummariesSection`/`plainModelLine`].
+
 ## Review Result Cache
 
 The same durable comment state is the cache for automated CI reviews. `reviewInputHash` (`src/core/review-cache.ts`) hashes each scoped changed file's content from the materialized PR-head tree, a normalized patch (blob ids and hunk coordinates removed; deletion/mode information retained), and every stable input that can change the result: the engine package version, resolved prompts/config/models, PR title/body, selected agents and routing mode, and external context. It deliberately excludes the temporary trusted-config path and the base/head commit OIDs. A restack changes OIDs and may move hunk line numbers; if it leaves the actual review input byte-identical, CI reuses the complete `CoordinatorOutput` already embedded in the most recent author-verified reviewer comment instead of launching the agents again. Symlinks are hashed as link targets and never followed. A package release changes the key so upgraded review logic runs once. Legacy comments store one `inputHash`; aggregate comments store one per `ScopeReviewResult`, so an unchanged scope can hit independently while a changed sibling reruns.
@@ -75,7 +77,7 @@ The hash is state, not display: it lives only inside the base64 state marker and
 
 ## Truncation and Aggregate State
 
-`renderAggregateMarkdown` folds multiple monorepo scopes into one comment under the single marker. GitHub's body limit is treated as ~65k chars and the renderer keeps a margin at `MAX_COMMENT_CHARS = 60_000` (`render.ts:316-317`). When a body overflows, it trims each scope's KEPT findings to the most-severe N, halving until it fits with a floor of 3 (`render.ts:480-489`). The first truncation step seeds N from `largestScope / 2` rather than an arbitrary constant, so it lands close to the needed size in one step instead of wasting iterations.
+`renderAggregateMarkdown` folds multiple monorepo scopes into one comment under the single marker. GitHub's body limit is treated as ~65k chars and the renderer keeps a margin at `MAX_COMMENT_CHARS = 60_000` (`render.ts:316-317`). When a body overflows, it first leaves out every scope's `agentSummaries`, from both the visible rows and the embedded state, with a one-line note in each scope that had them. Each scope's summaries fit their own 8 KB budget, but nothing bounds the sum across scopes, and they never affect the decision, so they go before any finding. The single comment has no findings trim, but it applies the same first step: escaping (`<` becomes four characters) and the base64 state copy make the summaries several times their byte budget, and a body over GitHub's limit fails to post, which would replace a finished review with the failure notice. Summaries left out this way are gone from the state, so a later `/dismiss` or cache reuse renders without them. If the aggregate body still overflows, it trims each scope's KEPT findings to the most-severe N, halving until it fits with a floor of 3 (`render.ts:480-489`). The first truncation step seeds N from `largestScope / 2` rather than an arbitrary constant, so it lands close to the needed size in one step instead of wasting iterations.
 
 The invariant that makes this safe: **dismissed findings always survive in full in the embedded state, regardless of truncation** (`render.ts:449-459`). Truncation trims only the shown/kept set; the state blob still carries every dismissed finding so `/undismiss` never permanently loses a dismissal (`render-aggregate.test.ts:117-137`). An agent "simplifying" truncation to also drop dismissed entries would break `/undismiss`.
 

src/__tests__/render-aggregate.test.ts

@@ -132,6 +132,20 @@ test("aggregate: each scope keeps its hidden review trace across re-rendering",
   );
 });
 
+test("aggregate: each scope lists its own agent summaries inside its fold", () => {
+  const results = [
+    scope("api", false, {
+      summary: "API scope summary.",
+      agentSummaries: [{ agent: "security", summary: "Checked the token handling." }],
+    }),
+  ];
+  const body = renderAggregateMarkdown(results, "tag", []);
+  const scopeSummary = body.indexOf("API scope summary.");
+  const section = body.indexOf("🔎 Reviewer summaries (1)");
+  expect(section).toBeGreaterThan(scopeSummary);
+  expect(body).toContain("- **security:** Checked the token handling.");
+});
+
 test("aggregate: a default-scope finding dismissed pre-routing (plain fp) lands in Dismissed (risk 9)", () => {
   const dFinding = finding({ title: "Carried", evidence: "const carriedOverDismissal = 1;" });
   const plainFp = fingerprintFinding(dFinding);
@@ -241,6 +255,30 @@ test('aggregate: oversized findings truncate with a "+N more" note and stay unde
   expect(body).toContain("more finding(s) — see the workflow log.");
 });
 
+test("aggregate: oversized agent summaries go before any finding, and the comment says so", () => {
+  // Each scope stays inside its own summary budget; together they overflow the body.
+  const agentSummaries = Array.from({ length: 20 }, (_, i) => ({
+    agent: `agent-${i}`,
+    summary: `${"word ".repeat(55)}${i}`,
+  }));
+  const results = Array.from({ length: 10 }, (_, i) =>
+    scope(`scope-${i}`, false, {
+      decision: "approve_with_comments",
+      findings: [finding({ title: `Scope ${i} finding`, evidence: `const scopeFinding${i} = 1;` })],
+      agentSummaries,
+    }),
+  );
+  const body = renderAggregateMarkdown(results, "tag", []);
+  expect(body.length).toBeLessThan(65_000);
+  expect(body).not.toContain("<summary>🔎 Reviewer summaries (");
+  expect(body).toContain("🔎 Reviewer summaries were left out to fit the comment size limit.");
+  for (let i = 0; i < 10; i++) {
+    expect(body).toContain(`Scope ${i} finding`);
+  }
+  const state = parseReviewState(body, "tag")!;
+  expect(state.scopes!.every((entry) => entry.review.agentSummaries === undefined)).toBe(true);
+});
+
 test("aggregate: unmatched files render as a coverage note", () => {
   const results = [scope("default", true, { findings: [] })];
   const body = renderAggregateMarkdown(results, "tag", [], undefined, {

src/__tests__/render.test.ts

@@ -117,6 +117,126 @@ test("review trace stays hidden and survives state-driven re-rendering", () => {
   expect(parseReviewState(rerendered, "tag")!.review.reviewTrace).toEqual(review.reviewTrace);
 });
 
+test("agent summaries render collapsed under the summary, one plain line per agent", () => {
+  const review: CoordinatorOutput = {
+    ...base,
+    summary: "Overall summary.",
+    agentSummaries: [
+      { agent: "docs", summary: "Checked the guide. Ping @someone <br> and <!-- tag:state=x -->." },
+      { agent: "correctness" },
+      { agent: "native-abi", notRun: "paths" },
+      { agent: "security", notRun: "router" },
+    ],
+  };
+  const body = renderMarkdown(review, "tag");
+  const section = body.indexOf("<details>\n<summary>🔎 Reviewer summaries (4)</summary>");
+  expect(section).toBeGreaterThan(body.indexOf("Overall summary."));
+  expect(body).toContain(
+    "- **docs:** Checked the guide. Ping @\u200Bsomeone and &lt;!-- tag:state=x -->.",
+  );
+  expect(body).toContain("- **correctness:** _no summary_");
+  expect(body).toContain("- **native-abi:** _not run: no changed file matches its `paths`_");
+  expect(body).toContain("- **security:** _not run: the router did not select it_");
+
+  const state = parseReviewState(body, "tag")!;
+  expect(state.review.agentSummaries).toEqual(review.agentSummaries);
+  expect(renderMarkdown(state.review, "tag", state.dismissed)).toContain(
+    "🔎 Reviewer summaries (4)",
+  );
+  expect(renderMarkdown(base, "tag")).not.toContain("Reviewer summaries");
+});
+
+test("a backtick before `@` does not keep a mention live in an agent summary", () => {
+  // A closing backtick ends the code span, and a lone backtick starts none.
+  const body = renderMarkdown(
+    {
+      ...base,
+      agentSummaries: [{ agent: "docs", summary: "Closed `x`@octocat, open `@expo/team." }],
+    },
+    "tag",
+  );
+  expect(body).toContain("Closed `x`@\u200Boctocat, open `@\u200Bexpo/team.");
+});
+
+const summaryRow = (summary: string): string => {
+  const body = renderMarkdown({ ...base, agentSummaries: [{ agent: "docs", summary }] }, "tag");
+  return body
+    .split("\n")
+    .find((line) => line.startsWith("- **docs:** "))!
+    .slice(12);
+};
+
+test("an agent summary breaks every `@` that markdown can put at the start of a mention", () => {
+  // GitHub renders `_@x_` as `<em>@x</em>`, where the `@` starts its text node.
+  expect(summaryRow("_@octocat_ and *@octocat* and ~~@octocat~~")).toBe(
+    "_@\u200Boctocat_ and *@\u200Boctocat* and ~~@\u200Boctocat~~",
+  );
+  expect(summaryRow("[@octocat](https://x.dev) and \\@octocat")).toBe(
+    "[@\u200Boctocat](https://x.dev) and \\@\u200Boctocat",
+  );
+  // Each `@` of a run is broken, or the zero-width space would sit in front of a live one.
+  expect(summaryRow("@@octocat and x @@@y")).toBe(
+    "@\u200B@\u200Boctocat and x @\u200B@\u200B@\u200By",
+  );
+  // A letter or digit before the `@` is never a mention, so an address stays whole.
+  expect(summaryRow("Mail foo@example.com or a1@b.dev.")).toBe("Mail foo@example.com or a1@b.dev.");
+});
+
+test("an agent summary cannot spell a mention with an HTML character reference", () => {
+  // GitHub decodes a reference before it looks for mentions; escaped, it shows as typed.
+  expect(summaryRow("&#64;octocat &#x40;octocat &#X40;octocat &#064;octocat &commat;octocat")).toBe(
+    "&amp;#64;octocat &amp;#x40;octocat &amp;#X40;octocat &amp;#064;octocat &amp;commat;octocat",
+  );
+  // A reference after the `@` spells a login character, but that `@` is broken anyway.
+  expect(summaryRow("@&#111;ctocat")).toBe("@\u200B&#111;ctocat");
+  // Other references, and a bare `&`, read as they did.
+  expect(summaryRow("Checks a && b; R&D; &lt;T&gt; &amp;")).toBe(
+    "Checks a && b; R&D; &lt;T&gt; &amp;",
+  );
+});
+
+test("an oversized single comment leaves the agent summaries out instead of failing to post", () => {
+  const findings = Array.from({ length: 12 }, (_, i) =>
+    finding({ file: `src/f${i}.ts`, title: `Finding ${i}`, rationale: "r".repeat(1500) }),
+  );
+  // Escaping makes `<` four characters, so summaries inside their byte budget can still
+  // push the body past GitHub's limit.
+  const agentSummaries = Array.from({ length: 13 }, (_, i) => ({
+    agent: `agent-${i}`,
+    summary: "<".repeat(560),
+  }));
+  const review: CoordinatorOutput = { ...base, findings, agentSummaries };
+  const body = renderMarkdown(review, "tag");
+  expect(body.length).toBeLessThan(65_000);
+  expect(body).not.toContain("<summary>🔎 Reviewer summaries (");
+  expect(body).toContain("🔎 Reviewer summaries were left out to fit the comment size limit.");
+  for (let i = 0; i < 12; i++) {
+    expect(body).toContain(`Finding ${i}`);
+  }
+  const state = parseReviewState(body, "tag")!;
+  expect(state.review.agentSummaries).toBeUndefined();
+  expect(state.review.findings).toHaveLength(12);
+  // A body that fits keeps them.
+  expect(renderMarkdown({ ...review, findings: [] }, "tag")).toContain(
+    "<summary>🔎 Reviewer summaries (13)</summary>",
+  );
+});
+
+test("an agent summary row from a newer version's state still reads as not run", () => {
+  const body = renderMarkdown(
+    {
+      ...base,
+      agentSummaries: [
+        { agent: "docs", notRun: "budget" as "paths" },
+        { agent: "ios", notRun: "constructor" as "paths" },
+      ],
+    },
+    "tag",
+  );
+  expect(body).toContain("- **docs:** _not run_");
+  expect(body).toContain("- **ios:** _not run_");
+});
+
 test("links a finding location to the PR diff line when the line is in the diff", () => {
   const out = renderMarkdown(
     { ...base, findings: [finding({ file: "src/a.ts", line: 12 })] },