diff --git a/src/browser/stores/ReviewStateStore.test.ts b/src/browser/stores/ReviewStateStore.test.ts index 802c4d3acf8..d0bc3157dae 100644 --- a/src/browser/stores/ReviewStateStore.test.ts +++ b/src/browser/stores/ReviewStateStore.test.ts @@ -10,7 +10,11 @@ import type { } from "@/common/orpc/schemas/reviewState"; import { REVIEW_STATE_SECTIONS } from "@/common/orpc/schemas/reviewState"; import type { Review } from "@/common/types/review"; -import { applyReviewStateDelta, withReviewStateSection } from "@/common/utils/reviewState"; +import { + applyReviewStateDelta, + assignSection, + capReviewStateSections, +} from "@/common/utils/reviewState"; import { installDom } from "../../../tests/ui/dom"; import { ReviewStateStore } from "./ReviewStateStore"; @@ -139,7 +143,10 @@ function createBackend(initial: ReviewStateSections = {}) { if (sections[section] !== undefined) { results[section] = "present"; } else { - sections = withReviewStateSection(sections, section, incoming); + sections = { ...sections }; + assignSection(sections, section, incoming); + // ReviewStateService.importLegacy caps what it imports; keep the double in step. + sections = capReviewStateSections(sections); results[section] = "applied"; } } diff --git a/src/browser/stories/mocks/reviewState.ts b/src/browser/stories/mocks/reviewState.ts index 1894b2c9433..0dd03e578cf 100644 --- a/src/browser/stories/mocks/reviewState.ts +++ b/src/browser/stories/mocks/reviewState.ts @@ -7,7 +7,11 @@ import type { ReviewStateUpdateOutput, } from "@/common/orpc/schemas/reviewState"; import { REVIEW_STATE_SECTIONS } from "@/common/orpc/schemas/reviewState"; -import { applyReviewStateDelta, withReviewStateSection } from "@/common/utils/reviewState"; +import { + applyReviewStateDelta, + assignSection, + capReviewStateSections, +} from "@/common/utils/reviewState"; /** * Seeds consumed by the next mock client, so story setup can stage review state before @@ -93,7 +97,10 @@ export function createMockReviewStateApi() { results[section] = "present"; continue; } - next = withReviewStateSection(next, section, incoming); + next = { ...next }; + assignSection(next, section, incoming); + // ReviewStateService.importLegacy caps what it imports; keep the mock in step. + next = capReviewStateSections(next); results[section] = "applied"; } const snapshot = next === current ? { sections: current } : commit(input.workspaceId, next); diff --git a/src/cli/serverSecurity.test.ts b/src/cli/serverSecurity.test.ts deleted file mode 100644 index ec9dd16b167..00000000000 --- a/src/cli/serverSecurity.test.ts +++ /dev/null @@ -1,95 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { isLoopbackHost, resolveServerAuthToken } from "./serverSecurity"; - -describe("isLoopbackHost", () => { - test("recognizes localhost", () => { - expect(isLoopbackHost("localhost")).toBe(true); - }); - - test("recognizes 127.0.0.1", () => { - expect(isLoopbackHost("127.0.0.1")).toBe(true); - }); - - test("recognizes 127.x.y.z", () => { - expect(isLoopbackHost("127.1.2.3")).toBe(true); - }); - - test("recognizes ::1", () => { - expect(isLoopbackHost("::1")).toBe(true); - }); - - test("rejects 0.0.0.0", () => { - expect(isLoopbackHost("0.0.0.0")).toBe(false); - }); - - test("rejects LAN IP", () => { - expect(isLoopbackHost("192.168.1.100")).toBe(false); - }); - - test("case insensitive", () => { - expect(isLoopbackHost("LOCALHOST")).toBe(true); - }); - - test("rejects hostnames starting with 127. that are not IPs", () => { - expect(isLoopbackHost("127.example.com")).toBe(false); - expect(isLoopbackHost("127.0.0.evil")).toBe(false); - }); - - test("recognizes bracketed IPv6 loopback [::1]", () => { - expect(isLoopbackHost("[::1]")).toBe(true); - }); - - test("rejects 127-prefix with wrong octet count", () => { - expect(isLoopbackHost("127.0.0")).toBe(false); - expect(isLoopbackHost("127.0.0.1.2")).toBe(false); - }); - - test("rejects 127-prefix with out-of-range octets", () => { - expect(isLoopbackHost("127.0.0.256")).toBe(false); - }); -}); - -describe("resolveServerAuthToken", () => { - test("explicit token wins over everything", () => { - const result = resolveServerAuthToken("0.0.0.0", "my-token", "env-token"); - expect(result.token).toBe("my-token"); - expect(result.generated).toBe(false); - }); - - test("env token used when no explicit token", () => { - const result = resolveServerAuthToken("0.0.0.0", undefined, "env-token"); - expect(result.token).toBe("env-token"); - expect(result.generated).toBe(false); - }); - - test("loopback host without token returns undefined", () => { - const result = resolveServerAuthToken("127.0.0.1"); - expect(result.token).toBeUndefined(); - expect(result.generated).toBe(false); - }); - - test("localhost without token returns undefined", () => { - const result = resolveServerAuthToken("localhost"); - expect(result.token).toBeUndefined(); - expect(result.generated).toBe(false); - }); - - test("non-loopback host without token generates token", () => { - const result = resolveServerAuthToken("0.0.0.0"); - expect(result.token).toBeDefined(); - expect(typeof result.token).toBe("string"); - expect(result.token).toHaveLength(64); // 32 bytes hex - expect(result.generated).toBe(true); - }); - - test("generated tokens are unique", () => { - const a = resolveServerAuthToken("0.0.0.0"); - const b = resolveServerAuthToken("0.0.0.0"); - expect(a.token).not.toBe(b.token); - }); - - test("whitespace-only explicit token treated as absent", () => { - const result = resolveServerAuthToken("0.0.0.0", " ", undefined); - expect(result.generated).toBe(true); - }); -}); diff --git a/src/cli/serverSecurity.ts b/src/cli/serverSecurity.ts deleted file mode 100644 index 1ae008bc562..00000000000 --- a/src/cli/serverSecurity.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { randomBytes } from "crypto"; - -/** - * Check if a host string is a literal loopback address. - * - * Only matches literal IPv4/IPv6 loopback addresses and "localhost". - * Hostnames that happen to start with "127." (e.g., "127.example.com") are - * NOT treated as loopback — this matters for auth-token generation policy. - */ -export function isLoopbackHost(host: string): boolean { - const normalized = host.trim().toLowerCase(); - - // Strip IPv6 brackets if present (e.g., "[::1]" → "::1"). - const bare = - normalized.startsWith("[") && normalized.endsWith("]") ? normalized.slice(1, -1) : normalized; - - if (bare === "localhost" || bare === "::1") return true; - - // Match 127.x.y.z only for literal IPv4 addresses (4 numeric octets, 0-255). - // This prevents hostnames like "127.example.com" from being misclassified. - if (bare.startsWith("127.")) { - const parts = bare.split("."); - if (parts.length === 4 && parts.every((p) => /^\d{1,3}$/.test(p) && Number(p) <= 255)) { - return true; - } - } - - return false; -} - -export interface ResolvedAuthToken { - /** The token to use (undefined = no auth required). */ - token: string | undefined; - /** Whether this token was auto-generated (vs. explicitly provided). */ - generated: boolean; -} - -/** - * Resolve the effective auth token for the server. - * - * Policy: - * - If an explicit token or env var is provided, use it. - * - If the host is loopback and no token is provided, allow unauthenticated (local dev). - * - If the host is non-loopback and no token is provided, generate a secure ephemeral token. - */ -export function resolveServerAuthToken( - host: string, - explicitToken?: string, - envToken?: string -): ResolvedAuthToken { - const explicit = explicitToken?.trim(); - const env = envToken?.trim(); - const provided = explicit && explicit.length > 0 ? explicit : env && env.length > 0 ? env : null; - if (provided != null) return { token: provided, generated: false }; - if (isLoopbackHost(host)) return { token: undefined, generated: false }; - const token = randomBytes(32).toString("hex"); - return { token, generated: true }; -} diff --git a/src/common/utils/reviewState.ts b/src/common/utils/reviewState.ts index cc1711e9b70..09db3046586 100644 --- a/src/common/utils/reviewState.ts +++ b/src/common/utils/reviewState.ts @@ -104,17 +104,6 @@ export function assignSection( sections[section] = value; } -/** Replace one whole section (used by the legacy import); caps still apply. */ -export function withReviewStateSection( - sections: ReviewStateSections, - section: S, - value: NonNullable -): ReviewStateSections { - const next: ReviewStateSections = { ...sections }; - assignSection(next, section, value); - return capReviewStateSections(next); -} - /** Combines a key present on both sides; absent means last write wins. */ type EntryMerge = ((existing: V, incoming: V) => V) | undefined; diff --git a/src/common/utils/tools/toolCatalog.test.ts b/src/common/utils/tools/toolCatalog.test.ts index ea03bb6fe67..08746ef4a95 100644 --- a/src/common/utils/tools/toolCatalog.test.ts +++ b/src/common/utils/tools/toolCatalog.test.ts @@ -5,7 +5,6 @@ import type { ToolPolicy } from "@/common/utils/tools/toolPolicy"; import { applyNativeToolSearchReplay, buildToolCatalog, - buildToolCatalogOverview, buildToolSearchModelOutput, collectDeferLoadingToolNames, dedupeNativeToolReferences, @@ -64,6 +63,36 @@ function baseTools(): Record { }; } +/** + * The overview prepareToolSearch appends to tool_catalog_search's description when `catalog` is + * the deferred MCP surface. The search tool's own description is empty, so the result is the + * overview alone, and "" when nothing is deferred (prepareToolSearch then drops the tool). + */ +function advertisedOverview(catalog: readonly ToolCatalogEntry[]): string { + const prepared = prepareToolSearch({ + tools: { + tool_catalog_search: fakeTool(""), + ...Object.fromEntries( + catalog.map((entry) => [ + entry.name, + mcpTool( + entry.description, + entry.paramText ? { p: { description: entry.paramText } } : {} + ), + ]) + ), + }, + mcpToolNames: catalog.map((entry) => entry.name), + mcpToolServers: Object.fromEntries( + catalog.flatMap((entry) => + entry.serverName === undefined ? [] : [[entry.name, entry.serverName]] + ) + ), + }); + const description = prepared.tools[TOOL_SEARCH_TOOL_NAME]?.description; + return typeof description === "string" ? description : ""; +} + /** * baseTools() with deferred definitions large enough for native deferral * (#5405); the padding is neutral prose so search scoring is unaffected. @@ -150,13 +179,13 @@ describe("catalog advertising (overview + server names)", () => { expect(byName.get("github_create_issue")).toBe("github"); }); - test("buildToolCatalogOverview groups deferred tool names by server, deterministically", () => { + test("the overview groups deferred tool names by server, deterministically", () => { const { catalog } = buildToolCatalog({ tools: baseTools(), mcpToolNames: MCP_NAMES, mcpToolServers: MCP_TOOL_SERVERS, }); - const overview = buildToolCatalogOverview(catalog); + const overview = advertisedOverview(catalog); expect(overview).toBe( "Deferred tool catalog overview (search to activate):\n" + "- github: github_create_issue\n" + @@ -172,7 +201,7 @@ describe("catalog advertising (overview + server names)", () => { paramText: `SECRET_PARAM_${i}`, serverName: "srv", })); - const overview = buildToolCatalogOverview(catalog); + const overview = advertisedOverview(catalog); expect(overview).toContain( "- srv: srv_tool_0, srv_tool_1, srv_tool_2, srv_tool_3, srv_tool_4, srv_tool_5, srv_tool_6, srv_tool_7, and more\n" ); @@ -181,10 +210,8 @@ describe("catalog advertising (overview + server names)", () => { }); test("overview is empty for an empty catalog and groups unknown servers as 'other tools'", () => { - expect(buildToolCatalogOverview([])).toBe(""); - const overview = buildToolCatalogOverview([ - { name: "mystery_tool", description: "", paramText: "" }, - ]); + expect(advertisedOverview([])).toBe(""); + const overview = advertisedOverview([{ name: "mystery_tool", description: "", paramText: "" }]); expect(overview).toContain("- other tools: mystery_tool"); }); @@ -301,7 +328,7 @@ describe("catalog advertising (overview + server names)", () => { const clampedCatalog: ToolCatalogEntry[] = [ { name: "long_tool", description: "", paramText: "", serverName: longServer }, ]; - const clamped = buildToolCatalogOverview(clampedCatalog); + const clamped = advertisedOverview(clampedCatalog); expect(clamped).not.toContain(longServer); expect(clamped).toContain(`${"s".repeat(63)}…`); @@ -312,7 +339,7 @@ describe("catalog advertising (overview + server names)", () => { paramText: "", serverName: `server-${String(i).padStart(3, "0")}`, })); - const bounded = buildToolCatalogOverview(bigCatalog); + const bounded = advertisedOverview(bigCatalog); expect(bounded.length).toBeLessThan(2200); expect(bounded).toMatch(/…and \d+ more servers \(use this tool to discover their tools\)/); }); @@ -404,7 +431,7 @@ describe("catalog advertising (overview + server names)", () => { paramText: "", serverName: evilServer, })); - const overview = buildToolCatalogOverview(catalog); + const overview = advertisedOverview(catalog); const matches = searchToolCatalog(catalog, "evil"); const elapsedMs = performance.now() - startedAt; // Bounded work: must not stall stream startup (generous CI headroom). @@ -421,7 +448,7 @@ describe("catalog advertising (overview + server names)", () => { const catalog: ToolCatalogEntry[] = [ { name: "t1_do_thing", description: "unrelated words", paramText: "", serverName: longToken }, ]; - const overview = buildToolCatalogOverview(catalog); + const overview = advertisedOverview(catalog); const advertisedLabel = `${"a".repeat(63)}…`; expect(overview).toContain(advertisedLabel); // Querying with the visible label (as a model would copy it) must match. diff --git a/src/common/utils/tools/toolCatalog.ts b/src/common/utils/tools/toolCatalog.ts index c33d540fe7e..b62063b7c1c 100644 --- a/src/common/utils/tools/toolCatalog.ts +++ b/src/common/utils/tools/toolCatalog.ts @@ -402,6 +402,13 @@ function groupCatalogByServer(catalog: readonly ToolCatalogEntry[]): Array<[stri return [...byServer.entries()].sort(([a], [b]) => a.localeCompare(b)); } +/** + * Build a compact, deterministic index of the deferred catalog grouped by MCP + * server, e.g. `- github: github_create_issue, …, and more`. + * Only names are listed — descriptions and parameter schemas stay deferred + * until activation, so the overview solves the "unknown unknowns" discovery + * problem without reintroducing schema token bloat. + */ function buildOverview(catalog: readonly ToolCatalogEntry[]): ToolCatalogOverview { const servers = groupCatalogByServer(catalog); const lines: string[] = []; @@ -440,18 +447,6 @@ function buildOverview(catalog: readonly ToolCatalogEntry[]): ToolCatalogOvervie }; } -/** - * Build a compact, deterministic index of the deferred catalog grouped by MCP - * server, e.g. `- github: github_create_issue, …, and more`. - * Only names are listed — descriptions and parameter schemas stay deferred - * until activation, so the overview solves the "unknown unknowns" discovery - * problem without reintroducing schema token bloat. Returns "" for an empty - * catalog. - */ -export function buildToolCatalogOverview(catalog: readonly ToolCatalogEntry[]): string { - return catalog.length === 0 ? "" : buildOverview(catalog).text; -} - /** * A previous overview stays valid while none of its claims became false: * - the server set is unchanged, so every line's server still exists, the diff --git a/src/node/runtime/runtimeScratchDir.test.ts b/src/node/runtime/runtimeScratchDir.test.ts index d25c945a0ed..2f320811585 100644 --- a/src/node/runtime/runtimeScratchDir.test.ts +++ b/src/node/runtime/runtimeScratchDir.test.ts @@ -229,6 +229,18 @@ describe("runtimeScratchDir", () => { expect(await fs.readdir(tempDir)).toEqual([]); }); + test("still returns a host dir when creation fails, so the prompt never names an unset variable", async () => { + // A regular file where the session dir should be makes mkdir fail. + const sessionsRoot = path.join(tempDir, "sessions"); + await fs.mkdir(sessionsRoot); + await fs.writeFile(path.join(sessionsRoot, "ws1"), "not a dir"); + const dir = getWorkspaceScratchDir(sessionsRoot, "ws1"); + + expect(await ensureScratchDirForSpec(new LocalRuntime(tempDir), { kind: "host", dir })).toBe( + dir + ); + }); + test("creates a runtime-side dir and returns the path the runtime expanded", async () => { const runtime = new LocalRuntime(tempDir); const scratch = path.join(tempDir, "remote-home", RUNTIME_SCRATCH_DIR_NAME, "ws1"); diff --git a/src/node/runtime/runtimeScratchDir.ts b/src/node/runtime/runtimeScratchDir.ts index 97ab6d4fc9e..9149853031e 100644 --- a/src/node/runtime/runtimeScratchDir.ts +++ b/src/node/runtime/runtimeScratchDir.ts @@ -149,9 +149,11 @@ export async function canBindMountHostPathsIntoContainers( * Create the workspace's scratch dir where its commands run and return the absolute path to * export as XUM_SCRATCH_DIR, or undefined when this runtime has none (the variable stays unset). * - * Host dirs are returned even when mkdir fails (see ensureWorkspaceScratchDir). Remote dirs are - * returned only once the runtime confirmed them: an exported path the agent cannot write to is - * worse than none. + * Host dirs are returned even when mkdir fails (logged), because the environment prompt always + * tells local/worktree agents to use $XUM_SCRATCH_DIR: an unset variable would turn + * `mkdir -p "$XUM_SCRATCH_DIR/logs"` into `/logs`, and a set-but-missing dir just makes the + * agent's own `mkdir -p` create it. Remote dirs are returned only once the runtime confirmed + * them: an exported path the agent cannot write to is worse than none. */ export async function ensureScratchDirForSpec( runtime: Runtime, diff --git a/src/node/runtime/workspaceScratchDir.test.ts b/src/node/runtime/workspaceScratchDir.test.ts index bc9ebc2e7f5..d935418e655 100644 --- a/src/node/runtime/workspaceScratchDir.test.ts +++ b/src/node/runtime/workspaceScratchDir.test.ts @@ -1,28 +1,14 @@ -import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import * as fs from "fs/promises"; -import * as os from "os"; +import { describe, expect, test } from "bun:test"; import * as path from "path"; -import { ensureWorkspaceScratchDir, getWorkspaceScratchDir } from "./workspaceScratchDir"; +import { getWorkspaceScratchDir } from "./workspaceScratchDir"; -describe("ensureWorkspaceScratchDir", () => { - let tempDir: string; +describe("getWorkspaceScratchDir", () => { + test("puts the dir inside the workspace session dir, so removal deletes it", () => { + const sessionsDir = path.join(path.sep, "xum", "sessions"); - beforeEach(async () => { - tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "workspace-scratch-dir-")); - }); - - afterEach(async () => { - await fs.rm(tempDir, { recursive: true, force: true }); - }); - - test("creates the dir inside the workspace session dir, so removal deletes it", async () => { - const sessionsDir = path.join(tempDir, "sessions"); - - const scratchDir = await ensureWorkspaceScratchDir(sessionsDir, "ws1"); - - expect(scratchDir).toBe(getWorkspaceScratchDir(sessionsDir, "ws1")); - expect(path.dirname(scratchDir)).toBe(path.join(sessionsDir, "ws1")); - expect((await fs.stat(scratchDir)).isDirectory()).toBe(true); + expect(path.dirname(getWorkspaceScratchDir(sessionsDir, "ws1"))).toBe( + path.join(sessionsDir, "ws1") + ); }); test("exports an absolute path even when the Xum root is relative", () => { @@ -31,15 +17,4 @@ describe("ensureWorkspaceScratchDir", () => { expect(path.isAbsolute(scratchDir)).toBe(true); expect(scratchDir).toBe(path.resolve(".xum-test", "sessions", "ws1", "scratch")); }); - - test("still returns the path when creation fails, so the prompt never names an unset variable", async () => { - // A regular file where the session dir should be makes mkdir fail. - const sessionsDir = path.join(tempDir, "sessions"); - await fs.mkdir(sessionsDir); - await fs.writeFile(path.join(sessionsDir, "ws1"), "not a dir"); - - expect(await ensureWorkspaceScratchDir(sessionsDir, "ws1")).toBe( - getWorkspaceScratchDir(sessionsDir, "ws1") - ); - }); }); diff --git a/src/node/runtime/workspaceScratchDir.ts b/src/node/runtime/workspaceScratchDir.ts index c870bf04207..d7a2e4b1e05 100644 --- a/src/node/runtime/workspaceScratchDir.ts +++ b/src/node/runtime/workspaceScratchDir.ts @@ -1,8 +1,5 @@ -import * as fsPromises from "fs/promises"; import * as path from "path"; import { assert } from "@/common/utils/assert"; -import { getErrorMessage } from "@/common/utils/errors"; -import { log } from "@/node/services/log"; /** * Durable, Xum-owned scratch dir for a workspace's helper scripts, logs and evidence. @@ -19,22 +16,3 @@ export function getWorkspaceScratchDir(sessionsDir: string, workspaceId: string) // because agent commands run with a different cwd. return path.resolve(sessionsDir, workspaceId, "scratch"); } - -/** - * Create the scratch dir if needed and return it. Creation is best-effort: the path is returned - * even when mkdir fails (logged), because the environment prompt always tells local/worktree - * agents to use $XUM_SCRATCH_DIR. An unset variable would turn `mkdir -p "$XUM_SCRATCH_DIR/logs"` - * into `/logs`; a set-but-missing dir just makes the agent's own `mkdir -p` create it. - */ -export async function ensureWorkspaceScratchDir( - sessionsDir: string, - workspaceId: string -): Promise { - const scratchDir = getWorkspaceScratchDir(sessionsDir, workspaceId); - try { - await fsPromises.mkdir(scratchDir, { recursive: true }); - } catch (error) { - log.warn(`Could not create workspace scratch dir ${scratchDir}: ${getErrorMessage(error)}`); - } - return scratchDir; -}