diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 94fed4e16..c6dab7f93 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -416,19 +416,20 @@ Primary is Skywalker. Bundled skill bodies that are operator slashes are **actio | `.claude/skills/` | Claude Code workspace skills | | `.codex/skills/` | Codex workspace skills | -Each `//SKILL.md` is one skill. Discovery dedupes by directory name: the first base dir that provides a given name wins, so an enabled plugin skill shadows a project-local skill of the same name. Plugin dirs are passed in discovery order (repo first), so a first-party catalog name wins over a later marketplace or project skill of the same name. `resolveSkillBody(cwd, ref, pluginDirs)` resolves a skill's body using the same ordered list (it accepts a bare name or a `plugin:name` ref, keying on the name). +Each `//SKILL.md` is one skill. Discovery dedupes by directory name: the first base dir that provides a given name wins, so an enabled plugin skill shadows a project-local skill of the same name. Plugin dirs are passed in discovery order (repo first), so a first-party catalog name wins over a later marketplace or project skill of the same name. Skills with `disable-model-invocation: true` are omitted from the returned listing but still claim the name (first-wins), so a lower-priority same-name skill cannot leak into the listing. `resolveSkillBody(cwd, ref, pluginDirs)` resolves a skill's body using the same ordered list (it accepts a bare name or a `plugin:name` ref, keying on the name) and **does not** hard-fail on `disable-model-invocation` — explicit `use_skill("name")` still loads background libraries. #### SKILL.md format -A skill file begins with a YAML frontmatter block, followed by the body that holds the instructions. Discovery parses `description`; `loadSkillCommands` also reads `user-invocable`. The skill's identifier (what `use_skill` and `/` take) is its directory name. A skill with no `SKILL.md` or an empty body is skipped. +A skill file begins with a YAML frontmatter block, followed by the body that holds the instructions. Discovery parses `description` and `disable-model-invocation`; `loadSkillCommands` also reads `user-invocable`. The skill's identifier (what `use_skill` and `/` take) is its directory name. A skill with no `SKILL.md` or an empty body is skipped. -| Field | Required | Description | -| ---------------- | ------------ | -------------------------------------------------------------------------------------------------------------------------------------------------- | -| `description` | yes | One-line summary shown in the prompt's lazy skills listing and the slash picker | -| `name` | conventional | Conventionally matches the directory name; the directory name is what is actually used as the identifier | -| `user-invocable` | no | When `false`, `loadSkillCommands` skips slash synthesis; the skill remains `use_skill` only. Untagged skills still become slashes (marketplace BC) | +| Field | Required | Description | +| -------------------------- | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `description` | yes | One-line summary shown in the prompt's lazy skills listing and the slash picker | +| `name` | conventional | Conventionally matches the directory name; the directory name is what is actually used as the identifier | +| `user-invocable` | no | When `false`, `loadSkillCommands` skips slash synthesis; the skill remains `use_skill` only. Untagged skills still become slashes (marketplace BC) | +| `disable-model-invocation` | no | When `true`, `discoverSkills` omits the skill from the lazy listing (but still claims the name for first-wins). Explicit `resolveSkillBody` / `use_skill("name")` still loads the body. Does not affect slash emission. | -There are no `type` or `disable-model-invocation` fields required for model invocation — a skill body is plain instruction text. `argument-hint` on frontmatter is preserved for the slash picker (greyed arg guidance). Multi-step orchestration is a separate mechanism (see Workflows above), not a skill `type`. +There is no skill `type` field required for model invocation — a skill body is plain instruction text. Background libraries (e.g. `git-worktrees`) set both `user-invocable: false` and `disable-model-invocation: true` so they are absent from slash and listing, yet recipes can still `use_skill("git-worktrees")`. `argument-hint` on frontmatter is preserved for the slash picker (greyed arg guidance). Multi-step orchestration is a separate mechanism (see Workflows above), not a skill `type`. #### Loading (model and operator) diff --git a/docs/PLUGINS.md b/docs/PLUGINS.md index ae347ee75..3848302e6 100644 --- a/docs/PLUGINS.md +++ b/docs/PLUGINS.md @@ -309,10 +309,14 @@ shape. become slashes (marketplace backward compatibility). Frontmatter `argument-hint` is preserved so the TUI can show greyed arg guidance (e.g. `/create-issue` → `[description] [--from-doc]`). This is an additional - surface: `discoverSkills` is unchanged, so the model can still auto-invoke any - skill via `use_skill` — including first-party recipes that are not operator - slashes (`dispatch`, `git-rebase`, `linear-issue-workflow`, `style`, - `philosophy`, `typescript`, `opsh`). The slash command is a direct user entry + surface: `discoverSkills` skips skills with `disable-model-invocation: true` + from the lazy listing (those stay loadable via explicit `use_skill` / + `resolveSkillBody`), so the model does not auto-suggest background libraries. + First-party recipes that are not operator slashes remain listed for + `use_skill` when they only set `user-invocable: false` (`dispatch`, + `git-rebase`, `linear-issue-workflow`, `style`, `philosophy`, `typescript`, + `opsh`). Background libs such as `git-worktrees` set both flags. The slash + command is a direct user entry point on top. - **First-party catalog.** `plugins/corbits-skills/` (id `corbits-skills`, kind `command`, `defaultEnabled: true`) is the bundled skill catalog. Origin diff --git a/plugins/corbits-skills/skills/git-worktrees/SKILL.md b/plugins/corbits-skills/skills/git-worktrees/SKILL.md new file mode 100644 index 000000000..c87702595 --- /dev/null +++ b/plugins/corbits-skills/skills/git-worktrees/SKILL.md @@ -0,0 +1,31 @@ +--- +name: git-worktrees +user-invocable: false +disable-model-invocation: true +description: Create a git worktree from origin/ and tear it down. Background library — load via use_skill("git-worktrees"); absent from slash and use_skill listing. +--- + +# git-worktrees + +Background recipe. Skywalker loads via `use_skill("git-worktrees")` and copies commands into an intern brief. Intern executes via `run_shell`. Skywalker does not run the git. + +## Create from origin/ + +```bash +git symbolic-ref refs/remotes/origin/HEAD | sed 's@^refs/remotes/origin/@@' +git fetch origin +git worktree add ../worktree/ -b origin/ +``` + +Always base new branches on `origin/` (whatever the repository uses). After creating the worktree, intern `cd`s into it and installs local dependencies (`bun install` when the project uses Bun; otherwise follow developer docs). Worktrees do not share `node_modules`. + +## Teardown + +```bash +cd +git fetch origin +git worktree remove ../worktree/ +git branch -d +``` + +If the worktree directory was already deleted: `git worktree prune`. diff --git a/plugins/corbits-skills/skills/linear-issue-workflow/SKILL.md b/plugins/corbits-skills/skills/linear-issue-workflow/SKILL.md index 5d11147c7..f2875547c 100644 --- a/plugins/corbits-skills/skills/linear-issue-workflow/SKILL.md +++ b/plugins/corbits-skills/skills/linear-issue-workflow/SKILL.md @@ -21,23 +21,15 @@ If the scope is unclear, `ask_operator` before proceeding. Do not guess. Read `branchName` from the issue (call `mcp__linear__get_issue` again if needed). -Spawn `task(agent="intern")` with this sequenced `run_shell` list copied into the brief. Intern executes; Skywalker does not run the git. +Load `use_skill("git-worktrees")`. Copy the create-from-origin/ recipe into an intern brief (substitute ``). Spawn `task(agent="intern")`. Intern executes; Skywalker does not run the git. -```bash -git symbolic-ref refs/remotes/origin/HEAD | sed 's@^refs/remotes/origin/@@' -git fetch origin -git worktree add ../worktree/ -b origin/ -``` - -Always base new branches on `origin/` (whatever the repository uses). After creating the worktree, intern `cd`s into it and installs local dependencies from developer documentation. Worktrees do not share `node_modules`. - -If intern fails, stop and `ask_operator`. If the operator rejects the issue before implementation, intern tears down the worktree (Phase 7 commands) rather than leaving it stranded. +If intern fails, stop and `ask_operator`. If the operator rejects the issue before implementation, intern tears down the worktree via the git-worktrees teardown recipe rather than leaving it stranded. ## Phase 3: Plan, attach, mark In Progress 1. Spawn `task(agent="explore")` if the codebase map is not already known. Brief it with the absolute worktree path (it must work there) and the issue: where changes go, existing patterns, related code. 2. Follow the `/implement` loop's greybeard step (Phase 4) for the approach. Present the plan to the operator and `ask_operator` whether to proceed. Do not start implementation until approved. -3. If the operator rejects the plan and the issue cannot be salvaged, intern tears down the worktree (Phase 7) rather than leaving it stranded. +3. If the operator rejects the plan and the issue cannot be salvaged, intern tears down the worktree via the git-worktrees teardown recipe rather than leaving it stranded. 4. Attach the plan to the Linear issue. **Do not post the plan as a comment** — comments are for discussion, not archives. Spawn `task(agent="build")` with a mechanical brief to write the approved plan to the worktree's `tmp/plan-.md` (do not commit it). Intern captures byte size with `wc -c`. Primary then: @@ -130,16 +122,7 @@ Phase 6 ends when the PR is open. Phase 7 runs **after the PR is merged** and ** 2. Re-read the issue with `mcp__linear__get_issue`. Flip checkboxes the merged PR actually completed on `main` via `mcp__linear__save_issue`. Never check a box on intent. 3. `mcp__linear__save_comment` with PR URL, merge SHA, and CI-green confirmation. Short. Present-tense facts. 4. If every outcome checkbox is checked, set state to `Done` with `mcp__linear__save_issue`. Otherwise leave In Progress. -5. Only then intern cleans up: - -```bash -cd -git fetch origin -git worktree remove ../worktree/ -git branch -d -``` - -If the worktree directory was already deleted: `git worktree prune`. +5. Only then intern cleans up: load `use_skill("git-worktrees")` and copy the teardown recipe into an intern brief (substitute `` and ``). ## Linear MCP tool reference diff --git a/src/agent/codex-tool-proxies.test.ts b/src/agent/codex-tool-proxies.test.ts index 002c63222..525b9393e 100644 --- a/src/agent/codex-tool-proxies.test.ts +++ b/src/agent/codex-tool-proxies.test.ts @@ -597,8 +597,8 @@ describe("update_plan proxy", () => { }); describe("allowDeleteFromCapabilities", () => { - test("docs allowlist (no delete_file) → false; build → true", () => { - expect(allowDeleteFromCapabilities({ mode: "allow", tools: DOCS_TOOLS })).toBe(false); + test("docs allowlist (includes delete_file) → true; build → true", () => { + expect(allowDeleteFromCapabilities({ mode: "allow", tools: DOCS_TOOLS })).toBe(true); expect(allowDeleteFromCapabilities({ mode: "allow", tools: BUILD_TOOLS })).toBe(true); expect(allowDeleteFromCapabilities(undefined)).toBe(true); expect(allowDeleteFromCapabilities({ mode: "exclude", tools: ["run_shell"] })).toBe(true); diff --git a/src/agent/codex-tool-proxies.ts b/src/agent/codex-tool-proxies.ts index 14716d2a4..56451072f 100644 --- a/src/agent/codex-tool-proxies.ts +++ b/src/agent/codex-tool-proxies.ts @@ -55,8 +55,8 @@ export interface CreateCodexToolProxiesOpts { runManageTasks: CodexRunManageTasks; /** * When false, Delete File and Update+Move refuse without calling `delete_file`. - * Defaults to true (implement / unconstrained). Docs leaves pass false because - * DOCS_TOOLS includes apply_patch but not delete_file. + * Defaults to true (implement / unconstrained). Pass false when the + * director allowlist omits delete_file (docs leaves mount it today). */ allowDelete?: boolean; /** diff --git a/src/agent/directors/brand-reviewer/package.test.ts b/src/agent/directors/brand-reviewer/package.test.ts index a1d1bdc4e..b79dcc51c 100644 --- a/src/agent/directors/brand-reviewer/package.test.ts +++ b/src/agent/directors/brand-reviewer/package.test.ts @@ -25,6 +25,7 @@ describe("brandReviewerPackage", () => { const allow = brandReviewerPackage.tools?.allow ?? []; expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("systemPrompt mentions DESIGN.md", () => { diff --git a/src/agent/directors/bruckheimer/package.test.ts b/src/agent/directors/bruckheimer/package.test.ts index 7e9d6f4e1..1056d5b85 100644 --- a/src/agent/directors/bruckheimer/package.test.ts +++ b/src/agent/directors/bruckheimer/package.test.ts @@ -26,6 +26,7 @@ describe("bruckheimerPackage", () => { const allow = bruckheimerPackage.tools?.allow ?? []; expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is docs", () => { diff --git a/src/agent/directors/critique/package.test.ts b/src/agent/directors/critique/package.test.ts index 160944919..90efa420a 100644 --- a/src/agent/directors/critique/package.test.ts +++ b/src/agent/directors/critique/package.test.ts @@ -54,14 +54,14 @@ describe("critiquePackage", () => { expect(critiquePackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is review surface without product writes", () => { + test("tools.allow is review surface with product writes", () => { const allow = critiquePackage.tools?.allow ?? []; expect(allow).toContain("read_file"); expect(allow).toContain("read_file"); expect(allow).not.toContain("use_skill"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is review", () => { diff --git a/src/agent/directors/critique/package.ts b/src/agent/directors/critique/package.ts index 1a669fc32..9e15a139a 100644 --- a/src/agent/directors/critique/package.ts +++ b/src/agent/directors/critique/package.ts @@ -46,7 +46,7 @@ API contract check (blocking when brief specifies signatures): - Prefer reading tests/callers; a tiny sync call via run_shell that would hang on a Promise is evidence. - Rank these as blocking, not style nits. -Write tools are not mounted. Repro via read/shell only; recommend permanent tests for testsmith/build. +Write tools are mounted with no path lock — do not use them. Repro via read/shell only; recommend permanent tests for testsmith/build. OUT OF LANE → refuse or reclassify under Blockers: - implementing fixes (route to build) diff --git a/src/agent/directors/draper/package.test.ts b/src/agent/directors/draper/package.test.ts index 9f89e23cb..6b22d71b9 100644 --- a/src/agent/directors/draper/package.test.ts +++ b/src/agent/directors/draper/package.test.ts @@ -20,12 +20,12 @@ describe("draperPackage", () => { expect(draperPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is review surface without product writes", () => { + test("tools.allow is review surface with product writes", () => { const allow = draperPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is review", () => { diff --git a/src/agent/directors/emil/package.test.ts b/src/agent/directors/emil/package.test.ts index 6fef61384..7a050c1a1 100644 --- a/src/agent/directors/emil/package.test.ts +++ b/src/agent/directors/emil/package.test.ts @@ -20,12 +20,12 @@ describe("emilPackage", () => { expect(emilPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is review surface without product writes", () => { + test("tools.allow is review surface with product writes", () => { const allow = emilPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is review", () => { diff --git a/src/agent/directors/explore/package.test.ts b/src/agent/directors/explore/package.test.ts index 9bf3cb31c..52ddd09fa 100644 --- a/src/agent/directors/explore/package.test.ts +++ b/src/agent/directors/explore/package.test.ts @@ -38,13 +38,13 @@ describe("explorePackage", () => { expect(explorePackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is read-only (no product writes)", () => { + test("tools.allow mounts product writes (lane: no product edits)", () => { const allow = explorePackage.tools?.allow ?? []; expect(allow).toContain("read_file"); expect(allow).toContain("grep"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is explore", () => { diff --git a/src/agent/directors/explore/package.ts b/src/agent/directors/explore/package.ts index 3b751fb28..a869de9ca 100644 --- a/src/agent/directors/explore/package.ts +++ b/src/agent/directors/explore/package.ts @@ -1,5 +1,5 @@ import type { DirectorPackage } from "../types.js"; -import { READ_TOOLS } from "../tool-sets.js"; +import { REVIEW_TOOLS } from "../tool-sets.js"; export const explorePackage: DirectorPackage = { id: "explore", @@ -22,7 +22,7 @@ FINISH BIAS: Prefer one thorough pass then report. Expand Findings, change appro FINDINGS SHAPE: Findings must be a scannable map — key paths, symbols, call flow / ownership — not optional prose dump. Cite paths. No drive-by refactors, no feature work, no review severity theater. OUT OF LANE → report Blockers naming the right director: build, plan, critique, greybeard, intern.`, - tools: { allow: READ_TOOLS }, + tools: { allow: REVIEW_TOOLS }, spawn: { maySpawn: false }, tier: "leaf", modelRole: "explore", diff --git a/src/agent/directors/gaasbot/package.test.ts b/src/agent/directors/gaasbot/package.test.ts index 3d4263f1d..f1b47b70b 100644 --- a/src/agent/directors/gaasbot/package.test.ts +++ b/src/agent/directors/gaasbot/package.test.ts @@ -19,12 +19,12 @@ describe("gaasbotPackage", () => { expect(gaasbotPackage.spawn.maySpawn).toBe(false); }); - test("denies product write tools (advice only)", () => { + test("mounts product write tools (lane discipline in prompts)", () => { const allow = gaasbotPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is plan", () => { diff --git a/src/agent/directors/greybeard/package.test.ts b/src/agent/directors/greybeard/package.test.ts index f8b620d8b..de77a07d0 100644 --- a/src/agent/directors/greybeard/package.test.ts +++ b/src/agent/directors/greybeard/package.test.ts @@ -44,13 +44,13 @@ describe("greybeardPackage", () => { expect(greybeardPackage.systemPrompt).toMatch(/never spawn a parallel diagnostic fleet/i); }); - test("tools.allow is orchestrator surface without product writes", () => { + test("tools.allow is orchestrator surface with product writes", () => { const allow = greybeardPackage.tools?.allow ?? []; expect(allow).toContain("task"); expect(allow).toContain("search_agents"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is review", () => { diff --git a/src/agent/directors/intern/package.test.ts b/src/agent/directors/intern/package.test.ts index c2921de4d..db899154c 100644 --- a/src/agent/directors/intern/package.test.ts +++ b/src/agent/directors/intern/package.test.ts @@ -17,12 +17,15 @@ describe("internPackage", () => { expect(internPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is shell-first minimal surface", () => { + test("tools.allow is shell-first with path writes", () => { const allow = internPackage.tools?.allow ?? []; expect(allow).toContain("run_shell"); expect(allow).toContain("read_file"); expect(allow).toContain("list_dir"); - for (const name of ["write_file", "edit_file", "delete_file", "grep", "search_files", "task"]) { + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); + for (const name of ["grep", "search_files", "task", "apply_patch"]) { expect(allow).not.toContain(name); } }); diff --git a/src/agent/directors/intern/package.ts b/src/agent/directors/intern/package.ts index c247909c2..fa9e7b1ff 100644 --- a/src/agent/directors/intern/package.ts +++ b/src/agent/directors/intern/package.ts @@ -3,7 +3,7 @@ import { INTERN_TOOLS } from "../tool-sets.js"; /** * Mechanical intern leaf (CL-5822). - * Shell/commands only — no judgment, no exploration, no product writes. + * Shell/commands first — no judgment, no exploration; path writes only when the brief requires them. */ export const internPackage: DirectorPackage = { id: "intern", diff --git a/src/agent/directors/neckbeard/package.test.ts b/src/agent/directors/neckbeard/package.test.ts index 20bc1cdce..9a9bf5889 100644 --- a/src/agent/directors/neckbeard/package.test.ts +++ b/src/agent/directors/neckbeard/package.test.ts @@ -25,12 +25,12 @@ describe("neckbeardPackage", () => { expect(neckbeardPackage.spawn.maySpawn).toBe(false); }); - test("denies product write tools", () => { + test("mounts product write tools", () => { const allow = neckbeardPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is review", () => { diff --git a/src/agent/directors/plan/package.test.ts b/src/agent/directors/plan/package.test.ts index 5bd548e46..94993331e 100644 --- a/src/agent/directors/plan/package.test.ts +++ b/src/agent/directors/plan/package.test.ts @@ -19,12 +19,12 @@ describe("planPackage", () => { expect(planPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is review surface without product writes", () => { + test("tools.allow is review surface with product writes", () => { const allow = planPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is plan", () => { diff --git a/src/agent/directors/registry.test.ts b/src/agent/directors/registry.test.ts index be552635a..c3d2081d3 100644 --- a/src/agent/directors/registry.test.ts +++ b/src/agent/directors/registry.test.ts @@ -97,7 +97,9 @@ describe("director registry", () => { expect(explore.description).toContain("agent id: explore"); expect(explore.capabilities?.mode).toBe("allow"); expect(explore.capabilities?.tools).toContain("read_file"); - expect(explore.capabilities?.tools).not.toContain("write_file"); + expect(explore.capabilities?.tools).toContain("write_file"); + expect(explore.capabilities?.tools).toContain("edit_file"); + expect(explore.capabilities?.tools).toContain("delete_file"); expect(explore.orchestrator).toBe(false); const grey = packageToProfile(DIRECTOR_REGISTRY.greybeard); @@ -123,7 +125,7 @@ describe("director registry", () => { expect(packageToProfile(g).orchestrator).toBe(true); }); - test("review and design leaves deny product write tools", () => { + test("closed directors mount product write tools", () => { for (const id of [ "critique", "greybeard", @@ -135,22 +137,30 @@ describe("director registry", () => { "testsmith", "tester", "gaasbot", + "intern", + "build", + "shakespeare", + "bruckheimer", + "brand-reviewer", + "skywalker", ] as const) { const allow = DIRECTOR_REGISTRY[id].tools?.allow ?? []; - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); } }); - test("build mounts product writes; intern is shell-only; other leaves do not spawn", () => { + test("build mounts product writes + apply_patch; intern mounts writes without apply_patch; other leaves do not spawn", () => { expect(DIRECTOR_REGISTRY.build.tools?.allow).toEqual( expect.arrayContaining(["write_file", "edit_file", "delete_file", "apply_patch"]), ); const internAllow = DIRECTOR_REGISTRY.intern.tools?.allow ?? []; expect(internAllow).toContain("run_shell"); - expect(internAllow).not.toContain("write_file"); - expect(internAllow).not.toContain("edit_file"); + expect(internAllow).toContain("write_file"); + expect(internAllow).toContain("edit_file"); + expect(internAllow).toContain("delete_file"); + expect(internAllow).not.toContain("apply_patch"); for (const id of DIRECTOR_IDS) { if (id === "skywalker" || id === "greybeard") continue; expect(DIRECTOR_REGISTRY[id].spawn.maySpawn).toBe(false); diff --git a/src/agent/directors/shakespeare/package.test.ts b/src/agent/directors/shakespeare/package.test.ts index 552fbcdee..084b32983 100644 --- a/src/agent/directors/shakespeare/package.test.ts +++ b/src/agent/directors/shakespeare/package.test.ts @@ -36,6 +36,7 @@ describe("shakespearePackage", () => { const allow = shakespearePackage.tools?.allow ?? []; expect(allow).toContain("write_file"); expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is docs", () => { diff --git a/src/agent/directors/tester/package.test.ts b/src/agent/directors/tester/package.test.ts index a6bff51e8..f6aba23d6 100644 --- a/src/agent/directors/tester/package.test.ts +++ b/src/agent/directors/tester/package.test.ts @@ -22,13 +22,13 @@ describe("testerPackage", () => { expect(testerPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is read-only (no product writes)", () => { + test("tools.allow mounts product writes (lane: never fix)", () => { const allow = testerPackage.tools?.allow ?? []; expect(allow).toContain("run_shell"); expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is test", () => { diff --git a/src/agent/directors/tester/package.ts b/src/agent/directors/tester/package.ts index b07de7c55..9735fe3b4 100644 --- a/src/agent/directors/tester/package.ts +++ b/src/agent/directors/tester/package.ts @@ -1,5 +1,5 @@ import type { DirectorPackage } from "../types.js"; -import { READ_TOOLS } from "../tool-sets.js"; +import { REVIEW_TOOLS } from "../tool-sets.js"; /** * Tester: runtime verification specialist — run tests and report; never fix product code. @@ -23,12 +23,12 @@ Workflow: 1. Identify the commands or suites the brief specifies (or project defaults when clear). 2. Run them via shell / harness-allowed tools. 3. Capture exit codes, key failures, and paths. -4. Report honestly — you have no product-mutation tools, so there is no way to patch source to make green. +4. Report honestly — do not patch source to make green; leave product fixes to build. If tests fail: document failures, suspected area, and blockers. Suggest a re-dispatch to build or testsmith when design gaps appear. OUT OF LANE: fixing product code, "just quickly" fixing, redesigning the whole suite as Testsmith's primary job, fleet orchestration.`, - tools: { allow: READ_TOOLS }, + tools: { allow: REVIEW_TOOLS }, spawn: { maySpawn: false }, tier: "leaf", modelRole: "test", diff --git a/src/agent/directors/testsmith/package.test.ts b/src/agent/directors/testsmith/package.test.ts index eedb84ba0..ab37e84c9 100644 --- a/src/agent/directors/testsmith/package.test.ts +++ b/src/agent/directors/testsmith/package.test.ts @@ -21,12 +21,12 @@ describe("testsmithPackage", () => { expect(testsmithPackage.spawn.maySpawn).toBe(false); }); - test("tools.allow is read-only (no product writes)", () => { + test("tools.allow mounts product writes (lane: design only)", () => { const allow = testsmithPackage.tools?.allow ?? []; expect(allow).toContain("read_file"); - expect(allow).not.toContain("write_file"); - expect(allow).not.toContain("edit_file"); - expect(allow).not.toContain("delete_file"); + expect(allow).toContain("write_file"); + expect(allow).toContain("edit_file"); + expect(allow).toContain("delete_file"); }); test("modelRole is test", () => { diff --git a/src/agent/directors/testsmith/package.ts b/src/agent/directors/testsmith/package.ts index 3f13c63be..6783f7554 100644 --- a/src/agent/directors/testsmith/package.ts +++ b/src/agent/directors/testsmith/package.ts @@ -1,5 +1,5 @@ import type { DirectorPackage } from "../types.js"; -import { READ_TOOLS } from "../tool-sets.js"; +import { REVIEW_TOOLS } from "../tool-sets.js"; /** * Testsmith: test design specialist — strategy and cases only; never implements product @@ -29,8 +29,8 @@ Design in the report. Prefer: OUT OF LANE: fixing production code, becoming the implementer, running the full verify-and-fix loop, fleet orchestration. -Read and search the codebase to ground the design; you have no product-mutation tools.`, - tools: { allow: READ_TOOLS }, +Read and search the codebase to ground the design; do not mutate product code.`, + tools: { allow: REVIEW_TOOLS }, spawn: { maySpawn: false }, tier: "leaf", modelRole: "test", diff --git a/src/agent/directors/tool-sets.test.ts b/src/agent/directors/tool-sets.test.ts index 4deae1f3f..2e73f12ae 100644 --- a/src/agent/directors/tool-sets.test.ts +++ b/src/agent/directors/tool-sets.test.ts @@ -3,14 +3,31 @@ import { DOCS_TOOLS, BUILD_TOOLS, ORCHESTRATOR_TOOLS, + PRODUCT_WRITE_TOOLS, READ_TOOLS, + REVIEW_TOOLS, + INTERN_TOOLS, SKYWALKER_TOOLS, } from "./tool-sets.js"; +describe("PRODUCT_WRITE_TOOLS", () => { + test("is write_file / edit_file / delete_file", () => { + expect([...PRODUCT_WRITE_TOOLS]).toEqual(["write_file", "edit_file", "delete_file"]); + }); +}); + +describe("READ_TOOLS", () => { + test("stays read-only (no path mutation)", () => { + for (const name of PRODUCT_WRITE_TOOLS) { + expect(READ_TOOLS as readonly string[]).not.toContain(name); + } + }); +}); + describe("DOCS_TOOLS", () => { - test("excludes run_shell and delete_file as envelope policy", () => { + test("excludes run_shell as envelope policy; includes delete_file", () => { expect(DOCS_TOOLS).not.toContain("run_shell"); - expect(DOCS_TOOLS).not.toContain("delete_file"); + expect(DOCS_TOOLS).toContain("delete_file"); }); test("keeps read/search/lsp/web + file writes + apply_patch", () => { @@ -24,6 +41,7 @@ describe("DOCS_TOOLS", () => { "web_search", "write_file", "edit_file", + "delete_file", "apply_patch", ]; for (const tool of expected) { @@ -44,16 +62,34 @@ describe("DOCS_TOOLS", () => { }); describe("SKYWALKER_TOOLS / ORCHESTRATOR_TOOLS", () => { - test("Skywalker mounts product writes; greybeard orchestrator surface does not", () => { - for (const name of ["write_file", "edit_file", "delete_file"] as const) { + test("both mount product writes and task", () => { + for (const name of PRODUCT_WRITE_TOOLS) { expect(SKYWALKER_TOOLS as readonly string[]).toContain(name); - expect(ORCHESTRATOR_TOOLS as readonly string[]).not.toContain(name); + expect(ORCHESTRATOR_TOOLS as readonly string[]).toContain(name); } expect(SKYWALKER_TOOLS).toContain("task"); expect(ORCHESTRATOR_TOOLS).toContain("task"); }); }); +describe("REVIEW_TOOLS / INTERN_TOOLS", () => { + test("compose PRODUCT_WRITE_TOOLS", () => { + for (const name of PRODUCT_WRITE_TOOLS) { + expect(REVIEW_TOOLS as readonly string[]).toContain(name); + expect(INTERN_TOOLS as readonly string[]).toContain(name); + } + }); + + test("intern stays shell-first without grep/search/task", () => { + expect(INTERN_TOOLS).toContain("run_shell"); + expect(INTERN_TOOLS).toContain("read_file"); + expect(INTERN_TOOLS).toContain("list_dir"); + for (const name of ["grep", "search_files", "task"] as const) { + expect(INTERN_TOOLS as readonly string[]).not.toContain(name); + } + }); +}); + describe("BUILD_TOOLS", () => { test("includes apply_patch alongside path mutation tools", () => { expect(BUILD_TOOLS).toContain("write_file"); @@ -66,4 +102,10 @@ describe("BUILD_TOOLS", () => { expect(BUILD_TOOLS).toContain("shell"); expect(BUILD_TOOLS).toContain("update_plan"); }); + + test("review/orchestrator/intern do not mount apply_patch", () => { + for (const surface of [REVIEW_TOOLS, ORCHESTRATOR_TOOLS, INTERN_TOOLS]) { + expect(surface as readonly string[]).not.toContain("apply_patch"); + } + }); }); diff --git a/src/agent/directors/tool-sets.ts b/src/agent/directors/tool-sets.ts index 9bf82dfbe..7cd5b1a4d 100644 --- a/src/agent/directors/tool-sets.ts +++ b/src/agent/directors/tool-sets.ts @@ -16,6 +16,13 @@ export const READ_TOOLS = [ "web_search", ] as const; +/** + * Path mutation tools shared by closed directors. Codex `apply_patch` stays on + * build/docs only — review/explore/orchestrator/intern mount these path tools + * alone (lane discipline lives in prompts, not the capability filter). + */ +export const PRODUCT_WRITE_TOOLS = ["write_file", "edit_file", "delete_file"] as const; + /** * Build: read + full file mutation. `shell` and `update_plan` are Codex * proxy names (createCodexToolProxies) for `run_shell` / the plan tool; both @@ -24,46 +31,43 @@ export const READ_TOOLS = [ */ export const BUILD_TOOLS = [ ...READ_TOOLS, - "write_file", - "edit_file", - "delete_file", + ...PRODUCT_WRITE_TOOLS, "apply_patch", "shell", "update_plan", ] as const; /** - * Docs leaves: read/search/lsp/web + file writes — no run_shell, no delete_file. + * Docs leaves: read/search/lsp/web + file writes — no run_shell. * Envelope policy only: docs leaves omit shell so they cannot mutate via the * terminal. There is no separate path-level lock on top of the tool envelope. * * Composed from READ_TOOLS minus run_shell so it tracks the read surface - * automatically; only the write tools are added explicitly. `apply_patch` is + * automatically; path writes come from PRODUCT_WRITE_TOOLS. `apply_patch` is * included so Codex docs leaves keep the proxy after the capability filter. * `update_plan` is included for the same reason (its proxy has no `run_shell` * dependency, so it is not excluded alongside `shell`). */ export const DOCS_TOOLS = [ ...READ_TOOLS.filter((t) => t !== "run_shell"), - "write_file", - "edit_file", + ...PRODUCT_WRITE_TOOLS, "apply_patch", "update_plan", ] as const; -/** Review / counsel: read surface, no writes. */ -export const REVIEW_TOOLS = [...READ_TOOLS] as const; +/** Review / counsel: read surface + path writes (lane discipline in prompts). */ +export const REVIEW_TOOLS = [...READ_TOOLS, ...PRODUCT_WRITE_TOOLS] as const; -/** Mechanical intern: shell-first, minimal surface. */ -export const INTERN_TOOLS = ["run_shell", "read_file", "list_dir"] as const; +/** Mechanical intern: shell-first + path writes when the brief requires them. */ +export const INTERN_TOOLS = ["run_shell", "read_file", "list_dir", ...PRODUCT_WRITE_TOOLS] as const; -/** Nested orchestrator surface (greybeard / package filter): dispatch only. */ -export const ORCHESTRATOR_TOOLS = [...READ_TOOLS, "search_agents", "task"] as const; - -/** Skywalker primary: orchestrator surface plus product writes for DIY tiny work. */ -export const SKYWALKER_TOOLS = [ - ...ORCHESTRATOR_TOOLS, - "write_file", - "edit_file", - "delete_file", +/** Nested orchestrator surface (greybeard / package filter): dispatch + path writes. */ +export const ORCHESTRATOR_TOOLS = [ + ...READ_TOOLS, + ...PRODUCT_WRITE_TOOLS, + "search_agents", + "task", ] as const; + +/** Skywalker primary: orchestrator surface (writes already composed). */ +export const SKYWALKER_TOOLS = [...ORCHESTRATOR_TOOLS] as const; diff --git a/src/agent/fleet-verbs-mount.test.ts b/src/agent/fleet-verbs-mount.test.ts new file mode 100644 index 000000000..ac53419b7 --- /dev/null +++ b/src/agent/fleet-verbs-mount.test.ts @@ -0,0 +1,75 @@ +/** + * Primary createAgentToolset mounts the six fleet verbs beside task / + * search_agents / read_agent_trace when subAgent (with the shared TUI + * sessions store) is wired. Leaves / no-subAgent toolsets stay without them. + */ +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, test } from "bun:test"; + +import { createSubAgentSessionStore } from "../subagent/session-store.js"; + +const FLEET_VERBS = [ + "spawn_agent", + "wait_agents", + "close_agent", + "resume_agent", + "interrupt_agent", + "followup_task", +] as const; + +describe("primary fleet verb mount", () => { + test("createAgentToolset registers the six fleet verbs when subAgent + sessions are set", async () => { + const cwd = mkdtempSync(join(tmpdir(), "corbits-fleet-mount-")); + const { createAgentToolset } = await import("./tools.js"); + const permissionGate = { + check: async () => ({ allowed: true }), + getSkipPermissions: () => false, + } as never; + const sessions = createSubAgentSessionStore(); + + const toolset = await createAgentToolset({ + cwd, + permissionGate, + onOperatorGate: async () => ({ kind: "option", index: 0 }), + subAgent: { + provider: { + providerName: "test", + baseURL: "http://127.0.0.1:0", + model: "test-model", + }, + getWorkdirBase: () => cwd, + sessions, + }, + }); + const names = toolset.dynamicRunner.currentDefinitions().map((d) => d.name); + expect(names).toContain("task"); + expect(names).toContain("read_agent_trace"); + for (const name of FLEET_VERBS) { + expect(names).toContain(name); + } + await toolset.dispose(); + }); + + test("createAgentToolset omits fleet verbs when subAgent is not set", async () => { + const cwd = mkdtempSync(join(tmpdir(), "corbits-fleet-mount-")); + const { createAgentToolset } = await import("./tools.js"); + const permissionGate = { + check: async () => ({ allowed: true }), + getSkipPermissions: () => false, + } as never; + + const toolset = await createAgentToolset({ + cwd, + permissionGate, + onOperatorGate: async () => ({ kind: "option", index: 0 }), + }); + const names = toolset.dynamicRunner.currentDefinitions().map((d) => d.name); + expect(names).not.toContain("task"); + for (const name of FLEET_VERBS) { + expect(names).not.toContain(name); + } + await toolset.dispose(); + }); +}); diff --git a/src/agent/tool-search.test.ts b/src/agent/tool-search.test.ts index 3368ed6c3..f70662fb2 100644 --- a/src/agent/tool-search.test.ts +++ b/src/agent/tool-search.test.ts @@ -83,6 +83,21 @@ describe("createToolIndex", () => { ); }); + test("orchestrator mode advertises the six fleet verbs", () => { + const advertised = advertisedToolNamesForSessionMode("orchestrator", FULL_AVAILABILITY); + for (const name of [ + "spawn_agent", + "wait_agents", + "close_agent", + "resume_agent", + "interrupt_agent", + "followup_task", + ] as const) { + expect(CORE_TOOL_NAMES).toContain(name); + expect(advertised).toContain(name); + } + }); + test("manage_tasks is advertised regardless of availability", () => { expect(coreToolNamesForSessionMode("orchestrator", NO_AVAILABILITY)).toContain("manage_tasks"); }); @@ -219,6 +234,16 @@ describe("advertisedTools", () => { const prefix = advertisedToolNamesForSessionMode("orchestrator", FULL_AVAILABILITY); expect(prefix).toContain("task"); expect(prefix).toContain("search_agents"); + for (const name of [ + "spawn_agent", + "wait_agents", + "close_agent", + "resume_agent", + "interrupt_agent", + "followup_task", + ] as const) { + expect(prefix).toContain(name); + } // advertisedTools only emits tools present in the registry; multi-agent // tools appear on the wire when createAgentToolset registers them. const names = advertisedTools(registry, [], prefix).map((d) => d.name); diff --git a/src/agent/tool-search.ts b/src/agent/tool-search.ts index eda40ea59..4d58ca801 100644 --- a/src/agent/tool-search.ts +++ b/src/agent/tool-search.ts @@ -39,9 +39,28 @@ export const CORE_TOOL_NAMES: readonly string[] = [ // round-trip. Catalog-only placement left the model discovering profiles then // failing on an unloaded task tool. "task", + // Fleet verbs (non-blocking spawn + lifecycle). Mounted on primary when + // subAgent is wired; advertised here so the model does not tool_search for + // them. Package allowlists (ORCHESTRATOR_TOOLS / SKYWALKER_TOOLS) are a + // separate, deferred change. + "spawn_agent", + "wait_agents", + "close_agent", + "resume_agent", + "interrupt_agent", + "followup_task", ]; -const ORCHESTRATOR_ONLY_TOOL_NAMES: readonly string[] = ["search_agents", "task"]; +const ORCHESTRATOR_ONLY_TOOL_NAMES: readonly string[] = [ + "search_agents", + "task", + "spawn_agent", + "wait_agents", + "close_agent", + "resume_agent", + "interrupt_agent", + "followup_task", +]; // Session-start facts that gate a core tool's advertisement. Each must be // knowable once, before the first inference call, and must never change for diff --git a/src/agent/tools.ts b/src/agent/tools.ts index cf6fcf5ab..23a46b14b 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -39,6 +39,17 @@ import { type SubAgentProvider, type SubAgentSessionStore, } from "../subagent/index.js"; +import { + createFleetRecords, + createSpawnAgentTool, + createWaitAgentsTool, +} from "../subagent/agent-fleet.js"; +import { + createCloseAgentTool, + createResumeAgentTool, + createInterruptAgentTool, + createFollowupTaskTool, +} from "../subagent/lifecycle-tools.js"; import { parseManageTasksArgs } from "./tasks.js"; import { createListDirTool } from "../util/list-dir.js"; import { createWebFetchTool } from "../tools/web-fetch.js"; @@ -263,6 +274,84 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise inheritedMcpTools, + run: runSubAgent, + ...(shellTimeout !== undefined ? { shellTimeout } : {}), + ...(shellEnv !== undefined ? { shellEnv } : {}), + ...(extraToolPlugins.length > 0 ? { extraToolPlugins } : {}), + ...(sa.onEvent !== undefined ? { onEvent: sa.onEvent } : {}), + ...(sa.onProgress !== undefined ? { onProgress: sa.onProgress } : {}), + ...(sa.sessions !== undefined ? { sessions: sa.sessions } : {}), + ...(sa.settings !== undefined ? { settings: sa.settings } : {}), + ...(sa.catalog !== undefined ? { catalog: sa.catalog } : {}), + ...(sa.profiles !== undefined ? { profiles: sa.profiles } : {}), + ...(args.getBlobReader !== undefined ? { getBlobReader: args.getBlobReader } : {}), + ...(sa.useWorktree !== undefined ? { useWorktree: sa.useWorktree } : {}), + ...(args.telemetry !== undefined ? { telemetry: args.telemetry } : {}), + }), + ); + if (sa.profiles !== undefined) { + orchestratorTools.push( + createSearchAgentsTool(() => { + const profiles = sa.profiles; + return typeof profiles === "function" ? profiles() : (profiles ?? []); + }), + ); + } + // Tier 1: the primary session is always an orchestrator and may + // target any worker (assertCanTargetAgent's rule), so no authority + // context is passed here — omitting it is treated as unrestricted, + // matching Tier 1's actual authority. + orchestratorTools.push(createReadAgentTraceTool(sa.getWorkdirBase)); + + // Mirror nested runSubAgent's orchestrator fleet mount (run.ts), but + // reuse the existing TUI/exec session store — do not allocate a private + // store only for these verbs. spawnAllowlist stays unwired on primary. + if (sa.sessions !== undefined) { + const fleetSessions = sa.sessions; + const fleetRecords = createFleetRecords(); + const fleetDeps = { + permissionGate, + inheritMcpTools: () => inheritedMcpTools, + ...(shellTimeout !== undefined ? { shellTimeout } : {}), + ...(shellEnv !== undefined ? { shellEnv } : {}), + ...(extraToolPlugins.length > 0 ? { extraToolPlugins } : {}), + cwd, + getWorkdirBase: sa.getWorkdirBase, + provider: sa.provider, + ...(args.getBlobReader !== undefined ? { getBlobReader: args.getBlobReader } : {}), + run: runSubAgent, + sessions: fleetSessions, + fleetRecords, + ...(sa.onEvent !== undefined ? { onEvent: sa.onEvent } : {}), + ...(sa.onProgress !== undefined ? { onProgress: sa.onProgress } : {}), + ...(sa.settings !== undefined ? { settings: sa.settings } : {}), + ...(sa.catalog !== undefined ? { catalog: sa.catalog } : {}), + ...(args.telemetry !== undefined ? { telemetry: args.telemetry } : {}), + }; + orchestratorTools.push( + createSpawnAgentTool(fleetDeps), + createWaitAgentsTool({ sessions: fleetSessions, fleetRecords }), + createCloseAgentTool({ sessions: fleetSessions }), + createResumeAgentTool({ sessions: fleetSessions }), + createInterruptAgentTool({ sessions: fleetSessions }), + createFollowupTaskTool({ sessions: fleetSessions }), + ); + } + } + const baseTools: AgentTool[] = [ ...fromToolRunner(posixTools).map((tool) => ({ ...tool, @@ -276,47 +365,7 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise inheritedMcpTools, - run: runSubAgent, - ...(shellTimeout !== undefined ? { shellTimeout } : {}), - ...(shellEnv !== undefined ? { shellEnv } : {}), - ...(extraToolPlugins.length > 0 ? { extraToolPlugins } : {}), - ...(args.subAgent.onEvent !== undefined ? { onEvent: args.subAgent.onEvent } : {}), - ...(args.subAgent.onProgress !== undefined - ? { onProgress: args.subAgent.onProgress } - : {}), - ...(args.subAgent.sessions !== undefined ? { sessions: args.subAgent.sessions } : {}), - ...(args.subAgent.settings !== undefined ? { settings: args.subAgent.settings } : {}), - ...(args.subAgent.catalog !== undefined ? { catalog: args.subAgent.catalog } : {}), - ...(args.subAgent.profiles !== undefined ? { profiles: args.subAgent.profiles } : {}), - ...(args.getBlobReader !== undefined ? { getBlobReader: args.getBlobReader } : {}), - ...(args.subAgent.useWorktree !== undefined - ? { useWorktree: args.subAgent.useWorktree } - : {}), - ...(args.telemetry !== undefined ? { telemetry: args.telemetry } : {}), - }), - ...(args.subAgent.profiles !== undefined - ? [ - createSearchAgentsTool(() => { - const profiles = args.subAgent!.profiles; - return typeof profiles === "function" ? profiles() : (profiles ?? []); - }), - ] - : []), - // Tier 1: the primary session is always an orchestrator and may - // target any worker (assertCanTargetAgent's rule), so no authority - // context is passed here — omitting it is treated as unrestricted, - // matching Tier 1's actual authority. - createReadAgentTraceTool(args.subAgent.getWorkdirBase), - ] - : []), + ...orchestratorTools, stringTool({ definition: manageTasksDefinition, handler: async (rawArgs: Record): Promise => { diff --git a/src/extensions/skills.ts b/src/extensions/skills.ts index 4ee5340e7..db0e99d28 100644 --- a/src/extensions/skills.ts +++ b/src/extensions/skills.ts @@ -50,13 +50,25 @@ function stripFrontmatter(raw: string): string { return raw.slice(end + 3).trim(); } -function parseSkillFrontmatter(raw: string): { name?: string; description?: string } { +function parseSkillFrontmatter(raw: string): { + name?: string; + description?: string; + disableModelInvocation?: boolean; +} { const block = frontmatterBlock(raw); if (block === undefined) return {}; - const out: { name?: string; description?: string } = {}; + const out: { + name?: string; + description?: string; + disableModelInvocation?: boolean; + } = {}; for (const line of block.split("\n")) { - const match = /^(name|description):\s*(.+)$/.exec(line.trim()); + const trimmed = line.trim(); + const match = /^(name|description):\s*(.+)$/.exec(trimmed); if (match) out[match[1] as "name" | "description"] = match[2]!.trim(); + if (/^disable-model-invocation:\s*true\s*$/.test(trimmed)) { + out.disableModelInvocation = true; + } } return out; } @@ -136,11 +148,15 @@ export async function resolveSkillBody( // Discover every available skill (name + one-line description) for the lazy // listing in the system prompt. Deduped by name: the first base dir that // provides a skill wins, so a higher-precedence dir shadows a lower one. +// Skills with `disable-model-invocation: true` are omitted from the listing +// but still occupy the name in `seen` so a lower-priority same-name skill +// cannot leak in. Explicit `use_skill` / `resolveSkillBody` loads still work. export async function discoverSkills( cwd: string, pluginDirs: string[] = [], ): Promise { - const seen = new Map(); + const seen = new Set(); + const skills: SkillSummary[] = []; for (const base of skillBaseDirs(cwd, pluginDirs)) { const entries = await readdir(base, { withFileTypes: true }).catch(() => undefined); if (entries === undefined) continue; @@ -149,8 +165,11 @@ export async function discoverSkills( const raw = await readRaw(join(base, entry.name, "SKILL.md")); if (raw === undefined) continue; const fm = parseSkillFrontmatter(raw); - seen.set(entry.name, { name: entry.name, description: fm.description ?? "" }); + // First-wins: claim the name even when skipping the listing. + seen.add(entry.name); + if (fm.disableModelInvocation) continue; + skills.push({ name: entry.name, description: fm.description ?? "" }); } } - return [...seen.values()]; + return skills; } diff --git a/src/plugins/skill-commands.ts b/src/plugins/skill-commands.ts index 9194f8483..86e981958 100644 --- a/src/plugins/skill-commands.ts +++ b/src/plugins/skill-commands.ts @@ -11,9 +11,9 @@ import { splitFrontmatter } from "./frontmatter.js"; // body (plus args) to the agent. Convention/internal skills opt out with // `user-invocable: false` in frontmatter and are not emitted as slash commands. // Untagged skills still become slash commands (marketplace BC). -// `disable-model-invocation` does not affect slash emission. A skill authored -// as `skills//SKILL.md` is still model-invoked via the `use_skill` tool; -// `discoverSkills` is unchanged, so the model can still auto-invoke any skill. +// `disable-model-invocation` does not affect slash emission — that flag only +// skips the skill from `discoverSkills` lazy listing. Explicit `use_skill` / +// `resolveSkillBody` still loads the body by name. const COMMAND_NAME_PATTERN = /^[a-z0-9]+(-[a-z0-9]+)*$/; @@ -60,7 +60,8 @@ export async function loadSkillCommands( continue; } // Opt-out of the slash surface. Untagged skills still emit a command - // (marketplace BC); `disable-model-invocation` does not affect this. + // (marketplace BC); `disable-model-invocation` does not affect this — + // it only skips discoverSkills listing (see src/extensions/skills.ts). if (frontmatter["user-invocable"] === false) continue; const name = diff --git a/tests/unit/corbits-skills-catalog.test.ts b/tests/unit/corbits-skills-catalog.test.ts index ac0a83361..ad5c272b2 100644 --- a/tests/unit/corbits-skills-catalog.test.ts +++ b/tests/unit/corbits-skills-catalog.test.ts @@ -17,6 +17,7 @@ const SKILL_DIRS = [ "typescript", "interview", "git-rebase", + "git-worktrees", "refactor", "pull-request-review", "create-issue", @@ -27,6 +28,7 @@ const SKILL_DIRS = [ const SPAWN_RECIPE_SKILLS = ["implement", "scribe", "review", "dispatch", "plan"] as const; +/** use_skill listing + resolve; not slash. No disable-model-invocation. */ const USE_SKILL_ONLY = [ "dispatch", "git-rebase", @@ -37,6 +39,9 @@ const USE_SKILL_ONLY = [ "opsh", ] as const; +/** Background libs: absent from slash and use_skill listing; explicit resolve only. */ +const BACKGROUND_ONLY = ["git-worktrees"] as const; + const SLASH_SKILLS = [ "implement", "refactor", @@ -52,6 +57,7 @@ const SLASH_SKILLS = [ const BANNED_TOKENS = ["TaskCreate", "@greybeard", 'intent="general"'] as const; const USER_INVOCABLE_FALSE = "user-invocable: false"; +const DISABLE_MODEL_INVOCATION = "disable-model-invocation: true"; async function listFilesRecursive(dir: string): Promise { const out: string[] = []; @@ -82,8 +88,8 @@ test("corbits-skills plugin has no agents directory", () => { expect(existsSync(join(pluginRoot, "agents"))).toBe(false); }); -test("corbits-skills catalog lists 16 skills with name and description", async () => { - expect(SKILL_DIRS).toHaveLength(16); +test("corbits-skills catalog lists 17 skills with name and description", async () => { + expect(SKILL_DIRS).toHaveLength(17); const entries = await readdir(join(pluginRoot, "skills"), { withFileTypes: true }); const dirs = entries .filter((entry) => entry.isDirectory()) @@ -114,13 +120,39 @@ test("create-issue selects Linear MCP, GitHub gh, and MEMORY.md preference", asy expect(skill).toContain("Preferred issue tracker:"); }); -test("use_skill-only skills set user-invocable: false", async () => { +test("use_skill-only skills set user-invocable: false without disable-model-invocation", async () => { for (const name of USE_SKILL_ONLY) { const skill = await Bun.file(join(pluginRoot, "skills", name, "SKILL.md")).text(); expect(skill).toContain(USER_INVOCABLE_FALSE); + expect(skill).not.toContain(DISABLE_MODEL_INVOCATION); + } +}); + +test("background-only skills set both exclusion flags", async () => { + for (const name of BACKGROUND_ONLY) { + const skill = await Bun.file(join(pluginRoot, "skills", name, "SKILL.md")).text(); + expect(skill).toContain(USER_INVOCABLE_FALSE); + expect(skill).toContain(DISABLE_MODEL_INVOCATION); + } +}); + +test("only background libs carry disable-model-invocation", async () => { + for (const name of SKILL_DIRS) { + const skill = await Bun.file(join(pluginRoot, "skills", name, "SKILL.md")).text(); + if ((BACKGROUND_ONLY as readonly string[]).includes(name)) { + expect(skill).toContain(DISABLE_MODEL_INVOCATION); + } else { + expect(skill).not.toContain(DISABLE_MODEL_INVOCATION); + } } }); +test("linear-issue-workflow references use_skill(git-worktrees)", async () => { + const skill = await Bun.file(join(pluginRoot, "skills/linear-issue-workflow/SKILL.md")).text(); + expect(skill).toContain('use_skill("git-worktrees")'); + expect(skill).not.toContain("git worktree add"); +}); + test("slash skills do not set user-invocable: false", async () => { for (const name of SLASH_SKILLS) { const skill = await Bun.file(join(pluginRoot, "skills", name, "SKILL.md")).text(); diff --git a/tests/unit/skills.test.ts b/tests/unit/skills.test.ts index 6ae3c37ed..b8884e89f 100644 --- a/tests/unit/skills.test.ts +++ b/tests/unit/skills.test.ts @@ -21,6 +21,57 @@ describe("skill discovery", () => { const names = (await discoverSkills(fixtureCwd, pluginDirs)).map((s) => s.name); expect(new Set(names).size).toBe(names.length); }); + + test("skips disable-model-invocation:true from listing but still occupies seen", async () => { + const root = await mkdtemp(join(tmpdir(), "skill-dmi-")); + const high = join(root, "high"); + const low = join(root, "low"); + try { + await mkdir(join(high, "skills", "bg-lib"), { recursive: true }); + await mkdir(join(low, "skills", "bg-lib"), { recursive: true }); + await writeFile( + join(high, "skills", "bg-lib", "SKILL.md"), + "---\nname: bg-lib\ndescription: high priority background\ndisable-model-invocation: true\n---\nHigh body.\n", + "utf8", + ); + await writeFile( + join(low, "skills", "bg-lib", "SKILL.md"), + "---\nname: bg-lib\ndescription: leaked lower priority\n---\nLow body that must not list.\n", + "utf8", + ); + const skills = await discoverSkills(root, [high, low]); + expect(skills.find((s) => s.name === "bg-lib")).toBeUndefined(); + expect(skills.some((s) => s.description.includes("leaked"))).toBe(false); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + + test("lists a sibling skill when a peer has disable-model-invocation", async () => { + const plugin = await mkdtemp(join(tmpdir(), "skill-dmi-peer-")); + try { + await mkdir(join(plugin, "skills", "bg-lib"), { recursive: true }); + await mkdir(join(plugin, "skills", "visible"), { recursive: true }); + await writeFile( + join(plugin, "skills", "bg-lib", "SKILL.md"), + "---\nname: bg-lib\ndescription: background\ndisable-model-invocation: true\n---\nHidden.\n", + "utf8", + ); + await writeFile( + join(plugin, "skills", "visible", "SKILL.md"), + "---\nname: visible\ndescription: still listed\n---\nVisible body.\n", + "utf8", + ); + const skills = await discoverSkills(plugin, [plugin]); + expect(skills.find((s) => s.name === "bg-lib")).toBeUndefined(); + expect(skills.find((s) => s.name === "visible")).toEqual({ + name: "visible", + description: "still listed", + }); + } finally { + await rm(plugin, { recursive: true, force: true }); + } + }); }); describe("skill resolution", () => { @@ -38,6 +89,25 @@ describe("skill resolution", () => { test("returns undefined for an unknown skill", async () => { expect(await resolveSkillBody(fixtureCwd, "does-not-exist-xyz", pluginDirs)).toBeUndefined(); }); + + test("resolveSkillBody still loads disable-model-invocation skills by name", async () => { + const plugin = await mkdtemp(join(tmpdir(), "skill-dmi-resolve-")); + try { + await mkdir(join(plugin, "skills", "git-worktrees"), { recursive: true }); + await writeFile( + join(plugin, "skills", "git-worktrees", "SKILL.md"), + "---\nname: git-worktrees\nuser-invocable: false\ndisable-model-invocation: true\ndescription: bg\n---\nCreate worktree recipe.\n", + "utf8", + ); + expect(await discoverSkills(plugin, [plugin])).toEqual([]); + const body = await resolveSkillBody(plugin, "git-worktrees", [plugin]); + expect(body).toBeDefined(); + expect(body).toContain("Create worktree recipe."); + expect(body!.startsWith("---")).toBe(false); + } finally { + await rm(plugin, { recursive: true, force: true }); + } + }); }); describe("path-like skill refs", () => { diff --git a/tests/unit/subagent.test.ts b/tests/unit/subagent.test.ts index a78df4283..e1d624efa 100644 --- a/tests/unit/subagent.test.ts +++ b/tests/unit/subagent.test.ts @@ -283,9 +283,9 @@ test("intent maps to closed director without profiles", async () => { expect(received?.systemPromptRole).toContain("PRIMARY INTENT"); expect(received?.capabilities?.mode).toBe("allow"); expect(received?.capabilities?.tools).toContain("read_file"); - expect(received?.capabilities?.tools).not.toContain("write_file"); - expect(received?.capabilities?.tools).not.toContain("edit_file"); - expect(received?.capabilities?.tools).not.toContain("delete_file"); + expect(received?.capabilities?.tools).toContain("write_file"); + expect(received?.capabilities?.tools).toContain("edit_file"); + expect(received?.capabilities?.tools).toContain("delete_file"); }); test("intent general is refused (no general director)", async () => {