Derive gen-catalog's emission bound from spec/generated-verbs.json - #938
Conversation
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.
|
Reviewed as the SDK side of this. The code and the tests are right — this is the contract we agreed, implemented exactly: permissive unmarshal, non-empty/non-whitespace filter, fail loud on unreadable and on no-usable-verb, no fallback to a hardcoded default, and no One design choice worth naming so nobody undoes it later: Three defects, all in prose, one of them user-facing. 1. The comment claims an upstream validator that does not exist.
None of that holds. #933 was closed unmerged — The narrowness is still correct, but its justification here is backwards. Nothing guarantees the file is well-formed before this function sees it — that is exactly why fail-loud is the contract, rather than a reason the read can be relaxed. As written, the next reader concludes there is upstream protection and may loosen this. Suggest replacing with something like: "This adds no BOM/stream/shape opinions of its own — not because something upstream validates the file (nothing does; see #933), but because a seventh JSON opinion is what #933 showed cannot define validity for the other six. It asserts one fact about this process's own state: I obtained a usable set to bound on, or I stop." 2. The error message sends the user to a command that doesn't exist. The no-usable-verb error ends with 3. The Worth stating in the PR body if it is not there already: the derived bound differs from the old hardcoded set only by |
- 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).
|
Addressed the review in
|
|
Re-checked at
Two things left, both procedural rather than about the code. This is still a draft. Draft status does not gate merging, it gates reviewing — it delays the Codex and Copilot cycle, which is the thing that takes wall-clock here. Worth marking ready for review so the bots start; There is no companion PR to wait for. The |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The PR description and implementation conflict on whether an upstream generated-verbs validator exists.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Derives the catalog generator’s HTTP verb bound from the shared declaration instead of a hardcoded list.
Changes:
- Adds fail-loud loading of generated verbs.
- Passes the derived bound into catalog construction.
- Adds loader tests and updates documentation.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
spec/generated-verbs.json |
Documents catalog derivation consequences. |
scripts/gen-catalog/main.go |
Loads and enforces the shared verb bound. |
scripts/gen-catalog/main_test.go |
Tests loading and fail-loud behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
What
Replace
gen-catalog's hardcoded verb allow-list with a derive fromspec/generated-verbs.json— the single shared declaration the six per-language SDK generators already read. This is the catalog-embed lane of basecamp-sdk#926.gen-catalogwas carrying its own copy of the emission bound (the set of HTTP methods the SDK generators emit). A copy of a fact that already has one authoritative source is exactly the divergence #925 was: six lists of one fact.The derive (contract, locked with the tag-pipeline owner)
loadGeneratedVerbs— one small isolated function:spec/generated-verbs.jsonpermissively withencoding/json(unknown keys ignored), takes theverbsstrings, keeps only the non-empty, non-whitespace ones.verbsabsent,[], or nothing but blanks).patch) survives the file dropping the verb — the dangerous direction — so an empty/absent bound stops the build, it is never read as "no restriction."make -jscheduling it concurrently with the checks it was meant to precede). A seventh JSON opinion cannot define validity for the other six. This function only asserts a fact about its own state: I obtained a usable set to bound on, or I stop.len(bound)==0short-circuit downstream that could turn an empty bound into allow-everything. An empty map (never reachable viarun, which errors first) would refuse every verb — the safe direction.Kept in one function on purpose: basecamp-sdk#935 may replace the JSON read with a generated Go constant. That should be a one-function swap, not a rewrite (short comment in the code notes this).
Everything else is unchanged: the exclusion-based path-item slot discovery (
isStructuralPathKey—$ref/summary/description/servers/parameters/x-), and the fail-loud-on-a-verb-outside-the-bound behavior — now the bound comes from the file, not a literal. The old five-verb map and the header comment that restated it are gone.Byte-identical catalog
#932 removed
patchfromgenerated-verbs.json, so the derived bound differs from the old hardcoded set only bypatch(unused — the API models no PATCH operation). No operation in the current spec uses a verb outside the generated set, socatalog.jsonregenerates byte-identical. Confirmed viamake catalog-check. If a future rebase ever makesmake catalog-checknon-green, that is a finding to investigate (an operation reaching a verb the runtimes can't serve), not a rebase artifact to regenerate away.Coupled doc edits carried in this same PR (one fact, one source)
scripts/gen-catalog/lives insidebasecamp-sdk, so the code derive and the prose that reasons about it are the same repo — one PR avoids a staleness window.spec/generated-verbs.json— the last sentence of thenot_patchnote describedgen-catalogas owning its own bound and "only ever accept[ing] a superset." That goes false the moment this derive lands (the bound is now identical by construction, not a hand-maintained superset). Replaced with the owner-provided text naming the two consequences: adding a verb here widensgen-catalogautomatically, and the criterion it inherits is the file's — whether all six runtimes can serve the verb.scripts/gen-catalog/main.goheader — already stale onmain(it said the generators "iterate exactly get/put/post/delete/patch", but A refusal must not destroy generated output: fixed in Kotlin and TypeScript, swept across the rest #932 removedpatch). Rewritten to name the file as the source and drop the hardcoded list.The bound's entry criterion (coupling worth knowing)
patchis absent from the array not because the API doesn't model it, but because 2 of 6 runtimes get it wrong: Ruby'sBaseServicehas nohttp_patch(NoMethodError at call time) and Kotlin'sServiceEmitterrenders PATCH viahttpPut(wrong method on the wire). So the array's entry criterion is "all six runtimes can actually serve this verb," not "the spec models it." Consequence now thatgen-catalogderives: adding a verb togenerated-verbs.jsonautomatically widensgen-catalog's accepted surface — correct for a catalog, but the next reader should know this bound now inherits a runtime-capability criterion, not a spec-modeling one.Tests
New
loadGeneratedVerbstests: unreadable file → error; unparseable file → error; absent-verbs/[]/all-blank → error (fail-loud, no usable bound); a valid file → correct bound; blanks dropped while a usable verb remains → that set; and the committedgenerated-verbs.jsonloads to a non-empty bound that excludespatch(post-#932). ExistingBuildtests pass the bound explicitly via atestVerbs()helper; the #925 non-generated-verb guard still refuses head/options/trace.Verification
make catalog-check→ byte-identical (catalog.jsonup to date, 265 ops / 32 tags)go build ./...(go module) → OKgo test ./pkg/basecamp/catalog/... ./scripts/gen-catalog/...→ passworking-directory: go) 0 issues. Note:scripts/gen-catalogis a separate module CI does not lint; running v2.13.2 against it with the go config surfaces only a pre-existing gosec G306 on the untouchedos.WriteFile(..., 0o644)inrun()(verbatim onmain) — deliberately not touched here (out of scope, would change generated-file perms).Review request
@coworker-70 — please review (tag-pipeline owner): (1) confirm which generator this read matches semantically, (2) confirm the
not_patchnote replacement text and the entry-criterion framing, and (3) coordinate the paired landing — thenot_patchedit and this derive must land together so neither side is briefly lying.Draft — do not merge.
Summary by cubic
Derives
gen-catalog's emission bound fromspec/generated-verbs.jsoninstead of a hardcoded five-verb map, so the catalog and the six SDK generators share one source of truth. The generated catalog stays byte-identical becausepatchwas already removed from the file and no operation uses it.Behavior
loadGeneratedVerbsreads the file permissively, keeps non-empty verbs, and fails the build if the file is unreadable, unparseable, or yields no usable verb.Buildnow takes the bound as a parameter; path-item slot discovery and the refusal of out-of-bound verbs are unchanged.Side effects
spec/generated-verbs.jsonnow automatically widensgen-catalog's accepted surface; the entry criterion is whether all six runtimes can serve the verb, not whether the API models it.scripts/gen-catalog/main.goand thenot_patchnote to match.Written for commit ce3ae13. Summary will update on new commits.