Repository navigation
Conversation
Production deploys a boxel commit with the catalog at the revision it pins, so Lint Catalog now lints the whole catalog at the pin and fails on every error a change adds there. A boxel-first change that breaks the catalog pins the head of its approved Merges-before catalog pull request, which --check-pin accepts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![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: 43e66c2761
ℹ️ 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".
| if [ "$pin" = "$linted" ]; then | ||
| echo "The pin is the catalog revision linted against this change above." | ||
| echo "errors=0" >> "$GITHUB_OUTPUT" | ||
| exit 0 |
There was a problem hiding this comment.
When the pin equals the initially checked-out catalog revision—as it does for the boxel-first paired flow—this hard-codes zero errors and skips creating the pinned recording. The ordinary report compares that revision against itself on the Boxel base branch, so lint errors already present in the newly pinned catalog revision are classified as existing rather than compared with the base branch's old pin. The check can therefore pass while the whole catalog at the new production pin contains errors, defeating this change's primary safeguard. Reuse catalog-lint-head.json, but still report it as pinned and compare it with the base branch's pin.
AGENTS.md reference: AGENTS.md:L278-L278
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 2973385. When the pin is the revision the first lint step linted, the pin step now copies that recording to catalog-pin-lint-head.json and outputs its error count. The pin report then compares it with the base branch's pin linted against the base branch, so errors a new pin brings count as this change's.
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 covers the Lint Catalog step logic (step order, if: conditions, which recording is compared with which baseline, the skip cases, push to main, fetch by sha), whether the --check-pin exception is safe, and whether the docs match the code. It does not cover lint-sweep.ts error matching beyond the new --pinned branch.
One blocking issue: when the pin equals a revision an earlier step already linted, the pin is compared with the wrong baseline. That is the case for the boxel-first pair and for most --bump PRs. The merge-commit requirement behind the new exception also has nothing enforcing it.
- Pin equals the linted revision: reuse the recording instead of reporting
errors=0, so the pin is compared with the base branch's pin (inline onlint-catalog.yaml, pin step). - Merge-commit dependency: make it mechanical, and fix the quoted deploy refusal text (inline on the
catalog-pairingskill, step 4). || truein Catalog Test Subset lets a resolution with own-side problems satisfypinVerdict(inline oncatalog-test-subset.yml).
| if [ "$pin" = "$linted" ]; then | ||
| echo "The pin is the catalog revision linted against this change above." | ||
| echo "errors=0" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
[Claude Code 🤖] When the pin equals the revision the first lint step checked out, this sets errors=0, so the pin is never compared with the base branch's pin. The head report covers it, but against a different baseline: the same catalog revision linted against the base branch. Errors the new pin has against both boxel trees get excused as "already present", even when the old pin didn't have them. That is the case the comment at line 175 rules out ("a change that moves the pin is answerable for the errors the new pin brings too").
This branch is taken in the two most common pin moves:
- an unpaired
--bumpPR, when catalogmainhasn't moved since the bump (catalog_refismain, solintedequals the pin); - the boxel-first pair this PR is built for, where the pin is the
Merges before:head andcatalog_refis that same head.
So whether a --bump onto a catalog main that is already broken against boxel main gets through depends on whether catalog main moved before CI ran.
Fix: in this branch, reuse the recording rather than skipping. Copy catalog-lint-head.json to catalog-pin-lint-head.json and output its error count, the same way the MAIN_REVISION branch does. The base step then lints the base pin, and the pin report compares the right pair. The comment at lines 142–145 and step 4 of the catalog-pairing skill ("that recording is reused") would then hold as written.
Regression, blocking: the PR's own rule doesn't hold on the paths it was written for.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 2973385. When the pin is the revision the first lint step linted, the pin step now copies that recording to catalog-pin-lint-head.json and outputs its error count. The pin report then compares it with the base branch's pin linted against the base branch, so errors a new pin brings count as this change's.
| 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 Catalog Test Subset checks. | ||
| 4. Merge the boxel pull request, then the catalog pull request right after it, **with a merge commit**. A squash or a rebase leaves the pinned commit off catalog `main`, and production's deploy of the pin then refuses with "isn't an ancestor of". |
There was a problem hiding this comment.
[Claude Code 🤖] Only this sentence and the pinVerdict message ask for "with a merge commit", and cardstack/boxel-catalog has squash and rebase merges enabled (allow_squash_merge / allow_rebase_merge are both true). Until now a pin had to be on catalog main before boxel merged, so the merge method didn't matter. With the new --check-pin exception, one squash click in the catalog leaves boxel main pinning a commit that is never on catalog main. The post-release run of "Deploy to production" then refuses, so production runs the new boxel against the older catalog this change breaks, until someone lands a re-pin PR and deploys again.
Can this be made mechanical? Two options:
- Recommended: have the catalog side resolve a pin that is the head of a merged pull request to that pull request's
merge_commit_sha, and deploy that when the trees are equal. This removes the dependency on merge method. - At least, have a boxel-catalog check fail a pull request that says
Merges after: cardstack/boxel#Nwhile #N pins its head, with text naming the required merge method. That is advisory only.
Separately, the quoted refusal text is wrong. deploy-production.yml refuses a revision that isn't on main with "… isn't on catalog main. Deploy a revision on main." "isn't an ancestor of" is a different check in catalog-deploy-check.ts.
Regression, introduced by the exception. Your call whether it blocks: it is safe only as long as every merger reads this line.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 2973385, by making the merge method not matter rather than enforcing one. dispatch-catalog-deploy.sh now checks whether the pinned commit is on catalog main. If it isn't, and it is the head of a catalog pull request that merged into main, the script deploys that pull request's merge commit, with a notice naming both. I checked the lookup against cardstack/boxel-catalog#787, which was squash-merged: its head is diverged from main and resolves to its merge commit ffcfb5c92c. A commit already on main, an open pull request's head and an unknown sha are left as they are. The docs no longer ask for a merge commit, and the wrong "isn't an ancestor of" quote is gone.
| --number="$PR_NUMBER" \ | ||
| --base="$PR_BASE" \ | ||
| --counterpart=cardstack/boxel-catalog \ | ||
| --out="$RUNNER_TEMP/catalog-pairing.json" || true |
There was a problem hiding this comment.
[Claude Code 🤖] || true keeps the resolution file even when pairing.ts exits 1, and resolvePairing still pushes pairs when the problem is on this pull request's own side. In openSide, the problem and landed cases, and stacked with a merged parent, all return a problem with no stackedOn. pinVerdict then sees resolution.stackedOn undefined, treats "neither side stacked" as met, and --check-pin goes green for a boxel pull request that targets a dead parent branch. Lint Catalog does go red on the same pairing, but main has no required status checks, so a green Catalog Test Subset beside it is misleading.
Fix: drop the file when the pairing has problems, so the pin waits on the catalog pull request like any other:
| --out="$RUNNER_TEMP/catalog-pairing.json" || true | |
| --out="$RUNNER_TEMP/catalog-pairing.json" || rm -f "$RUNNER_TEMP/catalog-pairing.json" |
Then update the comment above the step (Lint Catalog reports a malformed or one-sided pairing…) to match. Regression, non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 2973385. A failed pairing.ts run now deletes catalog-pairing.json (|| rm -f …), so a pairing with a problem can't excuse a pin, and the pin waits on its catalog pull request as before.
…shed pin's merge commit Lint Catalog now reports the catalog at the pin against the base branch's pin even when an earlier step already linted that revision. Catalog Test Subset drops a pairing resolution that has a problem. The lockstep catalog deploy maps a pinned head that a squash or rebase left off catalog main to its pull request's merge commit, so the merge method no longer matters, and the docs say which check holds a boxel-first pin to approval. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…xel-pr-whose-merges-after-catalog-pr' into cs-13447-the-catalog-pin-should-mean-this-boxel-works-with-the-whole
richardhjtan
left a comment
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] One blocking issue, inline on dispatch-catalog-deploy.sh.
The head of a catalog pull request descends only from its branch point, so production's catalog is usually past it on another line and the deploy check refuses it as diverged, even after a merge commit puts the head on main. Once the pull request has merged, deploy its merge commit instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Important
Merge order: this, then cardstack/boxel-catalog#810.
cardstack/boxel-catalog#810 documents this rule on the catalog side; merge it right after this one.
Merges before: cardstack/boxel-catalog#810
Production deploys a boxel commit with the catalog at the revision it pins in
packages/catalog/test-subset.json. Until now, nothing checked that this boxel works with the whole catalog at that revision.--check-pinand the subset consumers cover only the subset files, and Lint Catalog lints catalogmainand the paired pull request's head, not the pin. A boxel change could break a non-subset catalog file at the pin, merge green, and ship broken.Lint Catalog now lints the whole catalog at the pin 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. When the pin is the revision an earlier step already linted, that recording is reused.
The case that used to merge with a stale pin is a boxel-first pair: a boxel change that breaks the catalog, followed right after by the catalog fix. That change now pins the head of its catalog pull request.
--check-pinaccepts a pin at the head of an open catalog pull request only when that pull request is the one this change saysMerges before:, it is approved and ready for review, and neither side is stacked. These are the terms under which Lint Catalog already lets catalogmainwait for it (pinVerdictinpairing.ts). The catalog pull request then merges right after. 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, so deploying it would be refused as diverged. Catalog Test Subset now reads the pairing, and re-runs when the description is edited.The
catalog-pairing,catalog-test-subsetandcatalog-deployskills and the catalog README describe the new rule. The paired boxel-catalog pull request documents it on the catalog side.🤖 Generated with Claude Code