BC5 readiness: pairwise BC4↔BC5 + check-bc5-compat orchestrator + scheduled CI#308
Open
jeremy wants to merge 5 commits into
Open
BC5 readiness: pairwise BC4↔BC5 + check-bc5-compat orchestrator + scheduled CI#308jeremy wants to merge 5 commits into
jeremy wants to merge 5 commits into
Conversation
Extends `conformance/schema.json` with `pairwiseAssertions[]` and four rule types: pairwiseSupersetArray, pairwiseSupersetKeys, pairwiseEqual, pairwiseDeltaAllowed. The first three enforce the additive-only invariant between BC4 and BC5 snapshots of the same operation; the fourth is a per-test allowlist with a required `reason` so a future public compatibility report can index accepted divergences. Per-backend schema validation alone can't catch this class of regression: with every new BC5 field marked optional, a hypothetical case where BC4 emits `memories: [items]` and BC5 emits `memories: []` passes each backend's schema independently. The pairwise rules close that loop. Path syntax: dotted identifiers relative to each snapshot. The empty string addresses the body root; `foo.bar` defaults to `pages[0].body.foo.bar` for single-page snapshots; explicit `pages[N].body.X` or aggregate `pages[*].body.X` are available when needed. Adds the canonical canary rule on `GetMyNotifications`: BC3 commit 64acf34 aliases BC5 memories[] to bubble_ups[] so BC4 API consumers keep receiving the same population on BC5; the pairwiseSupersetArray rule fires immediately if that alias ever drops. A pairwiseSupersetKeys rule on the body root catches any other key removal between backends.
Implements `scripts/compare-canary-runs.sh`, the executable side of the pairwise rules added in the prior commit. Reads BC4 and BC5 snapshot directories (one wire snapshot per operation, format established by the TS live-runner), looks up each test's `pairwiseAssertions` in `live-my-surface.json`, and applies the four rule types in turn. Path resolution: `""` → body root, `foo.bar` → `pages[0].body.foo.bar`, `pages[N].body.X` → explicit page, `pages[*].body.X` → aggregate across pages (collected into a list, so SupersetArray on the aggregate checks "number of pages emitting this", not a sum of inner lengths; that's an intentional semantic — multi-page aggregation is the escape hatch, single-page page-relative addressing is the common case). Exit codes: 0 clean, 1 violations, 2 operator error. Violation output groups by operation and includes the rule + display path so the specific invariant that fired is obvious without re-running. Identical account state across the two runs is mandatory for pairwise results to be meaningful; CONTRIBUTING.md documents this in the forthcoming docs commit.
Adds two opt-in targets that thread the live canary pipeline end-to-end. `make conformance-live` runs one backend's pass: TS captures canonical wire snapshots, then Ruby/Python/Go/Kotlin replay-decode them. Requires LIVE_RECORD_DIR + BASECAMP_BACKEND in the env; not invoked by `make check`. `make check-bc5-compat` is the top-level orchestrator: it wipes LIVE_RECORD_DIR (default `tmp/live-canary`), runs `conformance-live` once with BASECAMP_BACKEND=bc4, again with BASECAMP_BACKEND=bc5 against BC5_HOST, then invokes `scripts/compare-canary-runs.sh` on the two captured snapshot directories. Failure at any stage fails the whole target. The TS runner writes to `$LIVE_RECORD_DIR/$BASECAMP_BACKEND/wire/`, so one LIVE_RECORD_DIR root is reused across both passes and the per- backend distinction lives in BASECAMP_BACKEND. The plan documents this explicitly to avoid a separate-dir mental model that would diverge from what the TS runner actually does.
Adds `.github/workflows/live-canary.yml` — a nightly cron + manual workflow_dispatch trigger that runs `make check-bc5-compat`. Gated on repo secrets (BASECAMP_TOKEN, BASECAMP_ACCOUNT_ID, BC5_HOST); the job no-ops with a clear log message when the secrets aren't configured rather than failing the run, so forks and clones without canary access don't break. Sets up the full polyglot toolchain mirroring the existing Conformance Tests job — Go, Node, Ruby, Python, Java/Gradle — since the orchestrator invokes one capture + four replays. Uploads the entire `tmp/live-canary` tree as a workflow artifact with 14-day retention so failures can be inspected post-hoc without rerunning a costly live capture. CONTRIBUTING.md "Live canary" section gains a "Pairwise BC4↔BC5 comparison" subsection (the four rule types, the path syntax, the canonical `memories` example), an "Orchestrator" subsection (the make target wiring with the BC4-then-BC5-then-compare flow), and a "Scheduled CI" subsection (secret requirements + opt-in behavior). The stale "PR 4 lands the orchestrator" reference removed; the live infra is now the documented state, not a forward promise. Identical account state across the two runs is the load-bearing operational requirement and is called out explicitly in the docs — the pairwise rules assume the BC4 and BC5 backends see equivalent fixtures; without that, `unreads`-style natural drift will false-fail rules.
PR 4 is now shipping the orchestrator + pairwise comparison, so the forward-looking "PR 3 / PR 4" markers in CONTRIBUTING and COORDINATION no longer describe pending work — they describe what's in this commit's parent series. Rewrite the references to name the artifacts directly (`make conformance-*-replay`, `scripts/compare-canary-runs.sh`, `make check-bc5-compat`) so the docs stay accurate after PR sequencing shifts.
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
github-actions
Bot
added
documentation
github-actions
Bot
added
enhancement
There was a problem hiding this comment.
Pull request overview
Adds the missing “BC4 ↔ BC5 additive-only invariant” layer to the live canary by introducing pairwise assertions, a snapshot comparison script, an end-to-end Makefile orchestrator, and a nightly GitHub Actions workflow to run the pipeline.
Changes:
- Extend the conformance test schema + live surface test fixture with
pairwiseAssertionsrules (superset arrays/keys, equality, and allowlisted deltas). - Add
scripts/compare-canary-runs.shto apply pairwise rules across two backend snapshot directories. - Add Makefile orchestration targets (
conformance-live,check-bc5-compat) and a scheduled workflow that runs the canary and uploads artifacts.
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 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/compare-canary-runs.sh |
New pairwise snapshot comparator that enforces additive-only invariants between BC4 and BC5. |
Makefile |
Adds orchestration targets to run BC4 pass, BC5 pass, then pairwise compare. |
conformance/schema.json |
Adds schema support for pairwiseAssertions rule definitions. |
conformance/tests/live-my-surface.json |
Adds canonical pairwise rules for GetMyNotifications (memories array + top-level keys). |
.github/workflows/live-canary.yml |
Nightly + manual workflow to run make check-bc5-compat and upload snapshots as artifacts. |
CONTRIBUTING.md |
Documents pairwise comparison semantics, orchestrator usage, and scheduled CI behavior. |
COORDINATION.md |
Minor update reflecting that PR4 plan item is now landed here. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+157
to
+163
| # don't have to pre-rename anything if conventions shift. | ||
| BC4_SNAPSHOT="" | ||
| BC5_SNAPSHOT="" | ||
| for candidate in "$OPERATION" "$NAME"; do | ||
| safe="$(echo "$candidate" | tr '[:upper:]' '[:lower:]' | sed 's/[^a-z0-9._-]/-/g')" | ||
| [ -f "$BC4_DIR/$safe.json" ] && BC4_SNAPSHOT="$BC4_DIR/$safe.json" | ||
| [ -f "$BC5_DIR/$safe.json" ] && BC5_SNAPSHOT="$BC5_DIR/$safe.json" |
| RULE_TYPE="$(jq -r '.type' <<<"$rule")" | ||
| mapfile -t RULE_PATHS < <(jq -r '.paths[]' <<<"$rule") | ||
|
|
||
| for upath in "${RULE_PATHS[@]:-}"; do |
Comment on lines
+111
to
+117
| jq_path="$(to_jq_path "$user_path")" | ||
|
|
||
| if [[ "$user_path" == *"[*]"* ]]; then | ||
| jq -c "[ $jq_path ]" "$snapshot" | ||
| else | ||
| jq -c "$jq_path" "$snapshot" | ||
| fi |
Comment on lines
+218
to
+222
| "paths": { | ||
| "type": "array", | ||
| "items": { "type": "string" }, | ||
| "description": "Dotted-path identifiers. Empty string \"\" addresses the body root." | ||
| }, |
| @test -n "$$BASECAMP_TOKEN" || (echo "BASECAMP_TOKEN is required" >&2; exit 2) | ||
| @test -n "$$BASECAMP_ACCOUNT_ID" || (echo "BASECAMP_ACCOUNT_ID is required" >&2; exit 2) | ||
| @test -n "$$BC5_HOST" || (echo "BC5_HOST is required (BC5 backend origin, e.g. https://5.basecampapi.com)" >&2; exit 2) | ||
| rm -rf "$(LIVE_RECORD_DIR)" |
| env: | ||
| BASECAMP_TOKEN: ${{ secrets.BASECAMP_TOKEN }} | ||
| BASECAMP_ACCOUNT_ID: ${{ secrets.BASECAMP_ACCOUNT_ID }} | ||
| BASECAMP_HOST: ${{ secrets.BASECAMP_HOST }} |
Comment on lines
+430
to
+431
| — applies pairwise rules. Fails on the first violation outside | ||
| `pairwiseDeltaAllowed`. |
Comment on lines
+491
to
+492
| # vs BC5 snapshots and fails on the first violation outside pairwiseDeltaAllowed. | ||
| # |
![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: a663ad9301
ℹ️ 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 on lines
+160
to
+163
| for candidate in "$OPERATION" "$NAME"; do | ||
| safe="$(echo "$candidate" | tr '[:upper:]' '[:lower:]' | sed 's/[^a-z0-9._-]/-/g')" | ||
| [ -f "$BC4_DIR/$safe.json" ] && BC4_SNAPSHOT="$BC4_DIR/$safe.json" | ||
| [ -f "$BC5_DIR/$safe.json" ] && BC5_SNAPSHOT="$BC5_DIR/$safe.json" |
There was a problem hiding this comment.
Use the same filename sanitizer as the TypeScript live capture when locating snapshots; this code lowercases and replaces non [a-z0-9._-] with -, but persistSnapshot writes files as testName.replace(/[^a-z0-9_-]+/gi, "_") (case-preserving with _). As a result, pairwise runs cannot find the captured files, mark operations as skipped, and still exit successfully, so check-bc5-compat can report "clean" without comparing BC4 vs BC5 at all.
Useful? React with 👍 / 👎.
![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
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=".github/workflows/live-canary.yml">
<violation number="1" location=".github/workflows/live-canary.yml:121">
P2: Apply the BC4 default here instead of exporting an empty secret. An unset secret becomes `''`, so `make` never applies `check-bc5-compat`'s `BASECAMP_HOST ?=` fallback.</violation>
</file>
<file name="Makefile">
<violation number="1" location="Makefile:466">
P2: `conformance-live` validates its required snapshot env vars too late, after the live TS pass has already run.</violation>
</file>
<file name="scripts/compare-canary-runs.sh">
<violation number="1" location="scripts/compare-canary-runs.sh:161">
P0: Match the TS runner's snapshot filename format here; the current lowercase/hyphen normalization won't find the files it actually writes.</violation>
<violation number="2" location="scripts/compare-canary-runs.sh:166">
P1: Treat a missing BC4/BC5 snapshot as an error; skipping it lets `check-bc5-compat` succeed without evaluating the declared pairwise rule.</violation>
<violation number="3" location="scripts/compare-canary-runs.sh:257">
P2: Compare `pairwiseEqual` values semantically instead of as raw JSON strings, or object key-order differences will false-fail the canary.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
| BC4_SNAPSHOT="" | ||
| BC5_SNAPSHOT="" | ||
| for candidate in "$OPERATION" "$NAME"; do | ||
| safe="$(echo "$candidate" | tr '[:upper:]' '[:lower:]' | sed 's/[^a-z0-9._-]/-/g')" |
There was a problem hiding this comment.
P0: Match the TS runner's snapshot filename format here; the current lowercase/hyphen normalization won't find the files it actually writes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/compare-canary-runs.sh, line 161:
<comment>Match the TS runner's snapshot filename format here; the current lowercase/hyphen normalization won't find the files it actually writes.</comment>
<file context>
@@ -0,0 +1,288 @@
+ BC4_SNAPSHOT=""
+ BC5_SNAPSHOT=""
+ for candidate in "$OPERATION" "$NAME"; do
+ safe="$(echo "$candidate" | tr '[:upper:]' '[:lower:]' | sed 's/[^a-z0-9._-]/-/g')"
+ [ -f "$BC4_DIR/$safe.json" ] && BC4_SNAPSHOT="$BC4_DIR/$safe.json"
+ [ -f "$BC5_DIR/$safe.json" ] && BC5_SNAPSHOT="$BC5_DIR/$safe.json"
</file context>
| [ -f "$BC5_DIR/$safe.json" ] && BC5_SNAPSHOT="$BC5_DIR/$safe.json" | ||
| done | ||
|
|
||
| if [ -z "$BC4_SNAPSHOT" ] || [ -z "$BC5_SNAPSHOT" ]; then |
There was a problem hiding this comment.
P1: Treat a missing BC4/BC5 snapshot as an error; skipping it lets check-bc5-compat succeed without evaluating the declared pairwise rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/compare-canary-runs.sh, line 166:
<comment>Treat a missing BC4/BC5 snapshot as an error; skipping it lets `check-bc5-compat` succeed without evaluating the declared pairwise rule.</comment>
<file context>
@@ -0,0 +1,288 @@
+ [ -f "$BC5_DIR/$safe.json" ] && BC5_SNAPSHOT="$BC5_DIR/$safe.json"
+ done
+
+ if [ -z "$BC4_SNAPSHOT" ] || [ -z "$BC5_SNAPSHOT" ]; then
+ SKIPPED_MISSING=$((SKIPPED_MISSING + 1))
+ FILE_MISSING_DETAIL="${FILE_MISSING_DETAIL} $OPERATION
</file context>
| env: | ||
| BASECAMP_TOKEN: ${{ secrets.BASECAMP_TOKEN }} | ||
| BASECAMP_ACCOUNT_ID: ${{ secrets.BASECAMP_ACCOUNT_ID }} | ||
| BASECAMP_HOST: ${{ secrets.BASECAMP_HOST }} |
There was a problem hiding this comment.
P2: Apply the BC4 default here instead of exporting an empty secret. An unset secret becomes '', so make never applies check-bc5-compat's BASECAMP_HOST ?= fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/live-canary.yml, line 121:
<comment>Apply the BC4 default here instead of exporting an empty secret. An unset secret becomes `''`, so `make` never applies `check-bc5-compat`'s `BASECAMP_HOST ?=` fallback.</comment>
<file context>
@@ -0,0 +1,133 @@
+ env:
+ BASECAMP_TOKEN: ${{ secrets.BASECAMP_TOKEN }}
+ BASECAMP_ACCOUNT_ID: ${{ secrets.BASECAMP_ACCOUNT_ID }}
+ BASECAMP_HOST: ${{ secrets.BASECAMP_HOST }}
+ BC5_HOST: ${{ secrets.BC5_HOST }}
+ LIVE_RECORD_DIR: tmp/live-canary
</file context>
Suggested change
| BASECAMP_HOST: ${{ secrets.BASECAMP_HOST }} | |
| BASECAMP_HOST: ${{ secrets.BASECAMP_HOST || 'https://3.basecampapi.com' }} |
Comment on lines
+466
to
+469
| conformance-live: conformance-typescript-live | ||
| @echo "==> Running cross-language wire-replay against just-captured snapshots..." | ||
| @test -n "$$LIVE_RECORD_DIR" || (echo "LIVE_RECORD_DIR is required" >&2; exit 1) | ||
| @test -n "$$BASECAMP_BACKEND" || (echo "BASECAMP_BACKEND is required" >&2; exit 1) |
There was a problem hiding this comment.
P2: conformance-live validates its required snapshot env vars too late, after the live TS pass has already run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Makefile, line 466:
<comment>`conformance-live` validates its required snapshot env vars too late, after the live TS pass has already run.</comment>
<file context>
@@ -449,6 +449,62 @@ conformance-python-replay:
+# stage fail the orchestrator.
+#
+# Opt-in target: not invoked by `make check`.
+conformance-live: conformance-typescript-live
+ @echo "==> Running cross-language wire-replay against just-captured snapshots..."
+ @test -n "$$LIVE_RECORD_DIR" || (echo "LIVE_RECORD_DIR is required" >&2; exit 1)
</file context>
Suggested change
| conformance-live: conformance-typescript-live | |
| @echo "==> Running cross-language wire-replay against just-captured snapshots..." | |
| @test -n "$$LIVE_RECORD_DIR" || (echo "LIVE_RECORD_DIR is required" >&2; exit 1) | |
| @test -n "$$BASECAMP_BACKEND" || (echo "BASECAMP_BACKEND is required" >&2; exit 1) | |
| conformance-live: | |
| @test -n "$$LIVE_RECORD_DIR" || (echo "LIVE_RECORD_DIR is required" >&2; exit 1) | |
| @test -n "$$BASECAMP_BACKEND" || (echo "BASECAMP_BACKEND is required" >&2; exit 1) | |
| $(MAKE) conformance-typescript-live | |
| @echo "==> Running cross-language wire-replay against just-captured snapshots..." |
| ;; | ||
|
|
||
| pairwiseEqual) | ||
| if [ "$BC4_VAL" != "$BC5_VAL" ]; then |
There was a problem hiding this comment.
P2: Compare pairwiseEqual values semantically instead of as raw JSON strings, or object key-order differences will false-fail the canary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/compare-canary-runs.sh, line 257:
<comment>Compare `pairwiseEqual` values semantically instead of as raw JSON strings, or object key-order differences will false-fail the canary.</comment>
<file context>
@@ -0,0 +1,288 @@
+ ;;
+
+ pairwiseEqual)
+ if [ "$BC4_VAL" != "$BC5_VAL" ]; then
+ violation "$OPERATION pairwiseEqual($DISPLAY): BC4=$BC4_VAL BC5=$BC5_VAL"
+ fi
</file context>
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
Closes the BC5-readiness canary loop: PRs 1–3 built the per-backend live capture (TS) and cross-language decode (Ruby/Python/Go/Kotlin) infrastructure. PR 4 adds the pairwise BC4↔BC5 comparison that catches the additive-only invariant — and the orchestrator + scheduled CI that runs the whole pipeline end-to-end on a daily cron.
Stacked on #301 (PR 3 — cross-language wire-replay decoders). Once #301 merges, this PR retargets
main.Why pairwise
Per-backend schema validation alone can't detect a class of regression: with every new BC5 field marked optional, a hypothetical case where BC4 emits `memories: [items]` and BC5 emits `memories: []` would pass each backend's schema independently — yet that's exactly the additive-only invariant the canary should catch.
The canonical canary rule lives on `GetMyNotifications`: BC3 commit 64acf34 aliases BC5 `memories[]` to `bubble_ups[]` so BC4 API consumers keep receiving the same population on BC5. The new `pairwiseSupersetArray: ["memories"]` rule fires immediately if that alias ever drops — the SDK↔BC3 contract made concrete.
What's in this PR
Path syntax (dotted identifiers on each snapshot)
Test plan
Operational requirement
Pairwise rules assume identical account state across the BC4 and BC5 runs. The same project list, same notifications, etc. Without that, naturally-drifting collections (`unreads`, etc.) will false-fail rules. CONTRIBUTING.md calls this out explicitly; the canary needs a dedicated test account with stable, equivalent fixtures replicated to each backend before scheduled CI can pass meaningfully.
Sequencing
After this lands, the BC5-readiness canary infrastructure is feature-complete (PRs 1–4). Remaining BC5 readiness work:
Summary by cubic
Adds pairwise BC4↔BC5 comparison to the live canary, a
check-bc5-compatorchestrator, and a nightly workflow that runs the full pipeline. This flags additive-only regressions (arrays shrinking, keys disappearing, or value drift) between backends.New Features
conformance/schema.json:pairwiseAssertions[]withpairwiseSupersetArray,pairwiseSupersetKeys,pairwiseEqual, andpairwiseDeltaAllowed(requiresreason).conformance/tests/live-my-surface.json: adds the canonicalmemoriessuperset rule onGetMyNotificationsand a top-level superset-keys check.scripts/compare-canary-runs.sh: applies pairwise rules to BC4/BC5 wire snapshots; exits 0 (clean) / 1 (violation) / 2 (operator error).Makefile:conformance-live(one backend’s capture + 4 replays) andcheck-bc5-compat(BC4 pass → BC5 pass → pairwise compare)..github/workflows/live-canary.yml: nightly cron and manual runs; gated on secrets; uploads 14‑day artifacts.Migration
BASECAMP_TOKEN,BASECAMP_ACCOUNT_ID,BC5_HOST(optionalBASECAMP_HOST, defaults to https://3.basecampapi.com).make check-bc5-compat(snapshots default totmp/live-canary).Written for commit a663ad9. Summary will update on new commits. Review in cubic