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

feat(reporter): emit pnpm:package-import-method from link_file#356

Merged
zkochan merged 6 commits into
mainfrom
feat/reporter-package-import-method
May 1, 2026
Merged

feat(reporter): emit pnpm:package-import-method from link_file#356
zkochan merged 6 commits into
mainfrom
feat/reporter-package-import-method

Conversation

@zkochan

@zkochan zkochan commented Apr 30, 2026

Copy link
Copy Markdown
Member

Summary

Third channel from the backfill (#347), and a course-correction on a
fourth.

pnpm:package-import-method. Adds the PackageImportMethod
variant to LogEvent and emits it from link_file's
log_method_once helper, mirroring pnpm's emit at
fs/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 for Auto and CloneOrCopy the
wire value is the post-fallback method rather than the optimistic
configured one.

The wire-format enum is clone | hardlink | copy exactly. Pacquet's
Npmrc::package_import_method carries Auto and CloneOrCopy on
top 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> to 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). The reporter
type monomorphises away — zero runtime cost.

Install-scoped dedupe. The bitfield atomic that gates
once-per-method emission is install-scoped — created in Install::run
and threaded down as a &'a AtomicU8 field on each install-layer
struct, matching the existing verified_files_cache pattern.
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, explicit Hardlink's EXDEV fallback to copy, and
the copy fallbacks in both auto_link and clone_or_copy_link)
emit after the copy returns Ok, so a copy failure can't record a
method that never succeeded.

Deferral on pnpm:stage resolution_started/done. Investigated
this 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_done after import
work 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 the
deferral on #347 alongside the dependency on a phase-separated
resolution pass.

Convention compliance

  • LogEvent::PackageImportMethod mirrors the upstream payload
    field-for-field; the wire enum uses pnpm's exact lowercase
    strings.
  • Code comment + commit message + this PR all reference the
    upstream permalink at the pinned SHA (086c5e91e8).
  • Recording-fake DI test in
    link_file::tests::log_method_once_emits_first_call_per_method_only
    drives 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.
  • Wire-shape test in pacquet-reporter
    (package_import_method_event_matches_pnpm_wire_shape)
    asserts every wire value renders correctly under the bunyan
    envelope.
  • No throttle (one emit per resolved method per install,
    atomic-gated).
  • Verified the test catches the regression: commented out the
    R::emit call in log_method_once, confirmed
    log_method_once_emits_first_call_per_method_only failed,
    then restored.
  • Tick the pnpm:package-import-method box 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 tests
    pass 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

@codecov

codecov Bot commented Apr 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.89809% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.60%. Comparing base (403d38c) to head (f368a69).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/package-manager/src/link_file.rs 92.92% 8 Missing ⚠️
crates/package-manager/src/create_virtual_store.rs 33.33% 4 Missing ⚠️
...package-manager/src/install_package_by_snapshot.rs 0.00% 4 Missing ⚠️
...kage-manager/src/create_virtual_dir_by_snapshot.rs 0.00% 3 Missing ⚠️
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.
📢 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.01     16.0±0.24ms   270.5 KB/sec    1.00     15.9±0.10ms   272.6 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.279 ± 0.068 2.176 2.420 1.00
pacquet@main 2.279 ± 0.083 2.199 2.478 1.00 ± 0.05
pnpm 5.747 ± 0.058 5.666 5.867 2.52 ± 0.08
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)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 504.3 ± 51.7 476.7 648.1 1.04 ± 0.12
pacquet@main 484.7 ± 20.6 469.5 529.4 1.00
pnpm 2261.0 ± 160.9 2049.6 2563.2 4.66 ± 0.39
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
      ]
    }
  ]
}

@zkochan zkochan force-pushed the feat/reporter-summary branch from bbd6d3e to 682bc70 Compare May 1, 2026 20:34
@zkochan zkochan force-pushed the feat/reporter-package-import-method branch from ae92c5b to fe3ca8b Compare May 1, 2026 20:39
@coderabbitai

coderabbitai Bot commented May 1, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Threads a generic reporter type R: Reporter and a per-install AtomicU8 logging flag through install, virtual-store/dir, CAS creation, and linking code; adds a new pnpm:package-import-method reporter event and emits it once per resolved import method during linking.

Changes

Cohort / File(s) Summary
Reporter events & tests
crates/reporter/src/lib.rs, crates/reporter/src/tests.rs
Add LogEvent::PackageImportMethod + PackageImportMethodLog and PackageImportMethod enum (lowercase wire); add/adjust serialization tests.
Linking logic
crates/package-manager/src/link_file.rs
Make link_file and helpers generic over R: Reporter; replace module-global one-shot logging with log_method_once<R>(logged, flag, WireImportMethod) that emits pnpm:package-import-method once per flag; expose LOG_FLAG_* as pub(crate); update/add tests.
CAS file creation
crates/package-manager/src/create_cas_files.rs
Make create_cas_files generic over R: Reporter, accept logged_methods: &AtomicU8, and call link_file::<R>(logged_methods, ...) inside parallel processing.
Virtual dir / store orchestration
crates/package-manager/src/create_virtual_dir_by_snapshot.rs, crates/package-manager/src/create_virtual_store.rs
Propagate R: Reporter generics into .run::<R>() calls, add logged_methods: &AtomicU8 fields/args, and update signatures/call sites.
Install entrypoints and flows
crates/package-manager/src/install.rs, crates/package-manager/src/install_frozen_lockfile.rs, crates/package-manager/src/install_package_by_snapshot.rs, crates/package-manager/src/install_package_from_registry.rs, crates/package-manager/src/install_without_lockfile.rs
Make top-level and sub-step run methods generic over R: Reporter; create and thread an install-scoped AtomicU8 (logged_methods) into subflows and tests; update recursive helper signatures and call-sites to propagate R.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • KSXGitHub

Poem

🐰 I hop through crates with a tiny, bright pen,
Thread R through runs and log each chosen when,
Once per method — clone, hardlink, copy shown,
A debug nibble in NDJSON sown,
🥕 Hooray, the rabbit stamps the log and hops home.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main change: adding a pnpm:package-import-method reporter event emission from the link_file function.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/reporter-package-import-method

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

@zkochan zkochan force-pushed the feat/reporter-summary branch from 682bc70 to baaeb0b Compare May 1, 2026 20:46
@zkochan zkochan force-pushed the feat/reporter-package-import-method branch from fe3ca8b to 713fce8 Compare May 1, 2026 20:47
@zkochan zkochan requested a review from Copilot May 1, 2026 20:51

Copilot AI 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.

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::LogEvent with PackageImportMethod plus a wire-format enum (clone|hardlink|copy).
  • Emit pnpm:package-import-method from Install::run immediately after pnpm: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.

Comment thread crates/reporter/src/tests.rs
Comment thread crates/reporter/src/tests.rs Outdated
Base automatically changed from feat/reporter-summary to main May 1, 2026 21:19
zkochan added 2 commits May 1, 2026 23:39
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.
@zkochan zkochan force-pushed the feat/reporter-package-import-method branch from 28b7138 to fca5693 Compare May 1, 2026 21:39
@zkochan zkochan requested a review from Copilot May 1, 2026 21:40

@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/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

📥 Commits

Reviewing files that changed from the base of the PR and between 403d38c and 28b7138.

📒 Files selected for processing (11)
  • crates/package-manager/src/create_cas_files.rs
  • crates/package-manager/src/create_virtual_dir_by_snapshot.rs
  • crates/package-manager/src/create_virtual_store.rs
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/package-manager/src/install_package_by_snapshot.rs
  • crates/package-manager/src/install_package_from_registry.rs
  • crates/package-manager/src/install_without_lockfile.rs
  • crates/package-manager/src/link_file.rs
  • crates/reporter/src/lib.rs
  • crates/reporter/src/tests.rs

Comment thread crates/package-manager/src/link_file.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.

Copilot AI 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.

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.

Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/reporter/src/tests.rs
Comment thread crates/package-manager/src/link_file.rs Outdated
Comment thread crates/package-manager/src/link_file.rs Outdated
Comment thread crates/package-manager/src/link_file.rs
Comment thread crates/package-manager/src/link_file.rs
Comment thread crates/reporter/src/lib.rs Outdated

@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/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

📥 Commits

Reviewing files that changed from the base of the PR and between 28b7138 and fca5693.

📒 Files selected for processing (11)
  • crates/package-manager/src/create_cas_files.rs
  • crates/package-manager/src/create_virtual_dir_by_snapshot.rs
  • crates/package-manager/src/create_virtual_store.rs
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/package-manager/src/install_package_by_snapshot.rs
  • crates/package-manager/src/install_package_from_registry.rs
  • crates/package-manager/src/install_without_lockfile.rs
  • crates/package-manager/src/link_file.rs
  • crates/reporter/src/lib.rs
  • crates/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

Comment thread crates/package-manager/src/link_file.rs Outdated
zkochan added 2 commits May 1, 2026 23:55
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`).
Copilot AI review requested due to automatic review settings May 1, 2026 22:01

Copilot AI 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.

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.

Comment thread crates/package-manager/src/link_file.rs Outdated
Comment thread crates/package-manager/src/create_cas_files.rs
@zkochan zkochan changed the title feat(reporter): emit pnpm:package-import-method at install start feat(reporter): emit pnpm:package-import-method from link_file May 1, 2026
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.
@zkochan zkochan merged commit 41cb43f into main May 1, 2026
14 of 15 checks passed
@zkochan zkochan deleted the feat/reporter-package-import-method branch May 1, 2026 22:17
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