fix(agent-bridge): surface real MITM startup-failure cause, not always port 443 (#3606) (#3608)

Integrated into release/v3.8.21 (#3606)

CHANGELOG.md

@@ -18,6 +18,7 @@
 - **security(oauth):** migrate the five public OAuth client_ids (Claude, Codex, Qwen, Kimi, GitHub Copilot — 9 server-side call-sites in `providerRegistry.ts` + `oauth.ts`) from string literals to `resolvePublicCred()` (Hard Rule #11), matching the existing Gemini/Antigravity pattern. The values decode byte-for-byte to the same public client_ids (env overrides still win), so OAuth flows are unchanged; the `check-public-creds` allowlist is now empty. The browser-bundled `codexDeviceFlow.ts` copy stays a literal by necessity (it cannot import `open-sse`). ([#3493](https://github.com/diegosouzapw/OmniRoute/issues/3493))
 - **fix(mcp):** `omniroute --mcp` no longer crashes on npm installs with `ERR_MODULE_NOT_FOUND` (e.g. `src/lib/combos/steps.ts`) — the MCP server runs from raw TypeScript and imports across `src/` + `open-sse/`, but the published `files` allowlist only shipped a handful of cherry-picked paths, so the transitive closure (~400 files) was absent from the tarball. `files` now ships the backend source the MCP server needs (`open-sse/` + `src/{domain,lib,mitm,server,shared,sse,types}/`, excluding the `src/app` UI), and a new regression test computes the MCP import closure and fails if any reachable source file is not covered by `files`. ([#3578](https://github.com/diegosouzapw/OmniRoute/issues/3578))
 - **fix(api):** `API_REFERENCE.md` no longer documents a non-existent `/api/guardrails*` / `/api/shadow*` surface (doc-fiction flagged by `check-docs-symbols`, frozen in `KNOWN_STALE_DOC_REFS`). The guardrail pipeline is real (`src/lib/guardrails`), so the two routes that map to actual behavior are now implemented — `GET /api/guardrails` (list the registered guardrails + status) and `POST /api/guardrails/test` (dry-run the pre-call pipeline over a sample input), both management-scoped — while the fictional `enable`/`disable`/`logs` rows and the entire `/api/shadow*` table (shadow A-B comparison is combo-config + `/api/combos/metrics`) were removed from the doc and dropped from the allowlist. ([#3496](https://github.com/diegosouzapw/OmniRoute/issues/3496))
+- **fix(agent-bridge):** the MITM "Start" button no longer reports a misleading "port 443 may be in use" for every failure cause — `startMitm()` only matched the EADDRINUSE stderr line and always threw the port-443 message, so a missing `ROUTER_API_KEY` or an `EACCES` permission error sent users debugging the wrong thing. The startup watcher now buffers the MITM child's stderr and `interpretMitmStartupError()` maps the real `server.cjs` `❌` cause (port-in-use / permission-denied / missing API key / any other diagnostic line) into the surfaced error; with no captured output it stays generic instead of guessing port 443. ([#3606](https://github.com/diegosouzapw/OmniRoute/issues/3606))
 
 ---
 

src/mitm/manager.ts

@@ -16,6 +16,42 @@ import { createLogger } from "@/shared/utils/logger.ts";
 
 const log = createLogger("mitm-manager");
 
+/**
+ * Map the MITM child process (`server.cjs`) stderr to the actual startup-failure
+ * cause. `server.cjs` emits one of several "❌"-prefixed lines on `server.on("error")`
+ * or on a missing API key, then exits. The old code only matched EADDRINUSE and so
+ * always blamed "port 443", misleading users whose real problem was a permission
+ * error or a missing ROUTER_API_KEY (#3606). The returned string is a controlled,
+ * secret-free diagnostic (it carries no stack and no credentials). (#3606)
+ */
+export function interpretMitmStartupError(stderr: string, port: number): string {
+  const text = (stderr || "").trim();
+  const lower = text.toLowerCase();
+
+  if (lower.includes("already in use")) {
+    return `MITM server failed to start: port ${port} is already in use`;
+  }
+  if (lower.includes("permission denied")) {
+    return `MITM server failed to start: permission denied for port ${port} (run with elevated privileges, or use a port ≥ 1024)`;
+  }
+  if (lower.includes("router_api_key")) {
+    return "MITM server failed to start: no API key was provided (ROUTER_API_KEY is required). Set a router API key in OmniRoute and retry.";
+  }
+
+  // Surface the first "❌ <message>" diagnostic line verbatim (marker stripped),
+  // so any other server.cjs failure is reported with its real cause.
+  for (const line of text.split(/\r?\n/)) {
+    const trimmed = line.trim();
+    if (trimmed.startsWith("❌")) {
+      const detail = trimmed.replace(/^❌\s*/, "").trim();
+      if (detail) return `MITM server failed to start: ${detail}`;
+    }
+  }
+
+  // Nothing diagnostic was captured — stay generic instead of guessing port 443.
+  return "MITM server failed to start (no diagnostic output was captured from the MITM server)";
+}
+
 // Store server process
 let serverProcess: ChildProcess | null = null;
 let serverPid: number | null = null;
@@ -314,13 +350,19 @@ export async function startMitm(
     fs.writeFileSync(PID_FILE, String(serverPid));
   }
 
+  // Buffer recent stderr so a startup failure can be reported with its real
+  // cause (capped to avoid unbounded growth on a chatty/looping process). (#3606)
+  let stderrBuffer = "";
+
   // Log server output
   proc.stdout?.on("data", (data) => {
     log.info({ source: "mitm-server" }, data.toString().trim());
   });
 
   proc.stderr?.on("data", (data) => {
-    log.error({ source: "mitm-server" }, data.toString().trim());
+    const chunk = data.toString();
+    stderrBuffer = (stderrBuffer + chunk).slice(-4000);
+    log.error({ source: "mitm-server" }, chunk.trim());
   });
 
   proc.on("exit", (code) => {
@@ -354,10 +396,11 @@ export async function startMitm(
       }
     });
 
-    // Check stderr for error messages
+    // Fail fast on any "❌" diagnostic line from server.cjs (covers EADDRINUSE,
+    // EACCES, missing ROUTER_API_KEY, and any other server.on("error") cause).
     proc.stderr?.on("data", (data) => {
-      const msg = data.toString().trim();
-      if (msg.includes("Port") && msg.includes("already in use")) {
+      const msg = data.toString();
+      if (msg.includes("❌")) {
         clearTimeout(timeout);
         if (!resolved) {
           resolved = true;
@@ -368,7 +411,7 @@ export async function startMitm(
   });
 
   if (!started) {
-    throw new Error("MITM server failed to start (port 443 may be in use)");
+    throw new Error(interpretMitmStartupError(stderrBuffer, port));
   }
 
   return {

tests/unit/mitm-startup-error-3606.test.ts

@@ -0,0 +1,49 @@
+/**
+ * Regression test for #3606 — MITM startup failure message is misleading.
+ *
+ * `startMitm()` used to always throw "port 443 may be in use" regardless of the
+ * real cause, because the stderr parser only matched EADDRINUSE. `server.cjs`
+ * already distinguishes EADDRINUSE / EACCES / missing-ROUTER_API_KEY / other on
+ * stderr (each prefixed with "❌"). `interpretMitmStartupError()` now maps the
+ * captured stderr to the actual cause so the user is not sent debugging port 443
+ * when the real problem is a missing API key or a permission error.
+ */
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { interpretMitmStartupError } from "../../src/mitm/manager.ts";
+
+test("EADDRINUSE stderr still reports the port-in-use cause", () => {
+  const msg = interpretMitmStartupError("❌ Port 443 already in use", 443);
+  assert.match(msg, /443/);
+  assert.match(msg, /in use/i);
+});
+
+test("EACCES stderr reports a permission cause, not port-in-use", () => {
+  const msg = interpretMitmStartupError("❌ Permission denied for port 443", 443);
+  assert.match(msg, /permission/i);
+  assert.doesNotMatch(msg, /in use/i);
+});
+
+test("missing ROUTER_API_KEY stderr reports the API-key cause, not port-in-use", () => {
+  const msg = interpretMitmStartupError("❌ ROUTER_API_KEY required", 443);
+  assert.match(msg, /ROUTER_API_KEY|API key/i);
+  assert.doesNotMatch(msg, /in use/i);
+});
+
+test("an arbitrary ❌ error line is surfaced verbatim (without the marker)", () => {
+  const msg = interpretMitmStartupError("some log\n❌ ENOENT: server.cjs missing\nmore log", 8443);
+  assert.match(msg, /ENOENT: server\.cjs missing/);
+  assert.doesNotMatch(msg, /❌/);
+});
+
+test("respects a non-default port in the port-in-use message", () => {
+  const msg = interpretMitmStartupError("❌ Port 8443 already in use", 8443);
+  assert.match(msg, /8443/);
+});
+
+test("with no captured stderr, falls back to a generic (non-misleading) message", () => {
+  const msg = interpretMitmStartupError("", 443);
+  // Must NOT assert it is specifically a port-443 problem when nothing was captured.
+  assert.doesNotMatch(msg, /port 443 may be in use/i);
+  assert.match(msg, /failed to start/i);
+});