fix(pd001,pd002): resolve imports per workspace member and ignore type-only imports

[osfv]

What changed and why

Fixes both defects reported in #966. They land on the same comparison in the PD detectors, so they are in one change as suggested there.

A - workspace roots

Imports were collected from the whole tree while declarations were resolved against the root manifest only, so scanning a monorepo root reported every dependency a member declares and imports in its own source as a PD002 phantom (or PD001 when an override happened to exist).

• buildOverrideContext now discovers workspace members (root workspaces, pnpm-workspace.yaml) via a new readWorkspaceMemberManifests in src/utils/package-json.ts, reusing the same pattern expansion readDirectDependencyNames already uses for the CVE scan, and exposes ctx.workspaceMembers (dir + declared set).
• PD001/PD002 resolve each importing file against the nearest enclosing member first, then the root (root node_modules is on every member's resolution path, so a root declaration still satisfies all files). The finding lists only the files whose owning package leaves the import undeclared. Shared in phantom-utils.undeclaredImportFiles.

B - type-only imports counted as runtime

scanAllImports / scanProjectForPackageUsage are a regex pass over raw text, so /** @type {import('postcss-load-config').Config} */ counted as a dynamic import and import type { X } from 'pkg' counted as a runtime import.

• Comments (//, /* */) are blanked before matching, string-aware so 'https://...' is not treated as a comment.
• import type ..., export type ..., and specifier lists where every entry is type-prefixed are skipped. A default import that happens to be named type, or a mixed list (import { type A, b }), still counts.

This applies to every consumer of the scanner, as discussed in the issue: PD001, PD002, the OA009 guard, and the --usage reachability filter. One existing assertion in tests/usage.test.ts treated import type as usage and was updated accordingly; a type-only reference to a vulnerable package no longer counts as "used".

Verification

Against the minimal repro from the issue (pnpm workspace root, apps/web declares and imports js-yaml) plus a stock postcss.config.mjs with the JSDoc annotation, main reports two PD002 findings and this branch reports none. New unit coverage in tests/usage.test.ts, tests/overrides/detectors/pd00{1,2}.test.ts, and tests/overrides/context-builder.test.ts. Rule docs for PD001/PD002 gained a short "What counts as an import" section.

Note: #966 is assigned to @alamb-hex. The maintainer's check-in on Sept 6 had no reply, so I went ahead; happy to close this in favour of theirs if they are still working on it.

Closes #966

[sonukapoor]

Thanks for this, and for the diagnosis in particular. You read #966 correctly on both halves, and a few things here are better than what the issue asked for.

undeclaredImportFiles filtering the file list rather than short-circuiting the whole package is the right call. A package imported in five members and declared in three now reports only the two that are genuinely undeclared, and the "Imported in:" line stays honest. I also checked the case I most expected to be wrong, and it is not: import { type A, b } from 'p' is correctly treated as a runtime import while import type { A } from 'p' is erased. That distinction trips most people up. The ${member.dir}/ suffix stopping apps/web from claiming apps/web-admin is a nice catch too, and you tested it.

I checked the tests would actually fail if the fix were reverted rather than assuming it, and they do.

There is one thing I need fixed before this can land, and it is not in the part of the change you were focused on.

stripComments has no regex-literal state, and the top-level scan has no escape handling even though the string branch right below it does. That means the very common /\// idiom gets misread as the start of a line comment, and everything after it on that line is blanked. /[/*]/ is worse: it opens a false block comment that runs to the next */ or to the end of the file.

The reason I care about this more than the false positives we are fixing is that scanAllImports is not only feeding PD001 and PD002. It also feeds --only-used, which suppresses findings. So a lost import there does not produce noise, it produces silence. Concretely, with this branch:

const isAbs = /^\//.test(p);
const minimist = require('minimist');

put those on one line and minimist is reported as unused, so --only-used prints "No known vulnerabilities found" while minimist@1.2.5 still carries a critical. Removing just the regex from that line brings the finding back. A scanner that quietly hides a critical is a worse outcome than one that reports a phantom, so I would rather take the false positives for another week than ship this.

The fix is small. The string branch at line 63 already consumes \ plus the next character as a pair. The top level needs the same thing, so a \ before a / cannot open a comment. Full regex-literal tracking would be more correct, but escape awareness covers every case I could construct.

Three things then:

1. escape handling at the top level of stripComments, covering both the // and /* branches
2. a fixture with /\//, /https?:\/\// and /[/*]/ followed by a real import, both on the same line and on later lines
3. an end to end --only-used test asserting a genuinely required vulnerable package is still reported. There is no test for that path at all today, which is how this got past CI, and given it now gates a suppression decision it should have one regardless of this PR

Everything else I found is follow-up rather than a change request, and I will file it separately so it does not sit on you:

• OA009's guard still resolves declarations against the root manifest, so it is now out of step with PD001 in a workspace. Worth noting the pd001 doc page this PR edits still describes the two as paired.
• packages/** and pkg-* workspace globs are not matched by the existing pattern expansion, so those monorepos keep the false positives. Pre-existing, not yours.
• stripComments builds its result with per-character concatenation, which is roughly 11x slower than the old pass. Not urgent, but #837 is open on performance.

One design question I would like your view on rather than a change. Dropping import type is unambiguously right for --usage and --only-used, which model runtime reachability. I am less sure it is right for PD001 and PD002, which model declaration hygiene: a type-only import of an undeclared transitive package really will break tsc the moment the parent drops it, which is close to what PD002's own message warns about. The comment and JSDoc half of this has no such tension and is simply correct. If you think the cleaner shape is for scanAllImports to record whether an import was type-only and let each caller apply its own policy, I would be open to that, either here or as a follow-up.

To be clear on credit: @alamb-hex diagnosed both defects in #966 down to the exact comparison, and he will be credited as reporter in the release notes alongside you as author.

[sonukapoor]

This is the one blocking thing. There is no regex-literal state here and no escape handling at the top level, so an escaped slash inside a regex reads as the start of a comment.

/^\// is the characters / ^ \ / /. The loop emits / and ^, then emits the \ as an ordinary character, then lands on / followed by / and blanks the rest of the line.

const isAbs = /^\//.test(p); const lodash = require('lodash');
// -> "const isAbs = /^\ "

require('lodash') is gone. Same for /https?:\/\//, which is about as common as regexes get.

This matters more than it looks because scanAllImports also feeds --only-used, which suppresses findings rather than adding them. I reproduced a real require('minimist') being dropped this way and --only-used then reporting "No known vulnerabilities found" against a live critical advisory.

The string branch just below at line 63 already does the right thing:

if (c === "\\" && i < n) { out += source[i]; i++; continue; }

The top level needs the same treatment, so a \ consumes the following character and a \/ can never open a comment.

[sonukapoor]

Same root cause as above, wider blast radius. A regex character class containing a slash, like /[/*]/ or /[^/*]+/ in glob or path parsing, opens a block comment here that runs until the next */ or to EOF.

const re = /[/*]/;
import lodash from 'lodash';
import b from 'pkg-b';
// both imports are erased

Unlike the // case this is not line bounded, so one occurrence near the top of a file can remove every import below it. The same escape/regex awareness fixes both branches.

[sonukapoor]

This is exactly the right instinct, and it is the test that shows the gap: you covered // inside a string literal but not // produced by a regex, which is where the bug is.

Could you add a sibling case with /\//, /https?:\/\// and /[/*]/ followed by a real import, both on the same line and on a later line? And separately an end to end --only-used assertion that a genuinely required vulnerable package is still reported. Nothing covers that path today, which is why CI stayed green here.

[sonukapoor]

Filed the three follow-ups I mentioned, so none of them sit on this PR:

• fix(oa009): declaration guard resolves against the root manifest, diverging from PD001 #1118 OA009's declaration guard still resolves against the root manifest, so it diverges from PD001 once this lands
• fix(workspaces): packages/** and prefix globs are not matched by workspace expansion #1119 packages/** and prefix globs are not matched by workspace expansion, so those monorepos keep the false positives
• perf(usage): stripComments makes import extraction ~11x slower #1120 the stripComments pass is about 11x slower on import extraction

All three are ours, not yours. #1118 and #1120 both want to land after this PR since they touch code it introduces.

The only thing still on you is the regex handling in stripComments and a fixture covering it. No rush.