Unbreak doc-constants-check: grant SPEC.md's two as-of pin citations - #605
Merged
Merged
Conversation
#601 added the documents merge-safe composite to SPEC.md, citing BC3 #12501 (2c0dafba13) as the commit that shipped Recording::DraftSubscribers and pinning the surface to a provenance at or after it. #590 added doc-constants-check, which rejects unmarked prose naming the current pin. Each merged green on its own branch; composed on main, the gate reads #601's two citations as unmarked pin restatements and every make check fails. Both citations are as-of facts bound to BC3 #12501 — they stay true when the pin advances — which is exactly what .unmarkedPinCitations grants. Record SPEC.md there with count 2.
There was a problem hiding this comment.
Pull request overview
This PR fixes a red make check on main caused by a cross-PR semantic conflict. #601 added the documents merge-safe composite to SPEC.md §18, whose prose (line 415) cites BC3 #12501 (2c0dafba13) twice as an as-of fact about the fix that shipped Recording::DraftSubscribers. #590 then added doc-constants-check, which rejects unmarked prose that restates the current provenance pin — and 2c0dafba13 happens to be that current pin. Each PR was green in isolation; composed on main the gate flags #601's two citations. The fix records SPEC.md in spec/doc-constants.json .unmarkedPinCitations with count: 2, matching the gate's own prescribed remedy for as-of citations that must not be marked <!-- @bc3-pin -->.
Changes:
- Add a
SPEC.mdgrant (count: 2) to.unmarkedPinCitationsinspec/doc-constants.json, with a reason binding both citations to a fixed BC3 observation.
I verified the change against the gate script (scripts/sync-doc-constants.rb) and the source prose:
2c0dafba13is the current pin (spec/api-provenance.json.bc3.revision=2c0dafba1312…).- Line 415 contains exactly two occurrences of the current pin, and it is the only line in
SPEC.mdmatching the pin prefix; the third SHA on the line (344581a379, #12494) is not the current pin and is correctly not counted. count: 2equals the gate's occurrence tally (it scans occurrences, not lines), so the grant neither under- nor over-covers.- Line 415 does not match the
CLASS_A_GRAMMAR_REfloor, so the grant is sufficient (a class-A "the pin is X" phrasing could not be waved through by a grant). - The grant is well-formed (positive integer
count, non-emptyreason), anddoc-constants.jsonitself is not scanned (only tracked*.mdfiles are), so the reason strings containing the SHA are safe. scripts/test-doc-constants.rbuses self-contained fixtures, so it needs no update.
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.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
Merging on green + Copilot's clean review without waiting for a Codex pass: main is red and every open PR's CI inherits the breakage, the diff is a 4-line grant using the gate's own prescribed remedy, and the doc-constants self-test passes. Happy to address any post-merge review findings in a follow-up. |
jeremy
added a commit
that referenced
this pull request
Aug 3, 2026
…iscovery * origin/main: deps(ts): bump the npm-dependencies group in /typescript with 2 updates (#609) deps(ruby): bump simplecov in /ruby in the bundler-dependencies group (#607) deps(kotlin): bump the ktor group in /kotlin with 5 updates (#608) Unbreak doc-constants-check: grant SPEC.md's two as-of pin citations (#605)
jeremy
added a commit
that referenced
this pull request
Aug 3, 2026
#590 landed `make doc-constants-check` on main after this branch was cut, and it gates SPEC.md §19's table against the `conformance/schema.json` assertion enum: a new type cannot ship undocumented. This branch adds `errorRaised` to that enum, so the rebase inherited the obligation and Spec Gates went red with "defines 22 assertion types, the table documents 21". The row says what the type is for and what declaring it costs — it switches the stop-on-mismatch policy off for that case, which is why every fixture declaring it needs a control sibling. The other finding in that same red run — SPEC.md §Documents restating the current pin — was #601's, not this branch's, and #605 has since fixed it on main. An earlier revision of this branch carried its own fix for it; that is dropped, so the only line this PR adds to SPEC.md is the one above.
jeremy
added a commit
that referenced
this pull request
Aug 3, 2026
* Refuse a malformed GET field instead of writing it back (#576) The shipped Todos and Cards merge-safe composites in Python, Ruby and TypeScript read each writable field off a GET and PUT the FULL representation back. Every value read is therefore a value written -- on a call that never mentioned the field -- and none of the three validated what they read. Two failure modes, the same defect wearing different clothes: erasure a falsey non-string coalesced away, wiping the field corruption a non-string forwarded verbatim, writing a number, boolean, array or object where a string belongs Probed against the unfixed code, one call each, `update(content:)` and `update(title:)`: Python Todos description=False,0,[],{} -> PUT description="" description=42,True,["x"] -> PUT description=42 / True / ["x"] assignees[0].id="100" -> PUT assignee_ids=["100"] Python Cards due_on=False,0,[],{} -> PUT with due_on OMITTED, which is exactly how BC3 erases the date due_on=42,True,["x"] -> PUT due_on=42 / true / ["x"] Ruby Todos description=false -> PUT description="" description=0,[],{},42,... -> PUT description=0 / [] / {} / 42 Ruby Cards every shape -> PUT due_on=<shape verbatim> TS Todos all eight shapes -> PUT description=<shape verbatim> TS Cards due_on=false,0 -> PUT with due_on OMITTED (erased) All three now treat an absent key or an explicit null as genuinely empty, pass an actual string verbatim, and raise before the PUT naming the field. The ID-list fields get the analogous check: an array, of objects, each with an integer id. One level up, the response itself must be an object -- on main a scalar or null body produced a raw TypeError/AttributeError instead of the documented statusless api_error. The rule underneath: a composite is safe exactly when a decoder REJECTS a wrong-typed field at runtime, not when a type merely claims one. Go (json.Unmarshal) and Swift (Codable) genuinely refuse. TypeScript's schema.d.ts is erased at build time and the generated Python and Ruby services return an untyped dict/Hash, so those three do it by hand, in a shared per-language helper (_merge_safe.py, merge_safe.rb, merge-safe.ts) rather than six copies. Kill coverage lands in the SHARED conformance fixtures, not per language. This defect survived five consecutive review passes because each pass fixed one instance; a shared fixture catches every instance at once, in every runner, permanently. Four cases across todos_write.json and cards_write.json assert errorRaised + requestCount 1 -- the guard must fire BEFORE the PUT, because a guard that fires after has already lost the field. errorRaised is a new assertion type, the code-agnostic inverse of noError: the six SDKs refuse the same body by two different mechanisms (hand-written guard vs model decoder) that share no canonical error code. Declaring it also tells the Kotlin and Swift runners that a decoder rejection is the point of the case rather than an under-specified fixture body. Writing that fixture immediately earned its keep: it found a FOURTH affected language. Kotlin's client-wide `Json { isLenient = true }` coerces a JSON scalar into a String field, so `"description": 42` decodes to "42" and the composite writes it back -- proven on the wire with a temporary requestBody assertion. #576 lists Kotlin as structurally safe; it is not, for scalars. It cannot be fixed by this PR's pattern either, since the coercion happens at decode and the composite only ever sees a String, so the fixtures use array/object shapes (which kotlinx.serialization does reject) and the scalar hole is filed separately. Red proof, against unfixed composites: 85 Python, 81 Ruby, 78 TypeScript unit failures, and 4/4/2 conformance kill-case failures (py/rb/ts). Go, Kotlin and Swift pass the kill cases both before and after, which is the point. Deliberately out of scope: the caller-side mirror (a closure assigning 42 inside edit), the Kotlin lenient-decoder hole, and the generated validating layer that would make all of these guards deletable (#578). * Close the errorRaised coverage gaps: a TS-discriminating kill case and six runner unit tests Three verifier findings on #597, all about the new assertion proving less than it claimed. The Cards kill cases did not discriminate in TypeScript. The generated updateVerbatim guards due_on with /^\d{4}-\d{2}-\d{2}$/.test(req.dueOn), and RegExp.test coerces its argument to a string first, so ["x"] ("x") and {} ("[object Object]") were already rejected before the PUT with or without the guard -- TypeScript conformance failed 2 kill cases against the unfixed composites, not 4, and TypeScript Cards had no regression protection at all. A fifth case uses ["2024-02-01"], which String() renders as exactly "2024-02-01": the format check waves it through and only the guard stops it. It still discriminates in Python and Ruby, and Go, Kotlin and Swift reject it structurally as they do any JSON array in a String field, so the shared fixture stays shared. The errorRaised handler had no unit test in any runner. Its failing branch is unreachable from conformance/tests/ -- every case declaring it is one the SDK does refuse -- so a handler that accepted everything would report green in all six runners at once, which is how #563 shipped a vacuous delayBetweenRequests check. The predicate is split out per runner and tested on both directions, with the message pinned verbatim in all six. Go also asserts the wiring, since a typo'd case label would fall through to the default and assert nothing. evaluateAssertions(dispatchFailed:) loses its default. The one call site passes it, but the default fails closed: a future call site that omitted it would report "the call succeeded" on a call that did not, reddening every errorRaised fixture far from the actual bug. Also fixes the Codex P2: the Swift HTTPS-enforcement probe recorded caughtError without setting dispatchFailed, so a trapped child process read as a successful call. Runner.swift now flags it, and both Swift and Kotlin derive the assertion from the union of the two signals rather than from call-site discipline. * Keep the errorRaised kill fixtures honest with a control-sibling gate Codex P2 on Runner.swift: declaring errorRaised switches OFF the #555 stop-on-mismatch policy in the decoder-backed runners. Swift's DecodingError branch and Kotlin's MissingFieldException/SerializationException branches normally fail loudly when a mock body no longer decodes into the generated model; when the fixture declares errorRaised they treat the refusal as the behaviour under test and pass. So if a model later gains a required field, or any unrelated field in one of these large bodies drifts, the decode fails for a reason unrelated to the field under test -- and errorRaised, requestCount: 1, requestMethod and requestPath all still hold. The case keeps passing and stops proving anything. The protection turns out to already exist, structurally: every kill body is a passing case's body with exactly one field perturbed, and that sibling does NOT declare errorRaised, so it keeps the full #555 policy and fails loudly on drift. Cards kill cases pair with update-preserves-due-on (differing only in due_on), Todos with update-merge (differing only in description). But nothing enforced the coupling -- edit one body without the other and it silently breaks. So enforce the claim rather than asserting it in a comment, which is the #576 lesson applied to #576's own fixtures: for every case declaring errorRaised, some case in the same file that does not declare it must have a mock body with the identical key set, differing in exactly one field. Runs in make conformance-fixtures-check, which CI already invokes. Proven non-vacuous: perturbing a kill body in a second field fails the gate (exit 1) and names the case, the missing control and the repair. * Pin the control gate to the response the kill case actually decodes Codex P2 on the gate added a commit ago: it matched a kill body against ANY queued response of ANY control case. Every kill case queues two object-shaped responses -- response 0 is the GET whose decoder rejection is under test, and response 1 is a decoy, queued so a runner cannot pass by exhausting the queue instead of refusing the field. The decoy is never consumed. So an unconsumed decoy could satisfy the gate while the body that actually gets decoded drifted away from its control, which is the same vacuity the gate exists to prevent, one level up. Reachable, not theoretical. Drift the consumed body by a second field and make the decoy differ from its control by exactly one, and the two gates split cleanly: the old one reports `ok` at exit 0, the new one fails at exit 1 naming the case. Now restricted on both sides: the FIRST mock response only, and a control exercising the SAME operation -- a body that decodes into a different model says nothing about whether this one still decodes. * Move $comment out of the assertion properties map, and metaschema-check first Codex P2: the errorRaised annotation sat INSIDE properties.assertions.items.properties, so it declared a property literally named "$comment" whose schema was a string. Draft 2020-12 requires every value under `properties` to be a schema object or boolean, so conformance/schema.json was not itself a valid schema -- and tests.schema.json references it, meaning a validator that meta-validates would reject the whole conformance schema before looking at a single fixture. Moved alongside `properties`, where JSON Schema puts annotations. The reason this shipped is that nothing checked it: the fixture pass validates fixtures AGAINST the schema and never validates the schema itself, so an invalid schema sails through. conformance-fixtures-check now runs --check-metaschema over schema.json and tests.schema.json FIRST, because validating fixtures against a schema that is not a valid schema proves nothing. Red proof, through the make target rather than the bare validator: putting the annotation back inside properties fails at REAL_EXIT=2 with conformance/schema.json::$.properties.assertions.items.properties['$comment']: '...' is not of type 'object', 'boolean' * Require a kill case to deliver its malformed value in a 2xx response Codex P2, the residual hole in the control gate: it compared operation and body but not the response OUTCOME. Change an errorRaised case's first mock response from 200 to 500, or to networkError while keeping its body, and the SDK fails on the HTTP or transport error instead. errorRaised is satisfied by that failure, requestCount / requestMethod / requestPath all stay green, and the malformed field is never decoded -- the case goes green having tested nothing, and body equality cannot see it. A kill case's premise is that the malformed value arrived in a SUCCESSFUL API response. That is what makes it the SDK's problem rather than the server's, and it is why #576 classifies the refusal as a statusless api_error rather than a transport or HTTP failure. So require it: the first mock response must carry a 2xx status and no networkError. Both failure modes proved red, each naming what it would have cost: status 500 -> "...so the call fails on the HTTP error before the body is decoded" (REAL_EXIT=1) networkError -> "...so the call fails in transport and the body is never decoded" (REAL_EXIT=1) * Require the control response to reach its decoder too Codex P2, the symmetric half of the previous commit: not_a_success was applied to the kill response but not to the control. A control earns its keep only by being DECODED -- that is what makes it fail loudly (#555) on model drift, which is the entire protection the kill case borrows from it. A sibling answering 500 or networkError with an object body never reaches its decoder, so it can sit green on its own HTTP/transport assertions while the drift it was supposed to catch goes unnoticed in both bodies. Same check now, on both sides. Red proof needed a second attempt, which is worth recording. Breaking a single control (update-preserves-due-on -> 500) did NOT fail the gate: cards_write.json has four non-errorRaised UpdateCard cases, and the gate correctly fell back to update-explicit-clear, whose body also matches on the same key set differing only in due_on. That first proof was vacuous -- it demonstrated the fallback working, not the check. With all four UpdateCard controls answering 500 the two versions split cleanly: the pre-fix gate reports `ok` at REAL_EXIT=0, the post-fix gate fails all three Cards kill cases at REAL_EXIT=1, naming the missing SUCCESSFUL (2xx) control. * Reject a kill case whose 2xx never reaches a decoder, and self-test the gate The control-sibling gate accepted any 2xx on both sides, which let a 204 through. A 204 is short-circuited before any parse — TypeScript returns `undefined`, Kotlin returns `Unit` without calling `parse`, Go rewrites the body to JSON `null` — so a kill case answering 204 never decodes its malformed field. The composite fails because the record came back absent, `errorRaised`, `requestCount: 1`, `requestMethod` and `requestPath` all still hold, and the control, still a 200, stays green. The gate printed `ok` and exited 0 for exactly that input: the same false green this gate exists to prevent, one layer down. Statuses are now an allowlist, {200, 201}, rather than a 204 exclusion. Two constraints meet there: Go's success arm is exactly {200, 201, 204}, so 202, 203, 205 and 206 are never decoded there at all; and 204/205 carry no body by definition. Closed-by-default, because a gate whose whole job is to prove a body is decoded cannot prove that for a status nobody has reasoned about. `not_a_success` is renamed `not_decoded` — a 204 does not fail the call, it bypasses the decode, and the old name said the wrong thing. Every rejection this gate makes was, until now, correct by inspection alone, which is the standard that let #576 through five review passes. So it gets a self-test: `conformance/test_check_kill_case_controls.py` crafts one input per claimed rejection and asserts the gate refuses it, driven through the real entry point via a new optional FIXTURE_DIR argument, with the real fixture set run as a positive control. Reverting only the two new status branches turns exactly four cases red — 204, 205, an undecoded 2xx, and the control-side 204 — and nothing else, so the suite is measured non-vacuous rather than assumed so. It runs inside `make conformance-fixtures-check`, which CI already invokes. * Document errorRaised in SPEC.md §19's gated assertion table #590 landed `make doc-constants-check` on main after this branch was cut, and it gates SPEC.md §19's table against the `conformance/schema.json` assertion enum: a new type cannot ship undocumented. This branch adds `errorRaised` to that enum, so the rebase inherited the obligation and Spec Gates went red with "defines 22 assertion types, the table documents 21". The row says what the type is for and what declaring it costs — it switches the stop-on-mismatch policy off for that case, which is why every fixture declaring it needs a control sibling. The other finding in that same red run — SPEC.md §Documents restating the current pin — was #601's, not this branch's, and #605 has since fixed it on main. An earlier revision of this branch carried its own fix for it; that is dropped, so the only line this PR adds to SPEC.md is the one above.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
make checkis red onmain(c441c235b): a cross-PR semantic conflict between the last two merges.dee221c85, merged first) added the documents merge-safe composite to SPEC.md. Its §18 prose cites BC3 #12501 (2c0dafba13) as the commit that shippedRecording::DraftSubscribers, and pins the surface to "a bc3 provenance at or after2c0dafba13" — one line, two backticked SHAs.c441c235b, merged second) addeddoc-constants-check, which rejects unmarked prose naming the current pin — and the current pin happens to be2c0dafba13.Each PR was green on its own branch; neither branch contained the other's change. Composed on
main, the gate reads #601's citations as unmarked pin restatements:The fix is the one the gate's own error message prescribes. Both citations are as-of facts bound to a fixed observation (BC3 #12501 / the floor it created), not claims about today's pin — they stay true when the pin advances, so marking them
<!-- @bc3-pin -->would be wrong (the writer would rewrite a historical citation on the next repin). RecordSPEC.mdinspec/doc-constants.json.unmarkedPinCitationswith count 2, matching the existing grants forspec/api-gaps/README.md(which already covers this same #12501 citation in its triage narrative) andfolders-api.md.Line 415 does not match the gate's class-A grammar floor ("the pin is X" / "at the pinned X"), so the grant is sufficient.
Verification
main):make check→make: *** [doc-constants-check] Error 1at SPEC.md:415, as quoted above.make doc-constants-check→ clean (REAL_EXIT=0), andruby scripts/test-doc-constants.rb→ all cases passed.Every open PR's
make checkinherits this breakage on rebase, so this wants to land ahead of the queue.Summary by cubic
Fixes false positives in
doc-constants-checkby granting SPEC.md two as‑of pin citations inspec/doc-constants.json. This prevents the gate from flagging historical references and restores a cleanmake checkonmain.Written for commit f9bd834. Summary will update on new commits.