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

Summary

clients/web/src/lib/ and clients/web/src/utils/ are both grab-bags whose names are industry-ambiguous. Empirically this codebase does follow a rule — utils/ = pure helpers, lib/ = plumbing — but it's undocumented and applied loosely, so a handful of modules sit on the wrong side of it. This issue sharpens the rule, writes it into AGENTS.md, and moves the misfiled modules.

The rule to codify

• src/utils/ — pure, side-effect-free functions. Input → output, no DOM/browser/storage I/O, no subsystem ownership. Trivially unit-testable with no mocks. (Anchors: jsonUtils, schemaUtils, toolUtils, maskSecrets, inspectorTabs.)
• src/lib/ — infrastructure / integration / stateful adapters. Modules that instantiate or compose subsystems, wrap @inspector/core, touch the DOM / window / sessionStorage, or otherwise produce side effects. (Anchors: environmentFactory composes InspectorClientEnvironment; remoteOAuthStorage is an adapter class over core/auth.)

One-liner for AGENTS.md: utils = functions that compute; lib = things that instantiate, adapt, or touch the environment. If it does I/O or wraps a subsystem, it's lib; if it's a pure transform, it's utils.

Confirmed misfiled (verified by reading the source)

• src/lib/sandbox-csp.ts → src/utils/ — pure CSP-string construction from an untrusted _meta.ui.csp; no side effects (only a type import from ext-apps).
• src/utils/browserTabVisibility.ts → src/lib/ — reads document.visibilityState and registers visibilitychange listeners (DOM side effects).
• src/utils/oauthResume.ts → src/lib/ — reads/writes window.sessionStorage (stateful I/O).

Move each module's co-located *.test.ts with it.

Borderline — audit case-by-case during the move

A coarse grep for sessionStorage / document. / window. / @inspector/core flags these, but several are false positives (type-only imports, guarded typeof document SSR checks, pure URL parsing). Read each and place per the rule:

• src/utils/pendingReauth.ts, src/utils/clearServerOAuthState.ts — likely sessionStorage I/O → probably lib.
• src/utils/deepLink.ts, src/utils/correlateTransportErrors.ts, src/utils/mcpNetworkHeaders.ts, src/utils/oauthUx.ts, src/utils/oauthFlow.ts, src/utils/oauthNetworkPhase.ts — likely pure (type-only core imports / URL parsing) → probably stay utils.
• src/lib/downloadFile.ts — mixed: the headline downloadBlob / downloadJsonFile are DOM side effects (stays lib), but the pure helpers it also exports (buildExportFilename, fileNameFromUri, isHttpUrl) are utils-shaped. Decide whether to split or leave whole (leaving whole in lib is acceptable).

Related straggler (in scope, cheap)

src/test/lib/downloadFile.test.ts breaks the side-by-side test convention — its three src/lib/ siblings (environmentFactory.test.ts, remoteOAuthStorage.test.ts, sandbox-csp.test.ts) are co-located with their source. Move it to src/lib/downloadFile.test.ts (or src/utils/ if downloadFile is reclassified). This is the only web-owned test currently living under src/test/ instead of beside its source.

The test-placement rule to codify (why src/test/ exists at all)

While we're documenting structure, capture the companion rule that the downloadFile.test.ts straggler violates, because it looks contradictory until spelled out: most tests live beside their source, yet there's also a src/test/ tree. These aren't competing conventions — src/test/ is specifically the home for tests whose source lives outside clients/web/src, plus shared test scaffolding. The rule:

• Side-by-side (<Name>.test.tsx next to the source) — the default for web's own src/ code. Components, hooks, lib/, utils/. This is the overwhelming majority.
• src/test/ — three things that cannot be co-located:
  1. Tests of the repo-root core/ package (src/test/core/…, ~99 files, mirroring the core/ folder layout). core/ physically lives at /core, outside clients/web/, is consumed via the @inspector/core alias, and has no package.json or test harness of its own. Its browser-facing behavior is tested here rather than in core/, because co-locating would pollute the shared, isomorphic package with web-only test infra (happy-dom, renderWithMantine, the web vitest config).
  2. The integration vitest project (src/test/integration/…, ~49 files, node env, 30s timeouts). Placement in that folder is the manifest — the project's glob keys off the path — and it mostly exercises core/ (transports, auth, storage) over real test servers.
  3. Shared test infrastructure — renderWithMantine.tsx, setup.ts, fixtures/, scrollAreaStoryAssertions.ts. Not tests of a source file, so there is nothing to sit beside.

One-liner for AGENTS.md: tests live beside their source, except tests for the repo-root core/ package (which lives outside clients/web/) and shared test scaffolding — both of which live under src/test/, with core/ tests mirroring the core/ layout and integration tests under src/test/integration/. (AGENTS.md already documents the core/-tests-under-src/test/core/ and integration-folder-is-the-manifest pieces; this makes the side-by-side-vs-src/test/ split itself explicit, so downloadFile.test.ts-style stragglers are unambiguous going forward.)

Acceptance

• src/lib vs src/utils rule documented in AGENTS.md (React/structure section) and, if useful, clients/web/README.md.
• Test-placement rule (side-by-side vs src/test/, and why) documented in AGENTS.md.
• Confirmed-misfiled modules moved (source + co-located test), imports updated.
• Borderline files audited and placed with a one-line rationale in the PR.
• downloadFile.test.ts relocated beside its source.
• npm run ci green (this is a pure move — no behavior change; coverage globs in vite.config.ts don't key off lib/utils, so no config change expected).

Notes

Nothing depends on the boundary — no path alias, no build/coverage config globs off lib vs utils — so this is a mechanical, behavior-preserving refactor. The alternative (collapse both into one dir) was considered and rejected: the pure-vs-plumbing signal is genuinely useful at import time in a codebase this test-heavy (90% per-file gate), and only ~3–4 files are actually misplaced.