Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -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/
Expand Down
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions plugins/corbits-skills/skills/native-integration/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
154 changes: 21 additions & 133 deletions plugins/corbits-skills/skills/review/SKILL.md
Original file line number Diff line number Diff line change
@@ -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.

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 <base-branch>...HEAD -- <file>` 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.

Expand Down Expand Up @@ -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
Expand All @@ -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 <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.

Expand All @@ -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.

Expand All @@ -184,24 +181,24 @@ 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<T, E>` 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.

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

- `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.
- `git log --oneline <base>..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

Expand All @@ -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:

Expand Down Expand Up @@ -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 <base>..HEAD` to understand the scope _(reviewer-of-record)_
3. Run `git diff <base>...HEAD --stat` to see which files changed _(reviewer-of-record)_
2. Run `git log --oneline <base>..HEAD` to understand the scope *(reviewer-of-record)*
3. Run `git diff <base>...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 <number-or-url> --approve --body "$(cat <<'EOF'
<body>
EOF
)"

# Comment — findings or notes that do not block merge by themselves
gh pr review <number-or-url> --comment --body "$(cat <<'EOF'
<body>
EOF
)"

# Request changes — at least one finding must be fixed before merge
gh pr review <number-or-url> --request-changes --body "$(cat <<'EOF'
<body>
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
## <Lens> · <Approve | Comment | Request changes>

<one line: what the branch does, present tense — not how it got there>

### Findings

- `path/to/file.ts:12` — <concrete problem and why it matters>
- `path/to/other.ts:40` — <…>

### Notes

- <optional; only load-bearing context the author needs>
```

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.
Loading
Loading