Repository navigation
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. What this changesReserve the generated CLI’s built-in --raw option so a schema property named raw receives a collision-free flag. Example: Generate a CLI for get-page with required string id and optional boolean raw.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review This PR fixes a current-main startup failure with a focused patch and sufficient runtime proof; the overlapping fix is still open, so repository policy does not permit closing this PR. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherCLI generation converts MCP tool schemas into Commander commands, mapping assigned flags back to original JSON property names before calling the server. flowchart LR
A[MCP tool schema] --> B[Tool metadata builder]
B --> C[Flag allocator with reserved raw]
C --> D[Generated Commander command]
D --> E[Original JSON arguments]
E --> F[MCP server call]
Technical reviewBest possible solution: Land one focused reservation fix, preferably retaining the generated-executable coverage in #487. Do we have a high-confidence way to reproduce the issue? Current-main allocation and template registration establish the duplicate --raw path, supported by the contributor’s recorded base failure; target code was not executed during this read-only review. Is this the best way to solve the issue? The patch fixes the allocation root cause using the existing suffix allocator and preserves other metadata consumers through an optional internal argument. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against e8c7be5dfecd. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed November 3, 2026, 8:35 AM ET / 13:35 UTC (Revision 2). |
…t collide Every generated tool command registers a built-in --raw <json> option, but assignCliNames only de-duplicated tool properties against each other. A tool with a property named `raw` therefore registered a second --raw and Commander threw at startup, so the generated CLI crashed on launch. Let callers pass the flag names they register themselves, and have generateCli reserve `raw`: the schema property becomes --raw-2 and the built-in keeps --raw. Other consumers of the tool metadata (list, call, emit-ts) register no --raw, so they keep the natural name. Co-Authored-By: Claude
112a838 to
97297ff
Compare
|
@clawsweeper re-review Rebased onto |
|
🦞👀 Re-review progress:
|
|
Closing as a duplicate of #487, which fixes the same One data point that may be useful there: the trigger for me was a server-side schema change. An HTTP MCP server added a boolean |
Problem
Every generated tool command registers a built-in
--raw <json>option.assignCliNamesonly de-duplicated tool properties against each other, so a tool with a property namedrawregistered a second--raw. Commander throws while the CLI builds its command tree, so the generated CLI crashes on startup, even for--help:I hit this when regenerating a CLI for an HTTP MCP server whose
get-pageandget-presentationtools have since gained a booleanrawparameter. A binary generated from the earlier schema had worked, so the bug stayed hidden until such a parameter was added on the server side.Fix
buildToolMetadata,buildToolMetadataListandextractOptionsaccept an optional list of flag names the caller registers itself, andassignCliNamestreats them as taken.generateClipasses['raw'], so a schema property namedrawbecomes--raw-2and the built-in keeps--raw. This follows the existing-Nsuffix scheme, so the name is open to discussion if you'd prefer something more descriptive. Users can also still pass"raw": truethrough--raw '<json>'.The reservation is opt-in on purpose:
mcporter list,callandemit-tsshare the same metadata builder throughtool-cache.tsbut register no--rawflag, so they keep the natural name.Changelog
Added an entry under 0.16.2.
Tests
Two unit tests in
tests/generate-cli-helpers.test.ts:rawreserved, arawproperty getsraw-3and a siblingraw_2getsraw-2(and the same throughbuildToolMetadataList);rawkeeps its natural name.Behavior proof
Generated from a local stdio fixture with one tool,
get-page, takingid(required string) andraw(boolean). Built withpnpm buildand run throughdist/cli.js generate-cli --runtime node --bundle.On
main(e8c7be5),get-page --helpcrashes:On this branch:
Fixture server
Generate with
node dist/cli.js generate-cli '{"name":"raw-demo","command":"node","args":["raw-fixture.mjs"]}' --runtime node --bundle cli.mjs.Verification
pnpm check(format, type-aware lint, typecheck) passes.Not covered
rawproperty was added totests/generate-cli-option-collisions.test.ts.-h/--help, could collide the same way. I haven't checked them, and they could be a follow-up.