Survive specs whose operation and parameter names collide - #31
Merged
Merged
Conversation
A spec with two operationIds that simplify to the same command name did not degrade — it killed the CLI. Commander rejects a duplicate while the command tree is being built, before any command runs, so `searchItems` and `searchItem` under one tag made every invocation fail, `--help` included: $ spec2cli --spec api.json items --help Error: cannot add command 'search' as already have command 'search' Colliding command names are now disambiguated by HTTP method, then by a numeric suffix for whatever still overlaps. Closes #23. Two more shapes that real specs use and this one mishandled: Parameters sharing a name across locations produced two flags of the same name, which Commander rejects the same way. The existing guard only caught body fields clashing with parameters, so `id` in the path plus `id` in the query still got through. Collisions are now tracked by name as well as by location, first declaration winning. Parameter names are not constrained to what a flag can be called. `filter[name]` became an unusable option; it is now offered as `--filter-name` while the request still goes out under the spec's own name. Also decodes JSON Pointer escapes when resolving refs. `~1` is the only way to write `/` inside a pointer segment (RFC 6901), so any ref into `paths` — such as `#/paths/~1pets/parameters/0` — silently missed and fell back to an unresolved `$ref`. Ref resolution now also refuses to walk into a non-object rather than yielding a partial. Reworked from #25 by @kevin-krause, rebased onto main after the original went stale. The refactor half of that PR is left out: main has since grown its own parameter-ref resolution, and the request-building extraction is handled separately. Its remaining URL fragment fix is ported alongside that work. Co-authored-by: Kevin Krause <kevin-krause@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QpcUFZcRHNurdJ2diVg37T
lucianfialho
added a commit
that referenced
this pull request
Aug 3, 2026
Brings in the name-collision work from #31 and the conditional spec cache from #30. Three files needed a decision: `dynamic-commands.ts` auto-merged into something that compiled but was wrong. #31 made Commander store options under a sanitized flag name; this branch reads required params back to decide whether any are missing. Merged naively those cross: a required `filter[name]` is stored as `filterName`, so the missing-input check looked it up under the spec's name and reported it absent no matter what the caller passed. Both readers now go through optionValueForParam. `flags.ts` — both sides added a boolean flag (`--reveal` here, `--refresh` on main); kept both, over the extracted VALUE_FLAGS set this branch introduced. `README.md` — both sides documented different sections; kept both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QpcUFZcRHNurdJ2diVg37T
lucianfialho
added a commit
that referenced
this pull request
Aug 3, 2026
`privacy scan` reports the fields the filter would touch, grouped by schema, and
which operations return them:
$ spec2cli privacy scan ./api.yaml
4 fields would be redacted:
Customer
email, full_name, phone, cpf
Returned by:
GET /customers
Turning the filter on without being able to see its reach was a bad trade in
both directions: too little and personal data goes out anyway, too much and a
field the caller needed comes back redacted with no hint why. Detection is by
name and format, so an unusually named field is missed — which is the reason to
be able to look. Part of #22; the runtime filtering half already shipped.
Array parameters given as JSON were sent as raw strings. The extractor names an
array after its items — `string[]`, not `array` — so the check for structured
types never matched and `["a","b"]` reached the server quoted.
The dead commander-builder module is gone. It had not been on the runtime path
for some time, duplicated command building, and had already drifted: #31 changed
it while the live path moved on. That is the same shape of divergence that let
--dry-run print credentials in the clear.
Its JSON-parameter tests are replaced against the live path, and doing so is what
surfaced the array bug: the old ones hand-built their Operation objects with
`type: "array"`, a value the extractor never produces, so they passed while the
behaviour they claimed to cover was broken.
Claude-Session: https://claude.ai/code/session_01QpcUFZcRHNurdJ2diVg37T
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reworked from #25 by @kevin-krause. Credit is his; the rebase and the extra tests are mine.
The bug
A spec with two operationIds that simplify to the same command name did not degrade — it killed the CLI. Commander rejects the duplicate while the command tree is being built, before any command runs:
searchItemsandsearchItemunder one tag is enough. Every invocation fails,--helpincluded, so there is no way to even discover what went wrong. Closes #23.Colliding names are now disambiguated by HTTP method (
search-get,search-post), then by a numeric suffix for whatever still overlaps.Two more shapes real specs use
Parameters sharing a name across locations produced two flags of the same name, which Commander rejects the same way. The existing guard only caught body fields clashing with parameters, so
idin the path plusidin the query still got through. Collisions are now tracked by name as well as by location, first declaration winning.Parameter names are not constrained to what a flag can be called.
filter[name]became an unusable option. It is now offered as--filter-name, while the request still goes out under the spec's own name — verified on the wire.Also
JSON Pointer escapes are decoded when resolving refs.
~1is the only way to write/inside a pointer segment (RFC 6901), so any ref intopaths—#/paths/~1pets/parameters/0— silently missed and fell back to an unresolved$ref. Ref resolution also refuses to walk into a non-object rather than yielding a partial.What I left out of #25, and why
That PR also extracted URL and request building out of
http.tsanddry-run.ts— the same de-duplication #29 does, three months earlier. Keeping both would conflict for no gain, so this branch carries only the parts that are not otherwise covered:resolveParameterfor$refparameters has since landed on main independently, in a weaker form. This upgrades it rather than re-adding it.On the delay
The PR sat from 21 May. The author's own comment saying it needed cleanup was posted the same day and was already stale — the branch is a single clean commit with no stray files. Meanwhile part of it was reimplemented on main independently and worse. That is on the review side, not his.
Testing
137 tests, up from 123. The new tests were checked against
mainfirst: 5 fail there (the command collision crash, three-way collision, flag sanitization, JSON Pointer escaping, and cross-location parameter collision). The other 4 pass on main and are kept as guards — they document behaviour rather than fix it, and the commit message says so.🤖 Generated with Claude Code
https://claude.ai/code/session_01QpcUFZcRHNurdJ2diVg37T