diff --git a/.expo-agents/code-review/shared.md b/.expo-agents/code-review/shared.md index f433bea..d02b281 100644 --- a/.expo-agents/code-review/shared.md +++ b/.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", diff --git a/README.md b/README.md index fe46d73..c28974a 100644 --- a/README.md +++ b/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 diff --git a/llp/0002-review-engine-pipeline.explainer.md b/llp/0002-review-engine-pipeline.explainer.md index 7b1a1a9..4dd4af4 100644 --- a/llp/0002-review-engine-pipeline.explainer.md +++ b/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: \n"`, 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. diff --git a/llp/0005-verification-fingerprints-rendering.explainer.md b/llp/0005-verification-fingerprints-rendering.explainer.md index cedde01..0c7233e 100644 --- a/llp/0005-verification-fingerprints-rendering.explainer.md +++ b/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 `@` or `@` 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 `@login` 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`. diff --git a/src/__tests__/render-aggregate.test.ts b/src/__tests__/render-aggregate.test.ts index 6eef4c8..40560a4 100644 --- a/src/__tests__/render-aggregate.test.ts +++ b/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("πŸ”Ž 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, { diff --git a/src/__tests__/render.test.ts b/src/__tests__/render.test.ts index 1bdbccf..48314e9 100644 --- a/src/__tests__/render.test.ts +++ b/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
and ." }, + { agent: "correctness" }, + { agent: "native-abi", notRun: "paths" }, + { agent: "security", notRun: "router" }, + ], + }; + const body = renderMarkdown(review, "tag"); + const section = body.indexOf("
\nπŸ”Ž Reviewer summaries (4)"); + expect(section).toBeGreaterThan(body.indexOf("Overall summary.")); + expect(body).toContain( + "- **docs:** Checked the guide. Ping @\u200Bsomeone and <!-- 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 `@x`, 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("@octocat @octocat @octocat @octocat @octocat")).toBe( + "&#64;octocat &#x40;octocat &#X40;octocat &#064;octocat &commat;octocat", + ); + // A reference after the `@` spells a login character, but that `@` is broken anyway. + expect(summaryRow("@octocat")).toBe("@\u200Boctocat"); + // Other references, and a bare `&`, read as they did. + expect(summaryRow("Checks a && b; R&D; <T> &")).toBe( + "Checks a && b; R&D; <T> &", + ); +}); + +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("πŸ”Ž 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( + "πŸ”Ž Reviewer summaries (13)", + ); +}); + +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 })] }, diff --git a/src/__tests__/review-internals.test.ts b/src/__tests__/review-internals.test.ts index cc7c77f..9b1ace7 100644 --- a/src/__tests__/review-internals.test.ts +++ b/src/__tests__/review-internals.test.ts @@ -18,11 +18,18 @@ import { buildReviewTrace, mergeTraceNotes, attachReviewTrace, + mergeAgentSummary, + buildAgentSummaries, + attachAgentSummaries, runReview, } from "../core/review.js"; -import type { ReviewRunOptions } from "../core/review.js"; +import type { AgentSummaryParts, ReviewRunOptions } from "../core/review.js"; import type { ReviewSource } from "../sources/source.js"; -import { REVIEW_TRACE_BYTES_LIMIT } from "../core/schema.js"; +import { + AGENT_SUMMARIES_BYTES_LIMIT, + AGENT_SUMMARY_CHARS, + REVIEW_TRACE_BYTES_LIMIT, +} from "../core/schema.js"; import type { LoadedConfig } from "../config/schema.js"; import type { PatchWorkspaceFile } from "../core/noise.js"; import type { CoordinatorOutput, Finding } from "../core/schema.js"; @@ -196,6 +203,119 @@ test("only the engine-built trace survives the coordinator boundary", () => { expect(attachReviewTrace(forged, engineTrace).reviewTrace).toEqual(engineTrace); }); +test("agent summaries keep each agent's parts and say why the others did not run", () => { + const summaries: AgentSummaryParts = new Map(); + mergeAgentSummary(summaries, "docs", "Checked the new guide.", 0); + mergeAgentSummary(summaries, "docs", "Checked the new guide.", 1); + mergeAgentSummary(summaries, "docs", "Checked the README.", 2); + mergeAgentSummary(summaries, "docs", "A third part is dropped.", 3); + mergeAgentSummary(summaries, "cross-cutting", "Traced the option across packages.", 0); + mergeAgentSummary(summaries, "correctness", undefined, 0); + expect( + buildAgentSummaries({ + ran: ["correctness", "docs", "cross-cutting"], + summaries, + skippedByPaths: ["native-abi"], + notRouted: ["security"], + }), + ).toEqual([ + { agent: "correctness" }, + { agent: "docs", summary: "Checked the new guide. Checked the README." }, + { agent: "cross-cutting", summary: "Traced the option across packages." }, + { agent: "native-abi", notRun: "paths" }, + { agent: "security", notRun: "router" }, + ]); + expect( + buildAgentSummaries({ ran: [], summaries: new Map(), skippedByPaths: [], notRouted: [] }), + ).toBe(undefined); +}); + +test("a chunked agent keeps the same summary parts whatever order its passes finish in", () => { + const build = (finished: [string, number][]) => { + const summaries: AgentSummaryParts = new Map(); + for (const [text, order] of finished) { + mergeAgentSummary(summaries, "docs", text, order); + } + return buildAgentSummaries({ ran: ["docs"], summaries, skippedByPaths: [], notRouted: [] }); + }; + const expected = [{ agent: "docs", summary: "Part one. Part two." }]; + expect( + build([ + ["Part one.", 0], + ["Part two.", 4], + ["Part three.", 9], + ]), + ).toEqual(expected); + expect( + build([ + ["Part three.", 9], + ["Part two.", 4], + ["Part one.", 0], + ]), + ).toEqual(expected); +}); + +test("agent ids that name Object.prototype members do not break the summaries", () => { + const ids = ["constructor", "__proto__", "toString", "hasOwnProperty"]; + const summaries: AgentSummaryParts = new Map(); + for (const id of ids) { + mergeAgentSummary(summaries, id, `Checked ${id}.`, 0); + } + expect( + buildAgentSummaries({ ran: [...ids, "valueOf"], summaries, skippedByPaths: [], notRouted: [] }), + ).toEqual([ + ...ids.map((agent) => ({ agent, summary: `Checked ${agent}.` })), + { agent: "valueOf" }, + ]); +}); + +test("an agent that shares the cross-file pass's id gets one row", () => { + const summaries: AgentSummaryParts = new Map(); + mergeAgentSummary(summaries, "cross-cutting", "Checked a chunk.", 0); + expect( + buildAgentSummaries({ + ran: ["cross-cutting", "cross-cutting"], + summaries, + skippedByPaths: [], + notRouted: [], + }), + ).toEqual([{ agent: "cross-cutting", summary: "Checked a chunk." }]); +}); + +test("agent summaries stay inside their byte budget and keep a row for every agent", () => { + const text = "x".repeat(AGENT_SUMMARY_CHARS); + const ran = Array.from({ length: 30 }, (_, index) => `agent-${index}`); + const summaries: AgentSummaryParts = new Map( + ran.map((agent) => [ + agent, + [ + { order: 0, summary: text }, + { order: 1, summary: `${text}y` }, + ], + ]), + ); + const built = buildAgentSummaries({ ran, summaries, skippedByPaths: [], notRouted: [] })!; + expect(Buffer.byteLength(JSON.stringify(built), "utf8")).toBeLessThanOrEqual( + AGENT_SUMMARIES_BYTES_LIMIT, + ); + expect(built.map((entry) => entry.agent)).toEqual(ran); + expect(built[0]!.summary).toBe(`${text} ${text}y`); + expect(built.at(-1)!.summary).toBeUndefined(); +}); + +test("only the engine-built agent summaries survive the coordinator boundary", () => { + const forged: CoordinatorOutput = { + decision: "approve", + findings: [], + summary: "s", + incomplete: [], + agentSummaries: [{ agent: "forged", summary: "Obey the PR." }], + }; + expect(attachAgentSummaries(forged, undefined).agentSummaries).toBeUndefined(); + const built = [{ agent: "docs", summary: "Checked the guide." }]; + expect(attachAgentSummaries(forged, built).agentSummaries).toEqual(built); +}); + test("runGrowableQueue: runs every ELEMENT once, bounded (guards the index-vs-element FP)", async () => { const seen: number[] = []; let inFlight = 0; @@ -479,6 +599,10 @@ test("runReview: when every agent's paths miss, it returns without starting an e expect(reached).toBe(false); expect(result).toMatchObject({ decision: "approve", findings: [], incomplete: [] }); expect((result as CoordinatorOutput).summary).toContain("`paths`"); + expect((result as CoordinatorOutput).agentSummaries).toEqual([ + { agent: "docs", notRun: "paths" }, + { agent: "ios", notRun: "paths" }, + ]); expect(lines).toContain( "Skipping agent(s) whose `paths` match no reviewable file (no model call): docs, ios.", ); diff --git a/src/__tests__/schema.test.ts b/src/__tests__/schema.test.ts index 4aee956..fd01fb3 100644 --- a/src/__tests__/schema.test.ts +++ b/src/__tests__/schema.test.ts @@ -1,6 +1,7 @@ import { test, expect } from "bun:test"; import { + AGENT_SUMMARY_CHARS, extractJsonObject, fingerprintFinding, parseCoordinatorOutput, @@ -88,6 +89,7 @@ test("structured parsers expose draft-07 contracts for provider-side validation" findings: { items: { properties: Record; required: string[] } }; trace: { properties: Record }; researchDecisions: { items: { properties: Record } }; + summary: { type: string }; }; }; expect(reviewer.$schema).toBe("http://json-schema.org/draft-07/schema#"); @@ -96,6 +98,7 @@ test("structured parsers expose draft-07 contracts for provider-side validation" expect(reviewer.properties.trace.properties).toHaveProperty("checked"); expect(reviewer.properties.trace.properties).toHaveProperty("uncertainties"); expect(reviewer.properties.researchDecisions.items.properties).toHaveProperty("outcome"); + expect(reviewer.properties.summary?.type).toBe("string"); const coordinator = parseCoordinatorOutput.jsonSchema as { properties: Record; @@ -105,6 +108,7 @@ test("structured parsers expose draft-07 contracts for provider-side validation" expect(coordinator.properties.incomplete).toBeUndefined(); expect(coordinator.properties.couldNotComplete).toBeUndefined(); expect(coordinator.properties.reviewTrace).toBeUndefined(); + expect(coordinator.properties.agentSummaries).toBeUndefined(); expect(parseVerdict.jsonSchema).toMatchObject({ type: "object", @@ -144,6 +148,46 @@ test("reviewer trace parses bounded diagnostics and fails soft when malformed", expect(malformed.findings.map((finding) => finding.title)).toEqual(["Keep the real finding"]); }); +test("a reviewer summary parses to one clipped line and fails soft when malformed", () => { + const output = parseReviewerOutput( + JSON.stringify({ findings: [], summary: " Checked the new\n option on iOS. " }), + ); + expect(output.summary).toBe("Checked the new option on iOS."); + const long = parseReviewerOutput( + JSON.stringify({ findings: [], summary: `${"word ".repeat(200)}end` }), + ); + expect(long.summary!.length).toBeLessThanOrEqual(AGENT_SUMMARY_CHARS + 1); + expect(long.summary!.endsWith("…")).toBe(true); + const malformed = parseReviewerOutput( + JSON.stringify({ + findings: [ + { + severity: "warning", + category: "quality", + file: "a.ts", + title: "Keep the real finding", + rationale: "r", + }, + ], + summary: { text: "not a string" }, + }), + ); + expect(malformed.summary).toBeUndefined(); + expect(malformed.findings.map((finding) => finding.title)).toEqual(["Keep the real finding"]); +}); + +test("a clipped reviewer summary never ends in half of an emoji", () => { + const clip = (summary: string) => + parseReviewerOutput(JSON.stringify({ findings: [], summary })).summary!; + const lone = /[\uD800-\uDBFF](?![\uDC00-\uDFFF])/; + // The cut lands between the two halves of the second emoji. + expect(clip(`${"x".repeat(AGENT_SUMMARY_CHARS - 1)}πŸ˜€πŸ˜€`)).toBe( + `${"x".repeat(AGENT_SUMMARY_CHARS - 1)}…`, + ); + expect(lone.test(clip(`${"x".repeat(AGENT_SUMMARY_CHARS - 2)}πŸ˜€πŸ˜€`))).toBe(false); + expect(lone.test(clip(` ${"x".repeat(AGENT_SUMMARY_CHARS - 3)}πŸ˜€πŸ˜€`))).toBe(false); +}); + test("reviewer research decisions parse bounded conclusions and fail soft when malformed", () => { const source = { title: "NWPathMonitor", diff --git a/src/core/render.ts b/src/core/render.ts index 6bc749e..11ab7c3 100644 --- a/src/core/render.ts +++ b/src/core/render.ts @@ -12,6 +12,7 @@ import { SEVERITY_RANK, } from "./schema.js"; import type { + AgentSummary, CoordinatorOutput, Decision, DismissalRecord, @@ -231,10 +232,14 @@ function location(finding: Finding, link?: LinkContext): string { return `\`${text}\``; } +/** GitHub's comment body limit is ~65k chars; keep a margin. */ +const MAX_COMMENT_CHARS = 60_000; + /** * GitHub comment body. The marker + embedded state enable in-place updates and * per-PR dismissals. Findings whose fingerprint appears in `dismissed` render in a - * collapsed "Dismissed" section instead of the main list. + * collapsed "Dismissed" section instead of the main list. An oversized body leaves + * out the agent summaries, from the visible rows and the embedded state, with a note. */ export function renderMarkdown( review: CoordinatorOutput, @@ -244,6 +249,28 @@ export function renderMarkdown( feedback: FeedbackRecord[] = [], pins: FeedbackPin[] = [], inputHash?: string, +): string { + // @ref LLP 0005#truncation-and-aggregate-state [implements] β€” over the size limit, the single comment leaves out the agent summaries, as the aggregate does first + const body = renderReviewBody(review, tag, dismissed, link, feedback, pins, inputHash); + if (body.length <= MAX_COMMENT_CHARS || !review.agentSummaries?.length) { + return body; + } + // Escaping and the base64 state copy make the summaries several times their byte + // budget, and a body over GitHub's limit fails to post: the review would be lost to + // the failure notice. The summaries never affect the decision, so they go. + const { agentSummaries: _leftOut, ...withoutSummaries } = review; + return renderReviewBody(withoutSummaries, tag, dismissed, link, feedback, pins, inputHash, true); +} + +function renderReviewBody( + review: CoordinatorOutput, + tag: string, + dismissed: DismissalRecord[], + link: LinkContext | undefined, + feedback: FeedbackRecord[], + pins: FeedbackPin[], + inputHash: string | undefined, + summariesLeftOut = false, ): string { const dismissedByFp = new Map(dismissed.map((record) => [record.fp, record])); const withFp = review.findings.map((finding) => ({ finding, fp: fingerprintFinding(finding) })); @@ -266,6 +293,11 @@ export function renderMarkdown( stripStateMarkers(review.summary), "", ); + lines.push( + ...(summariesLeftOut + ? [AGENT_SUMMARIES_LEFT_OUT, ""] + : agentSummariesSection(review.agentSummaries)), + ); if (review.incomplete.length > 0) { lines.push( @@ -506,6 +538,86 @@ export function inlineStubBody(tag: string, fp: string): string { ].join("\n"); } +/** + * What the review says about a roster agent that did not run. + */ +export const AGENT_NOT_RUN_TEXT: Record, string> = { + paths: "not run: no changed file matches its `paths`", + router: "not run: the router did not select it", +}; + +/** + * `AGENT_NOT_RUN_TEXT` for `reason`. A comment's state is re-rendered without a schema + * check (by `/dismiss`, or by an older version after a newer one added a reason), so an + * unknown reason still reads as "not run". + */ +export function agentNotRunText(reason: string): string { + return Object.hasOwn(AGENT_NOT_RUN_TEXT, reason) + ? AGENT_NOT_RUN_TEXT[reason as keyof typeof AGENT_NOT_RUN_TEXT] + : "not run"; +} + +/** Shown in place of the summaries a comment left out to fit GitHub's size limit. */ +const AGENT_SUMMARIES_LEFT_OUT = + "πŸ”Ž Reviewer summaries were left out to fit the comment size limit."; + +/** + * Each roster agent's own summary, or why it did not run, in a collapsed section. + * Empty when the review has none. + */ +function agentSummariesSection(summaries: AgentSummary[] = []): string[] { + if (summaries.length === 0) { + return []; + } + const rows = summaries.map(({ agent, summary, notRun }) => { + const text = notRun + ? `_${agentNotRunText(notRun)}_` + : typeof summary === "string" && summary + ? plainModelLine(summary) + : "_no summary_"; + return `- **${stripStateMarkers(agent)}:** ${text}`; + }); + return [ + "
", + `πŸ”Ž Reviewer summaries (${summaries.length})`, + "", + ...rows, + "", + "
", + "", + ]; +} + +/** + * An HTML character reference that spells `@`: `@`, `@`, `@`. GitHub + * decodes it before it looks for mentions. A reference after the `@` needs nothing: + * that `@` is broken like any other. Others (`<`, `&`) still render as written. + */ +const AT_REFERENCE_RE = /&(?=#0*64;|#[xX]0*40;|commat;)/g; + +/** + * Model prose as one plain line that cannot notify anyone: formatting tags removed, + * `@` character references and every `<` escaped, whitespace collapsed, and each `@` that + * could start a mention broken. + */ +function plainModelLine(text: string): string { + const flat = text + .replace(FORMATTING_TAG_RE, " ") + // Shown as typed: `&#64;` renders as the text `@`, never as `@`. + .replace(AT_REFERENCE_RE, "&") + .replace(/@x`), so only an ASCII letter or digit before the `@` + // is safe to trust, which keeps an email address whole. Everywhere else a zero-width + // space after the `@` keeps the text readable and stops the notification. + // A lookbehind, not a captured preceding character: a capture would consume the + // first `@` of `@@login`, leave the second unmatched, and `@\u200B@login` notifies. + return flat.replace(/(? per scope (findings rendered with * scope-namespaced ids), and the dismissed section. The embedded v1 `review` field * is a synthesized merge so v1 state consumers still see a valid shape; `scopes` - * carries the real per-scope data. Oversized bodies trim each scope's findings to - * the most severe N (halving until it fits, floor 3) with a per-scope note. + * carries the real per-scope data. An oversized body first leaves out every scope's + * agent summaries, then trims each scope's findings to the most severe N (halving + * until it fits, floor 3), each with a per-scope note. */ export function renderAggregateMarkdown( results: ScopeReviewResult[], @@ -741,7 +851,7 @@ export function renderAggregateMarkdown( const worst = worstDecision(results.map((result) => result.review.decision)); const unmatched = opts?.unmatchedFiles ?? []; - const buildBody = (limitPerScope: number): string => { + const buildBody = (limitPerScope: number, withSummaries: boolean): string => { const lines: string[] = [ commentMarker(tag), "## πŸ€– AI code review", @@ -829,6 +939,11 @@ export function renderAggregateMarkdown( if (result.review.summary) { lines.push(stripStateMarkers(result.review.summary), ""); } + if (withSummaries) { + lines.push(...agentSummariesSection(result.review.agentSummaries)); + } else if (result.review.agentSummaries?.length) { + lines.push(AGENT_SUMMARIES_LEFT_OUT, ""); + } lines.push(...requalificationAuditNote(requalifiedAll.map((entry) => entry.finding))); lines.push(...feedbackAuditNote(replies)); if (shown.length === 0) { @@ -893,9 +1008,16 @@ export function renderAggregateMarkdown( // array (reviewState below) is the single source of truth. A stale per-scope // copy that survived here would be read back on a later carried-over run and // could re-apply a record a human /undismiss already overrode at the top level. - const { feedback: _feedback, ...review } = result.review as CoordinatorOutput & { + const { + feedback: _feedback, + agentSummaries, + ...rest + } = result.review as CoordinatorOutput & { feedback?: unknown; }; + // State bytes count toward the comment size, so left-out summaries leave the + // state too. + const review = withSummaries && agentSummaries ? { ...rest, agentSummaries } : rest; return { scope: result.scope, isDefault: result.isDefault, @@ -947,7 +1069,14 @@ export function renderAggregateMarkdown( }; let limit = Number.POSITIVE_INFINITY; - let body = buildBody(limit); + let withSummaries = true; + let body = buildBody(limit, withSummaries); + // Agent summaries go before any finding: they never affect the decision, and each + // scope's summary budget does not bound the sum across scopes. + if (body.length > MAX_COMMENT_CHARS) { + withSummaries = false; + body = buildBody(limit, withSummaries); + } // Seed from the largest per-scope section the limit applies to β€” kept OR // requalified β€” so the halving loop shrinks whichever one is oversized. const largestScope = Math.max( @@ -959,7 +1088,7 @@ export function renderAggregateMarkdown( limit === Number.POSITIVE_INFINITY ? Math.max(3, Math.floor(largestScope / 2)) : Math.max(3, Math.floor(limit / 2)); - body = buildBody(limit); + body = buildBody(limit, withSummaries); } return body; } diff --git a/src/core/review.ts b/src/core/review.ts index e058e79..188412c 100644 --- a/src/core/review.ts +++ b/src/core/review.ts @@ -53,6 +53,8 @@ import { NO_TOOLS_INSTRUCTION, } from "./prompts.js"; import { + AGENT_SUMMARIES_BYTES_LIMIT, + AGENT_SUMMARY_PARTS_LIMIT, fingerprintFinding, isOverallRiskHandoff, parseReviewerOutput, @@ -62,6 +64,7 @@ import { REVIEW_TRACE_UNCERTAINTY_LIMIT, } from "./schema.js"; import type { + AgentSummary, CoordinatorOutput, FeedbackRecord, Finding, @@ -327,12 +330,19 @@ export async function runReview( } if (pathsSelection.agents.length === 0) { + const agentSummaries = buildAgentSummaries({ + ran: [], + summaries: new Map(), + skippedByPaths: pathsSelection.skipped, + notRouted: [], + }); const output: CoordinatorOutput = { decision: "approve", findings: [], summary: "No reviewer applies to these changes: no reviewable file matches any agent's `paths`.", incomplete: [], + ...(agentSummaries ? { agentSummaries } : {}), }; await appendStepSummary(renderPathsSkipsMarkdown(pathsSkipped)); await safeLog(logPath, { @@ -547,6 +557,10 @@ export async function runReview( // comment state. These are deliberately separate from findings: they never reach // the coordinator, verification, policy, or decision paths. const agentTrace: Record = {}; + // Each reviewer's own summary for the comment. Like the trace, it never reaches the + // coordinator, verification, policy, or decision paths. A Map, so an agent id such as + // `constructor` cannot read an inherited property. + const agentSummaryParts: AgentSummaryParts = new Map(); // First reviewer (by scheduling order) that produced each fingerprint, so a finding's // originating agent can be carried through the coordinator's merge/rewrite by matching // on fingerprint. Kept separate from agentFindings so the coordinator prompt and the @@ -604,6 +618,9 @@ export async function runReview( : "Router unavailable; running all agents.", ); } + const notRouted = pathsSelection.agents + .filter((agent) => !selectedAgents.includes(agent)) + .map((agent) => agent.id); // Split the diff into focused chunks so each reviewer call sees a small file // set (better recall than one giant blob), and run all agentΓ—chunk calls @@ -827,6 +844,12 @@ export async function runReview( ...overrides, }); + // Passes finish in any order, so each summary part is ordered by where its files + // sit in the diff: the same run always keeps the same parts. + const filePosition = new Map(workspace.files.map((file, index) => [file.path, index])); + const fileOrder = (files: PatchWorkspaceFile[]): number => + filePosition.get(files[0]?.path ?? "") ?? 0; + let completedPasses = 0; let failedPasses = 0; const taskProgress = (task: ReviewTask, line: string): void => @@ -863,6 +886,7 @@ export async function runReview( if (value.trace) { mergeTraceNotes(agentTrace, task.bucket, value.trace); } + mergeAgentSummary(agentSummaryParts, task.bucket, value.summary, fileOrder(task.files)); for (const finding of value.findings) { const fp = fingerprintFinding(finding); if (!agentByFp.has(fp)) { @@ -1467,8 +1491,18 @@ export async function runReview( // so setup advice is always the checker's, never model text. // `CoordinatorOutputSchema` knows the engine field so cached/embedded reviews can // parse it, but the coordinator must never author it. Strip any model-supplied - // value and attach only the trace assembled from reviewer pass outputs. - const outputWithTrace = attachReviewTrace(output, reviewTrace); + // value and attach only the trace assembled from reviewer pass outputs. The agent + // summaries follow the same rule. + const agentSummaries = buildAgentSummaries({ + ran: [...selectedAgents.map((agent) => agent.id), ...(chunked ? [CROSS_CUTTING_AGENT] : [])], + summaries: agentSummaryParts, + skippedByPaths: pathsSelection.skipped, + notRouted, + }); + const outputWithTrace = attachAgentSummaries( + attachReviewTrace(output, reviewTrace), + agentSummaries, + ); const reviewed = { ...outputWithTrace, setupNotes, @@ -1562,6 +1596,74 @@ export function attachReviewTrace( return { ...withoutTrace, ...(reviewTrace ? { reviewTrace } : {}) }; } +/** Each agent's pass summaries, with the diff position each pass started at. */ +export type AgentSummaryParts = Map; + +/** Record one pass's summary for its agent, at `order` (the pass's place in the diff). */ +export function mergeAgentSummary( + target: AgentSummaryParts, + agent: string, + summary: string | undefined, + order: number, +): void { + if (summary) { + target.set(agent, [...(target.get(agent) ?? []), { order, summary }]); + } +} + +/** + * One row per roster agent: the agents that ran, in run order, with their summaries, + * then the agents that did not run, with the reason. A chunked agent keeps its first + * `AGENT_SUMMARY_PARTS_LIMIT` distinct parts in diff order. Undefined when the list is + * empty. + */ +export function buildAgentSummaries(input: { + ran: string[]; + summaries: AgentSummaryParts; + skippedByPaths: string[]; + notRouted: string[]; +}): AgentSummary[] | undefined { + const rows: AgentSummary[] = [ + // An agent that shares the cross-file pass's id would otherwise get two rows. + ...[...new Set(input.ran)].map((agent) => { + const parts = [...(input.summaries.get(agent) ?? [])] + .sort( + (left, right) => left.order - right.order || left.summary.localeCompare(right.summary), + ) + .map((part) => part.summary); + const summary = [...new Set(parts)].slice(0, AGENT_SUMMARY_PARTS_LIMIT).join(" "); + return summary ? { agent, summary } : { agent }; + }), + ...input.skippedByPaths.map((agent) => ({ agent, notRun: "paths" as const })), + ...input.notRouted.map((agent) => ({ agent, notRun: "router" as const })), + ]; + if (rows.length === 0) { + return undefined; + } + // The summaries share GitHub's ~65k comment body with the findings and the hidden + // state, so over budget the last rows lose their text but keep their place. + for (let index = rows.length - 1; index >= 0; index--) { + if (Buffer.byteLength(JSON.stringify(rows), "utf8") <= AGENT_SUMMARIES_BYTES_LIMIT) { + break; + } + const { summary: _dropped, ...row } = rows[index]!; + rows[index] = row; + } + return rows; +} + +/** + * Replace any coordinator-authored agent summaries with the engine-assembled list, + * for the same reason as `attachReviewTrace`. + */ +export function attachAgentSummaries( + output: CoordinatorOutput, + agentSummaries: AgentSummary[] | undefined, +): CoordinatorOutput { + const { agentSummaries: _coordinatorSummaries, ...withoutSummaries } = output; + return { ...withoutSummaries, ...(agentSummaries ? { agentSummaries } : {}) }; +} + /** * Policy backstop: strip the internal risk handoff, drop suggestions unless * opted in, cap by count (most severe first), and downgrade diff --git a/src/core/schema.ts b/src/core/schema.ts index 27465e6..e01b35a 100644 --- a/src/core/schema.ts +++ b/src/core/schema.ts @@ -199,23 +199,50 @@ export const ReviewerTraceNotesSchema = z.object({ }); export type ReviewerTraceNotes = z.infer; +/** + * A reviewer's own short account of what it reviewed and concluded, shown to the + * readers of the review. A chunked agent keeps up to `AGENT_SUMMARY_PARTS_LIMIT` + * pass summaries, and the whole list stays under `AGENT_SUMMARIES_BYTES_LIMIT`. + */ +export const AGENT_SUMMARY_CHARS = 280; +export const AGENT_SUMMARY_PARTS_LIMIT = 2; +export const AGENT_SUMMARIES_BYTES_LIMIT = 8_000; + +function clipAgentSummary(text: string): string { + const flat = text.replace(/\s+/g, " ").trim(); + if (flat.length <= AGENT_SUMMARY_CHARS) { + return flat; + } + // A cut between the two halves of an emoji would leave half a character. + const cut = flat.slice(0, AGENT_SUMMARY_CHARS).replace(/[\uD800-\uDBFF]$/, ""); + const space = cut.lastIndexOf(" "); + return `${space > 0 ? cut.slice(0, space) : cut}…`; +} + /** Provider-facing shape each sub-reviewer is asked to emit. */ const ReviewerModelOutputSchema = z.object({ findings: z.array(ModelFindingSchema).default([]), trace: ReviewerTraceNotesSchema.optional(), researchDecisions: z.array(ResearchDecisionSchema).max(8).optional(), + summary: z + .string() + .optional() + .describe( + `One or two sentences, at most ${AGENT_SUMMARY_CHARS} characters, on what this pass reviewed and concluded`, + ), }); /** * Local trust boundary for reviewer output. Findings stay strict, while diagnostics - * fail soft: a malformed optional trace must never discard otherwise valid findings - * or turn a clean pass into a coverage gap. + * and the summary fail soft: a malformed optional field must never discard otherwise + * valid findings or turn a clean pass into a coverage gap. */ export const ReviewerOutputSchema = z .object({ findings: z.array(ModelFindingSchema).default([]), trace: z.unknown().optional(), researchDecisions: z.unknown().optional(), + summary: z.unknown().optional(), }) .transform((output) => { const trace = ReviewerTraceNotesSchema.safeParse(output.trace); @@ -223,10 +250,12 @@ export const ReviewerOutputSchema = z .array(ResearchDecisionSchema) .max(8) .safeParse(output.researchDecisions); + const summary = typeof output.summary === "string" ? clipAgentSummary(output.summary) : ""; return { findings: output.findings, ...(trace.success ? { trace: trace.data } : {}), ...(researchDecisions.success ? { researchDecisions: researchDecisions.data } : {}), + ...(summary ? { summary } : {}), }; }); export type ReviewerOutput = z.infer; @@ -239,6 +268,22 @@ export const ReviewTraceSchema = z.object({ }); export type ReviewTrace = z.infer; +/** + * Why a roster agent did not review the change: no changed file matches its `paths`, + * or the router did not select it. + */ +export const AGENT_NOT_RUN_REASONS = ["paths", "router"] as const; + +/** + * One roster agent in the review: its own summary when it ran, or why it did not run. + */ +export const AgentSummarySchema = z.object({ + agent: z.string(), + summary: z.string().optional(), + notRun: z.enum(AGENT_NOT_RUN_REASONS).optional(), +}); +export type AgentSummary = z.infer; + /** Mode-agnostic coordinator result; each Reporter decides how to render it. */ const CoordinatorModelOutputSchema = z.object({ decision: z.enum(DECISIONS), @@ -280,6 +325,12 @@ export const CoordinatorOutputSchema = CoordinatorModelOutputSchema.extend({ // Fail soft here too: the coordinator cannot author this engine field, and a // malformed injected value must not fail consolidation before the engine strips it. reviewTrace: ReviewTraceSchema.optional().catch(undefined), + /** + * Each reviewer's own summary, and the roster agents that did not run with the + * reason. Engine-owned like `reviewTrace`, but rendered; it never affects the + * decision or the finding set. + */ + agentSummaries: z.array(AgentSummarySchema).optional().catch(undefined), }); export type CoordinatorOutput = z.infer; diff --git a/src/reporters/terminal.ts b/src/reporters/terminal.ts index 68772cd..ab114d7 100644 --- a/src/reporters/terminal.ts +++ b/src/reporters/terminal.ts @@ -1,5 +1,11 @@ // @ref LLP 0008#terminal-reporter β€” stdout carries the report (progress/logging goes to stderr) and the decision maps to a process exit code for pre-push/pre-commit gating -import { decisionExitCode, decisionLabel, groupBySeverity, sortFindings } from "../core/render.js"; +import { + agentNotRunText, + decisionExitCode, + decisionLabel, + groupBySeverity, + sortFindings, +} from "../core/render.js"; import { SEVERITIES } from "../core/schema.js"; import type { CoordinatorOutput, Finding, Severity } from "../core/schema.js"; import type { Reporter } from "./reporter.js"; @@ -55,6 +61,16 @@ export class TerminalReporter implements Reporter { out.push(this.tally(review.findings), ""); out.push(review.summary, ""); + const agentSummaries = review.agentSummaries ?? []; + if (agentSummaries.length > 0) { + out.push(this.paint(BOLD, "πŸ”Ž Reviewer summaries:")); + for (const { agent, summary, notRun } of agentSummaries) { + const text = notRun ? agentNotRunText(notRun) : (summary ?? "no summary"); + out.push(this.paint(DIM, ` - ${agent}: ${text}`)); + } + out.push(""); + } + if (review.incomplete.length > 0) { out.push( this.paint(BOLD, "⏱️ Coverage note: some passes did not finish (partial coverage):"), diff --git a/templates/shared.md b/templates/shared.md index 1601f94..dda4335 100644 --- a/templates/shared.md +++ b/templates/shared.md @@ -202,10 +202,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",