BC5 readiness: cross-language wire-replay decoders#301
Conversation
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.
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.
Both come in transitively via openapi-typescript → @redocly/openapi-core (fast-uri through @redocly/ajv, brace-expansion through minimatch). npm audit on the affected versions reports: fast-uri ≤3.1.1 — high — path traversal + host confusion (GHSA-q3j6-qgpj-74h6 / GHSA-v39h-62p7-jpjc) brace-expansion 4.0.0–5.0.4 — moderate — zero-step sequence hangs the process (GHSA-f886-m6hf-6m8v) Lift both to the patched majors via overrides (alongside the existing minimatch pin). The override path is preferred over plain dependency bumps because the SDK doesn't depend on either package directly — the responsibility for forwarding the fix sits with this repo until the upstream packages cut new releases that pull the patches in transitively. Lands at the bottom of the BC5 stack so the wire-replay PRs above (#294, #301) inherit the fix on rebase.
6e78222 to
1a40114
Compare
Replay runners (PR 3) need the operation name to dispatch a decoder; without it on the snapshot top level, every replayer would have to re-parse the live fixture and rebuild the slug→operation map. Adding it once at capture time keeps the contract single-sourced.
Reads canonical wire snapshots written by the TS live canary, decodes each page through the Ruby SDK, and walks the raw JSON for required-field + extras detection. Output: per-test decode-result snapshots under <dir>/<backend>/decode/ruby/. Decoder boundary is the SDK's actual production path (JSON.parse + normalize_person_ids) — same code every paginator runs on every page, so normalize-layer regressions surface as decode_error here. The schema walker is a hand-rolled port of the TS validator's required + extras algorithm. No new gem dependencies; using json_schemer would create divergent "what counts as extra" semantics across languages against an OpenAPI doc that uses JSON Reference, not pure JSON Schema. Three startup gates: decoder coverage, snapshot completeness, snapshot recognition. Each prints actionable messages to stderr and exits 1 before any decode work; a missing top-level operation field is treated as a hard failure pointing at re-running the TS canary, not silently skipped. Mock conformance unchanged: 59 passed, 0 failed, 9 skipped.
Reads canonical wire snapshots from the TS live canary, decodes each page through the Python SDK's deserialize-and-normalize path, and walks the raw JSON for required-field + extras detection. Output: per-test decode-result snapshots under <dir>/<backend>/decode/python/. Decoder boundary is json.loads + basecamp.generated.services._base. _normalize_person_ids — the same code path every SDK request runs in production. Any normalize-layer regression (new personable shape, etc.) surfaces here as decode_error. The schema walker is a hand-rolled port of the TS validator's required + extras algorithm. No new dependencies; pyproject.toml untouched. Three startup gates: decoder coverage, snapshot completeness, snapshot recognition. Missing top-level operation is a hard failure pointing at re-running the TS canary. Mock conformance unchanged: 64 passed, 0 failed, 4 skipped.
Reads canonical wire snapshots from the TS live canary, decodes each page through the typed go/pkg/generated response types, and walks the raw JSON for required-field + extras detection. Output: per-test decode-result snapshots under <dir>/<backend>/decode/go/. Decoder boundary is generated.<Op>ResponseContent across all 10 ops — the same internal types oapi-codegen produces. Unexported mappers (projectFromGenerated, etc.) intentionally stay private; the replay runner imports the generated package directly so future maintainers don't try to "fix" the import. main()'s mode gate: when WIRE_REPLAY_DIR is set, delegate to ReplayRunner.Run() and exit; otherwise fall through to the existing mock-mode logic, which is untouched. Go's one-main()-per-package constraint makes a separate package's worth of cross-wiring a worse option than this small prefix. Walker is a hand-rolled port of the TS algorithm; map-iteration order is sorted alphabetically for stable cross-language extras-parity diffing. Three startup gates: decoder coverage, snapshot completeness, snapshot recognition. Missing top-level operation is a hard failure pointing at re-running the TS canary. Mock conformance unchanged: 68 passed, 0 failed, 0 skipped.
Reads canonical wire snapshots from the TS live canary, decodes each page through the Kotlin SDK's @serializable data classes, and walks the parsed JsonElement for required-field + extras detection. Output: per-test decode-result snapshots under <dir>/<backend>/decode/kotlin/. The replay-mode Json mirrors the existing mock-mode Json (ignoreUnknownKeys = true, coerceInputValues = false): additive BC5 fields decode without error. This is a forward-compat canary, not a strictness audit. Type-mismatches and missing required (non-nullable) fields still throw and surface as decode_error. Extras detection comes from the schema walker on the parsed JsonElement, so unknown wire fields surface even though the decoder ignores them. Four ops (assignment families + notifications) currently decode through JsonElement because the SDK doesn't yet expose typed MyAssignment / Notification models — TODO(pr3-followup) to swap to typed serializers when those models land. The walker output is the cross-language signal either way. Entry point is a separate :conformance:runReplay Gradle task, not a mode-gate in Main.kt — leaves the existing :conformance:run path untouched. No new dependencies in build.gradle.kts. Three startup gates: decoder coverage, snapshot completeness, snapshot recognition. Missing top-level operation is a hard failure pointing at re-running the TS canary. Mock conformance unchanged: 58 passed, 0 failed, 10 skipped.
Adds conformance-{ruby,python,go,kotlin}-replay targets, each with
required-env preflight (WIRE_REPLAY_DIR, BASECAMP_BACKEND). None of these
run as part of make check — they consume snapshots from the TS live
canary and are opt-in.
CONTRIBUTING gains a "Wire replay (cross-language)" subsection that
describes the two-stage flow (TS captures → other languages decode), the
three startup gates each runner enforces, and the five files to update
when adding a new operation to live-my-surface.json.
gitignore: cover conformance/runner/python/{.venv,__pycache__,*.pyc,uv.lock}
so test runs don't leave untracked files behind.
The orchestrating make conformance-live and make check-bc5-compat targets
that thread TS capture → all four replays → BC4↔BC5 comparison together
land in PR 4.
…e doc Required-field paths now use `/` as the separator (extras_seen still uses `.`) so the two streams are visually distinct in tooling and consistent across Ruby/Python/Go/Kotlin walkers. Verified end-to-end against a nested-required synthetic (Project.dock[0] missing all required fields): all four languages now emit byte-identical paths like `dock[0]/id`, `dock[0]/title`, etc. Header-comment fix: the script aborts when WIRE_REPLAY_DIR/BASECAMP_BACKEND are missing; it is not a no-op. Updated the comment to reflect the fail-fast behavior so future readers don't try to invoke it bare.
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.
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.
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)
…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.
bb85531 to
0d64a65
Compare
|
Rebased onto the rebased #294 ( |
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 code 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). Kotlin decodes fourMy*/notifications ops asJsonElementfor now.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 to prevent silent green passes.default.operationauthoritative when serializing snapshots.Written for commit 0d64a65. Summary will update on new commits.