* refactor(web): sharpen src/lib vs src/utils rule and move misfiled modules (#1776) Codify the empirical rule the web client already followed loosely — utils = pure functions that compute; lib = things that instantiate, adapt, or touch the environment — in AGENTS.md and clients/web/README.md, and move the modules that sat on the wrong side of it. Moves (source + co-located test): - lib/sandbox-csp -> utils (pure CSP-string construction; type-only import) - utils/browserTabVisibility -> lib (registers visibilitychange listeners) - utils/oauthResume -> lib (reads/writes sessionStorage) - utils/clearServerOAuthState -> lib (drives live InspectorClient / OAuthStorage) Borderline audits (kept in place): pendingReauth (pure type declarations), deepLink / correlateTransportErrors / mcpNetworkHeaders / oauthUx / oauthFlow / oauthNetworkPhase (pure transforms / type-only core imports) stay in utils; downloadFile stays whole in lib (dominant side is DOM side effects). Also relocate the lone src/test/lib/downloadFile.test.ts straggler beside its source at src/lib/downloadFile.test.ts, and document the test-placement rule (side-by-side by default; src/test/ only for core/ tests, the integration project, and shared scaffolding). Pure move — no behavior change; no build/coverage config keys off the boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * refactor(web): address review — fix stale paths, move customHeaders, one-way arrows (#1776) Responds to the @claude review on PR #1784: 1. Stale path references (fixed): update the four in-repo pointers left at pre-move paths — specification/v2_auth_mid_session.md (oauthResume, browserTabVisibility → lib/), and the two load-bearing sandbox security comments (sandbox_proxy.html, sandbox-controller.ts → src/utils/sandbox-csp). 2. lib/types/customHeaders (moved): it's pure (types + pure transforms), the rule's clearest counterexample. Moved lib/types/customHeaders.{ts,test.ts} → utils/ (no external importers; only its own test) and removed the undocumented lib/types/ subdir. Documented that utils/ is the home for pure shared domain types + their constructors — no separate types/ bucket. 3. One-way arrows (item 3): declare OAuthResumeAuthKind in utils/pendingReauth and re-export it from lib/oauthResume, so the only cross-directory arrow now points lib → utils. Codified that invariant in AGENTS.md. Wording nit: AGENTS.md + README now state diagnostic logging (console.warn/error) doesn't count as a side effect for the utils rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 2 — stale customHeaders refs, rule wording (#1776) Doc-only follow-up to the round-2 @claude review on PR #1784 (no code changed): 1. Stale paths (same class as round-1 item 1): the customHeaders move left three references to the deleted lib/types/customHeaders.ts path in specification/ (v2_ux_interfaces_phase_0_4_inventory.md, v2_ux_interfaces_plan.md ×2) — updated to utils/customHeaders.ts. 2. customHeaders anchor: note in AGENTS.md that its shape is staged for ServerSettingsForm (per v2_ux_interfaces_plan.md), so its zero-importer state reads as intentional rather than rot; kept as the no-types/-bucket illustration and led the anchor list with load-bearing modules. 3. Type-only-import clause: split the conflated clause — 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. Fixed in both AGENTS.md and the web README. Cosmetic: re-padded the v2_auth_mid_session.md table row to column width. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 3 — scope the "no types/ bucket" claim (#1776) Doc-only follow-up to the round-3 @claude review on PR #1784 (no code changed): 1. "no separate types/ bucket" overreached: clients/web/src/types/ does exist (navigation.ts + react-syntax-highlighter .d.ts ambient stubs wired via tsconfig.app.json paths). Scoped the claim in both AGENTS.md and the web README to "no types/ sub-bucket INSIDE lib/utils", and noted src/types/ is for ambient .d.ts stubs, not domain types. 2. README example list drifted from AGENTS.md's round-2 load-bearing anchors — realigned it (inspectorTabs/mcpNetworkHeaders, dropped the zero-importer customHeaders from the headline list). 3. Clarified v2_ux_interfaces_plan.md:185 so the v2 path reads as the copy's destination, not as the v1.5 source location. Follow-up filed as #1785 (board Todo): src/types/navigation.ts InspectorTab is dead and duplicates utils/inspectorTabs.ts — kept out of this pure-move PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 4 — reconcile src/types/ claim with #1785 (#1776) Doc-only follow-up to the round-4 @claude review on PR #1784 (no code changed): The round-3 scope fix overshot: it asserted src/types/ holds "not domain types at all", but the dead navigation.ts InspectorTab is a plain-.ts domain type still living there, and specification/v2_ux_interfaces_plan.md:181-182 actively prescribes placing InspectorTab there — so AGENTS.md and that spec doc gave opposite guidance about the same directory. - AGENTS.md + web 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 #1785; until then a pure domain type belongs in utils/, not src/types/. - v2_ux_interfaces_plan.md:181: marked the tab-enum placement guidance as superseded by #1785 (live type is InspectorTabId in utils/inspectorTabs.ts). Self-heals on merge of #1785. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 5 — fix garbled #1785 clause, restructure utils rule (#1776) Doc-only follow-up to the round-5 @claude review on PR #1784 (no code changed): 1. Grammar (item 1): the round-4 #1785 clause read "superseding [#1785] will remove it" — a participle with no subject. Reworded to "the dead navigation.ts InspectorTab, which #1785 will remove". 2. Readability (item 2): the src/utils/ bullet had accreted into a single ~1,400-char blob across five review rounds. Promoted the carve-outs to scannable sub-bullets (Domain types / Diagnostic logging / Importing from core) and lifted the src/types/ note out to its own line, since it's a statement about a sibling directory, not about utils. AGENTS.md only; the README's parallel sentence was already clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs+config: address review round 6 — correct the coverage-glob claim (#1776) Responds to the round-6 @claude review on PR #1784: 1. "no build/coverage glob" was factually wrong (should-fix): the web coverage include in clients/web/vite.config.ts DOES enumerate src/lib/** and src/utils/**. The conclusion (a move between them is coverage-neutral) survives, but the reason was wrong. Reworded both AGENTS.md and the web README to say nothing *enforces* the boundary — no path alias, and the include lists both dirs so a move is coverage-neutral — plus the important whitelist caveat: a module outside components/lib/utils/server falls out of the ≥90 gate entirely. 2. Removed a pre-existing duplicate glob in vite.config.ts (src/lib/**/*.{ts,tsx} appeared twice, lines 161 and 163). Inert (vitest de-dupes) but it sat three lines from the config the doc now describes. Nits: gave the README bullet the same round-5 sub-bullet restructure as AGENTS.md, and made its #1785 reference a real markdown link (bare #N doesn't autolink in rendered repo markdown). npm run ci green (config change re-verified through coverage + smokes + Storybook). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
156 lines
5.6 KiB
TypeScript
156 lines
5.6 KiB
TypeScript
/**
|
|
* Sandbox server controller: start/close and get URL.
|
|
* Used by server.ts (prod) and the Vite plugin (dev/test). Same process lifecycle as the main server.
|
|
*/
|
|
|
|
import { createServer, type Server } from "node:http";
|
|
import { readFileSync } from "node:fs";
|
|
import { dirname, join } from "node:path";
|
|
import { fileURLToPath } from "node:url";
|
|
|
|
const __dirname = dirname(fileURLToPath(import.meta.url));
|
|
|
|
export interface SandboxControllerOptions {
|
|
/** Port to bind (0 = dynamic). */
|
|
port: number;
|
|
/** Host to bind (default localhost). */
|
|
host?: string;
|
|
}
|
|
|
|
export interface SandboxController {
|
|
start(): Promise<{ port: number; url: string }>;
|
|
close(): Promise<void>;
|
|
getUrl(): string | null;
|
|
}
|
|
|
|
/**
|
|
* Resolve sandbox port from env: MCP_SANDBOX_PORT → SERVER_PORT → 0 (dynamic).
|
|
*/
|
|
export function resolveSandboxPort(): number {
|
|
const fromSandbox = process.env.MCP_SANDBOX_PORT;
|
|
if (fromSandbox !== undefined && fromSandbox !== "") {
|
|
const n = parseInt(fromSandbox, 10);
|
|
if (!Number.isNaN(n) && n >= 0) return n;
|
|
}
|
|
const fromServer = process.env.SERVER_PORT;
|
|
if (fromServer !== undefined && fromServer !== "") {
|
|
const n = parseInt(fromServer, 10);
|
|
if (!Number.isNaN(n) && n >= 0) return n;
|
|
}
|
|
return 0;
|
|
}
|
|
|
|
export function createSandboxController(
|
|
options: SandboxControllerOptions,
|
|
): SandboxController {
|
|
const { port, host = "localhost" } = options;
|
|
let server: Server | null = null;
|
|
let sandboxUrl: string | null = null;
|
|
|
|
// Defense-in-depth for the proxy page itself. Only `frame-ancestors` is set
|
|
// here — fetch directives (`default-src`, `connect-src`, etc.) are
|
|
// deliberately omitted because a `srcdoc` iframe clones its embedder's CSP
|
|
// policy container: any fetch directive on this header would be inherited by
|
|
// the inner app document and, since multiple CSPs intersect, would override
|
|
// the per-app `connect-src`/`img-src` allowlists the host bakes into the
|
|
// wrapped HTML (see src/utils/sandbox-csp.ts). The opaque-origin sandbox on
|
|
// the inner frame is the structural boundary; `frame-ancestors` ensures the
|
|
// proxy can only be embedded by the local inspector itself.
|
|
const SANDBOX_PROXY_CSP =
|
|
"frame-ancestors http://127.0.0.1:* http://localhost:*";
|
|
|
|
let sandboxHtml: string;
|
|
try {
|
|
const sandboxHtmlPath = join(__dirname, "../static/sandbox_proxy.html");
|
|
sandboxHtml = readFileSync(sandboxHtmlPath, "utf-8");
|
|
} catch (e) {
|
|
sandboxHtml =
|
|
"<!DOCTYPE html><html><body>Sandbox not loaded: " +
|
|
String((e as Error).message) +
|
|
"</body></html>";
|
|
}
|
|
|
|
return {
|
|
async start(): Promise<{ port: number; url: string }> {
|
|
if (server && sandboxUrl) {
|
|
const p = parseInt(new URL(sandboxUrl).port, 10);
|
|
return { port: p, url: sandboxUrl };
|
|
}
|
|
return new Promise((resolve) => {
|
|
// Guard so a `listen` error followed by a (theoretically possible)
|
|
// late `listening` callback doesn't double-resolve the promise. The
|
|
// first signal wins.
|
|
let settled = false;
|
|
const settle = (value: { port: number; url: string }) => {
|
|
/* v8 ignore next -- defensive double-settle guard: for a TCP listen
|
|
the `error` and `listening` events are mutually exclusive, so the
|
|
second-signal early-return is unreachable in practice. */
|
|
if (settled) return;
|
|
settled = true;
|
|
resolve(value);
|
|
};
|
|
|
|
server = createServer((req, res) => {
|
|
if (
|
|
req.method !== "GET" ||
|
|
(req.url !== "/sandbox" && req.url !== "/sandbox/")
|
|
) {
|
|
res.writeHead(404, { "Content-Type": "text/plain" });
|
|
res.end("Not Found");
|
|
return;
|
|
}
|
|
res.writeHead(200, {
|
|
"Content-Type": "text/html; charset=utf-8",
|
|
"Cache-Control": "no-store, no-cache, must-revalidate",
|
|
Pragma: "no-cache",
|
|
"Content-Security-Policy": SANDBOX_PROXY_CSP,
|
|
});
|
|
res.end(sandboxHtml);
|
|
});
|
|
server.on("error", (err: NodeJS.ErrnoException) => {
|
|
if (err.code === "EADDRINUSE") {
|
|
console.error(
|
|
`Sandbox: port ${port || "dynamic"} in use. MCP Apps tab may not work.`,
|
|
);
|
|
} else {
|
|
console.error("Sandbox server error:", err);
|
|
}
|
|
// Best-effort degradation: resolve with empty values rather than
|
|
// rejecting so callers (the Vite plugin in particular) keep
|
|
// attaching `/api/*`. `sandboxUrl` stays null, `getUrl()` keeps
|
|
// returning null, and the banner omits the sandbox line.
|
|
server = null;
|
|
settle({ port: 0, url: "" });
|
|
});
|
|
server.listen(port, host, () => {
|
|
const addr = server!.address();
|
|
const actualPort =
|
|
typeof addr === "object" && addr !== null && "port" in addr
|
|
? addr.port
|
|
: /* v8 ignore next -- unreachable for a TCP listen: `address()`
|
|
always returns an `AddressInfo` object with `port`. The
|
|
string form only occurs for unix-socket/pipe listens, which
|
|
this controller never performs. */
|
|
(addr as unknown as number);
|
|
sandboxUrl = `http://${host}:${actualPort}/sandbox`;
|
|
settle({ port: actualPort, url: sandboxUrl });
|
|
});
|
|
});
|
|
},
|
|
|
|
async close(): Promise<void> {
|
|
if (!server) return;
|
|
return new Promise((resolve) => {
|
|
server!.close(() => {
|
|
server = null;
|
|
sandboxUrl = null;
|
|
resolve();
|
|
});
|
|
});
|
|
},
|
|
|
|
getUrl(): string | null {
|
|
return sandboxUrl;
|
|
},
|
|
};
|
|
}
|