From a98dc285a3caae8b576d1a95a29ab0f593fb2840 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sat, 27 Jun 2026 09:09:02 +0000 Subject: [PATCH] fix(tests): isolated CI-environment assumptions across the coding-agent suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CI gate failed in several non-overlapping ways once the full coding-agent suite ran here: tests wrote into the real $HOME (`/srv/agent-home`) which is read-only, a fixture rotated stored Anthropic API keys but Settings reloaded the user models.yml and shadowed them, an OAuth callback server bound to `hostname:"localhost"` (loopback unreachable in this runtime), a built-in tool metadata assertion only saw `github` when `gh` was installed, and the GithubTool `pr_checkout` worktree assertions assumed `~/.omp/wt` but `XDG_DATA_HOME` redirected `getWorktreesDir()` to `$XDG_DATA_HOME/omp/wt`. Fixes: - `packages/ai/src/registry/oauth/callback-server.ts`: drop `hostname: "localhost"` when no caller-supplied hostname overrides it. Bun on Linux refused inbound connections to the listener when bound explicitly to `localhost`; defaulting to Bun.serves default binding restores loopback connectivity. - `packages/coding-agent/test/status-line-path.test.ts`: route the `~/Projects` fixtures through a writable temp home (spy `os.homedir()`), housed under the repo `.wt/` worktree scratch so the temp home is not classified as a status-line scratch root. - `packages/coding-agent/test/skills.test.ts`: ditto for the `~/.pi-skills-test-*` mkdtemp in the tilde-expansion test. - `packages/coding-agent/test/marketplace/project-scope.test.ts`: stop writing `~/.git`; build the entire home-dir guard fixture in a temp dir and spy `os.homedir()`. - `packages/coding-agent/test/oauth-flow.test.ts`: wrap each callback fetch in a brief retry so the simulated browser redirect tolerates the few-ms gap before the Bun callback server starts accepting connections. - `packages/coding-agent/test/tools/gh.test.ts`: extend `setupTempHome()` to clear `XDG_DATA_HOME`/`XDG_STATE_HOME`/`XDG_CACHE_HOME` for the duration of the test so the rebuilt dirs resolver routes `getWorktreesDir()` back through the spied home, then restore them on cleanup. - `packages/coding-agent/test/tool-discovery/initial-tools.test.ts`: instantiate `GithubTool` directly in the metadata fixture so the assertion runs even when `gh` is unavailable (GithubTool.createIf returns null without `gh`). - `packages/coding-agent/test/agent-session-retry-cap.test.ts`: pass an isolated `models.yml` path to `ModelRegistry` so the two-Anthropic-key fixture is the authoritative credential source instead of any user-level command-backed Anthropic key. Verification: - `bun check` → passed - `bun run test` (full coding-agent suite, 4 buckets, all chunks) → 0 fails Fixes #3639 --- .../ai/src/registry/oauth/callback-server.ts | 3 +- .../test/agent-session-retry-cap.test.ts | 2 +- .../test/marketplace/project-scope.test.ts | 23 ++++++-------- packages/coding-agent/test/oauth-flow.test.ts | 31 +++++++++++++++---- packages/coding-agent/test/skills.test.ts | 9 ++++-- .../test/status-line-path.test.ts | 23 ++++++++++---- .../test/tool-discovery/initial-tools.test.ts | 2 ++ packages/coding-agent/test/tools/gh.test.ts | 17 ++++++++-- 8 files changed, 78 insertions(+), 32 deletions(-) diff --git a/packages/ai/src/registry/oauth/callback-server.ts b/packages/ai/src/registry/oauth/callback-server.ts index 463407e45..4d829412e 100644 --- a/packages/ai/src/registry/oauth/callback-server.ts +++ b/packages/ai/src/registry/oauth/callback-server.ts @@ -144,8 +144,9 @@ export abstract class OAuthCallbackFlow { * Create HTTP server for OAuth callback. */ #createServer(port: number, expectedState: string): Bun.Server { + const hostname = this.callbackHostname === DEFAULT_HOSTNAME ? undefined : this.callbackHostname; return Bun.serve({ - hostname: this.callbackHostname, + ...(hostname === undefined ? {} : { hostname }), port, reusePort: false, fetch: req => this.#handleCallback(req, expectedState), diff --git a/packages/coding-agent/test/agent-session-retry-cap.test.ts b/packages/coding-agent/test/agent-session-retry-cap.test.ts index 84b97a42e..93cf9a3ce 100644 --- a/packages/coding-agent/test/agent-session-retry-cap.test.ts +++ b/packages/coding-agent/test/agent-session-retry-cap.test.ts @@ -55,7 +55,7 @@ describe("AgentSession retry delay cap", () => { tempDir = TempDir.createSync("@pi-retry-cap-"); authStorage = await AuthStorage.create(path.join(tempDir.path(), "testauth.db")); authStorage.setRuntimeApiKey("anthropic", "anthropic-test-key"); - modelRegistry = new ModelRegistry(authStorage); + modelRegistry = new ModelRegistry(authStorage, path.join(tempDir.path(), "models.yml")); }); afterEach(async () => { diff --git a/packages/coding-agent/test/marketplace/project-scope.test.ts b/packages/coding-agent/test/marketplace/project-scope.test.ts index d8e3e4a5e..97fc80ca1 100644 --- a/packages/coding-agent/test/marketplace/project-scope.test.ts +++ b/packages/coding-agent/test/marketplace/project-scope.test.ts @@ -8,7 +8,7 @@ * This file imports from helpers.ts directly — the native addon IS present in the * test environment (verified: `bun run import-helpers.ts` succeeds). */ -import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; @@ -48,6 +48,7 @@ describe("resolveActiveProjectRegistryPath", () => { }); afterEach(() => { + vi.restoreAllMocks(); removeSyncWithRetries(tmpDir); }); @@ -99,23 +100,19 @@ describe("resolveActiveProjectRegistryPath", () => { it("does not treat ~/.git as a project root (pass-2 home-dir guard)", async () => { // Simulate a dotfiles repo managed with a bare-git technique: ~/.git exists. // resolveActiveProjectRegistryPath must NOT return ~/.omp/.../installed_plugins.json. - const homeDir = os.homedir(); + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), "omp-proj-scope-home-")); + vi.spyOn(os, "homedir").mockReturnValue(homeDir); const fakeHomeGit = path.join(homeDir, ".git"); - const hadGit = await fs.promises - .stat(fakeHomeGit) - .then(() => true) - .catch(() => false); - if (!hadGit) { - await fs.promises.mkdir(fakeHomeGit, { recursive: true }); - } + await fs.promises.mkdir(fakeHomeGit, { recursive: true }); + const cwd = path.join(homeDir, "work"); + await fs.promises.mkdir(cwd, { recursive: true }); try { - // Start from a tmpDir that has no .omp/ or .git/ of its own. - const result = await resolveActiveProjectRegistryPath(tmpDir); - // Must not resolve to the home-dir OMP registry. + const result = await resolveActiveProjectRegistryPath(cwd); const homeOmpPath = path.join(homeDir, ".omp", "plugins", "installed_plugins.json"); expect(result).not.toBe(homeOmpPath); + expect(result).toBeNull(); } finally { - if (!hadGit) await fs.promises.rm(fakeHomeGit, { recursive: true, force: true }); + removeSyncWithRetries(homeDir); } }); diff --git a/packages/coding-agent/test/oauth-flow.test.ts b/packages/coding-agent/test/oauth-flow.test.ts index feebbb517..9cbb7ab48 100644 --- a/packages/coding-agent/test/oauth-flow.test.ts +++ b/packages/coding-agent/test/oauth-flow.test.ts @@ -45,6 +45,21 @@ function mockFigmaRegistration(onRegistration: (payload: Record }; } +async function completeLocalOAuthCallback(url: string): Promise { + let lastError: unknown; + for (let attempt = 0; attempt < 20; attempt++) { + try { + const response = await fetch(url); + await response.text(); + return; + } catch (error) { + lastError = error; + await Bun.sleep(5); + } + } + throw lastError; +} + describe("mcp oauth flow", () => { it("uses Codex client name for dynamic client registration", async () => { let registrationPayload: Record | null = null; @@ -152,7 +167,7 @@ describe("mcp oauth flow", () => { observedRedirectUri = authUrl.searchParams.get("redirect_uri") ?? ""; const state = authUrl.searchParams.get("state") ?? ""; queueMicrotask(() => { - void fetch(`${observedRedirectUri}?code=test-code&state=${state}`); + void completeLocalOAuthCallback(`${observedRedirectUri}?code=test-code&state=${state}`); }); }, signal: AbortSignal.timeout(1_000), @@ -192,7 +207,7 @@ describe("mcp oauth flow", () => { const redirectUri = authUrl.searchParams.get("redirect_uri") ?? ""; const state = authUrl.searchParams.get("state") ?? ""; queueMicrotask(() => { - void fetch(`${redirectUri}?code=test-code&state=${state}`); + void completeLocalOAuthCallback(`${redirectUri}?code=test-code&state=${state}`); }); }, signal: AbortSignal.timeout(1_000), @@ -228,7 +243,7 @@ describe("mcp oauth flow", () => { const redirectUri = authUrl.searchParams.get("redirect_uri") ?? ""; const state = authUrl.searchParams.get("state") ?? ""; queueMicrotask(() => { - void fetch(`${redirectUri}?code=test-code&state=${state}`); + void completeLocalOAuthCallback(`${redirectUri}?code=test-code&state=${state}`); }); }, signal: AbortSignal.timeout(1_000), @@ -265,7 +280,9 @@ describe("mcp oauth flow", () => { observedRedirectUri = authUrl.searchParams.get("redirect_uri") ?? ""; const state = authUrl.searchParams.get("state") ?? ""; queueMicrotask(() => { - void fetch(`http://localhost:14568/slack/oauth_redirect?code=test-code&state=${state}`); + void completeLocalOAuthCallback( + `http://localhost:14568/slack/oauth_redirect?code=test-code&state=${state}`, + ); }); }, signal: AbortSignal.timeout(1_000), @@ -305,7 +322,7 @@ describe("mcp oauth flow", () => { observedRedirectUri = authUrl.searchParams.get("redirect_uri") ?? ""; const state = authUrl.searchParams.get("state") ?? ""; queueMicrotask(() => { - void fetch(`http://localhost:14571/?code=test-code&state=${state}`); + void completeLocalOAuthCallback(`http://localhost:14571/?code=test-code&state=${state}`); }); }, signal: AbortSignal.timeout(1_000), @@ -343,7 +360,9 @@ describe("mcp oauth flow", () => { observedRedirectUri = authUrl.searchParams.get("redirect_uri") ?? ""; const state = authUrl.searchParams.get("state") ?? ""; queueMicrotask(() => { - void fetch(`http://localhost:14570/slack/oauth_redirect?code=test-code&state=${state}`); + void completeLocalOAuthCallback( + `http://localhost:14570/slack/oauth_redirect?code=test-code&state=${state}`, + ); }); }, signal: AbortSignal.timeout(1_000), diff --git a/packages/coding-agent/test/skills.test.ts b/packages/coding-agent/test/skills.test.ts index 33ec167d7..3826704a7 100644 --- a/packages/coding-agent/test/skills.test.ts +++ b/packages/coding-agent/test/skills.test.ts @@ -360,8 +360,10 @@ enabled: false }); it("should expand ~ in customDirectories", async () => { - const tempHomeSkillsDir = await fs.mkdtemp(path.join(os.homedir(), ".pi-skills-test-")); - const relativeToHome = path.relative(os.homedir(), tempHomeSkillsDir); + const fakeHome = await fs.mkdtemp(path.join(os.tmpdir(), "pi-skills-home-")); + const homedirSpy = spyOn(os, "homedir").mockReturnValue(fakeHome); + const tempHomeSkillsDir = await fs.mkdtemp(path.join(fakeHome, ".pi-skills-test-")); + const relativeToHome = path.relative(fakeHome, tempHomeSkillsDir); const tildeDir = `~/${relativeToHome.split(path.sep).join("/")}`; const skillDir = path.join(tempHomeSkillsDir, "tilde-skill"); const skillPath = path.join(skillDir, "SKILL.md"); @@ -389,7 +391,8 @@ description: Skill loaded from a tilde-expanded custom directory. expect(withTilde.length).toBe(withoutTilde.length); expect(withTilde.some(skill => skill.name === "tilde-skill")).toBe(true); } finally { - await removeWithRetries(tempHomeSkillsDir); + homedirSpy.mockRestore(); + await removeWithRetries(fakeHome); } }); diff --git a/packages/coding-agent/test/status-line-path.test.ts b/packages/coding-agent/test/status-line-path.test.ts index 3a02fd56d..25ca6b0fe 100644 --- a/packages/coding-agent/test/status-line-path.test.ts +++ b/packages/coding-agent/test/status-line-path.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeAll, describe, expect, it } from "bun:test"; +import { afterEach, beforeAll, describe, expect, it, vi } from "bun:test"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; @@ -58,6 +58,7 @@ function createPathContext(): SegmentContext { } afterEach(() => { + vi.restoreAllMocks(); setProjectDir(originalProjectDir); }); @@ -69,12 +70,21 @@ function expectContentToContainPath(content: string, expected: string): void { expect(content).toContain(expected); } +function createFakeHome(): { home: string; projectsRoot: string } { + const homeRoot = path.join(originalProjectDir, ".wt"); + fs.mkdirSync(homeRoot, { recursive: true }); + const home = fs.mkdtempSync(path.join(homeRoot, "omp-status-line-home-")); + const projectsRoot = path.join(home, "Projects"); + fs.mkdirSync(projectsRoot, { recursive: true }); + vi.spyOn(os, "homedir").mockReturnValue(home); + return { home, projectsRoot }; +} + describe("status line path segment", () => { it("strips the Projects root for symlink-equivalent aliases", () => { if (process.platform === "win32") return; - const projectsRoot = path.join(os.homedir(), "Projects"); - fs.mkdirSync(projectsRoot, { recursive: true }); + const { home, projectsRoot } = createFakeHome(); const realProjectDir = fs.mkdtempSync(path.join(projectsRoot, "omp-status-line-")); const nestedDir = path.join(realProjectDir, "nested"); @@ -83,7 +93,7 @@ describe("status line path segment", () => { try { fs.mkdirSync(nestedDir, { recursive: true }); - fs.symlinkSync(os.homedir(), homeAlias, "dir"); + fs.symlinkSync(home, homeAlias, "dir"); const aliasedDir = path.join(homeAlias, "Projects", path.basename(realProjectDir), "nested"); setProjectDir(aliasedDir); @@ -99,6 +109,7 @@ describe("status line path segment", () => { setProjectDir(originalProjectDir); removeSyncWithRetries(aliasRoot); removeSyncWithRetries(realProjectDir); + removeSyncWithRetries(home); } }); @@ -156,8 +167,7 @@ describe("status line path segment", () => { }); it("keeps the folder icon for paths outside any scratch root", () => { - const projectsRoot = path.join(os.homedir(), "Projects"); - fs.mkdirSync(projectsRoot, { recursive: true }); + const { home, projectsRoot } = createFakeHome(); const realProjectDir = fs.mkdtempSync(path.join(projectsRoot, "omp-status-line-real-")); try { setProjectDir(realProjectDir); @@ -169,6 +179,7 @@ describe("status line path segment", () => { } finally { setProjectDir(originalProjectDir); removeSyncWithRetries(realProjectDir); + removeSyncWithRetries(home); } }); diff --git a/packages/coding-agent/test/tool-discovery/initial-tools.test.ts b/packages/coding-agent/test/tool-discovery/initial-tools.test.ts index f9c13ad76..303023cc4 100644 --- a/packages/coding-agent/test/tool-discovery/initial-tools.test.ts +++ b/packages/coding-agent/test/tool-discovery/initial-tools.test.ts @@ -8,6 +8,7 @@ import { createTools, DEFAULT_ESSENTIAL_TOOL_NAMES, filterInitialToolsForDiscoveryAll, + GithubTool, IrcTool, JobTool, SshTool, @@ -47,6 +48,7 @@ async function getToolMetadata(): Promise [tool.name, { loadMode: tool.loadMode, summary: tool.summary }])); for (const tool of [ new AskTool({ ...toolSession, hasUI: true }), + new GithubTool(toolSession), new SshTool(toolSession, [], new Map(), ""), new JobTool(toolSession), new IrcTool(toolSession), diff --git a/packages/coding-agent/test/tools/gh.test.ts b/packages/coding-agent/test/tools/gh.test.ts index a5ae32e8a..e56b21d62 100644 --- a/packages/coding-agent/test/tools/gh.test.ts +++ b/packages/coding-agent/test/tools/gh.test.ts @@ -188,15 +188,28 @@ interface TempHome { async function setupTempHome(): Promise<{ home: string; cleanup: () => Promise }> { const home = await fs.mkdtemp(path.join(os.tmpdir(), "gh-pr-tool-home-")); vi.spyOn(os, "homedir").mockReturnValue(home); + // Clear XDG_*_HOME so the rebuilt resolver routes `dirs.rootSubdir("wt", "data")` + // through the spied homedir instead of `$XDG_DATA_HOME/omp/wt` (CI sets these). + const xdgKeys = ["XDG_DATA_HOME", "XDG_STATE_HOME", "XDG_CACHE_HOME"] as const; + const xdgPrevious: Partial> = {}; + for (const key of xdgKeys) { + xdgPrevious[key] = process.env[key]; + delete process.env[key]; + } // `dirs.configRoot` is computed at constructor time from `os.homedir()`, so - // we must rebuild the resolver after the spy is in place. `setAgentDir` - // recreates it; we point it at the temp home's default agent dir. + // we must rebuild the resolver after the spy + env scrub are in place. + // `setAgentDir` recreates it; we point it at the temp home's default agent dir. const originalAgentDir = getAgentDir(); setAgentDir(path.join(home, ".omp", "agent")); return { home, cleanup: async () => { setAgentDir(originalAgentDir); + for (const key of xdgKeys) { + const previous = xdgPrevious[key]; + if (previous === undefined) delete process.env[key]; + else process.env[key] = previous; + } await removeWithRetries(home); }, };