Event feed conformance: three corrections to what tier 2 claims (3/3) - #778
Conversation
There was a problem hiding this comment.
Pull request overview
Tightens tier-2 event-feed conformance claims and closes a streaming deduplication coverage gap.
Changes:
- Clarifies redirect coverage at the poll seam.
- Rejects scheduling-dependent virtual-time advances.
- Adds fixture 31 for lower-ID live stragglers.
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 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
go/pkg/basecamp/eventfeed/scenario_selftest_test.go |
Tests the new advance guard. |
go/pkg/basecamp/eventfeed/scenario_conformance_test.go |
Implements guarded virtual-time advancement. |
conformance/event-feed/README.md |
Revises tier-2 guarantees and fixture matrix. |
conformance/event-feed/fixtures/31-post-snapshot-straggler-below-served-id.json |
Adds streaming dedupe coverage. |
conformance/event-feed/fixtures/30-continuation-redirect-cross-origin.json |
Corrects the redirect fixture description. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
039aad6 to
4c8446c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
go/pkg/basecamp/eventfeed/scenario_conformance_test.go:494
- This observes only the net counts per timer name, not whether
NewTimerran. A timer can fire and be rearmed under the same name beforesettlesamples it (the connector does same-name rearming, e.g.repair-pollatcatchup.go:852), leaving the map equal tobeforeand allowing the scheduling-dependent advance. Conversely, removal alone is labeled as an arm. Track a monotonic timer-arm/firing generation infeedtest.Clockand add a same-name-rearm self-test so this guard detects activity independently of the snapshot schedule.
before := timerCounts(d.h.clock)
armed := false
d.h.clock.AdvanceSettling(millis(step.Ms), func() {
if !maps.Equal(timerCounts(d.h.clock), before) {
armed = true
conformance/event-feed/README.md:360
- The sentinel obligation is still present verbatim in the shared schema (
schema.json:906), and the fixture inventory at README line 239 still presents “zero foreign egress” without the seam qualification. As a result, the merged contract continues to make the exact tier-2 claim this PR says is withdrawn. Remove the sentinel requirement and qualify the inventory/schema text as driver construction rather than connector proof.
An earlier revision of this row claimed a harness obligation to "bind the
foreign origin to a sentinel listener whose any-request fails the scenario".
That is withdrawn. No implementation met it, and meeting it would prove
nothing: the foreign origin is unreachable **by construction of the harness**,
conformance/event-feed/README.md:131
- The shared schema still defines
advanceas firing timers armed inside the window (schema.json:1208) and does not mention this mandatory rejection. Drivers generated from or validated against that contract are therefore still instructed to implement the behavior this paragraph makes unscriptable. Update the schema description alongside the README so the cross-language contract is consistent.
Every driver must therefore FAIL an `advance` during which the connector arms
anything, naming `fireTimer` as the deterministic alternative — it fires one
named timer without moving the clock, so no re-selection is involved. This is
4c8446c to
cfd9b3d
Compare
cfd9b3d to
be5a1c7
Compare
Round summary — 3 findings, 3 fixed, 0 openSwept all three surfaces. Both review threads answered and resolved; all three suppressed comments from the round on head are addressed, since two of them carried findings no thread did.
The one that matteredFinding 1 was a test that could not fail, and the review's sharper half was in the suppressed block rather than the thread. Proven by mutation rather than argued: with the connector's Findings 2 and 3 are one mechanism — a correction that reached the README but not Not absorbed here: #789 (arrival-strict matching)#789 is the same class as finding 1 — "the driver cannot fail the contract it enforces" — and it is now the third known instance. I am deliberately not fixing it in this PR, and the reason is mechanism, not appetite. Finding 1's remedy is a per-step verdict read from an instrument the clock already had: additive, locally provable, no new concurrency. #789's remedy is a harness-wide state machine — an active-step tracked under So it should land as its own change on top of this stack. #705 made the same call for the same reason, which is why it recorded rather than patched. One piece of design input I do owe #789, posted there: this PR's finding is evidence about the shape of the remedy. Stack / rebaseRebased twice —
Gates
No finding in this round argued from bc3 server behaviour. I re-checked anyway, since the pins moved today: between |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
go/pkg/basecamp/eventfeed/scenario_selftest_test.go:365
- This control makes the new guard timing-dependent. After
Advancedelivers the due timer, the connector handles it on another goroutine; the driver only pollsArmCountuntilscenarioWatchdogexpires (scenario_conformance_test.go:508-519). If that goroutine is delayed past the watchdog, the scenario passes and can arm a timer afteradvancereturns—the exact cross-language divergence this rule is meant to eliminate. A timeout cannot prove that no arm is coming; either reject everyadvancethat fires a timer (while still allowing truly quiet windows), or add an explicit synchronous completion protocol.
{"expectConnect":{"url":"{{CABLE_URL:2}}"}}],
"finally":{"state":"awaiting_welcome"}}`
be5a1c7 to
81414d7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
conformance/event-feed/README.md:356
- Adding row 16 leaves the matrix summary inaccurate in two ways: the heading still says “fifteen,” and the introductory sentence says every listed mutation is killed even though row 15 is now explicitly only partial/below-seam. Update the heading and introduction so this conformance document does not continue overstating the suite’s proof.
| 15 | `follow-cross-origin-redirect` (follows a 302 to a foreign Location) | 30 — **partially**, and the boundary is below the seam. See the note under this table. |
| 16 | `discard-live-id-at-or-below-served-id` (streaming lane orders live ids against the highest poll-served id) | 31 — and 31 alone: verified to pass all of 01–30, because every other straggler either arrives with nothing yet served (20) or is buffered pre-cut (01, 12, 19) |
Stopping on the advance guard — third round, one classThe suppressed comment on the latest review ( The three rounds
Rounds 2 and 3 are not the same bug, but they are the same class: the driver cannot observe the quantity the rule names. Round 2 observed the wrong quantity; round 3 cannot observe the right one at all. A fourth selector on the same wait would be the treadmill. The real questionThe rule says: reject an advance during which the connector arms a timer. That predicate is decidable in a single-threaded driver and undecidable in a concurrent one — you can only ever conclude "nothing armed within N seconds". Which means the current rule has the disease it was written to cure. A rule that TypeScript decides exactly and Go decides by waiting is itself a cross-language divergence: the same fixture can be admitted in one language and rejected in another. So the question is not "how long should the wait be" — it is "what predicate can every driver decide identically and synchronously?" The candidate answer, and the measurement that makes it actionableCopilot's first alternative is the sound one: key the rule on FIRING, not arming. Every driver decides "did this advance fire a timer" synchronously, inside its own advance, on its own thread, with no waiting and no observation of the connector at all. It is sound as a conservative over-approximation because the reentrant divergence requires a firing to trigger it: no firing in the window ⇒ nothing can be armed in response ⇒ no divergence, provably, with no wait. It is strictly coarser than the true predicate, and the coarseness is the price of decidability. Its cost is that it forbids a shape currently legal — an advance whose window expires a timer that is not replaced. I measured that cost against the merged suite and it is zero. Fixture 05 is the only Both timers sit beyond the window, exactly as the fixture intends. Why I am not making the change hereIt rewrites the normative rule in What is shipping meanwhile, stated plainlyThe guard as it stands is conservative in the safe direction. Related: #789 is the same family — "the tier-2 driver cannot fail the contract it enforces" — and this is a further instance of its root shape, a predicate the driver cannot actually observe. If the firing-based rule is accepted, it should land with #789's work rather than as a fourth pass here. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (5)
conformance/event-feed/README.md:358
- Calling this a “partial kill” contradicts the next sentence, which says the redirect-follow mutation lives below the seam and no tier-2 harness can reach it. Describe it as unreachable/not killed so the explanatory note agrees with the mutation matrix.
**Row 15 is the family's one partial kill, and the reason is structural.** In
conformance/event-feed/README.md:355
- The table still says fixture 30 partially kills
follow-cross-origin-redirect, but the note below and the PR description establish that this mutation is below the poll seam and cannot be reached at tier 2. In a mutation kill matrix, an unreachable mutant is not partially killed; fixture 30 instead covers separate verdict-handling failures above the seam. Mark this row as not killed at tier 2.
This issue also appears on line 358 of the same file.
| 15 | `follow-cross-origin-redirect` (follows a 302 to a foreign Location) | 30 — **partially**, and the boundary is below the seam. See the note under this table. |
conformance/event-feed/README.md:366
- Fixture 30 cannot detect Location over-echoing. The driver reduces the scripted
LocationtoCanonicalOriginand supplies a generic safe cause (scenario_conformance_test.go:1065-1081), while this fixture asserts only the terminal reason. A connector that echoed all data it received would therefore still pass. Remove this kill claim; the separate hostile-cause unit test owns the redaction proof.
and no tier-2 harness can reach it. What fixture 30 does kill is the half above
the seam: a connector that mishandles the verdict — retrying it, classifying it
as anything but Terminal(`invalid_continuation`), or echoing more of the
`Location` than its origin — diverges on `finally` and fails.
conformance/event-feed/fixtures/30-continuation-redirect-cross-origin.json:3
- The fixture description claims it kills Location over-echoing, but the driver strips the Location to its origin before constructing
PollErrorand the fixture asserts only the terminal reason. Thus an implementation that unsafely preserves its entire seam input still passes this scenario. Remove the redaction claim or extend the contract and driver with an assertion that exposes a secret-bearing cause.
"description": "A VALIDATED same-origin `next` answers 302 with a cross-origin Location: the poll seam suppresses automatic redirect-following, and the foreign Location is Terminal(invalid_continuation) with zero egress to the foreign origin. {{NEXT:1}} substitutes same-origin, so the pre-poll validation PASSES and the second poll seam call is made (contrast fixture 26, where no request reaches the URL at all) — the redirect answer is where the per-hop rule bites. The Location host is literal, never substituted, and never served. NOTE: at tier 2 the poll lane is a seam, so the driver forms the redirect-refused verdict and the connector never sees a Location header \u2014 this fixture kills the half above the seam (mishandling the verdict: retrying it, misclassifying it, or echoing more of the Location than its origin), not follow-cross-origin-redirect itself, which lives below the seam and is proven by the Layer-1 adapter's own 302 test. See the row-15 note in README.md.",
conformance/event-feed/schema.json:906
- This contract description still overclaims redaction coverage. The tier-2 driver passes the connector only the canonical origin plus
errors.New("poll refused a redirect"), and fixture 30 checks onlyinvalid_continuation; no path/query text reaches the connector for the fixture to prove it is not echoed. Remove that item from the pinned behaviors.
"description": "Redirect on a continuation (fixture 30): the seam suppresses automatic following; a cross-origin/downgraded Location is Terminal(invalid_continuation). At tier 2 the poll lane is a SEAM, so the driver forms the redirect-refused verdict and the connector never sees a Location header: this pins the half above the seam (mishandling the verdict — retrying it, misclassifying it, or echoing more of the Location than its origin), not redirect-following itself, which lives below the seam and is proven by the Layer-1 adapter's own 302 test. No sentinel-listener obligation is imposed on the harness: the foreign origin is unreachable by construction of the harness, so a silent sentinel would be a statement about the driver rather than about the connector. See the row-15 note in README.md.",
Round 3's second suppressed finding — fixed in
|
Codex's P2 plus three Copilot findings, all taken.
* The firing-keyed advance guard read the due set from a settled state,
but nothing guaranteed the state HAD settled: an action's completion
can precede the timer arms its transition causes — expectConnect
returns when the dial is recorded, while the handshake deadline arms
on the connector's goroutine after — so an advance right behind an
action raced those arms, rejected on one schedule and accepted with
time moved past a deadline about to arm on the other. The earlier
rounds' conclusion stands (pending arms are undetectable in general);
what is new is that the DRIVER controls the preceding expectation, so
the rendezvous is AUTHORED rather than guessed: every advance must be
the scenario's first step or immediately follow expectTimers, whose
exact-set match is the settle (§23's per-state exact-set invariants
are what make a match mean settled), enforced at fixture load and
stated in the schema, README, and SPEC §23. The racing self-test
script and fixture 05 gain their rendezvous steps (the fixture's
expectState observed the state, not the timer set — the same race one
door over).
Proof, honestly: 60 rounds of the racing case never lost the race on
this machine (the arm wins natively), so the live flake has no
captured red; the deterministic red is the load rule — the
unrendezvoused script, run against the previous driver, was rejected
with the race-dependent "would fire [handshake-deadline]" verdict
where the rule now names the missing rendezvous, and on the other
schedule that same script would have advanced silently.
* Psych.parse("") returns false, not nil, so an empty registry or
allowlist crashed the duplicate-key walk on false.root. Falsy parse
results are empty documents; the shape validation speaks in its own
words. Red-proven: the empty-registry case crashed the previous
checker with the literal "undefined method 'root' for false
(NoMethodError)"; all 39 cases pass after.
* The cable-lane ceiling counts FRAMES, and now says so against the one
non-frame item: the read error that ends the pump — at most one per
attempt, because the pump exits by sending it — rides the queue or
the deferral slot with a SIZE the seam contract does not bound. The
built-in transport's errors are bounded by construction (fixed
shapes, renderings from configured text or placeholders, never server
bytes); a custom transport's error is its author's to bound. Stated,
not capped.
* The kill-matrix invariant still said every mutation is red-proven
while row 15 sits in the table pending Layer-1. It now says fifteen
of sixteen, names row 15 as the recorded exception pending the
Layer-1 adapters #819 tracks, and points at the note that accounts
for it — phrased outside the canonical grammar, so the README's
committed mention count is unchanged.
Gates: both eventfeed packages green, plain and -race;
event-feed-fixtures-check green (fixture 05 and the schema changed;
all 31 fixtures and both pin probes verify); tracking-issues self-test
39/39; live checker green (all 6 issues open); doc-constants-check
green under LC_ALL=C; gofmt clean.
…ment
Codex's re-raised P2 and Copilot's multi-document finding, both taken.
* The premise of the re-raise is half wrong and half right, and the
half matters. expectTimers is NOT an instantaneous comparison — the
step dispatches to awaitTimers, which polls under the scenario
watchdog until the outstanding set exactly matches (the snapshot
compare at stepSatisfiedLocked is the lookahead probe, not the
executor). What survives is the informationless-match class: a match
can only mean settled when the authored set could not exist BEFORE
the preceding transition was processed. Codex's own example is the
decidable case — an empty set can never contain an arm of the
preceding transition, so {"exact":{}} after a released failed mint
matches before backoff arms, exactly as if the rendezvous were
absent — and the load rule now rejects it: an advance's rendezvous
set must be non-empty (a scenario with nothing yet armed advances as
its first step). The limit no load rule can close is stated in the
schema, README, and SPEC §23 instead of implied: the authored set
must include at least one timer the preceding transition ARMS, and a
transition that only rearms a timer of the same kind and count is
invisible to set matching — such scripts use fireTimer. Red-proven:
the empty-rendezvous script against the previous driver consumed its
5s watchdog (empty match accepted, advance accepted) and failed only
for want of the fragment the rule now emits.
* Psych.parse and safe_load read only the FIRST YAML document, so
everything after a `---` separator — registry rows, allowlist
entries — silently vanished from the live check. The gate now parses
the STREAM and rejects a file holding more than one document with
its own diagnostic. Red-proven: a two-document registry hiding a row
and a two-document allowlist hiding a defective route both passed
the previous checker ("expected FAILURE but checker passed"); all 41
cases pass after the fix. The empty-file path moved with it: no
documents at all is the empty-document case, and the shape
validation still speaks.
One honest note: a single -race batch run failed once with its output
discarded by the batch's redirect; three subsequent captured runs
(-count=1 and -count=2) are clean with no data-race report, consistent
with the wall-clock-under-load flake class #794 deflaked — not
reproduced, and nothing in this commit touches connector concurrency.
Gates: tracking-issues self-test 41/41; live checker green (all 6
issues open); event-feed-fixtures-check green (schema changed); both
eventfeed packages green, plain and -race ×3; doc-constants-check
green under LC_ALL=C; gofmt clean.
…teps
Copilot's explicit-zero finding and Codex's third settle round, both
taken — and the second fix caught the first live instance of its own
bug class inside our own self-test.
* Explicit zero is not omission. The five config durations decoded into
plain int64, so {"stalenessMs":0} was indistinguishable from absence:
the range check skipped it and the driver silently used the default —
accepting a value the schema rejects (minimum 1) and substituting
another. The fields are pointers now; every SUPPLIED value is ranged,
explicit zero included, and the unmodeled backoff pair is rejected at
ANY supplied value (explicit zero used to slip that check too).
Red-proven: four zero-boundary self-tests (the three ranged
durations, one unmodeled) all failed against the previous driver.
* The settle instrument, round three. Codex's counter is real: the
welcome transition stops handshake-deadline and arms
confirmation-deadline in separate clock acquisitions, so an exact-set
match ALONE can coincide with a transient mid-surgery set. The audit
of every setState site found the codebase already holds
"surgery before announcement" deliberately at streaming,
awaiting-welcome, and awaiting-confirmation — with Backoff and
Connecting announcing before a single tail arm. That shape is what
makes the TWO-STEP rendezvous sound without touching the connector:
expectState blocks until the transition announces (bounding the
surgery), and any timer still unarmed at an announcement is exactly
what the following exact-set match waits for. Every advance must now
be first or follow expectState + expectTimers, enforced at load;
both steps block under the watchdog, so wrong authorship fails
loudly rather than diverging. The residue is stated, not implied: a
transition that announces no state change or only rearms a same-kind
same-count timer is invisible to both barriers — such scripts use
fireTimer. Schema, README, and SPEC §23 carry the rule.
The vindication: adding the state barrier to the racing self-test
immediately failed it with "outstanding timers
map[handshake-deadline:1 staleness:1], want exactly
map[handshake-deadline:1]" — the old expectTimers-only script had
been matching {handshake-deadline:1} in the pre-staleness TRANSIENT,
the precise coincidence Codex described, live in our own suite. The
script now authors the true settled set. Red-proven for the rule
itself: the barrier-less script against the previous driver was
rejected only by the race-dependent "would fire" verdict, never the
rendezvous diagnostic; fixture 05 already conformed.
Gates: both eventfeed packages green, plain and -race;
event-feed-fixtures-check green; tracking-issues live checker green
(all 6 issues open); doc-constants-check green under LC_ALL=C; gofmt
clean.
… coded
Copilot's presence saga round two and Codex's ceiling round four, both
taken.
* *int64 preserved explicit zero but read explicit JSON null exactly as
omission: {"stalenessMs":null} silently defaulted while the schema's
"type": "integer" rejects it, and null slipped the unmodeled backoff
rejection too. The five config durations are now a three-state
optionalMs — absent, null, value — via a value-type UnmarshalJSON
(encoding/json calls it for null where it short-circuits a pointer's,
which is why the pointer could not see the difference). Everything
SUPPLIED is judged: a value is ranged, null is refused with the
schema's own reasoning, and the unmodeled pair rejects any supplied
state including null. The driver keeps enforcing the schema's
judgments portably; this is the same charter that put the range check
in Go. Red-proven: five explicit-null self-tests (three ranged
durations, both unmodeled) all passed the previous driver silently.
* The ceiling's count moved because the chain moved. Re-derived at this
head, decodeMessageEvent no longer unmarshals the payload straight
into a typed struct: the exact-spelling decode (the null-vs-absent
frame-fields work) goes through a per-field map[string]json.RawMessage
first, whose field copies sum to frame scale when one field dominates.
A near-limit correlated message therefore exists as FOUR coexisting
frame-scale representations — wire bytes in the dispatching caller's
hands, parseFrame's envelope RawMessage copy, the per-field RawMessage
map, and the decoded Event's strings — until the decode returns. The
ceiling becomes (pump depth + 6 + capacity) × MAX_FRAME_BYTES, with
the four-link enumeration IN the SPEC text so the count is checkable
by reading, and the closure statement now records how the count last
moved: not by a fifth review noticing a party, but by a representation
being CODED into the chain — exactly the growth mode the rule names.
The earlier "three" was correct for the chain as it stood when it was
derived; the chain changed underneath it in a base merge, which is the
strongest argument yet for keeping the enumeration next to the number.
All four mirrored sites carry the figure.
Gates: both eventfeed packages green, plain and -race; tracking-issues
live checker green (all 6 issues open); doc-constants-check green under
LC_ALL=C; gofmt clean.
Codex's settle round four is declined with the mechanism examined and the contract strengthened; Copilot's ghost-file finding is taken. * Ghost rows. A mentions: 0 row guards a non-canonical promise by NAME alone, so a rename, delete, or typo made swept.fetch empty, 0 == 0 passed, and the row silently guarded nothing — green until the issue closed and CI blocked on a phantom promise. Every registered path must now be in the sweep's view (present, text, not excluded), failing closed with the row named. Red-proven: the ghost-file row passed the previous checker. The check also flushed out four self-test cases registering rows for files absent from their corpora — including the deleted-mention case, which now stages its file present with the sentence gone, truer to what it tests. All 42 cases pass. * The rearm rendezvous, examined rather than re-patched. Codex asks the barrier to require a NEWLY observed transition (a generation tied to the preceding action). The mechanism was worked through and it reintroduces the disease one level up: whether a state announcement lands before or after the PRECEDING expectation completes is itself a scheduling race — fixture 05's streaming announcement races its expectCheckpoint's completion — so generation-newness would make the suite's one legitimate advance flaky. Reachability was checked honestly: no committed fixture advances behind an announcement-free rearm. Fixture 05's serves are protocol frames whose receipt-side staleness resets happen-before the announcements its rendezvous observes, and its config pins staleness and repair-poll at ~11.5 virtual days against a 121-second window — the schema's own stalenessMs guidance — so old and new deadlines alike sit far outside the advance. What changes is the contract's specificity: the residue paragraph in README and SPEC now names the concrete case (a served live frame's pump-side staleness rearm), the sanctioned mitigation (override stalenessMs large, as fixture 05 does), and fireTimer for a script that wants the firing itself — a stated limit with a worked example instead of an abstract sentence. Gates: tracking-issues self-test 42/42; live checker green (all 26 registered files visible to the real sweep, all 6 issues open); doc-constants-check green under LC_ALL=C.
…t was Copilot's envelope presence gap and ceiling round five, both taken — the second by reframing the instrument rather than writing a fifth number. * assertDelayMs had every presence gap the config durations just closed: the pointer read "assertDelayMs": null as omission, silently dropping the assertion the script wrote, and inside a non-null envelope an absent or null min/max decoded to int64(0) — in range, silently a different envelope than the one authored, though the schema requires both members. The envelope is now three-state (optionalEnvelope) with three-state members (optionalMs): null refused at both levels with the schema's reasoning, an absent member a fixture error, values ranged, min ≤ max preserved — and the inner decode re-establishes DisallowUnknownFields, which the outer strict decoder cannot reach through a custom unmarshaler. Red-proven: three self-tests (null envelope, missing min, null member) all passed the previous driver silently. * The ceiling treadmill's fifth round named map bucket storage and copied keys — overhead proportional to MEMBER COUNT, which no per-representation count can bound: a frame of many tiny members inflates the maps past any chain-length figure. That is the signal the number was conflating two different questions, so the accounting now splits. RETAINED storage — what a consumer sizes against: frames and events held across blocking points, by holder — is (pump depth + 3 + capacity) × MAX_FRAME_BYTES, the enumeration that was stable before decode weights entered it. TRANSIENT decode-time allocation rides on top, per frame, in the state machine's hands alone: the representation chain plus decoder overhead proportional to member count, bounded by a small implementation-topology multiple that the SPEC deliberately does NOT publish — it moved when the exact-spelling decode was coded in, and pinning it would turn every decoder refactor into a spec change. What is contract: transients exist one frame at a time, between dequeue and decode return, so peak memory is the retained formula plus ONE frame's transient — never a per-slot or per-queue multiplier. All four mirrored sites carry the split. Gates: both eventfeed packages green, plain and -race; tracking-issues self-test 42/42; live checker green (all 6 issues open); doc-constants-check green under LC_ALL=C; gofmt clean.
Copilot's number-model finding, taken red-first. draft 2020-12's
"integer" is any number whose MATHEMATICAL value is integral — 1000.0
and 1e3 are integer instances a schema-valid fixture may carry — while
json.Unmarshal into int64 judges the spelling, so those fixtures failed
only in the Go driver, against the loader's portable-enforcement
charter. This is the repo's float-spelled-int class again, the one
FlexInt absorbs on the rich-text lane.
Every ms field now parses through one helper: json.Number first as
int64, else as float64 with an integrality check — decidable without
precision loss, since every in-range value sits far below float64's
2^53 exact-integer ceiling (maxScenarioMs ≈ 3.16e11), which the helper
states. A non-integral number is refused in the schema's terms ("is
not an integer: the schema's type is integer — a number whose
mathematical value is integral"), never in encoding/json's; a number
beyond 2^53 is refused as beyond any modeled value. optionalMs (the
five config durations and both envelope members) and advance.ms (a new
scenarioMs type, so the field-level unmarshaler leaves decodeStrict's
unknown-key strictness intact) share the helper.
Red-proven: 1000.0 and 1e3 failed to load against the previous driver,
and 1000.5 was refused with encoding/json's message rather than the
schema's — five cases across the focused spelling test and the
rejection table, all red first, all green after, with the accepted
spellings asserted to decode to exactly 1000.
Gates: both eventfeed packages green, plain and -race; doc-constants
green under LC_ALL=C; gofmt clean.
…nvert
The Lint failure and Copilot's + Codex's three threads (two of them the
same defect), folded into one commit.
* Lint: golangci-lint's unconvert flagged the int64() around a field
that is already int64 in the new spelling test. Dropped; the local
run reports 0 issues.
* Integrality is a fact about the TEXT, and the float64 detour erased
it: Float64 rounds 1000.00000000000001 to exactly 1000 — and
315575999999.99999 to exactly the maximum, which then PASSED the
range check too — before any Trunc could look. The 2^53 argument
guaranteed exact representation for integers, not for arbitrary
decimals; both threads are right. The literal json.Number preserves
is now judged exactly with big.Rat: integral iff the rational's
denominator is one, ranged via the numerator's IsInt64, no float
anywhere. An exponent is read as text first and refused for its
magnitude outside ±40, so an exponent bomb (1e999999999) is never
materialized into a billion-digit rational.
* json.Number's own Unmarshal accepts quoted numeric strings, so
{"ms":"1000"} decoded and passed every later check while the
schema's type refuses string instances. The first non-space byte
decides: a leading quote is refused as a string in the schema's
terms before any number parsing.
Red-proven: five new cases against the previous parser — the
rounding-boundary fraction and the near-maximum fraction both ACCEPTED
(the latter as the maximum itself), both quoted spellings ACCEPTED,
and the exponent bomb refused with the wrong words — alongside the
exact-maximum float spelling accepted both sides as the control. All
green after, with accepted spellings asserted to decode to the
literal's integral value.
Gates: both eventfeed packages green, plain and -race; golangci-lint
0 issues locally; doc-constants-check green under LC_ALL=C; gofmt
clean.
Both bots caught the same edge in the exponent guard: a significand can offset any exponent, so 1e44-digits x e-41 is exactly 1000 and the lexical +/-40 cap rejected schema-valid integers. Two exact string judgments replace it: the most significant nonzero digit's decimal place caps the value (above place 13 nothing fits the range), and a least significant nonzero digit below the units place is non-integral outright (decimal digits do not carry) -- which refuses 1e-999999999 without a billion-digit denominator. A 100000-character literal cap comes first so no multi-megabyte number is walked into a rational. The test table (authored before the session limit took the subagent that started this round) covers both bots' offset spellings, the fraction-led spelling of one, both bombs, and the parse-bomb literal; run against the pre-fix parser the offset cases fail to load, which is the red proof.
Copilot round: the peak equality read as a process-memory bound while the section itself allows an unbounded transport-authored error item and counts no runtime metadata -- it now says peak FRAME-PAYLOAD retention and names both exclusions. And the 100000-character literal cap was a Go-only constraint the schema never declared (1 followed by 100k zeros times e-100000 is mathematically 1): the schema's top-level description now sanctions the bound for every driver as a resource limit on spellings, not a value constraint, and the driver's comment points at the sanction.
Copilot: the platform's max integer as an exponent wrapped intLen - firstNZ + exp negative and bypassed the magnitude judgments. The exponent is bounded at +/-200000 right after parsing -- the literal cap already bounds the significand at 100000 digits, so no in-range value needs more to spell -- and both integer-extreme spellings are pinned. The negative-bomb case's expected message moves to the new guard's, which fires first.
Codex: a zero significand skipped the magnitude guard (firstNZ stays -1), handing the raw exponent onward. Zero now returns immediately -- integral however spelled, judged by the range check like any other zero -- pinned by the 0e199999 case landing in the range refusal, never a parse error.
Codex: 0e200001 is nine characters with value zero, and the exponent cap ran first. The significand scan now precedes exponent parsing entirely -- an exponent multiplies a significand, and a zero one needs no arithmetic -- pinned by the beyond-cap zero landing in the range refusal.
Settle rounds five and six, both declined as mechanism — with one sentence of the contract corrected, because Codex's new observation is true about the WORDING even though it changes nothing about the design. Round five re-litigates the stated residue verbatim: a serve in streaming whose pump-side staleness rearm races a following advance is the contract paragraph's own named example, with its two sanctioned outcomes (override stalenessMs large, as the suite's one advance does at ~11.5 virtual days against a 121-second window; or fireTimer for the firing the script actually wants). Reachability: exactly one advance exists across all committed fixtures and it follows the mitigation. Prior threads 3849496601, 3849540097, and 3849863896 carry the instrument history, including why the timer-generation barrier was examined and declined — whether an announcement lands before or after the preceding expectation completes is itself a scheduling race, so generation-newness makes the one legitimate advance flaky. Round six shows the "fails loudly" sentence overclaimed by a schedule: a pair naming the PRE-action state can pass on the schedule where the action has not yet been processed, so wrong authorship is at worst FLAKY — red whenever the transition lands first — never stably green. That is a fact about the wording, not the design: the settled guarantee always belonged to correctly authored pairs, and both documents now say exactly that instead of implying every wrong script fails every run. A loader-side transition table could reject pre-action pairs statically, but it would be a second model of the connector for the loader to drift against — a new instrument, not a cheap strengthening — and no committed script authors a pre-action pair. Gates: doc-constants-check green under LC_ALL=C; tracking-issues live checker green; no code changed, so the Go suites stand at this head's green.
Fixture 30's description and the README's row-15 note both named TestRedirectRefusalExposesOnlyTheLocationOrigin as the redaction proof's owner; the test that actually feeds the secret-bearing cause and walks the whole rendering and unwrap chain is TestRedirectRefusalRendersNoServerValue. The described obligation was met all along — only the name pointed at a ghost.
The exponent parse moves from Atoi to a fixed 64-bit ParseInt: int is 32 bits on some targets, where 1e9223372036854775807 failed as unreadable before reaching the magnitude judgment and the diagnostic changed by platform. A 64-bit overflow can only mean the exponent is past the ±200000 bound, so range errors take the bound's own verdict — pinned by beyond-int64 cases that fail against the Atoi version on every width. The schema's advance description and the rendezvous loader's comment both still claimed wrong authorship "fails loudly rather than diverging"; both now carry the README/SPEC qualification — a pre-action pair can pass on the schedule where the action is not yet processed, so wrong authorship is at worst flaky, never stably green.
… brought them under
…-numeric claim holds for every integer field The schema's top-level description promises that drivers judge each numeric literal by its exact mathematical value, so 1000.0 and 1e3 are integer instances a schema-valid fixture may carry. The Go loader honored that only for the ms-converted fields, which went through parseIntegralMs; every other schema integer — a status, an id, a capacity, a close code — decoded straight into int/int64, where encoding/json refuses 200.0 or 1e2 outright. So a fixture the schema accepts was non-portable by this driver alone, and each new integer field would have needed its own number type to opt in. Replace the per-field parser with one walk over the whole document before any typed decode: decode with UseNumber, judge every literal as text (the 100000-character cap, the exponent bound, the digit-place integrality and magnitude judgments, then big.Rat for the exact value), rewrite each integral literal to its plain integer spelling, and hand the typed script the result. The walk's magnitude cap is int64, the widest integer any schema field decodes into; the ms fields' 10-year range stays a value check after decode, since its floor differs by field. optionalMs and scenarioMs keep only what the walk cannot decide for them: the three JSON states and the refusal of a quoted number as the string instance it is. A refusal names the member path. The existing ms-spelling table keeps every case; its out-of-range spellings are now named by the walk's bound rather than by "ms value". A new table loads a status, an id and a capacity spelled 200.0 / 1e2 / 1.0 (and 2000e-1, 0.1e3) equal to their integer-spelled twin, and refuses one non-integral literal in each position naming the member.
… every integer the schema names Fixture 31's rows take the shipped nine-key poll row and eleven-key push payload the foundations now require. Every integer field in the tier-2 schema carries an explicit, portable ceiling: 9223372036854775807 for ids and epoch positions (§10's 64-bit contract), 2147483647 for counts, capacities and seconds — the narrowest native integer any driver language carries — and 65535 for a close code. The driver refused literals past int64 at load, so a schema-valid fixture could be accepted by an arbitrary-precision driver and refused by Go's; the schema is now the single judge of range, and the drivers enforce what it says rather than their own width.
Issue 798 closed on 2026-09-14 when #855 landed the generated retry loop's Retry-After clamping, so the registry's two entries for it tracked nothing and the gate this PR ships failed on its own rebase. The entries go, and MIGRATING.md's sentence that leaned on them is restated as the as-of fact it now is. Main's rewording of the two Retry-After sites the registry counted a canonical mention at is recorded as zero mentions.
…and decode the ping epoch at 64 bits An event's details object is server-owned and forwarded verbatim — the schema types it as an arbitrary object — so its numbers are whatever the server publishes and are judged by no field of this schema. The integer normalization now leaves details subtrees exactly as spelled instead of refusing a fraction or a wide integer the schema permits. The ping frame's optional epoch gains the lower bound the other integer fields already had and is decoded at a fixed 64-bit width, so a schema-valid value loads the same on every target.
…-code range, and two stale sentences The integer walk now leaves every opaque subtree exactly as spelled under one stated rule — an event's details object, and the body of a response the driver answers by status alone (every status but the 200 page and the 409 and 410 bodies it decodes) — rather than an exemption per finding. A server-owned fraction in either is schema-valid fixture data the walk has no field to judge it against. A server close code is a WebSocket status in RFC 6455's 1000–4999 range, decoded at a fixed width. The migration note no longer describes the retired two-tier Retry-After rule, the README's rendezvous example no longer attributes an announce-before-arm to Backoff (the Go reference arms first), and §23's buffer proof counts a buffered event's details bytes.
…et a resource ceiling, and close codes are the sendable ones The opaque-subtree rule compared a status literal's spelling, and map iteration could reach a body before the walk had rewritten the `200.0` beside it; it now judges the status by value. The two capacities are capped at a million — a driver may allocate them eagerly, so the largest portable integer was never a practical ceiling. A server close code is one a WebSocket stack will actually frame: 1000–1003, 1007–1011, or 3000–4999.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 358b89e94a
ℹ️ 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".
|
Follow-up for the capacity-ceiling finding in #778 (comment): #900 ranges |
Third of the split. Stacked on #705, which is stacked on #777 — review after
those. Three corrections to the tier-2 conformance family, all of them about the
suite claiming more than it proves.
1. Row 15's kill claim is partial, and the sentinel obligation is withdrawn
The mutation table said fixture 30 kills
follow-cross-origin-redirectpartlythrough "the harness obligation that the fixture's foreign origin is bound to a
sentinel listener whose any-request fails the scenario". Fixture 30's own
description said the same.
No implementation meets that obligation, and meeting it would prove nothing.
At tier 2 the poll lane is a seam: the driver receives the fixture's scripted
302 and hands the connector an already-formed redirect-refused verdict. The Go
driver says so itself — "the driver is the seam, so the foreign origin is
unreachable by construction". The connector never sees a
Locationheader andnever decides whether to follow one, so a silent sentinel is a statement about
the driver, not the connector.
That makes the mutation unreachable at this tier — it lives below the seam.
What fixture 30 does kill is the half above it: mishandling the verdict by
retrying it, misclassifying it, or echoing more of the
Locationthan itsorigin. Real zero-egress proof is the Layer-1 adapter's own 302 test, tracked
for G1b.
The fixture's description is amended too. Fixtures are merged contract and are
not edited lightly, but leaving the identical false claim in the file the other
five SDKs read would make this a half-correction. Prose only — no assertion
changed, both pin probes still verify.
This also corrects #705's own kill-matrix, which inherited the claim.
2. An advance whose window would fire any timer is rejected, in every driver
The driver ran
advancethrough the plain clockAdvance, with a commentnoting that a fixture wanting a chained firing "would pass
AdvanceSettlingtherendezvous". That is an opt-in, and the next fixture author is exactly who would
not take it.
Stress-testing the settle killed it, for reasons that are fundamental rather
than effort:
does not consume a staleness firing promptly — a window closing during a
delivery is latched and observed later, which is what §23 requires. A clock
that waited would hang against the behavior the spec mandates.
distinguishes "has not armed yet" from "will not arm".
So the reentrant clause is not settleable where the connector runs
concurrently, and the divergence is made unscriptable instead — by keying
the rule on firing, not arming. Arming happens on the connector's schedule,
so a driver could only detect it by waiting and then assuming nothing more was
coming — a heuristic wearing a MUST. What an advance would fire is decided from
the clock's own state before time moves: an
advancewhose window holds anyinitially due timer now fails, naming
fireTimeras the deterministicalternative. This is stricter than an arming rule on purpose — a firing that
arms nothing is rejected too; a script that wants that firing writes
fireTimerand names the timer. No firing in the window means nothing can bearmed in response, so an accepted advance is provably inert. Unconditional — a
schema field would let an author take the divergence rather than avoid it. Go
implements the decision as a single critical section (
AdvanceIfQuiet):due-set read and time movement under one hold of the clock's locks.
The family README carries the constraint, because it binds all six SDKs and they
are split across single-threaded and concurrent test clocks.
Self-tested both ways: the reject case (an advance across the handshake
deadline, which is due inside the window) fails, and the control (a quiet
window) still passes — without which the guard would reject every advance,
including fixture 05's.
3. Fixture 31 — a straggler below the entry page's served id
Closes the hole where a dedupe ordering live ids against the highest poll-served
id passes the whole suite.
Fixture 20 already proves a post-snapshot straggler is delivered, but its entry
page serves no events, so a highest-served-id implementation passes it too:
with nothing served the mark is unset and every id clears it. Every other
straggler in the suite is buffered pre-cut.
31 serves id 99 on the entry page and pushes 41 afterwards, in the streaming
lane. 41 must be delivered — dedupe tracks actually-delivered ids and 41 was
never served by poll — while a re-push of 99 is still suppressed, which keeps
this a dedupe fixture rather than a no-dedupe one.
The kill was verified, and the first two attempts were wrong in instructive
ways. A mutant in
admitLivesurvived — that is the buffer-admission path, anda streaming straggler never goes through it. A mutant in the shared
deliver()gate failed six fixtures, so it was broader than the hole and would not have
justified a new fixture. The mutant that matches the claim — the ordering applied
only to the streaming lane — passes all of 01–30 and is killed by 31 alone.
Verification
go build/go vet/go test -race -count=1pass; 23/23 fixtures green;5 driver self-test groups pass;
make event-feed-fixtures-checkclean with bothpin probes verified;
make go-lint0 issues.Summary by cubic
Tightens tier-2 event-feed conformance so the shared contract claims only deterministic behavior the suite can prove. Drivers now reject scheduling-dependent fixture scripts, while schema-valid numeric fixtures use portable value and range rules.
Conformance
advancemust be first or immediately followexpectStateplus non-emptyexpectTimers; windows with initially due timers are rejected, withfireTimeras the deterministic alternative.assertDelayMsdecoding preserves absent, null, and value states and rejects invalid bounds or unknown members.Checks
Written for commit 358b89e. Summary will update on new commits.
Reconciled with the shipped feed contract (5bfcd83)
Rebased onto the reconciled #705. Fixture 31's rows take the shipped nine-key poll row and eleven-key push payload, and every integer field in the schema now carries an explicit, portable ceiling (int64 for ids and epoch positions, int32 for counts/capacities/seconds, 65535 for a close code) so a schema-valid fixture is never one a driver must refuse at its own integer width — the open review thread on
schema.json.