Skip to content

fix(cli-generator): retry safety, $ref validation, and launcher exit codes - #17545

Merged
cadesark merged 12 commits into
mainfrom
cade/cli-agentmail-review-fixes
Aug 27, 2026
Merged

fix(cli-generator): retry safety, $ref validation, and launcher exit codes#17545
cadesark merged 12 commits into
mainfrom
cade/cli-agentmail-review-fixes

Conversation

@cadesark

@cadesark cadesark commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Description

Linear ticket: Refs

Fixes AgentMail's 11-item post-1.0.0 CLI review. They shipped on 0.31.2; current is 0.38.4, and four items had already been fixed in between — triage below, so nothing gets re-fixed or wrongly closed.

Everything is verified by regenerating and building the reporter's own CLI from agentmail-to/agentmail-docs, plus a second real spec (ElevenLabs) as a differential control. Two of the fixes below exist because that verification found my first attempt incomplete.


Per-item outcome

# Report Outcome
1a Every operation auto-retries ~4x on 5xx, including non-idempotent POSTs Fixed
1b x-fern-idempotent: true makes retries send no key at all Fixed
1c Retry should require a server-supported key Fixed
2 $ref schemas never resolved — validation off for params and bodies Fixed (3 distinct bugs)
3 npm launcher turned signal deaths into exit 0 Fixed
4 --page-all silently returns page 1 with exit 0 Fixed
5a --schema per-op entries lack httpMethod / path Deferred — reverses ADR-0006
5b globalFlags omits flags every subcommand shows Fixed (3 of the 8 listed are genuine)
5c Root --help lists 4 --format values, subcommand lists 7 Already fixed before this PR
6 generate-skills ships broken instructions (5 spots) Fixed (all 5)
7 Prerelease tags other than -alpha/-beta publish as npm latest Fixed
8 npm and GitHub Release ship different bytes per version Fixed
9 license: MIT metadata contradicts the shipped Apache-2.0 LICENSE Fixed
10a API error bodies double-encoded Already fixed before this PR
10b Auth failures name credential sources never configured Fixed
11a sdk.rs builds ClientConfig::default() — empty base URL Fixed
11b sdk_executor.rs hardcodes "SDK requests are idempotent" Fixed
11c custom.rs claims .fernignore protection it doesn't have Already fixed before this PR

Plus two bugs neither the report nor I spotted until the CLIs were actually built — see Found during verification.


1 — Retry semantics

Root cause was the call site, not method_allows_retry. decide_retry received method.idempotent || idempotency_key.is_some(), and the auto key is generated for every POST/PUT/PATCH — so that second clause made every non-idempotent operation retry-eligible. (I initially misread method_allows_retry in isolation as already correct.)

Compounding it, the marker suppressed key generation: user_provides_idempotency included method.idempotent, but the marker only means the operation exposes --idempotency-key, not that the caller passed it.

Retry-safety now requires a key the server is known to honor — x-fern-idempotent: true, or an explicit --idempotency-key. The auto key is still sent (it is what makes a retry safe where the endpoint consumes it) but no longer licenses one. Only a key actually present suppresses generation.

New behavior (their spec, against a 500ing mock):

verb before after
POST inboxes create 4 attempts 1 attempt, auto key still sent
PATCH 4 attempts 1 attempt
GET / DELETE 4 attempts 4 attempts (unchanged — idempotent by HTTP)
POST marked x-fern-idempotent 4 attempts, no key 4 attempts, same key on all four

sdk_executor.rs had the same shape hardcoded (true, // SDK requests are treated as idempotent), making every custom-command POST retry as idempotent. It now derives safety from the request's own Idempotency-Key header — the only signal available to a transport handed an already-built Request, and the right one. (11b)

They can now put the marker back, which their overrides currently warn against adding.

2 — $ref resolution

Three distinct bugs behind one symptom.

Bodies. validate_value had an object branch and nothing else, so a $ref to a scalar or array component accepted any value. On a spec where most schemas are component refs, validation was effectively off and --dry-run exited 0 on a malformed body. Scalars and arrays are now type-checked through one shared check_json_type, so the inline and component paths cannot drift again.

Ref chains. A bare-$ref component (A: {$ref: B}) was never followed. Now resolved by a bounded iterative resolver — recursion would not terminate on a cycle here, unlike the value-driven recursion elsewhere in the module.

Parameters. convert_parameter took no component_schemas and OpenApiParamSchema had no $ref field, so a $ref'd parameter deserialized to all-None: no type, enum, format or bounds. The array half was also broader than diagnosed — repeated was only ever set for body properties and multipart fields, so no query/header parameter was repeatable in any spelling.

New behavior:

--json '{"url": 123, ...}'   before: accepted, exit 0
                              after:  url: Expected type 'string', found integer

--labels a --labels b         before: "cannot be used multiple times"
                              after:  ?labels=alpha&labels=beta
--labels '["a","b"]'          before: ?labels=%5B%22a%22%5D  (URL-encoded JSON)
                              after:  ?labels=alpha&labels=beta
--labels a                    before: ?labels=a    after: ?labels=a  (unchanged)

3 — npm launcher exit codes

execFileSync throws with status: null and signal: "SIGTERM" on a signal death; the launcher's "status" in e test matched that and process.exit(null) coerces to 0. CI timeouts, SIGSEGV and OOM-kills all reported success to $?, on the npm install path only.

New behavior: a numeric status is required; otherwise the launcher exits 128 + signum (SIGTERM → 143), matching the shell convention. The rationale lives in the TS source, not in the customer's emitted ci.yml.

4 — Pagination flags

Registered only where method.pagination is set or the spec root declares the token query-param / response-path pair — the same treatment --no-stream already gets. Read sites moved to try_get_* so absence reads as "off" rather than panicking on an unknown arg id.

New behavior, and this one is a visible break worth calling out. On a spec with no pagination metadata the flags disappear from every operation (337 of 337 on the ElevenLabs control). Nothing that previously worked breaks — --page-all silently fetched exactly one page and exited 0 — but a script or agent already passing it flips from exit 0 to a hard unexpected argument error. The failure is now loud instead of silent.

5 — --schema completeness

5(a) is deferred, not fixed. httpMethod/path were dropped deliberately in cli-sdk#190 (feat: redesign --schema as the agent-facing contract, patrickthornton, 2026-06-26), vendored here by #16521. ADR-0006 rejected keeping them as an explicit alternative:

(G) Keep httpMethod and path for human debugging. Rejected: the design principle is "what the agent needs to use the command."

Restoring them is an ADR amendment, not a bug fix — and #190 was 88 files, porting every demo.sh/DEMO.md/README.md that piped --schema through jq from .[] to .operations[], so a reversal is a shape change rather than re-adding two fields. It shipped in the 0.19.x range, not 0.28.1 as the report states.

5(b) — 3 of the 8 listed flags are genuine. globalFlags went 10 → 14, adding --params, --no-retry, --no-extract, --help. The other five (--page-all/-limit/-delay, --no-pager) are per-op and correctly excluded per ADR-0006 — and the pagination gate above is what makes that claim true, since before it they really were advertised everywhere.

A third, worse than both: multipart body fields were missing from the contract entirely. They live in method.multipart_fields, not method.parameters, and build_operation_schema only walked the latter — so an upload operation advertised its query params and headers and nothing else:

audio-isolation convert --schema
  before:  required: []          properties: ['xi-api-key']
  after:   required: ['audio']   properties: ['audio','file_format','preview_b64','xi-api-key']

The flags existed and worked; they were invisible to anything reading the contract, so an agent driving purely from --schema could not invoke any multipart operation — 28 of them on the control spec. Fields are emitted into the body bucket as strings, which is what the flag surface takes (a text part its value, a file part a path); file: true tells an agent to pass a path, a per-part contentType surfaces where the spec pins one, and repeated parts render as an array. Builtin-colliding names are skipped, mirroring flag registration. Verified by reading the per-op contract for all 337 operations and driving each of the 28 multipart ones from exactly the fields its own input.required lists: 28 of 28 exit 0, where previously none were followable.

Two more --schema bugs, found by the differential. Both make the contract actively wrong rather than incomplete:

Array element types. A repeated flag carries param_type: "string" — correct for the flag surface, since clap collects strings — and --schema rendered that as the element type. text-to-dialogue convert --inputs, an array of DialogueInput, advertised items: {type: string}; an agent following the contract sent ["x"] and the validator correctly rejected it. array_item_type resolves the real element type through $ref and nullable compositions onto a new item_type, which --schema/--help render and the collector consults:

inputs.items:  {"type":"string"}  ->  {"type":"object"}

--inputs '{"text":"hi","voice_id":"v1"}' --inputs '{"text":"yo","voice_id":"v2"}'
  ->  {"inputs":[{"text":"hi","voice_id":"v1"},{"text":"yo","voice_id":"v2"}]}

item_type: None means string, byte-identical to every previous lowering, so --tags a --tags b still yields ["a","b"].

Required list. An object-valued property the parser recurses into had required: false hardcoded on its shorthand flag. Right for clap — leaf flags can satisfy it — wrong for the contract: agents drafts create advertised 5 required fields and supplying all 5 still failed on workflow, a 6th never listed. required_by_spec carries the spec's bit for --schema while clap keeps using required. The contract is now followable end to end.

6 — generate-skills

The reporter supplied the verbatim <bin>-shared/SKILL.md — the file every per-resource skill names as PREREQUISITE. Their 12 per-resource files were accurate; this one had five errors, and the pattern is the same in three of them: the shared file described the union of every affordance the generator can emit rather than what the spec produces.

Spot Before After
-o, --output documented, accepted by 0/130 ops emitted unconditionally gated on doc_has_binary_response; ffplay -/aplay - audio boilerplate replaced with media-agnostic text
② points at --schema paginable/binaryResponse hints that never appear emitted unconditionally names only affordances the API has; collapses to "available on every command in this CLI" when there are none
<bin> --help as the auth check verifies nothing <bin> auth status
④ exports both alternative auth env vars "Set the required environment variable(s)" "These are alternative schemes for the same request — set one of"
--format default json table on a TTY, json when piped

For ④, AuthStrategy is threaded to the emitter rather than guessed from scheme count: Any and Auto both apply one scheme per request, so All is the only strategy that genuinely needs every variable.

Their flag table went from 13 rows — six naming flags their CLI rejects — to the 7 that exist on every operation.

One note back to them on ③: they suggested auth me. I used auth status because it is grafted onto every Fern CLI (ADR-0007) whereas auth me is not a built-in and no operation is generically guaranteed. Trade-off: auth status confirms credentials resolve, not that they are valid.

7 — Pre-release dist-tags

Only -alpha/-beta were matched, so v1.1.0-rc.1 or -next.1 fell through to a bare npm publish and would move latest. New behavior: any SemVer pre-release gets a tag derived from its first identifier (-rc.1rc), with a prerelease fallback for an all-numeric or empty one (npm rejects a numeric dist-tag). Applied to both publish steps.

8 — Build profile

ci.yml built cargo build --release; cargo-dist's release.yml builds --profile dist (release + thin LTO) — 9.8 MB vs 10.9 MB in their measurements. npm is a distribution channel, so it now uses the profile named dist. The copy path moves with it: a custom cargo profile writes to target/<triple>/dist/, not .../release/. New behavior: both channels ship byte-identical binaries per tag.

9 — LICENSE

The Apache-2.0 file was never a licensing decision: copySdk copies ./sdk/ verbatim and sdk/LICENSE simply wasn't excluded, so the vendored runtime's license rode along into all 49 seed fixtures.

No other Fern generator ships a LICENSE by default. A survey of every seed tree finds exactly two LICENSE files outside seed/cli, both from explicit custom-license fixtures, and RustProject.writeLicenseFile returns early for anything but type: custom.

New behavior: LICENSE is excluded from the SDK copy via sdk/.sdk-ignore.json, and a new writeLicense step emits one only for license: { type: custom }, copying the Fern CLI's /tmp/LICENSE mount and honoring a configured filename. A basic license stays package metadata, as elsewhere. License text is never authored here.

Because Apache-2.0 §4 requires retaining the notice for the vendored runtime, the generated README now carries an ## Attribution section naming fern-cli-sdk and its license — emitted as a discrete Block, so mergeWithExisting lets a customer reword it and keep their wording across regens.

10 — Error surfaces

10(a) needed nothing — a JSON error body already lands as nested JSON under details with message/reason lifted out. No second parse.

10(b) reproduced exactly. decorate_with_source_hint listed every declared credential source and gated its shadowing advice on hints.len() > 1, always true on a two-scheme API. With only AGENTMAIL_API_KEY set:

before: "Credentials were supplied via: AGENTMAIL_TOKEN environment variable, keyring entry
         agentmail:TokenAuth …, AGENTMAIL_API_KEY environment variable, keyring entry
         agentmail:BearerAuth …. Run `auth status` … and check for shadowing."

after:  "Credentials were supplied via: AGENTMAIL_API_KEY environment variable."

AuthCredentialSource::populated_credential_hints filters through try_resolve — deliberately try_resolve, not resolve, so a keyring entry that exists but cannot be read (denied prompt) still counts as configured rather than misdirecting. Exposed via a defaulted AuthProvider method overridden only in the bearer/basic schemes and both composites, leaving the ten other impls untouched. Cli and Closure sources pass through unfiltered — the former resolves post-finalize, and re-invoking the latter could have side effects. Only whether a source resolved is used; no values are read.

11 — Custom-command scaffold

11(a) ClientConfig::default() carries base_url: String::new() for any API with no declared environment (see seed/rust-sdk/no-environment), so sdk::client(ctx) produced a client with no host and every custom command failed on a relative URL before the injected executor could help. The bridge now seeds from a new AppContext::effective_base_url(), mirroring the built-in path's --base-url > doc.base_url > server-root resolution.

New behavior: SDK network error: builder error → a real HTTP status from the API. This did not reproduce on a spec that declares environments, which is why it needed their spec to catch.

11(b) covered under item 1. 11(c) already fixed — the emitted .fernignore contains cli/<bin>/custom.rs.


Found during verification

Five bugs that were not in the report and that I only found by regenerating and building the customers' CLIs and driving every operation. Four are fixes to this PR's own earlier commits — none of them were caught by a unit test, which is the argument for the end-to-end pass:

Per-operation servers:. AgentMail's spec declares servers: on each operation and none at the root, so my first effective_base_url() read doc.root_url (empty) and the bridge still produced a hostless client — the same symptom 11(a) was meant to remove. It now falls back to the first operation that declares a server, walked in sorted order for determinism, mirroring effective_root_url(method, doc) on the built-in path.

Self-contradictory required lists. required_by_spec correctly restored a required parent that recursion had dropped — but it also added parents whose leaves were already listed. The executor rejects combining --a with --a.b, so following the advertised contract verbatim failed with Cannot combine --settings with --settings.auth_type. --schema now drops any ancestor of another required entry; the required leaves already imply the parent, and a required parent with no required leaves (the workflow case the field was added for) is untouched. Hit 8 of 337 ElevenLabs operations and 1 of 130 AgentMail ones; a sweep of both specs now reports zero.

The populated-hints filter never engaged. I added populated_credential_hints to AnyAuthProvider and AllAuthProvider but not to LayeredAuthProvider or RoutingAuthProvider, which inherited the trait default and returned the unfiltered hints. A RoutingAuthProvider is built whenever any operation declares per-operation security: — 129 of AgentMail's 130 do — so item 10(b) was unfixed on the common case while passing its unit tests. Audited the remaining five AuthProvider impls: the three OAuth-ish ones build prose from their own config rather than delegating, so the default is correct for them.

--help value name. The item_type plumbing reached --schema and the collector but not the help renderer, so an array-of-objects flag advertised <STRING> while --schema said items: {type: object}. value_name_for now prefers item_type on repeated flags: --inputs <STRING>--inputs <JSON_OBJECT>, --tags <STRING|null> unchanged.

anyOf: [array, null] parameters. My parameter fix reached 2 of the 31 array query parameters on the ElevenLabs spec. The other 29 use pydantic's Optional[list[T]] spelling, and OpenApiParamSchema had no composition fields at all. resolve_param_nullable_branch is the parameter counterpart to recognize_nullable_composite, which operates on OpenApiSchemaObject and so only ever reached body properties. My earlier commit message claimed arrays never set repeated "for any spelling"; it covered only the spellings it could see.


Changes Made

  • sdk/src/openapi/executor.rs — retry-safety gate; key generation no longer suppressed by the marker; validate_value scalar/array branch; check_json_type; resolve_schema_chain.
  • sdk/src/sdk_executor.rs — retry safety from the request's Idempotency-Key.
  • sdk/src/openapi/parser.rs$ref + composition on OpenApiParamSchema; resolve_param_schema_ref; resolve_param_nullable_branch; repeated for array parameters; array_item_type; required_by_spec.
  • sdk/src/openapi/commands.rsmethod_has_pagination gate; value_name_for (element-type-aware --help value names).
  • sdk/src/openapi/app.rseffective_base_url() (+ per-op server fallback); tolerant pagination reads; globalFlags additions; item-type-aware repeated collector.
  • sdk/src/openapi/help.rs — real array element type; required_by_spec in input.required; ancestor-drop so a required parent and its required leaves are never both advertised; multipart/form-data body fields in the per-op contract.
  • sdk/src/openapi/skill_emitter.rs — conditional per-op flag rows; AuthStrategy-aware auth setup; correct --format default.
  • sdk/src/auth/{credential,provider,schemes,compose,error}.rspopulated_credential_hints, including the Layered / Routing composite overrides without which the filter was inert.
  • sdk/src/openapi/discovery.rsitem_type, required_by_spec.
  • sdk/.sdk-ignore.json — exclude LICENSE.
  • src/emitPublishWorkflow.ts — launcher exit codes; pre-release dist-tags; dist build profile.
  • src/writeLicense.ts (new), src/runPipeline.ts, src/cli.ts — config-driven LICENSE.
  • src/emitReadme.ts — Attribution section.
  • src/generateSdk.ts — bridge seeds base_url.
  • Changelog entries under generators/cli/changes/unreleased/.
  • Updated README.md generator (if applicable) — n/a

Testing

  • Unit tests added/updated
  • Manual testing completed

1935 lib tests (up from 1916 at branch start) + 401 cli-generator TS tests. New coverage:

  • tests/idempotency_on_retry.rs — wiremock, reproducing the reported A/B on a 500ing mock: marked POST retries with the same key on every attempt; unmarked POST makes exactly one; an explicit key is used verbatim.
  • test_validate_body_rejects_wrong_type_behind_a_ref (+ the inverse guard), resolve_schema_chain coverage.
  • test_ref_typed_parameter_resolves_through_component_schemas, test_array_typed_parameter_is_repeatable, test_nullable_array_parameter_is_repeatable, test_true_union_parameter_is_not_promoted, test_nullable_scalar_parameter_resolves_its_type.
  • test_array_of_objects_records_its_element_type, test_recursed_object_property_keeps_the_spec_required_bit.
  • test_pagination_flags_hidden_without_metadata / _shown_with_per_op_config.
  • test_effective_base_url_falls_back_to_per_operation_server.
  • names_only_the_sources_that_actually_hold_a_value, shadowing_advice_appears_only_with_more_than_one_source (rewritten — it had encoded the bug, asserting an unpopulated keyring gets named).
  • shared_skill_omits_per_op_flags_the_spec_cannot_produce / _advertises_per_op_flags_when_the_spec_has_them / _says_set_one_of_for_alternative_schemes.
  • emitPublishWorkflow.test.ts — signal exit codes, pre-release dist-tags, unified build profile.
  • writeLicense.test.ts — 5 cases including the missing-mount (remote generation) path.
  • required_drops_an_ancestor_whose_leaves_are_also_required — pins both directions: the ancestor is dropped, and a required parent with no required leaves survives.
  • multipart_fields_appear_in_the_input_contract — required file part, repeated file part, text part, and a builtin-colliding name that must stay unadvertised.
  • test_repeated_flag_value_name_uses_the_element_type.

Both customers' CLIs regenerated, built, and driven end to end — this is what found the three defects above, two of which were regressions from this PR's own commits and neither of which any unit test caught. AgentMail: 13 of 15 checks clean, the two failures being the bugs now fixed. ElevenLabs: all 17 must-work checks pass, 333 dotted leaf flags, 0 of 337 operations accept --page-all, POST=1/GET=4 retry attempts.

The item-type collector change was the highest-risk item and is clean: string-element arrays are byte-identical (--tags 123["123"], --tags '{"a":1}' → literal), object elements decode correctly, and a synthetic spec confirmed integer/number/boolean elements type correctly with clean validation errors on bad input. The try_resolve keyring read on the auth-error path was also checked for OS prompts on macOS with a real keychain entry stored: no prompt, no hang, 0.05–0.09 s across repeated runs. (Note BearerAuth::apply already called try_resolve on every request before this PR, so keyring reads on the happy path are pre-existing.)

Differential against two real specs. An exhaustive sweep of 336 of 337 ElevenLabs operations, synthesizing a spec-valid body for each from openapi0.json and comparing pre-branch vs branch: 35 failures on both sides, zero regressions, and byte-identical dry-run payloads for all 301 operations that pass on both. The 35 shared failures are the synthesizer's limits, identical on each side. Plus 10 hand-picked operations on the shapes most at risk — that spec has 7 top-level request bodies that are pure unions with no type keyword, all still accepted.

Clippy clean in the changed files; the too_many_arguments warning on build_resource_command is pre-existing (6 occurrences of that lint on main without this change).

Behavior changes to review deliberately

  1. Retries drop substantially — from "every POST/PUT/PATCH retries up to 4x" to "only declared-idempotent or explicitly-keyed ones do". This is the fix that was asked for, but it is visible to anyone relying on the old aggressiveness.
  2. Pagination flags vanish on metadata-free specs — 337 of 337 operations on the control spec. Scripts already passing --page-all go from exit 0 to exit 3.
  3. $ref'd enum parameters are now enforced — 22 params across 19 operations on the control spec. Off-enum values that were previously forwarded to the server are now rejected locally. Correct (inline enums always were), but a spec whose enum is narrower than the live API will feel it.
  4. Array flags with non-string elements JSON-decode each occurrence — required for the corrected --schema contract to be usable. String-element arrays are unchanged.

Pre-existing bugs surfaced, not fixed here

Present identically on this branch and on a pre-branch baseline; filing separately rather than widening this PR:

  • A body property that is a bare oneOf union of objects is typed string and serializes as a quoted string on the wire (api-keys create-public-key --scope). Same root cause as the array case below — a true union has no single shape to promote — and the property is optional there, so no advertised contract is broken.
  • Array items that are a bare anyOf/oneOf union of objects still serialize as JSON strings on the wire (5 operations). array_item_type returns None for a union with no type/properties/allOf, so the element stays literal. Pre-existing — before this PR all non-string array elements were literal — so the fix is incomplete rather than regressive.
  • Query array parameters report items: {type: array}, because item_type is only assigned in the body path. Harmless at runtime (the collector sees None and keeps literals, which is correct for these) but the advertised element type is wrong.
  • conversational-ai knowledge-base search advertises the property as query while the flag is --query-param (renamed to avoid the global JMESPath --query); the rename is not disclosed in --schema.
  • Integer query params aren't type-checked: voices search --page-size abc exits 0 and serializes ?page_size=abc.
  • Required-nested-inside-optional-parent over-strictness on 12 operations.

Nine fixes from AgentMail's post-1.0.0 review (they shipped on 0.31.2; a few
items in that report were already fixed between then and 0.38.4 — see the PR
body for the triage).

**Retry safety.** `decide_retry` was passed
`method.idempotent || idempotency_key.is_some()`, and since the auto key is
generated for every POST/PUT/PATCH, that made every non-idempotent operation
retry-eligible: a 5xx on a create retried ~4x against endpoints with no
idempotency support at all. Retry-safety now requires a key the *server* is
known to honor — `x-fern-idempotent: true` or an explicit `--idempotency-key`.

Compounding it, the marker *suppressed* key generation: it was read as "the
caller supplies one", so a marked operation invoked without the flag retried
with no key, while the same operation without the marker got one. The marker
only means the operation exposes the flag, so only a key actually present now
suppresses generation.

`sdk_executor` had the same shape hardcoded (`true, // SDK requests are
treated as idempotent`), making every custom-command POST retry as idempotent.
It now derives safety from the request's own `Idempotency-Key` header, the only
signal available to a transport handed a built request.

**Validation.** `validate_value` had an object branch and nothing else, so a
`$ref` to a scalar or array component accepted any value. On a spec where most
schemas are component refs, validation was effectively off and `--dry-run`
exited 0 on a malformed body. Scalars and arrays are now type-checked (sharing
one `check_json_type` with the inline path so they cannot drift again), and
bare-`$ref` component chains are followed via a bounded iterative resolver.

Relatedly, `convert_parameter` had no access to component schemas and
`OpenApiParamSchema` had no `$ref` field, so a `$ref`'d parameter deserialized
to all-`None` — no type, enum, format or bounds. Array-typed parameters also
never set `repeated`, for any spelling, so `--labels a --labels b` was
rejected outright.

**Agent contract.** Pagination flags are registered only where the spec says
how to page; they were advertised everywhere and the executor's `pageToken` /
`nextPageToken` fallback silently returned page 1 with exit 0. Generated skills
now verify auth with `auth status` instead of `--help`, which reads no
credentials.

**Packaging.** The npm launcher called `process.exit(e.status)` on a signal
death, where `status` is null and Node coerces it to 0 — CI timeouts, SIGSEGV
and OOM-kills all reported success. Signal deaths now exit 128+signum. And any
SemVer pre-release gets a non-latest dist-tag; matching only `-alpha`/`-beta`
let `-rc.1` and `-next.1` move `latest`.

**Custom commands.** The SDK bridge built `ClientConfig::default()`, whose
`base_url` is empty for any API with no declared environment, so
`sdk::client(ctx)` failed on a relative URL. It now seeds from a new
`AppContext::effective_base_url()` mirroring the built-in path's resolution.

1922 lib tests pass (up from 1916) plus a wiremock suite reproducing the
reported A/B on a 500ing mock.
@cadesark cadesark self-assigned this Aug 26, 2026

@nitpickybot nitpickybot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the changes — everything looks good. No issues found.


To request another review, comment /ai-review on this pull request.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

…ase URL

Caught regenerating AgentMail's real CLI: their spec declares `servers:` on
each operation and none at the root, so `AppContext::effective_base_url()`
read `doc.root_url` (empty) and the bridge still produced a client with no
host — `SDK network error: builder error` on every custom command, the same
symptom the fix was meant to remove.

The built-in path already handles this via `effective_root_url(method, doc)`,
but the bridge has no operation in scope. It now falls back to the first
operation that declares a server, walked in sorted order so the answer is
stable across runs (the resource/method trees are `HashMap`s).

Verified against their spec: the custom command now reaches
`api.agentmail.to` and returns a real 403 for a bad key instead of failing to
build a URL.
@cadesark

Copy link
Copy Markdown
Contributor Author

End-to-end verification on AgentMail's real config

Generated their CLI with this branch via seed run --generator cli --path fern/apis/cli --local against agentmail-to/agentmail-docs, built it with cargo, and exercised it against local capture / 500ing servers. Their spec is the good test case: no root servers: block, no x-fern-pagination anywhere, and 1057 $refs.

This found a real flaw in the fix — pushed as a second commit

Their spec declares servers: per-operation and none at the root, so AppContext::effective_base_url() read doc.root_url (empty) and the bridge still produced a hostless client — same SDK network error: builder error the fix was meant to remove. effective_base_url() now falls back to the first operation that declares a server (sorted-order walk for determinism), mirroring what effective_root_url(method, doc) does on the built-in path.

Before: SDK network error: builder error. After: a real 403 Forbidden from api.agentmail.to — the request reached the host.

Results

Report Before After (this branch, their spec)
1a POST on 5xx ~4 attempts 1 attempt; auto key still sent
— GET on 5xx 4 attempts 4 attempts (correctly unchanged — idempotent by HTTP)
1b marked-idempotent send on 5xx 4 attempts, no key at all 4 attempts, same key on all four
2 --json '{"url": 123}' on webhooks create accepted, exit 0 url: Expected type 'string', found integer
— well-typed body accepted still accepted
2 --labels a --labels b rejected ?labels=alpha&labels=beta
--labels '["a","b"]' one literal string ?labels=alpha&labels=beta (identical)
4 pagination flags on inboxes list --help 4 advertised 0; --page-all now rejected with a similar argument exists: '--page-token'
6 generate-skills auth step agentmail --help agentmail auth status
11a sdk::client(ctx) with no --base-url builder error reaches api.agentmail.to
— with --base-url worked still wins

For 1b I restored the x-fern-idempotent: true marker on inboxes.messages.send that their overrides currently warn against adding — with this branch that warning no longer applies and they can put the marker back, which is the outcome they wanted.

Items 3 and 7 are workflow-template changes; the output mode here is local_files so no ci.yml is emitted. They're covered by the new assertions in emitPublishWorkflow.test.ts, which assert on the emitted YAML string directly.

1923 lib tests pass (up from 1916), including a new test_effective_base_url_falls_back_to_per_operation_server pinning the flaw above.

…nfig

**Build profile (report item 8).** `ci.yml` built `cargo build --release`
while cargo-dist's `release.yml` builds `--profile dist` (release + thin LTO),
so npm and the GitHub Release shipped different binaries for the same tag —
9.8 MB vs 10.9 MB in the reporter's measurements. npm is a distribution
channel, so it now uses the profile named `dist`. The copy path moves with it:
a custom cargo profile writes to `target/<triple>/dist/`, not `.../release/`.

**LICENSE (report item 9).** The generator shipped `Cargo.toml
license = "MIT"` (from `packageIdentity.license`) alongside an Apache-2.0
`LICENSE` file — a direct contradiction the reporter asked us to settle before
wide adoption.

The Apache-2.0 file was not a licensing decision: `copySdk` copies `./sdk/`
verbatim and `sdk/LICENSE` simply wasn't excluded, so the vendored runtime's
license rode along into all 49 seed fixtures. No other Fern generator ships a
LICENSE by default — a survey of every seed tree finds exactly two LICENSE
files outside `seed/cli`, both from explicit `custom-license` fixtures, and
`RustProject.writeLicenseFile` returns early for anything but `type: custom`.

So the CLI now behaves like the fleet: `LICENSE` is excluded from the SDK copy
via `sdk/.sdk-ignore.json`, and a new `writeLicense` step emits one only when
the customer configures `license: { type: custom }`, copying the Fern CLI's
`/tmp/LICENSE` mount and honoring a configured filename. A basic license
(`license: MIT`) mounts no file and stays package metadata, as elsewhere.
License text is never authored here.

Verified by regenerating AgentMail's CLI: no LICENSE emitted, `license = "MIT"`
still stamped in Cargo.toml.
The generated repo contains `fern-cli-sdk` copied verbatim by `copySdk`, and
Apache-2.0 s4 requires retaining that license notice when redistributing the
covered code. The previous commit stopped shipping the runtime's `LICENSE`
file — it contradicted the `license` field `packageIdentity` writes into
Cargo.toml, and no other Fern generator ships a LICENSE it wasn't asked for —
which removed the contradiction but also removed the notice.

The README now carries it as its own `## Attribution` section, so the notice
survives while the repo's own license stays whatever the customer declared.
Emitted as a discrete Block, which means `mergeWithExisting` treats it like any
other section: a customer who rewords it keeps their wording across regens.

Verified on AgentMail's generated CLI: the section renders, reaches the table
of contents, and no LICENSE file is emitted.
…are $ref

A differential run against the ElevenLabs spec showed the parameter fix in
0508357 reached 2 of its 31 array query parameters. The other 29 are
spelled `anyOf: [{type: array}, {type: 'null'}]` — pydantic's
`Optional[list[T]]` — and `OpenApiParamSchema` had no composition fields at
all, so those parameters still deserialized to all-`None`. The commit message
claimed arrays never set `repeated` "for any spelling"; the fix only covered
the two spellings it knew how to see.

`resolve_param_nullable_branch` is the parameter counterpart to
`recognize_nullable_composite`, which operates on `OpenApiSchemaObject` and so
only ever reached body properties. It takes the single non-null branch of a
`oneOf`/`anyOf`, resolves it through `$ref`, and leaves true unions (more than
one non-null branch) opaque.

Verified on the real spec, before -> after:

    --voice-ids v1 --voice-ids v2   rejected            -> ?voice_ids=v1&voice_ids=v2
    --voice-ids '["v1","v2"]'       ?voice_ids=%5B%22v1%22,%22v2%22%5D
                                                        -> ?voice_ids=v1&voice_ids=v2
    --voice-ids v1                  ?voice_ids=v1       -> ?voice_ids=v1  (unchanged)

The URL-encoded-JSON row is the one that mattered: it was near-certainly wrong
on the wire and failed silently.

Also corrects two changelog claims: the parameter entry now names all three
spellings, and the pagination entry states plainly that a script already
passing `--page-all` on a metadata-free spec goes from exit 0 to a hard error.
@cadesark

Copy link
Copy Markdown
Contributor Author

Deferred items from the AgentMail review, and where --schema stands

Recording this for the ADR-0006 conversation.

Item 5(a) — httpMethod / path were dropped deliberately

Not a regression to restore. ADR-0006 states it as a decision:

The principle is agent-aligned, not OpenAPI-aligned… Fields the agent doesn't need (httpMethod, path) are dropped at every scope.

Provenance, since the report attributes it to 0.28.1:

  • The drop landed in chore(cli-generator): sync cli-sdk@e16a44e #16521 chore(cli-generator): sync cli-sdk@e16a44e (commit 69ad14232d8, 2026-06-26) — a one-way vendored sync. That same commit brought ADR-0006 and rewrote the tests to assert the absence (assert!(schema.get("httpMethod").is_none())).
  • The actual decision was upstream in cli-sdk PR improvement: add model docs #169, which ADR-0006 cites in its Context. fern-api/cli-sdk is archived, so that PR isn't traceable from this monorepo.
  • It shipped in the 0.19.x range, not 0.28.1 — 0.28.1's changelog is entirely wire-test namespace stutter elision.

Restoring the fields means amending ADR-0006 (recording that real agent consumers wanted the HTTP plumbing to correlate with API docs) plus flipping the code and three assertions. Happy to write that once the decision is made.

Item 5(b) — 3 genuine globalFlags gaps, not 8

Current list is 10: --schema, --dry-run, --format, --base-url, --<ua-suffix>, --quiet, --debug, --query, --spec, --spec-raw.

Missing but registered on every operation: --params, --no-retry, --no-extract (+--help). The other five they listed are per-op and correctly excluded — and the pagination gate in this PR is what finally makes that ADR claim true, since before it --page-all really was advertised everywhere.

Two worse --schema bugs, found by the ElevenLabs differential

Same "an agent can't trust --schema" complaint, and higher impact than the missing flags. Both pre-existing — present identically on this branch and on the pre-branch baseline:

  1. Array-of-object body properties are advertised as strings. text-to-dialogue convert --inputs reports items: {type: string} and <STRING>, but the spec is an array of DialogueInput (required text, voice_id). An agent reading --schema builds ["x"] and gets hard-rejected. The validator is right; the advertisement is wrong. ~6 properties on that spec (tests, recipients, items, pronunciation_dictionary_locators, deployment_request.requests).
  2. input.required disagrees with the validator. conversational-ai agents drafts create omits workflow from input.required, then the validator rejects the body with $: Missing required property 'workflow'.

Worth folding into 5(b) rather than shipping the globalFlags additions alone.

Item 10(b) — confirmed reproducible

decorate_with_source_hint (sdk/src/auth/error.rs:131) lists every declared credential source rather than the ones that resolved, and gates its shadowing advice on hints.len() > 1 — always true for a two-scheme API. With only AGENTMAIL_API_KEY set:

"help": "Credentials were supplied via: AGENTMAIL_TOKEN environment variable, keyring entry
         agentmail:TokenAuth …, AGENTMAIL_API_KEY environment variable, keyring entry
         agentmail:BearerAuth …. Run `auth status` to see all visible sources and check for shadowing."

Both halves of their complaint from one function. Fix: filter hints to populated sources (env::var(name).is_ok(), keyring entry exists — no credential values touched, so ADR-0001 holds) and gate the shadowing sentence on more than one populated source. ~20 lines.

Needed nothing — already fixed between 0.31.2 and 0.38.4

Report Status
10(a) double-encoded error bodies Body now lands as nested JSON under details, message/reason lifted out
5(c) root --help listing 4 --format values Root and subcommand both list all 7 identically
11(c) custom.rs not in .fernignore Emitted .fernignore contains it

Other pre-existing bugs surfaced, unfiled

  • Integer query params aren't type-checked: voices search --page-size abc exits 0 and serializes ?page_size=abc.
  • Required-nested-inside-optional-parent over-strictness on 12 operations; one variant surfaces as a confusing Cannot combine --settings with --settings.auth_type when the parent is supplied as JSON.

… errors

AgentMail sent the verbatim text of `<bin>-shared/SKILL.md`, the file every
per-resource skill names as PREREQUISITE. Their 12 per-resource files are
accurate; this one had five errors. Four are fixed here (the fifth, the
`--help` auth step, was fixed in 0508357).

**Flags that do not exist.** `-o, --output` was emitted unconditionally, so on
an API with no binary responses it documented a flag the parser rejects — 0 of
their 130 operations accepted it. Same for the pagination flags. Both rows are
now gated on `doc_has_binary_response` / `doc_has_pagination`, mirroring the
registration conditions in `commands.rs`. The `ffplay -` / `aplay -` examples
were audio-API boilerplate; the description is now media-agnostic.

**`--schema` hints that cannot appear.** The paragraph told agents to "check
the per-op `--schema` output's `paginable` / `binaryResponse` hints". Those keys
are real, but only ever emitted for a spec that has such operations — for
theirs, neither could appear, so the advice was dead. The paragraph now names
only the affordances the API actually has, and collapses to "available on every
command in this CLI" when there are none.

**Both auth env vars exported.** `any: [BearerAuth, TokenAuth]` are alternative
schemes for the same credential, and the file told agents to export both —
which is precisely the shadowing scenario `decorate_with_source_hint` warns
about. `AuthStrategy` is now threaded to the emitter: anything but `All` says
"set **one** of". `Any` and `Auto` both apply one scheme per request, so `All`
is the only strategy that genuinely needs every variable.

**Wrong `--format` default.** Advertised `json`; the real default is `table` on
a TTY and `json` when piped, which subcommand `--help` has always stated
correctly.

Verified by regenerating their CLI and running `generate-skills`: all four
corrected, and the flag table now lists only the 7 flags that exist on every
one of their operations.
**Item 10(b) — auth hints.** `decorate_with_source_hint` listed every
*declared* credential source and gated its shadowing advice on
`hints.len() > 1`, which is always true on a two-scheme API. With only
`AGENTMAIL_API_KEY` set, the 401 named all four of that CLI's sources and told
the user to go looking for shadowing between variables they had never set.

`AuthCredentialSource::populated_credential_hints` filters through
`try_resolve`, and a defaulted `AuthProvider` method (overridden by the bearer
/ basic schemes and both composites) exposes it, so the ten other impls are
untouched. `try_resolve` rather than `resolve`: a keyring entry that exists but
cannot be read is *configured*, and hiding it would misdirect. `Cli` and
`Closure` sources pass through unfiltered — the former resolves post-finalize,
and re-invoking the latter could have side effects. Only whether a source
resolved is used; no values are read.

**Item 5(b) — globalFlags.** `--params`, `--no-retry`, `--no-extract` and
`--help` are registered on every operation but were absent from `globalFlags`,
so an agent coding against `--schema` could not discover them. The five other
flags the report listed are genuinely per-op and stay out — and the pagination
gate earlier in this PR is what makes that ADR-0006 claim true, since before it
`--page-all` really was advertised everywhere.

**Two `--schema` bugs the ElevenLabs differential surfaced**, both worse than
the missing flags because they make the contract actively wrong:

*Array element types.* A repeated flag carries `param_type: "string"` — the
flag surface, since clap collects strings — and `--schema` rendered that as the
element type. For `text-to-dialogue convert --inputs`, an array of
`DialogueInput`, the contract said `items: {type: string}`; an agent sent
`["x"]` and the validator correctly rejected it. `array_item_type` resolves the
real element type through `$ref` and nullable compositions onto a new
`item_type`, which `--schema`/`--help` render and the collector consults, so
`--inputs '{...}' --inputs '{...}'` now decodes to an array of objects instead
of an array of literal strings. `None` means string — byte-identical to every
previous lowering.

*Required list.* An object-valued property the parser recurses into had
`required: false` hardcoded on its shorthand flag. That is right for clap (leaf
flags can satisfy it) but wrong for the contract: `agents drafts create`
advertised 5 required fields, and supplying all 5 still failed on `workflow`,
a 6th that was never listed. `required_by_spec` carries the spec's bit for
`--schema` while clap keeps using `required`.

Verified on the real spec: the 6-field contract is now followable, repeated
object elements build the right array, and plain string arrays are unchanged.
…th filter reach real CLIs

Three defects found by building both customers' CLIs from this branch and
driving every operation. Two are regressions from this PR's own commits.

**Self-contradictory `required` (regression, 875c0cd).** `required_by_spec`
correctly restored a required parent that recursion had dropped — but it also
added parents whose leaves were *already* listed. The executor rejects
combining `--a` with `--a.b`, so following the advertised contract verbatim
failed:

    required = [settings, settings.auth_type, settings.name, settings.webhook_url]
    -> "Cannot combine --settings with --settings.auth_type"

`--schema` now drops any ancestor of another required entry. The required
leaves already imply the parent must be present, and they are the form a caller
can use together; a required parent with no required leaves — the `workflow`
case `required_by_spec` was added for — is untouched. Affected 8 of 337
ElevenLabs operations and 1 of 130 AgentMail ones; a sweep of both specs now
reports zero.

**The populated-hints filter never engaged (regression, 875c0cd).** I added
`populated_credential_hints` overrides to `AnyAuthProvider` and
`AllAuthProvider` but not to `LayeredAuthProvider` or `RoutingAuthProvider`,
which inherited the trait default and returned the *unfiltered* hints. A
`RoutingAuthProvider` is built whenever any operation declares per-operation
`security:` — 129 of AgentMail's 130 do — so item 10(b) was unfixed on the
common case while looking fixed in unit tests. Verified on the rebuilt CLI:

    before: "Credentials were supplied via: AGENTMAIL_TOKEN …, keyring …,
             AGENTMAIL_API_KEY …, keyring …. More than one source has a value…"
    after:  "Credentials were supplied via: AGENTMAIL_API_KEY environment variable."

Audited the remaining five `AuthProvider` impls: the three OAuth-ish ones build
prose from their own config rather than delegating, so the default is right for
them.

**`--help` value name (incomplete work, 875c0cd).** The `item_type`
plumbing reached `--schema` and the collector but not the help renderer, so an
array-of-objects flag advertised `<STRING>` while `--schema` said
`items: {type: object}`. Extracted `value_name_for`, which prefers `item_type`
on repeated flags. `--inputs <STRING>` -> `--inputs <JSON_OBJECT>`;
`--tags <STRING|null>` unchanged.
CI's `lint` and `biome` jobs failed on two formatting nits: a stray blank
line left behind when a test was moved between describe blocks, and a
line-wrap in `writeLicense.ts`.

Also removed the `as NodeJS.ErrnoException` in that file — `"code" in error`
already narrows to `Error & Record<"code", unknown>`, so the comparison needs
no assertion, and CLAUDE.md forbids one where a guard suffices.
…tract

Found by the ElevenLabs differential and the worst of the remaining
`--schema` gaps: multipart body fields live in `method.multipart_fields`, not
`method.parameters`, and `build_operation_schema` only walks the latter. So an
upload operation advertised its query parameters and headers and nothing else:

    audio-isolation convert --schema
      required   : []
      properties : ['xi-api-key']

while the spec's multipart schema has `audio` (required), `file_format` and
`preview_b64`. The flags existed and worked — check the `--help` output or the
repeatable-`--files` fix earlier in this PR — they were simply invisible to
anything reading the contract, so an agent driving purely from `--schema` could
not invoke *any* multipart operation. 28 operations on that spec.

Fields are emitted into the `body` bucket as strings, which is what the flag
surface actually takes: a text part its value verbatim, a file part a
filesystem path. `file: true` is what tells an agent to pass a path rather than
the content; a per-part `contentType` surfaces when the spec pins one; repeated
parts render as an array of strings. Builtin-colliding names are skipped,
mirroring `build_resource_command` — those args are never registered, so
advertising them would be the same class of lie this commit removes.

    after:
      required   : ['audio']
      properties : ['audio', 'file_format', 'preview_b64', 'xi-api-key']
      audio      : {"type":"string","location":"body","file":true,"description":…}

Verified by reading the per-op contract for all 337 operations of that spec,
then driving each of the 28 multipart ones from exactly the fields its own
`input.required` lists: 28 of 28 exit 0. Previously none were followable.
CI's `biome` job runs `biome check`, which includes the
`assist/source/organizeImports` rule that `biome format` and `biome lint` do
not. My `writeLicense` import went in next to `emitReadme` rather than in
sorted position, which only `pnpm check` catches locally.
@cadesark
cadesark merged commit 2dc5ea1 into main Aug 27, 2026
60 checks passed
@cadesark
cadesark deleted the cade/cli-agentmail-review-fixes branch August 27, 2026 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants