fix(cli-generator): stop emitting a wire-test suite where every test fails - #17568
fix(cli-generator): stop emitting a wire-test suite where every test fails#17568cadesark wants to merge 1 commit into
Conversation
…fails The generated harness pushed --no-pager on every invocation, but the CLI registers that flag only inside `if method_has_pagination(...)`. On a spec with no pagination metadata the flag exists on zero operations, so clap rejected the entire command line with "unexpected argument --no-pager" -- 100% of cases, not a subset. Any customer running `generateWireTests: true` against such a spec got a suite that could not pass. On a real customer spec: 678 of 678 failing. The flag was also inert: the pager only spawns under --page-all, which no case passes. And the gate itself is correct -- --no-pager is only ever read as `page_all && !no_pager` -- so the fix belongs on the harness side. Reproduced end to end on `query-parameters-openapi:with-wire-tests`: 0 passed / 2 failed before, 2/2 after. That fixture declares no pagination either, which is why nothing caught this when the gate landed in #17545 -- seed does run `cargo test --locked --all-features`, it just never ran here. Added a unit test asserting the harness pushes only flags the SDK registers unconditionally, and verified it fails on the bug.
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Removes the unconditional --no-pager push from the wire-test harness, with a matching regression test and regenerated seed fixture. Fix is correct and minimal; the only nits are that the new test's flag-extraction regex is brittle (it only matches literal args.push("--flag".to_string()), so a flag pushed via a variable or format! would slip through) and the comment blocks are verbose for generated Rust output.
- 🔵 2 suggestion(s)
To request another review, comment /ai-review on this pull request.
| const pushedFlags = [...rust.matchAll(/args\.push\("(--[a-z0-9-]+)"\.to_string\(\)\)/g)].map( | ||
| (match) => match[1] | ||
| ); |
There was a problem hiding this comment.
🔵 suggestion
The regex only catches string-literal pushes of the exact shape args.push("--x".to_string()). A flag pushed via format!, a variable, or with different spacing silently escapes the assertion, so the guard could quietly stop guarding. Not blocking, but worth a comment noting the limitation, or broadening to match any --[a-z0-9-]+ literal in the run_case body.
| // Deliberately no --no-pager: it is registered only on operations that | ||
| // declare pagination metadata, so pushing it unconditionally made clap | ||
| // reject every invocation ("unexpected argument '--no-pager'") on any spec | ||
| // without pagination markers — the whole suite, not one case. It was also | ||
| // inert here: the pager only spawns under --page-all, which no case passes. |
There was a problem hiding this comment.
🔵 suggestion
Five lines of post-mortem in every generated wire_test.rs is a lot of noise for consumers who never saw the bug. Consider trimming to one line in the emitted Rust and keeping the full rationale in the changelog/test comment where it already lives.
| // Deliberately no --no-pager: it is registered only on operations that | |
| // declare pagination metadata, so pushing it unconditionally made clap | |
| // reject every invocation ("unexpected argument '--no-pager'") on any spec | |
| // without pagination markers — the whole suite, not one case. It was also | |
| // inert here: the pager only spawns under --page-all, which no case passes. | |
| // No --no-pager: the CLI registers it only on paginated operations, and the | |
| // pager only spawns under --page-all, which no case passes. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Problem
generateWireTests: truecurrently produces a suite in which every test fails, on any spec without pagination metadata.The generated harness pushed
--no-pageron every invocation:But the CLI registers that flag only inside
if method_has_pagination(...)(sdk/src/openapi/commands.rs). On a spec with nox-fern-paginationmarkers, the flag exists on zero operations, so clap rejects the whole command line before the test does anything:That is 100% of cases, not a subset. On a real customer spec: 678 of 678 failing.
Cause
Two files on
maindisagree:generators/cli/src/wireTests/harness.ts:911pushes--no-pagerunconditionally (from fix(cli-generator): wire tests assert requests, drive multipart/binary uploads, cover non-2xx #17316).generators/cli/sdk/src/openapi/commands.rs:698registers it only behind the pagination gate (from fix(cli-generator): retry safety, $ref validation, and launcher exit codes #17545).The gate is correct —
--no-pageris only ever read aspagination.page_all && !pagination.no_pager, so on a non-paginated operation it has nothing to do. The flag was also inert in the harness: the pager can only spawn under--page-all, which no wire-test case passes. So the fix is to stop pushing it.Why nothing caught it
seed/cli/seed.ymldoes declaretestScript: cargo test --locked --all-features, andquery-parameters-openapi:with-wire-testsis the right fixture — but that spec has 0x-fern-paginationmarkers too, so the fixture is broken in exactly the same way. The coverage existed and was blind.Verification
Reproduced and fixed end to end on the real fixture:
Added a unit test asserting the harness pushes only flags the SDK registers unconditionally (
--base-url,--params,--json), and verified it fails on the bug before trusting it:430 TypeScript tests passing,
pnpm checkclean.Scope
Deliberately minimal so it can merge ahead of the rest — 35 lines. Split out of #17567, whose remaining fixes are unrelated to this and shouldn't hold it up.
Generated with Claude Code