From 96040d7934f2a82431883ab6dfd30fcb0fe5179f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 18 May 2026 13:50:07 +0000 Subject: [PATCH] docs(pacquet): add shared-process-state DI exception to the style guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Dependency injection for tests" gating list in `pacquet/CODE_STYLE_GUIDE.md` enumerated four reasons to reach past real fixtures for the DI seam: filesystem error kinds, deterministic time, external-service happy paths, and unreachable-by-design preconditions. It did not cover the case this PR retired from `default_store_dir`: tests that mutate a single per-process slot (env vars, cwd, umask, signal handlers, …) and so race with every other test in the same process. The pre-existing `EnvGuard` workaround restored correctness only by holding a binary-wide mutex around `unsafe { env::set_var(...) }`, which forecloses parallelism inside the affected tests and leaves `unsafe` in the test source. A capability-trait fake keeps the read deterministic and the mutation contained to the test that needs it. Adds that bullet to the style guide between "Deterministic time" and "External-service happy paths," and updates rule 7 in `pacquet/AGENTS.md` (and its `CLAUDE.md` / `GEMINI.md` symlink targets) so the summary mirrors the longer list. --- Written by an agent (Claude Code, claude-opus-4-7). --- pacquet/AGENTS.md | 4 +++- pacquet/CODE_STYLE_GUIDE.md | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) 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.