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

fix(cli): validated allowlists after tool discovery

Browse Source 
Moved strict --tools validation to the completed session registry so extension modules, custom tool directories, and plugin manifest tools are all eligible while unknown names still fail startup.
This commit is contained in:
roboomp
2026-08-13 09:12:12 +00:00
parent 6980275a50
commit 3749478239
7 changed files with 39 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/cli/args.ts
+14 -11
View File
@@ -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<string, { type: "boolean" | "string" }>,
extensionToolNames: readonly string[] = [],
): Args {
export function parseArgs(inputArgs: string[], extensionFlags?: Map<string, { type: "boolean" | "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 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
packages/coding-agent/src/cli/extension-flags.ts
+1 -2
View File
@@ -8,7 +8,6 @@ import { type Args, parseArgs } from "./args";
*/
export interface ExtensionFlagSink {
getFlags(): Map<string, { type: "boolean" | "string" }>;
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);
packages/coding-agent/src/cli/flag-tables.ts
+2 -14
View File
@@ -47,8 +47,6 @@ import { CliUsageError } from "./usage-error";
export interface ParseDeps {
logger: { warn: (message: string, meta?: Record<string, unknown>) => void };
parseThinking: (value: string | null | undefined) => ConfiguredThinkingLevel | undefined;
toolNames: readonly string[];
validateToolNames: boolean;
normalizeToolNames: (values: Iterable<string>) => string[];
thinkingEfforts: readonly string[];
}
@@ -190,18 +188,8 @@ export const STRING_SETTERS: Record<string, StringSetter> = {
.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) => {
packages/coding-agent/src/main.ts
+8 -6
View File
@@ -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
packages/coding-agent/test/cli-unknown-flag.test.ts
-1
View File
@@ -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<string, { type: "boolean" | "string" }>([["spawn-peer", { type: "string" }]]),
getToolNames: () => [],
setFlagValue: () => {},
};
const parsed = applyExtensionFlags(runner, ["--spawn-peer", "reviewer", "--typo"]);
packages/coding-agent/test/extension-flag-initial-message.test.ts
-5
View File
@@ -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<string, boolean | string>();
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);
},
packages/coding-agent/test/flag-tables.test.ts
+14 -8
View File
@@ -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<string, { type: "boolean" | "string" }>();
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/,
);
});
});