feat(resolve): full Node.js '#' subpath-imports (package.json "imports") support - #7028
Merged
Conversation
Ports the nub soak/security stack (nubjs/nub#442) with the wheelhouse shim workarounds baked in: - scripts/soak/: soak window parity gate + fixer (SOAK_DAYS=7 in constants.mts is the single source), soaked dependency updater (taze for npm, rustup cargo for crates), and the external-tools installer (SRI-verified rack + PATH handles + sfw firewall shims). - Soak surfaces: .npmrc min-release-age, tools/pnpm-workspace.yaml minimumReleaseAge (pnpm domain isolated in tools/ so the root stays npm-only), tools/taze.config.mts (imports SOAK_DAYS), .cargo/config.toml min-publish-age (nightly-only; inert on stable), .github/dependabot.yml cooldown per update block (the renovate-check equivalent, reworked for dependabot). - external-tools.json: exact pins + sha512 SRI for pnpm 11.8.0, npm 12, sfw-free/-enterprise 1.13.1, zizmor 1.26.1, agentshield 1.4.0, skillspector @2eb84478. - sfw shims carry the known workarounds: per-command recursion sentinel (not PATH-strip), fail-open when sfw is absent, symlink clobber guard, rack-pinned pnpm/npm resolution. - zizmor workflow (hash-pinned action, gate at high) with a documented starting config: official actions ref-pin, six existing third-party actions grandfathered pending a digest-pin sweep, cache-poisoning (49 Low-confidence findings) disabled, release-packages.yml excessive-permissions scoped-ignored pending a job-level split. - security-audit.yml gains always-run soak-gate, agent-scan (AgentShield over .claude/), and skills-scan (NVIDIA SkillSpector over .claude/skills/, static --no-llm path) jobs. - .claude/skills/soak/SKILL.md documents the workflow.
The zizmorcore/zizmor-action run hit startup_failure (repo Actions allowlist), and the rack binary is the better shape anyway: one pin source (external-tools.json), and local tools:install audits with the exact bits CI uses. Token-only-when-nonempty works around zizmor treating an empty --gh-token as real and then fatally erroring.
1.14.0 (published 2026-07-23) fixes two things the shims care about: sfw's own diagnostics now go to stderr (stdout stays transparent for callers capturing `pnpm --version` through a shim), and the child env gains a NO_PROXY loopback exemption (localhost,127.0.0.1,::1) so locally-mocked registries are never proxied. Inside the 7-day window until 2026-07-30, so both pins carry the dated soakBypass annotation; tools:check will demand its removal once the window clears — prune the two annotations then.
…aked releases All three cleared the 7-day window (zizmor 1.28.0 published 07-21, pnpm 11.15.1 07-19, npm 12.0.1 07-10), so no bypass annotations. pnpm 11.16/11.17 and taze 19.16.0 are still soaking; skillspector upstream (2.4/2.5) is entirely inside the window — follow-up bumps once cleared. Gate re-verified: zizmor 1.28.0 with the shipped config reports 0 findings at high across .github/.
… bot PR The soak gates fail closed by design: the day a soakBypass window clears, tools:check goes red until the two annotation lines come off. Failing closed is right; making a human notice is not. So: - external-tools.mts gains --fix (npm run tools:fix): prunes soakBypass annotations whose removable date has passed, then re-runs the checks. Valid-but-expired only — malformed dates stay findings for a human. - soak-autofix.yml (daily cron + dispatch) runs soak:fix + tools:fix, and when anything changed commits to bot/soak-autofix and opens (or force-updates) a PR. Note in-workflow: PRs opened with the default github.token don't trigger CI; set the optional SOAK_AUTOFIX_TOKEN secret to make the bot PRs run checks like any other. Verified end-to-end with a planted expired annotation: --fix prunes it, check returns green, and the fixer is idempotent (unit-tested).
…flows checkout v7.0.1 (3d3c42e5) + setup-node v7.0.0 (82076278) across soak-autofix / zizmor / security-audit — both releases cleared the 7-day window (07-20 / 07-14). Also splits the PATH export in agent-scan (SC2155). The legacy workflow fleet stays on ref pins per the documented digest-pin sweep in .github/zizmor.yml.
… unsafe An EXPIRED soakBypass / exclude pin means the version has fully soaked: the bypass no longer bypasses anything and the pin stays SRI-verified. Failing closed on that turned a no-risk cosmetic state into a red required check that flips overnight with zero code change — the exact noise that trains people to admin-bypass (and the model wheelhouse deliberately avoids: informational + auto-drop). Now: expired-but-VALID annotations are warnings (exit 0), surfaced by staleBypasses / staleExcludes and pruned by --fix + the daily soak-autofix workflow. Missing, malformed, or wrong-arithmetic annotations stay hard failures — unauditable IS unsafe. This also defuses the 2026-07-30 expiry of this PR's own sfw annotations.
- platformKey(): detect musl via the loader heuristic — the -musl pnpm pins were dead keys and a musl host silently installed glibc bits; tools with no -musl pin now fail loud instead. - RUSTUP_CARGO honors CARGO_HOME (custom cargo homes reported the rustup shim as missing). - parseExcludeEntries: tolerate a trailing comment on the minimumReleaseAgeExclude key line — previously the block never opened and every entry beneath escaped validation. - checkCatalogParity: malformed package.json is a Finding, not a crash. - soak-autofix workflow: main-ref guard (dispatch on a topic branch can't force-push the bot branch), concurrency group, and fixer exit status captured + re-raised AFTER the mechanical commit instead of '|| true' masking runtime failures. - sfw shims: fail-open is no longer silent-open — one stderr line when sfw is missing (never on the sentinel re-entry path). - GITHUB_TOKEN on the CI install steps (github.com release fetches). - schematic YYYY-MM-DD example dates in the yaml + skill (the concrete examples were expired copy-paste bait); em-dashes restored in external-tools.json (ensure_ascii artifact).
Wheelhouse lesson: enterprise sfw defaults to BLOCK for non-registry hosts, which breaks ordinary dev flows (API calls, git clones) the day a SOCKET_SECURITY_KEY lands. Free tier hardcodes ignore and disregards the var, so setting it unconditionally is always safe.
- checkDockerPrebake: parse the rustup install line's argument list instead of substring-matching the msrv (a multi-toolchain install line false-failed the check). - RUSTUP_CARGO resolves cargo.exe on win32. - soak-autofix: lease-checked force push (fetch the bot branch, then --force-with-lease) so a concurrent actor's commits are never clobbered.
…ost comment Same drift pullfrog flagged on the nub twin: the skill still said the gates "fail closed when a bypass window clears" — expired-but-valid annotations warn and get pruned by soak:fix / the soak-autofix workflow; invalid annotations are what fail. The shim comment now claims only what the source shows about SFW_UNKNOWN_HOST_ACTION (the enterprise config parses it; inert for free).
Greptile P1 on the aube twin: the pruners and stale lists accepted any valid-ISO annotation whose removable date had passed — including one whose removable was WRONG (earlier than published + SOAK_DAYS). Such an annotation must surface as the hard check failure it is; treating it as soaked would silently delete a bypass whose real window may still be open. All four surfaces (staleExcludes, fixWorkspaceYaml, staleBypasses, pruneExpiredSoakBypasses) now require the arithmetic to hold before an annotation counts as stale or prunable; regression tests cover the wrong-math-expired case.
The nub node-18 compat leg died on `download failed 500` — the first authed fetch of a PUBLIC sfw release asset after GITHUB_TOKEN was added to the step env. Whether that 500 was token-induced (an Actions token against a cross-org public asset endpoint) or a transient GitHub blip, one attempt was too brittle: download() now retries without auth when an authed fetch fails (public assets need no credential), and once more after 2s on a 5xx. Regression test pins the fallback dropping the Authorization header.
Adversarial self-review of the renovate/npmrc/yaml fixers, prompted by
the 20-line diff my own soak:fix produced on aube's renovate.json:
- fixRenovateConfig rewrote the WHOLE file via JSON.parse +
re-stringify, collapsing hand-written single-line arrays and
reformatting unrelated packageRules (aube's decmpfs musl hold among
them). It is now a targeted text edit: only the minimumReleaseAge
line changes, every other byte is preserved. A regression test
asserts exactly one changed line and that the decmpfs rule survives
verbatim.
- The insert path produced INVALID JSON for a minimal `{}` config
(`{,\n ...}`); guarded and covered by a test.
- fixNpmrc / fixWorkspaceYaml matched trailing `\s*$` under /m — `\s`
matches newlines, so the replacement swallowed blank lines after the
key. Now `[ \t]*$`; verified soak:fix is a no-op on a clean tree.
- checkRenovateConfig now also requires `internalChecksFilter: strict`.
Without it renovate's default flexible mode raises updates that have
NOT cleared minimumReleaseAge — the window silently stops biting.
- The no-pinned-asset error names the musl case and lists the pinned
platforms: sfw ships no musl asset, so an alpine runner hits this,
and the old message gave nothing to act on.
Verified alongside: decmpfs stays at 0.1.0 under `cargo update` (the
`=0.1.0` requirement holds, so the soak updater cannot smuggle in the
musl-breaking 0.1.2), and `--force-with-lease` correctly rejects a
concurrent update even when the preceding fetch fails, and still
creates the branch on a first run.
Adversarial self-review of the renovate/npmrc/yaml fixers, prompted by
the 20-line diff my own soak:fix produced on aube's renovate.json:
- fixRenovateConfig rewrote the WHOLE file via JSON.parse +
re-stringify, collapsing hand-written single-line arrays and
reformatting unrelated packageRules (aube's decmpfs musl hold among
them). It is now a targeted text edit: only the minimumReleaseAge
line changes, every other byte is preserved. A regression test
asserts exactly one changed line and that the decmpfs rule survives
verbatim.
- The insert path produced INVALID JSON for a minimal `{}` config
(`{,\n ...}`); guarded and covered by a test.
- fixNpmrc / fixWorkspaceYaml matched trailing `\s*$` under /m — `\s`
matches newlines, so the replacement swallowed blank lines after the
key. Now `[ \t]*$`; verified soak:fix is a no-op on a clean tree.
- checkRenovateConfig now also requires `internalChecksFilter: strict`.
Without it renovate's default flexible mode raises updates that have
NOT cleared minimumReleaseAge — the window silently stops biting.
- The no-pinned-asset error names the musl case and lists the pinned
platforms: sfw ships no musl asset, so an alpine runner hits this,
and the old message gave nothing to act on.
Verified alongside: decmpfs stays at 0.1.0 under `cargo update` (the
`=0.1.0` requirement holds, so the soak updater cannot smuggle in the
musl-breaking 0.1.2), and `--force-with-lease` correctly rejects a
concurrent update even when the preceding fetch fails, and still
creates the branch on a first run.
Auditing a sibling fleet repo (abitious) for compatibility surfaced an unguarded bypass: npm >= 11.17 has its OWN exclude surface, `min-release-age-exclude[]=<spec>`, parallel to pnpm's `minimumReleaseAgeExclude` block — and the gate validated only the pnpm side. `min-release-age-exclude[]=lodash@1.2.3` was therefore an unvalidated, never-expiring hole in exactly the rule the yaml side enforces. checkNpmrc now applies the same law to .npmrc: bare names and `@scope/*` globs are standing trust (the shape real repos use for trusted scopes, so this is not a churn tax), while a VERSION-PINNED entry needs the `# published: | removable:` annotation with correct arithmetic and real calendar dates. Tests cover trusted-glob, unannotated, correct, wrong-arithmetic, and impossible-date cases.
Verified rather than assumed, and the assumption was wrong: cargo treats
an [unstable] key it does not implement as a WARNING ("unused config key
`unstable.min-publish-age`") and exits 0. Measured on nightly
2026-03-21, which has no such -Z — so `cargo +nightly update` on a
merely-OLD nightly resolved every crate with NO window at all while the
run reported success. The tooling was claiming a protection it had not
applied.
updateCargo now captures stderr and treats that warning as a hard
failure: the lockfile changes are unsoaked, so say so and exit nonzero
with the fix (`rustup update nightly`). The detector is an exported,
unit-tested predicate pinning cargo's exact wording.
perry rides stable, where the key is expected to be inert, so there the
same detection downgrades to an explicit note naming dependabot cooldown
as the enforcing surface for cargo deps — no silent no-op either way.
…env bypass
Re-measured on a current nightly (2026-07-27, cargo 1.99.0-nightly): the
`-Z min-publish-age` feature IS implemented there and the window visibly
bites — it holds a too-fresh release back ("available: v0.2.189,
published 7 days ago"). Both measurements are now recorded in the
comment and the skill, since the OLD nightly (2026-03-21) is the
evidence that a stale toolchain skips the window silently.
Running the real updater surfaced the other half of the contract: the
window can make re-resolution IMPOSSIBLE, not just conservative. When a
requirement's only candidate is inside the window (aube today:
`clap_usage = "^4"`, whose 4.0.0 shipped 3 days ago) cargo fails the
whole update — correct behavior, but its own help line advertises
`CARGO_RESOLVER_INCOMPATIBLE_PUBLISH_AGE=allow`, a blanket env-var
bypass this design deliberately does not have. The updater now detects
that failure and prints ordered options (wait it out, repin so a soaked
version satisfies the requirement, or adopt the fresh release as a
reviewable commit) with an explicit warning against the env bypass.
Predicate is exported and unit-tested against cargo's real wording.
An independent hostile review of the three sibling PRs found real defects, including one where my own test had verified only the safe half of the case: - soak-autofix no longer force-pushes at all. `git fetch origin $BRANCH` UPDATES the remote-tracking ref (actions/checkout leaves the default wildcard refspec), so the following --force-with-lease took its lease against whatever another actor had just pushed and overwrote it — the classic fetch-before-lease anti-pattern, and the inline comment asserting "a concurrent actor's commits are never clobbered" was false. Demonstrated: a human commit onto the open autofix PR was discarded by the next scheduled run. My earlier test only covered the fetch-FAILS path (which is genuinely safe, rejecting with "stale info"). The step now stashes the fixes, bases the work on the existing bot branch when there is one, and plain-pushes: human commits survive by construction, an empty re-run exits 0 instead of pushing a no-op commit, and a genuine conflict fails loudly instead of being resolved by deletion. - fixWorkspaceYaml's prune set must EQUAL staleExcludes' warn set: it was missing the VERSION_PIN_RE guard, so a bare-name / `@scope/*` standing-trust entry sitting under an expired annotation line was deleted by --fix, silently re-arming the soak for a whole scope inside a bot commit advertised as touching only annotation lines. - download() retry semantics split by meaning: 401/403/404 with a token means the credential is the problem (retry unauthenticated), >=500 is transient (retry with the SAME auth). Dropping auth on 5xx made a private asset 404 on the retry, report a bogus "download failed 404", and never be able to succeed. The SRI is verified either way, so no retry can substitute a different artifact.
- fixRenovateConfig still matched a trailing `\s*$`, which under /m eats the NEWLINES after the value: replacing through it silently deleted the blank line that followed. That is the exact defect the same commit fixed in fixNpmrc and fixWorkspaceYaml, kept in the third fixer. Verified with a config carrying a blank line after the key: it disappeared before, survives now. - checkPins now rejects a soakBypass whose `version` is not the version actually pinned. Bump a pin and leave the annotation behind and the ledger vouches for a release that is no longer installed — "1.13.1 was adopted early" while 1.14.0 ships unreviewed. A mismatch is unauditable, so it is a hard finding, not a stale-annotation warning. - soak-autofix.yml's header still described the gates as failing closed on a cleared window; the fourth and last sibling of that stale premise. Expired is a warning, invalid still fails, and the workflow's job is convergence rather than rescue. - Two paths were changed without a test covering them, both added: the multi-arg `rustup toolchain install 1.91.0 1.93.0` case that motivated replacing the substring msrv match (only the negative case was covered), and the `>= 500` retry branch that the retry commit is named for (the existing test exercises only the auth fallback).
… a real npm CLI needs Compiling Socket Firewall (sfw — a TLS-MITM proxy CLI with undici, node-forge, iovalkey, zod, … in its graph) end-to-end surfaced four independent blockers. Fixed here: 1. auto-optimize feature skew (driver.rs / freshness.rs): the perry binary's baked-in cross-feature list tracks the branch it was BUILT from, but the auto-optimize cargo build resolves against the checkout on disk. One unknown `perry-runtime/<feat>` failed the whole resolve, and the silent prebuilt fallback linked without the routed ext-pump entrypoints — undefined-js_* errors two stages from the cause. New retain_workspace_declared_features() drops names the checkout's perry-runtime / perry-stdlib don't declare (features table + optional deps, fail-open on unreadable manifests) before the build stamp is computed, and the cargo-failure fallback now says what the consequence and remedy are. 2. perry-ext-zlib zstd surface: undici's web-fetch content decoding references js_zlib_create_zstd_decompress unconditionally, but only perry-stdlib's `compression` module carried the zstd codecs — and routing node:zlib to the ext archive strips that feature. Port the full surface (create factories, sync/async one-shots, streaming write-codec via zstd::stream::write) so the routed archive is self-sufficient. 3. class X extends DOMException (codegen + runtime): undici probes DOMException inheritability at module load (websocketerror.js), and the name was neither in the builtin-parent list nor backed by a subclass initializer — the compiled binary died at startup with 'DOMException is not a function'. Add js_dom_exception_subclass_init (stamps message/name/code onto the subclass instance) wired through both the explicit super() lowering and the implicit-ctor NativeInstanceBase chain walk. 4. panic-runtime dedup for prebuilt (panic=unwind) wrappers co-linked with a panic=abort auto-optimized stdlib (strip_dedup.rs): the name-containment rule never nominated the wrapper's panic_unwind member (stdlib bundles panic_abort under a different name), and the localize pass severed the std-cgu → panic_unwind __rust_drop_panic edge that abort stdlibs cannot re-provide. Nominate panic_unwind in the nosharedeps fixed-point (protected exactly when the stdlib can't cover it), and skip localizing panic symbols a sibling member still references. Allocator shims stay always-localized: leaving the wrapper's system-malloc shim global beats the runtime's mimalloc at link and breaks pointer classification (silent console loss). With these, sfw and sfw-free compile, link, and run as native arm64 binaries straight from their TypeScript entrypoints.
…s") support Replace the happy-path '#' handling from PerryTS#5039 with a spec-complete PACKAGE_IMPORTS_RESOLVE implementation (resolve/subpath_imports.rs): - package scope walk to the nearest package.json with an "imports" object, stopping at node_modules boundaries - exact keys; '*' wildcard patterns with Node's best-match rule (longest prefix, patternKeyCompare tie-break) - string / fallback-array / conditional-object targets; conditions matched in the exports resolver's priority order (perry, node, import, module, default, require - node above default) - bare-package targets re-enter node_modules resolution ('node:' builtins included) - spec rejections with descriptive errors: '#', '#/...', trailing '/', and targets or wildcard captures traversing '..'/node_modules or escaping the package directory - perry's TS-first extension probing, so "#lib/*": "./src/lib/*" resolves #lib/foo to src/lib/foo.ts Wired before the tsconfig-paths fallback in resolve_import (spec resolution outranks aliasing; falls through when no imports map governs the importer), and into check --check-deps so '#' imports stop producing false R003 "not found in node_modules" errors.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe pull request adds Node.js Node subpath-import resolution
Optimized library feature filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Importer
participant resolve_import
participant subpath_imports
participant DependencyResolver
Importer->>resolve_import: resolve `#specifier`
resolve_import->>subpath_imports: resolve_subpath_import
subpath_imports-->>resolve_import: File, External, NotDefined, or Err
DependencyResolver->>subpath_imports: resolve `#specifier`
subpath_imports-->>DependencyResolver: mapped file or external package
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
crates/perry/src/commands/deps.rs (1)
169-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider surfacing the resolver's reason instead of discarding it.
Err(_)throws away a fully descriptiveSubpathImportError(invalid#/…, escaping target, …), and the External-failure path recordsimport_sourcerather than the mapped specifier — so users get a generic "unresolved import#dep" with no hint that the real problem is a badimportstarget or a missingsome-pkg. Threading the error text (or mapped spec) into the diagnostic would make--check-depsoutput actionable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/deps.rs` around lines 169 - 186, The unresolved-import handling around SubpathImportOutcome must preserve and surface SubpathImportError details instead of matching Err(_). Update the relevant resolver flow to carry the error text, and for External failures report the mapped specifier rather than only import_source, while retaining the tsconfig_paths fallback and existing unresolved_imports tracking behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/zizmor.yml:
- Around line 32-38: Replace the blanket cache-poisoning disable in the zizmor
configuration with workflow/file-specific ignore entries for only the existing
grandfathered Swatinem/rust-cache findings. Preserve suppression for those known
release workflows while allowing cache-poisoning findings in other workflows to
fail CI; remove any entries for debt that has been cleaned up.
In `@changelog.d/6912-security-tooling-soak.md`:
- Line 1: Update the changelog entry’s SkillSpector description to state that
SHA-512 SRI verification applies only to downloaded artifacts, not the
Git-SHA-pinned uv-project installation.
In `@crates/perry-ext-zlib/src/stream.rs`:
- Around line 417-421: Update the CodecState::ZstdDec branch to flush the zstd
stream::write::Decoder before extracting its inner buffer, ensuring all buffered
decompressed bytes are written. Replace the direct into_inner() call with the
decoder operation that performs the final flush and returns the underlying
writer, while leaving the ZstdEnc branch unchanged.
In `@crates/perry-runtime/src/event_target.rs`:
- Around line 337-358: Update js_dom_exception_subclass_init to root this_value,
message, and name with RuntimeHandleScope before coercion and property writes.
Reload the boxed exception, message, and name values from their rooted handles
after every allocation or user-code-invoking operation, rather than reusing
exception, message_ptr, or name_ptr raw pointers; ensure all subsequent string
conversion and set_event_field calls use the reloaded values.
In `@crates/perry/src/commands/compile/optimized_libs/freshness.rs`:
- Around line 210-217: Update declared_feature_names and its collect_optional
logic to detect feature declarations using dep:<name> for each optional
dependency. Exclude those dependency names from the returned declared feature
set, while retaining optional dependencies that still expose Cargo’s implicit
same-named feature.
In `@crates/perry/src/commands/compile/resolve/subpath_imports.rs`:
- Around line 354-375: Validate the wildcard capture with the same
has_invalid_path_segment check used by the ./ target branch before substituting
it in the non-relative external-target path. Reject captures containing
traversal or node_modules segments via invalid_target, and only construct and
return SubpathImportOutcome::External after validation.
In `@scripts/soak/external-tools.mts`:
- Around line 98-136: Update checkPins to require valid release-date metadata
for every tool pin, reject future published dates, and calculate the pin’s age
against today. While the release is younger than SOAK_DAYS, require soakBypass
and validate its version and dates; allow the bypass to be absent once the
release reaches the required age, while preserving existing date arithmetic and
integrity checks.
- Around line 492-503: Update installDeps so a successfully spawned installer
with a non-zero res.status continues iterating through PM_DEP_INSTALLERS instead
of returning immediately. Return the status only for a successful installation,
while preserving the existing res.error fallback behavior and final fallback
result.
In `@scripts/soak/external-tools.test.mts`:
- Around line 328-333: Strengthen the installTool test for the uv-project entry
by capturing its emitted output and asserting that the printed uvx command
contains the expected package and pinned revision from the manifest. Keep the
existing no-install invocation and ensure the test fails if the command is
missing or unpinned.
In `@scripts/soak/soak.mts`:
- Around line 83-145: Extend the npmrc validation flow around checkNpmrcExcludes
and checkNpmrc to report version-pinned min-release-age exclusions whose
removable date has passed, matching the existing staleExcludes behavior for
workspace YAML. Update the npmrc fix path to prune those expired entries while
preserving bare-name exclusions and valid, non-expired annotations; reuse the
existing date and Finding conventions rather than adding a separate format.
- Around line 401-421: The checkTazeConfig function currently accepts comments
or unrelated substrings; validate actual Taze configuration bindings instead.
Require an import that binds SOAK_DAYS from the soak constants module and a
maturityPeriod property whose value is SOAK_DAYS, while preserving Finding
details for missing or invalid bindings.
In `@scripts/soak/soak.test.mts`:
- Around line 32-54: Update scripts/soak/soak.test.mts lines 32-54 to import
SOAK_MINUTES, interpolate canonical clean window values, and use addDaysIso to
derive removable dates instead of hardcoded seven-day fixtures; update
scripts/soak/external-tools.test.mts lines 37-60 to derive expired removable
dates from SOAK_DAYS rather than the hardcoded 2020-01-08 value, keeping
fixtures aligned with the soak constants.
---
Nitpick comments:
In `@crates/perry/src/commands/deps.rs`:
- Around line 169-186: The unresolved-import handling around
SubpathImportOutcome must preserve and surface SubpathImportError details
instead of matching Err(_). Update the relevant resolver flow to carry the error
text, and for External failures report the mapped specifier rather than only
import_source, while retaining the tsconfig_paths fallback and existing
unresolved_imports tracking behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fc79ed9-8b1c-4d75-b345-c6a74d6897b9
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktools/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.cargo/config.toml.claude/skills/soak/SKILL.md.github/dependabot.yml.github/workflows/security-audit.yml.github/workflows/soak-autofix.yml.github/workflows/zizmor.yml.github/zizmor.yml.npmrcchangelog.d/6912-security-tooling-soak.mdchangelog.d/7021-real-npm-cli-compile-gaps.mdchangelog.d/7028-subpath-imports.mdcrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/new_helpers.rscrates/perry-codegen/src/runtime_decls/strings_part2.rscrates/perry-ext-zlib/Cargo.tomlcrates/perry-ext-zlib/src/stream.rscrates/perry-runtime/src/event_target.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/optimized_libs.rscrates/perry/src/commands/compile/optimized_libs/driver.rscrates/perry/src/commands/compile/optimized_libs/freshness.rscrates/perry/src/commands/compile/optimized_libs/tests.rscrates/perry/src/commands/compile/resolve.rscrates/perry/src/commands/compile/resolve/subpath_imports.rscrates/perry/src/commands/compile/strip_dedup.rscrates/perry/src/commands/deps.rsexternal-tools.jsonpackage.jsonscripts/soak/constants.mtsscripts/soak/external-tools.mtsscripts/soak/external-tools.test.mtsscripts/soak/paths.mtsscripts/soak/soak.mtsscripts/soak/soak.test.mtsscripts/soak/update-deps.mtsscripts/soak/update-deps.test.mtstools/package.jsontools/pnpm-workspace.yamltools/taze.config.mts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry/src/commands/compile/optimized_libs/freshness.rs (1)
200-224: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat a missing
[features]table as part of the declared feature set.
declared_feature_namescurrently returnsNonewhen[features]is absent, soretain_workspace_declared_featuresskips filtering and keeps staleperry-runtime/<feature>entries. Since optional dependencies can expose same-nAMED implicit features, collect only optional dependencies and normalize a missing feature table to an empty set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/optimized_libs/freshness.rs` around lines 200 - 224, Update declared_feature_names to treat a missing [features] table as an empty BTreeSet instead of returning None, while still collecting optional dependency names and excluding those hidden by dep: entries. Preserve the existing filtering behavior in retain_workspace_declared_features so stale perry-runtime/<feature> entries are removed when no features table exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/perry/src/commands/compile/optimized_libs/freshness.rs`:
- Around line 200-224: Update declared_feature_names to treat a missing
[features] table as an empty BTreeSet instead of returning None, while still
collecting optional dependency names and excluding those hidden by dep: entries.
Preserve the existing filtering behavior in retain_workspace_declared_features
so stale perry-runtime/<feature> entries are removed when no features table
exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8feb6a23-8978-44e7-9bde-5f1b93f13ce2
📒 Files selected for processing (15)
.github/zizmor.ymlchangelog.d/6912-security-tooling-soak.mdcrates/perry-codegen/src/expr/this_super_call.rscrates/perry-ext-zlib/src/stream.rscrates/perry-runtime/src/event_target.rscrates/perry/src/commands/compile/optimized_libs/freshness.rscrates/perry/src/commands/compile/optimized_libs/tests.rscrates/perry/src/commands/compile/resolve/subpath_imports.rsexternal-tools.jsonscripts/soak/external-tools.mtsscripts/soak/external-tools.test.mtsscripts/soak/soak.mtsscripts/soak/soak.test.mtsscripts/soak/update-deps.mtsscripts/soak/update-deps.test.mts
🚧 Files skipped from review as they are similar to previous changes (11)
- external-tools.json
- crates/perry-codegen/src/expr/this_super_call.rs
- scripts/soak/update-deps.test.mts
- crates/perry/src/commands/compile/optimized_libs/tests.rs
- crates/perry-runtime/src/event_target.rs
- scripts/soak/update-deps.mts
- .github/zizmor.yml
- crates/perry-ext-zlib/src/stream.rs
- scripts/soak/external-tools.mts
- crates/perry/src/commands/compile/resolve/subpath_imports.rs
- scripts/soak/soak.test.mts
added 2 commits
July 30, 2026 08:32
# Conflicts: # changelog.d/7021-real-npm-cli-compile-gaps.md # crates/perry/src/commands/compile/optimized_libs.rs # crates/perry/src/commands/compile/optimized_libs/driver.rs # crates/perry/src/commands/compile/optimized_libs/freshness.rs # crates/perry/src/commands/compile/optimized_libs/tests.rs # crates/perry/src/commands/deps.rs
# Conflicts: # crates/perry/src/commands/compile/resolve.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/perry/src/commands/deps.rs (1)
141-182: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
ErrandNotDefinedare treated identically, discarding the resolver's descriptive rejection reason.
subpath_imports::resolve_subpath_importreturnsErrwith a specific reason for spec violations (invalid specifier, escaping target, etc.) — the PR explicitly highlights "descriptive rejection of invalid paths and escapes" as a resolver feature, andresolve.rssurfaces it viaeprintln!. Here,Err(_)is merged into the same arm asNotDefined(Line 166), so a genuinely invalid#specifier ends up reported the same generic way as an import simply not covered by anyimportsmap (eventually a plain "unresolved import" entry), losing the actionable detail that would help a user fix a malformedpackage.jsonversus install a missing package.Consider branching on
Err(e)separately to record/report the specific reason (e.g. attach it to the unresolved entry or print a warning), consistent with the compiler's behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/deps.rs` around lines 141 - 182, Handle Err(error) from resolve_subpath_import separately from SubpathImportOutcome::NotDefined in the subpath import branch. Preserve the tsconfig paths fallback for NotDefined, but report or attach the resolver’s descriptive error for invalid specifiers or escaping targets when recording the unresolved import, consistent with the compiler’s resolve_subpath_import behavior.crates/perry/src/commands/compile/resolve.rs (1)
345-370: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCorrect per Node semantics, but now unconditionally walks to filesystem root with no caching.
The algorithm matches Node's
NODE_MODULES_PATHSand legitimately fixes the pnpm nested-dependency case. However, unlike a "stop at first found" walk, this now always continues to the filesystem root for every call (once persearch_pathsentry, per bare-specifier resolution inresolve_import), doing repeatedis_dir()syscalls for directories most files in a project share. For large codebases with many source files, this is a real per-file I/O cost that wasn't previously incurred to this degree.Consider memoizing per starting directory (e.g. a
HashMap<PathBuf, Rc<Vec<PathBuf>>>cache threaded through the resolver state) so identical ancestor chains aren't re-walked for every importing file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/resolve.rs` around lines 345 - 370, Memoize the ancestor-directory walk performed by ancestor_node_modules_dirs so repeated resolutions from the same starting directory reuse the computed chain instead of repeating filesystem is_dir checks to the root. Add a cache to the resolver state, keyed by the starting PathBuf and storing the resulting node_modules directories, and update resolve_import/search-path resolution to consult and populate it while preserving nearest-first ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry/src/commands/compile/resolve.rs`:
- Around line 345-370: Memoize the ancestor-directory walk performed by
ancestor_node_modules_dirs so repeated resolutions from the same starting
directory reuse the computed chain instead of repeating filesystem is_dir checks
to the root. Add a cache to the resolver state, keyed by the starting PathBuf
and storing the resulting node_modules directories, and update
resolve_import/search-path resolution to consult and populate it while
preserving nearest-first ordering.
In `@crates/perry/src/commands/deps.rs`:
- Around line 141-182: Handle Err(error) from resolve_subpath_import separately
from SubpathImportOutcome::NotDefined in the subpath import branch. Preserve the
tsconfig paths fallback for NotDefined, but report or attach the resolver’s
descriptive error for invalid specifiers or escaping targets when recording the
unresolved import, consistent with the compiler’s resolve_subpath_import
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77206f7c-79ce-4a4c-8379-cdccf96392e3
📒 Files selected for processing (4)
crates/perry/src/commands/compile/optimized_libs/freshness.rscrates/perry/src/commands/compile/optimized_libs/tests.rscrates/perry/src/commands/compile/resolve.rscrates/perry/src/commands/deps.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry/src/commands/compile/resolve.rs (2)
937-944: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPreserve
nullas a terminal wildcard match.When the most-specific wildcard targets
null,collectemits no template, so broader exports can still add the path to the candidate list. Mapnullthrough conditional branches/array elements as a distinct blocked result and stop lower-priority wildcard expansion for that match; add a regression test for overlapping broad andnullpatterns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/resolve.rs` around lines 937 - 944, Update the wildcard expansion flow around collect and the pattern loop to represent a null template as a distinct terminal blocked result rather than omitting it. Propagate that blocked result through conditional branches and array elements, prevent lower-priority wildcard patterns from adding candidates after the most-specific match resolves to null, and add a regression test covering overlapping broad and null patterns.
921-925: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestrict package export patterns to exactly one
*.
split_once('*')accepts multi-wildcard keys and would treat them as validexportspatterns. Node requires subpackage patterns to contain a single*, so filter or reject keys with multiple asterisks before applying the wildcard expansion. Add a regression test for keys like"./**"or"./a*/b*".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/resolve.rs` around lines 921 - 925, Update the export-pattern filtering in the map iteration that builds patterns so keys are accepted only when they contain exactly one '*' before wildcard expansion; reject multi-wildcard keys such as "./**" and "./a*/b*". Add a regression test covering these invalid patterns and verify they are not expanded as valid exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/perry/src/commands/compile/resolve.rs`:
- Around line 937-944: Update the wildcard expansion flow around collect and the
pattern loop to represent a null template as a distinct terminal blocked result
rather than omitting it. Propagate that blocked result through conditional
branches and array elements, prevent lower-priority wildcard patterns from
adding candidates after the most-specific match resolves to null, and add a
regression test covering overlapping broad and null patterns.
- Around line 921-925: Update the export-pattern filtering in the map iteration
that builds patterns so keys are accepted only when they contain exactly one '*'
before wildcard expansion; reject multi-wildcard keys such as "./**" and
"./a*/b*". Add a regression test covering these invalid patterns and verify they
are not expanded as valid exports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 668bc234-478c-486b-af68-6856cf66587f
📒 Files selected for processing (4)
crates/perry/src/commands/compile.rscrates/perry/src/commands/compile/optimized_libs/freshness.rscrates/perry/src/commands/compile/optimized_libs/tests.rscrates/perry/src/commands/compile/resolve.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/perry/src/commands/compile/optimized_libs/freshness.rs
- crates/perry/src/commands/compile.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements spec-complete
#subpath-import resolution (Node's PACKAGE_IMPORTS_RESOLVE / PACKAGE_IMPORTS_EXPORTS_RESOLVE,lib/internal/modules/esm/resolve.js) in bothperry compileandperry check --check-deps. Replaces the happy-path-only#resolver from #5039.Semantics
package.jsonwith animportsobject, stopping atnode_modulesboundaries. (Documented deviation: apackage.jsonwithoutimports— e.g. a{"type":"module"}stub indist/— does not end the walk; treating stubs as scope roots could only ever fail.)*wildcard keys compete via Node'spatternKeyComparebest-match (ported exactly).perry/node/import/module/default/require— same model and samenode-above-defaultranking asresolve_exports(the two resolvers must agree; a{node, default: browser}pair must pick the node build). Node matches in object-key order, but serde_json maps are sorted so key order is unobservable here — caught by a failing test where alphabetical order madedefaultbeatnode.#,#/…, trailing/, targets not starting with./that aren't bare packages,../node_modulestraversal, lexical package-dir escape, and..-smuggling wildcard captures.node:builtins) returnExternaland re-enter ordinary package resolution, per spec../targets probe throughresolve_with_extensions, so"#lib/*": "./src/lib/*"resolves#lib/foo→src/lib/foo.ts(same policy as the tsconfig-pathsfallback).Wiring
resolve_import: subpath imports run before the tsconfig-paths fallback (spec resolution wins over aliasing);NotDefinedstill falls through so existing tsconfig#…aliases keep working.check --check-deps:#imports resolve through the same module;Externaltargets verify as packages/builtins. Fixes the falseerror[R003]: Package '#lib/x' not found in node_modulesfor every subpath import.Validation
node:targets, escape rejections, scope-walk cases, patternKeyCompare) — full suite: 778 passed, 0 failed.check --check-depswent from 3× R003 to clean; compiled binary runs and thenodecondition wins.#lib/*everywhere):check --check-depswent from 17× R003 to "All checks passed".Known limitation: invalid-specifier/target errors from the compile resolver surface as stderr warnings (resolve_import returns
Option— no diagnostics channel);checkreports them as R003 diagnostics.Summary by CodeRabbit
#subpath imports viapackage.json"imports", including exact/wildcard matches, conditional targets, and fallback arrays with TypeScript-friendly resolution.#imports (including forcheck --check-deps), avoiding false “missing package” errors.[features]table and updated related expectations.