From 0633c18ed666561275ccd5a2445902c74f9c5d5d Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 22 Sep 2026 13:54:43 +0530 Subject: [PATCH] docs: let a review ask that says fix apply the fixes Running the harness code-review agent with --fix (or --fix --comment) sometimes produced comments and no edits: the review contract in the pr-review skill, the route-skills directive, webjs-start-work and the workflow rules all stated flatly that the reviewer never fixes what it finds, and that read as outranking the flag. The unasked-for review cycle is still gone and a plain review still stops at the findings, resolves no threads and waits on no CI. What changes is that an explicit fix ask is honoured, and --fix --comment means both rather than comments instead of edits. --- .agents/rules/workflow.md | 2 +- .claude/hooks/route-skills.sh | 10 ++++++---- .claude/skills/pr-review/SKILL.md | 21 +++++++++++++-------- .claude/skills/webjs-start-work/SKILL.md | 2 +- test/hooks/route-skills.test.mjs | 10 +++++++--- 5 files changed, 28 insertions(+), 17 deletions(-) diff --git a/.agents/rules/workflow.md b/.agents/rules/workflow.md index 48f01e000..3a884e0a4 100644 --- a/.agents/rules/workflow.md +++ b/.agents/rules/workflow.md @@ -58,7 +58,7 @@ These project-level rules govern all operations inside this workspace. Antigravi - `webjs-scaffold-sync`: Trigger when changing the CLI generators, the scaffold templates, or the agent teaching skill. - `webjs-blog-write`: Trigger when writing, drafting, or editing a WebJs blog post under `blog/`. - `webjs-instagram-post`: Trigger when publishing an SEO post to the WebJs Instagram account. - - `pr-review`: Trigger when asked to review a PR. The review runs INLINE in the session that was asked, never in a subagent and never as a multi-round cycle, and posts through the GitHub review API as one review object (a summary plus line-anchored comments with suggestion blocks). It only reviews: it does not fix findings, resolve threads, or wait on CI. + - `pr-review`: Trigger when asked to review a PR. The review runs INLINE in the session that was asked, never in a subagent and never as a multi-round cycle, and posts through the GitHub review API as one review object (a summary plus line-anchored comments with suggestion blocks). A plain review ask stops at the findings: it resolves no threads and waits on no CI. An ask that says to fix them (`/code-review --fix`, "review and fix") applies the fixes too. - `use-railway`: Trigger when interacting with Railway deploys. - The framework teaching skill at `.agents/skills/webjs/` is a real directory rather than a symlink, and is the reference for building WebJs apps rather than a workflow trigger. - Always use the `view_file` tool on the matched skill's `SKILL.md` before executing its tasks. diff --git a/.claude/hooks/route-skills.sh b/.claude/hooks/route-skills.sh index 5611d6414..fc485cb65 100755 --- a/.claude/hooks/route-skills.sh +++ b/.claude/hooks/route-skills.sh @@ -198,14 +198,16 @@ fi # ask THIS agent for a review, the pr-review skill runs it like a human # reviewer working over the GitHub API instead of the dashboard: one # inline read, one posted review object (summary plus line-anchored -# comments with suggestion blocks). Review only: no reviewer subagent, -# no multi-round cycle, no fixing findings, no waiting on CI. The skill -# is agent-agnostic (plain gh/REST) and committed at +# comments with suggestion blocks). No reviewer subagent, no multi-round +# cycle, no waiting on CI. A plain review ask stops at the findings; an +# ask that says to FIX them (the built-in `/code-review --fix`, "review +# and fix") applies the fixes too, and `--fix --comment` means both. The +# skill is agent-agnostic (plain gh/REST) and committed at # .claude/skills/pr-review, exposed cross-agent via .agents/skills/. if has '(review|audit) (the |my |this )?(pr|diff|branch|change|changes|code|commit)' \ || has 'code ?review' \ || has '(review|look) .{0,20}(over )?for (bug|issue|correctness|regression)'; then - add_match "pr-review: the request is to review code. Invoke the pr-review skill and perform the review YOURSELF, inline in this session. NEVER spawn a reviewer subagent and NEVER run a multi-round review cycle. When the target is a pull request, post the review through the GitHub review API as ONE review object, a summary plus line-anchored comments that highlight the code to fix and carry suggestion blocks where a concrete replacement is obvious, exactly as the skill specifies. The reviewer ONLY reviews: it does not fix findings, does not resolve threads, and never waits on or reports CI. For a local diff with no PR, review inline and report the findings in the conversation instead. The owner decides what gets fixed, and fixing is separate work on a separate ask." + add_match "pr-review: the request is to review code. Invoke the pr-review skill and perform the review YOURSELF, inline in this session. NEVER spawn a reviewer subagent and NEVER run a multi-round review cycle. When the target is a pull request, post the review through the GitHub review API as ONE review object, a summary plus line-anchored comments that highlight the code to fix and carry suggestion blocks where a concrete replacement is obvious, exactly as the skill specifies. A plain review ask stops at the findings: it resolves no threads and never waits on or reports CI. But when the request asks for the findings to be FIXED (the built-in \`/code-review --fix\`, \`--fix --comment\`, \"review and fix\", \"apply the findings\"), APPLY the fixes; \`--fix --comment\` means do both, never comment instead of fixing. For a local diff with no PR, review inline and report the findings in the conversation instead." fi # --- verify: prove the change works by running the app ------------------ diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 6b8c201cb..32937233d 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -1,6 +1,6 @@ --- name: pr-review -description: Review a GitHub pull request the way a human reviewer would, entirely inline, and post the review through the GitHub review API as ONE review object, a summary plus line-anchored comments that highlight the code to fix and carry suggestion blocks where a concrete replacement is obvious. Trigger whenever the user asks to review a PR ("review the PR", "review #123", "look over this pull request", "review the branch/changes" when the branch has an open PR). The reviewer ONLY reviews. It never fixes findings, never waits on or reports CI, never resolves threads, and never delegates to a subagent or another agent. +description: Review a GitHub pull request the way a human reviewer would, entirely inline, and post the review through the GitHub review API as ONE review object, a summary plus line-anchored comments that highlight the code to fix and carry suggestion blocks where a concrete replacement is obvious. Trigger whenever the user asks to review a PR ("review the PR", "review #123", "look over this pull request", "review the branch/changes" when the branch has an open PR). A plain review ask stops at the findings: it never waits on or reports CI, never resolves threads, and never delegates to a subagent or another agent. When the ask says to fix the findings (`/code-review --fix`, "review and fix"), the fixes are applied too. when_to_use: | Examples that should trigger this skill: "review the PR" @@ -33,10 +33,14 @@ commands. itself, in the same session. Never spawn a reviewer subagent, a fleet, or a background task for it, and never run a multi-round review cycle. One read over the whole diff, one posted review, done. -2. **Review only.** The reviewer never fixes what it finds: no commits, - no pushes, no code edits, no resolving of threads. Findings are the - deliverable. Fixing them is separate work for whoever owns the - branch, on a separate ask. +2. **Findings are the deliverable, unless a fix was asked for.** A + plain review ask stops at the posted findings: no commits, no + pushes, no code edits, no resolving of threads. An ask that says to + fix them (`/code-review --fix`, `--fix --comment`, "review and + fix", "apply the findings") is a fix ask: apply the findings to the + working tree, and when comments were asked for too, do both rather + than commenting instead of fixing. Nothing in this skill overrides + a fix the user asked for. 3. **No CI.** Never wait on, read, or report CI or check status. Checks are the merge gate's business, not the reviewer's, and a review that stalls on a pending check has failed its one job of being fast @@ -168,6 +172,7 @@ gh api -X POST repos///pulls//reviews --input review.json A clean review still posts: a short summary saying it is clean, with no inline comments. Then tell the user the outcome in one or two -sentences, with the review's URL and the finding count. Stop there. No -fixing, no thread resolution, no follow-up issues, no re-review unless -they ask again. +sentences, with the review's URL and the finding count. Stop there: no +thread resolution, no follow-up issues, no re-review unless they ask +again. Stop before fixing too, unless the ask was to fix (see rule 2), +in which case apply the findings now. diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 46f16da64..366d0ed95 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -261,7 +261,7 @@ There is NO automated pre-merge review cycle in this workflow. Do not spawn revi Before flipping to ready, run everything the Definition of done demands: the full suites for every layer the change touches (full Node, browser, e2e, the Bun matrix, the two-app dogfood boot check). Launch them as parallel background tasks in one batch and collect EVERY result before reporting, because a task you forget to collect is a silently skipped layer. Then `gh pr ready ` and report back that the PR awaits the owner's review. Never report the PR ready with failing or unrun suites. -**If the owner explicitly asks you to review a PR, do it yourself, inline in this session, per the `pr-review` skill** (`.claude/skills/pr-review`, exposed cross-agent at `.agents/skills/pr-review`): one read over the whole diff, posted to the PR through the GitHub review API as one review object, a summary plus line-anchored comments with suggestion blocks. The reviewer ONLY reviews. It never fixes the findings, never resolves threads, never waits on or reports CI, and is never delegated to a subagent or expanded into rounds. The owner decides what gets fixed, and fixing is separate branch work on a separate ask. +**If the owner explicitly asks you to review a PR, do it yourself, inline in this session, per the `pr-review` skill** (`.claude/skills/pr-review`, exposed cross-agent at `.agents/skills/pr-review`): one read over the whole diff, posted to the PR through the GitHub review API as one review object, a summary plus line-anchored comments with suggestion blocks. A plain review ask stops at the findings: it resolves no threads, never waits on or reports CI, and is never delegated to a subagent or expanded into rounds. When the ask says to FIX the findings (`/code-review --fix`, `--fix --comment`, "review and fix"), apply them; with comments asked for too, do both rather than commenting instead of fixing. ### Acting on the owner's review comments diff --git a/test/hooks/route-skills.test.mjs b/test/hooks/route-skills.test.mjs index fa5ba6685..0997d131c 100644 --- a/test/hooks/route-skills.test.mjs +++ b/test/hooks/route-skills.test.mjs @@ -213,9 +213,13 @@ test('the pr-review directive and skill both keep the review contract', () => { // One read, not a cycle. A round count coming back is the regression. assert.match(directive, /NEVER run a multi-round review cycle/); assert.match(skill, /never run a multi-round\s+review cycle/i); - // Review only: fixing the findings is the author's job, on a separate ask. - assert.match(directive, /does not fix findings/); - assert.match(skill, /\*\*Review only\.\*\* The reviewer never fixes what it finds/); + // A plain review stops at the findings, but a fix ask (/code-review --fix) + // is honoured: commenting INSTEAD of fixing is the regression this guards. + assert.match(directive, /A plain review ask stops at the findings/); + assert.match(directive, /APPLY the fixes/); + assert.match(directive, /never comment instead of fixing/); + assert.match(skill, /Findings are the deliverable, unless a fix was asked for/); + assert.match(skill, /Nothing in this skill overrides\s+a fix the user asked for/); // Never blocks on CI, which is the merge gate's business. assert.match(directive, /never waits on or reports CI/); assert.match(skill, /\*\*No CI\.\*\* Never wait on, read, or report CI/);