diff --git a/README.md b/README.md index 939b260..0f468e2 100644 --- a/README.md +++ b/README.md @@ -534,6 +534,35 @@ paths: - Quote each glob. A YAML value that starts with `*` must be quoted to stay valid YAML for other tools. +`mention` lets an agent ask specific people for a human review. It takes a list of +GitHub logins or `org/team` slugs: + +```yaml +mention: + - octocat +``` + +- The agent decides when a change needs a human review: its role prompt tells it + when to set `humanReview`, one sentence on why. The PR comment then shows a + visible `๐Ÿ“ฃ @octocat: the docs reviewer asks for your review.` line with that + reason under the overall summary. Without a `humanReview`, the agent mentions no + one. +- Only the trusted config names people. The model writes only the reason, and the + reporter breaks every `@` mention in model-written text: the reason, the summaries, + and each finding's title, rationale and suggestion. +- The bot edits its comment in place on each re-review. GitHub also notifies a + person when an edit adds a mention of them, so a mention that first appears on a + re-review still notifies. Each person is notified once per comment: if a request + goes away on a later run and comes back, the line names them without a live + mention. A comment the bot deletes and posts again starts over. +- A team mention may not notify anyone. GitHub notifies only people with read access + to the repository, and a team only when the comment's author can see it. The + default `GITHUB_TOKEN` posts as `github-actions[bot]`, which is not an organization + member. This is untested: if a team must hear about it, list its people's logins. +- An invalid `mention` (not a list or an empty list) is ignored. An entry that is + not a login or team is left out, and the agent still mentions the others. Either + way the review names the agent file and the fix under **Review setup**. + For a real-world example, see eas-cli's [`.expo-agents/code-review/`](https://github.com/expo/eas-cli/tree/main/.expo-code-review) โ€” correctness/security/consistency agents, a stronger model for security, and diff --git a/llp/0002-review-engine-pipeline.explainer.md b/llp/0002-review-engine-pipeline.explainer.md index e4cfac2..fb47564 100644 --- a/llp/0002-review-engine-pipeline.explainer.md +++ b/llp/0002-review-engine-pipeline.explainer.md @@ -99,6 +99,8 @@ Each successful reviewer pass may also return a bounded conclusion-only trace: u 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`]. +A pass may also return `humanReview`, one sentence on why a person should review the change, parsed and cut like the summary. `runReview()` keeps the first reason per agent. `buildMentions` turns a reason into a mention only for an agent that ran and whose front matter has a `mention` list, and the people come from that list, never from the model. `attachMentions` attaches the result to `CoordinatorOutput.mentions` with the other engine-owned fields and strips any coordinator-authored value. A mention never affects the decision or the finding set [observed: `review.ts` `buildMentions`/`attachMentions`]. + 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 f06a939..2eddedb 100644 --- a/llp/0005-verification-fingerprints-rendering.explainer.md +++ b/llp/0005-verification-fingerprints-rendering.explainer.md @@ -65,6 +65,10 @@ The full review may contain an engine-owned `reviewTrace`. It is deliberately in 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`]. +The engine-owned `mentions` are the one place a review notifies people on purpose (the `/dismiss` and reply credits only name the person who acted). Each renders as a visible `> ๐Ÿ“ฃ @login: the \`agent\` reviewer asks for your review.` line above the summaries section, followed by the agent's reason through `plainModelLine`. In the aggregate comment the line sits inside its scope's fold, so a request opens that fold like a finding does. Every other piece of model text that reaches a comment, the summaries and each finding's title, rationale, suggestion, source titles and requalification reason, goes through `modelText`, which escapes `@` character references and breaks each `@` by the same rule as the summaries while it keeps the text's lines and formatting; so do the inline finding comments [observed: `render.ts` `modelText`/`breakMentions`]. The renderer checks every login against `MENTION_TARGET_RE` again, because a re-render reads the state back from the comment body. The reporter edits the comment in place on later runs. GitHub documents that an edit which adds a mention notifies that person, so a mention that first appears on a re-review still notifies [observed: GitHub Docs, "Basic writing and formatting syntax", mentioning people and teams; `render.ts` `mentionLines`]. + +That same rule would notify a person again each time a request returns: `humanReview` is model output recomputed on every run, so it can drop out on one push and come back on the next. The comment state therefore carries `mentioned`: the handles the body mentions for real (`live`) and every handle the comment has ever notified (`notified`). A handle stays live while it stays in the comment, and one that left and came back renders with a zero-width space after its `@`, so the line still names the person without notifying them. `report`, `reportAggregate` and the `/dismiss` re-render all pass the previous state's `mentioned` back in. A comment the bot deletes (`clear()`) and posts again starts with no state, so it notifies again [observed: `render.ts` `nextMentionState`; `reporters/github.ts`]. + ## 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. diff --git a/llp/0006-config-schema-loading-routing.explainer.md b/llp/0006-config-schema-loading-routing.explainer.md index c41179f..989963c 100644 --- a/llp/0006-config-schema-loading-routing.explainer.md +++ b/llp/0006-config-schema-loading-routing.explainer.md @@ -155,7 +155,8 @@ as a scalar, exactly as before lists existed, so `description`, `alwaysRun`, `mo the value. Keys whose value is a list ALSO land in `lists`: a one-line flow list (`paths: ["a/**", 'b/**', c/**]`, trailing comma allowed) or a block list (`paths:` followed by `- item` lines; blank and `#` lines inside are skipped, and the next key ends it) -[observed: `load.ts` `parseFrontmatter`, `parseFlowList`]. Only `paths` reads `lists`. +[observed: `load.ts` `parseFrontmatter`, `parseFlowList`]. Only `paths` and `mention` read +`lists`. `parseAgentPaths` validates `paths` at load time, because a mistyped glob would otherwise skip the agent on every PR with nothing reporting it. An invalid value is a **setup warning, not a load @@ -202,6 +203,20 @@ and its review-cache key are unchanged [observed: `load.ts` agent construction]. still runs only in scopes with a matching file. How the engine applies the filter is in [LLP 0002](0002-review-engine-pipeline.explainer.md#agent-paths-filter). +`mention` names the people an agent may ask for a human review: a list of GitHub logins or +`org/team` slugs, with an optional leading `@` that the loader drops. It is the only source of +whom a review notifies. The agent itself only writes the reason, so a PR cannot make the bot +mention anyone: CI loads agent files from the base commit, and the model never chooses a login. +`parseAgentMention` treats a value that is not a list, or an empty list, like an invalid `paths`: +a setup warning that names the file and the fix, and the agent mentions no one. An entry that does +not match `MENTION_TARGET_RE` is different: it is left out with a warning that names it, and the +agent still mentions the valid entries, because one typo should not silence the people listed +beside it. `MENTION_TARGET_RE` follows GitHub's rules: a login is at most 39 letters, digits and +single inner hyphens, with an optional Enterprise Managed User `_shortcode` suffix, and a team slug +is letters, digits and underscores joined by single hyphens. An agent without `mention` gets no +`mention` key, so its shape and review-cache key are unchanged [observed: `load.ts` +`parseAgentMention`; `schema.ts` `MENTION_TARGET_RE`]. + ## Budgets and Chunking Defaults `budget.totalPassesMinutes` defaults to 55, not the scaffolded workflow's full 90-minute timeout, diff --git a/src/__tests__/config-load.test.ts b/src/__tests__/config-load.test.ts index 6c6474c..2dfe507 100644 --- a/src/__tests__/config-load.test.ts +++ b/src/__tests__/config-load.test.ts @@ -298,3 +298,60 @@ test("loadReviewConfig: `includeSuggestions` loads as a floor, with a deprecatio } } }); + +test("loadReviewConfig: front-matter `mention` loads GitHub logins and teams onto the agent", async () => { + const root = await configDirWithAgent( + '---\nmention:\n - amandeepmittal\n - "@expo/docs"\n---\nIOS', + ); + try { + const config = await loadReviewConfig(root); + const ios = config.agents.find((agent) => agent.id === "ios")!; + const plain = config.agents.find((agent) => agent.id === "plain")!; + expect(ios.mention).toEqual(["amandeepmittal", "expo/docs"]); + expect("mention" in plain).toBe(false); + expect("setupWarnings" in config).toBe(false); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("loadReviewConfig: an invalid `mention` is a warning, and the agent mentions no one", async () => { + const cases: Array<[string, string]> = [ + ["mention: amandeepmittal", "is not a list"], + ["mention: []", "is empty"], + ['mention: ["aman deep"]', 'entry 1 ("aman deep") is not a GitHub login or team'], + ]; + for (const [line, expected] of cases) { + const root = await configDirWithAgent(`---\n${line}\n---\nIOS`); + try { + const config = await loadReviewConfig(root); + const ios = config.agents.find((agent) => agent.id === "ios")!; + expect("mention" in ios, line).toBe(false); + expect(config.setupWarnings, line).toHaveLength(1); + const [warning] = config.setupWarnings!; + expect(warning).toContain(path.join(CONFIG_DIRNAME, "agents", "ios.md")); + expect(warning).toContain(expected); + expect(warning).toContain("mentions no one"); + } finally { + await rm(root, { recursive: true, force: true }); + } + } +}); + +test("loadReviewConfig: an invalid `mention` entry is left out, and the valid ones still load", async () => { + const root = await configDirWithAgent( + '---\nmention: ["amandeepmittal", "aman deep", "expo/docs", "-x"]\n---\nIOS', + ); + try { + const config = await loadReviewConfig(root); + const ios = config.agents.find((agent) => agent.id === "ios")!; + expect(ios.mention).toEqual(["amandeepmittal", "expo/docs"]); + expect(config.setupWarnings).toHaveLength(1); + const [warning] = config.setupWarnings!; + expect(warning).toContain(path.join(CONFIG_DIRNAME, "agents", "ios.md")); + expect(warning).toContain('entries 2 ("aman deep"), 4 ("-x") are not a GitHub login or team'); + expect(warning).toContain("leaves them out"); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); diff --git a/src/__tests__/render-aggregate.test.ts b/src/__tests__/render-aggregate.test.ts index 40560a4..caafa90 100644 --- a/src/__tests__/render-aggregate.test.ts +++ b/src/__tests__/render-aggregate.test.ts @@ -146,6 +146,49 @@ test("aggregate: each scope lists its own agent summaries inside its fold", () = expect(body).toContain("- **security:** Checked the token handling."); }); +test("aggregate: a scope's mention renders under that scope's summary", () => { + const results = [ + scope("docs", false, { + summary: "Docs scope summary.", + mentions: [{ agent: "docs", logins: ["amandeepmittal"], reason: "New guide." }], + }), + ]; + const body = renderAggregateMarkdown(results, "tag", []); + const line = "> ๐Ÿ“ฃ @amandeepmittal: the `docs` reviewer asks for your review. New guide."; + expect(body.indexOf(line)).toBeGreaterThan(body.indexOf("Docs scope summary.")); +}); + +test("aggregate: a scope with a human-review request and no findings opens its fold", () => { + const results = [ + scope("docs", false, { + summary: "Docs summary for @summary-user.", + mentions: [{ agent: "docs", logins: ["amandeepmittal"], reason: "New guide." }], + }), + scope("api", false, {}), + ]; + const body = renderAggregateMarkdown(results, "tag", []); + expect(body).toContain("
\ndocs โ€” "); + expect(body).toContain("
\napi โ€” "); + expect(body).toContain("Docs summary for @\u200Bsummary-user."); +}); + +test("aggregate: the mention state rides the comment, so a returning request does not notify", () => { + const asking = [ + scope("docs", false, { + mentions: [{ agent: "docs", logins: ["amandeepmittal"], reason: "New guide." }], + }), + ]; + const first = renderAggregateMarkdown(asking, "tag", []); + expect(first).toContain("> ๐Ÿ“ฃ @amandeepmittal:"); + const dropped = renderAggregateMarkdown([scope("docs", false, {})], "tag", [], undefined, { + mentioned: parseReviewState(first, "tag")!.mentioned!, + }); + const back = renderAggregateMarkdown(asking, "tag", [], undefined, { + mentioned: parseReviewState(dropped, "tag")!.mentioned!, + }); + expect(back).toContain("> ๐Ÿ“ฃ @\u200Bamandeepmittal:"); +}); + 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); diff --git a/src/__tests__/render.test.ts b/src/__tests__/render.test.ts index 3712de4..bb95eeb 100644 --- a/src/__tests__/render.test.ts +++ b/src/__tests__/render.test.ts @@ -160,6 +160,112 @@ test("a backtick before `@` does not keep a mention live in an agent summary", ( expect(body).toContain("Closed `x`@\u200Boctocat, open `@\u200Bexpo/team."); }); +test("a mention renders visibly under the summary and names only valid logins", () => { + const review: CoordinatorOutput = { + ...base, + summary: "Overall summary.", + mentions: [ + { + agent: "docs", + logins: ["amandeepmittal", "expo/docs", "not a login"], + reason: "Ask @someone else
to look.", + }, + ], + }; + const body = renderMarkdown(review, "tag"); + const line = + "> ๐Ÿ“ฃ @amandeepmittal @expo/docs: the `docs` reviewer asks for your review. Ask @\u200Bsomeone else to look."; + expect(body).toContain(line); + expect(body.indexOf(line)).toBeGreaterThan(body.indexOf("Overall summary.")); + expect(body).not.toContain("not a login"); + + const state = parseReviewState(body, "tag")!; + expect(state.review.mentions).toEqual(review.mentions); + expect(renderMarkdown(state.review, "tag", state.dismissed)).toContain(line); + expect(renderMarkdown(base, "tag")).not.toContain("๐Ÿ“ฃ"); +}); + +/** The `@login` / `@org/team` mentions GitHub would notify in `body`, before its state. */ +const liveMentions = (body: string): string[] => + [...body.split("`); lines.push( ``, ); return lines.join("\n"); @@ -416,7 +444,7 @@ function indentContinuation(value: string, indent = " "): string[] { } function sourceLabel(value: string): string { - return stripStateMarkers(value) + return modelText(value) .replace(//g, ">") .replace(/([\\[\]])/g, "\\$1"); @@ -473,14 +501,14 @@ function renderFindingLines( // The one-line rationale stays here as the durable audit trail (the author // can collapse/resolve the thread; they cannot edit this comment). return [ - `- **${stripStateMarkers(finding.title)}** โ€” ${location(finding, link)} _(${finding.category})_ ยท \`id:${id}\` ยท ๐Ÿ’ฌ [inline comment](${inlineUrl})${replyAnnotation(reply)}`, - ...indentContinuation(oneLineRationale(stripStateMarkers(finding.rationale))), + `- **${modelText(finding.title)}** โ€” ${location(finding, link)} _(${finding.category})_ ยท \`id:${id}\` ยท ๐Ÿ’ฌ [inline comment](${inlineUrl})${replyAnnotation(reply)}`, + ...indentContinuation(breakMentions(oneLineRationale(stripStateMarkers(finding.rationale)))), "", ]; } const out = [ - `- **${stripStateMarkers(finding.title)}** โ€” ${location(finding, link)} _(${finding.category})_ ยท \`id:${id}\`${replyAnnotation(reply)}`, - ...indentContinuation(stripStateMarkers(finding.rationale)), + `- **${modelText(finding.title)}** โ€” ${location(finding, link)} _(${finding.category})_ ยท \`id:${id}\`${replyAnnotation(reply)}`, + ...indentContinuation(modelText(finding.rationale)), ]; if (finding.sources?.length) { const sources = finding.sources @@ -492,7 +520,7 @@ function renderFindingLines( // A rationale may end in raw HTML (`
`). GitHub requires a truly // blank line before it resumes Markdown parsing; without this separator the // suggestion's emphasis markers are rendered literally. - out.push("", ...indentContinuation(`**Suggestion:** ${stripStateMarkers(finding.suggestion)}`)); + out.push("", ...indentContinuation(`**Suggestion:** ${modelText(finding.suggestion)}`)); } // Separator so a rationale ending in `
` cannot swallow the next // bullet. Findings are already loose list items, so this changes no spacing. @@ -511,9 +539,9 @@ function renderFindingLines( export function renderInlineCommentBody(finding: Finding, tag: string, fp: string): string { const lines = [ inlineCommentMarker(tag, fp), - `**${severityHeading(knownSeverity(finding.severity))}: ${stripStateMarkers(finding.title)}** _(${finding.category})_ ยท \`id:${fp}\``, + `**${severityHeading(knownSeverity(finding.severity))}: ${modelText(finding.title)}** _(${finding.category})_ ยท \`id:${fp}\``, "", - stripStateMarkers(finding.rationale), + modelText(finding.rationale), ]; if (finding.sources?.length) { const sources = finding.sources @@ -522,7 +550,7 @@ export function renderInlineCommentBody(finding: Finding, tag: string, fp: strin lines.push("", `**Sources:** ${sources}`); } if (finding.suggestion) { - lines.push("", `**Suggestion:** ${stripStateMarkers(finding.suggestion)}`); + lines.push("", `**Suggestion:** ${modelText(finding.suggestion)}`); } lines.push( "", @@ -555,6 +583,55 @@ export const AGENT_NOT_RUN_TEXT: Record, str router: "not run: the router did not select it", }; +/** The valid handles a set of mentions names. */ +function mentionHandles(mentions: AgentMention[] = []): string[] { + // The state is parsed back from the comment body, so check every login again. + return mentions.flatMap(({ logins }) => logins.filter((login) => MENTION_TARGET_RE.test(login))); +} + +// @ref LLP 0005#comment-rendering [implements] โ€” a handle notifies once per comment: an edit that adds a mention notifies, so a request that drops out and comes back renders without a live mention +/** + * Which handles this render mentions for real. A handle stays live while it stays in + * the comment. One the comment notified before, which left and came back, renders + * as plain text: GitHub notifies a person when an edit adds a mention of them, so a + * request that flaps between runs would otherwise notify on every return. + */ +function nextMentionState(handles: string[], prior?: MentionState): MentionState { + const priorLive = new Set(prior?.live ?? []); + const priorNotified = new Set(prior?.notified ?? []); + const live = [...new Set(handles.map((handle) => handle.toLowerCase()))].filter( + (handle) => priorLive.has(handle) || !priorNotified.has(handle), + ); + return { live, notified: [...new Set([...priorNotified, ...live])] }; +} + +/** The state field for `mentionState`, or nothing when the comment never mentioned anyone. */ +function withMentionState(mentionState: MentionState): { mentioned?: MentionState } { + return mentionState.notified.length > 0 ? { mentioned: mentionState } : {}; +} + +/** + * One visible line per human review that an agent asked for, which mentions the people + * from the agent's config. A handle not in `live` shows without notifying. Empty when + * there are none. + */ +function mentionLines(mentions: AgentMention[] = [], live: string[] = []): string[] { + const liveSet = new Set(live); + return mentions.flatMap(({ agent, logins, reason }) => { + const handles = mentionHandles([{ agent, logins, reason }]); + if (handles.length === 0) { + return []; + } + const people = handles + .map((login) => (liveSet.has(login.toLowerCase()) ? `@${login}` : `@\u200B${login}`)) + .join(" "); + return [ + `> ๐Ÿ“ฃ ${people}: the \`${stripStateMarkers(agent)}\` reviewer asks for your review. ${plainModelLine(reason)}`, + "", + ]; + }); +} + /** * `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 @@ -612,19 +689,38 @@ const AT_REFERENCE_RE = /&(?=#0*64;|#[xX]0*40;|commat;)/g; 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(/(? = { @@ -818,10 +916,14 @@ export function renderAggregateMarkdown( tag: string, dismissed: DismissalRecord[], link?: LinkContext, - opts?: { unmatchedFiles?: string[] }, + opts?: { unmatchedFiles?: string[]; mentioned?: MentionState }, feedback: FeedbackRecord[] = [], pins: FeedbackPin[] = [], ): string { + const mentionState = nextMentionState( + results.flatMap((result) => mentionHandles(result.review.mentions)), + opts?.mentioned, + ); const dismissedByFp = new Map(dismissed.map((record) => [record.fp, record])); const idOf = (result: ScopeReviewResult, finding: Finding): string => scopedFingerprint(result.isDefault ? null : result.scope, finding); @@ -937,8 +1039,12 @@ export function renderAggregateMarkdown( // @ref LLP 0011#suppression-is-never-silent โ€” a reply-suppressed scope opens // its fold so the audit note is visible, matching renderMarkdown and LLP 0010's // visible-suppression rule (else a scope replies cleared reads as clean). + // A human-review request opens it too: the people it notifies must find it. + const mentions = mentionLines(result.review.mentions, mentionState.live); const open = - shown.length > 0 || requalifiedAll.length > 0 || replies.length > 0 ? " open" : ""; + shown.length > 0 || requalifiedAll.length > 0 || replies.length > 0 || mentions.length > 0 + ? " open" + : ""; const keptCount = shown.length + hidden; lines.push( ``, @@ -946,8 +1052,9 @@ export function renderAggregateMarkdown( "", ); if (result.review.summary) { - lines.push(stripStateMarkers(result.review.summary), ""); + lines.push(modelText(result.review.summary), ""); } + lines.push(...mentions); if (withSummaries) { lines.push(...agentSummariesSection(result.review.agentSummaries)); } else if (result.review.agentSummaries?.length) { @@ -993,7 +1100,7 @@ export function renderAggregateMarkdown( for (const { finding, id, scope } of allDropped) { const suffix = droppedSuffix(dismissedByFp.get(id), feedbackById.get(id)); lines.push( - `- **${stripStateMarkers(finding.title)}** โ€” ${location(finding, link)} \`id:${id}\` _(${stripStateMarkers(scope)})_${suffix}`, + `- **${modelText(finding.title)}** โ€” ${location(finding, link)} \`id:${id}\` _(${stripStateMarkers(scope)})_${suffix}`, ); } lines.push("", "_Re-add one with `/undismiss `._", "", ""); @@ -1071,7 +1178,11 @@ export function renderAggregateMarkdown( // decided โ€” or a maintainer's restore, which no later run could recover. lines.push( ``, ); return lines.join("\n"); @@ -1126,10 +1237,13 @@ export function parseReviewState(body: string, tag: string): ReviewState | null // collectPins lifts those into the set. Without this, the first render by this // version would write a state with no pins at all and silently release every // `/undismiss` a maintainer had already made. + const { mentioned: rawMentioned, ...rest } = parsed; + const mentioned = MentionStateSchema.safeParse(rawMentioned); return { - ...parsed, + ...rest, feedback: records, pins: collectPins(parsedPins.success ? parsedPins.data : [], records), + ...(mentioned.success ? { mentioned: mentioned.data } : {}), }; } } catch { diff --git a/src/core/review.ts b/src/core/review.ts index 7f2a093..cfaa02d 100644 --- a/src/core/review.ts +++ b/src/core/review.ts @@ -65,6 +65,7 @@ import { SEVERITY_RANK, } from "./schema.js"; import type { + AgentMention, AgentSummary, CoordinatorOutput, FeedbackRecord, @@ -563,6 +564,9 @@ export async function runReview( // 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(); + // The first reason each agent gives for a human review. Only agents whose config + // names people to mention turn it into a mention. + const humanReviewReasons = 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 @@ -889,6 +893,9 @@ export async function runReview( mergeTraceNotes(agentTrace, task.bucket, value.trace); } mergeAgentSummary(agentSummaryParts, task.bucket, value.summary, fileOrder(task.files)); + if (value.humanReview && !humanReviewReasons.has(task.bucket)) { + humanReviewReasons.set(task.bucket, value.humanReview); + } for (const finding of value.findings) { const fp = fingerprintFinding(finding); if (!agentByFp.has(fp)) { @@ -1494,16 +1501,16 @@ export async function runReview( // `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. The agent - // summaries follow the same rule. + // summaries and mentions 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 outputWithTrace = attachMentions( + attachAgentSummaries(attachReviewTrace(output, reviewTrace), agentSummaries), + buildMentions(selectedAgents, humanReviewReasons), ); const reviewed = { ...outputWithTrace, @@ -1666,6 +1673,33 @@ export function attachAgentSummaries( return { ...withoutSummaries, ...(agentSummaries ? { agentSummaries } : {}) }; } +/** + * One mention per agent that asked for a human review and whose config names people + * to mention, in run order. Undefined when there are none. + */ +export function buildMentions( + agents: LoadedAgent[], + reasons: ReadonlyMap, +): AgentMention[] | undefined { + const mentions = agents.flatMap((agent) => { + const reason = reasons.get(agent.id); + return agent.mention && reason ? [{ agent: agent.id, logins: agent.mention, reason }] : []; + }); + return mentions.length > 0 ? mentions : undefined; +} + +/** + * Replace any coordinator-authored mentions with the engine-assembled list, so model + * text can never choose whom the comment notifies. + */ +export function attachMentions( + output: CoordinatorOutput, + mentions: AgentMention[] | undefined, +): CoordinatorOutput { + const { mentions: _coordinatorMentions, ...withoutMentions } = output; + return { ...withoutMentions, ...(mentions ? { mentions } : {}) }; +} + /** * Policy backstop: strip the internal risk handoff, drop findings below the * severity floor, cap by count (most severe first), and approve when nothing diff --git a/src/core/schema.ts b/src/core/schema.ts index 02aeee8..f4c5057 100644 --- a/src/core/schema.ts +++ b/src/core/schema.ts @@ -226,7 +226,10 @@ 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 { +function clipModelLine(text: unknown): string { + if (typeof text !== "string") { + return ""; + } const flat = text.replace(/\s+/g, " ").trim(); if (flat.length <= AGENT_SUMMARY_CHARS) { return flat; @@ -248,6 +251,12 @@ const ReviewerModelOutputSchema = z.object({ .describe( `One or two sentences, at most ${AGENT_SUMMARY_CHARS} characters, on what this pass reviewed and concluded`, ), + humanReview: z + .string() + .optional() + .describe( + "Only when the role prompt asks for it: one sentence on why a person should review this change", + ), }); /** @@ -261,6 +270,7 @@ export const ReviewerOutputSchema = z trace: z.unknown().optional(), researchDecisions: z.unknown().optional(), summary: z.unknown().optional(), + humanReview: z.unknown().optional(), }) .transform((output) => { const trace = ReviewerTraceNotesSchema.safeParse(output.trace); @@ -268,12 +278,14 @@ export const ReviewerOutputSchema = z .array(ResearchDecisionSchema) .max(8) .safeParse(output.researchDecisions); - const summary = typeof output.summary === "string" ? clipAgentSummary(output.summary) : ""; + const summary = clipModelLine(output.summary); + const humanReview = clipModelLine(output.humanReview); return { findings: output.findings, ...(trace.success ? { trace: trace.data } : {}), ...(researchDecisions.success ? { researchDecisions: researchDecisions.data } : {}), ...(summary ? { summary } : {}), + ...(humanReview ? { humanReview } : {}), }; }); export type ReviewerOutput = z.infer; @@ -302,6 +314,37 @@ export const AgentSummarySchema = z.object({ }); export type AgentSummary = z.infer; +/** + * A GitHub login or an `org/team` slug, without the `@`: the only text a review + * mentions. A login is at most 39 characters of letters, digits and single inner + * hyphens, with an optional Enterprise Managed User `_shortcode` suffix. A team slug + * is letters, digits and underscores joined by single hyphens. + */ +export const MENTION_TARGET_RE = + /^(?=[A-Za-z0-9_-]{1,39}(?:\/|$))[A-Za-z0-9]+(?:-[A-Za-z0-9]+)*(?:_[A-Za-z0-9]+)?(?:\/(?=[A-Za-z0-9_-]{1,100}$)[A-Za-z0-9_]+(?:-[A-Za-z0-9_]+)*)?$/; + +/** + * An agent's request for a human review: the people its config names, and the + * agent's reason. + */ +export const AgentMentionSchema = z.object({ + agent: z.string(), + logins: z.array(z.string()), + reason: z.string(), +}); +export type AgentMention = z.infer; + +/** + * Who a review comment's mention lines notify, carried in its state. `live` are the + * handles the body mentions for real, and `notified` every handle the comment has + * ever mentioned. Handles are lowercase, as GitHub compares logins. + */ +export const MentionStateSchema = z.object({ + live: z.array(z.string()), + notified: z.array(z.string()), +}); +export type MentionState = z.infer; + /** Mode-agnostic coordinator result; each Reporter decides how to render it. */ const CoordinatorModelOutputSchema = z.object({ decision: z.enum(DECISIONS), @@ -349,6 +392,11 @@ export const CoordinatorOutputSchema = CoordinatorModelOutputSchema.extend({ * decision or the finding set. */ agentSummaries: z.array(AgentSummarySchema).optional().catch(undefined), + /** + * Human reviews that agents asked for. Engine-owned: the people come from each + * agent's trusted config, never from model text. + */ + mentions: z.array(AgentMentionSchema).optional().catch(undefined), }); export type CoordinatorOutput = z.infer; diff --git a/src/reporters/github.ts b/src/reporters/github.ts index 97f2d75..263921c 100644 --- a/src/reporters/github.ts +++ b/src/reporters/github.ts @@ -755,6 +755,7 @@ export class GitHubReporter implements Reporter { records, pins, inputHash, + state?.mentioned, ), ); } @@ -802,7 +803,7 @@ export class GitHubReporter implements Reporter { this.options.commentTag, dismissed, withInlineUrls(link, inlineUrls), - { unmatchedFiles }, + { unmatchedFiles, ...(state?.mentioned ? { mentioned: state.mentioned } : {}) }, records, pins, ), @@ -1741,7 +1742,7 @@ export class GitHubReporter implements Reporter { this.options.commentTag, next.dismissed, link, - undefined, + state.mentioned ? { mentioned: state.mentioned } : undefined, next.feedback, next.pins, ) @@ -1753,6 +1754,7 @@ export class GitHubReporter implements Reporter { next.feedback, next.pins, state.inputHash, + state.mentioned, ); await this.patchComment(existing.id, body); // Additive-only inline updates for exactly the fps this action named โ€” never a diff --git a/src/reporters/terminal.ts b/src/reporters/terminal.ts index 843ea37..ee62521 100644 --- a/src/reporters/terminal.ts +++ b/src/reporters/terminal.ts @@ -65,6 +65,10 @@ export class TerminalReporter implements Reporter { out.push(this.tally(review.findings), ""); out.push(review.summary, ""); + for (const { agent, logins, reason } of review.mentions ?? []) { + out.push(this.paint(BOLD, `๐Ÿ“ฃ ${agent} asks ${logins.join(", ")} for a review:`), reason, ""); + } + const agentSummaries = review.agentSummaries ?? []; if (agentSummaries.length > 0) { out.push(this.paint(BOLD, "๐Ÿ”Ž Reviewer summaries:")); diff --git a/templates/config.jsonc b/templates/config.jsonc index 8411e9b..163dbd3 100644 --- a/templates/config.jsonc +++ b/templates/config.jsonc @@ -12,7 +12,8 @@ // shared.md (prepended to every agent + coordinator) and coordinator.md are // reserved filenames. Per-agent overrides go in each file's YAML frontmatter, // e.g. `---\nmodel: anthropic/claude-opus-5-5\n---`. `paths: ["packages/ios/**"]` - // there limits an agent to changes that touch those globs. + // there limits an agent to changes that touch those globs, and `mention: [octocat]` + // names the people the review mentions when that agent asks for a human review. // @ref LLP 0009#config-and-prompt-templates [implements] โ€” suggestions off by default, not a schema limit "policy": { diff --git a/templates/shared.md b/templates/shared.md index 86fe314..307fb39 100644 --- a/templates/shared.md +++ b/templates/shared.md @@ -217,6 +217,10 @@ 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 `@`. +`humanReview` is optional. Set it only when your role prompt asks you to request a +human review, and only when this change needs one. Write one sentence on why a person +should review it. The engine decides whom to notify, so never name a person yourself. + Return **only** a single fenced ```json code block, an object of this shape: ```json