Make the conformance gap index bite in every runner, and catch URLSession's cancellation - #568
Merged
Merged
Conversation
…lation Three follow-ups from the fourth review pass, all on code this branch added. The schema documents an omitted delayBetweenRequests index as "require the minimum on every gap", but Go, TypeScript and Kotlin still measured only gap 0 — so the documentation promised coverage three runners did not provide. All five now iterate every gap when the index is omitted. A NAMED gap that does not exist now fails in all five. The bounds check had been sitting inside each runner's "did we record two requests" guard, so a fully dropped retry left one request and the assertion evaporated instead of firing — the worst outcome for a timing pin. Ruby and Python additionally reject a negative index rather than wrapping to the end the way the per-request assertions do; there is no sensible "last gap" semantic when the point is to name a specific backoff. Swift only recognised Swift-concurrency cancellation. URLSession reports a cancelled task as URLError(.cancelled), not CancellationError, so a genuinely cancelled download was still classified a retryable network blip and spent the whole budget. Both loops now test for either shape through a shared isCancellation, pinned by a test that fails four ways against the previous commit.
jeremy
added
bug
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 7 files
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="swift/Sources/Basecamp/HTTP/HTTPClient.swift">
<violation number="1" location="swift/Sources/Basecamp/HTTP/HTTPClient.swift:43">
P3: Generated documentation now says `isCancellation` converts backoff intervals and discusses `Retry-After`, while `sleepNanoseconds` loses that safety contract. Separate the existing conversion comment so it remains attached to `sleepNanoseconds`.</violation>
</file>
<file name="kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt">
<violation number="1" location="kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt:417">
P2: An `index` of `Int.MAX_VALUE` crashes the runner instead of failing the named-gap assertion: `gap + 1` overflows before the bounds check. Compare `gap` directly with the last valid gap index.</violation>
</file>
<file name="swift/Tests/BasecampTests/DownloadTests.swift">
<violation number="1" location="swift/Tests/BasecampTests/DownloadTests.swift:852">
P3: The catch-all in this new test discards the error without asserting it is `URLError(.cancelled)`, so the test can't tell whether the cancellation shape actually surfaced or was wrapped into something else. Since `HTTPClient.isCancellation` rethrows the error raw (`.fail(error)`), asserting `catch is URLError` (or `(error as? URLError)?.code == .cancelled`) would strengthen the regression coverage with no downside.</violation>
</file>
<file name="conformance/runner/typescript/runner.test.ts">
<violation number="1" location="conformance/runner/typescript/runner.test.ts:811">
P2: The TS runner never rejects negative gap indices for delayBetweenRequests, so a negative index bypasses the bounds check (`toBeGreaterThan(index+1)` is trivially satisfied) and then computes `times[-1]` → NaN, failing with a misleading "expected delay >= ...ms at gap -1 ... got NaNms" instead of the clean out-of-range error that Go/Ruby/Python emit. This contradicts the PR's "negative gap indices are rejected" goal and makes failures hard to diagnose. Consider adding `assertion.index < 0` to the bounds check so negative named gaps fail with the same "only N request(s) were made" message.</violation>
</file>
<file name="conformance/runner/ruby/runner.rb">
<violation number="1" location="conformance/runner/ruby/runner.rb:541">
P3: The out-of-bounds diagnostics conflate two different failure reasons into one request-count message. For a negative `index` the message "but only N request(s) were made" attributes the failure to insufficient requests, which is misleading — negative named-gap indices are rejected categorically, not because of the request count. Consider emitting a distinct message for the `index.negative?` case (e.g. "gap index must be non-negative") and keeping the request-count message for the `index >= delays.length` case.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
3 issues found across 18 files
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="conformance/runner/python/runner.py">
<violation number="1" location="conformance/runner/python/runner.py:472">
P2: A `min: 0` assertion skips `check_delay_gaps`, so a named nonexistent or negative gap passes rather than exercising its required bounds check. Test for presence instead of truthiness so zero-valued minima follow the same contract as other runners.</violation>
</file>
<file name="conformance/runner/ruby/runner.rb">
<violation number="1" location="conformance/runner/ruby/runner.rb:42">
P3: A zero-request run reports `only 1 request(s) were made`, obscuring the actual failure when request construction/dispatch fails before WebMock records anything. Use a gap-based message (or pass the request count) so the diagnostic remains accurate.</violation>
</file>
<file name="kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt">
<violation number="1" location="kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt:373">
P3: Failed omitted-index request assertions now report `index null` instead of the effective default index `0`, which makes conformance failures misleading. Keep an `effectiveIndex = assertion.index ?: 0` and use it in both resolution and diagnostics.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![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: 0c945c2cbb
ℹ️ 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".
Member
Author
|
Status note from the B4 author, since work on this branch is now being carried forward elsewhere. The five findings from the previous round are fixed and resolved in Three findings from the newest round land on code I added and are not yet fixed:
The two Codex findings about For context on what deliberately stayed out of #563 and remains open: #564 (Retry-After — the HTTP-date form, the |
…annot reach
Second review round on this PR. Five findings from cubic, two from the B4
agent, and one class the round exposed that neither had named: the runners'
assertion logic had no tests of its own, so every bounds branch here was
written blind and verified only by fixtures that pass.
An index of Int.MAX_VALUE crashed the Kotlin runner instead of failing its
assertion: `gap + 1 >= size` does the addition first, so the value wraps
negative and sails through the guard into an out-of-bounds read. Go had the
identical shape. Both now compare against the gap count and never add. Ruby
and Python were already safe — they compare against the gap count directly.
TypeScript did not reject a negative gap index at all: `toBeGreaterThan(index
+ 1)` is trivially satisfied by a negative, so `times[-1]` produced NaN and a
message about a delay rather than an out-of-range index — contradicting what
the other four runners did. All five now distinguish the two failures: a
negative index is refused categorically, which has nothing to do with how many
requests were made.
A `min` of zero silently disabled the whole assertion in Python and Ruby. Both
gated the check on the value's truthiness, and `0` is falsy in Python while an
absent `min` is nil in both — so `{"type": "delayBetweenRequests", "min": 0}`
asserted nothing at all. That is the exact false-green class this PR exists to
kill, reintroduced one layer up. The default now lands INSIDE the checked
function rather than at the call site, so no caller can gate it away, and a
zero minimum still requires the gap to EXIST.
Making the Kotlin index nullable to tell "omitted" from "0" also regressed
diagnostics: requestPath, requestMethod, requestBody and the header assertions
interpolated `assertion.index` into their failure messages, which printed
`index null` for an assertion that had actually used index 0. Each branch now
resolves the index once and reports the resolved value.
The Swift cancellation test caught every error and asserted nothing about it,
so it would have passed even if the cancellation were wrapped as
BasecampError.network — the exact regression it exists to catch. It now
requires URLError(.cancelled) to arrive raw. Inserting isCancellation above
sleepNanoseconds had also orphaned a doc comment, leaving generated
documentation claiming isCancellation clamps Retry-After; the comments are
reordered onto their own functions.
Underneath all of that: the delayBetweenRequests contract is now one function
per runner (checkDelayGaps / DelayGaps.check / check_delay_gaps) with a
committed unit test beside it, wired into `make check` and CI through a new
conformance-runner-tests target. Ten cases per runner, each named for a
behavior that regressed here or on #563 — omitted index catching a LATER
failing gap, omitted index with zero gaps failing, a named gap the run never
produced failing, a negative index rejected, an enormous index failing without
overflow, and a zero minimum still requiring the gap to exist. The runners are
test harnesses, but their assertion logic is code, and its bounds branches
never execute against a fixture that passes — which is precisely how #563
shipped an assertion that vacuously passed.
One residual false-green closed with them: an omitted index over zero gaps
passed in all five runners while checking nothing. Existing fixtures happen to
pair the delay assertion with requestCount, which limited the exposure, but the
contract itself was unsound and #558 is about to build conformance claims on
it.
conformance/schema.json and SPEC §19 now state the whole contract: never
vacuous, bounds-checked unconditionally, negatives rejected.
jeremy
force-pushed
the
fix/conformance-gap-index-and-urlsession-cancel
branch
from
August 1, 2026 07:09
0c945c2 to
4af8bde
Compare
github-actions
Bot
added
the
github-actions
Contributor
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
… helper test Third review round, two findings. The out-of-range diagnostic in Python and Ruby inferred the request count as `len(delays) + 1`. That inference assumes at least one request was made, so a run that failed during construction or dispatch — before the tracker recorded anything — reported "only 1 request(s) were made" when the true count was zero, pointing at the wrong failure. Both now take the count from the tracker and report it; Go, TypeScript and Kotlin already measured request times directly and were accurate. `make conformance-runner-tests` also ran `bundle exec` in the Ruby runner before anything had installed its gems. `make conformance` reaches the helper tests ahead of the `conformance-ruby` recipe that does the install, and invoking the target on its own had no installer at all, so a clean checkout hit a Bundler error instead of the test. It now mirrors the runner recipe with `bundle install --quiet` first.
jeremy
added a commit
that referenced
this pull request
Aug 1, 2026
A verification sweep found the new runner reporting green on things it was not checking. A conformance runner that does that is worse than no runner: it converts an untested SDK into a tested-looking one. The delayBetweenRequests evaluator measured gap 0 and ignored the assertion's index, and skipped the check entirely below two requests — the identical defect #568 had just fixed in the other five. It mattered here and not hypothetically: the two download-retry fixtures this branch un-skips carry index 0 AND index 1 assertions, so the second backoff was never measured. checkDelayGaps moves into an SDK-free ConformanceSupport target with #568's semantics and its committed test roster ported; 12 of the 13 cases fail against the evaluator as shipped. An executable target carrying @main cannot host XCTest, which is why the split exists. The two download skips are gone. #563 landed the authenticated hop-1 retry, and both cases now pass live rather than being deleted on faith. Nothing capability-shaped is skipped any more: the roster is empty and the only standing exclusion is the architectural link-header branch Kotlin and TypeScript share. The rest are review findings on this branch's own code, each a way a malformed fixture could have passed: - A case with no assertions ran an operation and verified nothing. An empty assertions array is schema-legal, so the fixture gate does not catch it. - An absent mockResponses key decoded as an empty queue, which is a deliberate declaration for the HTTPS case and a malformed fixture everywhere else. The two no longer collapse. - networkError: false alongside a status slipped past the exactly-one-of backstop and was served as a plain success. The schema pins the literal true; so does the runner now. - Path parameters coerced to 0 when missing or non-integral, so the request went to a different resource and the scripted queue answered it anyway. They throw. The two timesheet arms used a `== 0` sentinel to pick between two spellings of one key, which could not tell an absent key from an id of zero; they ask for the first key present instead. - configOverrides.maxItems reaches the SDK through one dispatch arm. Any other operation would have paginated unbounded while the fixture believed it had capped the walk, so that now fails loudly instead. - headerInjected ignored its index and validated the first request, which the schema documents as index-aware and the other runners implement. - errorType: "ambiguous" could never pass despite Swift mapping it. - An explicit expected: null compared nil against the literal "null" and always failed. - The HTTPS probe routed only http:// to the crash child, but the SDK traps on every non-HTTPS scheme outside the localhost carve-out, so an ftp:// or ws:// fixture would have taken the whole run down mid-suite. - SWIFT_CONFORMANCE_NO_SKIPS was enabled by the variable's presence, so an inherited empty value silently changed coverage. It compares to "1". One of these was not a runner bug at all. The single-key array unwrap that lets list fixtures decode also rewrote {"payload_url": ["is not a valid URL"]} into a bare array, so the SDK found no field errors and reported "bad request" — a false FAIL, and the same heuristic could as easily manufacture a pass. Kotlin took the success-only status guard in #549; this port predated it and now carries it too. SPEC retires the "where a runner does not exist yet, e.g. Swift" carve-out, adds swift to the §21 gate roster, and rosters the one architectural skip.
jeremy
added a commit
that referenced
this pull request
Aug 1, 2026
Swift was the only SDK not fixture-verified; the #508/#509 retry-lifecycle bugs lived exactly where no runner watched. This adds the sixth conformance runner, executing every mock fixture in conformance/tests/ through the SDK's public Transport seam — no @testable anywhere. 136 passed, 0 failed, 1 skipped of 137; the one skip is the architectural link-header line Kotlin and TypeScript share. Seven rounds of review turned it into the opposite of what it started as. The runner shipped with the same delayBetweenRequests defect #568 had just fixed in the other five — gap 0 only, index ignored, check skipped below two requests — which mattered because the two download fixtures this branch un-skips carry index 0 AND index 1. checkDelayGaps moves into an SDK-free ConformanceSupport target with #568's semantics and its test roster ported; 12 of 13 cases fail against the evaluator as shipped. Sixteen more findings, each a way a fixture could pass while testing nothing: a case with no assertions, an absent response queue, networkError:false, path params coerced to 0, maxItems silently ignored, headerInjected ignoring its index, errorType ambiguous unreachable, expected:null always failing, the HTTPS probe crashing the run on a non-http scheme, NO_SKIPS triggering on presence, requestCount as a lower bound that made the pagination cap assertions unfailable, no path check at all, an exemption too coarse to cover request zero, path and method checked on the first hop only, an unscoped request accepted, the query string dropped so refetching page 1 looked like following the link, and an operation short-circuited before the transport skipping every invariant at once. Eight carry a red proof against the un-fixed code. One was not a runner bug: the single-key array unwrap rewrote error bodies, so a bare field map decoded as an array and the SDK reported 'bad request'. Kotlin took the success-only status guard in #549; this port predated it. The two download-retry skips are deleted and pass live under SWIFT_CONFORMANCE_NO_SKIPS=1, #563 having landed the hop-1 retry. Five shared download fixtures gained indexed requestPath assertions, so all six runners now pin the redirect target rather than only counting hops. CI: test-swift runs the runner unit tests and the conformance suite on macos-15, and is in the required Conformance Tests fan-in's needs. SPEC retires the 'where a runner does not exist yet, e.g. Swift' carve-out, adds swift to the §21 gate roster, and rosters the one architectural skip. Follow-up: #573 (Kotlin carries the same requestCount lower bound).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three review findings arrived on #563 after it had already merged. All three are on code #563 added, and all three are the same failure mode: an assertion or a classification that looks like it covers something and does not.
An omitted
delayBetweenRequestsindex was documented as "every gap" and implemented as "gap 0" in three runners. #563 taught the assertion to take a gap index and updatedconformance/schema.jsonto say that omitting it "require[s] the minimum on every gap". Ruby and Python did that. Go, TypeScript and Kotlin kept measuringrequestTimes[1] - requestTimes[0]unconditionally, so every un-indexed timing fixture —retry.json's multi-retry cases included — validated only its first backoff there. All five now iterate.A delay assertion could pass while checking nothing. Two ways. A named gap that did not exist passed silently in all five, because the bounds check sat inside each runner's "did we record at least two requests" guard — so a fully dropped retry left one request, skipped the guard, and made the assertion evaporate rather than fire. And an omitted index over zero gaps passed everywhere, because "every gap must clear the minimum" is vacuously true when there are no gaps. Both are backwards: the case a timing pin most needs to catch is the one where the delay never happened. Neither can pass now.
Swift only recognised Swift-concurrency cancellation. #563 made cancellation terminal in both retry loops via
error is CancellationError, tested with aMockTransportthat throws exactly that.URLSessiondoes not: a cancelled task surfaces asURLError(.cancelled). So the shape that actually occurs in production still fell through to the generic catch, got classified a retryable network blip, and spent the entire attempt budget re-issuing a request the caller had abandoned — while firingonRetryfor each one. Both loops now test for either shape through a sharedisCancellation.The runners had no tests of their own
Every bounds branch above was written blind. The runners are test harnesses, but their assertion logic is code, and its bounds branches never execute against a fixture that passes — which is exactly how #563 shipped an assertion that vacuously passed, through a fully green five-runner conformance run.
So the contract is now one function per runner —
checkDelayGaps/DelayGaps.check/check_delay_gaps— with a committed unit test beside it, wired intomake checkand into the four per-language CI jobs through a newconformance-runner-teststarget. Eleven cases per runner, each named for a behavior that regressed here or on #563:Int.MAX_VALUE/MaxIntfails without overflowgap + 1wrapping past the guardlen(delays) + 1misattributing the failureProofs
The Swift pin fails five ways against the parent commit (
91f62074f), withgit show 91f62074f:swift/Sources/Basecamp/HTTP/HTTPClient.swiftswapped in:Three attempts, two
onRetrys, three start/end pairs — the #508/#509 lifecycle-imbalance class, on the transport shape that actually occurs.The runner bounds tests fail three ways against the logic as committed at
07c356693, lifted verbatim into the extracted signature:The
MaxIntcase is a real panic, not a failed assertion —gap + 1wraps negative and sails through the guard into an out-of-range read.The five-runner behavior was proven before the extraction with three probe fixtures. Expected-fail in all five; actual, on merged
main:Every cell now fails. The probes were scaffolding and are not committed; the unit tests above are their permanent form.
Review rounds
Three rounds, ten threads, all resolved (Copilot errored on all three; cubic and Codex covered).
Round 2 — cubic ×5: the Kotlin
Int.MAX_VALUEoverflow (Go had the identical shape); TypeScript never rejecting a negative index (toBeGreaterThan(index + 1)is trivially satisfied, thentimes[-1]produced NaN and a message about a delay); a Swift test whose catch-all asserted nothing about the error it caught; an orphaned doc comment left claimingisCancellationclampsRetry-After; and Ruby/Python collapsing two different failures into one misleading message.Round 2, B4 agent ×2 — a
min: 0truthiness bug in Python and Ruby, which is this PR's own bug shape one layer up:0is falsy in Python and an absentminis nil in both, so amin: 0assertion silently degraded to no assertion at all. The default now lands inside the checked function, where no caller can gate it away. And the nullable Kotlin index regressingrequestPath/requestMethod/requestBodydiagnostics toindex null.Round 3 — cubic ×1 and Codex ×2: the zero-request diagnostic; the helper tests not being invoked by required CI (an unrun test is not a guard — now wired into all four per-language jobs); and
bundle execrunning before anything installed the Ruby runner's gems.Verification
make checkgreen end to end (REAL_EXIT=0): Gook, TypeScript 1051, Python 763, Ruby 1011 runs / 2285 assertions, Kotlin BUILD SUCCESSFUL, Swift 278.Conformance, from the same run — Go 131/0/2, Kotlin 132/0/1, TypeScript 164 passed / 2 skipped, Ruby 122/0/11, Python 133/0/0. Runner unit tests: Go 11, Python 12, Ruby 11 runs / 20 assertions, Kotlin 10, TypeScript 11.
Follow-ups from the #563 review that remain open and are deliberately not in here: #564 (
Retry-After: HTTP-date form, the> 0guard, and which statuses honor it) and #567 (aTransportthrowingBasecampError.networkis classified terminal). #565 is no longer among them — the 401 refresh-replay budget is settled and lands in its own PR, since it changes SDK behavior in Python and Ruby rather than runner or Swift classification logic.