diff --git a/.agents/rules/workflow.md b/.agents/rules/workflow.md index 58c0b0a57..48f01e000 100644 --- a/.agents/rules/workflow.md +++ b/.agents/rules/workflow.md @@ -58,6 +58,7 @@ These project-level rules govern all operations inside this workspace. Antigravi - `webjs-scaffold-sync`: Trigger when changing the CLI generators, the scaffold templates, or the agent teaching skill. - `webjs-blog-write`: Trigger when writing, drafting, or editing a WebJs blog post under `blog/`. - `webjs-instagram-post`: Trigger when publishing an SEO post to the WebJs Instagram account. + - `pr-review`: Trigger when asked to review a PR. The review runs INLINE in the session that was asked, never in a subagent and never as a multi-round cycle, and posts through the GitHub review API as one review object (a summary plus line-anchored comments with suggestion blocks). It only reviews: it does not fix findings, resolve threads, or wait on CI. - `use-railway`: Trigger when interacting with Railway deploys. - The framework teaching skill at `.agents/skills/webjs/` is a real directory rather than a symlink, and is the reference for building WebJs apps rather than a workflow trigger. - Always use the `view_file` tool on the matched skill's `SKILL.md` before executing its tasks. diff --git a/.agents/skills/pr-review b/.agents/skills/pr-review new file mode 120000 index 000000000..c0f54c5c7 --- /dev/null +++ b/.agents/skills/pr-review @@ -0,0 +1 @@ +../../.claude/skills/pr-review \ No newline at end of file diff --git a/.claude/hooks/route-skills.sh b/.claude/hooks/route-skills.sh index e749fc9f2..5611d6414 100755 --- a/.claude/hooks/route-skills.sh +++ b/.claude/hooks/route-skills.sh @@ -192,22 +192,20 @@ if printf '%s' "$lc" | grep -q 'instagram' \ add_match "webjs-instagram-post: the request is to publish to the WebJs Instagram account. Invoke the webjs-instagram-post skill. Every post is SEO-only, so ALWAYS create a fresh branded image plus a keyword-rich caption, host the JPEG at a public HTTPS URL, and CONFIRM the image and caption with the user before the public publish. Never print or commit the access token." fi -# --- code-review: review the diff before a PR is ready ------------------ +# --- pr-review: review a PR inline, posted via the GitHub API ----------- # Triggers: review the PR/diff/branch/changes, code review, look it over -# for bugs. Reviewing every change before it is marked ready is a standing -# expectation, and a review is the CYCLE the webjs-start-work skill -# defines: one reviewer over the whole diff, a minor-or-must-fix call, and -# at most one delta round on the fixes, escalating to delta rounds plus a -# final whole-diff review only when the change shows it needs them. This -# directive and the skill must not resolve a case differently, which they -# once did for the final fix-check; test/hooks/review-loop-exit.test.mjs -# asserts the shared wording of the rules that decide when the cycle ends. -# code-review is a built-in Claude Code skill (no in-repo SKILL.md, so the -# portability test that guards project skills does not cover it). +# for bugs. The owner reviews every PR themselves before merge; when they +# ask THIS agent for a review, the pr-review skill runs it like a human +# reviewer working over the GitHub API instead of the dashboard: one +# inline read, one posted review object (summary plus line-anchored +# comments with suggestion blocks). Review only: no reviewer subagent, +# no multi-round cycle, no fixing findings, no waiting on CI. The skill +# is agent-agnostic (plain gh/REST) and committed at +# .claude/skills/pr-review, exposed cross-agent via .agents/skills/. if has '(review|audit) (the |my |this )?(pr|diff|branch|change|changes|code|commit)' \ || has 'code ?review' \ || has '(review|look) .{0,20}(over )?for (bug|issue|correctness|regression)'; then - add_match "code-review: the request is to review code. Invoke the code-review skill (it reviews the diff for correctness bugs plus reuse and simplification). A review of a PR runs the webjs-start-work review cycle, which is ONE fresh reviewer over the whole diff, never a fleet, and which defaults to its FAST shape because every round costs about 10 minutes and the owner should not have to ask for a short cycle. Fast shape: round 1 reads the whole diff; only a MUST-FIX fix buys a round and it buys exactly ONE, delta-scoped to those fix commits alone; a round whose fixes were all minor ends the cycle instead; then stop. A clean or minor-only round 1 therefore finishes with ONE review, which is the intended common case. Speed comes from running fewer rounds, never from lowering the bar inside a round. Escalate to the THOROUGH shape when the owner asks for a thorough or full review, or round 1 produced two or more must-fix findings or the one delta round produced any, or the diff touches the serializer, SSR or action dispatch, auth or session, the client router, or the elision analyser. Thorough shape: each later round is delta-scoped to the previous round's fix commits, and the first round that produces no fixes, whether it found nothing must-fix or everything it found was rejected or deferred, buys a FINAL review over the whole diff again. The code-review skill's findings feed that cycle as auxiliary input, not as a round of it. Every reviewer is spawned with the Agent tool as subagent_type general-purpose, model opus (Opus 5, never fable), run_in_background true, and isolation worktree. Judge each finding MINOR or MUST-FIX by SURFACE, never by importance: must-fix when it touches source, a test's ability to observe the defect it claims to cover, or a factual claim about runtime behavior in docs; minor for wording, naming, comment style, and nits about the review artifacts; when it could go either way it is must-fix. Only a FIX buys another round, since only a fix changed the branch, and a delta chain that keeps producing fixes stops after the fifth delta round, unfinished, rather than continuing; a rejection buys one refuter instead, and a deferral and a minor finding buy nothing. After the final review, a must-fix finding is fixed and gets ONE delta check of that fix alone; the cycle ends when nothing must-fix is left open, meaning the check came back with nothing or there was no fix to check because every must-fix finding the final review raised was rejected or deferred; a check that does find something must-fix gets that fixed and one more check of the same shape, and only if that one also finds something must-fix do you stop and report the PR unfinished. Do not file follow-up issues for review findings that are out of scope; report them to the owner, who decides, and fold a small same-file tweak into the PR. Never report the PR ready off a round that found something must-fix." + add_match "pr-review: the request is to review code. Invoke the pr-review skill and perform the review YOURSELF, inline in this session. NEVER spawn a reviewer subagent and NEVER run a multi-round review cycle. When the target is a pull request, post the review through the GitHub review API as ONE review object, a summary plus line-anchored comments that highlight the code to fix and carry suggestion blocks where a concrete replacement is obvious, exactly as the skill specifies. The reviewer ONLY reviews: it does not fix findings, does not resolve threads, and never waits on or reports CI. For a local diff with no PR, review inline and report the findings in the conversation instead. The owner decides what gets fixed, and fixing is separate work on a separate ask." fi # --- verify: prove the change works by running the app ------------------ diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md new file mode 100644 index 000000000..6b8c201cb --- /dev/null +++ b/.claude/skills/pr-review/SKILL.md @@ -0,0 +1,173 @@ +--- +name: pr-review +description: Review a GitHub pull request the way a human reviewer would, entirely inline, and post the review through the GitHub review API as ONE review object, a summary plus line-anchored comments that highlight the code to fix and carry suggestion blocks where a concrete replacement is obvious. Trigger whenever the user asks to review a PR ("review the PR", "review #123", "look over this pull request", "review the branch/changes" when the branch has an open PR). The reviewer ONLY reviews. It never fixes findings, never waits on or reports CI, never resolves threads, and never delegates to a subagent or another agent. +when_to_use: | + Examples that should trigger this skill: + "review the PR" + "review #1453" + "can you review this pull request" + "review my changes" (when the branch has an open PR) + "look over the diff for bugs" (when the diff is a PR) + Do NOT trigger for: fixing review findings (that is normal branch work, + a separate task from the review), responding to someone else's review + comments, merging, or reviewing a local uncommitted diff with no PR + (review that inline and report the findings in the conversation). +--- + +# Review a pull request (inline, posted via the GitHub API) + +Review a PR like a human reviewer who happens to use the GitHub API +instead of the dashboard. Same output a person would produce on +github.com: one submitted review carrying a summary and inline comments +anchored to the exact lines, with GitHub suggestion blocks where the fix +is concrete enough to spell out. + +This skill is agent-agnostic on purpose. Everything below is plain `gh` +CLI (or raw REST with any HTTP client and a token), no harness-specific +tools, so it works the same from any agent or harness that can run shell +commands. + +## The contract (read first, all four are hard rules) + +1. **Inline.** The agent that received the ask performs the review + itself, in the same session. Never spawn a reviewer subagent, a + fleet, or a background task for it, and never run a multi-round + review cycle. One read over the whole diff, one posted review, done. +2. **Review only.** The reviewer never fixes what it finds: no commits, + no pushes, no code edits, no resolving of threads. Findings are the + deliverable. Fixing them is separate work for whoever owns the + branch, on a separate ask. +3. **No CI.** Never wait on, read, or report CI or check status. Checks + are the merge gate's business, not the reviewer's, and a review that + stalls on a pending check has failed its one job of being fast + feedback on the code. +4. **Read-only on git.** Reviewing needs no checkout. Do not run any + git command that changes branch, HEAD, the index, or the working + tree (`checkout`, `switch`, `reset`, `restore`, `stash`, `merge`, + `rebase`, `clean`). Read-only inspection (`git log`, `git show`, + `git diff`, `git status`, `git blame`) is fine. + +## Steps + +### 1. Resolve the PR + +An explicit `#N` wins. Otherwise resolve the current branch's open PR +over REST (owner/repo from the git remote): + +```sh +BRANCH=$(git branch --show-current) +gh api "repos///pulls?head=:$BRANCH&state=open" \ + --jq '.[0].number' +``` + +If nothing resolves, ask the user which PR they mean rather than +guessing. + +### 2. Fetch what a reviewer reads + +All over REST (in repos that budget GraphQL, these cost nothing there): + +```sh +gh api repos///pulls/ \ + --jq '.title, .body, .head.sha, .head.repo.full_name, .user.login' +gh api repos///pulls/ -H "Accept: application/vnd.github.diff" +``` + +Then read every touched file in its PR-head state, not just the hunks, +so each edit is judged in context. Read locally if the branch happens to +be checked out, else through the contents API: + +```sh +gh api "repos//contents/?ref=" --jq .content | base64 -d +``` + +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 there 404s, and a sha pins every read to the exact +commit the review is posted against. Both values come from the first +call (`.head.repo.full_name`, `.head.sha`); on a same-repo PR the head +repo IS `/`, so one form covers both cases. + +Capture `head.sha` from the first call for `commit_id` too. + +### 3. Review the whole diff, yourself + +Judge the change against what it claims to do (title + body) and the +project's own stated rules (root `AGENTS.md`, `CONVENTIONS.md`, +per-package variants, when present). Correctness first: wrong behavior, +broken edge cases, a test that cannot observe the defect it claims to +cover, security problems, a doc stating something the code does not do. +Problems, not style nits, and no checklist narrowing: read the diff and +report whatever is actually wrong. A finding states the problem the way +a reviewer flags it, not the fix baked in as if already applied. + +### 4. Compose ONE review object + +The summary plus every inline comment go in a single review, which is +what makes GitHub render them as a grouped unit with the +"reviewed these changes" trail. Shape: + +```json +{ + "commit_id": "", + "event": "COMMENT", + "body": "", + "comments": [ + { "path": "src/x.js", "line": 42, "side": "RIGHT", + "body": "" }, + { "path": "src/y.js", "start_line": 10, "start_side": "RIGHT", + "line": 14, "side": "RIGHT", + "body": "" } + ] +} +``` + +- **Highlight a range** with `start_line` + `line` (both `side: RIGHT` + for added code, `LEFT` for deleted). A single-line comment omits + `start_line`. +- **Suggest the fix** where a concrete, self-contained replacement + exists, using a GitHub suggestion block in the comment body. The + block replaces EXACTLY the commented line range, so it must contain + the full replacement for those lines, correctly indented: + + ````markdown + This drops id 0 too. Strict-compare against null instead: + + ```suggestion + if (id !== null) { + ``` + ```` + + The user can then apply it with one click. Suggest only on lines the + diff adds or keeps (`side: RIGHT`), never on pure deletions, and skip + the suggestion when the real fix is bigger than the commented range; + state the problem and sketch the fix in prose instead. +- Every `line` must be part of the PR diff (a changed line or nearby + hunk context), or the API rejects the whole review. A finding on an + untouched line goes path-level into the summary body. +- Build the JSON with a real serializer (`jq -n`, python), never by + interpolating into a shell string, which mangles quotes and code + spans. + +**Verdict.** `event: "COMMENT"` on a PR you (the account posting) +authored, since GitHub forbids `APPROVE` / `REQUEST_CHANGES` on your +own PR, and that is the common case when the agent reviews the account +owner's work. On someone else's PR, pick the honest verdict: +`APPROVE`, `COMMENT`, or `REQUEST_CHANGES`. + +**Voice.** First person, plain, the way a person reviews code. Terse +inline findings, a summary that may go broad (what the change does +well, the one thing that matters). No AI or agent framing, no process +narration, no machinery tells (test counts, check status). + +### 5. Post it and report back + +```sh +gh api -X POST repos///pulls//reviews --input review.json +``` + +A clean review still posts: a short summary saying it is clean, with no +inline comments. Then tell the user the outcome in one or two +sentences, with the review's URL and the finding count. Stop there. No +fixing, no thread resolution, no follow-up issues, no re-review unless +they ask again. diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index bff754d51..065d403e8 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -108,7 +108,7 @@ The user's request typically names an issue by number (e.g. `#112`) or by descri If `ITEM_ID` is empty, step 2 did not find the card. Go back and add it rather than passing an empty `--id`, which fails. -6. **Open a DRAFT PR immediately, BEFORE writing any code.** This is the single most important ordering rule and it is NOT optional: the PR is opened at the START of the work, not the end. The whole point of the PR is to be the durable, append-only record of the change AS IT HAPPENS: every per-logical-unit commit lands on it, every design-rationale / decision / follow-up context comment is posted to it the moment that discussion happens, and every review round is posted to it. NONE of that is possible if the PR does not exist yet, which is exactly the failure a late `gh pr create` causes. So open it now, empty branch and all (the branch was already pushed in step 4). +6. **Open a DRAFT PR immediately, BEFORE writing any code.** This is the single most important ordering rule and it is NOT optional: the PR is opened at the START of the work, not the end. The whole point of the PR is to be the durable, append-only record of the change AS IT HAPPENS: every per-logical-unit commit lands on it, every design-rationale / decision / follow-up context comment is posted to it the moment that discussion happens, and the owner's review lands on it. NONE of that is possible if the PR does not exist yet, which is exactly the failure a late `gh pr create` causes. So open it now, empty branch and all (the branch was already pushed in step 4). Push one trivial initial commit if the branch has no commits yet (GitHub refuses a PR with no diff between head and base); the cleanest is to defer this step to immediately after the FIRST real commit, but never later than that. Open it as a DRAFT so it is clearly not yet ready to merge: @@ -123,13 +123,13 @@ The user's request typically names an issue by number (e.g. `#112`) or by descri The title MUST carry a conventional-commit prefix from the first moment (feat/fix/perf/breaking appear in the changelog; chore/docs/test/refactor do not), because a single-commit PR squashes on the COMMIT subject and a multi-commit PR on the TITLE. Refine the title/body as the change takes shape; the draft is a living document. Capture the issue URL/number for `Closes #` (already in the body). - From here on, the PR exists, so: commit per logical unit and push after each (the commits stream onto the PR); post design-rationale / decision / follow-up context comments to the PR as those discussions happen (do not hoard them for the end); and run every review round ON the PR. The PR is marked **ready for review** (`gh pr ready `) only at the very end, AFTER the Definition of done is satisfied and the review cycle has finished (the final whole-diff review, and the fix-check if it had one). Opening late and dumping everything at the end is the anti-pattern this step exists to kill. + From here on, the PR exists, so: commit per logical unit and push after each (the commits stream onto the PR); post design-rationale / decision / follow-up context comments to the PR as those discussions happen (do not hoard them for the end). The PR is marked **ready for review** (`gh pr ready `) only at the very end, AFTER the Definition of done is satisfied. From there the owner reviews it. Opening late and dumping everything at the end is the anti-pattern this step exists to kill. 7. **Report back briefly.** One short message to the user: issue title + number, new branch name, draft PR URL, "project card moved to In progress". Then continue with the actual work the user asked for. ## Definition of done (MUST be satisfied BEFORE marking the draft PR ready for review) -The PR is already open as a draft (step 6). "Done" here means the gate to flip it from draft to **ready for review** (`gh pr ready `), NOT the gate to create it. Everything below must be addressed, and the review cycle must have finished, before that flip. +The PR is already open as a draft (step 6). "Done" here means the gate to flip it from draft to **ready for review** (`gh pr ready `), NOT the gate to create it. Everything below must be addressed before that flip. **Bun parity is part of the task, not an afterthought.** webjs runs on Node 24+ AND Bun (#508). If the change touches a runtime-sensitive surface (the serializer, the node:http vs `Bun.serve` listener + request path, SSR / action / CSRF dispatch, streams, `node:crypto`, the TS stripper, auth / session / cors), then BEFORE you mark the PR ready you MUST (1) run the Bun matrix and report it green (`node scripts/run-bun-tests.js` plus the touched `test/bun/*.mjs` under `bun`), and (2) add or update a `test/bun/.mjs` cross-runtime assertion for the surface. This is enforced: `.claude/hooks/require-bun-parity-with-runtime-src.sh` BLOCKS a commit that stages runtime-sensitive source with no `test/bun/**` test (escape hatch `WEBJS_BUN_VERIFIED=1` only when an existing Bun script already covers it AND you ran it). Treat the parity, not just the Node result, as the bar. @@ -145,7 +145,7 @@ Doc drift is the #1 way a framework rots. Documentation MUST stay in sync with c - **Do NOT neuter a guard by `sed`-rewriting the source to a sentinel like `''`.** Shell-quoted escapes land as a literal control byte (a NUL/0x01) inside the file, which renders like a space in an editor but breaks the comparison and makes `grep` treat the file as binary (silent empty matches). Verify any byte-level edit with `od -c` on the changed line and `tr -d '\000' | wc -c` for stray NULs. Prefer the Edit tool (toggle the guard, run, toggle back) or `git stash`/`git stash pop` of the committed source over `sed` for this. The clean loop: commit fix+test, run test green, `git stash push -- ` (or Edit out the guard), run test red, `git stash pop` (or Edit the guard back), run test green again. The test having gone red in the middle is the proof. - **A counterfactual CLAIM decays.** "Reverting X reds Y" is true of a commit, not a branch: a later commit touching the same mechanism can make it false while every test stays green (a review-loop fix once made an older test non-discriminating exactly this way). So date the claim to the commit it was proven at, and when a later commit touches that mechanism, re-run the toggle and restate or correct the claim. This applies to mid-cycle fix commits too (see the speed rule in the review cycle). + **A counterfactual CLAIM decays.** "Reverting X reds Y" is true of a commit, not a branch: a later commit touching the same mechanism can make it false while every test stays green (a later fix commit once made an older test non-discriminating exactly this way). So date the claim to the commit it was proven at, and when a later commit touches that mechanism, re-run the toggle and restate or correct the claim. This applies to fix commits made in response to review feedback too. - **Integration** (server-level through `createRequestHandler`, SSR pipeline, scaffolds): behaviour across modules without a browser. - **Browser** (`*/test/**/browser/*.test.js`, run via `npm run test:browser` / `wtr`): anything touching hydration, client render, DOM, slots, the client router, custom-element upgrade. - **E2E** (`test/e2e/e2e.test.mjs`, run via `WEBJS_E2E=1`): full-stack behaviour observable only in a real browser against the running blog example, including **network probes** (was a module fetched or not), navigation, and streaming. @@ -242,69 +242,38 @@ If you find yourself writing "N/A" for every item except tests, that is a smell. If a PR ships without ANY of those touches and the change is user-visible, the PR is incomplete; do not mark it ready for review (leave it draft until the surfaces are addressed). -## Anatomy of a complete PR: four things, always +## Anatomy of a complete PR: three things, always -A finished PR is not just a diff. It carries four artifacts, and the PR is considered incomplete until all four exist. Treat this as the standing definition of a complete PR, applied automatically on every one: +A finished PR is not just a diff. It carries three artifacts, and the PR is considered incomplete until all three exist. Treat this as the standing definition of a complete PR, applied automatically on every one: 1. **A meaningful, conventional-commit-prefixed title.** The title MUST start with a conventional-commit type so the changelog is generated automatically: `feat:` for a new user-facing capability, `fix:` for a bug fix, `perf:` for a performance improvement, `breaking:` (or a `!` like `feat!:`) for a breaking change, and `chore:` / `docs:` / `test:` / `refactor:` for changes that should NOT appear in the changelog. After the prefix, be imperative, specific, what-and-why, under ~72 chars total. Example: `fix: shared rich values round-trip through the RPC serializer`, not `Fix serializer` or the issue number alone. **Why this matters (do not skip it):** PRs are squash-merged, so the PR TITLE becomes the squash commit subject on `main`, and `scripts/backfill-changelog.js` (run by the pre-commit hook on a version bump) extracts changelog entries by matching that subject against `^(feat|fix|breaking|perf)(scope)?!?:` and reads the commit BODY (the PR description) for the entry text. A non-prefixed title (e.g. `De-flake the prefetch e2e...`) produces ZERO changelog entries, which forces a hand-written changelog at release time, which is wrong. NEVER hand-write `changelog//.md`: fix the PR title/body instead so the automation produces it. If you ever find yourself about to hand-write a changelog, stop and correct the merged PR titles (or the release's source commits) so they are conventional-commit prefixed. 2. **A meaningful body.** `Closes #` near the top, a summary, what changed and why, the deliberately-excluded decisions, a test plan, and the docs surfaces touched (per the Definition of done above). This is the architectural narrative of the change. Because the squash commit body IS this PR description, write the first paragraph so it reads as the changelog entry text (the generator uses it), then continue with the rest. 3. **Context comments.** The reasoning from the working conversation that the diff and body do not capture, posted on the PR as the discussion happens (see "Capture significant design discussion as PR comments" below). The PR is the durable memory; the chat transcript is not. -4. **Review comments: a summary AND per-code-line comments.** Every review (each round of the review cycle and any manual review) posts a summary review plus an inline comment on each finding's `file:line` (see "Every PR review is posted ON the PR" below). -All four are written in the owner's voice (first person, plain, no AI/agent framing) and free of AGENTS.md invariant 11 banned glyphs. The no-machinery-tells rule binds the review and context comments; the PR BODY is the one place machinery evidence is REQUIRED content (the test plan and the dogfood results the Definition of done demands), so reporting it there is not a tell. The sections below specify the mechanics for items 3 and 4. +All three are written in the owner's voice (first person, plain, no AI/agent framing) and free of AGENTS.md invariant 11 banned glyphs. The no-machinery-tells rule binds the context comments; the PR BODY is the one place machinery evidence is REQUIRED content (the test plan and the dogfood results the Definition of done demands), so reporting it there is not a tell. The sections below specify the mechanics for item 3. -**Header every standalone comment with a short, meaningful bold heading** so a future reader (human or AI) knows what the comment is and what it is about before reading it. Put the heading on its own first line as bold markdown, blank line, then the body. Write the heading to fit THIS comment, do not pick from a fixed list. A good heading names the kind of comment and its topic, e.g. `**Design rationale: why analysis moved off boot, and what it costs**`, `**Review: lazy-boot model holds, one real bug**`, `**Decision: kept the derived gate over a declared allowlist**`, `**Follow-up: aliased-expose 404 filed as #N**`. A bare category word like `Context` or `Review` is the floor, not the goal; prefer a heading that also says the subject, so a reader scanning the PR's comment list can tell the boot-rationale note from the elision-review note without opening either. **Per-line inline review comments do NOT need a heading** because their `file:line` anchor already classifies them as review; keep those terse. The heading rule is for standalone, top-level comments (the PR body in item 2 is exempt, since it has its own `## Summary` structure). +**Header every standalone comment with a short, meaningful bold heading** so a future reader (human or AI) knows what the comment is and what it is about before reading it. Put the heading on its own first line as bold markdown, blank line, then the body. Write the heading to fit THIS comment, do not pick from a fixed list. A good heading names the kind of comment and its topic, e.g. `**Design rationale: why analysis moved off boot, and what it costs**`, `**Decision: kept the derived gate over a declared allowlist**`, `**Follow-up: aliased-expose 404 filed as #N**`. A bare category word like `Context` or `Review` is the floor, not the goal; prefer a heading that also says the subject, so a reader scanning the PR's comment list can tell the boot-rationale note from the elision note without opening either. **Threaded replies inside the owner's review comments do NOT need a heading** because the thread already classifies them. Keep those terse. The heading rule is for standalone, top-level comments (the PR body in item 2 is exempt, since it has its own `## Summary` structure). -## Pre-merge review cycle (MUST run before reporting "ready for merge") +## Review: the owner reviews every PR -Saying "ready for merge" before the review cycle completes is the single biggest source of low-quality PRs. The recurring pattern to AVOID: claim ready-for-merge, the user requests a review, find issues, fix them, claim ready-for-merge again, repeat 4-5 times before a review comes back clean. The cure is to run that cycle internally BEFORE the first "ready" signal. The user should only hear "ready to merge" after the cycle has finished AND the suites it deferred have run. +There is NO automated pre-merge review cycle in this workflow. Do not spawn reviewer subagents, do not run multi-round self-review loops, and do not post self-authored review objects to the PR. The review belongs to the owner (vivek7405), who reviews every PR themselves, inline or with an agent of their own choosing, once it is flipped to ready. Your job ends at handing them a reviewable PR and then acting on what their review finds. -### Every PR review is posted ON the PR (summary + per-line comments) +Before flipping to ready, run everything the Definition of done demands: the full suites for every layer the change touches (full Node, browser, e2e, the Bun matrix, the two-app dogfood boot check). Launch them as parallel background tasks in one batch and collect EVERY result before reporting, because a task you forget to collect is a silently skipped layer. Then `gh pr ready ` and report back that the PR awaits the owner's review. Never report the PR ready with failing or unrun suites. -This applies to EVERY review of a PR: each round of the review cycle below, AND any time the user asks you to "review the PR" manually. A review that lives only in your chat reply is not a review the PR carries. For every review you perform, post BOTH: +**If the owner explicitly asks you to review a PR, do it yourself, inline in this session, per the `pr-review` skill** (`.claude/skills/pr-review`, exposed cross-agent at `.agents/skills/pr-review`): one read over the whole diff, posted to the PR through the GitHub review API as one review object, a summary plus line-anchored comments with suggestion blocks. The reviewer ONLY reviews. It never fixes the findings, never resolves threads, never waits on or reports CI, and is never delegated to a subagent or expanded into rounds. The owner decides what gets fixed, and fixing is separate branch work on a separate ask. -1. **A summary review comment** stating what you reviewed and the overall outcome (which surface, what you found, or that it is clean). This is what you leave at the "Finish your review" step. -2. **A per-line inline comment for each finding**, anchored at `file:line` on the diff. Each states the PROBLEM only, the way a reviewer flags it before anyone has fixed it. Do NOT bake the resolution into the finding (ending a finding with "...Fixed." is wrong). The resolution is recorded separately, as a threaded reply, in the programmer half below. Post the won't-fix and false-positive findings as inline comments too, so the concern sits on the exact line; their reply carries the reason they are left as is. +### Acting on the owner's review comments -**Both go in ONE review object, via the reviews API, never as plain issue comments.** The summary and all its inline comments are submitted together with a single `POST /pulls//reviews` (the `--input review.json` call below). That is what makes GitHub render them as a grouped unit: the summary plus a `reviewed these changes - N comments` trail of the per-line comments beneath it. A review observation posted with `gh pr comment` (an issue comment) instead lands as a standalone box with NO trail, visually identical to a general comment, and disconnected from its inline notes. So: review content (summaries AND observations, every round) goes through the reviews API; `gh pr comment` issue comments are reserved for NON-review context (the design-rationale, decision, and follow-up notes from the section further down). Do not scatter review remarks across loose issue comments. If you catch yourself about to `gh pr comment` something that is really a review observation, fold it into the review summary instead. (Note: GitHub's mobile app tints every comment you author a light blue because of the `Author`/`Member` badge; that tint is author-association, NOT a review marker, so it is not a reliable signal. The reliable signal that something is a review is the `reviewed these changes` trail, which only a review object has.) +When the owner leaves review comments on the PR, work through each one following GitHub's real flow (fix, reply in the thread, resolve): -**Voice: write every PR comment as the repo owner (vivek7405) would write it.** First person, plain, the way a person reviews code. The whole review trail (summary AND inline comments) must read as if the owner typed it, not as a bot reporting a procedure. This is non-negotiable and applies to every PR review, forever, not just the one in front of you. - -Hard rules: - -- **No AI/agent framing.** Never refer to yourself as an AI or agent, never say "self-review", never number the rounds ("Round 2", "round 3 of the loop"), never say "you requested a manual re-review" or otherwise narrate the review process. -- **No machinery tells.** A human reviewer does NOT mention CI status ("CI is green", "all 5 gates pass"), test counts ("96 tests pass"), or meta-scaffolding ("Went over the X, Y, Z paths. Comments inline."). CI state lives in the checks UI, not in prose; the inline comments are obviously inline. Drop all of it. -- **Inline findings are terse and state the problem, not the fix.** Point at what is wrong on that line, the way a reviewer flags it. "`expose as exp` won't match this, so the route 404s." / "Says it scans on boot, but this is lazy now." / "A same-mtime, same-size recreate could still serve a stale parse, does that need handling?" The fix and won't-fix reasons go in the threaded reply, never in the finding itself. -- **Reference commits as clickable links, not bare SHAs.** GitHub does NOT auto-link a SHA inside a backtick code span, so `` `5fd02dc` `` renders as dead text. Always write a markdown link: `[`5fd02dc`](https://github.com/webjsdev/webjs/commit/5fd02dc)` (the short SHA resolves fine in the URL). Same for any commit referenced in a summary, reply, or context comment, e.g. "Fixed in [``](https://github.com/webjsdev/webjs/commit/).". A reviewer wants to click straight to the diff. -- **The summary may go broad.** Because the per-line comments carry the specifics, the summary is the place for an opinionated, architecture-level take: what the change does well, what you would keep an eye on, the one thing that actually matters. Still first person and plain, just not restricted to pointing at one line. Think of how you would brief a teammate on the PR in three or four sentences. - -The test for any comment: if it reads like a person who owns this repo wrote it offhand, it passes. If it reads like a status report or a tool's output, rewrite it. - -### Follow the real review flow: reviewer, then programmer, both roles - -GitHub's manual flow is: **Start a review**, add inline comments, **Finish your review**, leave a summary, **Submit review**. Then the author **fixes** each comment, **replies in the thread** that it is fixed, and **resolves** the thread. The reviewer and the programmer are the same person here, but that does NOT collapse the two roles into one comment. Reproduce the whole flow over the API every time, both halves. - -**Reviewer half (one review object).** Submit the summary plus all inline findings together with a single `POST /pulls//reviews`. That one call is Start-review + add-comments + Finish + Submit. Findings state the problem, not the fix. - -```sh -gh api -X POST repos/webjsdev/webjs/pulls//reviews --input review.json -# review.json: { "commit_id": "", "event": "COMMENT", -# "body": "", -# "comments": [ { "path": "", "line": , "side": "RIGHT", "body": "" } ] } -``` - -Use `event: "COMMENT"` (GitHub forbids APPROVE / REQUEST_CHANGES on your own PR). Each inline `line` must be a line that is in the PR diff (a changed or added line), or the API rejects the whole review; if a finding sits on an unchanged line outside the diff, note it path-level in the summary. Verify with `gh api repos/webjsdev/webjs/pulls//comments`. - -**Programmer half (after the review is submitted).** For each finding: - -1. **Fix it** on the branch (commit + push), or decide it is a won't-fix. -2. **Reply in the comment's thread** with the resolution. This is the "reply that it is fixed" step, not an edit of the finding: +1. **Fix it** on the branch (commit + push), or make the case in the thread for leaving it as is and let the owner decide. +2. **Reply in the comment's thread** with the resolution: ```sh gh api -X POST repos/webjsdev/webjs/pulls//comments//replies \ --input reply.json # reply.json: { "body": "Fixed in [``](https://github.com/webjsdev/webjs/commit/)." } ``` -3. **Resolve the thread** once it is concluded (fixed, or won't-fix-with-reason). Threads resolve ONLY via GraphQL `resolveReviewThread`; REST cannot do it: +3. **Resolve the thread** once its finding is fixed. Leave a debated or won't-fix thread OPEN for the owner to resolve, since the concern is theirs. Threads resolve ONLY via GraphQL `resolveReviewThread` (REST cannot do it), one of the two sanctioned GraphQL uses in `.claude/gh-budget.md`: ```sh # list unresolved review-thread node IDs gh api graphql -f query='query{repository(owner:"webjsdev",name:"webjs"){pullRequest(number:){reviewThreads(first:50){nodes{id isResolved}}}}}' \ @@ -313,9 +282,7 @@ Use `event: "COMMENT"` (GitHub forbids APPROVE / REQUEST_CHANGES on your own PR) gh api graphql -f query='mutation($t:ID!){resolveReviewThread(input:{threadId:$t}){thread{isResolved}}}' -f t= ``` -**Every round repeats the whole flow.** Each round of the review cycle, and each manual re-review the user asks for, is a NEW review object: a fresh `POST /pulls//reviews` carrying that round's summary and findings, followed by fix + reply + resolve for that round's threads. Never append a later round's findings into an earlier round's review, and never edit a prior finding to say it is fixed (reply instead). A round that REVIEWED and found nothing still posts a short summary review saying it is clean, with no inline comments. A round whose reviewer did not review (see the liveness rules) posts nothing, because there is no round to summarize and a clean review object on the PR would be a lie. - -Banned prose glyphs (AGENTS.md invariant 11) apply to every comment, reply, and summary body, so keep them clean. +**Voice: write every PR comment and reply as the repo owner (vivek7405) would write it.** First person, plain, the way a person talks about their own code. No AI/agent framing, no machinery tells (CI status, test counts, meta-scaffolding). Reference commits as clickable markdown links, never bare SHAs in code spans, because GitHub does not auto-link a SHA inside a backtick span. Write [`5fd02dc`](https://github.com/webjsdev/webjs/commit/5fd02dc). Banned prose glyphs (AGENTS.md invariant 11) apply to every comment and reply. ### Capture significant design discussion as PR comments (standing, automatic) @@ -323,135 +290,20 @@ Beyond review findings, proactively record the *reasoning* behind a PR as commen **Trigger (automatic, not on request):** whenever a conversation about an open PR produces a non-obvious design decision, a rejected alternative, a tradeoff accepted with eyes open, or context the diff alone does not explain, post it as a PR comment AS THE DISCUSSION HAPPENS. Use `gh pr comment --body-file /tmp/pr-comment.md` for cross-cutting narrative, or an inline `file:line` comment when it pertains to specific code. Same voice as review comments: first person, plain, owner's voice, no AI/agent framing, no machinery tells, no banned glyphs. -**This runs continuously across the PR's whole life, not once.** Because the PR opens as a draft at the START (step 6), there is a place to post from the first commit onward, so keep adding context throughout: when a mid-work investigation changes the approach, when a reviewer finding is resolved a particular way, when an edge case is discovered, when something is deliberately deferred. The acceptance test is concrete: a future AI agent (or the owner) who opens ONLY this PR, with zero access to this chat, should find every non-obvious "why" already written on it. If reconstructing the reasoning would require the chat transcript, a context comment is missing. Do not save it all for a single end-of-work dump; that recreates the exact gap the early-draft-PR rule exists to close. +**This runs continuously across the PR's whole life, not once.** Because the PR opens as a draft at the START (step 6), there is a place to post from the first commit onward, so keep adding context throughout: when a mid-work investigation changes the approach, when the owner's review finding is resolved a particular way, when an edge case is discovered, when something is deliberately deferred. The acceptance test is concrete: a future AI agent (or the owner) who opens ONLY this PR, with zero access to this chat, should find every non-obvious "why" already written on it. If reconstructing the reasoning would require the chat transcript, a context comment is missing. Do not save it all for a single end-of-work dump; that recreates the exact gap the early-draft-PR rule exists to close. **What's worth capturing (judgement, not a checklist):** why an approach won over a credible alternative; an experiment tried and reverted, with the reason; a tradeoff accepted knowingly (a cold-start cost, a known-small race window left in, a documented edge case); a constraint or invariant discovered mid-work; anything you would want explained if you returned to the PR with no memory of the conversation. Skip the trivial: routine fixes, mechanical edits, anything the diff already makes obvious. The bar is "would a future agent be missing important context without this", not "log everything". When the PR body already covers a decision, a short comment is fine or skip it; do not duplicate the whole body into a comment. -**A PR carries exactly two kinds of content: the code change and the review rounds.** Everything on it (body, commits, review summaries, inline findings, context comments) must be meaningful data about one or the other. Session and harness machinery is NOT PR content and must never be posted there. Concretely, keep OFF the PR: a subagent that could not be spawned or died, a tool that errored or was declined, a retry, an interruption, how many turns something took, and above all your own process mistakes in running the PR (a stale body you then fixed, a mirror you forgot to sync, a mis-posted comment). Those are conversation, not record. The test: would this still matter to someone reading the PR in a year who has no idea which agent or session produced it? A design decision passes. A rejected alternative passes. A review finding passes. "My reviewer spawn was declined and I retried" does not, and neither does "I got this wrong earlier in the PR and then corrected it", which reads as noise around a diff that already shows the correction. Fix the mistake and move on; do not narrate it onto the PR. - -### How the cycle works - -The draft PR is already open (step 6), so reviews post to it from the first round. Do NOT mark it ready for review or report "ready for merge" yet. - -**FAST is the default shape.** A reviewer spawn costs roughly 10 minutes of wall clock, so every round is a real, measurable tax on the PR, and the owner should never have to ask for a short cycle. The default therefore buys the SECOND read only when there is evidence the change needs it, and the thorough shape below is what that evidence escalates to. Speed is bought by running fewer rounds, NEVER by lowering the bar inside a round: the minor / must-fix call is unchanged, still by surface, still fail-open to must-fix. - -**The fast cycle (default):** - -1. **Round 1: ONE fresh reviewer over the WHOLE diff.** The same shape for every PR, whatever paths it touches. No fleet, no lenses, no jury, no per-diff tier choice. -2. **Judge each finding MINOR or MUST-FIX** (rule below), then fix, reject, or defer it (the three dispositions below). Apply every minor finding; they buy nothing. A rejection buys one REFUTER, a deferral buys nothing. -3. **Only a MUST-FIX fix buys a round, and it buys exactly ONE:** a delta-scoped reviewer whose QUESTION is those fix commits alone. A round whose fixes were ALL minor ends the cycle instead, because wording and naming edits cannot introduce the class of defect a round exists to catch. -4. **Then STOP.** The cycle ends when nothing must-fix is left OPEN. If that one delta round itself finds something must-fix, fix it, and escalate to the thorough cycle rather than chaining further: a fix that produces another must-fix finding is the evidence the escalation ladder is asking for. - -So a clean or minor-only round 1 finishes the cycle with ONE review. That is the intended common case, not a shortcut. - -**Escalate to the THOROUGH cycle when ANY of these hold**, checked at the end of round 1 and again after a delta round: - -- the owner asks for a thorough, full, or deep review; -- round 1 produced TWO OR MORE must-fix findings, or a delta round produced any, which says the change is not yet understood; -- the diff touches a surface where a miss is expensive: the serializer, SSR or action dispatch, auth or session, the client router, or the elision analyser. - -**The thorough cycle (on escalation):** steps 1 and 2 as above, except that the one-delta-round limit lifts, so each later round is delta-scoped to the previous round's fix commits and the chain continues while its rounds keep producing fixes. Then: - -4. **The first round that produces no fixes buys the FINAL review: ONE fresh reviewer over the WHOLE diff again**, on the head the fixes produced. Two different jobs are being done here. After delta rounds, it is the first read of the finished change rather than of a fragment. After a clean round, when it is the same diff at the same head, it is a second independent read, which is the check a single-reviewer cycle owes in place of the fleet's second opinion. Either way a clean round is not the end of the THOROUGH cycle. This is. **A delta chain that keeps producing fixes stops after the FIFTH DELTA ROUND**, unfinished, reported per its own entry in Failure handling rather than continued: a change whose repairs keep breaking something is signal about the change, not about the review, and the measured 18-round loop that motivated this spent its last four rounds catching regressions its own fixes had introduced. -5. **Judge the final review the same way, then STOP.** Its fixes get ONE delta-scoped check of those fix commits alone; its rejections get a refuter; its deferrals and minor findings are recorded and applied. The cycle ends when nothing must-fix is left OPEN, which means the check came back with nothing, or there was no fix to check because every must-fix finding it raised was rejected or deferred. A check that does find something must-fix gets that fixed and ONE more check of the same shape, and if that one does too, stop and report the PR unfinished. The final review is never re-run and the delta rounds are never re-entered. - -**Do not file follow-up issues for what a review turns up.** A finding that is real but out of scope goes in the reply to the owner, who decides whether it becomes tracked work. A small tweak in a file the PR already touches is folded into the PR instead. This is the standing no-proactive-follow-up rule, and it applies to review findings like anything else. - -**Minor or must-fix.** MUST-FIX when the finding touches what ships: source, a test's ability to OBSERVE the defect it claims to cover (a tautological assertion that stays green with the bug present counts), or a factual claim about runtime behavior in docs (a stated default, limit, or condition list). MINOR is everything else: wording, naming, comment style, PR-body counts, nits about the review artifacts. Judge by SURFACE, never by importance, because importance is judged by the same agent that wants to stop. When it could go either way, it is must-fix. - -**Every reviewer is the same spawn:** the Agent tool, `subagent_type: "general-purpose"`, `model: "opus"` (Opus 5, always, no other model anywhere in the cycle), `run_in_background: true`, `isolation: "worktree"`, carrying the prompt template at the end of this section. Reviewers are fresh and ONE-SHOT: a reviewer that carries context across rounds re-derives its own conclusions, which is the blind spot a fresh one exists to avoid. - -**Give the reviewer the diff and nothing else.** The PR diff, the PR title and body, the touched files, and the rule files it judges against (`AGENTS.md`, `CONVENTIONS.md`). NEVER prior PR comments or reviews, and never a growing list of already-handled findings: on #1159 the comment payload alone reached 171 KB by the fifth round. An unbiased reviewer is the point, and a duplicate finding costs you a second of reading. - -**Do not tell it what to look for.** The prompt sets the SCOPE (which diff) and nothing else: no list of defect classes, no "specifically check for X and Y", no ranking of what matters, however sure you are about where the risk sits. A checklist narrows a fresh reviewer to what you already suspect, which is the bias it exists to escape, and everything outside your list becomes what it does not look at. The one exception is a REFUTER, whose whole job is the single claim it is handed. Naming the touched files is scope; naming the bugs to hunt is steering. - -**Keep the cycle fast.** After a fix, run only the test file(s) covering the line you changed, with the counterfactual toggle the Definition of done mandates (a fix can make an older test non-discriminating without failing it). The e2e, full Node, browser, and Bun suites and the two-app dogfood check run ONCE, after the cycle ends. CI is not read during the cycle at all, and not at the end of it either. It is read once, at merge, under the merge gate below. Both rules change WHEN work happens, never WHETHER. - -**Do not restore what this replaced.** This cycle used to run a 16-agent fleet with a scout, parallel lenses, and a jury, pick round 1's shape by a path check, sort findings into two tiers, cap itself at five rounds, and poll a file to watch each spawn. Almost all of it is gone on purpose: termination is mostly structural now (only a fix buys a round, delta rounds narrow the question, the minor call stops wording from buying rounds, the final review plus one fix-check is a hard end). Reviews are async, so the harness completion notification is the signal, with at most an optional background progress check that never kills anything. The one exception to the removals is the round cap, which came back in a narrower form, because structure alone cannot bound a chain where every fix produces the next round's finding: it now bounds ONLY that case, at five delta rounds, instead of counting every round of the cycle. - -The two-review minimum went the same way, and this is the part most likely to be "restored" by mistake. It was introduced on the reasoning that a lone reviewer's miss is the cost of dropping the fleet and the final pass is what covers it. That reasoning was right about the risk and wrong about the price: it charged EVERY PR a second 10 minute round to cover a miss that mostly matters on a few surfaces, and in practice the second read on a small single-surface diff came back clean or minor. So the final review is now bought by evidence rather than owed by default, through the escalation ladder above, which keeps the protection where it pays. Do NOT reinstate an unconditional final review or an unconditional two-review floor; if the ladder is letting real defects through, tighten a TRIGGER, which is the knob that was built for it. +**A PR carries exactly two kinds of content: the code change and its review.** Everything on it (body, commits, context comments, review replies) must be meaningful data about one or the other. Session and harness machinery is NOT PR content and must never be posted there. Concretely, keep OFF the PR: a subagent that could not be spawned or died, a tool that errored or was declined, a retry, an interruption, how many turns something took, and above all your own process mistakes in running the PR (a stale body you then fixed, a mirror you forgot to sync, a mis-posted comment). Those are conversation, not record. The test: would this still matter to someone reading the PR in a year who has no idea which agent or session produced it? A design decision passes. A rejected alternative passes. A review finding passes. "A subagent spawn was declined and I retried" does not, and neither does "I got this wrong earlier in the PR and then corrected it", which reads as noise around a diff that already shows the correction. Fix the mistake and move on; do not narrate it onto the PR. -Each round must: +### Merge is gated on green CI, enforced at the branch level, not by trust -1. **Spawn the round's reviewer** per the spawn spec, and act on the harness completion notification when it arrives. Do not badger it for results and do not re-read its message hoping for a different answer. +A PR must not merge until all CI checks pass. `main` branch protection requires the five `ci.yml` checks (Conventions, Unit+integration, Browser, E2E, Build) before any merge. If `gh api repos/webjsdev/webjs/branches/main/protection` shows `required_status_checks: null`, run `bash scripts/protect-main.sh` once (needs repo admin) to restore it. Do not work around a red or pending check. Wait for green, and fix whatever is red before merging. - **Liveness.** Only two things are evidence a reviewer is alive: the harness status, and byte growth on its transcript when the output file is one. Its own prose never is (one signed off with "I'm partway through the careful pass" while its status read `killed`). - - **A dead spawn is not a round.** Declined, errored, killed, or empty means nothing ran. It does not count toward the cycle and does not advance it. - - **Waiting is not blocking.** The spawn is async precisely so the turn stays free, so never wait on it with a foreground `sleep`: that stalls everything and hands back exactly what the async spawn bought. Keep working, or, if there is genuinely nothing else to do, do NOTHING and let the completion notification arrive. A blocking probe is worse than no probe. - - **A progress check is optional, runs in the BACKGROUND, and never kills.** Rising bytes on the spawn's output path (`stat -c %s`, following the symlink) prove it is working, so leave it alone however long it has run. Nothing proves the opposite: a file that never grows may simply be a stub the harness does not write to, and elapsed time never proves a stall, since reviewers here routinely run 5 to 10 minutes while working normally. So there is no AUTOMATIC re-spawn trigger short of a killed or errored harness status; never wire one to a flat file or a timer. Giving up applies ONLY where there is no growth to see (a flat file, a stub-backed spawn), and "none to see" means you PROBED and saw none, never that you did not look, so the probe is optional only while you are content to wait and required before you give up. There it is a DELIBERATE call, weighing how long it has been against what a restart costs, and that judgement is the one place elapsed time legitimately counts. A transcript that is still growing is never abandoned, whatever the clock says. Giving up means STOPPING it (`TaskStop`) and re-spawning, so there is never a second reviewer in flight, no question of which return is the round, and no late result to reconcile. Never read the transcript's contents: it is large enough to swamp your own context, and a subagent's words are not evidence either way. - - **A reviewer that returns without reviewing is also not a round.** Anything that is not a finding list or the literal `CLEAN` is a non-review, including "I could not fetch the diff", a refusal, or an answer to another question. The absence of findings is not a clean round. A REFUTER is the one exception, since its job is not to review: it answers `REFUTED` or `STANDS`, and either is a complete result. - - **Re-spawn rather than asking.** Spawn it again, varying the approach after a few identical failures. Never stop mid-cycle to report a failed spawn or hand back a half-finished cycle. - - **NEVER substitute an inline self-review.** Reviewing your own work re-derives the assumptions that produced the bug; that downgrade already shipped three real bugs through a PR two inline passes had called clean. Only a reviewer that cannot be produced at all stops the cycle, reported once at the end and kept out of the PR (a spawn that could not run is session tooling, not a fact about the change). - - **Working-tree safety.** Every worktree shares ONE `.git`, so a reviewer's git write reaches this session's checkout (one ran `git checkout main` mid-cycle and the local checkout regressed). - - `isolation: "worktree"` on every spawn, so a stray checkout cannot move the files under this session. - - The read-only git prohibition in the prompt, which covers the shared refs and config that isolation cannot. - - After EACH spawn resolves, before acting on findings, check the repo: `git rev-parse --is-inside-work-tree` is `true` and `git config --get core.bare` is NOT `true` (spawning isolated reviewers has flipped it; repair with `git config core.bare false`, then `git worktree prune`, plus `git worktree remove -f -f .claude/worktrees/agent-*` for a locked leftover, never touching worktrees outside `.claude/worktrees/`). In the task's worktree, HEAD is still the feature branch and `git status` is clean; in the primary, HEAD is `main` (`git checkout -f main` if it moved). Run it after failed spawns too, since a spawn that died after creating its worktree is the likeliest leaker. GitHub is unaffected either way; this only repairs the local repo. - -2. **For each finding, do exactly ONE of three things.** There is no fourth, and "mention it and move on" is not one. A REFUTER, a fresh spawn told to DISPROVE a claim (does it reproduce in the code as written, is the behavior intended, is it already guarded or tested somewhere the finder did not look), has two uses here: BEFORE acting on a must-fix finding whose fix would be expensive or behavior-changing, and AFTER rejecting any must-fix finding, since a rejection is your own unadjudicated judgment and one cheap spawn is what adjudicates it. That second use is what a rejection buys instead of a whole round, and it terminates: one spawn per rejection, never a refuter of a refuter. A refuted finding is a rejection carrying the refuter's reason. On the post-rejection use only, a refuter that answers `STANDS` has contradicted your rejection, so the rejection does not hold: fix the finding, or defer it if it is genuinely out of scope. That is one of the three dispositions arriving late, not a fourth. In a delta round the fix it usually produces buys its round like any other; on the final review it joins that phase's single fix-check instead, since the final review is never re-run. (On the pre-action gate there is no rejection yet, so `STANDS` there simply means the finding is real and you act on it.) The two uses fail differently when no refuter can be produced: on the pre-action gate, act on the finding as real, since the gate is an optimization and must fail toward the finding being genuine; on the post-rejection adjudication, keep the rejection but record it as UNREFUTED on its thread and carry it into the end-of-cycle report, so the user can second-guess it. Trivial or obviously-real findings skip the first use. - - **Fix it** on the branch (commit + push to update the PR), OR - - **Reject it** with a one-sentence reason, stated to the user and recorded on the finding's thread. Rejection has to be defensible ("flagged as a security issue, but this runs server-side only and never sees user input"), not hand-waved. OR - - **Defer it** when it is genuine but out of scope (a pre-existing bug, an unrelated hygiene problem, a separate feature). A finding on code this PR adds or changes is NEVER out of scope, whatever its size. Deferral is not a way to drop a finding: record it on the thread, and carry it into the end-of-cycle report, where the USER decides what gets filed (the cycle never files a follow-up issue on its own; invoke `webjs-file-issue` only on their go-ahead). **The final summary review also carries a deferral ledger**, one line per deferral with its reason, so a cold reader sees them all without walking the threads. When unsure, fix it here. - - - **Record every finding ON THE PR**, through the mechanics in `### Every PR review is posted ON the PR` and `### Follow the real review flow`, which are authoritative: one review object per round carrying the summary plus every inline `file:line` finding, each stating the problem only, with the disposition (`fixed in ` / `rejected because ` / `deferred as out of scope because `) in a threaded reply, then the thread resolved. Post rejections and false positives too, so the reasoning is auditable. A round that found nothing posts a short summary saying so. Build the review JSON with a real serializer, never by interpolating into a shell string: a review on #1115 lost every code reference to shell command substitution and had to be reposted. - -**When the cycle FINISHES, run everything it deferred:** the full suites for every layer the change touches (e2e, Node, browser, Bun matrix, the two-app dogfood boot check, per the Definition of done). Launch them as parallel background tasks in one batch and collect EVERY result before reporting: a task you forget to collect is a silently skipped layer. A cycle that STOPPED unfinished runs none of this and says so in the report, because these gate the flip to ready for review and that flip is not happening. - -**A fix is never the end.** A fix changes the branch, so the changed branch needs its own round; that is what the delta rounds are, and why a round with no fixes still buys the final review. Never report "fixed it" or "ready to merge" off a round that found something must-fix, however obviously correct the fix looks. On #1159 three consecutive rounds each found problems introduced by the previous round's fix, which is what a re-used reviewer, already invested in that fix, is worst at seeing. - -**A standalone "review the PR" request IS this cycle, not a one-shot.** Re-enter at round 1 over the whole diff however many times the PR has been reviewed before, since the ask itself says the existing trail is not trusted. It also overrides the trivial-change skip below: when the user asks for a review, they get one. Then fix, reject, or defer, run the delta rounds, run the final review, and only then report back. - -### When to skip the cycle - -Skip only for PRs that change a single line of trivially-correct content (a doc typo, a renamed local variable, a one-token config bump). Anything that touches logic, public surface, the build, the importmap, security-relevant code, or multiple files goes through the cycle without exception. A bias toward running it is correct; a bias toward skipping it is the exact failure mode this rule exists to prevent. - -### Reporting after the cycle - -After the final review (and its fix-check, if it had one) and the deferred suites, report exactly this shape: - -> PR # is up at . Reviewed it over rounds plus a final pass over the whole diff; nothing must-fix is left open. Issues found and fixed: . Out-of-scope findings, recorded on the PR and awaiting your call on filing: . Ready to merge. - -When anything was deferred, expand each one right there (the finding, its one-sentence reason, its thread), and END WITH A DIRECT QUESTION, on its own line, asking whether to file follow-up issues and which ones. Filing happens only on the user's answer; silence is never consent. - -**Only a round where a fresh subagent actually REVIEWED counts toward ``.** A declined, errored, killed, or empty spawn produced no round, and neither did one that returned without reviewing, so neither can be the final review nor part of the total. An inline pass of your own is not a round at all. A cycle whose reviewer never reviewed has run ZERO rounds, and the honest report is that the review is blocked and why, not a count and not "ready to merge". - -If you cannot honestly say the final review left nothing must-fix open, you cannot say "ready to merge". Mention any finding you rejected as a false positive so the user can second-guess it. Every finding must be accounted for here as fixed, rejected-with-reason, or deferred, and must also appear on the PR, so the report and the PR agree; a deferred finding missing from its thread, this report, or the ledger is a dropped finding. - -**Merge is gated on green CI, enforced at the branch level, not by trust.** A PR must not merge until all CI checks pass. `main` branch protection requires the five `ci.yml` checks (Conventions, Unit+integration, Browser, E2E, Build) before any merge; if `gh api repos/webjsdev/webjs/branches/main/protection` shows `required_status_checks: null`, run `bash scripts/protect-main.sh` once (needs repo admin) to restore it. Do not work around a red or pending check. Wait for green, and fix whatever is red before merging. - -**This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate, which reads EVERY check and fixes what is red before merging, so a failure gets caught here whatever the ready-to-merge report claimed, and reading it earlier only parks the finished cycle on a multi-minute CI run. Be precise about what enforces that, because the two halves are not equally strong. Branch protection MECHANICALLY refuses the merge for the required contexts only, and `ci.yml` defines roughly twice as many jobs as `main` requires, so the rest are held by the INSTRUCTION above to read every check rather than by anything that can refuse a merge. Removing the end-of-cycle read did not weaken that, since the removed read was an instruction too and branch protection covered the same subset before and after. It does mean a green REQUIRED set is not the same as green CI, so read `gh pr checks` in full rather than trusting the merge button to have judged for you. What that costs is worth stating plainly, because it looks like a gap. Every `ci.yml` job with no counterpart in the deferred local suites now fails for the first time at merge rather than before the ready signal. That is a CLASS, not a list. Its membership moves as jobs are added and as the local suites grow to cover them, so derive it when you need it, by reading `.github/workflows/ci.yml` against the deferred set named above. Do NOT write the membership down here. - -That instruction is load-bearing and was expensive to learn. Three attempts to write the membership into this paragraph were each wrong, some by naming a job the local suites already cover, some by reading complete while omitting one they do not, and every one of them was caught by review rather than by the author. The reason the error keeps recurring is structural: deciding membership means holding `ci.yml` and the deferred set side by side and checking a non-obvious local counterpart for each job, which is a derivation, and a derivation frozen into prose is wrong the moment either side moves. So derive it, and do not leave the answer here for the next reader to trust. - -The class is real and not small, and what it costs is the same work done later rather than work skipped, which is the trade that was chosen. It is a real cost rather than a free one. The local suites the Definition of done demands still run at the end of the cycle and still gate the flip to ready for review, so only the CI read moved. +CI is read ONCE, at merge, never in a mid-work sleep loop. Read `gh pr checks` in full rather than trusting the merge button to have judged for you, because `ci.yml` defines roughly twice as many jobs as `main` requires, so the non-required ones are held by this instruction rather than by anything that can refuse a merge. **NEVER use `gh pr merge --admin` to bypass a FAILING check.** `--admin` skips ALL branch-protection gates, not only the review requirement, so a red check merges silently and lands broken code on `main`. This has happened (a Unit-test failure was admin-merged, breaking `main`). It is acceptable ONLY to bypass a required-review gate on a PR whose CI is confirmed all-green, so re-run `gh pr checks ` first and confirm EVERY check reads `pass` (a `BLOCKED` state can mean review-required OR a failing check, so never assume which). -### Subagent prompt template - -**The template fetches the diff and metadata over REST on purpose.** The porcelain equivalents go through GraphQL, and this template is pasted into EVERY reviewer in every round, so it was the single largest consumer of that budget in this skill. REST is a separate budget and returns the same bytes. Both are reads, so the read-only git constraint in the template is unaffected. See `.claude/gh-budget.md`. - -One template serves every reviewer in the cycle: round 1, each delta round, the final whole-diff review, the final review's fix-check, and a refuter. Only the question in its numbered step 5 changes. - -``` -Review PR # (branch ``) at https://github.com/webjsdev/webjs/pull/ for anything genuinely wrong with it, judged against the project's AGENTS.md and CONVENTIONS.md (root + per-package). - -HARD CONSTRAINT, read first: you are running against a repository the main session is actively using, and every worktree of it shares ONE `.git` directory, so a git write here reaches the main session's checkout even from an isolated worktree. You are a READ-ONLY reviewer. Do NOT run any command that changes git branch, HEAD, the index, or the working tree: no `git checkout`, `git switch`, `git reset`, `git restore`, `git stash`, `git pull`, `git fetch` that moves refs, `git merge`, `git rebase`, `git clean`, `git branch -f`, or `git worktree`. Any of these silently corrupts the main session's checkout (it moved HEAD off the branch and looked like lost work, and a stray worktree op once flipped the shared repo's `core.bare` to `true`). You do NOT need to switch branches to review. Use `gh api repos///pulls/ -H "Accept: application/vnd.github.diff"` for the diff and `gh api repos///pulls/` for metadata, and read any file at its PR-branch state with `gh api repos///contents/?ref= --jq .content | base64 -d`. All of those read from GitHub, so they work whether or not the branch exists locally, which matters because a PR you were asked to review may not be checked out here at all. If the branch does happen to be the one checked out, reading files in place is fine too. The only git you may run is read-only inspection (`git log`, `git show`, `git diff` WITHOUT changing state, `git status`, `git blame`). If you think you need to change git state to do the review, you are wrong; report what you found instead. - -You start with no prior context on this PR. Steps: - -1. Run `gh api repos/webjsdev/webjs/pulls/ -H "Accept: application/vnd.github.diff"` to see the full diff. -2. Run `gh api repos/webjsdev/webjs/pulls/ --jq '.title, .body'` to see what the author claims it does. -3. Read every file the diff touches in its current state (not just the diff hunks) so you see edits in context. -4. Read root AGENTS.md, the per-package AGENTS.md for each touched package, and CONVENTIONS.md if a scaffolded template was touched. -5. The question for this round is a SCOPE, not a checklist: .> Review it as a whole and report whatever is actually wrong. - -Report findings as a numbered list with file:line references. Problems only. No suggestions, no nits about style if the rule isn't enforceable. If you find nothing genuinely wrong, say exactly `CLEAN` on its own line and stop. Do not pad with "looks good overall" or summaries. (A REFUTER answers differently, since it is judging one claim rather than reviewing: say exactly `REFUTED` or `STANDS` on its own line, then one or two sentences of reason, and ignore the finding-list and `CLEAN` contract entirely.) - -If you CANNOT review (you could not fetch the diff, you have no access to the repo or PR, the branch does not resolve), say exactly `BLOCKED` on its own line followed by one line naming what you are missing. Do NOT report `CLEAN` in that case: `CLEAN` means you looked and found nothing, and reporting it for a review you could not perform is the single worst outcome here, because it ends the cycle on a review that never happened. -``` - ## After a merge: decide on a version bump, automatically After ANY PR that lands a user-facing change (a `feat` / `fix` / `perf` / `breaking` to a published package: `core`, `server`, `cli`, `ui`, `intellisense`, `mcp`; `intellisense` lives at `packages/editors/intellisense`, the rest at `packages/`) merges into `main`, assess whether a release bump is owed and open a release PR WITHOUT being asked. The user should not have to ask "do we need to bump versions?". Docs-only / chore / scaffold-doc changes do NOT bump on their own; they ride to the next functional bump. @@ -477,7 +329,7 @@ If a package has qualifying commits since its last `changelog//.md 4. `npm install --package-lock-only` and stage `package-lock.json` (a desync reds CI's `npm ci`). 5. Changelog: the pre-commit hook runs `scripts/backfill-changelog.js`, which parses `^(feat|fix|perf|breaking):` from commit subjects in the package's tree. **Squash-merge subjects are PR titles with no conventional prefix, so the generator finds nothing and the hook fails.** Hand-write `changelog//.md` (match an existing file's frontmatter: `package`, `version`, `date`, `commit_count`; sections ordered Breaking, Features, Performance, Fixes; entries link the PR and the squash commit) and stage it; then the commit passes. 6. Open the release PR. Note in the body that merging it adds the `changelog/**.md` files to `main`, which triggers `release.yml` to `npm publish` and cut GitHub Releases (idempotent). -7. Run the review cycle on it too (a release publishes to npm; a wrong bump level, missed package, or inaccurate changelog is worth catching). Merge is still user-gated. +7. Flip it ready for the owner's review like any other PR (a release publishes to npm, so a wrong bump level, missed package, or inaccurate changelog is worth their eyes). Merge is still user-gated. ### Then: make sure the deployed Railway services actually picked it up @@ -496,7 +348,7 @@ A merge updates `main` and npm, but the two in-repo apps deployed to Railway (`e - Opens the PR as a DRAFT at the START (step 6), not at the end. It is NOT created late once all the work is done. At draft-create time: - The body MUST include `Closes #` near the top so merging auto-closes the issue and the project card auto-moves to Done. If the work turns out to only partially address the issue, use a plain `#N` reference, not `Closes`. - The PR MUST be assigned to vivek7405 (`gh pr create ... --assignee vivek7405`). Matches the project's per-issue-owner convention. - - It stays a draft until the Definition of done is satisfied and the review cycle has finished; then `gh pr ready ` flips it to ready for review. + - It stays a draft until the Definition of done is satisfied, then `gh pr ready ` flips it to ready for the owner's review. - Does not make commits FOR you. Subsequent work follows the standard webjs git workflow (commit per logical unit, push after each, run tests before committing); those commits stream onto the already-open PR. - Does not merge. Merging is always user-approved per the project's git rules. @@ -504,8 +356,5 @@ A merge updates `main` and npm, but the two in-repo apps deployed to Railway (`e - If the TASK worktree's `git status` is dirty at start (a prior session died mid-work in it): stop and ask the user to commit, stash, or abandon that work. Never silently lose changes. A dirty PRIMARY checkout is not a blocker and not yours to fix; the worktree cuts from `origin/main` regardless. - If the issue is already in `In progress` (someone else's work, or a prior branch left open): report this and ask the user whether to continue on the existing branch, branch off a fresh main, or pick a different issue. -- If the TASK worktree regressed mid-loop (its HEAD detached or off the feature branch, work seemingly "gone"): a review subagent mutated shared git state. In the PRIMARY, HEAD on `main` is the healthy state, not a regression. Do NOT panic or redo work. The local feature-branch ref and `origin/` still point at the latest commit (every logical unit was pushed). Recover with `git -C checkout ` (anchored: run bare from the primary it would succeed and park the PRIMARY on the feature branch, since a detached worktree no longer holds it); confirm with `git log --oneline origin/main..HEAD` and `git status` clean. The PR on GitHub was never affected (the GitHub-reading reviewer still saw correct content), so no re-push or force-push is needed. -- If the FIFTH delta round still produces fixes: commit them, then stop instead of running a sixth. What makes this stop unfinished is not open findings, since that round's findings were fixed, but that its FIXES are on the branch unreviewed, so say exactly that. Report the rounds run, those unreviewed fixes, anything rejected or deferred along the way, and your read on why the fixes keep breaking something. Withhold the flip to ready for review; the branch, the commits, and the card all stay exactly as they are, and the deferred suites do not run, since they gate a flip that is not happening. -- If the final review's fix-check keeps surfacing must-fix findings (two of them in a row): stop there per the last step of the cycle, withhold the flip to ready for review, and report the rounds run, the open findings, and your read on why the fixes keep breaking something. The branch, the commits, and the card all stay exactly as they are. -- If a round's reviewer cannot be produced (a spawn declined at the permission prompt, an internal error, a killed task, a reviewer you stopped and re-spawned, or a return that is neither a finding list nor `CLEAN`): the round did not happen. Re-spawn it, varying the approach after a few identical failures, and do NOT stop mid-cycle to report the failure or ask how to proceed: recovering costs seconds and interrupting costs the cycle its momentum. Only a reviewer that cannot be produced at all blocks the cycle; then withhold the flip to ready for review and say once, at the end, that the review did not run and why. Keep that out of the PR body and PR comments, since a spawn that could not run is session tooling rather than a fact about the change. Do NOT review it yourself inline and count that as the round; an inline pass is what let three real bugs through a supposedly clean PR. The branch, the commits, and the card all stay exactly as they are. Full rules in the liveness block of the review cycle. +- If the TASK worktree regressed mid-loop (its HEAD detached or off the feature branch, work seemingly "gone"): a subagent mutated shared git state. In the PRIMARY, HEAD on `main` is the healthy state, not a regression. Do NOT panic or redo work. The local feature-branch ref and `origin/` still point at the latest commit (every logical unit was pushed). Recover with `git -C checkout ` (anchored: run bare from the primary it would succeed and park the PRIMARY on the feature branch, since a detached worktree no longer holds it); confirm with `git log --oneline origin/main..HEAD` and `git status` clean. The PR on GitHub was never affected (anything reading the PR from GitHub still saw correct content), so no re-push or force-push is needed. - If the `gh project item-edit` call fails (auth scope, missing field): report the failure clearly and offer to do the move manually via the web UI. The branch creation still stands. diff --git a/examples/blog/.agents/rules/workflow.md b/examples/blog/.agents/rules/workflow.md index d3f7c7c1d..26f7e0b62 100644 --- a/examples/blog/.agents/rules/workflow.md +++ b/examples/blog/.agents/rules/workflow.md @@ -53,19 +53,12 @@ Every code change must include: "N/A because " in the PR body. Docs land on the same PR as the code, never as a follow-up. 4. Convention check: `webjs check` must pass. -5. Pre-merge self-review loop. Before saying the PR is ready for merge, run - fresh-context review rounds until one round finds zero issues. Antigravity - primitive: open a new Cascade thread or a fresh side-panel session for - each round so the reviewer has no prior context on the implementation - decisions. Minimum two rounds; rotate focus each round. Skip the loop - only for one-line trivial changes; skipping on a change that touches - logic, public surface, build, security, or multiple files is the exact - failure mode the loop exists to prevent. The full rule, prompt template, - and reporting contract live in the **Pre-merge self-review loop** section - of CONVENTIONS.md. -The user should never have to ask for tests, documentation, or the -self-review loop. +How a PR gets REVIEWED is deliberately not specified here. Use whatever your +team already does. WebJs has opinions about the code (the conventions above, +`webjs check`, the test layers) and none about your review process. + +The user should never have to ask for tests or documentation. ## Git rules diff --git a/examples/blog/.cursorrules b/examples/blog/.cursorrules index c67687766..c1bbb5b50 100644 --- a/examples/blog/.cursorrules +++ b/examples/blog/.cursorrules @@ -57,18 +57,12 @@ Every code change must include: "N/A because " in the PR body. Docs land on the same PR as the code, never as a follow-up. 4. Convention check: `webjs check` must pass. -5. Pre-merge self-review loop. Before saying the PR is ready for merge, run - fresh-context review rounds until one round finds zero issues. Cursor - primitive: open a NEW composer tab and prompt the review there so the - reviewer has no prior context on your decisions. Minimum two rounds; - rotate focus each round. Skip the loop only for one-line trivial - changes; skipping on a change that touches logic, public surface, build, - security, or multiple files is the exact failure mode the loop exists - to prevent. The full rule, prompt template, and reporting contract live - in the **Pre-merge self-review loop** section of CONVENTIONS.md. -The user should never have to ask for tests, documentation, or the -self-review loop. +How a PR gets REVIEWED is deliberately not specified here. Use whatever your +team already does. WebJs has opinions about the code (the conventions above, +`webjs check`, the test layers) and none about your review process. + +The user should never have to ask for tests or documentation. ## Git rules diff --git a/examples/blog/.github/copilot-instructions.md b/examples/blog/.github/copilot-instructions.md index 56b0659db..03edfdbd5 100644 --- a/examples/blog/.github/copilot-instructions.md +++ b/examples/blog/.github/copilot-instructions.md @@ -53,19 +53,13 @@ Every code change must include: "N/A because " in the PR body. Docs land on the same PR as the code, never as a follow-up. 4. Convention check: `webjs check` must pass. -5. Pre-merge self-review loop. Before saying the PR is ready for merge, - run fresh-context review rounds until one round finds zero issues. - Copilot primitive: open a NEW chat session (reset the side panel) for - each round so the reviewer has no prior context on the implementation - decisions. Minimum two rounds; rotate focus each round. Skip the loop - only for one-line trivial changes; skipping on a change that touches - logic, public surface, build, security, or multiple files is the exact - failure mode the loop exists to prevent. The full rule, prompt - template, and reporting contract live in the **Pre-merge self-review - loop** section of CONVENTIONS.md. - -The user should never have to ask for tests, documentation, or the -self-review loop. The commit-per-logical-unit rule lives under "Git rules" + +How a PR gets REVIEWED is deliberately not specified here. Use whatever your +team already does. WebJs has opinions about the code (the conventions above, +`webjs check`, the test layers) and none about your review process. + +The user should never have to ask for tests or documentation. The +commit-per-logical-unit rule lives under "Git rules" below, not here, since it governs how work is grouped rather than what each change must include. diff --git a/packages/cli/templates/.agents/rules/workflow.md b/packages/cli/templates/.agents/rules/workflow.md index 384685f8d..3cfbf8b4e 100644 --- a/packages/cli/templates/.agents/rules/workflow.md +++ b/packages/cli/templates/.agents/rules/workflow.md @@ -59,9 +59,10 @@ Read `AGENTS.md` first. Full hosted docs are at https://webjs.dev/docs. tsconfig), either of which would 500 the app at runtime. Everything else it reports is a warning that cannot fail the build. Widen or narrow the gate in `package.json` rather than in the workflow. -5. Pre-merge self-review: before saying a PR is ready, run fresh-context review - rounds until one round finds zero issues (minimum two rounds, rotate focus). - Skip only for a one-line trivial change. + +How a PR gets REVIEWED is deliberately not specified here. Use whatever your +team already does. WebJs has opinions about the code (the conventions above, +`webjs check`, the test layers) and none about your review process. ## Git rules diff --git a/packages/cli/templates/.github/pull_request_template.md b/packages/cli/templates/.github/pull_request_template.md index 808394f48..c698a389e 100644 --- a/packages/cli/templates/.github/pull_request_template.md +++ b/packages/cli/templates/.github/pull_request_template.md @@ -33,7 +33,3 @@ in [`CONVENTIONS.md`](../CONVENTIONS.md) for the full guidance. there. - [ ] **Scaffold scripts / codegen** (if the project has any). Updated when the change affects what new instances generate. -- [ ] **Pre-merge self-review loop.** Ran N rounds; last round clean. - Skip only for one-line trivial changes. See the **Pre-merge - self-review loop** section in [`CONVENTIONS.md`](../CONVENTIONS.md) - for the prompt template and reporting contract. diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs deleted file mode 100644 index b4529c5a2..000000000 --- a/test/hooks/review-loop-exit.test.mjs +++ /dev/null @@ -1,308 +0,0 @@ -// Guards the pre-merge review cycle across the two files that carry it: the -// webjs-start-work skill (the normative rules) and the skill-routing hook -// (the injected review directive, which once kept an OLD exit condition -// after the skill had moved on, steering standalone reviews back into a -// shape the skill no longer described). -// -// These are static assertions over committed text on purpose: the cycle is -// prose executed by agents, so the counterfactual for "the cycle silently -// reverted" is the text no longer carrying its load-bearing anchors. Each -// assertion fails if its hunk is reverted or typo-drifted. - -import { test } from 'node:test'; -import assert from 'node:assert/strict'; -import { readFileSync, existsSync } from 'node:fs'; -import { fileURLToPath } from 'node:url'; -import { dirname, resolve } from 'node:path'; - -const here = dirname(fileURLToPath(import.meta.url)); -const repo = resolve(here, '../..'); -const skill = readFileSync(resolve(repo, '.claude/skills/webjs-start-work/SKILL.md'), 'utf8'); -const hook = readFileSync(resolve(repo, '.claude/hooks/route-skills.sh'), 'utf8'); - -test('the skill prescribes ONE reviewer, never a fleet', () => { - // Round 1 is a single reviewer over the whole diff, with no path-based - // tier choice deciding its shape. - assert.match(skill, /Round 1: ONE fresh reviewer over the WHOLE diff/); - assert.match(skill, /No fleet, no lenses, no jury, no per-diff tier choice/); - // Later rounds narrow the QUESTION to the fixes, not the evidence. The - // fast default allows exactly one such round; the escalated cycle chains - // them. Both phrasings must keep saying the question is the fix commits. - assert.match(skill, /delta-scoped reviewer whose QUESTION is those fix commits alone/); - assert.match(skill, /each later round is delta-scoped to the previous round's fix commits/); - // Structure alone cannot bound a chain where every fix produces the next - // round's finding, which is the case the deleted budget was written for. - assert.match(skill, /\*\*A delta chain that keeps producing fixes stops after the FIFTH DELTA ROUND\*\*/); - // The stop needs a reporting shape of its own: the reporting section - // covers only the converged path. - assert.match(skill, /If the FIFTH delta round still produces fixes/); - // What makes this stop unfinished is unreviewed fixes, not open findings, - // which is the opposite of its sibling stop and the thing a report copied - // from that sibling gets wrong. - assert.match(skill, /but that its FIXES are on the branch unreviewed, so say exactly that/); - // And the do-not-restore paragraph must admit the cap came back rather - // than claiming termination is entirely structural. - assert.match(skill, /The one exception to the removals is the round cap, which came back in a narrower form/); - // The final whole-diff pass is what a clean delta round buys, and it is - // the reason a clean round is not by itself the end. - assert.match(skill, /buys the FINAL review: ONE fresh reviewer over the WHOLE diff again/); - assert.match(skill, /Either way a clean round is not the end of the THOROUGH cycle/); - // The final review's findings end in a fix plus ONE delta check of that - // fix, which does not re-open the cycle. - assert.match(skill, /Its fixes get ONE delta-scoped check of those fix commits alone/); - assert.match(skill, /The cycle ends when nothing must-fix is left OPEN/); - // The same two words for the same disposition in every rule that names - // it, since "refuted or deferred" silently dropped the unrefutable - // rejection the refuter-unavailable rule creates. - assert.match(skill, /every must-fix finding it raised was rejected or deferred/); - assert.match(skill, /The final review is never re-run and the delta rounds are never re-entered/); - // A fix-check that finds something gets ONE more of the same shape, then - // the cycle stops unfinished rather than looping. - assert.match(skill, /ONE more check of the same shape, and if that one does too, stop and report the PR unfinished/); - // NOTE: a clean round 1 no longer buys the final review unconditionally. - // That floor is now bought by the escalation ladder, which the FAST - // default test below owns; asserting it here too would contradict it. -}); - -test('the skill pins every reviewer to Opus, async, and worktree-isolated', () => { - assert.match(skill, /`model: "opus"` \(Opus 5, always, no other model anywhere in the cycle\)/); - assert.match(skill, /`run_in_background: true`/); - assert.match(skill, /`isolation: "worktree"`/); - assert.match(skill, /`subagent_type: "general-purpose"`/); - // No reviewer anywhere in the skill is pinned to another model family. - assert.ok(!/fable/i.test(skill), 'a reviewer was pinned back to fable'); -}); - -test('FAST is the default cycle, and the thorough one is bought by evidence', () => { - // The owner should never have to ask for a short cycle. A reviewer spawn - // costs about 10 minutes, so an unconditional second round taxes every PR - // to cover a miss that mostly matters on a few surfaces. - assert.match(skill, /\*\*FAST is the default shape\.\*\*/); - assert.match(skill, /a clean or minor-only round 1 finishes the cycle with ONE review/i); - // Speed must come from fewer ROUNDS, never from a softer bar inside one. - // Reclassifying findings as minor is the cheat this forbids. - assert.match(skill, /Speed is bought by running fewer rounds, NEVER by lowering the bar inside a round/); - - // All three escalation triggers, since dropping any one silently widens - // the fast path over changes that were meant to get the second read. - assert.match(skill, /the owner asks for a thorough, full, or deep review/); - assert.match(skill, /TWO OR MORE must-fix findings/); - assert.match(skill, /the serializer, SSR or action dispatch, auth or session, the client router, or the elision analyser/); - - // The final review still EXISTS; it is conditional, not deleted. - assert.match(skill, /buys the FINAL review: ONE fresh reviewer over the WHOLE diff again/); - // And the five-delta cap survives on the escalated path. - assert.match(skill, /\*\*A delta chain that keeps producing fixes stops after the FIFTH DELTA ROUND\*\*/); - - // A review finding that is out of scope is reported, not filed. - assert.match(skill, /\*\*Do not file follow-up issues for what a review turns up\.\*\*/); - - // The do-not-restore note must record WHY the two-review floor went, or - // the next agent reads the missing final review as a regression to fix. - assert.match(skill, /Do NOT reinstate an unconditional final review or an unconditional two-review floor/); - assert.ok( - !/every PR gets at least two reviews/.test(skill), - 'the unconditional two-review floor is back in the skill', - ); - - // The hook mirrors the skill, so the default must match on both sides. - assert.match(hook, /defaults to its FAST shape/); - assert.match(hook, /Escalate to the THOROUGH shape/); - assert.match(hook, /Do not file follow-up issues for review findings that are out of scope/); -}); - -test('the minor / must-fix call is by surface, not by importance', () => { - assert.match(skill, /\*\*Minor or must-fix\.\*\*/); - // The three must-fix surfaces, including the tautological-test case that - // an importance test would wrongly eject. - assert.match(skill, /ability to OBSERVE the defect it claims to cover/); - assert.match(skill, /factual claim about runtime behavior in docs/); - assert.match(skill, /Judge by SURFACE, never by importance/); - // Doubt resolves toward keeping the cycle open. - assert.match(skill, /When it could go either way, it is must-fix/); - // Only a FIX buys a round. A rejection produces no fix commits, so a - // round would re-pose the same question over an unchanged head to a - // reviewer that is never told what was already handled, which is how the - // pre-final loop lost its bound when the round budget went. The refuter - // is what adjudicates a rejection, and it terminates in one spawn. - // Under the fast default it is specifically a MUST-FIX fix, since a - // minor-only round cannot introduce the class of defect a round catches. - assert.match(skill, /\*\*Only a MUST-FIX fix buys a round, and it buys exactly ONE:\*\*/); - assert.match(skill, /A rejection buys one REFUTER, a deferral buys nothing/); - assert.match(skill, /what a rejection buys instead of a whole round, and it terminates: one spawn per rejection, never a refuter of a refuter/); - // A refuter has two verdicts and the cycle must define both, or a finding - // whose rejection was contradicted ends with no disposition at all. - assert.match(skill, /On the post-rejection use only, a refuter that answers `STANDS` has contradicted your rejection/); - // The pre-action gate has no rejection to overturn, so STANDS means the - // finding is real, and a final-phase fix cannot buy a round that phase - // forbids. - assert.match(skill, /On the pre-action gate there is no rejection yet/); - assert.match(skill, /on the final review it joins that phase's single fix-check instead/); - // The two uses fail in opposite directions when no refuter can be spawned. - assert.match(skill, /on the pre-action gate, act on the finding as real/); - assert.match(skill, /keep the rejection but record it as UNREFUTED/); -}); - -test('the removed machinery stays removed, with the reason recorded', () => { - // The paragraph that tells a future reader the omissions were deliberate. - assert.match(skill, /Do not restore what this replaced/); - assert.match(skill, /Almost all of it is gone on purpose: termination is mostly structural now/); - // The fleet workflow itself is gone from the repo. - assert.ok(!existsSync(resolve(repo, '.claude/workflows/deep-review.js')), 'the deep-review fleet workflow is back'); - // None of the removed mechanisms may re-enter, as a rule or as vocabulary. - // The do-not-restore paragraph describes them WITHOUT these words ("two - // tiers", "a 5-round budget", "poll a file"), so the expected count is - // zero and any occurrence is a re-introduction rather than a mention. - for (const [label, re] of [ - ['the substantive/prose tier vocabulary', /substantive[^.]{0,40}(tier|prose)|(tier|prose)[^.]{0,40}substantive/i], - ['the round budget', /round budget|over budget/i], - ['the polling watchdog', /watchdog/i], - ]) { - assert.ok(!re.test(skill), `${label} is back in the skill`); - } -}); - -test('CI is read only at the merge gate, never at the end of the cycle', () => { - // The third removal in this section, pinned the same way as the fleet and - // the two-review floor above: the note that records WHY it went, plus the - // counterfactual that the read itself has not crept back. - assert.match(skill, /\*\*This is the ONLY place CI is read, on purpose\. Do not add one back to the end of the review cycle\.\*\*/); - // The cost it accepts must stay stated, or the next reader takes the - // removal for an oversight and restores the read to "fix" it. It is stated - // as an open CLASS derived from ci.yml, never a written-down membership. - // Three attempts at writing that membership down were each wrong, in one - // direction or the other, so the instruction not to enumerate is itself - // the load-bearing part and is pinned alongside the class phrasing. - assert.match(skill, /Every `ci\.yml` job with no counterpart in the deferred local suites/); - assert.match(skill, /That is a CLASS, not a list/); - assert.match(skill, /Do NOT write the membership down here/); - - // The four instructions that USED to make the cycle wait on CI. Each is - // gone, and a revert of the hunk that removed it puts its phrasing back. - for (const [label, re] of [ - ['the ready-to-merge condition', /suites it deferred have run AND CI has been read green/], - ['the keep-the-cycle-fast rule', /Never wait on CI between rounds/], - ['the end-of-cycle batch', /and only now read CI|plus a background CI watch/], - ['the report preamble', /the deferred suites, and the CI read, report exactly/], - ]) { - assert.ok(!re.test(skill), `${label} tells the cycle to read CI again`); - } - - // The gate leans on reading every check, NOT on branch protection alone, - // which mechanically covers only the required subset. Overstating that was - // the removal's original justification and it was false for exactly the - // jobs it was justifying the cost of. - assert.match(skill, /Branch protection MECHANICALLY refuses the merge for the required contexts only/); - assert.match(skill, /a green REQUIRED set is not the same as green CI/); - - // The gate the removal leans on has to stay strict, since it is now the - // only CI checkpoint there is. - assert.match(skill, /\*\*Merge is gated on green CI, enforced at the branch level, not by trust\.\*\*/); - assert.match(skill, /\*\*NEVER use `gh pr merge --admin` to bypass a FAILING check\.\*\*/); -}); - -test('the cycle keeps the guarantees the trim was not allowed to touch', () => { - // A fix is never the end: the delta round after a fix is what the whole - // cycle exists to force. - assert.match(skill, /A fix changes the branch, so the changed branch needs its own round/); - // The pre-fix wording said "Fixing (or rejecting) a must-fix finding - // changes the branch", which both restates the reversed rule and is not - // true of a rejection. It must not survive anywhere in the file. - assert.ok(!/Fixing \(or rejecting\)/.test(skill), 'the reversed rejection-buys-a-round rule is back'); - // A dead or non-reviewing spawn is not a round, and an inline pass is - // never a substitute for one. - assert.match(skill, /A dead spawn is not a round/); - // Waiting must not block the turn, which is the whole point of spawning - // the reviewer in the background, and the optional progress check must - // never kill on a timer or on a flat file (elapsed time says nothing: a - // reviewer here runs 5 to 10 minutes while working normally, and a file - // that never grows may simply be a stub the harness does not write to). - assert.match(skill, /\*\*Waiting is not blocking\.\*\*/); - assert.match(skill, /never wait on it with a foreground `sleep`/); - assert.match(skill, /\*\*A progress check is optional, runs in the BACKGROUND, and never kills\.\*\*/); - assert.match(skill, /elapsed time never proves a stall/); - assert.match(skill, /never wire one to a flat file or a timer/); - // There is no automatic trigger beyond a dead status, so giving up is a - // deliberate call, and that call is the one place elapsed time legitimately - // counts. It is scoped to the state with nothing to read, it requires - // having actually probed, and it stops the reviewer rather than leaving a - // second one in flight. - assert.match(skill, /Giving up applies ONLY where there is no growth to see/); - assert.match(skill, /that judgement is the one place elapsed time legitimately counts/); - // The give-up clause must not reach the one state that HAS a positive - // signal, or it contradicts "leave it alone however long it has run". - assert.match(skill, /A transcript that is still growing is never abandoned, whatever the clock says/); - // An abandoned reviewer was produced and may still return, so it is not a - // failed spawn, and its late findings are read rather than discarded. - // "No growth to see" must mean you looked, or never probing re-authorizes - // abandoning a reviewer that is in fact working. - assert.match(skill, /means you PROBED and saw none, never that you did not look/); - // Giving up stops the reviewer, so no second one is ever left in flight to - // return late findings the cycle has no phase or review object to absorb. - assert.match(skill, /Giving up means STOPPING it \(`TaskStop`\) and re-spawning/); - assert.ok(!/Take whichever returns first as the round/.test(skill), 'the two-reviewers-in-flight case is back'); - // Step 1 must not ban polling outright while the bullet below sanctions a - // background size probe of the same spawn. - assert.ok(!/Do not poll it and do not re-read its message/.test(skill), 'step 1 bans the probe the progress check sanctions'); - assert.match(skill, /Do not badger it for results/); - // And the do-not-restore paragraph must not claim a replacement mechanism - // richer than what is actually there, which is how a polling watchdog - // grew back once already. - assert.match(skill, /the harness completion notification is the signal, with at most an optional background progress check that never kills anything/); - // The liveness sentence must not claim the harness status is the ONLY - // signal while the progress check reads byte growth as one. - assert.match(skill, /Only two things are evidence a reviewer is alive/); - assert.match(skill, /Never read the transcript.s contents/); - assert.match(skill, /A reviewer that returns without reviewing is also not a round/); - assert.match(skill, /NEVER substitute an inline self-review/); - // The literal sentinels the cycle reads a reviewer's answer by. - assert.match(skill, /say exactly `CLEAN` on its own line and stop/); - assert.match(skill, /say exactly `BLOCKED` on its own line/); - // The template must not keep offering the no-fix-commit delta round that - // cycle step 3 replaced with the final review, since following it re-opens - // the unbounded path. - assert.ok(!/delta round following a round with no fix commits/.test(skill), 'the removed no-fix delta round is back in the prompt template'); - // Working-tree safety: isolation, the read-only git prohibition, and the - // per-spawn repo-health check that catches a leaked worktree. - assert.match(skill, /\*\*Working-tree safety\.\*\*/); - assert.match(skill, /You are a READ-ONLY reviewer/); - assert.match(skill, /After EACH spawn resolves, before acting on findings, check the repo/); - // Reviewers stay starved of prior review context. - assert.match(skill, /NEVER prior PR comments or reviews/); - // The prompt sets the scope and nothing else. A defect-class checklist - // narrows the reviewer to what the author already suspects, which is the - // bias a fresh reviewer exists to escape. - assert.match(skill, /\*\*Do not tell it what to look for\.\*\*/); - assert.match(skill, /no list of defect classes, no "specifically check for X and Y"/); - assert.match(skill, /The question for this round is a SCOPE, not a checklist/); - // The three dispositions and the deferral ledger survive. - assert.match(skill, /the cycle never files a follow-up issue on its own/); - assert.match(skill, /final summary review also carries a deferral ledger/); - assert.match(skill, /END WITH A DIRECT QUESTION/); -}); - -test('the routed review directive states the same cycle as the skill', () => { - // The directive must not describe a fleet or a tiered round 1. - assert.ok(!/deep-review/.test(hook), 'route-skills.sh still routes round 1 to the deep-review fleet'); - assert.ok(!/round budget|OVER BUDGET/i.test(hook), 'route-skills.sh reverted to the round budget'); - // The shape, in lockstep with the skill. - assert.match(hook, /ONE fresh reviewer over the whole diff, never a fleet/); - assert.match(hook, /each later round is delta-scoped/); - assert.match(hook, /the first round that produces no fixes, whether it found nothing must-fix or everything it found was rejected or deferred, buys a FINAL review over the whole diff again/); - assert.match(hook, /model opus \(Opus 5, never fable\)/); - assert.match(hook, /isolation worktree/); - // The judgment rule, including its fail-open direction. - assert.match(hook, /MINOR or MUST-FIX by SURFACE, never by importance/); - assert.match(hook, /when it could go either way it is must-fix/); - assert.match(hook, /ONE delta check of that fix alone/); - assert.match(hook, /Never report the PR ready off a round that found something must-fix/); - // The directive must resolve the fix-check case the SAME way the skill - // does, and must not both end the cycle and forbid reporting it. - assert.match(hook, /the cycle ends when nothing must-fix is left open, meaning the check came back with nothing or there was no fix to check because every must-fix finding the final review raised was rejected or deferred/); - assert.match(hook, /a check that does find something must-fix gets that fixed and one more check of the same shape, and only if that one also finds something must-fix do you stop and report the PR unfinished/); - assert.match(hook, /Only a FIX buys another round/); - assert.match(hook, /a delta chain that keeps producing fixes stops after the fifth delta round, unfinished, rather than continuing/); - // The code-review skill's own findings are input to the cycle, not a - // round of it. - assert.match(hook, /auxiliary input, not as a round of it/); -}); diff --git a/test/hooks/route-skills.test.mjs b/test/hooks/route-skills.test.mjs index 6ad2d55a5..fa5ba6685 100644 --- a/test/hooks/route-skills.test.mjs +++ b/test/hooks/route-skills.test.mjs @@ -178,7 +178,7 @@ test('webjs-instagram-post stays quiet without instagram intent', () => { assert.ok(!routed(ctx, 'webjs-instagram-post'), 'non-instagram publish must not route to instagram'); }); -test('code-review routes on review phrases', () => { +test('pr-review routes on review phrases', () => { for (const p of [ 'review the PR', 'review my changes', @@ -187,10 +187,43 @@ test('code-review routes on review phrases', () => { 'review the branch', ]) { const { ctx } = run(p); - assert.ok(routed(ctx, 'code-review'), `expected code-review route for: ${p}`); + assert.ok(routed(ctx, 'pr-review'), `expected pr-review route for: ${p}`); } }); +// The routing test above only proves a skill NAMED pr-review is reached. It +// says nothing about what the injected directive tells the model to DO, and +// the rules below are the whole point of the skill: reviews happen inline, +// the reviewer only reviews, and it never blocks on CI. Nothing else in the +// suite would notice if a round count, a reviewer subagent, or a CI wait +// were put back, so both carriers of those rules are pinned here. +// +// This replaces test/hooks/review-loop-exit.test.mjs, which guarded the +// previous review cycle's load-bearing wording across the same two files and +// caught them drifting apart once. +test('the pr-review directive and skill both keep the review contract', () => { + const { ctx } = run('review the PR'); + const directive = ctx.split('\n').find((l) => l.startsWith('- pr-review:')); + assert.ok(directive, 'expected a pr-review directive line'); + const skill = readFileSync(resolve(REPO, '.claude/skills/pr-review/SKILL.md'), 'utf8'); + + // Inline, never delegated: a subagent reviewer is what this replaced. + assert.match(directive, /NEVER spawn a reviewer subagent/); + assert.match(skill, /Never spawn a reviewer subagent/i); + // One read, not a cycle. A round count coming back is the regression. + assert.match(directive, /NEVER run a multi-round review cycle/); + assert.match(skill, /never run a multi-round\s+review cycle/i); + // Review only: fixing the findings is the author's job, on a separate ask. + assert.match(directive, /does not fix findings/); + assert.match(skill, /\*\*Review only\.\*\* The reviewer never fixes what it finds/); + // Never blocks on CI, which is the merge gate's business. + assert.match(directive, /never waits on or reports CI/); + assert.match(skill, /\*\*No CI\.\*\* Never wait on, read, or report CI/); + // The output is a real GitHub review object, not a chat-only reply. + assert.match(directive, /post the review through the GitHub review API as ONE review object/); + assert.match(skill, /pulls\/\/reviews/); +}); + test('verify routes on verify / dogfood phrases', () => { for (const p of [ 'verify the fix works', @@ -234,13 +267,14 @@ test('every skill the hook can route to is committed in-repo (no dangling refere // The hook names skills it routes to; each PROJECT skill MUST have a // committed `.claude/skills//SKILL.md`, or a fresh clone routes a // prompt at a skill that does not exist (the #543 portability bug). The - // regex matches only project-skill names (webjs-* and use-railway); - // built-in Claude Code skills the hook also routes (code-review, verify) - // ship with the CLI for everyone, so they are intentionally exempt. + // regex matches only project-skill names (webjs-*, use-railway, + // pr-review); the built-in Claude Code skill the hook also routes + // (verify) ships with the CLI for everyone, so it is intentionally + // exempt. // Extract the project-skill names from the hook source and assert each is // present in the repo. const hookSrc = readFileSync(HOOK, 'utf8'); - const names = [...new Set((hookSrc.match(/\b(?:webjs-[a-z-]+|use-railway)\b/g) || []))]; + const names = [...new Set((hookSrc.match(/\b(?:webjs-[a-z-]+|use-railway|pr-review)\b/g) || []))]; assert.ok(names.length >= 4, `expected the hook to reference its skills; found ${names.join(', ')}`); for (const name of names) { const skillFile = resolve(REPO, '.claude/skills', name, 'SKILL.md'); diff --git a/test/scaffolds/scaffold-agent-skills.test.js b/test/scaffolds/scaffold-agent-skills.test.js new file mode 100644 index 000000000..4909656eb --- /dev/null +++ b/test/scaffolds/scaffold-agent-skills.test.js @@ -0,0 +1,82 @@ +/** + * Guards the boundary between the monorepo's OWN agent skills and what a + * generated app ships. + * + * The framework keeps a set of workflow skills for working on WebJs itself + * (`.claude/skills/*`, exposed cross-agent through `.agents/skills/*` + * symlinks). Exactly ONE of them, `webjs`, is a teaching surface for app + * authors and belongs in a generated app. Everything else is monorepo process: + * how we file issues, how we sync docs, how we review our own PRs. Shipping + * any of it would push our process onto a team that already has its own, and + * `pr-review` is the live example, since how a team reviews pull requests is + * theirs to decide. + * + * Nothing structural enforces that today: `create.js` copies the skill by a + * hardcoded path, so a future change that copies the skills DIRECTORY instead + * would ship the lot silently, and the app would look fine. These assertions + * are that enforcement. + * + * They also pin the reverse direction: `webjs` must actually arrive, since the + * skill is the only teaching surface that survives `npm run gallery:clear`, so + * an app missing it has no reference at all once the gallery is stripped. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtemp, rm, readFile } from 'node:fs/promises'; +import { existsSync, readdirSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +import { scaffoldApp } from '../../packages/cli/lib/create.js'; + +// The one skill an app author needs. Anything else under .agents/skills/ in a +// generated app is monorepo process that leaked. +const APP_SKILLS = ['webjs']; + +for (const template of ['full-stack', 'api']) { + test(`${template} scaffold ships the webjs skill and no monorepo process skills`, async () => { + const cwd = await mkdtemp(join(tmpdir(), 'webjs-scaffold-skills-')); + try { + await scaffoldApp('demo', cwd, { template, install: false }); + const skillsDir = join(cwd, 'demo', '.agents', 'skills'); + assert.ok(existsSync(skillsDir), '.agents/skills/ must exist in a generated app'); + + const shipped = readdirSync(skillsDir).sort(); + assert.deepEqual(shipped, APP_SKILLS, + `a generated app must ship exactly ${APP_SKILLS.join(', ')}; found ${shipped.join(', ') || '(none)'}`); + + // The skill has to be the real thing, not an empty directory. + assert.ok(existsSync(join(skillsDir, 'webjs', 'SKILL.md')), 'the `webjs` skill ships its SKILL.md'); + assert.ok(existsSync(join(skillsDir, 'webjs', 'references')), 'the `webjs` skill ships its references/'); + + // Named explicitly so the failure message says WHY, rather than only + // that a directory listing changed. + for (const monorepoOnly of ['pr-review', 'webjs-start-work', 'webjs-file-issue', 'webjs-doc-sync']) { + assert.ok(!existsSync(join(skillsDir, monorepoOnly)), + `${monorepoOnly} is a monorepo workflow skill and must not ship to a generated app`); + } + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); +} + +test('the generated agent rules do not mandate a review process', async () => { + // WebJs is opinionated about the code (conventions, `webjs check`, the test + // layers) and deliberately silent on how a team reviews a PR. The scaffold + // used to ship a mandatory multi-round self-review loop plus a PR-template + // checkbox pointing at a CONVENTIONS.md section that did not exist. + const cwd = await mkdtemp(join(tmpdir(), 'webjs-scaffold-skills-')); + try { + await scaffoldApp('demo', cwd, { template: 'full-stack', install: false }); + for (const rel of ['.agents/rules/workflow.md', '.github/pull_request_template.md']) { + const p = join(cwd, 'demo', rel); + if (!existsSync(p)) continue; + const src = await readFile(p, 'utf8'); + assert.doesNotMatch(src, /self-review/i, `${rel} must not mandate a self-review loop`); + assert.doesNotMatch(src, /fresh-context review/i, `${rel} must not mandate review rounds`); + } + } finally { + await rm(cwd, { recursive: true, force: true }); + } +});