fix(cli): reject unknown --flags instead of starting an agent
Bare `omp --list-models` (or any other stale/typoed --flag) was silently consumed by `parseArgs` and the agent went on to start a real session, connect to the configured MCP servers, and hang waiting on the model. Any positional after the unknown flag was reinterpreted as the initial prompt, so a documentation drift turned into an unintended LLM invocation. `parseArgs` now tracks flag-shaped tokens that did not match any built-in or extension-registered flag in a new `unrecognizedFlags: string[]` field, and `reportUnrecognizedFlags` prints a clean `Error: unknown flag(s): …` line plus the `--help` hint. `runRootCommand` invokes the helper right after the post-extension reparse and `process.exit(2)`s before any session, MCP, or initial-message work runs. The validation is gated on the extension-aware reparse, so extension flags (`--spawn-peer`, `--headless`, `--plan`, …) still pass through the same way `applyExtensionFlags` already handles them. `-` (stdin marker) and `--` (POSIX separator) are deliberately allowed through. Fixes #2459
This commit is contained in:
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed unknown `--`-prefixed flags being silently consumed as prompt text, which let a stale or typoed flag start a real agent session (connecting to MCP servers, waiting on the model) instead of failing fast. `parseArgs` now tracks unrecognized flag-shaped tokens and `runRootCommand` calls `reportUnrecognizedFlags` immediately after the post-extension reparse, exiting `2` with `Error: unknown flag: --…` before any session, MCP, or initial-message work runs. Extension-registered flags still pass cleanly since the validation runs after the extension-aware reparse ([#2459](https://github.com/can1357/oh-my-pi/issues/2459)).
|
||||
|
||||
## [15.12.4] - 2026-06-13
|
||||
|
||||
### Breaking Changes
|
||||
|
||||
@@ -53,8 +53,19 @@ export interface Args {
|
||||
approvalMode?: "always-ask" | "write" | "yolo";
|
||||
messages: string[];
|
||||
fileArgs: string[];
|
||||
/** Unknown flags (potentially extension flags) - map of flag name to value */
|
||||
/** Extension-registered flags this parse recognized — name to value. */
|
||||
unknownFlags: Map<string, boolean | string>;
|
||||
/**
|
||||
* `--`/`-` prefixed tokens this parse could not match against any built-in
|
||||
* or {@link extensionFlags} entry. The startup parse runs *before*
|
||||
* extensions load, so it always lists every extension-registered flag here;
|
||||
* the post-extension reparse in {@link applyExtensionFlags} clears those
|
||||
* once the real flag set is known. Anything still present after that
|
||||
* reparse is a genuine typo or stale flag and {@link reportUnrecognizedFlags}
|
||||
* surfaces it as a hard error so the agent does not silently start a
|
||||
* session with the misparsed positionals as a prompt (issue #2459).
|
||||
*/
|
||||
unrecognizedFlags: string[];
|
||||
}
|
||||
|
||||
export function parseArgs(inputArgs: string[], extensionFlags?: Map<string, { type: "boolean" | "string" }>): Args {
|
||||
@@ -67,6 +78,7 @@ export function parseArgs(inputArgs: string[], extensionFlags?: Map<string, { ty
|
||||
messages: [],
|
||||
fileArgs: [],
|
||||
unknownFlags: new Map(),
|
||||
unrecognizedFlags: [],
|
||||
};
|
||||
|
||||
for (let i = 0; i < args.length; i++) {
|
||||
@@ -229,8 +241,17 @@ export function parseArgs(inputArgs: string[], extensionFlags?: Map<string, { ty
|
||||
result.skills = args[++i].split(",").map(s => s.trim());
|
||||
} else if (arg.startsWith("@")) {
|
||||
result.fileArgs.push(arg.slice(1)); // Remove @ prefix
|
||||
} else if (!arg.startsWith("-")) {
|
||||
result.messages.push(arg);
|
||||
} else if (!arg.startsWith("-") || arg === "-" || arg === "--") {
|
||||
// Plain positional, lone `-` (stdin marker), or POSIX positional
|
||||
// separator `--` — pass through as a message rather than flagging it.
|
||||
if (arg !== "--") result.messages.push(arg);
|
||||
} else {
|
||||
// Flag-shaped (`-x`, `--name`) but unrecognized at this parse. Record
|
||||
// it so the post-extension reparse can decide whether to surface it
|
||||
// as a hard error. `--flag=value` already split `value` into the next
|
||||
// slot; the standard "drop unconsumed equals value" guard below
|
||||
// removes it so it does not leak into messages (issue #2459).
|
||||
result.unrecognizedFlags.push(arg);
|
||||
}
|
||||
// Drop an unconsumed `--flag=value` value (e.g. a boolean flag): when no
|
||||
// branch advanced past the spliced token, remove it so it does not fall
|
||||
@@ -243,6 +264,24 @@ export function parseArgs(inputArgs: string[], extensionFlags?: Map<string, { ty
|
||||
return result;
|
||||
}
|
||||
|
||||
/**
|
||||
* 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
|
||||
* the print from the exit keeps the helper unit-testable without forking a
|
||||
* process (issue #2459).
|
||||
*/
|
||||
export function reportUnrecognizedFlags(
|
||||
args: Pick<Args, "unrecognizedFlags">,
|
||||
write: (text: string) => void = text => process.stderr.write(text),
|
||||
): boolean {
|
||||
if (args.unrecognizedFlags.length === 0) return false;
|
||||
const flags = args.unrecognizedFlags;
|
||||
const plural = flags.length === 1 ? "" : "s";
|
||||
write(`${chalk.red(`Error: unknown flag${plural}: ${flags.join(", ")}`)}\n`);
|
||||
write(`Run \`${APP_NAME} --help\` for available flags.\n`);
|
||||
return true;
|
||||
}
|
||||
|
||||
export function getExtraHelpText(): string {
|
||||
return `${chalk.bold("Environment Variables:")}
|
||||
${chalk.dim("# Core Providers")}
|
||||
|
||||
@@ -21,7 +21,7 @@ import {
|
||||
} from "@oh-my-pi/pi-utils";
|
||||
import chalk from "chalk";
|
||||
import { reset as resetCapabilities } from "./capability";
|
||||
import type { Args } from "./cli/args";
|
||||
import { type Args, reportUnrecognizedFlags } from "./cli/args";
|
||||
import { applyExtensionFlags, type ExtensionFlagSink } from "./cli/extension-flags";
|
||||
import { processFileArguments } from "./cli/file-processor";
|
||||
import { buildInitialMessage } from "./cli/initial-message";
|
||||
@@ -1197,6 +1197,15 @@ export async function runRootCommand(
|
||||
},
|
||||
};
|
||||
const initialArgs = applyExtensionFlags(extensionFlagSink, rawArgs) ?? parsedArgs;
|
||||
// Fail fast on stale/typo flags (e.g. `omp --list-models`) now that we
|
||||
// know the real extension flag set. Without this check the unrecognized
|
||||
// token gets silently consumed and any following positional leaks as the
|
||||
// initial prompt — kicking off a real LLM session, MCP connection, and
|
||||
// tool calls (issue #2459). Exit code 2 matches the conventional
|
||||
// "command line usage error" convention.
|
||||
if (reportUnrecognizedFlags(initialArgs)) {
|
||||
process.exit(2);
|
||||
}
|
||||
const processedFiles =
|
||||
initialArgs.fileArgs.length > 0
|
||||
? await logger.time("processFileArguments", () =>
|
||||
|
||||
@@ -179,6 +179,7 @@ describe("ACP lazy startup", () => {
|
||||
messages: [],
|
||||
fileArgs: [],
|
||||
unknownFlags: new Map(),
|
||||
unrecognizedFlags: [],
|
||||
noSkills: true,
|
||||
noRules: true,
|
||||
noTools: true,
|
||||
@@ -330,6 +331,7 @@ describe("ACP lazy startup", () => {
|
||||
messages: [],
|
||||
fileArgs: [],
|
||||
unknownFlags: new Map(),
|
||||
unrecognizedFlags: [],
|
||||
noSkills: true,
|
||||
noRules: true,
|
||||
noTools: true,
|
||||
|
||||
@@ -0,0 +1,139 @@
|
||||
import { describe, expect, it } from "bun:test";
|
||||
import { parseArgs, reportUnrecognizedFlags } from "@oh-my-pi/pi-coding-agent/cli/args";
|
||||
import { applyExtensionFlags } from "@oh-my-pi/pi-coding-agent/cli/extension-flags";
|
||||
|
||||
// Regression coverage for issue #2459: `omp --list-models` (a stale flag) was
|
||||
// silently consumed as a prompt instead of failing fast — the agent started a
|
||||
// real session, connected to MCP, and hung waiting for the model. Any
|
||||
// `--`-prefixed token that does not match a built-in OR an extension-registered
|
||||
// flag must surface as a hard error before any session/MCP work happens.
|
||||
describe("parseArgs — unrecognized flag tracking (#2459)", () => {
|
||||
it("records a bare unknown --flag instead of silently consuming it", () => {
|
||||
const parsed = parseArgs(["--list-models"]);
|
||||
|
||||
expect(parsed.unrecognizedFlags).toEqual(["--list-models"]);
|
||||
expect(parsed.messages).toEqual([]);
|
||||
});
|
||||
|
||||
it("records the unknown flag without letting `--flag=value` leak `value` into messages", () => {
|
||||
const parsed = parseArgs(["--list-models=verbose", "hello"]);
|
||||
|
||||
expect(parsed.unrecognizedFlags).toEqual(["--list-models"]);
|
||||
// "verbose" was the spliced equals-value and gets dropped by the
|
||||
// unconsumed-equals-value guard; only the real positional survives.
|
||||
expect(parsed.messages).toEqual(["hello"]);
|
||||
});
|
||||
|
||||
it("records a typo in a known flag name and stops the value from binding", () => {
|
||||
const parsed = parseArgs(["--modle", "opus", "hi"]);
|
||||
|
||||
expect(parsed.unrecognizedFlags).toEqual(["--modle"]);
|
||||
expect(parsed.model).toBeUndefined();
|
||||
// `opus` still leaks into messages here (no way to tell from one token
|
||||
// whether it was the flag's intended value); the caller exits on the
|
||||
// non-empty unrecognizedFlags before the prompt is ever sent.
|
||||
expect(parsed.messages).toEqual(["opus", "hi"]);
|
||||
});
|
||||
|
||||
it("records an unknown short flag (`-x`)", () => {
|
||||
const parsed = parseArgs(["-x", "msg"]);
|
||||
|
||||
expect(parsed.unrecognizedFlags).toEqual(["-x"]);
|
||||
expect(parsed.messages).toEqual(["msg"]);
|
||||
});
|
||||
|
||||
it("leaves built-in flags out of unrecognizedFlags", () => {
|
||||
const parsed = parseArgs(["--print", "--model", "opus", "hi"]);
|
||||
|
||||
expect(parsed.unrecognizedFlags).toEqual([]);
|
||||
expect(parsed.print).toBe(true);
|
||||
expect(parsed.model).toBe("opus");
|
||||
expect(parsed.messages).toEqual(["hi"]);
|
||||
});
|
||||
|
||||
it("treats `-` (stdin marker) and `--` (POSIX separator) as non-flags, not unrecognized", () => {
|
||||
// `-` is a stdin marker by convention and shows up in pipelines; `--`
|
||||
// is the POSIX positional separator. Neither is a typo and neither
|
||||
// should fire the unknown-flag error.
|
||||
const dash = parseArgs(["-"]);
|
||||
expect(dash.unrecognizedFlags).toEqual([]);
|
||||
expect(dash.messages).toEqual(["-"]);
|
||||
|
||||
const ddash = parseArgs(["--", "hello"]);
|
||||
expect(ddash.unrecognizedFlags).toEqual([]);
|
||||
// `--` itself is dropped (it is a separator, not a message), `hello`
|
||||
// is the positional.
|
||||
expect(ddash.messages).toEqual(["hello"]);
|
||||
});
|
||||
|
||||
it("clears extension-registered flags from unrecognizedFlags on the post-extension reparse", () => {
|
||||
const argv = ["--spawn-peer", "reviewer", "review the diff"];
|
||||
|
||||
// Startup parse: extensions not loaded yet → unknown.
|
||||
const startup = parseArgs(argv);
|
||||
expect(startup.unrecognizedFlags).toEqual(["--spawn-peer"]);
|
||||
|
||||
// Extension-aware reparse: now recognized → unrecognizedFlags clear.
|
||||
const reparsed = parseArgs(argv, new Map([["spawn-peer", { type: "string" }]]));
|
||||
expect(reparsed.unrecognizedFlags).toEqual([]);
|
||||
expect(reparsed.unknownFlags.get("spawn-peer")).toBe("reviewer");
|
||||
expect(reparsed.messages).toEqual(["review the diff"]);
|
||||
});
|
||||
|
||||
it("keeps a genuine typo in unrecognizedFlags after an extension-aware reparse", () => {
|
||||
// `--spawn-peer` is an extension flag, `--list-models` is a typo. After
|
||||
// the extension-aware reparse only the typo remains and the caller
|
||||
// surfaces it.
|
||||
const argv = ["--spawn-peer", "reviewer", "--list-models"];
|
||||
const reparsed = parseArgs(argv, new Map([["spawn-peer", { type: "string" }]]));
|
||||
|
||||
expect(reparsed.unrecognizedFlags).toEqual(["--list-models"]);
|
||||
expect(reparsed.unknownFlags.get("spawn-peer")).toBe("reviewer");
|
||||
});
|
||||
|
||||
it("propagates unrecognizedFlags through applyExtensionFlags so callers can surface them", () => {
|
||||
const runner = {
|
||||
getFlags: () => new Map<string, { type: "boolean" | "string" }>([["spawn-peer", { type: "string" }]]),
|
||||
setFlagValue: () => {},
|
||||
};
|
||||
const parsed = applyExtensionFlags(runner, ["--spawn-peer", "reviewer", "--typo"]);
|
||||
|
||||
expect(parsed).not.toBeNull();
|
||||
expect(parsed?.unrecognizedFlags).toEqual(["--typo"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("reportUnrecognizedFlags — stderr error surface", () => {
|
||||
function capture(unrecognized: string[]): { wrote: string[]; reported: boolean } {
|
||||
const wrote: string[] = [];
|
||||
const reported = reportUnrecognizedFlags({ unrecognizedFlags: unrecognized }, text => {
|
||||
wrote.push(text);
|
||||
});
|
||||
return { wrote, reported };
|
||||
}
|
||||
|
||||
it("returns false and writes nothing when there are no unrecognized flags", () => {
|
||||
const { wrote, reported } = capture([]);
|
||||
|
||||
expect(reported).toBe(false);
|
||||
expect(wrote).toEqual([]);
|
||||
});
|
||||
|
||||
it("returns true and writes a singular `unknown flag` line for one entry", () => {
|
||||
const { wrote, reported } = capture(["--list-models"]);
|
||||
|
||||
expect(reported).toBe(true);
|
||||
const text = wrote.join("");
|
||||
expect(text).toContain("Error: unknown flag: --list-models");
|
||||
// `--help` hint guides the user toward the actual surface.
|
||||
expect(text).toContain("--help");
|
||||
});
|
||||
|
||||
it("uses the plural form and joins multiple flags when several are unrecognized", () => {
|
||||
const { wrote, reported } = capture(["--foo", "--bar"]);
|
||||
|
||||
expect(reported).toBe(true);
|
||||
const text = wrote.join("");
|
||||
expect(text).toContain("Error: unknown flags: --foo, --bar");
|
||||
});
|
||||
});
|
||||
@@ -8,6 +8,7 @@ function createArgs(messages: string[]): Args {
|
||||
messages,
|
||||
fileArgs: [],
|
||||
unknownFlags: new Map(),
|
||||
unrecognizedFlags: [],
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -25,6 +25,7 @@ function buildArgs(resume: string, sessionDir?: string): Args {
|
||||
messages: [],
|
||||
fileArgs: [],
|
||||
unknownFlags: new Map(),
|
||||
unrecognizedFlags: [],
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -17,6 +17,7 @@ function buildResumeArgs(resume: string): Args {
|
||||
messages: [],
|
||||
fileArgs: [],
|
||||
unknownFlags: new Map(),
|
||||
unrecognizedFlags: [],
|
||||
};
|
||||
}
|
||||
|
||||
@@ -27,6 +28,7 @@ function buildForkArgs(fork: string, noSession = false): Args {
|
||||
messages: [],
|
||||
fileArgs: [],
|
||||
unknownFlags: new Map(),
|
||||
unrecognizedFlags: [],
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user