fix: run the pinned installer instead of the overlay's copy - #154
Open
andrepimenta wants to merge 1 commit into
Open
fix: run the pinned installer instead of the overlay's copy#154andrepimenta wants to merge 1 commit into
andrepimenta wants to merge 1 commit into
Conversation
`yarn install` was installing zero base skills for anyone with CONSENSYS_SKILLS_DIR set, and still exiting 0. tools/sync chose its installer with `-x "$SCRIPT_DIR/install"`. npm and Yarn do not preserve the executable bit when unpacking a tarball, so every tools/* file arrives 0644 in a consumer install and that test is always false there — while being always true in a git checkout, which is the only place it ever got exercised. The pinned installer was therefore never selected, and the fallback loop ran instead. That loop keeps the LAST matching source and SOURCES appends CONSENSYS_SKILLS_DIR after METAMASK_SKILLS_DIR, so the private overlay's copy of tools/install won every time. An overlay predating the `base:` frontmatter key has no notion of base skills, so it skipped all 16 as "excluded by --domain" and reported "Install complete." This is a regression from #135. Before that, sync preferred `$METAMASK_SKILLS_DIR/tools/install`, which the CLI defaults to the .skills-cache clone — a git checkout, so the bit survived and the first branch always won. #135 correctly stopped executing code from that unpinned clone, but replaced the condition with one that cannot hold in the artifact it was meant to protect. The misrouting to the overlay had been latent since the fallback was introduced; it only became observable once `base:` gave the two installers different behaviour. Test `-f` and invoke via `bash`, so the bit is irrelevant either way. Applied the same treatment to tools/bootstrap and tools/deploy, which exec a sibling installer directly for the same reason. The regression test builds the fixture that reproduces it — package installer at 0644 as npm ships it, overlay installer at 0755 as git checks it out — and asserts the pinned one runs. Reverting only tools/sync fails it with `STALE-OVERLAY`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Summary
yarn installinstalls zero base skills for anyone withCONSENSYS_SKILLS_DIRset, and reportsInstall complete.while doing it. Found while testing 0.3.0 end to end from the published package in metamask-mobile.This is a regression from #135 — my own change.
Root cause
tools/syncpicked its installer with an executable test:npm and Yarn do not preserve the executable bit when unpacking a tarball, so every
tools/*file arrives0644in a consumer install:So the guard is always false in the published package and always true in a git checkout — which is the only place it was ever exercised. The pinned installer was never selected. The fallback loop ran instead, and since it keeps the last match while
SOURCESappendsCONSENSYS_SKILLS_DIRafterMETAMASK_SKILLS_DIR, the private overlay'stools/installwon every install.An overlay predating the
base:key has no notion of base skills, so it skipped all 16 asexcluded by --domainand exited 0.Deterministic on a real checkout — identical command either side:
Why it survived review
Two conditions had to line up, and #135 supplied both:
base:is the first flag it doesn't implement, turning a silent misroute into zero skills.Before #135, sync preferred
$METAMASK_SKILLS_DIR/tools/install, which the CLI defaults to the.skills-cacheclone — a git checkout, so the bit survived and the first branch always won. #135 was right to stop executing code from that unpinned clone, but swapped in a condition that cannot hold in the artifact it was protecting.It cannot reproduce from a source checkout, so neither CI nor local dev would ever see it.
Changes
tools/sync-f,exec bash "$INSTALL_BIN"tools/bootstrap,tools/deploytest/cli.test.mjsThe test builds the fixture that reproduces it — package installer at
0644as npm ships it, overlay at0755as git checks it out — and asserts the pinned one runs. Reverting onlytools/syncfails it withSTALE-OVERLAY.Testing
yarn test— 70/70 (69 + the new one)yarn lint,yarn lint:changelog— clean0.3.0in metamask-mobile:chmod +xon the package tools restores all 16 base skillsFollow-up
Needs a 0.3.1 — 0.3.0 cannot deliver base skills to overlay users. metamask-mobile#35263 is pinned at
^0.3.0and picks the patch up automatically.Also worth noting: the supply-chain property #135 claimed has never actually held for a consumer. Until this lands, every install executes
tools/installfrom the git cache or the overlay rather than the lockfile-pinned package.