Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

feat(reporter): add NDJSON reporting engine with pnpm:stage smoke test#349

Merged
zkochan merged 6 commits into
mainfrom
feat/reporter-engine
May 1, 2026
Merged

feat(reporter): add NDJSON reporting engine with pnpm:stage smoke test#349
zkochan merged 6 commits into
mainfrom
feat/reporter-engine

Conversation

@zkochan

@zkochan zkochan commented Apr 30, 2026

Copy link
Copy Markdown
Member

Summary

Lands the reporting engine from #344's design as the first step toward
#345. Establishes:

  • A typed LogEvent enum mirroring @pnpm/core-loggers, starting with
    the pnpm:stage channel.
  • The Reporter capability 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 per
    line) and SilentReporter (no-op).
  • A --reporter=ndjson|silent CLI flag dispatched at the entry point via
    turbofish into the install pipeline.
  • Smoke test: Install::run emits pnpm:stage importing_started /
    importing_done bracketing the install, validated by a recording-fake
    test using the per-test-static-mutex pattern from Refactor and document the dependency injection pattern for tests #339.

The default reporter is silent until 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=default and the JS-reporter spawn-and-pipe. Picking the
    delivery vehicle (system Node + bundled wrapper script vs. an npx
    bridge package vs. waiting for the Integration Milestone N-API path) is
    its own discussion and shouldn't be tangled with the engine PR.
  • MPSC writer task and emit-side throttle. Earn their complexity once
    there are multiple concurrent emit points and a high-volume channel
    (pnpm:fetching-progress). Today there's one stage emit at start/end
    of install — stderr().lock() is the right level of complexity.
  • Identifier interning (Arc<str>). Earns its complexity once channels
    fire per-package; not yet.
  • Schema-version pinning + CI check. Needs a separate decision on
    which pnpm version pacquet targets and whether the check runs against a
    TS schema, a JSON Schema, or a Rust-side round-trip.
  • All the other LogEvent variants. Added under chore(reporter): backfill missing log events in already-ported code #347 as each crate's
    emissions are wired.

Tests

  • cargo nextest run -p pacquet-reporter: 4 tests, all passing.
    • stage_event_matches_pnpm_wire_shape — JSON shape assertion against
      name, stage, level, prefix, time, hostname, pid.
    • stage_phases_serialize_in_pnpm_form — snake_case phase names match
      pnpm'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 #339
      per-test-static-mutex DI pattern.
  • cargo nextest run -p pacquet-package-manager install_emits_stage_events_bracketing_the_run: integration test using
    an empty v9 lockfile fixture (cheapest "real" install path), asserting
    exactly [ImportingStarted, ImportingDone].
  • just ready is green: 253 tests pass, 2 skipped, lint clean.

Test plan

  • cargo nextest run -p pacquet-reporter
  • cargo nextest run -p pacquet-package-manager
  • just ready
  • pacquet --help lists --reporter with ndjson / silent values
    and silent as default.

References

Summary by CodeRabbit

  • New Features

    • Added a global --reporter flag (ndjson, silent); defaults to silent.
    • Introduced NDJSON structured event logging (stage start/done events) for install/add operations to enable machine-readable progress output.
  • Chores

    • Added a packaged reporter component available to the workspace.
  • Tests

    • Added tests validating reporter output, event ordering, and hostname handling.

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

codecov Bot commented Apr 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.35294% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.55%. Comparing base (414e72d) to head (13936ad).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
crates/reporter/src/lib.rs 20.00% 16 Missing ⚠️
crates/cli/src/cli_args.rs 75.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Apr 30, 2026

Copy link
Copy Markdown

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00     16.2±0.42ms   267.9 KB/sec    1.01     16.4±0.60ms   264.5 KB/sec

@github-actions

github-actions Bot commented Apr 30, 2026

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.296 ± 0.062 2.186 2.430 1.01 ± 0.06
pacquet@main 2.277 ± 0.108 2.169 2.546 1.00
pnpm 5.917 ± 0.072 5.845 6.064 2.60 ± 0.13
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)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 513.4 ± 35.4 488.7 607.8 1.00
pacquet@main 526.6 ± 28.3 497.1 587.0 1.03 ± 0.09
pnpm 2190.5 ± 96.5 2095.4 2403.6 4.27 ± 0.35
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
      ]
    }
  ]
}

zkochan added 2 commits April 30, 2026 20:11
…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 KSXGitHub left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread crates/package-manager/src/install.rs Outdated
Comment thread crates/package-manager/src/install.rs Outdated
Comment thread crates/package-manager/src/install.rs Outdated
Comment thread crates/package-manager/src/install.rs
Comment thread crates/package-manager/src/install.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
zkochan added 2 commits April 30, 2026 22:21
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.
@zkochan zkochan requested a review from KSXGitHub April 30, 2026 20:54
@zkochan zkochan enabled auto-merge (squash) April 30, 2026 20:54

@KSXGitHub KSXGitHub left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just remembered a detail before sleep.

So quick review.

More will come later.

Comment thread crates/reporter/src/lib.rs Outdated

@KSXGitHub KSXGitHub left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some suggestions.

Also found a bug.

Claude shall examine and assess the suggestions and reviews, and then take appropriate actions.

Comment thread crates/package-manager/src/install.rs Outdated
Comment thread crates/package-manager/src/install.rs Outdated
Comment thread crates/package-manager/src/install.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/reporter/src/lib.rs Outdated
…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.
@coderabbitai

coderabbitai Bot commented May 1, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds a new pacquet-reporter crate with structured logging (NDJSON and silent reporters), a global --reporter CLI option, and threads reporter generics through Add and Install so chosen reporter is used during package-manager operations.

Changes

Cohort / File(s) Summary
Workspace & Root Manifests
Cargo.toml
Added workspace dependencies: pacquet-reporter (path crates/reporter) and gethostname for shared use.
CLI
crates/cli/Cargo.toml, crates/cli/src/cli_args.rs, crates/cli/src/cli_args/add.rs, crates/cli/src/cli_args/install.rs
Added --reporter global flag and ReporterType enum; CliArgs::run dispatches subcommands with concrete reporter types; AddArgs::run and InstallArgs::run made generic over R: Reporter.
Package Manager
crates/package-manager/Cargo.toml, crates/package-manager/src/add.rs, crates/package-manager/src/install.rs
Added pacquet-reporter dep; Add::run and Install::run made generic over Reporter; Install::run emits Stage start/done events and tests updated to pass explicit reporter.
Reporter Crate
crates/reporter/Cargo.toml, crates/reporter/src/lib.rs, crates/reporter/src/tests.rs
New pacquet-reporter crate implementing Reporter trait, LogEvent/StageLog/Stage/LogLevel types, SilentReporter and NdjsonReporter (writes NDJSON to stderr), hostname capability and tests including a RecordingReporter test fixture.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Poem

🐰 I hop through logs both quiet and loud,
I stamp each stage and make the team proud,
Ndjson lines or silence kept,
Start and done — the events are swept,
Tiny hops, big builds — hooray! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main addition: a NDJSON reporting engine integrated with pnpm:stage logging, which is the primary feature introduced across the reporter crate and CLI integration.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/reporter-engine

Review rate limit: 8/10 reviews remaining, refill in 7 minutes and 21 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@zkochan

zkochan commented May 1, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 1, 2026

Copy link
Copy Markdown
✅ Actions performed

Full review triggered.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 win

Avoid process-aborting panic in add flow.

In this updated run path, Line 68 still uses .expect("resolve latest tag"). A transient registry/network failure will panic instead of returning AddError, which is especially risky with release panic = "abort". Please propagate this failure via a dedicated AddError variant.

🤖 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 win

Documented 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::NoLockfile and assert only Stage::ImportingStarted is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8911479 and 13936ad.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (11)
  • Cargo.toml
  • crates/cli/Cargo.toml
  • crates/cli/src/cli_args.rs
  • crates/cli/src/cli_args/add.rs
  • crates/cli/src/cli_args/install.rs
  • crates/package-manager/Cargo.toml
  • crates/package-manager/src/add.rs
  • crates/package-manager/src/install.rs
  • crates/reporter/Cargo.toml
  • crates/reporter/src/lib.rs
  • crates/reporter/src/tests.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8911479 and 13936ad.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (11)
  • Cargo.toml
  • crates/cli/Cargo.toml
  • crates/cli/src/cli_args.rs
  • crates/cli/src/cli_args/add.rs
  • crates/cli/src/cli_args/install.rs
  • crates/package-manager/Cargo.toml
  • crates/package-manager/src/add.rs
  • crates/package-manager/src/install.rs
  • crates/reporter/Cargo.toml
  • crates/reporter/src/lib.rs
  • crates/reporter/src/tests.rs

Comment thread crates/reporter/src/tests.rs
@zkochan zkochan requested a review from KSXGitHub May 1, 2026 10:08
@zkochan zkochan disabled auto-merge May 1, 2026 19:10
@zkochan zkochan merged commit 76c6ffa into main May 1, 2026
13 of 15 checks passed
@zkochan zkochan deleted the feat/reporter-engine branch May 1, 2026 19:11
KSXGitHub pushed a commit that referenced this pull request May 1, 2026
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.
KSXGitHub pushed a commit that referenced this pull request May 1, 2026
…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.
@KSXGitHub KSXGitHub mentioned this pull request May 1, 2026
3 tasks
KSXGitHub pushed a commit that referenced this pull request May 1, 2026
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants