refactor(web): sharpen the src/lib vs src/utils rule and move misfiled modules

[cliffhall]

Closes #1776

Codifies the empirical rule the web client already followed loosely — utils = functions that compute; lib = things that instantiate, adapt, or touch the environment (I/O or wraps a subsystem → lib; pure transform → utils) — in AGENTS.md and clients/web/README.md, and moves the modules that sat on the wrong side of it. Pure, behavior-preserving move: nothing keys off the boundary (no path alias, no build/coverage glob).

Moves (source + co-located test)

Module	From → To	Why
sandbox-csp	lib → utils	Pure CSP-string construction from untrusted _meta.ui.csp; only a type import + console.warn.
browserTabVisibility	utils → lib	Reads document.visibilityState, registers visibilitychange listeners (DOM side effects).
oauthResume	utils → lib	Reads/writes window.sessionStorage (stateful I/O).
clearServerOAuthState	utils → lib	Drives the live InspectorClient / OAuthStorage subsystem.

Borderline audits (per the rule)

Kept in utils — pure transforms / type-only @inspector/core imports (a type-only import is not a subsystem dependency):

• pendingReauth — pure type declarations, no runtime code.
• deepLink, correlateTransportErrors, mcpNetworkHeaders, oauthNetworkPhase — pure URL/Map/Set/header parsing.
• oauthUx, oauthFlow — re-export core copy/predicates + constants; sessionStorage is mentioned only in a doc comment.

Kept whole in lib:

• downloadFile — mixes DOM-side-effect helpers (downloadBlob/downloadJsonFile) with a few pure ones; left whole on its dominant (side-effect) side rather than split, which the issue flags as acceptable.

Straggler + docs

• Relocated the lone src/test/lib/downloadFile.test.ts beside its source at src/lib/downloadFile.test.ts — it was the only web-owned test living under src/test/ instead of side-by-side.
• Documented the test-placement rule in AGENTS.md: side-by-side by default; src/test/ only for core/ tests (mirroring core/ layout), the integration project, and shared scaffolding.

Verification

• npm run ci green (validate → coverage ≥90 gate → verify:build-gate → smoke → Storybook).

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 45s —— View job

---

Review: refactor(web): sharpen the src/lib vs src/utils rule

• Gather context / read the diff
• Verify no dangling imports after the moves
• Check moved test files' relative paths
• Audit doc changes (AGENTS.md / clients/web/README.md)
• Re-audit the classification of remaining lib/utils modules
• Post review

---

Good refactor. The rule is genuinely useful (a legible signal at import time rather than an arbitrary split), the classification calls are all defensible, and the mechanics are clean. I verified the move statically:

• All import specifiers resolve. Every importer of the four moved modules points at the new path (App.tsx:181,196,209, App.test.tsx:559, createAppBridgeFactory.ts:22, pendingReauth.ts:3). No stragglers.
• No vi.mock() string paths reference the old locations (the failure mode a rename-only refactor most often trips on — a mock path is a string literal, not an import, so tsc can't catch it). Grepped; clean.
• Moved co-located tests all use same-directory relative imports (./browserTabVisibility.js, ./sandbox-csp, …), so their depth is unaffected. downloadFile.test.ts correctly went ../../lib/downloadFile → ./downloadFile for the depth change.
• Purity scan agrees with the placements. No file left in utils/ touches window/document/*Storage/fetch outside a doc comment (oauthFlow.ts:18, sandbox-csp.ts:84 are comment-only mentions). Every file in lib/ either has DOM/storage markers or drives a subsystem — clearServerOAuthState.ts has zero side-effect markers but mutates persisted OAuth state through OAuthStorage/InspectorClient, so lib is right for the subsystem reason, not the I/O one.

Three things worth addressing.

---

1. Stale path references left behind (should fix)

Four in-repo references still point at the pre-move paths. None affect runtime — they're prose and code comments — but a refactor whose whole premise is "file location is a legible signal" is undercut by comments that point at the wrong location, and AGENTS.md's maintenance rule asks for doc updates on any rename.

File	Line	Says	Should say
specification/v2_auth_mid_session.md	142	utils/oauthResume.ts, utils/browserTabVisibility.ts	lib/… (the utils/pendingReauth.ts in the same row is still correct)
specification/v2_auth_mid_session.md	326	clients/web/src/utils/oauthResume.ts	clients/web/src/lib/oauthResume.ts
clients/web/static/sandbox_proxy.html	21	src/lib/sandbox-csp.ts	src/utils/sandbox-csp.ts
clients/web/server/sandbox-controller.ts	56	src/lib/sandbox-csp.ts	src/utils/sandbox-csp.ts

The two sandbox-csp ones matter a bit more than the spec doc: both are load-bearing security comments explaining where the CSP is built, i.e. exactly the pointer a reader follows when auditing the sandbox.

Fix this →

---

2. src/lib/types/customHeaders.ts is the one module the new rule doesn't cover

It's not in the PR's borderline-audit list, and it's the clearest counterexample to the rule as written:

// clients/web/src/lib/types/customHeaders.ts
export interface CustomHeader { name: string; value: string; enabled: boolean }
export const getEnabledHeaders = (headers: CustomHeaders): CustomHeaders => …
export const headersToRecord = (headers: CustomHeaders): Record<string, string> => …
export const migrateFromLegacyAuth = (bearerToken?: string, headerName?: string): CustomHeaders => …

Types plus pure array/object transforms — zero I/O, zero subsystem ownership, testable with no mocks. By the rule this is textbook utils. It sits in lib only because it's under a types/ subdirectory that neither AGENTS.md nor the web README mentions.

I'd resolve it one of two ways, not leave it ambiguous:

• (a) Move lib/types/customHeaders.{ts,test.ts} → utils/customHeaders.{ts,test.ts} (or utils/types/), consistent with sandbox-csp moving for the same reason; or
• (b) Keep it and add a sentence to both docs saying src/lib/types/ is a third bucket for shared domain types + their pure constructors, deliberately outside the compute/environment split.

(a) is more consistent with the rest of the PR; (b) is fine if the types/ subdir is intentional. Either way the rule should stop having a silent exception living inside the directory it governs.

Fix this →

---

3. Minor: the one utils/ → lib/ arrow

After the move, the dependency arrows between the two directories are:

lib/oauthResume.ts   → utils/inspectorTabs.js, utils/oauthFlow.js   (value imports, fine)
utils/pendingReauth.ts → lib/oauthResume.js                          (type-only — the only one)

The new rule explicitly sanctions this (a type-only import is not a subsystem dependency), so it isn't a violation. But it's the sole arrow pointing from the pure layer into the infra layer, and the type it pulls is about as pure as a type gets:

export type OAuthResumeAuthKind = "step_up" | "reauth";

Declaring OAuthResumeAuthKind in utils/pendingReauth.ts and re-exporting it from lib/oauthResume.ts (which already re-exports OAUTH_PENDING_SERVER_KEY from utils/oauthFlow.js, so the pattern is established) would make the arrows strictly one-way — lib → utils, never back. Purely optional; call it if you think the one-way invariant is worth stating in the rule.

---

Nit on the rule's wording

AGENTS.md:248 says src/utils/ is "pure, side-effect-free functions", but three of the named anchors log: jsonUtils (console.error ×4), schemaUtils (console.warn ×3), and the newly-moved sandbox-csp (console.warn at line 70). The intent is obviously "no meaningful side effects", but as literally worded, sandbox-csp's warn is a violation of the rule the same commit uses to justify moving it. One clause — "diagnostic logging (console.warn/console.error) doesn't count as a side effect" — closes it.

---

Not verified

I did not run the test suite — clients/web/node_modules isn't installed in this job, so npm run validate / coverage weren't available. Everything above is static analysis (import graph, grep for string-literal mock paths, side-effect marker scan). Your npm run ci green run covers the executable side; the moves are pure renames (git detected all six as R100 except the two files with import-line edits), so the risk surface is exactly the import graph, which I did check exhaustively.

Verdict: approve-with-nits. Item 1 (stale paths) is the one I'd actually want fixed before merge; item 2 is worth a decision either way; item 3 and the wording nit are take-or-leave.
· branch v2/1776-lib-utils-rule

[cliffhall]

Thanks for the thorough review — addressed all four points in 86fbdfc.

1. Stale path references — fixed. All four updated: specification/v2_auth_mid_session.md (lines 142 & 326 → lib/oauthResume.ts / lib/browserTabVisibility.ts, pendingReauth left as utils/), and the two load-bearing sandbox security comments in static/sandbox_proxy.html and server/sandbox-controller.ts (→ src/utils/sandbox-csp.ts). Agreed those two matter most — they're the pointer an auditor follows.

2. lib/types/customHeaders.ts — moved to utils/ (option a). You're right it's the clearest counterexample: pure types + pure transforms, no I/O, no subsystem. Went with (a) for consistency with sandbox-csp moving for the same reason. Worth noting it had no path importers at all — the ExperimentalFeaturesPanel hits were a same-named customHeaders prop of a different HeaderPair type, so only its own co-located test referenced it. Removed the undocumented lib/types/ subdir entirely and documented in both AGENTS.md and the README that utils/ is the home for pure shared domain types + their constructors (no separate types/ bucket).

3. The one utils/ → lib/ arrow — done. Moved the OAuthResumeAuthKind declaration into utils/pendingReauth.ts and re-export it from lib/oauthResume.ts (following the existing OAUTH_PENDING_SERVER_KEY re-export pattern). Cross-directory arrows are now strictly one-way (lib → utils), and I codified that invariant in the AGENTS.md rule.

Wording nit — fixed. Both docs now state: diagnostic logging (console.warn/console.error) does not count as a side effect for the utils rule, so sandbox-csp/jsonUtils/schemaUtils warns aren't violations.

npm run ci green after the changes (coverage ≥90 gate, build-gate, smokes, Storybook).

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 38s —— View job

---

Review: refactor(web): sharpen the src/lib vs src/utils rule (round 2)

• Gather context / read the full diff
• Verify the four round-1 fixes landed correctly
• Re-check the import graph for dangling / stale paths
• Audit doc wording in AGENTS.md + web README
• Post review

---

All four round-1 items landed correctly. Re-verified from scratch rather than taking the reply at face value:

• Stale paths (item 1) — all four fixed: specification/v2_auth_mid_session.md:142,326 → lib/oauthResume.ts / lib/browserTabVisibility.ts (with utils/pendingReauth.ts correctly left alone), and the two load-bearing sandbox security comments (static/sandbox_proxy.html:21, server/sandbox-controller.ts:56) → src/utils/sandbox-csp.ts.
• customHeaders (item 2, option a) — moved to utils/; src/lib/types/ is gone entirely (ls src/lib shows a flat directory). The "no path importers" claim checks out — the ExperimentalFeaturesPanel hits are a customHeaders: HeaderPair[] prop of a structurally different {key, value} type, unrelated to CustomHeader.
• One-way arrows (item 3) — verified mechanically. grep "from \"\.\./lib" src/utils/ returns nothing; the only cross-directory edges are lib/oauthResume.ts:29,36,37 → utils/{inspectorTabs,oauthFlow,pendingReauth}. Strictly lib → utils. OAuthResumeAuthKind now originates in utils/pendingReauth.ts:12 with a comment explaining why it lives there, and lib/oauthResume.ts:43 re-exports it — no consumer churn, no cycle.
• Wording nit — the diagnostic-logging carve-out is in both docs.

Independent re-audit of the placements (grep for window|document|sessionStorage|localStorage|fetch|navigator across every non-test file in src/utils/): four files hit, all false positives — oauthFlow.ts:18 and sandbox-csp.ts:52,84,131-137 are doc-comment prose, and the fetch hits in correlateTransportErrors.ts / mcpNetworkHeaders.ts are FetchRequestEntry identifiers and comments about the fetch log, never the API. src/utils/ is genuinely clean.

Three things, all smaller than round 1.

---

1. The customHeaders move re-created the exact issue round-1 item 1 fixed (should fix)

Fixing the four oauthResume/sandbox-csp references was right, but moving lib/types/customHeaders.ts in the same commit left three new references to the now-deleted path — and nothing catches them, since prettier/eslint are scoped to core/** and each client's src (specification/ isn't linted or format-checked, and these are prose, not imports).

File	Line	Says
specification/v2_ux_interfaces_phase_0_4_inventory.md	23	`CustomHeader` from `clients/web/src/lib/types/customHeaders.ts`
specification/v2_ux_interfaces_plan.md	51	`clients/web/src/lib/types/customHeaders.ts` — `CustomHeader` / `CustomHeaders` shape
specification/v2_ux_interfaces_plan.md	185	copy `clients/web/src/lib/types/customHeaders.ts` from v1.5 verbatim

Line 23 is the one that matters — it's prescriptive guidance ("use CustomHeader from <path> when the form edits headers specifically"), i.e. a reader is meant to follow it, and it now dead-ends. The two plan.md hits are more historical ("copy from v1.5"), so leaving them is defensible if you treat that doc as a frozen record — but then the inventory one should still be updated.

Fix this →

---

2. utils/customHeaders.ts is dead code, and the docs now anchor the rule on it

The module has no importers at all — you established this yourself while making the move. Its only consumer is its own co-located test:

src/utils/customHeaders.ts       # 8 exports: CustomHeader, CustomHeaders,
                                 # createEmptyHeader, createHeaderFromBearerToken,
                                 # getEnabledHeaders, headersToRecord,
                                 # recordToHeaders, migrateFromLegacyAuth
src/utils/customHeaders.test.ts  # the only file that imports it

That's fine as a fact about the codebase (it predates this PR — v2_ux_interfaces_plan.md:185 shows it was copied from v1.5 ahead of the ServerSettingsForm work that would consume it). But this PR promotes it: both AGENTS.md:248 and the web README now list customHeaders as a named anchor for the utils/ rule, and AGENTS.md gives it the dedicated "shared domain types plus their pure constructors/transforms … there is no separate types/ bucket" clause. Anchoring a rule on a module nothing imports is a weak example — a reader who greps for it to see the pattern in action finds only a test.

Two ways out, either fine:

• Swap the anchor to something load-bearing (maskSecrets, deepLink, mcpNetworkHeaders) and keep customHeaders only as the illustration for the no-types/-bucket clause; or
• Note the staging in a half-sentence ("customHeaders — shape staged for ServerSettingsForm, see v2_ux_interfaces_plan.md") so the zero-importer state reads as intentional rather than as rot.

Not suggesting you delete it in this PR — that's a scope change and it'd need its own issue (deleting it also drops ~8 exports out of the coverage denominator, which is a separate conversation).

---

3. The type-only-import clause doesn't actually describe its own examples (nit)

AGENTS.md:248 reads:

A module that only imports types from @inspector/core (or elsewhere) is likewise pure and belongs here — a type-only import is not a subsystem dependency (e.g. pendingReauth is pure type declarations, oauthUx/oauthFlow re-export core copy/predicates).

pendingReauth fits (both its core imports are import type). The other two don't — they're value re-exports:

// utils/oauthFlow.ts:7
export { isUnauthorizedError } from "@inspector/core/auth/utils.js";

// utils/oauthUx.ts:1-12  — 7 runtime functions plus 3 types
export { authRecoveryRestoredMessage, isActionTriggeredOAuthRecovery,
         oauthPreRedirectToastCopy, /* … */ } from "@inspector/core/auth/oauthUx.js";

They belong in utils/ for a different reason than the one the sentence gives: the re-exported values are themselves pure functions, so re-exporting them doesn't take on a subsystem. As written, someone applying the rule literally would have to move both to lib/ (they import runtime, not just types) — which nobody wants. Splitting it into two clauses fixes it:

…a type-only import is not a subsystem dependency (pendingReauth). Re-exporting pure functions or constants from core is likewise not subsystem ownership (oauthUx, oauthFlow) — what makes a module lib is wrapping core's stateful runtime, not merely importing from it.

The same conflation is in the web README's condensed version ("a type-only import from @inspector/core doesn't count as a subsystem dependency"), which then lists sandbox-csp and customHeaders as examples — both of which are fine, so the README wording is only under-inclusive rather than wrong.

---

Cosmetic

specification/v2_auth_mid_session.md:142 — the edited table row got shorter and no longer pads its trailing | to the column width the surrounding rows use. Nothing checks it (prettier is scoped to core/** and each client's src, not specification/), so purely aesthetic in the raw file; renders identically.

---

Not verified

Same limitation as round 1: clients/web/node_modules isn't installed in this job, so I could not run validate / coverage / tsc. Everything above is static — import-graph traversal, a grep for vi.mock() string-literal paths against all old locations (clean; that's the failure mode a rename-only refactor most often trips on since tsc can't see mock paths), and an environment-marker scan of every file in src/utils/. Your npm run ci green run covers the executable side, and the remaining risk surface is exactly the import graph, which I checked exhaustively.

One coverage note worth stating since I couldn't run the gate: none of the moves change a file's content, and the web coverage include is directory-agnostic for clients/web/src, so per-file ≥90 is unaffected by the relocation — consistent with your green run.

Verdict: approve. Item 1 is a 3-line doc fix I'd take before merge (it's the same class of thing you just fixed, one commit later); items 2 and 3 are wording polish on the rule itself. The refactor is correct and the rule is a genuine improvement — the one-way-arrow invariant in particular is the part most likely to keep paying off, since it's the only piece of this that's mechanically checkable later.
· branch v2/1776-lib-utils-rule

[cliffhall]

Thanks — round-2 items addressed in c77df99 (doc-only, no code changed since the last green npm run ci).

1. Stale lib/types/customHeaders.ts references — fixed. Good catch that the customHeaders move re-created exactly the class round-1 fixed. Updated all three in specification/: v2_ux_interfaces_phase_0_4_inventory.md:23 (the prescriptive one) and v2_ux_interfaces_plan.md:51,185 → clients/web/src/utils/customHeaders.ts. Updated all three rather than treating plan.md as frozen, so a grep for the old path is fully clean.

2. customHeaders is dead code / weak anchor — noted the staging. Went with your second option: AGENTS.md now says the shape is staged for ServerSettingsForm (pointing at v2_ux_interfaces_plan.md), so the zero-importer state reads as intentional. Also swapped the leading anchor list to load-bearing modules (mcpNetworkHeaders in place of sandbox-csp/customHeaders) so the rule's primary examples are all things that are actually imported; customHeaders stays only as the illustration for the no-types/-bucket clause. Agreed deleting it is out of scope (separate issue — it'd change the coverage denominator).

3. Type-only-import clause didn't describe oauthUx/oauthFlow — fixed. You're right they're value re-exports, so the old wording would've forced them into lib. Split it into two clauses in both docs: a type-only import (pendingReauth) and re-exporting core's pure functions/constants (oauthUx/oauthFlow) are distinct reasons to stay in utils — what makes a module lib is wrapping core's stateful runtime, not merely importing from it.

Cosmetic: re-padded the v2_auth_mid_session.md:142 table row to the column width.

Since this round was purely docs/spec (specification/ isn't linted or format-checked, and no .ts/.tsx changed), the executable gate from the prior green npm run ci still holds.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 44s —— View job

---

Review: refactor(web): sharpen the src/lib vs src/utils rule (round 3)

• Read the full diff + round-3 commit
• Verify the three round-2 items landed
• Re-check import graph / stale paths repo-wide
• Audit final doc wording (AGENTS.md + web README)
• Post review

---

All three round-2 items landed. Re-verified independently rather than trusting the reply:

• Stale lib/types/customHeaders.ts refs (item 1) — closed, and closed completely. A repo-wide grep for all six old paths (lib/types/customHeaders, utils/oauthResume, utils/browserTabVisibility, utils/clearServerOAuthState, lib/sandbox-csp, test/lib/downloadFile) across *.ts|tsx|md|html|json|mjs returns nothing. Both plan.md hits were updated rather than left as a frozen record — good call, it makes the grep a clean invariant.
• Anchor / staging note (item 2) — AGENTS.md:248 now names mcpNetworkHeaders among the anchors and marks customHeaders as "a shape staged for ServerSettingsForm, see v2_ux_interfaces_plan.md, so it currently has no importer but its own test". That reads as intentional now.
• Type-only vs value re-export (item 3) — split into two clauses in both docs, and the split is now factually right: of the three non-import type core imports left in src/utils/, oauthFlow.ts:7 and oauthUx.ts:12 are pure-value re-exports (covered by the second clause) and correlateTransportErrors.ts:1-4 is import type (first clause).
• Cosmetic table padding — fixed at v2_auth_mid_session.md:142.

Mechanical re-checks, all clean: no vi.mock() string path references any old location; grep "\.\./lib/" src/utils/ returns nothing, so the one-way lib → utils invariant holds (only edges are lib/oauthResume.ts:29,36,37); OAuthResumeAuthKind originates at utils/pendingReauth.ts:10 with a comment explaining why, re-exported export type-only at lib/oauthResume.ts:43 — no cycle, no runtime cost. And the new test-placement bullet is accurate: src/test/ now contains exactly core/, fixtures/, integration/, renderWithMantine.tsx, setup.ts, scrollAreaStoryAssertions.ts — the four things the doc names, and nothing else. The downloadFile.test.ts straggler really was the last one.

One new finding, from following the rule's own claim to its conclusion.

---

1. "there is no separate types/ bucket" is contradicted by clients/web/src/types/ (should fix)

Both new doc sections assert it flatly:

AGENTS.md:248 — …there is no separate types/ bucket.
clients/web/README.md:62 — …no separate types/ bucket.

But clients/web/src/types/ exists and is load-bearing:

clients/web/src/types/
├── navigation.ts                                # export type InspectorTab = "tools" | "apps" | …
├── react-syntax-highlighter-grammar.d.ts        # ambient module stubs, wired via
├── react-syntax-highlighter-prism-light.d.ts    # tsconfig.app.json paths (lines 51, 54, 57)
└── react-syntax-highlighter-style.d.ts

Sharper still: the prescription to use it lives four lines above a line this PR edited. specification/v2_ux_interfaces_plan.md:181-182 says "Place it under clients/web/src/types/navigation.ts rather than in core/mcp/types.ts" — and line 185, the customHeaders line, is one of the three the round-2 commit rewrote. So the PR walked past the counterexample while fixing its neighbour.

This matters because "no types/ bucket" is the stated justification for deleting lib/types/ and moving customHeaders. As written it's a claim about the whole web src/ tree, and the tree disagrees. The move itself is still right — a types/ subdirectory nested inside the two directories the rule governs is exactly the silent exception worth removing. It's the scope of the sentence that overreaches.

Scoping it costs one clause, e.g.:

…there is no types/ sub-bucket inside lib/utils. (src/types/ is a different thing: ambient .d.ts module stubs wired through tsconfig.app.json paths — not a home for domain types.)

Fix this →

---

2. src/types/navigation.ts is dead and duplicates a live type (observation, likely its own issue)

While confirming the above — InspectorTab has zero references anywhere in the repo, including tests. The live tab-identifier type is src/utils/inspectorTabs.ts:

// src/types/navigation.ts  — 0 importers
export type InspectorTab = "tools" | "apps" | "prompts" | "resources" | "logs" | "tasks" | "protocol";

// src/utils/inspectorTabs.ts — the one actually used (App.tsx, lib/oauthResume.ts, …)
export const INSPECTOR_TAB_IDS = ["Apps","Tools","Prompts","Resources","Tasks","Logs","Protocol","Network"] as const;
export type InspectorTabId = (typeof INSPECTOR_TAB_IDS)[number];

Two competing tab-id types differing in case and membership (navigation.ts lacks Network, keeps a lowercase spelling), one of them dead. By the newly-codified rule it's a pure domain type and belongs in utils/ — i.e. folded into inspectorTabs.ts — which would also empty src/types/ of everything but the .d.ts stubs and make the "no types bucket" line true as written.

I wouldn't do it in this PR (same reasoning you applied to not deleting customHeaders — it's a scope change), but it's the file the new rule most directly implicates and it goes unmentioned. Worth an issue on the board, or a half-sentence in the doc if you'd rather keep it.

---

Nits

• The two docs' example lists drifted. Round 2 deliberately swapped AGENTS.md's anchors to load-bearing modules (jsonUtils, schemaUtils, toolUtils, maskSecrets, inspectorTabs, deepLink, mcpNetworkHeaders) so the primary examples are all things something imports. README.md:62's condensed list still ends …deepLink, sandbox-csp, customHeaders — carrying the zero-importer module as a headline example, without the "staged for ServerSettingsForm" caveat that makes it read as intentional in AGENTS.md. One-word fix if you want the two to agree.
• v2_ux_interfaces_plan.md:185 now reads "copy clients/web/src/utils/customHeaders.ts from v1.5 verbatim" — in v1.5 that file sat at lib/types/, so strictly the sentence names the v2 destination as if it were the v1.5 source. Parses fine as "the file at this path is a verbatim copy"; purely cosmetic, calling it out only because the alternative reading is mildly confusing to a future reader.

---

Not verified

Same limitation as rounds 1 and 2: clients/web/node_modules isn't installed in this job (tsc --noEmit fails at Cannot find type definition file for 'node'), so I couldn't run validate / coverage / typecheck. Everything above is static — full-repo grep for the six pre-move paths, import-graph traversal of every importer of the moved modules, a vi.mock() string-literal sweep (the failure mode tsc structurally can't catch on a rename), an environment-marker scan of every non-test file in src/utils/, and a directory listing of src/test/ against the doc's claim. Your green npm run ci covers the executable side; round 3 changed only .md files under specification/ and the two doc files, so nothing executable moved since that run.

Verdict: approve. Item 1 is a one-clause doc scope fix — the same "the rule shouldn't have a silent exception" argument this PR is built on, applied one directory up. Item 2 is a follow-up issue, not this PR's job. The refactor itself is correct and the invariant it establishes (one-way lib → utils, now mechanically greppable) is the part that'll keep paying off.
· branch v2/1776-lib-utils-rule

[cliffhall]

Thanks — round-3 items addressed in ffcd657 (doc-only again; no .ts/.tsx changed since the last green npm run ci).

1. "no separate types/ bucket" contradicted by src/types/ — fixed. You're right, the claim overreached: clients/web/src/types/ exists (holds navigation.ts + the three react-syntax-highlighter ambient .d.ts stubs wired via tsconfig.app.json paths). Scoped both docs to no types/ sub-bucket inside lib/utils (which is what the customHeaders move actually settles), and noted src/types/ is for ambient .d.ts module stubs, not domain types. Good catch that the counterexample sat four lines above one I'd just edited.

2. src/types/navigation.ts dead / duplicates inspectorTabs — filed as a follow-up. Confirmed InspectorTab has zero references and drifts from the live InspectorTabId (case + missing Network). Kept it out of this pure-move PR for the same coverage-denominator reason as customHeaders; tracked as #1785 (on the board, Todo). Removing it will also make the "no domain types in src/types/" framing true as written.

Nits — both fixed. Realigned the README example list to AGENTS.md's round-2 load-bearing anchors (inspectorTabs/mcpNetworkHeaders, dropped the zero-importer customHeaders from the headline list). And reworded v2_ux_interfaces_plan.md:185 so the v2 path reads as the copy's destination rather than being named as the v1.5 source location.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 6s —— View job

---

Review: refactor(web): sharpen the src/lib vs src/utils rule (round 4)

• Read the round-3 commit + full diff
• Verify the round-3 fixes landed
• Re-check import graph / stale paths repo-wide
• Audit final doc wording
• Post review

---

Round-3 items all landed; re-verified independently rather than from the reply.

• src/types/ scope (item 1) — both docs now say no types/ sub-bucket inside lib/utils and call out src/types/ separately. The narrow claim is correct: src/lib/ is flat (12 files, no subdirs) and src/utils/ has none either.
• navigation.ts (item 2) — confirmed dead (InspectorTab has zero refs repo-wide) and filed as refactor(web): remove dead src/types/navigation.ts InspectorTab, fold into utils/inspectorTabs #1785, labeled v2, assigned. Correctly kept out of this PR.
• Nits — README's anchor list now matches AGENTS.md's round-2 list exactly (jsonUtils, schemaUtils, toolUtils, maskSecrets, inspectorTabs, deepLink, mcpNetworkHeaders), with customHeaders demoted to the domain-types clause; plan.md:185 now reads "clients/web/src/utils/customHeaders.ts is a verbatim copy of the v1.5 module", which resolves the source-vs-destination ambiguity.

New mechanical checks this round, all clean:

• Every relative import in clients/web/src resolves on disk. Ran an exhaustive resolver over all from "./…" / from "../…" specifiers (.js → .ts/.tsx/index.*/.d.ts/asset), 0 unresolved. Stronger than a spot-check of the moved modules' importers, and the closest available stand-in for the tsc I can't run here.
• Repo-wide grep for all six pre-move paths across ts|tsx|md|html|json|mjs: nothing.
• One-way arrows hold: grep "\.\./lib/" src/utils/ → nothing; the only cross edges are lib/oauthResume.ts:29,36,37 → utils/{inspectorTabs,oauthFlow,pendingReauth}, the last import type.
• Environment-marker scan of every non-test file in src/utils/: 2 hits, both doc-comment prose (oauthFlow.ts:18, sandbox-csp.ts:52,84,131-137). The 3 non-import type core imports are oauthFlow.ts:7 / oauthUx.ts:12 (pure-value re-exports, second clause) and correlateTransportErrors.ts:4 (a type import spanning lines, so my grep caught the closing brace) — all correctly covered by the rule as reworded.
• src/test/ contains exactly core/ fixtures/ integration/ renderWithMantine.tsx setup.ts scrollAreaStoryAssertions.ts — precisely the four things the new test-placement bullet names.

One finding, and it's the round-3 finding recursing one level.

---

1. The src/types/ clause overshot the scope fix, and now contradicts a spec doc (should fix)

Round 3 asked for the claim to be scoped; the fix scoped it correctly for lib/utils but added a second, stronger assertion about src/types/ itself:

AGENTS.md:248 — …the top-level src/types/, which is not a home for domain types at all — it holds ambient .d.ts module stubs (e.g. the react-syntax-highlighter shims wired through tsconfig.app.json paths).
clients/web/README.md:62 — …the top-level src/types/ is separate: ambient .d.ts module stubs, not domain types.

src/types/ holds four files, and one of them is a plain .ts domain type:

clients/web/src/types/navigation.ts          ← export type InspectorTab = "tools" | "apps" | …
clients/web/src/types/react-syntax-highlighter-{grammar,prism-light,style}.d.ts

So the sentence is true of ¾ of the directory. That would be a shrug — except specification/v2_ux_interfaces_plan.md:181-182 prescribes the opposite, in the imperative:

**Tab enum placement**: `InspectorTab` is a UI-routing concept, not an MCP
or transport concept. Place it under `clients/web/src/types/navigation.ts`
rather than in `core/mcp/types.ts`.

That's three lines above plan.md:185, the customHeaders line this PR has now edited in two separate commits — the same near-miss pattern as round 3. Two in-repo docs now give opposite guidance about the same directory, and AGENTS.md is the one billed as "the contract for changing this codebase". A reader following plan.md puts a domain type where AGENTS.md says none belongs.

#1785 makes AGENTS.md true, so this self-heals on merge of that issue — but until then the doc asserts a state the tree doesn't have. Cheapest fix is a parenthetical that ties the two together, e.g. "…it holds ambient .d.ts module stubs … (the one domain type still there, the dead navigation.ts, is tracked for removal in #1785)", plus a one-line note at plan.md:181 that the placement is superseded. Either half alone would do; the parenthetical is the higher-value one since AGENTS.md is what agents load.

Fix this →

---

Not verified

Same limitation as rounds 1–3: clients/web/node_modules isn't installed in this job, so no validate / coverage / tsc. Everything above is static — the exhaustive relative-import resolution sweep described above, a repo-wide grep for the six pre-move paths, a vi.mock() string-literal sweep, an environment-marker scan of src/utils/, and a src/test/ listing against the doc's claim. Round 3 and round 4 changed only .md files, so your green npm run ci from the code round still covers the executable side; content-identical moves don't shift the per-file ≥90 gate.

Verdict: approve. Item 1 is a one-parenthetical doc fix and the only thing outstanding — the same "don't leave a silent exception inside the rule's own scope" argument, applied to the sentence written to close it. The refactor is correct, and the one-way lib → utils invariant remains the durable part: it's the only piece of this that a future reviewer can check with a single grep.
· branch v2/1776-lib-utils-rule

[cliffhall]

Thanks — round-4 item addressed in 3fecb5c (doc-only; no code changed since the last green npm run ci).

1. src/types/ claim overshot + contradicted plan.md — fixed. You're right: my round-3 wording ("not domain types at all") asserted a state the tree doesn't have yet — navigation.ts is a plain-.ts domain type still there — and v2_ux_interfaces_plan.md:181-182 prescribes putting InspectorTab there, so AGENTS.md and that spec doc gave opposite guidance about the same directory. Did both halves you suggested:

• AGENTS.md + README: softened to "not the place for new domain types" and added a parenthetical that the one lingering domain type (dead navigation.ts) is tracked for removal in refactor(web): remove dead src/types/navigation.ts InspectorTab, fold into utils/inspectorTabs #1785; until then a pure domain type belongs in utils/.
• plan.md:181: marked the tab-enum placement guidance as superseded by refactor(web): remove dead src/types/navigation.ts InspectorTab, fold into utils/inspectorTabs #1785, pointing at the live InspectorTabId in utils/inspectorTabs.ts.

So the two docs now agree, and it fully self-heals when #1785 merges. Good call catching that this was the round-3 finding recursing one level — the fix now describes the actual current state rather than the post-#1785 one.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 50s —— View job

---

Review: refactor(web): sharpen the src/lib vs src/utils rule (round 5)

• Read the round-4 commit + full diff
• Verify the round-4 fix landed
• Re-check import graph / stale paths repo-wide
• Audit final doc wording
• Post review

---

Round-4 item landed, and both halves of the suggested fix are there. Verified independently:

• AGENTS.md + README no longer claim src/types/ holds only ambient stubs — both now say it "is not the place for new domain types" and name the lingering navigation.ts as tracked in refactor(web): remove dead src/types/navigation.ts InspectorTab, fold into utils/inspectorTabs #1785. That matches the tree (src/types/ = navigation.ts + three react-syntax-highlighter*.d.ts).
• plan.md:181-184 carries the superseded note, pointing at the live InspectorTabId in utils/inspectorTabs.ts. The two docs now agree instead of prescribing opposite things about the same directory.
• refactor(web): remove dead src/types/navigation.ts InspectorTab, fold into utils/inspectorTabs #1785 exists, is OPEN, labeled v2, assigned to @cliffhall — so the self-heal is actually tracked, not just asserted.

Mechanical re-checks this round, all clean:

• All 882 relative import specifiers under clients/web/src resolve on disk (from "./…", from "../…", dynamic import(), and vi.mock() string literals — the last being the one tsc structurally can't catch on a rename). 0 unresolved.
• Repo-wide grep for all six pre-move paths across ts|tsx|md|html|json|mjs: nothing. The invariant round 3 established still holds after round 4's edits.
• One-way arrows hold: grep '\.\./lib' src/utils/ → nothing. The only cross edges are lib/oauthResume.ts:29,36,37 → utils/{inspectorTabs,oauthFlow,pendingReauth}, the last import type.
• Env-marker scan of every non-test file in src/utils/: 2 hits, both doc-comment prose (oauthFlow.ts:18, sandbox-csp.ts:84). The one non-import type core import left is oauthFlow.ts:7's pure-value re-export — covered by the reworded second clause. Inverse check on src/lib/: every file carries a DOM/storage/@inspector/core marker, so nothing in lib is silently pure.
• src/ top level is components/ hooks/ lib/ test/ theme/ types/ utils/ — no third grab-bag the rule fails to cover.

One nit, and it's in the sentence round 4 added.

---

1. The new #1785 clause is garbled (nit, but it's in the contract doc)

AGENTS.md:248:

(One plain-.ts domain type still sits there, the dead navigation.ts InspectorTab, superseding [#1785] will remove it — until then a pure domain type belongs in utils/, not src/types/.)

"superseding #1785 will remove it" doesn't parse — the participle has no subject, and as written it reads as if something is superseding the issue rather than the issue superseding the placement. Presumably a merge of two phrasings ("which #1785 will remove" and "superseded by #1785"). The README's parallel sentence is fine ("the one that lingers there, dead navigation.ts, is tracked for removal in #1785"), so it's only the AGENTS.md copy.

Worth fixing because AGENTS.md is the file every agent loads verbatim, and this is the clause that tells a reader where to put a new domain type. Suggested: "…the dead navigation.ts InspectorTab, which #1785 will remove — until then a pure domain type belongs in utils/, not src/types/."

Fix this →

---

2. The src/utils/ bullet has accreted past readability (observation)

AGENTS.md:248 is now a single ~1,400-character bullet holding the rule plus five parenthetical carve-outs, each added by a different review round: the anchor list, the domain-types/no-types/-sub-bucket clause, the src/types/ distinction, the #1785 aside, the diagnostic-logging carve-out, and the two @inspector/core clauses. Every one is individually justified — collectively they bury the actual rule (pure functions in, pure functions out) under its own exception list, and the src/types/ material is now longer than the src/utils/ material it's nested inside.

Not a correctness problem and not worth another round on its own. But if you're touching the line for item 1 anyway, promoting the carve-outs to sub-bullets under src/utils/ (Domain types / Diagnostic logging / Importing from core) would make it scannable, and would let the src/types/ note move out to its own line where it belongs — it's a statement about a sibling directory, not about utils.

---

Not verified

Same limitation as rounds 1–4: clients/web/node_modules isn't installed in this job, so no validate / coverage / tsc. Everything above is static — the 882-specifier import resolution sweep (including vi.mock() literals), a repo-wide grep for the six pre-move paths, env-marker scans of src/utils/ and src/lib/, an InspectorTab reference count, and a src/ layout listing against the docs' claims. Rounds 3 and 4 changed only .md files, so the green npm run ci from commit 86fbdfc still covers the executable side; content-identical moves don't shift the per-file ≥90 gate.

Verdict: approve — merge-ready. Item 1 is a one-clause grammar fix, item 2 is optional polish; neither blocks. Four rounds in, the substantive work has held up under repeated re-verification: the moves are correct, the docs now describe the tree as it actually is (rather than as it will be after #1785), and the one-way lib → utils invariant remains the durable payoff — it's the only part of this a future reviewer can check with a single grep.
· branch v2/1776-lib-utils-rule

[cliffhall]

Thanks — both round-5 items addressed in c949218 (doc-only; no code changed since the last green npm run ci).

1. Garbled #1785 clause — fixed. You're right, "superseding [#1785] will remove it" was a botched merge of two phrasings. Now reads: "…the dead navigation.ts InspectorTab, which #1785 will remove; until then a pure domain type belongs in utils/, not src/types/." (AGENTS.md only — the README's parallel sentence was already clean.)

2. Over-accreted bullet — restructured (took the optional polish since I was in the line). The src/utils/ bullet had grown to a single ~1,400-char blob burying the actual rule under five review rounds' worth of parentheticals. Promoted the carve-outs to scannable sub-bullets — Domain types / Diagnostic logging / Importing from @inspector/core — and lifted the src/types/ note out to its own line, since (as you noted) it's a statement about a sibling directory, not about utils. The rule itself (pure in, pure out) now leads and the exceptions hang beneath it.

Appreciate the five rounds of re-verification — the import-graph and one-way-arrow greps in particular. I think it's genuinely merge-ready now.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 12s —— View job

---

Review: refactor(web): sharpen the src/lib vs src/utils rule (round 6)

• Read the round-5 commit + full diff
• Verify the two round-5 items landed
• Re-check import graph / stale paths repo-wide
• Audit final doc wording
• Post review

---

Both round-5 items landed, verified independently:

• Garbled #1785 clause — fixed. AGENTS.md:253 now reads "the dead navigation.ts InspectorTab, which [refactor(web): remove dead src/types/navigation.ts InspectorTab, fold into utils/inspectorTabs #1785] will remove; until then a pure domain type belongs in utils/, not src/types/." Parses.
• Bullet restructure — done, and done better than I suggested: the carve-outs are now three labelled sub-bullets (Domain types / Diagnostic logging / Importing from @inspector/core) and the src/types/ material was lifted out to its own paragraph between the src/lib/ bullet and the one-way-arrow rule. The rule now leads and the exceptions hang beneath it, which was the point.

Mechanical re-checks, all clean:

• All 882 relative specifiers under clients/web/src resolve on disk (static from, dynamic import(), and vi.mock()/vi.doMock() string literals — the last being what tsc structurally can't catch on a rename). 0 unresolved.
• Repo-wide grep for the six pre-move paths across every file extension (not just ts|tsx|md|html|json|mjs): one hit, AGENTS.md:249's intentional "removing lib/types/ is what the customHeaders move settles". Nothing else.
• One-way arrows hold: grep '\.\./lib' src/utils/ → nothing. Only cross edges are lib/oauthResume.ts:29,36,37 → utils/{inspectorTabs,oauthFlow,pendingReauth}, the last import type. Also confirmed src/utils/ imports nothing from components/ or hooks/ — the pure layer is a leaf.
• Env-marker scan of src/utils/ (non-test): 2 files hit, both doc-comment prose (oauthFlow.ts:18, sandbox-csp.ts:52,84,131-137). Of the 7 core imports left in utils/, 5 are import type and 2 are pure-value re-exports (oauthFlow.ts:7, oauthUx.ts:12) — exactly the two clauses the rule now names. Inverse scan of src/lib/: all 6 files carry DOM/storage/core-runtime markers, so nothing in lib is silently pure.
• Both anchor lists are now exhaustive, not illustrative: src/lib/'s six anchors are the six files in src/lib/. Nice property to have landed on.

One new finding. It's in a sentence this PR wrote, and it's the one sentence nobody has checked against the build config.

---

1. "no build/coverage glob" is not true — the coverage include enumerates both directories by name (should fix)

Both docs close on the same claim:

AGENTS.md:257 — Nothing keys off the boundary (no path alias, no build/coverage glob) …
clients/web/README.md:64 — Nothing keys off the boundary — it's a human-legible import-time signal.

The "no path alias" half is correct (I grepped every *.json/*.config.*/*.yml/*.mjs in the repo — no tsconfig paths, eslint, or tsup entry mentions either directory). The coverage half isn't:

// clients/web/vite.config.ts:159-164
include: [
  'src/components/**/*.{ts,tsx}',
  'src/lib/**/*.{ts,tsx}',     // ← names the boundary
  'src/utils/**/*.{ts,tsx}',   // ← names the boundary
  'src/lib/**/*.{ts,tsx}',     // ← and again (pre-existing dup, see below)
  'clients/web/server/**/*.{ts,tsx}',
  …core/*
],

The conclusion survives — both directories are included, so moving a file between them is coverage-neutral, and your green coverage run confirms it. But the stated reason is wrong in a way that matters for the doc's actual job ("when adding a module, place it by the rule"): that include is a whitelist, so directory placement absolutely does key off the ≥90 gate. Anything landing outside components/lib/utils/server is silently ungated — which today includes src/hooks/**, src/theme/**, src/types/**, and src/App.tsx itself. A reader who takes "no coverage glob" at face value and starts a third grab-bag directory gets no gate and no warning.

One clause fixes it, e.g.: "Nothing enforces the boundary (no path alias; the coverage include in vite.config.ts lists both directories, so a move between them is inert — but note it's a whitelist: a module outside components/lib/utils/server falls out of the ≥90 gate entirely)."

Fix this →

Adjacent (pre-existing, not yours): 'src/lib/**/*.{ts,tsx}' appears twice, at lines 161 and 163. Identical on origin/v2/main, and this PR doesn't touch vite.config.ts, so it's not a regression — vitest de-dupes glob matches so it's inert. But it's a one-line delete sitting three lines from the config the doc is describing, and you'd be in the file anyway if you take the wording fix.

---

Nits

• The README bullet didn't get round-5's restructure. AGENTS.md:248-251 is now three labelled sub-bullets; clients/web/README.md:62 still packs all five carve-outs (diagnostic logging, type-only import, pure re-export, domain types, no-types/-sub-bucket, src/types/, #1785) into one ~800-character sentence — the exact shape round 5 flagged in AGENTS.md. It's a condensed summary by design so it's defensible, but the two docs now differ in structure as well as length. Take or leave.
• clients/web/README.md:62 writes #1785 bare, where AGENTS.md:253 uses a full markdown link. GitHub only autolinks #N in issues/PRs/commits — not in rendered repository markdown — so the README's reference renders as plain text and dead-ends. Locally consistent though (README.md:22 already has a bare (#1769)), so this is repo-convention-level, not a defect.

---

Not verified

Same limitation as rounds 1–5: clients/web/node_modules isn't installed in this job, so no validate / coverage / tsc. Everything above is static — the 882-specifier resolution sweep (including mock literals), an all-extension repo-wide grep for the six pre-move paths, env-marker scans of both src/utils/ and src/lib/, an importer census for customHeaders and InspectorTab, and a grep of every config file in the repo for src/lib/src/utils/lib/types — which is how item 1 surfaced. Rounds 3–5 changed only .md, so the green npm run ci from 86fbdfc still covers the executable side; content-identical moves don't shift the per-file gate.

Verdict: approve. Item 1 is a wording fix to one clause (plus an optional one-line config dedupe) and doesn't block. Six rounds in, the code half has been stable since 86fbdfc and has survived repeated independent re-verification; the durable payoff remains the one-way lib → utils invariant, which a future reviewer can check with a single grep — and, as of this round, the exhaustive-anchor property, where both doc lists happen to name exactly the files in each directory.
· branch v2/1776-lib-utils-rule

[cliffhall]

Thanks — round-6 addressed in 2683a9d.

1. "no build/coverage glob" was factually wrong — fixed. Good catch, and it's the one sentence that mattered to get right since the doc's job is telling people where to put a new module. The web coverage include in vite.config.ts does enumerate src/lib/** and src/utils/**. Reworded both docs: nothing enforces the boundary (no path alias; the include lists both dirs, so a move between them is coverage-neutral — which is why the refactor was gate-safe), plus the whitelist caveat you flagged — a module placed outside components/lib/utils/server falls out of the ≥90 gate silently. That caveat is the actually-useful part for a reader adding a directory.

2. Duplicate glob — removed. Deleted the second 'src/lib/**/*.{ts,tsx}' (was at lines 161 and 163). Inert (vitest de-dupes), pre-existing on origin/v2/main, but as you said I was in the file for the wording fix's sake, and it makes the config match the doc. npm run ci re-run green with the config touched.

Nits — both done. Gave the README bullet the same sub-bullet restructure as AGENTS.md (Diagnostic logging / Importing from core / Domain types), lifted its src/types/ note to its own line, and made #1785 a real markdown link (agreed bare #N dead-ends in rendered repo markdown, even though README.md:22's (#1769) set a bare-ref precedent).

The exhaustive-anchor property you noted (each doc's anchor list = exactly the files in that dir) is a nice invariant — I'll try to keep it true as files come and go.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 6m 7s —— View job

---

Review: refactor(web): sharpen the src/lib vs src/utils rule (round 7)

• Read the round-6 commit + full diff
• Verify the round-6 fixes landed
• Re-check import graph / stale paths repo-wide
• Audit final doc wording against the config
• Post review

---

Round-6 items landed, and this round I could verify the coverage claim empirically rather than by reading the config — the CI logs for this branch carry the full v8 table, which settles the question the doc sentence is about.

• Coverage-glob wording — AGENTS.md:257 now says nothing enforces the boundary, that the include lists both dirs (hence gate-safe), plus the whitelist caveat. Confirmed against the last green run's coverage table (c949218, job 89741854007): rows exist for …ts/web/src/lib (98.42 / 98.83 / 100 / 98.36), …/web/src/utils (97.94 / 96.21 / 100 / 98.95) and …nts/web/server (99.57) — and there is no row for hooks, theme, types, or App.tsx. So both halves of the new sentence are true of the actual gate, including the whitelist caveat. (Worth noting the include mixes relative bases — src/** vs clients/web/server/** vs path.join(repoRoot, 'core/**') — and the table proves all three resolve, so the mixed style is harmless, just untidy.)
• Duplicate glob — removed; the pre-removal run's table has the same rows, so it was inert as expected.
• README restructure — now mirrors AGENTS.md's sub-bullets (Diagnostic logging / Importing from core / Pure domain types), src/types/ lifted to its own line, #1785 a real markdown link.

Mechanical re-checks, all clean: 830 relative specifiers under clients/web/src + server/ resolve on disk (static from, dynamic import(), vi.mock/vi.doMock literals); repo-wide grep for all six pre-move paths across every extension → nothing; grep '\.\./lib' src/utils/ → nothing (only cross edges are lib/oauthResume.ts:29,36,37, the last import type); env-marker scan of src/utils/ → 2 files, both doc-comment prose; every file in src/lib/ carries a DOM/storage/core-runtime marker; src/lib's six anchors are still exactly the six files in it.

Nothing blocking in the diff. Two things I found while verifying, both pre-existing and out of scope — reporting them because one of them cost this PR a CI run and the other is the live instance of the caveat the PR just documented.

---

1. The coverage gate is flaky — it failed a docs-only commit on this PR (pre-existing, worth an issue)

Run 30182348415 (commit 3fecb5c, round 4 — markdown only) failed the Enforce per-file coverage gate step. Not a coverage regression: 299 files / 4470 tests all passed, and no threshold was missed. It died on a single unhandled error:

Vitest caught 1 unhandled error during the test run.
ReferenceError: window is not defined
 ❯ resolveUpdatePriority  react-dom-client.development.js:1308:7
 ❯ dispatchSetState       react-dom-client.development.js:9126:14
 ❯ Timeout._onTimeout     @mantine/core/esm/components/Transition/use-transition.mjs:61:13
This error originated in "src/components/groups/ServerRemoveConfirmModal/ServerRemoveConfirmModal.test.tsx"

That's exactly the #1760 class AGENTS.md documents — a Mantine Transition timer firing after happy-dom tears down window. The attributed file is a red herring: ServerRemoveConfirmModal.test.tsx renders only through renderWithMantine (env="test", synchronous transitions), and vitest itself says the error merely fired while that file was running. The plausible source is the one opt-in escape hatch — ViewHeader.test.tsx uses renderWithMantineTransitions (env="default", real timers) in three places (lines 248, 319, 387). Each drives its assertions to completion, but a Transition that opens and is torn down before its timer resolves still leaves a pending setTimeout, which is precisely the leak the helper's own comment warns about.

Frequency: 1 of the 6 completed runs on this branch (the other five are clean of Unhandled Errors), so it's intermittent — the worst kind, since it makes a green run non-evidence. The mitigation isn't wrong, it's just incomplete: env="test" covers the default path, and the residual leak lives in the opt-out.

File an issue / investigate →

---

2. The new whitelist caveat has a live instance today: src/hooks/ is ungated, and two hooks have no tests at all

The sentence this PR added ("a module placed outside those directories … falls out of the ≥90 gate entirely, silently") isn't hypothetical. src/hooks/ holds 917 lines across four modules; the coverage table confirms none of them are reported. Two of the four have no test file:

src/hooks/usePaginatedList.ts      + usePaginatedList.test.tsx     ✓
src/hooks/useScrollMemory.ts       + useScrollMemory.test.tsx      ✓
src/hooks/useServerJsonImport.ts   (308 lines)  — no test, ungated
src/hooks/useImportClientConfig.ts              — no test, ungated

Both are real logic (config/JSON import parsing + wiring), consumed by ServerImportJsonModal / ServerImportConfigModal. They execute during those components' tests, so some of their lines are exercised — but because include never names src/hooks/**, nothing is measured and nothing is enforced. Two authors evidently already noticed enough to co-locate tests for the other two hooks; the gate just never asked.

That's a nice validation of the caveat you documented, and the natural follow-up: either add src/hooks/** to the coverage include (and bring the two untested hooks to ≥90), or state in the doc why hooks/ is deliberately out. Out of scope here — same reasoning you applied to customHeaders and navigation.ts.

File an issue →

---

Nit

ViewHeader.test.tsx:22 hand-rolls a bare MantineProvider (forced defaultColorScheme="dark" for a useComputedColorScheme assertion), which AGENTS.md's #1760 rule says never to do. It's benign — it passes env="test", so it doesn't reintroduce the leak class, and renderWithMantine has no way to express a forced color scheme. Either add a colorScheme option to the helper or note the carve-out in the rule; purely cosmetic either way, and unrelated to this PR.

---

Not verified

clients/web/node_modules is absent in this job (vite doesn't resolve — I tried a scoped vitest run --coverage and it failed at config load), so I still can't run validate / coverage / tsc locally. What's different this round: instead of reasoning about the coverage config statically, I read the actual v8 tables from this branch's CI runs, which is what confirmed both halves of the new doc sentence and turned up finding 1. Everything else is static — the 830-specifier resolution sweep, an all-extension grep for the six pre-move paths, env-marker scans of src/utils/ and src/lib/, and a config-wide grep for anything keying off the two directory names (only the two include lines). The head-of-branch run (30183123111, commit 2683a9d4) was still in progress when I posted; given finding 1, if it goes red, check whether it's the window is not defined flake before assuming a real regression.

Verdict: approve — merge-ready, nothing outstanding in the diff. Seven rounds in, the code has been unchanged since 86fbdfc and the docs now match the tree and the build config, the last of which I could finally check against a real gate run rather than by inference. Both findings above are pre-existing repo issues the verification surfaced, not PR defects; the flaky gate is the one I'd track soonest, since it silently converts "CI is green" into a coin flip.
· branch v2/1776-lib-utils-rule

[cliffhall]

Thanks — appreciate the empirical verification this round (reading the actual v8 tables from the branch's CI runs to confirm the coverage sentence, rather than inferring). Glad the wording now matches both the tree and the gate.

Both findings are pre-existing and out of scope for this PR, as you noted — so rather than churn the diff, I've tracked them:

• Finding 1 (flaky coverage gate, Flaky test teardown: Mantine Transition timer fires after happy-dom window teardown (window is not defined) #1760-class Transition timer leak) → test(web): flaky coverage gate — Mantine Transition timer leaks 'window is not defined' after teardown (#1760 class) #1786 (board, Todo). Captured your diagnosis: the ServerRemoveConfirmModal attribution is a red herring and the likely source is ViewHeader.test.tsx's three renderWithMantineTransitions (env="default", real timers) call sites leaving a pending setTimeout. Folded the ViewHeader.test.tsx:22 bare-MantineProvider nit into that issue too (add a colorScheme option to renderWithMantine, or note the carve-out), since it's the same file/area.
• Finding 2 (src/hooks/ ungated; useServerJsonImport + useImportClientConfig untested) → test(web): src/hooks/ is outside the coverage gate — two hooks (useServerJsonImport, useImportClientConfig) have no tests #1787 (board, Todo). It really is the live instance of the caveat this PR just documented — nice. Also noted your include-base-normalization observation there.

That leaves nothing outstanding in this PR's diff — the code has been stable since 86fbdfc and the docs now match the tree and the build config. I'll leave the branch here as merge-ready.