chore: replace the review cycle with owner review and a pr-review skill - #1453
Conversation
vivek7405
changed the title
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff. The shape is right: the cycle is gone, the new skill says the four things that actually matter (inline, review only, no CI, read-only git), and putting it in .claude/skills/ with the .agents/ symlink matches how every other skill here is shared, so a non-Claude agent gets it for free. The GitHub-API mechanics are accurate, including the bit people usually get wrong, that a multi-line anchor needs start_line plus start_side and not just a range in the body.
Three things below. The markdown one on the start-work skill is a real rendering break, not a nit.
One finding I cannot anchor to a line, because the files are not in this diff: the review loop is only half removed from the repo. packages/cli/templates/.agents/rules/workflow.md:62 still tells every scaffolded app to "run fresh-context review rounds", packages/cli/templates/.github/pull_request_template.md:36 still has a "Ran N rounds; last round clean" checkbox, and examples/blog/.agents/rules/workflow.md:57 plus its .github/copilot-instructions.md:57 mirror say the same. Those are .agents rule files, which is exactly the surface this PR is about, so right now a fresh webjs create app ships the workflow we just deleted. I would rather decide that deliberately than discover it later: either fold the template and blog fix into this PR, or say out loud that the scaffold keeps the loop on purpose and note it in the PR body.
vivek7405
changed the title
vivek7405
changed the title
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff again at this head. Clean.
The earlier findings are all resolved properly, not just patched: the complete-PR list closes as its own paragraph again, the skill now reads PR files from the head repo at the head sha (with .head.repo.full_name added to the metadata call it needs), and the new contract guard pins the inline-only, review-only, no-CI rules across both the directive and the skill file, which is the job the deleted drift test used to do.
Two things I checked specifically and am satisfied by. The revert is surgical: the per-agent scaffold surface is fully restored while the self-review-loop removal survives it, and the net diff contains no leftover from the reverted work. And the removed "see the Pre-merge self-review loop section in CONVENTIONS.md" references were dangling in every copy, since neither the scaffold's nor the blog's CONVENTIONS.md ever carried that section, so deleting the pointers leaves nothing orphaned.
One thing to keep an eye on after merge, not a change request: the routing hook only takes effect once this lands on main, so until then a review ask still injects the old cycle directive and the session has to override it from the branch's own rules, which is exactly what this review did.
The owner reviews every PR themselves, inline or with an agent of their own choosing. Sessions no longer spawn reviewer subagents, run multi-round self-review loops, or post self-authored review objects. The webjs-start-work skill now ends at a reviewable PR (Definition of done, full suites, gh pr ready) and acting on the owner's review comments; the route-skills code-review directive injects an inline-only guard instead of the cycle, and the review-loop-exit guard test goes with the cycle it guarded.
When the owner asks for a PR review, the agent now reviews inline and posts the review through the GitHub review API the way a human reviews on the dashboard: one review object carrying a summary plus line-anchored comments, highlighting ranges via start_line/line and offering suggestion blocks where a concrete replacement is obvious. The reviewer only reviews: it never fixes findings, never resolves threads, never waits on or reports CI, and is never delegated to a subagent. The skill is written harness-agnostically (plain gh/REST, no harness-specific tools) at .claude/skills/pr-review and exposed cross-agent via the .agents/skills/ symlink, following the existing convention. route-skills now routes review phrases to it, and the portability test guards it like any other project skill.
Three findings from the review on this PR. The paragraph closing the complete-PR list lost its blank line when the review-comments item was removed above it, so it rendered as a lazy continuation inside the context-comments bullet. The skill read touched files at the base repo by branch name, which 404s on a fork PR because that branch does not exist there. It now addresses the head repo at the head sha, which covers both cases and pins the read to the commit the review is posted against. The routing test only proved a skill named pr-review was reached, so nothing guarded what the injected directive says. review-loop-exit had done that job for the old cycle and caught drift once. The new assertions pin the no-subagent, no-rounds, review-only, and no-CI rules across both the directive and the skill, and a counterfactual confirms they red when the rule is removed.
WebJs is opinionated about the code (the conventions, `webjs check`, the test layers) and has no business dictating how another team reviews a pull request. The scaffold did anyway: every generated app carried a mandatory multi-round self-review loop in its agent rules, plus a PR checklist item citing a Pre-merge self-review loop section of CONVENTIONS.md that the scaffold does not ship, so the reference was dangling on top of being unwanted. Both are gone, replaced by a note that the review process is the team's own call. The stale mirrors in examples/blog go with them. The monorepo keeps its own `pr-review` skill. It is not shipped, because create.js copies exactly one skill by hardcoded path, and the new scaffold test pins that: a generated app of either template ships the `webjs` skill and nothing else, and its agent rules mandate no review process. Nothing structural enforced this before, so a change that copied the skills directory wholesale would have shipped our workflow into every app silently.
A generated app no longer carries per-agent rule files or vendor tool config. `AGENTS.md`, the open standard Cursor, opencode, Antigravity and the Copilot coding agent read natively, is the entry point, and it routes to `.agents/` for everything else: the teaching skill and the workflow rules. Removed from the scaffold: `CLAUDE.md`, `CONVENTIONS.md`, `.claude.json`, `.claude/settings.json`, and the nine `.claude/hooks/*` scripts. #1368 had already dropped `GEMINI.md` and the Copilot bridge, so this finishes the job on one principle: how another team runs its tools is that team's call, and a framework has no business shipping its own tool config into their repo. Nothing that actually protects an app was lost, because the enforcement that mattered was already agent-agnostic or is now written down. `check-server-imports` duplicated `webjs check`'s no-server-import-in-browser-module rule, which CI runs, and its own header said so. `guard-branch-context` duplicated `.hooks/pre-commit`, which blocks commits on main at the git level and therefore binds every agent, editor, and human rather than one. The test, commit-cadence and worktree hooks restate rules `.agents/rules/workflow.md` already carries. `block-prose-punctuation` enforced OUR house style, including our brand casing, which a user's app has no reason to inherit. Two gaps were real and are covered in `.agents/` rather than dropped. `block-raw-htmlelement` had no `webjs check` equivalent, so the skill's components reference now teaches why a component extends WebComponent and never raw HTMLElement, naming all four failure modes and saying plainly that no rule catches it. And `.claude.json` was what wired the read-only `@webjsdev/mcp` server, so the scaffold's AGENTS.md now tells the reader to register it in whatever MCP config their agent uses. The monorepo keeps its own `.claude/` alongside `.agents/`, since that is our tooling and is never scaffolded. Root AGENTS.md now spells out the two audiences, which had been conflated.
…us .agents" This reverts commit ef5bab9.
Every skill symlinked into `.agents/skills/` must have an entry in `.agents/rules/workflow.md`, and `test/repo-health/agent-skill-parity.test.mjs` enforces both directions. The entry was missing from the commit that added the skill.
8427d0e to
7eab48e
Compare
1 participant
Closes #1454
Summary
Removes the automated pre-merge review cycle and replaces it with owner review plus a new
pr-reviewskill.The cycle is gone.
webjs-start-workno longer carries the fast/thorough shapes, delta rounds, refuters, reviewer spawn spec, liveness rules, or the subagent prompt template. Sessions do not spawn reviewer subagents, run multi-round self-review loops, or post self-authored review objects. The job ends at a reviewable PR, flipped to ready, handed to the owner.pr-reviewis the new skill. When the owner asks for a review, the asked agent runs it INLINE and posts through the GitHub review API the way a human reviews on the dashboard: one review object carrying a summary plus line-anchored comments,start_line/linerange highlighting, and ```suggestion blocks where a concrete replacement is obvious. The reviewer ONLY reviews. It never fixes findings, resolves threads, waits on CI, or delegates to a subagent. It is written harness-agnostically in plaingh/REST (no harness-specific tooling), committed at `.claude/skills/pr-review` and exposed cross-agent through the `.agents/skills/pr-review` symlink, matching how every other skill here is shared.The scaffold stops mandating a review process.
packages/cli/templates/.agents/rules/workflow.mddrops the mandatory multi-round self-review loop and the PR template drops its "Ran N rounds; last round clean" checkbox, which also cited aCONVENTIONS.mdsection the scaffold never shipped. Both are replaced by a note that the review process is the team's own call. The stale mirrors inexamples/bloggo with them. WebJs stays opinionated about the code and silent about how another team reviews a pull request.Test plan
node --test 'test/hooks/*.test.mjs'green (175). Includes a new guard pinning the no-subagent, no-rounds, review-only and no-CI rules across BOTH the routing directive and the skill file, doing the job the deletedreview-loop-exit.test.mjsdid for the old cycle. Counterfactual: putting "spawn a reviewer subagent" back into the directive reds it; restoring it greens it.node --test 'test/scaffolds/*.test.js'green (69), including the newscaffold-agent-skills.test.js: a generated app of either template ships exactly thewebjsskill (sopr-reviewand the other monorepo workflow skills cannot leak into a user's app), and its agent rules mandate no review process. Counterfactual: expectingpr-reviewto ship reds both template cases, so the assertion reads the real generated tree.node --test test/repo-health/agent-skill-parity.test.mjsgreen. This caught a real miss: every skill symlinked into.agents/skills/must have an entry in.agents/rules/workflow.md, and thepr-reviewentry was missing from the commit that added the skill.refin the new skill, and the missing guard test).Docs surfaces: N/A because no framework API, CLI flag, or app-authoring convention changed. The agent-workflow and scaffold-rule surfaces that DO describe review behaviour are updated above.