Compare what the six generators emitted, not what two of them agree on - #752
Conversation
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
There was a problem hiding this comment.
Pull request overview
Adds a cross-SDK gate ensuring generated service inventories remain aligned across all six SDKs.
Changes:
- Compares eight generated/service-accessor inventories with checked Go carve-outs.
- Adds synthetic positive and negative self-tests.
- Integrates the checks into
make, CI, and documentation.
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.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
SPEC.md |
Documents the enforced inventory parity. |
scripts/check-service-inventory-parity |
Implements the cross-SDK gate. |
scripts/test-check-service-inventory-parity.rb |
Tests gate failure modes with synthetic trees. |
scripts/sync-doc-constants.rb |
Updates derivation commentary. |
Makefile |
Adds parity check targets. |
.github/workflows/test.yml |
Runs both checks in CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6db976fa45
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6db976fa45
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Round 2 convergence stateAll 7 review threads answered and resolved: 2 fixed prose (Copilot), 1 fixed with code (Codex, stale Python modules), 4 declined with reasoning and follow-up issues (#755 reachability ×3, #756 operation assignment ×1). A third issue, #757, records the root defect the stale-module thread surfaced: Codex: clean at matched head — Copilot — old head, and I could not re-request it. Its review is against CI: rollup SUCCESS, 41 contexts, 0 failures, 0 pending. Not merging — held for review as instructed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
scripts/check-service-inventory-parity:341
- This diagnosis is not valid for all compared renderings. Case 3 deliberately creates a Kotlin service file/accessor mismatch while every split table still agrees, and a Go-only mismatch can be missing hand-written wiring rather than a mapping difference. Report the inventory disagreement without asserting that the split tables caused it.
failures << "`#{name}` is emitted by #{present.join(', ')} but NOT by #{absent.join(', ')} — " \
"the split tables disagree about whether this service exists."
scripts/check-service-inventory-parity:220
- This footer is emitted for every failure, including a missing input, duplicate extraction, extraction-floor failure, or stale Go carve-out. In those cases it incorrectly diagnoses split-table drift and can send maintainers to the wrong place. Keep the footer neutral and let the specific failure identify whether extraction, accessor emission, carve-outs, or mappings need correction.
This issue also appears on line 340 of the same file.
warn " The service split is one mapping transcribed into five generator configs."
warn " This gate compares what they EMITTED. A name that appears in some SDKs and"
warn " not others means a tag mapping was updated in some tables and not the rest."
scripts/sync-doc-constants.rb:403
- This says all six inputs are generated output, but the new checker explicitly reads Go from
go/pkg/basecamp/client.go, whose service accessors are hand-written. Describe Go separately so this rationale matches the actual evidence being compared.
# scripts/check-service-inventory-parity compares the GENERATED service
# inventories of all six SDKs — which is not the sixth hand-copy rejected above,
# since it reads generator OUTPUT rather than reimplementing the mapping. It also
36a6134 to
e650cec
Compare
|
Rebased onto Re-verified locally against post-merge
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e650cecab0
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
scripts/sync-doc-constants.rb:402
- This new “closed elsewhere” paragraph now contradicts line 356 in the same comment, which still says “no gate compares them to each other.” Since this PR introduces exactly that gate, update the earlier explanation too so future readers do not get opposite descriptions of the derivation's safety.
# That residue is real and is closed ELSEWHERE, not here:
# scripts/check-service-inventory-parity compares the GENERATED service
# inventories of all six SDKs — which is not the sixth hand-copy rejected above,
The service split is one mapping hand-transcribed into five generator configs, and nothing compared them. The per-SDK `check-*-service-drift` scripts compare operationIds, never service names, so a service added to three tables and forgotten in the other two produced five different SDK surfaces and a fully green `make`. #745 made SPEC section 5's roster derived, but from the generated Kotlin and Swift accessors only, because those two encode account-scoping directly. Codex named the residue: those are two independent transcriptions, not two renderings of one root, so a service omitted from BOTH leaves them agreeing and certifies the old roster and the old count. Agreement between two transcriptions is not confirmation against a root. `scripts/check-service-inventory-parity` reads what the generators ALREADY EMITTED and asserts the emissions agree — five generated service directories, the two accessor files, and Go's hand-written accessors. Reading output rather than reimplementing the mapping is what keeps it from being the sixth hand-copy #745 rejected. Committed generated files only, so it needs no toolchain and lands in the spec-gates job beside kt-check-drift. All eight agree on 53 today. It also checks each accessor file against its own services directory, which is the other half: section 5's roster reads accessors, so a generator that emitted a service file and no accessor would leave the roster short with every directory agreeing the service exists. Normalization is one rule per source, expressed as fields on a SOURCES entry rather than a per-SDK branch: files to drop, one suffix to strip, one identifier casing. Go's three divergences are data too, and are checked for STALENESS rather than merely applied — a fold that closes or a spelling that changes fails the gate instead of quietly ceasing to mean anything. The self-test drives the real checker over synthetic trees built by inverting that normalization, so the builder round-tripping 53 names is itself a positive control. Ten negative cases, and the guard-to-case mapping in its header is measured, not reasoned: each guard was neutered in a copy and the red set recorded. Neutering the parity comparison turns 1, 2, 3 and 9 red; the duplicate check 4; the floor 5; the existence check 6; the two staleness checks 7 and 8; the both-spellings check 10. Neutering either carve-out APPLICATION turns both positive controls red, which is the only way a rule that prevents false positives can be pinned. SPEC.md's derivation rule said no gate compares the tables. It does now, and the sentence says so; the `@service-count` marker beside it is untouched. Appendix F's preamble and sync-doc-constants.rb's note that the residue was open are updated for the same reason. Closes #747
Two review findings, both correct. CODEX: a stale Python module counted as still emitted. Of the five generators, four sweep outputs the current mapping no longer produces (generate-services.ts:1736, generate-services.rb:344, Main.kt:65, and Swift regenerates whole); `python/scripts/generate_services.py` does not. So dropping a Python service leaves its module on disk, and a directory listing counts the corpse — the exact divergence this gate exists to catch, passing it. It reaches CI, which I had assumed it did not. `check-python-service- drift.sh` regenerates to a tmpdir and `diff -rq`s, so it reports `Only in <committed>: fanfares.py` and fails — verified. But the Python CI job does not run that script. It regenerates IN PLACE and tests `git status --porcelain`, which is blind to a stale file that is already committed: nothing rewrites it, nothing deletes it, so there is no diff. Verified in a throwaway repo: 0 porcelain lines, corpse still present. Python is now read from its generated `__init__.py`, which is rewritten whole every run and so cannot name a service that is no longer emitted. It also does not import `_base.py`/`_async_base.py`, so the hand-written infrastructure drops out of the reading instead of being filtered from it. The generator defect itself is #757; this only stops one gate believing a corpse. Two cases, both red-proven against the pre-fix checker, which fails 11 with "`fanfares` is emitted by python but NOT by ..." and passes 12: 11. stale module on disk, absent from the barrel -> must PASS 12. barrel short a service the directory still has -> must FAIL Case 11 is pass-shaped on purpose: nothing else can pin a fix whose whole effect is to stop a false failure. Reverting the source to a directory listing turns 11 and 12 red and nothing else, measured. COPILOT: the motivation prose was false. It said the per-SDK drift scripts compare operationIds and never service names. TypeScript, Ruby, Python and Swift regenerate-and-diff whole trees, so they do see service filenames; only Kotlin's fast check is operationId-only. The real gap is the AXIS, not the granularity: each validates one SDK against its own generator and can see no other SDK. Reworded in the script header, the Makefile and the workflow, and SPEC's derivation rule now names the barrel rather than claiming five directories. A gate whose stated motivation is false is one nobody can size later, which is the whole reason this is worth a commit rather than a footnote. Refs #757
Codex, correctly. Python's entry borrowed Ruby's `strip_suffix: "_service"`,
and the comment said "Ruby suffixes every file, so one rule covers both".
Sharing the rule is exactly the bug: it is right for Ruby and wrong for
Python.
`generate_services.py`'s `service_filename` renames one module and tests
for it by name:
snake = to_snake_case(name)
if snake == "webhooks": return "webhooks_service.py"
return f"{snake}.py"
Every other service keeps its snake name verbatim — including one whose
canonical name legitimately ends in `_service`. A blind suffix strip took
such a name apart, so Python reported a service the other seven did not
have and the gate failed on a mapping where every generator agreed.
That direction matters. This was a FALSE POSITIVE, not a miss: a red
build blocking a correct change, and an invitation to "fix" a mapping
that was never wrong. Ruby keeps `strip_suffix:` because suffixing every
file genuinely is its rule; Python gets `rename:`, an exact-match map
applied before any suffix logic. Two instruments, because the two
generators do two different things.
Case 13 adds a service whose canonical name is `notification_service`
across all eight renderings and requires a PASS. The roster still holds
`webhooks`, so one case pins both halves: the module that IS renamed and
one that merely looks like it.
Red-proven against the pre-fix checker, which fails it with both sides of
the split it invents:
`notification` is emitted by python but NOT by typescript, ruby, ...
`notification_service` is emitted by typescript, ruby, ... but NOT by python
Kill matrix re-measured. Respelling the rename as `strip_suffix` turns 13
red and nothing else, which is what makes it a pin rather than a smoke
alarm. Deleting the rename outright, or never applying it, turns both
positive controls plus 11 and 13 red — 11 and 13 are pass-shaped and go
red on any global breakage, and the header now says so rather than
implying they are specific.
Four Copilot findings, all correct, and one class: the gate was precise
about what it MEASURES and loose about what it CLAIMS.
Three of the four arrived as SUPPRESSED comments inside the review body,
which never become threads and so never move the unresolved count. Two
sweeps read clean past them. The fourth appeared only in the newest
review, since the block regenerates each time.
The per-failure message asserted "the split tables disagree about whether
this service exists". Self-test case 3 is a standing counterexample: it
constructs a Kotlin service file with no accessor beside it, where every
split table agrees and a generator disagrees with ITSELF. And go-accessors
is hand-written, so a Go-only absence is missing wiring, not a mapping
difference. It now reports which renderings lack the service and where to
look, and asserts nothing about why.
The failure footer had the same defect at summary level: it printed for
EVERY failure class — missing input, duplicate extraction, collapsed
extraction, stale carve-out — and told all of them a tag mapping had
drifted. Five of this suite's own cases would have sent a maintainer to
the wrong file. Now neutral.
sync-doc-constants.rb said this gate compares "the GENERATED service
inventories of all six SDKs". Go has no generated per-service files, so
that was wrong in the one place the argument turns on it: "not a sixth
hand-copy, because it reads generator OUTPUT" is weaker if a rendering is
hand-written. Seven of eight are generated; Go is the exception, which is
exactly why it carries three carve-outs and now the type check below. The
same over-claim is corrected in the script header and SPEC. Its earlier
paragraph also still said "no gate compares them to each other", which
this PR falsifies; it now points at the gate without weakening the
argument for not writing a sixth copy.
Separately, an open thread: the Go rendering recorded only accessor NAMES,
so `Gauges() *ReportsService` contributed `gauges` and the gate reported
parity while Go handed callers the wrong service. Both types exist, so the
compiler is silent, and Go's accessors ARE its inventory here — nothing
else would catch it. Now checked as a pair. All 51 satisfy name == type
stem today. A backreference in the pattern would have made a mismatch fail
to EXTRACT, reported as "go-accessors is missing gauges" — true, and a bad
description of a line spelling `gauges` right there.
Three new cases, all red-proven against the pre-fix checker:
14. Go accessor returning another service type -> pre-fix: PASSED,
"53 services agree across 8 renderings", the miss exactly
15. stale carve-out failure must not say "tag mapping" -> pre-fix:
"failed correctly but MISDIAGNOSED"
3. now also asserts the parity message does NOT blame the split tables
15 and 3 use a new `expect_fail_without`: the only way to hold a
diagnostic honest is to assert what it must NOT say. Kill matrix
re-measured — the type check pins 14 alone, the footer pins 15 alone, the
parity message pins 3 alone.
5e3e48a to
b43d7aa
Compare
Copilot's suppressed findings — all four accepted, fixed in
|
| # | finding | disposition |
|---|---|---|
| 1 | :341 per-failure message asserts the split tables disagree |
fixed |
| 2 | :220 (+340) footer diagnoses mapping drift for every failure class |
fixed |
| 3 | sync-doc-constants.rb:403 claims all six inputs are generated output |
fixed |
| 4 | sync-doc-constants.rb:402 contradicts line 356 in the same comment |
fixed |
They are one class, and it is the same class as the two per-SDK-drift threads earlier in this PR: the gate is precise about what it MEASURES and loose about what it CLAIMS. Third catch of it, which is a signal about the prose, not three separate edits.
1 and 2 — stop naming a cause. The message said "the split tables disagree about whether this service exists". My own case 3 is a standing counterexample: it builds a Kotlin service file with no accessor beside it, where every split table agrees and a generator disagrees with itself. And go-accessors is hand-written, so a Go-only absence is missing wiring, not a mapping difference. The footer had the same defect at summary level — it printed for missing inputs, duplicate extraction, extraction-floor collapse and stale carve-outs, and told all of them a tag mapping had drifted. Five of this suite's own cases would have sent a maintainer to the wrong file. Both now report which renderings disagree and where to look, and assert nothing about why.
3 and 4 — Go is not generated output. The rationale's whole weight is "this is not a sixth hand-copy, because it reads generator OUTPUT", and that is materially weaker if one rendering is hand-written. Seven of the eight are generated; Go is the exception, having no generated per-service files. That is now stated wherever the argument is made (the script header, sync-doc-constants.rb, and SPEC), together with the reason it is acceptable — Go's accessors are still an independent statement of which services exist — and the consequence: Go alone needs three recorded carve-outs, and now the type check below. The earlier paragraph's "no gate compares them to each other" is corrected too, without weakening the case against writing a sixth copy.
Open thread :191 — Go accessor return type. Correct, fixed, resolved.
Gauges() *ReportsService compiles (both types exist), and reading only the accessor name recorded gauges, so all eight inventories agreed while Go handed callers the wrong service. Go's accessors are its inventory here, so nothing else would have caught it. Now checked as a pair; all 51 satisfy name == type stem today. I did not use a backreference in the extraction pattern: that makes a mismatch fail to extract, surfacing as "go-accessors is missing gauges" — true, and a poor description of a line spelling gauges right there.
Red proofs (pre-fix checker via SERVICE_INVENTORY_CHECKER, git show HEAD~1: into a scratch copy)
| case | expects | pre-fix result |
|---|---|---|
| 14. Go accessor returning another service type | FAIL | passed — OK: ... 53 services agree across 8 renderings, the miss exactly |
| 15. stale carve-out failure must not say "tag mapping" | FAIL without that phrase | "failed correctly but MISDIAGNOSED" |
| 3. parity message must not blame the split tables | FAIL without that phrase | MISDIAGNOSED |
15 and 3 use a new expect_fail_without helper. A gate that fails for the right reason while naming a cause it cannot observe is still a defect, and the only way to pin a diagnostic is to assert what it must not say.
Kill matrix re-measured — each new guard pins exactly one case: accessor-vs-type → 14, footer → 15, parity message → 3. (Case 15 also goes red if the carve-out staleness check is removed, since it needs that failure to exist before it can inspect how it is described. The header table says so.)
LC_ALL=C make check-service-inventory-parity test-check-service-inventory-parity doc-constants-check lint-actions → REAL_EXIT=0, 17/17 PASS, re-run after rebasing onto 1c883d8a5.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The service split is one mapping hand-transcribed into five generator configs, and nothing compared them. The per-SDK
check-*-service-driftscripts compare operationIds, never service names, so a service added to three tables and forgotten in the other two produced five different SDK surfaces and a fully greenmake.#745 made SPEC §5's roster derived, but from the generated Kotlin and Swift accessors only, because those two encode account-scoping directly. Codex named the residue: those are two independent transcriptions, not two renderings of one root, so a service omitted from both leaves them agreeing and certifies the old roster and the old count.
The instrument
scripts/check-service-inventory-parityreads what the generators already emitted and asserts the emissions agree. Reading output rather than reimplementing the mapping is what keeps it from being the sixth hand-copy #745 rejected. Committed generated files only — no toolchain — so it lands in the spec-gates job besidekt-check-drift.Eight renderings, all agreeing on 53 today:
It also checks each accessor file against its own services directory. That is the other half: §5's roster reads accessors, so a generator that emitted a service file and no accessor would leave the roster short with every services directory agreeing the service exists.
Normalization is data
One rule per source, expressed as fields on a
SOURCESentry rather than a per-SDK branch — drop-list, one suffix to strip, one identifier casing. A gate whose premise is "five hand-copies drift" cannot itself become a pile of per-SDKifs.Go's three divergences are data too, and are checked for staleness rather than merely applied: a fold that closes (
Automation()appears) or a spelling that changes (Timesheet→Timesheets) fails the gate, so a carve-out cannot quietly stop meaning anything. Same device asspec/doc-constants.json's committed marker counts.Red proofs — the guard→case map is measured, not reasoned
The self-test builds synthetic trees by inverting the gate's own normalization, so the builder round-tripping 53 names is itself a load-bearing positive control (a builder that spelled anything differently would make every negative case fail for the wrong reason).
Each guard was neutered in a copy of the checker (mutation asserted to match exactly one site, verified on disk, tracked file never touched —
git statusclean throughout) and the red set recorded:The last two rows are the measured result. Applying a carve-out is what makes Go's 51 accessors line up with the canonical 53; delete either and the real tree stops passing. A rule that exists to prevent false positives can only be pinned by a case that is supposed to pass — same shape as the
explainedfilter intest-check-grouped-client-coverage.rb.Case 1 is the #745 residue in its literal form: a service in the TypeScript, Ruby and Python tables and in neither Kotlin nor Swift. Case 3 is the one that anchors §5. Case 9 proves the Go carve-outs are three named divergences, not a blanket exemption.
Prose
SPEC.md's derivation rule said "five hand-maintained copies of one table with no gate comparing them". This PR falsifies that, so it says otherwise now; the<!-- @service-count -->marker beside it is untouched andmarkerCounts.service-count.SPEC.mdstill reads 7. Appendix F's preamble andsync-doc-constants.rb's note that the residue was open are updated for the same reason — a decline (or a closure) that lives only in a commit message gets re-litigated.Verification
All under
LC_ALL=C:make check-service-inventory-parity,make test-check-service-inventory-parity,make doc-constants-check,make lint-actions, andmake check-gradle-serialization+ its self-test (the newcheck-targetsentries are in its walked graph). Both CI steps carryLC_ALL: Cso the UTF-8-pinned reads stay proven.Closes #747
Summary by cubic
Checks cross-SDK service inventories by comparing what the generators emitted instead of trusting agreement between Kotlin/Swift accessors alone. Previously, SPEC §5 could pass if both accessors omitted the same service; now eight renderings across six SDKs must agree, and accessor files are checked against their own service directories.
scripts/check-service-inventory-parityandscripts/test-check-service-inventory-parity.rb, Make targetscheck-service-inventory-parityandtest-check-service-inventory-parity, and CI steps that run both underLC_ALL=C.__init__.pybarrel (not the directory), Kotlin/Swift accessors, and Go accessors; normalizes names, rejects duplicates, enforces a floor, pins reads to UTF‑8.webhooks_servicerename instead of stripping any_servicesuffix to avoid false positives on legitimate names; adds a pass-shaped self-test for that case.scripts/sync-doc-constants.rbto reference the new gate, note Python’s barrel, and clarify that per‑SDK drift checks validate freshness per SDK while this gate enforces cross‑SDK parity.Written for commit b43d7aa. Summary will update on new commits.
Round 2 (36a6134)
Codex, stale Python modules — fixed. Python is read from its generated
__init__.pybarrel, not its directory. Of the five generators only Python's fails to delete outputs a mapping stopped producing (generate-services.ts:1736,generate-services.rb:344,Main.kt:65all sweep), so a dropped Python service left its module on disk and a directory listing counted the corpse as still emitted.It reaches CI, which I had assumed it did not and was wrong about.
check-python-service-drift.shcatches it (Only in <committed>: fanfares.py, verified) — but the Python CI job does not run that script; it regenerates in place and testsgit status --porcelain, which is blind to a stale file already committed (verified in a throwaway repo: 0 porcelain lines, corpse still present). The generator defect and the weaker CI step are filed as #757.Two new self-test cases, both red-proven against the pre-fix checker:
`fanfares` is emitted by python but NOT by ...Reverting the Python source to a directory listing turns exactly 11 and 12 red and nothing else (measured; the header's guard-to-case table is updated, including the honest note that case 11 is pass-shaped and so also goes red for any global breakage).
Copilot, false motivation prose — fixed. The claim that the per-SDK drift scripts "compare operationIds, never service names" was wrong: TypeScript, Ruby, Python and Swift regenerate-and-diff whole trees and do see service filenames; only Kotlin's fast check is operationId-only. The real gap is the axis — each validates one SDK against its own generator and can see no other SDK. Reworded in the script header, the Makefile, the workflow step, and SPEC's derivation rule (which also now names the barrel instead of claiming five directories).
Declined, with follow-ups. Codex's three client-wiring findings (
:81TypeScript client,:77TypeScript root exports,:85Ruby accessors) are one class — generated-but-unreachable — and a different invariant from generator-output parity. Filed as #755, which records that the exposure is two SDKs and three doors, not five: Kotlin and Swift accessors are generated, Go is already covered by this gate, Python by #748's test. Codex's:307(per-service operation assignment) is a third invariant and a materially bigger instrument; filed separately as #756 because it shares no remedy with #755. Reasoning is written into each thread, not only here.