AGENTS.md

1# AGENTS.md (pacquet)
2 
3Guidance for AI coding agents working in `pnpm/`.
4 
5**Read [`../AGENTS.md`](../AGENTS.md) first.** It covers the conventions that apply across the whole monorepo — GitHub PR workflow, signing agent-authored content, conventional commit messages, code-reuse philosophy, "never ignore test failures," and the PR-conflict resolution script. This file specializes those rules for pacquet's Rust code and adds pacquet-only ones.
6 
7## What this project is
8 
9`pacquet` is the [pnpm](https://pnpm.io) CLI implemented in Rust. It is one of
10two parallel implementations of the same package manager — the other is the
11TypeScript pnpm CLI (the workspaces outside `pnpm/`). The two are kept
12behaviorally identical: the same commands, flags, defaults, error codes, file
13formats, lockfile shape, and directory layout. pacquet is not a downstream port
14that trails the TypeScript CLI; it is a source of truth in its own right, at
15near-complete feature parity, and the two stacks are developed together.
16 
17## The cardinal rule
18 
19**pacquet and the TypeScript pnpm CLI must stay behaviorally identical.**
20They are parallel implementations of one package manager, developed together at
21near-complete feature parity. Any user-visible change — a command, flag,
22default, error code or message, lockfile/manifest/state-file format, log
23emission parsed by `@pnpm/cli.default-reporter`, store layout, or hook
24semantic — must land in both stacks at the same time. The repo-wide statement
25of this obligation lives in
26[`../AGENTS.md`](../AGENTS.md#keep-pnpm-and-pacquet-in-sync); this section is the
27pacquet-side detail.
28 
29Neither stack is downstream of the other. You are not "porting from" the
30TypeScript code: when you implement or change behavior in pacquet, make the
31equivalent change in the TypeScript workspaces in the same PR, and vice versa.
32If you genuinely can't (different expertise, scope too large, or the other
33stack hasn't grown the surrounding feature yet), ship your side and say so in
34the PR description so the matching commits can follow before it lands.
35 
36Working rules:
37 
381. **Keep the two implementations in agreement.** When you touch behavior in
39   pacquet, find the counterpart in the TypeScript workspaces — they live at
40   the repo root (`pnpm/` for the CLI entry, `pkg-manager/`, `resolving/`,
41   `lockfile/`, `store/`, `fetching/`, `config/`, `hooks/`, and so on; see the
42   [repo-structure section](../AGENTS.md#repository-structure)) — and change it
43   there too. The two must agree on logic, edge cases, config resolution, error
44   messages, and file/lockfile formats.
452. **Match observable behavior, not structure.** Structural similarity (similar
46   function decomposition and names) is a convenience for cross-referencing, not
47   a requirement. What must match is what a user or a downstream tool can
48   observe.
493. **Don't diverge unilaterally.** Do not add a feature, flag, or quirk to one
50   stack without the other, and do not "fix" a behavior in only one. A genuine
51   bug present in both is fixed in both.
524. **Log emissions are part of behavioral identity.** A function that fires
53   `pnpm:<channel>` events through the reporter must use the same call site,
54   payload, and ordering in both stacks so `@pnpm/cli.default-reporter` parses
55   pacquet's NDJSON the same way it parses the TypeScript CLI's. See
56   [Reporter / log events](./CODE_STYLE_GUIDE.md#reporter--log-events)
57   in the style guide for the convention (channel mapping, threading
58   `R: Reporter`, emit-site placement, recording-fake tests).
595. **Prefer real fixtures; reach for the dependency-injection seam
60   only when they can't cover the branch.** Most happy paths and
61   error paths should be tested with a `tempfile::TempDir`, the
62   mocked registry, or an integration test that spawns the actual
63   binary. Use the DI seam — a capability trait on the `Host`
64   provider, threaded as `Sys: <Bounds>` — only for branches a real
65   fixture can't reach portably: filesystem error kinds
66   (`PermissionDenied`, `ENOSPC`, …), deterministic time, shared
67   process-global state a test would otherwise mutate
68   (`env::set_var`, `set_current_dir`, the umask, …), or the
69   external-service happy paths in features like `pnpm login` (2FA)
70   and `pnpm publish` (OIDC / provenance) when those land. See
71   [Dependency injection for tests](./CODE_STYLE_GUIDE.md#dependency-injection-for-tests)
72   in the style guide for the gating rule, the names (`Sys`, `Host`,
73   `Fs*`, `Clock`, `EnvVar`, …), the eight principles, and the
74   `modules-yaml` worked example.
75 
76If the intended behavior is unclear or looks wrong, stop and ask the user
77rather than guessing.
78 
79## Modeling branded string types
80 
81TypeScript pnpm leans on *branded* string types. A branded string is a
82plain string narrowed by a phantom property (for example,
83`type PkgName = string & { __brand: 'PkgName' }`), so the type system can
84track intent that the runtime cannot see. Some brands are stamped through
85a validating constructor. Others are minted with a bare `as` type assertion and
86have no runtime check at all. Both stacks must preserve that distinction,
87because it is part of the public contract pnpm exposes through manifest,
88lockfile, state, and config files. The TypeScript brand and the Rust newtype
89must agree on validation policy.
90 
91Rules for a Rust newtype standing in for a branded string type ("the
92TypeScript side" below is its TypeScript counterpart):
93 
941. **Declare a newtype wrapper.** Do not collapse the brand into a plain
95   `String` or `&str`. Give the type its own struct so misuse is a type
96   error in pacquet too.
972. **If upstream always validates before construction, validate too.**
98   When every brand site in pnpm runs through a checking factory, pacquet's
99   wrapper must construct only via `TryFrom<String>` and/or `FromStr`. Do
100   not provide an infallible public constructor that takes an arbitrary
101   string.
1023. **If upstream never validates, just brand for type-safety.** Some
103   upstream brands exist purely to keep the type system from confusing
104   one string slot with another. For example, a brand may exist to prevent
105   a `PkgId` from being passed where a `PkgName` is expected, even though
106   the value is never validated at runtime. In that case the Rust wrapper
107   should expose an infallible `From<String>` (and `From<&str>` when
108   convenient). The type-safety win is the whole point, and no validator
109   is needed.
1104. **If upstream occasionally constructs without validation, expose
111   `from_str_unchecked`.** When pnpm sometimes mints the brand via a bare
112   `as` assertion, skipping its validator, add a `from_str_unchecked` (or
113   similarly named) constructor on the Rust side so callers can opt into
114   the same unchecked path explicitly. Keep the validating constructor as
115   well. `from_str_unchecked` is the escape hatch, not the default.
1165. **Match upstream serde behavior.** If the branded type crosses a
117   JSON, YAML, or INI boundary (manifest files, lockfiles, state files,
118   config files, and similar), wire the wrapper into serde so the
119   validation policy survives serialization:
120   - `#[serde(try_from = "String")]` for deserialization, so
121     deserialized values go through the validator.
122   - `#[serde(into = "String")]` for serialization.
123   Use both when the type is round-tripped.
1246. **Derive simple conversions with `derive_more`.** When the conversion
125   impls implied by the rules above are mechanical (a one-liner that
126   wraps or unwraps the inner field), use `#[derive(derive_more::From)]`
127   and `#[derive(derive_more::Into)]` rather than handwriting an `impl`
128   block. Fall back to a manual `impl` only when the conversion needs
129   custom logic, such as validation or normalization. `derive_more` is
130   already a workspace dependency.
1317. **String-literal unions become `enum`s.** If upstream uses a string
132   literal type or a union of string literals (for example,
133   `'auto' | 'always' | 'never'`), model it as a Rust `enum`, not a
134   newtype wrapper. The set of valid values is closed, so encode that.
1358. **Template literal types are branded strings.** If upstream uses a
136   string template literal type (for example,
137   ``` `${string}@${string}` ```), treat it the same as a branded string
138   type. Use a newtype wrapper with the validation discipline from rules
139   2 through 5 above.
140 
141## Follow the project guides
142 
1431. Follow the contributing guide in [`CONTRIBUTING.md`](./CONTRIBUTING.md), and **ALWAYS** double-check before committing. It covers commit message format, writing style, setup, and the automated checks to run before committing.
1442. Follow the code style guide in [`CODE_STYLE_GUIDE.md`](./CODE_STYLE_GUIDE.md), and **ALWAYS** double-check before committing. It covers code-level conventions not enforced by tooling: imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching, `pipe-trait`, error handling, test layout, logging during tests, and cloning of `Arc` and `Rc`.
145 
146## Repo layout (inside `pnpm/`)
147 
148- `crates/` — library and binary crates that make up pacquet.
149  - `cli`, `package-manager`, `package-manifest`, `lockfile`, `store-dir`,
150    `tarball`, `registry`, `network`, `npmrc`, `fs`, `executor`,
151    `diagnostics`, `testing-utils`.
152- `tasks/` — developer tooling: `integrated-benchmark`, `micro-benchmark`,
153  `registry-mock`.
154- `CONTRIBUTING.md` — commit-message format, writing style, setup, and the
155  automated checks to run before submitting. Read it before submitting code.
156- `CODE_STYLE_GUIDE.md` — manual code-style conventions beyond what `cargo
157  fmt`, `taplo`, and clippy enforce: imports, modules, naming, ownership
158  and borrowing, trait bounds, pattern matching, `pipe-trait`, error
159  handling, test layout, and `Arc`/`Rc` cloning. Read it before submitting
160  code.
161 
162The Rust workspace (`Cargo.toml`, `Cargo.lock`, `rust-toolchain.toml`,
163`justfile`, `.cargo/`, `.taplo.toml`, etc.) lives at the **repo root**, not
164inside `pnpm/`. Run `cargo` and `just` from the repo root.
165 
166## Commands
167 
168Prefer `just` recipes when one fits; drop down to `cargo` / `taplo` / etc.
169directly when you need flags the recipe doesn't expose (e.g. filtering tests
170by crate or name — see below).
171 
172- `just ready` — run the same checks CI runs (typos, fmt, check, test, lint).
173  Run this before declaring a task complete.
174- `just test` — `cargo nextest run`.
175- `just lint` — `cargo clippy --locked --workspace --all-targets -- --deny warnings`.
176- `just check` — `cargo check --locked --workspace --all-targets`.
177- `just fmt` — `cargo fmt` + `taplo format`.
178- `just cli -- <args>` — run the pacquet binary.
179- `just registry-mock <args>` — manage the mock registry used by tests.
180- `just integrated-benchmark <args>` — compare revisions or compare against
181  pnpm itself (see `CONTRIBUTING.md`).
182 
183Warnings are errors (`--deny warnings` in lint). Do not silence them with
184`#[allow(...)]` unless there is a specific, justified reason.
185 
186## Tests
187 
188- Tests live alongside the code they exercise (standard Cargo layout) plus
189  integration tests under each crate's `tests/`. Shared pacquet fixtures live
190  under `crates/testing-utils/src/fixtures/`; registry package fixtures live
191  under `../pnpr/.fixtures/packages/`.
192- Snapshot tests use `insta`. When an intentional change alters a snapshot,
193  review the diff carefully, then accept with `cargo insta review`. Never
194  accept snapshot changes blindly.
195- Tests that need the mocked registry start `pnpr` through
196  `pacquet-testing-utils`; `cargo test` / `cargo nextest run` should not
197  require a separate `just registry-mock launch` step.
198- When a behavior change spans both stacks, keep their tests in sync — give
199  pacquet a Rust test for the same scenario the TypeScript stack covers (and
200  vice versa) whenever it translates. Matching test coverage is the easiest
201  way to prove behavioral parity.
202- The active test-porting plan lives in
203  [`plans/TEST_PORTING.md`](./plans/TEST_PORTING.md). It enumerates the
204  upstream TypeScript tests scheduled to be ported (with file paths and line
205  numbers) and the conventions expected of the ports — `known_failures`
206  modules, `pacquet_testing_utils::allow_known_failure!` at the
207  not-yet-implemented boundary, and the practice of temporarily breaking the
208  subject under test to verify the ported test actually catches the
209  regression. Consult it before adding ported tests, and update its
210  checkboxes as items land.
211- When temporarily breaking an implementation (to prove a test catches the
212  regression, or for any other experiment), revert with `git restore <file>`,
213  never by moving a saved backup copy into place. Cargo's freshness check is
214  mtime-based, and an mtime-preserving restore leaves the binary compiled
215  from the broken source looking fresh — later test runs then fail in
216  impossible-looking, "flaky" ways with nothing pointing at the stale
217  artifact. Details in the "Test the tests" section of
218  [`plans/TEST_PORTING.md`](./plans/TEST_PORTING.md). If test outcomes ever
219  flip with no code change, `touch` the implementation file and rerun before
220  debugging anything else.
221 
222### No "tolerant" tests for missing tools
223 
224Tests must not be tolerant of a missing build / runtime environment by
225silently `return`-ing early when a tool isn't found. Patterns like:
226 
227```rust
228fn skip_if_no_git() -> bool {
229    if std::process::Command::new("git").arg("--version").output().is_err() {
230        eprintln!("skipping: `git` not on PATH");
231        return true;
232    }
233    false
234}
235 
236#[test]
237fn my_test() {
238    if skip_if_no_git() {
239        return;
240    }
241    // ...
242}
243```
244 
245are forbidden. If the test needs a tool, just call into it and let the
246existing `.unwrap()` / `.expect(...)` panic when the tool is absent — a
247failing test in an under-provisioned environment is the correct signal.
248Tolerance defeats the purpose of testing: if the environment really
249doesn't have the required tools, that's the *environment's* fault and it
250needs to be fixed.
251 
252This applies in particular to `git`, `node`, and `npm` — git is ubiquitous
253on developer machines, and Node.js is a documented prerequisite for
254building pnpm. There is no realistic environment in which pacquet's tests
255should run *and* these tools should be absent.
256 
257The only marginally acceptable exception is platform-locked tools — APIs
258or binaries that exist on one OS but not another. Even then, prefer
259`#[cfg_attr(target_os = "windows", ignore = "...")]` (or the matching
260`#[cfg(unix)]` gate already used in this crate for `/bin/sh` shims) over a
261runtime probe-and-skip helper. The gate is visible to `cargo test` and
262shows up in the test report; a silent `return` does not.
263 
264### Running tests narrowly
265 
266Running the full suite is slow. While iterating, target what you're working
267on:
268 
269```sh
270# One crate
271cargo nextest run -p pacquet-lockfile
272 
273# One test by name substring
274cargo nextest run -p pacquet-lockfile &lt;name_substring&gt;
275 
276# One integration test file
277cargo nextest run -p pacquet-lockfile --test &lt;file_stem&gt;
278```
279 
280Run `just ready` (full suite) before handing the PR off.
281 
282## Style
283 
284`CODE_STYLE_GUIDE.md` is the source of truth. Highlights:
285 
286- Choose owned vs. borrowed parameters to minimize copies; widen to the most
287  encompassing type (`&Path` over `&PathBuf`, `&str` over `&String`) when it
288  doesn't force extra copies.
289- Prefer `Arc::clone(&x)` / `Rc::clone(&x)` over `x.clone()` for reference-
290  counted types, so the cost is visible at the call site.
291- Follow the test-logging guidance in the style guide — log before non-
292  `assert_eq!` assertions, `dbg!` complex structures, skip logging for simple
293  scalar `assert_eq!`.
294- Follow [Rust API Guidelines](https://rust-lang.github.io/api-guidelines/naming.html)
295  for naming.
296- **No star imports inside module bodies.** Write `use super::{Foo, bar}`
297  instead of `use super::*;`, and the same for any other glob whose
298  target is a module you control. Two forms stay allowed: external-crate
299  preludes such as `use rayon::prelude::*;` and root-of-module
300  re-exports such as `pub use submodule::*;` in a `lib.rs`. See the
301  "No star imports" section in `CODE_STYLE_GUIDE.md`.
302 
303### Comments
304 
305Same baseline as [`../AGENTS.md`](../AGENTS.md#comments): write code that explains itself; comments are for the non-obvious *why*, not a translation of the *what*.
306 
307Rust-specific additions:
308 
309-   **Doc comments (`///`, `//!`) are rustdoc-visible API documentation.** Use them for item contracts. Put implementation-only rationale in regular `//` comments.
310-   **Tests are documentation. Do not duplicate them in prose.** If a behavioral scenario, edge case, failure mode, or worked example is already captured by a test (its name, its setup, its assertions), do not also narrate it in the doc comment on the implementation. The doc comment should state the contract once; the test demonstrates the behavior. The same applies in reverse: a test's own doc comment should not re-explain what the asserts already say, only the *why* if it is not obvious.
311-   **`// SAFETY:`, `// TODO:`, and similar prefixes are the exception.** They signal hidden invariants or known follow-ups that a reader cannot recover from the code alone.
312 
313Prefer renaming, restructuring, or extracting a helper over leaving a comment. Reach for prose only when the right names and types genuinely cannot carry the information.
314 
315### Preserve existing method chains
316 
317When editing existing code, do not break a method chain (including `pipe-trait`
318`.pipe(...)` chains) into intermediate `let` bindings unless you can justify
319the rewrite. Valid justifications include a chain that fails to compile after
320your edit, a borrow checker rejection, a meaningful performance win from
321splitting it up, or any other concrete reason the chain cannot stay as it is.
322Refactoring for style alone is not a justification when the task is something
323else. Keep the surrounding code shape intact and confine your edits to what
324the task asks for.
325 
326When the change you need can fit inside the existing chain, keep it there.
327For example, swapping a `PathBuf::from` allocation for a `Path::new` borrow:
328 
329```diff
330 output
331     .stdout
332     .pipe(String::from_utf8)
333     .expect("convert stdout to UTF-8")
334     .trim_end()
335-    .pipe(PathBuf::from)
336+    .pipe(Path::new)
337     .parent()
338     .expect("parent of root manifest")
339     .to_path_buf()
340```
341 
342Do not flatten the chain just because you happen to be editing nearby:
343 
344```diff
345-output
346-    .stdout
347-    .pipe(String::from_utf8)
348-    .expect("convert stdout to UTF-8")
349-    .trim_end()
350-    .pipe(PathBuf::from)
351-    .parent()
352-    .expect("parent of root manifest")
353-    .to_path_buf()
354+let stdout = String::from_utf8(output.stdout).expect("convert stdout to UTF-8");
355+Path::new(stdout.trim_end()).parent().expect("parent of root manifest").to_path_buf()
356```
357 
358If you do need to break a chain, state the justification in your reply, the
359commit message, or the PR description so a reviewer can confirm the rewrite
360was warranted. If the rewrite is purely stylistic, raise it with the user as
361its own change rather than including it in an unrelated edit.
362 
363## Code reuse (pacquet specifics)
364 
365The general "search before you write / extract shared code / prefer mature
366crates / keep deps at the right level" rules from
367[`../AGENTS.md`](../AGENTS.md#code-reuse-and-avoiding-duplication) apply.
368Pacquet-specific notes:
369 
370- Shared helpers tend to live in `crates/fs`, `crates/testing-utils`, and
371  `crates/diagnostics` — check there first.
372- Check whether the workspace already depends on something suitable (see
373  `[workspace.dependencies]` in the root `Cargo.toml`) before adding a new
374  dependency.
375- **Keep dependencies at the right level.** Add a new dependency to the
376  specific crate that needs it, not to the workspace root or to a shared
377  crate unless multiple crates actually depend on it.
378 
379## Errors and diagnostics
380 
381User-facing errors go through `miette` via the `pacquet-diagnostics` crate.
382Match pnpm's error codes and messages where pnpm defines them — error codes
383are part of the public contract, not implementation detail. See
384<https://pnpm.io/errors> for the canonical list.
385 
386## Commit and PR hygiene
387 
388- Keep commits focused. A bug fix commit should not also refactor or
389  reformat unrelated code.
390- When a change has a counterpart in the TypeScript pnpm CLI, land both
391  together; if they must be split, cross-reference the matching PR so a
392  reviewer can confirm the two stacks stay in sync.
393- Run `just ready` before pushing.
394- The repo-wide husky `pre-push` hook runs `pnpm/scripts/pre-push-rust.sh`,
395  which checks `rustfmt`, `taplo`, `cargo clippy` (with `--all-targets -D
396  warnings`), `cargo doc` (with `RUSTDOCFLAGS=-D warnings`), and `cargo
397  dylint`. Make sure your environment
398  can run cargo (the hook needs it) before pushing; `cargo-dylint` is
399  detected at runtime and skipped with a warning if not installed.
400 
401### Commit messages
402 
403Conventional Commits applies (see
404[`../AGENTS.md`](../AGENTS.md#commit-messages) for the full type list). Use
405a scope that names the crate or area being touched, matching the existing
406history (`git log --oneline` for examples). Pacquet adds one type beyond the
407standard list:
408 
409- `bench`: benchmark-only changes.
410 
411Examples (from this repo's history):
412 
413```
414fix(network): set explicit timeouts on default reqwest client
415feat(lockfile): support npm-alias dependencies in snapshots
416perf(store-dir): share one read-only StoreIndex across cache lookups
417```
418 
419## Things not to do
420 
421- Do not add a feature, flag, or behavior to one stack without making the
422  same change to the other. The two move together.
423- Do not change lockfile format, store layout, `.npmrc` semantics, or CLI
424  surface in only one stack — those are the shared contract and must change
425  in both at once.
426- A dependency that is already declared in `[workspace.dependencies]` in the
427  root `Cargo.toml` may be added to any crate that needs it.
428- Do not add a dependency that is not already declared in the workspace
429  without an explicit human request. If there is a clear benefit and
430  justification for pulling in a new third-party crate, ask the human to
431  approve it and to add it to `[workspace.dependencies]` rather than adding
432  it yourself. Consult `deny.toml` when evaluating candidates.
433- Do not introduce `unsafe` without a clear justification and review.
434- Do not disable lints, tests, or CI checks to make a PR green.
435