diff --git a/.claude/skills/catalog-deploy/SKILL.md b/.claude/skills/catalog-deploy/SKILL.md index c941a7bf4be..b041f718abb 100644 --- a/.claude/skills/catalog-deploy/SKILL.md +++ b/.claude/skills/catalog-deploy/SKILL.md @@ -18,6 +18,8 @@ The catalog realm (`/catalog/`) serves boxel-catalog's files, and its cards impo - **Before the release** (**Deploy the pinned catalog before the release**). The check runs against the boxel production still runs, so this ships the catalog changes the new boxel needs (a boxel pull request that says `Merges after: cardstack/boxel-catalog#N`), and refuses when the range has a catalog change that needs the new boxel. A refusal or failure here is only a notice and doesn't hold back the release. - **After the release** (**Deploy the pinned catalog after the release**). This ships the rest, such as catalog changes pinned by the boxel pull request that brings the code they need. If it fails or refuses, the job goes red: production then runs the new boxel against an older catalog. + This deploys the whole catalog at the pin, so boxel's Lint Catalog lints the whole catalog at the pin against every boxel change, not only the subset files boxel's tests load. A boxel change that breaks the catalog pins the head of the catalog pull request that fixes it, which merges right after the boxel one (the `catalog-pairing` skill). Once that pull request has merged, `dispatch-catalog-deploy.sh` deploys its merge commit in place of the pinned head, with a notice naming both, whatever the merge method. The head branches off before production's catalog, so the check would refuse it as diverged, and after a squash or rebase it isn't on `main` at all. + When production's catalog is already at the pin, both do nothing. When a single range holds catalog changes the new boxel needs _and_ ones that need the new boxel, the run before the release refuses, so the first kind reaches production only after the release. - **Ahead of boxel, by hand.** A catalog change that needs nothing new from boxel doesn't have to wait for a boxel deploy. Run **Deploy to production** with `revision` empty, which deploys catalog `main`'s head, or with a catalog `main` SHA. Start it from Discord (see below) or from boxel-catalog's Actions tab. diff --git a/.claude/skills/catalog-pairing/SKILL.md b/.claude/skills/catalog-pairing/SKILL.md index 25ddef050f7..30ddd0158e4 100644 --- a/.claude/skills/catalog-pairing/SKILL.md +++ b/.claude/skills/catalog-pairing/SKILL.md @@ -1,6 +1,6 @@ --- name: catalog-pairing -description: How a boxel pull request and a boxel-catalog pull request that depend on each other are paired — a `Merges before:` / `Merges after:` line in each description naming the other, declared from both sides — and which merges first. Covers when a change needs a pair (a boxel change the catalog's cards must follow, such as renaming or removing a host tool, command, base export or type the catalog imports; or a catalog definition boxel's tests need first), the declaration syntax and the rules a declaration must meet, merge order in each direction and in the three-PR flow, what boxel's Lint Catalog does with the pairing (it lints the paired catalog pull request's head and refuses to let catalog main be left broken behind a pull request that isn't ready), and how to read its failures. Use before opening or editing any pull request in cardstack/boxel-catalog; before opening a boxel pull request that changes something boxel-catalog's cards import or that pins or tests a catalog change; whenever Lint Catalog fails; and whenever a check says `pairing:`, "doesn't name … back", "waiting on cardstack/boxel-catalog#… to merge", "is not approved yet", "pair the two", "Merges before" / "Merges after", "which predates it", or "can't contain it yet". +description: How a boxel pull request and a boxel-catalog pull request that depend on each other are paired — a `Merges before:` / `Merges after:` line in each description naming the other, declared from both sides — and which merges first. Covers when a change needs a pair (a boxel change the catalog's cards must follow, such as renaming or removing a host tool, command, base export or type the catalog imports; or a catalog definition boxel's tests need first), the declaration syntax and the rules a declaration must meet, merge order in each direction and in the three-PR flow, what boxel's Lint Catalog does with the pairing (it lints the paired catalog pull request's head, refuses to let catalog main be left broken behind a pull request that isn't ready, and fails on any error the change adds to the whole catalog at the revision boxel pins), why a boxel-first change pins its catalog pull request's head, and how to read its failures. Use before opening or editing any pull request in cardstack/boxel-catalog; before opening a boxel pull request that changes something boxel-catalog's cards import or that pins or tests a catalog change; whenever Lint Catalog fails; and whenever a check says `pairing:`, "doesn't name … back", "waiting on cardstack/boxel-catalog#… to merge", "is not approved yet", "pair the two", "Merges before" / "Merges after", "which predates it", or "can't contain it yet". --- # Pairing boxel and boxel-catalog pull requests @@ -11,7 +11,7 @@ boxel-catalog's cards import boxel (host tools, `@cardstack/base`, runtime types ## When a change needs a pair -- **A boxel change the catalog has to follow.** Renaming or removing a host tool or command (`@cardstack/boxel-host/tools/*`), a base export, a field or a type that catalog cards import, or tightening a type or lint rule they trip. The boxel pull request merges first, and the catalog pull request that adapts the cards merges right after it. +- **A boxel change the catalog has to follow.** Renaming or removing a host tool or command (`@cardstack/boxel-host/tools/*`), a base export, a field or a type that catalog cards import, or tightening a type or lint rule they trip. The boxel pull request merges first, and the catalog pull request that adapts the cards merges right after it. The boxel pull request also pins the catalog pull request's head (see [The pin covers the whole catalog](#the-pin-covers-the-whole-catalog)). - **A catalog change boxel's tests need.** A new or changed definition in the catalog test subset (the `catalog-test-subset` skill). The catalog pull request merges first, and the boxel pull request that re-pins and tests it merges after. - **Both at once: the three-PR flow.** A catalog definition that needs new platform code, and whose boxel tests need the new definition. Platform pull request A merges, then catalog pull request C, then boxel pull request B, which re-pins and tests. C is tested against B's head, and that head needs A's platform code. So B carries A by merging A's branch into it and following A until A merges. B may target `main` with A merged in, or be stacked on A (below) and be retargeted to `main` once A merges. Stacked, B can't pass the catalog-main gate as a `Merges before` side until it's retargeted, since merging it lands it on A's branch, not `main`. @@ -63,10 +63,25 @@ Edit a description with `gh pr edit --repo --body-file ` **boxel-catalog's Boxel Test Subset** pairs by branch name: it tests a catalog pull request against the boxel branch with the same name, or against boxel `main` if there is none. So give the two branches the same name as well, and push the boxel branch first. -**boxel's Catalog Test Subset** pin check needs no declaration. The pinned commit identifies its catalog pull request, and the check fails until that pull request merges. +4. It lints the whole catalog at the revision `packages/catalog/test-subset.json` pins against the change, and compares it with the base branch's pin linted against the base branch. Every error the change adds there fails, paired or not (the failure title is `catalog lint (pin)`). The pairing only decides which revision the error tells you to pin instead. When the pin is the revision step 2 or 3 already linted, that recording is reused. + +**boxel's Catalog Test Subset** pin check reads the pairing too. It compares only the subset files at the pin with catalog `main`, so it passes a pin from a catalog pull request that touches no subset file without looking at the pull request at all. When a subset file at the pin differs from `main`, the pinned commit identifies its catalog pull request, and the check fails until that pull request merges, with one exception. The exception is a pin at the head of the catalog pull request this change says `Merges before:`, while that pull request is open, ready for review and approved, neither side is stacked, and no `Merges after:` pair of this change is still open. A pin at any other commit of that pull request fails, because the head is what lands. The check re-runs when the description is edited, but not when the catalog pull request is approved, so re-run it by hand then. Either way, the approval gate for a boxel-first pair is Lint Catalog's catalog-main step (step 3), which fails until the catalog pull request is approved. + +## The pin covers the whole catalog + +Manual Deploy [boxel] to production deploys the catalog at the revision the deployed boxel commit pins (the `catalog-deploy` skill). So the pin has to mean "this boxel works with the whole catalog at this revision", not only with the subset files boxel's tests load. Lint Catalog's step 4 enforces that. + +For a boxel-first pair, this means the boxel pull request pins the catalog pull request's head. The old pin is a catalog revision that the boxel change breaks, and production would deploy the two together. The steps: + +1. Open the catalog pull request with the fix, paired with the boxel pull request in both descriptions. +2. In the boxel pull request, set `revision` in `packages/catalog/test-subset.json` to the catalog pull request's head, run `pnpm --dir packages/catalog catalog:test-subset`, run the manifest's tests, and commit the new pin. Re-pin each time the catalog pull request gets new commits. +3. Get the catalog pull request approved, then re-run the boxel pull request's Lint Catalog, and its Catalog Test Subset check if that one ran. +4. Merge the boxel pull request, then the catalog pull request right after it. Once it has merged, the lockstep deploy (`dispatch-catalog-deploy.sh`) deploys its merge commit in place of the pinned head, whatever the merge method: the head branches off before production's catalog, which the deploy check refuses, and after a squash or rebase it isn't on catalog `main` at all. The merge commit carries the same change and everything `main` had when it merged. + +Before the boxel pull request merges, the pin is a commit catalog `main` doesn't have yet. Until the catalog pull request merges, any other boxel pull request that touches the manifest fails `--check-pin`, waiting on it. That's one more reason to merge the catalog pull request right after. ## Merging -- **Boxel first:** merge the boxel pull request only once its catalog pull request is approved and neither one is stacked, then merge the catalog one right after. Until the catalog one merges, catalog `main` fails against boxel `main`, and boxel-catalog CI is red for everyone. +- **Boxel first:** the boxel pull request pins the catalog pull request's head. Merge the boxel pull request only once its catalog pull request is approved and neither one is stacked, then merge the catalog one right after. Until the catalog one merges, catalog `main` fails against boxel `main`, and boxel-catalog CI is red for everyone. - **Catalog first:** merge the catalog pull request, re-pin the boxel pull request onto catalog `main` (`pnpm --dir packages/catalog catalog:test-subset --bump`), re-run its checks, then merge it. Lint Catalog enforces the re-pin: it fails the boxel pull request that declares `Merges after:` the catalog pull request until that has merged and the pin contains its merge commit, since production deploys the catalog at the revision boxel pins. - **Each environment gets the catalog on its own schedule.** Staging syncs catalog `main` on every catalog merge. Production gets only what boxel-catalog's "Deploy to production" deploys: the revision boxel pins, before and after Manual Deploy [boxel] to production releases, or a catalog `main` revision someone deploys by hand. That deploy refuses while a catalog pull request in it says `Merges after:` a boxel pull request production doesn't run. So a catalog change that needs boxel code declares its pair even when Lint Catalog wouldn't require one. The `catalog-deploy` skill covers the deploy and its refusals. diff --git a/.claude/skills/catalog-test-subset/SKILL.md b/.claude/skills/catalog-test-subset/SKILL.md index fc2329d41f2..9591c322343 100644 --- a/.claude/skills/catalog-test-subset/SKILL.md +++ b/.claude/skills/catalog-test-subset/SKILL.md @@ -75,11 +75,14 @@ What the pin may point at depends on what it is for. This repo's `PreToolUse` ho **For a commit you push.** Every test stack and CI job fetches the pinned files from GitHub, so the pin must be a commit there: - catalog `main` (`--bump` pins its HEAD), when the boxel change uses the definitions as `main` has them; -- or the head of the catalog PR this boxel PR depends on, when its tests need that change (step 3 below). Re-pin each time the catalog PR gets new commits. +- or the head of the catalog PR this boxel PR depends on, when its tests need that change (step 3 below). Re-pin each time the catalog PR gets new commits; +- or the head of the catalog PR this boxel PR merges before, when the boxel change breaks the catalog and that PR fixes it (the `catalog-pairing` skill's _The pin covers the whole catalog_). + +The pin covers more than the subset. Production deploys a boxel commit with the whole catalog at the revision it pins, so boxel's Lint Catalog lints the whole catalog at the pin against every change, and fails on any error the change adds there, including errors in files that aren't in the subset. Never commit a pin that exists only in a local catalog checkout (the check fails with "is not a commit in cardstack/boxel-catalog"), and never commit a pin you haven't run the manifest's `tests` consumers against. -**For merge.** Every subset file at the pin must match catalog `main` byte for byte. `--check-pin` enforces this on every boxel PR that changes the manifest. +**For merge.** Every subset file at the pin must match catalog `main` byte for byte. `--check-pin` enforces this on every boxel PR that changes the manifest. The one exception is a pin at the head of the catalog PR this boxel PR says `Merges before:`, while that PR is approved and neither side is stacked. That catalog PR merges right after the boxel PR, and the pin then matches `main`. The lockstep deploy then deploys that PR's merge commit in place of the pinned head, whatever the merge method. - `waiting on cardstack/boxel-catalog# to merge`: the pin is a commit in an open catalog PR. Merge that PR first, then re-run the check. A pin from a catalog PR passes this check once the PR merges, whether as a merge commit, a squash or a rebase. A boxel PR that declares `Merges after:` that catalog PR still has to re-pin with `--bump` (below). - "main has changed … since": catalog `main` has a newer version of a subset file. Re-pin with `--bump`, run the sync, and re-run the consumers. If the message names a boxel PR that already pins a matching revision, merging boxel `main` after it lands works too. @@ -105,16 +108,17 @@ A catalog change to a subset file reaches deployments as soon as the catalog mer ## What keeps the pin, the manifest and the served files in sync -| Check | Where it runs | What it catches | -| --------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `--check-pin` | boxel's `Catalog Test Subset` workflow, on every PR; it checks the pin only when the PR changes the manifest, the sync script or the workflow | a subset file at the pin that differs from catalog `main`: a pin still waiting on its catalog PR, a pin main has moved past, or a commit that isn't on GitHub | -| closure check | every sync, and the same workflow | a subset file importing something a subset-only stack can't serve | -| per-file hashes in the marker | every sync (including each stack start) | a `packages/catalog/test-subset/` file edited after the sync wrote it | -| `setupCatalogTestSubset(hooks)` | every host and realm-server test module that uses the subset | a served subset at a different revision or file list than the manifest (or the host build); a clone copy that differs from the pin, or from the local checkout the sync read; a served file edited in place (skipped, with a warning, on a stack whose sync predates the per-file hashes) | -| `catalog-test-subset-consumers-test.ts` | realm-server CI | a test using the subset that the manifest's `tests` doesn't list, or a listed entry that matches no such test | -| `lint:no-catalog-subset-copies` | CI Lint, as part of `pnpm lint` in `packages/base` | a file in `packages/base` with a subset file's name, or declaring a class a subset file exports (the class half needs the pinned files; when they can't be fetched it warns and checks file names only) | -| `Boxel Test Subset` | boxel-catalog, on PRs and pushes to `main` that touch a subset file | a catalog change that breaks boxel's tests, on the same-named boxel branch or boxel `main` | -| `Catalog Test Subset Consumers` | boxel, on PRs and pushes to `main` touching that workflow, the manifest, the sync, the subset test helpers, or the CI boot path | a boxel change that would break the run boxel-catalog calls, tested against the pinned revision | +| Check | Where it runs | What it catches | +| --------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `--check-pin` | boxel's `Catalog Test Subset` workflow, on every PR; it checks the pin only when the PR changes the manifest, the sync script, `pairing.ts` or the workflow | a subset file at the pin that differs from catalog `main`: a pin still waiting on its catalog PR (unless it is the head of the approved catalog PR this change merges before), a pin main has moved past, or a commit that isn't on GitHub | +| Lint Catalog, at the pin | boxel's `Lint Catalog` workflow, on every PR and push to `main` | a boxel change that adds a lint or type error anywhere in the catalog at the pinned revision, subset or not | +| closure check | every sync, and the same workflow | a subset file importing something a subset-only stack can't serve | +| per-file hashes in the marker | every sync (including each stack start) | a `packages/catalog/test-subset/` file edited after the sync wrote it | +| `setupCatalogTestSubset(hooks)` | every host and realm-server test module that uses the subset | a served subset at a different revision or file list than the manifest (or the host build); a clone copy that differs from the pin, or from the local checkout the sync read; a served file edited in place (skipped, with a warning, on a stack whose sync predates the per-file hashes) | +| `catalog-test-subset-consumers-test.ts` | realm-server CI | a test using the subset that the manifest's `tests` doesn't list, or a listed entry that matches no such test | +| `lint:no-catalog-subset-copies` | CI Lint, as part of `pnpm lint` in `packages/base` | a file in `packages/base` with a subset file's name, or declaring a class a subset file exports (the class half needs the pinned files; when they can't be fetched it warns and checks file names only) | +| `Boxel Test Subset` | boxel-catalog, on PRs and pushes to `main` that touch a subset file | a catalog change that breaks boxel's tests, on the same-named boxel branch or boxel `main` | +| `Catalog Test Subset Consumers` | boxel, on PRs and pushes to `main` touching that workflow, the manifest, the sync, the subset test helpers, or the CI boot path | a boxel change that would break the run boxel-catalog calls, tested against the pinned revision | Each of these fails with a message naming the fix. Don't work around a failure by editing a generated copy or the marker (`catalog-test-subset.txt`) by hand. diff --git a/.github/workflows/catalog-test-subset.yml b/.github/workflows/catalog-test-subset.yml index a8b5c9f7d93..2589ab0cd6a 100644 --- a/.github/workflows/catalog-test-subset.yml +++ b/.github/workflows/catalog-test-subset.yml @@ -12,9 +12,15 @@ name: Catalog Test Subset # without checking the catalog. Otherwise a catalog change to a subset file # would turn every open boxel PR red until someone re-pinned. A change to the # host's shimmed modules re-runs only the closure check, which reads them. +# +# The pin may also be the head of an open catalog pull request this change +# merges before, while that pull request is approved to merge right after it +# (openPinVerdict in scripts/pairing.ts). The pairing is declared in the +# description, so editing the description re-runs this. on: pull_request: + types: [opened, synchronize, reopened, edited] permissions: contents: read @@ -32,6 +38,7 @@ jobs: sparse-checkout: | packages/catalog/test-subset.json packages/catalog/scripts/sync-test-subset.ts + packages/catalog/scripts/pairing.ts packages/host/app/lib/externals.ts sparse-checkout-cone-mode: false @@ -41,6 +48,7 @@ jobs: pin_paths=( packages/catalog/test-subset.json packages/catalog/scripts/sync-test-subset.ts + packages/catalog/scripts/pairing.ts .github/workflows/catalog-test-subset.yml ) if git diff-tree -r --quiet HEAD^1 HEAD -- "${pin_paths[@]}"; then @@ -61,9 +69,29 @@ jobs: with: node-version: "24" + # Lint Catalog reports a malformed or one-sided pairing, so a pairing + # problem doesn't fail this job. It drops the resolution instead, since a + # pairing with a problem can't excuse a pin, and without a resolution the + # pin waits on any open catalog pull request it comes from. + - name: Read the catalog pairing + if: steps.touched.outputs.pin == 'true' + env: + PR_BODY: ${{ github.event.pull_request.body }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_BASE: ${{ github.event.pull_request.base.ref }} + GH_TOKEN: ${{ github.token }} + run: | + node packages/catalog/scripts/pairing.ts \ + --repository="$GITHUB_REPOSITORY" \ + --number="$PR_NUMBER" \ + --base="$PR_BASE" \ + --counterpart=cardstack/boxel-catalog \ + --out="$RUNNER_TEMP/catalog-pairing.json" || + rm -f "$RUNNER_TEMP/catalog-pairing.json" + - name: Pinned subset files match boxel-catalog main if: steps.touched.outputs.pin == 'true' - run: node packages/catalog/scripts/sync-test-subset.ts --check-pin + run: node packages/catalog/scripts/sync-test-subset.ts --check-pin --pairing="$RUNNER_TEMP/catalog-pairing.json" env: GITHUB_TOKEN: ${{ github.token }} diff --git a/.github/workflows/lint-catalog.yaml b/.github/workflows/lint-catalog.yaml index d033b7ec899..13b0b36eebe 100644 --- a/.github/workflows/lint-catalog.yaml +++ b/.github/workflows/lint-catalog.yaml @@ -7,6 +7,10 @@ name: Lint Catalog # including the ones it would leave catalog main with behind a paired catalog # pull request that isn't ready to follow it. See packages/catalog/README.md. # +# Production deploys a boxel commit with the catalog at the revision it pins in +# packages/catalog/test-subset.json, so this also lints the whole catalog at +# the pin against each change, and fails on every error the change adds there. +# # A pull request declares its catalog pair in its description, so editing the # description re-runs this. on: @@ -137,19 +141,51 @@ jobs: echo "errors=$(jq '.diagnostics | length' "$RUNNER_TEMP/catalog-main-lint-head.json")" >> "$GITHUB_OUTPUT" git -C contents checkout --quiet --detach "$paired" + # Production deploys this boxel with the catalog at its pin, so the + # whole catalog at the pin has to work with the change, not only the + # subset files boxel's tests load. When the pin is a revision a step + # above already linted against the change, that recording is reused. + # It is still reported again below, as the pin, against the base + # branch's pin: the reports above compare a revision only with itself. + - name: Lint the catalog at this change's pin + id: pin + working-directory: packages/catalog + env: + MAIN_REVISION: ${{ steps.main.outputs.revision }} + run: | + pin=$(jq -r .revision test-subset.json) + linted=$(git -C contents rev-parse HEAD) + echo "revision=$pin" >> "$GITHUB_OUTPUT" + if [ "$pin" = "$linted" ]; then + cp "$RUNNER_TEMP/catalog-lint-head.json" "$RUNNER_TEMP/catalog-pin-lint-head.json" + elif [ "$pin" = "$MAIN_REVISION" ]; then + cp "$RUNNER_TEMP/catalog-main-lint-head.json" "$RUNNER_TEMP/catalog-pin-lint-head.json" + else + git -C contents fetch --quiet --depth=1 origin "$pin" + git -C contents checkout --quiet --detach FETCH_HEAD + node scripts/lint-sweep.ts --record="$RUNNER_TEMP/catalog-pin-lint-head.json" + git -C contents checkout --quiet --detach "$linted" + fi + echo "errors=$(jq '.diagnostics | length' "$RUNNER_TEMP/catalog-pin-lint-head.json")" >> "$GITHUB_OUTPUT" + # Errors the catalog also has against the base branch are not this # change's doing: the catalog may already be broken against boxel main. # So when this change's lint finds errors, lint the same catalog revision # against the base branch, and the report counts only the difference. - # Catalog main, when it was linted above, gets the same comparison. + # Catalog main, when it was linted above, gets the same comparison. The + # catalog at the pin is compared with the base branch's own pin linted + # against the base branch, since a change that moves the pin is answerable + # for the errors the new pin brings too. # A push to main has no base to compare with, so every error counts. - name: Lint the catalog against the base branch id: base - if: github.event_name == 'pull_request' && (steps.head.outputs.errors != '0' || (steps.main.outputs.errors || '0') != '0') + if: github.event_name == 'pull_request' && (steps.head.outputs.errors != '0' || (steps.main.outputs.errors || '0') != '0' || (steps.pin.outputs.errors || '0') != '0') env: HEAD_ERRORS: ${{ steps.head.outputs.errors }} MAIN_ERRORS: ${{ steps.main.outputs.errors || '0' }} MAIN_REVISION: ${{ steps.main.outputs.revision }} + PIN_ERRORS: ${{ steps.pin.outputs.errors || '0' }} + PIN_REVISION: ${{ steps.pin.outputs.revision }} run: | head=$(git rev-parse HEAD) git checkout --detach HEAD^1 @@ -167,6 +203,12 @@ jobs: git -C contents checkout --quiet --detach "$MAIN_REVISION" node "$RUNNER_TEMP/lint-sweep.ts" --record="$RUNNER_TEMP/catalog-main-lint-base.json" fi + if [ "$PIN_ERRORS" != 0 ]; then + base_pin=$(jq -r '.revision // empty' test-subset.json 2>/dev/null || true) + git -C contents fetch --quiet --depth=1 origin "${base_pin:-$PIN_REVISION}" + git -C contents checkout --quiet --detach FETCH_HEAD + node "$RUNNER_TEMP/lint-sweep.ts" --record="$RUNNER_TEMP/catalog-pin-lint-base.json" + fi # Unpaired, this is catalog main, and errors the change adds to it fail # with how to declare a pair. Paired, this is the catalog pull request's @@ -174,7 +216,7 @@ jobs: # main lint skips the base step, and without its baseline this report # would count errors the catalog already has as new ones. - name: Report catalog lint errors - if: ${{ !cancelled() && steps.head.outcome == 'success' && steps.main.outcome != 'failure' && steps.base.outcome != 'failure' }} + if: ${{ !cancelled() && steps.head.outcome == 'success' && steps.main.outcome != 'failure' && steps.pin.outcome != 'failure' && steps.base.outcome != 'failure' }} run: | args=(--report="$RUNNER_TEMP/catalog-lint-head.json") if [ -f "$RUNNER_TEMP/catalog-lint-base.json" ]; then @@ -190,7 +232,7 @@ jobs: # pull request this change merges before is approved, to merge right # after it. - name: Report what this change does to catalog main - if: ${{ !cancelled() && steps.main.outcome == 'success' && steps.base.outcome != 'failure' }} + if: ${{ !cancelled() && steps.main.outcome == 'success' && steps.pin.outcome != 'failure' && steps.base.outcome != 'failure' }} run: | args=(--report="$RUNNER_TEMP/catalog-main-lint-head.json" --pairing="$RUNNER_TEMP/catalog-pairing.json") if [ -f "$RUNNER_TEMP/catalog-main-lint-base.json" ]; then @@ -198,6 +240,21 @@ jobs: fi node "$RUNNER_TEMP/lint-sweep.ts" "${args[@]}" + # Every error the change adds to the catalog at its pin fails, paired or + # not: production deploys this boxel with that catalog. A pairing only + # names the catalog revision to pin instead. + - name: Report what this change does to the catalog at its pin + if: ${{ !cancelled() && steps.pin.outcome == 'success' && steps.base.outcome != 'failure' }} + run: | + args=(--report="$RUNNER_TEMP/catalog-pin-lint-head.json" --pinned) + if [ -f "$RUNNER_TEMP/catalog-pin-lint-base.json" ]; then + args+=(--baseline="$RUNNER_TEMP/catalog-pin-lint-base.json") + fi + if [ -f "$RUNNER_TEMP/catalog-pairing.json" ]; then + args+=(--pairing="$RUNNER_TEMP/catalog-pairing.json") + fi + node "$RUNNER_TEMP/lint-sweep.ts" "${args[@]}" + # A catalog pull request this change merges after has to be in the pin # before this change merges, and nothing re-runs this job when that pull # request merges. So while it is open this fails, after the lint above diff --git a/packages/catalog/README.md b/packages/catalog/README.md index a9ea48e949f..85281302353 100644 --- a/packages/catalog/README.md +++ b/packages/catalog/README.md @@ -100,6 +100,7 @@ The catalog type-checks and lints its cards against this monorepo, so a platform - It lints boxel-catalog `main`, unless the boxel pull request's description pairs it with a boxel-catalog pull request, in which case it lints that pull request's head. A pair is a line in each description naming the other, keyed by merge order (`Merges before:` on the one that lands first, `Merges after:` on the other), and `scripts/pairing.ts` checks it from both sides. A pull request in a pair targets `main`, or is stacked on an open pull request of its own repository, and is retargeted to `main` once that pull request merges. The `catalog-pairing` skill describes the protocol. Editing the description re-runs the workflow. - With a pair, it lints catalog `main` against the change as well. The catalog's own CI Lint always lints against boxel `main`, so a catalog fix for a breaking boxel change can't pass there until the boxel change merges, and until the catalog pull request then merges, catalog `main` fails with whatever the boxel change broke. A catalog pull request the change merges after has to land first, so until it does the job fails, waiting on it. Otherwise, errors the change adds to catalog `main` pass only while a catalog pull request the change merges before is open, ready for review, and approved, by GitHub's own review decision. While either of the two is stacked on a parent, this fails too, because the stacked one lands on its parent's branch rather than `main`; retarget it once the parent merges. Once the catalog pull request is approved and neither is stacked, re-run the job, then merge the catalog pull request right after the boxel one. A change on the catalog side, including a push, doesn't re-run this job, so re-run it by hand. Without a pair, errors the change adds to catalog `main` fail, and the job says which lines to add to pair the change with a catalog fix. +- It also lints the whole catalog at the revision `test-subset.json` pins, because production deploys a boxel commit with the catalog at its pin. Every error the change adds there fails, paired or not, compared with the base branch's pin linted against the base branch. A change that breaks the catalog pins the head of the catalog pull request that fixes it, one it merges before, and that pull request merges right after it. Until then, the catalog `main` gate above fails unless the catalog pull request is approved. When the fix also changes a subset file, the Catalog Test Subset check accepts the pin on the same terms. Once the catalog pull request has merged, the lockstep deploy deploys its merge commit in place of the pinned head, whatever the merge method. - It fails only on errors the change introduces. When linting the catalog against the change finds errors, the job lints the same catalog revision against the pull request's base branch too, and errors that appear in both runs are listed as already present rather than failing the check. On pushes to `main` there is no base to compare with, so any error fails. - The job summary lists each error, linking catalog files to their line in boxel-catalog. diff --git a/packages/catalog/scripts/dispatch-catalog-deploy.sh b/packages/catalog/scripts/dispatch-catalog-deploy.sh index f8e4d58ab21..ab5cad83dc6 100755 --- a/packages/catalog/scripts/dispatch-catalog-deploy.sh +++ b/packages/catalog/scripts/dispatch-catalog-deploy.sh @@ -50,6 +50,26 @@ if [ -n "$expiry" ]; then fi fi +# A boxel change that merges before the catalog pull request fixing what it +# breaks pins that pull request's head. The head descends only from where the +# pull request branched, so production's catalog is usually past it on another +# line, and the deploy check refuses a revision that diverged from it. Squashed +# or rebased, the head isn't on catalog main at all. Either way, once that pull +# request has merged, deploy its merge commit: it is on main, carries the same +# change, and holds everything main had when it merged. A merge commit behind +# production's catalog deploys nothing, as any revision behind it does. +# A failed request prints its error body, so only a successful one counts. +if merged=$(gh api "repos/$repo/commits/$revision/pulls" \ + --jq "[.[] | select(.merged_at != null and .base.ref == \"main\" and .head.sha == \"$revision\")][0] // empty | \"\(.number) \(.merge_commit_sha)\"" 2>/dev/null) && + [ -n "$merged" ]; then + read -r merged_pr merged_sha <<<"$merged" + if [ "$merged_sha" != "$revision" ]; then + echo "::notice title=catalog deploy::$revision is the head of $repo#$merged_pr, which merged as $merged_sha, so this deploys $merged_sha." + revision=$merged_sha + by_hand="Until then, run \"Deploy to production\" with revision $revision by hand: $runs" + fi +fi + # The catalog workflow's run name starts with the reason, so this run's marker # finds the run it starts. GitHub cuts a long run name short in the API, so the # marker leads. diff --git a/packages/catalog/scripts/lint-sweep.ts b/packages/catalog/scripts/lint-sweep.ts index 80febc4f486..c49935ce5f9 100644 --- a/packages/catalog/scripts/lint-sweep.ts +++ b/packages/catalog/scripts/lint-sweep.ts @@ -30,6 +30,14 @@ // does they fail, waiting on it. With no pairing they fail, saying how to // declare one. // +// node scripts/lint-sweep.ts --report= [--baseline=] [--pairing=] --pinned +// Reports a recording of the catalog at the revision this change pins in +// test-subset.json. Production deploys a boxel commit with the catalog at +// its pin, so every error the change adds there fails, paired or not; the +// pairing only says which catalog revision to pin instead. Its baseline is +// the base branch's pin linted against the base branch, so a new pin's +// errors count as the change's too. +// // Errors are matched by linter, file, rule, message and position first, then // the rest without position, so an error in a boxel file that the change only // moved still matches, and a repeated error the change adds is the one @@ -47,7 +55,7 @@ import { import { tmpdir } from 'node:os'; import { isAbsolute, join, relative, resolve } from 'node:path'; -import { mainVerdict, type Resolution } from './pairing.ts'; +import { mainVerdict, type Pair, type Resolution } from './pairing.ts'; type Linter = 'lint:types' | 'lint:js' | 'lint:hbs'; @@ -408,10 +416,51 @@ function annotation(message: string) { .replace(/\n/g, '%0A'); } +// What to do about errors a change adds to the catalog at its pin: pin a +// catalog revision the change works with, which a paired catalog pull request +// is, or keep the change compatible with the catalog as pinned. +function pinAdvice(resolution: Resolution | undefined) { + let deploys = + `Production deploys this boxel with the catalog at the revision ` + + `packages/catalog/test-subset.json pins, so the whole catalog there has ` + + `to work with it, not only the subset files boxel's tests load.`; + let open = (key: Pair['key']) => + resolution?.pairs.find((p) => p.key === key && !p.merged); + let repin = (pair: Pair) => + `set \`revision\` to ${pair.headSha}, run ` + + `\`pnpm --dir packages/catalog catalog:test-subset\`, run the manifest's ` + + `tests against it, and commit the new pin`; + let after = open('merges-after'); + if (after) { + return ( + `${deploys} Pin the head of ${after.repository}#${after.number}, which ` + + `this change merges after: ${repin(after)}.` + ); + } + let before = open('merges-before'); + if (before) { + return ( + `${deploys} Pin the head of ${before.repository}#${before.number}, ` + + `which this change merges before: ${repin(before)}. The Catalog Test ` + + `Subset check accepts that pin while ${before.repository}#` + + `${before.number} is approved, and it merges right after this change.` + ); + } + return ( + `${deploys} Keep the change compatible with the catalog at the pin, or ` + + `pin a catalog revision it works with: catalog main ` + + `(\`pnpm --dir packages/catalog catalog:test-subset --bump\`) when it ` + + `already works there, or else a boxel-catalog pull request that fixes ` + + `the catalog, paired with this change as packages/catalog/README.md ` + + `describes.` + ); +} + async function report( headPath: string, baselinePath: string | undefined, pairingPath: string | undefined, + pinned: boolean, ) { let head = JSON.parse(readFileSync(headPath, 'utf8')) as Recording; let baseline = baselinePath @@ -431,23 +480,32 @@ async function report( // With a pairing, this recording is either a paired catalog pull request's // head, which the change is linted with, or catalog main, which the change // must not leave failing behind a pull request that isn't ready. - let pair = resolution?.pairs.find( - (p) => !p.merged && p.headSha === catalog.revision, - ); - let gated = Boolean(resolution) && !pair; - let subject = pair - ? `${pair.repository}#${pair.number}'s head (${at})` - : gated - ? `catalog main (${at})` - : at; + // A pinned recording is reported on its own terms, whatever pull request + // its revision belongs to. + let pair = pinned + ? undefined + : resolution?.pairs.find( + (p) => !p.merged && p.headSha === catalog.revision, + ); + let gated = !pinned && Boolean(resolution) && !pair; + let subject = pinned + ? `the catalog at this change's pin (${at})` + : pair + ? `${pair.repository}#${pair.number}'s head (${at})` + : gated + ? `catalog main (${at})` + : at; let linting = `Linting ${subject} against this change`; + let title = pinned ? 'catalog lint (pin)' : 'catalog lint'; let summary = [ - pair - ? `### Catalog lint: ${pair.repository}#${pair.number}` - : gated - ? `### Catalog lint: main` - : `### Catalog lint`, + pinned + ? `### Catalog lint: pin` + : pair + ? `### Catalog lint: ${pair.repository}#${pair.number}` + : gated + ? `### Catalog lint: main` + : `### Catalog lint`, '', ]; let passes = added.length === 0; @@ -472,19 +530,24 @@ async function report( `::${passes ? 'notice' : 'error'} title=catalog lint (main)::${annotation(advice)}`, ); } else { - advice = pair - ? `Fix them in ${pair.repository}#${pair.number}, which this change ` + - `is paired with.` - : verdict - ? verdict.message - : `Keep the change compatible with the catalog as it is, or fix the ` + - `catalog in a boxel-catalog pull request and pair the two in ` + - `their descriptions, as packages/catalog/README.md describes.`; + advice = pinned + ? pinAdvice(resolution) + : pair + ? `Fix them in ${pair.repository}#${pair.number}, which this ` + + `change is paired with.` + : verdict + ? verdict.message + : `Keep the change compatible with the catalog as it is, or fix the ` + + `catalog in a boxel-catalog pull request and pair the two in ` + + `their descriptions, as packages/catalog/README.md describes.`; for (let d of added) { console.log( - `::error title=catalog lint::${annotation(`${location(d)} ${d.rule}: ${d.message}`)}`, + `::error title=${title}::${annotation(`${location(d)} ${d.rule}: ${d.message}`)}`, ); } + if (pinned) { + console.log(`::error title=${title}::${annotation(advice)}`); + } } summary.push( `${linting} finds ${introduced}. ${advice}`, @@ -505,7 +568,7 @@ async function report( existing, ); console.log( - `::warning title=catalog lint::${annotation( + `::warning title=${title}::${annotation( `${subject} already has ${existing.length} lint error(s) against the base branch; they do not fail this check.`, )}`, ); @@ -541,9 +604,10 @@ if (recordPath && !reportPath) { resolve(reportPath), baselinePath ? resolve(baselinePath) : undefined, pairingPath ? resolve(pairingPath) : undefined, + args.includes('--pinned'), ).catch((error) => fail(String(error))); } else { fail( - 'pass either --record= or --report= [--baseline=] [--pairing=]', + 'pass either --record= or --report= [--baseline=] [--pairing=] [--pinned]', ); } diff --git a/packages/catalog/scripts/pairing.test.ts b/packages/catalog/scripts/pairing.test.ts index 290cf881886..4c7e9862693 100644 --- a/packages/catalog/scripts/pairing.test.ts +++ b/packages/catalog/scripts/pairing.test.ts @@ -7,6 +7,7 @@ import { after, before, beforeEach, test } from 'node:test'; import { mainVerdict, + openPinVerdict, pinVerdict, resolvePairing, type Resolution, @@ -604,3 +605,78 @@ test('the catalog-main gate refuses a pair this change merges before when the pa /cardstack\/boxel-catalog#791 is stacked on cardstack\/boxel-catalog#780, so it can't land on catalog main right after this change/, ); }); + +test('the pin may wait on the head of an approved pair this change merges before', () => { + let verdict = openPinVerdict( + { repository: BOXEL, number: 6454, pairs: [openBefore()] }, + 791, + '791'.padEnd(40, '0'), + ); + assert.equal(verdict?.passes, true); + assert.match( + verdict!.message, + /Merge cardstack\/boxel-catalog#791 right after this change/, + ); +}); + +test('the pin may not wait on a commit of that pair other than its head', () => { + let verdict = openPinVerdict( + { repository: BOXEL, number: 6454, pairs: [openBefore()] }, + 791, + 'abc'.padEnd(40, '0'), + ); + assert.equal(verdict?.passes, false); + assert.match(verdict!.message, /not its head \(7910+\)/); +}); + +test('the pin may not wait on a pair this change merges before until it is approved', () => { + let verdict = openPinVerdict( + { + repository: BOXEL, + number: 6454, + pairs: [openBefore({ approved: false })], + }, + 791, + '791'.padEnd(40, '0'), + ); + assert.equal(verdict?.passes, false); + assert.match( + verdict!.message, + /cardstack\/boxel-catalog#791 is not approved yet/, + ); +}); + +test('the pin may not wait on a pair this change merges before while this change is stacked', () => { + let verdict = openPinVerdict( + { + repository: BOXEL, + number: 6454, + stackedOn: 'cardstack/boxel#6417', + pairs: [openBefore()], + }, + 791, + '791'.padEnd(40, '0'), + ); + assert.equal(verdict?.passes, false); + assert.match(verdict!.message, /stacked on cardstack\/boxel#6417/); +}); + +test('the pin has no say over a catalog pull request this change does not merge before', () => { + let resolution: Resolution = { + repository: BOXEL, + number: 6454, + pairs: [openBefore({ key: 'merges-after' })], + }; + assert.equal( + openPinVerdict(resolution, 791, '791'.padEnd(40, '0')), + undefined, + ); + assert.equal( + openPinVerdict( + { repository: BOXEL, number: 6454, pairs: [openBefore()] }, + 792, + '792'.padEnd(40, '0'), + ), + undefined, + ); +}); diff --git a/packages/catalog/scripts/pairing.ts b/packages/catalog/scripts/pairing.ts index 53bdd75637d..4696e05a7c6 100644 --- a/packages/catalog/scripts/pairing.ts +++ b/packages/catalog/scripts/pairing.ts @@ -842,6 +842,77 @@ export function mainVerdict(resolution: Resolution) { }; } +// Whether the catalog test subset may pin a commit of the open catalog pull +// request `pull`, ahead of its merge. Production deploys a boxel commit with +// the catalog at the revision it pins, so a change that breaks catalog main +// and merges before the catalog pull request that fixes it has to pin that +// fix: the catalog it pins otherwise is one it breaks. That pull request can't +// merge first, so the pin may wait on it under the terms catalog main waits on +// it (mainVerdict): open, ready for review, approved, and neither side +// stacked, to merge right after this change. The pin has to be its head, the +// commit that lands. Answers undefined when `pull` isn't a pull request this +// change merges before, so the pin waits on it to merge like any other. +export function openPinVerdict( + resolution: Resolution, + pull: number, + pin: string, +) { + let pair = resolution.pairs.find( + (p) => p.key === 'merges-before' && !p.merged && p.number === pull, + ); + if (!pair) { + return undefined; + } + let ref = `${pair.repository}#${pair.number}`; + let after = resolution.pairs.find( + (p) => p.key === 'merges-after' && !p.merged, + ); + let notReady = after + ? `this change merges after ${after.repository}#${after.number}, which ` + + `hasn't merged, so this change can't merge yet` + : resolution.stackedOn + ? `this change is stacked on ${resolution.stackedOn}; retarget it to ` + + `main once that merges` + : pair.stackedOn + ? `${ref} is stacked on ${pair.stackedOn}; retarget it to main once ` + + `that merges` + : pair.draft + ? `${ref} is still a draft; mark it ready for review and get it ` + + `approved` + : pair.approved === false + ? `${ref} is not approved yet; get it approved` + : pair.approved === null + ? `whether ${ref} is approved could not be read ` + + `(${pair.approvalError})` + : undefined; + if (notReady) { + return { + passes: false, + message: + `The pin is a commit in ${ref}, which this change merges before. It ` + + `may pin that pull request only while it can merge right after this ` + + `change, and ${notReady}. Then re-run this check.`, + }; + } + if (pair.headSha !== pin) { + return { + passes: false, + message: + `The pin is a commit in ${ref}, which this change merges before, but ` + + `not its head (${pair.headSha}), which is the commit that lands. ` + + `Re-pin to the head, run the manifest's tests against it, and commit ` + + `the new pin.`, + }; + } + return { + passes: true, + message: + `The pin is the head of ${ref}, which this change merges before and ` + + `which is approved. Merge ${ref} right after this change. If ${ref} ` + + `gets new commits, re-pin to its head.`, + }; +} + function describe(here: string, pair: Pair) { let state = pair.merged ? 'merged' diff --git a/packages/catalog/scripts/sync-test-subset.ts b/packages/catalog/scripts/sync-test-subset.ts index 961a5ae4576..2e28bd6f36a 100644 --- a/packages/catalog/scripts/sync-test-subset.ts +++ b/packages/catalog/scripts/sync-test-subset.ts @@ -6,8 +6,12 @@ // node scripts/sync-test-subset.ts --into-clone also merge into contents/ // node scripts/sync-test-subset.ts --remove-from-clone // node scripts/sync-test-subset.ts --bump re-pin to catalog main -// node scripts/sync-test-subset.ts --check-pin fail unless every subset file at the pin -// matches catalog main +// node scripts/sync-test-subset.ts --check-pin [--pairing=] +// fail unless every subset file at the pin +// matches catalog main, or the pin is the +// head of the approved catalog pull request +// this change merges before ( is +// scripts/pairing.ts's resolution) // node scripts/sync-test-subset.ts --check-no-copies= // fail when holds a copy of a // subset definition @@ -40,6 +44,8 @@ import { import { basename, dirname, join, relative, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; +import { openPinVerdict, type Resolution } from './pairing.ts'; + const catalogDir = resolve(dirname(fileURLToPath(import.meta.url)), '..'); const repoRoot = resolve(catalogDir, '..', '..'); const manifestPath = join(catalogDir, 'test-subset.json'); @@ -97,6 +103,17 @@ function readManifest(): Manifest { return manifest; } +// The pairing scripts/pairing.ts resolved for the pull request under check. +// It may be missing when the pairing couldn't be read; the pin then waits on +// any open catalog pull request it comes from. +function readPairing(path: string): Resolution | undefined { + if (!existsSync(path)) { + return undefined; + } + let text = readFileSync(path, 'utf8').trim(); + return text ? (JSON.parse(text) as Resolution) : undefined; +} + function fail(message: string): never { console.error(`catalog test subset: ${message}`); process.exit(1); @@ -765,8 +782,10 @@ function currentBranch() { // doesn't matter. A commit from a catalog pull request passes once that pull // request merges, whether it merged as a merge commit, a squash or a rebase, // and fails while the pull request is open or once main changes a subset file -// again. -async function checkPin(manifest: Manifest) { +// again. The one open pull request a pin may come from is the head of the +// catalog pull request this change merges before, while it is approved to +// merge right after this change (openPinVerdict in pairing.ts). +async function checkPin(manifest: Manifest, pairing?: Resolution) { let headers: Record = { accept: 'application/vnd.github+json', }; @@ -846,6 +865,19 @@ async function checkPin(manifest: Manifest) { waitingFiles.length === 100 || waitingFiles.some(({ filename }) => changed.includes(filename))) ) { + // A change that merges before the catalog pull request fixing what it + // breaks pins that pull request's head, so the pin may wait on it while it + // is ready to merge right after this change (pairing.ts). + let verdict = pairing + ? openPinVerdict(pairing, waitingOn.number, manifest.revision) + : undefined; + if (verdict?.passes) { + log(`${differ}. ${verdict.message}`); + return; + } + if (verdict) { + fail(`${differ}. ${verdict.message}`); + } fail( `${differ}, because the pin is a commit in ${waitingOn.html_url}, which hasn't merged: ` + `waiting on ${manifest.repository}#${waitingOn.number} to merge. ` + @@ -899,7 +931,11 @@ async function main() { let args = new Set(process.argv.slice(2)); let noCopiesArg = [...args].find((a) => a.startsWith('--check-no-copies=')); if (args.has('--check-pin')) { - await checkPin(readManifest()); + let pairingArg = [...args].find((a) => a.startsWith('--pairing=')); + let pairing = pairingArg + ? readPairing(resolve(pairingArg.slice('--pairing='.length))) + : undefined; + await checkPin(readManifest(), pairing); return; } if (args.has('--remove-from-clone')) {