diff --git a/.claude/hooks/require-worktree-for-edits.sh b/.claude/hooks/require-worktree-for-edits.sh new file mode 100755 index 000000000..941e0a5b1 --- /dev/null +++ b/.claude/hooks/require-worktree-for-edits.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# PreToolUse hook (Write|Edit|MultiEdit|NotebookEdit): BLOCK editing a TRACKED +# file that resides in a repo's PRIMARY checkout. Task work always happens in +# a linked worktree (AGENTS.md: one task per git worktree, ALWAYS); the +# primary checkout stays an untouched mirror of main. Untracked and gitignored +# files (session state, scratch, settings.local.json) stay editable. +# The predicate runs against the FILE's directory, never the session cwd: the +# harness resets cwd to the primary checkout between commands while edits +# legitimately target worktree files by absolute path. +# Contract: exit 0 = allow, exit 2 = block (message on stderr). +# Escape hatch: WEBJS_NO_WORKTREE_GATE=1. + +if [ "${WEBJS_NO_WORKTREE_GATE:-0}" = "1" ]; then exit 0; fi +if ! command -v jq >/dev/null 2>&1; then exit 0; fi + +input=$(cat) +fp=$(printf '%s' "$input" | jq -r '.tool_input.file_path // .tool_input.notebook_path // empty') +if [ -z "$fp" ]; then exit 0; fi + +dir=$(dirname "$fp") +if [ ! -d "$dir" ]; then exit 0; fi +if ! git -C "$dir" rev-parse --is-inside-work-tree >/dev/null 2>&1; then exit 0; fi + +# Both paths ABSOLUTE, or the comparison lies: invoked via -C on a SUBDIR of +# the primary, git prints --git-dir absolute and --git-common-dir relative +# (../../.git), so a raw string compare misclassifies the primary as a linked +# worktree and the gate fails open for the whole source tree (caught in +# review; the tests only covered root-level files). +gd=$(git -C "$dir" rev-parse --path-format=absolute --git-dir 2>/dev/null) || exit 0 +gcd=$(git -C "$dir" rev-parse --path-format=absolute --git-common-dir 2>/dev/null) || exit 0 +if [ -z "$gd" ] || [ -z "$gcd" ]; then exit 0; fi +# Linked worktree: --git-dir carries /worktrees/ and differs from the +# common dir. Only the primary checkout has them equal. +if [ "$gd" != "$gcd" ]; then exit 0; fi + +# Untracked or gitignored files are session-local and stay editable. +if ! git -C "$dir" ls-files --error-unmatch -- "$fp" >/dev/null 2>&1; then exit 0; fi + +top=$(git -C "$dir" rev-parse --show-toplevel 2>/dev/null || echo "this repo") +{ + echo "BLOCKED: '$fp' is a tracked file in the PRIMARY checkout ($top)." + echo "Task work always happens in a linked worktree (AGENTS.md: one task per git worktree, ALWAYS):" + echo " git worktree add -b / ../- origin/main" + echo "Work there by absolute path; the primary checkout stays on main untouched." + echo "Escape hatch for a deliberate exception: WEBJS_NO_WORKTREE_GATE=1." +} >&2 +exit 2 diff --git a/.claude/settings.json b/.claude/settings.json index edba8f7e2..7a0487212 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -1,4 +1,9 @@ { + "permissions": { + "allow": [ + "Agent" + ] + }, "hooks": { "UserPromptSubmit": [ { @@ -11,6 +16,15 @@ } ], "PreToolUse": [ + { + "matcher": "Write|Edit|MultiEdit|NotebookEdit", + "hooks": [ + { + "type": "command", + "command": ".claude/hooks/require-worktree-for-edits.sh" + } + ] + }, { "matcher": "Write|Edit|MultiEdit", "hooks": [ diff --git a/.claude/skills/webjs-blog-write/SKILL.md b/.claude/skills/webjs-blog-write/SKILL.md index 16d97c961..225eadc25 100644 --- a/.claude/skills/webjs-blog-write/SKILL.md +++ b/.claude/skills/webjs-blog-write/SKILL.md @@ -90,8 +90,8 @@ The test before you ship: read the draft aloud and ask whether the author would ## Process: from topic to a published, true post 1. **Pick an SEO topic backed by shipped work.** Mine the git history, the merged PRs, the per-package changelog, and the closed issues for a real, shipped capability that a developer would search for and that no existing post already covers. High-intent angles (migration from another framework, a concrete how-to) rank best. -2. **De-duplicate against EVERY existing post, directly AND indirectly.** Read the titles and skim the bodies of all `blog/*.md`. A new post must not restate a published one, even partially. If two candidate topics overlap, either merge them or sharpen each to a distinct angle and cross-link rather than repeat. Two posts that share a mechanism should each own a different facet and reference the other, not re-explain it. -3. **For a feature post, verify every factual claim by dogfooding before you publish.** This is not optional for a post that describes runtime behaviour. Scaffold a real app (`webjs create`), boot it (on Node AND Bun where the claim is runtime-sensitive), and confirm each claim with a real probe: `curl` for HTTP and header behaviour, a browser for hydration, streaming, client-router, and a11y. If a claim turns out false, FIX the post to state the truth, and file a framework issue via `webjs-file-issue` if the framework itself is wrong. A blog post that states a behaviour the framework does not have is the failure this step exists to prevent. Do not report a post done off unverified claims. +2. **De-duplicate against EVERY existing post, directly AND indirectly.** Delegate the sweep to one read-only `Explore` agent over `blog/*.md` returning per-post topic-and-angle summaries (the corpus is small, so the win is keeping your context clean rather than parallelism), then judge overlap yourself against its return. A new post must not restate a published one, even partially. If two candidate topics overlap, either merge them or sharpen each to a distinct angle and cross-link rather than repeat. Two posts that share a mechanism should each own a different facet and reference the other, not re-explain it. +3. **For a feature post, verify every factual claim by dogfooding before you publish.** This is not optional for a post that describes runtime behaviour. Scaffold a real app (`webjs create`) and run the verification's independent parts as ONE batch of background Bash tasks: the Node boot and the Bun boot in parallel (BOTH stay mandatory where a claim is runtime-sensitive, one never satisfies the other), with the per-claim `curl` probes against whichever boot they need, and EVERY task's result collected before the post is called verified, since a forgotten background probe is an unverified claim shipping. Browser-dependent probes (hydration, streaming, client-router, a11y) stay in the main session, where the browser tooling lives. Confirm each claim with a real probe: `curl` for HTTP and header behaviour, a browser for hydration, streaming, client-router, and a11y. If a claim turns out false, FIX the post to state the truth, and file a framework issue via `webjs-file-issue` if the framework itself is wrong. A blog post that states a behaviour the framework does not have is the failure this step exists to prevent. Do not report a post done off unverified claims. 4. **Write the post** in the voice from Step 0, obeying the hard rules. 5. **Place the file** at repo-root `blog/.md`. Do not put it under `website/`. Confirm the front matter has `title` and `date` (the index drops files missing either). 6. **Self-check before committing.** Grep your own draft: `grep -nE '#[0-9]{3,4}' blog/.md` must return nothing, and scan for process tells ("dogfood", "self-review", "verified by"). Confirm no em-dash or banned pause punctuation. Read the whole post once as a reader who has never seen the codebase. diff --git a/.claude/skills/webjs-doc-sync/SKILL.md b/.claude/skills/webjs-doc-sync/SKILL.md index 50d9079ac..4ee17f062 100644 --- a/.claude/skills/webjs-doc-sync/SKILL.md +++ b/.claude/skills/webjs-doc-sync/SKILL.md @@ -84,6 +84,8 @@ applies, then update or consciously skip each. ## Per-change sync procedure +The surface checks below are independent reads, so run them fanned out: one read-only `Explore` agent per doc surface, spawned in ONE message, each answering "does this surface describe the changed behavior, and does it still tell the truth at head?". The agents REPORT; the edits stay in the main session, because parallel writers on overlapping docs collide. A single-surface change needs no fan-out. + 1. Identify the change's IDENTIFYING TOKENS: the export name, CLI flag, config key, file-convention string, or feature phrase a doc would mention. 2. Grep those tokens across every surface to see where the feature is (or should @@ -103,6 +105,8 @@ applies, then update or consciously skip each. ## Audit-mode procedure (sweep shipped work for drift) +An audit is the fan-out's best case: every surface of the map gets its own read-only `Explore` agent, all spawned in one message, so the sweep costs the slowest surface rather than the sum. Reads fan out, writes stay here. + Use this to find existing gaps (for example, across the Done items on the project board): diff --git a/.claude/skills/webjs-file-issue/SKILL.md b/.claude/skills/webjs-file-issue/SKILL.md index acf4babc9..18f9bfd54 100644 --- a/.claude/skills/webjs-file-issue/SKILL.md +++ b/.claude/skills/webjs-file-issue/SKILL.md @@ -39,6 +39,8 @@ If the user's description is very thin (e.g. "track adding dark mode as a todo") A few greps now save the implementing agent a cold-start investigation later, and the difference is visible in the filed issue. + **For an issue touching more than one area, fan the grounding out instead of hunting serially.** The four collections above are independent reads, so spawn parallel READ-ONLY `Explore` agents in ONE message, one per concern (where-to-edit paths; landmines via `git log` and prior-issue search; invariants from AGENTS.md; test and doc surfaces), then synthesize their returns into the Implementation-notes section yourself. Explore agents cannot write, which is the point: nothing edits the repo mid-filing. Inline grounding stays fine for a small single-area issue. Either way the gate is unchanged: a body that fails the cold-start test is unready, however it was researched. + **Cold-start test, apply it to your own draft before filing.** Reread the body as if you had never seen this conversation. Could you start work from it alone, without asking a single clarifying question and without first having to find where the relevant code lives? If not, it is not ready. Two concrete failure signs: the body names an area ("the router", "the prefetch logic") rather than a path, or it describes the symptom without saying where the fix goes. The user should never have to ask for this. If they do, the skill was not followed. diff --git a/.claude/skills/webjs-instagram-post/SKILL.md b/.claude/skills/webjs-instagram-post/SKILL.md index d83f697f2..712300287 100644 --- a/.claude/skills/webjs-instagram-post/SKILL.md +++ b/.claude/skills/webjs-instagram-post/SKILL.md @@ -111,10 +111,12 @@ only has to exist for that one call, then it is deleted. ```sh SLUG=works-without-javascript # match the post topic BR=chore/ig-social-$SLUG -git worktree add -b "$BR" ../webjs-ig-social origin/main -mkdir -p ../webjs-ig-social/website/public/social -cp "$OUT" ../webjs-ig-social/website/public/social/$SLUG.jpg -( cd ../webjs-ig-social +PRIMARY=$(git worktree list --porcelain | sed -n 's/^worktree //p' | head -1) # every path below roots here, so nothing nests under a task worktree or lands beside the session cwd (sed, not awk: awk splits a path containing spaces) +git -C "$PRIMARY" worktree add -b "$BR" ../webjs-ig-social origin/main +IG="$PRIMARY/../webjs-ig-social" +mkdir -p "$IG/website/public/social" +cp "$OUT" "$IG/website/public/social/$SLUG.jpg" +( cd "$IG" git add website/public/social/$SLUG.jpg git commit -q -m "chore: add $SLUG Instagram social card asset" git push -q -u origin "$BR" ) @@ -126,7 +128,12 @@ IMG="https://raw.githubusercontent.com/webjsdev/webjs/$BR/website/public/social/ After the post is published in Step 4, tear the branch down so nothing lingers: ```sh -git worktree remove ../webjs-ig-social --force +# Shell state does NOT survive across tool calls, and Step 4's user +# confirmation sits between setup and here, so re-derive BOTH variables +# (an empty $PRIMARY silently degrades `git -C ""` to cwd-relative). +PRIMARY=${PRIMARY:-$(git worktree list --porcelain | sed -n 's/^worktree //p' | head -1)} +BR=${BR:-chore/ig-social-} # fill the slug if the variable is gone +git -C "$PRIMARY" worktree remove ../webjs-ig-social --force git branch -D "$BR" git push origin --delete "$BR" ``` diff --git a/.claude/skills/webjs-research-record/SKILL.md b/.claude/skills/webjs-research-record/SKILL.md index 0af99580b..d7a784703 100644 --- a/.claude/skills/webjs-research-record/SKILL.md +++ b/.claude/skills/webjs-research-record/SKILL.md @@ -26,6 +26,10 @@ The framework's reference docs (the skill at `.agents/skills/webjs/`) exist ONLY A research record has **no code diff**. A PR is a "merge this diff" object, so a research PR needs an empty-commit hack and leaves a dangling branch behind (see the now-deleted branch from #559). An issue is the native home for a writeup with threaded discussion. It is filterable by the `research` label, and it fits the project model where the GitHub Project board backed by issues is the source of truth. Earlier records used closed PRs (#546, #553); the convention going forward is a labeled issue. Do not convert the old PRs unless asked. +## Investigate in parallel + +A comparison or options analysis is multi-angle work, and each angle is an independent read: one option's codebase evidence, the prior art, the project's own constraints in AGENTS.md, what the ecosystem does. For a multi-option or multi-angle investigation, spawn one read-only `Explore` agent per option or angle in ONE message, each returning evidence with concrete file paths and citations, then synthesize yourself. Reads fan out; the writeup and its judgment stay with you, because the record's voice and conclusions are the caller's, not an agent vote. A single-question spike needs no fan-out. Nothing here changes WHERE the record lands (the closed `research`-labeled issue below). + ## Lifecycle: backlog item then findings in the SAME issue Research often starts as a planned **backlog item**, an OPEN `research`-labeled issue ("research whether X") sitting in the board's Todo column. When an agent actually does the research, the findings go into **that same issue**, NOT a new PR: diff --git a/.claude/skills/webjs-scaffold-sync/SKILL.md b/.claude/skills/webjs-scaffold-sync/SKILL.md index 75ccbe39a..7f53642e8 100644 --- a/.claude/skills/webjs-scaffold-sync/SKILL.md +++ b/.claude/skills/webjs-scaffold-sync/SKILL.md @@ -178,6 +178,8 @@ it applies, then update or consciously skip each. ## Per-change sync procedure +Run the mandatory generate + boot + check verifications in PARALLEL when more than one template is affected: each verification is its own background Bash task, launched in one batch, with EVERY task's result collected before the sync is declared done (a forgotten background boot is a silently unverified template). Prefer the in-process harness (`createRequestHandler` + `Request` objects, the dogfood boot-check pattern in the start-work skill): it binds no port, so parallel runs cannot collide. When a real `webjs dev` listen is unavoidable, give each boot a distinct `--port`. + 1. Identify the change's IDENTIFYING TOKENS: the demo route (`app/features/`, `app/api/features/`), the template name, the generated file path, the convention phrase, or the scoping phrase (e.g. "full-stack only"). @@ -213,6 +215,8 @@ it applies, then update or consciously skip each. ## Audit-mode procedure (sweep the scaffold for drift) +Fan the sweep out like the doc-sync audit: one read-only `Explore` agent per surface of the map, spawned in one message; the agents report and the edits stay in the main session. + 1. List the shipped scaffold changes (new demos, new template, scoping changes, changed generated files). 2. For each, pull its tokens and run the surface grep above. diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 0fb19120d..bd3f78deb 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -63,22 +63,16 @@ The user's request typically names an issue by number (e.g. `#112`) or by descri If not present, add it: `gh project item-add 1 --owner webjsdev --url https://github.com/webjsdev/webjs/issues/`. -3. **Verify we're on `main` with a clean tree and pull latest.** If the working tree is dirty or already on a feature branch, STOP and ask the user how to proceed. Do not silently abandon their work. +3. **Fetch, and leave the primary checkout alone.** `git fetch origin`. The task's worktree cuts from `origin/main`, so a dirty or mid-something primary checkout neither blocks starting nor gets "fixed"; it is never edited at all (enforced by `.claude/hooks/require-worktree-for-edits.sh`, which blocks tracked-file edits in a primary checkout). - ```sh - git status --porcelain # must be empty - git branch --show-current # must be main - git pull origin main - ``` - -4. **Create the feature branch AND push it to origin immediately.** Pick the prefix from the issue labels: `enhancement` to `feat/`, `bug` to `fix/`, `documentation` to `docs/`, otherwise `chore/`. Build the slug from the issue title (lowercase, kebab-case, max 30 chars, drop conjunctions). The empty branch goes to GitHub right away so the work survives any local-machine failure even before the first commit. +4. **Create the task's WORKTREE and push its branch immediately.** One task, one worktree, ALWAYS; there is no lone-agent plain-branch path, because "no other agent is active" is unverifiable mid-task. Pick the prefix from the issue labels: `enhancement` to `feat/`, `bug` to `fix/`, `documentation` to `docs/`, otherwise `chore/`. Build the slug from the issue title (lowercase, kebab-case, max 30 chars, drop conjunctions). The push happens right away so the work survives any local-machine failure even before the first commit. ```sh - git checkout -b / - git push -u origin / + git worktree add -b / ../- origin/main + git -C ../- push -u origin / ``` - After this step, ALSO push after every subsequent commit (`git push` is cheap and is the safety net against losing work). Do not batch multiple commits before pushing. + ALL work for the task happens inside that worktree, by absolute path when the session's cwd resets. A fresh worktree has NO `node_modules`; see AGENTS.md for the symlink remedy (#954). Cleanup after merge is automatic (`cleanup-merged-worktree.sh`). After this step, ALSO push after every subsequent commit (`git push` is cheap and is the safety net against losing work). Do not batch multiple commits before pushing. 5. **Move the project card from Todo to In progress.** Resolve the four IDs and call `item-edit`: @@ -235,13 +229,13 @@ A finished PR is not just a diff. It carries four artifacts, and the PR is consi 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 self-review round 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, no machinery tells) and free of AGENTS.md invariant 11 banned glyphs. The sections below specify the mechanics for items 3 and 4. +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. **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). ## Pre-merge self-review loop (MUST run before reporting "ready for merge") -Saying "ready for merge" before the review loop 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 cycles before a review comes back clean. The cure is to run that loop internally BEFORE the first "ready" signal. The user should only hear "ready to merge" after the loop has converged. +Saying "ready for merge" before the review loop 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 cycles before a review comes back clean. The cure is to run that loop internally BEFORE the first "ready" signal. The user should only hear "ready to merge" after the loop has converged AND the suites the loop deferred have run AND CI has been read green. ### Every PR review is posted ON the PR (summary + per-line comments) @@ -296,7 +290,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 self-review loop, 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 finds nothing still posts a short summary review saying it is clean, with no inline comments. +**Every round repeats the whole flow.** Each round of the self-review loop, 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 step 1) 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. @@ -310,43 +304,71 @@ Beyond review findings, proactively record the *reasoning* behind a PR as commen **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 loop 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. Run rounds of self-review until ONE round finds zero new issues. Each round must: +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. Run rounds of self-review until ONE round finds zero new issues. + +**Keep the loop fast. A round is a reading pass over the diff and should cost seconds, so nothing slow belongs between rounds.** Two rules, and both change WHEN work happens, never WHETHER: + +- **No full test suite inside the loop.** After a fix, run only the specific test file(s) covering the line you just changed (`node --test `), which is also what the commit rule needs (a logical unit commits when its own tests pass). Do NOT run the e2e suite, the full Node suite, the browser suite, the Bun matrix, or the four-app dogfood boot check between rounds. Those run ONCE, after the last clean round, and the Definition of done still requires every layer the change touches. This is a timing rule, not permission to skip a layer. +- **Never wait on CI between rounds.** Pushing is fire-and-forget: push, then start the next round immediately. No `gh pr checks --watch` mid-loop. Read CI once, after the last clean round. A round that finds something means another push anyway, which supersedes the run you would have been watching, and merge is gated on green CI regardless, so waiting mid-loop buys nothing. + +**Shape the rounds for wall clock: deep-review first, then delta-scoped verification.** Sequential broad rounds re-AUDIT the same surface N times; this shape audits it once, in parallel, then each later round asks only about what changed (while still holding the full changed files as evidence). -1. **Spawn a fresh general-purpose subagent** (use the Agent tool with `subagent_type: "general-purpose"`). A fresh subagent has no prior context on the decisions you made, so it's less likely to share your blind spots. +- **Round 1 is the committed deep-review workflow, for EVERY task that enters the loop** (`.claude/workflows/deep-review.js`, run via the Workflow tool as `{ name: "deep-review", args: "" }`). What it does: a scout reads the diff and proposes up to six dynamic lenses tailored to the PR's nature, which run in parallel alongside six fixed ones (correctness, security, blast radius, test adequacy, invariants and doc drift, fresh eyes; at most THREE reviewers per run are pinned to fable and the rest to opus, so two model families always read the diff at a fixed fable cost), then an adversarial jury refutes each finding. The whole run is HARD-CAPPED at 24 agents by default (`{ pr, maxAgents }` overrides, clamped 8 to 60): dynamic lenses trim first, jury sizes shrink next, and a finding the budget cannot verify comes back in an `unverified` list, never silently dropped, so raise the cap or re-run when that list is non-empty. Its CONFIRMED findings are the round's findings, the ones you act on; the jury-REJECTED ones come back too, carrying their refuters' reasons, but only for the audit trail (post them with the round per step 2's record rule; they are already adjudicated, need no action, and do not by themselves force another round). Uniform on purpose: its cost self-scales (juries convene per finding, so a quiet PR is cheap), and a uniform rule cannot be misjudged the way a by-risk bifurcation can, which matters because a `packages/*/src` bug ships into every end-user app. The whole run counts as ONE round, clean only when it confirms nothing, and its confirmed findings feed this same loop (fix, post as one review object, delta round). A deep-review run that dies or errors is a failed spawn under the liveness rules below: re-run it, and never substitute an inline pass. +- **Every later round is delta-scoped: ONE reviewer whose QUESTION is what the last round changed** (or, when a round produced NO fix commits, the riskiest section of the original diff). Delta-scoped narrows the question, never the evidence: the reviewer still receives the full changed files and MUST trace the fix's blast radius, grepping every symbol, rule, or concept the fix touches across the whole PR surface and comparing each other occurrence, since a small fix can break something far from its own hunk. What it does not do is re-audit the unchanged surface from scratch, which is the all-pairs reasoning that makes broad rounds slow. If it finds something, fix and repeat the delta shape. Re-run deep-review in full, not a delta, when the surface round 1 audited is no longer the surface under review: a fix rewrote something well outside the original diff (for shipped source, a fix spanning files is reason enough), OR the PR's SCOPE changed after round 1, new work folded in, a user-directed pivot, an issue absorbed mid-review. A delta round verifies fixes to an audited surface; it cannot stand in for the audit of a surface round 1 never saw. +- **Starve reviewers of context they do not need.** Each gets the PR diff, the PR title and body (the author's claims, which the review checks against), the touched files, and the rule files it judges against (`AGENTS.md`, `CONVENTIONS.md`), nothing more. NEVER feed them prior PR comments or reviews (measured on #1159: the comment payload alone reached 171 KB by the fifth round and dominated its cost), and never a growing already-handled exclusion list. Dedupe repeat findings yourself: a duplicate costs you a second of reading, while the exclusion list costs every reviewer minutes of ingestion. - **Working-tree safety (non-negotiable).** Review subagents share THIS session's working directory. A reviewer that runs `git checkout` / `switch` / `reset` / `stash` / `rebase` silently moves the main session's HEAD off the branch (it has happened: a reviewer ran `git checkout main` mid-loop and the local checkout regressed; commits were safe only because they were already pushed). Two defenses, apply BOTH: - - Spawn the reviewer with `isolation: "worktree"` so any git op it runs is contained in its own throwaway worktree, never the main checkout. (A read-only reviewer needs no shared state; `gh pr diff` reads from GitHub.) - - Include the read-only git prohibition in the prompt (it is baked into the template below). Belt and suspenders: isolation contains the damage, the prompt prevents the attempt. - - After EACH round returns, before acting on findings, run a one-line repo-health check and repair if needed (the worktree isolation in the first bullet has itself corrupted the main repo: spawning isolated reviewers flipped the main repo's `core.bare` to `true` and left a locked `.claude/worktrees/agent-*` worktree, after which `git checkout` failed with "this operation must be run in a work tree"). Check ALL of: +Each round must: + +1. **Run the round's reviewer: the round-1 deep-review workflow, or the single delta verifier** (the delta verifier via the Agent tool with `subagent_type: "general-purpose"` and `run_in_background: true`; round 1 via the Workflow tool, which also runs in the background and reports through its completion notification). A fresh reviewer has no prior context on the decisions you made, so it's less likely to share your blind spots. That is also why reviewers are ONE-SHOT, never persistent teammates: a teammate that keeps context across rounds re-derives its own prior conclusions, which is the blind-spot sharing this rule exists to kill. Teammates fit collaborating work; reviewing is the one job where accumulated context is the failure mode. **Async plus a watchdog, never an UNBOUNDED wait in either direction.** A synchronous spawn blocks the whole turn, so a hung reviewer is indistinguishable from a working one for as long as the harness takes to give up (a delta round that should cost 1 to 5 minutes once ate 20 this way, unwatchable from inside the blocked call). So spawn the reviewer in the background AND, immediately, start a background watchdog on the task's output file. Take the path from the spawn's own tool result or task notification, NEVER a guess (a watchdog once aimed at the newest file in the tasks directory and watched its own output). The watchdog has TWO MODES, decided by what that file actually is: + - **Transcript mode**, when the file demonstrably grows: probe its mtime every 30 seconds and fire on about 2 minutes of stall or 10 minutes total. On a fire, peek at harness evidence only (the task's status, whether the file is still growing; the words in any `tail` are NEVER read as a prose claim of liveness), stop a stalled reviewer (`TaskStop`) and re-spawn it per the re-spawn bullet, leave a progressing one alone. + - **Stub mode**, when the file is a static stub whose mtime never moves, which is the PERMANENT HEALTHY state of every worktree-isolated reviewer, and therefore the mode for every reviewer you spawn directly (two healthy isolated ones were once killed on exactly that misread): there is NO mid-flight activity signal, so the watchdog is the 10-minute hard timer alone, the harness's completion or killed notification is the only earlier news, and the wait is blind but BOUNDED, which is the point. Kill only on the hard cap or a killed/errored task status, nothing softer. + When the reviewer completes first, stop the watchdog. (Backgrounding does not protect you from a declined spawn: a refusal lands on the tool call itself, and a Workflow call can be declined the same way.) + + **Reviewer liveness (non-negotiable).** The harness status is the ONLY liveness signal. For BOTH reviewer kinds, an Agent spawn and a Workflow run alike, the completion signal is the harness notification and the result it carries, and the mid-flight signal is the watchdog in whichever mode the task's output file supports, an activity probe on a real transcript or the bare hard timer on a stub (`TaskStop` works on either task id), so neither kind is ever an UNBOUNDED wait. A run whose harness status is killed or errored is a dead round exactly like a dead spawn. Transcript-file activity (mtime, growth) is harness evidence where the file is a real transcript, and no signal at all where it is a static stub; the WORDS in a transcript are an agent's own prose and count for nothing in either mode. A spawn that was declined at the permission prompt, returned an internal error, or came back with an empty result means NOTHING is running and no review happened. Never read an agent's own prose as evidence it is alive: a background reviewer once signed off with "Yes, it's running. I have the diff and I'm partway through the careful pass" while its harness status read `killed`, and taking that at face value is exactly what turns a dead round into an unbounded wait. If a background reviewer is in play anyway (a `/code-review` agent, or a named agent from an earlier turn), its status is the harness completion notification for that task, which arrives on its own and carries the agent's real result, never what the agent last said, and a `killed` or errored status is terminal. Do not poll it, do not wait on it, do not re-read its message hoping for a different answer. `TaskList` is the shared to-do list, NOT a registry of running subagents, so an empty `TaskList` is evidence of nothing here. A dead STRAY background agent (a `/code-review` agent, a named agent from an earlier turn) is not by itself a blocked loop: it was never the round's reviewer, so write it off and run the round's real reviewer, the deep-review workflow for round 1 or the delta verifier after it. Only the round's own reviewer being unproducible blocks the loop. The rules below follow: recognise the failure, re-spawn it per the re-spawn bullet, and only a reviewer that cannot be produced at all blocks the loop. + - **A failed spawn means the round did not happen.** Do not count it as a round, do not report it as a round, and do not let it advance the loop toward "last round clean". A failed spawn is not something to stop and report on its own, though: re-spawn it per the re-spawn bullet. Do still run the repo-health check below: a spawn that dies after its isolated worktree was created is the likeliest leaker of all, so that check keys off the SPAWN, not off a round completing. + - **A reviewer that returns without reviewing is also a failed round.** A subagent can come back successfully and still have reviewed nothing ("I could not fetch the diff", a refusal, an answer to some other question, a summary of the PR body with no reading of the code). The prompt tells it to report `BLOCKED` plus what it is missing when it CANNOT review, which covers the could-not-fetch case; a refusal or an off-topic answer will not carry the sentinel, so do not rely on it alone: any answer that is not the reviewer's expected result shape is the same state, and the absence of findings is NOT a clean round. The expected shape is per reviewer kind: the delta verifier returns a finding list or the literal `CLEAN`; a deep-review run returns its structured result with `confirmed` and `rejected` lists, and anything else from it (prose, a partial result, nothing) means the round did not happen. Treat it exactly like a failed spawn: no round happened, so re-spawn per the re-spawn bullet, feeding it whatever the reviewer said it was missing (the diff, a path, the base branch). + - **NEVER downgrade to an inline self-review.** The fresh subagent is required, not preferred, because reviewing your own work re-derives the same assumptions that produced the bug. That downgrade has already shipped three real bugs into a PR that two inline rounds had called clean (an unguarded `.action=${fn}` property path, a test-less branch the suite would not have missed, and a factually wrong commit message), every one of them caught by the first genuine subagent round that followed. An inline pass is strictly worse than no review, because it looks like one. + - **Re-spawn it. Do not interrupt the user over a failed spawn.** A spawn that broke mechanically (an internal error, a timeout, a returned-without-reviewing result) is harness noise, so just spawn it again, adjusting the prompt if the reviewer said what it was missing. Keep re-spawning until one takes. A handful of consecutive identical failures means something structural is wrong, so vary the approach (a smaller scope, a different focus, a fresh prompt) rather than firing the same call a tenth time. Recovering costs a few seconds; asking the user costs the whole loop its momentum. **Never** stop mid-loop to report a failed spawn, ask how to proceed, or hand back a half-finished loop. The user asked for a converged review, not a status update, and the loop is not done until a round comes back clean. + - **A genuinely unrecoverable reviewer is the only stopping point, and even that is reported at the end.** If re-spawning cannot produce a working reviewer at all, the loop has not converged, so withhold the flip to ready for review and say plainly what happened, once, when you report back. Keep it in the conversation: a spawn that could not run is session tooling, not a fact about the change, so it does NOT go into the PR body or a PR comment (the PR records the review, not the harness). What you must never do is substitute an inline pass to keep the loop moving, which is the failure this whole block exists to prevent. + + **Working-tree safety (non-negotiable).** A review agent runs against the repository THIS session is using, and every worktree of a repo shares ONE `.git` directory, so its git writes reach the main checkout even from an isolated worktree. A reviewer that runs `git checkout` / `switch` / `reset` / `stash` / `rebase` silently moves the main session's HEAD off the branch (it has happened: a reviewer ran `git checkout main` mid-loop and the local checkout regressed; commits were safe only because they were already pushed). Two defenses: + - For every reviewer YOU spawn directly (the delta verifier, a refuter): `isolation: "worktree"`, so its WORKING TREE is its own throwaway checkout and a stray `git checkout` cannot move the files under this session. It does NOT wall off the repo: the worktree shares this repo's `.git`, so ref and config writes still land on the shared repository. (A read-only reviewer needs no shared state; `gh pr diff` reads from GitHub.) + - For ALL reviewers, the read-only git prohibition in the prompt (baked into the delta template below and into deep-review's own agent preambles; the workflow's internal agents run on this defense alone, which is why the check below matters after its runs too). Belt and suspenders where both apply: isolation contains the working-tree half of the damage, the prompt prevents the attempt at the shared half that isolation cannot reach. + - After EACH spawn or workflow run resolves, before acting on findings, run a one-line repo-health check. Every one, not every round: a spawn that was declined, errored, or died did not produce a round but may well have created a worktree first, so it needs the check MORE than a clean return does. Run it and repair if needed (the worktree isolation in the first bullet has itself corrupted the main repo: spawning isolated reviewers flipped the main repo's `core.bare` to `true` and left a locked `.claude/worktrees/agent-*` worktree, after which `git checkout` failed with "this operation must be run in a work tree"). Check ALL of: - `git rev-parse --is-inside-work-tree` is `true` and `git config --get core.bare` is NOT `true`. If the work tree is broken, repair with `git config core.bare false`, then `git worktree prune` (and `git worktree remove -f -f .claude/worktrees/agent-*` for any locked leftover). Do NOT touch the user's OWN unrelated worktrees (anything outside `.claude/worktrees/`). - - `git rev-parse --abbrev-ref HEAD` is still the feature branch and `git status` is clean. If HEAD moved (or points at a now-deleted branch / `0000000`), `git checkout -f ` (or `main` post-merge) restores it. + - In the TASK's worktree, `git rev-parse --abbrev-ref HEAD` is still the feature branch and `git status` is clean (`git -C ...`; the worktree itself holds the branch, so a checkout there cannot collide). In the PRIMARY checkout the healthy HEAD is `main`; a primary sitting on anything else means something mutated it, so put it back with `git checkout -f main`. Never try `git checkout -f ` in the primary: the task worktree holds that branch and git refuses. The merge / commits are always safe on GitHub regardless; this only repairs the LOCAL repo. Nothing is lost. If you cannot cleanly repair, `git checkout -f main && git pull` to resync, then continue. - Pass a prompt that: + For the DELTA verifier (round 1's prompts live inside the deep-review workflow), pass a prompt that: - Names the PR number and branch. - - Tells it to fetch the diff (`gh pr diff --repo webjsdev/webjs`) and read the changed files in context. + - Tells it to fetch the diff (`gh pr diff --repo webjsdev/webjs`) and read the changed files in context. Those plus the PR title and body (`gh pr view --repo webjsdev/webjs --json title,body`) and the rule files, never prior PR comments or reviews, per the starve rule above. A delta round keeps the same evidence but makes the fix commits' diff its question, with a blast-radius instruction per the round-shape rules; a delta pass following a round with no fix commits takes the riskiest section of the original diff as its question instead. - Instructs it to look for: bugs, regressions, security issues, missed edge cases, broken invariants, doc drift, test gaps, stylistic problems against `AGENTS.md` and `CONVENTIONS.md` (root and per-package). - Asks for a numbered list with `file:line` references. Problems only, no suggestions. - - Ends with: "If you find nothing genuinely wrong, say exactly `CLEAN` and stop. Do not pad." + - Contains, verbatim: "If you find nothing genuinely wrong, say exactly `CLEAN` on its own line and stop." plus a no-padding instruction. Keep `CLEAN` and its "on its own line" exactly as written; the loop tests for that literal sentinel, so a paraphrase in the prompt makes a clean round unrecognisable. + - Tells it to say `BLOCKED` plus what it is missing if it CANNOT review (no diff, no access, wrong repo), instead of falling through to `CLEAN`. Without this the two sanctioned outputs are a finding list or `CLEAN`, so a reviewer that never saw the diff has found nothing genuinely wrong and reports itself clean, which is precisely the non-review the liveness rules must catch. -2. **For each finding the subagent reports**, do exactly ONE of these three. There is no fourth option, and "mention it and move on" is not allowed: +2. **For each finding the subagent reports**, do exactly ONE of these three. There is no fourth option, and "mention it and move on" is not allowed. One gate first, for a DELTA round's findings on shipped source (round-1 findings skip it, since deep-review's juries already refuted them): a finding whose fix would be expensive or behavior-changing gets an adversarial REFUTER before you act, a fresh subagent handed the same evidence and told to DISPROVE the claim (does the scenario reproduce in the code as written, is the behavior actually intended, is it already guarded or tested somewhere the finder did not look). A finding the refuter kills is a rejection with the refuter's reason; the rest proceed. The refuter is a reviewer spawn like any other: step 1's working-tree-safety rules (`isolation: "worktree"`, read-only git, the per-spawn repo-health check) and liveness rules (re-spawn a failed one) apply to it in full, and if a refuter cannot be produced, act on the finding ungated, since the gate is an optimization and must fail toward treating the finding as real. This costs one cheap spawn and saves the expensive failure, a confident false positive that buys a wrong "fix" plus the delta rounds to unwind it. Trivial or obviously-real findings skip the gate. - **Fix it** on the branch (commit + push to update the PR), OR - - **Reject it** explicitly with a one-sentence reason written in your reply to the user and in the PR body. Rejection has to be defensible (e.g. "the agent flagged X as a security issue but X runs server-side only and never reaches user input"). False positives are real; reject them on the merits, don't just hand-wave. OR + - **Reject it** explicitly with a one-sentence reason, stated in your reply to the user and recorded on the finding's thread (`rejected because `, per the posting rules; not in the PR body). Rejection has to be defensible (e.g. "the agent flagged X as a security issue but X runs server-side only and never reaches user input"). False positives are real; reject them on the merits, don't just hand-wave. OR - **File it** as a tracked issue when it is genuine but out of scope for THIS PR (a pre-existing bug, an unrelated dependency/hygiene problem, a separate feature). "Out of scope" is NOT a reason to drop a finding. INVOKE the `webjs-file-issue` skill rather than hand-rolling `gh issue create`: it gates on grounding the body in the real codebase first, which is exactly what an out-of-scope review finding needs and exactly what a hurried two-command version omits. Capture the issue number. Verify it landed (`gh project item-list 1 --owner webjsdev --limit 20000`, the board is past 200 items so the default cap of 30 reports a false negative). A finding you call out-of-scope without an issue number is an unfiled finding, which is the exact mistake this clause exists to prevent. If unsure whether something is in-scope, default to fixing it in this PR; only file-and-defer when it is clearly separable and fixing it here would mean scope creep. - **Record every genuine finding as a comment ON THE PR, the way a human reviewer would.** The self-review trail must live on the PR, not only in your reply to the user (a finding that exists only in the chat transcript is invisible to anyone reading the PR later). For findings tied to specific lines, post an INLINE review comment at `file:line` (`gh api repos/webjsdev/webjs/pulls//comments --input comment.json`, writing the body into that JSON file first, or a `gh pr review` with line comments). Build that JSON with a real serializer, never by interpolating into a shell string: review text is dense with backticks, and an unquoted shell string runs them as command substitution. That is exactly how a review on #1115 lost every code reference in its inline comments and had to be deleted and reposted. For cross-cutting or round-summary notes, use `gh pr comment --body-file /tmp/pr-comment.md`. Each comment states the finding, its `file:line`, and its disposition (`fixed in ` / `rejected because ` / `filed as #`). Post rejected findings and false positives too, so the reasoning is auditable on the PR. A `CLEAN` round can be noted briefly. Do this as part of the loop, not as an afterthought. (Reminder: em-dashes and the other banned glyphs in AGENTS.md invariant 11 apply to PR comment bodies too, since they go through the same tooling.) + **Record every genuine finding ON THE PR, exactly the way a person reviewing on GitHub would.** The review trail must live on the PR, not only in your reply to the user, since a finding that exists only in the chat transcript is invisible to anyone reading the PR later. **Post it through the mechanics in `### Every PR review is posted ON the PR` and `### Follow the real review flow`, which are authoritative; do not improvise a different shape here.** In short: the round's summary and all its inline `file:line` findings go up as ONE review object, each finding states the problem only, and the disposition (`fixed in ` / `rejected because ` / `filed as #`) goes in a threaded reply, after which the thread is resolved. That is what a real reviewer does on GitHub, and it is the only shape that renders as a grouped review rather than scattered boxes. -3. **If the round found any findings (even rejected ones)**, run another round with a fresh subagent. The new round picks a slightly different focus prompt: if round 1 was broad, round 2 zooms in on the file you most edited; if round 2 zoomed in, round 3 zooms out to cross-file consistency, etc. Rotate focus to avoid the agent rediscovering the same surface. + Post rejected findings and false positives too, so the reasoning is auditable on the PR. A round that reviewed and found nothing posts a short summary review saying so. Do this as part of the loop, not as an afterthought. Build the review JSON with a real serializer, never by interpolating into a shell string: review text is dense with backticks, and an unquoted shell string runs them as command substitution. That is exactly how a review on #1115 lost every code reference and had to be deleted and reposted. (Reminder: em-dashes and the other banned glyphs in AGENTS.md invariant 11 apply to every comment body, since they go through the same tooling.) -4. **If the round reports `CLEAN`**, the loop is done. +3. **If the round produced any actionable findings (a deep-review run's CONFIRMED list, or anything a delta verifier reports, including findings YOU then reject, since your own rejection is unadjudicated judgment in a way a jury's is not)**, resolve them and run a delta-scoped round: ONE fresh reviewer whose question is the fix commits' diff, with the full changed files as evidence and the blast-radius grep the round-shape rules mandate (when the round produced no fix commits, the question is the riskiest section instead). Fresh matters: on #1159, three consecutive rounds each found problems INTRODUCED by the previous round's fix, which is exactly what a re-used reviewer, already invested in that fix, is worst at seeing. If the delta round finds something, fix and repeat the delta shape; deep-review re-runs only when the round-shape rules call for it (a fix well outside the original diff, or a scope change). -The minimum is TWO rounds. A clean first round is rare and usually means the review was too shallow; if round 1 is clean, spawn a second one with a sharper, narrower focus before believing the result. +4. **If the round reports `CLEAN`** (a deep-review run is clean only when it confirms nothing), the loop is done, with one exception: a clean round 1 still gets the delta-shaped pass the minimum-two-rounds rule requires. Once done, NOW run everything the loop deferred: the full suites for every layer the change touches (e2e, Node, browser, Bun matrix, the four-app dogfood boot check, per the Definition of done), and only now read CI. Run them in PARALLEL: launch each suite as its own background Bash task in one batch, plus the CI read as a background watch (an `until` loop that exits when no check is pending), then collect EVERY launched task's result before reporting anything. The set is independent work, so its wall clock is the slowest suite rather than the sum, and a background task you forget to collect is a silently skipped layer, so the report waits for all of them. This changes only the shape; the WHEN-not-WHETHER rule above still decides what runs. -**The LAST round must be clean, always. A fix is never the end of the loop.** The moment a round surfaces a genuine finding and you fix it (or reject it), you have NOT finished: the fix changed the branch, so it needs its own round. Run another round on the new HEAD and keep going until one round finds zero issues. Do NOT report "fixed it" or "ready to merge" after a round that found something, even if the fix is obviously correct and tests pass. The recurring failure this prevents: review finds X, you fix X and immediately report done, having never re-reviewed the fix. If you ever notice you are about to report a result where the most recent review round found a finding, stop and run another round first. +The minimum is TWO rounds: deep-review, then at least one delta verify. A clean deep-review is rare and usually means the lenses were too shallow for this PR; before believing it, run one delta-shaped pass with the riskiest section as its question anyway. -**A standalone "review the PR" request IS the loop, not a one-shot.** When the user asks you to review an existing PR (separately from the post-`gh pr create` flow), you re-enter this exact loop: spawn a fresh round, and if it finds anything, fix/reject/file it and run further rounds until the last one is clean, BEFORE you report back. "I reviewed it and fixed one thing" is not an acceptable stopping point; "I reviewed it in a loop and the last round was clean" is. The user should never have to ask "was the last review clean?" because you should not have stopped until it was. +**The LAST round must be clean, always. A fix is never the end of the loop.** The moment a round surfaces a genuine finding and you fix it (or reject it), you have NOT finished: the fix changed the branch, so it needs its own round. Run another round on the new HEAD and keep going until one round finds zero issues. Do NOT report "fixed it" or "ready to merge" after a round that found something, even if the fix is obviously correct and tests pass. The recurring failure this prevents: review finds X, you fix X and immediately report done, having never re-reviewed the fix. If you ever notice you are about to report a result where the most recent review round found a finding, stop and run another round first. The ONE case that ends the loop without a clean round is a reviewer that cannot be produced at all, meaning it cannot be spawned or never returns an actual review despite the re-spawns (see the liveness rules in step 1), and that ends it as BLOCKED, never as done. + +**A standalone "review the PR" request IS the loop, not a one-shot, and it ALWAYS starts with a fresh deep-review run.** When the user asks you to review an existing PR (separately from the post-`gh pr create` flow), you re-enter this exact loop from round 1: a full deep-review, regardless of how many review cycles the PR has already been through, because the ask itself says the existing trail is not trusted or not current. An explicit ask also overrides the trivial-change skip below: when the user asks for a review, they get one, however small the diff. Then, if it finds anything, fix/reject/file it and run delta rounds until the last one is clean, BEFORE you report back. "I reviewed it and fixed one thing" is not an acceptable stopping point; "I reviewed it in a loop and the last round was clean" is. The user should never have to ask "was the last review clean?" because you should not have stopped until it was. Arriving here skips step 1's preamble, so read it before spawning, BOTH halves. The working-tree-safety block still applies (worktree isolation for reviewers you spawn directly, the read-only git prohibition for every reviewer including deep-review's internal agents, and the repo-health check after each spawn or workflow run, including failed ones), and so do the liveness rules: the sole reason to stop short of clean is a reviewer that cannot be produced, which you report as blocked and never paper over with an inline pass of your own. ### When to skip the loop @@ -354,10 +376,12 @@ Skip only for PRs that change a single line of trivially-correct content (a doc ### Reporting after the loop -After the loop converges, report exactly this shape to the user: +After the loop converges and the deferred suites plus the CI read (step 4) are done, report exactly this shape to the user: > PR # is up at . Self-review loop ran rounds; last round clean. Issues found and fixed during the loop: . Out-of-scope findings filed as follow-ups: . Ready to merge. +**Only a round in which fresh subagents actually REVIEWED counts toward ``, and a deep-review run, all its lenses and juries together, is ONE round.** A spawn that was declined, errored, was killed, or returned nothing did not produce a round, so it can be neither the clean last round nor part of the total, and an inline pass you did yourself is not a round at all (see the liveness rules in step 1). Neither does a spawn that returned a result without reviewing: a completed call and a real subagent result are NOT the test, because a reviewer that says "I could not fetch the diff" satisfies both and reviewed nothing. The test is that it came back in its expected result shape: findings or the literal `CLEAN` from a delta verifier, the structured `confirmed`/`rejected` result from a deep-review run. A loop whose reviewer never reviewed has run ZERO rounds, and the honest report there is that the review is blocked and why, not a count and not "ready to merge". + If you cannot honestly say "last round clean", you cannot say "ready to merge". If a finding is rejected as a false positive, mention it in the report so the user can second-guess the rejection. Every finding the loop surfaced must be accounted for in this report as fixed, rejected-with-reason, or filed-with-issue-number; if you reported an out-of-scope finding to the user but cannot point to its issue number, you have not finished the loop. Every genuine finding must ALSO appear as a comment on the PR (see step 2), so the report and the PR agree. **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. @@ -367,9 +391,9 @@ If you cannot honestly say "last round clean", you cannot say "ready to merge". ### Subagent prompt template ``` -Review PR # at https://github.com/webjsdev/webjs/pull/ for bugs, regressions, security issues, missed edge cases, broken invariants, doc drift, test gaps, and style violations against the project's AGENTS.md and CONVENTIONS.md (root + per-package). +Review PR # (branch ``) at https://github.com/webjsdev/webjs/pull/ for bugs, regressions, security issues, missed edge cases, broken invariants, doc drift, test gaps, and style violations against the project's AGENTS.md and CONVENTIONS.md (root + per-package). -HARD CONSTRAINT, read first: you are running in a SHARED working directory that the main session is actively using. 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). You do NOT need to switch branches to review: the branch is already checked out, so read files in place; use `gh pr diff ` and `gh pr view ` (which read from GitHub, not the local tree) for the diff and metadata. 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. +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 pr diff ` and `gh pr view ` for the diff and 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: @@ -377,9 +401,11 @@ You start with no prior context on this PR. Steps: 2. Run `gh pr view --repo webjsdev/webjs --json 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. Specifically check: . +5. Specifically check: . 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. + +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 loop on a review that never happened. ``` ## After a merge: decide on a version bump, automatically @@ -432,7 +458,8 @@ A merge updates `main` and npm, but the four in-repo apps deployed to Railway (` ## Failure handling -- If `git status` is dirty: stop and ask the user to stash, commit, or abandon their pending work before creating the new branch. Never silently lose changes. +- 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 local checkout regressed mid-loop (HEAD on `main` or a base commit, the feature branch's work seemingly "gone"): a review subagent mutated shared git state. 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 checkout `; 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 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 round's reviewer cannot be produced (a deep-review run or a delta spawn that is declined at the permission prompt, errors, dies, hangs past its watchdog, or returns without its expected result shape): the round did not happen. Re-run it (re-invoke the workflow, re-spawn the verifier), varying the approach after a few identical failures, and do NOT stop mid-loop to report the failure or ask how to proceed: recovering costs seconds and interrupting costs the loop its momentum. Only a reviewer that cannot be produced at all blocks the loop, which is its one exit that is not a clean round; 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 step 1 of the self-review loop. - 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/.claude/workflows/deep-review.js b/.claude/workflows/deep-review.js new file mode 100644 index 000000000..6243d4a49 --- /dev/null +++ b/.claude/workflows/deep-review.js @@ -0,0 +1,232 @@ +export const meta = { + name: 'deep-review', + description: 'Deep multi-agent PR review: parallel finder lenses, then adversarial verification; confirmed findings are actionable, jury-rejected ones return for the audit trail', + whenToUse: 'The round-1 review for any PR entering a review loop, or a heavyweight pass on demand. Works on any repo. Pass the PR number as args, "owner/repo#123" for another repository, or { pr, repo, lenses, maxAgents } where explicit lenses skip the scout and maxAgents caps the whole run (default 24).', + phases: [ + { title: 'Scope', detail: 'a scout reads the diff and proposes dynamic lenses for this PR' }, + { title: 'Find', detail: 'six fixed lenses plus up to six dynamic ones, in parallel, at most three pinned to fable and the rest opus' }, + { title: 'Verify', detail: 'adversarial refuters per finding, majority rules' }, + ], +} + +// args: a PR number ("123" or 123), an "owner/repo#123" string, or { pr, repo, lenses, maxAgents }. +// When no repo is given, every agent detects it from its own cwd via gh. +// lenses: optional [{ key, prompt }] of extra lenses; when given, the scout is +// skipped and these run as the dynamic set instead. +let pr = null +let repo = null +if (typeof args === 'number') pr = String(args) +else if (typeof args === 'string' && args.trim()) { + const s = args.trim() + if (s.includes('#')) { const [r, n] = s.split('#'); repo = r.trim(); pr = n.trim() } + else pr = s +} else if (args && typeof args === 'object' && args.pr) { + pr = String(args.pr) + repo = args.repo ? String(args.repo) : null +} +const givenLenses = (args && typeof args === 'object' && Array.isArray(args.lenses)) ? args.lenses : null +// Hard agent budget for the WHOLE run (scout + finders + jurors). Degrades +// gracefully: dynamic lenses are trimmed first, jury sizes shrink next, and a +// finding the budget cannot verify is returned marked unverified, never +// silently dropped. +const rawMax = (args && typeof args === 'object' && Number.isFinite(Number(args.maxAgents))) ? Number(args.maxAgents) : 24 +const MAX_AGENTS = Math.min(60, Math.max(8, Math.floor(rawMax))) +if (!pr) throw new Error('Pass the PR number as args, e.g. Workflow({ name: "deep-review", args: "123" }), or "owner/repo#123", or { pr, repo, lenses, maxAgents }') + +const REPO_FLAG = repo ? ` --repo ${repo}` : '' +const REPO_NOTE = repo + ? `The repository is ${repo}. Pass --repo ${repo} to gh pr commands; gh api has no --repo flag, so there the repo rides in the URL path (repos/${repo}/...).` + : 'Detect the repository once with `gh repo view --json nameWithOwner -q .nameWithOwner` from your working directory; pass it as --repo to gh pr commands and put it in the URL path of gh api calls (gh api has no --repo flag).' +const REPO_PATH = repo || '/' + +const FINDINGS = { + type: 'object', + required: ['findings'], + properties: { + findings: { + type: 'array', + items: { + type: 'object', + required: ['file', 'line', 'title', 'detail', 'severity'], + properties: { + file: { type: 'string', description: 'repo-relative path' }, + line: { type: 'number', description: '1-indexed line at the PR head' }, + title: { type: 'string', description: 'one-sentence statement of the defect' }, + detail: { type: 'string', description: 'concrete failure scenario: inputs/state leading to wrong behavior' }, + severity: { type: 'string', enum: ['critical', 'major', 'minor'] }, + }, + }, + }, + }, +} + +const VERDICT = { + type: 'object', + required: ['refuted', 'reason'], + properties: { + refuted: { type: 'boolean' }, + reason: { type: 'string', description: 'one or two sentences: why the finding is wrong, or why it survives' }, + }, +} + +// Shared preamble for every agent. Git worktrees of one repo share a single +// .git directory, so a git write from any agent can reach the primary checkout. +const SAFETY = `HARD CONSTRAINT: you may be running against a repository another session is actively using, and git worktrees share ONE .git directory. You are READ-ONLY on git: never run checkout, switch, reset, restore, stash, pull, ref-moving fetch, merge, rebase, clean, branch -f, or worktree. Read-only inspection only (log, show, diff without changing state, status, blame). + +${REPO_NOTE} + +Fetch your evidence from GitHub so it works regardless of the local checkout: +- Diff: gh pr diff ${pr}${REPO_FLAG} +- Claims: gh pr view ${pr}${REPO_FLAG} --json title,body +- Head branch name: gh pr view ${pr}${REPO_FLAG} --json headRefName +- Any file at head: gh api "repos/${REPO_PATH}/contents/?ref=" --jq .content | base64 -d +Do NOT fetch PR comments or prior reviews. If the repo carries contributor rules (AGENTS.md, CONTRIBUTING.md, CONVENTIONS.md, or similar at the root or per-package), read the relevant ones and judge against them.` + +const LENSES = [ + { key: 'correctness', prompt: 'Correctness of the change itself: logic errors, inverted conditions, off-by-ones, broken control flow, wrong API usage, error paths that swallow or misreport. Trace each changed function end to end.', model: 'opus' }, + { key: 'security', prompt: 'Security: injection, authz/authn gaps, CSRF surface changes, secrets or server-only code reaching the client, open redirects, unsafe deserialization, trust-boundary violations. Weight anything on an authentication, serialization, or request-dispatch path most heavily.', model: 'fable' }, + { key: 'blast-radius', prompt: 'Ripple effects: for every symbol, export, config key, or rule the diff touches, grep the WHOLE repo at head for its other users and check each still holds. A small change that breaks a distant caller is your only quarry.', model: 'opus' }, + { key: 'tests', prompt: 'Test adequacy: would the PR\'s tests FAIL if each functional change were reverted (counterfactual)? Name any changed behavior with no failing-test proof, any test asserting the mock rather than the behavior, and any test layer the repo\'s contributor rules demand that this change touches but does not cover.', model: 'opus' }, + { key: 'invariants-docs', prompt: 'Invariants and doc drift: check the diff against every invariant and convention the repo\'s contributor rules state, and check every doc surface that describes the changed behavior still tells the truth at head.', model: 'fable' }, + { key: 'fresh-eyes', prompt: 'Broad second-opinion pass: read the diff cold and report anything genuinely wrong, with no assigned angle. Prefer depth on the riskiest hunk over breadth.', model: 'fable' }, +] +// Every fixed lens pins its model: half opus, half fable. Two model families +// reading the same diff have different blind spots, and pinning makes the +// split deterministic instead of inheriting whatever the session runs. +// MAX_FABLE caps fable-pinned reviewers across the whole run: the three fixed +// fable lenses spend the budget, so dynamic lenses take opus. Diversity is +// preserved (both families always read the diff) at a fixed fable cost. +const MAX_FABLE = 3 + +const LENS_PROPOSALS = { + type: 'object', + required: ['lenses'], + properties: { + lenses: { + type: 'array', + maxItems: 6, + items: { + type: 'object', + required: ['key', 'prompt'], + properties: { + key: { type: 'string', description: 'short kebab-case name for the lens' }, + prompt: { type: 'string', description: 'one-paragraph reviewer charter: what this lens hunts and why THIS PR earns it' }, + }, + }, + }, + }, +} + +phase('Scope') +let spent = 0 +let dynamic = [] +if (givenLenses) { + dynamic = givenLenses.slice(0, 6).map((l) => ({ key: String(l.key), prompt: String(l.prompt) })) + log(`using ${dynamic.length} caller-provided dynamic lenses, scout skipped`) +} else { + spent += 1 + const scout = await agent( + `${SAFETY} + +You are the SCOUT for a deep review of PR ${pr}. Read the diff and the PR body, understand what kind of work this PR actually does, and propose ZERO to six ADDITIONAL review lenses tailored to it. Six fixed lenses already run regardless, so never duplicate their ground: correctness of the changed logic, security, repo-wide ripple effects of touched symbols, test adequacy and counterfactuals, invariant and doc drift, and a broad fresh-eyes pass. + +Propose a lens only when THIS PR's nature earns it. Examples of the kind of thing that earns one: concurrency or race conditions if the diff touches async coordination; serialization compatibility if a wire format changed; migration or data-loss paths if storage schemas moved; API backward compatibility if public signatures changed; performance if a hot path was rewritten; accessibility if UI semantics changed; prompt-injection surfaces if agent-facing prose or tooling changed. Zero is a fine answer for a PR whose nature the fixed six already cover. + +Each proposal is a key plus a one-paragraph reviewer charter written like an order: what to hunt, where in this diff, and what evidence would confirm it.`, + { label: 'scout:lenses', phase: 'Scope', schema: LENS_PROPOSALS }, + ) + dynamic = ((scout && scout.lenses) || []).slice(0, 6) + log(`scout proposed ${dynamic.length} dynamic lens(es)${dynamic.length ? ': ' + dynamic.map((l) => l.key).join(', ') : ''}`) +} +// Budget: fixed lenses always run; dynamic ones fit in what remains after +// reserving at least 4 jury slots for the verify phase. +const dynamicAllowed = Math.max(0, MAX_AGENTS - spent - LENSES.length - 4) +if (dynamic.length > dynamicAllowed) { + log(`agent budget ${MAX_AGENTS}: trimming dynamic lenses ${dynamic.length} to ${dynamicAllowed}`) + dynamic = dynamic.slice(0, dynamicAllowed) +} +let fableUsed = LENSES.filter((l) => l.model === 'fable').length +const DYNAMIC = dynamic.map((l) => { + const model = fableUsed < MAX_FABLE ? 'fable' : 'opus' + if (model === 'fable') fableUsed += 1 + return { key: `dyn-${l.key}`, prompt: l.prompt, model } +}) +const ALL_LENSES = [...LENSES, ...DYNAMIC] + +phase('Find') +log(`deep-review of PR ${pr}${repo ? ` in ${repo}` : ''}: ${ALL_LENSES.length} lenses in parallel (${LENSES.length} fixed, ${DYNAMIC.length} dynamic)`) + +const found = await parallel(ALL_LENSES.map((l) => () => + agent( + `${SAFETY}\n\nYou are ONE review lens over PR ${pr}. Your single charter:\n${l.prompt}\n\nReport only genuine problems with concrete failure scenarios. No style nits, no suggestions, no padding. Return an empty findings array if you find nothing real.`, + { label: `find:${l.key}`, phase: 'Find', schema: FINDINGS, ...(l.model ? { model: l.model } : {}) }, + ))) + +// Barrier is deliberate: dedup needs every finder's output before the +// expensive verification stage spends refuters on duplicates. +const all = found.filter(Boolean).flatMap((r) => r.findings) +const byKey = new Map() +const rank = { critical: 0, major: 1, minor: 2 } +for (const f of all) { + const key = `${f.file}:${f.line}` + const prev = byKey.get(key) + if (!prev || rank[f.severity] < rank[prev.severity]) byKey.set(key, f) +} +const deduped = [...byKey.values()].sort((a, b) => rank[a.severity] - rank[b.severity]) +const CAP = 12 +const toVerify = deduped.slice(0, CAP) +if (deduped.length > CAP) log(`capping verification at ${CAP} of ${deduped.length} deduped findings (dropped ${deduped.length - CAP} lowest-severity; re-run after fixes to catch them)`) +log(`${all.length} raw findings, ${deduped.length} after dedup, verifying ${toVerify.length}`) + +if (toVerify.length === 0) return { pr, repo, confirmed: [], rejected: [], unverified: [], stats: { raw: all.length, deduped: deduped.length, verified: 0, lenses: ALL_LENSES.length, dynamic: DYNAMIC.map((l) => l.key), maxAgents: MAX_AGENTS, fable: ALL_LENSES.filter((l) => l.model === 'fable').length }, note: 'no findings survived dedup; treat as a clean deep pass' } + +phase('Verify') +// Adaptive adversarial jury: 3 refuters for critical, 2 for major, 1 for minor. +// A finding dies when a strict MAJORITY of its jury refutes it, so a 1-1 split +// on a major finding survives (fail-open toward treating it as real). +const juries = { critical: 3, major: 2, minor: 1 } + +// Allocate jurors within the remaining budget, severity-first (toVerify is +// already severity-sorted). A finding allocated zero jurors is returned +// UNVERIFIED rather than silently dropped. +spent += ALL_LENSES.length +let juryBudget = Math.max(0, MAX_AGENTS - spent) +const allocated = [] +const unverified = [] +for (const f of toVerify) { + const take = Math.min(juries[f.severity], juryBudget) + if (take === 0) { unverified.push(f); continue } + juryBudget -= take + allocated.push({ f, take }) +} +if (unverified.length) log(`agent budget ${MAX_AGENTS}: ${unverified.length} finding(s) returned unverified (no jury slots left); raise maxAgents or re-run after fixes`) + +const verified = await parallel(allocated.map(({ f, take }) => () => + parallel(Array.from({ length: take }, (_, i) => () => + agent( + `${SAFETY}\n\nYou are an adversarial verifier. A reviewer claims this defect in PR ${pr}:\n\nFILE: ${f.file}:${f.line}\nCLAIM: ${f.title}\nSCENARIO: ${f.detail}\n\nTry to REFUTE it. Read the actual code at head, trace the scenario, and decide whether the defect is real. Angle ${i + 1}: ${i === 0 ? 'does the claimed scenario actually reproduce in the code as written?' : i === 1 ? 'is the behavior actually correct or intended, making the claim a false positive?' : 'is this already guarded, tested, or handled somewhere the finder did not look?'} If you cannot confirm the defect is real, refuted=true.`, + { label: `refute:${f.file.split('/').pop()}:${f.line}`, phase: 'Verify', schema: VERDICT }, + ))) + .then((votes) => { + const cast = votes.filter(Boolean) + const refutes = cast.filter((v) => v.refuted).length + // An empty jury (every refuter died) leaves the finding CONFIRMED: + // fail-open toward the finding being real, never silently dropped. + const dead = cast.length > 0 && refutes * 2 > cast.length + return { ...f, confirmed: !dead, jury: cast.length, refutes, reasons: cast.map((v) => v.reason) } + }))) + +const results = verified.filter(Boolean) +const confirmed = results.filter((r) => r.confirmed) +const rejected = results.filter((r) => !r.confirmed) +log(`confirmed ${confirmed.length}, refuted ${rejected.length}`) + +return { + pr, + repo, + confirmed, + rejected, + unverified, + stats: { raw: all.length, deduped: deduped.length, verified: allocated.length, lenses: ALL_LENSES.length, dynamic: DYNAMIC.map((l) => l.key), maxAgents: MAX_AGENTS, fable: ALL_LENSES.filter((l) => l.model === 'fable').length }, + note: 'Confirmed findings feed the normal review loop: fix, post as one review object, then a delta round. Rejected ones carry their refuters\' reasons for the audit trail.', +} diff --git a/.gitignore b/.gitignore index 0d9584459..72437ab73 100644 --- a/.gitignore +++ b/.gitignore @@ -70,6 +70,8 @@ Thumbs.db !.claude/hooks/** !.claude/skills/ !.claude/skills/** +!.claude/workflows/ +!.claude/workflows/** # test artifacts coverage/ diff --git a/AGENTS.md b/AGENTS.md index d39d9333c..faaf9649e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -37,15 +37,15 @@ itself): commands, repo-health git config, changelog flow, dev error overlay. ### Before starting ANY work: verify and sync the branch -1. `git branch --show-current`. If on `main` / `master`, **STOP** and `git checkout -b feature/`. +1. Work NEVER happens in the primary checkout. Cut the task's worktree first: `git worktree add -b / ../- origin/main`, and do ALL work there. 2. Verify the branch matches the task. Don't mix unrelated work. 3. Sync with parent: `git fetch origin && git log HEAD..origin/main --oneline`. If there are upstream commits, `git rebase origin/main` first. -Claude Code enforces step 1 via `.claude/hooks/guard-branch-context.sh`. Other agents check manually. +Claude Code enforces step 1 via `.claude/hooks/require-worktree-for-edits.sh` (blocks tracked-file edits in a primary checkout; test `test/hooks/require-worktree-for-edits.test.mjs`, escape hatch `WEBJS_NO_WORKTREE_GATE=1`). Other agents check manually. -### One task per git worktree when agents run concurrently +### One task per git worktree, ALWAYS -WebJs is worked by MULTIPLE agents at once. If more than one agent (or more than one in-flight task) shares ONE working checkout, they collide: a `git checkout` in one moves `HEAD` under the other, so the next commit lands on the WRONG branch. This has happened (a `chore: release` commit landed on an unrelated `feat/` branch, with a contaminated changelog). So give each task its own worktree: +WebJs is worked by MULTIPLE agents at once, and "no other agent is active right now" is unverifiable mid-task (another session can start any minute), so the worktree rule is unconditional. If more than one agent (or more than one in-flight task) shares ONE working checkout, they collide: a `git checkout` in one moves `HEAD` under the other, so the next commit lands on the WRONG branch. This has happened (a `chore: release` commit landed on an unrelated `feat/` branch, with a contaminated changelog). So give each task its own worktree: ```sh git worktree add -b / ../- origin/main @@ -55,7 +55,7 @@ cd ../- # do ALL work for the task here **A fresh worktree has NO `node_modules`** (git worktrees do not copy it), so running an app from one (`webjs dev` / `webjs start`, the test runner, a scaffolded app) fails to resolve `@webjsdev/*` until you install or link it. `webjs doctor` warns for this exact case (#954) and `webjs dev` / `webjs start` print the cause + remedy instead of a raw `ERR_MODULE_NOT_FOUND`. Fix by installing in the worktree (`npm install`) or symlinking the primary checkout's modules (`ln -s ..//node_modules node_modules`). When only a subset of `@webjsdev/*` packages was edited, link those from the worktree and the rest from the primary checkout so a built `dist/` (e.g. `@webjsdev/core`) still resolves. -Git enforces one-branch-per-worktree, so separate worktrees make the collision impossible. Before any commit in a shared checkout, confirm `git branch --show-current` is still the branch you created; if it moved, you are colliding, switch to a worktree. A lone agent in a clean checkout may still use a plain branch. The repo's `.hooks/pre-commit` additionally BLOCKS a published-library (`core`/`server`/`cli`/`mcp`/`ui`/`intellisense`) version bump on any non-`chore/release-*` branch, the canonical wrong-branch-release symptom. +Git enforces one-branch-per-worktree, so separate worktrees make the collision impossible. There is NO lone-agent exception: every task cuts a worktree, and the primary checkout stays an untouched mirror of main (tracked-file edits there are hook-blocked). The repo's `.hooks/pre-commit` additionally BLOCKS a published-library (`core`/`server`/`cli`/`mcp`/`ui`/`intellisense`) version bump on any non-`chore/release-*` branch, the canonical wrong-branch-release symptom. **Cleanup is automatic after a merge.** The `.claude/hooks/cleanup-merged-worktree.sh` PostToolUse hook fires after any `gh pr merge` and removes each linked worktree whose branch is merged AND whose tree is clean, so a merged branch's worktree never leaks (accumulated stale worktrees are exactly what it prevents). It is conservative: it KEEPS anything with uncommitted changes, an unmerged branch, or the worktree you ran the merge from (you cannot remove your current directory, so `cd` out and `git worktree remove` it yourself), and never touches the primary checkout. Disable with `WEBJS_NO_WORKTREE_CLEANUP=1`. Test: `test/hooks/cleanup-merged-worktree.test.mjs`. @@ -65,7 +65,7 @@ A Skill is model-invoked, so it fires only when the model judges a match. The `. ### Autonomous mode (sandbox / bypass permissions) -When interactive approval is disabled, never block on questions. Auto-decide: on `main`, auto-create `feature/`; auto-rebase if the parent moved; auto-merge when ready; **delete** feature/fix branches after merge but **keep** long-lived ones (dev, staging, release/*); auto-generate meaningful commit messages; fix failing tests / convention violations rather than asking. Autonomous mode is MORE disciplined, not less, with the same quality bar. +When interactive approval is disabled, never block on questions. Auto-decide: cut the task's worktree from `origin/main` (auto-create `/` per the label scheme); auto-rebase if the parent moved; auto-merge when ready; **delete** feature/fix branches after merge but **keep** long-lived ones (dev, staging, release/*); auto-generate meaningful commit messages; fix failing tests / convention violations rather than asking. Autonomous mode is MORE disciplined, not less, with the same quality bar. ### Code workflow (mandatory) diff --git a/test/hooks/require-worktree-for-edits.test.mjs b/test/hooks/require-worktree-for-edits.test.mjs new file mode 100644 index 000000000..f15481740 --- /dev/null +++ b/test/hooks/require-worktree-for-edits.test.mjs @@ -0,0 +1,110 @@ +// The require-worktree-for-edits hook blocks tracked-file edits in a repo's +// PRIMARY checkout and allows everything else: the same file in a linked +// worktree, untracked files, gitignored files, non-repo paths, and the +// WEBJS_NO_WORKTREE_GATE=1 escape hatch. AGENTS.md: one task per git +// worktree, ALWAYS. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawnSync, execSync } from 'node:child_process'; +import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from 'node:fs'; +import { join, dirname, resolve } from 'node:path'; +import { tmpdir } from 'node:os'; +import { fileURLToPath } from 'node:url'; + +const HOOK = resolve( + dirname(fileURLToPath(import.meta.url)), + '../../.claude/hooks/require-worktree-for-edits.sh' +); + +function runHook(filePath, env = {}) { + return spawnSync('bash', [HOOK], { + input: JSON.stringify({ tool_input: { file_path: filePath } }), + // Pin the escape hatch off so an inherited WEBJS_NO_WORKTREE_GATE=1 in the + // invoking environment cannot silently flip the block assertions. + env: { ...process.env, WEBJS_NO_WORKTREE_GATE: '0', ...env }, + encoding: 'utf8', + }); +} + +function makeRepo() { + const dir = mkdtempSync(join(tmpdir(), 'wt-gate-')); + const g = (cmd) => execSync(`git ${cmd}`, { cwd: dir, stdio: 'pipe' }); + g('init -q -b main'); + g('config user.email t@t'); + g('config user.name t'); + writeFileSync(join(dir, 'tracked.txt'), 'hello\n'); + mkdirSync(join(dir, 'sub', 'deep'), { recursive: true }); + writeFileSync(join(dir, 'sub', 'deep', 'nested.txt'), 'hello\n'); + writeFileSync(join(dir, '.gitignore'), 'ignored.txt\n'); + g('add .'); + g('commit -q -m init'); + return dir; +} + +test('tracked file in the PRIMARY checkout is blocked (counterfactual: the gate fires)', () => { + const repo = makeRepo(); + try { + const r = runHook(join(repo, 'tracked.txt')); + assert.equal(r.status, 2, `expected block, got ${r.status}: ${r.stderr}`); + assert.match(r.stderr, /PRIMARY checkout/); + assert.match(r.stderr, /git worktree add/); + } finally { rmSync(repo, { recursive: true, force: true }); } +}); + +test('a tracked file in a SUBDIRECTORY of the primary is blocked (git prints git-dir absolute and common-dir relative there, the fail-open the review caught)', () => { + const repo = makeRepo(); + try { + const r = runHook(join(repo, 'sub', 'deep', 'nested.txt')); + assert.equal(r.status, 2, `expected block, got ${r.status}: ${r.stderr}`); + } finally { rmSync(repo, { recursive: true, force: true }); } +}); + +test('the same tracked file in a LINKED worktree is allowed', () => { + const repo = makeRepo(); + const wt = `${repo}-wt`; + try { + execSync(`git worktree add -q -b feat/x ${wt}`, { cwd: repo, stdio: 'pipe' }); + const r = runHook(join(wt, 'tracked.txt')); + assert.equal(r.status, 0, r.stderr); + const rSub = runHook(join(wt, 'sub', 'deep', 'nested.txt')); + assert.equal(rSub.status, 0, rSub.stderr); + } finally { + execSync(`git worktree remove --force ${wt}`, { cwd: repo, stdio: 'pipe' }); + rmSync(repo, { recursive: true, force: true }); + } +}); + +test('an untracked file in the primary checkout is allowed', () => { + const repo = makeRepo(); + try { + writeFileSync(join(repo, 'scratch-note.md'), 'x'); + assert.equal(runHook(join(repo, 'scratch-note.md')).status, 0); + } finally { rmSync(repo, { recursive: true, force: true }); } +}); + +test('a gitignored file in the primary checkout is allowed', () => { + const repo = makeRepo(); + try { + writeFileSync(join(repo, 'ignored.txt'), 'x'); + assert.equal(runHook(join(repo, 'ignored.txt')).status, 0); + } finally { rmSync(repo, { recursive: true, force: true }); } +}); + +test('a path outside any git repo is allowed', () => { + const dir = mkdtempSync(join(tmpdir(), 'wt-norepo-')); + try { + writeFileSync(join(dir, 'f.txt'), 'x'); + assert.equal(runHook(join(dir, 'f.txt')).status, 0); + } finally { rmSync(dir, { recursive: true, force: true }); } +}); + +test('WEBJS_NO_WORKTREE_GATE=1 bypasses the block', () => { + const repo = makeRepo(); + try { + assert.equal(runHook(join(repo, 'tracked.txt'), { WEBJS_NO_WORKTREE_GATE: '1' }).status, 0); + } finally { rmSync(repo, { recursive: true, force: true }); } +}); + +test('a nonexistent-directory path is allowed (new file in a new dir elsewhere)', () => { + assert.equal(runHook('/nonexistent-dir-xyz/f.txt').status, 0); +});