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

fix(cli): allowed extension tools in allowlists

Browse Source 
Deferred --tools validation until extension discovery, then validated against built-in and registered tool names while preserving strict rejection of unknown names.

Fixes #8421
This commit is contained in:
roboomp
2026-08-13 08:53:59 +00:00
parent 326d24bd40
commit 6980275a50
9 changed files with 74 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+4
View File
@@ -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
packages/coding-agent/src/cli/args.ts
+11 -3
View File
@@ -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<string, { type: "boolean" | "string" }>): Args {
export function parseArgs(
inputArgs: string[],
extensionFlags?: Map<string, { type: "boolean" | "string" }>,
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<string, { ty
if (i + 1 < args.length && args[i + 1] !== PROFILE_BOOTSTRAP_BOUNDARY_ARG) {
const consumed = consumeBuiltInStringValue(arg, args, i + 1);
i = consumed.index;
STRING_SETTERS[arg](result, consumed.value, PARSE_DEPS);
STRING_SETTERS[arg](result, consumed.value, parseDeps);
}
} else if (OPTIONAL_VALUE_FLAGS.has(arg)) {
const config = OPTIONAL_FLAGS[arg];
packages/coding-agent/src/cli/extension-flags.ts
+7 -10
View File
@@ -8,6 +8,7 @@ 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;
}
@@ -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);
}
packages/coding-agent/src/cli/flag-tables.ts
+11 -7
View File
@@ -47,7 +47,8 @@ import { CliUsageError } from "./usage-error";
export interface ParseDeps {
logger: { warn: (message: string, meta?: Record<string, unknown>) => void };
parseThinking: (value: string | null | undefined) => ConfiguredThinkingLevel | undefined;
builtinToolNames: readonly string[];
toolNames: readonly string[];
validateToolNames: boolean;
normalizeToolNames: (values: Iterable<string>) => string[];
thinkingEfforts: readonly string[];
}
@@ -191,12 +192,15 @@ export const STRING_SETTERS: Record<string, StringSetter> = {
);
// 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;
},
packages/coding-agent/src/main.ts
+17 -1
View File
@@ -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);
},
packages/coding-agent/test/cli-unknown-flag.test.ts
+1
View File
@@ -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<string, { type: "boolean" | "string" }>([["spawn-peer", { type: "string" }]]),
getToolNames: () => [],
setFlagValue: () => {},
};
const parsed = applyExtensionFlags(runner, ["--spawn-peer", "reviewer", "--typo"]);
packages/coding-agent/test/extension-flag-dispatch.test.ts
+4
View File
@@ -15,6 +15,10 @@ class FakeExtensionFlagSink implements ExtensionFlagSink {
]);
}
getToolNames(): readonly string[] {
return [];
}
setFlagValue(name: string, value: boolean | string): void {
this.#values.set(name, value);
}
packages/coding-agent/test/extension-flag-initial-message.test.ts
+7 -2
View File
@@ -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<string, boolean | string>();
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);
},
packages/coding-agent/test/flag-tables.test.ts
+12 -7
View File
@@ -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<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/,
);
});
});