Skip to content

Commit e42b490

Browse files
Split deep review from pull request surface review (#971)
* Split deep review from pull request surface review * Pin review interview gating in skills catalog test * Review skill recommends the fleet instead of routing it Skills do not route the fleet: classify-and-recommend, primary dispatches. Worktree checkout belongs to pull-request-review.
1 parent 862fdc8 commit e42b490

6 files changed

Lines changed: 103 additions & 10 deletions

File tree

CHANGELOG.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,18 @@ matching `## [X.Y.Z]` section (plus install instructions). Do not maintain
1111
parallel copies under `docs/` or `scripts/notes/`. At cut time: rename
1212
`## [Unreleased]` to `## [X.Y.Z] - YYYY-MM-DD`, then run the release script.
1313

14+
## [Unreleased]
15+
16+
### Changed
17+
18+
- `/review` now classifies the target first (diff vs topic, interview
19+
only when the object or base is genuinely missing), then dispatches a
20+
selected fleet: Critic always, Greybeard on architecture/API/approach,
21+
other lenses only when the files warrant them, one target per wave.
22+
`/pull-request-review` keeps the worktree checkout plus a surface pass
23+
(Critic on the diff plus at most one extra lens), loading `/review`
24+
for quality rules only.
25+
1426
## [0.3.24] - 2026-09-13
1527

1628
### Added

docs/ARCHITECTURE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -431,7 +431,7 @@ Corbits Code **ships a bundled catalog** as the first-party data-only plugin `pl
431431

432432
`discoverRepoPlugins` locates `plugins/` next to the source root, at `dist/plugins`, or at `dirname(execPath)/plugins`. It never scans the session cwd for the bundled catalog.
433433

434-
Primary is Skywalker. Bundled skill bodies are **how-to playbooks** (steps, done-definition) — not director personas and not fleet routers. Identity and who-does-what live on director system prompts. Default slashes: `/implement`, `/plan`, `/refactor`, `/review`, `/pull-request-review`, `/create-issue`, `/scribe`, `/interview`, `/ast-grep`, `/lexicon`. `/review` is how to review a branch (base, scope, signal); `/pull-request-review` is worktree checkout then the review skill; `/scribe` is how to maintain PRODUCT / ARCHITECTURE / IMPLEMENTATION; `/implement` is the per-commit greybeard → implement → gate → critic loop and does not steal planning from `/plan`; substantial Builder work consumes a counsel / `/plan` plan first; `/plan` authors an eng change plan and does not implement or file tickets; `/create-issue` remains the tracker command — Linear MCP when available, otherwise `ask_operator` for the platform and persists `Preferred issue tracker` in `.corbits/MEMORY.md`; `/lexicon` owns director-prompt drift and size against the agents repo at a pinned commit. The less-is-more quality bar (`idiot-proof`) is baked into builder and critic; it is not a slash. There is no first-party dispatch skill — Skywalker orchestrates natively. Draper and emil are closed directors via `spawn_agent(agent=…)`, not slashes. There is no catch-all worker. The operator types the slash; the primary follows the playbook.
434+
Primary is Skywalker. Bundled skill bodies are **how-to playbooks** (steps, done-definition) — not director personas and not fleet routers. Identity and who-does-what live on director system prompts. Default slashes: `/implement`, `/plan`, `/refactor`, `/review`, `/pull-request-review`, `/create-issue`, `/scribe`, `/interview`, `/ast-grep`, `/lexicon`. `/review` classifies the target first, then dispatches a selected fleet; `/pull-request-review` is worktree checkout plus a surface pass, loading `/review` for quality rules only; `/scribe` is how to maintain PRODUCT / ARCHITECTURE / IMPLEMENTATION; `/implement` is the per-commit greybeard → implement → gate → critic loop and does not steal planning from `/plan`; substantial Builder work consumes a counsel / `/plan` plan first; `/plan` authors an eng change plan and does not implement or file tickets; `/create-issue` remains the tracker command — Linear MCP when available, otherwise `ask_operator` for the platform and persists `Preferred issue tracker` in `.corbits/MEMORY.md`; `/lexicon` owns director-prompt drift and size against the agents repo at a pinned commit. The less-is-more quality bar (`idiot-proof`) is baked into builder and critic; it is not a slash. There is no first-party dispatch skill — Skywalker orchestrates natively. Draper and emil are closed directors via `spawn_agent(agent=…)`, not slashes. There is no catch-all worker. The operator types the slash; the primary follows the playbook.
435435

436436
#### Discovery and precedence
437437

docs/PRODUCT.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ the file path and parse details.
107107

108108
The TUI has an extensible slash-command framework. Built-ins include `/help` (shortcut + command overlay), `/model` (models-only picker for connected accounts; **Alt+A** or `/connect` adds a provider), `/settings`, `/permissions`, `/plugins`, `/clear`, `/new`, `/mcp` (enable, disable, or remove servers), and `/yolo` (persists as the user-global skip-permissions default; `--dangerously-skip-permissions` still forces this process; secret-guard and authz still apply; `/yolo [on|off|toggle]`, bare `/yolo` toggles), plus a `/<name>` command per available workflow. When a session starts with the persisted default already on, the TUI shows a startup notice ("Permission prompts are disabled by your saved default…") so the silent machine-wide default is never invisible; `corbits exec` prints the equivalent warning to stderr. Plugins can register additional commands.
109109

110-
**Default skills** exist out of the gate as first-party slash **actions**, not director names: `/implement`, `/plan`, `/refactor`, `/review`, `/pull-request-review`, `/create-issue`, `/scribe`, `/interview`, `/ast-grep`, `/lexicon`. Each one is a how-to playbook — the slash sends the skill body to the primary, which follows the steps. Skills do not assign identity or route the fleet; that stays on director system prompts. `/review` is how to review a branch; `/scribe` is how to maintain PRODUCT / ARCHITECTURE / IMPLEMENTATION; `/implement` is the per-commit review/build/critique loop — it does not steal planning from `/plan`. Substantial Builder work consumes a counsel / `/plan` plan first; tiny parent-DIY stays plan-optional. `/plan` authors an eng change plan (files, AC, non-goals, risks, ordered steps) and does not implement. `/create-issue` remains the tracker command: Linear MCP when available; otherwise it `ask_operator`s for the platform (GitHub etc.) and persists `Preferred issue tracker` in `.corbits/MEMORY.md` (GitHub via `gh issue create`). `/lexicon` owns director-prompt drift and size against the agents repo at a pinned commit. There is no first-party dispatch skill — Skywalker orchestrates natively. `git-rebase`, `linear-issue-workflow`, `style`, `philosophy`, `native-integration`, `typescript`, `ponytail`, and `opsh` stay `use_skill` only (`user-invocable: false`). Bake-only bars such as `idiot-proof` and `native-runtime` are not slashes and are not listed for `use_skill`. Draper and emil are not slashes; they remain closed directors via `spawn_agent(agent=…)`. There is no catch-all worker. Slash names are also available to the model via `skill_search` (descriptions) then `use_skill` (body). Disable the catalog in `/plugins` (`corbits-skills`) if you want them gone.
110+
**Default skills** exist out of the gate as first-party slash **actions**, not director names: `/implement`, `/plan`, `/refactor`, `/review`, `/pull-request-review`, `/create-issue`, `/scribe`, `/interview`, `/ast-grep`, `/lexicon`. Each one is a how-to playbook — the slash sends the skill body to the primary, which follows the steps. Skills do not assign identity or route the fleet; that stays on director system prompts. `/review` classifies the target first, then dispatches a selected fleet; `/pull-request-review` is worktree checkout plus a surface pass, loading `/review` for quality rules only; `/scribe` is how to maintain PRODUCT / ARCHITECTURE / IMPLEMENTATION; `/implement` is the per-commit review/build/critique loop — it does not steal planning from `/plan`. Substantial Builder work consumes a counsel / `/plan` plan first; tiny parent-DIY stays plan-optional. `/plan` authors an eng change plan (files, AC, non-goals, risks, ordered steps) and does not implement. `/create-issue` remains the tracker command: Linear MCP when available; otherwise it `ask_operator`s for the platform (GitHub etc.) and persists `Preferred issue tracker` in `.corbits/MEMORY.md` (GitHub via `gh issue create`). `/lexicon` owns director-prompt drift and size against the agents repo at a pinned commit. There is no first-party dispatch skill — Skywalker orchestrates natively. `git-rebase`, `linear-issue-workflow`, `style`, `philosophy`, `native-integration`, `typescript`, `ponytail`, and `opsh` stay `use_skill` only (`user-invocable: false`). Bake-only bars such as `idiot-proof` and `native-runtime` are not slashes and are not listed for `use_skill`. Draper and emil are not slashes; they remain closed directors via `spawn_agent(agent=…)`. There is no catch-all worker. Slash names are also available to the model via `skill_search` (descriptions) then `use_skill` (body). Disable the catalog in `/plugins` (`corbits-skills`) if you want them gone.
111111

112112
Providers are **models-first**: there is no standalone `/login` command. `/model` opens a **models-only list** (Recent, Favorites, then connected provider/model rows) — type-to-filter owns printable keys, so Connect is never a bare letter. **Alt+A** or `/connect` opens a dedicated add-provider selector over every first-class kind (OpenAI dual-path ChatGPT OAuth or API key, xAI, OpenCode Zen, Anthropic, Google, OpenCode Go, Z.AI Coding Plan, Ollama, Custom), each annotated with its live account count and never filtered out for “already connected.” **Alt+F** toggles favorite on the highlighted model. **Alt+D** persists the highlighted pair as the default without switching the live session. Advanced provider drill-down (edit/delete/tiers) stays on the advanced surface, not a bare printable key while the model list is filtering. OAuth providers open their existing browser login with a named account step so multiple accounts per kind coexist (`codex/work`, …). API-key providers use the same named-instance step before the key (auth-only form: instance name + key + fixed catalog base URL), so personal and team keys land as distinct catalog rows (`openai/default`, `anthropic/work`, …); reusing a name re-keys that instance after confirm. Custom remains a free-form single endpoint (full manual form). Successful connect refreshes the catalog and reopens the model list focused on the new account’s default model. OpenCode Go lists models from the live `/zen/go/v1/models` catalog (packaged seed on fetch failure), routes each by its protocol metadata (chat completions, OpenAI responses, or Anthropic messages) and can show subscription usage in the status bar when active (rolling 5h / weekly / monthly windows when the usage API responds; omitted on auth or network failure). When Go returns a quota or rate-limit error — including some HTTP 400 responses that carry limit payloads — Corbits classifies them so quota aborts cleanly and short provider rate limits remain retryable. On a free-tier or subscription quota hit, wait for the window to reset or use OpenCode Zen free models.
113113

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

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -126,16 +126,29 @@ glab mr view --output json | jq -r '.target_branch'
126126
git branch -r | grep -E 'origin/(main|master)$' | head -1 | sed 's/.*origin\///'
127127
```
128128

129-
### Step 8: Load Code Review Skill
129+
### Step 8: Load Review Quality Rules Only
130130

131-
Load and follow the `code-review` skill to perform the actual review. The code-review skill provides guidance on:
131+
Load the `review` skill for quality rules only — signal over noise,
132+
cite the check, comment tone and body. Skip its classify and fleet
133+
dispatch: this skill already owns the target (this PR) and the fleet
134+
(the surface pass below).
132135

133-
- Scope determination using git diff
134-
- Handling pre-existing code
135-
- Convention compliance
136-
- Test coverage philosophy
137-
- Signal over noise (avoiding unactionable findings)
138-
- Review checklist
136+
### Step 9: Surface Pass
137+
138+
Run a surface pass on the diff: Critic on the diff plus at most one
139+
extra lens. Do not dispatch a wider fleet.
140+
141+
### Step 10: Post the Review on GitHub
142+
143+
Post the findings on the pull request:
144+
145+
```bash
146+
# For GitHub PRs
147+
gh pr review <number> --comment --body "<review body>"
148+
149+
# Or leave a plain comment
150+
gh pr comment <number> --body "<review body>"
151+
```
139152

140153
## Cleanup
141154

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,29 @@ description: Perform a code review or pull request review on a branch
77

88
Use this skill when performing code reviews or pull request reviews.
99

10+
## Classify, Then Recommend a Selected Fleet
11+
12+
First classify the review target, then recommend only the fleet the
13+
target warrants. Do not fan out a default wide fleet. This skill does
14+
not route the fleet — the primary (Skywalker orchestrator) dispatches;
15+
the classification below tells it which lenses the target warrants.
16+
17+
Classify the review target as one of:
18+
19+
- Diff: a branch, PR, path, or post-PR work.
20+
- Topic: an architecture or approach question.
21+
- Interview: only when the review object or base is genuinely missing.
22+
Never run interview as ritual.
23+
24+
Then the primary dispatches the warranted lenses with `spawn_agent`, one target per wave:
25+
26+
- Critic always.
27+
- Greybeard when architecture, API, or approach is at stake.
28+
- Draper, Emil, Gaasbot, Bruckheimer, or Neckbeard only when the
29+
touched files warrant that lens.
30+
31+
When the target is a PR, read the PR tree from the worktree — worktree checkout belongs to `/pull-request-review`; never review the local checkout as a stand-in for the PR.
32+
1033
## Base Branch Determination
1134

1235
Before reviewing, you must determine the correct base branch. Use these methods

tests/unit/corbits-skills-catalog.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,51 @@ test("only background and bake-only skills carry disable-model-invocation", asyn
181181
}
182182
});
183183

184+
test("review skill is the classify-then-selected-fleet recipe", async () => {
185+
const skill = await Bun.file(
186+
join(pluginRoot, "skills/review/SKILL.md"),
187+
).text();
188+
expect(skill).toContain("Classify the review target");
189+
expect(skill).toContain("Critic always");
190+
expect(skill).toContain("Greybeard");
191+
expect(skill).toContain("one target per wave");
192+
expect(skill).toContain("read the PR tree");
193+
expect(skill).not.toContain("deep-agent-review");
194+
});
195+
196+
test("review skill recommends the fleet but does not route it or own the worktree", async () => {
197+
const skill = await Bun.file(
198+
join(pluginRoot, "skills/review/SKILL.md"),
199+
).text();
200+
expect(skill).toContain("does\nnot route the fleet");
201+
expect(skill).toContain("the primary (Skywalker orchestrator) dispatches");
202+
expect(skill).toContain(
203+
"worktree checkout belongs to `/pull-request-review`",
204+
);
205+
});
206+
207+
test("review skill gates interview as exception, never ritual", async () => {
208+
const skill = await Bun.file(
209+
join(pluginRoot, "skills/review/SKILL.md"),
210+
).text();
211+
expect(skill).toContain("Never run interview as ritual");
212+
});
213+
214+
test("pull-request-review is the worktree surface pass", async () => {
215+
const skill = await Bun.file(
216+
join(pluginRoot, "skills/pull-request-review/SKILL.md"),
217+
).text();
218+
expect(skill).toContain("worktree");
219+
expect(skill).toContain("quality rules only");
220+
expect(skill).toContain("at most one");
221+
expect(skill).toContain("Post the Review on GitHub");
222+
expect(skill).not.toContain("Classify the review target");
223+
});
224+
225+
test("no third review slash exists", () => {
226+
expect(existsSync(join(pluginRoot, "skills/deep-agent-review"))).toBe(false);
227+
});
228+
184229
test("review skill does not own GitHub posting or Linear In Review", async () => {
185230
const skill = await Bun.file(
186231
join(pluginRoot, "skills/review/SKILL.md"),

0 commit comments

Comments
 (0)