From f5acb60cac513cbc2534ac4046f7c8f1b15187a3 Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Tue, 2 Jun 2026 13:44:25 -0300 Subject: [PATCH] chore(mitm): fix stub comment accuracy + broaden stub-drift guard (PR review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses findings from the multi-agent PR review of the #3066 fix: - manager.stub.ts comments: the previous inline comment claimed the throwing ops (getMitmStatus/startMitm/stopMitm) are "dynamic-import paths that should never hit the stub at runtime" — factually wrong: those are static imports too, baked into the bundled build just like getAllAgentsStatus. Rewrote the file header to describe the real split — exports with a safe degraded value return it (getCachedPassword/ setCachedPassword/clearCachedPassword → null/no-op, getAllAgentsStatus → []) while getMitmStatus/startMitm/stopMitm throw STUB_ERROR — and trimmed the inline comment. Comment-only; no runtime/build change (the export still exists). - stub-drift guard test: now scans ALL of src/ instead of only src/app — src/lib/tailscaleTunnel.ts statically imports getCachedPassword/setCachedPassword from @/mitm/manager and is pulled into routes transitively, so the src/app-only scan had a false-negative blind spot. Also skips inline `type` imports (erased at build, need no runtime export) and detects stub exports from declaration AND `export { … }` forms (no false-positive if the stub later uses class/re-export). Verified: next-config suite 4/4, typecheck:core / lint clean. --- src/mitm/manager.stub.ts | 23 +++++++++++----------- tests/unit/next-config.test.ts | 36 +++++++++++++++++++++++----------- 2 files changed, 37 insertions(+), 22 deletions(-) diff --git a/src/mitm/manager.stub.ts b/src/mitm/manager.stub.ts index bb3193f462..08ffee4e84 100644 --- a/src/mitm/manager.stub.ts +++ b/src/mitm/manager.stub.ts @@ -1,7 +1,12 @@ -// Build-time stub for @/mitm/manager -// Used by Turbopack during next build to avoid native module resolution errors. -// Dynamic import() in route handlers should load the REAL manager at runtime. -// If this stub is reached at runtime, the build alias is incorrectly applied. +// Build-time stub for @/mitm/manager, aliased in by Turbopack during `next build` +// (the Docker image build) so native MITM modules aren't bundled. Routes that +// *statically* import @/mitm/manager get this stub baked in and may reach it at +// runtime in the bundled/container build. Exports that have a safe degraded value +// return it (getCachedPassword/setCachedPassword/clearCachedPassword → null/no-op, +// getAllAgentsStatus → empty list) because MITM needs host access the container +// lacks; getMitmStatus/startMitm/stopMitm throw STUB_ERROR since they can't return +// anything meaningful without the real MITM process. Routes that need real MITM at +// runtime dynamic-import @/mitm/manager.runtime (the real module) instead. const STUB_ERROR = "MITM manager stub reached at runtime — build alias applied incorrectly. " + @@ -13,13 +18,9 @@ export const clearCachedPassword = () => {}; export const getMitmStatus = async () => { throw new Error(STUB_ERROR); }; -// Statically imported by /api/tools/agent-bridge/state, so the stub MUST export it or -// the Turbopack build fails ("Export getAllAgentsStatus doesn't exist"). Unlike the -// heavy ops above (dynamic-import paths that should never hit the stub at runtime), a -// static import IS baked into the bundled build, so this is legitimately reached at -// runtime there — it returns the truthful "no agents" state (an empty list) instead of -// throwing. MITM/agent bridge needs host-level access and is otherwise non-functional -// in a bundled/container build. See issue #3066. +// Must be exported or the Turbopack build fails ("Export getAllAgentsStatus doesn't +// exist") — /api/tools/agent-bridge/state imports it statically. Returns the truthful +// empty agent list in the bundled build rather than throwing (see file header). See #3066. export const getAllAgentsStatus = (): never[] => []; export const startMitm = async ( _apiKey: string, diff --git a/tests/unit/next-config.test.ts b/tests/unit/next-config.test.ts index a6f1869a4f..475735d936 100644 --- a/tests/unit/next-config.test.ts +++ b/tests/unit/next-config.test.ts @@ -109,10 +109,13 @@ test("next config declares Turbopack aliases, runtime assets and server external test("manager.stub.ts exports every name statically imported from @/mitm/manager", async () => { const fs = await import("node:fs"); - const appDir = path.join(process.cwd(), "src", "app"); + const srcDir = path.join(process.cwd(), "src"); - // Collect named imports from `... from "@/mitm/manager"` (NOT manager.runtime, which - // is loaded via dynamic import() and resolves to the real module at runtime). + // Collect value names imported via `... from "@/mitm/manager"` across ALL of src/ — + // not just src/app: src/lib/tailscaleTunnel.ts imports from it and is pulled into + // routes transitively, so a src/app-only scan would miss that surface. NOT + // manager.runtime (loaded via dynamic import(), resolves to the real module at + // runtime). Inline `type` imports are erased at build time and need no stub export. const collectImports = (dir: string, acc: Set): void => { for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { const full = path.join(dir, entry.name); @@ -126,7 +129,9 @@ test("manager.stub.ts exports every name statically imported from @/mitm/manager let m: RegExpExecArray | null; while ((m = re.exec(src)) !== null) { for (const raw of m[1].split(",")) { - const name = raw.trim().split(/\s+as\s+/)[0].trim(); + const token = raw.trim(); + if (!token || /^type\s/.test(token)) continue; // type-only import: no runtime export needed + const name = token.split(/\s+as\s+/)[0].trim(); if (name) acc.add(name); } } @@ -134,22 +139,31 @@ test("manager.stub.ts exports every name statically imported from @/mitm/manager }; const imported = new Set(); - collectImports(appDir, imported); + collectImports(srcDir, imported); // Sanity: the guard is meaningless if the scan finds nothing to check. Kept generic // (>= 1 import) rather than asserting a specific symbol, so the test stays valid if any // single agent-bridge/traffic-inspector route is later renamed or removed. - assert.ok(imported.size > 0, "expected at least one static @/mitm/manager import in src/app"); + assert.ok(imported.size > 0, "expected at least one static @/mitm/manager import in src/"); const stubSrc = fs.readFileSync( path.join(process.cwd(), "src", "mitm", "manager.stub.ts"), "utf-8" ); - const stubExports = new Set( - [...stubSrc.matchAll(/export\s+(?:const|function|async\s+function)\s+([A-Za-z0-9_]+)/g)].map( - (m) => m[1] - ) - ); + // Collect stub exports from both declaration forms and named re-export blocks so the + // guard doesn't false-positive if the stub later uses `export class` / `export { … }`. + const stubExports = new Set(); + for (const m of stubSrc.matchAll( + /export\s+(?:const|let|var|class|function|async\s+function)\s+([A-Za-z0-9_]+)/g + )) { + stubExports.add(m[1]); + } + for (const m of stubSrc.matchAll(/export\s*\{([^}]*)\}/g)) { + for (const part of m[1].split(",")) { + const exported = part.trim().split(/\s+as\s+/).pop()?.trim(); // `x as y` exports y + if (exported) stubExports.add(exported); + } + } const missing = [...imported].filter((name) => !stubExports.has(name)); assert.deepEqual(