BC5 readiness: pairwise BC4↔BC5 + check-bc5-compat orchestrator + scheduled CI#308
Open
jeremy wants to merge 28 commits into
Open
BC5 readiness: pairwise BC4↔BC5 + check-bc5-compat orchestrator + scheduled CI#308jeremy wants to merge 28 commits into
jeremy wants to merge 28 commits into
Conversation
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.
![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".
![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
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
jeremy
added a commit
that referenced
this pull request
May 27, 2026
… rule Post-launch reconciliation for the pairwise BC4<->BC5 canary. Clears the 14 review threads on #308 and corrects the memories rule to match what BC5 master actually shipped. scripts/compare-canary-runs.sh: - P0 (codex/cubic): match the TS live runner's snapshot filename scheme exactly -- sanitize the test name with [^A-Za-z0-9_-]+ -> "_" (case preserved), not lowercase/hyphen/operation-first. The old form matched no files, so check-bc5-compat could report clean without comparing anything. - P1 (cubic): a declared pairwise test missing a snapshot on either backend is now a hard error (exit 2), not a silent skip-and-pass. - empty `paths` loop (copilot): guard ${#RULE_PATHS[@]} instead of "${RULE_PATHS[@]:-}" so an empty array can't run against the body root. - pairwiseEqual (cubic): compare via jq deep-equality so object key-order differences don't false-fail. - exit-code contract (copilot): snapshot-read jq failures remap to the documented exit 2 instead of leaking jq's status under set -e. conformance/schema.json: pairwiseAssertions[].paths gains minItems:1. Makefile: - rm -rf "$(LIVE_RECORD_DIR)" guarded against empty / "/" / "..". - conformance-live validates LIVE_RECORD_DIR/BASECAMP_BACKEND before the TS live pass (invoked from the recipe after the guards) rather than after it. - check-bc5-compat comment: "reports all violations", not "fails on first". - new check-compare-canary target, wired into `make check`. .github/workflows/live-canary.yml: default BASECAMP_HOST in the workflow expression so an unset optional secret can't clobber the Makefile default. CONTRIBUTING.md: align the "all violations" wording; reframe the memories example to the waiver pattern. memories canary rule (conformance/tests/live-my-surface.json): the prior "BC3 commit 64acf34 aliases memories[] to bubble_ups[]" claim is counterfactual -- that commit does not exist, and BC5 master shipped `json.memories []` while BC4 four still populates it. The pairwiseSupersetArray rule now states the intended invariant and is waived by a temporary pairwiseDeltaAllowed entry pointing at spec/api-gaps/memories-emptied-regression.md (added on the reconciliation branch); remove the waiver once BC3 #10947 (which carries `json.memories @bubble_ups`) merges. scripts/test-compare-canary-runs.sh: network-free regression coverage for the filename scheme (incl. proof the old form finds nothing), missing-snapshot hard-fail, memories-waiver scoping, pairwiseEqual key-order, and the empty-paths guard.
github-actions
Bot
added
the
documentation
github-actions
Bot
removed
the
documentation
There was a problem hiding this comment.
3 issues found across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
jeremy
added a commit
that referenced
this pull request
May 27, 2026
The waiver `reason` strings read "BC4 four still populates it", which scans as a typo to anyone without the post-launch topology in hand (BC4 is the `four` branch). Reword to "BC4 (the four branch)" in the live-my-surface.json description + pairwiseDeltaAllowed reason and the CONTRIBUTING.md example. Addresses the Copilot re-review on PR #308.
github-actions
Bot
added
the
documentation
github-actions
Bot
removed
the
documentation
jeremy
added a commit
that referenced
this pull request
May 28, 2026
…s branch The brief's SDK-absorption runbook read as if the pairwiseDeltaAllowed waiver and conformance/tests/live-my-surface.json were present here, but those canary files are on PR #308's branch (this branch is the registry record only). Reword to state #308 introduces the waiver and that it gets removed from live-my-surface.json once #308 and BC3 #10947 both land. Also reframe the "no Smithy change" bullet to "no structural change" — this PR does realign the memories doc comment (documentation, not a shape change).
jeremy
added a commit
that referenced
this pull request
May 28, 2026
COORDINATION.md said the pairwiseSupersetArray rule "is waived via pairwiseDeltaAllowed", which reads as if the canary surface exists on this branch — it does not (conformance/tests/live-my-surface.json is on PR #308). Reword to attribute the rule + waiver to #308's branch, matching the same fix already made in the memories-emptied-regression brief.
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.
github-actions
Bot
added
documentation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09cc06d1ac
ℹ️ 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".
schema.json requires a reason on pairwiseDeltaAllowed, but the compare script also runs standalone (check-bc5-compat, the scheduled workflow) with no schema-validation step in front of it — a reason-less waiver still flattened into ALLOW_PATHS and suppressed enforcement unaudited. The extraction now rejects waivers without a non-empty string reason (exit 2). Scenario R pins it; scenario P's assertion follows the combined waiver error message.
A LIVE_RECORD_DIR (or, theoretically, a mktemp result) beginning with '-' would be parsed as an rm option; end option parsing explicitly.
The compare + harness scripts guard for it with a clear error; the Makefile comment now says so up front for contributors running make check on stock macOS bash.
…waiver reason * Each snapshot's recorded .operation must match the test entry's operation before comparison — a stale or overwritten file with the right name exits 2 instead of being compared as this test's capture. Scenario S pins it. * conformance/schema.json aligns with the runtime waiver guard: the pairwiseDeltaAllowed conditional now sets minLength 1 on reason, so an empty-string reason fails schema validation the same way it fails the compare script. TS conformance suite re-run green (84 passed).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a23eb9b14
ℹ️ 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".
schema.json's minItems:1 on paths doesn't run in front of standalone invocations (check-bc5-compat, the scheduled workflow). An empty waiver paths array is functionally harmless (it waives nothing) but is always a fixture mistake — reject it with the same exit-2 operator error as the other waiver shape violations. Scenario T pins it.
… message * The rm -rf guard now refuses '..' only as a PATH SEGMENT (leading, interior, trailing, or the whole path) instead of any substring, so benign names like tmp/live-canary..pr308 are allowed while every traversal form still refuses. Patterns unit-verified for all six shapes. * The regression harness preflights jq with the same clear error as the compare script instead of failing mid-setup with a raw command-not-found. * snapshot_read_error's hint no longer blames only malformed JSON — an invalid jq path expression built from a rule's path entry lands in the same handler.
The aggregate is a list, not an object — the rule flags it as an invalid target rather than evaluating it. Say so and point rule authors at pages[N].body.X for keys checks on paginated endpoints.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbfbb9de6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The aggregate handling keyed off any '[*]' substring, broader than the documented pages[*].body.X form — an undocumented items[*].foo would stream through jq with undefined comparison semantics instead of surfacing the fixture mistake. Both the read and the supersetArray totals branch now see only validated leading-pages[*] paths; anything else exits 2 as an unsupported path. Scenario U pins it.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
* CONTRIBUTING's prerequisites now list the shared tooling make check needs beyond per-SDK toolchains: jq, and bash >= 4.4 for the pairwise-canary scripts (macOS /bin/bash is 3.2 — brew install bash; the scripts fail fast with that exact hint). * Scenario U2 pins that a second '[*]' after a valid leading pages[*] is rejected as unsupported (the guard already covered it).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a37b58c37
ℹ️ 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".
* An absolute LIVE_RECORD_DIR equal to the repo checkout (or any of its ancestors, e.g. LIVE_RECORD_DIR=$PWD or $HOME) passed the guards and rm -rf'd the working tree before the canary ran. A new prefix check refuses any path the checkout lives under; absolute paths elsewhere (/tmp/canary) and in-checkout subdirs still work. Patterns unit-verified for checkout, $HOME, /tmp, relative, and in-checkout-absolute shapes. * The harness's mktemp gets an explicit template — current macOS accepts a bare -d (empirically verified here), but older BSD variants insist on a template and it costs nothing.
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 case where BC4 emits
memories: [items]and BC5 emitsmemories: []passes each backend's schema independently — yet that's exactly the additive-only invariant the canary should catch. This is not hypothetical: BC5mastershipped precisely this regression (see below).The canonical canary rule lives on
GetMyNotifications. The intended invariant: BC5memories[]should remain a superset of BC4's. But BC5mastershippedjson.memories []while BC4 (thefourbranch) still populates it — a confirmed live regression, not the once-assumed alias (there is no commit 64acf34). The fix is written but unmerged: BC3 #10947 carriesjson.memories @bubble_ups(head9dc63e2e). So thepairwiseSupersetArray: ["memories"]rule is currently waived by a temporarypairwiseDeltaAllowed: ["memories"]entry pointing atspec/api-gaps/memories-emptied-regression.md; remove the waiver (restoring the hard-fail) once #10947 merges and provenance is repinned.What's in this PR
conformance/schema.json—pairwiseAssertions[]with four rule types:pairwiseSupersetArray,pairwiseSupersetKeys,pairwiseEqual,pairwiseDeltaAllowed(with requiredreasonso a future public compatibility report can index accepted divergences).conformance/tests/live-my-surface.json— thememoriesSupersetArray rule + a top-level SupersetKeys rule onGetMyNotifications.scripts/compare-canary-runs.sh— applies pairwise rules to a pair of snapshot directories. Exit 0 clean / 1 violation / 2 operator error.Makefile—make conformance-live(one backend's capture + 4 replays) andmake check-bc5-compat(BC4 pass + BC5 pass + pairwise). Both opt-in; not inmake check..github/workflows/live-canary.yml— nightly cron +workflow_dispatch, gated on repo secrets (no-ops with a log message when missing rather than failing the run). Uploads snapshots as a 14-day artifact so failures can be inspected post-hoc without rerunning.CONTRIBUTING.md— new "Pairwise BC4↔BC5 comparison", "Orchestrator", and "Scheduled CI" subsections under "Live canary".Path syntax (dotted identifiers on each snapshot)
""(empty string) addresses the body root.foo.bardefaults topages[0].body.foo.bar— the common single-page case.pages[N].body.Xaddresses a specific page.pages[*].body.Xaggregates across pages into a list (each page's value becomes one element; useful when you want a per-page count, not a sum of inner lengths).Test plan
jq empty conformance/schema.json— clean JSONjq empty conformance/tests/live-my-surface.json— clean JSONbash -n scripts/compare-canary-runs.sh— shell syntax cleanpairwiseSupersetArrayflags BC5 memories shorter than BC4pairwiseSupersetKeysflags BC5 missing a top-level key BC4 haspairwiseEqualflags value divergencepairwiseDeltaAllowedwaives a flagged pathpages[*].bodyaggregation collects per-page valuesmake -n check-bc5-compatresolves correctlyactionlint .github/workflows/live-canary.ymlcleanzizmor .github/workflows/live-canary.ymlcleanmake validate-api-gapscleanmake check-bucket-flat-paritycleanOperational 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:
scripts/detect-api-gaps.sh+ weekly cron workflow (api-gap detection tooling; validation already shipped in PR 1).main).Summary by cubic
Adds pairwise BC4↔BC5 comparison to the live canary with a
check-bc5-compatorchestrator and nightly CI. Hardens the compare script (filename matching, pagination totals, strict types, snapshot/test integrity) and now rejects reason‑less waivers, empty waiver paths, and any[*]use that isn’t the leadingpages[*]segment (including a second[*]); docs also call outjqand bash ≥ 4.4 prerequisites.New Features
conformance/schema.json:pairwiseAssertions[]withpairwiseSupersetArray,pairwiseSupersetKeys,pairwiseEqual, andpairwiseDeltaAllowed(requires a non‑emptyreason).scripts/compare-canary-runs.sh(exit 0 clean / 1 violation / 2 operator error), Make targetsconformance-liveandcheck-bc5-compat, and.github/workflows/live-canary.yml(nightly + manual, defaultsBASECAMP_HOSTtohttps://3.basecampapi.com, uploads 14‑day artifacts).conformance/tests/live-my-surface.json: keeps thememoriessuperset rule onGetMyNotifications, waived viapairwiseDeltaAllowedwhile the confirmed BC5 regression is tracked; adds a top‑levelpairwiseSupersetKeysrule.Bug Fixes
pages[]with per‑page keys); enforces snapshot.operationmatches the test; preserves trailing body‑root""paths; requires waiverpathsas non‑empty arrays of strings and a non‑emptyreason; materializesjqreads to surface parse errors; requires bash ≥ 4.4;pairwiseEqualuses deep equality;pairwiseSupersetKeysstrictly types targets (null counts as empty, non‑object is a violation);pages[*]compares total items across pages; rejects any[*]that isn’t the leadingpages[*](including a second[*]) with an unsupported‑path error; read‑error message now also covers invalid jq path expressions.conformance-liveabsolutizesLIVE_RECORD_DIR; safe cleanup guard strips all trailing slashes and refuses empty,.,/, or any..path segment; usesrm -rf --; defaults resolved in the recipe shell; addscheck-compare-canaryand wires it intomake check.scripts/test-compare-canary-runs.shadds scenarios for filename scheme, missing snapshots, body‑root paths, malformed/non‑array tests files, paginated totals, waiver type/paths/reason validation, per‑page shape checks, rule type/paths validation, snapshot/test operation mismatch, and unsupported[*]usage including a second star; preflightsjqwith a clear error.pages[*]guidance with implementation; clarifypairwiseSupersetKeysdoes not acceptpages[*]aggregates (usepages[N].body.X); documentjqand bash ≥ 4.4 prerequisites for the pairwise scripts.Written for commit 0a37b58. Summary will update on new commits.