feat(reporter): emit pnpm:package-import-method from link_file#356
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #356 +/- ##
==========================================
+ Coverage 88.56% 88.60% +0.03%
==========================================
Files 65 65
Lines 5790 5880 +90
==========================================
+ Hits 5128 5210 +82
- Misses 662 670 +8 ☔ 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.2793731705200004,
"stddev": 0.06791551203224801,
"median": 2.2658823357199998,
"user": 2.5359107599999993,
"system": 1.9338418599999998,
"min": 2.17553298072,
"max": 2.41961104972,
"times": [
2.3399836377199996,
2.26373924972,
2.25256765572,
2.32405312172,
2.26802542172,
2.25634910572,
2.17553298072,
2.21806486972,
2.41961104972,
2.27580461272
]
},
{
"command": "pacquet@main",
"mean": 2.27946231812,
"stddev": 0.08297649693343234,
"median": 2.25152686622,
"user": 2.5136359599999993,
"system": 1.93186496,
"min": 2.19919995972,
"max": 2.4780837507199998,
"times": [
2.4780837507199998,
2.22530060172,
2.27936116372,
2.22857266072,
2.3308405897199997,
2.27448107172,
2.22716095772,
2.32642484572,
2.19919995972,
2.2251975797199997
]
},
{
"command": "pnpm",
"mean": 5.74723036902,
"stddev": 0.05793691768355945,
"median": 5.73273784572,
"user": 8.522976860000002,
"system": 2.69603206,
"min": 5.66620915472,
"max": 5.86706235572,
"times": [
5.81841335372,
5.72994339072,
5.72834463672,
5.73645024372,
5.73553230072,
5.70731157272,
5.71278084672,
5.7702558347199995,
5.66620915472,
5.86706235572
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.5043387294,
"stddev": 0.05166144210164993,
"median": 0.4854546462,
"user": 0.22435207999999998,
"system": 0.7919618799999999,
"min": 0.47665835170000004,
"max": 0.6480937747,
"times": [
0.6480937747,
0.4936173347,
0.4831139097,
0.48709789470000003,
0.47665835170000004,
0.4833132947,
0.49830977470000004,
0.4768029677,
0.5125685937,
0.48381139770000003
]
},
{
"command": "pacquet@main",
"mean": 0.48473150150000005,
"stddev": 0.02062072144696403,
"median": 0.47699975670000005,
"user": 0.21734768,
"system": 0.8030786799999999,
"min": 0.46954739370000004,
"max": 0.5294188347,
"times": [
0.5294188347,
0.47407260970000004,
0.47858357570000004,
0.47314244870000005,
0.4717800507,
0.46954739370000004,
0.4803084937,
0.5164620946999999,
0.4770445447,
0.47695496870000004
]
},
{
"command": "pnpm",
"mean": 2.2610286374,
"stddev": 0.16086871420875984,
"median": 2.2942697082,
"user": 2.83541538,
"system": 1.28795348,
"min": 2.0495550757,
"max": 2.5631599117,
"times": [
2.3352457187,
2.3098706976999996,
2.4140748946999997,
2.2919644497,
2.1373150486999997,
2.0974058266999998,
2.5631599117,
2.2965749666999997,
2.1151197837,
2.0495550757
]
}
]
} |
bbd6d3e to
682bc70
Compare
ae92c5b to
fe3ca8b
Compare
|
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:
📝 WalkthroughWalkthroughThreads a generic reporter type Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Install::run::<R>
participant VirtualStore as CreateVirtualStore::run::<R>
participant DirSnapshot as CreateVirtualDirBySnapshot::run::<R>
participant CAS as create_cas_files::<R>
participant Linker as link_file::<R>
participant Reporter as Reporter
Installer->>VirtualStore: start run (with &AtomicU8 logged_methods)
VirtualStore->>DirSnapshot: create dir for snapshot (forward logged_methods)
DirSnapshot->>CAS: create_cas_files::<R>(logged_methods,...)
CAS->>Linker: parallel link_file::<R>(logged_methods,...)
Linker->>Reporter: log_method_once<R>(logged_methods, flag, WireImportMethod)
Reporter-->>Linker: acknowledge event emitted
Linker-->>CAS: link result
CAS-->>DirSnapshot: CAS creation complete
DirSnapshot-->>VirtualStore: dir created
VirtualStore-->>Installer: run complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Comment |
682bc70 to
baaeb0b
Compare
fe3ca8b to
713fce8
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new pnpm-compatible reporter channel (pnpm:package-import-method) so installs can emit the configured/selected package import strategy near install start, matching pnpm’s NDJSON wire shape and event ordering expectations.
Changes:
- Extend
pacquet_reporter::LogEventwithPackageImportMethodplus a wire-format enum (clone|hardlink|copy). - Emit
pnpm:package-import-methodfromInstall::runimmediately afterpnpm:context. - Add/extend tests to validate JSON wire shape and to assert the install event sequence includes the new event.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/reporter/src/lib.rs | Adds the new pnpm:package-import-method event variant and wire-format enum/types for serialization. |
| crates/reporter/src/tests.rs | Adds a serialization test for the new event and checks enum-to-wire string mapping. |
| crates/package-manager/src/install.rs | Emits the new event at install start and updates integration test expectations/ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds the `PackageImportMethod` variant to `LogEvent` and emits it from `Install::run` after `pnpm:context`, before `pnpm:stage importing_started`. Mirrors pnpm's emit at https://github.com/pnpm/pnpm/blob/086c5e91e8/fs/indexed-pkg-importer/src/index.ts#L32. The wire-format enum has only `clone | hardlink | copy`; pacquet's `Npmrc::package_import_method` adds `Auto` and `CloneOrCopy`. Both collapse to `clone` because that's the optimistic path the upstream auto-importer attempts first. For the explicit `Hardlink` / `Copy` / `Clone` settings the wire value matches reality. A TODO at the emit site notes that the value should be re-derived after the first successful import once the fallback signal is surfaced past the per-package import path. While auditing this channel, also deferred `pnpm:stage resolution_started/done`: pacquet's no-lockfile flow interleaves resolution and import per dependency, so emitting `resolution_done` after import work has already happened would break the JS reporter's per-package state. Pick this back up once pacquet's no-lockfile flow gains a separate resolution pass. Frozen-lockfile installs (pacquet's primary path today) don't need these events anyway — pnpm itself skips them in `deps-restorer`. The deferral is recorded under #347. Tests: - New `package_import_method_event_matches_pnpm_wire_shape` in the reporter crate asserts every wire value renders correctly. - The install integration test grows to assert the five-event success sequence (Context, PackageImportMethod, ImportingStarted, ImportingDone, Summary) and spot-checks the wire mapping for the default `Auto` config (collapses to `Clone`). - Verified the test catches the regression by temporarily commenting out the new emit and confirming the failure shape, then restored. Refs #347. Engine: #345. Previous channel: #355.
Previously the channel emitted the *configured* import method optimistically at install start, with a TODO to defer the emit until the actual fallback path (`Auto` / `CloneOrCopy`) had resolved. Closes that TODO. The resolved method is already known inside `link_file`'s `log_method_once` helper — the same once-per-process atomic that the existing `tracing::info!` diagnostic uses. Threads `R: Reporter` through `link_file`, `auto_link`, `clone_or_copy_link`, and the call chain above (`create_cas_files`, `create_virtual_dir_by_snapshot`, `install_package_by_snapshot`, `create_virtual_store`, `install_package_from_registry`, `install_frozen_lockfile`, `install_without_lockfile`) so the helper can call `R::emit(LogEvent::PackageImportMethod(...))` alongside the existing `tracing::info!`. The reporter type monomorphises away — zero runtime cost from the threading. Refactors `log_method_once` to take its bitfield atomic as a parameter so a unit test can pass a fresh atomic and observe the once-per-method contract without racing other tests on the process-global `LOGGED_METHODS`. The install integration test no longer asserts a `PackageImportMethod` event in its empty-lockfile sequence, since the empty-lockfile path makes no `link_file` calls. The new `log_method_once_emits_first_call_per_method_only` test in `link_file::tests` covers the channel directly.
28b7138 to
fca5693
Compare
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/package-manager/src/link_file.rs`:
- Around line 77-95: The dedupe state is stored in the module-static
LOGGED_METHODS which makes package-import-method logging global across installs;
change this to be install-scoped by removing/avoiding the static and threading
the dedupe AtomicU8 (or a small dedupe struct) from the install entry point into
link_file::log_method_once: update log_method_once to accept a &AtomicU8 (or
&mut DedupeState) parameter instead of using LOGGED_METHODS, keep the flag
constants (LOG_FLAG_CLONE, LOG_FLAG_HARDLINK, LOG_FLAG_COPY) and the method
matching logic, and update all call sites of log_method_once to pass the
per-install dedupe state created at install startup so each install instance
dedupes independently (ensure type matches Reporter generic R usage).
🪄 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: e5021df8-4be0-49db-bb50-bb7bdefbaa32
📒 Files selected for processing (11)
crates/package-manager/src/create_cas_files.rscrates/package-manager/src/create_virtual_dir_by_snapshot.rscrates/package-manager/src/create_virtual_store.rscrates/package-manager/src/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install_package_from_registry.rscrates/package-manager/src/install_without_lockfile.rscrates/package-manager/src/link_file.rscrates/reporter/src/lib.rscrates/reporter/src/tests.rs
Per review feedback on #356: the doc comment on `package_import_method_event_matches_pnpm_wire_shape` cited `cloneOrCopy` (camelCase) as the spelling pacquet's config enum deserializes from. `pacquet_npmrc::PackageImportMethod` is annotated with `rename_all = "kebab-case"`, so the on-disk and config spelling is `clone-or-copy`. Update the example to match what `.npmrc` actually contains. Drop the redundant `"method {expected}"` failure message from the inner `assert_eq!`; `assert_eq!` already prints both sides and the extra context wasn't worth keeping.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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/package-manager/src/link_file.rs`:
- Around line 174-175: The log_method_once::<R>(&LOGGED_METHODS, LOG_FLAG_COPY,
WireImportMethod::Copy) call is currently executed before the fs::copy syscall
so a copy event can be emitted even when the copy fails; change the call order
so fs::copy(source_file, target_link) is executed first and only on success
invoke log_method_once (e.g., propagate or map the fs::copy result, then call
log_method_once and return the successful result). Update the same pattern for
the other copy paths that use fs::copy and the same logging symbols
(LOGGED_METHODS, LOG_FLAG_COPY, WireImportMethod::Copy) so logging only occurs
after a successful copy.
🪄 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: cdba54db-eea6-4bcf-8aa2-716d3c2613e8
📒 Files selected for processing (11)
crates/package-manager/src/create_cas_files.rscrates/package-manager/src/create_virtual_dir_by_snapshot.rscrates/package-manager/src/create_virtual_store.rscrates/package-manager/src/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install_package_from_registry.rscrates/package-manager/src/install_without_lockfile.rscrates/package-manager/src/link_file.rscrates/reporter/src/lib.rscrates/reporter/src/tests.rs
✅ Files skipped from review due to trivial changes (1)
- crates/package-manager/src/install.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/reporter/src/tests.rs
- crates/reporter/src/lib.rs
- crates/package-manager/src/install_package_by_snapshot.rs
Per coderabbit review on #356: the previous commit kept `LOGGED_METHODS` as a module-static, which suppresses the `pnpm:package-import-method` reporter event for every install after the first in the same process. That contradicts upstream pnpm, where `createIndexedPackageImporter` builds a fresh closure per install, so the dedupe state is install-scoped. Threads an `&AtomicU8` from `Install::run` down through `InstallFrozenLockfile`, `InstallWithoutLockfile`, `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `InstallPackageBySnapshot`, `InstallPackageFromRegistry`, `create_cas_files`, and `link_file`. Each install layer carries the atomic as a struct field (matching the existing `verified_files_cache` pattern). The atomic type is `Sync`, so the rayon par_iter and tokio futures in the chain work without further changes. Removes the module-static `LOGGED_METHODS`. The unit test on `log_method_once` already passed its own atomic, so its contract is unchanged. Other `link_file` tests that don't care about emit semantics now construct an inline `&AtomicU8::new(0)` as a throwaway dedupe state; they exercise `SilentReporter` so the atomic is never read for emission anyway. Practical effect today is zero — pacquet runs as a one-shot CLI and exits after one install — but the change removes a footgun for tests that exercise multiple installs in the same nextest binary, and for any future embedded use.
Per Copilot review on #356: four call sites in `link_file` were emitting the structured `pnpm:package-import-method` event (and the companion `tracing::info!`) *before* the underlying `fs::copy` ran. On a copy failure that would record a method that never actually succeeded. Move the emit inside `.inspect(|_| ...)` on the `io::Result`'s `Ok` branch in: - explicit `Hardlink` → cross-device fallback to `fs::copy` - explicit `Copy` - `auto_link`'s `LINK_STATE_COPY` arm - `clone_or_copy_link`'s `LINK_STATE_COPY` arm The explicit `Clone` arm and the `Hardlink` happy path were already correct (`reflink_copy::reflink(...).inspect(...)` and emit-after- `Ok(())` respectively), so they're untouched. Also refresh two stale doc comments in `crates/reporter/src/lib.rs`: - `LogEvent::PackageImportMethod` no longer says "fires once per install when the importer is constructed"; it now describes the lazy first-success-per-method semantics with the install-scoped atomic from #356. - The `PackageImportMethod` enum doc no longer references the deleted `import_method_for_wire` helper, and explains that the wire value is the resolved method `link_file` actually used (so `auto` falling back to hardlink emits `hardlink`, not the optimistic `clone`).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per Copilot review on #356: - The `LOG_FLAG_*` constants in `link_file` were widened to `pub(crate)` in the previous commit but they're only referenced from within this module (the inline `mod tests` can already see parent private items). Restore them to module-private to keep the crate's internal API surface tight. - `Reporter::emit` may be invoked concurrently from arbitrary threads — `link_file` is called from `create_cas_files`'s rayon `par_iter`, and tarball / store-index work runs across tokio workers. Document that contract on the trait so future reporter implementations don't reach for unsynchronized global state. Note that both production sinks (`SilentReporter`, `NdjsonReporter`) already satisfy it.
Summary
Third channel from the backfill (#347), and a course-correction on a
fourth.
pnpm:package-import-method. Adds thePackageImportMethodvariant to
LogEventand emits it fromlink_file'slog_method_oncehelper, mirroring pnpm's emit atfs/indexed-pkg-importer/src/index.ts:32.The structured event fires alongside the existing
tracing::info!diagnostic the first time each resolved method (
clone/hardlink/
copy) actually succeeds, so forAutoandCloneOrCopythewire value is the post-fallback method rather than the optimistic
configured one.
The wire-format enum is
clone | hardlink | copyexactly. Pacquet'sNpmrc::package_import_methodcarriesAutoandCloneOrCopyontop of those, but those are dispatched-on by the auto-importer's
fallback chain, not emitted, so the wire enum stays in lockstep with
pnpm's. Threading the reporter through the link path required adding
<R: Reporter>tolink_file,auto_link,clone_or_copy_link,and the call chain above (
create_cas_files,create_virtual_dir_by_snapshot,install_package_by_snapshot,create_virtual_store,install_package_from_registry,install_frozen_lockfile,install_without_lockfile). The reportertype monomorphises away — zero runtime cost.
Install-scoped dedupe. The bitfield atomic that gates
once-per-method emission is install-scoped — created in
Install::runand threaded down as a
&'a AtomicU8field on each install-layerstruct, matching the existing
verified_files_cachepattern.Mirroring upstream pnpm's per-importer closure capture (a process-
static would suppress emits on every install after the first in the
same process — fine for the one-shot CLI today but a footgun for
tests and any future embedded use).
Emit-only-on-success. All four
fs::copy-bearing branches(explicit
Copy, explicitHardlink's EXDEV fallback to copy, andthe copy fallbacks in both
auto_linkandclone_or_copy_link)emit after the copy returns
Ok, so a copy failure can't record amethod that never succeeded.
Deferral on
pnpm:stage resolution_started/done. Investigatedthis channel before picking the current one and concluded it can't
land cleanly today: pacquet's no-lockfile flow interleaves resolution
and import per dependency, so emitting
resolution_doneafter importwork has already happened would break the JS reporter's per-package
state. Frozen-lockfile installs (the primary path today) don't need
the events — pnpm itself skips them in
deps-restorer. Recorded thedeferral on #347 alongside the dependency on a phase-separated
resolution pass.
Convention compliance
LogEvent::PackageImportMethodmirrors the upstream payloadfield-for-field; the wire enum uses pnpm's exact lowercase
strings.
upstream permalink at the pinned SHA (
086c5e91e8).link_file::tests::log_method_once_emits_first_call_per_method_onlydrives the helper directly with its own atomic and asserts the
once-per-method emission contract — sidesteps cross-test
interference on the production install-scoped atomic.
pacquet-reporter(
package_import_method_event_matches_pnpm_wire_shape)asserts every wire value renders correctly under the bunyan
envelope.
atomic-gated).
R::emitcall inlog_method_once, confirmedlog_method_once_emits_first_call_per_method_onlyfailed,then restored.
pnpm:package-import-methodbox in chore(reporter): backfill missing log events in already-ported code #347 on merge.Test plan
cargo nextest run -p pacquet-reporter— all wire-shape testspass including the new
package_import_method_event_matches_pnpm_wire_shape.cargo nextest run -p pacquet-package-manager log_method_once_emits_first_call_per_method_only— passes; per-test atomic, asserts once-per-method contract.
just ready— 259 tests pass, lint clean.References
pnpm:context), feat(reporter): emit pnpm:summary at install end #355 (pnpm:summary).core/core-loggers/src/packageImportMethodLogger.ts.fs/indexed-pkg-importer/src/index.ts:32.