diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 14334fddd..a52f46fc1 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- 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 ### Breaking Changes diff --git a/packages/coding-agent/src/cli/args.ts b/packages/coding-agent/src/cli/args.ts index a2785eb96..67784ffd3 100644 --- a/packages/coding-agent/src/cli/args.ts +++ b/packages/coding-agent/src/cli/args.ts @@ -108,7 +108,8 @@ export interface Args { const PARSE_DEPS: ParseDeps = { logger, parseThinking: parseCliThinkingLevel, - builtinToolNames: [...BUILTIN_TOOL_NAMES, ...HIDDEN_TOOL_NAMES], + toolNames: [...BUILTIN_TOOL_NAMES, ...HIDDEN_TOOL_NAMES], + validateToolNames: false, normalizeToolNames, thinkingEfforts: CLI_THINKING_LEVELS, }; @@ -142,12 +143,19 @@ function consumeBuiltInStringValue(flag: string, args: string[], valueIndex: num return { value, index: valueIndex }; } -export function parseArgs(inputArgs: string[], extensionFlags?: Map): Args { +export function parseArgs( + inputArgs: string[], + extensionFlags?: Map, + extensionToolNames: readonly string[] = [], +): Args { // Work on a copy: the `--option=value` handling below splices the value // into the array, and callers reuse the same argv (the post-extension // reparse in `runRootCommand` parses it a second time). Mutating the input // would corrupt that later parse, so never touch the caller's array. const args = [...inputArgs]; + const parseDeps: ParseDeps = extensionFlags + ? { ...PARSE_DEPS, toolNames: [...PARSE_DEPS.toolNames, ...extensionToolNames], validateToolNames: true } + : PARSE_DEPS; const result: Args = { messages: [], fileArgs: [], @@ -214,7 +222,7 @@ export function parseArgs(inputArgs: string[], extensionFlags?: Map; + getToolNames(): readonly string[]; setFlagValue(name: string, value: boolean | string): void; } @@ -29,18 +30,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(), runner.getToolNames()); + // `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..2d8eae8cb 100644 --- a/packages/coding-agent/src/cli/flag-tables.ts +++ b/packages/coding-agent/src/cli/flag-tables.ts @@ -47,7 +47,8 @@ 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[]; + toolNames: readonly string[]; + validateToolNames: boolean; normalizeToolNames: (values: Iterable) => string[]; thinkingEfforts: readonly string[]; } @@ -191,12 +192,15 @@ export const STRING_SETTERS: Record = { ); // 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(", ")}.`, - ); + // stale `--tools bash,ssh` after the ssh tool's removal). The startup + // parse defers this check until extensions have registered their tools. + if (deps.validateToolNames) { + const unknown = names.filter(name => !deps.toolNames.includes(name)); + if (unknown.length > 0) { + throw new CliUsageError( + `Unknown tool${unknown.length === 1 ? "" : "s"} in --tools: ${unknown.join(", ")}. Valid tools: ${deps.toolNames.join(", ")}.`, + ); + } } result.tools = names; }, diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index d1e98a749..bea616dd6 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -425,7 +425,19 @@ 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(), + getToolNames: () => runner.getAllRegisteredTools().map(tool => tool.definition.name), + setFlagValue: (name, value) => { + runner.setFlagValue(name, value); + }, + } + : undefined, + args.rawArgs, + ); return nextSession; }; } @@ -1615,6 +1627,10 @@ export async function runRootCommand( : await loadSessionExtensions(sessionOptions, cwd, settingsInstance, eventBus); const extensionFlagSink: ExtensionFlagSink = { getFlags: () => ExtensionRunner.aggregateFlags(extensionsResult.extensions), + getToolNames: () => + extensionsResult.extensions.flatMap(extension => + Array.from(extension.tools.values(), tool => tool.definition.name), + ), setFlagValue: (name, value) => { extensionsResult.runtime.flagValues.set(name, value); }, diff --git a/packages/coding-agent/test/cli-unknown-flag.test.ts b/packages/coding-agent/test/cli-unknown-flag.test.ts index ba142dcf8..0d5f93738 100644 --- a/packages/coding-agent/test/cli-unknown-flag.test.ts +++ b/packages/coding-agent/test/cli-unknown-flag.test.ts @@ -113,6 +113,7 @@ describe("parseArgs — unrecognized flag tracking (#2459)", () => { it("propagates unrecognizedFlags through applyExtensionFlags so callers can surface them", () => { const runner = { getFlags: () => new Map([["spawn-peer", { type: "string" }]]), + getToolNames: () => [], setFlagValue: () => {}, }; const parsed = applyExtensionFlags(runner, ["--spawn-peer", "reviewer", "--typo"]); 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..252274716 100644 --- a/packages/coding-agent/test/extension-flag-initial-message.test.ts +++ b/packages/coding-agent/test/extension-flag-initial-message.test.ts @@ -103,6 +103,7 @@ describe("extension flags vs initial message", () => { const sessionId = "019ea530-ffff-7000-8000-000000000000"; const sink: ExtensionFlagSink = { getFlags: () => extFlags, + getToolNames: () => [], setFlagValue: () => {}, }; const parsed = applyExtensionFlags(sink, ["--continue", sessionId]); @@ -123,6 +124,7 @@ describe("extension flags vs initial message", () => { const rawArgs = ["--continue", sessionId, "--spawn-peer", "reviewer", "do next"]; const sink: ExtensionFlagSink = { getFlags: () => extFlags, + getToolNames: () => [], setFlagValue: () => {}, }; const parsed = applyExtensionFlags(sink, rawArgs); @@ -147,6 +149,7 @@ describe("extension flags vs initial message", () => { const sink: ExtensionFlagSink = { getFlags: () => extFlags, + getToolNames: () => [], setFlagValue: () => {}, }; const extensionArgs = applyExtensionFlags(sink, rawArgs); @@ -212,6 +215,7 @@ describe("applyExtensionFlags (single-parser flag resolution)", () => { const values = new Map(); return { values, + getToolNames: () => [], getFlags: () => flagMap, setFlagValue: (name, value) => { values.set(name, value); @@ -221,8 +225,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" }); @@ -330,6 +334,7 @@ describe("registerFlag with built-in-named flags (r3323473227)", () => { ); const sink: ExtensionFlagSink = { getFlags: () => ExtensionRunner.aggregateFlags([ext]), + getToolNames: () => [], setFlagValue: (name, value) => { runtime.flagValues.set(name, value); }, diff --git a/packages/coding-agent/test/flag-tables.test.ts b/packages/coding-agent/test/flag-tables.test.ts index 62173cc08..e24a071ba 100644 --- a/packages/coding-agent/test/flag-tables.test.ts +++ b/packages/coding-agent/test/flag-tables.test.ts @@ -85,19 +85,24 @@ 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 extension discovery", () => { + expect(parseArgs(["--tools", "bash,intercom"]).tools).toEqual(["bash", "intercom"]); + }); + + it("accepts registered extension tools and still rejects unknown names after discovery", () => { + const extensionFlags = new Map(); + expect(parseArgs(["--tools", "read,intercom"], extensionFlags, ["intercom"]).tools).toEqual(["read", "intercom"]); + expect(() => parseArgs(["--tools", "bash,ssh"], extensionFlags, ["intercom"])).toThrow(CliUsageError); + expect(() => parseArgs(["--tools", "bash,ssh"], extensionFlags, ["intercom"])).toThrow( + /Unknown tool in --tools: ssh/, + ); }); });