Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 158 additions & 2 deletions packages/cli/src/lib/argv-hoist.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,154 @@ export function isVersionRequest(argv: readonly string[]): boolean {
return false;
}

/**
* Accumulator for {@link rewriteHelpJsonRequest} while scanning argv.
*/
type HelpJsonScan = {
hasHelp: boolean;
hasJson: boolean;
commandPath: string[];
fields: string | undefined;
};

/**
* True when `token` is a boolean-style flag that does NOT consume a following
* value token — a known boolean global flag (`--verbose`, `--json`, its `-v`
* alias, or a `--no-<flag>` negation). Everything else that starts with `-` is
* assumed to be value-taking, so its next token is a flag value rather than a
* command-path segment.
*
* Used by {@link scanHelpJsonToken} to avoid swallowing a real path segment
* after a boolean flag (`issue --verbose list`) while still discarding the
* values of value flags (`--org acme`, `--limit 5`).
*/
function isBooleanFlagToken(token: string): boolean {
if (token.length === 2 && token[0] === "-" && token[1] !== "-") {
const flag = FLAG_BY_SHORT.get(token[1] ?? "");
return flag ? !flag.takesValue : false;
}
if (!token.startsWith("--")) {
return false;
}
const name = token.slice(2);
if (name.startsWith("no-") && NEGATABLE_NAMES.has(name.slice(3))) {
return true;
}
const flag = FLAG_BY_NAME.get(name);
return flag ? !flag.takesValue : false;
}

/**
* Fold a single argv token into the {@link HelpJsonScan} accumulator.
*
* Recognizes `--help` (and its `-h` alias), `--json`, and `--fields` (both
* spaced and `=` forms), collects non-flag tokens as the command path, and
* drops all other flags — including the spaced value of any value-taking flag
* (`--org acme`, `--limit 5`) so those values never leak into the resolved
* command path.
*
* @returns The number of tokens consumed (1, or 2 when a value flag's spaced
* value is dropped alongside it).
*/
function scanHelpJsonToken(
argv: readonly string[],
index: number,
scan: HelpJsonScan
): number {
const token = argv[index] ?? "";
// `-h` is Stricli's built-in short alias for `--help`, so it must trigger the
// JSON rewrite too — otherwise `sentry -h --json` falls through to text usage.
if (token === "--help" || token === "-h") {
scan.hasHelp = true;
return 1;
}
if (token === "--json") {
scan.hasJson = true;
return 1;
}
if (token.startsWith("--fields=")) {
scan.fields = token.slice("--fields=".length);
return 1;
}
const next = argv[index + 1];
// A spaced value is only present when the next token isn't itself a flag —
// `--fields --json` leaves --fields valueless rather than eating --json.
const hasSpacedValue = next !== undefined && !next.startsWith("-");
if (token === "--fields") {
if (hasSpacedValue) {
scan.fields = next;
return 2;
}
return 1;
}
if (!token.startsWith("-")) {
scan.commandPath.push(token);
return 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Positionals leak into help path

Medium Severity

rewriteHelpJsonRequest treats every non-flag token as a command path segment, so positionals like an issue id are forwarded into help --json …. introspectCommand then rejects the extra segment and returns a not-found JSON error instead of help for the leaf command. Stricli’s bare --help ignores those positionals, so adding --json regresses those invocations from usable text help to an error.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0d4f5d6. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this as-is by design. The rewrite is scoped to command paths (sentry [<group> <command>] --help --json) — the forms agents actually use to discover a command's shape. When a positional like an issue id is present, forwarding it produces a structured JSON Command not found error (exit 60), which is the documented, machine-readable behavior for the JSON/agent use case this feature targets; it doesn't crash or hang. Making <command> <positional> --help --json fall back to the leaf command's help would require running route-tree resolution inside the argv preprocessor (argv-hoist.ts is currently a dependency-light string transform on the hot preprocessArgv path). Given this PR is already risk: high, I'd rather not couple the preprocessor to the command graph here — happy to do it as a follow-up if maintainers want the fallback.

}
// Any other flag is irrelevant to the help command's structured output and
// is dropped. A value flag (`--org acme`, `--limit 5`) also drops its spaced
// value so it isn't mistaken for a command-path segment; a boolean flag
// (`--verbose`) leaves the following token for the path. An `=`-form flag
// (`--org=acme`) already carries its value inline, so it never consumes the
// following token — dropping it would swallow a real command-path segment.
if (hasSpacedValue && !token.includes("=") && !isBooleanFlagToken(token)) {
return 2;
}
Comment thread
cursor[bot] marked this conversation as resolved.
return 1;
Comment thread
cursor[bot] marked this conversation as resolved.
}

/**
* Rewrite a flag-based `--help --json` request into a `help` command invocation.
*
* Stricli handles `--help` internally by printing its own text usage and
* ignores `--json` entirely, so `sentry --help --json` and
* `sentry <command> --help --json` never produce structured output. Agents and
* tooling reach for `--help` first, so we rewrite these forms to the dedicated
* `help` command — which already emits JSON via {@link introspectAllCommands}
* and {@link introspectCommand} — giving both help UX paths identical JSON.
*
* The rewrite only fires when **both** `--help` and `--json` appear before any
* `--` escape separator. A bare `--help` (no `--json`) is left untouched so
* Stricli's existing human usage output is preserved unchanged.
*
* The command path is the sequence of non-flag tokens (e.g. `issue list`), and
* a `--fields <value>` (or `--fields=<value>`) flag is carried through so field
* selection keeps working. The result is `["help", "--json", ...path]` with
* `--fields` appended when present.
*
* @param argv - Raw CLI arguments (e.g., `process.argv.slice(2)`)
* @returns The rewritten `help`-command argv, or `null` if the request is not a
* `--help --json` combination and should be processed normally.
*/
export function rewriteHelpJsonRequest(
argv: readonly string[]
): string[] | null {
const scan: HelpJsonScan = {
hasHelp: false,
hasJson: false,
commandPath: [],
fields: undefined,
};

for (let i = 0; i < argv.length; ) {
// Tokens after -- are positional/pass-through — a --help there is not ours.
if (argv[i] === "--") {
return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escape aborts valid help rewrite

Low Severity

On encountering --, rewriteHelpJsonRequest returns null immediately even when --help and --json were already seen earlier in argv. The documented rule is only that those flags must appear before the escape; a later -- should stop scanning, not discard a rewrite that already qualified.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0d4f5d6. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. Returning null on any pre--- scan hitting -- keeps the rule simple and conservative: a -- escape means everything after it is opaque pass-through, and I'd rather defer to Stricli's normal handling than commit to a JSON rewrite when an escape is in play (documented in the PR as -- tool --help --json not being rewritten). In practice --help --json ... -- combined with a trailing escape is not a form agents use for help discovery, so the extra complexity to salvage it isn't worth the risk on this path. Leaving as-is.

}
i += scanHelpJsonToken(argv, i, scan);
}

if (!(scan.hasHelp && scan.hasJson)) {
return null;
}

const rewritten = ["help", "--json", ...scan.commandPath];
if (scan.fields !== undefined) {
rewritten.push("--fields", scan.fields);
}
return rewritten;
}

/**
* Move global flags from any position in argv to the end.
*
Expand Down Expand Up @@ -309,10 +457,14 @@ export function rewriteDashedFlagValues(argv: readonly string[]): string[] {
* Preprocess raw CLI argv before Stricli dispatch.
*
* Composes the argv transforms applied on every invocation:
* 1. A top-level `--version` (see {@link isVersionRequest}) is normalized to a
* 1. A flag-based `--help --json` request (see {@link rewriteHelpJsonRequest})
* is rewritten to the dedicated `help` command so JSON help works for the
* `--help` forms agents reach for (`sentry --help --json`,
* `sentry issue --help --json`), matching `sentry help --json`.
* 2. A top-level `--version` (see {@link isVersionRequest}) is normalized to a
* plain `["--version"]` so the application-level version handler prints it
* regardless of how deep in the route tree it appeared.
* 2. Otherwise, dashed flag values are rewritten (see
* 3. Otherwise, dashed flag values are rewritten (see
* {@link rewriteDashedFlagValues}), then global flags are hoisted to the
* tail (see {@link hoistGlobalFlags}).
*
Expand All @@ -323,6 +475,10 @@ export function rewriteDashedFlagValues(argv: readonly string[]): string[] {
* @returns The argv to hand to Stricli's `run`
*/
export function preprocessArgv(argv: readonly string[]): string[] {
const helpJson = rewriteHelpJsonRequest(argv);
if (helpJson) {
return helpJson;
}
if (isVersionRequest(argv)) {
return ["--version"];
}
Expand Down
172 changes: 172 additions & 0 deletions packages/cli/test/lib/argv-hoist.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
isVersionRequest,
preprocessArgv,
rewriteDashedFlagValues,
rewriteHelpJsonRequest,
} from "../../src/lib/argv-hoist.js";

describe("hoistGlobalFlags", () => {
Expand Down Expand Up @@ -466,6 +467,20 @@ describe("preprocessArgv", () => {
]);
});

test("rewrites --help --json to the help command instead of hoisting", () => {
expect(preprocessArgv(["--help", "--json"])).toEqual(["help", "--json"]);
expect(preprocessArgv(["issue", "list", "--help", "--json"])).toEqual([
"help",
"--json",
"issue",
"list",
]);
});

test("leaves a bare --help to normal hoisting (Stricli renders text help)", () => {
expect(preprocessArgv(["issue", "--help"])).toEqual(["issue", "--help"]);
});

test("leaves a wrapped-command --version (after --) to hoisting, not version", () => {
expect(
preprocessArgv(["monitor", "run", "job", "--", "tool", "--version"])
Expand Down Expand Up @@ -509,3 +524,160 @@ describe("preprocessArgv", () => {
).toEqual(["release", "set-commits", "1.0.0", "--from", "--auto"]);
});
});

describe("rewriteHelpJsonRequest", () => {
test("rewrites top-level --help --json to the help command", () => {
expect(rewriteHelpJsonRequest(["--help", "--json"])).toEqual([
"help",
"--json",
]);
});

test("rewrites a group --help --json to help <group>", () => {
expect(rewriteHelpJsonRequest(["issue", "--help", "--json"])).toEqual([
"help",
"--json",
"issue",
]);
});

test("recognizes the -h short alias for --help", () => {
// Stricli treats `-h` as an alias of `--help`, so the JSON rewrite must
// fire for it too — otherwise `sentry -h --json` falls through to text usage.
expect(rewriteHelpJsonRequest(["-h", "--json"])).toEqual([
"help",
"--json",
]);
expect(rewriteHelpJsonRequest(["issue", "-h", "--json"])).toEqual([
"help",
"--json",
"issue",
]);
});

test("rewrites a nested command --help --json to help <group> <command>", () => {
expect(
rewriteHelpJsonRequest(["issue", "list", "--help", "--json"])
).toEqual(["help", "--json", "issue", "list"]);
});

test("is order-insensitive between --help and --json", () => {
expect(rewriteHelpJsonRequest(["--json", "issue", "--help"])).toEqual([
"help",
"--json",
"issue",
]);
});

test("carries a --fields value through to the help command", () => {
expect(
rewriteHelpJsonRequest([
"issue",
"list",
"--help",
"--json",
"--fields",
"path,brief",
])
).toEqual(["help", "--json", "issue", "list", "--fields", "path,brief"]);
});

test("carries a --fields=value form through to the help command", () => {
expect(
rewriteHelpJsonRequest(["issue", "--help", "--json", "--fields=path"])
).toEqual(["help", "--json", "issue", "--fields", "path"]);
});

test("drops unrelated flags from the rewritten path", () => {
expect(
rewriteHelpJsonRequest(["--verbose", "issue", "--help", "--json"])
).toEqual(["help", "--json", "issue"]);
});

test("drops a value flag's spaced value so it never becomes a path segment", () => {
// `--org acme` / `--limit 5` must not leak `acme` / `5` into the command
// path, which would resolve the wrong command or a not-found error.
expect(
rewriteHelpJsonRequest([
"issue",
"list",
"--org",
"acme",
"--help",
"--json",
])
).toEqual(["help", "--json", "issue", "list"]);
expect(
rewriteHelpJsonRequest([
"issue",
"list",
"--limit",
"5",
"--help",
"--json",
])
).toEqual(["help", "--json", "issue", "list"]);
});

test("keeps a path segment following a boolean flag", () => {
// `--verbose` is a known boolean flag, so the token after it (`list`) is a
// real command-path segment, not a flag value.
expect(
rewriteHelpJsonRequest(["issue", "--verbose", "list", "--help", "--json"])
).toEqual(["help", "--json", "issue", "list"]);
});

test("keeps a path segment following an =-form value flag", () => {
// `--org=acme` carries its value inline, so the next token (`issue`/`list`)
// is a real command-path segment. A naive length check would treat the
// whole `org=acme` string as an unknown value flag and swallow `issue`.
expect(
rewriteHelpJsonRequest([
"--org=acme",
"issue",
"list",
"--help",
"--json",
])
).toEqual(["help", "--json", "issue", "list"]);
expect(
rewriteHelpJsonRequest(["issue", "--limit=5", "list", "--help", "--json"])
).toEqual(["help", "--json", "issue", "list"]);
});

test("does not let --fields swallow a following flag", () => {
// `--fields --json`: --fields has no value, and --json must still register
// so the rewrite fires.
expect(
rewriteHelpJsonRequest(["issue", "list", "--help", "--fields", "--json"])
).toEqual(["help", "--json", "issue", "list"]);
});

test("returns null for bare --help without --json", () => {
expect(rewriteHelpJsonRequest(["issue", "--help"])).toBeNull();
});

test("returns null for --json without --help", () => {
expect(rewriteHelpJsonRequest(["issue", "list", "--json"])).toBeNull();
});

test("returns null when neither flag is present", () => {
expect(rewriteHelpJsonRequest(["issue", "list"])).toBeNull();
});

test("ignores --help --json after the -- escape separator", () => {
// `sentry monitor run <slug> -- tool --help --json` must forward the flags
// to the wrapped command, not print the CLI's JSON help.
expect(
rewriteHelpJsonRequest([
"monitor",
"run",
"job",
"--",
"tool",
"--help",
"--json",
])
).toBeNull();
});
});
Loading