From c231c5239625d664ccfbe3bc6bc3c00552415ca8 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 12 Sep 2026 20:10:37 -0700 Subject: [PATCH 1/3] Split deep review from pull request surface review --- CHANGELOG.md | 12 ++++++++ docs/ARCHITECTURE.md | 2 +- docs/PRODUCT.md | 2 +- .../skills/pull-request-review/SKILL.md | 29 ++++++++++++++----- plugins/corbits-skills/skills/review/SKILL.md | 22 ++++++++++++++ tests/unit/corbits-skills-catalog.test.ts | 27 +++++++++++++++++ 6 files changed, 84 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fc68895f..517c279e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,18 @@ matching `## [X.Y.Z]` section (plus install instructions). Do not maintain parallel copies under `docs/` or `scripts/notes/`. At cut time: rename `## [Unreleased]` to `## [X.Y.Z] - YYYY-MM-DD`, then run the release script. +## [Unreleased] + +### Changed + +- `/review` now classifies the target first (diff vs topic, interview + only when the object or base is genuinely missing), then dispatches a + selected fleet: Critic always, Greybeard on architecture/API/approach, + other lenses only when the files warrant them, one target per wave. + `/pull-request-review` keeps the worktree checkout plus a surface pass + (Critic on the diff plus at most one extra lens), loading `/review` + for quality rules only. + ## [0.3.24] - 2026-09-13 ### Added diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index dd4f62168..bdb46e280 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -431,7 +431,7 @@ Corbits Code **ships a bundled catalog** as the first-party data-only plugin `pl `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. -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. +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. #### Discovery and precedence diff --git a/docs/PRODUCT.md b/docs/PRODUCT.md index ec86ce205..51bc6afd9 100644 --- a/docs/PRODUCT.md +++ b/docs/PRODUCT.md @@ -107,7 +107,7 @@ the file path and parse details. 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 `/` 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. -**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. +**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. 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. diff --git a/plugins/corbits-skills/skills/pull-request-review/SKILL.md b/plugins/corbits-skills/skills/pull-request-review/SKILL.md index f2178f8b0..ab1058b84 100644 --- a/plugins/corbits-skills/skills/pull-request-review/SKILL.md +++ b/plugins/corbits-skills/skills/pull-request-review/SKILL.md @@ -126,16 +126,29 @@ glab mr view --output json | jq -r '.target_branch' git branch -r | grep -E 'origin/(main|master)$' | head -1 | sed 's/.*origin\///' ``` -### Step 8: Load Code Review Skill +### Step 8: Load Review Quality Rules Only -Load and follow the `code-review` skill to perform the actual review. The code-review skill provides guidance on: +Load the `review` skill for quality rules only — signal over noise, +cite the check, comment tone and body. Skip its classify and fleet +dispatch: this skill already owns the target (this PR) and the fleet +(the surface pass below). -- Scope determination using git diff -- Handling pre-existing code -- Convention compliance -- Test coverage philosophy -- Signal over noise (avoiding unactionable findings) -- Review checklist +### Step 9: Surface Pass + +Run a surface pass on the diff: Critic on the diff plus at most one +extra lens. Do not dispatch a wider fleet. + +### Step 10: Post the Review on GitHub + +Post the findings on the pull request: + +```bash +# For GitHub PRs +gh pr review --comment --body "" + +# Or leave a plain comment +gh pr comment --body "" +``` ## Cleanup diff --git a/plugins/corbits-skills/skills/review/SKILL.md b/plugins/corbits-skills/skills/review/SKILL.md index d285e1bec..c3400fc79 100644 --- a/plugins/corbits-skills/skills/review/SKILL.md +++ b/plugins/corbits-skills/skills/review/SKILL.md @@ -7,6 +7,28 @@ description: Perform a code review or pull request review on a branch Use this skill when performing code reviews or pull request reviews. +## Classify, Then Dispatch a Selected Fleet + +First classify the review target, then dispatch only the fleet the +target warrants. Do not fan out a default wide fleet. + +Classify the review target as one of: + +- Diff: a branch, PR, path, or post-PR work. +- Topic: an architecture or approach question. +- Interview: only when the review object or base is genuinely missing. + Never run interview as ritual. + +Then dispatch a selected fleet with `spawn_agent`, one target per wave: + +- Critic always. +- Greybeard when architecture, API, or approach is at stake. +- Draper, Emil, Gaasbot, Bruckheimer, or Neckbeard only when the + touched files warrant that lens. + +When the target is a PR, read the PR tree (fetch the PR branch into a +worktree) — never review the local checkout as a stand-in for the PR. + ## Base Branch Determination Before reviewing, you must determine the correct base branch. Use these methods diff --git a/tests/unit/corbits-skills-catalog.test.ts b/tests/unit/corbits-skills-catalog.test.ts index dd1476bf6..a8c3b2757 100644 --- a/tests/unit/corbits-skills-catalog.test.ts +++ b/tests/unit/corbits-skills-catalog.test.ts @@ -181,6 +181,33 @@ test("only background and bake-only skills carry disable-model-invocation", asyn } }); +test("review skill is the classify-then-selected-fleet recipe", async () => { + const skill = await Bun.file( + join(pluginRoot, "skills/review/SKILL.md"), + ).text(); + expect(skill).toContain("Classify the review target"); + expect(skill).toContain("Critic always"); + expect(skill).toContain("Greybeard"); + expect(skill).toContain("one target per wave"); + expect(skill).toContain("read the PR tree"); + expect(skill).not.toContain("deep-agent-review"); +}); + +test("pull-request-review is the worktree surface pass", async () => { + const skill = await Bun.file( + join(pluginRoot, "skills/pull-request-review/SKILL.md"), + ).text(); + expect(skill).toContain("worktree"); + expect(skill).toContain("quality rules only"); + expect(skill).toContain("at most one"); + expect(skill).toContain("Post the Review on GitHub"); + expect(skill).not.toContain("Classify the review target"); +}); + +test("no third review slash exists", () => { + expect(existsSync(join(pluginRoot, "skills/deep-agent-review"))).toBe(false); +}); + test("review skill does not own GitHub posting or Linear In Review", async () => { const skill = await Bun.file( join(pluginRoot, "skills/review/SKILL.md"), From 573a3146695a840b0d25565eb962f8cae34887e1 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 12 Sep 2026 20:12:24 -0700 Subject: [PATCH 2/3] Pin review interview gating in skills catalog test --- tests/unit/corbits-skills-catalog.test.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/unit/corbits-skills-catalog.test.ts b/tests/unit/corbits-skills-catalog.test.ts index a8c3b2757..bfec56b30 100644 --- a/tests/unit/corbits-skills-catalog.test.ts +++ b/tests/unit/corbits-skills-catalog.test.ts @@ -193,6 +193,13 @@ test("review skill is the classify-then-selected-fleet recipe", async () => { expect(skill).not.toContain("deep-agent-review"); }); +test("review skill gates interview as exception, never ritual", async () => { + const skill = await Bun.file( + join(pluginRoot, "skills/review/SKILL.md"), + ).text(); + expect(skill).toContain("Never run interview as ritual"); +}); + test("pull-request-review is the worktree surface pass", async () => { const skill = await Bun.file( join(pluginRoot, "skills/pull-request-review/SKILL.md"), From 049ee93c12f5eb628c23f4ea2324b604ad806a91 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 12 Sep 2026 21:59:34 -0700 Subject: [PATCH 3/3] 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. --- plugins/corbits-skills/skills/review/SKILL.md | 13 +++++++------ tests/unit/corbits-skills-catalog.test.ts | 11 +++++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/plugins/corbits-skills/skills/review/SKILL.md b/plugins/corbits-skills/skills/review/SKILL.md index c3400fc79..14cfa51dd 100644 --- a/plugins/corbits-skills/skills/review/SKILL.md +++ b/plugins/corbits-skills/skills/review/SKILL.md @@ -7,10 +7,12 @@ description: Perform a code review or pull request review on a branch Use this skill when performing code reviews or pull request reviews. -## Classify, Then Dispatch a Selected Fleet +## Classify, Then Recommend a Selected Fleet -First classify the review target, then dispatch only the fleet the -target warrants. Do not fan out a default wide fleet. +First classify the review target, then recommend only the fleet the +target warrants. Do not fan out a default wide fleet. This skill does +not route the fleet — the primary (Skywalker orchestrator) dispatches; +the classification below tells it which lenses the target warrants. Classify the review target as one of: @@ -19,15 +21,14 @@ Classify the review target as one of: - Interview: only when the review object or base is genuinely missing. Never run interview as ritual. -Then dispatch a selected fleet with `spawn_agent`, one target per wave: +Then the primary dispatches the warranted lenses with `spawn_agent`, one target per wave: - Critic always. - Greybeard when architecture, API, or approach is at stake. - Draper, Emil, Gaasbot, Bruckheimer, or Neckbeard only when the touched files warrant that lens. -When the target is a PR, read the PR tree (fetch the PR branch into a -worktree) — never review the local checkout as a stand-in for the PR. +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. ## Base Branch Determination diff --git a/tests/unit/corbits-skills-catalog.test.ts b/tests/unit/corbits-skills-catalog.test.ts index bfec56b30..2787ab76b 100644 --- a/tests/unit/corbits-skills-catalog.test.ts +++ b/tests/unit/corbits-skills-catalog.test.ts @@ -193,6 +193,17 @@ test("review skill is the classify-then-selected-fleet recipe", async () => { expect(skill).not.toContain("deep-agent-review"); }); +test("review skill recommends the fleet but does not route it or own the worktree", async () => { + const skill = await Bun.file( + join(pluginRoot, "skills/review/SKILL.md"), + ).text(); + expect(skill).toContain("does\nnot route the fleet"); + expect(skill).toContain("the primary (Skywalker orchestrator) dispatches"); + expect(skill).toContain( + "worktree checkout belongs to `/pull-request-review`", + ); +}); + test("review skill gates interview as exception, never ritual", async () => { const skill = await Bun.file( join(pluginRoot, "skills/review/SKILL.md"),