Guard the install a user gets, not the tree a contributor has - #257
Open
burakdede wants to merge 2 commits into
Open
Guard the install a user gets, not the tree a contributor has#257burakdede wants to merge 2 commits into
burakdede wants to merge 2 commits into
Conversation
`check:no-native` scanned the whole `node_modules` tree, so it could not tell a build tool in a contributor's checkout from a native dependency shipped to users. It refused Miniflare, a dev-only Cloudflare emulator that never reaches a published package, on the strength of a transitive `sharp`. Invariant 7 is a promise about someone installing Lorepack. The check now reads the production closure of the published packages, which is what a user gets, so it says something true and checkable rather than something broader and wrong. Narrowing a guard is only safe if the part that matters is proven to survive, so `tools/arch/test/no-native.test.ts` is the mutation rather than the happy path: a published package declaring a native dependency must fail it. Manifests are read as well as the installed closure, because the first catches a declaration the moment it appears in a diff and the second catches one that arrives transitively. `workerd` ships a post-install script, declined exactly as esbuild's is: the binary arrives through platform optional dependencies, verified by starting a Worker with live D1 and R2 bindings with the script refused. Closes #256
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.
Closes #256. Unblocks #86 and the rest of Phase 6.
The problem
check:no-nativescanned the wholenode_modulestree. It could not tell a build tool in a contributor's checkout from a native dependency shipped to users, so it refused Miniflare, a dev-only Cloudflare emulator that never reaches a published package, on the strength of a transitivesharp.Why narrowing is the honest fix
Invariant 7 is "no Python, Docker, compiler toolchain, native add-on, model download, API key or account. Ever." It is a promise about someone installing Lorepack.
sharpin a contributor'snode_modulesbreaks none of it.The check now reads the production closure of the published packages, which is exactly what a user gets. That makes it say something true and checkable instead of something broader and wrong.
I rejected the easier options on #256: an allowlist is a slope, and dropping the emulator would mean testing Worker handlers as plain functions, which cannot prove the Worker starts or the bindings work, and that is most of what #86 is for.
Narrowing a guard is only safe if you prove what survives
tools/arch/test/no-native.test.tsis the mutation, not the happy path. A test that only ran the check and expected zero would pass just as happily against a check that had stopped looking.Both halves are checked, and they catch different things: manifests catch a native dependency the moment it is declared, when a reviewer is looking at the diff; the installed closure catches one that arrives transitively, which no manifest mentions. My first attempt read only the installed tree, and the mutation test caught that a declared-but-uninstalled dependency slipped straight through.
The dependency this admits
wranglerandminiflare, dev-only, recorded independencies.mdwith the reasoning and the checks.workerdis native and ships a post-install script. It is declined, exactly as esbuild's already is in this repo, because the binary arrives through@cloudflare/workerd-<platform>optional dependencies instead. Verified by starting a Worker with live D1 and R2 bindings with the script refused, sopnpm install --ignore-scriptsstays a true statement about this workspace.What it buys, proven rather than asserted
packages/deploy-cloudflare/test/emulator.test.ts:Admitting a dependency to buy a capability and then not verifying the capability would be the worst of both, so that file exists.
Verified
pnpm verifygreen (17 checks), includingcheck:no-nativeon the narrowed rule and the three guard tests.