fix(generate-cli): emit flag names commander can parse - #332
Conversation
A property whose name starts with an uppercase letter or underscore produced `---flag`, which commander rejects while the option is constructed, so every command in the generated CLI failed before it could run. A `no_*` property produced `--no-x`, which commander treats as a negated boolean: it stores the parsed value under `x` and defaults it to true when the flag is absent, so the generated command read an always-undefined key and dropped the argument. Derive both the flag and its placeholder from one helper so the two spellings cannot drift apart again.
inferType already normalizes a union `type` such as `["array", "null"]` to `array`, but the two container checks next to it compared the raw value, so a nullable array lost both its item type and its enum members. The option was still generated as an array, which left the signature, the placeholder, the help choices and the call example disagreeing with each other, and made the generated parser coerce every element to a string. Ask inferType from both checks so the container answer matches the type the option is generated with.
inferSchemaDisplayType returned the raw outputSchema title, and emit-ts splices that value into the emitted interface as a type name. A title with a space therefore produced a module that does not parse, so the generated .ts and .d.ts files were unusable rather than merely imprecise. Fold the title into an identifier and fall back to the structural type when it cannot become one.
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![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: da9d0a378b
ℹ️ 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".
| .replace(/^-+/, ''); | ||
| // Commander reads `--no-x` as a negated boolean: it stores the parsed value under `x` and | ||
| // defaults it to true when the flag is absent, so the generated command would never see it. | ||
| return flag.startsWith('no-') ? `no${flag.slice(3)}` : flag; |
There was a problem hiding this comment.
When a tool schema contains both no_cache and nocache (or no-cache and nocache), this branch maps every property to the same nocache CLI name. renderToolCommand consequently emits duplicate --nocache options and reads cmdOpts.nocache for both argument assignments, so callers cannot provide distinct values for these legal schema properties. Preserve a distinct escaping for the reserved no- prefix or detect and reject option-name collisions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 31bb10b. You are right, and the escape was the wrong half of the trade.
Measured on the previous commit:
extractOptions({ no_cache: {...}, nocache: {...} })
-> [{ property: 'no_cache', cliName: 'nocache' },
{ property: 'nocache', cliName: 'nocache' }]
I took the second option the PR body listed rather than a different escape, since any escape can collide with some other legal property name. The flag is now spelled straight from the property (--no-cache), and the generated module builds each option explicitly and clears the negation commander would otherwise infer:
function defineOption(flags, description, parser) {
const option = new Option(flags, description);
if (parser) option.argParser(parser);
option.negate = false;
return option;
}With that, attributeName() is noCache, which is the key the generated command already computed, and an absent flag stays undefined instead of defaulting to true. Both properties now round-trip:
$ ./proof-cli.ts fetch --query mcp --no-cache true --nocache false --sources web,news
{"Query":"mcp","no_cache":true,"nocache":false,"sources":["web","news"]}
The regression test now renders a schema carrying no_cache, nocache and url together, asserts the three emitted flags are distinct, and checks each one against the key commander stores it under.
Escaping `no_cache` to `--nocache` collapsed it onto a `nocache` property that a schema may legally declare alongside it, emitting two identical flags that both resolved to one storage key. Keep the flag spelled from the property name and build each option explicitly instead, clearing the negation commander would otherwise infer, so `--no-cache` is stored under `noCache` and defaults to undefined when absent. This is the alternative the PR body offered; review showed it is the correct one.
… suite
The suite that renders a CLI and inspects the file still looked for the literal
`.option("--cells`, which the explicit Option construction replaced. This suite
is skipped on win32, so the local run did not catch it.
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed August 26, 2026, 3:49 PM ET / 19:49 UTC. ClawSweeper reviewWhat this changesThe PR normalizes generated CLI option names and TypeScript type labels so legal MCP JSON Schema property names and output titles produce runnable, parseable artifacts. Merge readinessKeep open: current main still emits malformed Commander flag names for legal schema properties, while the latest branch resolves the prior separator finding with focused source changes, regression coverage, successful checks, and real behavior proof. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: PASS (completed) Assertions:
How this fits togetherMCPorter turns MCP tool schemas into generated Commander CLIs and TypeScript signatures. Schema metadata feeds option generation and type rendering, then the generated command maps parsed values back to the original tool argument keys. flowchart LR
A[MCP tool schema] --> B[Tool metadata]
B --> C[Option-name normalization]
C --> D[Generated Commander CLI]
D --> E[Original tool arguments]
B --> F[Signature renderer]
F --> G[Generated TypeScript types]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Merge the generator-side compatibility repair so generated CLIs preserve original schema keys while using Commander-safe flags and parseable TypeScript output. Do we have a high-confidence way to reproduce the issue? Yes: current main source deterministically produces leading or repeated dashes from uppercase and separator-heavy property names before Commander registers the option; the PR also supplies Node 24 after-fix runs. Is this the best way to solve the issue? Yes: normalizing and disambiguating names at the generator boundary, while retaining original schema keys for invocation, is the narrowest maintainable repair. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ae3d9000c320. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (11 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
A title that already looked like an identifier was emitted verbatim, and a
reserved word looks like one, so `{"title":"class"}` reached the emitted module
as `Promise<class>` and the declaration stopped parsing. Reserved words now take
the same folding path every other non-identifier title takes, which keeps the
title in the emitted name instead of dropping it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BhEWU7mL1HkUQZjeB4dNzq
|
[P2] Reject TypeScript reserved words in schema titles - fixed in 4e32483.
Which words are in the set: I ran every reserved word through Behavior proofSame stdio fixture as the PR body, with the output schema titled {"name":"search","description":"Search the web",
"inputSchema":{"type":"object","required":["Query"],
"properties":{"Query":{"type":"string","description":"Search text"}}},
"outputSchema":{"title":"class","type":"object"}}
TestsOne new case, red against the previous head: It renders the module and hands it to the TypeScript parser, so the invariant is pinned against GatesProduction delta for this commit is +45/-1 in |
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Two legal property spellings can normalize onto one flag - `Query` beside `query`, or `no_cache` beside `noCache` - and commander refuses a command that declares the same flag twice, so the generated module threw at load and no command in it could run. Flag names are now assigned across the whole property list: schema order keeps the plain flag and a later property takes the first suffix no other property claims. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhEWU7mL1HkUQZjeB4dNzq
|
[P2] Reject colliding normalized flag names - confirmed and fixed. One correction to the finding, measured rather than argued: commander 15 does not accept the duplicate and read one value into both arguments. That also means the collision is not something this branch introduced. On What this branch changes is the reach: FixFlag names are now assigned once for the whole property list instead of per property. Schema order keeps the plain flag, and a later property takes the first function assignCliNames(properties: string[]): string[] {
const natural = properties.map((property) => toCliOption(property));
const claimed = new Set(natural);
const used = new Set<string>();
return natural.map((base) => {
let name = base;
let suffix = 2;
while (used.has(name) || (name !== base && claimed.has(name))) {
name = `${base}-${suffix}`;
suffix += 1;
}
used.add(name);
return name;
});
}The placeholder is built from the assigned name rather than recomputed from the property, so Behavior proofFixture with both colliding pairs at once: {"name":"search","description":"Search the web",
"inputSchema":{"type":"object","required":["Query"],"properties":{
"Query": {"type":"string","description":"Search text"},
"query": {"type":"string","description":"Legacy search text"},
"no_cache": {"type":"boolean","description":"Bypass the cache"},
"noCache": {"type":"boolean","description":"Legacy bypass"}}}}Each property carries its own value; nothing is read twice. TestsTwo new cases, red against the previous head: The first one adds every emitted flag to a real GatesProduction delta for this commit is +26/-3 in |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
The title guard listed JavaScript reserved words, so four spellings TypeScript refuses in a type position reached the emitted module verbatim and the .d.ts stopped parsing. The set is now measured against the parser rather than assumed, and the suite renders a title for every keyword TypeScript spells so the parser, not a copy of its rules, decides which ones have to be folded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HYFLTVUC1VVb2mjRXNuv13
|
[P2] Reject remaining TypeScript type-context keywords - fixed in 37a1ae5, with one correction to the finding.
The guard was incomplete all the same, and you were right that the set was the wrong shape: it listed JavaScript reserved words, so the spellings TypeScript refuses only in a type position were never in it. Rendering a title for every keyword TypeScript spells and handing each emitted module back to the parser names them: Those four are the whole set that failed; Behavior, one stdio fixture whose tool declares The set now carries two measured groups rather than one assumed one: the spellings that fail to parse in a type position ( The regression is the parser's answer rather than a copy of its rules: GatesLocal runtime is Node 22.20.0 while the repo asks for Production delta for this commit is +22/-5 in On the live verification step
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦞 diamond lobster
A property whose characters all normalize away, such as `___`, left an empty CLI name, so the generated module declared `--` and commander rejected it while the command was being built. The suffix pass inherited the same empty base and spelled the next colliding property `---2`. The stem is assigned inside toCliOption, before names are handed to the collision pass, so a schema declaring `option` beside `___` still gets two distinct flags and the placeholder matches the flag it labels. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGpB942NkzLFqzwubdyp7C
A property such as `2fa` produces `--2fa`, which commander accepts and stores under the key `2fa`. The generated module read it as `cmdOpts.2fa` and assigned `args.2fa`, so the artifact failed to parse before commander was reached. Keys that cannot be spelled as a property access are now read through a subscript; every key that can be is emitted unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LGpB942NkzLFqzwubdyp7C
|
[P2] Give empty normalized names a valid flag stem — confirmed and fixed in 8355ab6, with one addition the finding did not reach. You are right, and the reproduction holds exactly as written. Two things the finding did not name, both visible in that same run. The suffix pass inherits the empty base. The artifact does not reach commander at all. The empty name is also spliced into the generated source as a property access, so the module fails to transpile first:
Fix
return normalized === '' ? EMPTY_CLI_OPTION_STEM : normalized; // 'option'A schema declaring Separately, and droppable on its own commit if you would rather not take it (5a32d16, one helper plus three call sites): a key that cannot be spelled as a property access is read through a subscript. Every key that can be is emitted unchanged, so existing generated output is byte-identical. function propertyAccess(target: string, key: string): string {
return /^[A-Za-z_$][\w$]*$/.test(key) ? `${target}.${key}` : `${target}[${JSON.stringify(key)}]`;
}Behavior proofSame fixture, same four properties, on this branch: All four properties round-trip to their own flag, including the real TestsFour new cases, red against The last two build the block through GatesProduction delta for these two commits is +23/-11 across two files. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Verified this PR against a real-world server that trips the non-identifier property case: the official grafana/mcp-grafana server (streamable HTTP), whose proxied Tempo tool Environment: Linux, Node v24.19.0, pnpm 10.34.5 via corepack. Before (main @ ae3d900): After (this branch @ 5a32d16): the same invocation bundles cleanly; the emitted line is Runtime checks against the live server, all passing:
Test suite on this branch (Node 24): One thing I double-checked while reading the diff: the generated |
|
Thank you - the The negative probe is the part I had not covered. My fixture only proves the flag parses and the key round-trips outward; letting the server reject a malformed On the Node version: you are right, and the PR description was misleading. I have rewritten the Gates section to lead with your Node 24 numbers (1734 passed, Thanks also for re-deriving the |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
Commander derives an option's storage key by splitting the flag on dashes and upper-casing each segment's first character, so an empty segment has no character to read and `addOption` throws while the generated command is being built. A property spelling any of `foo__bar`, `baz_` or `filter_Query` normalized to a flag carrying such a segment, and the generated CLI failed at module load rather than at parse time. Collapsing runs and trimming either end before `assignCliNames` keeps the collision pass free to disambiguate two spellings that now land on one flag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DCMA8LRouQDoxh9Lr9n88o
|
[P2] Normalize repeated separators before emitting Commander flags — confirmed and fixed in 2d1b765, with two shapes the finding did not reach. The reproduction holds exactly as written and the mechanism is the one you named. Measured against commander 15.0.0, the version the lockfile resolves:
Fix
.replace(/-{2,}/g, '-')
.replace(/^-|-$/g, '');Collapsing before To check the class rather than the three names, I enumerated every property name up to five characters over Behavior proofA stdio server declaring The last line is the part the flag name alone does not settle: the server sees the original schema keys, so collapsing the separator changed the flag and nothing else. TestsFour new cases in The third one hands the emitted flags to commander's own |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
2 participants
Summary
mcporter generate-cliderives every flag name from the tool's JSON Schema property name. Two legal property spellings produce a flag that commander does not read the way the generated code expects.The first one is fatal. A property whose name starts with an uppercase letter produces
---query, which commander rejects while it is still constructing the option, so the generated artifact throws at module load and no command in it can run, not even--help. Uppercase property names are what the .NET and Java MCP server SDKs emit by default.Root cause
toCliOptionprefixes a dash for every uppercase character with no guard for the leading position, andbuildPlaceholderrepeats the same expression instead of calling it:Querybecomes-query, so the emitted line is.option("---query <-query>", ...):The second spelling fails more quietly.
no_cachebecomes--no-cache, which commander treats as a negated boolean:The generated command reads the key it computed itself,
cmdOpts.noCache(src/cli/generate/template.ts:449), which is therefore alwaysundefined. Commander also gives a negated option an implicittruedefault, so the argument is dropped when the option is optional and reported as missing when it is required, even though the user supplied it.Fix
Derive the flag and its placeholder from one helper and drop a leading dash, so the name commander is handed is always a valid long flag.
For the
--no-case the first revision escaped the flag to--nocache; review pointed out that this collapses onto anocacheproperty a schema may legally declare alongsideno_cache, emitting two identical flags. Any escape can collide with some other legal name, so the flag is now spelled straight from the property and the generated module builds each option explicitly instead:attributeName()is thennoCache, which is the key the generated command already computes, and an absent flag staysundefinedrather than defaulting totrue.Collision: two spellings that normalize onto one flag
Fifth commit.
Querybesidequery, orno_cachebesidenoCache, are distinct legal properties that both normalize to one flag, and commander refuses a command that declares the same flag twice, soaddOptionthrows while the command is being built:This one is not new here - a
no_cache/noCacheschema already dies that way onae3d900- butQueryused to crash on---querybefore any collision could matter, so fixing that spelling exposes the pair. Flag names are therefore assigned across the whole property list rather than per property: schema order keeps the plain flag, and a later property takes the first-2,-3, ... suffix that no other property claims naturally, so a schema that also declaresquery_2does not get a second--query-2. The placeholder is built from the assigned name, and the generated command keeps readingargs.<property>from the key commander stores that flag under.Empty flag name: a property whose characters all normalize away
Sixth and seventh commits.
___is a legal property name whose every character normalizes away, leaving an empty flag name, and the empty name is inherited by the suffix pass as well, so a schema declaring_beside it emits---2:The stem is therefore assigned inside
toCliOption, before names reach the collision pass, so it participates in flag assignment like any natural name and a schema declaringoptionbeside___still gets two distinct flags.The empty name is also spliced into the generated source as a property access, so the artifact fails to transpile before commander is reached -
cmdOpts.andargs.___ = cmdOpts.;. That half outlives the stem fix for one more spelling:2fais legal,--2fais a flag commander accepts and stores under the key2fa, butcmdOpts.2fadoes not parse. Seventh commit, droppable on its own - a key that cannot be spelled as a property access is read through a subscript, and every key that can be is emitted unchanged:A fixture declaring all four properties at once:
Empty flag segment: a separator run or a trailing separator
Eighth commit, from review. Commander derives an option's storage key by splitting the flag on dashes and upper-casing each segment's first character, so a segment that is empty has no character to read:
Three property shapes reached such a segment:
foo__bar(a run),baz_(a trailing separator) andfilter_Query, which reaches a run by mixing the two naming conventions rather than by repeating one - the uppercase rule emits-qnext to the underscore's own dash. Runs are collapsed and one dash is trimmed from either end before names reach the collision pass, sofoo__barbesidefoo_barstill gets two flags. Enumerating every property name up to five characters overa B _ - 2 .and registering each emitted flag on a realCommandrejects none of the 9330.Unlike the leading-dash case this one is invisible at construction, so it is pinned at
Command.addOptionrather than atnew Option.Adjacent finding: a nullable array loses its item type and its enum members
Second commit, drop it if you would rather not take it - one helper plus two call sites.
inferTypealready normalizes a uniontypesuch as["array", "null"]toarray(pinned bytests/generate-cli-helpers.test.ts:137), but the two container checks beside it compare the raw value:The option is still generated as an array while its item type and enum members are dropped, so the signature, the placeholder, the help choices and the call example all disagree for one descriptor. For a numeric variant the generated parser then picks
parseArrayOption(value, 'string'), so--scores 1,2reaches the tool as["1","2"].Adjacent finding: a multi-word
outputSchema.titlemakes the emitted TypeScript unparseableThird and fourth commits, also droppable - one helper in
list-signature.ts.inferSchemaDisplayTypereturns the raw title andemit-tssplices that value into the emitted interface as a type name, so{"title":"Search Results"}producesPromise<Search Results>and the emitted.d.tsdoes not parse. This only changes how a title is folded into an identifier; it does not change the existing decision to use the title as the type name.Review pointed out that a title spelling a reserved word, such as
class, already matches the identifier pattern and so was still emitted verbatim asPromise<class>. Reserved words now fail that test and fold like any other non-identifier title, so the title survives asClassinstead of being dropped. The set was measured withtscrather than assumed: 30 reserved words fail to parse in that position, andvoid,null,true,falseandthisparse into a keyword type that no longer describes the schema.Behavior proof
One stdio fixture, one tool, exercising all three cases at once:
{"name":"search","description":"Search the web", "inputSchema":{"type":"object","required":["Query","no_cache"],"properties":{ "Query": {"type":"string","description":"Search text"}, "no_cache": {"type":"boolean","description":"Bypass the cache"}, "nocache": {"type":"boolean","description":"Legacy flag"}, "sources": {"type":["array","null"],"items":{"type":"string","enum":["web","news"]}}}}, "outputSchema":{"title":"Search Results","type":"object"}}Generated CLI:
The
no_cachecase on its own. The crash above hides it, so this run uses the same fixture withQueryrenamed toquery:mcporter list proof:mcporter emit-ts proof --mode types:Tests
The new cases, red against unpatched
main:One more case,
keeps distinct flags for property names that only differ by the no- prefix, passes on both revisions by design: it is the guard against the escape this PR first tried and then dropped.Four of the cases build the options through
renderToolCommandand hand the emitted flag strings to commander's ownOptionandCommand, so both the flag grammar and the no-duplicates rule are pinned against commander itself rather than against a copy of its rules.Gates
Measured on Node 24.18.1, the runtime the repo asks for.
pnpm check(oxfmt, oxlint--type-aware --deny-warnings,tsc --noEmit) is clean on this branch.The seven live in
tests/chrome-devtools-relay-handoff.test.tsandtests/runtime-chrome-relay-handoff.test.ts, and they are a property of this Windows machine rather than of this branch. Reverting the branch's three production files toae3d900and rerunning those two suites gives the same seven test names, character for character:Nothing in this PR touches the relay path. @DominionZA2's Node 24 run of this branch reports 1734 passed with those suites green, which is what the same code looks like off Windows: #332 (comment)
Targeted:
Production delta is +154/-14 across three files; the remaining +381/-4 is tests.