Repository navigation
Lint Catalog fails a boxel PR whose merged Merges-after catalog PR isn't in the pin - #6585
Conversation
…n't in the pin Production deploys the catalog at the revision the deployed boxel pins, so a boxel PR that merges after a catalog PR has to pin past that PR's merge commit. The pairing check now reads the manifest's revision and fails when it doesn't contain the merged counterpart's merge commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review checked whether the new pin check actually stops a Merges after boxel PR from merging with a stale pin. It looked at the pairing resolution, the compare semantics, the merge commit used for a stacked counterpart, and the workflow triggers. Lint-sweep reporting outside the pairing gate was not reviewed.
The comparison logic and its tests are right. But the check only runs on a Lint Catalog run that starts after the catalog PR merges, and nothing starts one. A boxel PR that went green while its Merges after pair was open can still merge with a stale pin. Blocking.
- Keep Lint Catalog red while a
merges-afterpair is open and--pinis passed, so that the--bumppush is what turns it green (see the thread onpairing.ts,pull.merged && key === 'merges-after'). - Shorten step 5 of
catalog-test-subset, now that it always starts with--bump(non-blocking, see the thread there).
| ); | ||
| continue; | ||
| } | ||
| if (pull.merged && key === 'merges-after' && pinnedRevision) { |
There was a problem hiding this comment.
[Claude Code 🤖] The pin check only runs when Lint Catalog runs again after the catalog pull request merges, and nothing makes it run again. So the case this check is for can still merge on a green run from before the catalog merge.
How it happens: a boxel PR declares Merges after: cardstack/boxel-catalog#N while #N is open, and its change adds no errors to catalog main. resolvePairing reports no problems for an open merges-after pair (the new test "an open pull request this change merges after is not held to the pin yet" asserts that). mainVerdict's "waiting on … to merge" only applies when lint-sweep.ts finds added errors (gated && resolution ? mainVerdict(...)), so Lint Catalog goes green. Then #N merges. The workflow runs only on opened/synchronize/reopened/edited of the boxel PR, so nothing re-runs it. The PR stays green with a pin from before the merge, and someone skipping --bump doesn't push, so no new run starts. Re-running Catalog Test Subset after the merge, as the skill says to, doesn't re-run this job either.
Suggested fix: when --pin is passed and a merges-after pair is still open, keep Lint Catalog red with something like "waiting on #N to merge, then re-pin with --bump". The --bump push then triggers the run that does the pin comparison. This should be a failure at the end of the job, not a pairing-step exit 1, because an exit 1 there would skip linting the paired catalog head, and that is the useful signal while #N is open. One way is to write it into the resolution and have the report step fail on it, like the main verdict does. The catalog-pairing skill's "Lint Catalog enforces the re-pin" line overstates the check until this is fixed.
Regression in the sense that the PR's stated goal isn't met for this path. Blocking, unless the gap is accepted on purpose and the skill text is reworded to match.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 1c80898. While --pin is passed and a merges-after pair is open, the resolution records the pin. A new last step, "Wait for the re-pin", runs pairing.ts --pin-verdict, which fails with "… hasn't merged, so …'s pin can't contain it yet … Once it merges, run pnpm --dir packages/catalog catalog:test-subset --bump and push: that push re-runs this check." It runs after the lint steps, so the paired head is still linted, and the pairing step itself still exits 0 for an open pair. The catalog-pairing skill now describes both failures.
| 3. **Push the boxel branch, with its manifest entry, before you push the boxel-catalog branch or open its PR.** boxel-catalog's `Boxel Test Subset` workflow decides which files to test from the manifest on the same-named boxel branch, looked up at the moment it runs. If that branch isn't on GitHub yet, the workflow falls back to boxel `main`. That manifest doesn't list a file you are adding, so the check passes without running a single test. A changed file fails loudly against boxel `main` instead of passing, but the tests it ran still weren't yours. If the catalog workflow ran before the boxel branch existed, re-run it once the branch is pushed. Then open the boxel-catalog PR, set the manifest's `revision` to its head sha (a commit on a pushed branch fetches fine), and push the boxel branch again. Re-pin whenever you push to the catalog PR. Pin before you open or merge the boxel PR. | ||
| 4. **Validate the catalog PR against boxel.** boxel-catalog's `Boxel Test Subset` workflow runs the manifest's `tests` consumers whenever a PR touches a subset path. It runs them against the boxel branch of the same name when one exists, otherwise against boxel `main`, so matching branch names are how a catalog change and the boxel change that depends on it are tested together before either merges. A manual run takes any `boxel_ref`. The catalog workflow only decides whether to run and which boxel ref to use. The tests run in boxel's reusable `.github/workflows/catalog-test-subset-consumers.yml`, which the catalog calls at boxel `main`, so a CI setup change belongs in boxel, not in the catalog. The workflow's steps, and those of the `test-web-assets.yaml` it calls, come from boxel `main`. What the steps run comes from the chosen ref: its mise tasks, composite actions, manifest and tests, and the source the host dist is built from. So a paired boxel branch's edit to one of those steps reaches catalog runs only once it merges; the branch's own boxel PR run is what tests it. And if the catalog run fails on a task or argument the paired branch doesn't have, the branch is behind `main`: merge `main` into it. While the pin is a catalog PR commit, boxel's `Catalog Test Subset` check fails with `waiting on cardstack/boxel-catalog#<N> to merge`. That is expected until the catalog merges. | ||
| 5. **Merge the catalog PR first.** Then re-run the boxel PR's `Catalog Test Subset` check. It passes once every subset file at the pin matches catalog `main`, whether the catalog PR merged as a merge commit, a squash or a rebase, so the pin doesn't have to move. If it fails with "main has changed … since" (the catalog PR got commits after you pinned, or another catalog PR changed a subset file), re-pin with `pnpm --dir packages/catalog catalog:test-subset --bump`, run the sync, and re-run the consumers. Then merge the boxel PR. | ||
| 5. **Merge the catalog PR first.** Then re-pin the boxel PR onto catalog `main` with `pnpm --dir packages/catalog catalog:test-subset --bump`, run the sync, re-run the consumers, and push. Production deploys the catalog at the revision the deployed boxel pins, so the pin has to contain the catalog PR's merge commit, not only its head: Lint Catalog fails a boxel PR that declares `Merges after:` a merged catalog PR whose merge commit the pin predates. `Catalog Test Subset` passes once every subset file at the pin matches catalog `main`. If it fails with "main has changed … since" (the catalog PR got commits after you pinned, or another catalog PR changed a subset file), re-pin with `pnpm --dir packages/catalog catalog:test-subset --bump`, run the sync, and re-run the consumers. Then merge the boxel PR. |
There was a problem hiding this comment.
[Claude Code 🤖] This step now starts by always re-pinning with --bump onto catalog main. That makes the second half redundant: "Catalog Test Subset passes once every subset file at the pin matches catalog main … if it fails with 'main has changed … since' … re-pin with --bump". Right after a --bump, the pin is main, so that failure can't happen unless main moves again before merge. Suggest cutting the second half to one sentence about that case (main moved again after you bumped → bump again), so the step reads as one procedure. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 1c80898. Step 5 now ends with a single sentence: if catalog main moves again before the boxel PR merges and Catalog Test Subset fails with "main has changed … since", bump again.
Nothing re-runs Lint Catalog when a catalog pull request merges, so a run that went green while the pair was open would stand with a stale pin. The job's last step now fails while a Merges-after pair is open, after the lint steps have judged its head, so the re-pin push is the run that compares the pin with the merge commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-fails-a-boxel-pr-whose-merges-after-catalog-pr
2 participants
Important
Merge order: #6587 is stacked on this pull request and merges after it. There is no catalog pair for this pull request itself.
Manual Deploy [boxel] to production deploys the catalog at the revision the deployed boxel commit pins in the catalog test subset manifest. A boxel pull request that declares a
Merges after:line naming a catalog pull request needs that catalog change in production, and it gets there only if the boxel pull request re-pinned past the catalog pull request's merge commit. The catalog-pairing skill describes the re-pin, but nothing checked it, so a skipped--bumpshipped the boxel change to production without the catalog change it depends on.Lint Catalog's pairing step now reads the manifest's
revisionand, once theMerges aftercatalog pull request has merged, fails unless GitHub's compare shows that revision contains the merge commit. The error names both commits and the fix:cardstack/boxel-catalog#N merged as <sha>, but cardstack/boxel#M pins <rev>, which predates it. Run pnpm --dir packages/catalog catalog:test-subset --bump.The check is opt-in through a new--pinflag, which only boxel's workflow passes, sopairing.tsrun from the catalog side is unchanged.A pin at the catalog pull request's head is not enough: after a squash or rebase merge that head is not on catalog
main, and the deploy only ships revisions onmain. For a counterpart that was stacked in its own repository, the commit checked is the one its stack reachedmainthrough, the merge commit of the last parent in the chain. Nothing re-runs Lint Catalog when a catalog pull request merges, so while theMerges afterpair is open the job's last step fails, saying to re-pin once it merges. That step runs after the lint steps have judged the pair's head, and the re-pin push is the run that compares the pin with the merge commit.Merges beforepairs are not held to the pin.The catalog-test-subset and catalog-pairing skills no longer say the pin can stay on the catalog pull request's head after it merges.
Linear: CS-13451
🤖 Generated with Claude Code