Skip to content

Commit 8aae312

Browse files
committed
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.
1 parent 480ea66 commit 8aae312

5 files changed

Lines changed: 46 additions & 146 deletions

File tree

.prettierignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ plugins/corbits-skills/skills/opsh/SKILL.md
88
plugins/corbits-skills/skills/refactor/SKILL.md
99
plugins/corbits-skills/skills/scribe/SKILL.md
1010
plugins/corbits-skills/skills/ast-grep/SKILL.md
11+
plugins/corbits-skills/skills/review/SKILL.md
1112

1213
tmp/
1314
.claude/

CHANGELOG.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,13 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename
1919

2020
### Changed
2121

22+
- 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.
2223
- Restore the scribe skill 1:1 with GaaS. ask_operator mapping stays on native-integration. Slash /scribe remains.
2324
- Restore the ast-grep skill 1:1 with GaaS. run_shell mapping stays on native-integration. Slash /ast-grep remains.
2425
- 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.
2526
- 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.
2627
- Restore the refactor skill 1:1 with GaaS. ask_operator mapping stays on native-integration. Slash /refactor remains.
27-
- Ignore GaaS opsh, refactor, scribe, and ast-grep SKILL.md in prettier so table/list alignment stays 1:1.
28+
- Ignore GaaS opsh, refactor, scribe, ast-grep, and review SKILL.md in prettier so GaaS emphasis/table alignment stays 1:1.
2829
- 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.
2930
- 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.
3031
- Restore the interview skill body 1:1 with GaaS (AskUserQuestion). Operator-ask mapping stays on native-integration. Slash /interview remains.

plugins/corbits-skills/skills/native-integration/SKILL.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ GaaS ast-grep invokes `sg` as a CLI. Corbits extras: run `sg` via `run_shell`. D
4343

4444
Slash names that differ from GaaS skill ids: `/review` is GaaS `code-review`; `/create-issue` is GaaS `linear-create`. Keep those Corbits names.
4545

46+
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.
47+
4648
GaaS refactor says "ask clarifying questions" / "ask the user". Corbits extras: `ask_operator` (tool mapping above). Do not fork the GaaS refactor body.
4749

4850
GaaS scribe uses the `question` tool. Corbits extras: `ask_operator` (tool mapping above). Do not fork the GaaS scribe body.

plugins/corbits-skills/skills/review/SKILL.md

Lines changed: 21 additions & 133 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,9 @@
11
---
22
name: review
3-
argument-hint: "[paths | PR | diff | hygiene | architecture]"
4-
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.
3+
description: Perform a code review or pull request review on a branch
54
---
65

7-
# Review
8-
9-
How to review a branch, pull request, or path. Findings only — do not implement fixes.
6+
# Code Review
107

118
Use this skill when performing code reviews or pull request reviews.
129

@@ -54,7 +51,7 @@ the intent of the branch author.
5451

5552
### When Base Cannot Be Determined
5653

57-
If none of the above methods provide a clear answer, do not assume. `ask_operator`:
54+
If none of the above methods provide a clear answer, do not assume. Ask the user:
5855

5956
> I cannot determine which branch this was based off. What branch should I use
6057
> 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
9491
the project's established conventions. Pre-existing code that appears in
9592
the diff due to refactoring is exempt from this requirement.
9693

97-
If reviewing TypeScript code, consider loading the `typescript`
94+
If reviewing TypeScript code, consider loading the `typescript-conventions`
9895
skill for detailed guidance on type patterns, naming, and idioms.
9996

100-
## Delegating to fleet agents
97+
## Delegating to Sub-agents
10198

102-
When delegating file review to fleet agents, provide the output of
99+
When delegating file review to sub-agents, provide the output of
103100
`git diff <base-branch>...HEAD -- <file>` rather than the full file contents.
104101

105-
Fleet agents that receive full files cannot distinguish branch changes from
102+
Sub-agents that receive full files cannot distinguish branch changes from
106103
pre-existing code and will flag out-of-scope issues.
107104

108-
If full files must be provided for context, explicitly instruct the worker
105+
If full files must be provided for context, explicitly instruct the sub-agent
109106
which line ranges were modified by the branch and that only those ranges are
110107
in scope.
111108

@@ -146,7 +143,7 @@ as you would speak to a colleague:
146143

147144
- Frame suggestions as questions or proposals, not commands.
148145
"Could we extract this?" not "Extract this."
149-
- Explain the _why_ -- "so that X" or "because Y" -- not just the _what_.
146+
- Explain the *why* -- "so that X" or "because Y" -- not just the *what*.
150147
- Avoid words like "wrong", "broken", "bad". Describe the consequence
151148
instead: "this will produce incorrect values when..." not "this is wrong."
152149
- If the fix is small, suggest it concretely. A one-liner suggestion is
@@ -156,7 +153,7 @@ as you would speak to a colleague:
156153

157154
## Describe the Branch As It Stands
158155

159-
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.
156+
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.
160157

161158
Re-derive the description from `git diff <base>...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.
162159

@@ -170,7 +167,7 @@ Past-tense framing rots: as the branch evolves through review, descriptions of "
170167

171168
## Cite the Check
172169

173-
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.
170+
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.
174171

175172
**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.
176173

@@ -184,24 +181,24 @@ Some properties — subtle concurrency bugs, performance pathologies, security g
184181

185182
**Bad → Good:**
186183

187-
- "All commit messages are clean." → cite the specific audit command (see _Commit-Message Style Audit_), or strike.
184+
- "All commit messages are clean." → cite the specific audit command (see *Commit-Message Style Audit*), or strike.
188185
- "Tests pass." → "`npm test` exited 0," or strike.
189186
- "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<T, E>` return pattern, same `assert`-style guards," or strike.
190187
- "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."
191188

192189
## Reviewer-of-Record Checks
193190

194-
"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.
191+
"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.
195192

196193
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.
197194

198195
**Reviewer-of-record must run, in-session, and read the raw output:**
199196

200197
- `git diff <base>...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.
201198
- `git log --oneline <base>..HEAD` — confirm the commits on the branch match the issue's scope; unexpected or off-topic commits are stop conditions.
202-
- 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.
199+
- 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.
203200

204-
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.
201+
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.
205202

206203
## Commit-Message Coherence
207204

@@ -227,7 +224,7 @@ surprised by what the diff actually contains, that is a problem worth raising.
227224

228225
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.
229226

230-
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.
227+
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.
231228

232229
**Subject-line audits.** Most checks scan the output of:
233230

@@ -274,123 +271,14 @@ Affirmative claims about these audits must cite the specific command whose outpu
274271

275272
## Review Checklist
276273

277-
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.
274+
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.
278275

279276
1. Determine the base branch using the methods in "Base Branch Determination"
280-
2. Run `git log --oneline <base>..HEAD` to understand the scope _(reviewer-of-record)_
281-
3. Run `git diff <base>...HEAD --stat` to see which files changed _(reviewer-of-record)_
277+
2. Run `git log --oneline <base>..HEAD` to understand the scope *(reviewer-of-record)*
278+
3. Run `git diff <base>...HEAD --stat` to see which files changed *(reviewer-of-record)*
282279
4. Review each changed file, focusing only on lines modified by the branch
283280
5. For every commit on the branch, verify:
284-
- Diff matches the commit message (see "Commit-Message Coherence")
285-
- Subject and body pass the style audit (see "Commit-Message Style Audit") _(reviewer-of-record)_
281+
- Diff matches the commit message (see "Commit-Message Coherence")
282+
- Subject and body pass the style audit (see "Commit-Message Style Audit") *(reviewer-of-record)*
286283
6. Check that new code follows project conventions
287284
7. Summarize findings with specific file:line references; cite the check behind any affirmative claim (see "Cite the Check")
288-
8. When the review targets an open GitHub PR, post the review on the PR (see "Post the Review on GitHub")
289-
290-
## Post the Review on GitHub
291-
292-
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.
293-
294-
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.
295-
296-
### When to post
297-
298-
Post when any of these is true:
299-
300-
- The user asked for a PR review (`pull-request-review`, a PR URL, or an explicit "review #N")
301-
- `linear-issue-workflow` Phase 5 self-review has cleared and Phase 6 is opening or updating the PR
302-
- An open PR exists for the branch and the review's purpose is to leave a record on it
303-
304-
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.
305-
306-
### Multi-persona reviews
307-
308-
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.
309-
310-
| Lens | What it owns | When to post |
311-
| ---------------------- | ------------------------------------------------------------------ | ----------------------------------------------------------------------------------------- |
312-
| Primary / orchestrator | Verdict on the branch as it stands; residual findings; waiver list | Always when posting |
313-
| Critic | Behavioral bugs, missing tests, architecture, commit coherence | When a critic director ran |
314-
| Greybeard | Waiver rulings and intentional exceptions | When Greybeard authorized any waiver, or when product/architecture judgment was requested |
315-
| OSS / quality | Public-API, packaging, polish bar for shippable surface | When that lens was explicitly run |
316-
317-
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.
318-
319-
If only the primary review ran, post once. Do not invent extra personas.
320-
321-
### Command
322-
323-
Use a real GitHub review, not a floating issue comment, for the primary verdict:
324-
325-
```bash
326-
# Approve — branch is ready to merge as it stands
327-
gh pr review <number-or-url> --approve --body "$(cat <<'EOF'
328-
<body>
329-
EOF
330-
)"
331-
332-
# Comment — findings or notes that do not block merge by themselves
333-
gh pr review <number-or-url> --comment --body "$(cat <<'EOF'
334-
<body>
335-
EOF
336-
)"
337-
338-
# Request changes — at least one finding must be fixed before merge
339-
gh pr review <number-or-url> --request-changes --body "$(cat <<'EOF'
340-
<body>
341-
EOF
342-
)"
343-
```
344-
345-
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.
346-
347-
### Body shape (no AI slop)
348-
349-
Write like a senior engineer leaving a review on a busy PR. Short. Specific. Present tense. No journey narration.
350-
351-
**Required shape:**
352-
353-
```markdown
354-
## <Lens> · <Approve | Comment | Request changes>
355-
356-
<one line: what the branch does, present tense — not how it got there>
357-
358-
### Findings
359-
360-
- `path/to/file.ts:12` — <concrete problem and why it matters>
361-
- `path/to/other.ts:40` — <…>
362-
363-
### Notes
364-
365-
- <optional; only load-bearing context the author needs>
366-
```
367-
368-
When the review is clean:
369-
370-
```markdown
371-
## Review · Approve
372-
373-
Hub list/get/upload under `/api/tenants/:id/artifacts`; Library reads that plane.
374-
375-
No findings.
376-
```
377-
378-
**Hard bans** (delete on sight before posting):
379-
380-
- Throat-clearing: "Great work", "Thanks for this", "Overall this looks solid", "Happy to approve"
381-
- AI filler: "I'd like to highlight", "It's worth noting", "In conclusion", "Going forward"
382-
- Journey talk: "this PR adds… then fixes… after feedback…"
383-
- Fake balance: praising three things to soften one finding
384-
- Emoji, decorative headers, horizontal rules used as ornament
385-
- Restating the diff file-by-file when there is no finding
386-
- "LGTM" alone with no one-line present-tense description of what the branch does
387-
388-
**Findings rules:**
389-
390-
- Every finding has a `path:line` (or `path` when line is meaningless) and a concrete failure mode
391-
- Severity is the review action (`--request-changes` vs `--comment`), not adjectives in the body
392-
- 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)
393-
394-
### After posting
395-
396-
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.

0 commit comments

Comments
 (0)