Skip to content

feat(pnpmfile): emit ctx.log records as pnpm:hook ndjson on stdout#440

Merged
jdx merged 4 commits intomainfrom
claude/strange-chandrasekhar-98d62c
May 1, 2026
Merged

feat(pnpmfile): emit ctx.log records as pnpm:hook ndjson on stdout#440
jdx merged 4 commits intomainfrom
claude/strange-chandrasekhar-98d62c

Conversation

@jdx
Copy link
Copy Markdown
Contributor

@jdx jdx commented Apr 30, 2026

Summary

Routes pnpmfile ctx.log(...) calls through a structured channel, so under --reporter=ndjson they appear as {\"name\":\"pnpm:hook\",\"prefix\":<root>,\"from\":<pnpmfile>,\"hook\":...,\"message\":...} records on stdout — matching pnpm's surface — while default mode still gets the friendly [pnpmfile] message line on stderr. Unblocks the ndjson pnpm:hook ports tracked in test/PNPM_TEST_IMPORT.md: hooks.ts:366 (readPackage) and hooks.ts:468 (afterAllResolved). hooks.ts:404 stays parked because it additionally requires --global-pnpmfile, which is a separate feature.

Also mirrors is-positive and is-negative fixtures (referenced pervasively across pnpm's test suite — e.g. hooks.ts:217, :404, plus 5 substitution sites in pnpm_install_misc.bats) so future ports can use the real fixtures verbatim instead of substituting is-odd/is-even.

Implementation

Both pnpmfile shims (the one-shot for afterAllResolved / preResolution and the long-lived host for readPackage) now emit ctx.log(...) calls as sentinel-tagged JSON lines on stderr (__AUBE_HOOK_LOG__ {\"hook\":...,\"message\":...}). A new spawn_stderr_forwarder task reads child stderr line-by-line and:

  • Sentinel-tagged lines: enriched with name=pnpm:hook + project root (prefix) + pnpmfile path (from), then either written to stdout as ndjson (when --reporter=ndjson is active) or to stderr as [pnpmfile] message (default mode).
  • Untagged lines: passed through verbatim, so require()-time stack traces and user console.error output keep working unchanged.

Both spawn paths switch from Stdio::inherit() to Stdio::piped() for stderr. The one-shot path awaits the forwarder before returning so records can't be reordered after the next install step. The ReadPackageHost aborts its forwarder on Drop.

A process-wide pnpmfile::set_ndjson_reporter(bool) is flipped by main when --reporter=ndjson is in effect; all hook entry points (run_after_all_resolved, run_pre_resolution, ReadPackageHost::spawn) gain a prefix: &Path parameter that callers in install/mod.rs, update.rs, and commands/mod.rs wire to cwd.

Tests

Two new ports in test/pnpm_install_hooks.bats, bumping hooks.ts coverage 8/22 → 10/22:

  • pnpmfile: readPackage ctx.log surfaces as pnpm:hook ndjson on stdout — ports hooks.ts:366. Uses @pnpm.e2e/pkg-with-1-dep + @pnpm.e2e/dep-of-pkg-with-1-dep directly (the ^100.0.0 constraint already serves both 100.0.0 and 100.1.0, so the hook's pin to 100.0.0 is a real downgrade without needing addDistTag).
  • pnpmfile: afterAllResolved ctx.log surfaces as pnpm:hook ndjson on stdout — ports hooks.ts:468. Uses pnpm's @pnpm.e2e/pkg-with-1-dep fixture verbatim.

Both assert hook, message, non-empty prefix, non-empty from. The readPackage variant additionally asserts the lockfile reflects the hook's actual mutation (regression guard if the hook silently no-ops).

Fixtures

  • is-positive versions 1.0.0, 2.0.0, 3.0.0, 3.1.0 (28 KB total)
  • is-negative versions 1.0.0, 1.0.1, 2.0.0, 2.0.1, 2.0.2, 2.1.0 (40 KB total)
  • Mirrored via the canonical procedure documented at the top of test/registry/config.yaml: temporarily added the npmjs uplink, fetched packuments + tarballs through Verdaccio, then reverted the uplink. Tarball URLs in the packuments rewritten to http://localhost:4873/... and uplink metadata (_distfiles, _uplinks) cleared to match the existing offline-only fixture format.

The doc convention in PNPM_TEST_IMPORT.md is updated to call these out alongside is-odd/is-even/is-number/semver.

Test plan

  • cargo fmt --check
  • cargo clippy --all-targets -- -D warnings
  • cargo test (1221 tests across the workspace, 0 failures)
  • mise run test:bats test/pnpm_install_hooks.bats — 12/12 (10 active + 2 documented divergence skips)
  • mise run test:bats test/install.bats test/pnpmfile.bats — 70/70 unchanged
  • Smoke test: offline verdaccio --config config.yaml (no uplinks) serves both packuments and tarballs for is-positive and is-negative correctly.
  • Manual smoke: aube install --reporter=ndjson against a fixture pnpmfile with ctx.log calls in both readPackage and afterAllResolved emits well-formed pnpm:hook records on stdout; default mode emits [pnpmfile] message on stderr.

🤖 Generated with Claude Code


Note

Medium Risk
Touches install/update dependency-resolution hook execution and rewires Node child stderr handling; mistakes could reorder/lose hook logs or affect hook process lifecycle across commands.

Overview
Adds structured forwarding of pnpmfile ctx.log output: Node hook shims now write sentinel-tagged JSON to stderr, and a Rust stderr forwarder re-emits these as pnpm:hook ndjson records on stdout when --reporter=ndjson is enabled (otherwise keeps a [pnpmfile] … stderr fallback).

Updates hook spawning APIs (preResolution, afterAllResolved, readPackage) to take a prefix/cwd, pipes hook stderr instead of inheriting it, and drains per-hook forwarder tasks to keep resolve-time logs ordered ahead of afterAllResolved output. Extends tests to assert the new pnpm:hook surface and mirrors is-positive/is-negative fixtures for future ports.

Reviewed by Cursor Bugbot for commit 86563b8. Bugbot is set up for automated code reviews on this repo. Configure here.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

Routes pnpmfile ctx.log(...) calls through a sentinel-tagged stderr channel so they appear as {\"name\":\"pnpm:hook\",...} ndjson records on stdout under --reporter=ndjson and as [pnpmfile] message on stderr otherwise. Both shim types (one-shot and long-lived readPackage host) now pipe stderr through a spawn_stderr_forwarder task, and all spawn APIs gain a prefix: &Path parameter that callers wire to cwd. Forwarder handles are returned separately from hosts and explicitly drained before afterAllResolved to keep resolve-time log records ordered ahead of post-resolve records in the ndjson stream.

Confidence Score: 5/5

Safe to merge; no P0/P1 issues found and the design correctly handles forwarder lifetime and ordering.

All P0/P1 concerns raised in prior review threads have been addressed: the forwarder is no longer stored in or aborted by ReadPackageHost on drop, drain_forwarder surfaces panics via tracing::warn, and the bats grep check is properly wrapped with run + assert_success. The one remaining new observation (silently ignored I/O errors on the stderr read loop) is P2-only.

No files require special attention beyond the minor P2 in spawn_stderr_forwarder.

Important Files Changed

Filename Overview
crates/aube/src/pnpmfile.rs Core change: sentinel-tagged stderr forwarding for ctx.log, new spawn_stderr_forwarder, NDJSON_REPORTER global flag, updated spawn APIs to return forwarder handles alongside hosts. Design is sound; stderr I/O errors are silently swallowed on loop exit.
crates/aube/src/commands/install/mod.rs Wires cwd as prefix into spawn and chain calls, handles new tuple return type, and adds drain_forwarders calls before afterAllResolved in both install branches. drop(resolver) correctly moved before drain_forwarders in the fresh-resolve path.
crates/aube/src/commands/update.rs Mirrors install/mod.rs changes for the update path: spawn returns (host, forwarders), drain_forwarders called before afterAllResolved. Straightforward and correct.
test/pnpm_install_hooks.bats Two new tests for readPackage and afterAllResolved ctx.log ndjson output. grep for lockfile regression is correctly wrapped with run + assert_success.
test/registry/config.yaml Adds steps 4-6 to fixture-mirroring instructions, clarifying how to strip Verdaccio uplink metadata.

Fix All in Claude Code

Reviews (4): Last reviewed commit: "fix(test): clear leftover _uplinks metad..." | Re-trigger Greptile

Comment thread crates/aube/src/pnpmfile.rs Outdated
Comment thread crates/aube/src/pnpmfile.rs
Comment thread test/pnpm_install_hooks.bats
Comment thread crates/aube/src/pnpmfile.rs Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

Benchmark changes

Versions:

  • aube: 1.5.1 -> 1.5.2
  • pnpm: 11.0.2 -> 11.0.3

Public ratios: warm installs vs Bun 4x -> 8x; warm installs vs pnpm 5x -> 12x.

Benchmark aube bun pnpm
Fresh install (warm cache) 1021ms -> 278ms (-73%) 4134ms -> 2190ms (-47%) 4717ms -> 3334ms (-29%)
CI install (warm cache, GVS disabled) 2920ms -> 567ms (-81%) 3396ms -> 2336ms (-31%) 4864ms -> 2805ms (-42%)
CI install (cold cache, GVS disabled) 10801ms -> 4414ms (-59%) 10012ms -> 4651ms (-54%) 9722ms -> 5543ms (-43%)

86563b8 vs a077bcb | aube/bun/pnpm | 3 scenarios | 3 runs | 500mbit/50ms | generated by Codex.

@jdx jdx force-pushed the claude/strange-chandrasekhar-98d62c branch from 1adaf5b to 884b919 Compare April 30, 2026 23:55
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 884b919. Configure here.

Comment thread test/registry/storage/is-positive/package.json Outdated
jdx and others added 2 commits April 30, 2026 19:04
Routes `ctx.log(...)` calls from `readPackage` / `afterAllResolved` /
`preResolution` hooks through a stderr sentinel-tagged channel, which a
new forwarder task re-emits as `{"name":"pnpm:hook",...}` on stdout under
`--reporter=ndjson` (matching pnpm's surface) or `[pnpmfile] message` on
stderr in default mode. Both shim paths switch from `Stdio::inherit` to
`Stdio::piped` for stderr; require()-time errors and user `console.error`
output pass through unchanged. Unblocks the ndjson `pnpm:hook` ports
tracked in PNPM_TEST_IMPORT.md (hooks.ts:366, :468).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pnpm tests reference is-positive and is-negative pervasively (notably
hooks.ts:217, hooks.ts:404, plus 5 misc.ts ports that previously had to
substitute is-odd). Mirrors all published versions of both packages
under test/registry/storage/ so future ports can use the real fixtures
verbatim.

Tightens the afterAllResolved ndjson port to use @pnpm.e2e/pkg-with-1-dep
exactly like hooks.ts:468, removing one substitution from the new port.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jdx jdx force-pushed the claude/strange-chandrasekhar-98d62c branch from 884b919 to 4828654 Compare May 1, 2026 00:08
autofix-ci Bot and others added 2 commits May 1, 2026 00:11
The committed packument carried `_uplinks.npmjs` etag + fetched
timestamp, divergent from every other fixture (which all have
`_uplinks: {}`). The PR description claimed the metadata was cleared,
but a smoke test that queried the packument through Verdaccio after the
jq cleanup re-populated `_uplinks` — Verdaccio writes that metadata back
to storage on every packument fetch, regardless of whether uplinks are
configured.

Re-clears the metadata and expands the mirror procedure docstring in
test/registry/config.yaml so the next person doesn't fall into the same
trap (tarball fetches through Verdaccio are safe, packument fetches are
not).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jdx jdx merged commit 73a6617 into main May 1, 2026
18 checks passed
@jdx jdx deleted the claude/strange-chandrasekhar-98d62c branch May 1, 2026 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant