From 1d9659c6a6ba95e7825f9d2ea14240bee22c0ec3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 31 Aug 2026 20:41:29 -0400 Subject: [PATCH] chore: address Copilot review round 4 on #2200 Both findings are my own comments contradicting each other, in the same file. - The no-pin example list still named `open` and `zod`, which the array right below it now pins. It names `ajv`, `commander` and `undici` instead, and says what the criterion actually is: absence is a statement about the installed tree, not about the manifests. - The consolidated group is described as root-OWNED aliases rather than `core/`'s runtime dependencies, since `@hono/node-server` is reached from web client code alone and is here for the resolution reason, not because `core/` imports it. Comments only. npm run local:gate passes. Signed-off-by: cliffhall --- vitest.shared.mts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/vitest.shared.mts b/vitest.shared.mts index 8c7883f0..6f169e48 100644 --- a/vitest.shared.mts +++ b/vitest.shared.mts @@ -60,8 +60,10 @@ export function vitestSharedPaths(clientDir: string) { // client's resolution chain is guaranteed to find one — not that they are // all root `dependencies`: `express` is test-only and sits in the root // `devDependencies`. Nor is this the complete root runtime set; a package - // that resolves unambiguously without help (`ajv`, `commander`, `open`, - // `undici`, `zod`) needs no pin and has none. + // no client install carries a copy of (`ajv`, `commander`, `undici`) + // resolves to the root on its own and needs no pin. Absence from this list + // is a statement about the installed tree, not about the manifests — check + // the tree before adding or removing an entry. // // `express` and `yaml` are reached only through `test-servers/src` — // express by the http/oauth servers, yaml by `load-config.ts` — which is @@ -92,11 +94,14 @@ export function vitestSharedPaths(clientDir: string) { find: /^proper-lockfile$/, replacement: path.resolve(repoRoot, "node_modules/proper-lockfile"), }, - // The rest of `core/`'s runtime dependencies, consolidated into the root - // manifest by #2195. Each of these used to be declared by the clients that - // reached it and was pinned to `/node_modules` accordingly; once - // the declarations went away those paths stopped existing, so the pin has - // to follow the package to the root. Left un-repointed they would resolve + // The rest of the root-owned aliases, consolidated into the root manifest + // by #2195. Mostly `core/`'s runtime dependencies, but not only — + // `@hono/node-server` is reached from web client code alone and is here for + // the same resolution reason rather than because `core/` imports it. Each + // used to be declared by the clients that reached it and was pinned to + // `/node_modules` accordingly; once the declarations went away + // those paths stopped existing, so the pin has to follow the package to + // the root. Left un-repointed they would resolve // to a directory that is not there — or, worse, to a transitive copy some // unrelated dependency happened to drag in, which is the duplicate this // whole pin list exists to prevent.