diff --git a/pacquet/AGENTS.md b/pacquet/AGENTS.md index 246c269219..7f21896f11 100644 --- a/pacquet/AGENTS.md +++ b/pacquet/AGENTS.md @@ -49,7 +49,9 @@ Before writing code for a feature, bug fix, or behavior change: binary. Use the DI seam — a capability trait on the `Host` provider, threaded as `Sys: ` — only for branches a real fixture can't reach portably: filesystem error kinds - (`PermissionDenied`, `ENOSPC`, …), deterministic time, or the + (`PermissionDenied`, `ENOSPC`, …), deterministic time, shared + process-global state a test would otherwise mutate + (`env::set_var`, `set_current_dir`, the umask, …), or the external-service happy paths in features like `pnpm login` (2FA) and `pnpm publish` (OIDC / provenance) when those land. See [Dependency injection for tests](./CODE_STYLE_GUIDE.md#dependency-injection-for-tests) diff --git a/pacquet/CODE_STYLE_GUIDE.md b/pacquet/CODE_STYLE_GUIDE.md index 2936c193d3..178a0d71e6 100644 --- a/pacquet/CODE_STYLE_GUIDE.md +++ b/pacquet/CODE_STYLE_GUIDE.md @@ -512,6 +512,7 @@ The dependency-injection seam described below is the **narrow second route**. Re - **Filesystem error branches the host OS won't reproduce portably.** `PermissionDenied`, `ENOSPC`, a directory that disappears mid-walk, a chmod that fails after the file exists — provoking these on real disks is platform-specific, racy, or both. A fake that returns the exact `io::ErrorKind` is the only portable way to drive the branch. - **Deterministic time.** Asserting that `prunedAt` equals a specific HTTP-date (RFC 7231 IMF-fixdate, what `httpdate::fmt_http_date` emits), or that a throttled emitter fires on the second sample, needs the clock to be a known value. The real `SystemTime::now` makes those assertions flaky. +- **Shared process-global state that tests would otherwise mutate.** When a branch depends on a single per-process slot — environment variables, the current working directory, the umask, signal handlers, the global allocator — the only way to exercise it without DI is to write to that slot, and the write is observed by every other test in the same process. A serialisation lock (the `EnvGuard` pattern that pnpm/pacquet#343 + pnpm/pnpm#11718 retired from `default_store_dir`) restores correctness only by forcing the affected tests to run single-threaded, and leaves an `unsafe { env::set_var(...) }` block at the call site. A capability-trait fake keeps the read deterministic and the mutation contained to the test that needs it, so nextest's in-process parallelism stays useful and `unsafe` stays out of test code. The reverse follows too: a real-fixture happy path should not be promoted to DI just because it crosses a process-global slot — only the *mutation* side of the slot is the problem; reading whatever the shell already has is fine. - **External-service happy paths that can't be staged in CI.** Upstream pnpm has features whose *normal* flow depends on real external systems — `pnpm login`'s 2FA prompt round-trip, the OIDC token exchange and provenance attestation in `pnpm publish`, and similar — where the happy path itself is what needs faking, not just the error path. When pacquet ports those features, DI is the right tool for their tests too. (As of writing, these are not yet ported; this exception is documented so the convention is in place when they land.) - **Unreachable-by-design preconditions.** When a function declares a capability bound but a specific test exercises a branch that never reaches that capability, the fake satisfies the bound with `unreachable!` and documents the precondition. See the worked example below.