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

fix(coding-agent): close remaining extension-flag edge cases (GPT-5.5 review)

Browse Source 
Three issues from an adversarial review, all rooted in the startup argv parse
running before extensions load:

1. Flag-looking string values (`--name --print`): the extension-aware reparse
   consumed the following token as the value, disagreeing with the startup
   parse that treated `--print` as the built-in flag — so the reparse could
   silently flip command shape. Extension string flags now consume a following
   token only in `--flag=value` form or when it is not flag-looking; pass a
   flag-looking value as `--flag=value`. Keeps both parses consistent.

2. `@file` string values (`--target @notes.md`): file args were processed from
   the startup parse, which misreads the value as a file and reads it into the
   prompt. processFileArguments now runs on the extension-aware parse
   (initialArgs.fileArgs); pipedInput stays early for mode detection.

3. Built-in collisions: an extension flag named like a built-in (e.g. `model`)
   was consumed by the built-in branch and never delivered to the runner.
   registerFlag now rejects names in BUILTIN_FLAG_NAMES with a clear error
   (isolated per-extension by loadExtension's try/catch).

Adds tests for all three plus the documented startup-parse misclassification.
This commit is contained in:
Erik Svilich
2026-05-31 04:45:51 +02:00
committed by can1357
parent 295c52a307
commit d6f887d152
4 changed files with 132 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/cli/args.ts
+56 -1
View File
@@ -54,6 +54,54 @@ export interface Args {
unknownFlags: Map<string, boolean | string>;
}
/**
* Long names of every built-in CLI flag recognized by {@link parseArgs}.
* Extension flags that would shadow one of these are rejected at registration
* (see ExtensionAPI.registerFlag), because a built-in branch in parseArgs would
* consume the flag before the extension ever sees it. Keep in sync with the
* flag branches below.
*/
export const BUILTIN_FLAG_NAMES: ReadonlySet<string> = new Set([
"help",
"version",
"allow-home",
"mode",
"continue",
"resume",
"session",
"fork",
"provider",
"model",
"smol",
"slow",
"plan",
"api-key",
"system-prompt",
"append-system-prompt",
"provider-session-id",
"no-session",
"session-dir",
"models",
"no-tools",
"no-lsp",
"no-pty",
"tools",
"thinking",
"print",
"export",
"hook",
"extension",
"plugin-dir",
"no-extensions",
"no-skills",
"no-rules",
"no-title",
"auto-approve",
"yolo",
"approval-mode",
"skills",
"list-models",
]);
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
@@ -216,7 +264,14 @@ export function parseArgs(inputArgs: string[], extensionFlags?: Map<string, { ty
if (extFlag.type === "boolean") {
result.unknownFlags.set(flagName, true);
} else if (extFlag.type === "string" && i + 1 < args.length) {
result.unknownFlags.set(flagName, args[++i]);
// Consume the value in `--flag=value` form, or when the next token
// is not flag-looking. A `-`-prefixed token in space form is left
// to be parsed as its own flag, so this extension-aware parse agrees
// with the extension-unaware startup parse on command shape; pass a
// flag-looking value as `--flag=value`.
if (equalsValueIndex !== -1 || !args[i + 1].startsWith("-")) {
result.unknownFlags.set(flagName, args[++i]);
}
}
}
// Unknown flags without extensionFlags are silently ignored (first pass)
packages/coding-agent/src/extensibility/extensions/loader.ts
+6
View File
@@ -10,6 +10,7 @@ import type { KeyId } from "@oh-my-pi/pi-tui";
import { hasFsCode, isEacces, isEnoent, logger } from "@oh-my-pi/pi-utils";
import * as Zod from "zod/v4";
import { type ExtensionModule, extensionModuleCapability } from "../../capability/extension-module";
import { BUILTIN_FLAG_NAMES } from "../../cli/args";
import { loadCapability } from "../../discovery";
import { getExtensionNameFromPath } from "../../discovery/helpers";
import type { ExecOptions } from "../../exec/exec";
@@ -180,6 +181,11 @@ class ConcreteExtensionAPI implements ExtensionAPI, IExtensionRuntime {
name: string,
options: { description?: string; type: "boolean" | "string"; default?: boolean | string },
): void {
if (BUILTIN_FLAG_NAMES.has(name)) {
throw new Error(
`Extension flag "--${name}" collides with a built-in CLI flag and cannot be registered; choose a different name.`,
);
}
this.extension.flags.set(name, { name, extensionPath: this.extension.path, ...options });
if (options.default !== undefined) {
this.runtime.flagValues.set(name, options.default);
packages/coding-agent/src/main.ts
+15 -12
View File
@@ -790,16 +790,7 @@ export async function runRootCommand(
if (parsedArgs.noTitle || parsedArgs.mode === "rpc" || parsedArgs.mode === "rpc-ui" || parsedArgs.mode === "acp") {
Bun.env.PI_NO_TITLE = "1";
}
const { pipedInput, fileText, fileImages } = await logger.time("prepareInitialMessage", async () => {
const pipedInput = await readPipedInput();
if (parsedArgs.fileArgs.length === 0) {
return { pipedInput, fileText: undefined, fileImages: undefined };
}
const processed = await processFileArguments(parsedArgs.fileArgs, {
autoResizeImages: settingsInstance.get("images.autoResize"),
});
return { pipedInput, fileText: processed.text, fileImages: processed.images };
});
const pipedInput = await logger.time("readPipedInput", readPipedInput);
const autoPrint = pipedInput !== undefined && !parsedArgs.print && parsedArgs.mode === undefined;
const isInteractive = !parsedArgs.print && !autoPrint && parsedArgs.mode === undefined;
const mode = parsedArgs.mode || "text";
@@ -964,10 +955,22 @@ export async function runRootCommand(
}
const initialArgs = applyExtensionFlags(session.extensionRunner, rawArgs) ?? parsedArgs;
// Process @file args from the extension-aware parse, so an extension
// string-flag value such as `--target @notes.md` is consumed as the flag's
// value rather than read as a file into the prompt. File args are not
// needed earlier (session setup depends only on pipedInput/mode).
const processedFiles =
initialArgs.fileArgs.length > 0
? await logger.time("processFileArguments", () =>
processFileArguments(initialArgs.fileArgs, {
autoResizeImages: settingsInstance.get("images.autoResize"),
}),
)
: undefined;
const { initialMessage, initialImages } = buildInitialMessage({
parsed: initialArgs,
fileText,
fileImages,
fileText: processedFiles?.text,
fileImages: processedFiles?.images,
stdinContent: pipedInput,
});
packages/coding-agent/test/extension-flag-initial-message.test.ts
+55
View File
@@ -2,6 +2,8 @@ import { describe, expect, it } from "bun:test";
import { parseArgs } from "../src/cli/args";
import { applyExtensionFlags, type ExtensionFlagSink } from "../src/cli/extension-flags";
import { buildInitialMessage } from "../src/cli/initial-message";
import { ExtensionRuntime, loadExtensionFromFactory } from "../src/extensibility/extensions/loader";
import { EventBus } from "../src/utils/event-bus";
// Regression coverage for extension-registered flags leaking into the initial
// prompt. The CLI parses argv twice: once at startup (before extensions load,
@@ -36,6 +38,34 @@ describe("extension flags vs initial message", () => {
expect(parsed.noTools).toBe(true);
expect(parsed.messages).toEqual(["do the task"]);
});
it("does not consume a flag-looking string value in space form, keeping command shape (P1#2)", () => {
// `--print` after an extension flag (unknown at startup) must stay the
// built-in print flag in BOTH parses, so the reparse cannot silently flip
// command behavior. Flag-looking values must be passed as `--flag=value`.
const parsed = parseArgs(["--spawn-peer", "--print", "hello"], extFlags);
expect(parsed.unknownFlags.has("spawn-peer")).toBe(false);
expect(parsed.print).toBe(true);
expect(parsed.messages).toEqual(["hello"]);
});
it("consumes a flag-looking string value in equals form", () => {
const parsed = parseArgs(["--spawn-peer=--print", "hello"], extFlags);
expect(parsed.unknownFlags.get("spawn-peer")).toBe("--print");
expect(parsed.print).toBeUndefined();
expect(parsed.messages).toEqual(["hello"]);
});
it("treats an @-prefixed string value as the flag's value, not a file arg (P1#1)", () => {
const parsed = parseArgs(["--spawn-peer", "@notes.md", "hello"], extFlags);
expect(parsed.unknownFlags.get("spawn-peer")).toBe("@notes.md");
expect(parsed.fileArgs).toEqual([]);
expect(parsed.messages).toEqual(["hello"]);
});
it("documents the P1#1 startup-parse leak: without flags, an @-value is misread as a file arg", () => {
// This is the startup parse (extensions not loaded). `runRootCommand` must
// run processFileArguments on the extension-aware parse, not this one, or
// `@notes.md` gets read into the prompt as a file.
const parsed = parseArgs(["--spawn-peer", "@notes.md", "hello"]);
expect(parsed.fileArgs).toEqual(["notes.md"]);
});
it("builds the initial prompt from the real message, not the flag value, when flags are known", () => {
const parsed = parseArgs(["--spawn-peer", "reviewer", "review the diff"], extFlags);
@@ -129,3 +159,28 @@ describe("applyExtensionFlags (single-parser flag resolution)", () => {
expect(runner.values.size).toBe(0);
});
});
describe("registerFlag built-in collision guard (P2#3)", () => {
it("rejects an extension flag that shadows a built-in CLI flag", async () => {
await expect(
loadExtensionFromFactory(
api => {
api.registerFlag("model", { type: "string" });
},
process.cwd(),
new EventBus(),
new ExtensionRuntime(),
),
).rejects.toThrow(/collides with a built-in/);
});
it("allows a non-colliding extension flag", async () => {
const ext = await loadExtensionFromFactory(
api => {
api.registerFlag("spawn-peer", { type: "string" });
},
process.cwd(),
new EventBus(),
new ExtensionRuntime(),
);
expect(ext.flags.has("spawn-peer")).toBe(true);
});
});