smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge remote-tracking branch 'origin/farm/fba8665b/persist-empty-advisor-tools'

Browse Source
This commit is contained in:
can1357
2026-07-11 08:26:20 +02:00
parent c893e7ab73 dabb2291a7
commit 4c75ba44e1
6 changed files with 79 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+3
View File
@@ -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
packages/coding-agent/src/advisor/__tests__/advisor.test.ts
+13
View File
@@ -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");
packages/coding-agent/src/advisor/__tests__/config.test.ts
+36
View File
@@ -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);
packages/coding-agent/src/advisor/config.ts
+11 -8
View File
@@ -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<string>(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<Watchdog
advisors: (result.advisors ?? []).map(a => ({
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;
});
packages/coding-agent/src/modes/components/advisor-config.ts
+15 -7
View File
@@ -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<string>, 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<string>, 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)",
);
}
packages/coding-agent/src/session/agent-session.ts
+1 -1
View File
@@ -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;