diff --git a/packages/coding-agent/src/cli/args.ts b/packages/coding-agent/src/cli/args.ts index 67784ffd3..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,8 +108,6 @@ export interface Args { const PARSE_DEPS: ParseDeps = { logger, parseThinking: parseCliThinkingLevel, - toolNames: [...BUILTIN_TOOL_NAMES, ...HIDDEN_TOOL_NAMES], - validateToolNames: false, normalizeToolNames, thinkingEfforts: CLI_THINKING_LEVELS, }; @@ -143,19 +141,13 @@ function consumeBuiltInStringValue(flag: string, args: string[], valueIndex: num return { value, index: valueIndex }; } -export function parseArgs( - inputArgs: string[], - extensionFlags?: Map, - extensionToolNames: readonly string[] = [], -): Args { +export function parseArgs(inputArgs: string[], extensionFlags?: Map): 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 parseDeps = PARSE_DEPS; const result: Args = { messages: [], fileArgs: [], @@ -340,6 +332,17 @@ export function parseArgs( return result; } +/** Reject requested tool names absent from the fully discovered session registry. */ +export function validateToolNames(requested: readonly string[] | undefined, known: readonly string[]): void { + if (!requested) return; + const knownNames = new Set(known); + const unknown = requested.filter(name => !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 3e106d1f7..997b0db97 100644 --- a/packages/coding-agent/src/cli/extension-flags.ts +++ b/packages/coding-agent/src/cli/extension-flags.ts @@ -8,7 +8,6 @@ import { type Args, parseArgs } from "./args"; */ export interface ExtensionFlagSink { getFlags(): Map; - getToolNames(): readonly string[]; setFlagValue(name: string, value: boolean | string): void; } @@ -36,7 +35,7 @@ export interface ExtensionFlagSink { */ export function applyExtensionFlags(runner: ExtensionFlagSink | undefined, rawArgs: string[]): Args | null { if (!runner) return null; - const parsed = parseArgs(rawArgs, runner.getFlags(), runner.getToolNames()); + 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 2d8eae8cb..7e56921ba 100644 --- a/packages/coding-agent/src/cli/flag-tables.ts +++ b/packages/coding-agent/src/cli/flag-tables.ts @@ -47,8 +47,6 @@ import { CliUsageError } from "./usage-error"; export interface ParseDeps { logger: { warn: (message: string, meta?: Record) => void }; parseThinking: (value: string | null | undefined) => ConfiguredThinkingLevel | undefined; - toolNames: readonly string[]; - validateToolNames: boolean; normalizeToolNames: (values: Iterable) => string[]; thinkingEfforts: readonly string[]; } @@ -190,18 +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). 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(", ")}.`, - ); - } - } + // 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 bea616dd6..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"; @@ -430,7 +430,6 @@ export function createAcpSessionFactory(args: AcpSessionFactoryOptions): AcpSess runner ? { getFlags: () => runner.getFlags(), - getToolNames: () => runner.getAllRegisteredTools().map(tool => tool.definition.name), setFlagValue: (name, value) => { runner.setFlagValue(name, value); }, @@ -1627,10 +1626,6 @@ 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); }, @@ -1701,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/cli-unknown-flag.test.ts b/packages/coding-agent/test/cli-unknown-flag.test.ts index 0d5f93738..ba142dcf8 100644 --- a/packages/coding-agent/test/cli-unknown-flag.test.ts +++ b/packages/coding-agent/test/cli-unknown-flag.test.ts @@ -113,7 +113,6 @@ 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-initial-message.test.ts b/packages/coding-agent/test/extension-flag-initial-message.test.ts index 252274716..c24113318 100644 --- a/packages/coding-agent/test/extension-flag-initial-message.test.ts +++ b/packages/coding-agent/test/extension-flag-initial-message.test.ts @@ -103,7 +103,6 @@ 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]); @@ -124,7 +123,6 @@ 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); @@ -149,7 +147,6 @@ describe("extension flags vs initial message", () => { const sink: ExtensionFlagSink = { getFlags: () => extFlags, - getToolNames: () => [], setFlagValue: () => {}, }; const extensionArgs = applyExtensionFlags(sink, rawArgs); @@ -215,7 +212,6 @@ describe("applyExtensionFlags (single-parser flag resolution)", () => { const values = new Map(); return { values, - getToolNames: () => [], getFlags: () => flagMap, setFlagValue: (name, value) => { values.set(name, value); @@ -334,7 +330,6 @@ 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 e24a071ba..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"; @@ -92,16 +92,22 @@ describe("--tools validation", () => { expect(result.tools).toEqual(["grep", "glob"]); }); - it("defers unknown-name validation until extension discovery", () => { + 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("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/, + it("rejects names absent from the final registry", () => { + expect(() => validateToolNames(["read", "missing"], ["read", "intercom", "custom_tool"])).toThrow( + /Unknown tool in --tools: missing/, ); }); });