Bound the research MCP's environment, deadline, and network cost (#70)

.expo-code-review/config.jsonc

@@ -20,14 +20,15 @@
   // *.min.js, *.map, __snapshots__/*.snap, @generated markers).
   "noise": { "additionalIgnores": [] },
 
-  // Trusted host-side documentation research. Only bounded API identifiers leave
-  // the runner; scoped web results are independently checked against fixed provider
-  // allowlists before ECR fetches and fences official documentation passages.
+  // Reviewer-invoked documentation research. The reviewing model chooses the
+  // outbound query terms and URLs; they are shape-checked and bounded, not
+  // confidentiality-checked. Results are independently re-checked against fixed
+  // provider allowlists before ECR fetches and fences official passages.
   "research": {
     "enabled": true,
     "maxQueries": 8,
     "resultsPerQuery": 2,
-    "timeoutMs": 15000
+    "timeoutMs": 30000
   },
 
   // Large diffs are split into focused chunks by changed-line count, plus a

README.md

@@ -190,7 +190,7 @@ Enable it only in the root config, which CI loads from the PR's trusted base:
     "enabled": true,
     "maxQueries": 8,
     "resultsPerQuery": 2,
-    "timeoutMs": 15000
+    "timeoutMs": 30000
   }
 }
 ```
@@ -205,13 +205,26 @@ the current absolute Node executable, so a PR-owned `PATH` entry cannot replace
 either component. Each review gets an owner-only temporary MCP config and append-only
 audit. Claude receives that explicit config under `--strict-mcp-config`, with project
 settings and slash commands disabled; OpenCode receives the same fixed local command.
-The Brave credential is passed to the MCP child, not the model process.
 
-The MCP is the outbound security boundary. Search queries are normalized before
+That command is a wrapper, not the server. Both engines merge a configured MCP `env`
+onto their own environment instead of replacing it, so the declared block alone cannot
+bound the child — the engine's model credential reaches it, and OpenCode additionally
+passes down the runner's whole ambient environment. The wrapper therefore rebuilds the
+environment from an explicit allowlist before starting the server, and loads no parser
+and opens no socket of its own. The server process sees the search key, the call
+bounds, and locale/proxy settings; it never sees a model credential. The Brave
+credential travels the same way and is never added to the model process env.
+
+The MCP bounds the shape, host, and volume of outbound requests. It is not a
+confidentiality boundary: the reviewing model chooses the query terms and URLs, and a
+low-entropy identifier can carry repository-derived data past every check below.
+Enable research only where repository-derived terms may be shared with Brave and the
+documentation providers. Search queries are normalized before
 logging or networking: quoted literals, URLs, email addresses, paths, prose stop
 words, overlong/high-entropy tokens, and unsupported punctuation are removed;
-credential-shaped or secret-labeled input fails closed. The remaining query must be
-at most eight short tokens and contain an API-like symbol. Direct URLs must use plain
+credential-shaped or secret-labeled input fails closed. The remaining query must be at
+most eight short tokens and either contain an API-like symbol or be a short multi-word
+lowercase concept phrase. Direct URLs must use plain
 HTTPS with no credentials, port, query string, or fragment; suspicious/high-entropy
 path segments fail closed. The fixed provider host/path allowlist and redirect,
 response-size, content-type, and timeout checks still apply after that first gate.
@@ -219,6 +232,17 @@ These deterministic checks greatly reduce accidental exfiltration; they are not
 proof that every low-entropy string is harmless, so reviewer prompts also forbid
 sending repository text and the review-wide MCP budget defaults to eight calls.
 
+`maxQueries` bounds MCP calls, not network requests. One search selects up to four
+providers, and each issues its own discovery request plus a page fetch per candidate,
+so eight calls can mean roughly thirty discovery requests and over a hundred page
+downloads. Every call therefore reports its own ledger — discovery requests, page
+fetches, redirect hops, total HTTP requests, and elapsed time — and the review log and
+Actions summary report the totals. `timeoutMs` is the MCP's own end-to-end deadline for
+one call, enforced by the server across discovery, redirects, retrieval, and
+extraction; a call that hits it returns what it already has rather than failing. It has
+to live there because OpenCode's `timeout` bounds only tool discovery and Claude
+provides no per-call timeout at all.
+
 For non-Expo providers, discovery sends a fixed, provider-owned `site:` scope plus
 the bounded query to Brave's fixed Web Search endpoint. Search snippets and titles
 are never treated as evidence. ECR independently rejects off-allowlist result URLs,
@@ -276,8 +300,11 @@ same audit trail in the step summary, while `.runs/reviews.jsonl` keeps the quer
 plus bounded returned passages for short-lived operational inspection. Reviewers
 are instructed to attach `sources` only when documentation materially supports a
 finding. ECR accepts only exact URLs returned during that review, restores canonical
-titles, carries citations through coordination, and renders them below the finding;
-invented or unrelated citations are dropped.
+titles, carries citations through coordination, and renders them below the finding.
+A citation to a URL this review never retrieved is dropped outright. Relatedness is a
+separate, weaker guarantee: a cited finding is escalated to the verifier with the
+audited passage inline, which judges whether that passage actually supports the claim
+and strips the citation when it does not.
 
 Reviewers also emit a bounded `researchDecisions` record only when documentation
 materially confirms a finding candidate or proves one safe. ECR grounds those records

llp/0013-platform-research.explainer.md

@@ -24,10 +24,20 @@ supplies only enablement, bounds, and an optional absolute fallback index path
 arbitrary command, endpoint, host, or argument vector.
 
 The MCP remains a process boundary rather than an in-process import. Its stdout is
-the only result channel, its environment is an allowlist, and ECR caps calls and
-results. Each run uses an owner-only temporary MCP configuration and append-only audit.
-A tool failure remains local to research and never skips or weakens an ordinary review
-pass.
+the only result channel, and ECR caps calls, results, and per-call duration. Each run
+uses an owner-only temporary MCP configuration and append-only audit. A tool failure
+remains local to research and never skips or weakens an ordinary review pass.
+
+The environment ECR declares in that configuration is a request, not a boundary. Both
+Claude Code and OpenCode merge a server's `env` onto the environment they already hold
+rather than replacing it, so the declared allowlist alone leaves the engine's model
+credential in the child — and under OpenCode the runner's entire ambient environment,
+because the SDK spawns its server with an unfiltered `process.env`. ECR therefore does
+not rely on the engines for this. The command in the configuration names a wrapper that
+rebuilds the environment from an explicit allowlist and then starts the real server.
+The wrapper is deliberately inert: it loads no parser and opens no socket, so the one
+process that still holds the engine's credentials is not the one that parses untrusted
+remote documents. Anything the engine merged in stops there.
 
 Starting a review does not derive searches from the patch, prefetch documentation, or
 start a crawl. ECR only prepares the bounded MCP configuration and audit path. Reviewer
@@ -41,9 +51,21 @@ identifier searches.
 `review-research-mcp serve` uses Brave Web Search as discovery for non-Expo
 providers. Each request combines the already-sanitized API/concept query with a fixed
 provider-owned `site:` scope. The endpoint is fixed, redirects are rejected, response
-size/time/hit count are capped, and only the fixed `BRAVE_SEARCH_API_KEY` credential
-is forwarded into the MCP child. Search titles, snippets, and ranking are untrusted
-discovery hints, never evidence.
+size/time/hit count are capped, and the fixed `BRAVE_SEARCH_API_KEY` is the only
+credential the wrapper admits into the server's environment. Search titles, snippets,
+and ranking are untrusted discovery hints, never evidence.
+
+One reserved call is not one request. A search selects up to four providers, each of
+which issues its own discovery request and then downloads candidate pages until it has
+enough results, and every page may cost up to six round trips through the redirect
+chain. The call budget alone therefore understates outbound traffic by more than an
+order of magnitude, so each call carries a ledger — discovery requests, page fetches,
+redirect hops, total requests, elapsed time — recorded in the audit and summed for the
+review. The same seam carries one end-to-end deadline per call, because no engine can
+supply it: OpenCode's `mcp.timeout` bounds tool discovery rather than execution, and
+Claude passes no per-call timeout at all, leaving only a per-hop limit that multiplies
+instead of bounding. A call that reaches its deadline returns the evidence it already
+has rather than failing.
 
 Every discovered URL must independently pass the provider's exact HTTPS host/path
 allowlist before a fetch begins. Redirects are handled manually and must remain on
@@ -139,6 +161,15 @@ Media3, Glide, or OkHttp—when library behavior also matters. Repository owners
 not documentation ownership, so a `packages/expo-*` path does not replace Apple or
 Android with Expo documentation.
 
+These checks bound the SHAPE of an outbound request, not its information content. The
+reviewing model reads repository data and then chooses the query terms and URL path,
+and a token that satisfies every rule above — short, low-entropy, API-shaped — can
+still encode repository-derived bytes. The provider allowlist fixes who receives a
+request; it does not constrain what the request says. Research is therefore an
+outbound disclosure channel to Brave and the documentation providers, appropriate only
+where repository-derived terms may be shared with them, and the reviewer prompt's
+instruction not to send repository text is a mitigation rather than an enforcement.
+
 MCP output is schema-validated, limited to HTTPS URLs from the requested provider,
 truncated, and sanitized. Returned passages are labeled untrusted reference data in
 the reviewer prompt. The Brave credential is scoped to the MCP child and is never

src/__tests__/research-mcp/mcp.test.ts

@@ -151,15 +151,26 @@ test("stdio MCP starts without an index and reports unavailable remote discovery
     });
     const content = response.content as Array<{ type: string; text?: string }>;
     const payload = JSON.parse(content.find((block) => block.type === "text")?.text ?? "") as {
-      retrieval: { localIndex: unknown; scopedWebSearch: boolean };
+      retrieval: {
+        localIndex: unknown;
+        scopedWebSearch: boolean;
+        network: Record<string, number>;
+      };
       warnings: string[];
       results: unknown[];
     };
-    expect(payload.retrieval).toEqual({
+    expect(payload.retrieval).toMatchObject({
       scopedWebSearch: false,
       expoSearch: false,
       localIndex: null,
     });
+    // No key and no provider reachable, so the call must not have touched the network.
+    expect(payload.retrieval.network).toMatchObject({
+      searchRequests: 0,
+      documentRequests: 0,
+      redirects: 0,
+      totalRequests: 0,
+    });
     expect(payload.warnings[0]).toMatch(/BRAVE_SEARCH_API_KEY is not set/);
     expect(payload.results).toEqual([]);
   } finally {

src/__tests__/research-mcp/network.test.ts

@@ -0,0 +1,119 @@
+import { expect, test } from "bun:test";
+
+import {
+  createResearchNetwork,
+  ResearchDeadlineError,
+  totalResearchNetwork,
+} from "../../research-mcp/network.js";
+import { fetchDocumentationUrl } from "../../research-mcp/direct-fetch.js";
+
+const html =
+  "<html><head><title>Doc</title></head><body><main><h1>Doc</h1>" +
+  "<p>CameraView barcodeScannerSettings documentation body text.</p></main></body></html>";
+
+function page(): Response {
+  return new Response(html, { status: 200, headers: { "content-type": "text/html" } });
+}
+
+test("the call deadline bounds a redirect chain that the per-hop timeout cannot", async () => {
+  // Every hop answers quickly enough to beat the hardcoded 10s per-attempt limit,
+  // which is exactly how a chain reaches 60s with no end-to-end bound.
+  const network = createResearchNetwork(async (input) => {
+    const url = new URL(typeof input === "string" ? input : ((input as URL).href ?? String(input)));
+    await new Promise((resolve) => setTimeout(resolve, 60));
+    const next = new URL(url.href);
+    next.pathname += "/hop";
+    return new Response(null, { status: 302, headers: { location: next.href } });
+  }, 200);
+
+  await expect(
+    fetchDocumentationUrl("https://docs.expo.dev/versions/latest/sdk/camera", {
+      fetchImplementation: network.fetch,
+    }),
+  ).rejects.toThrow();
+  expect(network.expired()).toBe(true);
+  // Without the deadline this walks all six hops; the deadline cuts it short.
+  expect(network.counts().totalRequests).toBeLessThan(6);
+});
+
+test("an expired deadline refuses the next request instead of starting it", async () => {
+  let issued = 0;
+  const network = createResearchNetwork(async () => {
+    issued++;
+    return page();
+  }, 1);
+  await new Promise((resolve) => setTimeout(resolve, 20));
+
+  await expect(network.fetch("https://docs.expo.dev/x")).rejects.toBeInstanceOf(
+    ResearchDeadlineError,
+  );
+  expect(issued).toBe(0);
+});
+
+test("the ledger separates paid discovery from page fetches and redirect hops", async () => {
+  const network = createResearchNetwork(async (input) => {
+    const href = typeof input === "string" ? input : ((input as URL).href ?? String(input));
+    if (href.startsWith("https://api.search.brave.com/")) {
+      return new Response("{}", { status: 200, headers: { "content-type": "application/json" } });
+    }
+    if (href.endsWith("/redirect-me")) {
+      return new Response(null, {
+        status: 302,
+        headers: { location: "https://docs.expo.dev/final" },
+      });
+    }
+    return page();
+  }, 10_000);
+
+  await network.fetch("https://api.search.brave.com/res/v1/web/search?q=x");
+  await network.fetch("https://docs.expo.dev/redirect-me");
+  await network.fetch("https://docs.expo.dev/final");
+
+  expect(network.counts()).toMatchObject({
+    searchRequests: 1,
+    // The redirect hop is a round trip, not a distinct page.
+    documentRequests: 1,
+    redirects: 1,
+    totalRequests: 3,
+  });
+});
+
+test("a redirect from a discovery endpoint never drives a counter negative", async () => {
+  // The search backends also use `redirect: "manual"` and treat a 3xx as an
+  // error, so a redirect must undo the counter it actually incremented.
+  const network = createResearchNetwork(
+    async () =>
+      new Response(null, {
+        status: 302,
+        headers: { location: "https://api.search.brave.com/res/v2/web/search" },
+      }),
+    10_000,
+  );
+  await network.fetch("https://api.search.brave.com/res/v1/web/search?q=x");
+
+  const counts = network.counts();
+  expect(counts).toMatchObject({ searchRequests: 0, documentRequests: 0, redirects: 1 });
+  expect(counts.documentRequests).toBeGreaterThanOrEqual(0);
+  expect(counts.searchRequests).toBeGreaterThanOrEqual(0);
+});
+
+test("per-call ledgers sum into one review-wide total", () => {
+  expect(
+    totalResearchNetwork([
+      {
+        searchRequests: 4,
+        documentRequests: 16,
+        redirects: 2,
+        totalRequests: 22,
+        elapsedMs: 1_200,
+      },
+      { searchRequests: 1, documentRequests: 3, redirects: 0, totalRequests: 4, elapsedMs: 300 },
+    ]),
+  ).toEqual({
+    searchRequests: 5,
+    documentRequests: 19,
+    redirects: 2,
+    totalRequests: 26,
+    elapsedMs: 1_500,
+  });
+});

src/__tests__/research.test.ts

@@ -1,6 +1,9 @@
 import { expect, test } from "bun:test";
+import { spawn } from "node:child_process";
+import { copyFile, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import path from "node:path";
+import { fileURLToPath } from "node:url";
 
 import { ReviewConfigSchema, ScopeReviewConfigSchema } from "../config/schema.js";
 import {
@@ -19,6 +22,70 @@ import {
   toResearchProvenance,
 } from "../core/research.js";
 
+/**
+ * The declared `env` block is NOT the boundary. Both Claude Code and OpenCode merge
+ * it onto the environment they already hold, so this asserts on the environment the
+ * wrapper actually hands the server — the only place the guarantee is real.
+ */
+test("the wrapper hands the MCP server a constructed environment, not an inherited one", async () => {
+  const directory = await mkdtemp(path.join(tmpdir(), "ecr-wrapper-test-"));
+  try {
+    const recordPath = path.join(directory, "record.json");
+    // Stands in for the real server: the wrapper resolves ./cli.{js,ts} beside itself.
+    await writeFile(
+      path.join(directory, "cli.ts"),
+      `import { writeFileSync } from "node:fs";\n` +
+        `writeFileSync(${JSON.stringify(recordPath)}, JSON.stringify(Object.keys(process.env).sort()));\n`,
+      "utf8",
+    );
+    for (const name of ["wrapper.ts", "child-env.ts"]) {
+      await copyFile(
+        fileURLToPath(new URL(`../research-mcp/${name}`, import.meta.url)),
+        path.join(directory, name),
+      );
+    }
+
+    const exitCode = await new Promise<number>((resolve) => {
+      const child = spawn(process.execPath, [path.join(directory, "wrapper.ts"), "serve"], {
+        env: {
+          // Everything an engine was measured to merge in, plus the legitimate block.
+          PATH: process.env.PATH ?? "",
+          HOME: "/root",
+          ANTHROPIC_API_KEY: "must-not-reach-the-server",
+          CLAUDE_CODE_OAUTH_TOKEN: "must-not-reach-the-server",
+          AWS_SECRET_ACCESS_KEY: "must-not-reach-the-server",
+          GITHUB_TOKEN: "must-not-reach-the-server",
+          OPENCODE_CONFIG_CONTENT: "{}",
+          NODE_OPTIONS: "--require /evil.js",
+          LANG: "C.UTF-8",
+          LC_ALL: "C.UTF-8",
+          BRAVE_SEARCH_API_KEY: "search-only",
+          REVIEW_RESEARCH_AUDIT_PATH: path.join(directory, "audit.jsonl"),
+          REVIEW_RESEARCH_MAX_CALLS: "8",
+          REVIEW_RESEARCH_MAX_RESULTS: "2",
+          REVIEW_RESEARCH_TIMEOUT_MS: "30000",
+        },
+        stdio: "ignore",
+      });
+      child.on("exit", (code) => resolve(code ?? 1));
+    });
+    expect(exitCode).toBe(0);
+
+    const names = JSON.parse(await readFile(recordPath, "utf8")) as string[];
+    expect(names.sort()).toEqual([
+      "BRAVE_SEARCH_API_KEY",
+      "LANG",
+      "LC_ALL",
+      "REVIEW_RESEARCH_AUDIT_PATH",
+      "REVIEW_RESEARCH_MAX_CALLS",
+      "REVIEW_RESEARCH_MAX_RESULTS",
+      "REVIEW_RESEARCH_TIMEOUT_MS",
+    ]);
+  } finally {
+    await rm(directory, { recursive: true, force: true });
+  }
+}, 30_000);
+
 test("research child environment forwards only locale, proxy variables, and the fixed search key", () => {
   expect(
     researchChildEnvironment({

src/config/load.ts

@@ -44,7 +44,7 @@ const RESEARCH_CONFIG_DEFAULTS: LoadedConfig["research"] = {
   enabled: false,
   maxQueries: 8,
   resultsPerQuery: 2,
-  timeoutMs: 15_000,
+  timeoutMs: 30_000,
 };
 
 /** Default OpenCode tool toggles for a reviewer: read the repo, never mutate it. */

src/core/research.ts

@@ -9,6 +9,8 @@ import { fileURLToPath } from "node:url";
 
 import type { LoadedConfig } from "../config/schema.js";
 import { readResearchAudit } from "../research-mcp/audit.js";
+import { researchChildEnvironment } from "../research-mcp/child-env.js";
+import { totalResearchNetwork, type ResearchNetworkCounts } from "../research-mcp/network.js";
 import type { Finding, FindingSource, ResearchDecision } from "./schema.js";
 export { OPENCODE_RESEARCH_TOOLS } from "./tools.js";
 
@@ -57,6 +59,12 @@ export interface ResearchProvenance {
    * url-rejected, budget-exhausted). Rejected input text is never recorded.
    */
   rejections?: Array<{ tool: string; reason: string; count: number }>;
+  /**
+   * Review-wide outbound totals. Reported separately from the call budget because
+   * `maxQueries` bounds MCP calls, and one call fans out into several discovery
+   * requests plus a page download per candidate.
+   */
+  network?: ResearchNetworkCounts;
   /** Grounded reviewer declarations that documentation changed a concrete decision. */
   decisions?: Array<ResearchDecision & { agent: string }>;
   usefulness?: ResearchUsefulness;
@@ -103,36 +111,18 @@ export interface ResearchMcpRuntime {
   cleanup(): Promise<void>;
 }
 
-const RESEARCH_PROXY_ENV_KEYS = [
-  "HTTP_PROXY",
-  "HTTPS_PROXY",
-  "NO_PROXY",
-  "http_proxy",
-  "https_proxy",
-  "no_proxy",
-] as const;
-const RESEARCH_SEARCH_API_KEY = "BRAVE_SEARCH_API_KEY";
-
-export function researchChildEnvironment(
-  source: NodeJS.ProcessEnv = process.env,
-): NodeJS.ProcessEnv {
-  const environment: NodeJS.ProcessEnv = {
-    LANG: "C.UTF-8",
-    LC_ALL: "C.UTF-8",
-    ...(process.platform === "win32" && source.SystemRoot ? { SystemRoot: source.SystemRoot } : {}),
-  };
-  for (const key of RESEARCH_PROXY_ENV_KEYS) {
-    if (source[key]) environment[key] = source[key];
-  }
-  if (source[RESEARCH_SEARCH_API_KEY]) {
-    environment[RESEARCH_SEARCH_API_KEY] = source[RESEARCH_SEARCH_API_KEY];
-  }
-  return environment;
-}
+export { researchChildEnvironment };
 
+/**
+ * The engine spawns the WRAPPER, not the server. Both Claude Code and OpenCode
+ * merge the config's `env` block onto their own environment instead of replacing
+ * it, so the block below cannot bound the child on its own; the wrapper rebuilds
+ * the environment from an explicit allowlist before the real server starts. See
+ * research-mcp/wrapper.ts.
+ */
 export function bundledResearchServer(): { command: string; args: string[] } {
-  const builtEntry = fileURLToPath(new URL("../research-mcp/cli.js", import.meta.url));
-  const sourceEntry = fileURLToPath(new URL("../research-mcp/cli.ts", import.meta.url));
+  const builtEntry = fileURLToPath(new URL("../research-mcp/wrapper.js", import.meta.url));
+  const sourceEntry = fileURLToPath(new URL("../research-mcp/wrapper.ts", import.meta.url));
   return {
     command: process.execPath,
     args: [existsSync(builtEntry) ? builtEntry : sourceEntry],
@@ -141,8 +131,12 @@ export function bundledResearchServer(): { command: string; args: string[] } {
 
 /**
  * Create one owner-only MCP configuration and append-only audit for a review run.
- * The model process receives only the config path; the Brave credential is passed
- * directly to the bounded MCP child and never added to the model process env.
+ * The model process receives only the config path.
+ *
+ * The `env` block below is a request the engine merges rather than applies — see
+ * bundledResearchServer. The wrapper it names is what actually bounds the server's
+ * environment, so the Brave credential and these limits are the only things the
+ * server can see, whatever the engine passed down.
  */
 export async function createResearchMcpRuntime(
   config: LoadedConfig["research"],
@@ -164,6 +158,10 @@ export async function createResearchMcpRuntime(
       REVIEW_RESEARCH_AUDIT_PATH: auditPath,
       REVIEW_RESEARCH_MAX_CALLS: String(config.maxQueries),
       REVIEW_RESEARCH_MAX_RESULTS: String(config.resultsPerQuery),
+      // The MCP enforces this itself as a per-call deadline. OpenCode's `timeout`
+      // only bounds tool DISCOVERY and Claude has no equivalent, so neither engine
+      // can bound how long a call actually runs — the server has to.
+      REVIEW_RESEARCH_TIMEOUT_MS: String(config.timeoutMs),
     }).flatMap(([key, value]) => (value === undefined ? [] : [[key, value]])),
   );
   await writeFile(
@@ -233,6 +231,8 @@ export async function researchProvenanceFromAudit(
     warnings: [...new Set(warnings)].slice(0, 10),
   };
   const provenance = toResearchProvenance(run);
+  const ledgers = records.flatMap((record) => (record.network ? [record.network] : []));
+  if (ledgers.length > 0) provenance.network = totalResearchNetwork(ledgers);
   if (rejections.length > 0) {
     const counts = new Map<string, number>();
     for (const rejection of rejections) {
@@ -313,6 +313,13 @@ export function formatResearchProgress(provenance: ResearchProvenance): string[]
       );
     }
   }
+  if (provenance.network) {
+    const { searchRequests, documentRequests, redirects, totalRequests } = provenance.network;
+    lines.push(
+      `  research network: ${searchRequests} search request(s), ${documentRequests} page fetch(es), ` +
+        `${redirects} redirect(s) — ${totalRequests} HTTP request(s) total`,
+    );
+  }
   for (const rejection of provenance.rejections ?? []) {
     lines.push(
       `  research: ${rejection.count} ${rejection.tool} call(s) rejected before execution (${rejection.reason})`,
@@ -364,6 +371,16 @@ export function renderResearchMarkdown(provenance: ResearchProvenance): string {
       );
     }
   }
+  if (provenance.network) {
+    const { searchRequests, documentRequests, redirects, totalRequests, elapsedMs } =
+      provenance.network;
+    lines.push(
+      "",
+      `Outbound: **${searchRequests}** search request(s), **${documentRequests}** page fetch(es), ` +
+        `**${redirects}** redirect(s) — **${totalRequests}** HTTP request(s) across ` +
+        `${(elapsedMs / 1000).toFixed(1)}s of call time.`,
+    );
+  }
   for (const rejection of provenance.rejections ?? []) {
     lines.push(
       `- ⚠️ ${rejection.count} \`${escapeMarkdownLabel(rejection.tool)}\` call(s) rejected before execution (${escapeMarkdownLabel(rejection.reason)}).`,

src/research-mcp/audit.ts

@@ -1,6 +1,7 @@
 import { appendFile, mkdir, readFile, rmdir } from "node:fs/promises";
 import { randomUUID } from "node:crypto";
 
+import type { ResearchNetworkCounts } from "./network.js";
 import type { SearchResult } from "./types.js";
 
 export type ResearchToolName = "search_platform_docs" | "fetch_platform_doc";
@@ -52,6 +53,12 @@ export interface ResearchAuditResult {
       >
   >;
   warnings: string[];
+  /**
+   * What this one reserved call actually spent on the network. Recorded because
+   * the reservation count alone is a poor proxy: a single search can issue four
+   * paid discovery requests and sixteen page downloads.
+   */
+  network?: ResearchNetworkCounts;
   error?: string;
 }
 
@@ -196,6 +203,7 @@ export class ResearchAudit {
     input: ResearchAuditInput,
     results: SearchResult[],
     warnings: string[] = [],
+    network?: ResearchNetworkCounts,
   ): Promise<void> {
     await this.append({
       type: "completed",
@@ -204,6 +212,7 @@ export class ResearchAudit {
       input,
       results: results.map(boundedResult),
       warnings: warnings.slice(0, 10).map((warning) => warning.slice(0, 500)),
+      ...(network ? { network } : {}),
       timestamp: new Date().toISOString(),
     });
   }

src/research-mcp/child-env.ts

@@ -0,0 +1,81 @@
+// @ref LLP 0013#one-package-two-binaries [implements] — the bounded MCP's environment is constructed, never inherited
+/**
+ * The single definition of what the bounded documentation MCP is allowed to see
+ * in its environment.
+ *
+ * Two callers share it, and they need it for different reasons:
+ *
+ *  - `createResearchMcpRuntime` writes `researchChildEnvironment()` into the
+ *    engine's MCP configuration. That block is a REQUEST, not a guarantee: both
+ *    Claude Code and OpenCode merge it onto the environment the engine already
+ *    has rather than replacing it, so the declared allowlist alone never bounds
+ *    the child.
+ *  - `wrapper.ts` applies `researchWrapperEnvironment()` when it spawns the real
+ *    server. That IS the guarantee — the server process is handed a constructed
+ *    environment, so whatever the engine merged in stops at the wrapper.
+ *
+ * Nothing here imports a parser, a network client, or the config schema: the
+ * wrapper must stay loadable without pulling untrusted-content machinery into
+ * the one process that still holds the engine's credentials.
+ */
+
+const PROXY_ENV_KEYS = [
+  "HTTP_PROXY",
+  "HTTPS_PROXY",
+  "NO_PROXY",
+  "http_proxy",
+  "https_proxy",
+  "no_proxy",
+] as const;
+
+export const RESEARCH_SEARCH_API_KEY = "BRAVE_SEARCH_API_KEY";
+
+/**
+ * Bounds ECR sets on the child through the config env block. These are inputs to
+ * the server, so the wrapper must forward them; every one is a number, a path ECR
+ * itself chose, or both, and none is a credential.
+ */
+export const RESEARCH_RUNTIME_ENV_KEYS = [
+  "REVIEW_RESEARCH_AUDIT_PATH",
+  "REVIEW_RESEARCH_INDEX_PATH",
+  "REVIEW_RESEARCH_MAX_CALLS",
+  "REVIEW_RESEARCH_MAX_RESULTS",
+  "REVIEW_RESEARCH_TIMEOUT_MS",
+] as const;
+
+/**
+ * Locale, proxy configuration, and the search-only credential — the environment
+ * the MCP actually needs to do its job.
+ *
+ * NODE_OPTIONS is deliberately absent: it is arbitrary code injection into the
+ * process that parses untrusted remote documents. So are PATH and HOME — the
+ * wrapper spawns the server by absolute path and the server writes only to the
+ * audit path it is given.
+ */
+export function researchChildEnvironment(
+  source: NodeJS.ProcessEnv = process.env,
+): NodeJS.ProcessEnv {
+  const environment: NodeJS.ProcessEnv = {
+    LANG: "C.UTF-8",
+    LC_ALL: "C.UTF-8",
+    ...(process.platform === "win32" && source.SystemRoot ? { SystemRoot: source.SystemRoot } : {}),
+  };
+  for (const key of PROXY_ENV_KEYS) {
+    if (source[key]) environment[key] = source[key];
+  }
+  if (source[RESEARCH_SEARCH_API_KEY]) {
+    environment[RESEARCH_SEARCH_API_KEY] = source[RESEARCH_SEARCH_API_KEY];
+  }
+  return environment;
+}
+
+/** The child environment plus the bounds ECR passes through the config block. */
+export function researchWrapperEnvironment(
+  source: NodeJS.ProcessEnv = process.env,
+): NodeJS.ProcessEnv {
+  const environment = researchChildEnvironment(source);
+  for (const key of RESEARCH_RUNTIME_ENV_KEYS) {
+    if (source[key] !== undefined) environment[key] = source[key];
+  }
+  return environment;
+}

src/research-mcp/cli.ts

@@ -63,6 +63,7 @@ async function main() {
         : {}),
       maxCalls: boundedInteger("REVIEW_RESEARCH_MAX_CALLS", 8, 1, 20),
       maxResultsPerCall: boundedInteger("REVIEW_RESEARCH_MAX_RESULTS", 3, 1, 3),
+      timeoutMs: boundedInteger("REVIEW_RESEARCH_TIMEOUT_MS", 30_000, 1_000, 60_000),
       ...(process.env.BRAVE_SEARCH_API_KEY
         ? { braveApiKey: process.env.BRAVE_SEARCH_API_KEY }
         : {}),

src/research-mcp/network.ts

@@ -0,0 +1,132 @@
+// @ref LLP 0013#search-fetch-and-optional-index-boundary [implements] — one per-call deadline and one request ledger for every outbound request
+/**
+ * The outbound seam for a single MCP tool call: one deadline and one ledger,
+ * wrapped around the fetch implementation every search and fetch path already
+ * receives.
+ *
+ * Two problems made this necessary, and both are structural rather than
+ * incidental:
+ *
+ *  - No engine can bound a call's duration. OpenCode's `mcp.timeout` bounds tool
+ *    DISCOVERY, not execution, and Claude passes no per-call timeout at all. The
+ *    only hardcoded bound was 10s PER REDIRECT HOP, which multiplies: a page that
+ *    redirects five times costs 60s, and a four-provider search walking its
+ *    candidates costs minutes. The deadline has to live here.
+ *  - The audit reserved one budget unit per tool call and reported nothing about
+ *    what that unit spent. One call can issue four paid searches and sixteen page
+ *    downloads, so "8 calls" understated real traffic by more than an order of
+ *    magnitude.
+ *
+ * Wrapping fetch (rather than threading a signal through every signature) means
+ * every existing caller is covered automatically, including paths added later.
+ */
+import type { FetchImplementation } from "./brave-search.js";
+
+/** Discovery endpoints. Everything else is a documentation page or asset. */
+const SEARCH_ENDPOINTS = [
+  "https://api.search.brave.com/",
+  "https://qex7pb7d46-dsn.algolia.net/",
+  "https://lysine.dev/okhttp/search/search_index.json",
+];
+
+export interface ResearchNetworkCounts {
+  /** Discovery requests, i.e. the ones that cost search-provider quota. */
+  searchRequests: number;
+  /** Documentation page and asset requests, excluding redirect hops. */
+  documentRequests: number;
+  /** Redirect responses followed. Each one is an extra round trip. */
+  redirects: number;
+  /** Every HTTP request actually issued, redirect hops included. */
+  totalRequests: number;
+  elapsedMs: number;
+}
+
+export class ResearchDeadlineError extends Error {
+  constructor(timeoutMs: number) {
+    super(`Documentation research call exceeded its ${timeoutMs}ms deadline`);
+    this.name = "ResearchDeadlineError";
+  }
+}
+
+export interface ResearchNetwork {
+  /** Drop-in replacement for the caller's fetch: deadline-bound and counted. */
+  fetch: FetchImplementation;
+  /** The call deadline, for loops that should stop rather than fail per-item. */
+  signal: AbortSignal;
+  expired(): boolean;
+  counts(): ResearchNetworkCounts;
+}
+
+export function createResearchNetwork(
+  base: FetchImplementation,
+  timeoutMs: number,
+): ResearchNetwork {
+  const startedAt = Date.now();
+  const controller = new AbortController();
+  const timer = setTimeout(() => controller.abort(new ResearchDeadlineError(timeoutMs)), timeoutMs);
+  // The MCP is a short-lived stdio process; never hold the loop open for this.
+  timer.unref?.();
+
+  let searchRequests = 0;
+  let documentRequests = 0;
+  let redirects = 0;
+  let totalRequests = 0;
+
+  const wrapped: FetchImplementation = async (input, init) => {
+    if (controller.signal.aborted) throw new ResearchDeadlineError(timeoutMs);
+
+    const href = typeof input === "string" ? input : input instanceof URL ? input.href : input.url;
+    const isSearch = SEARCH_ENDPOINTS.some((endpoint) => href.startsWith(endpoint));
+    totalRequests++;
+    if (isSearch) searchRequests++;
+    else documentRequests++;
+
+    // Keep the caller's own per-attempt timeout AND add the call deadline, so a
+    // single slow hop still fails fast while the whole call stays bounded.
+    const signal = init?.signal
+      ? AbortSignal.any([init.signal, controller.signal])
+      : controller.signal;
+
+    const response = await base(input, { ...init, signal });
+    if (response.status >= 300 && response.status < 400) {
+      redirects++;
+      // A redirect hop is a round trip, not a distinct resource, so undo the
+      // classification count. It has to be the SAME counter that was
+      // incremented: the search backends use `redirect: "manual"` too and treat
+      // a 3xx as an error, so unconditionally decrementing documentRequests
+      // drives it negative the first time a discovery endpoint redirects.
+      if (isSearch) searchRequests--;
+      else documentRequests--;
+    }
+    return response;
+  };
+
+  return {
+    fetch: wrapped,
+    signal: controller.signal,
+    expired: () => controller.signal.aborted,
+    counts: () => ({
+      searchRequests,
+      documentRequests,
+      redirects,
+      totalRequests,
+      elapsedMs: Date.now() - startedAt,
+    }),
+  };
+}
+
+/** Sum per-call ledgers into one review-wide total. */
+export function totalResearchNetwork(
+  counts: readonly ResearchNetworkCounts[],
+): ResearchNetworkCounts {
+  return counts.reduce<ResearchNetworkCounts>(
+    (total, entry) => ({
+      searchRequests: total.searchRequests + entry.searchRequests,
+      documentRequests: total.documentRequests + entry.documentRequests,
+      redirects: total.redirects + entry.redirects,
+      totalRequests: total.totalRequests + entry.totalRequests,
+      elapsedMs: total.elapsedMs + entry.elapsedMs,
+    }),
+    { searchRequests: 0, documentRequests: 0, redirects: 0, totalRequests: 0, elapsedMs: 0 },
+  );
+}

src/research-mcp/remote-search.ts

@@ -98,6 +98,12 @@ export interface RemoteSearchOptions {
   fetchImplementation?: FetchImplementation;
   language?: Language;
   sourceKinds?: SourceKind[];
+  /**
+   * The call deadline. The wrapped fetch already fails once it passes, but the
+   * candidate walk below should stop and return what it has rather than march
+   * through every remaining candidate collecting identical timeout warnings.
+   */
+  deadline?: AbortSignal;
 }
 
 export interface RemoteSearchResponse {
@@ -229,7 +235,11 @@ export async function searchRemoteDocumentation(
   // downloading every discovery candidate.
   const fetched: SearchResult[] = [];
   let candidateIndex = 0;
-  while (fetched.length < limit && candidateIndex < candidates.length) {
+  while (
+    fetched.length < limit &&
+    candidateIndex < candidates.length &&
+    !options.deadline?.aborted
+  ) {
     const outstanding = limit - fetched.length;
     const batch = candidates.slice(candidateIndex, candidateIndex + outstanding);
     candidateIndex += batch.length;

src/research-mcp/server.ts

@@ -4,6 +4,7 @@ import { z } from "zod";
 
 import { ResearchAudit } from "./audit.js";
 import type { FetchImplementation } from "./brave-search.js";
+import { createResearchNetwork } from "./network.js";
 import { sanitizeDocumentationQuery } from "./query-sanitizer.js";
 import {
   DIRECT_DOCUMENT_CONTEXT_MODES,
@@ -34,6 +35,8 @@ export interface DocumentationServerOptions {
   auditPath?: string;
   maxCalls?: number;
   maxResultsPerCall?: number;
+  /** End-to-end deadline for one tool call. Neither engine can enforce this. */
+  timeoutMs?: number;
 }
 
 function defaultProviders(platform: PlatformFilter): ProviderId[] {
@@ -47,7 +50,9 @@ export async function createDocumentationServer(options: DocumentationServerOpti
   const index = options.indexPath ? await loadSearchIndex(options.indexPath) : undefined;
   const maxCalls = Math.min(20, Math.max(1, options.maxCalls ?? 8));
   const maxResultsPerCall = Math.min(3, Math.max(1, options.maxResultsPerCall ?? 3));
+  const timeoutMs = Math.min(60_000, Math.max(1_000, options.timeoutMs ?? 30_000));
   const audit = new ResearchAudit(options.auditPath, maxCalls);
+  const baseFetch = options.fetchImplementation ?? fetch;
   const server = new McpServer({
     name: "review-research-mcp",
     version: "0.2.0",
@@ -115,6 +120,8 @@ export async function createDocumentationServer(options: DocumentationServerOpti
         query: sanitizedQuery,
       };
       const requestId = await audit.reserve("search_platform_docs", auditInput);
+      // One deadline and one request ledger for everything this call issues.
+      const network = createResearchNetwork(baseFetch, timeoutMs);
       try {
         const localResults = index
           ? searchDocumentation(index, sanitizedQuery, {
@@ -145,7 +152,7 @@ export async function createDocumentationServer(options: DocumentationServerOpti
                 const documents = await searchExpoAlgolia(
                   sanitizedQuery,
                   perProviderLimit,
-                  options.fetchImplementation ?? fetch,
+                  network.fetch,
                 );
                 return {
                   results: documents.map((document, position) => ({
@@ -178,7 +185,7 @@ export async function createDocumentationServer(options: DocumentationServerOpti
                 const results = await searchOkHttpDocumentation(
                   sanitizedQuery,
                   perProviderLimit,
-                  options.fetchImplementation ?? fetch,
+                  network.fetch,
                 );
                 if (results.length > 0) {
                   return { results, warnings: [] as string[] };
@@ -199,9 +206,8 @@ export async function createDocumentationServer(options: DocumentationServerOpti
             try {
               return await searchRemoteDocumentation(provider, sanitizedQuery, perProviderLimit, {
                 apiKey: options.braveApiKey,
-                ...(options.fetchImplementation
-                  ? { fetchImplementation: options.fetchImplementation }
-                  : {}),
+                fetchImplementation: network.fetch,
+                deadline: network.signal,
                 ...(language ? { language } : {}),
                 ...(sourceKinds ? { sourceKinds } : {}),
               });
@@ -235,10 +241,13 @@ export async function createDocumentationServer(options: DocumentationServerOpti
           })
           .slice(0, boundedLimit);
         const uniqueWarnings = [...new Set(warnings)].slice(0, 10);
+        const network_ = network.counts();
         const payload = {
           notice: untrustedMaterialNotice,
           retrieval: {
             scopedWebSearch: Boolean(options.braveApiKey),
+            // What this single budget unit actually cost.
+            network: network_,
             expoSearch: selectedProviders.includes("expo"),
             localIndex: index
               ? {
@@ -256,6 +265,7 @@ export async function createDocumentationServer(options: DocumentationServerOpti
           auditInput,
           results,
           uniqueWarnings,
+          network_,
         );
         return {
           content: [{ type: "text", text: JSON.stringify(payload, null, 2) }],
@@ -339,16 +349,18 @@ export async function createDocumentationServer(options: DocumentationServerOpti
         context,
       };
       const requestId = await audit.reserve("fetch_platform_doc", auditInput);
+      // A single URL still costs up to six round trips through the redirect
+      // chain, so this call needs the same deadline and ledger as a search.
+      const network = createResearchNetwork(baseFetch, timeoutMs);
       try {
         const fetched = await fetchDocumentationUrl(target.url.href, {
           provider: target.provider,
           ...(sanitizedQuery ? { query: sanitizedQuery } : {}),
           context,
           limit,
-          ...(options.fetchImplementation
-            ? { fetchImplementation: options.fetchImplementation }
-            : {}),
+          fetchImplementation: network.fetch,
         });
+        const network_ = network.counts();
         const payload = {
           notice: untrustedMaterialNotice,
           retrieval: {
@@ -357,6 +369,7 @@ export async function createDocumentationServer(options: DocumentationServerOpti
             sourceKind: fetched.sourceKind,
             canonicalUrl: fetched.canonicalUrl,
             context: fetched.context,
+            network: network_,
           },
           results: fetched.results,
         };
@@ -370,6 +383,8 @@ export async function createDocumentationServer(options: DocumentationServerOpti
             url: fetched.canonicalUrl,
           },
           fetched.results,
+          [],
+          network_,
         );
         return {
           content: [{ type: "text", text: JSON.stringify(payload, null, 2) }],

src/research-mcp/wrapper.ts

@@ -0,0 +1,63 @@
+#!/usr/bin/env node
+// @ref LLP 0013#one-package-two-binaries [implements] — the environment boundary the engine's MCP config cannot provide
+/**
+ * Environment boundary for the bounded documentation MCP.
+ *
+ * ECR declares a minimal `env` in the engine's MCP configuration, but neither
+ * engine treats that as a replacement: Claude Code and OpenCode both MERGE it
+ * onto the environment the engine already holds. Verified by spawning each
+ * engine against a probe MCP that records variable names — the child saw the
+ * engine's model credential on both, and OpenCode additionally passed through
+ * the runner's whole ambient environment.
+ *
+ * So the boundary has to be ours. The engine spawns this wrapper; the wrapper
+ * spawns the real server with an environment it CONSTRUCTS. Whatever the engine
+ * merged in reaches this process and stops here.
+ *
+ * This file deliberately does almost nothing. It loads no HTML/JSON parser and
+ * opens no socket, because it is the one process in the chain that still holds
+ * the engine's credentials. Everything that touches untrusted remote content
+ * runs in the child, which never receives them.
+ */
+import { spawn } from "node:child_process";
+import { existsSync } from "node:fs";
+import { constants } from "node:os";
+import { fileURLToPath } from "node:url";
+
+import { researchWrapperEnvironment } from "./child-env.js";
+
+const builtEntry = fileURLToPath(new URL("./cli.js", import.meta.url));
+const sourceEntry = fileURLToPath(new URL("./cli.ts", import.meta.url));
+
+const child = spawn(
+  // The current interpreter by absolute path, never a PATH lookup: during a
+  // review the cwd is the untrusted PR-head tree.
+  process.execPath,
+  [existsSync(builtEntry) ? builtEntry : sourceEntry, ...process.argv.slice(2)],
+  {
+    env: researchWrapperEnvironment(process.env),
+    // The child owns the engine's stdio directly, so the wrapper never sits in
+    // the MCP byte stream and cannot truncate, buffer, or reorder a message.
+    stdio: "inherit",
+  },
+);
+
+const FORWARDED_SIGNALS = ["SIGINT", "SIGTERM", "SIGHUP"] as const;
+for (const signal of FORWARDED_SIGNALS) {
+  // The engine signals the process it spawned — us. Pass it on, or the server
+  // outlives the review and keeps its audit lock.
+  process.on(signal, () => {
+    child.kill(signal);
+  });
+}
+
+child.on("error", (error: Error) => {
+  process.stderr.write(`review-research-mcp: failed to start bounded server: ${error.message}\n`);
+  process.exitCode = 1;
+});
+
+child.on("exit", (code, signal) => {
+  // Report a signal death as the conventional 128+n rather than a silent 0, so a
+  // killed server is distinguishable from a clean shutdown.
+  process.exitCode = signal ? 128 + (constants.signals[signal] ?? 0) : (code ?? 1);
+});

templates/config.jsonc

@@ -26,10 +26,20 @@
 
   // Optional bounded platform research (ROOT-ONLY; off by default). Reviewer and
   // cross-file passes can call ECR's bundled MCP for exact API-symbol searches and
-  // supported documentation URLs. The MCP sanitizes queries, uses fixed provider
-  // allowlists, audits results, and never receives model credentials. BRAVE_SEARCH_API_KEY
-  // enables fixed site-scoped discovery; Expo uses its public documentation search.
-  // indexPath remains an optional offline fallback only.
+  // supported documentation URLs. The MCP uses fixed provider allowlists and audits
+  // every call. It runs behind a wrapper that rebuilds its environment from an
+  // explicit allowlist, because both engines MERGE the configured env onto their own
+  // rather than replacing it — so the server sees the search key and these limits,
+  // and never the model credential. BRAVE_SEARCH_API_KEY enables fixed site-scoped
+  // discovery; Expo uses its public documentation search.
+  //
+  // Queries are shape-checked, not confidentiality-checked: the reviewing model
+  // chooses the outbound terms, so enable this only where repository-derived terms
+  // may be shared with Brave and the documentation providers.
+  //
+  // maxQueries bounds MCP CALLS, not requests — one search can issue a discovery
+  // request per provider plus a page fetch per candidate. timeoutMs is the MCP's own
+  // end-to-end deadline per call. indexPath remains an optional offline fallback.
   // "research": {
   //   "enabled": true,
   //   "maxQueries": 8,