From bf1937bebf78e9fa7d5eecc6dc6c092142ee61d1 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Fri, 8 May 2026 01:41:35 +0200 Subject: [PATCH] fix: render NDJSON output cleanly through @pnpm/default-reporter (#403) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(cli): canonicalize `--dir` so reporter `prefix` matches subprocess `process.cwd()` `pacquet --reporter=ndjson` piped into `@pnpm/cli.default-reporter` emitted every progress / stats line with a `. | ` path prefix because the bunyan-envelope `prefix` field was the literal `"."` (derived from the manifest's parent of `./package.json`). The reporter compares each event's `prefix` to its own `cwd` (an absolute, canonicalized path from `process.cwd()`), and the mismatch routed every log through `zoomOut`. Canonicalize `--dir` once at the top of `CliArgs::run`, mirroring pnpm's `config/reader/src/index.ts:270` where `cwd = fs.realpathSync(betterPathResolve(cliOptions.dir ...))` becomes `pnpmConfig.dir` and is threaded into every event's `prefix` as `lockfileDir`. Ports the same realpath behavior to pacquet so a default-reporter subprocess and pacquet agree on the project root. Side effect: `pacquet init` now derives the package `name` from the working-directory basename (matching pnpm) instead of leaving it as `""` — the `.` parent had no `file_name`. Snapshot updated. * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(tarball): emit `pnpm:fetching-progress started` with one-indexed `attempt` The default reporter's `reportBigTarballsProgress` filters started events with `log.attempt === 1` so a transient retry doesn't reset the "Downloading : …/" progress line. Pacquet's loop counter is zero-indexed and was being passed straight through, so every started event was emitted with `attempt: 0` and the reporter silently dropped the entire channel — no big-tarball progress lines ever rendered. Pnpm's underlying `node-retry` `op.attempt(cb)` callback hands `cb` a 1-indexed counter, which `packageRequester` forwards verbatim into the log. Match that wire shape by emitting `attempt + 1` at the fetching-progress site, mirroring the same `+ 1` adjustment already present at the neighbouring `pnpm:request-retry` emit. Updates the recording-fake test to assert `[1, 2]` (one-indexed) and fixes a stray `attempt: 0` in the reporter serialization test. * test(cli): add NDJSON `prefix` regression test, refine review nits Addresses three review comments on #403: * Add an integration test (`install_emits_canonical_prefix_in_ndjson_events`) that runs `pacquet --reporter=ndjson install` and asserts every event's `prefix` is the canonicalized workspace path — never `"."`. Pins the user-visible regression: the original failure mode was the default-reporter subprocess prepending ` | ` to every progress / stats line because the emitted `"."` never matched its own `process.cwd()`. * Reword the canonicalize-failure context to reference the `--dir` argument explicitly so a stat / permission failure points the user at the flag they passed. * Restore the `assertion_line` field on `init__should_create_package_json.snap` so its metadata matches the surrounding insta snapshots. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- pacquet/crates/cli/Cargo.toml | 2 +- pacquet/crates/cli/src/cli_args.rs | 15 ++++- pacquet/crates/cli/tests/install.rs | 56 +++++++++++++++++++ .../init__should_create_package_json.snap | 4 +- pacquet/crates/reporter/src/tests.rs | 2 +- pacquet/crates/tarball/src/lib.rs | 11 +++- pacquet/crates/tarball/src/tests.rs | 8 ++- 7 files changed, 89 insertions(+), 9 deletions(-) diff --git a/pacquet/crates/cli/Cargo.toml b/pacquet/crates/cli/Cargo.toml index bc846bf990..c70b16ce59 100644 --- a/pacquet/crates/cli/Cargo.toml +++ b/pacquet/crates/cli/Cargo.toml @@ -29,6 +29,7 @@ pacquet-diagnostics = { workspace = true } clap = { workspace = true } derive_more = { workspace = true } +dunce = { workspace = true } home = { workspace = true } miette = { workspace = true } pipe-trait = { workspace = true } @@ -40,7 +41,6 @@ pacquet-store-dir = { workspace = true } pacquet-testing-utils = { workspace = true } assert_cmd = { workspace = true } -dunce = { workspace = true } command-extra = { workspace = true } insta = { workspace = true } pretty_assertions = { workspace = true } diff --git a/pacquet/crates/cli/src/cli_args.rs b/pacquet/crates/cli/src/cli_args.rs index 266cf44c80..4030119805 100644 --- a/pacquet/crates/cli/src/cli_args.rs +++ b/pacquet/crates/cli/src/cli_args.rs @@ -7,7 +7,7 @@ use crate::State; use add::AddArgs; use clap::{Parser, Subcommand, ValueEnum}; use install::InstallArgs; -use miette::Context; +use miette::{Context, IntoDiagnostic}; use pacquet_executor::execute_shell; use pacquet_npmrc::Npmrc; use pacquet_package_manifest::PackageManifest; @@ -82,6 +82,19 @@ impl CliArgs { /// Execute the command pub async fn run(self) -> miette::Result<()> { let CliArgs { command, dir, reporter } = self; + // Canonicalize `--dir` so the bunyan-envelope `prefix` emitted by + // the reporter is the same absolute, symlink-resolved path that + // `@pnpm/cli.default-reporter` derives via `process.cwd()`. Without + // this, a default `--dir=.` leaves `prefix` as `"."`, the reporter + // never matches it against its `cwd`, and every progress / stats + // line gets a redundant `.` path prefix prepended. Mirrors pnpm's + // + // `cwd = fs.realpathSync(betterPathResolve(cliOptions.dir ...))`, + // later assigned to `pnpmConfig.dir` (used as the install + // `lockfileDir`, threaded into every event's `prefix`). + let dir = dunce::canonicalize(&dir) + .into_diagnostic() + .wrap_err_with(|| format!("canonicalizing the `--dir` argument: {}", dir.display()))?; let manifest_path = || dir.join("package.json"); let npmrc = || Npmrc::current(env::current_dir, home::home_dir, Default::default).leak(); // `require_lockfile` is the "this subcommand cannot run without a diff --git a/pacquet/crates/cli/tests/install.rs b/pacquet/crates/cli/tests/install.rs index 6e221d915e..6822814873 100644 --- a/pacquet/crates/cli/tests/install.rs +++ b/pacquet/crates/cli/tests/install.rs @@ -170,6 +170,62 @@ fn frozen_lockfile_should_be_able_to_handle_big_lockfile() { drop((root, mock_instance)); // cleanup } +/// Regression test for the NDJSON `prefix` field. `--reporter=ndjson` +/// must emit each bunyan envelope with the canonicalized install root +/// — not the relative `"."` that `dir.join("package.json").parent()` +/// produced when `--dir` defaulted to `.`. The downstream consumer +/// (`@pnpm/cli.default-reporter` running in a separate process) compares +/// every event's `prefix` to its own `process.cwd()` and prepends a +/// redundant ` | ` adornment whenever they disagree, so a `"."` +/// prefix made every progress / stats line render with `. | `. +#[test] +fn install_emits_canonical_prefix_in_ndjson_events() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + eprintln!("Creating package.json..."); + let manifest_path = workspace.join("package.json"); + let package_json = serde_json::json!({ + "dependencies": { + "@pnpm.e2e/hello-world-js-bin-parent": "1.0.0", + }, + }); + fs::write(&manifest_path, package_json.to_string()).expect("write to package.json"); + + eprintln!("Executing command with --reporter=ndjson..."); + let output = + pacquet.with_args(["--reporter=ndjson", "install"]).output().expect("run pacquet install"); + assert!( + output.status.success(), + "pacquet install exited non-zero: stderr={}", + String::from_utf8_lossy(&output.stderr), + ); + + eprintln!("Collecting `prefix` values from NDJSON stderr..."); + let stderr = String::from_utf8(output.stderr).expect("stderr is utf-8"); + let prefixes: Vec = stderr + .lines() + .filter_map(|line| serde_json::from_str::(line).ok()) + .filter_map(|val| val.get("prefix").and_then(|p| p.as_str()).map(str::to_owned)) + .collect(); + assert!( + !prefixes.is_empty(), + "expected at least one event with a `prefix` field; stderr was:\n{stderr}", + ); + + let expected = dunce::canonicalize(&workspace).expect("canonicalize workspace"); + let expected = expected.to_str().expect("workspace path is UTF-8"); + for prefix in &prefixes { + assert_eq!( + prefix, expected, + "every event's prefix must be the canonicalized install root, not relative", + ); + } + + drop((root, mock_instance)); // cleanup +} + #[test] fn should_install_circular_dependencies() { let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = diff --git a/pacquet/crates/cli/tests/snapshots/init__should_create_package_json.snap b/pacquet/crates/cli/tests/snapshots/init__should_create_package_json.snap index 619dab6830..32ecf1c3ee 100644 --- a/pacquet/crates/cli/tests/snapshots/init__should_create_package_json.snap +++ b/pacquet/crates/cli/tests/snapshots/init__should_create_package_json.snap @@ -1,10 +1,10 @@ --- source: crates/cli/tests/init.rs -assertion_line: 31 +assertion_line: 20 expression: package_json_content --- { - "name": "", + "name": "workspace", "version": "1.0.0", "description": "", "main": "index.js", diff --git a/pacquet/crates/reporter/src/tests.rs b/pacquet/crates/reporter/src/tests.rs index 7978242a50..96f2939ac9 100644 --- a/pacquet/crates/reporter/src/tests.rs +++ b/pacquet/crates/reporter/src/tests.rs @@ -232,7 +232,7 @@ fn fetching_progress_event_matches_pnpm_wire_shape() { let event = LogEvent::FetchingProgress(FetchingProgressLog { level: LogLevel::Debug, message: FetchingProgressMessage::Started { - attempt: 0, + attempt: 1, package_id: "react@18.0.0".to_string(), size: None, }, diff --git a/pacquet/crates/tarball/src/lib.rs b/pacquet/crates/tarball/src/lib.rs index 9423c485a2..dc15621dfd 100644 --- a/pacquet/crates/tarball/src/lib.rs +++ b/pacquet/crates/tarball/src/lib.rs @@ -838,12 +838,21 @@ async fn fetch_and_extract_once( // reporter checks `size != null` before rendering a percent // gauge, so this admits "we don't know yet" only when we truly // don't know. + // + // `attempt` is one-indexed (the in-flight attempt) to match + // pnpm's wire shape — `node-retry`'s `op.attempt(cb)` callback + // hands `cb` a 1-indexed counter, which `packageRequester` + // forwards verbatim into the `attempt` field. Pacquet's loop + // counter is zero-indexed, so emit `attempt + 1`. The default + // reporter's `reportBigTarballsProgress` filters on + // `log.attempt === 1` (so retries don't reset the progress + // line), so a zero would silence every "Downloading …" line. let send_result = client.get(package_url).send().await; let size = send_result.as_ref().ok().and_then(|r| r.content_length()); R::emit(&LogEvent::FetchingProgress(FetchingProgressLog { level: LogLevel::Debug, message: FetchingProgressMessage::Started { - attempt, + attempt: attempt + 1, package_id: package_id.to_owned(), size, }, diff --git a/pacquet/crates/tarball/src/tests.rs b/pacquet/crates/tarball/src/tests.rs index 1d21ff82da..9d2b06ecab 100644 --- a/pacquet/crates/tarball/src/tests.rs +++ b/pacquet/crates/tarball/src/tests.rs @@ -1548,8 +1548,10 @@ async fn run_with_mem_cache_recovers_from_owning_fetch_error() { /// tarball pipeline: /// /// * `pnpm:fetching-progress started` once per *attempt* — so a 503 + -/// 200 retry pattern emits twice with `attempt = 0` then -/// `attempt = 1`. `size` carries the response's `Content-Length` +/// 200 retry pattern emits twice with `attempt = 1` then +/// `attempt = 2` (one-indexed, matching pnpm's wire shape — the +/// default reporter's `reportBigTarballsProgress` filters on +/// `attempt === 1`). `size` carries the response's `Content-Length` /// (mockito sends one for `with_body`). /// * `pnpm:fetching-progress in_progress` is throttled to ~200ms; the /// tiny FASTIFY tarball used here downloads in well under that, so @@ -1621,7 +1623,7 @@ async fn fetching_progress_and_fetched_events_fire_during_download() { }) .collect(); let attempts: Vec = started.iter().map(|(a, _)| *a).collect(); - assert_eq!(attempts, vec![0, 1], "started must fire once per attempt; got {captured:?}"); + assert_eq!(attempts, vec![1, 2], "started must fire once per attempt; got {captured:?}"); // Both attempts have a response head (mockito sends Content-Length // for `with_body(...)` and `with_status(503)` likewise), so both // `started` events must carry a populated `size`. Pinning this