Skip to content

Event feed conformance: three corrections to what tier 2 claims (3/3) - #778

Open
jeremy wants to merge 71 commits into
event-feed-go-connectorfrom
event-feed-conformance-driver
Open

Event feed conformance: three corrections to what tier 2 claims (3/3)#778
jeremy wants to merge 71 commits into
event-feed-go-connectorfrom
event-feed-conformance-driver

Conversation

@jeremy

@jeremy jeremy commented Aug 18, 2026

Copy link
Copy Markdown
Member

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-redirect partly
through "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 Location header and
never 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 Location than its
origin. 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 advance through the plain clock Advance, with a comment
noting that a fixture wanting a chained firing "would pass AdvanceSettling the
rendezvous". 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:

  • Waiting for a firing to be consumed deadlocks. The connector deliberately
    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.
  • Waiting for the follow-on arming requires knowing one is coming. Nothing
    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 advance whose window holds any
initially due timer now fails, naming fireTimer as the deterministic
alternative. This is stricter than an arming rule on purpose — a firing that
arms nothing is rejected too; a script that wants that firing writes
fireTimer and names the timer. No firing in the window means nothing can be
armed 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 admitLive survived — that is the buffer-admission path, and
a 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=1 pass; 23/23 fixtures green;
5 driver self-test groups pass; make event-feed-fixtures-check clean with both
pin probes verified; make go-lint 0 issues.


Summary by cubic

Makes tier‑2 event‑feed scripts deterministic and schema‑faithful. Advances now fail if they would fire any timer, every advance is an authored two‑step rendezvous, and all ms fields are parsed as integers by mathematical value under strict limits. Adds fixture 31 to prove streaming stragglers below a served id are still delivered. Scopes the Go‑only cable‑lane memory ceiling, and enforces a tracked‑issues registry with a detector and committed mention counts. Hardens numeric parsing across platforms and clarifies that a mis‑authored rendezvous can be flaky rather than always failing.

  • Advance fails if any timer is initially due; use fireTimer for deterministic firings.
  • Every advance must immediately follow expectState then a non‑empty expectTimers.
  • All ms fields accept integer‑valued numbers (e.g. 1000.0, 1e3) and refuse strings, non‑integral numbers, oversized literals/exponents, or values beyond 10 virtual years; values are modeled as int64 end‑to‑end.
  • Numeric parsing now uses exact rational checks and fixed 64‑bit exponent parsing, with platform‑invariant diagnostics and a 100000‑character literal cap.
  • assertDelayMs and config durations are three‑state (absent/null/value); null is refused, both min and max are required, and unknown members are rejected.
  • Adds fixture 31 to prove a post‑snapshot streaming straggler below a served id is delivered while true duplicates are suppressed.
  • Publishes the GO reference’s cable‑lane retained ceiling and states peak payload equals retained plus one frame’s transient decode; transports must bound reads.
  • Enforces spec/tracking-issues.yml with a sweep detector, duplicate‑key guards, multi‑document and empty‑file checks, and committed mention counts; scripts/check-known-defect-issues-open gains offline self‑tests and clearer diagnostics.
  • Clarifies docs: the zero‑egress redirect proof is a Layer‑1 obligation tracked in #819; a mis‑authored rendezvous is flaky rather than deterministically failing.

Migration

  • Place expectState then a non‑empty expectTimers immediately before any advance, or replace the advance with fireTimer.
  • Ensure all ms values are integers by value within 10 virtual years; assertDelayMs must include both min and max; null is invalid.

Written for commit e7b115f. Summary will update on new commits.

Review in cubic

Copilot AI balanced review requested due to automatic review settings August 18, 2026 21:12
@jeremy jeremy added the conformance Conformance test suite label Aug 18, 2026
@github-actions github-actions Bot added the go label Aug 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread go/pkg/basecamp/eventfeed/scenario_conformance_test.go Outdated
Comment thread conformance/event-feed/README.md
Copilot AI review requested due to automatic review settings August 18, 2026 21:30
@jeremy
jeremy force-pushed the event-feed-conformance-driver branch from 039aad6 to 4c8446c Compare August 18, 2026 21:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 NewTimer ran. A timer can fire and be rearmed under the same name before settle samples it (the connector does same-name rearming, e.g. repair-poll at catchup.go:852), leaving the map equal to before and allowing the scheduling-dependent advance. Conversely, removal alone is labeled as an arm. Track a monotonic timer-arm/firing generation in feedtest.Clock and 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 advance as 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

@jeremy

jeremy commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

Round summary — 3 findings, 3 fixed, 0 open

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

# Source Finding Outcome
1 thread + suppressed scenario_conformance_test.go:494 the advance guard measured firings, not arming fixed ab41f581a
2 thread + suppressed README.md:360 the sentinel obligation was withdrawn in prose only fixed be5a1c7e0
3 suppressed README.md:131 schema.json's advance still instructs drivers to implement the unscriptable clause fixed be5a1c7e0

The one that mattered

Finding 1 was a test that could not fail, and the review's sharper half was in the suppressed block rather than the thread. Clock.advance removes a fired timer from the registry before any observer runs, so comparing Outstanding() snapshots reports an ordinary expiry as an arm — and, symmetrically, cannot see a same-name rearm at all, which is exactly what the connector does with repair-poll/poll-retry.

Proven by mutation rather than argued: with the connector's backoff arming deleted, the old self-test still passed (REAL_EXIT=0); with the fix in place the same mutation fails (REAL_EXIT=1). The test had been passing on the handshake deadline's own removal. The guard now reads feedtest.Clock.ArmCount(), the monotonic timer-creation count the clock already maintained for tie-breaking.

Findings 2 and 3 are one mechanism — a correction that reached the README but not schema.json, which the README names as the contract. I swept the family for the whole class rather than patching the two that were reported; it closes at exactly three sites, all fixed.

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 h.mu and consulted by recorders running on other goroutines, with atomic handoff between adjacent steps. Its naive spelling has a failure direction worse than the bug it fixes: a set/clear leaves a window between adjacent steps matching no expectation, which turns a correct connector red on a scheduling accident. That needs its own design, its own per-class red-proof matrix, and a positive argument that no conforming connector can be reddened — none of which belongs inside a PR titled "three corrections to what tier 2 claims" without changing what this PR is.

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. Outstanding() failed because a reconstructable snapshot collapses distinct histories onto one value. deliveredAt is the same kind of witness and collapses the same way — which is why it orders saves against deliveries and nothing else. Count the event, don't reconstruct it from state.

Stack / rebase

Rebased twice — event-feed-go-connector moved under me mid-session (3eaf48a39cf1d06b96). Now on the settled head, and GitHub's computed diff agrees with the local one at 460 additions / 8 files (it was 20,936 / 62 against the stale base, over Copilot's 20,000-line review limit).

cf1d06b96 landed in my file territory (scenario_conformance_test.go, scenario_selftest_test.go); its terminal-contract guards and this PR's advance guard are disjoint and the full suite is green on the merged result.

Gates

go test ./pkg/basecamp/eventfeed/... REAL_EXIT=0 · gofmt 0 · go vet 0 · golangci-lint 0 issues · make doc-constants-check REAL_EXIT=0 — all under LC_ALL=C. make event-feed-fixtures-check REAL_EXIT=0 under LC_ALL=C.UTF-8, stated explicitly because make conformance is broken under LC_ALL=C (#774).

No finding in this round argued from bc3 server behaviour. I re-checked anyway, since the pins moved today: between 8be5c67de5 and 6adb6050cd the only change on the event-feed contract surface is a 12-line authorization-scope addition to stream_tickets_controller.rb (read-scoped tokens may now mint). events_channel.rb, event/feed.rb, event/feed/position.rb and doc/api/sections/event_feed.md are untouched, so the poll envelope, the absent-next predicate and the disconnect reasons this PR pins have not moved. The README's verified at bc3 8be5c67de5 header is an as-of fact and correctly does not advance; PR-T's true-up remains the place that re-verifies.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Advance delivers the due timer, the connector handles it on another goroutine; the driver only polls ArmCount until scenarioWatchdog expires (scenario_conformance_test.go:508-519). If that goroutine is delayed past the watchdog, the scenario passes and can arm a timer after advance returns—the exact cross-language divergence this rule is meant to eliminate. A timeout cannot prove that no arm is coming; either reject every advance that 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"}}`

Copilot AI review requested due to automatic review settings August 19, 2026 02:37
@jeremy
jeremy force-pushed the event-feed-conformance-driver branch from be5a1c7 to 81414d7 Compare August 19, 2026 02:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot AI review requested due to automatic review settings August 19, 2026 02:40
@jeremy

jeremy commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

Stopping on the advance guard — third round, one class

The suppressed comment on the latest review (scenario_selftest_test.go:365, against cfd9b3d4e) is correct, and it is the third round on this guard. Per AGENTS.md that is a signal about the instrument, not a queue, so I am naming the pattern and the real question rather than writing the third patch.

The three rounds

  1. 1de86dfe7 — the guard is introduced, comparing snapshots of Outstanding().
  2. This round — the snapshot is the wrong instrument. A firing removes its timer before any observer runs (so an expiry reads as an arm) and a same-name rearm leaves the set identical (so repair-poll rearms are invisible). Fixed in 28d03cc18 by counting timer creations.
  3. This round, suppressed — a timeout cannot prove that no arm is coming. If the connector's goroutine is starved past scenarioWatchdog, the guard passes and the arm lands afterwards.

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 question

The 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 actionable

Copilot'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 advance in all 31 fixtures, and its window fires nothing:

PROBE advance 121000ms: outstanding before=[staleness repair-poll] after=[staleness repair-poll] armCount 6->6

Both timers sit beyond the window, exactly as the fixture intends. fireTimer already covers the "make this timer fire" need deterministically, so the coarser rule costs authors nothing they cannot express.

Why I am not making the change here

It rewrites the normative rule in README.md and schema.json for all six SDKs, forbidding a fixture shape that is legal today, and it inverts a self-test I added this round. That is a contract decision, not a driver fix, and it is the third round on one class — so it wants a decision rather than my patch.

What is shipping meanwhile, stated plainly

The guard as it stands is conservative in the safe direction. ArmCount is monotonic and rises only on a real NewTimer, so the guard cannot reject a well-formed script; its only defect is the missed detection round 3 names, under a starvation longer than the watchdog. It is strictly better than what it replaced, which was reporting expiries as arms and missing rearms entirely. So this PR is not shipping a wrong-direction failure — it is shipping a detector that is sound but not complete, with the completeness question written down here rather than left in a suppressed block.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Location to CanonicalOrigin and 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 PollError and 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 only invalid_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.",

Copilot AI review requested due to automatic review settings August 19, 2026 02:44
@jeremy

jeremy commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

Round 3's second suppressed finding — fixed in 961143eee

The suppressed comment on README.md:356 is right, and it is the sharper kind of finding for this PR specifically: the correction landed and its own summary was left behind.

Adding row 16 left the heading reading fifteen against sixteen rows, and withdrawing row 15's whole-kill claim left the introduction still saying every mutation is shown red against at least one fixture before it counts. So a document whose entire job is to state precisely what tier 2 proves was overstating the suite in two places at once — including in the first line a reader sees.

Both now say so, with the exception stated at the top rather than left to be discovered in row 15:

One row is an exception, and it is the reason this heading is worth reading twice. Row 15's mutation lives BELOW the tier-2 seam, so this suite kills only the half above it […] A matrix that counted the partial row as a kill would be making exactly the class of claim this family exists to check.

I swept the family for the rest of the class before fixing, as with the schema corrections: these two lines are the only places the matrix's size or completeness is asserted anywhere under conformance/event-feed/.

The count is now checked mechanically rather than by eye — the sixteen rows are contiguous 1..16 and the heading agrees. Gates re-run green: make event-feed-fixtures-check REAL_EXIT=0 (LC_ALL=C.UTF-8, per #774), make doc-constants-check REAL_EXIT=0 (LC_ALL=C).

This one is a plain fix, not an escalation — it is a different mechanism from the advance-guard class in the comment above, which stays stopped pending a decision on the predicate.

@jeremy

jeremy commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@codex review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

Suppressed comments (1)

go/pkg/basecamp/eventfeed/scenario_fixture_test.go:140

  • The exponent bound rejects schema-valid zero spellings such as 0e200001. This literal is only 8 characters and has mathematical value 0, so it is valid for assertDelayMs.min/max; however, the code checks the exponent before the zero short-circuit. Detect an all-zero mantissa before parsing/bounding the exponent so only nonzero values need the ±200000 guard.
	mant, exp := lit, 0
	if i := strings.IndexAny(lit, "eE"); i >= 0 {
		mant = lit[:i]
		e, err := strconv.Atoi(strings.TrimPrefix(lit[i+1:], "+"))
		if err != nil {
			return 0, fmt.Errorf("%s is not a number this driver can read", lit)
		}
		// Bound the exponent before any place arithmetic: at the platform's
		// integer extremes, intLen - firstNZ + exp wraps and the magnitude
		// judgments below judge garbage. The literal cap above bounds the
		// significand at 100000 digits, so no in-range value needs an
		// exponent beyond ±200000 to spell.
		if e > 200000 || e < -200000 {
			return 0, fmt.Errorf("%s is beyond any modeled ms value", lit)
		}
		exp = e
	}

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2d2497355c

ℹ️ 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".

Comment thread go/pkg/basecamp/eventfeed/scenario_fixture_test.go Outdated
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.
@jeremy

jeremy commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: a840022276

ℹ️ 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".

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

…ent-feed-conformance-driver

* origin/event-feed-go-connector:
  Event feed: three store identities that could still drift, closed
@jeremy

jeremy commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@codex review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 6b755f7f73

ℹ️ 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".

…ent-feed-conformance-driver

* origin/event-feed-go-connector:
  Event feed: say why the raw read failure is not the dial path, and pin it
@jeremy

jeremy commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@codex review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.

Comment thread go/pkg/basecamp/eventfeed/scenario_fixture_test.go

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 54049d7e83

ℹ️ 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".

Comment thread go/pkg/basecamp/eventfeed/scenario_fixture_test.go
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.
@jeremy

jeremy commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: a8e69626c3

ℹ️ 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".

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

conformance/event-feed/fixtures/30-continuation-redirect-cross-origin.json:3

  • This description points to a nonexistent test. The corresponding proof is TestRedirectRefusalRendersNoServerValue in go/pkg/basecamp/eventfeed/catchup_test.go:1475, so the fixture currently misidentifies its supporting test.
"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). {{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) \u2014 the redirect answer is where the per-hop rule bites. What tier 2 verifies is the per-hop refusal DECISION \u2014 the driver classifies the scripted Location through the SHIPPED predicate (checkContinuation, via a test export), never an ad-hoc re-implementation \u2014 and the loop's response: the continuation terminal, the socket explicitly closed, and no further seam calls. That the adapter's HTTP client makes zero requests to the refused URL is below the poll seam and owned by Layer-1 adapter conformance \u2014 a recorded obligation, not an existing proof: those adapters are still pending (tracked in #819), so follow-cross-origin-redirect itself remains NOT killed at tier 2 (row 15). The Location host is literal, never substituted, and never served. Redaction is deliberately not claimed here: the driver reduces the Location to its origin best-effort (an unreducible Location carries none) before the connector runs, so no path or query text exists for a connector to over-echo; TestRedirectRefusalExposesOnlyTheLocationOrigin owns that proof. See the row-15 note in README.md.",  "config": {

conformance/event-feed/README.md:458

  • This names a test that does not exist. The redaction proof is TestRedirectRefusalRendersNoServerValue in go/pkg/basecamp/eventfeed/catchup_test.go:1475; using the current name makes the documented proof impossible to locate.
belongs to `TestRedirectRefusalExposesOnlyTheLocationOrigin`, which feeds a

@jeremy

jeremy commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: a8e69626c3

ℹ️ 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".

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

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.
@jeremy

jeremy commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@codex review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

conformance/event-feed/schema.json:1213

  • The schema still says a wrongly authored rendezvous “fails loudly rather than diverging,” but the updated README (lines 203–206) and SPEC (lines 3913–3915) acknowledge that a pre-action pair can pass when the action has not yet been processed, making it flaky. This stronger schema claim therefore still promises more than the driver proves; mirror the same pre-action/flakiness qualification here.
      "description": "Advance virtual now by ms. The normative virtual-advance algorithm (README) fires due timers in deadline order, re-evaluating after each fire, with timers scheduled during the advance whose deadlines land inside the window also firing and ties breaking by creation order. That reentrant clause is UNSCRIPTABLE wherever the connector runs concurrently with the driver, so no fixture may rely on it: every driver MUST REJECT an advance whose window would fire ANY timer, naming fireTimer as the deterministic alternative. The test is what would FIRE, decided from the clock's state before time moves — not what gets armed, which happens on the connector's schedule and can only be sampled. An advance that fires nothing never wakes anything, so it cannot cause an arm; that is what makes the check complete rather than probabilistic. A firing that replaces nothing is rejected too. And an advance is deterministic only from a scripted rendezvous point: an action's completion can precede the timer arms its transition causes, so every advance must be the scenario's first step or immediately follow a TWO-STEP rendezvous: expectState, then expectTimers, enforced at load (an empty expectTimers set is rejected with it — it orders nothing). Neither step alone settles: a set match can coincide with a transient mid-surgery set (the welcome transition stops handshake-deadline and arms confirmation-deadline in separate clock acquisitions), and an announcement can precede a tail arm. Together they do: the announcement bounds the surgery, and any timer still unarmed at the announcement is exactly what the exact-set match then waits for. Both block under the watchdog, so wrong authorship fails loudly rather than diverging. A transition that announces no state change — or only rearms a timer of the same kind and count — is invisible to this rendezvous, and a script that would advance behind one uses fireTimer instead.",

go/pkg/basecamp/eventfeed/scenario_fixture_test.go:154

  • strconv.Atoi makes exponent handling platform-width again. On a 32-bit Go target, values such as 1e9223372036854775807 fail here before reaching the shared magnitude check, producing the wrong diagnostic and breaking the new platform-independent self-test/loader contract. Parse into a fixed-width type, reject out-of-range exponents as beyond the modeled value, then convert only after the ±200000 bound.
		e, err := strconv.Atoi(expText)

go/pkg/basecamp/eventfeed/scenario_fixture_test.go:483

  • This comment makes the same absolute claim the PR corrects elsewhere. A stale pre-action state/timer pair can match before the action is processed, so a wrongly authored rendezvous does not fail on every schedule; it can be flaky. Keeping the implementation comment aligned with the README/SPEC avoids overstating what this structural check guarantees.
	// blocks fail loudly on the watchdog when the authored state or set is
	// wrong; nothing diverges silently.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 38c28cc54a

ℹ️ 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".

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.
@jeremy

jeremy commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@codex review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: e7b115f12f

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conformance Conformance test suite github-actions Pull requests that update GitHub Actions go python Pull requests that update the Python SDK ruby Pull requests that update the Ruby SDK spec Changes to the Smithy spec or OpenAPI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants