diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e52f011e6..24ccd57a2 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -42,6 +42,9 @@ ### Removed - Removed the bundled plan subagent from available task agents. +### Fixed + +- Fixed advisor config preserving an explicit empty tool list so `/advisor config` can disable all advisor tools. ([#5155](https://github.com/can1357/oh-my-pi/issues/5155)) ## [16.4.2] - 2026-07-10 diff --git a/packages/coding-agent/src/advisor/__tests__/advisor.test.ts b/packages/coding-agent/src/advisor/__tests__/advisor.test.ts index 16911c00b..179756d80 100644 --- a/packages/coding-agent/src/advisor/__tests__/advisor.test.ts +++ b/packages/coding-agent/src/advisor/__tests__/advisor.test.ts @@ -1827,6 +1827,19 @@ describe("advisor", () => { expect(text).toContain("read, grep, glob (default)"); }); + it("renders an explicit no-tools advisor distinctly from the omitted default", async () => { + const uiTheme = await getThemeByName("dark"); + if (!uiTheme) throw new Error("theme unavailable"); + setThemeInstance(uiTheme); + const overlay = make({ + advisors: [{ name: "Blank", tools: [] }], + }); + + const text = strip(overlay.render(200)); + expect(text.toLowerCase()).toContain("no tools"); + expect(text).not.toContain("read, grep, glob (default)"); + }); + it("moves the preview with keyboard selection and preserves an explicit tool set", async () => { const uiTheme = await getThemeByName("dark"); if (!uiTheme) throw new Error("theme unavailable"); diff --git a/packages/coding-agent/src/advisor/__tests__/config.test.ts b/packages/coding-agent/src/advisor/__tests__/config.test.ts index 44d14f936..2875d751f 100644 --- a/packages/coding-agent/src/advisor/__tests__/config.test.ts +++ b/packages/coding-agent/src/advisor/__tests__/config.test.ts @@ -56,6 +56,27 @@ describe("discoverAdvisorConfigs", () => { expect(sharedInstructions).toBe("Shared baseline for all advisors."); }); + it("distinguishes omitted tools, explicit no-tools, and invalid-only lists", async () => { + const yaml = [ + "advisors:", + " - name: No Tools", + " tools: []", + " - name: Default Tools", + " - name: Invalid Only", + " tools: [reed]", + ].join("\n"); + await Bun.write(path.join(tmp, "WATCHDOG.yml"), yaml); + + const { advisors } = await discoverAdvisorConfigs(tmp, agentDir); + const noTools = advisors.find(a => a.name === "No Tools"); + const defaultTools = advisors.find(a => a.name === "Default Tools"); + const invalidOnly = advisors.find(a => a.name === "Invalid Only"); + + expect(noTools?.tools).toEqual([]); + expect(defaultTools?.tools).toBeUndefined(); + expect(invalidOnly?.tools).toBeUndefined(); + }); + it("ignores a malformed YAML file without throwing", async () => { await Bun.write(path.join(tmp, "WATCHDOG.yml"), "advisors: [unclosed bracket"); const result = await discoverAdvisorConfigs(tmp, agentDir); @@ -206,6 +227,21 @@ describe("WATCHDOG.yml file round-trip", () => { expect(sharedInstructions).toContain("Shared baseline."); }); + it("round-trips an explicit empty tools list without collapsing it into the default", async () => { + const file = path.join(tmp, "WATCHDOG.yml"); + const explicitNoToolsDoc: WatchdogConfigDoc = { + advisors: [{ name: "No Tools", tools: [] }, { name: "Default Tools" }], + }; + + await saveWatchdogConfigFile(file, explicitNoToolsDoc); + const serializedDoc = await loadWatchdogConfigFile(file); + expect(serializedDoc).toEqual(explicitNoToolsDoc); + + const { advisors } = await discoverAdvisorConfigs(tmp, tmp); + expect(advisors.find(a => a.name === "No Tools")?.tools).toEqual([]); + expect(advisors.find(a => a.name === "Default Tools")?.tools).toBeUndefined(); + }); + it("removes the file when the doc is empty so legacy discovery resumes", async () => { const file = path.join(tmp, "WATCHDOG.yml"); await saveWatchdogConfigFile(file, doc); diff --git a/packages/coding-agent/src/advisor/config.ts b/packages/coding-agent/src/advisor/config.ts index 1d0f15bb5..ae1633ed2 100644 --- a/packages/coding-agent/src/advisor/config.ts +++ b/packages/coding-agent/src/advisor/config.ts @@ -12,9 +12,10 @@ import { collectConfigCandidates } from "./watchdog"; * with an optional `:level` thinking suffix (e.g. `x-ai/grok-code-fast:high`), * resolved exactly like any other model override; `tools` is a subset of * `BUILTIN_TOOL_NAMES` — any built-in name, including mutating tools such as - * `edit`/`write`/`bash` (the advisor is a full agent). Omitted or empty falls - * back to the default `read`/`grep`/`glob` subset. `instructions` is the - * advisor's specialization, appended to the shared baseline. + * `edit`/`write`/`bash` (the advisor is a full agent). Omitted falls back to + * the default `read`/`grep`/`glob` subset; an explicit empty list grants no + * tools. `instructions` is the advisor's specialization, appended to the shared + * baseline. */ export interface AdvisorConfig { name: string; @@ -93,11 +94,13 @@ const KNOWN_TOOL_NAMES = new Set(BUILTIN_TOOL_NAMES); /** * Keep only valid tool names from an advisor's `tools` list, dropping unknowns * with a warning. The advisor is a full agent, so any built tool may be granted; - * the runtime further filters to what's actually available this session. An empty - * result (or no list) means "use the default subset" (read/grep/glob). + * the runtime further filters to what's actually available this session. + * `undefined` means "use the default subset" (read/grep/glob); only an explicit + * raw empty list means "no tools". */ function filterAdvisorTools(tools: string[] | undefined, sourcePath: string): string[] | undefined { - if (!tools || tools.length === 0) return undefined; + if (tools === undefined) return undefined; + if (tools.length === 0) return []; // Normalize legacy aliases (search→grep, find→glob) and dedupe before validating. const filtered = normalizeToolNames(tools).filter(name => { if (KNOWN_TOOL_NAMES.has(name)) return true; @@ -238,7 +241,7 @@ export async function loadWatchdogConfigFile(filePath: string): Promise ({ name: a.name, model: a.model?.trim() || undefined, - tools: a.tools?.length ? [...a.tools] : undefined, + tools: a.tools === undefined ? undefined : [...a.tools], instructions: a.instructions?.trim() ? a.instructions : undefined, })), }; @@ -257,7 +260,7 @@ export function serializeWatchdogConfig(doc: WatchdogConfigDoc): string { out.advisors = doc.advisors.map(a => { const entry: AdvisorConfig = { name: a.name }; if (a.model?.trim()) entry.model = a.model; - if (a.tools?.length) entry.tools = [...a.tools]; + if (a.tools !== undefined) entry.tools = [...a.tools]; if (a.instructions?.trim()) entry.instructions = a.instructions; return entry; }); diff --git a/packages/coding-agent/src/modes/components/advisor-config.ts b/packages/coding-agent/src/modes/components/advisor-config.ts index 7f28bc4f9..34750b32f 100644 --- a/packages/coding-agent/src/modes/components/advisor-config.ts +++ b/packages/coding-agent/src/modes/components/advisor-config.ts @@ -82,9 +82,9 @@ function previewLine(text: string | undefined): string { return first.length > PREVIEW_WIDTH ? `${first.slice(0, PREVIEW_WIDTH - 1)}…` : first; } -/** Default when the set is empty or exactly read/grep/glob; else the available-ordered subset. */ +/** Omitted means default read/grep/glob; an explicit empty set means no tools. */ function commitTools(selected: ReadonlySet, all: readonly string[]): string[] | undefined { - if (selected.size === 0) return undefined; + if (selected.size === 0) return []; if (selected.size === ADVISOR_DEFAULT_TOOL_NAMES.size) { let matchesDefault = true; for (const name of ADVISOR_DEFAULT_TOOL_NAMES) { @@ -98,6 +98,11 @@ function commitTools(selected: ReadonlySet, all: readonly string[]): str return all.filter(name => selected.has(name)); } +function formatAdvisorTools(tools: readonly string[] | undefined, emptyLabel: string): string { + if (tools === undefined) return "read, grep, glob (default)"; + return tools.length > 0 ? tools.join(", ") : emptyLabel; +} + /** Soft-wrap plain text to `width`, returning at least one (possibly empty) line. */ function wrap(text: string, width: number): string[] { if (!text) return [""]; @@ -266,7 +271,7 @@ export class AdvisorConfigOverlayComponent implements Component { #advisorPreview(advisor: AdvisorConfig, bodyWidth: number): string[] { const model = advisor.model?.trim() || this.#defaultModelLabel || "advisor role default"; - const tools = advisor.tools?.length ? advisor.tools.join(", ") : "read, grep, glob (default)"; + const tools = formatAdvisorTools(advisor.tools, "no tools"); const lines = [ theme.bold(advisor.name || "(unnamed)"), "", @@ -303,13 +308,16 @@ export class AdvisorConfigOverlayComponent implements Component { const advisor = doc.advisors[0]; if (!advisor) return false; return ( - advisor.name === "default" && !advisor.model?.trim() && !advisor.tools?.length && !advisor.instructions?.trim() + advisor.name === "default" && + !advisor.model?.trim() && + advisor.tools === undefined && + !advisor.instructions?.trim() ); } #advisorSummary(advisor: AdvisorConfig): string { const model = advisor.model?.trim() || this.#defaultModelLabel || "advisor role default"; - const tools = advisor.tools?.length ? advisor.tools.join(", ") : "(default: read/grep/glob)"; + const tools = formatAdvisorTools(advisor.tools, "no tools"); return `${model} · ${tools}`; } @@ -384,7 +392,7 @@ export class AdvisorConfigOverlayComponent implements Component { return; } const modelDescription = advisor.model?.trim() || this.#defaultModelLabel || "advisor role default"; - const toolsDescription = advisor.tools?.length ? advisor.tools.join(", ") : "(default: read/grep/glob)"; + const toolsDescription = formatAdvisorTools(advisor.tools, "no tools"); const items: SelectItem[] = [ { value: "name", label: "Name", description: advisor.name }, { value: "model", label: "Model", description: modelDescription }, @@ -524,7 +532,7 @@ export class AdvisorConfigOverlayComponent implements Component { this.#setScreen( "tools", list, - "Enter / click toggle · select Done or Esc to apply (empty or read/grep/glob = default)", + "Enter / click toggle · select Done or Esc to apply (empty = no tools; read/grep/glob = default)", ); } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 11c739b5e..381b581f3 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -2509,7 +2509,7 @@ export class AgentSession { if (this.#advisorSharedInstructions) systemPrompt.push(this.#advisorSharedInstructions); if (config.instructions?.trim()) systemPrompt.push(config.instructions.trim()); - const names = config.tools?.length ? new Set(config.tools) : ADVISOR_DEFAULT_TOOL_NAMES; + const names = config.tools === undefined ? ADVISOR_DEFAULT_TOOL_NAMES : new Set(config.tools); const tools = (this.#advisorTools ?? []).filter(t => names.has(t.name)); const primaryProviderSessionId = this.sessionId;