BC5 readiness: cross-language wire-replay decoders#301
Open
jeremy wants to merge 19 commits into
Open
Conversation
github-actions
Bot
added
documentation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 15 files
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="kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/ReplayRunner.kt">
<violation number="1" location="kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/ReplayRunner.kt:211">
P2: Catch `Exception` instead of `Throwable` so fatal JVM errors are not swallowed as decode failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
This PR extends the BC5-readiness conformance suite with cross-language wire-replay decoders: TypeScript remains the sole live HTTP runner that captures canonical wire snapshots, while Ruby/Python/Go/Kotlin add opt-in replay modes that decode those snapshots through their SDK boundaries and emit per-test decode-result snapshots for required-field + extras detection.
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.
Changes:
- Add wire-replay runners + OpenAPI schema walkers for Ruby, Python, Go, and Kotlin, producing
<WIRE_REPLAY_DIR>/<backend>/decode/<lang>/...outputs. - Amend TS live snapshot persistence to include a top-level
operationfor replay dispatch. - Add Makefile replay targets and contributor documentation for the two-stage flow.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds conformance-*-replay targets with env-var preflight and help text updates. |
| kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/SchemaWalker.kt | Kotlin OpenAPI schema walker for missing_required + extras_seen. |
| kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/ReplayRunner.kt | Kotlin wire-replay runner that decodes snapshots and writes decode-result JSON. |
| kotlin/conformance/build.gradle.kts | Adds Gradle :conformance:runReplay JavaExec task. |
| CONTRIBUTING.md | Documents the cross-language wire replay flow and required update points when adding operations. |
| conformance/runner/typescript/wire-capture.ts | Documents and types the persisted snapshot format (operation + pages). |
| conformance/runner/typescript/live-runner.test.ts | Persists snapshots with top-level operation for replay dispatch. |
| conformance/runner/ruby/schema-walker.rb | Ruby OpenAPI schema walker used by the replay runner. |
| conformance/runner/ruby/replay-runner.rb | Ruby wire-replay runner that decodes snapshots and emits decode-result JSON. |
| conformance/runner/python/schema_walker.py | Python OpenAPI schema walker used by the replay runner. |
| conformance/runner/python/replay_runner.py | Python wire-replay runner that decodes snapshots and emits decode-result JSON. |
| conformance/runner/go/schema_walker.go | Go OpenAPI schema walker used by the replay runner. |
| conformance/runner/go/replay_runner.go | Go wire-replay runner that decodes snapshots and emits decode-result JSON. |
| conformance/runner/go/main.go | Adds a WIRE_REPLAY_DIR mode-gate to dispatch to the replay runner. |
| .gitignore | Ignores Python conformance runner virtualenv + bytecode artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
Bot
added
documentation
github-actions
Bot
added
documentation
github-actions
Bot
added
the
documentation
github-actions
Bot
removed
the
documentation
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
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="kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/SchemaWalker.kt">
<violation number="1" location="kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/SchemaWalker.kt:66">
P2: `default` is checked after generic `2xx`, which violates the expected response-schema precedence and can select the wrong schema when both exist.
(Based on your team's feedback about OpenAPI response schema resolution order.) [FEEDBACK_USED]</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Required-field paths now use `/` as the separator (walkExtras still uses `.`) so the two streams are visually distinct in tooling and consistent across Ruby/Python/Go/Kotlin walkers. New joinSlash helper sits beside joinDot; walkRequired switches to it; walkExtras is unchanged.
Three reviewer-noted hardenings on ReplayRunner + its Gradle task:
build.gradle.kts: drop the explicit environment("WIRE_REPLAY_DIR" / ...)
calls. JavaExec inherits the parent process env by default; the previous
`?: ""` fallbacks were smuggling empty strings into the child env, which
let the runner read/write relative to CWD or `<dir>/<empty>/...` instead
of failing fast on the missing-env gate.
ReplayRunner.kt main(): treat blank values as missing too (`takeUnless
{ it.isBlank() }`). Defense in depth — covers shells/launchers that pass
empty strings through even when the build script doesn't.
ReplayRunner.kt decode loop: catch Exception, not Throwable. Fatal JVM
errors (OOM, StackOverflow, LinkageError) shouldn't be silently demoted
to a decode-failure record; they should propagate. Same change applied to
the body-not-parseable-JSON catch for consistency.
…arator 3d771f0 / 04e9eea switched the required-walk path separator to `/` (matching Python/Kotlin) but left both walkers' module + function docstrings saying "dotted-path strings" for required output. Update the docs to match the code: required walks emit "owner/id"; extras walks still emit "owner.new_field". Adds an explicit conventions line calling out the two-separator split so future maintainers don't re-introduce the drift. No code changes.
The Python and Go replay runners were using truthiness/zero-value checks
to detect a missing `bodyText` field on wire snapshots, which conflated
a missing field with an empty-but-present one. A genuinely-empty body
(HTTP 204, or a 200 with `""`) would be silently replaced with a
re-serialized `body: null` → `"null"`, which `json.loads`/`json.Unmarshal`
parse successfully, turning a real decode failure into a silent green.
Python: distinguish None from empty string explicitly.
raw = page.get("bodyText")
body_text: str = raw if raw is not None else json.dumps(page["body"])
Go: change `BodyText string` to `BodyText *string` on `wirePage` so
encoding/json distinguishes a missing key (nil) from an empty value
(&""). The body-serialization fallback survives only for legacy
snapshots that lack `bodyText` entirely; new TS-canary captures always
set the field (even to `""`).
Ruby and Kotlin already handle this correctly (Ruby `""` is truthy;
Kotlin `contentOrNull` returns `""` for empty primitives, only nulling
on `JsonNull`), so they need no change.
The response-schema lookup in all five walkers (TS, Ruby, Python, Go, Kotlin) put "default" inside the explicit candidate list, which meant the actual precedence was "200, 201, 202, 203, 204, default, then any other 2xx" — but every comment described it as "200, then any 2xx, then default" (Copilot called this out on the three new walkers). Behavior now matches the comments: check the common explicit 2xx codes first, then scan for any other 2xx key, then fall back to "default" as the absolute last resort. Zero behavior change for any current OpenAPI operation in this repo — no operation combines "default" with a 2xx key outside 200..204 — but the lookup is now unambiguous if one is ever added.
Three related fixes prompted by review feedback on the wire-replay runners, plus regression tests pinning the boundary. Ruby — `SDK_DECODE` had `return nil if body_text.empty?` that silently green-passed empty wire payloads, diverging from the production SDK (`Basecamp::Http#json` calls `JSON.parse(@Body)` without an empty-body guard, so empty bodies raise `JSON::ParserError`). Drop the guard so the runner mirrors production. Python / Go — extract `_resolve_body_text` / `resolveBodyText` helpers so the empty-vs-missing distinction (already fixed inline in a1bd6df) is testable without standing up a full ReplayRunner. Regression tests assert: empty bodyText passes through as `""`, missing bodyText falls back to a serialized body, a non-empty bodyText wins over body, and the decoder errors on an empty bodyText. Ruby — minimal Minitest regression test against `SDK_DECODE` directly, adding `minitest` to the Gemfile so `bundle exec ruby` can run it. Kotlin — `coverageGate()` extracted the snapshot's `operation` via `snap["operation"]?.jsonPrimitive?.contentOrNull`, which throws if the value is non-primitive (JsonNull, JsonObject, JsonArray) and crashes the gate with a stack trace instead of emitting the intended "snapshot missing operation field" message. Cast defensively with `(snap["operation"] as? JsonPrimitive)?.contentOrNull` so a malformed snapshot fails the gate cleanly.
The conformance/runner/go module builds to a binary named `go` (after the module's last path component) when developers run `go build` locally without `-o`. The existing entry covers `conformance-runner` (a previous build-target name); add the current default so the binary doesn't show up as untracked in working trees.
jeremy
force-pushed
the
bc5-readiness-wire-replay-decoders
branch
from
909c17c to
148b021
Compare
github-actions
Bot
removed
the
documentation
Copilot flagged that the Go replay runner's coverage-gate
snapshot-recognition pass silently `continue`'d on `ReadFile` and
`json.Unmarshal` errors, so a corrupted snapshot would slip past
the gate and produce a confusing later failure. Audit revealed
parallel patterns across the four runners:
Go — silent-continue on read AND parse errors.
Kotlin — silent skip on non-JsonObject root (`as? JsonObject ?: return`);
uncaught throw on parse failure.
Ruby — uncaught throw on parse/read failure.
Python — uncaught throw on parse/read failure.
Bring all four into the same shape: emit a gate message naming the
offending snapshot file and continue to the next, so the runner exits
with `Run() => 1` and a complete list of malformed inputs rather than
crashing on the first one or — worse — silently shrinking the verified
set.
github-actions
Bot
added
the
documentation
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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/replay_runner.py">
<violation number="1" location="conformance/runner/python/replay_runner.py:129">
P2: `UnicodeDecodeError` from `read_text()` is not caught, so malformed UTF-8 snapshots crash the coverage gate instead of producing a diagnostic message. Consider catching `(OSError, UnicodeDecodeError)` together, or split the read and parse into separate try blocks.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Cubic flagged that the Python coverage-gate snapshot reader at
replay_runner.py:128 caught `OSError` and `json.JSONDecodeError`
but not `UnicodeDecodeError`. `Path.read_text()` decodes UTF-8 by
default, and malformed bytes raise `UnicodeDecodeError` — a
`ValueError`, not an `OSError` — so a corrupt snapshot crashed
the gate instead of emitting a clear diagnostic.
Widen the `except` chain to record "Snapshot X is not valid
UTF-8: …" alongside the existing read/parse messages, and add a
regression test that drives `coverage_gate()` against a tempdir
snapshot starting with `\xff\xfe`.
Cross-language audit for symmetry (verified empirically):
Ruby — `File.read` does not enforce UTF-8 at read time;
`JSON.parse` raises `JSON::ParserError` on invalid
bytes. Already caught.
Go — `os.ReadFile` returns raw bytes; `json.Unmarshal`
raises `*json.SyntaxError` on invalid bytes. Already
caught.
Kotlin — JVM `InputStreamReader` (under `File.readText()`)
silently replaces invalid bytes with U+FFFD; any
resulting non-JSON surfaces as `SerializationException`.
Already caught.
Only Python needed the fix; the other three runners reach the
same outcome via their existing handlers.
Same review pass, four small hardening fixes:
Kotlin ReplayRunner.kt:221
`decodeSnapshot` reads `page["bodyText"]?.jsonPrimitive` directly,
which throws if the underlying `JsonElement` is `JsonNull`,
`JsonObject`, or `JsonArray` — crashing the runner before it can
record `decode_error`. Mirror the operation-field defensive cast
in `coverageGate()`: `as? JsonPrimitive` first, then `contentOrNull`.
Kotlin ReplayRunner.kt:147
`wireDir.listFiles(...)` order is filesystem-dependent, making gate
diagnostics nondeterministic across machines/CI shards. Sort by
name to match the Python/Ruby/Go runners.
Go replay_runner.go:313
A snapshot like `{"operation":"GetProject"}` unmarshals cleanly
with `Pages == nil`; `decodeSnapshot` loops zero times and `Run()`
records zero failures — a silent green-pass with no decode
actually attempted. Reject missing/empty `pages` and
pages_count-vs-len(pages) mismatches in `readSnapshot`. Adds four
table-style regression tests (missing pages, empty pages,
mismatched count, matching count).
Go replay_runner_test.go:58
Dead `*json.SyntaxError` variable (`var se ...; _ = se`) discarded
without use. Drop the line and the now-unused `encoding/json`
import.
Verified locally:
go test ./conformance/runner/go/... → 8 pass
make conformance-go → 68 / 0 / 0
make conformance-kotlin → 58 / 0 / 10 (skips pre-existing)
github-actions
Bot
removed
the
documentation
…pread
Copilot flagged that `persistSnapshot` built the payload as
`{ operation, ...snapshot }`. `WireSnapshot` doesn't currently
carry an `operation` key, so the behavior is correct today —
but the spread order means a future addition to `WireSnapshot`
with a same-named field would silently override the parameter
the caller passed in. Flip to `{ ...snapshot, operation }` so
the explicit argument wins.
Single one-liner; no behavior change for any existing snapshot.
github-actions
Bot
added
the
documentation
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.
Summary
Companion to #294 (TS canonical wire-capture canary). Adds wire-replay mode to the Ruby, Python, Go, and Kotlin runners so they consume the canonical wire snapshots TS captures against a live backend, decode each page through their SDK, and walk the raw JSON for required-field + extras detection. Swift remains excluded from the live canary surface.
The architecture pairs with §5e/5f of the BC5-readiness plan: TS is the only live HTTP runner; everyone else replays from disk. Decode-result snapshots land at
<dir>/<backend>/decode/<lang>/withschema_version: 1.Per the cross-SDK omnibus pattern, this is one PR with one commit per SDK plus a TS amendment commit (operation field on snapshots) and one infrastructure commit (Make targets + CONTRIBUTING + gitignore).
What ships
operationat the top level so replay runners dispatch without re-parsing the live fixture. Backwards compatible at the type level; runtime gates fail loud if a pre-PR3 snapshot shows up.JSON.parse + normalize_person_ids).json.loads + _normalize_person_ids. Same code path every SDK request runs in production.generated.<Op>ResponseContenttypes fromgo/pkg/generated. Mode-gate prepended to existingmain(); mock-mode body untouched.@Serializabledata classes via the sameJson { ignoreUnknownKeys = true }the mock runner uses. Separate:conformance:runReplayGradle task; existing:conformance:rununtouched. Four ops decode throughJsonElementbecause typedMyAssignment/Notificationmodels don't yet exist (TODO noted).conformance/runner/typescript/schema-validator.ts's required + extras algorithm. No new library dependencies in any language; usingjson_schemer/jsonschema/santhosh-tekuri/networkntwould create divergent extras semantics across runners.operationis a hard failure pointing at re-running the TS canary.conformance-{ruby,python,go,kotlin}-replay, each with required-env preflight (WIRE_REPLAY_DIR,BASECAMP_BACKEND). None run as part ofmake check.Verification
Cross-language walker parity confirmed end-to-end against synthetic snapshots covering all 10 ops in
live-my-surface.json:The synthetic
GetProject {}body produces byte-identicalmissing_required([app_url, created_at, id, name, status, updated_at, url]) across all four languages.schema_versionlock:All four mock baselines preserved exactly: Ruby 59, Python 64, Go 68, Kotlin 58 (counts match those documented in #294's plan).
Coverage gate failure modes verified by injection (missing decoder, missing snapshot, unknown operation, pre-PR3 snapshot lacking
operationfield) — each emits an actionable message pointing at the right side to fix (TS dispatch, fixture, or this runner).Test plan
make conformance-rubyshows 59/0/9make conformance-pythonshows 64/0/4make conformance-goshows 68/0/0make conformance-kotlinshows 58/0/10make conformance-typescriptpasses (TS amendment is back-compat)WIRE_REPLAY_DIR=<dir> BASECAMP_BACKEND=bc4 make conformance-<lang>-replayfor each language fails preflight when env unset, succeeds against TS-captured snapshotsoperationfield) with a clear "Re-run the TS live canary" messageSequencing
PR 4 (pairwise BC4↔BC5 comparison +
check-bc5-compatorchestrator + scheduled CI) consumes both the TS canary's<backend>/wire/snapshots and these decode-result snapshots; it lands separately.Summary by cubic
Adds cross-language wire-replay for Ruby, Python, Go, and Kotlin using TS-captured snapshots. TS now persists
operation; runners decode via production paths, report missing required fields and extras, and write per-test results for BC5 readiness.New Features
operationon wire snapshots for replay dispatch.<dir>/<backend>/wire/*.json, decode via SDK production paths, and write to<dir>/<backend>/decode/<lang>/withschema_version: 1. Walkers emitmissing_required(slash paths) andextras_seen(dot paths).operation). Make targets:conformance-{ruby,python,go,kotlin}-replay(Kotlin uses:conformance:runReplay). CONTRIBUTING and.gitignoreupdated.Bug Fixes
bodyText; Python/Go distinguish missing vs emptybodyTextso empty bodies trigger decode errors.operationandbodyText, narrows decoder catch toException, and sorts snapshot files for stable messages.pagesandpages_countmismatches (with regression tests), preventing silent green passes.default.operationauthoritative when serializing snapshots.Written for commit bb85531. Summary will update on new commits.