From 3ae50a4f4bbd948241ac264f7f93986c6e1a79f5 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 30 Sep 2026 13:22:55 -0700 Subject: [PATCH] refactor(skills): fold the ship loop and integration rules into implement and review --- AGENTS.md | 2 +- docs/ARCHITECTURE.md | 2 +- docs/PLUGINS.md | 3 +- docs/PRODUCT.md | 2 +- .../corbits-skills/skills/implement/SKILL.md | 177 ++--------- .../skills/linear-issue-workflow/SKILL.md | 288 ------------------ .../skills/native-integration/SKILL.md | 146 --------- plugins/corbits-skills/skills/review/SKILL.md | 30 ++ src/telemetry/classify.ts | 1 - src/telemetry/product-events.test.ts | 1 - 10 files changed, 59 insertions(+), 593 deletions(-) delete mode 100644 plugins/corbits-skills/skills/linear-issue-workflow/SKILL.md delete mode 100644 plugins/corbits-skills/skills/native-integration/SKILL.md diff --git a/AGENTS.md b/AGENTS.md index 1d1f724a8..e81d80f86 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,7 +4,7 @@ ## Before You Start -Load the `style`, `philosophy`, and `native-integration` skills. Confirm working-tree status (`git status`) and run `git log --oneline -5`. When the task touches the agent loop, directors, tools, or prompts, read the relevant doc in `/docs` before writing code. +Load the `style` and `philosophy` skills. Confirm working-tree status (`git status`) and run `git log --oneline -5`. When the task touches the agent loop, directors, tools, or prompts, read the relevant doc in `/docs` before writing code. New contributors: configure git hooks and verify the environment before the first commit. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 8fb353472..4c99244fd 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -468,7 +468,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`, `/issue`, `/docs`, `/interview`. `/review` classifies the target first, then dispatches a selected fleet; `/pull-request` finds or opens the PR for the current branch, and `/review` takes a PR target and reviews it from a worktree; `/docs` 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; `/issue` finds or creates the tracker issue — Linear MCP when available, otherwise `ask_operator` for the platform and persists `Preferred issue tracker` in `.corbits/MEMORY.md` 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`, `/issue`, `/docs`, `/interview`. `/review` classifies the target first, then dispatches a selected fleet; `/pull-request` finds or opens the PR for the current branch, and `/review` takes a PR target and reviews it from a worktree; `/docs` is how to maintain PRODUCT / ARCHITECTURE / IMPLEMENTATION; `/implement` takes a plan and a ticket to a pushed branch (worktree, per-commit planner → implement → gate → reviewer loop, whole-branch review, push) 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; `/issue` finds or creates the tracker issue — Linear MCP when available, otherwise `ask_operator` for the platform and persists `Preferred issue tracker` in `.corbits/MEMORY.md` 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/PLUGINS.md b/docs/PLUGINS.md index 1bf7acfa1..dc0c5227e 100644 --- a/docs/PLUGINS.md +++ b/docs/PLUGINS.md @@ -331,8 +331,7 @@ shape. `resolveSkillBody`), so the model does not auto-suggest background libraries. First-party recipes that are not operator slashes remain listed for `skill_search` / `use_skill` when they only set `user-invocable: false` - (`linear-issue-workflow`, `style`, `philosophy`, `native-integration`, - `typescript`). Background libs such as `git-worktrees` set both flags: not a + (`style`, `philosophy`, `typescript`). Background libs such as `git-worktrees` set both flags: not a slash and not listed by `skill_search`/`discoverSkills`, but still loadable by explicit `use_skill`/`resolveSkillBody` name. The slash command is a direct user entry point on top. diff --git a/docs/PRODUCT.md b/docs/PRODUCT.md index 0bbfb1530..f1e9a23b6 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`, `/compact` (fold conversation context now, optional trailing instructions to the summarizer; does not wait for the 60% occupancy governor; idle success shows the fold and does not start a new turn), `/mcp` (enable, disable, or remove servers), `/handoff [optional instructions]` (folds context through the shared operator pipeline, then immediately starts the next turn with the instructions as the inbound content — default copy when omitted; unlike `/compact`, which stops after the fold, handoff always re-infers, so the operator can pivot goals without `/clear`; a handoff issued mid-tool-batch queues behind the in-flight batch and whichever boundary fires first runs the single fold), and `/yolo` (`/yolo [on|off|toggle]`, bare `/yolo` toggles), plus a `/` command per available workflow. `/yolo` persists skip-permissions to the active settings file. That file is the user-global `~/.corbits/settings.json` by default, making the setting machine-wide; explicit `--config ` selects a different active file, and `/yolo` writes that file. An ordinary TUI launch without the same `--config` returns to the user-global source and does not modify the custom file. `--dangerously-skip-permissions` and `--yolo` are process-only aliases. Secret-guard and authz still apply. When a session starts with its active persisted setting already on, the TUI and `corbits exec` warn that permission prompts are disabled by saved settings at the active settings path and direct the operator to edit that file to re-enable them. 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`, `/issue`, `/docs`, `/interview`. 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` finds or opens the PR for the current branch, and `/review` takes a PR target and reviews it from a worktree; `/docs` 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. `/issue` finds or creates the tracker issue: 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`). There is no first-party dispatch skill — Skywalker orchestrates natively. `linear-issue-workflow`, `style`, `philosophy`, `native-integration`, and `typescript` stay `use_skill` only (`user-invocable: false`); `git-worktrees` is a background library that is 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`, `/issue`, `/docs`, `/interview`. 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` finds or opens the PR for the current branch, and `/review` takes a PR target and reviews it from a worktree; `/docs` is how to maintain PRODUCT / ARCHITECTURE / IMPLEMENTATION; `/implement` takes a plan and a ticket to a pushed branch (worktree, per-commit planner/build/reviewer loop, whole-branch review, push, hand off to `/pull-request`) — 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. `/issue` finds or creates the tracker issue: 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`). There is no first-party dispatch skill — Skywalker orchestrates natively. `style`, `philosophy`, and `typescript` stay `use_skill` only (`user-invocable: false`); `git-worktrees` is a background library that is 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/implement/SKILL.md b/plugins/corbits-skills/skills/implement/SKILL.md index 1ad6a1527..e08680824 100644 --- a/plugins/corbits-skills/skills/implement/SKILL.md +++ b/plugins/corbits-skills/skills/implement/SKILL.md @@ -1,174 +1,47 @@ --- name: implement -description: Per-commit workflow with planner review, build gate, and reviewer loop. +description: Ship a planned, ticketed change from worktree to pushed branch with per-commit review. +argument-hint: "[ticket-id]" --- # Implement -A disciplined implementation workflow that produces reviewed, verified commits. Load this skill when you want each commit to go through architectural review, build verification, and code review before it lands. +Take a plan and a ticket to a pushed branch. Each commit is built, gated, and reviewed before the next one starts. `/implement` does not plan; `/plan` and `/issue` come first. -## Preflight +## 1. Preflight -Requires a git repo. Run `git rev-parse --show-toplevel`; if it fails, stop and tell the operator to `git init` first. Work in a worktree from `origin/`, never in the main checkout: `use_skill("git-worktrees")` for the commands. +- Requires a git repo. Run `git rev-parse --show-toplevel`; if it fails, stop and tell the operator to `git init` first. +- Requires a plan (files, acceptance criteria, non-goals, risks, ordered steps) and a ticket. If either is missing, stop and point to `/plan` or `/issue`. A tiny change the primary does itself may skip the plan but not the ticket. +- When the ticket is in Linear and Linear MCP is mounted, set it to In Progress now. If MCP is missing, say the status could not be updated. -## Prerequisites +## 2. Worktree -Before using this workflow, load the `style` and `philosophy` skills. Follow their conventions throughout. +Work in a worktree from `origin/` (`use_skill("git-worktrees")`). The branch name comes from the ticket (for Linear, its `gitBranchName`). A stacked change bases on the previous branch. Do all work there, never in the main checkout. -## When to Use +Track the commit-sized units of the plan with `manage_tasks`. -This is a standalone skill, loaded on request. It is not part of dispatch. Use it when you want a single agent to work through a series of commits with review discipline. +## 3. Per commit -The caller defines what work to do and where the commit boundaries are. This skill defines _how_ each commit gets produced. +For each unit, in order: -## Tracking Progress +1. **Approach.** For anything non-trivial, describe the approach to `spawn_agent(agent="planner")`: what changes and why, files touched, trade-offs, uncertainties. Adjust for real problems; disagree only with a reason. +2. **Implement and test.** Follow the repo's test conventions. Bug fixes start with a test that fails for the right reason, then the fix. Features get a test that asserts the designed behavior. The test lands in the same commit as the code, and so do doc updates the change requires. Keep scope to the unit; note other work for a later commit. +3. **Build gate.** Run the project's full pipeline (format, lint, build, test), never a partial one. Fix failures you caused. Report pre-existing failures to the operator. Record the commands and exit statuses. +4. **Commit.** Follow `CONTRIBUTING.md` for the subject and body. No ticket ids or AI attribution in commit messages. +5. **Reviewer loop.** `spawn_agent(agent="reviewer")` on `git show HEAD` with the intent from step 1, limited to this commit's scope. Fix every verified or high-confidence finding, re-run the build gate, and amend (or `git rebase -i` with `edit` for an earlier commit). Repeat until clean or until what remains is a conscious decision, not an oversight. -Use `manage_tasks` to track progress throughout the workflow. It gives the user real-time visibility into what you're doing. +The build must pass before every commit, amend, and rebase stop. -### Initial Planning +## 4. Dispositions -Before starting implementation, use `manage_tasks` to add each commit-sized unit of work from the caller's instructions: +Every finding outside the current commit gets one: (a) fix here, (b) separate commit on this branch, (c) new issue filed this session with its id in the status update, or (d) accept as-is. "Out of scope" and "later" are not dispositions. Only the operator may choose (d). Decisions go through the primary, not a worker. -- **title**: Clear imperative description of the unit of work -- **description**: Enough context that you could pick it up cold -- **active form**: Present continuous form for the status line (e.g., "Refactoring HTTP client retry logic") +## 5. Whole-branch gate -### During the Per-Commit Workflow +When every unit has landed, run `/review` on `base..HEAD` (the whole branch, not the last commit). Fix every finding, or get an explicit operator waiver for it. -When you begin a unit of work, mark its task `in_progress` with `manage_tasks`. As you move through the workflow steps, update the task's active form to reflect which step you're in: +## 6. Push and hand off -- **Step 1**: "Reviewing approach with Planner: {subject}" -- **Step 2**: "Implementing: {subject}" -- **Step 3**: "Running build gate: {subject}" -- **Step 4**: "Committing: {subject}" -- **Step 5**: "Running Reviewer loop: {subject}" +Push the branch (`bin/git-push-scoped origin ` when the repo has it, else `git push -u origin `). Never force-push a published branch. Then run `/pull-request` to open the PR. When the PR is ready for review, the Linear ticket moves to In Review; never Done on PR open, and never left In Review after merge when work remains. -When the commit lands and Reviewer is clean, mark the task `completed`. - -### Discovered Work - -If new work surfaces during implementation (Planner suggests a preparatory refactor, Reviewer reveals a missing edge case that warrants its own commit), add a new task with `manage_tasks` and work it through the full per-commit workflow. - -## Workflow Per Commit - -For each logical unit of work that results in a commit, follow these steps in order. Do not skip steps. - -### Step 1: Planner Review - -Mark the task `in_progress` and set the active form to "Reviewing approach with Planner: {subject}". - -Before writing any code, describe your implementation approach to Planner and ask for feedback. - -**What to send Planner:** - -- What you're about to change and why -- Which files you expect to touch -- Any design decisions or trade-offs you're considering -- Anything you're uncertain about - -**How to handle feedback:** - -- If Planner identifies problems with your approach, adjust before proceeding -- If Planner suggests a fundamentally different approach, consider it seriously -- You don't need to agree with every suggestion, but you need a reason to disagree -- Once you're aligned on approach, move to Step 2 - -Use `spawn_agent(agent="planner")` for this step. - -### Step 2: Implement and Test - -Update the active form to "Implementing: {subject}". - -The order of operations depends on whether you're fixing a bug or building a feature. In both cases, follow the repository's existing test conventions — look at how existing tests are structured, where they live, what framework they use, and match that style. If the repository has no existing tests, ask the caller what test framework and conventions to use before proceeding. - -**For bug fixes (test-first):** - -1. Write a test that reproduces the bug. -2. Run the test and verify it **fails**. If it doesn't fail, you don't understand the bug well enough to fix it. Go back and refine the test until it demonstrates the broken behavior. -3. Implement the fix, following the approach reviewed in Step 1. -4. Run the test again and verify it **passes**. If it doesn't pass, your fix is incomplete. - -**For new features:** - -1. Implement the feature, following the approach reviewed in Step 1. -2. Write a test that exercises the new functionality and asserts on the expected behavior. The test should verify that the code works as designed and implemented, not just that it doesn't crash. -3. Run the test and verify it **passes**. - -Keep the test focused on the behavior introduced by this commit. Don't test unrelated functionality. The test is part of the deliverable, not an afterthought. - -The test lands in the same unit of work as the implementation (Step 4) — same commit when committing — one logical unit (source of truth: style skill, AGENTS.md). When the caller passes testsmith-designed cases, land them as the implementation tests; any case left unlanded goes in the report with why so the caller can route a tester run. - -Keep the scope tight to what was discussed. If you discover additional work is needed, finish the current commit's scope first and note the additional work for a future commit. When the landing alters documented behavior beyond the discussed doc scope, flag it for the caller so a shakespeare docs pass can follow. - -### Step 3: Build Gate - -Update the active form to "Running build gate: {subject}". - -Run `make` (or the project's equivalent full pipeline: format, lint, build, test). - -- If the build passes, move to Step 4 -- If the build fails due to your changes, fix the failures and re-run until it passes -- If the build fails due to pre-existing issues unrelated to your changes, report the failure to the caller and let them decide how to proceed -- Do not move forward with a broken build -- Do not substitute partial builds (e.g., running only the compiler) for the full pipeline -- Record the exact verification commands and their exit statuses: the report maps each success criterion to pass, fail, or blocked with command evidence - -### Step 4: Commit - -Update the active form to "Committing: {subject}". - -Create the commit. Follow the commit message conventions from the `style` skill. Include the test in the same unit of work as the implementation — same commit when committing — one logical unit — and update the docs when the commit changes documented behavior. Worker-chain branch/PR convention: branch name carries the issue id, the PR body ends with `Fixes CL-…` and carries no AI-attribution lines (CONTRIBUTING: title is a Conventional Commits subject, body is Summary/Verification). - -### Step 5: Reviewer Loop - -Update the active form to "Running Reviewer loop: {subject}". - -Ask Reviewer to review the committed change. - -**How to run:** - -1. Use `spawn_agent(agent="reviewer")` to ask it to review the output of `git show HEAD`. Include the intent from Step 1 (what the change is meant to accomplish and the approach agreed with Planner) so Reviewer can evaluate whether the implementation matches the plan, not just surface-level quality. Tell Reviewer to limit its findings to the scope of the current commit -- pre-existing issues in touched files are out of scope. -2. Read its findings -3. For each issue marked VERIFIED or HIGH confidence: fix it -4. Re-run the build gate (Step 3) to verify fixes -5. Land the fixes on the right commit. If the target is HEAD, `git commit --amend`. Otherwise use `git rebase -i` with `edit` on the target commit: - - ``` - git rebase -i - # In the editor, change "pick ..." to "edit ..." - # ... make the fix at the stop ... - git add - git commit --amend --no-edit - git rebase --continue - ``` - - If the situation calls for more elaborate history surgery, search your available skills for one whose description covers git rebase or branch-history cleanup, and load it. Re-run the build gate after the rebase completes. - -6. Ask Reviewer to review `git show HEAD` again. Re-include the original intent from Step 1 and tell it what you fixed since the last pass so it can focus on verifying the fixes and checking for new issues rather than re-reviewing the entire change from scratch. -7. Repeat until Reviewer comes back clean or all remaining findings are acknowledged and intentional - -**When to stop looping:** - -- Reviewer reports no issues -- Remaining findings are judgment calls you've consciously decided against, not oversights -- The build passes after the last round of fixes - -### Step 6: Next - -Mark the current task `completed` with `manage_tasks`. Move to the next unit of work and return to Step 1. - -## Guidelines - -**Don't shortcut the loop.** The value is in the discipline. Skipping Planner "because this change is simple" or skipping Reviewer "because the build passes" defeats the purpose. - -**Keep commits focused, but do not drop findings.** When Reviewer surfaces something outside the current commit's scope, every finding must be assigned one of four dispositions: (a) fix in the current commit, (b) commit it separately on this branch, (c) file a new issue with concrete acceptance criteria, or (d) accept it as-is. "Out of scope" is not a disposition. "Note it for later" is not a disposition unless you also say which of (a)–(d) "later" means. - -**Disposition (d) always requires operator approval** — neither you nor planner can drop a finding on your own. For (c), the issue must be filed in this session, with its ID or URL in the status update; a promise to file it later is dropping the work. If you are orchestrated by karen, route the decision through karen's section 9 procedure (consult planner, paste his recommendation verbatim, escalate to the operator for any "accept as-is" or any unclear answer). If you are running directly, consult the operator before choosing (c) or (d). - -**Build must pass before every commit, amend, and rebase stop.** Never commit code that doesn't compile or pass tests. Fix build failures first, then commit, amend, or continue the rebase. - -**Planner is for approach, Reviewer is for execution.** Planner reviews your plan before you write code. Reviewer reviews your code after you write it. Don't conflate the two. - -## Acknowledgment - -After reviewing this skill, state: "I have reviewed the implement skill and am ready to follow the commit workflow." +Remove the worktree after the PR merges, or when the operator abandons the change. diff --git a/plugins/corbits-skills/skills/linear-issue-workflow/SKILL.md b/plugins/corbits-skills/skills/linear-issue-workflow/SKILL.md deleted file mode 100644 index 50513bb76..000000000 --- a/plugins/corbits-skills/skills/linear-issue-workflow/SKILL.md +++ /dev/null @@ -1,288 +0,0 @@ ---- -name: linear-issue-workflow -description: Ship a feature or fix from a Linear issue, from claim to reviewed PR. -argument-hint: " [--reviewer ]" -user-invocable: false ---- - -# Linear Issue Workflow - -Use this skill when implementing features or fixes tracked in Linear. - -## Phase 1: Understand the Issue - -Fetch the Linear issue using `mcp__linear__get_issue`. The returned issue includes title, description, status, branch name, and other metadata you will need later. - -Ask the user clarifying questions if the scope is unclear before proceeding. - -## Phase 2: Set Up Worktree - -Read the branch name from the `branchName` field on the issue fetched in Phase 1 (call `mcp__linear__get_issue` again if needed). - -Determine the repository's default branch: - -```bash -git symbolic-ref refs/remotes/origin/HEAD | sed 's@^refs/remotes/origin/@@' -``` - -Fetch the latest changes and create a worktree based on the remote default branch: - -```bash -git fetch origin -git worktree add ../worktree/ -b origin/ -``` - -**Important**: Always base new branches on `origin/` (where `` is `main`, `master`, or whatever the repository uses). This ensures your branch starts from the latest remote state, even if your local default branch is out of date. - -After creating the worktree: - -```bash -cd ../worktree/ -``` - -Then install local dependencies (look at developer documentation for how to do this). - -Worktrees share the `.git` directory but have their own working directory. The `node_modules` directory is NOT shared, so each worktree needs its own dependency installation. - -All subsequent work — exploration, planning, implementation, and review — happens in the worktree directory. - -## Phase 3: Explore and Plan - -1. Use the `explore` subagent to understand the codebase. Brief it with the absolute path to the worktree (it must `cd` there before doing anything else), and ask it to cover: - - Where changes need to be made - - Existing patterns to follow - - Related code that might be affected - -2. Create an implementation plan covering: - - Files to modify - - New functions/types to add - - Tests to write - -3. Present the plan to the user and ask if they would like any changes before proceeding. Do not start implementation until the user approves the plan. - - If the user rejects the plan and the issue cannot be salvaged with a revised plan, tear down the worktree and branch with the Phase 7 commands rather than leaving them stranded. - -4. Attach the plan as a file to the Linear issue. **Do not post the plan as a comment** — comments are for discussion, not archives, and a multi-page plan dumped inline creates noise on the issue. - - The procedure has four steps, three of which are MCP or HTTP calls. The signed upload URL from `prepare_attachment_upload` expires 60 seconds after it is returned, so steps 2 and 3 must happen in immediate succession — do not pause for unrelated work between them. - - 1. Write the plan to a file under the worktree's `tmp/` (e.g., `tmp/plan-.md`). `tmp/` is the project's throwaway directory — do not commit the file (and if the project does not yet have `tmp/` gitignored, do not add to git). Capture its byte size with `wc -c < tmp/plan-.md`. - - 2. Call `mcp__linear__prepare_attachment_upload` with `issue`, `filename`, `contentType: "text/markdown"`, and `size`. The response contains `uploadRequest.url`, `uploadRequest.headers`, and `assetUrl`. **Step 3 must follow within 60 seconds — the signed URL expires.** - - 3. PUT the raw file bytes to `uploadRequest.url`. Pass every header from `uploadRequest.headers` verbatim — exact casing is required; modified or omitted headers return HTTP 403. One `-H` flag per entry: - - ```bash - curl -X PUT --data-binary @tmp/plan-.md \ - -H ": " \ - -H ": " \ - "" - ``` - - Do not base64-encode the file, do not transform it. If the PUT fails with 403 because the URL expired (more than 60 seconds since step 2), **call `prepare_attachment_upload` again for a fresh URL** — retrying the dead URL will keep failing. - - 4. Call `mcp__linear__create_attachment_from_upload` with `issue`, the `assetUrl` from step 2, `title: "Implementation plan"`, and `subtitle: ""` (so the entry stays identifiable if multiple plans accumulate across re-plans). - - Keep the local file through implementation and review — it is the working reference, colocated with the code in the worktree's `tmp/`. Phase 7's `git worktree remove` deletes it along with the rest of the worktree. The Linear attachment is the canonical copy; the local file is a working convenience for the active branch. Re-planning overwrites the local file — Linear retains prior versions as separate attachments via the `` subtitle. - - Exception: plans with no structure — no file-by-file breakdown, no enumerated steps, no headings, no nested lists — can be posted as a comment instead. Structural shape, not length, is the test; a single-sentence plan is fine inline, a 10-line bulleted plan is not. - -5. Mark the issue as "In Progress" using `mcp__linear__save_issue` with the appropriate state. - -## Phase 4: Implement - -Carry out the plan from Phase 3 in the worktree using whichever implementation approach fits the work and your role. - -Break the work into commit-sized units. Each commit should represent one logical change that can be reviewed and understood independently. - -### Keep the issue checkboxes current - -If the issue description contains a task list (`- [ ]` items), tick the boxes as you complete each one. The checkboxes are the issue's at-a-glance progress signal — leaving them stale makes the issue lie about what's done. - -Update the description with `mcp__linear__save_issue`, passing the full description with the relevant `- [ ]` flipped to `- [x]`. Do not rewrite or reorganize the surrounding text — only flip the box. Update as each item finishes, not in a single batch at the end; partial progress is what the checkboxes exist to show. - -If the issue description has no task list, skip this step — do not invent one. - -## Phase 5: Self-Review - -Once implementation is complete, run a **whole-branch code review** before pushing. The review covers the entire diff from the base branch to HEAD — not just the most recent commit — so any finding anywhere in the branch's history is in scope. - -### Reviewer-of-Record Checks (in-session) - -The reviewer-of-record is the agent whose verdict ships — in this workflow, you, the orchestrator. Before delegating any part of the review, run the audits whose value depends on the reviewer-of-record's own eyes on raw output. The canonical command list and patterns live in `review`'s _Reviewer-of-Record Checks_ and _Commit-Message Style Audit_ sections; load `review` and follow them in this session. - -Read the raw output. Stop conditions — fix before continuing: - -- Any violation surfaced by the commit-message audits. The canonical pattern lists (prefixes, vague subjects, body issues, length limits) live in `style` and `review`'s _Commit-Message Style Audit_; this section does not maintain a copy. -- `Bin` marker on any file you did not expect to be binary (source code, markdown, config). -- Files in the diff outside the issue's scope. - -After fixing a stop condition, re-run the in-session checks. Repeat until the output is clean. Do not dispatch the subagent until then. - -### Subagent review (deeper read) - -Dispatch the `reviewer` director with `spawn_agent(agent="reviewer")` for the file-by-file behavioral read, architectural review, and commit-message coherence check. Running these in a subagent keeps the deeper output out of the main context and gives independent eyes on patterns. - -Brief the subagent with: - -- The absolute path to the worktree (it must `cd` there before doing anything else). -- The base branch to diff against (the remote default branch resolved in Phase 2, e.g. `origin/main`), so `review` does not dead-end on its "ask the user" path. Make explicit that the review must cover the full `base..HEAD` range, every commit on the branch. -- An instruction to load the `review` skill and follow its checklist against the current branch, _except_ the items marked _(reviewer-of-record)_ — the orchestrator has already run those. -- The Linear issue ID and a one-line summary of the change's _intent_ — what the change is for. "This branch refactors retry logic to use exponential backoff" is fine; "this branch should not introduce blocking calls in the sendPack path" pre-frames findings and is forbidden. -- A request for findings with `file:line` references — not PR-comment prose. -- An instruction that the subagent's final message must list every finding verbatim, with no summarization or omission. This prevents collapse during transmission; it does _not_ prevent the more fundamental loss of signals that do not fit a finding shape at all (binary markers, surprising stat counts). Those belong to the reviewer-of-record checks above. - -Do **not** include author-supplied "blocking criteria" or "things to look for" in the brief. The skill itself is the criteria; supplementing it narrows the subagent's lens to what you already suspect might be wrong, suppressing unknown-unknowns. The intent summary above is bounded for the same reason — keep it to _what the change is for_, never _what the reviewer should find_. - -### Fix every finding - -Treat the returned findings as a worklist and **fix every one**. The `review` skill's "Signal Over Noise" guidance has already filtered out pedantic taste-only nitpicks upstream — anything that survived to the final findings is something the reviewer judged worth the author's time. "Nit," "minor," "stylistic," and "suggestion" describe the reviewer's confidence about severity; they are not dispositions and do not authorize skipping. The only path to leaving a finding unfixed is a Planner waiver (see below). - -- Fix issues in additional commits, or via `git rebase -i` with `edit` on the target commit when a fix belongs on an earlier commit (mark the target `edit`, make the fix at the stop, `git commit --amend --no-edit`, `git rebase --continue`). -- After fixing, re-run the reviewer-of-record checks in-session, then re-dispatch the review subagent against the full branch as it now stands. Every re-review is a fresh, self-contained read of `base..HEAD` exactly as it is — no scoping to the delta from a prior pass, no carryover ledger of earlier findings. The new findings replace the previous set; the previous are gone. -- Repeat fix-and-review until the review returns clean. -- Cap at three re-reviews. The cap counts subagent dispatches; in-session check failures the orchestrator fixes between dispatches do not consume a slot. If findings remain after the third, stop and surface the situation to the user directly. Do not soften it: tell the user plainly that the branch has been through three review-and-fix passes and the reviewer is still finding issues, list each outstanding finding with its `file:line`, and state explicitly that the branch is **not ready to push**. Do not proceed to Phase 6, do not propose a waiver, and do not offer to "just push anyway" — wait for the user to decide whether to keep iterating, rescope the issue, or abandon the branch. - -### Waivers - -The only path to leaving a finding unfixed is a Planner waiver. If you believe a finding should not be fixed — because the "fix" would be pure churn (taste-only rewording, unrelated refactor, change the project has explicitly chosen not to make) or because you actively disagree with the reviewer — dispatch the `planner` subagent with the finding, your proposed disposition, and the relevant diff. **A Planner "waive" ruling is the waiver** — record it in the disposition note and move on; no further user sign-off is needed. Accept Planner's call by default. Escalate to the user only if (a) you actively disagree with Planner's ruling, or (b) the Planner subagent is unreachable or returns an unusable response. In either case, present both positions (or the failure mode) and let the user decide. Do not route routine waiver requests through the user, and never waive a finding on your own authority. - -By the time the gate closes, every finding is fixed or Planner-waived; the findings themselves are iteration history and are not preserved as a Phase 6 artifact. Hold onto each Planner ruling — Phase 6 names the waivers as present-state exceptions on the branch and cites the ruling that authorized each one. Do not keep the fix SHAs, the fixed findings, or the per-iteration log; none of those describe the branch as it stands. - -## Phase 6: Push and PR - -Once the Phase 5 gate has cleared (the review returned clean, or every remaining finding has a Planner waiver), work through the steps below in order. Drafting precedes confirmation so the user authorizes specific artifacts — the title, body, and self-review comment — rather than a promise about what they will say. - -### Rebase against the remote default branch - -```bash -git fetch origin -git rebase origin/ -``` - -Verify the build still passes after rebasing. - -### Re-derive the PR artifacts from the current diff - -The PR title, body, and self-review comment are PR-shaped artifacts and are bound by `review`'s _Describe the Branch As It Stands_ rule: present-tense statements of what the code _does_, never past-tense narration of how it got there. - -Draft from the diff, not from memory. The implementation history is the single largest source of journey framing; memory of the work reliably reproduces it. Read the output of both: - -```bash -git diff origin/...HEAD -git log origin/..HEAD --format='%s' -``` - -The full diff matters, not just `--stat`. `--stat` shows filenames and line counts; a present-tense description of behaviour has to come from the diff itself. - -After drafting each artifact, scan it for past-tense verbs (`was`, `were`, `had`, `added`, `fixed`, `refactored`, `introduced`, `removed`, `updated`, `changed`) and rewrite each into present-tense. If the rewrite forces dropping a sentence because it has no current-state content, that sentence was journey narrative and should not have been in the artifact. - -### PR title shape - -A single present-tense statement of what the code does. `style`'s commit-subject rules apply: start with a verb, no leading prefix (no `feat:`, no `[scope]`, no ticket ID), no trailing punctuation, no abbreviations. - -### PR body shape - -```markdown -## Summary - -<1-3 bullets describing what the code does, present tense> - -## Verification - -` works end-to-end against ``. -State what is true now, not what was done.> - -Closes -``` - -`## Summary` and `## Verification` are the only sections in the body. Do not add `## Changes`, `## Context`, `## Notes`, or other sections — the diff is right there, and additional prose either restates the diff (past-tense) or re-narrates the journey (also past-tense). - -### Self-review comment shape - -The comment names the _current_ state of the branch: clean, or carrying named Planner-waived exceptions. Never the iteration history, never the prior findings, never the SHAs of fixes. - -**Clean review (the common case).** A one-line comment body: - -``` -Self-review (`review` skill) returned clean. -``` - -**Waivers exist.** Name each waived finding as a present-state exception on the branch. Cite the Planner ruling as a present-state authorization, not as past-tense paraphrase. Describe what the current code _does_, not what an earlier iteration had: - -```markdown -## Self-Review (`review` skill) - -The branch carries the following intentional exceptions, each authorized -by Planner: - -- ``: . Authorized by Planner on grounds of . -``` - -Do not paste the verbatim Phase 5 findings. Do not list fixes by SHA. Do not narrate the iteration history — not in the disposition, not in a "for context" preamble, not in a closing summary. The merged result is what ships; intermediate states are not part of the artifact. - -### Confirm with the user - -Surface the drafted title, body, and self-review comment to the user. The user confirms whether to push and post with those exact artifacts. Do not push until confirmed. - -### Push and post - -```bash -git push -u origin - -gh pr create \ - --title "" \ - --reviewer \ - --body "$(cat <<'EOF' - -EOF -)" -``` - -Post the self-review comment. For the clean case, the single-quoted form is enough (the backticks inside single quotes are literal, not command substitution): - -```bash -gh pr comment --body 'Self-review (`review` skill) returned clean.' -``` - -For the waivers case, use a heredoc: - -```bash -gh pr comment --body "$(cat <<'EOF' - -EOF -)" -``` - -## Phase 7: Cleanup After Merge - -After the PR has been merged — or after a Phase 3 plan rejection that abandons the issue — clean up the worktree and local branch. The orchestrator is still `cd`'d into the worktree from Phase 2; leave it before running these commands: - -```bash -cd # leave the worktree -git fetch origin -git worktree remove ../worktree/ -git branch -d -``` - -If the worktree directory was already manually deleted, prune stale worktree references: - -```bash -git worktree prune -``` - -## Linear MCP Tool Reference - -Common operations: - -| Action | Tool | -| --------------- | ------------------------------------------------------------------------------------------------------------------ | -| Fetch issue | `mcp__linear__get_issue` | -| Get branch name | `mcp__linear__get_issue` (read `branchName` from result) | -| Update status | `mcp__linear__save_issue` (set the state) | -| Add comment | `mcp__linear__save_comment` | -| Attach file | `mcp__linear__prepare_attachment_upload` → PUT → `mcp__linear__create_attachment_from_upload` (see Phase 3 step 4) | -| List teams | `mcp__linear__list_teams` | diff --git a/plugins/corbits-skills/skills/native-integration/SKILL.md b/plugins/corbits-skills/skills/native-integration/SKILL.md deleted file mode 100644 index 3d7463725..000000000 --- a/plugins/corbits-skills/skills/native-integration/SKILL.md +++ /dev/null @@ -1,146 +0,0 @@ ---- -name: native-integration -description: Corbits rules for Linear status, tracker-agnostic issues, and posting reviews on GitHub -user-invocable: false ---- - -# Native integration - -Corbits-specific rules that the other skills rely on. Load alongside `style` and `philosophy`. Workers get this body baked; the primary loads it with `use_skill`. - -## Test runner - -Corbits tests use `bun:test` (`bun test`, `bun run test`). Agent commands run through `bash`. - -## Fleet - -Spawn with `spawn_agent(agent=)` and a closed director id; `intent="general"` is not a spawn. - -Substantial coder work consumes a planner or `/plan` plan (files, acceptance criteria, non-goals, risks, ordered steps) and blocks if that plan is missing. Tiny parent-DIY work stays plan-optional. `/plan` and planner author; they do not ship. Disposition decisions in `/implement` go through the primary, not a worker. - -## Linear claim-first - -When the work tracks a Linear issue and Linear MCP is available, set the issue to In Progress before explore/build thrash. Parallel lanes claim their own IDs. Use `use_skill("git-worktrees")` to create the worktree. - -Move the issue to In Review when the PR is ready for review. Draft or WIP PRs stay In Progress. Never mark the issue Done on PR open, and do not leave it In Review after merge when work remains. If Linear MCP is unavailable, report that status could not be updated. - -`linear-issue-workflow` owns the full Linear ship loop. - -## Tracker-agnostic issues - -`/issue` is not Linear-only: - -1. Linear MCP available: Linear. -2. Else ask which tracker (GitHub, GitLab, other) unless `.corbits/MEMORY.md` already has `Preferred issue tracker`. -3. Persist that preference in MEMORY.md (never secrets). -4. GitHub: `gh issue create`. GitLab: `glab issue create`. - -Do not restate MCP tool names or schemas in chat. Call the tools. - -## 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 (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 `reviewer` for behavioral/architecture, `planner` 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 | -| Reviewer | Behavioral bugs, missing tests, architecture, commit coherence | When a reviewer director ran | -| Planner | Waiver rulings and intentional exceptions | When the planner 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 - -### 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. The Linear claim-first section above owns the In Review write; posting a GitHub review does not set Linear state. diff --git a/plugins/corbits-skills/skills/review/SKILL.md b/plugins/corbits-skills/skills/review/SKILL.md index 3feeb1ee2..d64b551b2 100644 --- a/plugins/corbits-skills/skills/review/SKILL.md +++ b/plugins/corbits-skills/skills/review/SKILL.md @@ -39,6 +39,36 @@ When the target is a PR (number, URL, or branch), review it from a worktree, nev If any step fails, stop and ask instead of working around it. +## Post the Review on GitHub + +When the branch has an open GitHub PR, post the finished review on it. A review that only lives in chat is not done. Posting delivers decided findings; do not reopen the analysis while writing the body. + +Post when the operator asked to review a PR, when `/implement`'s whole-branch gate is running for a branch with an open PR, or when a PR exists and the review's purpose is a record on it. Do not post a private local read. + +Use a real review for the verdict, `--approve`, `--comment`, or `--request-changes`: + +```bash +gh pr review --comment --body-file +``` + +Body shape, short and in present tense: + +```markdown +## Review · + + + +### Findings + +- `path/to/file.ts:12` — concrete problem and why it matters + +### Notes + +- Only load-bearing context. +``` + +A clean review says "No findings." after the one-line description. Every finding has a `path:line` and a concrete failure mode; severity is the review action, not adjectives. No throat-clearing, no praise to soften a finding, no journey narration, no emoji, no "nit:" taste. When more than one lens produced a distinct judgment (reviewer, planner waiver, warden), each posts its own labelled review, and only the lens that owns the merge verdict may approve. Paste the review URL back to the operator. Posting does not change Linear state. + ## Base Branch Determination Before reviewing, you must determine the correct base branch. Use these methods diff --git a/src/telemetry/classify.ts b/src/telemetry/classify.ts index e2befdfc1..d13fcec92 100644 --- a/src/telemetry/classify.ts +++ b/src/telemetry/classify.ts @@ -88,7 +88,6 @@ const FIRST_PARTY_SKILL_NAMES: ReadonlySet = new Set([ "implement", "interview", "issue", - "linear-issue-workflow", "philosophy", "plan", "pull-request", diff --git a/src/telemetry/product-events.test.ts b/src/telemetry/product-events.test.ts index 97c721e9c..a815de5ab 100644 --- a/src/telemetry/product-events.test.ts +++ b/src/telemetry/product-events.test.ts @@ -227,7 +227,6 @@ test("first-party skill names are reported by name; everything else stays custom // name either — the allowlist is the closed set, not the skills directory. // Hidden-from-discovery background skills (explicit loads still resolve) // stay custom. - expect(classifySkillName("native-integration")).toBe("custom"); expect(classifySkillName("Review")).toBe("custom"); expect(classifySkillName("")).toBe("custom"); });