From d8a9a19e52d5d3f8c31a89bcf92ee25a587e81f4 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Fri, 4 Sep 2026 16:27:14 -0700 Subject: [PATCH] Restore the /review skill body 1:1 with GaaS code-review Keep the /review slash name. Park GitHub posting, Linear In Review, ask_operator, fleet mapping, and findings-only on native-integration. Ignore GaaS review SKILL.md in prettier so emphasis stays 1:1. --- .prettierignore | 1 + CHANGELOG.md | 3 +- .../skills/native-integration/SKILL.md | 2 + plugins/corbits-skills/skills/review/SKILL.md | 154 +++--------------- tests/unit/corbits-skills-catalog.test.ts | 32 ++-- 5 files changed, 46 insertions(+), 146 deletions(-) diff --git a/.prettierignore b/.prettierignore index f94cbe91c..2e42c1586 100644 --- a/.prettierignore +++ b/.prettierignore @@ -8,6 +8,7 @@ plugins/corbits-skills/skills/opsh/SKILL.md plugins/corbits-skills/skills/refactor/SKILL.md plugins/corbits-skills/skills/scribe/SKILL.md plugins/corbits-skills/skills/ast-grep/SKILL.md +plugins/corbits-skills/skills/review/SKILL.md tmp/ .claude/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 732931bb5..9b32a2ec8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,12 +20,13 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename ### Changed - Drop unused `@opentui/keymap`, `@opentui/solid`, and `solid-js`. The interactive TUI is imperative `@opentui/core` only. +- Restore the /review skill body 1:1 with GaaS code-review. Slash stays /review. GitHub posting, Linear In Review, ask_operator, and findings-only stay on native-integration. - Restore the scribe skill 1:1 with GaaS. ask_operator mapping stays on native-integration. Slash /scribe remains. - Restore the ast-grep skill 1:1 with GaaS. run_shell mapping stays on native-integration. Slash /ast-grep remains. - Restore the opsh skill 1:1 with GaaS. Tool/shell mapping stays on native-integration. user-invocable: false stays so it remains use_skill-only. - Restore the pull-request-review skill 1:1 with GaaS. ask_operator, /review mapping, and GitHub posting stay on native-integration. Slash /pull-request-review remains. - Restore the refactor skill 1:1 with GaaS. ask_operator mapping stays on native-integration. Slash /refactor remains. -- Ignore GaaS opsh, refactor, scribe, and ast-grep SKILL.md in prettier so table/list alignment stays 1:1. +- Ignore GaaS opsh, refactor, scribe, ast-grep, and review SKILL.md in prettier so GaaS emphasis/table alignment stays 1:1. - Restore the git-rebase skill body 1:1 with GaaS. Intern execution recipe stays on native-integration. user-invocable: false stays so it remains use_skill-only. - Restore the linear-issue-workflow skill body 1:1 with GaaS. Claim-first, In Review, and git-worktrees extras stay on native-integration. user-invocable: false stays so it remains use_skill-only. - Restore the interview skill body 1:1 with GaaS (AskUserQuestion). Operator-ask mapping stays on native-integration. Slash /interview remains. diff --git a/plugins/corbits-skills/skills/native-integration/SKILL.md b/plugins/corbits-skills/skills/native-integration/SKILL.md index 030227509..7b94a09fc 100644 --- a/plugins/corbits-skills/skills/native-integration/SKILL.md +++ b/plugins/corbits-skills/skills/native-integration/SKILL.md @@ -43,6 +43,8 @@ GaaS ast-grep invokes `sg` as a CLI. Corbits extras: run `sg` via `run_shell`. D Slash names that differ from GaaS skill ids: `/review` is GaaS `code-review`; `/create-issue` is GaaS `linear-create`. Keep those Corbits names. +GaaS code-review uses "ask the user", "sub-agents"/"subagent", and `typescript-conventions`. Corbits extras: slash stays `/review`; `ask_operator` (tool mapping above); fleet `spawn_agent` for sub-agents; load `typescript` not `typescript-conventions`; findings-only — do not implement fixes; GitHub posting and Linear In Review (this skill). Do not fork the GaaS code-review body. + GaaS refactor says "ask clarifying questions" / "ask the user". Corbits extras: `ask_operator` (tool mapping above). Do not fork the GaaS refactor body. GaaS scribe uses the `question` tool. Corbits extras: `ask_operator` (tool mapping above). Do not fork the GaaS scribe body. diff --git a/plugins/corbits-skills/skills/review/SKILL.md b/plugins/corbits-skills/skills/review/SKILL.md index 55437fb9b..d285e1bec 100644 --- a/plugins/corbits-skills/skills/review/SKILL.md +++ b/plugins/corbits-skills/skills/review/SKILL.md @@ -1,12 +1,9 @@ --- name: review -argument-hint: "[paths | PR | diff | hygiene | architecture]" -description: Review a branch, PR, or path. Determines base and scope, judges only the branch diff, and reports findings with evidence. Does not implement fixes. +description: Perform a code review or pull request review on a branch --- -# Review - -How to review a branch, pull request, or path. Findings only — do not implement fixes. +# Code Review Use this skill when performing code reviews or pull request reviews. @@ -54,7 +51,7 @@ the intent of the branch author. ### When Base Cannot Be Determined -If none of the above methods provide a clear answer, do not assume. `ask_operator`: +If none of the above methods provide a clear answer, do not assume. Ask the user: > I cannot determine which branch this was based off. What branch should I use > as the base for comparison? Common options include `main`, `master`, or @@ -94,18 +91,18 @@ All new logic, patterns, and naming introduced by the branch should follow the project's established conventions. Pre-existing code that appears in the diff due to refactoring is exempt from this requirement. -If reviewing TypeScript code, consider loading the `typescript` +If reviewing TypeScript code, consider loading the `typescript-conventions` skill for detailed guidance on type patterns, naming, and idioms. -## Delegating to fleet agents +## Delegating to Sub-agents -When delegating file review to fleet agents, provide the output of +When delegating file review to sub-agents, provide the output of `git diff ...HEAD -- ` rather than the full file contents. -Fleet agents that receive full files cannot distinguish branch changes from +Sub-agents that receive full files cannot distinguish branch changes from pre-existing code and will flag out-of-scope issues. -If full files must be provided for context, explicitly instruct the worker +If full files must be provided for context, explicitly instruct the sub-agent which line ranges were modified by the branch and that only those ranges are in scope. @@ -146,7 +143,7 @@ as you would speak to a colleague: - Frame suggestions as questions or proposals, not commands. "Could we extract this?" not "Extract this." -- Explain the _why_ -- "so that X" or "because Y" -- not just the _what_. +- Explain the *why* -- "so that X" or "because Y" -- not just the *what*. - Avoid words like "wrong", "broken", "bad". Describe the consequence instead: "this will produce incorrect values when..." not "this is wrong." - If the fix is small, suggest it concretely. A one-liner suggestion is @@ -156,7 +153,7 @@ as you would speak to a colleague: ## Describe the Branch As It Stands -Review comments and PR descriptions must describe the branch's _current_ diff against the base, not the journey that produced it. The merged result is what ships; intermediate states, earlier review rounds, and pre-iteration code are not part of the artifact. +Review comments and PR descriptions must describe the branch's *current* diff against the base, not the journey that produced it. The merged result is what ships; intermediate states, earlier review rounds, and pre-iteration code are not part of the artifact. Re-derive the description from `git diff ...HEAD` every time. Do not trust the existing PR description — it may have been written before the branch was iterated on, and reviewers reading it now should see what is true now. @@ -170,7 +167,7 @@ Past-tense framing rots: as the branch evolves through review, descriptions of " ## Cite the Check -A review report may include affirmative verification claims — "tests pass," "messages are clean," "no regressions," "convention compliance verified." When a review says something is _verified_, the reader takes that as a checked fact. If the check was never run, the report is dishonest. The dishonesty does not surface until a human reviewer finds the issue the agent claimed did not exist. +A review report may include affirmative verification claims — "tests pass," "messages are clean," "no regressions," "convention compliance verified." When a review says something is *verified*, the reader takes that as a checked fact. If the check was never run, the report is dishonest. The dishonesty does not surface until a human reviewer finds the issue the agent claimed did not exist. **For every affirmative claim in your review, you must be able to cite the specific check that proved it.** A check is a concrete artifact: a command whose output you read, a tool invocation whose results you inspected, a file/line range you examined for a specific pattern. "I considered it" and "it looked fine to me" are not checks. @@ -184,14 +181,14 @@ Some properties — subtle concurrency bugs, performance pathologies, security g **Bad → Good:** -- "All commit messages are clean." → cite the specific audit command (see _Commit-Message Style Audit_), or strike. +- "All commit messages are clean." → cite the specific audit command (see *Commit-Message Style Audit*), or strike. - "Tests pass." → "`npm test` exited 0," or strike. - "Convention compliance verified." → "diffed naming and error-handling shape in the new handlers against `src/api/user.ts` and `src/api/billing.ts`; same `Result` return pattern, same `assert`-style guards," or strike. - "No race conditions." → "read `lock.go:42-68`, traced lock acquisition order; no acquire-while-holding cycles in those lines. Did not analyze interactions with `pool.go` or callers outside the diff." ## Reviewer-of-Record Checks -"Cite the Check" requires that affirmative claims have backing. This rule narrows it: for the checks below, the backing must be the reviewer-of-record's own eyes on raw output. The reviewer-of-record is the agent whose verdict ships — when a workflow delegates the deeper read to a fleet agent, the orchestrator remains the reviewer-of-record and these checks stay with them. +"Cite the Check" requires that affirmative claims have backing. This rule narrows it: for the checks below, the backing must be the reviewer-of-record's own eyes on raw output. The reviewer-of-record is the agent whose verdict ships — when a workflow delegates the deeper read to a subagent, the orchestrator remains the reviewer-of-record and these checks stay with them. Their value is in catching unknown-unknowns. A delegate asked for "a punch list of findings" returns things that fit the punch-list shape; `Bin 0 -> 8181 bytes` on a `.ts` file does not look like a finding, it looks like stat noise, and gets collapsed away. Only direct inspection preserves the signal. @@ -199,9 +196,9 @@ Their value is in catching unknown-unknowns. A delegate asked for "a punch list - `git diff ...HEAD --stat` — scan for `Bin` markers on any file you did not expect to be binary (source code, markdown, config) and for files outside the branch's stated scope. - `git log --oneline ..HEAD` — confirm the commits on the branch match the issue's scope; unexpected or off-topic commits are stop conditions. -- The subject and body audits enumerated under _Commit-Message Style Audit_. That section is the canonical command catalog and pattern list; this section's contribution is the delegation rule — those audits are reviewer-of-record, not fleet-agent work. +- The subject and body audits enumerated under *Commit-Message Style Audit*. That section is the canonical command catalog and pattern list; this section's contribution is the delegation rule — those audits are reviewer-of-record, not subagent work. -Delegating the deeper read (file-by-file behavioral review, architectural analysis, commit-message coherence) to a fleet agent is fine and often valuable for context isolation. Delegating the checks above is not — their value is the raw output landing in front of the reviewer-of-record's eyes. +Delegating the deeper read (file-by-file behavioral review, architectural analysis, commit-message coherence) to a subagent is fine and often valuable for context isolation. Delegating the checks above is not — their value is the raw output landing in front of the reviewer-of-record's eyes. ## Commit-Message Coherence @@ -227,7 +224,7 @@ surprised by what the diff actually contains, that is a problem worth raising. Coherence (above) checks that each message accurately describes its diff. Style audit checks that each message conforms to the project's commit-message rules (see the `style` skill, which is the canonical source for the prefix-family list and other rules). The two are independent; both have to be run. -Each check below is reviewer-of-record territory (see _Reviewer-of-Record Checks_) — run them in-session and read the raw output yourself. Do not delegate them; a fleet agent asked to "verify style compliance" will return a generic "looks fine" read with no audit trail. +Each check below is reviewer-of-record territory (see *Reviewer-of-Record Checks*) — run them in-session and read the raw output yourself. Do not delegate them; a subagent asked to "verify style compliance" will return a generic "looks fine" read with no audit trail. **Subject-line audits.** Most checks scan the output of: @@ -274,123 +271,14 @@ Affirmative claims about these audits must cite the specific command whose outpu ## Review Checklist -Items marked _(reviewer-of-record)_ must be run by the agent whose verdict ships — see _Reviewer-of-Record Checks_. Do not delegate them to a fleet agent. Unmarked items are delegable. +Items marked *(reviewer-of-record)* must be run by the agent whose verdict ships — see *Reviewer-of-Record Checks*. Do not delegate them to a subagent. Unmarked items are delegable. 1. Determine the base branch using the methods in "Base Branch Determination" -2. Run `git log --oneline ..HEAD` to understand the scope _(reviewer-of-record)_ -3. Run `git diff ...HEAD --stat` to see which files changed _(reviewer-of-record)_ +2. Run `git log --oneline ..HEAD` to understand the scope *(reviewer-of-record)* +3. Run `git diff ...HEAD --stat` to see which files changed *(reviewer-of-record)* 4. Review each changed file, focusing only on lines modified by the branch 5. For every commit on the branch, verify: - - Diff matches the commit message (see "Commit-Message Coherence") - - Subject and body pass the style audit (see "Commit-Message Style Audit") _(reviewer-of-record)_ + - Diff matches the commit message (see "Commit-Message Coherence") + - Subject and body pass the style audit (see "Commit-Message Style Audit") *(reviewer-of-record)* 6. Check that new code follows project conventions 7. Summarize findings with specific file:line references; cite the check behind any affirmative claim (see "Cite the Check") -8. When the review targets an open GitHub PR, post the review on the PR (see "Post the Review on GitHub") - -## Post the Review on GitHub - -When the branch under review has an open GitHub pull request, **post the finished review on the PR**. A review that only lives in the chat session is not done. - -This step is the delivery of the review, not a second pass of analysis. By the time you post, findings are already decided. Do not reopen the read while drafting the body. - -### When to post - -Post when any of these is true: - -- The user asked for a PR review (`pull-request-review`, a PR URL, or an explicit "review #N") -- `linear-issue-workflow` Phase 5 self-review has cleared and Phase 6 is opening or updating the PR -- An open PR exists for the branch and the review's purpose is to leave a record on it - -Do **not** post when the user only asked for a private/local read with no PR, or when the branch has no open PR and creating one is out of scope. - -### Multi-persona reviews - -When the workflow ran more than one review lens (for example `critic` for behavioral/architecture, `greybeard` for waivers or product judgment, an OSS/quality agent for packaging and public-API bar), each lens that produced a distinct judgment **posts its own review**. Do not collapse independent verdicts into one mushy "team thinks" paragraph. - -| Lens | What it owns | When to post | -| ---------------------- | ------------------------------------------------------------------ | ----------------------------------------------------------------------------------------- | -| Primary / orchestrator | Verdict on the branch as it stands; residual findings; waiver list | Always when posting | -| Critic | Behavioral bugs, missing tests, architecture, commit coherence | When a critic director ran | -| Greybeard | Waiver rulings and intentional exceptions | When Greybeard authorized any waiver, or when product/architecture judgment was requested | -| OSS / quality | Public-API, packaging, polish bar for shippable surface | When that lens was explicitly run | - -Same GitHub account is fine. Label each post so a human can tell which lens spoke. Prefer separate `gh pr review` / `gh pr comment` posts over one mega-comment when more than one lens has substance. - -If only the primary review ran, post once. Do not invent extra personas. - -### Command - -Use a real GitHub review, not a floating issue comment, for the primary verdict: - -```bash -# Approve — branch is ready to merge as it stands -gh pr review --approve --body "$(cat <<'EOF' - -EOF -)" - -# Comment — findings or notes that do not block merge by themselves -gh pr review --comment --body "$(cat <<'EOF' - -EOF -)" - -# Request changes — at least one finding must be fixed before merge -gh pr review --request-changes --body "$(cat <<'EOF' - -EOF -)" -``` - -Secondary persona posts may use `gh pr review --comment` or `gh pr comment`. Prefer `gh pr review --comment` so they appear in the Reviews timeline. Never `--approve` from a secondary lens that did not own the merge verdict. - -### Body shape (no AI slop) - -Write like a senior engineer leaving a review on a busy PR. Short. Specific. Present tense. No journey narration. - -**Required shape:** - -```markdown -## · - - - -### Findings - -- `path/to/file.ts:12` — -- `path/to/other.ts:40` — <…> - -### Notes - -- -``` - -When the review is clean: - -```markdown -## Review · Approve - -Hub list/get/upload under `/api/tenants/:id/artifacts`; Library reads that plane. - -No findings. -``` - -**Hard bans** (delete on sight before posting): - -- Throat-clearing: "Great work", "Thanks for this", "Overall this looks solid", "Happy to approve" -- AI filler: "I'd like to highlight", "It's worth noting", "In conclusion", "Going forward" -- Journey talk: "this PR adds… then fixes… after feedback…" -- Fake balance: praising three things to soften one finding -- Emoji, decorative headers, horizontal rules used as ornament -- Restating the diff file-by-file when there is no finding -- "LGTM" alone with no one-line present-tense description of what the branch does - -**Findings rules:** - -- Every finding has a `path:line` (or `path` when line is meaningless) and a concrete failure mode -- Severity is the review action (`--request-changes` vs `--comment`), not adjectives in the body -- No "nit:" / "minor:" / "suggestion:" as a way to smuggle unactionable taste — if it is not worth the author's time, drop it (see Signal Over Noise) - -### After posting - -Paste the review URL(s) back to the user. Do not mark the Linear issue Done. `--request-changes` is not merge-ready. While the PR is open and ready for review, the issue stays In Review — including after `--request-changes`. Do not ping-pong it back to In Progress. `linear-issue-workflow` owns the In Review write; this skill does not set Linear state. diff --git a/tests/unit/corbits-skills-catalog.test.ts b/tests/unit/corbits-skills-catalog.test.ts index b24d5d1b2..6c89406f6 100644 --- a/tests/unit/corbits-skills-catalog.test.ts +++ b/tests/unit/corbits-skills-catalog.test.ts @@ -224,19 +224,24 @@ test("philosophy skill is 1:1 with GaaS philosophy", async () => { expect(skill).not.toContain("use_skill("); }); -test("review skill is a code-review playbook, not a director router", async () => { +test("review skill is 1:1 with GaaS code-review except slash name", async () => { const skill = await Bun.file(join(pluginRoot, "skills/review/SKILL.md")).text(); - expect(skill).toContain("git diff ...HEAD"); + expect(skill).toContain("name: review"); + expect(skill).not.toContain("name: code-review"); + expect(skill).toContain("description: Perform a code review or pull request review on a branch"); + expect(skill).toContain("# Code Review"); + expect(skill).toContain("Ask the user"); + expect(skill).toContain("sub-agents"); + expect(skill).toContain("typescript-conventions"); expect(skill).toContain("Cite the Check"); - expect(skill).toContain("Signal Over Noise"); - expect(skill).toContain("Pre-existing Code"); - expect(skill).toContain("do not implement fixes"); - expect(skill).toContain("Findings only"); + expect(skill).toContain("git diff ...HEAD"); + expect(skill).not.toContain("ask_operator"); + expect(skill).not.toContain("argument-hint"); + expect(skill).not.toContain("Findings only"); + expect(skill).not.toContain("Post the Review on GitHub"); expect(skill).not.toContain("spawn_agent"); expect(skill).not.toContain("wait_agents"); - expect(skill).not.toContain('task(agent="critic")'); - expect(skill).not.toContain('task(agent="neckbeard")'); - expect(skill).not.toContain('task(agent="greybeard")'); + expect(skill).not.toContain("fleet agents"); }); test("refactor skill is 1:1 with GaaS refactor", async () => { @@ -370,10 +375,11 @@ test("linear-issue-workflow is 1:1 with GaaS", async () => { expect(skill).not.toContain('set the issue state to "In Review"'); }); -test("review skill does not own the Linear In Review write", async () => { +test("review skill does not own GitHub posting or Linear In Review", async () => { const skill = await Bun.file(join(pluginRoot, "skills/review/SKILL.md")).text(); - expect(skill).toContain("`linear-issue-workflow` owns the In Review write"); - expect(skill).not.toContain("Reviewers do not change Linear state"); + expect(skill).not.toContain("Post the Review on GitHub"); + expect(skill).not.toContain("`linear-issue-workflow` owns the In Review write"); + expect(skill).not.toContain("this skill does not set Linear state"); }); test("slash skills do not set user-invocable: false", async () => { @@ -437,6 +443,8 @@ test("native-integration maps GaaS tool names and parks Corbits extras", async ( expect(skill).toContain("Do not fork the GaaS refactor body"); expect(skill).toContain("Do not fork the GaaS scribe body"); expect(skill).toContain("Do not fork the GaaS ast-grep body"); + expect(skill).toContain("Do not fork the GaaS code-review body"); + expect(skill).toContain("findings-only"); expect(skill).toContain("run `sg` via `run_shell`"); expect(skill).toContain("prove"); });