Skip to content

OPU-38: analyze a publishable package's own install hook - #108

Merged
MoSLoF merged 1 commit into
mainfrom
claude/depsnort-project-structure-5re5ui
Aug 25, 2026
Merged

OPU-38: analyze a publishable package's own install hook#108
MoSLoF merged 1 commit into
mainfrom
claude/depsnort-project-structure-5re5ui

Conversation

@MoSLoF

@MoSLoF MoSLoF commented Aug 25, 2026

Copy link
Copy Markdown
Owner

The gap

Scanning github.com/Medium/phantomjs (phantomjs-prebuilt@2.1.16) as its own repo produced zero install-hook findings despite "install": "node install.js" — a script that downloads a native binary, SHA-256-verifies it, and extracts it with extract-zip. ExtractInstallSurface's main loop is keyed on a node's npm.path attribute (set to . for the root or node_modules/name for a dependency by every lockfile-based resolver); a root node built from a bare package.json with no lockfile carries no npm.path at all, so the main loop's if relDir == "" { continue } guard silently skips it.

Correct for the common case (scanning your own application — your own scripts are dev tooling, not supply-chain risk), wrong for evaluating a package's own risk profile before adding it as a dependency or before publishing it — exactly the install hook that runs on every consumer's npm install.

The fix

A new pass after the main loop, gated on three conditions: the node is a registered graph root, it has no npm.path (not already handled by the main loop), and its package.json does NOT carry "private": true (application roots' postinstall/prepare scripts are build steps, not consumer-facing install hooks). When all three hold, the root's install scripts and load-time entry module are analyzed exactly the way a lockfile-resolved dependency's are.

Review correction to the handoff's stated scope

The write-up claims this "does not affect lockfile-based scanning." True for package-lock.json, bun.lock, and yarn.lock's own root — confirmed by reading each resolver directly. Not true for pnpm-lock.yaml: pnpmlock.go's synthesized root never sets npm.path either. Built a live pnpm-lock.yaml fixture (private and publishable shapes) confirming this is a genuine, correctly-functioning bonus — pnpm-based publishable packages had exactly the same undisclosed gap — but the shipped FP calibration set (four VS Code extension repos, all package-lock.json-based) never exercised this code path.

Review finding — a real misattribution bug the shipped tests don't catch

The new section reads absRoot/package.json for every node satisfying the npm.path-empty condition, gated on roots[n.ID]. yarn.go's own documented design — a yarn tree "contributes no npm.path" for its dependency nodes either — means a yarn.lock-resolved graph can contain dependency nodes with an equally empty npm.path, sitting alongside the root. Without the roots[n.ID] gate (present and correct in the shipped diff, but untested), every such dependency node would have the root's own install.js misattributed to it. Added TestOPU38DoesNotMisattributeToNonRootNodes; mutation-proven — removing the guard reproduces the exact misattribution.

Review finding — a vacuous test, for a benign reason

The shipped TestOPU38LockfileRootNotDuplicated claims to verify no double-analysis of an already-resolved root. Mutation-tested by removing its guard: the test still passed. Root cause, confirmed by reading graph.go: AddNode/AddEdge are both idempotent by ID, so re-running the analysis on identical content converges to the same graph state regardless of the guard — the assertion structurally cannot distinguish "ran once" from "ran twice, deduplicated." The guard is real and worth keeping purely as a performance optimization, just not provable the way the test claimed. Corrected the comment rather than leaving a misleading assertion standing, and pointed to the new discriminating test.

Validation

  • gofmt -l clean, go build ./..., go vet ./... silent.
  • Full suite green (34 packages), -race clean on internal/ecosystem/npm.
  • Live CLI validation against an independently-built reproduction of phantomjs-prebuilt's actual install chain (download via request, SHA-256 verify, extract-zip) confirms VC-002b fires and VC-002f does not (the download-then-extract-a-shipped-binary shape matches esbuild's D-25/D-28 non-cradle precedent) — exit 0 without -fail-on-eligible, exit 2 with it, matching the handoff's own before/after table exactly.
  • A live pnpm fixture confirms the previously-undisclosed pnpm impact surface behaves correctly in both directions (private stays clean, publishable fires).

Residual limitations (carried forward from the handoff)

  • Only npm is covered — PyPI/RubyGems/Cargo have the analogous root-hook gap, unaddressed here.
  • "private": true is npm's own convention, not a semantic guarantee — a package setting it for unrelated internal reasons would be excluded; a workspace root omitting it is included.
  • A bare package.json with no lockfile still leaves the project's own declared dependencies unresolved — a separate, pre-existing coverage gap.

Files

  • internal/ecosystem/npm/installsurface.go — the new pass.
  • internal/ecosystem/npm/opu38_test.go — shipped tests, plus the new regression and corrected comment.
  • docs/DECISIONS.md — D-132.

Generated by Claude Code

A root project node built from a bare package.json with no lockfile
carries no npm.path attribute, so ExtractInstallSurface's main loop
silently skips its own install scripts -- correct when scanning your
own application, wrong when evaluating a publishable npm package's
own risk profile (the install hook that runs on every consumer's
npm install). Adds a pass after the main loop, gated on: the node is
a registered graph root, it has no npm.path (not already handled),
and its package.json does not carry "private": true (application
roots' build steps would FP at a high rate under the same scrutiny).

Review found the handoff's own stated scope was incomplete:
pnpmlock.go's synthesized root never sets npm.path either (only
npm.go's v1/v2/v3 lockfile path, bunlock.go, and yarn.go's root
actually do, contrary to the "does not affect lockfile-based
scanning" claim), so this patch also closes the same gap for
pnpm-lock.yaml-based publishable packages -- a genuine, correctly-
functioning bonus the shipped FP calibration set never exercised.

Also found a real misattribution bug the shipped tests didn't catch:
yarn.go's own documented design means a yarn.lock-resolved graph can
contain non-root dependency nodes with an equally empty npm.path,
sitting alongside the root. Without the roots[n.ID] guard (present
and correct in the shipped diff, but untested), every such dependency
node would have the root's own install.js content misattributed to
it. Added a mutation-proven test reproducing exactly that scenario.

Separately found the shipped duplicate-detection test was vacuous for
a benign reason: graph.AddNode/AddEdge are both idempotent by ID, so
the test cannot distinguish "ran once" from "ran twice, deduplicated"
regardless of the guard it claims to verify. The guard is real and
worth keeping as a performance optimization, just not provable the
way the test claimed; corrected the comment rather than leaving a
misleading assertion standing.

Live CLI validation against an independently-built reproduction of
phantomjs-prebuilt's actual install chain (download via request,
SHA-256 verify, extract-zip) confirms VC-002b fires and VC-002f does
not, matching the esbuild D-25/D-28 non-cradle precedent, and matches
the handoff's own before/after exit-code table exactly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PLd1shywzWPsLgkpLxEyPj
@MoSLoF
MoSLoF merged commit 3092df3 into main Aug 25, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants