Retire pairwise BC4↔BC5 canary; keep single-backend BC5 live canary#394
Merged
Conversation
BC5 has replaced BC4 in production (COORDINATION.md: "there is no live BC4 backend"). The pairwise forward-compat canary added in #308 captured snapshots from two live backends and asserted BC5 was an additive superset of BC4 — but with one backend both passes capture identical snapshots and the comparison passes vacuously, a nightly false green. Simplify to a single-backend live canary against production BC5. This preserves the durable value — live schema validation, extras-observed forward-compat reporting, and 4-language wire-decode consistency — while removing the now-vacuous pairwise machinery. The one live rule it carried (the `memories` additive-only waiver) is already settled by documented contract (bc3 #11628: BC5 ships `memories: []` permanently). - Workflow: single-backend BC5 canary; scope secrets behind the branch-gated `basecamp-canary` environment (clears the 7 zizmor secrets-outside-env findings); reduce to the CANARY_BASECAMP_* secret set (no BC5_HOST); run `make conformance-canary`. - Makefile: replace check-bc5-compat with conformance-canary (one conformance-live pass, BASECAMP_BACKEND=production); delete check-compare-canary and drop it from `make check`. - Delete scripts/compare-canary-runs.sh and its unit test. - Fixture: drop pairwiseAssertions on GetMyNotifications; keep liveAssertions (BC5 schema validation). - Docs: retire the pairwise narrative in CONTRIBUTING.md/COORDINATION.md and the api-gap record; keep the memories: [] contract (now settled by documentation rather than a live rule). Pairwise engine recoverable from git history (#308) if a legacy backend ever warrants restoring it.
github-actions
Bot
added
documentation
Sensitive Change Detection (shadow mode)This PR modifies control-plane files:
|
Spec Change Impact
SDKs Needing Updates
|
github-actions
Bot
added
enhancement
There was a problem hiding this comment.
Pull request overview
This PR retires the now-vacuous pairwise BC4↔BC5 live canary and simplifies the system to a single-backend live conformance canary against production BC5, while also tightening secrets handling for the scheduled workflow.
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:
- Replace the two-backend
check-bc5-compatorchestrator + pairwise compare with a single-backendconformance-canarytarget runningconformance-liveonce against production. - Remove the pairwise comparison scripts/tests and drop the
pairwiseAssertionsfixture forGetMyNotifications. - Update docs and the scheduled workflow to reflect the single-backend model and scope secrets behind a protected GitHub environment.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/api-gaps/memories-emptied-regression.md | Updates the narrative to reflect the retirement of pairwise enforcement and the settled BC5 memories: [] contract. |
| scripts/test-compare-canary-runs.sh | Deletes the regression test suite for the removed pairwise compare script. |
| scripts/compare-canary-runs.sh | Deletes the pairwise BC4↔BC5 snapshot comparison implementation. |
| Makefile | Replaces check-bc5-compat with conformance-canary and removes the compare-script regression target from make check. |
| COORDINATION.md | Updates coordination guidance to the single-backend BC5 canary and removes pairwise expectations. |
| CONTRIBUTING.md | Updates live-canary docs to the single-backend conformance-canary flow and retains pairwise as a historical note. |
| conformance/tests/live-my-surface.json | Removes pairwiseAssertions for GetMyNotifications, keeping schema validation via liveAssertions. |
| .github/workflows/live-canary.yml | Converts the workflow to single-backend BC5 and gates secrets via the basecamp-canary environment. |
💡 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: 35bc991dd7
ℹ️ 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".
- Codex (Makefile): BASECAMP_BACKEND drives the fixture-override prefix (BASECAMP_<BACKEND>_*) in conformance/runner/typescript/fixtures.ts and must be a valid Backend value (bc4|bc5). `production` broke the advertised BASECAMP_BC5_* overrides and isn't a Backend value; use `bc5` (production runs BC5). Updated the Makefile label, comments, and docs. - Copilot (live-canary.yml, CONTRIBUTING.md): reworded to state that branch-gating comes from the basecamp-canary environment's deployment-branch policy gating the whole job (a job runs only after its environment's protection rules pass), not from the secrets being environment-scoped. Kept org-level CANARY_BASECAMP_* naming per the cross-repo convention.
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.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…P_HOST
- cubic (schema.json): the pairwise compare engine is gone, but the fixture
contract still blessed `pairwiseAssertions` — a future fixture could declare
pairwise rules that validate yet silently never run. Replaced the definition
with an explicit rejection (`"not": {}`) plus a note pointing at COORDINATION.md
and the git-history recovery path (PR #308).
- Copilot (CONTRIBUTING.md): `BASECAMP_HOST` is optional only for the bare
`conformance-typescript-live` target (SDK falls back to its default origin);
the `conformance-canary` orchestrator and CI require it with no default.
Clarified the optional-env bullet to say so.
github-actions
Bot
added
documentation
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…abel; doc accuracy Secret scoping (cubic P1, Copilot x2): org/repo secrets are readable by any workflow in the repo, so the basecamp-canary environment gates this job but not the credential — only *environment* secrets scope the credential to the gated job. Reworked the workflow header/comment and CONTRIBUTING to specify environment secrets on basecamp-canary, keeping CANARY_BASECAMP_* as a cross-repo naming convention (not org-level). Qualified the branch-gate claim: repo admins can bypass environment protection rules by default (cubic P2), so it's defense-in-depth, not absolute. Backend label hijack (cubic P2): `make BASECAMP_BACKEND=bc4 conformance-canary` propagated as a command-line variable and overrode the recipe-shell env prefix, capturing production traffic under the wrong label (verified). Pin bc5 as a sub-make command-line variable, which wins over the inherited override. Comment/doc accuracy: - Makefile:526 (cubic P3, Copilot): stop overstating the Backend type (bc4|bc5|unknown); describe the canary requirement (bc5). - live-my-surface.json (Copilot): the GetMyNotifications description implied it enforces memories-empty; it only asserts liveCallSucceeds + liveSchemaValidate. Reworded to what's actually asserted, keeping the contract as context.
github-actions
Bot
added
the
documentation
github-actions
Bot
removed
the
documentation
- memories-emptied-regression.md (Copilot): bubble_ups / scheduled_bubble_ups are modeled in openapi.json, so they schema-validate normally rather than surfacing as extras-observed. Reworded so extras-observed describes future, not-yet-modeled additions. - CONTRIBUTING.md (Copilot): the Backend label bullet said (bc4|bc5) but the type also has `unknown` (the unset default). Reworded to note the unset default and drop the incomplete constraint.
github-actions
Bot
added
documentation
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
…havior cubic P3: unset BASECAMP_BACKEND (unknown) disables only the per-backend BASECAMP_<BACKEND>_* overrides; the generic BASECAMP_<FIXTURE> overrides still apply before discovery (fixtures.ts fromEnv). Reworded so 'discovery only' no longer misstates it.
github-actions
Bot
added
documentation
Comment on lines
509
to
510
| WIRE_REPLAY_DIR="$$LRD" $(MAKE) conformance-kotlin-replay | ||
| @echo "==> conformance-live: capture + replay complete for backend $$BASECAMP_BACKEND" |
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.
Why
make checkfails on 7 zizmorsecrets-outside-envfindings inlive-canary.yml(real-account secrets read in step-levelenv:without anenvironment:). Chasing that surfaced a deeper issue: the workflow (added in #308) is a pairwise BC4↔BC5 forward-compat canary, but BC5 has replaced BC4 in production (COORDINATION.md: "there is no live BC4 backend"). With one backend, both passes capture identical snapshots and the comparison passes vacuously — a nightly false green that tests nothing.The one live rule it carried (the
memoriesadditive-only waiver) is already settled by documented contract (bc3 #11628: BC5 shipsmemories: []permanently).What
Simplify to a single-backend live canary against production BC5 — preserving the durable value (live schema validation, extras-observed forward-compat reporting, 4-language wire-decode consistency) and removing the now-vacuous pairwise machinery. The zizmor fix (scope secrets behind a branch-gated
basecamp-canaryenvironment) still applies, now with a smaller secret set.basecamp-canaryenvironment (clears all 7 findings); secrets are theCANARY_BASECAMP_*set (noBC5_HOST), provisioned as environment secrets on that environment so the credential is scoped to this job (an org/repo secret would be readable by any workflow in the repo); runsmake conformance-canary.check-bc5-compat→conformance-canary(oneconformance-livepass,BASECAMP_BACKEND=production; hardenedrm -rfguard preserved); deletedcheck-compare-canaryand dropped it frommake check.compare-canary-runs.sh+ its unit test.pairwiseAssertionsonGetMyNotifications; keptliveAssertions.CONTRIBUTING.md/COORDINATION.md/ the api-gap record; kept thememories: []contract (now settled by documentation).Verification
make lint-actions(zizmor) → No findings (original 7 cleared)actionlint .github/workflows/live-canary.yml→ OKmake conformance-canarywithout secrets → exits with a clear required-env message (graceful, matching the workflow no-op gate);make -ndry-runs cleanlymake validate-api-gaps→ cleanFollow-up (parked — not in this PR)
Blocked on Basecamp personal access token support. Running the canary in CI requires an OAuth token in a secret, and Basecamp has no PAT/service-token story yet — a user OAuth token in CI is a no-go. So provisioning is parked until that lands, not merely waiting on an admin.
When it's unblocked: create the
basecamp-canaryenvironment (default-branch policy, no required reviewers) and provisionCANARY_BASECAMP_TOKEN/CANARY_BASECAMP_ACCOUNT_ID/CANARY_BASECAMP_HOSTas environment secrets on that environment — not org/repo secrets. Per review (cubic P1), only environment secrets scope the credential to this job; an org/repo secret of the same name would be readable by any workflow in the repo, so the environment would gate the job but not the secret.CANARY_BASECAMP_*stays a cross-repo naming convention (each app on its own<app>-canaryenvironment), not a shared org secret.Until then the canary no-ops gracefully (missing secrets → clean skip), so this PR is safe to land and simply leaves the canary dormant. The zizmor fix and the pairwise retirement stand on their own.
The pairwise engine remains recoverable from git history (#308) if a reachable legacy backend ever warrants restoring it.
Summary by cubic
Retires the BC4↔BC5 pairwise canary and replaces it with a single-backend live canary against production BC5. Pins the backend label to
bc5, removes the false-green nightly, clears the 7 zizmor findings, and keeps live schema validation and 4-language decode checks.Refactors
make conformance-canary; job bound to thebasecamp-canaryenvironment; secrets are environment-scopedCANARY_BASECAMP_*; noBC5_HOST; clears all 7 zizmor findings.conformance-canary; pinBASECAMP_BACKEND=bc5inside the sub-make to prevent override; removecheck-bc5-compat/check-compare-canary; updatemake check; keep the hardenedrm -rfguard.pairwiseAssertionsinconformance/schema.jsonto prevent dead pairwise rules; recovery noted in docs.scripts/compare-canary-runs.shand its unit test.GetMyNotificationsto reflect live-call + schema-validate only; clarifyBASECAMP_HOSTis optional only forconformance-typescript-live(canary/CI require it); tighten environment-gating wording; notebubble_ups/scheduled_bubble_upsare modeled (not extras-observed); document thatBASECAMP_BACKENDdefaults tounknownwhen unset and that disables only per-backend overrides — genericBASECAMP_<FIXTURE>overrides still apply; the canary requiresbc5.Migration
basecamp-canaryenvironment (default-branch policy as desired).CANARY_BASECAMP_TOKEN→BASECAMP_TOKEN,CANARY_BASECAMP_ACCOUNT_ID→BASECAMP_ACCOUNT_ID,CANARY_BASECAMP_HOST→BASECAMP_HOST.Written for commit 22fed3d. Summary will update on new commits.