One validator, six readers: stop the generators disagreeing about the shared declaration by jeremy · Pull Request #933 · basecamp/basecamp-sdk

jeremy · 2026-09-18T22:46:02Z

This PR is open as evidence, not as a proposal I am asking to merge. Review found that its own premise does not hold — see "Why this may not be buildable" below — and the decision between this shape and removing the runtime input is with the operator.

Stacked on #932 — merge that first. It carries a data-loss fix that should not wait on this, and this PR's diff is only the commit on top.

The class

The six generators each read spec/generated-verbs.json, and each of them also validated it. They disagreed about what it meant five times in four review rounds, every one in a different language for a different reason, and every one on invalid input:

language	what diverged
Kotlin	`JsonPrimitive.content` renders a number as text, so `["get", 1]` read as `get`/`1`
Rust	`filter_map` dropped the non-string and accepted the rest
Ruby	`String#strip` is ASCII-only, so U+00A0 and U+2003 survived it where Python's `str.strip` removed them
Swift	`String.allSatisfy` is vacuously true for `""`, so the empty string passed a character-class check
TypeScript	JavaScript's `$` matches before a final line terminator, so `"get\n"` passed the same check

Two repairs were tried and neither held.

Require a non-blank string in every loader. That moved the disagreement from "is this a string" to "what counts as blank".
Replace the predicate with a positive [a-z]+ format rule, on the reasoning that defining the allowed set makes whitespace, case, BOMs and normalisation moot. That moved it again, to "what does this language's predicate actually match".

The second was a deliberate call and it was wrong, and the way it was wrong is the useful part: it did not narrow the population, it relocated it. The bound was never the rule being expressed. It is the union of six standard libraries' string and regex semantics, which is why every round found another instance — and Kotlin's was in the parse rather than in a check, so "validate more carefully" could not have reached it either.

The shape

One implementation rejects a malformed declaration. The six loaders take the strings verbatim.

scripts/check-generated-verbs.rb is that implementation. It is strict enough that valid input has no interpretive room left: a non-empty array of distinct lowercase ASCII tokens, and no unexpected top-level keys (so verb for verbs is questioned rather than silently ignored while the real key is absent). Every JSON parser in the six languages agrees about a file like that — which is the whole point, because all five divergences happened on input this gate now makes unreachable.

The loaders perform no validation, not less of it. Each is a parse and a field read, with a failure message pointing at the gate. Anything left behind would be another chance to disagree.

No order rule, deliberately. The array is ordered and that order is emission order in every generator, so it is content, not formatting: sorting it would reorder every generated method in seven SDKs, and there is nothing to canonicalise because the file is declaring the order rather than spelling a set in an arbitrary sequence. Determinism, which a canonical form usually buys, a JSON array already gives.

Why a prerequisite and not a CI job

This is the part that decides whether the mechanism works, and the Makefile settles it: there is no single generation entry point. ts-generate, ts-generate-services, rb-generate, rb-generate-services, py-generate, py-generate-services, kt-generate-services, swift-generate and rs-generate-services are each directly invokable, and generate merely aggregates them. A CI-only validator is bypassed by anyone running make rb-generate locally — which is exactly where a hand-edit to the declaration gets made.

So it is an order-only prerequisite of all nine, and also a member of check-targets, so a hand-edit that never regenerates anything is caught too. Order-only because it is a gate rather than a dependency: nothing in those recipes consumes its output.

The self-test is the argument

scripts/test-check-generated-verbs.rb, 18 cases. The negative ones are not invented — each is a shape that actually told the six loaders apart over those four rounds: 1, " ", " ", " ", "", "get\n", "GET", and a duplicate. Every one of them now dies once, in one language, before any generator runs.

Its positive control includes a non-alphabetical order, which pins the no-order-rule decision rather than leaving it to a comment.

What moved rather than disappeared

The malformed-declaration cases left scripts/test-generator-verb-inversion.rb and the compiled harness. Asserting them against a loader now would assert the absence of a check, which is the point rather than a regression. The half that remains is the one that still matters: each loader takes its bound from the declaration rather than from a private literal, pinned by the scenario that withholds a verb the spec really uses.
scripts/gen-catalog/main.go reads no declaration — it keeps its own deliberately looser superset bound, is owned elsewhere, and is untouched here. This gate only constrains the file, so a looser reader of a file this strict cannot be made wrong by it.

Verification

make check green end to end; all seven SDKs regenerate byte-identical. The validator passes on the committed declaration and fails on all 13 negative shapes with the offending index named.

Why this may not be buildable, found in reviewing it

The design is one validator, six readers: one implementation defines validity strictly enough that the six loaders can parse without interpreting. Review found two things that falsify the premise rather than the implementation.

jq accepts a stream of top-level JSON values, so the gate never establishes the file is one document. Two consecutive objects each carrying verbs pass here while the six loaders reject or read them differently.
jq accepts a UTF-8 BOM that Python's json.loads and TypeScript's JSON.parse reject.

The validator is therefore a seventh implementation with its own JSON semantics. It cannot define validity, because "valid JSON" is not one thing across seven parsers — it can only assert its own reading and hope the other six agree. A slurp check and a BOM check would be predicates six and seven of exactly the class this design was built to end, after Kotlin's JsonPrimitive coercion, Rust's filter_map, Ruby's ASCII strip, Swift's allSatisfy and JavaScript's $.

Alongside that, the "the validator actually runs" requirement has now needed eight entry points: the root *-generate targets, swift/Makefile, rust/Makefile, the release workflows, the standalone *-check-drift targets, and ordering within check-targets itself. Each is real and each is closable. The count is the argument.

The alternative is to remove the runtime input: bake the verb set into each generator at build time from one definition, so there is nothing to load, no environment override, no file to hand-edit and no gate to run. Every bypass above ceases to exist rather than being closed. The honest complication is that only three of six languages have a build step to bake at — Ruby, Python and TypeScript would need a generated source file plus a drift check, which the repo already has the pattern for but is a different design.

And a third option worth weighing, which I would not have raised at the start. The verb set is four strings that have not changed in this project's life, and #925's exposure was latent — no operation in the spec uses a verb outside the list, which is why the hole survived unnoticed. The defect #925 actually named was discovery by verb list, and that is fixed in #931 and stays fixed however this is decided. What remains is an emission bound, and six copies of an emission bound cannot silently drop an operation: each refuses loudly by name, and check-operation-assignment-parity catches any cross-SDK divergence. It is worth asking whether the shared declaration now costs more than the duplication it removed.

The stale-filename findings in this PR are fixed (11 references left behind by renaming the gate from Ruby to bash+jq). The four above are answered and left open on purpose.

The six generators each read spec/generated-verbs.json and each validated it, and they disagreed about what it meant five times in four review rounds — Kotlin's JsonPrimitive rendering a number as text, Rust's filter_map dropping a non-string, Ruby's ASCII String#strip keeping U+00A0 and U+2003, Swift's allSatisfy being vacuously true for the empty string, JavaScript's $ matching before a final line terminator. Every one of those on invalid input, and each in a different language for a different reason. Two repairs were tried. Requiring a non-blank string moved the disagreement from "is this a string" to "what counts as blank". A positive [a-z]+ format rule moved it again, to "what does this language's predicate actually match". The population was never bounded by the rule being expressed; it is bounded by the union of six standard libraries' string and regex semantics. So the loaders stopped validating. scripts/check-generated-verbs.rb is the only thing that rejects a malformed declaration, and it is strict enough that valid input has no interpretive room left: a non-empty array of distinct lowercase ASCII tokens, no unexpected top-level keys. Every JSON parser in the six languages agrees about a file like that. It is an order-only prerequisite of every *-generate target rather than a CI job. There is no single generation entry point — ts-generate, rb-generate, py-generate, swift-generate, kt-generate-services and rs-generate-services are each directly invokable and `generate` only aggregates them — so a CI-only gate is bypassed by the `make rb-generate` a hand-edit is made next to. It is in check-targets as well, so a hand-edit that never regenerates is caught too. No order rule: the array's order is emission order, so it is content rather than formatting, and sorting it would reorder every generated method in seven SDKs.

chatgpt-codex-connector · 2026-09-18T22:46:11Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-19T04:41:13.424020Z	`12c1a0f`	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

github-actions · 2026-09-18T22:46:14Z

Sensitive Change Detection (shadow mode)

This PR modifies control-plane files:

.github/workflows/smithy-verify.yml
.github/workflows/test.yml

Shadow mode — this check is informational only. When activated, changes to these paths will require approval from a maintainer.

Copilot

Copilot review overview

🟡 Changes recommended

Loader path overrides and language-local generation targets can bypass the sole validator.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2

Open (2)

Enforce generated verbs validation across all generation entry points · New
Validate the same generated verbs file consumed by loaders · New

What changed in this PR

Centralizes generated-verbs.json validation while simplifying six generator loaders.

Changes:

Adds a strict shared validator with comprehensive self-tests.
Makes generation targets depend on the validator.
Removes duplicated loader validation and obsolete tests.

[!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.

File	Description
`Makefile`	Wires validation into generation and checks.
`.github/workflows/test.yml`	Runs validator checks in CI.
`scripts/check-generated-verbs.rb`	Implements shared validation.
`scripts/test-check-generated-verbs.rb`	Tests valid and malformed declarations.
`scripts/test-generator-verb-inversion.rb`	Removes loader-validation cases.
`scripts/test-compiled-generator-refusal`	Removes malformed-declaration scenario.
`typescript/scripts/path-items.ts`	Simplifies TypeScript loading.
`python/scripts/gen_common.py`	Simplifies Python loading.
`ruby/scripts/generate-services.rb`	Simplifies Ruby service loading.
`ruby/scripts/generate-metadata.rb`	Simplifies Ruby metadata loading.
`kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/PathItems.kt`	Simplifies Kotlin loading.
`swift/Sources/BasecampGenerator/PathItems.swift`	Simplifies Swift loading.
`rust/generator/src/main.rs`	Simplifies Rust loading.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

jeremy · 2026-09-18T23:38:31Z

+# It is an ORDER-ONLY PREREQUISITE of every *-generate target rather than a CI
+# job, because there is no single generation entry point: ts-generate,
+# rb-generate, py-generate, swift-generate, kt-generate-services and
+# rs-generate-services are each directly invokable and `generate` only aggregates
+# them, so a CI-only gate is bypassed by the `make rb-generate` that a hand-edit


Partly fixed in 3a5ecb0 — swift/Makefile and rust/Makefile call the gate directly now, and the release path goes through smithy-verify.yml.

Leaving this open with #discussion_r4051375698, because between them they are the finding: you named entry points five and six, that thread names the seventh (make py-check-drift, make rs-check-drift and their siblings run generators standalone), and there is no reason to think the enumeration terminates. This repo is built to have many paths, so "a prerequisite on every path" is fighting its shape — the same curve the loader predicates traced, one layer up.

Escalated with the question of whether the runtime input should exist at all rather than answered with an eighth prerequisite.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bb3f63c565

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Four bypasses of the one requirement the design rests on — that the validator runs, on the file actually used. The sharpest invalidated the gate outright: the checker read GENERATED_VERBS_FILE while the loaders read BASECAMP_GENERATED_VERBS, so `BASECAMP_GENERATED_VERBS=/tmp/bad.json make rb-generate` validated the committed file and then fed the generator the other one. A gate that validates a different artifact than the one consumed is not a gate. It honours the loaders' variable first now, and the self-test asserts it. swift/Makefile and rust/Makefile expose their own generate targets, which the root prerequisites never covered; both call the gate directly. The tag-triggered release workflows call neither test.yml nor the root targets, so the gate moved into smithy-verify.yml, the reusable workflow every release does call — which is the path where an unvalidated declaration would reach a published package. And it is bash + jq rather than Ruby. It runs in front of every generator, so it may only depend on what every contributor has: CONTRIBUTING lists bash and jq as shared tooling and Ruby as the Ruby SDK's own requirement, so a Ruby gate made `make ts-generate-services` fail with `ruby: command not found` for a TypeScript-only contributor.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a5ecb0fa8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-09-18T23:20:15Z

 	 echo "==> All checks passed"

-check-targets: check-gradle-serialization test-check-gradle-serialization test-promote-migrating lint-actions sync-spec-version-check smithy-check smithy-mapper-test behavior-model-check provenance-check sync-api-version-check doc-constants-check url-routes-check catalog-check bc3-route-parity test-bc3-route-parity go-check-drift go-check-wrapper-drift go-check-generated-drift check-grouped-client-coverage test-check-grouped-client-coverage auth-routable-check check-service-inventory-parity test-check-service-inventory-parity check-operation-assignment-parity test-check-operation-assignment-parity kt-check-drift swift-check-drift rs-check-drift go-check ts-check rb-check kt-check swift-check py-check rs-check check-required-tags test-check-required-tags check-bucket-flat-parity validate-api-gaps check-deprecation-parity check-fixture-coverage kt-check-optional-arrays-and-scalars go-check-optional-pointers test-enhance-request-reachability check-idempotency-parity check-write-semantics-parity check-retry-metadata-parity check-runner-test-reachability conformance check-fixture-execution check-replay-decoder-parity check-readme-env-vars test-check-readme-env-vars check-orphaned-doc-comments test-check-orphaned-doc-comments lint-npm-lockfile-writes test-lint-npm-lockfile-writes test-assert-sdk-built test-assert-lockfiles-unchanged check-projected-examples test-generator-verb-inversion
+check-targets: check-gradle-serialization test-check-gradle-serialization test-promote-migrating lint-actions sync-spec-version-check smithy-check smithy-mapper-test behavior-model-check provenance-check sync-api-version-check doc-constants-check url-routes-check catalog-check bc3-route-parity test-bc3-route-parity go-check-drift go-check-wrapper-drift go-check-generated-drift check-grouped-client-coverage test-check-grouped-client-coverage auth-routable-check check-service-inventory-parity test-check-service-inventory-parity check-operation-assignment-parity test-check-operation-assignment-parity kt-check-drift swift-check-drift rs-check-drift go-check ts-check rb-check kt-check swift-check py-check rs-check check-required-tags test-check-required-tags check-bucket-flat-parity validate-api-gaps check-deprecation-parity check-fixture-coverage kt-check-optional-arrays-and-scalars go-check-optional-pointers test-enhance-request-reachability check-idempotency-parity check-write-semantics-parity check-retry-metadata-parity check-runner-test-reachability conformance check-fixture-execution check-replay-decoder-parity check-readme-env-vars test-check-readme-env-vars check-orphaned-doc-comments test-check-orphaned-doc-comments lint-npm-lockfile-writes test-lint-npm-lockfile-writes test-assert-sdk-built test-assert-lockfiles-unchanged check-projected-examples test-generator-verb-inversion check-generated-verbs test-check-generated-verbs


Validate declarations before standalone drift checks

Adding the validator only to check-targets leaves directly invoked drift targets such as make py-check-drift and make rs-check-drift unguarded, even though those are prescribed as standalone pre-merge commands. With {"verbs":["get","post","put","delete",1]}, Python carries the number harmlessly and Rust drops it, so both regenerate the same output and report no drift despite the declaration being invalid; make each drift target that executes a generator depend on check-generated-verbs rather than relying on the aggregate make check.

AGENTS.md reference: AGENTS.md:L337-L341

Useful? React with 👍 / 👎.

Correct, and not fixed — deliberately, because this is the seventh entry point found in two rounds and the count is now the finding.

The sequence: root *-generate targets → swift/Makefile and rust/Makefile generate → the release workflows → and now the standalone *-check-drift targets. Each one is real, each is closed by another prerequisite, and there is no reason to think this enumeration terminates: this repository is built to have many paths — six languages, six Makefiles, six CI jobs, seven release workflows — so "put a gate on every path" is fighting its shape. It is the same curve the loader predicates traced, one layer up: each round closed the instance and the class kept producing.

So this is escalated rather than patched, with the question being whether the runtime input should exist at all. If the verb set were baked into each generator from one definition — nothing loaded at runtime, no environment override, no file to hand-edit, no gate to run — every bypass in this thread and the four above ceases to exist rather than being closed one at a time. That has costs I have written up for the operator, including that three of the six languages have no build step to bake anything at.

Leaving this open, because it is the live one and the decision is not mine to make.

The precedence was backwards: GENERATED_VERBS_FILE, which exists only so the self-test can drive this script, took priority over the variable the six loaders actually honour. A valid GENERATED_VERBS_FILE therefore masked a malformed BASECAMP_GENERATED_VERBS and the gate passed immediately before the generator loaded the bad one — the invariant defeated by the gate's own plumbing, which is the same failure the variable mismatch was, one line further in. The self-test asserts the precedence directly rather than only asserting that each variable works on its own.

jeremy · 2026-09-18T23:38:32Z

Head is 4dbec41. Five of seven threads resolved; two left open on purpose, and they are the ones that matter.

Fixed

The gate validated a different file than the loaders read — GENERATED_VERBS_FILE vs BASECAMP_GENERATED_VERBS. This invalidated the design outright, and the first fix had the precedence backwards, so the loaders' variable now wins and a self-test case asserts it with both set.
swift/Makefile and rust/Makefile expose their own generate targets; both call the gate.
The release path — the tag-triggered workflows call neither test.yml nor the root targets, so the gate moved into smithy-verify.yml, which every release does call.
The Ruby dependency — the gate is bash + jq now. CONTRIBUTING.md lists bash and jq as shared tooling and Ruby as the Ruby SDK's own requirement, so a Ruby prerequisite would have broken make ts-generate-services for a TypeScript-only contributor. "Ruby is already in make check" would not have covered it: make check is not what someone regenerating one SDK runs.

Open, and escalated rather than patched

Seven entry points in two rounds: the root *-generate targets, the two sub-Makefiles, the release workflows, and now the standalone *-check-drift targets. Each is real and each is closable with another prerequisite — and that is the problem. This repository is built to have many paths, so "a gate on every path" is fighting its shape, and it is the same curve the loader predicates traced one layer up: every round closed the instance while the class kept producing.

The question I have put to the operator is whether the runtime input should exist at all. If the verb set were baked into each generator from one definition, every bypass in these threads ceases to exist rather than being closed. The honest complication is that only three of the six languages have a build step to bake anything at — Ruby, Python and TypeScript would need a generated source file and a drift check, which is a real gate but a different design. That trade is written up for the operator; it is not mine to decide, and I have stopped adding prerequisites until it comes back.

Copilot

Copilot review overview

🟡 Changes recommended

The central gate accepts multi-document JSON and is not consistently ordered against or aligned with every generator invocation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3 · 1 · 10

Open (14)

Drift targets can bypass or race the validator · New
Inconsistent verb-file override across validators and generators · New
Enforce generated verbs validation across all generation entry points
Validator accepts multiple top-level JSON values · New
Validator path incorrectly includes .rb extension · New
Docstring references nonexistent .rb validator · New
Validator path incorrectly includes .rb extension · New
Validator path incorrectly includes .rb extension · New
Validator path incorrectly includes .rb extension · New
Self-test references nonexistent .rb validator · New
Validator path incorrectly includes .rb extension · New
Validator path incorrectly includes .rb extension · New
Validator path incorrectly includes .rb extension · New
Validator path incorrectly includes .rb extension · New

Resolved since last review (1)

Validate the same generated verbs file consumed by loaders

jeremy · 2026-09-19T04:37:12Z

 	 echo "==> All checks passed"

-check-targets: check-gradle-serialization test-check-gradle-serialization test-promote-migrating lint-actions sync-spec-version-check smithy-check smithy-mapper-test behavior-model-check provenance-check sync-api-version-check doc-constants-check url-routes-check catalog-check bc3-route-parity test-bc3-route-parity go-check-drift go-check-wrapper-drift go-check-generated-drift check-grouped-client-coverage test-check-grouped-client-coverage auth-routable-check check-service-inventory-parity test-check-service-inventory-parity check-operation-assignment-parity test-check-operation-assignment-parity kt-check-drift swift-check-drift rs-check-drift go-check ts-check rb-check kt-check swift-check py-check rs-check check-required-tags test-check-required-tags check-bucket-flat-parity validate-api-gaps check-deprecation-parity check-fixture-coverage kt-check-optional-arrays-and-scalars go-check-optional-pointers test-enhance-request-reachability check-idempotency-parity check-write-semantics-parity check-retry-metadata-parity check-runner-test-reachability conformance check-fixture-execution check-replay-decoder-parity check-readme-env-vars test-check-readme-env-vars check-orphaned-doc-comments test-check-orphaned-doc-comments lint-npm-lockfile-writes test-lint-npm-lockfile-writes test-assert-sdk-built test-assert-lockfiles-unchanged check-projected-examples test-generator-verb-inversion
+check-targets: check-gradle-serialization test-check-gradle-serialization test-promote-migrating lint-actions sync-spec-version-check smithy-check smithy-mapper-test behavior-model-check provenance-check sync-api-version-check doc-constants-check url-routes-check catalog-check bc3-route-parity test-bc3-route-parity go-check-drift go-check-wrapper-drift go-check-generated-drift check-grouped-client-coverage test-check-grouped-client-coverage auth-routable-check check-service-inventory-parity test-check-service-inventory-parity check-operation-assignment-parity test-check-operation-assignment-parity kt-check-drift swift-check-drift rs-check-drift go-check ts-check rb-check kt-check swift-check py-check rs-check check-required-tags test-check-required-tags check-bucket-flat-parity validate-api-gaps check-deprecation-parity check-fixture-coverage kt-check-optional-arrays-and-scalars go-check-optional-pointers test-enhance-request-reachability check-idempotency-parity check-write-semantics-parity check-retry-metadata-parity check-runner-test-reachability conformance check-fixture-execution check-replay-decoder-parity check-readme-env-vars test-check-readme-env-vars check-orphaned-doc-comments test-check-orphaned-doc-comments lint-npm-lockfile-writes test-lint-npm-lockfile-writes test-assert-sdk-built test-assert-lockfiles-unchanged check-projected-examples test-generator-verb-inversion check-generated-verbs test-check-generated-verbs


Correct, and it is the eighth entry point.

check-targets lists the validator as a sibling, so make -j can schedule it concurrently with the drift checks, and make ts-check-drift or make rs-check-drift directly bypasses it. The sequence so far: root *-generate targets, swift/Makefile, rust/Makefile, the release workflows, the standalone drift targets, and now ordering within check-targets itself.

I am not adding a ninth prerequisite. This repository is built to have many paths — six languages, six Makefiles, six CI jobs, seven release workflows — so a gate on every path is fighting its shape, and the count is the argument rather than any single instance. Together with the two JSON-semantics findings above, that is the case for removing the runtime input instead of guarding it.

Open pending the operator's call.

+# BASECAMP_GENERATED_VERBS first: it is what the LOADERS honour, so the gate has
+# to validate whatever they are about to read. GENERATED_VERBS_FILE is the
+# self-test's own handle on this script.
+VERBS_FILE="${GENERATED_VERBS_FILE:-${BASECAMP_GENERATED_VERBS:-$ROOT_DIR/spec/generated-verbs.json}}"


jeremy · 2026-09-19T04:37:08Z

+if ! jq -e . "$VERBS_FILE" > /dev/null 2>&1; then
+  jq . "$VERBS_FILE" 2>&1 | head -3 | sed 's/^/it is not valid JSON: /' | die


Correct, and deliberately not fixed — this finding and the BOM one are what settle the design question this PR is really asking, so patching them would bury the evidence.

jq accepting a stream of top-level values, and jq accepting a UTF-8 BOM that Python's json.loads and TypeScript's JSON.parse reject, both show the same thing: the validator is a seventh implementation with its own JSON semantics. It cannot define validity, because "valid JSON" is not one thing across seven parsers — it can only assert its own reading and hope the other six agree.

That is the premise of "one validator, six readers" failing on its own terms. A slurp check and a BOM check would be predicates six and seven of the exact class this design was built to end: Kotlin's JsonPrimitive coercion, Rust's filter_map, Ruby's ASCII strip, Swift's allSatisfy, JavaScript's $. Adding two more would make the count seven and the lesson zero.

Left open on purpose. The structural decision is with the operator, between baking the verb set into each generator at build time — nothing loaded, nothing to validate, no parser to disagree — and accepting a gate that cannot be made airtight. This thread is evidence for that call, not a queue item.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4dbec418a7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-09-18T23:42:03Z

+if ! jq -e . "$VERBS_FILE" > /dev/null 2>&1; then
+  jq . "$VERBS_FILE" 2>&1 | head -3 | sed 's/^/it is not valid JSON: /' | die


Reject JSON forms that the generators cannot parse

This jq validity check accepts inputs that the generator parsers reject. For example, a declaration prefixed with a UTF-8 BOM passes this check and the subsequent shape validation, but Python's json.loads and TypeScript's JSON.parse reject it; similarly, jq accepts concatenated top-level JSON documents as a stream while those consumers reject the extra data. In either case make check-generated-verbs reports success and the generation pipeline later fails, so require exactly one consumer-compatible JSON document and cover these cases in the self-test.

AGENTS.md reference: AGENTS.md:L88-L89

Useful? React with 👍 / 👎.

Correct, and deliberately not fixed — this finding and the BOM one are what settle the design question this PR is really asking, so patching them would bury the evidence.

jq accepting a stream of top-level values, and jq accepting a UTF-8 BOM that Python's json.loads and TypeScript's JSON.parse reject, both show the same thing: the validator is a seventh implementation with its own JSON semantics. It cannot define validity, because "valid JSON" is not one thing across seven parsers — it can only assert its own reading and hope the other six agree.

That is the premise of "one validator, six readers" failing on its own terms. A slurp check and a BOM check would be predicates six and seven of the exact class this design was built to end: Kotlin's JsonPrimitive coercion, Rust's filter_map, Ruby's ASCII strip, Swift's allSatisfy, JavaScript's $. Adding two more would make the count seven and the lesson zero.

Left open on purpose. The structural decision is with the operator, between baking the verb set into each generator at build time — nothing loaded, nothing to validate, no parser to disagree — and accepting a gate that cannot be made airtight. This thread is evidence for that call, not a queue item.

chatgpt-codex-connector · 2026-09-18T23:42:03Z

+# unset; giving the test variable precedence (as the first version of this line
+# did) let a valid GENERATED_VERBS_FILE mask a malformed BASECAMP_GENERATED_VERBS
+# and pass the gate immediately before the generator loaded the bad one.
+VERBS_FILE="${BASECAMP_GENERATED_VERBS:-${GENERATED_VERBS_FILE:-$ROOT_DIR/spec/generated-verbs.json}}"


Align the override with the compiled generator inputs

When BASECAMP_GENERATED_VERBS is set for make kt-generate-services, swift-generate, or rs-generate-services, this line validates the override, but those recipes do not pass --verbs; their Kotlin, Swift, and Rust entry points therefore still read their committed default paths. A valid override can consequently let the gate pass while the compiled generator consumes an unvalidated malformed file, and an invalid override can block a generator that would never read it. Fresh evidence beyond the earlier override comment is that these three loaders do not consult this environment variable at all, so propagate the selected path through their CLI arguments or validate their actual input instead.

AGENTS.md reference: AGENTS.md:L88-L89

Useful? React with 👍 / 👎.

Correct, and not fixed for the same reason as #discussion_r4051461976.

The precise shape: Ruby, Python and TypeScript honour BASECAMP_GENERATED_VERBS, so the gate follows it to stay pointed at the file those loaders will read. Kotlin, Swift and Rust take --verbs or their default path and their recipes do not forward the variable — so with it set, the gate validates one file and three generators read another.

Closing this means either teaching three recipes to forward an environment variable or removing the override, and both are more mechanism in a design whose premise is already in question above. The override exists so self-tests can point loaders at crafted declarations; a design with no runtime input needs neither the override nor the gate.

Open pending the operator's call.

The gate was written as Ruby, rewritten as bash and jq so it could run in front of every generator without putting Ruby in a TypeScript-only contributor's way, and the executable lost its extension in the move. Eleven docstrings across six languages and three scripts kept pointing at scripts/check-generated-verbs.rb, a file that does not exist. Every one of them is a reference a reader would follow and find nothing at, which is the same stale-claim shape as a count that no longer matches its loop — here eleven times from one rename. Worth seeing as a number rather than as eleven separate corrections.

jeremy · 2026-09-19T04:37:48Z

Head is 12c1a0f. Eleven stale references fixed; four findings answered and left open on purpose.

Fixed: the gate was written in Ruby, rewritten in bash + jq so it could run in front of every generator without putting Ruby in a TypeScript-only contributor's way, and the executable lost its .rb in the move. Eleven docstrings across six languages and three scripts kept pointing at a file that does not exist. make check green.

Open, because they are the answer to the question this PR exists to ask. Review found that the premise does not hold:

jq accepts a stream of top-level JSON values, so the gate never establishes the file is one document.
jq accepts a UTF-8 BOM that Python's json.loads and TypeScript's JSON.parse reject.

So the validator is a seventh implementation with its own JSON semantics. It cannot define validity, because "valid JSON" is not one thing across seven parsers. Writing the slurp check and the BOM check would make them predicates six and seven of the exact class this design was built to end — after Kotlin's JsonPrimitive, Rust's filter_map, Ruby's strip, Swift's allSatisfy and JavaScript's $. Fixing them would bury the evidence, so they stay.

The other two are the same story from the other side: the BASECAMP_GENERATED_VERBS override points the gate at a different file than Kotlin, Swift and Rust read, and check-targets ordering is the eighth entry point needed to make "the validator actually runs" true.

The body now records all of this, plus the alternative (bake the verb set in at build time — nothing loaded, no override, no gate, every bypass gone) and a third option I would not have raised at the start: the verb set is four strings that have never changed, #925's exposure was latent, and the defect it actually named — discovery by verb list — is fixed in #931 and stays fixed however this is decided. What remains is an emission bound, and six copies of one cannot silently drop an operation: each refuses by name, and check-operation-assignment-parity catches divergence. It is worth asking whether the shared declaration costs more than the duplication it removed.

This PR is open as evidence, not as something I am asking to merge. The call is with the operator.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 12c1a0f0eb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-09-19T04:41:10Z

+# unset; giving the test variable precedence (as the first version of this line
+# did) let a valid GENERATED_VERBS_FILE mask a malformed BASECAMP_GENERATED_VERBS
+# and pass the gate immediately before the generator loaded the bad one.
+VERBS_FILE="${BASECAMP_GENERATED_VERBS:-${GENERATED_VERBS_FILE:-$ROOT_DIR/spec/generated-verbs.json}}"


Preserve empty loader overrides in the validator

When BASECAMP_GENERATED_VERBS is present but empty, :- treats it as unset, so the gate validates the committed declaration and exits successfully, while the Ruby, Python, and TypeScript loaders honor the empty value as a path and fail afterward (for example, BASECAMP_GENERATED_VERBS='' make py-generate-services). Fresh evidence beyond the prior override-precedence finding is this set-but-empty case; use the shell expansion that distinguishes unset from empty, or make every loader apply the same fallback semantics.

AGENTS.md reference: AGENTS.md:L88-L89

Useful? React with 👍 / 👎.

jeremy · 2026-09-19T07:08:11Z

Closing this rather than landing it.

The premise — one validator in front of six loaders — fails on its own terms, and this PR is what established that. jq accepts a JSON stream and a UTF-8 BOM that Python's json.loads and TypeScript's JSON.parse reject, which makes the validator a seventh implementation with its own JSON semantics. It cannot define validity; it can only assert its own reading and hope the other six agree. Separately, placing the gate reached eight entry points in two rounds, ending at make -j scheduling the validator concurrently with the checks inside check-targets itself.

Landing it would ship a gate that does not gate. The open threads here are not a backlog — they are the evidence, and they have done their work.

The resolution is recorded on #935: keep spec/generated-verbs.json as the single definition, stop reading it as an input, and emit the verb list as a generated constant into each language's existing generated tree, where the regenerate-and-byte-diff checks already prove every committed copy current on every generating path. That dissolves the parser disagreement, the BASECAMP_GENERATED_VERBS split, the entry-point enumeration and the -j ordering problem, rather than guarding each one.

Not urgent, and the reason is worth stating: the exposure is latent — no operation in the committed spec uses a verb outside the five — and the file is a generation-time input that no shipped SDK source reads. main stands as it is.

The eleven stale .rb reference fixes in here are worth keeping; they will come back with the work on #935 rather than in a PR of their own.

- 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).

) * 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).

Copilot AI balanced review requested due to automatic review settings September 18, 2026 22:46

github-actions Bot added github-actions Pull requests that update GitHub Actions typescript Pull requests that update TypeScript code ruby Pull requests that update the Ruby SDK kotlin swift python Pull requests that update the Python SDK rust Rust SDK labels Sep 18, 2026

Copilot started reviewing on behalf of jeremy September 18, 2026 22:46

View session

jeremy mentioned this pull request Sep 18, 2026

A refusal must not destroy generated output: fixed in Kotlin and TypeScript, swept across the rest #932

Merged

Copilot AI reviewed Sep 18, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Sep 18, 2026

View reviewed changes

Comment thread scripts/check-generated-verbs.rb Outdated

Comment thread .github/workflows/test.yml

Comment thread Makefile

jeremy added this pull request to stack #934 September 18, 2026 23:09

jeremy mentioned this pull request Sep 18, 2026

Language-independent policy is transcribed once per language #935

Open

chatgpt-codex-connector Bot reviewed Sep 18, 2026

View reviewed changes

jeremy requested a balanced review from Copilot September 18, 2026 23:36

Copilot started reviewing on behalf of jeremy September 18, 2026 23:37

View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Sep 18, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Sep 19, 2026

View reviewed changes

jeremy closed this Sep 19, 2026

An error occurred while trying to automatically change base from sdk925-refusal-completeness to main September 19, 2026 07:10

jeremy mentioned this pull request Sep 19, 2026

Derive gen-catalog's emission bound from spec/generated-verbs.json #938

Merged

		if ! jq -e . "$VERBS_FILE" > /dev/null 2>&1; then
		jq . "$VERBS_FILE" 2>&1 \| head -3 \| sed 's/^/it is not valid JSON: /' \| die

Conversation

jeremy commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The class

The shape

Why a prerequisite and not a CI job

The self-test is the argument

What moved rather than disappeared

Verification

Why this may not be buildable, found in reviewing it

Uh oh!

chatgpt-codex-connector Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

github-actions Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Sensitive Change Detection (shadow mode)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

jeremy Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

jeremy Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

jeremy commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

jeremy Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

jeremy Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

jeremy Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

jeremy Sep 19, 2026

jeremy commented Sep 18, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 18, 2026 •

edited

Loading

github-actions Bot commented Sep 18, 2026 •

edited

Loading