feat(plugin-js): JS/TS plugin — install, import-graph, typecheck/test/lint/bundle drivers, bin-e2e - #394
Open
raphaelvigee wants to merge 70 commits into
Open
feat(plugin-js): JS/TS plugin — install, import-graph, typecheck/test/lint/bundle drivers, bin-e2e#394raphaelvigee wants to merge 70 commits into
raphaelvigee wants to merge 70 commits into
Conversation
6 tasks
raphaelvigee
force-pushed
the
raphaelvigee/feat-plugin-js-m8-bin-e2e
branch
from
August 13, 2026 23:25
4de9d21 to
23e8ab5
Compare
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
Speed was the plugin's stated #1 design goal from the start (ai-docs/js-plugin-plan.md) but had never actually been measured. Adds #[ignore]d ad-hoc benches (never run under tst/cargo test by default) in importgraph.rs measuring Provider::import_graph's cold cost (first parse+resolve per package) vs warm cost (M5's per-package memoization) on synthetic workspaces of 20 and 150 packages, plus a parse-vs-resolve phase split. Results (darwin/arm64, this machine only — not transferable to other targets without re-running there): cold ~2.5-3ms/package, ~0.16-0.2ms/file; warm ~500ns total across 150 packages (cache hit never re-enters the build path, confirming M5's fix delivers as designed). Resolution (oxc_resolver) is 97%+ of cold cost, parsing (oxc_parser) is noise by comparison. No regression baseline existed before this, so this is establishing one, not comparing against one. Flagged as a real gap: no JS/TS scenario exists in crates/bench/crates/bench-corpus (Go-only today), so this path has no standing CI perf-regression coverage — building that out is the natural follow-up, left for later rather than folded into this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
Extends heph-bench to measure the new plugin-js the same way it already
measures plugin-go, and generalizes Tier B (dist.rs) across languages
instead of duplicating the go-specific path.
- crates/bench-corpus: generate_js_tree produces a synthetic, network-free
pnpm workspace (zero third-party deps by design — no js_install fetch,
no lockfile, fully hermetic) with the same layered-DAG shape the bash
and go generators already use. Wired via js_packages/js_max_depth,
mirroring go_packages/go_max_depth exactly; 0 (the default) is a no-op,
so existing callers get byte-identical output.
- crates/bench: dist.rs rewritten around a Lang abstraction (GO/JS
consts: name, provider-option fragment, package-count accessor,
incrementalize fn) instead of two near-duplicate go/js code paths.
`run dist --lang go|js|both` (default go, preserving existing-caller
behavior exactly). `both` reports each language as its own distinct
ScenarioResult ("<scenario>-go"/"<scenario>-js") — RunResults already
carried a Vec of scenarios for exactly this, so results are never
silently summed/averaged across languages.
- The `corpus` CLI subcommand was missing --js-packages/--js-max-changed
entirely (bench-corpus's library supported them, main.rs never exposed
them) — added, mirroring --go-packages/--go-max-depth.
Found and fixed a real, pre-existing bug while mirroring the go path for
js: Tier B's `heph r build //<lang>/...` invocation is the two-positional
form, which parses as `label("build") && //<lang>/...` — no BUILD file in
the corpus sets that label, so it has always matched zero targets and
exited 0, silently measuring an empty build. This affected the existing
go benchmark too, not just the new js one. Fixed to the query form
(`heph r -e '//<lang>/...'`), verified against a real binary (built a
fixture, confirmed the old form reports "matched 0 targets" and the new
one actually executes targets), with a regression test that fails if the
matcher regresses back to the broken form.
Artifact pipeline (a real decision, not a side effect): plugin-js-cdylib
is now built and published by heph.yml's `build` job alongside
plugin-go-cdylib/plugin-gha-cdylib, in the same pre-release artifact
bundle. No public heph-js-plugin.json manifest is generated (unlike
go/gha) — bench builds its own local manifest from the raw dylib, same as
it always has for go; declaring the plugin generally installable is a
separate, larger decision left unmade. devenv.nix's `e2e` script
deliberately does NOT build plugin-js-cdylib — there is no
shipped_js_cdylib_loads bin-e2e test yet to justify the build cost,
mirroring the existing lint-but-don't-e2e-test asymmetry.
perf.yml wires --lang js into the perfbench job's Tier B step, gated
best-effort on the baseline release actually having a js-plugin asset
(older releases won't). Known, disclosed gap: no oxlint/esbuild toolchain
is provisioned on any CI runner yet, so the JS Tier B comparison reports
"no results" until one is — the target-matcher fix above makes that
failure mode honest (real attempt, real failure) rather than the
previous silent zero-targets false success.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
Makes the js plugin generally installable the same way go/gha already
are: plugins: - { identifier: { url: https://.../heph-js-plugin.json } }
now resolves for any real workspace, not just this bench pipeline's own
locally-built manifest (crates/bench/src/dist.rs's write_dist_config,
unaffected — it still builds its own manifest straight from the raw
dylib and never reads this one).
Generated the same way as go/gha: tools/pluginmanifest against the
per-os/arch heph-js-plugin_<os>_<arch>.<ext> release assets the `build`
job already publishes (wired in the prior commit on this PR), with the
same URL-base/checksum-sidecar shape. perf.yml is unaffected — it
downloads only the raw dylib, not this manifest.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
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. Wires it into the e2e devenv script as a fourth staged artifact (symmetric with go/gha in both the local-build and HEPH_E2E_FROM/CI branches), and adds four real bin-e2e tests proving it: - shipped_js_cdylib_loads_and_answers_across_the_abi: construction + the sync inspect functions ABI call round-trips cleanly. The js Provider never overrides functions() (falls through to the trait's empty-vec default, unlike go's build_addr), so this asserts no line in `inspect functions` output starts with "js." rather than asserting empty stdout outright -- the fs provider's own functions are always registered regardless of plugin config, so an empty-stdout assertion is simply wrong (caught by review, the first draft failed deterministically). - shipped_js_cdylib_discovers_a_real_workspace_package: query -e //... against a real pnpm-shaped fixture resolves //packages/foo:package_info -- proves Provider::list/list_packages cross the seam intact. - shipped_js_cdylib_resolves_pnpm_workspace_glob_membership: two real package.json files, one inside the packages/* glob and one deliberately outside it declared as a dependency; resolving the in-glob package's config must fail on the unresolved dependency, proving the excluded package was never admitted as a workspace member. Added because the discovery test above (despite its own doc comments originally claiming otherwise) never actually exercises workspace.rs's glob-membership resolution -- Provider::list discovers by package.json presence alone, independent of pnpm-workspace.yaml. Caught by review; the stale overclaiming comments were corrected rather than left standing. - js_plugin_construction_failure_logs_before_the_abort: the same log-sink-before-abort regression coverage the go test already has, ported for js's identical construction-failure shape. Verified for real: ran the full `e2e` devenv script (release build of heph + all three plugin cdylibs, staged, macOS-portable-patched) end to end -- 17/17 tests passed across the whole bin-e2e suite, all four new plugin_dylib_js.rs tests included, against real dlopen'd release artifacts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M3wZfyPsG8stfRQuybLRjN
…ot phantom check_phantom_dependencies only accepted a package's own direct dependencies/devDependencies/peerDependencies, hard-failing on the extremely common real-world pattern of importing a companion package (e.g. `@eslint/js` from eslint.config.js) that isn't itself declared but is a genuine transitive dependency of something that is (e.g. typescript-eslint), per the lockfile. transitive_declared_closure widens the accepted set by walking the lockfile's resolved dependency graph outward from the manifest's own direct deps. Since the lockfile is already a hashed input, this stays exactly as hermetic as accepting a direct dependency — unlike relying on ambient node_modules hoisting, which is what the check exists to reject in the first place. A hermeticity review caught that widening the check alone would let Provider::get succeed while deps::resolve_one_dependency still failed to wire an Input for the same package (deterministically on pnpm, narrowly on npm under nested hoisting) — reopening the exact same-source-different-host non-determinism this subsystem exists to close. Fixed by unifying both paths onto one shared primitive (lockfile::resolve_transitive / ResolvedGraph::transitive_reachable), so the phantom-check's accepted set and the Input-wiring resolver's reachable set can no longer diverge.
…_test's sandbox test_deps_config only fed the test file's own transitive import closure (closure.bare_specifiers) into the third-party js_install Input list. A test-runner config (vitest.config.ts/jest.config.ts) is a separate root the runner loads independently — nothing in the test file imports it, so its own plugin imports (@vitejs/plugin-react, @rolldown/plugin-babel, @lingui/vite-plugin, vite-plugin-svgr, @vitest/browser-playwright, ...) were never staged in the sandbox at all, even when properly declared in package.json. resolve_runner_config_referenced_files now also collects (without following) bare specifiers encountered while scanning the config and any relatively-imported base config in its chain. test_deps_config feeds them through the same declared-dependency check and deps::resolve_one_dependency wiring closure.bare_specifiers already gets — necessary in its own right, not just for consistency: a shared runner config commonly lives outside the package's own directory (found via find_nearest_test_runner_config's ancestor walk), so it is never part of the graph check_phantom_dependencies already validates. Known related gap, left open rather than silently claimed as covered: js_lint's eslint_plugins field only scans a flat config's own leaf file for plugin imports, not a relatively-imported base config's — documented at resolve_eslint_config_referenced_files's call site.
…r; fix runner-config bare-specifier file paths The 'declared in package.json but has no lockfile resolution' error couldn't distinguish 'no lockfile was loaded at all' (a wrong or absent workspace_root) from 'a lockfile loaded but has no entry for this name' (genuinely stale) — both produce the same None. Now names which one, and how many packages a loaded lockfile actually parsed. Also: BareSpecifierSite::file for a runner config's own bare imports was left as an absolute path (resolve_runner_config_referenced_files has no workspace_root to convert against internally) instead of workspace-relative like every other BareSpecifierSite in this crate — fixed at the test_deps_config call site. Diagnostic-only (the file field is never used to resolve anything, only to name the source in an error), but confusing and inconsistent.
A heph workspace can contain more than one independent npm/pnpm project (each with its own lockfile) nested at different points, not just one shared at the configured workspace root. Provider::lockfile now ancestor-searches from each package's own directory for the nearest lockfile, the same walk-up-by-presence pattern already used for tsconfig/eslint/bundler-config discovery, instead of assuming a single fixed lockfile path. Provider::find_resolved_graph_for resolves a bare (name, version) thirdparty addr against every discovered root and fails loudly on a genuine disagreement (full ResolvedPackage equality, not just integrity — resolved feeds JsInstallDef's hash too) rather than silently picking one, since thirdparty_addr's scheme has no per-project scoping. The check always walks the full workspace, not only when the per-Provider cache is empty: a cache hit for one root says nothing about a second, not-yet-resolved root that could disagree, and skipping the walk whenever any match is already cached would make the check order-dependent on the engine's own incremental resolution order. The walk's own root list is cached once per Provider lifetime to keep this cheap across repeated resolutions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
group's relocated (strip_prefix/prefix/rename) output has always lived only inside heph's own build graph/sandbox, with codegen_tree hardcoded to None. Add an opt-in `codegen = "copy"` config field (mirroring pluginexec's identical field) so a relocating group can also write its result onto real disk via heph's existing codegen write-back path, needed by a plugin-js consumer that materializes a resolved node_modules tree for IDE visibility. `in_place` is rejected: a relocating group's inputs are `deps` addrs, never `@heph/fs` file addrs, so it has no way to declare the overwritten path as one of its own hashed inputs the way `in_place`'s staleness guard requires elsewhere. `codegen` on a non-relocating (aggregate, transparent) group is also rejected — it has no artifact of its own to materialize. Every existing group(...) call site is unaffected: codegen defaults to None, and the def hash (already hand-computed, not a generic spec serialization) only folds it in when relocating, so an ordinary aggregate group's hash is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
js_install's download has always lived at a synthetic path
(@heph/js/thirdparty/<name>@<version>) nothing ever relocated to
<consuming_pkg>/node_modules/<name> — the only path Node's own module
resolution ever looks at. Confirmed via live user testing: every
resolved third-party addr was correctly declared as an Input, but the
sandbox's real node_modules never had the files, so every js_test run
touching a third-party package failed with "Cannot find module".
ai-docs/js-plugin-plan.md's "Per-target node_modules reconstruction"
called this out as required for v1; it was simply never implemented.
Fix: reuse the existing group driver as a zero-copy relocation
primitive rather than inventing a new mechanism. deps::
resolve_one_dependency now returns a thirdparty::node_modules_addr
(package @heph/js/node_modules, target "group", every variable part —
consuming_pkg/local_name/resolved_name/version/goos/goarch — as an
Addr arg, never folded into the package string, since a consuming
package nested under an @scope-looking directory makes positional
encoding ambiguous) instead of js_install's raw addr.
Provider::node_modules_group_spec synthesizes the group's config
(strip_prefix = js_install's synthetic package path, prefix =
<consuming_pkg>/node_modules/<local_name>). local_name (what the
consumer's own source/config names) and resolved_name (what
js_install actually downloads) are kept distinct throughout since an
npm/pnpm alias makes them diverge.
This changes the Input addr every js_test/js_typecheck/js_bundle/
js_lint/package_info target wires for any third-party dependency, so
it invalidates the cache for all of them — expected and necessary,
since the old addr keyed a broken (unreachable) artifact.
Also fixes a second, independent bug in the same area:
bundle_closure_step, typecheck_deps_config, and test_deps_config each
classified an import edge landing inside a real ambient node_modules
(only possible when one happens to exist on the host) the same as a
genuine first-party file, wiring it as a raw fs:file Input at that
ambient path — a build depending on host filesystem state no input
hashes, and never even reached on a fresh checkout. All three now
share one classify_resolved_edge helper that routes a node_modules-
landed edge through the same lockfile-driven resolution instead.
Extends relocation to the full transitive third-party closure (not
just directly-imported names) for js_test/js_typecheck via
deps::resolve_transitive_closure, walking ResolvedGraph::
transitive_reachable from each package's own direct deps — a resolved
package's own internal dependencies (axios needing follow-redirects)
need the same node_modules materialization a directly-imported name
gets, or the real test/typecheck run hits the identical failure one
edge deeper. js_bundle is deliberately excluded: a bundler marks
third-party names --external and never descends into their own
requires at build time. js_lint is deliberately excluded for now: it
resolves plugin/extends names from config text, not an import graph.
Finally, adds a NODE_MODULES_SYNC_TARGET ("node_modules") per package:
an aggregating group (identity transform, codegen = "copy") over every
relocated dependency a package resolves. Nothing else in the graph
depends on it — codegen write-back only fires for a directly-requested
top-level target, so `heph run //pkg:node_modules` is the only way to
get a hermetically-resolved dependency onto real disk where an IDE can
see it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…check find_resolved_graph_for's cross-lockfile ambiguity check compared every discovered root's entry for a shared (name, version) unconditionally, including a root whose own lockfile records that entry with empty integrity and no resolved URL — a real, observed npm lockfile shape (a degenerate/deduped graph-key record, not a genuine resolution), not a hypothetical. Comparing it against another root's actually-resolved entry produced a false-positive "resolves to different content" hard error, live-confirmed across hundreds of packages at once in a real multi-project workspace: every package one project's degenerate lockfile record happened to share a name with, resolved anywhere else in the workspace. An empty-integrity entry never pinned any content to disagree with — it can't confirm or deny a match. Only entries that actually carry integrity participate in the comparison now; fewer than two such entries means nothing to disambiguate, so resolution succeeds via whichever root has real content. Two roots that both genuinely resolved the package, with real but differing content, still fail loudly exactly as before — this narrows what counts as a competing resolution, not what counts as a conflict. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…egrity A hermeticity review caught a more insidious version of the bug the previous commit fixed: dropping an empty-integrity entry from the ambiguity comparison entirely (not just skipping the integrity field) meant a root with empty integrity but a real, genuinely different resolved/os/cpu/dependencies/has_install_script would silently pass unchecked — using an unrelated project's resolved URL for that root's own dependency, with no error, since the winning entry's own integrity still verifies fine against its own bytes. find_resolved_graph_for now compares every discovered root's entry against the chosen "winner" via entries_agree_where_comparable, which exempts integrity from comparison only when one side is empty, and resolved only when one side is None — mirroring how a genuinely degenerate lockfile entry naturally lacks both, not just one. Every other field, and either of these two when both sides actually populate it, still compares exactly as before. Also sorts matches by root path before picking, so which root's data appears first in an error (or gets silently picked in the fully-degenerate all-empty case) no longer depends on HashMap iteration order. New regression test proves the caught gap is closed: empty integrity paired with a real, differing resolved URL still hard-fails. The already-shipped false-positive fix (empty integrity vs. a fully resolved entry) and both pre-existing genuine-conflict tests still pass unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
Live-confirmed right after the previous two fixes shipped: two independent, unrelated projects both genuinely resolving the *identical* published package (byte-identical integrity and resolved URL) still hard-failed as "ambiguous" — hundreds of times in one real workspace — because each project's own npm install ran at a different time, and the shared package's own transitive dependency resolved to a slightly different patch version. entries_agree_where_comparable was comparing `dependencies` (and `os`/`cpu`) unconditionally, but none of these three fields are part of js_install's own cache key (driver_install.rs's JsInstallDef: name/version/integrity/resolved/ goos/goarch/has_install_script/scripts_allowed only) — dependencies is pure graph-traversal bookkeeping about *other* packages, and os/cpu only gate whether an install applies to the current platform (checked separately, downstream, against whichever root this function returns) — comparing either was never correct. The comparator now only compares integrity, resolved, and has_install_script — exactly what determines js_install's actual output — with the pre-existing per-field empty/None exemptions unchanged. New regression test proves two roots with identical integrity/resolved but differing dependencies now resolve without error; all four pre-existing ambiguity tests (genuine integrity conflict, genuine resolved conflict, empty-integrity exemption, empty- integrity-with-real-diverging-resolved-still-conflicts) still pass unchanged. Also fixes an unrelated logging bug spotted in the same live output: result.rs's "get def for group" error context Debug-formatted the TargetAddr instead of using its own Display impl (already used one line above for the identical value), printing a multi-line struct dump instead of the addr string. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
NpmLockfile::resolved_graph() and PnpmLockfile::resolved_graph() built their (name, version) -> ResolvedPackage map with a blind BTreeMap insert-on-collision. npm's own dedup/hoisting can leave a nested node_modules path with no integrity/resolved of its own alongside the real, genuinely-resolved entry for the same package elsewhere in one lockfile (pnpm: peer-suffix variants of one version collapse the same way). Iteration is path/key-alphabetical, not "real entry first", so whichever happened to sort last silently won — live failure was js_install erroring "no recognized integrity algorithm in \"\"" for a package the lockfile actually did pin correctly. Fix: on a graph_key collision, prefer whichever entry has non-empty integrity, and hard-fail via entries_agree_where_comparable (moved out of provider.rs, now shared) when two non-degenerate entries actually disagree — the same fields/reasoning as the cross-lockfile-root ambiguity check, since a silent pick there would be exactly as wrong. resolved_graph() is now fallible; threaded through the OnceCell-cached wrapper in provider.rs via get_or_try_init. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…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
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
…eout The RUNNER_TIMEOUT deadline (added earlier this session) was a working but insufficient fix: it correctly killed a wedged vitest/jest, but only after waiting the full 20 minutes even when the runner had actually finished and printed results seconds earlier. Explicitly rejected as "not solving the problem" — the driver needs to detect real completion and exit immediately. Mechanism: vitest is now also invoked with `--reporter=default --reporter=json --outputFile=<path>` (jest: `--json --outputFile=<path>`), writing a structured, Jest-compatible result to an undeclared sandbox scratch file (`.heph-js-test-result.json`, same pattern as _golist's `.heph-gocache`). `exec_runner` polls for that file on its own dedicated task and, the instant it parses, treats the run as over — verdict from the JSON's `success` field, never the process's eventual exit code — and force- kills the child via a manually-triggered cancellation token composed into the same SIGINT->grace->SIGKILL teardown a real Ctrl-C already goes through. RUNNER_TIMEOUT (20 min) remains only as a last-resort backstop for a runner that produces neither a normal exit nor a parseable result file. JS_TEST_FORMAT_VERSION bumped (invocation shape changed). Went through a full 3-agent review board (hermeticity, code-quality, feature-quality) in parallel, all findings acted on: - BLOCKER (code-quality, empirically reproduced): the first draft raced the result-file poll against OutputReader::recv() in one tokio::select! loop. On macOS, recv()'s multi-threaded backend parks the task in block_in_place - never Pending - so the poll arm starves for as long as the runner stays silent, i.e. exactly the wedged-runner case this exists to fix. Reproducing under `flavor = "multi_thread"` (what every production entry point actually uses; bare #[tokio::test] defaults to current-thread and never hits this path) hung for 60s+. Fixed by moving the poll onto its own task, decoupled from stream draining - the same safe shape pluginexec and plugin-oci's docker_build.rs already use for this reader. All new tests that exercise a hanging fake runner now use `flavor = "multi_thread"` so a regression here fails loudly again. - BLOCKER (feature-quality): the headline claim - "JSON verdict wins over exit code" - had zero coverage for a normally-exiting process; every test used a runner that hangs after writing the file, which only proves the mechanism via the kill path, never the plain post-EOF fallback. Added tests asserting the JSON verdict wins in both directions (success=false + exit 0, success=true + exit 1) against a runner that exits immediately. - MAJOR (feature-quality): jest's entire run()/arg-building path (--json --outputFile=<path> insertion) was untested - every test defaulted to vitest. Added a jest-specific detection test. - MAJOR (feature-quality): the stale-result-file guard (remove_file at the top of exec_runner, guarding a reused sandbox path) had no regression test. Added one that seeds a stale success file and confirms it's ignored. - MAJOR (hermeticity): the new CLI flags are an invocation-shape change per JS_TEST_FORMAT_VERSION's own doc, which requires a bump - it hadn't been. Bumped to 2. - MINOR (code-quality): a genuine stream-read error was silently folded into plain EOF, losing the earlier "wait for test runner: <io error>" diagnostic. Now tracked separately and surfaced when no JSON result exists to fall back on. - MINOR (hermeticity/code-quality): the result-file read now runs through hcore::blocking::run (the codebase's standard bounded-blocking-pool helper) rather than inline std::fs::read on a tokio worker. - Nits: dropped an unused Default derive, an unnecessary Arc::clone, and a failure-message check that string-matched test_failure_detail's internal "no output" sentinel instead of checking the actual stdout/stderr inputs. Disclosed, not blocking: no test covers a genuinely torn/partial result-file write (the polling code's tolerance for it is asserted by comment, not by a test); no test drives real ctoken cancellation racing JSON detection; the exact vitest/jest CLI flag syntax is reasoned from documentation, not verified against a real toolchain (none exists in this devenv - the existing #[ignore]d real-vitest tests predate this change and don't check for the result file at all). DeadlineCancellable/EitherCancellable are two small near-duplicate combinators rather than one generalized one - left as-is rather than refactoring already-tested code under time pressure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…g it CI caught this (linux/amd64, linux/arm64) and the user hit it live against a real vitest run within minutes of the previous fix publishing: the "detects completion, exits promptly" mechanism itself could still hang indefinitely. Root cause: this driver always spawns its runner with setsid: false (matching a real vitest/jest process, never made a session leader), so killpg can never reach a descendant the runner backgrounds that inherits its stdout/stderr — which is exactly what vitest's own dependency-optimizer esbuild service does, the original bug's own trigger. proc_exec::output() has always handled this: DRAIN_DEADLINE (500ms) bounds the post-exit drain and abandons it once the reaped child's own output is in hand, precisely because a stray descendant's copy of the pipe can outlive the child indefinitely. The previous commit's rewrite moved off output() onto a hand-rolled, unbounded drain loop and lost that bound entirely - so once the direct child was killed (or exited), a lingering descendant still holding the write end open left exec_runner waiting on an EOF that would never come. Same symptom as the original bug, reached through the new mechanism instead of the old one. Fix: race the drain against the child being reaped (mirroring proc_exec::output()'s own two-phase structure exactly - drain reaching EOF doesn't end the wait, the wait ending doesn't yet end the drain, whichever lands first the other keeps going), then bound any remaining drain by DRAIN_DEADLINE and abandon it, logging a warning, using whatever was captured. New regression test backgrounds a long-lived descendant that inherits the pipe and exits the direct child normally - reproduces the exact shape (confirmed by first running it against the prior commit's drain loop, where it hangs past its own 15s bound). Verified for real, not just compiled: ran the full plugin-js driver_test suite natively on both darwin/arm64 (this machine) and linux/arm64 (a fresh, uncached Docker container - not the devenv CI image, but real Linux, real kernel pipes, no emulation) to directly address why the previous commit passed everywhere I could check yet still failed on both Linux CI runners and in the user's real environment. All 23 driver_test tests pass on both. (One unrelated, pre-existing toolchain.rs test fails in the bare Docker image over a missing tool the devenv shell provides - unrelated to this diff, not investigated further here.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
Root cause of a live, real-world report: `@/gqlv3` (a tsconfig `paths`
alias) failed to resolve in vitest's dependency-optimizer scan even though
the aliased file was physically present in the sandbox and correctly
staged.
Diagnosed by tracing the whole js_src/js_test mechanism end to end
(query construction -> deps["codegen"] wiring -> query-addr Input
expansion -> sandbox staging), confirmed via `heph inspect deps`/`heph
inspect outputs`/direct --shell sandbox inspection with the user, live: the
codegen output WAS staged correctly, and js_test's own import-graph
resolver DOES use the package's tsconfig to resolve `paths`-aliased
specifiers into concrete first-party file Inputs. But `test_deps_config`
never declared the tsconfig FILE ITSELF as an Input -- only js_typecheck
and js_bundle did. Per architecture.md's isolation model ("a target sees
only its declared inputs"), that meant tsconfig.json was never staged into
js_test's sandbox at all. heph's own resolution worked fine on the host;
Vite's own alias resolution (a `tsconfig-paths`-style plugin reading
tsconfig.json directly, at runtime, inside the sandbox) had nothing to
read. This was an already-disclosed "known scope trim" in the module
docs, not a regression from this session's other work -- but it's the
thing actually blocking the user right now, and js_typecheck/js_bundle
already have the correct mechanism to mirror.
Fix: `test_deps_config` now finds the resolved tsconfig (the package's own,
or the nearest scoped ancestor's) and its whole `extends` chain, declares
each as an Input under a new `"tsconfig"` dep group (mirroring
`typecheck_deps_config` exactly), and threads `tsconfig_path`/
`tsconfig_content` through to `JsTestDef`, hashed directly (same
deliberate redundancy with the declared Input that `runner_config_content`
already has, and that `js_typecheck` has for its own `tsconfig_content`).
`JS_TEST_FORMAT_VERSION` bumped to 3 -- a cached result from before this
change never had the tsconfig staged, so it must not stand in for having
run with it.
New tests (mirroring the existing `typecheck_deps_config`/
`bundle_deps_config` tsconfig tests exactly): tsconfig group present +
content captured, absent when no tsconfig exists, and the `extends` chain
declared + both files' content hashed. Plus a `JsTestDef` hash-sensitivity
test for `tsconfig_content`, mirroring the existing `runner_config_content`
one.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…an hang without a timeout Two independent gaps in js_test's completion handling, both reported live against the real monorepo: 1. exec_runner only ever reported captured stdout/stderr once it returned, so a stuck run showed nothing at all until it finished — forcing `heph run --shell` to reproduce by hand what had already printed. It now tees every chunk, as it arrives, to the engine's live terminal sink (when one is wired — unconditional on --shell, see run()'s doc comment) and to a log.txt in the sandbox, mirroring the pluginexec/docker_build.rs convention. 2. A failed Vite dependency-optimizer scan leaves an internal promise permanently unresolved, so vitest never writes the result file and never exits — a confirmed, unfixed upstream bug (vitejs/vite#22934, vitest-dev/vitest#9799), not a slow-but-working case. exec_runner now recognizes Vite's own diagnostic line the instant it's printed and kills the runner immediately instead of waiting out the 20-minute RUNNER_TIMEOUT backstop. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…not an instant kill A hermeticity review of the previous commit flagged a real correctness gap: DEPENDENCY_SCAN_HANG_MARKER was an unanchored substring match that killed the runner the instant it appeared on either stream, with no chance for a legitimately-finishing run (one that happens to print the same words for an unrelated reason) to still succeed. Fix: anchor the marker to Vite's actual `(!) `-prefixed diagnostic, and route detection through the existing poll task instead of cancelling inline. The marker only records a timestamp; the poll task's existing per-tick result-file check keeps running (and still wins first, every tick), and only kills the runner once DEPENDENCY_SCAN_HANG_GRACE (5s) elapses with still no result file. A real result file discovered within that window wins the race regardless of whether the marker ever appeared. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…m + vitest crates/pluginjs-e2e mirrors crates/plugingo-e2e's shape for the js plugin: a real Engine, in-process, against a real npm-installed workspace on disk, running the real vitest binary — not the fake-shell-script harness the existing unit tests use. Covers the passing/failing paths, that a cache hit is a real LocalCacheHit (not just inferred from EResult, since js_test declares no output artifacts), and that editing an imported (not just the test) file invalidates the cache. Adds pkgs.nodejs_24 to devenv.nix: js_test/js_typecheck/js_bundle/js_lint all resolve their tool from <workspace_root>/node_modules/.bin/<tool>, a disclosed non-hermetic escape hatch, so a real npm install needs npm on PATH the same way the go toolchain tests already need go. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…ith what a real subprocess resolves Found live, via the new pluginjs-e2e suite: a real vitest run reported "No test files found" against a test file that plainly existed in the sandbox. Root cause has nothing to do with js specifically — on macOS, /tmp and /var are symlinks to /private/tmp and /private/var (the default TMPDIR a test harness's tempfile::tempdir() resolves into). Every absolute path a managed driver builds off the workspace root (sandbox_dir and everything derived from it, e.g. the absolute test-file path js_test passes vitest as a CLI argument) inherited that uncanonicalized prefix, but a real subprocess doing its own filesystem walk resolves the same file to its canonical path. The two strings never matched. Fixed at the source: crates/config's get_cwd() (HEPH_CWD or the process cwd) now canonicalizes before returning, so bootstrap.rs's get_root() -> ConfigYamlExt::resolve() -> Engine::new() chain is canonical end to end for the real CLI binary. Engine::new also writes the canonicalized root back into cfg.root itself (not just a local used to derive home), so root()/PluginInit.root/RunRequest.tree_root_path — the field crossing the plugin ABI to every driver, including out-of-process cdylib plugins — get it too, not only sandbox-derived paths. A first version of this fix only canonicalized inside Engine::new, which a hermeticity review caught as dead code for the real binary (production always pre-populates Config.home_dir from the pre-canonicalization root, bypassing the branch that used the fixed value) and incomplete even where it did apply (cfg.root itself stayed raw). Both are covered by dedicated regression tests, verified red without the fix and green with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
The only place in the whole codebase shelling out to plain "sh" — everywhere else (pluginexec's own bash driver, every other shell-out) uses bash. Adds a test that actually discriminates the two: asserts on $0 rather than a bash-specific language feature, since macOS's /bin/sh is literally the same bash 3.2 binary as /bin/bash and would pass a feature-based check either way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…sions Missed when the crate was added: like plugingo-e2e, pluginjs-e2e depends on the root heph package with default features on (for htestkit), so it needs the same --exclude on the fuse-sandbox feature-off clippy pass and its cargo tree -i fuser probe, plus a place in qualityCrates for the fmt check. tests/lint_gate.rs caught both gaps in CI (Lint and, since that test lives in the root package, every Test job too). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…l root Engine::new now unconditionally canonicalizes cfg.root, so skip_dirs() comes back canonical even though this test's own helper hands it a raw tempdir path. The assertion compared against that same raw path, which only matches on a host where TMPDIR isn't itself a symlink — caught by darwin CI, where /tmp/var is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…error twice `failure_detail_from_json` always appended the raw stdout/stderr tail as a "supplement" to the structured `testResults[].message`, on the reasoning that some diagnostics (a warning printed outside any test result) only ever live in the raw stream. But when the structured message is already a substring of that raw tail — the common case, since vitest's own `Error:`-prefixed terminal dump and the JSON reporter's `message` field both derive from the same underlying error — appending it anyway just prints the identical failure twice: once terse, once in full under `--- raw stdout/stderr ---`. Now skips the supplement when the raw tail already contains the structured message verbatim, and keeps it otherwise (the genuine "message only lives in the raw stream" case is unaffected). Two new tests cover both branches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…prefix Root cause of a live, real-world report: `setupFiles: ["src/tests/browser.setup.tsx"]` (no leading `./`, completely idiomatic — both jest and vitest resolve `setupFiles`-family entries relative to root regardless of a prefix) silently resolved to nothing in `resolve_config_value_path`, which only handled `<rootDir>/...`, `./...`, and `../...`. The setup file was therefore never declared as an Input, never staged into the sandbox, and vitest's own later fetch of it failed with `Failed to fetch dynamically imported module` and no resolution error anywhere — a missing static asset, not an unresolved import specifier, so none of the usual diagnostics fired. Diagnosed by ruling out every other candidate first (upstream Vite/vitest dependency-optimizer races, cache warmth, CLI invocation shape, staging order) via faithful fixture reproduction on a real Vitest browser-mode setup — none reproduced the actual symptom. `DEBUG=vite:deps` on the real failing run showed the dependency-optimizer crawl entries list containing only the test file, never the setupFiles entry; grepping the resolver found the missing prefix-only branch. Fix: fall through to the same `probe_first_party_path` check the other branches use, with no prefix required. Safe against misclassifying a bare npm package name (`setupFilesAfterEnv: ["jest-extended"]`) as a file, since `probe_first_party_path` only returns `Some` when a file actually exists on disk relative to the config's own directory. Also fixes an adjacent instance of the same silent-drop pattern found during the same review: a `setupFiles`-family value that resolves to neither `<rootDir>` nor an on-disk file is a bare npm package name in practice, but was previously just dropped — never staged, never declared. Now collected as a bare specifier the same way the config's own `import`/`require` statements already are, which routes it through the existing phantom-dependency check (fails loudly if undeclared) and dependency resolution (stages it if declared), rather than leaving it silently missing from the sandbox. New tests cover both the bare-relative-path resolution and the bare-package-name collection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…renced files
Root cause of the deeper half of the same live report: `browser.setup.tsx`
(now correctly staged) itself does `import { loadMessages } from
"@/locale"` — a tsconfig `paths` alias (`@/* -> ./src/*`) pointing at a
*directory* (`src/locale/index.ts`). `resolve_runner_config_referenced_files`'s
import scan only ever tried `resolve_config_import_specifier` (relative
`./`/`../` only) and `bare_specifier_package_name` (a valid npm scope
shape) for each import site inside a referenced file. `@/locale` is
neither — `bare_specifier_package_name("@/locale")` is `None`, same as the
already-tested `"@/components/Foo"` case — so it fell through both
branches and was silently dropped: `src/locale/index.ts` (and its own
`catalogs/`) were never declared as Inputs, never staged, and Vite's own
runtime resolution of `@/locale` failed with "does the file exist?" and no
indication why, even though the file was right there on the real host
tree (confirmed live: `ls` inside the actual sandbox showed `compiled/`
present — a codegen target's automatic output — but `index.ts`/`catalogs/`
missing).
Fix: thread the package's resolved tsconfig (`test_deps_config` already
has it via `find_nearest_tsconfig`) through to
`resolve_runner_config_referenced_files`, and try the same
`Resolvers`/`oxc_resolver` machinery `build_test_closure` already uses for
the test file's own graph. Only a result landing *outside* `node_modules`
counts as a newly-discovered first-party file — a specifier that resolves
into an installed package is left entirely to the existing
`bare_specifier_package_name`/lockfile-driven path, never decided by
walking `oxc_resolver` against ambient `node_modules` (the M3-review
lesson this crate already learned once).
`resolve_eslint_config_referenced_files`/the bundler-config caller pass
`None`: an eslint flat config or esbuild JSON config importing app code via
a tsconfig alias is out of scope for this fix (already-disclosed gaps in
those callers' own doc comments).
New test reproduces the exact shape: a setupFiles-referenced file
importing via a tsconfig `paths` alias to a directory index.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…ynamic import()
`import(`./catalogs/${locale}.po`)` is Vite's own "dynamic import with a
variable" pattern — it globs the whole prefix directory at dev-server time
and needs every matching file present, not just the one a given test run
picks. The import-graph scanner only ever handled string-literal `import()`
arguments; anything else (including this shape) was coarsened into an
unresolved-dynamic-imports counter with no Input declared, so the directory
was silently absent from the sandbox.
Recognize the single-interpolation, literal-prefix/literal-suffix shape in
the AST, thread it through `ImportGraph`/`TestClosure` as a
`ResolvedGlobSite`, and declare it as one `fs:glob` Input in
`test_deps_config` — reusing the same mechanism plugin-go already uses for
static-embed subtrees. Shapes no bundler could resolve either (multiple
interpolations, a nested suffix path, a bare prefix) still coarsen as before.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…les too The previous fix (f83c284) only wired the new GlobImportSite handling into the test file's own import closure (build_package_import_graph/ build_test_closure). It missed the other place a dynamic import() gets discovered: resolve_runner_config_referenced_files, the separate BFS that walks setupFiles/setupFilesAfterEnv-referenced files. That's exactly the path the real report goes through — a browser-mode setup file named by setupFiles imports a tsconfig-aliased module, which itself dynamically imports a locale catalog directory — so the directory was still silently missing from the sandbox even after the first fix landed. Thread GlobImportSite through RunnerConfigScan the same way, using the same join_glob_dir core (now shared between both pipelines), and declare it as a glob Input in test_deps_config alongside the test file's own glob_sites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
js_test and js_lint carried no labels at all, so label(test)/label(lint) queries (and anything scripted on top of them, e.g. CI) couldn't select every JS test/lint target the way they already can for Go. Mirror plugin-go's own generic + tool-family-specific pairing: test/go-test in target_test.rs, lint/go-lint in driver_lint.rs. js_lint has no separate fix/check variant (no --fix support today), so it gets the single lint/js-lint pair rather than go's lint vs lint-check split. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
… read-only Every js_test Input was byte-copied into the sandbox on every run, including node_modules and any workspace-sibling code pulled in via "external" — the same cost plugin-go already avoids for its Go SDK by marking that dep group read-only, which routes staging through a symlink instead of a full copy. Mirror that here for the three groups nothing ever writes into at test time: "external" (resolved third-party/workspace-sibling code), "runner_config", and "tsconfig" (config files, only ever read). The unnamed "" group (the test file's own first-party closure) stays writable — Vitest's toMatchSnapshot() writes .snap files next to the test file, so read-only staging there would turn a first-time snapshot write into an EACCES crash. "codegen" stays writable too: a js_src target's generated files land at real source-tree paths inside the package, the same co-located-with- hand-written-source shape that makes "" unsafe, and its own Content shape isn't uniform enough across every possible js_src-labelled driver to assume it's safe the way the other three groups are. annotations never reach the input hash or JsTestDef::hash, so this is cache-key-stable — no existing js_test result goes stale. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…fig deps read-only" This reverts commit b38a5b8.
… directory symlink The first attempt (b38a5b8, reverted in 4b5f065) marked external/ runner_config/tsconfig read-only via a plain directory symlink (READ_ONLY_ANNOTATION alone) and broke real js_test runs: Node's require() calls fs.realpath() on the required file before walking up for node_modules, and a symlinked ancestor directory resolves to the stage's own isolated path — which has no matching node_modules chain (or any sibling) next to it. vitest.config.ts's own plugin imports, all genuinely present in the sandbox's node_modules, failed to resolve for exactly this reason. Add STAGE_PER_FILE_ANNOTATION alongside READ_ONLY_ANNOTATION: link_tree hardlinks every file directly into its real sandbox path instead of symlinking a directory, so no ancestor in the destination tree is ever a symlink and realpath() returns the sandbox path itself. Slightly more syscalls than a directory symlink (O(files) vs O(depth)), still far cheaper than the byte-copy this exists to skip. Added crates/driver-support/src/stage.rs's per_file_single_file_input_has_no_symlinked_ancestor as the regression test this bug needed and didn't have: it walks every ancestor of a per-file-staged leaf and asserts none is a symlink, and that the leaf's own realpath lands inside the sandbox rather than in stage/ — the exact property Node's resolver depends on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
Mirrors plugin-go's `test = False` / `recursive = True` mechanism: `provider_state(provider="js", enabled=False)` disables every js target kind for a package (package_info/js_typecheck/js_lint/js_test/js_bundle/ node_modules sync); `test=False`/`lint=False` disable one target kind at a time; `recursive=True` extends whichever key it's set alongside to descendant packages. Extracts the shared closest-package-wins precedence algorithm (state_is_recursive/state_applies_to/applicable_states) out of plugin-go into hplugin::provider so both plugins share one implementation instead of two copies of the same logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…ust errors Real bug, found via a new pluginjs-e2e test that drives a real oxlint binary end to end: both oxlint and eslint exit 0 on warning-severity findings by default, only failing the process on an error-severity rule. js_lint only checked the process exit code, so a real, reported violation (e.g. oxlint's own no-debugger, which defaults to "warn") silently passed the target — the lint gate was effectively a no-op for most default rule configurations. Fix: pass --deny-warnings (oxlint) / --max-warnings 0 (eslint) so any finding, warning or error, fails the target. Adds js_lint e2e coverage in crates/pluginjs-e2e (real npm install + real oxlint, mirroring the existing js_test e2e shape) — previously js_lint had none. The driver's own #[ignore]d "real oxlint" unit tests never actually exercised this path (their harness always passes empty inputs, so run() takes the early no-op return before invoking any linter) — noted as a separate, pre-existing gap, not fixed here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…swallows every source Reproduced the reported "No files found to lint. Please check your paths and ignore patterns." exactly against a real oxlint (1.78.0): js_lint always passes every first-party source file as an *explicit* path argument, and oxlint still applies the resolved config's own ignorePatterns to explicitly-named files — if every declared source happens to match, oxlint reports zero files and (on this version) exits nonzero, even though heph genuinely staged real files. --no-ignore does not override config-level ignorePatterns (verified), so there is no cheap CLI-flag fix that preserves heph's declared-Input set. This is a linter-config/heph-Input mismatch, not a staging bug — the files are real and correctly staged. Until ignorePatterns-aware filtering exists on the heph side (a larger, separate change), make the failure diagnosable instead of confusing: report how many source files were declared and which config resolved, and when oxlint's own "No files found to lint" wording is detected, point at the two real remedies (narrow the ignore rule, or opt the package out via the provider_state(lint = False) knob added earlier this session). New e2e (crates/pluginjs-e2e) reproduces the exact scenario with a real oxlint 1.78.0 under a real ignorePatterns-matches-everything config and asserts both the failure and the new actionable message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
…g to oxlint Reported live bug, second half: "i'm not using oxlint, i'm using eslint" — but the js provider's `linter` option had no auto-detection and defaulted to `"oxlint"` unconditionally. In a workspace with only eslint installed, that default made heph resolve a stray PATH `oxlint` binary and run it, producing an oxlint-specific failure the workspace owner had no way to explain (they never chose oxlint). `linter` now has no static default. Unset, it auto-detects from what's actually installed at `<workspace_root>/node_modules/.bin/` — never `PATH`, since a stray global binary unrelated to the project is exactly the wrong signal to decide this on. Detection fails loudly (not a guess) when neither `oxlint` nor `eslint` is installed, or when both are and the choice is genuinely ambiguous — mirroring the `pkgmanager` option's existing "no default, an ambiguous silently- picked answer is worse than requiring an explicit choice" precedent in this same provider. An explicitly configured `linter` is unaffected. `crates/pluginjs-e2e`'s shared workspace config now passes `linter: None`, so every js_lint e2e test exercises real detection rather than an assumed default; added two more fixtures/tests proving detection picks eslint correctly and that the `--max-warnings 0` fix from the prior commit actually works against a real eslint (previously untested against real eslint end to end). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
raphaelvigee
force-pushed
the
raphaelvigee/feat-plugin-js-m8-bin-e2e
branch
from
August 17, 2026 08:53
c3a68c8 to
b0fc51d
Compare
…onfig file Two changes, both from live user feedback on the auto-detection commit: 1. Detection now checks for each tool's own config file at the workspace root (.oxlintrc.json; eslint.config.*/.eslintrc.*) instead of binary presence in node_modules/.bin. A config file is the workspace owner's actual declared choice; binary presence isn't — a repo mid-migration between tools, or one that carries both as transitive/dev tooling for unrelated reasons, can have both binaries installed with only one actually configured, which made the binary-presence version spuriously call that "ambiguous". lint_config_candidates (the per-tool config filename list) moved from provider.rs into toolchain.rs so detect_linter can reuse it instead of duplicating the knowledge. 2. `linter` is no longer a provider-construction-level `Config`/YAML option at all — it's set (or left to auto-detect) per package via `provider_state(provider = "js", linter = "oxlint" | "eslint")`, the same closest-package-wins + `recursive` shape `enabled`/`test`/ `lint` already have. `resolved_host_linter` now resolves per-package states and caches the binary/version by *resolved linter name* (a keyed HashMap<String, OnceCell<...>> instead of a single OnceCell) since different packages can now resolve to different linters in the same run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
… root alone js_lint is a per-package target, but toolchain::detect_linter only ever checked the bare workspace root for a linter config — a package with its own config and no root-level config at all (a very normal monorepo shape) got a false "could not detect a linter" even though the linter config was right there. Fixed to reuse the exact same ancestor-chain walk Provider::lint_config already resolves the config's own *content* against (importgraph::find_nearest_lint_config, pkg_dir up to workspace_root, both inclusive) instead of checking workspace_root in isolation. When both tools have a config somewhere on that chain, the closer one wins — mirroring every other closest-declaration-wins resolution this plugin already has (provider_state's own ancestor precedence, find_nearest_lint_config itself); only a genuine tie (both configs in the identical directory) is treated as ambiguous. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NLhaQSMoXo1ExzAeCbu3an
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
1 participant
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.
Combines the full
plugin-jsstack (previously #361 → #362 → #363 → #364 → #365 → #292 → #367 → #370) into a single PR rebased onmaster, per request. Those 8 PRs are closed; their branches are deleted; all 45 commits are preserved here, unchanged in content, just retargeted.Milestones in this PR
js_installdriverjs_typecheckdriver, per-packagetscjs_testdriver, per-test-file granularityImportGraphcache,js_lintdriverjs_bundledriver, esm/cjs + node/browser variantsbin-e2e: dlopen the shipped js plugin cdylib for real, plus (post-test(bin-e2e): dlopen the shipped js plugin cdylib for real #370, folded in during this collapse) the peer-dependency lockfile fixes,js_testCI=1+ subprocess deadline kill, and thejs_srccodegen-input labelRebase note
Rebased cleanly onto
master(6 commits ahead at the time: kache/R2 cache backend, static Go linking by default,oci_loadref changes, build-version stamping,CARGO_TARGET_DIRoverride removal). Two conflicts, both mechanical:.github/workflows/heph.yml— master added akache stats + drain uploadsstep that must run last in thebuildjob; this stack added anUpload js plugin artifactstep. Resolved by placing the js-plugin upload before the kache drain step, preserving both.devenv.nix— master replaced direct$CARGO_TARGET_DIRreads with atarget-dirhelper script across everydevenvscript. Resolved by adopting the helper in thee2escript's js-plugin additions rather than keeping the old$CARGO_TARGET_DIRreference.cargo build --workspace --lockedverified clean post-rebase.