feat(pr-review): land verified reviews on a GitHub PR#11
Conversation
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
ojowwalker77
left a comment
ojowwalker77
left a comment
There was a problem hiding this comment.
Splus review — 💬 comment (0 must-fix)
Reviewed main...HEAD (9 files, +688) at SCIP-precise tier. The PR is the test of its own feature — and the feature found a real bug in itself: buildDiffAnchorIndex treats the trailing "" that git diff leaves after split("\n") as a context line, so the last file in any diff gets one phantom commentable line past its final hunk (an inline comment there would 422). One concern below, plus the engine's complexity nit. No must-fix → comment, not request-changes.
flowchart LR
pr["prReview.ts<br/>mapper · 1 concern"]:::hot --> idx["index.ts<br/>re-export"]
idx --> mcp["mcp/index.ts<br/>prReview tool"]
skill["skills/pr-review"] -.drives.-> mcp
classDef hot fill:#3a0d0d,stroke:#e5534b,color:#fff;
Posted by the new prReview flow (payload built by @splus/shared, posted via gh api).
| 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 === "") { |
There was a problem hiding this comment.
concern · maintainability/correctness — phantom commentable line
diff.split("\n") on real git diff output (always newline-terminated) yields a trailing "". Inside the last file's last hunk, inHunk is still true, so this raw === "" clause classifies that empty string as a context line: it adds newLine to commentable and advances. Result: one line past the final hunk is reported anchorable, and anchorFinding would emit a line GitHub rejects with 422.
Git always emits a space marker for context lines (even blank ones → " "), so an empty string inside a hunk is only ever the split artifact — drop the clause:
| if (marker === "+" || marker === " " || raw === "") { | |
| if (marker === "+" || marker === " ") { |
There was a problem hiding this comment.
Fixed in 4c9dc6a — dropped the raw === "" clause and added a regression test (a trailing newline … does not add a phantom line). The mapper found this in itself. ✅
| * 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 { |
There was a problem hiding this comment.
nit · maintainability.cognitive-complexity — complexity 26
Inherent to a line-by-line diff state machine, but reviewable-ness would improve by extracting the hunk-body line classification (the marker switch) into a small classifyBodyLine helper. Not blocking.
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
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
1 participant
Brings Splus closer to the PR review workflow: the agent's verified findings become a real GitHub PR review — inline comments anchored to the diff + a verdict — while staying local-first, agent-led, and key-free.
What's here
@splus/shareddiff-anchor mapper (prReview.ts, +11 tests) — pure/deterministicfile:line→ GitHubRIGHT-side anchor. Multi-line within a hunk, collapses cross-hunk, folds out-of-diff findings into the summary (never dropped), picks the event from the must-fix count.prReviewMCP tool — read-only; returns the gh-ready JSON + thegh api .../reviewscommand. The server never shellsgh— the agent posts.splus-pr-reviewskill — resolve the PR (gh pr view) → reviewbase..HEAD→ post. Wired intoinstall.sh.skills/review;docs/TOOLS.md+CHANGELOG.Test
This PR is itself the test — Splus reviews it via the new flow.
https://claude.ai/code/session_01477bB25ArAEBeAfLRswLn8
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.