Replace the pre-merge review cycle with owner review and a pr-review skill

[vivek7405]

Problem

Agent sessions run an automated pre-merge review cycle before reporting a PR ready: a fresh reviewer subagent over the whole diff, a minor-or-must-fix call on every finding, delta rounds bought by must-fix fixes, refuters for rejections, and an escalation ladder to a final whole-diff pass. It has been narrowed repeatedly (#1224 cut it to a single reviewer, #1180 dropped the forced minimum rounds, #1278 made the fast shape the default, #1315 removed the end-of-cycle CI read), but the shape has stayed the same and the cost has not gone away: each reviewer spawn is roughly ten minutes of wall clock, and the whole apparatus exists to approximate a review the repo owner would rather do himself.

The owner wants review ownership back. When he asks for a review, he wants it done inline by the session he asked, not delegated to a subagent and not expanded into rounds, and he wants the result to land on the PR the way a human review does, through the GitHub review API rather than as a chat reply.

Two things ride along with the removal. The code-review routing directive in the skill-routing hook restates the whole cycle, so it has to change with the skill or the two contradict each other (they already did once, which is what #1160 and test/hooks/review-loop-exit.test.mjs exist for). And the scaffold pushes a heavier version of the same idea onto users: every app webjs create generates carries a mandatory multi-round self-review loop in its agent rules plus a PR-template checkbox, and that checkbox cites a "Pre-merge self-review loop" section of CONVENTIONS.md that the scaffold does not ship, so the reference is dangling on top of being unwanted. How another team reviews a pull request is that team's call.

Design / approach

Remove the cycle, replace it with owner review, and add ONE new skill for the case where the owner asks this agent to review.

pr-review, a new skill. It reviews a PR the way a human reviewer would if they used the API instead of the dashboard: resolve the PR, read the diff and every touched file at the PR head, then submit a single review object carrying a summary plus line-anchored inline comments, using start_line + start_side for multi-line ranges and ```suggestion blocks where a concrete self-contained replacement exists. Four hard rules, all of which are the point of the change: the review runs INLINE in the session that was asked (never a subagent, never rounds), it ONLY reviews (it does not fix findings and does not resolve threads), it never waits on or reports CI (checks are the merge gate's business), and it stays read-only on git.

The skill must be harness-agnostic. Write it in plain gh CLI / REST with no harness-specific tooling, so any agent that can run shell commands executes it identically. It follows the existing sharing convention: the real file lives at .claude/skills/pr-review/ and .agents/skills/pr-review is a relative symlink to it, exactly like every other skill here.

It is monorepo-only. A generated app must not receive it, for the same reason the scaffold should not mandate a review loop.

Scaffold. Delete the self-review mandate rather than rewriting it, and replace it with a short note that the review process is the team's own call. WebJs stays opinionated about the code (the conventions, webjs check, the test layers) and silent about process.

Implementation notes (for the implementing agent)

Where to edit (line numbers are against main at the time of filing):

• .claude/skills/webjs-start-work/SKILL.md: the cycle is one contiguous block, ## Pre-merge review cycle at L259 through the start of ## After a merge at L455. Inside it, ### How the cycle works (L332) holds the fast/thorough shapes and ### Subagent prompt template (L431) holds the reviewer prompt. Do NOT delete the whole range blindly: ### Capture significant design discussion as PR comments sits inside it and is unrelated to reviewing, so it must survive. The GitHub reply/resolve mechanics also survive, but re-framed as the AUTHOR acting on the owner's review rather than as the reviewer half of a self-review.
• .claude/hooks/route-skills.sh: the code-review block, comment at L195 and the add_match directive at L210. Route review phrasings to pr-review with an inline-only directive instead.
• test/hooks/review-loop-exit.test.mjs: delete it. It guards the cycle's load-bearing wording across the skill and the hook, so it goes with the cycle. Replace it, do not just delete it (see landmines).
• test/hooks/route-skills.test.mjs: the routing test asserting code-review, plus the portability guard near the end whose regex enumerates project-skill names and asserts each has a committed .claude/skills/<name>/SKILL.md. pr-review must be added to that regex so the new skill is covered by it.
• .agents/rules/workflow.md: the skill list under ## Custom Skills Usage (around L51 to L62). Every symlinked skill needs an entry here.
• packages/cli/templates/.agents/rules/workflow.md L62 (5. Pre-merge self-review: ...) and packages/cli/templates/.github/pull_request_template.md L36 (the Ran N rounds checkbox).
• examples/blog/: three stale mirrors of the same mandate, in .agents/rules/workflow.md, .cursorrules, and .github/copilot-instructions.md.

Landmines:

• test/repo-health/agent-skill-parity.test.mjs enforces the skill list in BOTH directions. Adding the .agents/skills/pr-review symlink without adding a matching bullet to .agents/rules/workflow.md fails it. This is easy to miss because it lives outside test/hooks/, so a green hooks run proves nothing about it. Run the repo-health suite too.
• Deleting the guard test without replacing it leaves the new contract unguarded. The routing test only proves a skill NAMED pr-review is reached; it says nothing about what the directive tells the model to do, so a round count or a reviewer subagent could be reintroduced with the suite still green. Add assertions pinning the no-subagent, no-rounds, review-only and no-CI rules across BOTH carriers (the injected directive and the skill file), which is exactly the job review-loop-exit.test.mjs did for the old cycle. Prove it with a counterfactual: neutering the directive must red it.
• The .agents/skills/* entries are relative symlinks, ../../.claude/skills/<name>. Create the new one the same way; do not copy the directory, or the two drift.
• packages/cli/lib/create.js copies exactly one skill by hardcoded path (.agents/skills/webjs, around L714, falling back to the repo-root canonical when the prepack bundle is absent). So pr-review does not ship to generated apps today, but nothing enforces that, and a future change that copies the skills DIRECTORY instead would ship every monorepo workflow skill into every user's app silently. Add a scaffold test that pins it.
• The prose-punctuation hook scans new content (AGENTS.md invariant 11): no em-dash, no space-surrounded hyphen or semicolon as a pause, and WebJs capitalized wherever it names the project in prose. It also fires on plain-string assertion messages in test files, where wrapping a term in backticks is the fix. Content written through a shell heredoc bypasses the hook, so re-scan anything authored that way.
• A markdown list item followed by a paragraph with no blank line is a lazy continuation and renders INSIDE the bullet. Removing a numbered item from a list and leaving the closing paragraph flush against the previous one produces exactly this, silently.
• Reading a PR's files for review must address the HEAD repo at the HEAD sha, not the base repo at a branch name: a fork PR's branch does not exist in the base repo, so a branch-name ref 404s. .head.repo.full_name and .head.sha come from the same metadata call the review already needs for commit_id.

Invariants to respect:

• Invariant 11 (prose punctuation and brand casing), per the hook above.
• The one-task-one-worktree rule from AGENTS.md; a fresh worktree has no node_modules, so run npm run worktree:link before the suites.
• The gh budget rule in .claude/gh-budget.md: prefer gh api over REST; GraphQL is reserved for Projects V2 and resolveReviewThread. The new skill should follow it, which it does naturally since the reviews API is REST.

Tests and doc surfaces:

• test/hooks/route-skills.test.mjs (routing + the new contract guard + the portability regex), test/repo-health/agent-skill-parity.test.mjs, and a new test/scaffolds/ test pinning that a generated app of EITHER template ships exactly the webjs skill.
• Docs: the skill files themselves plus .agents/rules/workflow.md are the doc surface. No framework API, CLI flag, or app-authoring convention changes, so the docs site and marketing site are N/A.

Acceptance criteria

• The pre-merge review cycle is gone from webjs-start-work: no reviewer spawn spec, no delta rounds, no refuters, no escalation ladder, no subagent prompt template
• A pr-review skill exists, is harness-agnostic (plain gh/REST, no harness-specific tooling), and is reachable cross-agent through .agents/skills/pr-review
• The skill posts ONE review object via the reviews API carrying a summary plus line-anchored comments, documents multi-line anchoring and ```suggestion blocks, and states the four hard rules (inline, review-only, no CI, read-only git)
• Review phrasings route to pr-review with an inline-only directive, and the directive does not contradict the skill
• A guard test pins the no-subagent, no-rounds, review-only and no-CI rules across BOTH the directive and the skill file
• A counterfactual proves that guard actually fires (reintroducing a reviewer subagent reds it)
• A scaffold test proves a generated app of either template ships exactly the webjs skill, with a counterfactual proving the assertion reads the real generated tree
• webjs create output no longer mandates a review process, and the dangling CONVENTIONS.md reference in the PR template is gone
• test/repo-health/agent-skill-parity.test.mjs passes, meaning the new skill is listed in .agents/rules/workflow.md
• Hook, scaffold, and repo-health suites are green