feat(reporter): add NDJSON reporting engine with pnpm:stage smoke test#349
Conversation
Introduces `pacquet-reporter` carrying the typed `LogEvent` enum, the no-`self` `Reporter` capability trait per #339, and `NdjsonReporter` / `SilentReporter` implementations. Adds the `--reporter=ndjson|silent` flag and threads a `R: Reporter` generic through `Install::run`, emitting `pnpm:stage` `importing_started` / `importing_done` to bracket the install — one channel wired end-to-end as a smoke test for the schema-mirror approach. The wire format mirrors what `@pnpm/core-loggers` defines and `@pnpm/cli.default-reporter` consumes under `pnpm --reporter=ndjson`. Engine only. The JS-reporter spawn-and-pipe (`--reporter=default`), the backfill of missing log events in already-ported code (#347), and the porting convention in AGENTS.md (#346) land in follow-up PRs. The default reporter is `silent` until the spawn-and-pipe exists, so user- facing output is unchanged. Refs #345. Design: #344.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
- Coverage 88.65% 88.55% -0.10%
==========================================
Files 64 65 +1
Lines 5684 5769 +85
==========================================
+ Hits 5039 5109 +70
- Misses 645 660 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.29603445308,
"stddev": 0.06233388740641967,
"median": 2.29967804308,
"user": 2.4927929799999995,
"system": 1.97689898,
"min": 2.18590673758,
"max": 2.4295298135800003,
"times": [
2.4295298135800003,
2.32036126558,
2.28731933358,
2.25880053258,
2.31847206758,
2.26869073758,
2.31585037758,
2.18590673758,
2.31203675258,
2.26337691258
]
},
{
"command": "pacquet@main",
"mean": 2.27749333378,
"stddev": 0.1078266402374849,
"median": 2.24978301408,
"user": 2.55741168,
"system": 1.9479360800000003,
"min": 2.1689671285800003,
"max": 2.54618575758,
"times": [
2.1689671285800003,
2.3348093805800003,
2.25077008958,
2.2487959385800003,
2.20785465058,
2.54618575758,
2.20452492158,
2.30084323658,
2.21022370658,
2.30195852758
]
},
{
"command": "pnpm",
"mean": 5.9165698550800006,
"stddev": 0.07164660677750598,
"median": 5.900804170080001,
"user": 8.773773979999998,
"system": 2.72057228,
"min": 5.84530031558,
"max": 6.0642847165800005,
"times": [
5.85894051858,
5.84833396458,
5.8975148095800005,
5.86414101758,
6.0642847165800005,
5.90409353058,
5.98694409358,
5.97171472158,
5.84530031558,
5.92443086258
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.51339679078,
"stddev": 0.03541172614588264,
"median": 0.5008452249800001,
"user": 0.23764143999999998,
"system": 0.80654106,
"min": 0.48871423948000003,
"max": 0.60782256848,
"times": [
0.60782256848,
0.5041269754800001,
0.49869113248,
0.49771638448,
0.49575915148000005,
0.53554474148,
0.5025108514800001,
0.49917959848000004,
0.48871423948000003,
0.50390226448
]
},
{
"command": "pacquet@main",
"mean": 0.52657416258,
"stddev": 0.02834828677672562,
"median": 0.52444824398,
"user": 0.23584084,
"system": 0.8179369599999999,
"min": 0.49709987248000004,
"max": 0.58697403748,
"times": [
0.58697403748,
0.54788000748,
0.50533041248,
0.54796843248,
0.49959918348,
0.50199172648,
0.52426419648,
0.52463229148,
0.49709987248000004,
0.53000146548
]
},
{
"command": "pnpm",
"mean": 2.19053259558,
"stddev": 0.09646625966654238,
"median": 2.15923307098,
"user": 2.95803084,
"system": 1.3527048600000002,
"min": 2.09540653548,
"max": 2.40355441448,
"times": [
2.40355441448,
2.16945350448,
2.26134889048,
2.12847962448,
2.14901263748,
2.09540653548,
2.12134443348,
2.27149163348,
2.10748341848,
2.19775086348
]
}
]
} |
…ceDir Upstream emits stage events with `prefix: lockfileDir` — the directory holding `pnpm-lock.yaml`, which collapses to the workspace root when a workspace is present. The previous comment described the *result* in pacquet's no-workspace world but didn't name the upstream concept or point at the helper to mirror once workspaces land. Replacing the comment with a TODO that links `findWorkspaceDir` makes the migration path explicit so the next person editing this site doesn't have to re-derive it. No behavior change.
Adding `global = true` to the clap attribute lets users write either
`pacquet --reporter=ndjson install` or `pacquet install --reporter=ndjson`,
matching pnpm. Without it the flag is parsed by the top-level `CliArgs`
only, so a sub-command-side use ("pacquet install --reporter=...")
produced "unexpected argument '--reporter' found".
KSXGitHub
left a comment
There was a problem hiding this comment.
These are my suggestions.
@zkochan If you agree with them, let your Claude Code handle them.
If you don't agree, let me know by mentioning @KSXGitHub.
As for Claude Code, don't mention me by @KSXGitHub. I don't want unneeded annoyance.
Address feedback on the reporter PR. reporter: - Drop the dependency-injection issue link (#339) from `Reporter`, the recording-fake test, and the module-level docs; convention is visible from the code. - Wrap `bunyan` and `bole` references as markdown links. - Demote `Envelope`'s `#[serde(flatten)]` rationale to a regular `//` comment so private impl detail isn't part of `///` rustdoc. - Introduce `pub trait EnvVar` + `pub struct RealEnvVar` (reporter- local capability trait) and split `hostname` into a generic `resolve_hostname<E>` plus a non-generic cached production wrapper. Add four unit tests covering `HOSTNAME`, `COMPUTERNAME` fallback, precedence, and the unset case. - Switch the wire-shape test to `pipe-trait`; add `pipe-trait` to dev-dependencies. - Fix intra-link rule violation in the `SilentReporter` test doc. package-manager: - Tighten the install `prefix` extraction to fail loudly on non-UTF-8 / parentless manifest paths instead of silently emitting an empty string. Workspace-aware resolution tracked at #357. - Replace `concat!` YAML literals in two tests with `text_block_macros::text_block!`; add as dev-dependency. - Convert bare-prose identifiers in `install_emits_stage_events_bracketing_the_run` to intra-doc links; drop the recording-fake DI prose. Long inline `mod tests` extraction tracked at #358.
`EnvVar` is `pub` but `resolve_hostname` is private; rustdoc rejects the intra-link with `-D rustdoc::private-intra-doc-links` (the rule added to `CODE_STYLE_GUIDE.md` in #351). Rephrase the doc to talk about "env-dependent helpers" generally without naming the private function.
KSXGitHub
left a comment
There was a problem hiding this comment.
Just remembered a detail before sleep.
So quick review.
More will come later.
KSXGitHub
left a comment
There was a problem hiding this comment.
Some suggestions.
Also found a bug.
Claude shall examine and assess the suggestions and reviews, and then take appropriate actions.
…ility Address the second round of review feedback on #349. reporter: - Replace env-var-based hostname (HOSTNAME / COMPUTERNAME) with the real gethostname(2) syscall via the `gethostname` crate. The env-var approach was unsound: shells set `HOST` (zsh) or `HOSTNAME` (bash) as private variables that don't propagate to spawned binaries, so a packaged `pacquet` would have observed an empty hostname in most production environments. - Introduce `pub trait GetHostName` + `pub struct RealApi`. Real resolution lives in `RealApi::get_host_name`. Tests can supply their own impl. - Cache the production value in `static HOSTNAME: LazyLock<String> = LazyLock::new(RealApi::get_host_name)`. The wire emit path reads `&HOSTNAME` directly; the previous `hostname()` wrapper is no longer needed. - Drop the now-redundant `EnvVar` trait, `RealEnvVar`, `resolve_hostname`, and the four env-resolution unit tests (those scenarios no longer exist after the syscall switch). reporter test layout: - Move the tests module into its own file at `crates/reporter/src/tests.rs` per the unit-test-layout rule in `CODE_STYLE_GUIDE.md`. Drop `use super::*` in favor of explicit merged imports of just the items the tests touch. - Two new tests: `get_host_name_capability_is_mockable` (proves the trait dispatch works for fakes) and `real_api_returns_a_non_empty_host_name` (smoke-checks the real syscall path). Documentation style: - Restructure the workspace-prefix and stage-bracketing comments to drop em dashes per `CONTRIBUTING.md`'s writing-style guidance and to render `bunyan` as a markdown link. - Fix the `@pnpm/core-loggers` link to point at the package directory rather than its `src` subdirectory. Imports: - Replace seven occurrences of `pacquet_reporter::SilentReporter` qualified at the call site with a single `use pacquet_reporter:: SilentReporter` in the test module.
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant CLI as "CLI (--reporter)"
participant Cli as "CliArgs::run"
participant PM as "PackageManager (Add/Install)"
participant Rep as "Reporter (Ndjson/Silent)"
participant Stderr as "stderr (NDJSON)"
CLI->>Cli: choose reporter
Cli->>PM: call .run::<R>() with chosen reporter
PM->>Rep: emit(Stage::ImportingStarted)
alt NdjsonReporter
Rep->>Stderr: write NDJSON line
else SilentReporter
Rep->>Rep: drop event
end
PM->>PM: perform work
PM->>Rep: emit(Stage::ImportingDone)
alt NdjsonReporter
Rep->>Stderr: write NDJSON line
else SilentReporter
Rep->>Rep: drop event
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 8/10 reviews remaining, refill in 7 minutes and 21 seconds. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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/package-manager/src/add.rs (1)
48-89:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAvoid process-aborting panic in add flow.
In this updated
runpath, Line 68 still uses.expect("resolve latest tag"). A transient registry/network failure will panic instead of returningAddError, which is especially risky with releasepanic = "abort". Please propagate this failure via a dedicatedAddErrorvariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/package-manager/src/add.rs` around lines 48 - 89, The code currently calls PackageVersion::fetch_from_registry(...).await.expect("resolve latest tag") in Add::run, which will abort on transient registry/network failures; replace the expect with proper error propagation by adding a new AddError variant (e.g., FetchLatestVersion or ResolveLatestVersionError) and convert the fetch error into that variant (using ? after mapping the fetch error to AddError via map_err or .await.map_err(AddError::FetchLatestVersion)?), so PackageVersion::fetch_from_registry is awaited without expect and failures return Err(AddError::...) from Add::run.
🧹 Nitpick comments (1)
crates/package-manager/src/install.rs (1)
557-627: ⚡ Quick winDocumented early-error stage behavior is not asserted yet.
This test’s doc explains both success and early-error semantics, but the body verifies only the success case. Add a second scenario that forces
InstallError::NoLockfileand assert onlyStage::ImportingStartedis captured.Proposed test extension
#[tokio::test] async fn install_emits_stage_events_bracketing_the_run() { static EVENTS: Mutex<Vec<LogEvent>> = Mutex::new(Vec::new()); @@ assert_eq!(stages, [Stage::ImportingStarted, Stage::ImportingDone]); + + // Early error path: frozen lockfile requested but absent. + EVENTS.lock().unwrap().clear(); + let result = Install { + tarball_mem_cache: &Default::default(), + http_client: &Default::default(), + config, + manifest: &manifest, + lockfile: None, + dependency_groups: [DependencyGroup::Prod], + frozen_lockfile: true, + resolved_packages: &Default::default(), + } + .run::<RecordingReporter>() + .await; + assert!(matches!(result, Err(InstallError::NoLockfile))); + + let captured = EVENTS.lock().unwrap(); + let stages: Vec<Stage> = captured + .iter() + .map(|e| match e { + LogEvent::Stage(StageLog { stage, .. }) => *stage, + }) + .collect(); + assert_eq!(stages, [Stage::ImportingStarted]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/package-manager/src/install.rs` around lines 557 - 627, Add a second scenario after the successful run that constructs an Install with lockfile: None (to trigger InstallError::NoLockfile), calls Install::run::<RecordingReporter>().await and asserts it returns the NoLockfile error, then inspects the static EVENTS (used by RecordingReporter) and asserts the captured stages equal only [Stage::ImportingStarted]; reference the RecordingReporter/ EVENTS static, Install::run, and InstallError::NoLockfile to locate where to add the failing-case assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/package-manager/src/add.rs`:
- Around line 48-89: The code currently calls
PackageVersion::fetch_from_registry(...).await.expect("resolve latest tag") in
Add::run, which will abort on transient registry/network failures; replace the
expect with proper error propagation by adding a new AddError variant (e.g.,
FetchLatestVersion or ResolveLatestVersionError) and convert the fetch error
into that variant (using ? after mapping the fetch error to AddError via map_err
or .await.map_err(AddError::FetchLatestVersion)?), so
PackageVersion::fetch_from_registry is awaited without expect and failures
return Err(AddError::...) from Add::run.
---
Nitpick comments:
In `@crates/package-manager/src/install.rs`:
- Around line 557-627: Add a second scenario after the successful run that
constructs an Install with lockfile: None (to trigger InstallError::NoLockfile),
calls Install::run::<RecordingReporter>().await and asserts it returns the
NoLockfile error, then inspects the static EVENTS (used by RecordingReporter)
and asserts the captured stages equal only [Stage::ImportingStarted]; reference
the RecordingReporter/ EVENTS static, Install::run, and InstallError::NoLockfile
to locate where to add the failing-case assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cb9b33d5-7e6d-4fdd-8561-bd928f28c1c0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlcrates/cli/Cargo.tomlcrates/cli/src/cli_args.rscrates/cli/src/cli_args/add.rscrates/cli/src/cli_args/install.rscrates/package-manager/Cargo.tomlcrates/package-manager/src/add.rscrates/package-manager/src/install.rscrates/reporter/Cargo.tomlcrates/reporter/src/lib.rscrates/reporter/src/tests.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/reporter/src/tests.rs`:
- Line 121: Remove the noisy debug print that writes the hostname to stderr in
the test: delete the eprintln!("RealApi::get_host_name() = {host:?}"); statement
in crates/reporter/src/tests.rs (the call that prints the result of
RealApi::get_host_name()); if you want to keep the info for local debugging,
replace it with a non-leaking logger call (e.g., trace/debug) or wrap it behind
a cfg(debug_assertions) guard so CI no longer emits runner metadata.
🪄 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: 41dfebb6-f57e-49ee-99f7-1fe7f8f1b863
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlcrates/cli/Cargo.tomlcrates/cli/src/cli_args.rscrates/cli/src/cli_args/add.rscrates/cli/src/cli_args/install.rscrates/package-manager/Cargo.tomlcrates/package-manager/src/add.rscrates/package-manager/src/install.rscrates/reporter/Cargo.tomlcrates/reporter/src/lib.rscrates/reporter/src/tests.rs
PR #364's narrowed test imports in `crates/package-manager/src/install.rs` left out the symbols that the stage-event smoke test from PR #349 references (`LogEvent`, `Reporter`, `Stage`, `StageLog`, plus `Lockfile`). The test module compiles cleanly with `--tests` enabled once the imports are restored — without this, `cargo check --tests` fails on the merged main even before this branch's changes are applied. Found while applying PR #364's "No star imports" rule to my own test files; surfacing the fix here instead of leaving main red.
…action The unit-test module in `crates/package-manager/src/install.rs` was left referencing five symbols not in scope after PRs #349 and #364 landed on main in succession. #349 added a test that uses `Lockfile`, `Reporter`, `LogEvent`, `StageLog`, and `Stage`; #364 (no star imports) replaced the previous `use super::*;` with `use super::{Install, InstallError};` and didn't carry over the additional symbols the new test needed. The result is `cargo test -p pacquet-package-manager --lib` failing to compile on `main` itself across all three CI platforms. Restore the missing items by name in the test imports so the no-star-imports rule still holds: use pacquet_lockfile::Lockfile; use pacquet_reporter::{LogEvent, Reporter, SilentReporter, Stage, StageLog}; Verified pre-existing on `origin/main` (`git switch --detach origin/main && cargo build --tests -p pacquet-package-manager` produces the same nine errors). Per AGENTS.md ("If a test was already broken on main, fix it as part of your work"), fixing here.
The no-star-imports sweep in #364 dropped symbols the reporter smoke test in #349 needs. `crates/package-manager/src/install.rs::tests` references `Stage`, `LogEvent`, `Reporter`, `StageLog`, and `Lockfile` in its body, but the explicit-import list only kept `Install` and `InstallError`. Restore the missing names so the crate compiles. Confirmed pre-existing on `origin/main` (`cargo check --tests -p pacquet-package-manager` reproduces). Same drive-by fix is in flight as part of #332. Picking it up here so this PR's CI is not blocked by the upstream regression.
Summary
Lands the reporting engine from #344's design as the first step toward
#345. Establishes:
LogEventenum mirroring@pnpm/core-loggers, starting withthe
pnpm:stagechannel.Reportercapability trait per the DI pattern in Refactor and document the dependency injection pattern for tests #339 — no&self,associated function, generic threading at call sites, unit-struct
implementations.
NdjsonReporter(writes pnpm-wire-format records to stderr, one perline) and
SilentReporter(no-op).--reporter=ndjson|silentCLI flag dispatched at the entry point viaturbofish into the install pipeline.
Install::runemitspnpm:stageimporting_started/importing_donebracketing the install, validated by a recording-faketest using the per-test-
static-mutex pattern from Refactor and document the dependency injection pattern for tests #339.The default reporter is
silentuntil the spawn-and-pipe wiring exists,so user-facing output is unchanged on this PR.
What's deferred
These were in the original #345 scope but split into follow-ups so this PR
stays reviewable:
--reporter=defaultand the JS-reporter spawn-and-pipe. Picking thedelivery vehicle (system Node + bundled wrapper script vs. an
npxbridge package vs. waiting for the Integration Milestone N-API path) is
its own discussion and shouldn't be tangled with the engine PR.
there are multiple concurrent emit points and a high-volume channel
(
pnpm:fetching-progress). Today there's one stage emit at start/endof install —
stderr().lock()is the right level of complexity.Arc<str>). Earns its complexity once channelsfire per-package; not yet.
which pnpm version pacquet targets and whether the check runs against a
TS schema, a JSON Schema, or a Rust-side round-trip.
LogEventvariants. Added under chore(reporter): backfill missing log events in already-ported code #347 as each crate'semissions are wired.
Tests
cargo nextest run -p pacquet-reporter: 4 tests, all passing.stage_event_matches_pnpm_wire_shape— JSON shape assertion againstname,stage,level,prefix,time,hostname,pid.stage_phases_serialize_in_pnpm_form— snake_case phase names matchpnpm's.
silent_reporter_drops_events— no panic, no I/O.recording_fake_captures_emitted_events— exercises the Refactor and document the dependency injection pattern for tests #339per-test-
static-mutex DI pattern.cargo nextest run -p pacquet-package-manager install_emits_stage_events_bracketing_the_run: integration test usingan empty v9 lockfile fixture (cheapest "real" install path), asserting
exactly
[ImportingStarted, ImportingDone].just readyis green: 253 tests pass, 2 skipped, lint clean.Test plan
cargo nextest run -p pacquet-reportercargo nextest run -p pacquet-package-managerjust readypacquet --helplists--reporterwithndjson/silentvaluesand
silentas default.References
engine slice; spawn-and-pipe + extra channels follow.
selfcapability + per-teststaticmutex DI).
@pnpm/core-loggers(pinned): https://github.com/pnpm/pnpm/tree/3b12eb27de/core/core-loggers/srcstageLogger: https://github.com/pnpm/pnpm/blob/3b12eb27de/core/core-loggers/src/stageLogger.tsSummary by CodeRabbit
New Features
Chores
Tests