fix: point the site art record at a commit that exists - #24
Conversation
checkRecord walked the overlap of the directory and the record, so dropping a view from both left matching subsets and passed, and a stray file whose entry omitted `render` passed too: VIEWS[name] and entry.render were both undefined, so the comparison fell through to a sha256 that matched. It now requires both sides to hold exactly the three views.
#22 recorded brand commit 03aeae1, the head of the branch it packaged from. That branch was squash-merged as 4c36b2e and deleted, so the recorded commit is unreachable and `--check --brand` fails against any fresh brand clone: the record names a commit nobody can resolve, which is the failure the record exists to prevent. Both scripts are byte for byte the same at 4c36b2e. --repoint moves the record onto another commit, but only one holding the recorded scripts unchanged, so a squash merge no longer strands it.
📝 WalkthroughWalkthroughThe site art script now requires directory, record, and Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to Maintainers with an unfetched brand checkout can be unable to repoint the record after merge. Fetch the remote before resolving the target commit. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (2 skipped: 2 unsupported.)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
apps/website/src/assets/art/README.md-65-65 (1)
65-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFetch
origin/mainbefore resolving the commit.If
$BRANDwas fetched before the merge, localorigin/maincan still select the pre-merge commit.repointthen rejects that commit when its scripts do not match the recorded hashes, so the documented workflow fails instead of updating the record.Proposed fix
+git -C "$BRAND" fetch origin node scripts/site-art.mjs --repoint "$(git -C "$BRAND" rev-parse origin/main)" --brand "$BRAND"
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: 7b8ad3d7-d46e-4989-acfb-e187477550b2
📒 Files selected for processing (4)
apps/website/scripts/site-art.mjsapps/website/src/assets/art/README.mdapps/website/src/assets/art/site-art.jsonapps/website/test/site-art.test.mjs
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
Two follow-ups to #22, one of which is broken on
mainright now.The record names a commit nobody can resolve. #22 recorded brand commit
03aeae1, the head of the branch it packaged from. origin89hq/brand#15 was squash-merged as4c36b2eand its branch deleted, so03aeae1is unreachable andnode scripts/site-art.mjs --check --brandfails against any fresh brand clone. A record pointing at a commit that does not exist is the exact failure the record was added to prevent. Both scripts are byte for byte identical at4c36b2e, so the record moves there and verifies again.--repoint <commit> --brand DIRperforms that move: it reads both recorded scripts at the candidate commit, requires their hashes to match what the record already holds, and only then rewritesbrand.commit. The image hashes never change. Pointed at a commit that predates or rewrites the scripts it refuses and leaves the record untouched. The README now covers this as the step after the brand PR merges, since any squash merge will strand the packaged commit.checkRecordwalked the overlap of the directory and the record, so two cases passed that should not have: dropping a view from both sides left matching subsets, and a stray file whose record entry omittedrenderpassed becauseVIEWS[name]andentry.renderwere bothundefined, sending the comparison through to a sha256 that matched. It now requires both sides to hold exactly the three views. Found by CodeRabbit on #22, after that PR had merged.Validation:
just checkon this branch, plus--checkand--check --brandagainst the committed record, which now resolves4c36b2e. Each new guard was mutation-tested — reverting it fails its own test and nothing else.