Qv PR Review

PR Review

Manual-trigger PR review for any GitHub PR in the configured repository. Produces:

1. A short overview in chat (for the user — not posted anywhere).
2. A PENDING GitHub review containing only per-file:line inline comments (no review body).

The user submits the pending review manually from the GitHub UI.

When to use this skill

Use when:

• User asks to review a PR or provides a PR URL
• User invokes /qv-pr-review
• Triggered as follow-up from /qv-sdk-pr-status, /qv-pr-mine, or another pod's status/my skill

The skill applies to any PR; scoped repository instructions and PR-template formats applicable to the touched paths are discovered dynamically (see step 4 / step 5).

Inputs

• Required: PR URL (e.g. https://github.com/tetherto/qvac/pull/1234)
• Optional: user-provided notes/comments to seed the review (focus areas, specific concerns)

If PR URL is missing, ask for it. Nothing else to ask unless the user's seed notes are ambiguous.

Review philosophy

Carefully check the changes, focusing on:

1. High-risk issues — bugs that corrupt data, break security/auth, violate gitflow, or fail CI.
2. Potential bugs — logic errors, wrong types, missing error handling, race conditions, off-by-one, unhandled edge cases.
3. Unintentional or non-obvious caveats — subtle behavior changes that aren't called out (default flips, silent fallbacks, ordering changes, hidden coupling, regex gaps, schema gaps).
4. Breaks to existing functionality — changes that look additive but alter behavior of existing callers (signature changes, default changes, removed branches, semantically different return values).

Style nits, doc polish, and unverified hunches are NOT the focus. Don't pad the review with them.

Severity tiers

Use these tiers when assembling findings. Tiers drive what surfaces in the chat overview and what is proposed for inline comments. The user always has final say over what gets posted.

• High — almost-certain bug, security issue, gitflow blocker, CI failure, or break of existing behavior. Surfaces in chat overview. Proposed for inline by default.
• Medium — likely bug, non-obvious caveat, missing test coverage on a risky path, or a subtle behavior change. Surfaces in chat overview. Proposed for inline by default.
• Low — style, minor docstring drift, optional ergonomic improvement. Surfaces in chat overview when material (informative for the reviewer). Not proposed for inline by default — only added if the user explicitly opts them in.

Selection rule: never silently include a Low finding in the inline payload. The user picks (see step 7b).

Safety rules — DO NOT TOUCH THE USER'S LOCAL REPO

This skill is read-only with respect to the user's local working tree. The user may have uncommitted changes, be on a feature branch, or have pending work — never disturb it.

Forbidden commands (no matter the circumstance):

• git switch, git checkout (any ref/file)
• git reset (any mode), git restore
• git stash (push, pop, drop, anything)
• git pull, git merge, git rebase, git cherry-pick
• git clean
• gh pr checkout
• Any write to files inside the user's working tree

Worktree mode carve-out

/qv-pr-review runs by default in worktree mode (see step 0a). The dedicated cache directory at ~/.cache/qvac-pr-review/ is fully isolated from the user's working tree — it lives outside the repo entirely. Inside that cache directory only, the shared script (worktree-prepare.mjs) is allowed to:

• git fetch <remote> "pull/<n>/head:refs/pr/<n>/head"
• git worktree add --detach <cache-path> refs/pr/<n>/head
• git -C <cache-path> reset --hard refs/pr/<n>/head (when SHA drifted)
• git -C <cache-path> reset --hard HEAD (when the cached worktree is dirty at the same SHA)
• git -C <cache-path> clean -fdx (only after SHA drift, to evict stale untracked build/test artifacts)
• git worktree remove --force <cache-path> and git worktree prune
• Read-only diagnostics: git -C <cache-path> rev-parse|log|show|diff|status

These run from the script, not from the agent. The agent itself MUST NOT run any of the forbidden commands above — including inside the cache path. The agent only Reads/Greps/Globs source files in the cache path and never writes to them during /qv-pr-review.

The same cache path is also used by /qv-pr-test, so it may contain untracked build/test artifacts such as node_modules, dist, native build/ directories, or logs. Those artifacts are ignored by /qv-pr-review; the patch is computed from committed refs (<BASE_REF>...HEAD), not from the worktree's unstaged or untracked state.

File access rules

If you need PR file contents:

• Worktree mode (default): Read/Grep/Glob files at the path printed by step 0a (<cache-path>/...). The path is at the PR head SHA.
• Fallback (or --no-worktree): gh api repos/{owner}/{repo}/contents/{path}?ref={sha} and write to /tmp/.

Read repository instructions and conventions from the user's current workspace as-is — do not switch branches to "get the latest" version.

Efficiency rules

Every shell call costs a user approval. Keep the total small (~5-8 calls).

• Use dedicated tools, not shell. Read instead of cat/head/tail, Grep instead of grep/rg, Glob instead of find, Write instead of echo > / heredoc.
• Do not gh pr checkout. Use worktree mode (default, see step 0a) for full local context at the PR head SHA. The cache lives under ~/.cache/qvac-pr-review/ and never touches the user's working tree.
• Fetch each piece of data ONCE. Save PR JSON / patch to /tmp/pr-<num>.json and /tmp/pr-<num>.patch, reuse via Read/Grep. The worktree path is reused across step calls; don't re-prepare it.
• Skip gh pr checks — statusCheckRollup in gh pr view --json already has every check.
• Fetch CI logs only for failing jobs, not every job. One gh run view --log-failed --job <id> per failing job.
• Never run encoding forensics (file, od, wc -c, cat -A) unless a CI log explicitly names an encoding issue.

Workflow

Copy this checklist and track progress:

- [ ] 0a. Prepare worktree (default-on; skip if user passed --no-worktree)
- [ ] 1. Parse PR URL
- [ ] 2. Fetch PR data (2 shell calls)
- [ ] 3. Validate gitflow
- [ ] 4. Read applicable repository instructions for the touched paths
- [ ] 5. Validate PR title + body against the discovered format rules
- [ ] 6. Review: CI + general + security + rules — classify findings by severity
- [ ] 6b. Apply SDK plugin checklist (only if PR touches plugin paths)
- [ ] 7a. Print risk overview in chat (high + medium + material lows)
- [ ] 7b. Ask user which findings to include as inline comments (high+medium pre-selected, lows opt-in)
- [ ] 8. Assemble inline comments + write payload (only the user-confirmed set)
- [ ] 9. Pre-flight check (count, files, line numbers)
- [ ] 10. Show gh api command, wait for user confirmation
- [ ] 11. POST the PENDING review
- [ ] 12. Output link to pending review

0a. Prepare worktree (default-on)

Worktree mode is the default — full local Read/Grep/Glob context at the PR head SHA, isolated under ~/.cache/qvac-pr-review/, never touches the user's working tree. The same script also fetches the PR's base ref and writes the canonical PR diff to /tmp/. Skip this step only if the user invoked /qv-pr-review URL --no-worktree.

node .agents/skills/_lib/pr-skills/worktree-prepare.mjs <PR-URL>

Parse the script's output:

• stdout on success has four lines:

  WORKTREE_PATH=<absolute path>
  HEAD_SHA=<sha>
  PATCH_PATH=/tmp/pr-<num>.patch
  BASE_REF=<remote>/<baseRefName>

  • WORKTREE_PATH: the working root for files at the PR head SHA. Use this for all Read/Grep/Glob in steps 6 and 7a.
  • PATCH_PATH: a unified diff computed locally with git diff <BASE_REF>...HEAD (3-dot). 3-dot semantics match GitHub's PR view exactly — only what the PR introduces, regardless of how far behind the base the PR is. Use this anywhere the workflow refers to the patch; do NOT use 2-dot.
  • BASE_REF: the local tracking ref the diff was computed against (e.g. upstream/main). Useful if you need to re-run a custom diff inside the worktree.

• stderr on failure has a single line:

  WORKTREE_FALLBACK=<one-line reason>

  The script's exit code is 0 even on failure. If you observe WORKTREE_FALLBACK, fall back to the API-only flow: fetch file contents via gh api repos/{owner}/{repo}/contents/{path}?ref={headRefOid} and the patch via gh pr diff <num> --patch > /tmp/pr-<num>.patch. Surface the fallback reason once in the chat overview's ### Verified (no action) section so the user knows local context is missing — e.g. "Worktree prep failed (<reason>); excerpts come from gh api."

When the user passes --no-worktree, skip this step entirely and use the API-only flow without surfacing any fallback note.

1. Parse PR URL

Extract owner, repo, pr_number from the URL. If ~/.config/qvac-pr-skills/config.json exists, verify the PR repo matches github.repo; otherwise use the repo in the provided PR URL.

2. Fetch PR metadata

gh pr view <num> --repo tetherto/qvac \
  --json number,title,state,mergeable,baseRefName,headRefName,headRefOid,isCrossRepository,headRepositoryOwner,files,author,body,statusCheckRollup \
  > /tmp/pr-<num>.json

In worktree mode (default), the patch is already at /tmp/pr-<num>.patch from step 0a — do NOT re-fetch it via gh pr diff.

In --no-worktree mode (or after a WORKTREE_FALLBACK), additionally:

gh pr diff <num> --repo tetherto/qvac --patch > /tmp/pr-<num>.patch

Everything else comes from these files via Read/Grep. No additional shell calls for PR data.

3. Gitflow validation

Read baseRefName, headRefName, isCrossRepository, headRepositoryOwner from /tmp/pr-<num>.json. Full gitflow rules are in docs/gitflow.md.

Allowed directions (fork to upstream):

Blocker patterns:

• release-* to main — WRONG
• main to release-* — WRONG
• release-* to release-* — WRONG
• feature-* / tmp-* to main — WRONG
• main to feature-* / tmp-* — WRONG
• Head branch in upstream org (not a fork) — flag as suspicious

Release-PR extra checks (base is release-<pkg>-<x.y.z>):

• packages/<pkg>/package.json version must increase vs base
• packages/<pkg>/CHANGELOG.md must be updated
• Verify patch fixes already landed on main (cherry-picked commits)

4. Read applicable repository instructions for the touched paths

Use file-reading tools, not shell commands, for instruction discovery:

1. Read the root AGENTS.md.
2. For every touched path from /tmp/pr-<num>.json (files[].path), read each nested AGENTS.md between the repository root and that path. The nearest file has the most specific guidance.
3. Follow links from those instruction files to the relevant package README, contribution guide, architecture document, workflow, or configuration source.
4. If .github/teams/<pod>.json has ownedPaths matching the touched files, use that file for current pod scope and ownership rather than a copied package list.

5. Validate PR title + body against discovered format rules

If scoped instructions or the applicable PR template define a format, validate the PR title and body against it. Common shape (used by the SDK pod and likely others):

Title (format: TICKET prefix[tag]: subject or prefix[notask]: subject):

• Prefix: feat fix doc test chore infra
• Tags (not combinable): [api] [bc] [mod] [notask] [skiplog]
• [api] required when diff adds new exports/public API surface
• [bc] required when diff removes/changes existing public API signatures
• [mod] required when model constants change

Body — use the matching .github/PULL_REQUEST_TEMPLATE/<template>.md if one is referenced by the loaded rule:

• "What problem" describes user impact, not implementation
• "How it solves" is high-level, not line-by-line
• [bc] requires BEFORE/AFTER code blocks
• [api] requires usage example
• [mod] requires Added/Removed models list
• Unused template sections deleted

If no format rule applies, skip this step. Title/body violations go in the chat overview only, not as inline PR comments.

6. Review dimensions

Apply the review philosophy. Classify every finding as High, Medium, or Low. Skip any dimension with no findings.

• Gitflow: wrong merge direction, missing version bump/changelog on release PRs (almost always High)
• Lockfile drift: any touched package.json whose dependency specifiers changed, with no pnpm-lock.yaml in the same PR. CI installs frozen, so the stale lockfile aborts the install before the project matrix is built and blocks every open PR, not only this one. Check the package.json hunks for changed specifiers rather than assuming a manifest edit implies one; a scripts or files change needs no lockfile update (High)
• CI: non-green checks (ignore *Approval*/approval-worker). Name the failing job + actual error. For failing jobs only: gh run view --repo tetherto/qvac --log-failed --job <job_id> > /tmp/pr-<num>-<job_id>.log (High)
• Bugs / correctness: logic errors, wrong types, off-by-one, missing error handling, race conditions, unhandled edge cases (High or Medium)
• Breaks to existing functionality: changes that look additive but alter callers' behavior — signature changes, default flips, removed branches, semantically different return values (High)
• Non-obvious caveats: silent fallbacks, schema/regex gaps, validation that doesn't actually validate the property the test name implies, hidden ordering/coupling, addon contract drift (Medium)
• Security: injection, secrets in code/logs, auth/authz, unsafe deserialization, SSRF, prototype pollution, crypto misuse, dependency risks (High unless clearly informational)
• Test coverage on risky paths: a new feature or refactor that ships without tests proving the risky bit (Medium)
• Docs website: a user-facing packages/sdk / packages/cli / packages/sdk-python change with no docs/website/content/docs diff and no library-only note in the body — point at /qv-docs-update (Medium). Skip reference/**-only diffs: those are generated by the qv-sdk-changelog skill on release
• Repo rules: violations of the repository instructions loaded in step 4 (Low unless they hide a bug)
• User notes: address seed notes explicitly if provided. Prioritize per the user's framing.

When verifying a suspected bug, attempt to construct a concrete reproduction (input → code path → observed behavior). If you cannot, classify it Medium at most and say so.