diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 686456f6d..095cdc341 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -6,6 +6,7 @@ - Fixed the status-line git branch display freezing on the previous branch after the first branch switch, caused by the HEAD watcher binding to a file inode that git unlinks on its atomic HEAD rename ([#8412](https://github.com/can1357/oh-my-pi/issues/8412)). - Fixed Pi extension contexts omitting the runtime `mode`, which made documented TUI guards silently disable extension UI ([#8419](https://github.com/can1357/oh-my-pi/issues/8419)). +- Fixed extension-registered tool names being rejected by `--tools` before extension discovery, preventing least-privilege sessions from allowlisting plugin tools ([#8421](https://github.com/can1357/oh-my-pi/issues/8421)). ## [17.3.0] - 2026-08-13 diff --git a/packages/coding-agent/src/cli/args.ts b/packages/coding-agent/src/cli/args.ts index a2785eb96..412e78289 100644 --- a/packages/coding-agent/src/cli/args.ts +++ b/packages/coding-agent/src/cli/args.ts @@ -6,7 +6,7 @@ import { $env, APP_NAME, logger } from "@oh-my-pi/pi-utils"; import chalk from "@oh-my-pi/pi-utils/chalk"; import type { ServiceTierOpenAISettingValue } from "../config/service-tier"; import { CLI_THINKING_LEVELS, type ConfiguredThinkingLevel, parseCliThinkingLevel } from "../thinking"; -import { BUILTIN_TOOL_NAMES, HIDDEN_TOOL_NAMES, normalizeToolNames } from "../tools/builtin-names"; +import { normalizeToolNames } from "../tools/builtin-names"; import { OPTIONAL_FLAGS, OPTIONAL_VALUE_FLAGS, @@ -108,7 +108,6 @@ export interface Args { const PARSE_DEPS: ParseDeps = { logger, parseThinking: parseCliThinkingLevel, - builtinToolNames: [...BUILTIN_TOOL_NAMES, ...HIDDEN_TOOL_NAMES], normalizeToolNames, thinkingEfforts: CLI_THINKING_LEVELS, }; @@ -148,6 +147,7 @@ export function parseArgs(inputArgs: string[], extensionFlags?: Map !knownNames.has(name)); + if (unknown.length === 0) return; + throw new CliUsageError( + `Unknown tool${unknown.length === 1 ? "" : "s"} in --tools: ${unknown.join(", ")}. Valid tools: ${known.join(", ")}.`, + ); +} + /** * Emit a stderr error listing the unrecognized flags and return `true` when * there were any. Caller is expected to exit with a non-zero status. Splitting diff --git a/packages/coding-agent/src/cli/extension-flags.ts b/packages/coding-agent/src/cli/extension-flags.ts index 42e25a606..997b0db97 100644 --- a/packages/coding-agent/src/cli/extension-flags.ts +++ b/packages/coding-agent/src/cli/extension-flags.ts @@ -29,18 +29,14 @@ export interface ExtensionFlagSink { * semantics and surfaces in `unknownFlags` — without consuming the following * message or overwriting the built-in field. No built-in name list to maintain. * - * Returns `null` when there is no sink or no registered extension flags, in - * which case the caller keeps its original startup parse (an extension-aware - * re-parse would be identical anyway). + * Returns `null` only when there is no sink. Once extensions have loaded, the + * reparse always runs so `--tools` can be validated against their registered + * tools even when no extension registered CLI flags. */ export function applyExtensionFlags(runner: ExtensionFlagSink | undefined, rawArgs: string[]): Args | null { - const extensionFlags = runner?.getFlags(); - if (!runner || !extensionFlags || extensionFlags.size === 0) { - return null; - } - const parsed = parseArgs(rawArgs, extensionFlags); - // `parseArgs` only records registered extension flags in `unknownFlags`, so - // every entry here is a flag this runner owns that was actually passed. + if (!runner) return null; + const parsed = parseArgs(rawArgs, runner.getFlags()); + // `parseArgs` records extension flag values in `unknownFlags`. for (const [name, value] of parsed.unknownFlags) { runner.setFlagValue(name, value); } diff --git a/packages/coding-agent/src/cli/flag-tables.ts b/packages/coding-agent/src/cli/flag-tables.ts index 6c50a177b..7e56921ba 100644 --- a/packages/coding-agent/src/cli/flag-tables.ts +++ b/packages/coding-agent/src/cli/flag-tables.ts @@ -47,7 +47,6 @@ import { CliUsageError } from "./usage-error"; export interface ParseDeps { logger: { warn: (message: string, meta?: Record) => void }; parseThinking: (value: string | null | undefined) => ConfiguredThinkingLevel | undefined; - builtinToolNames: readonly string[]; normalizeToolNames: (values: Iterable) => string[]; thinkingEfforts: readonly string[]; } @@ -189,15 +188,8 @@ export const STRING_SETTERS: Record = { .map(s => s.trim()) .filter(Boolean), ); - // An unknown name silently narrowing the toolset is worse than a failed - // launch: scripts keep running believing the tool is available (e.g. a - // stale `--tools bash,ssh` after the ssh tool's removal). - const unknown = names.filter(name => !deps.builtinToolNames.includes(name)); - if (unknown.length > 0) { - throw new CliUsageError( - `Unknown tool${unknown.length === 1 ? "" : "s"} in --tools: ${unknown.join(", ")}. Valid tools: ${deps.builtinToolNames.join(", ")}.`, - ); - } + // Validation runs after session tool discovery. At this point extension, + // custom, plugin-manifest, and MCP tools are not all known yet. result.tools = names; }, "--thinking": (result, value, deps) => { diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index d1e98a749..54ff3b1d1 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -23,7 +23,7 @@ import { } from "@oh-my-pi/pi-utils"; import chalk from "@oh-my-pi/pi-utils/chalk"; import { reset as resetCapabilities } from "./capability"; -import { type Args, reportUnrecognizedFlags } from "./cli/args"; +import { type Args, reportUnrecognizedFlags, validateToolNames } from "./cli/args"; import { applyExtensionFlags, type ExtensionFlagSink } from "./cli/extension-flags"; import { processFileArguments } from "./cli/file-processor"; import { buildInitialMessage } from "./cli/initial-message"; @@ -425,7 +425,18 @@ export function createAcpSessionFactory(args: AcpSessionFactoryOptions): AcpSess if (args.parsedArgs.apiKey && !args.baseOptions.model && nextSession.model) { args.authStorage.setRuntimeApiKey(nextSession.model.provider, args.parsedArgs.apiKey); } - applyExtensionFlags(nextSession.extensionRunner, args.rawArgs); + const runner = nextSession.extensionRunner; + applyExtensionFlags( + runner + ? { + getFlags: () => runner.getFlags(), + setFlagValue: (name, value) => { + runner.setFlagValue(name, value); + }, + } + : undefined, + args.rawArgs, + ); return nextSession; }; } @@ -1685,6 +1696,13 @@ export async function runRootCommand( preloadedExtensions: extensionsResult, }); + try { + validateToolNames(initialArgs.tools, session.getAllToolNames()); + } catch (error) { + await session.dispose(); + throw error; + } + // Cold-revive support: a `parked` subagent ref restored from disk (Agent Hub // scan, collab mirror, resumed process) has a sessionFile but no in-memory // reviver, so `ensureLive` (IRC sends, hub focus) would refuse it. Install a diff --git a/packages/coding-agent/test/extension-flag-dispatch.test.ts b/packages/coding-agent/test/extension-flag-dispatch.test.ts index b61706ea4..c1c267de2 100644 --- a/packages/coding-agent/test/extension-flag-dispatch.test.ts +++ b/packages/coding-agent/test/extension-flag-dispatch.test.ts @@ -15,6 +15,10 @@ class FakeExtensionFlagSink implements ExtensionFlagSink { ]); } + getToolNames(): readonly string[] { + return []; + } + setFlagValue(name: string, value: boolean | string): void { this.#values.set(name, value); } diff --git a/packages/coding-agent/test/extension-flag-initial-message.test.ts b/packages/coding-agent/test/extension-flag-initial-message.test.ts index 0c53aa5a7..c24113318 100644 --- a/packages/coding-agent/test/extension-flag-initial-message.test.ts +++ b/packages/coding-agent/test/extension-flag-initial-message.test.ts @@ -221,8 +221,8 @@ describe("applyExtensionFlags (single-parser flag resolution)", () => { it("returns null when there is no runner", () => { expect(applyExtensionFlags(undefined, ["--spawn-peer", "x", "task"])).toBeNull(); }); - it("returns null when the runner registered no flags", () => { - expect(applyExtensionFlags(fakeRunner({}), ["--whatever", "task"])).toBeNull(); + it("reparses with an empty extension registry so unknown flags remain visible", () => { + expect(applyExtensionFlags(fakeRunner({}), ["--whatever", "task"])?.unrecognizedFlags).toEqual(["--whatever"]); }); it("applies and strips a string flag in space form", () => { const runner = fakeRunner({ "spawn-peer": "string" }); diff --git a/packages/coding-agent/test/flag-tables.test.ts b/packages/coding-agent/test/flag-tables.test.ts index 62173cc08..50ff29365 100644 --- a/packages/coding-agent/test/flag-tables.test.ts +++ b/packages/coding-agent/test/flag-tables.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "bun:test"; -import { parseArgs } from "../src/cli/args"; +import { parseArgs, validateToolNames } from "../src/cli/args"; import { OPTIONAL_VALUE_FLAGS, STRING_VALUE_FLAGS } from "../src/cli/flag-tables"; import { CliUsageError } from "../src/cli/usage-error"; @@ -85,19 +85,30 @@ describe("--session-dir", () => { }); }); -describe("--tools legacy aliases", () => { +describe("--tools validation", () => { it("maps search and find to grep and glob", () => { const result = parseArgs(["--tools", "search,find,grep"]); expect(result.tools).toEqual(["grep", "glob"]); }); - it("rejects unknown tool names instead of silently narrowing the toolset", () => { - // Removed tools (ssh, job, irc, launch, search_tool_bm25) used to be - // dropped with only a log-file warning, so `--tools bash,ssh` ran with - // just bash and no visible notice. - expect(() => parseArgs(["--tools", "bash,ssh"])).toThrow(CliUsageError); - expect(() => parseArgs(["--tools", "bash,ssh"])).toThrow(/Unknown tool in --tools: ssh/); + it("defers unknown-name validation until all session tools are discovered", () => { + expect(parseArgs(["--tools", "bash,intercom"]).tools).toEqual(["bash", "intercom"]); + expect(parseArgs(["--tools", "read,custom_tool"], new Map()).tools).toEqual(["read", "custom_tool"]); + }); +}); + +describe("--tools discovered-registry validation", () => { + it("accepts extension and custom tools after they enter the session registry", () => { + expect(() => + validateToolNames(["read", "intercom", "custom_tool"], ["read", "intercom", "custom_tool"]), + ).not.toThrow(); + }); + + it("rejects names absent from the final registry", () => { + expect(() => validateToolNames(["read", "missing"], ["read", "intercom", "custom_tool"])).toThrow( + /Unknown tool in --tools: missing/, + ); }); });