Repository navigation
ci(gate): every imported package must be declared by the package that ships it (#4394) - #4407
Merged
Merged
Conversation
… ships it (#4394) A bare specifier RESOLVING from a package directory says nothing about whether that package DECLARES it. The workspace root carries `devDependencies.react`, so a `node_modules/react` symlink sits at the root and Node's — and TypeScript's — upward walk reaches it from every package directory: require.resolve('react', { paths: ['packages/core/src'] }) -> resolves @object-ui/core declares 'react' in -> no field at all So a dependency-direction check performed by resolution returns the wrong answer confidently, and the disagreement stays silent until a consumer's install. `pnpm check:phantom-deps` asks the other question, over `src/**` of every package in the changeset `fixed` group, in two tiers: a RUNTIME file must name its imports in a field a consumer installs (`dependencies`/`peerDependencies`/ `optionalDependencies`), while a test/benchmark file — which nothing published resolves — may also be answered by the package's own `devDependencies` or the workspace root's. Type-only imports are strict, node builtins need `@types/node`, host-injected modules (`vscode`) earn the import by declaring their types plus `engines`, and the one bundled application is an exemption whose two properties are re-derived on every run. The first full run was not green: `@object-ui/plugin-detail` imports `react-router-dom` in two renderers and declared it nowhere, while its three siblings using the same hooks all declare the same peer. Its rollup config externalises every bare specifier, so the published `dist/index.js` carried an import the manifest never mentioned. Fixed here with the siblings' range, which is why the gate lands enforcing with no baseline ledger. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
…ntom-dependency-gate
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
ACCEPT — PM 复核 (session
Flipping ready + arming auto-merge. Generated by Claude Code |
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
2 participants
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.
Fixes #4394
The trap being closed
A bare specifier resolving from a package directory says nothing about whether that package declares it. The workspace root carries
devDependencies.react, so anode_modules/reactsymlink sits at the root and Node's — and TypeScript's — upward directory walk reaches it from any package directory. The card's own probe, re-run on this branch's tree:pnpm's isolated
node_modulesprevents phantom dependencies between workspace packages; it does not defend a package against the root's own dev dependencies. So a dependency-direction check performed by resolution — the obvious way to "measure the actual import graph", which is what #4389's brief asked for — returns the wrong answer, confidently, and the disagreement stays silent until somebody else's install.pnpm check:phantom-depsasks the other question, the one root hoisting cannot answer for anyone: is it declared.Violation-surface reading, and the landing it picked
Measured first, over
src/**of all 40 packages in the changesetfixedgroup — 2641 files, 12664 module specifiers. The full reading, by tier:main@types/node__benchmarks__, and core declaresexports)Landing: enforcing, no baseline ledger. The runtime surface was a small fixable set, and both entries are resolved in this PR rather than baselined:
@object-ui/plugin-detail->react-router-dom, 2 sites — a real defect, fixed.PermissionFacetLink.tsx:10andrecord-reference-rail.tsx:22takeLinkanduseParamsfrom the router; the manifest named it in no field at all, while all three siblings using the same hooks (app-shell,layout,plugin-designer) declare it as a peer of^6.0.0 || ^7.0.0. Not latent: that package's rollup config externalises every bare specifier (external: (id) => !/^[./]/.test(id) && !id.startsWith(__dirname)), so the publisheddist/index.jscarried an import the manifest never mentioned. Fixed with the siblings' range — a fourth spelling of the same claim is the packages/react-runtime 的 peerDependencies.react 是无上界的>=18,允许尚不存在的 React 20+ #3741 mistake — plus apatchchangeset, since a published manifest gaining a peer is user-visible.object-ui(the VS Code extension) ->vscode, 5 sites — graded, not fixed. The extension host injects it; it is not a runtime npm package and declaring it as a dependency would produce an uninstallable package. Handled as aHOST_PROVIDEDentry that the package earns by declaring@types/vscodeandengines.vscode, both re-checked on every run.The tooling tier's 1873 is not baselined either, and the reason is in the shape of the number: 74 of its 76 pairs are exactly
vitest,@testing-library/react,@testing-library/jest-dom,@testing-library/user-event— one coherent convention, this repo installing its test toolchain once at the root. Demanding thirty packages re-declare those is churn that protects no consumer, because nothing published resolves those files. So a tooling file may be answered by the package's own fields or the workspace root's, and the size of that concession is printed on every run (1878 tooling import(s) are declared only by the workspace root) rather than absorbed. What it still catches is the real thing: a test import of a package declared nowhere.Pre-fix RED (#4118)
Two directions, both captured. First, the card's exact synthetic case — a throwaway
packages/core/src/__phantom_probe_4394__.tscontainingimport { useState } from 'react'plusimport type { ReactNode } from 'react', created and deleted without touching any existing file, and not present in this branch:That is the false-GREEN in one frame:
tscreports zero diagnostics — notTS2307— because it asked whether the module resolves, and it does.Second, and stronger, the gate was red on unmodified
main— this class was already live, so the pre-fix red needed no synthesis:Reverse verification
Predicted direction first, then run:
Cannot find module '../check-phantom-dependencies.mjs',0 test, exit 1. Restored.plugin-detailfix back out (git checkout origin/main -- packages/plugin-detail/package.json) -> gate exit 1, and exactly two self-tests red (has no phantom dependency,pins the defect this gate found), 40 of 42 still passing. Restored.node:fsis not a legal package name — npm allows no:— so asking the package-name grammar before the builtin check filed all 127node:-prefixed imports as path aliases and graded none of them. The verdict did not move; only a counter did (alias=127, builtin=48instead ofalias=0, builtin=175), which is why the self-test pins it by counter.The gradings, each stated rather than buried
.d.tskeeps the reference, so a consumer type-checking against the package needs those types installed; it is one edit from becoming a value import with nothing reporting the promotion; and a carve-out here is the tolerant-consumer shape AGENTS.md #0.1 rejects. Measured cost today: zero — none of the 7 runtime violations was type-only, so strictness is free at the moment it lands.@types/nodein any of the package's own fields,devDependenciesincluded — a types package is never a runtime import, sodependencieswould be the wrong field. Measured: every builtin import in a package lacking it is in a tooling file (33 pairs, all of them); every runtime builtin import (@object-ui/cli'sfs/path/url/child_process) is in a package that declares it.@/…is skipped by GRAMMAR, not by an alias list — npm has no empty scope, so it cannot name a package.@object-ui/consoleis the one exemption, and it is re-derived every run: an application build (vite build, nobuild.lib, norollupOptions.external) inlines its ~325 devDependency imports into thedist/bundle it publishes. If either property changes the exemption fails loudly instead of quietly covering a package that has started shipping bare imports.Stated scope
src/**of the changesetfixedgroup, read from.changeset/config.jsonfor the reasoncheck-changeset-presence.mjsgives — a hand-written glob missesapps/console. Not checked: the reverse direction (a declared dependency nothing imports), files outsidesrc/, and layering — "may@object-ui/coreimport React at all" is AGENTS.md §3 and #4389, and a core test importing React satisfies this gate while still being wrong. Answering a second question badly is worse than not answering it.One honesty correction made during review: an earlier draft of the header claimed every build program excludes tests or emits nothing from them. #4006 has measured the opposite (73
*.test.d.tsin publisheddist/), and three packages listsrcinfiles. The claim that actually holds — and the one the tooling tier rests on — is that nothing resolves those files, so no install can fail on them. Header corrected to say that and to point at #4006; measurement added as a comment there rather than as a twin issue.Verification
node scripts/check-phantom-dependencies.mjs— green on the merged tree (40 packages, 2648 files, 12700 specifiers).pnpm exec vitest run scripts/__tests__/— 39 files, 893 tests passed, including the new 42-test self-test andci-cd-pipeline-doc.test.ts, which pins the doc job-table row againstci.ymlin both directions.pnpm type-check:scripts— clean (BundledApplicationtypedef added so the injectable exemption table types throughallowJs).pnpm exec eslinton both new files — clean.node scripts/check-control-bytes.mjs— OK; plus agrep -naPself-scan of every touched file for control bytes beyond the gate's surface (none).node scripts/check-changeset-presence.mjs/check-changeset-fixed.mjs/check-changeset-no-major.mjs— green.ci.ymledit: parses; the new step sits at index 7 of thetype-checkjob, immediately afterpnpm install --frozen-lockfileat index 6, carrying the sameif: steps.relevant.outputs.should_run == 'true'guard as its siblings.pnpm install --frozen-lockfile— clean, so the 3-line lockfile change is in sync.pnpm exec vitest run packages/plugin-detail/src/__tests__/PermissionFacetLink.sduiCarrier.test.tsx— 2 passed (the renderer whose import was undeclared).Risks
.gitattributesroutespnpm-lock.yamlthrough thepnpm-mergedriver, so a concurrent branch conflicts textually rather than silently.type-check, so a phantom import now fails the job before the slower gates — intended, but it changes which red a contributor sees first.Generated by Claude Code