Skip to content

test(bin-e2e): dlopen the shipped js plugin cdylib for real - #370

Closed
raphaelvigee wants to merge 51 commits into
raphaelvigee/feat-plugin-js-m7-benchfrom
raphaelvigee/feat-plugin-js-m8-bin-e2e
Closed

test(bin-e2e): dlopen the shipped js plugin cdylib for real#370
raphaelvigee wants to merge 51 commits into
raphaelvigee/feat-plugin-js-m7-benchfrom
raphaelvigee/feat-plugin-js-m8-bin-e2e

Conversation

@raphaelvigee

@raphaelvigee raphaelvigee commented Aug 5, 2026

Copy link
Copy Markdown
Member

Part 8/8 of the stack — closes a gap flagged repeatedly since M0: plugin-js-cdylib was lint/fmt covered (devenv.nix's qualityCrates) but never actually loaded across the real ABI seam by any test.

What's here

  • devenv.nix's e2e script now stages plugin-js-cdylib as a fourth artifact, symmetric with go/gha in both the local-build and HEPH_E2E_FROM/CI branches (fingerprinting, copying, macOS-portable-patching all updated together).
  • Four new real bin-e2e tests in crates/bin-e2e/tests/plugin_dylib_js.rs:
    • shipped_js_cdylib_loads_and_answers_across_the_abi — construction + the sync inspect functions ABI call round-trips cleanly.
    • shipped_js_cdylib_discovers_a_real_workspace_packagequery -e //... against a real pnpm-shaped fixture resolves //packages/foo:package_info.
    • shipped_js_cdylib_resolves_pnpm_workspace_glob_membership — proves a package outside the packages/* glob is never admitted as a workspace member, across the real seam.
    • js_plugin_construction_failure_logs_before_the_abort — the same log-sink-before-abort regression coverage the go test already has.

Two things review caught worth naming

The first draft of shipped_js_cdylib_loads_and_answers_across_the_abi asserted inspect functions produced completely empty stdout — wrong, and it failed deterministically: the fs builtin provider's own functions are always registered regardless of plugin config, so stdout is never actually empty. Fixed to assert no line starts with js. instead (the js Provider never overrides functions(), unlike go's build_addr).

The discovery test's own doc comments originally claimed to exercise pnpm's workspace-glob membership resolution — they didn't. Traced it: Provider::list/list_packages discover packages by package.json presence alone, independent of pnpm-workspace.yaml content entirely; the glob-resolution path (workspace.rs's resolve_members) is only reached from Provider::get, which a plain query -e //... never calls. Rather than leave an overclaiming comment standing, added the dedicated shipped_js_cdylib_resolves_pnpm_workspace_glob_membership test that actually reaches that path, and corrected the original test's docs.

Verified for real, not just compiled

Ran the full e2e devenv script end to end — release build of heph + all three plugin cdylibs, staged, macOS-portable-patched, full bin-e2e suite against real dlopen'd artifacts:

tests/cli.rs:            4 passed
tests/plugin_dylib.rs:    4 passed
tests/plugin_dylib_js.rs: 4 passed  (all new)
tests/shell_pty.rs:       3 passed
tests/tui_pty.rs:         2 passed
17/17 total

Test plan

  • cargo build -p bin-e2e --tests
  • cargo clippy -p bin-e2e --all-targets -- -D warnings (clean)
  • cargo fmt --check -p bin-e2e
  • nix-instantiate --parse devenv.nix
  • Full e2e run against real staged release artifacts — 17/17 passed
  • CI (tst, lint, bin_e2e) on this PR

Stack created with GitHub Stacks CLIGive Feedback 💬

@raphaelvigee
raphaelvigee force-pushed the raphaelvigee/feat-plugin-js-m8-bin-e2e branch from a1bfe40 to 5380ba2 Compare August 7, 2026 11:21
@raphaelvigee
raphaelvigee marked this pull request as ready for review August 7, 2026 12:57
@raphaelvigee
raphaelvigee force-pushed the raphaelvigee/feat-plugin-js-m8-bin-e2e branch from 5380ba2 to 0ec0d6c Compare August 7, 2026 12:58
@raphaelvigee
raphaelvigee force-pushed the raphaelvigee/feat-plugin-js-m8-bin-e2e branch from 0ec0d6c to 32f0499 Compare August 7, 2026 16:36
@raphaelvigee
raphaelvigee force-pushed the raphaelvigee/feat-plugin-js-m8-bin-e2e branch from 32f0499 to 538a871 Compare August 7, 2026 18:31
@raphaelvigee
raphaelvigee force-pushed the raphaelvigee/feat-plugin-js-m8-bin-e2e branch from 538a871 to 20f1382 Compare August 7, 2026 19:56
raphaelvigee and others added 19 commits August 10, 2026 11:12
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…out (#392)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…de` knob (#393)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…it in (#387)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
First-party JS/TS ecosystem plugin (M0+M1), mirroring the plugin-go /
plugin-go-cdylib two-crate split (logic crate + thin stabby cdylib wrapper).

- Provider: package.json discovery, pnpm-workspace.yaml + npm "workspaces"
  member resolution.
- js_install: hermetic per-(name,version,integrity) dependency fetch, SRI
  verification, platform baked into the cache key, postinstall scripts off
  by default with an explicit allowlist enforced before any network I/O.

Reviewed by feature-quality/code-quality/hermeticity; a tar-slip symlink
escape and an uncacheable (host-toolchain-dependent) lifecycle-script hash
were found and fixed, each with a regression test.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
Temporary, will be reverted once the real cause is identified. Prints
what `cargo update` would change in CI's actual environment before the
--locked build, since every local reproduction attempt (native darwin,
explicit --target, full --workspace, cleared cache forcing a fresh
index) succeeds while CI fails identically and fast on every retry.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
…ke --locked builds

Revert the temporary CI diagnostics now that the root cause is confirmed.
M2 of the JS/TS plugin: fixes the deferred optionalDependencies hard-fail
(platform-restricted optional deps now silently skip instead of failing
Provider::get), then adds a real import-graph resolver on top of M1's
package.json-declaration wiring.

- oxc_parser + oxc_resolver extract and resolve import/require/dynamic-import
  specifiers per real Node condition-set semantics (separate ESM/CJS/types
  resolvers), building two distinct graphs (runtime vs type-only edges).
- Phantom-dependency detection: an import resolving into a package not in the
  declared-dependency closure (deps + devDeps + peerDeps) is a hard error
  naming the file/specifier/package, hermetic against a fresh checkout (no
  ambient node_modules required) via a bare-specifier name check.
- Conformance corpus (crates/plugin-js/src/pluginjs/conformance.rs) covering
  exports-map condition ordering, wildcard specificity, array fallbacks,
  null-blocked subpaths, self-referencing imports, and the "imports" field —
  cross-checked live against a real Node binary when present, self-gated
  otherwise.

Reviewed by feature-quality/code-quality/hermeticity. Four BLOCKERs found and
fixed: the phantom-dep check being a no-op without ambient node_modules,
peerDependencies never counted as declared, an oxc_resolver default reading
the ambient NODE_PATH env var, and a resolved-but-unclassifiable edge
silently passing instead of failing closed. Each has a regression test.

Explicitly deferred (named, not silent): import-equals require() and
require.resolve() extraction, per-specifier `type` modifier detection,
provider-lifetime resolver caching (currently rebuilt per Provider::get
call — a stated perf follow-up), and pinning a hermetic Node toolchain so
the conformance corpus's live cross-check is guaranteed to run in CI rather
than opportunistically.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
…rde_json's Map ordering

oxc_resolver (a new plugin-js dependency) requires serde_json's
preserve_order feature unconditionally, which is a workspace-wide Cargo
choice. That silently flipped serde_json::Map from an always-sorted
BTreeMap to an insertion-order IndexMap everywhere, including in
plugin-oci's manifest/config json! literals, which relied on that
implicit sort for canonical, cache-key-stable output.

Sort explicitly at encode time instead of depending on which Map
backing happens to be active.
…uirk that changed after v18

array_exports_matched_entry_missing_on_disk_hard_fails_no_fallback's live
cross-check was authored against v18.12.1's --experimental-import-meta-resolve.
Later Node no longer checks file existence for array-form exports entries
during resolution, deferring it to module load — so any newer Node found on
PATH now resolves where the hard-coded expectation says it should throw.

plugin-js's own resolver (the assertion that actually matters here) still
implements the documented algorithm correctly and is unaffected; only the
live Node cross-check is dropped for this one fixture.
M3 of the JS/TS plugin: a cacheable js_typecheck ManagedDriver running
tsc --noEmit per package, fed by the M2 import graph rather than blind
package.json declarations.

- Toolchain: tstool=host (only supported mode) resolves tsc from
  node_modules/.bin or PATH, queried once per Provider lifetime and hashed
  alongside the tsconfig content — disclosed non-hermetic escape hatch,
  same shape as the design doc's stated M1+ gap.
- Input scoping fixed through review to actually match what tsc reads:
  plain (non-type-only) cross-package imports, not just `import type`;
  third-party .d.ts inputs resolved via the same lockfile mechanism as
  js_install (works with no ambient node_modules, not just when one
  happens to exist on disk); tsconfig include/exclude honored for
  first-party sources; the full tsconfig extends chain declared and
  hashed; a shared/ancestor tsconfig with unscoped include is now a loud
  Provider::get error instead of a silently unsound cache key.

Reviewed by feature-quality/code-quality/hermeticity — five BLOCKERs found
(all variations on "the declared Input set doesn't match what tsc actually
reads") and fixed, each with a regression test proving the specific
divergence. Tests requiring a real tsc binary are #[ignore]d with a named
reason rather than silently skipping and reading as a pass.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
M4 of the JS/TS plugin: a cacheable js_test ManagedDriver with one target
per test file (vitest default, jest alt via a single testrunner config
option), fed by the M2/M3 import graph rather than package-level caching —
the differentiator the design doc calls out over Turborepo/Nx, which never
get finer than per-package task caching.

- Per-test-file Input scoping: the test file's own runtime-transitive
  closure (BFS over ImportGraph::runtime_edges, bounded to the owning
  package), third-party deps resolved via the same lockfile-driven
  mechanism js_install/js_typecheck use (no ambient node_modules
  dependency, per the M3 lesson), and the resolved test-runner config.
- Runner-config discovery covers vitest's own fallback (vite.config.*) and
  jest's package.json "jest" field, not just the dedicated config
  filenames.

Reviewed by feature-quality/code-quality/hermeticity. Three BLOCKERs found
and fixed: a js_test addr's file= argument accepted an absolute path or a
`..`-escape with no validation, letting a target read/exec outside the
workspace and sandbox entirely (fixed at both Provider::get and
defensively again in the driver's run()); vitest/jest config-referenced
files (setupFiles, globalSetup, a shared base config reached via a
relative import) were untracked, so editing one didn't bust the cache of
every test that depends on it; and the primary runner-config file itself
could go undiscovered for the vite.config.*/package.json-jest-field
layouts. Each has a regression test.

Explicitly deferred (named, not silent): per-test-file Provider::get
rebuilds the whole package's import graph from scratch rather than
caching it once per package (a real warm-cache-path cost); the
cross-package one-hop trim now risks a stale test-pass cache hit, not
just a missed diagnostic, for a barrel-file re-export; test-runner-specific
alias resolution (vitest resolve.alias, jest moduleNameMapper) isn't fed
into the resolver.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
M5 of the JS/TS plugin.

Perf fix (prerequisite): deps_config, typecheck_config, and test_config
each rebuilt the whole package's ImportGraph from scratch on every
Provider::get call — for js_test, once per test file. Flagged independently
by both the M2 and M4 reviews and deferred twice. Added a per-package
memoized cache (keyed OnceCell behind a Mutex whose critical section is
just the get-or-insert, so unrelated packages never serialize behind one
lock) shared by all import-graph consumers, plus a call-count test proving
it actually memoizes. Also added the equivalent cache for workspace-member
discovery, the same O(P) redundant-walk shape found alongside it.

js_lint: cacheable per-package target, oxlint default + eslint (with
type-aware parserOptions.project support) via a single linter config
option — same naming rule as every other driver here. Reuses the lockfile-
driven third-party resolution and tsconfig-extends-chain handling already
established for js_typecheck/js_test.

Reviewed by feature-quality/code-quality/hermeticity — five BLOCKERs found
and fixed: a fabricated package.json-config fallback that doesn't match
how oxlint/eslint actually discover config and would break real
invocations; an unconditional hard-fail on packages with zero lintable
source files (now a clean no-op); only the first parserOptions.project
entry in a multi-entry eslint config being tracked, silently dropping the
rest from the cache key; an unvalidated parserOptions.project path letting
a repo-controlled eslint config make heph read and hash an arbitrary host
file (e.g. /etc/hostname) with no workspace-containment check — a real
escape, now a hard error instead of a silent fallback; and eslint configs'
own relative-path extends/imports (a shared base config) going untracked,
the same class of gap already fixed once for js_test's runner config.

Explicitly deferred: converging graph_cache/tsc_cache/testrunner_cache/
linter_cache onto the repo's existing hmemoizer primitive for panic
containment across the ABI seam (pre-existing pattern, not a new
regression, scoped as its own follow-up); a same-package concurrent-race
test for the new cache (single-flight correctness argued from tokio's
OnceCell contract, not yet proven under real concurrency); no bin-e2e
coverage for any JS driver's dlopen/ABI-crossing seam yet.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
M6 of the JS/TS plugin, the final milestone: a cacheable js_bundle
ManagedDriver wrapping esbuild, with format (esm/cjs) and target
(node/browser) as plain addr args resolved with a flat default rather
than porting the Go plugin's ancestry/universe variant machinery — this
plugin's addr model is a single flat workspace, so there's no ancestor
chain or cross-subtree variant-pin problem for that machinery to solve
(researched and confirmed against crates/plugin-go/src/plugingo/variant.rs
and this crate's own flat workspace.rs model before deciding).

Whole-graph cache key by design (the entry point's full transitive
closure via ImportGraph::runtime_edges, cross-package recursion unlike
js_test's one-hop trim), third-party deps resolved via the same
lockfile-driven mechanism as every other driver, tsconfig (plus its
extends chain) declared and staged the same way js_typecheck does.

Reviewed by feature-quality/code-quality/hermeticity. Two functional
BLOCKERs found and fixed: the discovered third-party import closure was
never wired into esbuild's --external flags, so bundling any package with
a real npm runtime dependency failed outright; and the output directory
didn't vary by format/target, so esm and cjs variants of the same package
collided on the same declared output path. A third BLOCKER (missing
tsconfig Input) meant path-aliased/JSX/decorator-configured TS entry
points silently failed or cached wrong. All three fixed with regression
tests, including a real-esbuild end-to-end proof for the external-deps fix.

Explicitly deferred (disclosed in module docs): target=browser doesn't
yet change what's resolved (only what esbuild is told to assume) since
the resolver has no browser condition/main-field support; a
config-reference-scanning helper shared with js_test/js_lint reads a
config-referenced file before checking workspace containment rather than
after (dormant for js_bundle's only supported config format today, live
for js_test already, flagged as a cross-cutting follow-up not specific
to this milestone); rollup/webpack/vite bundlers.

Note for follow-up: the design doc's v1 scope table lists six drivers
including js_format, but the Milestones list only ever defined M0-M6 for
the other five (install/typecheck/test/lint/bundle) — js_format has no
milestone and isn't built. Flagged in plugin-js-cdylib's module doc; needs
either an M7 or a corrected scope table.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
raphaelvigee and others added 18 commits August 12, 2026 23:58
…here

Live testing turned up a fourth failure mode, distinct from the prior
three ambiguity/collision fixes: some packages (ts-log,
ts-interface-checker, tailwindcss-scoped-preflight, @types/react in a
real workspace) have exactly one lockfile entry, and it has no
integrity/resolved at all — not a dedup/collision artifact. Root cause
confirmed against npm's own issue tracker: `npm install` (unlike `npm
ci`) can satisfy a package from its local cache and strip
resolved/integrity from an existing package-lock.json entry instead of
repopulating them — a known npm CLI bug (npm/cli#4263, #4460, #6301),
not a heph bug.

There's nothing safe to fetch+verify against in this case, so
js_install correctly still fails — but it did so with "no recognized
integrity algorithm in \"\"", surfaced deep inside the SRI parser with
no indication of cause. Move the check up to where the js_install spec
is built and name the actual cause plus the fix (regenerate the
lockfile).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…n't block

Per explicit product decision: reverts the previous commit's hard-fail
for a package with no integrity anywhere in the workspace's lockfiles.
That fail-closed behavior is the textbook-correct hermeticity default,
but the actual condition (npm/cli#4263: `npm install` stripping
resolved/integrity from an existing lockfile entry when a package is
satisfied from local cache — not a heph bug) hit hundreds of packages
across a real monorepo, with no fix available except regenerating
lockfiles the user doesn't control the timing of.

js_install now installs unverified when `integrity` is empty, logging
a `tracing::warn!` so it's diagnosable, rather than blocking the
build. This is a deliberate, recorded hermeticity trade-off, not an
oversight — see driver_install.rs's module doc and
fetch_and_extract's doc for the full reasoning.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…lifecycle scripts

Two more live-testing bugs, both in driver_install.rs:

1. extract_tarball: a real published tarball (pngjs@7.0.0) ships an
   explicit directory entry (`lib/`) with mode 0666 — no execute bit —
   followed by files inside it. `Entry::unpack` applies that mode
   immediately per-entry, so the very next entry (a file inside that
   directory) fails to be created: Unix file creation is gated on the
   parent directory's execute bit, not the child's own mode. Upstream
   `tar::Archive::unpack` avoids this by deferring all directory
   permissions to the end (tar-rs#242); that API can't be used here
   (needs per-entry `package/` root stripping), so instead directory
   entries never get their archive-recorded mode applied at all —
   matching this codebase's own `hartifactcontent::unpack`, which
   already only ever adds permissions during extraction, never narrows
   them from untrusted archive data.

2. run_lifecycle_scripts: `Command::status()` inherits the parent's
   stdio by default, so an allow-listed package's own lifecycle
   script writing to stdout/stderr went straight to the real terminal
   underneath heph's TUI (which owns the alternate screen for the
   whole run) — corrupting it. Switched to `.output()`: captured, not
   inherited, and a failing script's captured output now lands in the
   error instead of vanishing into whatever the terminal happened to
   be showing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…os/arch

`goos`/`goarch` were copied verbatim from the Go plugin's own vocabulary
(Go's actual env var names) into every js_install/thirdparty addr, config
key, and function name in this crate, despite this plugin having nothing
to do with Go. Per explicit request: renamed throughout plugin-js to
os/arch — addr args (`@goos=linux,goarch=amd64` -> `@os=linux,arch=amd64`),
JsInstallDef/JsInstallSpec fields, thirdparty_addr/node_modules_addr
params, platform::current_goos/current_goarch -> current_os/current_arch.

plugin-go's own goos/goarch stays untouched — that crate genuinely is
about Go, and the naming is correct there.

BREAKING: this changes the js_install/thirdparty addr format
(`goos=`/`goarch=` args -> `os=`/`arch=`) and JsInstallDef's cache-key
field names — any addr strings or targets referencing the old args are
invalidated. Explicit, requested rename; plugin-js is still pre-1.0
(milestone-numbered, not yet stably released), so this is the right
time for it.

Only naming values (`"linux"`, `"amd64"`, canonical Go/OCI spelling) are
unchanged — those still come from `hcore::htplatform`, shared with the
Go plugin for a reason (npm/pnpm's own `os`/`cpu` restriction-list values
are separately mapped via `platform::npm_cpu`, untouched by this rename).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
js_install's allow-listed postinstall scripts (esbuild, sharp, etc.) run
in a sandbox with only their own package present, so any script that
requires a sibling dependency (esbuild's install.js requiring its own
@esbuild/<platform> optionalDependency) fails even when the sibling is
correctly resolved in the lockfile.

Resolve each package's required + optional dependencies from the
already-fetched lockfile graph and declare them as driver Inputs,
materialized at sandbox_ws_dir/node_modules by the engine's default
placement (an ancestor of sandbox_pkg_dir, so Node's own ancestor
node_modules walk finds them with no cwd change). A self-reference
symlink makes the package's own name resolvable the same way. Optional
dependencies that are unresolvable or platform-mismatched are skipped
and noted in the lifecycle-script failure context; required ones that
are unresolvable or platform-mismatched are a hard error.

Also fixes a pngjs install failure: extract_tarball was applying a
tarball directory entry's own restrictive permissions immediately,
blocking creation of files inside it on the next entry — now uses
create_dir_all for directories instead of Entry::unpack(). And
lifecycle scripts now run with captured stdio instead of inherited,
since inherited stdio was corrupting the TUI.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…lings

Sibling node_modules materialization only walked a script-owning
package's *direct* dependencies/optionalDependencies. Real postinstall
scripts routinely reach further: @sentry/cli's postinstall requires
`which`, and `which` itself requires `isexe` — a dependency of a
dependency, never a direct edge of @sentry/cli at all. The second hop
was never materialized, so the script failed with a plain
MODULE_NOT_FOUND for isexe.

Walk the resolved graph transitively from the script package's own
edges, at every depth applying the same required-hard-fail /
optional-skip asymmetry per edge. seen_names dedupes (a diamond
dependency isn't fetched or declared as an Input twice) and bounds
the walk (a cycle back to an already-visited name is never
re-queued). A cycle back to the script-owning package by name is
dropped outright — already reachable via the driver's own
self-reference symlink, and would otherwise be a js_install target
declaring itself as its own Input.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…nal in js_test

build_test_closure bucketed a resolved import as first-party whenever
the resolved path fell physically inside the owning package's own
directory (starts_with(pkg_dir)) — true even for a real, ambient
node_modules nested inside that same package (a common on-disk shape:
version-conflict dedup, or a stray local install). test_deps_config
takes closure.files straight to a raw fs:file Input with no further
classification, while closure.external_files goes through
classify_resolved_edge onto the lockfile-driven relocated
js/node_modules:group addr — so an ambient-nested third-party file
landed in both buckets, and the two targets collided on the identical
sandbox path (confirmed live: @apollo/client nested inside
mgmt/backoffice's own node_modules).

Fix: also require thirdparty_pkg_name_from_path(...).is_none() before
treating an edge as first-party, matching what classify_resolved_edge
already checks — an edge under any node_modules/ path component is
third-party regardless of physical containment.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…es materialization, fix ambient-nested classification

Two related gaps in the js_test/js_typecheck/js_bundle node_modules
relocation mechanism (distinct from js_install's own lifecycle-script
sibling walk fixed earlier):

1. ResolvedGraph::transitive_reachable only followed `dependencies`
   edges, never `optionalDependencies`. vite depends on rolldown,
   which ships its native binding as an optionalDependencies entry
   per platform — the binding was never relocated into the sandbox,
   so vite failed to load at require-time (not a lifecycle-script
   failure, ordinary module resolution, one edge past what this
   closure declared). Now walks a platform-matching optional edge
   exactly like a required one; an unresolvable or mismatched one is
   silently skipped, mirroring the established asymmetry elsewhere in
   this crate. Threaded os/arch through transitive_reachable and its
   three callers (resolve_transitive, resolve_transitive_closure,
   transitive_declared_closure) to keep the phantom-dependency check
   and the actual Input-wiring in agreement, per resolve_transitive's
   own doc on why that agreement matters.

2. build_test_closure classified a resolved import as first-party
   whenever it fell physically inside the owning package's own
   directory, even when that meant a real, ambient node_modules
   nested inside the same package (confirmed live: @apollo/client
   nested inside mgmt/backoffice's own node_modules). That bucket
   goes straight to a raw fs:file Input with no further
   classification, while the sibling bucket goes through
   classify_resolved_edge onto the relocated js/node_modules:group
   addr — so the two collided on the identical sandbox path. Now
   also requires thirdparty_pkg_name_from_path(...).is_none() before
   treating an edge as first-party.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
The node_modules relocation mechanism (deps::resolve_transitive_closure,
used by js_test/js_typecheck/js_bundle) placed every third-party package
flat, one node_modules/<name> per name per consuming package, chosen by
whichever version a BFS reached first. Real npm/pnpm lockfiles routinely
resolve the same name to different versions for different consumers —
diamond dependencies, which npm itself handles via nested node_modules
overrides. The flat model could only ever materialize one version, so
whichever consumer needed the other one broke at test/build run time
with an opaque, deep Node.js module-resolution error (confirmed live:
estree-walker resolved to v2.0.2 for most consumers but v3.0.3 for
@module-federation/vite/@vitest/mocker specifically, and only one
version was ever wired, producing ERR_PACKAGE_PATH_NOT_EXPORTED).

ResolvedGraph::transitive_reachable now runs two passes: the original
flat BFS (unchanged, first-reached-wins) establishes the default
placement for every name, then a second pass walks every reachable
node's own dependency edges and detects divergence from that default.
A genuine diamond conflict gets nested one level under the diverging
node's own placement — exactly where npm's own override would live,
and exactly where Node's own ancestor node_modules walk looks first.
Nesting is capped at one level: if a node that is itself already an
override has a further diverging edge, that would require depth-2
nesting, which is a loud, named hard error rather than a silent
wrong placement.

node_modules_addr's own shape is unchanged for the flat case (the new
nested_under arg is only ever inserted when actually nesting), so the
overwhelming majority of existing targets keep an identical addr and
cache key across this change.

A design review (hermeticity/product-vision/feature-quality) and a
follow-up code-quality review both ran on this before landing; the
code-quality pass caught a real panic in the first draft (an edge
whose graph key is unresolvable due to an npm: alias mismatch was
`.expect()`-ed instead of skipped like every other unresolvable-edge
case in this file) — fixed and regression-tested.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
The just-shipped diamond-dependency fix capped override nesting at
exactly one level, hard-failing anything deeper as "not supported yet".
The first real repo it ran against needed depth 2 immediately:
@netskope-ui/core was itself an override (its own name resolving to a
different version elsewhere in the closure), and its own dependency on
@floating-ui/react diverged again from that name's flat default —
exactly the shape a one-level cap can't represent, so the build simply
failed instead of running.

ResolvedGraph::transitive_reachable's override pass is now a worklist
over (graph_key, placement path) pairs instead of a single flat/override
distinction: a node whose own edge diverges from the flat default gets
nested one level inside whichever placement it was reached from, and
its own edges are then walked the same way, so a chain of any length
resolves. A node reached as a divergent target from more than one
distinct placement still gets its own independently-computed subtree at
each one (the two-different-parents case the previous design already
handled). The only remaining hard-fail is a genuine cycle in the
"who overrides whom" relation hitting a generous (16-level) safety cap
— pathological input, not a real dependency tree.

node_modules_addr's chain is encoded as one dedicated Addr arg per
element (n0, n1, ...) rather than a single joined string, so a scoped
package name in the chain never has a re-splitting ambiguity, and the
flat (no-conflict) case's addr stays exactly as before.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
@lingui/vite-plugin calls @lingui/conf's getConfig() at plugin-load
time, which does its own lilconfig-style filesystem walk for
lingui.config.{js,cjs,ts,mjs} — never named anywhere in vitest.config.ts's
own text, so it's invisible to resolve_runner_config_referenced_files'
static scan by construction. The config was never declared as an
Input, so it was simply absent from the sandbox: "No Lingui config
found" at plugin-load time, even on a project that has one.

Verified the exact search order against @lingui/conf's real published
source (lilconfigSync("lingui", { searchPlaces: [...] })) rather than
guessing. Only the file-based forms are handled; package.json's
"lingui" key and the .linguirc dotfile forms are out of scope for now
since they're rare in practice.

This is a narrow, precedented special case for the one plugin actually
confirmed to need it — a general escape hatch for arbitrary
cosmiconfig-based tools (postcss, tailwind, babel, ...) is a bigger,
separate design decision heph doesn't have a mechanism for yet.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
Confirmed live: the previous filename-guessing special case only
found lingui.config.{js,cjs,ts,mjs} — lingui's own default search
filenames. A project using a non-default filename (config.lingui.ts)
passes it explicitly via lingui({ configPath: '...' }), which makes
@lingui/conf's getConfig() skip its own default search entirely and
use that path verbatim — invisible to the filename table no matter
what it guesses.

Extend the existing setupFiles/globalSetup-style key scan
(RUNNER_CONFIG_FILE_KEYS) with a parallel PLUGIN_CONFIG_PATH_KEYS
list (currently just "configPath"), resolved as a plain filesystem
path rather than an import specifier — lingui's own configPath value
isn't required to have a "./" prefix the way a setupFiles import is.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…r, not workspace root

Root cause of the Lingui config still not being found after two prior
fixes staged it correctly: js_test invoked vitest with cwd =
sandbox_ws_dir (the sandboxed workspace root), not the package's own
directory. lingui() is called with no options in the reported config,
so @lingui/conf's getConfig() falls through to its own ambient search
from process.cwd() — and that search only ever walks *ancestor*
directories. With cwd = workspace root, a config living in the
package directory (a descendant, never an ancestor) could never be
found no matter how correctly it was staged.

Every path argument these four drivers already pass to their tool
(tsconfig_abs, entry_abs, outdir_abs, config_abs, test_file_abs, and
group_staged_paths' list-file entries) is already absolute, so cwd
never mattered for *finding* those — it only matters for a tool's own
ambient, cwd-relative behavior. sandbox_pkg_dir is also what a real,
non-heph invocation actually runs with in practice (cd package &&
vitest, pnpm --filter pkg test), so this fixes the whole class of bug
for any tool doing cosmiconfig-style ambient discovery, not just
Lingui specifically.

Added a regression test using a tiny always-failing shell script (no
real vitest/jest needed) that reports its own $PWD through this
driver's existing failure-detail path — verified it fails against the
old cwd choice and passes against the new one.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
test_failure_detail/tsc_failure_detail/lint_failure_detail/bundle_failure_detail
all truncated stdout/stderr to their last N lines. A tool that prints its
error name/message first and a long stack/diagnostic after (vitest's own
error dump does this) had the actual message silently cut, leaving only
orphaned trailing stack frames with no indication anything was omitted.

hplugin::error::head_and_tail_lines keeps both ends, with an omission
marker between, so the leading message always survives regardless of how
long the tail is.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
Real failure: js_test crashed with `Error: Cannot find package
'playwright'`. vitest optionally depends on @vitest/browser-playwright
(walked fine via existing optionalDependencies handling), which
unconditionally imports playwright at module load despite declaring it
only as a peerDependencies entry. ResolvedPackage had no field for peer
deps at all, so transitive_reachable could never walk to a genuinely-
installed peer.

- ResolvedPackage gains peer_dependencies. npm resolves it via the same
  ancestor-node_modules walk already used for dependencies/optionalDependencies
  (npm's package-lock.json never writes a resolved edge for a peer, only
  a bare copy of the package.json range).
- transitive_reachable's flat and override passes both walk peer edges
  with the same platform-gated, miss-tolerant treatment as optional deps.
- provider.rs's lifecycle-script sibling-resolution walk (js_install
  postinstall requires) gets the identical peer-edge treatment — same
  defect class, different call site.

While verifying pnpm needed no equivalent change, found and fixed a
second, unrelated, pre-existing bug: pnpm's `packages:` section keys a
package with its own peerDependencies WITHOUT a peer suffix (confirmed
against a real `pnpm@9.15.9 install`), but `snapshots:` keys the same
package WITH one. resolved_graph() joined them on the same key, so the
lookup always missed for any such package — silently leaving ALL of
its dependencies empty, not just the peer edge, for any pnpm project
using a package like react-dom. Fixed by driving the graph from
snapshots (source of dependency edges) with metadata joined via the
stripped peer suffix.

Known, pre-existing, documented limitation (not introduced here, not
fixed): pass 1's flat-reachability BFS has no bias toward `dependencies`
edges over optional/peer edges for the same name — whichever is reached
first in seed/BFS order wins the flat placement. optional_dependencies
already shared this exposure; peer deps widen it since a peer's whole
purpose is naming a widely-shared library likely to also have a real
dependencies edge elsewhere. Sandbox materialization stays correct
regardless (pass 2 re-derives the right per-consumer placement via
nested overrides) — the residual risk is narrower: resolve_transitive's
single-name flat-only lookup, used only for a first-party import with no
declared edge of its own. Pinned with a test
(transitive_reachable_flat_placement_prefers_whichever_edge_type_is_seeded_first_not_dependencies)
rather than redesigned here — changing pass 1's priority is a separate,
cross-cutting design change affecting already-shipped optional-dependency
behavior too, not something to bundle into this fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
heph's js_test invocation is headless by construction — no TTY, no
human able to answer a prompt or press a key — exactly what CI
conventionally signals to a well-behaved CLI tool. Defense-in-depth
alongside the existing explicit `run` subcommand (which already
defeats vitest's own default watch-mode selection): vitest and jest
both gate other interactive behavior (reporter live-redraw, keypress
handling) on isCI/isTTY checks beyond just watch-vs-run selection.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…adline

CI=1 and the run subcommand only reduce the odds vitest wedges after
finishing its own work (a confirmed live upstream failure mode: a
failed dependency-optimizer scan leaving a background esbuild service
alive) — they don't bound it. proc_exec::output otherwise waits on the
child's own exit with no timeout at all, so a wedged runner hangs the
whole heph run forever with no distinguishing signal from "still
working".

DeadlineCancellable composes the caller's own cancellation token with a
deadline (fires on whichever comes first) rather than wrapping the
subprocess future in a bare tokio::time::timeout, which would just drop
the future and leak the orphaned child. Composing tokens instead routes
through proc_exec's existing SIGINT-then-grace-then-SIGKILL teardown —
the same path a real user cancellation already goes through — and the
original ctoken is never itself touched, so a target that merely timed
out is never mistaken upstream for a user-cancelled run.

Verified proc_exec::output actually errors (not a killed-but-Ok status)
once its cancellable fires, confirmed empirically rather than assumed,
and regression-tested against a real subprocess that actively ignores
SIGINT — the exact shape of a wedged child that won't respond to a
plain interrupt.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…nput

Mirrors plugin-go's go_src mechanism: a BUILD author marks an ordinary
codegen target labels = ["js_src"]; each of the 4 JS driver Input-
computation functions (test/typecheck/lint/bundle _deps_config) now
queries "{scope} && label(js_src) && tree_output(pkg)" and wires the
result in as a declared Input, alongside the existing import-graph-
derived ones. Because it's a declared Input, the engine schedules and
runs the codegen target automatically before whichever target needs
it, and stages its output into the sandbox before the driver's
subprocess (vitest/tsc/eslint/esbuild) runs — no change needed to
importgraph.rs's own resolution walk, since the downstream tool
resolves the generated import itself once the file physically exists,
exactly as `go list` already does for go_src.

Fixes the real reported gap: a js_test importing GraphQL-codegen
output previously required a human to remember to run the codegen
target first.

Design reviewed in parallel by product-vision/hermeticity/feature-
quality before implementation (per this repo's review-board process).
Consensus: mirror go_src's mechanism, but two things needed resolving
first, both addressed here:

- Scope: go_src's *default* scope (no explicit root override) is
  already the package's subtree, not the bare package — go_src's own
  history is why: a bare-package floor shipped a real bug (an embed
  that resolved on some runs and failed on others, when a generator
  sits in a sub-package of the consumer). js_codegen_default_scope
  mirrors this exactly, confirmed sufficient for the reported case
  (codegen output lands inside the consuming package itself). No
  go_codegen_root equivalent (ancestor-scope-widening for codegen
  landing in a *different* package) yet — deferred, disclosed in
  js_src_query_addr's doc.
- Cost: js_test has per-*test-file* target granularity, not go_src's
  per-*package* granularity, so the query must resolve once per
  package and be shared across every test file and all 4 drivers, not
  recomputed per target. js_src_query_addr is a single pure function
  every call site routes through unchanged, so all 4 _deps_config
  functions format the byte-identical query addr for the same package
  by construction — the engine's own per-addr memoization does the
  rest. Also excludes the js provider from the query (mirroring go_src
  excluding go) — without it, every js_test/typecheck/lint/bundle
  candidate in scope would pay a full get_spec just to be rejected on
  the label check.

Disclosed, not silently inherited: a genuine dependency cycle through
a js_src-labelled target is silently dropped from the query match by
the engine's own existing query-cycle handling (the identical,
already-shipped go_src behavior, not new here) — no dedicated
diagnostic for this case yet.

Known gap, not yet covered: no in-process e2e test exists for this
(crates/e2e has no JS harness at all yet, a separate, pre-existing
infrastructure gap; real vitest also isn't available in this devenv).
Unit-tested at the provider layer: query construction (scope/label-
before-tree_output ordering/js-provider exclusion), and that all four
_deps_config functions wire the identical codegen addr for the same
package. Not yet covered: ported cycle-containment regressions
(plugin-go's own 5-test suite for this exact hazard class), failure
attribution when a js_src target itself fails, and output-collision
handling for two conflicting js_src targets.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
@raphaelvigee

Copy link
Copy Markdown
Member Author

Superseded by #394 — retargeting this PR's base to master was blocked by GitHub's stack tracking even after closing/unlinking, so a fresh PR was opened from the same branch directly against master instead.

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.

1 participant