From 174d38d82295b058f45e9334414789e054cbf3e1 Mon Sep 17 00:00:00 2001 From: ojowwalker77 Date: Wed, 24 Jun 2026 12:54:32 -0300 Subject: [PATCH 1/3] feat(pr-review): land verified reviews on a GitHub PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring Splus closer to the PR review workflow: turn the agent's verified findings into a real GitHub pull-request review (inline comments + verdict), without breaking the local-first, agent-led, no-API-key invariants. - @splus/shared/prReview.ts — deterministic diff→comment anchor mapper (buildDiffAnchorIndex / anchorFinding / buildReviewPayload) + 11 tests. Resolves file:line to a RIGHT-side anchor, folds out-of-diff findings into the summary, picks the event from the must-fix count, tags comments for re-review dedup. Pure, zero-inference: the "where", not the "what". - prReview MCP tool — read-only; returns the gh-ready payload + the `gh api .../reviews` command. The server never shells gh. - splus-pr-review skill — resolve the PR → review base..HEAD → post. Wired into install.sh for Claude/Codex/OpenCode. - Mirrored the directive into skills/review; docs/TOOLS.md + CHANGELOG. Claude-Session: https://claude.ai/code/session_01477bB25ArAEBeAfLRswLn8 --- CHANGELOG.md | 19 +++ docs/TOOLS.md | 27 ++++ install.sh | 12 +- packages/mcp/src/index.ts | 113 +++++++++++++- packages/shared/src/index.ts | 1 + packages/shared/src/prReview.test.ts | 174 +++++++++++++++++++++ packages/shared/src/prReview.ts | 225 +++++++++++++++++++++++++++ skills/pr-review/SKILL.md | 122 +++++++++++++++ skills/review/SKILL.md | 5 +- 9 files changed, 688 insertions(+), 10 deletions(-) create mode 100644 packages/shared/src/prReview.test.ts create mode 100644 packages/shared/src/prReview.ts create mode 100644 skills/pr-review/SKILL.md diff --git a/CHANGELOG.md b/CHANGELOG.md index d132f29..8236c8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,25 @@ All notable changes to Splus. Format follows [Keep a Changelog](https://keepachangelog.com); this project uses [semantic versioning](https://semver.org). +## [Unreleased] + +Closer to the PR review workflow: land a verified Splus review on a GitHub pull request. + +### Added +- **`prReview` MCP tool** + **`@splus/shared` diff-anchor mapper** — turns the agent's + verified survivors into a ready-to-post GitHub Pull Request Reviews payload. The mapper + (`buildDiffAnchorIndex` / `anchorFinding` / `buildReviewPayload`) is pure and + deterministic: it walks the PR's unified diff, resolves each finding's `file:line` to a + RIGHT-side inline anchor (multi-line within a hunk; collapses cross-hunk), folds + out-of-diff findings into the summary (never dropped), and picks the review event from + the must-fix count. The server stays read-only and never shells `gh` — it returns the + JSON and the `gh api … /reviews` command for the agent to post. The "what" (comment + prose) stays the agent's; the "where" (the anchor) is deterministic. +- **`splus-pr-review` skill** — drives the round-trip: resolve the PR (`gh pr view` → + base/head/number) → run the existing review protocol scoped to the PR's `base..HEAD` → + emit verified survivors as a real PR review (inline comments + verdict). Wired into the + installer for Claude Code / Codex / OpenCode. + ## [1.1.0] — 2026-06-09 Dynamic grounding, history facts, a checkable protocol, and memory that ages. diff --git a/docs/TOOLS.md b/docs/TOOLS.md index 3a650b4..9caac57 100644 --- a/docs/TOOLS.md +++ b/docs/TOOLS.md @@ -17,6 +17,7 @@ API key and no cloud step; the coding agent connected over stdio is the reviewer | [`mute`](#mute) | yes | Silence an entire rule for this repo. | | [`learnings`](#learnings) | no | List what's been learned on this repo. | | [`report`](#report) | no | Audit protocol coverage deterministically, then render the offline HTML report. | +| [`prReview`](#prreview) | no | Anchor verified findings to the diff → a ready-to-post GitHub PR review payload. | | [`index`](#index) | yes | Build a SCIP index for compiler-grade blast radius. | ## Typical flow @@ -265,6 +266,32 @@ Read-only. --- +## `prReview` + +The PR-native deliverable — `report`'s sibling for when the review IS a GitHub +pull request. You supply your **verified** survivors and a summary; this is the +deterministic *where* (the *what* — every comment's prose — is yours). It +recomputes the diff for `mode`/`base` (use the **same** scope you reviewed), +anchors each finding to a real **RIGHT-side** diff line, folds any **out-of-diff** +finding (an unchanged caller the change breaks) into the summary instead of +dropping it, picks the review **event** from the must-fix count (`must-fix > 0` → +`REQUEST_CHANGES`; else `COMMENT`, or `APPROVE` when `approveWhenClean`), and tags +each comment with a hidden `` marker for re-review dedup. It +**does not touch the network** — it returns the exact JSON and the `gh api … +/reviews --input` command for **you** to post. Read-only. (The `splus-pr-review` +skill drives the whole flow: resolve the PR → review `base..HEAD` → post.) + +| Param | Type | Default | Description | +|---|---|---|---| +| `root` | string | server CWD | Repo root. | +| `mode` | `working` \| `staged` \| `base` \| `all` | `working` | The scope you reviewed — recomputes the diff anchors resolve against. A PR is `base`. | +| `base` | string | — | Base ref — required when `mode: "base"` (the PR base). | +| `summary` | string | — | The review summary body (your prose; markdown + mermaid render on GitHub). | +| `approveWhenClean` | boolean | `false` | With zero must-fix, `APPROVE` rather than just `COMMENT`. | +| `findings` | object[] | — | Your verified survivors: `{ id?, tier, file, line, endLine?, body }`. Tiers drive the verdict; `body` is the comment markdown (rationale + a `suggestion` block). | + +--- + ## `index` Build a compiler-grade **SCIP index** so cross-file blast radius resolves precisely diff --git a/install.sh b/install.sh index 051d3ef..952bcd9 100755 --- a/install.sh +++ b/install.sh @@ -333,28 +333,28 @@ if [ -z "${SPLUS_NO_WIRE:-}" ] && [ -d "$INSTALL_DIR/skills" ]; then # Claude Code — native skills (auto-triggered by name + user-invocable). if command -v claude >/dev/null 2>&1 || [ -d "$HOME/.claude" ]; then mkdir -p "$HOME/.claude/skills" - for s in review prefs; do + for s in review pr-review prefs; do [ -d "$INSTALL_DIR/skills/$s" ] || continue rm -rf "$HOME/.claude/skills/splus-$s" cp -R "$INSTALL_DIR/skills/$s" "$HOME/.claude/skills/splus-$s" done - ok "Claude Code skills (splus-review, splus-prefs)" + ok "Claude Code skills (splus-review, splus-pr-review, splus-prefs)" fi # Codex — custom prompts, slash-invocable (/splus-review). if command -v codex >/dev/null 2>&1 || [ -d "$HOME/.codex" ]; then mkdir -p "$HOME/.codex/prompts" - for s in review prefs; do + for s in review pr-review prefs; do [ -f "$INSTALL_DIR/skills/$s/SKILL.md" ] || continue { skill_body "$INSTALL_DIR/skills/$s/SKILL.md"; skill_refs "$s"; } > "$HOME/.codex/prompts/splus-$s.md" done - ok "Codex prompts (/splus-review, /splus-prefs)" + ok "Codex prompts (/splus-review, /splus-pr-review, /splus-prefs)" fi # OpenCode — commands, slash-invocable (/splus-review). if command -v opencode >/dev/null 2>&1 || [ -d "$HOME/.config/opencode" ]; then mkdir -p "$HOME/.config/opencode/command" - for s in review prefs; do + for s in review pr-review prefs; do [ -f "$INSTALL_DIR/skills/$s/SKILL.md" ] || continue { printf -- '---\ndescription: %s\n---\n' "$(skill_desc "$INSTALL_DIR/skills/$s/SKILL.md")" @@ -362,7 +362,7 @@ if [ -z "${SPLUS_NO_WIRE:-}" ] && [ -d "$INSTALL_DIR/skills" ]; then skill_refs "$s" } > "$HOME/.config/opencode/command/splus-$s.md" done - ok "OpenCode commands (/splus-review, /splus-prefs)" + ok "OpenCode commands (/splus-review, /splus-pr-review, /splus-prefs)" fi fi diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index cf87604..c5a75a0 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -26,6 +26,7 @@ * floor — re-ground on the deterministic finding floor for a scope (no directive) * preferences — show the merged SPLUS.md contract (repo + ~/.splus) * report — render the review as a standalone offline HTML report (final step) + * prReview — assemble a diff-anchored GitHub PR review payload (the PR-native deliverable) * dismiss — teach Splus a finding is noise (generalizes semantically) * accept — teach Splus a finding was real (reinforces + stores recallable memory) * note — remember a discovered repo convention (→ recall) @@ -43,6 +44,7 @@ import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; import { z } from "zod"; import { applyPolicy, + buildReviewPayload, changedExportedSymbols, diffText, inspect as engineInspect, @@ -54,6 +56,7 @@ import { type InspectKind, type Report, type SplusConfig, + type VerifiedFinding, } from "@splus/shared"; import { applySuppression, @@ -94,7 +97,7 @@ When the user asks you to review code: 3. VERIFY every finding by trying to refute it against the cited line. Drop any you can't defend. Then REPORT survivors as must-fix / concern / nit with file:line and a concrete fix. 4. TEACH the repo: \`dismiss \` for noise, \`accept \` for real, \`note\` to record a convention. \`preferences\` shows the active \`SPLUS.md\`; \`recall\` surfaces what was learned here before. -Other tools: \`report\` audits your protocol coverage deterministically (pass \`keptIds\`; it sees which exports you actually inspected and which floor findings got an explicit fate) and renders the offline HTML deliverable, \`mute\` silences a rule, \`learnings\` lists what's been taught, \`index\` builds a SCIP index for precise blast radius.`; +Other tools: \`report\` audits your protocol coverage deterministically (pass \`keptIds\`; it sees which exports you actually inspected and which floor findings got an explicit fate) and renders the offline HTML deliverable, \`prReview\` assembles a diff-anchored GitHub PR review payload to post with \`gh\` (the PR-native deliverable — reviewing a pull request), \`mute\` silences a rule, \`learnings\` lists what's been taught, \`index\` builds a SCIP index for precise blast radius.`; const server = new McpServer({ name: "splus", version: VERSION }, { instructions: SERVER_INSTRUCTIONS }); @@ -285,7 +288,7 @@ function discoveryDirective(files: string[], changedSymbols: string[] = []): str "4. VERIFY — before posting anything, re-read each candidate's cited line and try to REFUTE it. Drop any you can't defend (already handled nearby, speculative, the line doesn't actually demonstrate it). A wrong comment costs more than a missed nit.", "5. REPORT — the survivors as must-fix / concern / nit with file:line and a concrete fix. Never invent a finding; every claim cites a real line.", "6. TEACH — `dismiss ` when the user agrees something is noise, `accept ` when they act on a real one — Splus learns this repo both ways.", - "7. RENDER — the deliverable that ends the review: call the `report` tool with `keptIds` (the floor ids your verified review keeps). Its response OPENS with a deterministic protocol audit — computed from this session's actual tool calls — certifying every changed export above was `inspect`ed and every floor finding got an explicit fate (kept / dismissed / accepted). Close any gap it lists, then fill the returned HTML template with the verdict + your verified survivors + the file-level impact graph, and write `splus-report.html`. One self-contained, offline file — the artifact a dev keeps next to the diff.", + "7. RENDER — the deliverable that ends the review: call the `report` tool with `keptIds` (the floor ids your verified review keeps). Its response OPENS with a deterministic protocol audit — computed from this session's actual tool calls — certifying every changed export above was `inspect`ed and every floor finding got an explicit fate (kept / dismissed / accepted). Close any gap it lists, then fill the returned HTML template with the verdict + your verified survivors + the file-level impact graph, and write `splus-report.html`. One self-contained, offline file — the artifact a dev keeps next to the diff. When the review IS a GitHub pull request (you reviewed `mode:base` against the PR base), the PR-native deliverable is `prReview` instead: it anchors your verified survivors to the diff and hands back the `gh api` payload to post them as an actual PR review (inline comments + verdict). Still close the audit first.", ].join("\n"); } @@ -623,6 +626,110 @@ server.registerTool( async ({ root, keptIds }) => ok(`${auditBlock(rootOf(root), keptIds)}\n\n${reportInstructions()}`), ); +// --- prReview (land the review on a GitHub PR) ----------------------------- + +/** + * Render the unanchored findings — those whose line isn't in the diff (an + * unchanged caller the change breaks, a whole-file concern) — as a deterministic + * markdown appendix. They can't be inline comments, so they ride in the summary + * body; never silently dropped. + */ +function unanchoredMarkdown(unanchored: VerifiedFinding[]): string { + if (!unanchored.length) return ""; + const rows = unanchored + .map((f) => `- **${f.tier}** \`${f.file}:${f.line}\` — ${f.body.replace(/\n+/g, " ")}`) + .join("\n"); + return `\n\n### Findings outside the diff\n${rows}`; +} + +server.registerTool( + "prReview", + { + title: "Assemble a GitHub PR review payload", + description: + "The PR-native deliverable: turn your VERIFIED findings into a ready-to-post GitHub Pull " + + "Request Reviews API payload, each anchored to the right diff line. YOU are still the driver — " + + "this does NOT touch the network or shell `gh`; it recomputes the diff for `mode`/`base` " + + "(use the SAME scope you reviewed), resolves each finding to a RIGHT-side inline anchor (or " + + "folds it into the summary as an out-of-diff note — never dropped), picks the review event " + + "from the must-fix count (must-fix>0 → REQUEST_CHANGES; else COMMENT, or APPROVE when " + + "`approveWhenClean`), and tags each comment with a hidden id marker so a re-review can dedup. " + + "Returns the exact JSON to POST and the `gh api` command to post it. Run AFTER you've verified " + + "every survivor — this is the report's PR-native sibling, not a substitute for the protocol.", + inputSchema: { + root: z.string().optional().describe("Repo root (default: server CWD)."), + mode: z + .enum(["working", "staged", "base", "all"]) + .optional() + .describe("The scope you reviewed — used to recompute the diff the anchors resolve against (default 'working'; a PR is 'base')."), + base: z.string().optional().describe("Base git ref — required when mode='base' (the PR base)."), + summary: z.string().describe("The review summary body (your prose; markdown + mermaid render on GitHub)."), + approveWhenClean: z + .boolean() + .optional() + .describe("When there are no must-fix findings, APPROVE rather than just COMMENT (default false)."), + findings: z + .array( + z.object({ + id: z.string().optional().describe("Floor/agent finding id — embedded as a dedup marker."), + tier: z.enum(["must-fix", "concern", "nit"]), + file: z.string().describe("Repo-relative path (matches the diff)."), + line: z.number().describe("New-side line the finding starts on."), + endLine: z.number().optional().describe("New-side end line for a multi-line finding."), + body: z.string().describe("The comment markdown — rationale + a concrete fix / ```suggestion block."), + }), + ) + .describe("Your VERIFIED survivors. Order is preserved; tiers drive the verdict."), + }, + annotations: { readOnlyHint: true, openWorldHint: false }, + }, + async ({ root, mode, base, summary, approveWhenClean, findings }) => { + const repo = rootOf(root); + const m = (mode ?? "working") as ReviewMode; + if (m === "base" && !base) return fail("mode='base' requires a `base` ref (the PR base)."); + const diff = diffText(repo, toMode(m, base ?? null)); + if (!diff.trim()) { + return fail( + `No diff for mode='${m}'${base ? ` base='${base}'` : ""} — nothing to anchor against. ` + + `(mode='all' has no diff; for a PR use mode='base' with the PR's base ref.)`, + ); + } + const payload = buildReviewPayload({ + diff, + summary, + approveWhenClean, + findings: findings as VerifiedFinding[], + }); + // Out-of-diff findings can't be inline — append them to the body so the PR + // review still carries them. Deterministic rendering; the prose was the agent's. + const postBody = payload.body + unanchoredMarkdown(payload.unanchored); + const post = { + event: payload.event, + body: postBody, + comments: payload.comments, + }; + const recipe = [ + "=== Splus · post this as a PR review (you drive the network call) ===", + `Event: ${payload.event} · ${payload.comments.length} inline comment(s)` + + (payload.unanchored.length ? ` · ${payload.unanchored.length} folded into the summary (out of diff)` : ""), + "", + "1. Resolve the PR coordinates (if you haven't):", + " gh pr view --json number,baseRefName,headRefName,headRepositoryOwner,url", + "2. Write the payload below to a file (e.g. .splus-cache/pr-review.json), then POST it:", + " gh api repos/{owner}/{repo}/pulls/{number}/reviews --method POST --input .splus-cache/pr-review.json", + " (commit_id is optional — GitHub defaults to the PR's latest commit.)", + "", + "If the API 422s on a comment, that line left the diff since you reviewed — re-run `review` on the", + "current head and rebuild. To avoid double-posting on a re-review, first minimize/resolve prior", + "Splus comments (they carry a `` marker) or skip ids already present on the PR.", + "", + "--- PAYLOAD (POST verbatim) ---", + JSON.stringify(post, null, 2), + ].join("\n"); + return ok(recipe); + }, +); + // --- dismiss --------------------------------------------------------------- server.registerTool( @@ -953,7 +1060,7 @@ async function main(): Promise { await server.connect(transport); process.stderr.write( "splus-mcp ready (stdio) — local engine, no network · you are the reviewer; " + - "tools: review, inspect, floor, preferences, report, dismiss, accept, note, recall, mute, learnings, index\n", + "tools: review, inspect, floor, preferences, report, prReview, dismiss, accept, note, recall, mute, learnings, index\n", ); } diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 1003025..5d2f41b 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -349,3 +349,4 @@ export async function changedExportedSymbols(root: string, files: string[], diff // The per-repo review contract (`SPLUS.md`): loader + binding policy. export * from "./splusMd.js"; +export * from "./prReview.js"; diff --git a/packages/shared/src/prReview.test.ts b/packages/shared/src/prReview.test.ts new file mode 100644 index 0000000..47e7a4a --- /dev/null +++ b/packages/shared/src/prReview.test.ts @@ -0,0 +1,174 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + buildDiffAnchorIndex, + anchorFinding, + reviewEvent, + buildReviewPayload, + commentMarker, +} from "./prReview.js"; + +// A small but realistic two-file unified diff: one added file region and one +// edit with surrounding context + a removed line. +const DIFF = [ + "diff --git a/src/auth.ts b/src/auth.ts", + "index 1111111..2222222 100644", + "--- a/src/auth.ts", + "+++ b/src/auth.ts", + "@@ -10,6 +10,8 @@ export function login(req) {", + " const u = req.user;", // 10 ctx + " if (!u) return null;", // 11 ctx + "- const ok = check(u.pass);", // (removed — LEFT) + "+ const ok = check(u.password);", // 12 add + "+ if (!ok) return null;", // 13 add + " return session(u);", // 14 ctx + " }", // 15 ctx + "@@ -40,3 +42,4 @@ function helper() {", + " const x = 1;", // 42 ctx + "+ const y = 2;", // 43 add + " return x;", // 44 ctx + " }", // 45 ctx + "diff --git a/src/db.ts b/src/db.ts", + "index 3333333..4444444 100644", + "--- a/src/db.ts", + "+++ b/src/db.ts", + "@@ -1,2 +1,3 @@", + "+import x from 'x';", // 1 add + " const a = 1;", // 2 ctx + " const b = 2;", // 3 ctx +].join("\n"); + +test("indexes new-side commentable lines per file (added + context, not removed)", () => { + const idx = buildDiffAnchorIndex(DIFF); + const auth = idx.get("src/auth.ts"); + assert.ok(auth); + // First hunk: 10,11 ctx, 12,13 added, 14,15 ctx all commentable on the new side. + for (const n of [10, 11, 12, 13, 14, 15]) assert.equal(auth!.commentable.has(n), true, `line ${n}`); + // Second hunk: 42..45. + for (const n of [42, 43, 44, 45]) assert.equal(auth!.commentable.has(n), true, `line ${n}`); + // A removed-only old line (the old `const ok = check(u.pass)`) is not a new-side line. + assert.equal(auth!.commentable.has(16), false); + + const db = idx.get("src/db.ts"); + assert.ok(db); + for (const n of [1, 2, 3]) assert.equal(db!.commentable.has(n), true); +}); + +test("anchors a single added line on the RIGHT", () => { + const idx = buildDiffAnchorIndex(DIFF); + assert.deepEqual(anchorFinding(idx, "src/auth.ts", 12), { + path: "src/auth.ts", + side: "RIGHT", + line: 12, + }); +}); + +test("multi-line anchor within one hunk sets start_line + line", () => { + const idx = buildDiffAnchorIndex(DIFF); + assert.deepEqual(anchorFinding(idx, "src/auth.ts", 12, 13), { + path: "src/auth.ts", + side: "RIGHT", + line: 13, + start_line: 12, + }); +}); + +test("range spanning two hunks collapses to a single-line comment on the end", () => { + const idx = buildDiffAnchorIndex(DIFF); + // 13 is in hunk 0, 43 is in hunk 1 — cross-hunk, so no start_line. + assert.deepEqual(anchorFinding(idx, "src/auth.ts", 13, 43), { + path: "src/auth.ts", + side: "RIGHT", + line: 43, + }); +}); + +test("falls back to the start line when only it is in the diff", () => { + const idx = buildDiffAnchorIndex(DIFF); + // 12 is commentable, 999 is not → anchor on 12. + assert.deepEqual(anchorFinding(idx, "src/auth.ts", 12, 999), { + path: "src/auth.ts", + side: "RIGHT", + line: 12, + }); +}); + +test("returns null for a file not in the diff and a line outside any hunk", () => { + const idx = buildDiffAnchorIndex(DIFF); + assert.equal(anchorFinding(idx, "src/missing.ts", 5), null); + assert.equal(anchorFinding(idx, "src/auth.ts", 100), null); +}); + +test("an added line whose content starts with +++ is not mistaken for a header", () => { + const diff = [ + "diff --git a/m.md b/m.md", + "--- a/m.md", + "+++ b/m.md", + "@@ -1,1 +1,2 @@", + " title", // 1 ctx + "+++ not a header, just markdown", // 2 add — content begins with +++ + ].join("\n"); + const idx = buildDiffAnchorIndex(diff); + const m = idx.get("m.md"); + assert.ok(m); + assert.equal(m!.commentable.has(2), true); + // The file is still `m.md`, not the bogus path from the content line. + assert.equal(idx.has("not a header, just markdown"), false); +}); + +test("a deleted file (+++ /dev/null) yields no commentable lines", () => { + const diff = [ + "diff --git a/gone.ts b/gone.ts", + "--- a/gone.ts", + "+++ /dev/null", + "@@ -1,2 +0,0 @@", + "-const a = 1;", + "-const b = 2;", + ].join("\n"); + const idx = buildDiffAnchorIndex(diff); + assert.equal(idx.size, 0); +}); + +test("reviewEvent maps the verdict", () => { + assert.equal(reviewEvent(2), "REQUEST_CHANGES"); + assert.equal(reviewEvent(0), "COMMENT"); + assert.equal(reviewEvent(0, true), "APPROVE"); + assert.equal(reviewEvent(1, true), "REQUEST_CHANGES"); +}); + +test("buildReviewPayload anchors, partitions unanchored, and counts must-fix for the verdict", () => { + const payload = buildReviewPayload({ + diff: DIFF, + summary: "Splus review.", + findings: [ + { id: "f-1", tier: "must-fix", file: "src/auth.ts", line: 12, body: "case-sensitive compare" }, + { id: "f-2", tier: "concern", file: "src/db.ts", line: 1, body: "unused import" }, + // Out-of-diff finding (an unchanged caller) — can't be inline. + { id: "f-3", tier: "nit", file: "src/auth.ts", line: 500, body: "caller assumes old shape" }, + ], + }); + + assert.equal(payload.event, "REQUEST_CHANGES"); // one must-fix + assert.equal(payload.comments.length, 2); + assert.equal(payload.unanchored.length, 1); + assert.equal(payload.unanchored[0]!.id, "f-3"); + + // The id marker is appended so a re-review can find and skip its own comments. + const c0 = payload.comments.find((c) => c.line === 12)!; + assert.ok(c0.body.includes(commentMarker("f-1"))); + assert.equal(c0.path, "src/auth.ts"); + assert.equal(c0.side, "RIGHT"); +}); + +test("buildReviewPayload approves a clean review when opted in", () => { + const payload = buildReviewPayload({ + diff: DIFF, + summary: "Looks good.", + approveWhenClean: true, + findings: [{ tier: "nit", file: "src/db.ts", line: 1, body: "tiny nit" }], + }); + assert.equal(payload.event, "APPROVE"); + assert.equal(payload.comments.length, 1); + // No id → no marker appended. + assert.equal(payload.comments[0]!.body, "tiny nit"); +}); diff --git a/packages/shared/src/prReview.ts b/packages/shared/src/prReview.ts new file mode 100644 index 0000000..de8ded7 --- /dev/null +++ b/packages/shared/src/prReview.ts @@ -0,0 +1,225 @@ +/** + * PR review emission — the deterministic "where" half of landing a Splus review + * on a GitHub pull request. The agent decides WHAT to say (the finding, the fix, + * the verdict's prose); this module decides WHERE each comment can legally land + * and assembles the GitHub Pull Request Reviews API payload. Pure and + * zero-inference: given the PR's unified diff, it resolves each finding's + * `file:line` to an anchor the API will accept, or reports it as un-anchorable so + * the caller folds it into the summary (never silently dropped). + * + * GitHub only accepts an inline review comment on a line that appears in the PR + * diff, identified by `line` (the file's line number) + `side` (RIGHT = the new + * version, LEFT = the old). Splus findings cite NEW-side lines, so this resolves + * RIGHT-side anchors only; a finding about an unchanged caller resolves to null + * and belongs in the summary body, not inline. + * + * This lives in `@splus/shared` rather than the Rust engine on purpose: it's the + * deterministic *rendering* of an output destination (where `gh` is shelled from + * the TS side), not finding *grounding*. It can move into the engine later + * without changing this contract. + */ + +/** A review action — drives the PR's overall state. */ +export type ReviewEvent = "APPROVE" | "REQUEST_CHANGES" | "COMMENT"; + +/** A position the GitHub Pull Request Reviews API will accept for an inline comment. */ +export interface ReviewAnchor { + path: string; + side: "RIGHT"; + /** New-side line the comment anchors to (the END of the range, per the API). */ + line: number; + /** Set only for a multi-line comment whose start is in the SAME hunk as `line`. */ + start_line?: number; +} + +/** Per-file record of which new-side lines a comment can legally anchor to. */ +interface FileAnchors { + /** New-side line numbers present in the diff (added or context) — RIGHT-commentable. */ + commentable: Set; + /** New-side line → index of the hunk it belongs to (multi-line comments can't cross hunks). */ + hunk: Map; +} + +/** Index of commentable new-side lines per file, built from a unified diff. */ +export type DiffAnchorIndex = Map; + +// `+++ b/path` file header — only meaningful BEFORE a hunk opens (inside a hunk +// an added line of content can also start with `+++`, so state-guard the match). +const FILE_RE = /^\+\+\+ (.+)$/; +// `@@ -old,oldCount +new,newCount @@` — we only need the new-side start. +const HUNK_RE = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/; + +/** Git prefixes the new path with `b/` by default; repo-relative paths drop it. */ +function normalizePath(p: string): string { + if (p.startsWith("b/") || p.startsWith("a/")) return p.slice(2); + return p; +} + +/** + * Walk a unified diff line-by-line, tracking the running new-side line number, and + * record every new-side line that appears in the diff (added `+` or context ` `). + * Those — and only those — are the lines GitHub will accept an inline RIGHT-side + * comment on. Removed (`-`) lines never advance the new side and aren't included. + */ +export function buildDiffAnchorIndex(diff: string): DiffAnchorIndex { + const index: DiffAnchorIndex = new Map(); + let file: string | null = null; + let newLine = 0; + let hunkIdx = -1; + let inHunk = false; + + for (const raw of diff.split("\n")) { + // A new file section resets parsing — the next `+++` is a header again. + if (raw.startsWith("diff --git")) { + inHunk = false; + file = null; + continue; + } + // File header is only a header before the first hunk of its section; inside a + // hunk, a line starting with `+++` is added CONTENT and must not match here. + if (!inHunk) { + const fileMatch = raw.match(FILE_RE); + if (fileMatch) { + const target = fileMatch[1]!; + // `+++ /dev/null` is a deletion: no new side to comment on. + file = target === "/dev/null" ? null : normalizePath(target); + continue; + } + } + const hunkMatch = raw.match(HUNK_RE); + if (hunkMatch) { + newLine = Number(hunkMatch[1]); + hunkIdx++; + inHunk = true; + continue; + } + if (!inHunk || file === null) continue; + + const marker = raw[0]; + if (marker === "\\") continue; // "\ No newline at end of file" — not a real line + if (marker === "-") continue; // removed (LEFT only) — doesn't advance the new side + if (marker === "+" || marker === " " || raw === "") { + // added, context, or a blank context line: present on the new side, commentable. + const fa = index.get(file) ?? { commentable: new Set(), hunk: new Map() }; + fa.commentable.add(newLine); + fa.hunk.set(newLine, hunkIdx); + index.set(file, fa); + newLine++; + } + // Anything else inside a well-formed hunk shouldn't occur — ignore defensively. + } + return index; +} + +/** + * Resolve a finding's `file:startLine[..endLine]` to a GitHub review anchor, or + * `null` when no part of the range appears in the diff (an out-of-diff finding — + * e.g. an unchanged caller the change breaks — which the caller surfaces in the + * summary instead). Anchors `line` to the END of the range per the API; sets the + * multi-line `start_line` only when both ends are commentable AND in the same hunk. + */ +export function anchorFinding( + index: DiffAnchorIndex, + file: string, + startLine: number, + endLine?: number, +): ReviewAnchor | null { + const fa = index.get(file); + if (!fa) return null; + const end = endLine ?? startLine; + const lo = Math.min(startLine, end); + const hi = Math.max(startLine, end); + + const hiOk = fa.commentable.has(hi); + const loOk = fa.commentable.has(lo); + if (!hiOk && !loOk) return null; + if (!hiOk) return { path: file, side: "RIGHT", line: lo }; + if (!loOk || lo === hi) return { path: file, side: "RIGHT", line: hi }; + // Both ends are in the diff but in different hunks: the API rejects a + // cross-hunk range, so collapse to a single-line comment on the end line. + if (fa.hunk.get(lo) !== fa.hunk.get(hi)) return { path: file, side: "RIGHT", line: hi }; + return { path: file, side: "RIGHT", line: hi, start_line: lo }; +} + +/** A verified survivor of the review, ready to post. `body` is agent-authored. */ +export interface VerifiedFinding { + /** Floor/agent id — embedded as a hidden marker so a re-review can dedup. */ + id?: string; + tier: "must-fix" | "concern" | "nit"; + file: string; + line: number; + endLine?: number; + /** The comment markdown the agent wrote (the "what"): rationale + a fix/suggestion. */ + body: string; +} + +/** One inline comment in the reviews-API payload. */ +export interface ReviewComment { + path: string; + side: "RIGHT"; + line: number; + start_line?: number; + body: string; +} + +/** The assembled GitHub Pull Request Reviews API request body. */ +export interface ReviewPayload { + event: ReviewEvent; + body: string; + comments: ReviewComment[]; + /** Findings that could not be anchored to a diff line — fold these into `body`. */ + unanchored: VerifiedFinding[]; +} + +/** + * The verdict → review state map. Any unresolved must-fix requests changes; + * a clean review either approves (when the caller opts in) or just comments. + */ +export function reviewEvent(mustFix: number, approveWhenClean = false): ReviewEvent { + if (mustFix > 0) return "REQUEST_CHANGES"; + return approveWhenClean ? "APPROVE" : "COMMENT"; +} + +/** Hidden dedup marker appended to a comment body, keyed by finding id. */ +export function commentMarker(id: string): string { + return ``; +} + +/** + * Assemble the reviews-API payload from the verified survivors and the + * agent-authored summary. Deterministic: it anchors each finding (or files it + * under `unanchored`), counts must-fix to pick the event, and tags each comment + * with a hidden id marker for re-review dedup. The caller fills the summary prose + * (and is expected to weave in the returned `unanchored` findings). + */ +export function buildReviewPayload(args: { + diff: string; + findings: VerifiedFinding[]; + summary: string; + approveWhenClean?: boolean; +}): ReviewPayload { + const index = buildDiffAnchorIndex(args.diff); + const comments: ReviewComment[] = []; + const unanchored: VerifiedFinding[] = []; + let mustFix = 0; + + for (const f of args.findings) { + if (f.tier === "must-fix") mustFix++; // verdict counts ALL findings, anchored or not + const anchor = anchorFinding(index, f.file, f.line, f.endLine); + if (!anchor) { + unanchored.push(f); + continue; + } + comments.push({ + ...anchor, + body: f.id ? `${f.body}\n\n${commentMarker(f.id)}` : f.body, + }); + } + + return { + event: reviewEvent(mustFix, args.approveWhenClean), + body: args.summary, + comments, + unanchored, + }; +} diff --git a/skills/pr-review/SKILL.md b/skills/pr-review/SKILL.md new file mode 100644 index 0000000..3e7e436 --- /dev/null +++ b/skills/pr-review/SKILL.md @@ -0,0 +1,122 @@ +--- +name: Splus PR Review +description: This skill should be used when the user wants the review to land ON a GitHub pull request — "review PR #123", "review this pull request and post comments", "leave a Splus review on the PR", "post the review to GitHub", "approve/request changes on the PR". Runs the agent-led Splus protocol against the PR's base..HEAD diff, then posts the verified survivors as a real GitHub PR review (inline comments + verdict) with `gh`. No API key. +user-invocable: true +allowed-tools: + - mcp__splus__review + - mcp__splus__inspect + - mcp__splus__floor + - mcp__splus__preferences + - mcp__splus__recall + - mcp__splus__prReview + - mcp__splus__report + - mcp__splus__dismiss + - mcp__splus__accept + - mcp__splus__note + - mcp__splus__mute + - Task + - Read + - Grep + - Glob + - Bash +--- + +# Splus PR Review + +The review is the same disciplined, precision-first review the `splus-review` +skill runs — this skill only changes the **input** (a GitHub pull request, not +your working tree) and the **output** (a real PR review with inline comments and +a verdict, not a local HTML file). Everything in between — ground, investigate in +fresh sub-agents, verify by refutation, teach — is identical. **A wrong comment +on someone's PR costs more than a missed nit.** Post nothing you didn't verify. + +This needs `gh` authenticated (`gh auth status`). The Splus engine and reasoning +stay 100% local; the only network call is the `gh` round-trip YOU make to post. + +## 0. Resolve the PR — what am I reviewing, and where do comments land? + +```sh +# Current branch's PR (omit the number), or a specific one: +gh pr view [] --json number,baseRefName,headRefName,headRefOid,url,title,body +git fetch origin # ensure the base ref exists locally +``` + +You need: the **base ref** (drives the diff scope), the PR **number** and repo +**owner/name** (where the review posts), and the **title/body** (the stated +intent — the only narrative your reviewers should trust). If the head isn't +checked out locally, `gh pr checkout ` first. + +## 1–4. Run the review protocol against the PR diff + +Run the full `splus-review` flow, scoped to the PR: + +- **Ground:** `review` with `mode:base` and `base:`. That's the + PR-correct range (`base...HEAD`, merge-base). Read `SPLUS.md` first — it + overrides engine defaults and your taste. +- **Investigate:** partition the changed files into units and spawn **fresh + `Task` sub-agents** that see the diff cold (the protocol in + `references/investigate.md` of the `splus-review` skill). Hand each only its + files, the contract, the floor, and the PR title/body — never an + implementation narrative. Each `inspect`s callers / blast radius / complexity + and `recall`s prior burns instead of guessing. +- **Verify:** a separate refutation pass re-reads each candidate's cited line and + drops anything it can't defend (`references/verify.md`). This gate matters more + here — the survivors become public comments. + +Trace every changed exported contract into its callers (return-shape drift is the +most-missed real bug), and post **no checklist padding** — generic hardening that +the diff didn't introduce is noise that wastes a reviewer's attention. + +## 5. Emit — land the verified review on the PR + +Write each survivor's comment markdown (the **what**): the rationale and a +concrete fix — prefer a GitHub ` ```suggestion ` block so the author can commit it +in one click. Then call **`prReview`** (the deterministic **where**): + +``` +prReview( + mode: "base", base: "", + approveWhenClean: , + summary: "", + findings: [ { id, tier, file, line, endLine?, body }, … ] // your VERIFIED survivors +) +``` + +`prReview` anchors each finding to a real diff line (RIGHT side), folds any +**out-of-diff** finding (an unchanged caller the change breaks) into the summary +instead of dropping it, picks the review **event** from the must-fix count +(`must-fix>0` → `REQUEST_CHANGES`; else `COMMENT`, or `APPROVE` when you opted +in), tags each comment with a hidden `` marker, and hands back +the exact JSON plus the `gh` command. Post it: + +```sh +# write the returned PAYLOAD to .splus-cache/pr-review.json, then: +gh api repos/{owner}/{repo}/pulls/{number}/reviews --method POST --input .splus-cache/pr-review.json +``` + +- **Verdict prose** opens the summary: SAFE TO MERGE (0 must-fix) or CHANGES + REQUESTED with the count, then 1–3 sentences on what you reviewed and how blast + radius resolved (heuristic vs SCIP). The mermaid graph (modules = nodes, impact + = edges) is the PR-native version of the HTML report's centerpiece. +- **A 422 on a comment** means that line left the diff since you reviewed — + re-run `review` on the current head (`headRefOid`) and rebuild the payload. +- **Re-review on a pushed update:** don't double-post. First minimize/resolve + your prior Splus comments (they carry the `` marker), or skip + ids already present on the PR. Reviewing only the new commits keeps it quiet. + +> The HTML `report` and `prReview` are siblings, not rivals: `report` is the +> offline artifact a dev keeps next to the diff; `prReview` is the review that +> lives on the PR. Use `prReview` when the deliverable is the pull request itself. + +## 6. Teach — make diligence compound + +- `dismiss ` when the author convincingly pushes back (it was noise). +- `accept ` when they act on a real finding (reinforces + becomes recallable). +- `note ""` for anything you learned about the repo. +- `mute ` when a whole class is unwanted here. + +## The standard you're held to +Same as `splus-review`, raised: every posted comment cites a real line, survived +a refutation pass, and anchors to the diff. Every floor finding got an explicit +fate. Nothing generic, nothing unverified, nothing silently dropped. The review +that lands on the PR is the one you'd defend in the thread. diff --git a/skills/review/SKILL.md b/skills/review/SKILL.md index 2d47f1a..80afe21 100644 --- a/skills/review/SKILL.md +++ b/skills/review/SKILL.md @@ -80,7 +80,10 @@ with a deterministic protocol audit** computed from this session's actual tool calls: changed exports never `inspect`ed, floor findings with no explicit fate (kept / dismissed / accepted). The audit can't be talked past — close every gap it lists, then fill the returned HTML template — the offline artifact the dev -keeps next to the diff. +keeps next to the diff. When the review IS a GitHub pull request, the PR-native +deliverable is `prReview` instead (post the verified survivors as inline comments ++ a verdict with `gh`) — use the `splus-pr-review` skill, which runs this same +protocol scoped to the PR's `base..HEAD`. ### 5. Teach — make diligence compound - `dismiss ` when the user agrees something is noise (generalizes). From 4c9dc6abf05a946ffbe2a3a7b1bbc0473a9f17ca Mon Sep 17 00:00:00 2001 From: ojowwalker77 Date: Wed, 24 Jun 2026 12:58:30 -0300 Subject: [PATCH 2/3] fix(pr-review): drop phantom commentable line from trailing diff newline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The self-review on PR #11 caught it: `git diff` output is newline-terminated, so `split("\n")` leaves a trailing "". Inside the last file's last hunk that empty string was classified as a context line, marking one line past the final hunk as anchorable (an inline comment there would 422). Git always emits a space marker for context lines, so a bare "" inside a hunk is only the split artifact — drop the `raw === ""` clause. Adds a regression test. Claude-Session: https://claude.ai/code/session_01477bB25ArAEBeAfLRswLn8 --- packages/shared/src/prReview.test.ts | 10 ++++++++++ packages/shared/src/prReview.ts | 6 ++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/shared/src/prReview.test.ts b/packages/shared/src/prReview.test.ts index 47e7a4a..859aa03 100644 --- a/packages/shared/src/prReview.test.ts +++ b/packages/shared/src/prReview.test.ts @@ -129,6 +129,16 @@ test("a deleted file (+++ /dev/null) yields no commentable lines", () => { assert.equal(idx.size, 0); }); +test("a trailing newline (real git diff) does not add a phantom line past the last hunk", () => { + // `git diff` output is newline-terminated, so `split("\n")` leaves a trailing "". + // It must not be classified as a context line on the diff's last file. + const diff = "diff --git a/x.ts b/x.ts\n--- a/x.ts\n+++ b/x.ts\n@@ -1,1 +1,2 @@\n a\n+b\n"; + const idx = buildDiffAnchorIndex(diff); + const x = idx.get("x.ts")!; + assert.deepEqual([...x.commentable].sort((a, b) => a - b), [1, 2]); // not [1, 2, 3] + assert.equal(anchorFinding(idx, "x.ts", 3), null); +}); + test("reviewEvent maps the verdict", () => { assert.equal(reviewEvent(2), "REQUEST_CHANGES"); assert.equal(reviewEvent(0), "COMMENT"); diff --git a/packages/shared/src/prReview.ts b/packages/shared/src/prReview.ts index de8ded7..cc9fcb3 100644 --- a/packages/shared/src/prReview.ts +++ b/packages/shared/src/prReview.ts @@ -98,8 +98,10 @@ export function buildDiffAnchorIndex(diff: string): DiffAnchorIndex { const marker = raw[0]; if (marker === "\\") continue; // "\ No newline at end of file" — not a real line if (marker === "-") continue; // removed (LEFT only) — doesn't advance the new side - if (marker === "+" || marker === " " || raw === "") { - // added, context, or a blank context line: present on the new side, commentable. + if (marker === "+" || marker === " ") { + // added or context: present on the new side, commentable. (A blank context + // line carries the " " marker too; a bare "" is only the trailing artifact + // `git diff`'s final newline leaves after split — never a real line.) const fa = index.get(file) ?? { commentable: new Set(), hunk: new Map() }; fa.commentable.add(newLine); fa.hunk.set(newLine, hunkIdx); From fe674fd1a2d97de0b0d1058ea8059e8806553979 Mon Sep 17 00:00:00 2001 From: ojowwalker77 Date: Wed, 24 Jun 2026 16:37:28 -0300 Subject: [PATCH 3/3] fix(pr-review): harden path normalization for git mnemonic prefixes normalizePath only stripped the default a/ b/ prefixes; under diff.mnemonicPrefix git emits c/ i/ o/ w/, so findings silently fell to the summary instead of anchoring inline. Strip the full one-letter prefix set. Fails safe under diff.noprefix (bare paths left as-is). Adds a test. Claude-Session: https://claude.ai/code/session_01477bB25ArAEBeAfLRswLn8 --- packages/shared/src/prReview.test.ts | 8 ++++++++ packages/shared/src/prReview.ts | 10 ++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/shared/src/prReview.test.ts b/packages/shared/src/prReview.test.ts index 859aa03..9e6165a 100644 --- a/packages/shared/src/prReview.test.ts +++ b/packages/shared/src/prReview.test.ts @@ -116,6 +116,14 @@ test("an added line whose content starts with +++ is not mistaken for a header", assert.equal(idx.has("not a header, just markdown"), false); }); +test("strips git mnemonic prefixes (diff.mnemonicPrefix) so paths match findings", () => { + // Under `diff.mnemonicPrefix` the new side is `w/`/`i/`/`c/`, not `b/`. + const diff = ["diff --git i/src/x.ts w/src/x.ts", "--- i/src/x.ts", "+++ w/src/x.ts", "@@ -1,1 +1,2 @@", " a", "+b"].join("\n"); + const idx = buildDiffAnchorIndex(diff); + assert.equal(idx.has("src/x.ts"), true); // not "w/src/x.ts" + assert.deepEqual(anchorFinding(idx, "src/x.ts", 2), { path: "src/x.ts", side: "RIGHT", line: 2 }); +}); + test("a deleted file (+++ /dev/null) yields no commentable lines", () => { const diff = [ "diff --git a/gone.ts b/gone.ts", diff --git a/packages/shared/src/prReview.ts b/packages/shared/src/prReview.ts index cc9fcb3..3459a7b 100644 --- a/packages/shared/src/prReview.ts +++ b/packages/shared/src/prReview.ts @@ -49,9 +49,15 @@ const FILE_RE = /^\+\+\+ (.+)$/; // `@@ -old,oldCount +new,newCount @@` — we only need the new-side start. const HUNK_RE = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/; -/** Git prefixes the new path with `b/` by default; repo-relative paths drop it. */ +// Git tags diff paths with a one-letter prefix + `/`: `a`/`b` by default, or the +// mnemonic `c`(commit) `i`(index) `o`(object) `w`(worktree) under +// `diff.mnemonicPrefix`. Strip it so the path matches a finding's repo-relative +// one. (Under `diff.noprefix` there's no prefix and a bare path is left as-is; +// the only residual ambiguity is a no-prefix repo whose top-level dir is a single +// letter in this set — rare enough to accept, and it fails safe to the summary.) +const GIT_DIFF_PREFIXES = new Set(["a", "b", "c", "i", "o", "w"]); function normalizePath(p: string): string { - if (p.startsWith("b/") || p.startsWith("a/")) return p.slice(2); + if (p.length > 2 && p[1] === "/" && GIT_DIFF_PREFIXES.has(p[0]!)) return p.slice(2); return p; }