feat(companion): handle --help per subcommand so it cannot start a run - #681
feat(companion): handle --help per subcommand so it cannot start a run#681cjsteigerwald wants to merge 9 commits into
Conversation
`--help` is only recognised as the first argument. For anything else it lands in argv, and because every subcommand parser treats an unrecognised token as a positional, it is carried into the command as data. For `adversarial-review` that means `--help` is joined into the review's focus text and a full review runs: minutes of wall clock and a real model turn, for someone who asked what the flags were. The same shape applies to any subcommand whose parser accepts positionals. main() now checks for --help, -h or help in argv before the dispatch switch, and prints usage for that subcommand alone. Checking before the switch is the point: a help request can never reach a handler that would dispatch. Bare `--help`, `-h` and `help` as the subcommand still print the full usage block, unchanged. The usage lines move into a Map keyed by subcommand so a single line can be printed without duplicating the text. The full block prints in the same order as before. Tests: `adversarial-review --help` prints usage and — the assertion that matters — starts no Codex turn, checked against the fake app server's recorded lastTurnStart; and `task -h` prints only the task line. Both verified non-vacuous against db52e28, where they fail.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6622ade45f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| // Checked before the switch, so help can never reach a handler that would dispatch. | ||
| if (USAGE_LINES.has(subcommand) && isHelpRequest(argv)) { |
There was a problem hiding this comment.
The plugin entrypoints pass $ARGUMENTS as one quoted argument (for example, plugins/codex/commands/adversarial-review.md:50), so /codex:adversarial-review --base main --help reaches this check with argv equal to ["--base main --help"]. isHelpRequest therefore returns false; the handler subsequently splits the string and starts a full focused review with --help as focus text, preserving the costly behavior this change is intended to prevent whenever help is combined with another option. Normalize argv before performing this check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5770f78 — thank you, this was the difference between the fix working and only appearing to work.
I reproduced it exactly as you described. Running the real invocation shape started a genuine review:
$ node scripts/codex-companion.mjs adversarial-review "--base main --help"
[codex] Starting Codex task thread.
[codex] Turn started (01a03503-8c7c-7d10-891f-b37dce89f48f).
I had to cancel that job. My tests passed discrete argv entries, which is not how the plugin commands invoke this — they pass "$ARGUMENTS" as one quoted argument — so the tests never exercised the path that actually matters.
The check now runs normalizeArgv(argv) first, the same normalization parseCommandInput applies, so detection sees the tokens the handler would.
Normalizing brings a consequence worth calling out, because fixing this naively introduces a new bug: the bare word help becomes a token in ordinary focus text, so adversarial-review "review the help system" would print usage instead of running the review. isHelpRequest therefore matches only --help and -h. A bare help subcommand is still handled separately before dispatch, so codex-companion.mjs help is unchanged.
Two tests:
- help combined with another flag in a single quoted string prints usage and starts no turn — verified non-vacuous against 6622ade, where it fails
- focus text containing "help" still reaches
turn/startwith the focus intact. To be straight about it, this one passes with and without the fix: it is a guard against the regression normalizing would otherwise introduce, not evidence of the original bug.
Full suite: 95 passing / 0 failing of 95.
|
Correction to my earlier test-suite claims on this PR. I reported 3 pre-existing failures on
function filterJobsForCurrentClaudeSession(jobs) {
const sessionId = getCurrentClaudeSessionId();
if (!sessionId) return jobs;
return jobs.filter((job) => job.sessionId === sessionId);
}The Unsetting the variable turns all three green, and every branch is fully green: So: there are no pre-existing failures on One thing this does suggest: since this plugin is for Claude Code, contributors are likely to run |
Addresses the P1 review finding on openai#681. The plugin commands invoke the companion with "$ARGUMENTS" as a single quoted argument (plugins/codex/commands/adversarial-review.md:50), so `/codex:adversarial-review --base main --help` reaches main() as argv === ["--base main --help"]. Comparing raw tokens misses the flag, the request falls through to the handler, and parseCommandInput then splits the string itself and starts a full review with --help as focus text -- preserving the exact behaviour this change exists to prevent whenever help is combined with any other argument. The check now runs normalizeArgv(argv) first, which is the same normalization parseCommandInput applies, so detection sees the same tokens the handler would. Normalizing has a consequence that must be handled at the same time: the bare word "help" now appears as a token in ordinary focus text. Matching it would turn `adversarial-review "review the help system"` into a usage dump instead of the review the user asked for. isHelpRequest therefore matches only --help and -h. A bare `help` subcommand is still handled separately before dispatch, so `codex-companion.mjs help` is unaffected. Tests: - help combined with another flag in one quoted string prints usage and starts no Codex turn. Verified non-vacuous against 6622ade, where it fails. - focus text containing the word "help" still runs the review and reaches turn/start with the focus intact. This one passes both with and without the fix: it is a guard against the regression normalizing would otherwise introduce, not evidence of the original bug.
|
@codex review 5770f78 is the current head and has not been reviewed — the previous pass covered 6622ade. It normalizes Worth scrutinising: whether |
Self-review follow-up to 5770f78, which introduced this while fixing the P1. Normalizing argv before help detection is necessary, but it also splits focus text into tokens. A token scan therefore matched user prose: `adversarial-review "why does --help start a review"` produced a --help token and printed usage, so that review could never be run at all. That is a hard block on legitimate input, not merely a surprise, and it is a defect the previous commit introduced. Help detection now parses rather than scans, and treats the request as help only when the flag is present AND nothing else was asked for -- no focus text left over: return options.help === true && positionals.length === 0; That keeps every case right: --help -> usage --base main --help -> usage (main is consumed as a value) --model spark -h -> usage "why does --help start a review" -> review, focus intact "review the help system" -> review, focus intact Test: focus text containing --help reaches turn/start with the focus preserved. Verified non-vacuous against 5770f78, where it fails. Full suite: 96 passing / 0 failing of 96.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5770f78873
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // raw token comparison misses the flag -- the handler would then split it itself and | ||
| // start a full review with --help as focus text, which is exactly what this prevents. | ||
| // Checked before the switch, so help can never reach a handler that would dispatch. | ||
| if (USAGE_LINES.has(subcommand) && isHelpRequest(normalizeArgv(argv))) { |
There was a problem hiding this comment.
When the companion is invoked directly with a single quoted focus argument, such as adversarial-review "review --help handling", the shell removes the quotes and normalizeArgv splits that one positional into review, --help, and handling; this check then prints usage instead of running the requested focused review. Escaped/quoted raw tokens and tokens after the parser's -- delimiter have the same problem because help detection discards that positional context, so detect help without treating protected focus text as a flag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed in 50648ea, before this comment arrived — I found the same thing reviewing my own change and posted it above.
adversarial-review "review --help handling" now runs the focused review with the focus intact, because help requires the flag and no focus text left over rather than a token match.
Your point about the -- delimiter is handled too, and by the same mechanism rather than a special case: after addad6f, detection parses with the subcommand's real schema, so anything following -- is a positional and a literal --help in focus text stays focus text.
"review --help handling" -> review, focus intact
"-- --help" -> review, focus intact
There is a regression test for the quoted-focus case. To be straight about it, that test passes both with and without the fix, so it is a guard against reintroducing the token-scan approach rather than evidence of the original bug.
|
Follow-up: I checked the concern I raised in my own review request, and it was real — so 50648ea fixes it before it reaches you. Normalizing argv is necessary for the P1 fix, but it also splits focus text into tokens. My 5770f78 token scan therefore matched user prose: That review could never be run at all — a hard block on legitimate input, and a defect 5770f78 introduced while fixing the P1. Help detection now parses instead of scanning, and treats it as help only when the flag is present and nothing else was asked for: return options.help === true && positionals.length === 0;Behaviour across the cases that matter:
Test asserts the focus text reaches Current head is 50648ea. Full suite 96 passing / 0 failing of 96. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50648ea762
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function isHelpRequest(argv) { | ||
| const { options, positionals } = parseArgs(normalizeArgv(argv), { | ||
| valueOptions: HELP_DETECTION_VALUE_OPTIONS, | ||
| booleanOptions: ["help"], |
There was a problem hiding this comment.
When help follows a valid boolean option, such as the plugin-shaped adversarial-review "--wait --help", this parser recognizes only help, so --wait becomes a positional and isHelpRequest returns false. The actual review parser then consumes --wait while leaving --help as focus text, starting a full Codex review instead of printing usage. Fresh evidence in the current head is that the help parser omits the boolean options accepted by each handler; reuse the subcommand's option schema or include those options here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in addad6f — this was a real bug in the head you reviewed.
adversarial-review "--wait --help" put --wait into positionals (my help parser did not know it), the "no focus text left over" rule saw a positional and returned false, and the real parser then consumed --wait and reviewed --help as focus text.
I took your first suggestion rather than the second, because the hand-maintained list was the defect — its contents were only the symptom, and any option added to a handler later would have reopened the gap silently. There is now one schema per subcommand:
const COMMAND_OPTION_SCHEMAS = new Map([
["setup", { valueOptions: ["cwd"], booleanOptions: ["json", "enable-review-gate", "disable-review-gate"] }],
["review", REVIEW_OPTION_SCHEMA],
["adversarial-review", REVIEW_OPTION_SCHEMA],
...
]);read by both the handler and help detection. The handlers' inline literals are now lookups into it, which is the only way the guarantee holds over time.
Behaviour:
| input | result |
|---|---|
--wait --help |
usage |
--background --json --help |
usage |
--base main --help |
usage |
status --all --help |
usage |
"why does --help start a review" |
review, focus intact |
"review --help handling" |
review, focus intact |
"-- --help" |
review, focus intact |
Test asserts --wait --help prints usage and starts no turn; non-vacuous against 50648ea, where it fails. Full suite 98 passing / 0 failing of 98.
Addresses the P1 review finding on 50648ea. Help detection parsed with its own hand-maintained option list, so it did not know options the handlers accept. `adversarial-review "--wait --help"` therefore put --wait into positionals, the "no focus text left over" rule saw a positional and returned false, and the real parser then consumed --wait and started a full review with --help as focus text -- the behaviour this PR exists to prevent. The list was the defect, not its contents: any option added to a handler later would reintroduce the same gap silently. There is now one schema per subcommand in COMMAND_OPTION_SCHEMAS, read by both the handler and help detection, so the two cannot disagree. The handlers' inline literals are replaced by lookups into it, which is the only way the guarantee holds over time. Parsing with the real schema also fixes the `--` case for free: anything after the delimiter is a positional, so a literal `--help` in focus text stays focus text rather than being read as a flag. Behaviour: --help -> usage --wait --help -> usage --background --json --help -> usage --base main --help -> usage status --all --help -> usage "why does --help start a review" -> review, focus intact "review --help handling" -> review, focus intact "-- --help" -> review, focus intact Tests: help following another recognized flag prints usage and starts no turn (non-vacuous against 50648ea, where it fails); and a quoted focus argument keeps its help-looking words, which passes both ways and is a guard rather than evidence, since 50648ea already handled that case. Full suite: 98 passing / 0 failing of 98.
|
@codex review Current head is addad6f; the previous passes covered 6622ade, 5770f78 and 50648ea. This round replaces the hand-maintained help-detection option list with one Worth scrutinising:
|
addad6f claimed the handlers' inline parse literals were replaced by lookups into COMMAND_OPTION_SCHEMAS. That was true for setup, review, adversarial-review, task, transfer and status, but not for result and cancel, which kept literals duplicating their map entries. Those two could therefore still drift from help detection -- the exact failure that commit set out to make impossible. Both now read the shared schema, so every subcommand in USAGE_LINES has one definition. handleTaskWorker and handleTaskResumeCandidate keep their own literals deliberately: neither is in USAGE_LINES, so neither is help-handled, and task-worker's --job-id has no business in a user-facing schema. No behaviour change -- the literals and the map entries were identical, which is why the suite did not catch the gap. Verified by hand as well: result --json -> parses (reports no finished jobs) cancel --json -> parses (reports nothing to cancel) result --help -> usage cancel "--json --help" -> usage Full suite: 98 passing / 0 failing of 98.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: addad6f071
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const { options, positionals } = parseArgs(normalizeArgv(argv), { | ||
| valueOptions: schema.valueOptions ?? [], | ||
| booleanOptions: [...(schema.booleanOptions ?? []), "help"], | ||
| aliasMap: { ...(schema.aliasMap ?? {}), h: "help" } | ||
| }); |
There was a problem hiding this comment.
When help is combined with the accepted -C <dir> option, this alias map does not recognize -C, so it and its value become positionals and isHelpRequest returns false. parseCommandInput subsequently recognizes -C as cwd but leaves --help as positional input, causing commands such as task or adversarial-review to dispatch work instead of printing usage. Include the shared C: "cwd" alias here or perform help detection through parseCommandInput.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a057dac — and worth flagging that this still reproduced on fda50dc, which the later automated pass reported clean. I verified before fixing:
adversarial-review "-C /tmp --help" -> started a review
task "-C /tmp --help" -> started a task
status "-C /tmp --help" -> looked for a job named "--help"
adversarial-review "--cwd /tmp --help" -> Usage: (control)
Root cause exactly as you describe: isHelpRequest called parseArgs directly, so it never saw the C: "cwd" alias that parseCommandInput injects.
I took your second suggestion rather than the first. Adding C: "cwd" to the alias map here would have worked today, but it is the same shape as the bug you caught in the previous round — a second copy of the parser's knowledge that drifts the moment a shared alias is added. Detection now goes through parseCommandInput, the entry point the handlers use, so it inherits the alias and the argv normalization instead of restating them:
const { options, positionals } = parseCommandInput(argv, {
...schema,
booleanOptions: [...(schema.booleanOptions ?? []), "help"],
aliasMap: { ...(schema.aliasMap ?? {}), h: "help" }
});That was the last remaining copy.
| input | result |
|---|---|
-C /tmp --help |
usage |
--cwd /tmp --help |
usage |
--wait --help |
usage |
"why does --help start a review" |
review |
"review --help handling" |
review |
"-- --help" |
review |
Test asserts -C <dir> --help prints usage and starts no turn; non-vacuous against fda50dc. Suite 99 passing / 0 failing of 99.
| ["result", { valueOptions: ["cwd"], booleanOptions: ["json"] }], | ||
| ["cancel", { valueOptions: ["cwd"], booleanOptions: ["json"] }] |
There was a problem hiding this comment.
These new result and cancel entries are read only by help detection: handleResult and handleCancel still parse separate inline option literals. Consequently, the promised single-source invariant does not hold for these two public commands, and adding an option to either handler can again make help detection classify that valid option as positional and execute the command rather than show help. Make both handlers consume these map entries like the other public handlers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed in fda50dc, which landed before this comment — I found the same gap self-checking addad6f and said so in the thread.
Your diagnosis matches mine exactly: the result and cancel map entries were read only by help detection while the handlers kept inline literals, so the single-source invariant that commit claimed did not actually hold for those two public commands.
Both handlers now consume the map entries like the others. handleTaskWorker and handleTaskResumeCandidate keep their own literals deliberately — neither is in USAGE_LINES, so neither is help-handled, and task-worker's --job-id is not a user-facing option.
|
Two follow-ups from checking my own work, plus one observation about the suite. 1. addad6f overstated what it did. Its message says the handlers' inline parse literals were replaced by lookups into
No behaviour change: the literals and map entries were identical, which is why the suite did not catch the gap. Verified by hand too: 2. On the transcription risk I raised — I compared every schema against the originals at 6622ade and they match field for field. Worth an independent check rather than my own word. 3. A flaky test, reported as an observation. During this work Current head is fda50dc, suite 98 passing / 0 failing of 98. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…tood Addresses the second P1 on openai#681. It still reproduced on fda50dc, which the automated pass had reported clean. Help detection called parseArgs directly, so it never saw the `C: "cwd"` alias that parseCommandInput injects for every subcommand. `-C <dir> --help` therefore left -C and its value as positionals, the "no focus text left over" rule returned false, and the handler dispatched with --help still in the input: adversarial-review "-C /tmp --help" -> started a review task "-C /tmp --help" -> started a task status "-C /tmp --help" -> looked for a job named "--help" Detection now goes through parseCommandInput, the same entry point the handlers use, so it inherits the shared alias and argv normalization instead of restating them. That is the same reason the option schemas were unified: every copy of the parser's knowledge is a copy that can drift, and this was the last one left. -C /tmp --help -> usage "why does --help start a review" -> review --cwd /tmp --help -> usage "review --help handling" -> review --wait --help -> usage "-- --help" -> review Test: help combined with -C prints usage and starts no turn. Verified non-vacuous against fda50dc, where it fails. Full suite: 99 passing / 0 failing of 99.
|
@codex review Current head is a057dac. Flagging one thing for this round: the P1 about the a057dac routes help detection through Worth scrutinising:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a057dac675
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| booleanOptions: [...(schema.booleanOptions ?? []), "help"], | ||
| aliasMap: { ...(schema.aliasMap ?? {}), h: "help" } | ||
| }); | ||
| return options.help === true && positionals.length === 0; |
There was a problem hiding this comment.
When a job ID accompanies the help flag, as in the plugin-shaped cancel "task-live --help", parsing produces options.help === true and positionals === ["task-live"], so this condition rejects the help request. handleCancel then uses the first positional as the job reference and actually interrupts and marks that job cancelled. Help detection should allow the structured job-ID positional for cancel (and similarly status/result) rather than applying the free-form focus-text rule to every command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 4747ec2. This was the worst of the findings so far — the others wasted a model turn, this one destroyed state. Reproduced on a057dac:
cancel "job-x --help" -> No job found for "job-x". Run /codex:status to list known jobs.
status "job-x --help" -> No job found for "job-x". ...
result "job-x --help" -> No job found for "job-x". ...
cancel "--help" -> Usage: (control)
With a real job present, cancel "task-live --help" cancelled task-live. Asking what a command does should never destroy the thing being asked about.
Your framing was the fix: the "no positionals" rule only makes sense where the positional is arbitrary user prose. Schemas now mark that explicitly, and only free-form subcommands require an empty positional list:
return schema.freeFormPositionals !== true || positionals.length === 0;review, adversarial-review and task are free-form; setup, transfer, status, result and cancel are not.
| input | result |
|---|---|
cancel "job-x --help" |
usage |
status "job-x --help" |
usage |
result "job-x --help" |
usage |
"why does --help start a review" |
review |
"review --help handling" |
review |
task "explain --help output" |
task |
--wait --help, -C /tmp --help |
usage |
The test writes a running job into state, runs cancel "task-live --help", then asserts usage was printed and the job is still running — printing usage alone would not prove the job survived. Non-vacuous against a057dac, where it is cancelled.
Suite 100 passing / 0 failing of 100.
Addresses the P1 on a057dac. This one was destructive rather than wasteful. The "help only when no positionals remain" rule was applied to every subcommand. It exists because review focus text and task prompts are arbitrary user prose, where a --help token may be something the user meant literally. status, result and cancel take a structured job id instead, and there the leftover positional is not ambiguous at all -- so the rule rejected the help request and the handler proceeded: cancel "task-live --help" -> cancelled task-live status "task-live --help" -> looked up task-live result "task-live --help" -> looked up task-live Asking what a command does should never destroy the thing being asked about. Schemas now mark which subcommands take free-form positionals, and only those require the positional list to be empty: return schema.freeFormPositionals !== true || positionals.length === 0; review, adversarial-review and task are free-form; setup, transfer, status, result and cancel are not. cancel "job-x --help" -> usage "why does --help start a review" -> review status "job-x --help" -> usage "review --help handling" -> review result "job-x --help" -> usage "explain --help output" (task) -> task --wait --help / -C /tmp --help -> usage Test: a running job in state, `cancel "task-live --help"`, then assert usage was printed AND the job is still running -- the assertion that matters, since printing usage alone would not prove the job survived. Verified non-vacuous against a057dac, where the job is cancelled. Full suite: 100 passing / 0 failing of 100.
|
@codex review Current head is 4747ec2. This round distinguishes subcommands whose positionals are free-form prose ( Worth scrutinising:
For context on this thread: seven findings so far on this PR, five of them defects introduced by the preceding fix. I would rather over-request review than assume this round converged. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4747ec2956
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // here is automatically known to both, so the two can never disagree again. | ||
| const COMMAND_OPTION_SCHEMAS = new Map([ | ||
| ["setup", { valueOptions: ["cwd"], booleanOptions: ["json", "enable-review-gate", "disable-review-gate"] }], | ||
| ["review", REVIEW_OPTION_SCHEMA], |
There was a problem hiding this comment.
When --help is accompanied by any positional, such as review "--scope working-tree focus --help", this shared classification suppresses help and validateNativeReviewRequest instead rejects the positional as unsupported focus text. Unlike adversarial-review, review has no valid free-form positional use case, so it should not inherit freeFormPositionals: true; help can safely win over every positional for this subcommand.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b15c07a. This is the question I flagged for this round, and you answered it correctly — I had it wrong.
Reproduced on 4747ec2:
review "--scope working-tree focus --help"
-> `/codex:review` ... does not support custom focus text
A help request answered with an unrelated complaint. Your reasoning holds: validateNativeReviewRequest rejects all focus text, so review has no free-form positional use and help can safely win over any positional. It only inherited the classification because it shares a handler with adversarial-review.
The parse options stay shared as REVIEW_PARSE_OPTIONS, since those genuinely are identical. Only the positional semantics differ now:
["review", REVIEW_PARSE_OPTIONS],
["adversarial-review", { ...REVIEW_PARSE_OPTIONS, freeFormPositionals: true }],| input | result |
|---|---|
review "--scope working-tree focus --help" |
usage |
review "some focus text" |
unchanged focus-text error |
adversarial-review "review --help handling" |
review, focus intact |
adversarial-review "--wait --help" |
usage |
cancel "job-x --help" |
usage |
Test asserts usage is printed, the focus-text error is not emitted, and no turn starts. Non-vacuous against 4747ec2. Suite 101 passing / 0 failing of 101.
Addresses the P2 on 4747ec2. review and adversarial-review share a handler, so they shared one schema and review inherited freeFormPositionals: true. But the two differ precisely on positionals: adversarial-review takes focus text, where a --help token may be prose the user meant literally, while validateNativeReviewRequest rejects ALL focus text, so review has no free-form use at all. The consequence was a help request answered with an unrelated complaint: review "--scope working-tree focus --help" -> `/codex:review` ... does not support custom focus text The parse options stay shared as REVIEW_PARSE_OPTIONS, since they genuinely are identical. Only the positional classification differs: adversarial-review adds freeFormPositionals, review does not. review "--scope working-tree focus --help" -> usage review "some focus text" -> unchanged focus-text error adversarial-review "review --help handling" -> review, focus intact cancel "job-x --help" -> usage Test asserts usage is printed, the focus-text error is NOT emitted, and no turn starts. Verified non-vacuous against 4747ec2, where it fails. Full suite: 101 passing / 0 failing of 101.
|
@codex review Current head is b15c07a.
Worth scrutinising:
Eight findings so far on this PR. I am not treating this round as converged. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b15c07ab79
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Only free-form subcommands need to defend against a flag-looking token that the user | ||
| // meant as prose. Where the positional is a structured job id, a leftover positional is | ||
| // not a reason to dispatch -- `cancel "job-1 --help"` must print usage, not cancel job-1. | ||
| return schema.freeFormPositionals !== true || positionals.length === 0; |
There was a problem hiding this comment.
When task receives both --prompt-file and a positional, the positional does not protect literal prompt text because readTaskPrompt unconditionally uses the file, yet this rule suppresses help whenever that ignored positional exists. For example, plugin-shaped input task "--prompt-file prompt.txt ignored --help" starts a real Codex turn using prompt.txt instead of printing usage; make the free-form exception conditional on the positional actually being the selected prompt source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in bc4cb6f. Notably this is the first of the three questions I raised for the previous round — I checked task "--prompt-file p.txt --help" (no positional, prints usage) and concluded it was fine, missing the case where a positional accompanies the file. You found the case I did not construct.
Reproduced on b15c07a:
task "--prompt-file prompt.txt ignored --help" -> started a real Codex turn using prompt.txt
task "--prompt-file prompt.txt --help" -> Usage: (the case I had tested)
readTaskPrompt returns the file unconditionally when --prompt-file is set and never reads the positional, so there was no literal prompt text to protect.
The classification is now a predicate over the parsed options rather than a flag, since whether the positional is the input depends on the other options:
positionalsAreFreeForm: (options) => !options["prompt-file"]adversarial-review declares () => true — its focus text is always used. Everything else omits it, so help wins over a structured job id or a discarded positional.
| input | result |
|---|---|
task "--prompt-file f.txt ignored --help" |
usage |
task "--prompt-file f.txt --help" |
usage |
task "do the thing --help" |
task, prompt intact |
adversarial-review "why does --help start a review" |
review, focus intact |
cancel "job-x --help" |
usage |
review "--scope working-tree focus --help" |
usage |
Two tests: help wins when --prompt-file displaces the positional (non-vacuous against b15c07a, where a turn starts), and a literal prompt mentioning --help still runs with the prompt intact. The second passes both ways — it guards the protection this change narrows rather than evidencing the bug.
Suite 103 passing / 0 failing of 103.
…e input Addresses the P1 on b15c07a. task was marked free-form unconditionally, but readTaskPrompt returns the file whenever --prompt-file is given and never looks at the positional. A positional alongside --prompt-file is therefore discarded, so there is no literal prompt text to protect -- yet it still suppressed help: task "--prompt-file prompt.txt ignored --help" -> started a real Codex turn using prompt.txt The classification is now a predicate over the parsed options rather than a flag, because whether the positional is the input depends on the other options: positionalsAreFreeForm: (options) => !options["prompt-file"] adversarial-review declares () => true; its focus text is always used. Every other subcommand omits it, so help wins over a structured job id or an ignored positional. task "--prompt-file f.txt ignored --help" -> usage task "--prompt-file f.txt --help" -> usage task "do the thing --help" -> task, prompt intact adversarial-review "why does --help ..." -> review, focus intact cancel "job-x --help" -> usage review "--scope ... focus --help" -> usage Tests: help wins when --prompt-file displaces the positional (non-vacuous against b15c07a, where a turn starts); and a literal prompt mentioning --help still runs with the prompt intact. The second passes both ways -- it guards the protection this change narrows rather than evidencing the bug. Full suite: 103 passing / 0 failing of 103.
|
@codex review Current head is bc4cb6f.
Worth scrutinising, in the same shape as the last two findings — a positional that looks like input but is not:
Nine findings on this PR now, six of them defects introduced by the preceding fix. Still not treating it as converged. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc4cb6fdf7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const { options, positionals } = parseCommandInput(argv, { | ||
| ...schema, | ||
| booleanOptions: [...(schema.booleanOptions ?? []), "help"], | ||
| aliasMap: { ...(schema.aliasMap ?? {}), h: "help" } | ||
| }); |
There was a problem hiding this comment.
When a help flag immediately follows a value-taking option, parseArgs consumes it as that option's value, so options.help remains unset and the handler dispatches. For example, the plugin-shaped adversarial-review "--model --help" starts a review attempt with model --help instead of printing usage; -h and the other value options have the same problem. Treat a standalone help token as help before allowing it to satisfy a missing option value, while preserving explicit inline values such as --model=--help if those must remain supported.
Useful? React with 👍 / 👎.
1 participant
Problem
--helpis only recognised as the first argument (subcommand === "--help"). Anywhere else it lands inargv, and becauseparseArgstreats any unrecognised--tokenas a positional, it is carried into the subcommand as data rather than handled as a request for usage.For
adversarial-reviewthe positionals become the review's focus text, so:runs a full review against the default base — minutes of wall clock and a real model turn — for someone who asked what the flags were. I burned ~10 minutes to this twice in one afternoon before realising the flag was being eaten.
The same shape applies to any subcommand whose parser accepts positionals;
adversarial-reviewis simply the one where the cost is highest.Fix
main()checks for--help,-horhelpinargvbefore the dispatchswitch, and prints usage for that subcommand alone.Checking before the switch is the substance of the change, not an implementation detail: it means a help request can never reach a handler capable of dispatching. Putting the check inside each handler would leave the same bug one refactor away.
Bare
--help,-h, andhelpas the subcommand still print the full usage block, unchanged.The usage lines move into a
Mapkeyed by subcommand so one line can be printed without duplicating the text. The full block prints in the same order as before.Tests
Two tests in
tests/runtime.test.mjs:adversarial-review --helpprints usage and starts no Codex turn — asserted against the fake app server's recordedlastTurnStart, since "printed usage" alone would not prove the review was skippedtask -hprints only the task line, not the whole blockBoth verified non-vacuous against
db52e28, where they fail.Full suite: 93 passing / 0 failing of 93.
Correction: an earlier revision of this description reported 3 pre-existing failures on
main. That was wrong. They were caused byCODEX_COMPANION_SESSION_IDbeing set in my shell:filterJobsForCurrentClaudeSessionthen filters jobs tojob.sessionId === sessionId, and thestatus/resulttest fixtures are handcrafted without asessionId, so they were all filtered out. With that variable unset the suite is green. There are no pre-existing failures.