You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
A refusal must not destroy generated output: fixed in Kotlin and TypeScript, swept across the rest - #932
Follow-up to #931, which merged about nine minutes after it opened — roughly forty seconds before Copilot's review landed. That review found eight things, all of them real, and four further rounds found more. #925 stays closed; nothing here reopens the hole, and everything here is a place #931's claims were not yet true.
Why this should not sit. One of these is a data-loss fix, and it is live on main right now. A validation refusal in the Kotlin generator erases the committed generated tree — it cleared models/, services/, Metadata.kt and ServiceAccessors.kt before parsing, so the fail-closed behaviour #931 introduced destroys output on the way out. Everything else here is correctness; that one is damage.
What this PR does not do
One class found here is not fixed here: the six generators each validate spec/generated-verbs.json themselves, and they disagree about what it means. Five instances in four rounds, each a different language for a different reason — Kotlin's jsonPrimitive.content coercion, Rust's filter_map, Ruby's ASCII-only String#strip, Swift's allSatisfy being vacuously true for the empty string, JavaScript's $ matching before a final line terminator.
That has been escalated and decided, and the decision is a restructure rather than another predicate: one strict validator, six readers that take the strings verbatim, with the validator wired as a prerequisite of every *-generate target and into make check. It is a separate PR because this one carries a data-loss fix that should not wait on a mechanism change.
So this PR leaves two known holes in the per-loader check it inherits — Swift accepts "", TypeScript accepts "get\n" — both on malformed declaration content only, both strictly better than main's state, and both deleted outright by the follow-up, which removes the per-loader check rather than mending it. Fixing them here would have been the sixth predicate fix in four rounds and would have obscured the decision.
The decision history, since it is more useful than a tidy narrative: the first repair required a non-blank string in every loader; the second replaced that with a positive [a-z]+ format rule on the reasoning that defining the allowed set makes whitespace, case, BOMs and normalisation moot. Neither held. The format rule did not narrow the class — it relocated it, from "what counts as blank" to "what does this language's predicate actually match". The population is bounded by the union of six standard libraries' string and regex semantics, not by the rule being expressed, which is why each round found another. That call was made deliberately and was wrong, and it is what the restructure is a response to.
PATCH leaves the declaration
spec/generated-verbs.json named five verbs because all six generators' old lists did. Two of the six runtimes cannot serve one of them:
Ruby — BaseService delegates get post put delete post_raw put_raw. The generator emits http_patch(...), which is NoMethodError at call time.
Kotlin — ServiceEmitter's "PATCH" -> branch emits httpPut, putting the wrong method on the wire.
The Basecamp API models no PATCH operation, so nothing regressed and nothing regenerates differently — but a declaration whose whole job is to name what the SDKs can actually emit was false on its first day. A PATCH operation now refuses generation by name, which is the same treatment HEAD gets and for the same reason. The declaration records the two fixes needed to put it back.
scripts/gen-catalog/main.go still lists patch in its own bound. That generator is owned elsewhere and untouched here; it only ever accepts a superset of what these generators emit, so a looser bound there cannot admit anything the strict one lets through.
A refusal must not destroy the output tree
Kotlin's Main.kt deleted models/*.kt, services/*.kt, Metadata.kt and ServiceAccessors.ktbefore parsing. A refusal therefore erased the committed generated tree — a validation failure turned into data loss, and #931's "no partial output was written" claim was wrong for the one generator whose default output is the committed tree.
The first repair was "parse before delete", and both halves of that were wrong. Parsing is only one thing that can refuse — semantic validation is another, and every !! on a field that might be absent is a third; MetadataEmitter.parse still ran after the clean. And deleting is only one way to destroy: overwriting is equally destructive, which is how the TypeScript instance survived two passes, because that pipeline never deletes.
The invariant, stated correctly: no destructive write — delete or overwrite — before everything that can refuse has run.
Where the class was found, and what was done
Kotlin renders its whole output into a map and clears only afterwards, so no failure anywhere in generation can destroy the tree. Swift had the same shape — it removed Models/ and Services/ then emitted — and got the same fix. The original finding was Kotlin-only because Kotlin's default output is the committed tree, not because Swift was safe.
TypeScript's pipeline is the second place, and it is fixed the way Kotlin was, minimally: strip-account-id.ts is step one, its output is a committed artifact, and it now walks every path item through the shared helper before it writes. paths only — every downstream step walks spec.paths, so validating webhooks would refuse over something no generated artifact is built from — and no emission bound, which belongs to generate-services.ts and runs before its first write.
Redirects were a third instance one layer down: the shell truncates > targets before the command runs, so rb-generate's metadata.json and types.rb and the TypeScript pipeline's metadata.ts and person-id-sites.ts were emptied before their generator could refuse. All four use a temp file and a move, clearing the fragment on failure.
The sweep, since "I looked and found nothing" is not checkable
generator
first destructive write
validation before it?
Kotlin
Main.kt clears the four generated paths
fixed — renders every file first
Swift
main.swiftremoveItems Models/, Services/
fixed — same shape
Rust
main.rs:152remove_dir_all inside write()
already safe — render(&model)? first
Ruby (services)
generate-services.rb:381 per-service File.write
safe — walk completes at :374; stale deletes last
Ruby (metadata, types)
the Makefile redirect at the committed file
fixed — temp + move
Python (services)
generate_services.py:1275write_text
safe — walk at :1260; sweep deliberately last
Python (metadata)
generate_metadata.py:64 writes args.output
safe — walks before opening the output
TypeScript (services)
generate-services.ts:1791
safe — groupOperations at :1788
TypeScript (pipeline)
strip-account-id.ts, step one
fixed — validates before writing
jq routes
generate-url-routes → .tmp then mv
already safe; fragment now cleaned on failure
Go
—
no repo-owned generator: the client comes from oapi-codegen, which owns its own writing. Not verified the same way, and not in this PR's reach
How strongly each row is held, since "verified" is doing a lot of work there
Proven by a test that fails on the unfixed code: Kotlin. Deleting retry.max from one behavior-model entry fails at the far end of generation, and reverting the render-then-clean ordering makes that scenario — and only that scenario — report four destroyed sentinels. I checked it both ways.
Proven for the refusals the harness drives: Kotlin, Swift, Rust, TypeScript's service generator. Every scenario carries the seeded-tree assertion, so for those inputs nothing is written or destroyed. That is narrower than "no refusal destroys anything".
Verified by reading the code: Swift's ordering for late failures, Rust's render-before-write, and Ruby's, Python's and TypeScript's walk-completes-before-first-write. No crafted input currently reaches those generators' emission stages as a failure, so there is nothing for a test to drive. Named rather than implied.
Out of reach: Go. There is no repo-owned path-item walk to order.
What is NOT claimed
The claim is about the six repo-owned generators, not "every generator" — Go's client is oapi-codegen's and is outside it.
This PR does not make generation atomic, and does not claim to. Every row above means a refusal cannot destroy anything. None of them means a crash mid-write leaves a consistent tree — a failure between the first write and the last still leaves a partial tree in every generator, and a later pipeline step can still fail for reasons unrelated to #925 (patch-flexible-ids.ts refuses when the Person schema layout moves).
A general wrapper that snapshotted src/generated and restored it on any failure was built, verified, and withdrawn: a new general-purpose mechanism introduced as a tail on a defect fix became the thing under review rather than the bug. That work, the sweep above, and the reasoning are filed as #936, which deserves its own PR and its own review.
additionalOperations is named everywhere, not just where a bound catches it
OpenAPI 3.2's additionalOperations is a map of method to Operation. The bounded walks already refused it, as an unemittable verb. The verb-agnostic ones — the four metadata extractors and the jq route generator, which take no bound on purpose — read it as one operation, found no operationId, and skipped it, dropping every operation inside without a word. That is the exact failure #925 is about, one level down.
Every walk now refuses it by name. The route filter also refuses a path-item field that is not an operation object with a non-empty operationId, so a scalar no longer errors obliquely through .value.operationId and an unrecognised object no longer reaches the route table as a method naming null.
PATCH, and the loaders — what changed and what moved
spec/generated-verbs.json gained a [a-z]+ format rule in place of #931's per-loader "non-empty string", which is an improvement on main and is where the loader work stops in this PR. The rest of that class is the follow-up described above.
The compiled generators get a test that can see a regression
The sharpest finding. scripts/test-generator-verb-inversion.rb drives Ruby, Python and jq. TypeScript, Kotlin, Swift and Rust carry four more independent copies of the walk, and nothing in the repository could see them regress: their drift checks regenerate from the committed openapi.json, which declares only verbs the SDKs emit, so all four could revert to silently skipping an unknown verb with every drift check green.
scripts/test-compiled-generator-refusal <language> runs the real generator binary through three scenarios and asserts four things in each: a non-zero exit, a message naming the offending operation, that no generated file was written, and that the output tree it was pointed at is untouched.
An unemitted verb.openapi.json with one operation's verb moved from get to head.
A declaration-sourced bound. The unmodified spec, against a declaration that withholds a verb the spec really uses. Without this, a generator still holding a private five-verb list would pass scenario 1 by rejecting HEAD while ignoring spec/generated-verbs.json entirely — which is exactly how PATCH's removal could have gone unenforced in four languages.
A malformed declaration. The [a-z]+ rule, asserted against the compiled loaders with the non-breaking space that told them apart.
An additionalOperations map, and 5. a non-string operationId — the two guards no committed spec exercises.
A malformed retry entry, and this is the one with the data-loss teeth. Every other scenario fails during the spec parse, which any "parse before delete" ordering already survives, so none of them can tell a generator that renders before destroying from one that merely hoisted its first failure. This one fails at the far end of generation. Reverting the render-then-clean ordering makes this scenario, and only this scenario, report four destroyed sentinels.
It runs for Kotlin alone, and skips by name elsewhere: Swift and Rust default a missing retry field and generate successfully, so the fixture is not fatal for them and a "must fail" assertion would fail for the wrong reason. That gap is real — Swift's render-before-delete ordering is verified by reading the code, not by this test — and it is stated rather than papered over.
The fourth assertion is the one with teeth, and it only works because the output directory is seeded first. An empty directory cannot show data loss — clearing nothing leaves nothing — so the first version of this test passed against the unfixed Kotlin generator. Seeded, it fails on it with four named sentinels, which is how the ordering bug above was confirmed rather than assumed. The seeds are compared by content and the "nothing written" check is an exact-path set difference against them, so an overwritten Metadata.kt or mod.rs fails alongside a deleted one; the seed list is snapshotted by reading the tree back, because macOS folds Models/ and models/ into one directory and the Linux runners do not.
Wired into ts-check, kt-check, swift-check and rs-check, and into each language's CI job, where the toolchain is present by construction so nothing can skip.
One structural note. Kotlin runs the installed distribution rather than the Gradle run task, so every Gradle invocation stays in the Makefile recipe. scripts/check-gradle-serialization resolves one level of script delegation and attributes a delegated Gradle call to every target that calls the script; with the wrapper invoked from the shared script it marked the TypeScript, Swift and Rust targets as Kotlin-project builds and demanded order-only edges describing no real collision. kt-test-verb-refusal joins the kotlin/ chain at its head, because scripts/test-check-gradle-serialization.rb chains its probes after conformance-kotlin — the tail is load-bearing for that self-test.
Verification
make check green end to end, including every drift check: all seven SDKs still regenerate byte-identical.
scripts/test-generator-verb-inversion.rb is up from 21 cases to 48, covering PATCH's refusal, additionalOperations through the unbounded metadata and route walks (the bounded service walks would have rejected it regardless, so the first version of those cases pinned nothing), the six hostile declaration shapes, and the two route-filter shapes. Cases are numbered as they run — hand-numbering went wrong twice inside one review round, and a duplicate makes the mutation counts unreadable.
Refreshed mutation numbers, which are the argument for the inversion rather than a longer list:
walk restored to
cases failing (of 48)
the original five-verb list
15
the eight verbs OpenAPI 3.1 names
8
The eight-verb list repairs the verbs it was extended with and nothing else: 3.2's query and additionalOperations, and every case where a field that is neither a known non-operation field nor a readable operation is stepped over in silence, still fail.
All four compiled generators pass all three scenarios: refused by name, exit 1, nothing written, nothing destroyed.
Also here
operationId is refused, not skipped, in all six walkers.Generators iterate a five-verb list, so an operation on any other HTTP method is silently dropped from every SDK #925's own outcome — an operation silently lost — was surviving with a different trigger. Swift continued past an operation with no operationId and exited zero; the four metadata and person-id extractors skipped it too; the Ruby, Python, TypeScript and Kotlin service generators failed obliquely on a nil, a KeyError or an NPE. Rust was the only one that already refused by name. All six refuse in the shared walk now, naming method and path, rejecting an empty or non-string id alongside an absent one.
Every fallible input read precedes the first delete in the Kotlin generator, not just the spec parse — a malformed --options-order file erased the tree the same way. A failure between the first write and the last still leaves a partial tree; that is a different class from a refusal, and closing it means rendering to a temporary tree and swapping.
generate-path-mapping.ts goes through the shared walker. It had its own walk that skipped parameters by name and stepped silently past an operation with no operationId, so an unrecognised field never reached PATH_TO_OPERATION.
The new Gradle order-only edge is registered in scripts/test-check-gradle-serialization.rb, verified by deleting the edge and watching the self-test fail.
Known red, unrelated to this diff
Trivy (Python SDK) fails on a dependency advisory. This PR changes no Python dependency and no lockfile, and it is not a required status context, so it does not block the merge.
One process note, since it is the reason this is a second PR
#931 merged at 19:45:02Z, opened 19:36Z. Copilot's review arrived at roughly 19:45:40Z and Codex's shortly after. No approving review existed at the merge, and there were no threads to resolve because no review had landed — which is the failure mode AGENTS.md describes for #162, reproduced exactly. Every finding in that review is above. Nothing was lost, but the fixes reached main in a follow-up rather than in the PR that introduced them.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The catalog and verb loaders remain inconsistent, and the new tests do not fully enforce the stated refusal and output-preservation guarantees.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Strengthens generator refusal so unsupported OpenAPI operations fail safely and preserve generated output.
Changes:
Removes unsupported PATCH generation and validates verb declarations.
Refuses unsupported operation maps and malformed path items.
Adds compiled-generator refusal checks across SDK toolchains.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Adding cases 22–28 leaves this file’s header stale: it still says there are 21 cases (and gives mutation counts “of 21”), and it says the four compiled walks are only reproduced by hand rather than covered in CI. Update that overview to describe the 28-case script plus the new compiled-generator harness so future maintainers do not rely on obsolete coverage claims.
check("10. OpenAPI 3.2's `additionalOperations` map is refused by name, not read as one operation") do
status, output, = ruby_services(spec({ "/{accountId}/widgets.json" => {
"get" => operation("ListWidgets"),
"additionalOperations" => { "PURGE" => operation("PurgeWidgets") }
} }))
Review threads: 7 resolved on 0bc968c — 6 fixed, 1 declined and routed.
Fixed
Blank-string rule now identical in all six declaration loaders (Codex + Copilot, same finding). Ruby's metadata extractor validates the declaration at all rather than trusting fetch.
Sentinel comparison is by content, and "nothing written" is an exact-path set difference against the seeds rather than a name exclusion, so Metadata.kt/ServiceAccessors.kt/mod.rs no longer get a pass. Seeds are snapshotted from the filesystem, since macOS folds Models/ and models/ into one directory and the runners do not.
Each compiled generator now runs a second scenario against a declaration with a verb withheld, so it must honour spec/generated-verbs.json rather than merely reject HEAD. This was the sharpest of the round: rejecting HEAD alone proved nothing about whether the declaration is read at all.
The additionalOperations fixtures moved to the Ruby and Python metadata extractors — the unbounded walkers that actually dropped the nested operations. The service-generator cases pinned nothing, since the emission bound rejected the field regardless.
Declined, with the reason in the thread: scripts/gen-catalog/main.go still lists patch in its own bound. The finding is correct. That generator is owned by another lane and my brief permits editing only its tracking-claim comment; the fix is theirs to make, either by dropping patch or by consuming the shared declaration its comment now points at. Routed rather than taken.
Self-test is up to 32 cases. Mutation numbers refreshed: the original five-verb list fails 13 of 32, the eight verbs OpenAPI 3.1 names still fail 6. make check green, all seven SDKs byte-identical.
Review found the declaration and the walks each fell short of the claim.
PATCH leaves the declaration. Every generator's old five-verb list named
it, but Ruby's BaseService delegates only get/post/put/delete and Kotlin
renders a PATCH operation through httpPut, so the declaration named a
verb two SDKs get wrong. No PATCH operation exists, so nothing changes
today; one now refuses by name instead of shipping broken.
The Kotlin generator cleared its output directory before parsing, so a
refusal erased the committed tree. It parses first now.
OpenAPI 3.2's additionalOperations is a map of method to Operation. The
bounded walks refused it as an unknown verb, but the verb-agnostic ones
read it as one operation with no operationId and dropped everything
inside it. Every walk names it now, and the route filter also refuses a
field carrying no operationId rather than emitting a route naming null.
The Kotlin and Rust declaration loaders coerced non-strings where the
other four rejected them.
Four compiled generators had no test that could see a regression: their
drift checks read the committed spec, which carries only emitted verbs.
scripts/test-compiled-generator-refusal runs each real binary against a
HEAD spec and asserts a named refusal that writes nothing and destroys
nothing — seeding the output directory first, since an empty one cannot
show data loss.
…ay six times
A refusal that rewrites a seeded file rather than deleting it left it
present, so the existence check called the output tree untouched while
its contents had already been replaced. The sentinels are hashed before
the run and verified after.
Kotlin rejected a whitespace-only verb and Rust trimmed, while Ruby,
Python, TypeScript and Swift accepted any non-empty string — the
cross-generator divergence a shared declaration exists to prevent, one
level down. All six now require a non-blank string, Ruby's metadata
extractor validates at all, and the self-test asserts each bad shape
against two loaders rather than one.
Rejecting HEAD proves nothing on its own: a generator still holding its
own five-verb list would pass that and ignore spec/generated-verbs.json
entirely, which is how PATCH's removal could go unenforced in four
languages. Each compiled generator now also runs against a declaration
that withholds a verb the spec really uses, and must refuse that verb.
The output-tree assertion tightened with it. Excluding the seeds by name
excused a real Metadata.kt or mod.rs, the very names a refusing generator
might write; every file under the output directory must now be a seed by
path, and the seeds are snapshotted from the filesystem so a
case-insensitive Models/models does not disagree with a literal list.
The additionalOperations cases went through the service generators,
where the emission bound would have rejected the field anyway. The
metadata extractors are the walkers that dropped its nested operations,
so they get the fixture instead.
Case numbers come from the run now. Hand-numbering was wrong twice
within one review round, and a duplicate makes the mutation counts in
the header unreadable.
Second variation of one class in two rounds: the loaders read the shared
declaration differently. First it was type coercion, then blankness —
Kotlin and Rust rejected a space the others took, then Ruby's ASCII strip
took a non-breaking space the Unicode-aware ones rejected.
Harmonising the predicate a third time would have been the wrong move.
Every language defines whitespace differently (Java's
Character.isWhitespace excludes U+00A0, Rust's char::is_whitespace
includes it, Ruby's String#strip is ASCII-only), and case, normalisation
and a leading BOM were all still open. An HTTP method is a token, so the
rule is now a positive character class — [a-z]+ — written the same way in
all six loaders. Blankness stops being a question nobody can answer once.
The self-test drives the shapes that told the loaders apart, against two
loaders each, and the compiled test gained a third scenario so the four
binaries have to agree with the scripted two rather than merely look
equivalent.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Unresolved generator-validation and macOS CI portability issues prevent safe approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
scripts/test-generator-verb-inversion.rb:20
The PR description says this self-test grew from 21 to 28 cases, but the current suite contains 32 checks (as this header now states and the runtime counter reports). Please update the verification claim so the review record accurately describes the coverage added here.
# restore the walks below to the ORIGINAL five-verb list and 13 of these 32 cases
# fail; restore them to the EIGHT verbs OpenAPI 3.1 names and 6 still fail —
Makefile:926
This new order-only edge is not covered by the Gradle serialization self-test. scripts/test-check-gradle-serialization.rb still deletes only the three pre-existing edges (its edges table at lines 75-82), so removing conformance-runner-tests-kotlin: | kt-test-verb-refusal would not be exercised by the negative cases even though this target is now a sixth Kotlin Gradle invocation. Add a deletion case for this edge (and update the case-count prose) so the new serialization invariant cannot regress silently.
# ORDER-ONLY EDGE: head of the kotlin/ chain; rationale at smithy-mapper-test.
conformance-runner-tests-kotlin: | kt-test-verb-refusal
Review threads: 8 resolved on 016fc74 — 7 fixed, 1 declined and routed. Rebased onto main (7ec6e82); make check green end to end, all seven SDKs byte-identical.
The Unicode-blank finding was the second variation of one class in two rounds — six hand-written loaders disagreeing about what the shared declaration means, first by type coercion, then by blankness. Rather than harmonise a third predicate, the rule changed: an entry must match [a-z]+, a positive character class with no dependence on any language's Unicode tables. That closes case, normalisation, BOM and surrogates in the same move, and it is pinned against all six loaders rather than argued. Detail is in the PR body under "why that took a different instrument".
Flagging for whoever merges: one of these is a data-loss fix. Until it lands, a validation refusal in the Kotlin generator erases the committed generated tree — it cleared models/, services/, Metadata.kt and ServiceAccessors.kt before parsing, so the fail-closed behaviour #931 introduced destroys output on the way out. Everything else here is correctness; that one is damage.
Require actionable missing-operationId error in test
scripts/test-generator-verb-inversion.rb:364
The missing-operationId assertion still passes with the old broken Python behavior: without the new guard, parse_operation() raises KeyError: 'operationId', which is non-zero and contains the expected text. Also require GET /{accountId}/widgets.json so the test proves the new actionable refusal names the affected operation instead of accepting a generic exception.
The scalar-path-item case took the count to 48 and the header still said
47; the PR body still said 35 from an earlier round. The mutation totals
are the evidence for the inversion, so a stale census is a stale claim —
the failure this whole change set keeps being caught by.
Re-ran both variants rather than adjusting the numbers by hand: the
original five-verb list fails 15 of 48, the eight verbs OpenAPI 3.1 names
fail 8 of 48. Header and body both say so now.
This case accepts any non-zero exit when no Ruby service body was emitted, so an unrelated early failure can pass without proving that the HEAD operation was identified. Match both the method/path and operation name, consistent with the adjacent refusal case and this file's content-assertion rule.
Require Python HEAD operation match in refusal case
scripts/test-generator-verb-inversion.rb:343
This case accepts any non-zero exit when no Python service body was emitted, so an unrelated early failure can pass without proving that the HEAD operation was identified. Match both the method/path and operation name, consistent with the adjacent refusal case and this file's content-assertion rule.
Both HEAD-beside-a-GET cases checked a non-zero exit and an absent
service body, which an unrelated early failure satisfies too. They name
the method, the path and the operation now, which is the rule this file
already states and which the cases either side of them already followed.
Third time in this change set a test passed for a reason its name did not
claim, after the unseeded output directory and the empty expectations.
Neither was passing ONLY on the loose assertion — both still pass
tightened, and the mutation totals are unchanged at 15 of 48 for the
five-verb list and 8 of 48 for the eight.
Head is 3d34797. Both findings from Copilot's "Previously missed (2)" block on 136e3cb — carried in the review body under a summary reading "Findings: None", with no threads, so nothing in a count could see them. Sixth time this week.
Both HEAD-beside-a-GET cases (Ruby and Python) asserted a non-zero exit and an absent service body. An unrelated early failure satisfies both. They now assert the message names HEAD, the path and HeadWidgets — the rule this file already states in its header, and the one the cases either side of them already followed.
Neither was passing only on the loose assertion. Both still pass tightened, and the mutation totals are unchanged: the five-verb list fails 15 of 48, the eight verbs OpenAPI 3.1 names fail 8 of 48. So nothing was hiding behind them — worth saying, because it was the thing to check.
That is the third case in this PR of a test passing for a reason its name did not claim, after the unseeded output directory and the empty expectations. All three were the same shape: an assertion too weak to separate the fixed code from the broken code.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The standalone Swift refusal target bypasses toolchain gating and fails on hosts without Swift.
Review effort: Balanced Findings: None
Previously missed (2)
In code that hasn't changed since last review
Gate swift-test-verb-refusal for missing Swift
Makefile:570
swift-test-verb-refusal is exposed as a standalone target, but it invokes the Swift runner unconditionally. The HAS_SWIFT guard added later is part of swift-check's recipe, so it does not protect make swift-test-verb-refusal; on hosts without Swift, that command fails instead of following the repository's toolchain-gated check behavior. Gate this target directly, as swift-check-drift does.
Update stale coverage scenario inventory
scripts/test-compiled-generator-refusal:37
The coverage inventory is stale: the harness now runs nine common scenarios, plus a TypeScript-only first-write case and a Kotlin-only late-failure case, but this header still says “FIVE SCENARIOS” and documents only the first three. Update the inventory so the regression gate accurately describes the refusal paths it covers.
…able
The standalone swift-test-verb-refusal invoked the runner unconditionally
while every other Swift target skips, so a direct make on a host without
the toolchain failed rather than skipping.
Gating it in place would not have worked: HAS_SWIFT is assigned further
down the file, so an ifdef beside the other *-test-verb-refusal targets
reads as undefined and skips unconditionally — including where Swift is
installed. Verified that happening before moving the target into the
Swift section, where the variable is in scope, and left a pointer behind
saying why it is not with its siblings. A gate that skips when its input
is present is worse than one that fails.
The harness header said FIVE SCENARIOS while the file ran eleven, and
called them "a table rather than a list that grows by one each review
round" — a sentence its own contents refuted, having grown from five to
eleven by review rounds. All nine common rows and both language-specific
ones are enumerated now, and the claim is replaced by what is true: each
row exists because the committed spec exercises none of these shapes, so
nothing else can see these guards regress.
Head is 9f75856. Both findings from Copilot's "Previously missed (2)" on 3d34797 are fixed — and the first one had a trap in it worth naming.
1. swift-test-verb-refusal gating. The finding is right: the standalone target invoked the runner unconditionally while every other Swift target skips.
Gating it in place would not have worked.HAS_SWIFT is assigned at Makefile:1264, and the *-test-verb-refusal targets sit at ~569 — so an ifdef there reads as undefined and skips unconditionally, including on hosts that do have Swift. I applied the four-line shape at 569 first, ran it, and got SKIP on a machine with a working toolchain. The target is now defined in the Swift section where the variable is in scope, with a pointer left beside its siblings explaining why it is not with them. A gate that skips when its input is present is worse than one that fails.
That is the correction to what you specified — the shape was right, the location was not.
2. The stale scenario header. It said "FIVE SCENARIOS" while the file ran eleven, and described them as "a table rather than a list that grows by one each review round" — a sentence its own contents refuted, since they grew from five to eleven by review rounds. All nine common rows and both language-specific ones are enumerated now, and the refuted claim is replaced by what is actually true: each row exists because the committed spec exercises none of these shapes, so no drift check can see these guards regress.
I did not sweep either file for a third, and correcting the enumeration turned up no dead or duplicated scenario: nine unconditional run_scenario calls, plus TypeScript's first-write case and Kotlin's late-failure case — ten per language, which is what each run reports.
One thing I noticed and did not change, reporting rather than fixing: swift-check inlines the same gated command rather than depending on swift-test-verb-refusal. It is duplication, not a defect — both paths gate identically — and touching it was outside what you asked for.
make check green on the previous head with these changes in the tree; branch is on 7ec6e82 with no merge commits. Trivy (Python SDK) remains red on a dependency advisory and is not a required context.
Reject non-object values before unsupported-method validation
rust/generator/src/model.rs:515
Reject a field whose value is not an object before checking whether its key is an emitted verb. Currently the scalar frobnicate fixture reaches the unsupported-method branch and tells the user to add a runtime helper, even though the value cannot be an operation. This also lets the compiled test pass on the wrong diagnostic because it checks only the field and path. Return the same “not an operation object” refusal as the other walkers, then require that wording in the compiled scenario.
Assert the intended malformed declaration validation message
scripts/test-generator-verb-inversion.rb:270
Match the declaration-validation message rather than the filename. Every command reads bad-verbs.json, so output.include?("verbs") can succeed for an unrelated read or parse error without proving that any reader rejected the malformed array for the intended reason.
Assert the malformed operation ID and field marker
scripts/test-generator-verb-inversion.rb:468
Assert the actual operation-ID diagnostic and field marker. The path /widgets.json itself contains the substring get, so the current assertion can pass even when the error never identifies the malformed get operation.
swift-check repeated the gated command instead of depending on the target
that already carried it, so make swift-check swift-test-verb-refusal ran
the compiled harness twice and the two conditionals could drift apart --
the duplication failure this change set has spent six rounds on, in the
fix for it.
swift-check takes the target as a prerequisite now and the inline block
is gone. Make deduplicates a prerequisite within one invocation; a second
copy of a recipe it cannot.
DELIBERATE ORDERING CHANGE: a prerequisite runs BEFORE the recipe, so the
refusal gate now precedes the Swift build rather than following it. That
is the better order -- the cheap check fails first -- but it is a reorder
and should not read as accidental. Nothing takes swift-check as a
prerequisite, so nothing else pulls the harness earlier than intended;
check-targets lists it as a sibling, which is how swift-check-drift is
wired too.
The comment on swift-test-verb-refusal said it was gated directly because
swift-check's guard did not reach it. That guard no longer exists, so the
comment described a file that had changed under it.
swift-check repeated the gated command instead of depending on the target that already carried it (#discussion_r4052378660). So make swift-check swift-test-verb-refusal ran the compiled harness twice, and two conditionals could drift apart — the duplication failure this PR has spent six rounds on, inside the fix for it. swift-check takes the target as a prerequisite now and the inline block is gone.
Two things stated so they do not read as accidental: a prerequisite runs before the recipe, so the refusal gate now precedes the Swift build rather than following it — better ordering, since the cheap check fails first, but it is a reorder. And nothing takes swift-check as a prerequisite, so nothing pulls the harness earlier than intended; check-targets lists it as a sibling, matching swift-check-drift.
Also corrected a comment that went stale in the same edit: it said the target was gated directly because swift-check's guard did not reach it, and that guard no longer exists.
Declined
All three are true. None is being written here. They carried no threads — they are in the review body only, like the six that preceded them — so there is nothing to link.
scripts/test-generator-verb-inversion.rb:270 — the assertion looks for verbs in the output, which can match the filename bad-verbs.json rather than the refusal.
scripts/test-generator-verb-inversion.rb:468 — the path /widgets.json contains the substring get, so the assertion can pass without the error naming the malformed operation.
rust/generator/src/model.rs:515 — a scalar field reaches the unsupported-verb branch and advises adding a runtime helper for something that cannot be an operation.
Why these are declined rather than queued. They are the third, fourth and fifth variation of one class — an assertion that can pass for the wrong reason — after two earlier rounds of the same thing: refusal cases accepting any non-zero exit, then a case accepting a generic KeyError. The finding rate across rounds is 2, 2, 2, 2, 4 — rising after fixes, not falling.
That is unbounded for a structural reason rather than a sloppy one. Every substring assertion in a test that shells out to a generator has this property: the haystack is prose, the needle is a fragment of it, and any fragment can turn up somewhere it was not meant to. A reviewer can generate one of these against this file indefinitely, so tightening the next three does not reach the end of the list — it produces round seven.
The remedy that closes the class is not another assertion. It is for the harness to assert on a structured refusal — an exit code plus a machine-readable marker the generator emits — instead of substring-matching prose. That is a change to what the six generators emit, so it belongs in its own PR against #936 rather than bolted onto a defect fix. Recorded there.
Trivy (Python SDK) remains red on a dependency advisory and is not a required context. make check green; all seven SDKs byte-identical.
)
* Derive gen-catalog's emission bound from spec/generated-verbs.json
gen-catalog carried its own five-verb allow-list — a known copy of the
same fact six SDK generators already read from spec/generated-verbs.json,
which is exactly the divergence #925 was. Replace the hardcoded map with a
permissive derive from that single declaration (basecamp-sdk#926):
- loadGeneratedVerbs reads the file with encoding/json, keeps the
non-empty/non-whitespace verb strings, and FAILS LOUD on an unreadable
or unparseable file AND on a file that yields no usable verb (verbs
absent, [], or all blank). No fallback to a hardcoded default: a silent
fallback is how a stale superset (e.g. one still naming patch) survives
the file dropping it, so an empty bound stops the build rather than
reading as "no restriction." No downstream len==0 short-circuit turns an
empty bound into allow-everything.
- The read is one isolated function so the #935 switch to a generated Go
constant is a one-function swap, not a rewrite.
- Build takes the bound as a parameter; the exclusion-based path-item slot
discovery and the fail-loud-on-out-of-bound-verb behavior are unchanged,
now bounded by the file rather than the literal.
Post-#932 the file no longer lists patch, so the derived bound differs
from the old literal only by patch (unused) and catalog.json regenerates
byte-identical.
Also fixes the stale header comment (it still named the five hardcoded
verbs) and updates the not_patch note in generated-verbs.json, whose last
sentence described gen-catalog as carrying a superset — now false, since
the bound is identical by construction.
* Correct gen-catalog derive comments and no-verb error
- Cite #935 (policy transcribed once per language), not #926 (the merged
catalog PR), for the 'generators can never disagree' argument; keep #925
as the original failure.
- Fix the loadGeneratedVerbs rationale: no upstream validator exists (#933
closed unmerged and proved a 'prerequisite of every generate target'
unreachable), so fail-loud IS the contract — not a reason to relax the read.
- Drop the nonexistent 'make check-generated-verbs' target from the
no-usable-verb error; point at the verbs array in the file instead.
- Say why defaultGeneratedVerbs is a constant, not a flag (an overridable
bound produced two of #933's findings).
11 version files and 8 lockfiles, written by scripts/bump-version.sh.
MIGRATING.md's '# Unreleased' section is promoted to '# v0.20.0'.
Thirteen commits since v0.19.0 (2026-09-16), the substantial ones being
the #925 chain — #931 finding operations by exclusion so no HTTP verb can
drop one in silence, #932 stopping a refusal from destroying generated
output, and #938 deriving gen-catalog's emission bound from the shared
declaration — plus #926's embedded tool Catalog, #930's Automation split,
#924's event-feed body validation at the wrapper's decode, and #937's
anyio bump for CVE-2026-63374.
Ran the release's own preflight here rather than discovering it at tag
time: promote-migrating --check passes, rs-publish-check packages and
dry-run-uploads 0.20.0 clean, and all four lockfile freshness assertions
hold.
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
github-actionsPull requests that update GitHub ActionskotlinpythonPull requests that update the Python SDKrubyPull requests that update the Ruby SDKrustRust SDKspecChanges to the Smithy spec or OpenAPIswifttypescriptPull requests that update TypeScript code
2 participants
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.
github-actions
Bot
added
github-actions
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Follow-up to #931, which merged about nine minutes after it opened — roughly forty seconds before Copilot's review landed. That review found eight things, all of them real, and four further rounds found more. #925 stays closed; nothing here reopens the hole, and everything here is a place #931's claims were not yet true.
What this PR does not do
One class found here is not fixed here: the six generators each validate
spec/generated-verbs.jsonthemselves, and they disagree about what it means. Five instances in four rounds, each a different language for a different reason — Kotlin'sjsonPrimitive.contentcoercion, Rust'sfilter_map, Ruby's ASCII-onlyString#strip, Swift'sallSatisfybeing vacuously true for the empty string, JavaScript's$matching before a final line terminator.That has been escalated and decided, and the decision is a restructure rather than another predicate: one strict validator, six readers that take the strings verbatim, with the validator wired as a prerequisite of every
*-generatetarget and intomake check. It is a separate PR because this one carries a data-loss fix that should not wait on a mechanism change.So this PR leaves two known holes in the per-loader check it inherits — Swift accepts
"", TypeScript accepts"get\n"— both on malformed declaration content only, both strictly better thanmain's state, and both deleted outright by the follow-up, which removes the per-loader check rather than mending it. Fixing them here would have been the sixth predicate fix in four rounds and would have obscured the decision.The decision history, since it is more useful than a tidy narrative: the first repair required a non-blank string in every loader; the second replaced that with a positive
[a-z]+format rule on the reasoning that defining the allowed set makes whitespace, case, BOMs and normalisation moot. Neither held. The format rule did not narrow the class — it relocated it, from "what counts as blank" to "what does this language's predicate actually match". The population is bounded by the union of six standard libraries' string and regex semantics, not by the rule being expressed, which is why each round found another. That call was made deliberately and was wrong, and it is what the restructure is a response to.PATCH leaves the declaration
spec/generated-verbs.jsonnamed five verbs because all six generators' old lists did. Two of the six runtimes cannot serve one of them:BaseServicedelegatesget post put delete post_raw put_raw. The generator emitshttp_patch(...), which isNoMethodErrorat call time.ServiceEmitter's"PATCH" ->branch emitshttpPut, putting the wrong method on the wire.The Basecamp API models no PATCH operation, so nothing regressed and nothing regenerates differently — but a declaration whose whole job is to name what the SDKs can actually emit was false on its first day. A PATCH operation now refuses generation by name, which is the same treatment HEAD gets and for the same reason. The declaration records the two fixes needed to put it back.
scripts/gen-catalog/main.gostill listspatchin its own bound. That generator is owned elsewhere and untouched here; it only ever accepts a superset of what these generators emit, so a looser bound there cannot admit anything the strict one lets through.A refusal must not destroy the output tree
Kotlin's
Main.ktdeletedmodels/*.kt,services/*.kt,Metadata.ktandServiceAccessors.ktbefore parsing. A refusal therefore erased the committed generated tree — a validation failure turned into data loss, and #931's "no partial output was written" claim was wrong for the one generator whose default output is the committed tree.The first repair was "parse before delete", and both halves of that were wrong. Parsing is only one thing that can refuse — semantic validation is another, and every
!!on a field that might be absent is a third;MetadataEmitter.parsestill ran after the clean. And deleting is only one way to destroy: overwriting is equally destructive, which is how the TypeScript instance survived two passes, because that pipeline never deletes.The invariant, stated correctly: no destructive write — delete or overwrite — before everything that can refuse has run.
Where the class was found, and what was done
Kotlin renders its whole output into a map and clears only afterwards, so no failure anywhere in generation can destroy the tree. Swift had the same shape — it removed
Models/andServices/then emitted — and got the same fix. The original finding was Kotlin-only because Kotlin's default output is the committed tree, not because Swift was safe.TypeScript's pipeline is the second place, and it is fixed the way Kotlin was, minimally:
strip-account-id.tsis step one, its output is a committed artifact, and it now walks every path item through the shared helper before it writes.pathsonly — every downstream step walksspec.paths, so validatingwebhookswould refuse over something no generated artifact is built from — and no emission bound, which belongs togenerate-services.tsand runs before its first write.Redirects were a third instance one layer down: the shell truncates
>targets before the command runs, sorb-generate'smetadata.jsonandtypes.rband the TypeScript pipeline'smetadata.tsandperson-id-sites.tswere emptied before their generator could refuse. All four use a temp file and a move, clearing the fragment on failure.The sweep, since "I looked and found nothing" is not checkable
Main.ktclears the four generated pathsmain.swiftremoveItemsModels/,Services/main.rs:152remove_dir_allinsidewrite()render(&model)?firstgenerate-services.rb:381per-serviceFile.writegenerate_services.py:1275write_textgenerate_metadata.py:64writesargs.outputgenerate-services.ts:1791groupOperationsat :1788strip-account-id.ts, step onegenerate-url-routes→.tmpthenmvoapi-codegen, which owns its own writing. Not verified the same way, and not in this PR's reachHow strongly each row is held, since "verified" is doing a lot of work there
retry.maxfrom one behavior-model entry fails at the far end of generation, and reverting the render-then-clean ordering makes that scenario — and only that scenario — report four destroyed sentinels. I checked it both ways.render-before-write, and Ruby's, Python's and TypeScript's walk-completes-before-first-write. No crafted input currently reaches those generators' emission stages as a failure, so there is nothing for a test to drive. Named rather than implied.What is NOT claimed
The claim is about the six repo-owned generators, not "every generator" — Go's client is
oapi-codegen's and is outside it.This PR does not make generation atomic, and does not claim to. Every row above means a refusal cannot destroy anything. None of them means a crash mid-write leaves a consistent tree — a failure between the first write and the last still leaves a partial tree in every generator, and a later pipeline step can still fail for reasons unrelated to #925 (
patch-flexible-ids.tsrefuses when thePersonschema layout moves).A general wrapper that snapshotted
src/generatedand restored it on any failure was built, verified, and withdrawn: a new general-purpose mechanism introduced as a tail on a defect fix became the thing under review rather than the bug. That work, the sweep above, and the reasoning are filed as #936, which deserves its own PR and its own review.additionalOperationsis named everywhere, not just where a bound catches itOpenAPI 3.2's
additionalOperationsis a map of method to Operation. The bounded walks already refused it, as an unemittable verb. The verb-agnostic ones — the four metadata extractors and the jq route generator, which take no bound on purpose — read it as one operation, found nooperationId, and skipped it, dropping every operation inside without a word. That is the exact failure #925 is about, one level down.Every walk now refuses it by name. The route filter also refuses a path-item field that is not an operation object with a non-empty
operationId, so a scalar no longer errors obliquely through.value.operationIdand an unrecognised object no longer reaches the route table as a method namingnull.PATCH, and the loaders — what changed and what moved
spec/generated-verbs.jsongained a[a-z]+format rule in place of #931's per-loader "non-empty string", which is an improvement onmainand is where the loader work stops in this PR. The rest of that class is the follow-up described above.The compiled generators get a test that can see a regression
The sharpest finding.
scripts/test-generator-verb-inversion.rbdrives Ruby, Python and jq. TypeScript, Kotlin, Swift and Rust carry four more independent copies of the walk, and nothing in the repository could see them regress: their drift checks regenerate from the committedopenapi.json, which declares only verbs the SDKs emit, so all four could revert to silently skipping an unknown verb with every drift check green.scripts/test-compiled-generator-refusal <language>runs the real generator binary through three scenarios and asserts four things in each: a non-zero exit, a message naming the offending operation, that no generated file was written, and that the output tree it was pointed at is untouched.An unemitted verb.
openapi.jsonwith one operation's verb moved fromgettohead.A declaration-sourced bound. The unmodified spec, against a declaration that withholds a verb the spec really uses. Without this, a generator still holding a private five-verb list would pass scenario 1 by rejecting HEAD while ignoring
spec/generated-verbs.jsonentirely — which is exactly how PATCH's removal could have gone unenforced in four languages.A malformed declaration. The
[a-z]+rule, asserted against the compiled loaders with the non-breaking space that told them apart.An
additionalOperationsmap, and 5. a non-stringoperationId— the two guards no committed spec exercises.A malformed retry entry, and this is the one with the data-loss teeth. Every other scenario fails during the spec parse, which any "parse before delete" ordering already survives, so none of them can tell a generator that renders before destroying from one that merely hoisted its first failure. This one fails at the far end of generation. Reverting the render-then-clean ordering makes this scenario, and only this scenario, report four destroyed sentinels.
It runs for Kotlin alone, and skips by name elsewhere: Swift and Rust default a missing retry field and generate successfully, so the fixture is not fatal for them and a "must fail" assertion would fail for the wrong reason. That gap is real — Swift's render-before-delete ordering is verified by reading the code, not by this test — and it is stated rather than papered over.
The fourth assertion is the one with teeth, and it only works because the output directory is seeded first. An empty directory cannot show data loss — clearing nothing leaves nothing — so the first version of this test passed against the unfixed Kotlin generator. Seeded, it fails on it with four named sentinels, which is how the ordering bug above was confirmed rather than assumed. The seeds are compared by content and the "nothing written" check is an exact-path set difference against them, so an overwritten
Metadata.ktormod.rsfails alongside a deleted one; the seed list is snapshotted by reading the tree back, because macOS foldsModels/andmodels/into one directory and the Linux runners do not.Wired into
ts-check,kt-check,swift-checkandrs-check, and into each language's CI job, where the toolchain is present by construction so nothing can skip.One structural note. Kotlin runs the installed distribution rather than the Gradle run task, so every Gradle invocation stays in the Makefile recipe.
scripts/check-gradle-serializationresolves one level of script delegation and attributes a delegated Gradle call to every target that calls the script; with the wrapper invoked from the shared script it marked the TypeScript, Swift and Rust targets as Kotlin-project builds and demanded order-only edges describing no real collision.kt-test-verb-refusaljoins the kotlin/ chain at its head, becausescripts/test-check-gradle-serialization.rbchains its probes afterconformance-kotlin— the tail is load-bearing for that self-test.Verification
make checkgreen end to end, including every drift check: all seven SDKs still regenerate byte-identical.scripts/test-generator-verb-inversion.rbis up from 21 cases to 48, covering PATCH's refusal,additionalOperationsthrough the unbounded metadata and route walks (the bounded service walks would have rejected it regardless, so the first version of those cases pinned nothing), the six hostile declaration shapes, and the two route-filter shapes. Cases are numbered as they run — hand-numbering went wrong twice inside one review round, and a duplicate makes the mutation counts unreadable.Refreshed mutation numbers, which are the argument for the inversion rather than a longer list:
The eight-verb list repairs the verbs it was extended with and nothing else: 3.2's
queryandadditionalOperations, and every case where a field that is neither a known non-operation field nor a readable operation is stepped over in silence, still fail.All four compiled generators pass all three scenarios: refused by name, exit 1, nothing written, nothing destroyed.
Also here
operationIdis refused, not skipped, in all six walkers. Generators iterate a five-verb list, so an operation on any other HTTP method is silently dropped from every SDK #925's own outcome — an operation silently lost — was surviving with a different trigger. Swiftcontinued past an operation with nooperationIdand exited zero; the four metadata and person-id extractors skipped it too; the Ruby, Python, TypeScript and Kotlin service generators failed obliquely on a nil, aKeyErroror an NPE. Rust was the only one that already refused by name. All six refuse in the shared walk now, naming method and path, rejecting an empty or non-string id alongside an absent one.--options-orderfile erased the tree the same way. A failure between the first write and the last still leaves a partial tree; that is a different class from a refusal, and closing it means rendering to a temporary tree and swapping.generate-path-mapping.tsgoes through the shared walker. It had its own walk that skippedparametersby name and stepped silently past an operation with nooperationId, so an unrecognised field never reachedPATH_TO_OPERATION.scripts/test-check-gradle-serialization.rb, verified by deleting the edge and watching the self-test fail.Known red, unrelated to this diff
Trivy (Python SDK)fails on a dependency advisory. This PR changes no Python dependency and no lockfile, and it is not a required status context, so it does not block the merge.One process note, since it is the reason this is a second PR
#931 merged at 19:45:02Z, opened 19:36Z. Copilot's review arrived at roughly 19:45:40Z and Codex's shortly after. No approving review existed at the merge, and there were no threads to resolve because no review had landed — which is the failure mode AGENTS.md describes for #162, reproduced exactly. Every finding in that review is above. Nothing was lost, but the fixes reached
mainin a follow-up rather than in the PR that introduced them.