diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.resume-model.test.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.resume-model.test.ts index 5b5d535d0a8e..f89942ab5d93 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.resume-model.test.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.resume-model.test.ts @@ -69,6 +69,16 @@ vi.mock("./mcp/tool-metadata", () => ({ getMcpToolMetadata: vi.fn().mockReturnValue(undefined), })); +// Returns a truthy stub so the rest of the suite doesn't trip the "cloud run +// registered no local tools" warning. +const createLocalToolsMcpServer = vi.hoisted(() => + vi.fn(() => ({ instance: {} })), +); + +vi.mock("./mcp/local-tools", () => ({ + createLocalToolsMcpServer, +})); + // Import after the mocks so ClaudeAcpAgent resolves the mocked SDK const { ClaudeAcpAgent } = await import("./claude-agent"); type Agent = InstanceType; @@ -135,6 +145,7 @@ describe("ClaudeAcpAgent session creation", () => { commands: [], models: [], }); + createLocalToolsMcpServer.mockClear(); // No gateway: fetchGatewayModels returns [] and the requested model is // kept as a custom option — mirrors the gateway-outage failure mode. delete process.env.ANTHROPIC_BASE_URL; @@ -289,6 +300,28 @@ describe("ClaudeAcpAgent session creation", () => { }, ); + it("passes repository-less mode to the local-tools server", async () => { + const agent = makeAgent(); + + await agent.newSession({ + cwd, + mcpServers: [], + _meta: { + environment: "cloud", + channelMode: true, + taskRunId: "run-channel", + }, + }); + + expect(createLocalToolsMcpServer).toHaveBeenCalledWith( + expect.objectContaining({ cwd }), + expect.objectContaining({ + environment: "cloud", + channelMode: true, + }), + ); + }); + // The SDK does not carry the model across resume — without an explicit // setModel the resumed session silently runs the SDK default (opus). it.each([ diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts index e07a2c404fe4..b0984bac654a 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts @@ -1979,6 +1979,7 @@ export class ClaudeAcpAgent extends BaseAcpAgent { // needs so the session doesn't pin the whole meta object. const baseBranch = meta?.baseBranch; const environment = meta?.environment; + const channelMode = meta?.channelMode; const spokenNarration = resolveSpokenNarration(meta); const requestFinish = this.buildRequestFinish(taskId, meta?.taskRunId); const buildInProcessMcpServers = (): Record< @@ -1996,6 +1997,7 @@ export class ClaudeAcpAgent extends BaseAcpAgent { }, { environment, + channelMode, spokenNarration, background: meta?.mode === "background", }, diff --git a/products/desktop/packages/agent/src/adapters/claude/mcp/local-tools.test.ts b/products/desktop/packages/agent/src/adapters/claude/mcp/local-tools.test.ts index 25254322451b..ed66813da98a 100644 --- a/products/desktop/packages/agent/src/adapters/claude/mcp/local-tools.test.ts +++ b/products/desktop/packages/agent/src/adapters/claude/mcp/local-tools.test.ts @@ -79,4 +79,27 @@ describe("createLocalToolsMcpServer", () => { await client.close(); }); + + it("exposes lazy repository tools in a repository-less cloud run", async () => { + const server = createLocalToolsMcpServer( + { cwd: "/tmp/workspace", token: "ghs_x" }, + { environment: "cloud", channelMode: true }, + ); + if (!server) { + throw new Error("expected the local-tools server to be registered"); + } + + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + await server.instance.connect(serverTransport); + const client = new Client({ name: "test", version: "1.0.0" }); + await client.connect(clientTransport); + + const { tools } = await client.listTools(); + const names = tools.map((tool) => tool.name); + expect(names).toContain("list_repos"); + expect(names).toContain("clone_repo"); + + await client.close(); + }); }); diff --git a/products/desktop/packages/agent/src/adapters/claude/tools.test.ts b/products/desktop/packages/agent/src/adapters/claude/tools.test.ts index 6f87485db47a..0bb2ff5fa295 100644 --- a/products/desktop/packages/agent/src/adapters/claude/tools.test.ts +++ b/products/desktop/packages/agent/src/adapters/claude/tools.test.ts @@ -18,17 +18,26 @@ describe("toSdkPermissionMode", () => { }); describe("isToolAllowedForMode stays authoritative for auto", () => { - it.each(["Bash", "Edit", "Write", "NotebookEdit", "BashOutput", "KillShell"])( - "auto-allows %s in auto mode", - (tool) => { - expect(isToolAllowedForMode(tool, "auto")).toBe(true); - }, - ); + it.each([ + "Bash", + "Edit", + "Write", + "NotebookEdit", + "BashOutput", + "KillShell", + "mcp__posthog-code-tools__list_repos", + "mcp__posthog-code-tools__clone_repo", + ])("auto-allows %s in auto mode", (tool) => { + expect(isToolAllowedForMode(tool, "auto")).toBe(true); + }); - it.each(["Bash", "Edit", "Write"])( - "still gates %s in default mode", - (tool) => { - expect(isToolAllowedForMode(tool, "default")).toBe(false); - }, - ); + it.each([ + "Bash", + "Edit", + "Write", + "mcp__posthog-code-tools__list_repos", + "mcp__posthog-code-tools__clone_repo", + ])("still gates %s in default mode", (tool) => { + expect(isToolAllowedForMode(tool, "default")).toBe(false); + }); }); diff --git a/products/desktop/packages/agent/src/adapters/claude/tools.ts b/products/desktop/packages/agent/src/adapters/claude/tools.ts index 1911b037fdc0..97fd18e60e55 100644 --- a/products/desktop/packages/agent/src/adapters/claude/tools.ts +++ b/products/desktop/packages/agent/src/adapters/claude/tools.ts @@ -43,6 +43,11 @@ const BASE_ALLOWED_TOOLS = [ ...AGENT_TOOLS, ]; +const AUTO_ALLOWED_LOCAL_TOOLS = [ + "mcp__posthog-code-tools__list_repos", + "mcp__posthog-code-tools__clone_repo", +]; + const AUTO_ALLOWED_TOOLS: Record> = { // Auto mode is hands-off: it auto-approves file edits and shell commands on // top of the base read/search/web/agent tools. Without WRITE_TOOLS and @@ -50,7 +55,12 @@ const AUTO_ALLOWED_TOOLS: Record> = { // call, which contradicts what the mode advertises. MCP tools are still gated // separately (do_not_use is denied, needs_approval still prompts) in // canUseTool, so auto stays narrower than bypassPermissions. - auto: new Set([...BASE_ALLOWED_TOOLS, ...WRITE_TOOLS, ...BASH_TOOLS]), + auto: new Set([ + ...BASE_ALLOWED_TOOLS, + ...WRITE_TOOLS, + ...BASH_TOOLS, + ...AUTO_ALLOWED_LOCAL_TOOLS, + ]), default: new Set(BASE_ALLOWED_TOOLS), acceptEdits: new Set([...BASE_ALLOWED_TOOLS, ...WRITE_TOOLS]), plan: new Set(BASE_ALLOWED_TOOLS), diff --git a/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.test.ts b/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.test.ts index 1dbfc4b87dd7..2938a770f48f 100644 --- a/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.test.ts +++ b/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.test.ts @@ -929,6 +929,106 @@ describe("CodexAppServerAgent", () => { expect(requestPermission).not.toHaveBeenCalled(); }); + it("auto-accepts repository tools from the built-in local MCP server in auto mode", async () => { + const stub = makeStubRpc({ + initialize: {}, + "thread/start": { thread: { id: "thr_1" } }, + }); + const requestPermission = vi.fn(); + const client = { + sessionUpdate: async () => {}, + requestPermission, + extNotification: async () => {}, + } as unknown as AgentSideConnection; + const agent = new CodexAppServerAgent(client, { + processOptions: { binaryPath: "/bundle/codex" }, + model: "gpt-5.5", + rpcFactory: stub.factory, + }); + await agent.initialize(init); + await agent.newSession({ + cwd: "/repo", + _meta: { + environment: "cloud", + channelMode: true, + permissionMode: "auto", + }, + } as unknown as NewSessionRequest); + + stub.emit("item/started", { + item: { + type: "mcpToolCall", + id: "m1", + server: "posthog-code-tools", + tool: "clone_repo", + arguments: { repo: "PostHog/posthog" }, + }, + }); + const decision = await stub.invokeRequest("mcpServer/elicitation/request", { + threadId: "thr_1", + turnId: "turn_1", + serverName: "posthog-code-tools", + mode: "form", + message: + 'Allow the posthog-code-tools MCP server to run tool "clone_repo"?', + }); + + expect(decision).toMatchObject({ action: "accept" }); + expect(requestPermission).not.toHaveBeenCalled(); + }); + + // The inverse of the auto-accept above: outside the hands-off modes a + // repository tool must still go through the permission prompt. + it("prompts for repository tools outside hands-off modes", async () => { + const stub = makeStubRpc({ + initialize: {}, + "thread/start": { thread: { id: "thr_1" } }, + }); + const requestPermission = vi.fn(async () => ({ + outcome: { outcome: "selected", optionId: "decline" }, + })); + const client = { + sessionUpdate: async () => {}, + requestPermission, + extNotification: async () => {}, + } as unknown as AgentSideConnection; + const agent = new CodexAppServerAgent(client, { + processOptions: { binaryPath: "/bundle/codex" }, + model: "gpt-5.5", + rpcFactory: stub.factory, + }); + await agent.initialize(init); + await agent.newSession({ + cwd: "/repo", + _meta: { + environment: "cloud", + channelMode: true, + permissionMode: "read-only", + }, + } as unknown as NewSessionRequest); + + stub.emit("item/started", { + item: { + type: "mcpToolCall", + id: "m1", + server: "posthog-code-tools", + tool: "clone_repo", + arguments: { repo: "PostHog/posthog" }, + }, + }); + const decision = await stub.invokeRequest("mcpServer/elicitation/request", { + threadId: "thr_1", + turnId: "turn_1", + serverName: "posthog-code-tools", + mode: "form", + message: + 'Allow the posthog-code-tools MCP server to run tool "clone_repo"?', + }); + + expect(requestPermission).toHaveBeenCalledOnce(); + expect(decision).toMatchObject({ action: "decline" }); + }); + it("auto-accepts a gated PostHog exec sub-tool in local hands-off modes", async () => { const stub = makeStubRpc({ initialize: {}, diff --git a/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts b/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts index 25e54abe5b46..b9fefdbc1f71 100644 --- a/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts +++ b/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts @@ -53,6 +53,7 @@ import { estimateTokens, } from "../claude/context-breakdown"; import { isLocalSkillCommandChunk } from "../local-skill"; +import { LOCAL_TOOLS_MCP_NAME } from "../local-tools"; import { resolveSpokenNarration } from "../session-meta"; import { AppServerClient, @@ -1939,6 +1940,22 @@ export class CodexAppServerAgent extends BaseAcpAgent { ); } + private shouldAutoAcceptMcpToolCall(mcp: { + server: string; + tool: string; + args: unknown; + }): boolean { + const isHandsOffMode = + this.config.mode === "auto" || this.config.mode === "full-access"; + const isRepositoryTool = + mcp.server === LOCAL_TOOLS_MCP_NAME && + (mcp.tool === "list_repos" || mcp.tool === "clone_repo"); + return ( + (isHandsOffMode && isRepositoryTool) || + this.shouldAutoAcceptPostHogExec(mcp) + ); + } + /** * Server-initiated requests. Simple approvals resolve to a `{ decision }` envelope (a bare * string is rejected); richer ones (AskUserQuestion / permission profile / elicitation) go @@ -1953,7 +1970,7 @@ export class CodexAppServerAgent extends BaseAcpAgent { logger: this.logger, resolveMcpToolCall: (serverName) => this.mcp.byServer(serverName), shouldAutoAcceptMcpToolCall: (mcp) => - this.shouldAutoAcceptPostHogExec(mcp), + this.shouldAutoAcceptMcpToolCall(mcp), }); if (richer.handled) { return richer.response; @@ -2001,7 +2018,7 @@ export class CodexAppServerAgent extends BaseAcpAgent { // Codex has no MCP-specific approval; a known MCP call surfaces the real server/tool/args // so the host renders the proper MCP permission (incl. PostHog `exec` unwrapping). const mcp = this.mcp.byItemId(detail.itemId); - if (mcp && this.shouldAutoAcceptPostHogExec(mcp)) { + if (mcp && this.shouldAutoAcceptMcpToolCall(mcp)) { return { decision: "accept" }; } // kind + content route plain command/file approvals to Execute/EditPermission (not the fallback). diff --git a/products/desktop/packages/agent/src/adapters/local-tools/tools/clone-repo.test.ts b/products/desktop/packages/agent/src/adapters/local-tools/tools/clone-repo.test.ts new file mode 100644 index 000000000000..b0b833af1d74 --- /dev/null +++ b/products/desktop/packages/agent/src/adapters/local-tools/tools/clone-repo.test.ts @@ -0,0 +1,262 @@ +import { existsSync } from "node:fs"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { pathToFileURL } from "node:url"; +import { execGit } from "@posthog/git/git-exec"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("../../../utils/github-token", () => ({ + resolveGithubToken: vi.fn(() => undefined), +})); + +const { cloneRepoTool, GITHUB_AUTH_CONFIG_KEY } = await import("./clone-repo"); + +const REPO_URL = "https://github.com/PostHog/posthog.git"; + +describe("clone_repo", () => { + let cwd: string; + let sourcePath: string; + let targetPath: string; + let previousConfigGlobal: string | undefined; + + async function git(args: string[], repoPath: string): Promise { + const result = await execGit(args, { cwd: repoPath }); + if (result.exitCode !== 0) { + throw new Error(result.stderr || result.error); + } + return result.stdout.trim(); + } + + beforeEach(async () => { + cwd = await mkdtemp(path.join(tmpdir(), "posthog-code-clone-tool-")); + sourcePath = path.join(cwd, "source"); + targetPath = path.join(cwd, "repos", "PostHog", "posthog"); + await mkdir(sourcePath, { recursive: true }); + + await git(["init", "--initial-branch=master", "."], sourcePath); + await git(["config", "user.name", "PostHog Code test"], sourcePath); + await git(["config", "user.email", "test@posthog.com"], sourcePath); + await git(["config", "commit.gpgsign", "false"], sourcePath); + await writeFile(path.join(sourcePath, "README.md"), "master\n"); + await git(["add", "README.md"], sourcePath); + await git(["commit", "-m", "initial commit"], sourcePath); + await git(["tag", "v1"], sourcePath); + await git(["checkout", "-b", "feature"], sourcePath); + await writeFile(path.join(sourcePath, "README.md"), "feature\n"); + await git(["add", "README.md"], sourcePath); + await git(["commit", "-m", "feature commit"], sourcePath); + await git(["checkout", "master"], sourcePath); + + // Serve github.com/PostHog/posthog from the local fixture. This lands in a + // config file rather than GIT_CONFIG_* env so the tool's own auth env (which + // takes GIT_CONFIG_COUNT) still applies on top of it. + const configPath = path.join(cwd, "gitconfig"); + await writeFile( + configPath, + `[url "${pathToFileURL(sourcePath).href}"]\n\tinsteadOf = ${REPO_URL}\n`, + ); + previousConfigGlobal = process.env.GIT_CONFIG_GLOBAL; + process.env.GIT_CONFIG_GLOBAL = configPath; + }); + + afterEach(async () => { + if (previousConfigGlobal === undefined) { + delete process.env.GIT_CONFIG_GLOBAL; + } else { + process.env.GIT_CONFIG_GLOBAL = previousConfigGlobal; + } + await rm(cwd, { recursive: true, force: true }); + }); + + it("clones one commit of the requested branch, without tags or a token in origin", async () => { + const result = await cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "PostHog/posthog", branch: "feature" }, + ); + + expect(result.isError).toBeUndefined(); + expect(await git(["rev-parse", "--abbrev-ref", "HEAD"], targetPath)).toBe( + "feature", + ); + expect( + await git(["rev-parse", "--is-shallow-repository"], targetPath), + ).toBe("true"); + expect(await git(["rev-list", "--count", "HEAD"], targetPath)).toBe("1"); + expect(await git(["tag", "--list"], targetPath)).toBe(""); + // The token rides in an http.extraHeader env var, so it must not have been + // persisted into the checkout's config the way a URL credential would be. + expect( + await git(["config", "--get", "remote.origin.url"], targetPath), + ).toBe(REPO_URL); + expect( + await git(["config", "--local", "--list"], targetPath), + ).not.toContain("test-token"); + }); + + // Regression: a tag leaves a detached HEAD, where rev-parse --abbrev-ref + // prints the literal "HEAD"; the message must name the requested ref. + it("reports the requested ref when checking out a tag", async () => { + const result = await cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "PostHog/posthog", branch: "v1" }, + ); + + expect(result.isError).toBeUndefined(); + expect(result.content[0].text).toContain("on branch v1"); + }); + + // A retargeted origin is what turns the missing-branch fetch below into a + // request to somewhere we never meant to talk to, carrying the token with it. + it.each([ + { + case: "carries embedded credentials", + origin: + "https://x-access-token:stale-token@github.com/PostHog/posthog.git", + }, + { + case: "points at another host", + origin: "https://evil.example.com/PostHog/posthog.git", + }, + ])("normalizes an existing clone origin that $case", async ({ origin }) => { + await mkdir(targetPath, { recursive: true }); + await git(["init", "."], targetPath); + await git(["remote", "add", "origin", origin], targetPath); + + const result = await cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "PostHog/posthog" }, + ); + + expect(result.isError).toBeUndefined(); + expect( + await git(["config", "--get", "remote.origin.url"], targetPath), + ).toBe(REPO_URL); + }); + + // Regression: an unscoped http.extraHeader is sent to every HTTP remote, so a + // fetch against a non-GitHub origin would hand over the live token. + it("scopes the auth header to github.com", async () => { + const env = { + GIT_CONFIG_COUNT: "1", + GIT_CONFIG_KEY_0: GITHUB_AUTH_CONFIG_KEY, + GIT_CONFIG_VALUE_0: "AUTHORIZATION: basic placeholder", + }; + const urlmatch = async (url: string): Promise => + (await execGit(["config", "--get-urlmatch", "http", url], { env })) + .stdout; + + expect(await urlmatch("https://github.com/PostHog/posthog.git")).toContain( + "AUTHORIZATION: basic", + ); + expect(await urlmatch("https://evil.example.com/x.git")).toBe(""); + }); + + // Regression: a slug like "PostHog/.." collapses through path.join onto the + // whole repos/ tree, and a clone failure there would rm -rf every prior + // checkout. The full input matrix lives in parseGithubUrl's own tests; this + // guards the tool's wiring to it. + it("rejects a traversal slug without touching the workspace", async () => { + const keptCheckout = path.join(cwd, "repos", "keep"); + await mkdir(keptCheckout, { recursive: true }); + + const result = await cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "git@github.com:PostHog/.." }, + ); + + expect(result.isError).toBe(true); + expect(existsSync(keptCheckout)).toBe(true); + }); + + // Regression: a reuse-path failure used to fail every subsequent call for + // the same repo, with no way to force a fresh clone. + it("re-clones a wedged checkout that holds no local work", async () => { + await mkdir(targetPath, { recursive: true }); + await git(["init", "."], targetPath); + + const result = await cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "PostHog/posthog" }, + ); + + expect(result.isError).toBeUndefined(); + expect( + await git(["rev-parse", "--is-shallow-repository"], targetPath), + ).toBe("true"); + }); + + // The self-heal above must never cost the agent uncommitted work. + it("keeps a wedged checkout that holds local work", async () => { + await mkdir(targetPath, { recursive: true }); + await git(["init", "."], targetPath); + const workPath = path.join(targetPath, "work-in-progress.md"); + await writeFile(workPath, "unsaved edits\n"); + + const result = await cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "PostHog/posthog" }, + ); + + expect(result.isError).toBe(true); + expect(existsSync(workPath)).toBe(true); + }); + + it("cleans up the target after a failed clone so a retry starts fresh", async () => { + const result = await cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "PostHog/posthog", branch: "does-not-exist" }, + ); + + expect(result.isError).toBe(true); + expect(existsSync(targetPath)).toBe(false); + }); + + // Regression: without per-target serialization the loser of the race rm -rfs + // the winner's in-progress checkout from its failure path. + it("serializes concurrent clones of the same repo", async () => { + const [first, second] = await Promise.all([ + cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "PostHog/posthog" }, + ), + cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "PostHog/posthog" }, + ), + ]); + + expect(first.isError).toBeUndefined(); + expect(second.isError).toBeUndefined(); + expect( + await git(["rev-parse", "--is-shallow-repository"], targetPath), + ).toBe("true"); + }); + + it("fetches a missing branch into an existing shallow clone", async () => { + await cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "PostHog/posthog", branch: "master" }, + ); + + const result = await cloneRepoTool.handler( + { cwd, token: "test-token" }, + { repo: "PostHog/posthog", branch: "feature" }, + ); + + expect(result.isError).toBeUndefined(); + expect(await git(["rev-parse", "--abbrev-ref", "HEAD"], targetPath)).toBe( + "feature", + ); + expect( + await git( + ["rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{upstream}"], + targetPath, + ), + ).toBe("origin/feature"); + expect( + await git(["rev-parse", "--is-shallow-repository"], targetPath), + ).toBe("true"); + expect(await git(["rev-list", "--count", "HEAD"], targetPath)).toBe("1"); + }); +}); diff --git a/products/desktop/packages/agent/src/adapters/local-tools/tools/clone-repo.ts b/products/desktop/packages/agent/src/adapters/local-tools/tools/clone-repo.ts index 41e7ab043008..6ccfb7f26dec 100644 --- a/products/desktop/packages/agent/src/adapters/local-tools/tools/clone-repo.ts +++ b/products/desktop/packages/agent/src/adapters/local-tools/tools/clone-repo.ts @@ -1,13 +1,26 @@ import * as fs from "node:fs"; +import * as fsPromises from "node:fs/promises"; import * as path from "node:path"; -import { createGitClient } from "@posthog/git/client"; -import { getCurrentBranch } from "@posthog/git/queries"; -import { CloneSaga } from "@posthog/git/sagas/clone"; +import { + execGit, + execGitWithRetry, + type GitExecResult, +} from "@posthog/git/git-exec"; import { parseGithubUrl } from "@posthog/git/utils"; import { z } from "zod"; import { resolveGithubToken } from "../../../utils/github-token"; import { defineLocalTool, type LocalToolResult } from "../registry"; +const GIT_TIMEOUT_MS = 10 * 60 * 1000; +const GITHUB_BASE_URL = "https://github.com/"; + +/** + * Scoping the auth header to github.com is what keeps the token from being + * sent to other hosts; an unscoped `http.extraHeader` rides along on every + * HTTP remote git talks to. + */ +export const GITHUB_AUTH_CONFIG_KEY = `http.${GITHUB_BASE_URL}.extraHeader`; + const cloneRepoSchema = { repo: z .string() @@ -26,10 +39,58 @@ function fail(text: string): LocalToolResult { return { content: [{ type: "text", text }], isError: true }; } +/** + * Carries the token as an `http.extraHeader` in the child's environment, so it + * never reaches `.git/config` the way a credential embedded in the remote URL + * would. + */ +function gitEnv(token: string | undefined): Record { + // Repos declaring `filter=lfs` fail outright when git-lfs isn't installed; + // skipping the smudge filter leaves pointer files instead. + const env: Record = { GIT_LFS_SKIP_SMUDGE: "1" }; + if (!token) return env; + const basicAuth = Buffer.from(`x-access-token:${token}`).toString("base64"); + return { + ...env, + GIT_CONFIG_COUNT: "1", + GIT_CONFIG_KEY_0: GITHUB_AUTH_CONFIG_KEY, + GIT_CONFIG_VALUE_0: `AUTHORIZATION: basic ${basicAuth}`, + }; +} + +/** + * Concurrent calls for the same repo (a parallel tool-call batch, a client + * retry racing a slow clone) would otherwise interleave on one checkout, and + * the failure-path cleanup could delete a directory the other call is still + * cloning into. + */ +const inFlight = new Map>(); + +function withCloneLock( + targetPath: string, + task: () => Promise, +): Promise { + const previous = inFlight.get(targetPath); + const current = previous ? previous.then(task, task) : task(); + inFlight.set(targetPath, current); + const cleanup = (): void => { + if (inFlight.get(targetPath) === current) inFlight.delete(targetPath); + }; + current.then(cleanup, cleanup); + return current; +} + /** * Lazily brings a repo into a repo-less channel session's scratch workspace. * Clones into `/repos/` (a subdir of the session cwd, so no session * restart / cwd rebind is needed) and reports the path for the agent to cd into. + * + * Goes straight to `git` rather than through the simple-git client and its + * clone saga: this only ever runs inside agent-server, against a scratch + * checkout the desktop client never touches, so the client's repo locking and + * rollback bookkeeping buy nothing (same-session calls are serialized by + * `withCloneLock`), and the raw subprocess passes `GIT_CONFIG_*` auth through + * as-is. */ export const cloneRepoTool = defineLocalTool({ name: "clone_repo", @@ -43,6 +104,7 @@ export const cloneRepoTool = defineLocalTool({ handler: async (ctx, args): Promise => { const { repo, branch } = args; const token = resolveGithubToken() ?? ctx.token; + const env = gitEnv(token); // Never surface the token to the model/transcript: git may echo the remote // URL (with its embedded basic-auth credential) into error output. @@ -50,8 +112,9 @@ export const cloneRepoTool = defineLocalTool({ token ? text.split(token).join("***") : text; // parseGithubUrl accepts owner/repo shorthand and full https/ssh URLs, - // validates the host, and normalizes away path traversal (a crafted URL - // can't escape the scratch workspace via the path.join below). + // validates the host, and rejects dot segments and unsafe characters in + // owner and repo, so a crafted URL can't escape the scratch workspace via + // the path.join below. const parsed = parseGithubUrl(repo); if (!parsed) { return fail( @@ -61,9 +124,36 @@ export const cloneRepoTool = defineLocalTool({ const slug = `${parsed.owner}/${parsed.repo}`; const repoName = parsed.repo; const targetPath = path.join(ctx.cwd, "repos", slug); + const cloneUrl = `${GITHUB_BASE_URL}${slug}.git`; + + const git = (gitArgs: string[], cwd?: string): Promise => + execGit(gitArgs, { cwd, env, timeoutMs: GIT_TIMEOUT_MS }); + + // For the network-touching calls (clone, fetch), so a transient blip + // retries with backoff instead of failing the whole tool call. + const gitWithRetry = ( + gitArgs: string[], + cwd?: string, + ): Promise => + execGitWithRetry(gitArgs, { cwd, env, timeoutMs: GIT_TIMEOUT_MS }); + + const run = async ( + gitArgs: string[], + cwd?: string, + exec: typeof git = git, + ): Promise => { + const result = await exec(gitArgs, cwd); + if (result.exitCode !== 0) { + throw new Error(result.stderr.trim() || result.error || "git failed"); + } + return result.stdout.trim(); + }; const done = async (note?: string): Promise => { - const checkedOut = (await getCurrentBranch(targetPath)) ?? branch ?? null; + const head = await git(["rev-parse", "--abbrev-ref", "HEAD"], targetPath); + const headRef = head.exitCode === 0 ? head.stdout.trim() : null; + const checkedOut = + headRef && headRef !== "HEAD" ? headRef : (branch ?? null); return { content: [ { @@ -76,52 +166,128 @@ export const cloneRepoTool = defineLocalTool({ }; }; + // The clone is single-branch, so a branch the agent asks for later isn't + // in the checkout yet — fetch it at depth 1 and register its refspec. const checkout = async (): Promise => { if (!branch) return null; + const local = await git(["checkout", branch], targetPath); + if (local.exitCode === 0) return null; try { - await createGitClient(targetPath).checkout(branch); + const refspec = `+refs/heads/${branch}:refs/remotes/origin/${branch}`; + await run( + ["fetch", "--depth", "1", "--no-tags", "origin", refspec], + targetPath, + gitWithRetry, + ); + const configured = await git( + ["config", "--get-all", "remote.origin.fetch"], + targetPath, + ); + if (!configured.stdout.split("\n").includes(refspec)) { + await run( + ["config", "--add", "remote.origin.fetch", refspec], + targetPath, + ); + } + await run( + ["checkout", "-b", branch, "--track", `origin/${branch}`], + targetPath, + ); return null; } catch (err) { return fail( `Cloned ${slug} to ${targetPath} but failed to check out branch "${branch}": ${redact( err instanceof Error ? err.message : String(err), - )}. The default branch is checked out instead.`, + )}. The previously checked out branch is still active.`, ); } }; - // Idempotent: a prior clone (retry, reconnected session, LLM loop) leaves - // the repo in place. Reuse it instead of letting git abort on a non-empty - // destination, which the agent would receive as an opaque error. - if (fs.existsSync(path.join(targetPath, ".git"))) { - return ( - (await checkout()) ?? - (await done(`${slug} already cloned at ${targetPath}`)) - ); - } + const freshClone = async (): Promise => { + try { + await fsPromises.mkdir(path.dirname(targetPath), { recursive: true }); + const cloneArgs = [ + "clone", + "--depth", + "1", + "--single-branch", + "--no-tags", + ]; + if (branch) { + cloneArgs.push("--branch", branch); + } + await run( + [...cloneArgs, cloneUrl, targetPath], + undefined, + gitWithRetry, + ); + return done(); + } catch (err) { + // A partial clone would make the retry above take the "already cloned" + // path against a broken checkout. + await fsPromises.rm(targetPath, { recursive: true, force: true }); + return fail( + `clone_repo failed: ${redact( + err instanceof Error ? err.message : String(err), + )}`, + ); + } + }; - // GitHub accepts a token as the basic-auth username for https clones; this - // covers private repos. Public repos clone fine without it. - const cloneUrl = token - ? `https://x-access-token:${token}@github.com/${slug}.git` - : `https://github.com/${slug}.git`; - - try { - const result = await new CloneSaga().run({ - repoUrl: cloneUrl, - targetPath, - }); - if (!result.success) { - return fail(`clone_repo failed: ${redact(result.error)}`); + // A wedged checkout (stale lock, broken config) would otherwise fail every + // future call for this repo, but one holding local work must not be + // deleted to recover. + const discardIfClean = async (): Promise => { + const status = await git(["status", "--porcelain"], targetPath); + if (status.exitCode !== 0 || status.stdout.trim() !== "") { + return false; } + await fsPromises.rm(targetPath, { recursive: true, force: true }); + return true; + }; - return (await checkout()) ?? (await done()); - } catch (err) { - return fail( - `clone_repo failed: ${redact( - err instanceof Error ? err.message : String(err), - )}`, - ); - } + return withCloneLock(targetPath, async () => { + // Idempotent: a prior clone (retry, reconnected session, LLM loop) + // leaves the repo in place. Reuse it instead of letting git abort on a + // non-empty destination, which the agent would receive as an opaque + // error. + if (fs.existsSync(path.join(targetPath, ".git"))) { + try { + // Only this tool writes to `repos//`, so origin should + // already be `cloneUrl`. Anything else was retargeted after the + // clone: normalize it before the fetch below, both to keep a + // credential out of the config and to keep the fetch pointed at the + // repo we were asked for. Read the stored value rather than + // `remote get-url`, which resolves `url..insteadOf` and would + // mask both. + const originUrl = await run( + ["config", "--get", "remote.origin.url"], + targetPath, + ); + if (originUrl !== cloneUrl) { + await run(["remote", "set-url", "origin", cloneUrl], targetPath); + } + } catch (err) { + if (await discardIfClean()) { + return freshClone(); + } + return fail( + `clone_repo couldn't secure the existing origin: ${redact( + err instanceof Error ? err.message : String(err), + )}`, + ); + } + const checkoutFailure = await checkout(); + if (checkoutFailure) { + if (await discardIfClean()) { + return freshClone(); + } + return checkoutFailure; + } + return done(`${slug} already cloned at ${targetPath}`); + } + + return freshClone(); + }); }, }); diff --git a/products/desktop/packages/agent/src/pi/repository-tools-extension.test.ts b/products/desktop/packages/agent/src/pi/repository-tools-extension.test.ts new file mode 100644 index 000000000000..b149676c185b --- /dev/null +++ b/products/desktop/packages/agent/src/pi/repository-tools-extension.test.ts @@ -0,0 +1,35 @@ +import type { + ExtensionAPI, + ToolDefinition, +} from "@earendil-works/pi-coding-agent"; +import { describe, expect, it } from "vitest"; +import { createPiRepositoryToolsExtension } from "./repository-tools-extension"; + +describe("createPiRepositoryToolsExtension", () => { + it("registers the repo-less clone and discovery tools", async () => { + type RegisteredTool = Pick; + const registered: RegisteredTool[] = []; + const extension = createPiRepositoryToolsExtension("/tmp/workspace"); + await extension.factory({ + registerTool: (tool: ToolDefinition) => { + registered.push(tool); + }, + } as unknown as ExtensionAPI); + + expect(registered.map((tool) => tool.name)).toEqual([ + "list_repos", + "clone_repo", + ]); + const cloneTool = registered.find((tool) => tool.name === "clone_repo"); + expect(cloneTool).toBeDefined(); + await expect( + cloneTool?.execute( + "call-1", + { repo: "not a repository" }, + undefined, + undefined, + {} as never, + ), + ).rejects.toThrow('clone_repo: invalid repo "not a repository"'); + }); +}); diff --git a/products/desktop/packages/agent/src/pi/repository-tools-extension.ts b/products/desktop/packages/agent/src/pi/repository-tools-extension.ts new file mode 100644 index 000000000000..ae700fefe075 --- /dev/null +++ b/products/desktop/packages/agent/src/pi/repository-tools-extension.ts @@ -0,0 +1,61 @@ +import type { + ExtensionFactory, + InlineExtension, +} from "@earendil-works/pi-coding-agent"; +import { defineTool } from "@earendil-works/pi-coding-agent"; +import { convertJsonSchemaToTypebox } from "@posthog/harness/extensions/mcp/schema"; +import { z } from "zod"; +import { enabledLocalTools, type LocalToolCtx } from "../adapters/local-tools"; + +const REPOSITORY_TOOL_NAMES = new Set(["list_repos", "clone_repo"]); +type NamedInlineExtension = Exclude; + +function toolLabel(name: string): string { + return name + .replaceAll("_", " ") + .replace(/^./, (first) => first.toUpperCase()); +} + +function createRepositoryToolsFactory(cwd: string): ExtensionFactory { + return (pi) => { + const context: LocalToolCtx = { cwd }; + const tools = enabledLocalTools(context, { channelMode: true }).filter( + (tool) => REPOSITORY_TOOL_NAMES.has(tool.name), + ); + + for (const localTool of tools) { + const schema = z.object(localTool.schema); + pi.registerTool( + defineTool({ + name: localTool.name, + label: toolLabel(localTool.name), + description: localTool.description, + promptSnippet: localTool.description, + parameters: convertJsonSchemaToTypebox(z.toJSONSchema(schema)), + execute: async (_toolCallId, params) => { + const parsed = schema.safeParse(params); + if (!parsed.success) { + throw new Error(parsed.error.message); + } + const result = await localTool.handler(context, parsed.data); + if (result.isError) { + throw new Error( + result.content.map((item) => item.text).join("\n"), + ); + } + return { content: result.content, details: {} }; + }, + }), + ); + } + }; +} + +export function createPiRepositoryToolsExtension( + cwd: string, +): NamedInlineExtension { + return { + name: "posthog-code-repository-tools", + factory: createRepositoryToolsFactory(cwd), + }; +} diff --git a/products/desktop/packages/agent/src/pi/rpc-client.test.ts b/products/desktop/packages/agent/src/pi/rpc-client.test.ts index eb2e8d57357f..2d894ca279be 100644 --- a/products/desktop/packages/agent/src/pi/rpc-client.test.ts +++ b/products/desktop/packages/agent/src/pi/rpc-client.test.ts @@ -58,6 +58,7 @@ process.stdin.resume(); JSON.stringify({ providerOptions: { apiKey: "proxy-key" }, projectTrusted: true, + channelMode: false, }), ); }); @@ -67,17 +68,22 @@ process.stdin.resume(); } }); - it("runs the RPC host with Electron's Node mode enabled", async () => { + it("passes channel mode privately and enables Electron's Node mode", async () => { const directory = await mkdtemp(join(tmpdir(), "pi-electron-node-mode-")); const hostPath = join(directory, "host.mjs"); const capturePath = join(directory, "capture.txt"); await writeFile( hostPath, ` -import { closeSync, writeFileSync } from "node:fs"; +import { closeSync, readFileSync, writeFileSync } from "node:fs"; +const bootstrap = JSON.parse(readFileSync(3, "utf8")); closeSync(3); -writeFileSync(${JSON.stringify(capturePath)}, process.env.ELECTRON_RUN_AS_NODE ?? ""); +writeFileSync(${JSON.stringify(capturePath)}, JSON.stringify({ + nodeMode: process.env.ELECTRON_RUN_AS_NODE ?? "", + channelMode: bootstrap.channelMode, + apiKey: bootstrap.providerOptions.apiKey, +})); process.stdin.resume(); `, ); @@ -85,12 +91,19 @@ process.stdin.resume(); cliPath: hostPath, cwd: directory, providerOptions: { apiKey: "proxy-key" }, + channelMode: true, }); try { await client.start(); await vi.waitFor(async () => { - await expect(readFile(capturePath, "utf8")).resolves.toBe("1"); + await expect(readFile(capturePath, "utf8")).resolves.toBe( + JSON.stringify({ + nodeMode: "1", + channelMode: true, + apiKey: "proxy-key", + }), + ); }); } finally { await client.stop(); diff --git a/products/desktop/packages/agent/src/pi/rpc-client.ts b/products/desktop/packages/agent/src/pi/rpc-client.ts index 05fac7b0b59f..769cb2bffe91 100644 --- a/products/desktop/packages/agent/src/pi/rpc-client.ts +++ b/products/desktop/packages/agent/src/pi/rpc-client.ts @@ -33,6 +33,12 @@ export interface PiRpcProviderOptions { baseUrl?: string; } +interface PiRpcBootstrap { + providerOptions: PiRpcProviderOptions; + projectTrusted?: boolean; + channelMode?: boolean; +} + type RpcClientProcessAccess = { process?: ChildProcess; }; @@ -97,6 +103,7 @@ class SecurePiRpcClient extends RpcClient { private readonly secureOptions: RpcClientOptions, private readonly providerOptions: PiRpcProviderOptions, private readonly projectTrusted: boolean, + private readonly channelMode: boolean, ) { super(secureOptions); } @@ -186,7 +193,8 @@ class SecurePiRpcClient extends RpcClient { JSON.stringify({ providerOptions: this.providerOptions, projectTrusted: this.projectTrusted, - }), + channelMode: this.channelMode, + } satisfies PiRpcBootstrap), ); await new Promise((resolve) => setTimeout(resolve, 100)); @@ -308,11 +316,17 @@ export type PiRpcClientOptions = Pick< sessionFile?: string; providerOptions: PiRpcProviderOptions; projectTrusted?: boolean; + channelMode?: boolean; }; export function createPiRpcClient(options: PiRpcClientOptions): PiRpcClient { - const { sessionFile, providerOptions, projectTrusted, ...rpcOptions } = - options; + const { + sessionFile, + providerOptions, + projectTrusted, + channelMode, + ...rpcOptions + } = options; const args = sessionFile ? ["--session-file", sessionFile] : []; const cliPath = rpcOptions.cliPath ?? @@ -326,5 +340,6 @@ export function createPiRpcClient(options: PiRpcClientOptions): PiRpcClient { }, providerOptions, projectTrusted ?? false, + channelMode === true, ); } diff --git a/products/desktop/packages/agent/src/pi/rpc-host.ts b/products/desktop/packages/agent/src/pi/rpc-host.ts index 7e43de8c899e..9b7654f47a6e 100644 --- a/products/desktop/packages/agent/src/pi/rpc-host.ts +++ b/products/desktop/packages/agent/src/pi/rpc-host.ts @@ -7,11 +7,13 @@ import { POSTHOG_PI_QUEUE_ENTRY_TYPE, readPersistedPiQueue, } from "./queue-persistence"; +import { createPiRepositoryToolsExtension } from "./repository-tools-extension"; import { sanitizePiHostEnvironment } from "./rpc-environment"; interface PiRpcBootstrap { providerOptions?: PosthogProviderOptions; projectTrusted?: boolean; + channelMode?: boolean; } interface PiHostRequest { @@ -44,6 +46,13 @@ const runtime = await createHarnessRuntime({ cwd, bootstrap.projectTrusted ?? false, ), + ...(bootstrap.channelMode + ? { + resourceLoaderOptions: { + extensionFactories: [createPiRepositoryToolsExtension(cwd)], + }, + } + : {}), ...providerOptions, }); diff --git a/products/desktop/packages/agent/src/server/agent-server.test.ts b/products/desktop/packages/agent/src/server/agent-server.test.ts index c73cc5a62a40..0fea83c00b6c 100644 --- a/products/desktop/packages/agent/src/server/agent-server.test.ts +++ b/products/desktop/packages/agent/src/server/agent-server.test.ts @@ -14,6 +14,7 @@ import type { ContentBlock } from "@agentclientprotocol/sdk"; import type { Adapter } from "@posthog/shared"; import { zipSync } from "fflate"; import jwt from "jsonwebtoken"; +import { HttpResponse, http } from "msw"; import { type SetupServerApi, setupServer } from "msw/node"; import { afterAll, @@ -547,6 +548,66 @@ describe("AgentServer HTTP Mode", () => { }); }, 30000); + it("enables repository tools for a repository-less cloud session", async () => { + await mkdir("/tmp/workspace", { recursive: true }); + mswServer.use( + http.get( + "http://localhost:8000/api/projects/:projectId/tasks/:taskId/", + () => + HttpResponse.json({ + id: "test-task-id", + title: "Test task", + description: null, + origin_product: "user_created", + repository: null, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + }), + ), + ); + + const testServer = createServer({ + repositoryPath: undefined, + }) as unknown as { + start(): Promise; + session: { sessionMeta: { channelMode?: boolean } } | null; + }; + await testServer.start(); + + expect(testServer.session?.sessionMeta.channelMode).toBe(true); + }, 30000); + + // A task pinned to a repository gets its checkout provisioned; handing it + // clone tools would be wrong even though it has no repositoryPath. + it("keeps repository tools disabled when the task carries a repository", async () => { + await mkdir("/tmp/workspace", { recursive: true }); + mswServer.use( + http.get( + "http://localhost:8000/api/projects/:projectId/tasks/:taskId/", + () => + HttpResponse.json({ + id: "test-task-id", + title: "Test task", + description: null, + origin_product: "user_created", + repository: "PostHog/posthog", + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + }), + ), + ); + + const testServer = createServer({ + repositoryPath: undefined, + }) as unknown as { + start(): Promise; + session: { sessionMeta: { channelMode?: boolean } } | null; + }; + await testServer.start(); + + expect(testServer.session?.sessionMeta.channelMode).toBeUndefined(); + }, 30000); + it("links native agent state before initializing the session", async () => { const originalClaudeConfigDir = process.env.CLAUDE_CONFIG_DIR; const originalCodexHome = process.env.CODEX_HOME; @@ -4027,8 +4088,11 @@ describe("AgentServer HTTP Mode", () => { config: { repositoryPath: undefined }, shouldContain: [ "Cloud Task Execution — No Repository Mode", - "Clone the repository into /tmp/workspace/repos//", - "gh repo clone / /tmp/workspace/repos//", + "call `list_repos`", + "Call `clone_repo`", + "It creates a shallow clone", + "git fetch --deepen=50 origin ", + "git fetch --deepen=200 origin ", "If the user explicitly asks you to open or update a pull request", "open a draft pull request", "unless the user explicitly asks", @@ -4038,17 +4102,22 @@ describe("AgentServer HTTP Mode", () => { "Generated-By: PostHog Code", "Task-Id: test-task-id", ], - shouldNotContain: [], + shouldNotContain: ["gh repo clone"], }, { label: "createPr false", config: { repositoryPath: undefined, createPr: false }, shouldContain: [ "Cloud Task Execution — No Repository Mode", - "You may clone a repository and make local edits in that clone", + "Call `clone_repo`", + "You may make local edits in a repository cloned with `clone_repo`", "Do NOT create branches, commits, push changes, or open pull requests in this run", ], - shouldNotContain: ["open a draft pull request", "gh pr create --draft"], + shouldNotContain: [ + "open a draft pull request", + "gh pr create --draft", + "gh repo clone", + ], }, ])( "returns no-repository prompt for $label", diff --git a/products/desktop/packages/agent/src/server/agent-server.ts b/products/desktop/packages/agent/src/server/agent-server.ts index 53d5fd8527da..0d80b1681fff 100644 --- a/products/desktop/packages/agent/src/server/agent-server.ts +++ b/products/desktop/packages/agent/src/server/agent-server.ts @@ -1743,6 +1743,10 @@ export class AgentServer { ) : []; const sessionCwd = this.config.repositoryPath ?? "/tmp/workspace"; + // Only a run with no repository at all gets the discover-and-clone tools; + // a multi-repository workspace already has its repos on disk. + const channelMode = + !this.config.repositoryPath && this.taskRepositories.length === 0; const sessionMeta = { sessionId: payload.run_id, taskRunId: payload.run_id, @@ -1754,6 +1758,7 @@ export class AgentServer { allowedDomains: this.config.allowedDomains, jsonSchema: preTask?.json_schema ?? null, permissionMode: initialPermissionMode, + ...(channelMode && { channelMode: true }), posthogExecPermissionRegex: this.posthogExecPermissionRegexSource, ...(this.config.baseBranch && { baseBranch: this.config.baseBranch }), ...(runtimeAdapter === "claude" && @@ -3698,17 +3703,22 @@ ${signedCommitInstructions}${prLinkInstructions}${shellEfficiencyInstructions}${ } if (!this.config.repositoryPath && this.taskRepositories.length === 0) { + const repositoryInstructions = ` +When the task requires a GitHub repository: +- If the repository is not specified, call \`list_repos\` and use the task context to choose it. If multiple repositories remain plausible, ask the user. +- Call \`clone_repo\` with the chosen \`owner/repo\` and optional branch. It creates a shallow clone under \`/tmp/workspace/repos//\` and returns the path. +- Work from inside the returned path for all code changes. +- The clone starts with one commit. If older history is genuinely needed, fetch it in bounded steps with \`git fetch --deepen=50 origin \`, then \`git fetch --deepen=200 origin \`. Use \`git fetch --unshallow\` only when the task explicitly requires full history, such as a long-range blame or bisect. +`; const publishInstructions = this.config.createPr === false ? ` When the user asks for code changes: -- You may clone a repository and make local edits in that clone +- You may make local edits in a repository cloned with \`clone_repo\` - Do NOT create branches, commits, push changes, or open pull requests in this run` : shouldAutoCreatePr ? ` -When the user asks to clone or work in a GitHub repository: -- Clone the repository into /tmp/workspace/repos// using \`gh repo clone / /tmp/workspace/repos//\` -- Work from inside that cloned repository for follow-up code changes +When the user asks for code changes in a GitHub repository: - After completing code changes in a cloned repository, create a branch, stage your changes with \`git add\` and commit them with the \`git_signed_commit\` tool (do NOT use \`git commit\`/\`git push\` — they are blocked), and open a draft pull request from inside the clone without waiting to be asked. Before opening the PR, check the cloned repo for a PR template at \`.github/pull_request_template.md\` (or variants; fall back to the org's \`.github\` repo via \`gh api\`) and use it as the body structure, and search for matching open issues with \`gh issue list --search\` to include \`Closes #\` / \`Refs #\` links. - Keep the PR description brief overall. Summarize only the most important changes — do NOT enumerate every change you made. A few sentences or bullets is plenty. ${whyContextInstruction.trimStart()} @@ -3717,9 +3727,7 @@ ${prMentionSafetyInstruction.trimStart()} - End the PR description with a horizontal rule followed by this footer line: ${prFooter} - Always create the PR as a draft. Do not ask for confirmation before publishing completed code changes` : ` -When the user explicitly asks to clone or work in a GitHub repository: -- Clone the repository into /tmp/workspace/repos// using \`gh repo clone / /tmp/workspace/repos//\` -- Work from inside that cloned repository for follow-up code changes +When the user explicitly asks for code changes in a GitHub repository: - If the user explicitly asks you to open or update a pull request, create a branch, stage your changes with \`git add\` and commit them with the \`git_signed_commit\` tool (do NOT use \`git commit\`/\`git push\` — they are blocked), and open a draft pull request from inside the clone. Before opening the PR, check the cloned repo for a PR template at \`.github/pull_request_template.md\` (or variants; fall back to the org's \`.github\` repo via \`gh api\`) and use it as the body structure, and search for matching open issues with \`gh issue list --search\` to include \`Closes #\` / \`Refs #\` links. - Keep the PR description brief overall. Summarize only the most important changes — do NOT enumerate every change you made. A few sentences or bullets is plenty. ${whyContextInstruction.trimStart()} @@ -3739,9 +3747,8 @@ When the user asks about analytics, data, metrics, events, funnels, dashboards, - Use tools like insight-query, query-run, event-definitions-list, and others to answer questions directly When the user asks for code changes or software engineering tasks: -- Let them know you can help but don't have a repository connected for this session -- If they have not specified a repository to clone, offer to write code snippets, scripts, or provide guidance -${publishInstructions} +- Choose and clone a repository only when the task requires one. For questions and analysis, answer without cloning when possible. +${repositoryInstructions}${publishInstructions} Important: - Prefer using MCP tools to answer questions with real data over giving generic advice. diff --git a/products/desktop/packages/agent/src/server/pi-agent-server.ts b/products/desktop/packages/agent/src/server/pi-agent-server.ts index 0fb220e9000a..13269d83e3ca 100644 --- a/products/desktop/packages/agent/src/server/pi-agent-server.ts +++ b/products/desktop/packages/agent/src/server/pi-agent-server.ts @@ -500,6 +500,7 @@ export class PiAgentServer { this.config.apiUrl, ), }, + channelMode: !this.config.repositoryPath, }); const runtime = new PiRuntime(client); const unsubscribeConversation = runtime.onConversationEvent((event) => diff --git a/products/desktop/packages/git/src/git-exec.test.ts b/products/desktop/packages/git/src/git-exec.test.ts new file mode 100644 index 000000000000..853ab1a9fce8 --- /dev/null +++ b/products/desktop/packages/git/src/git-exec.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it, vi } from "vitest"; +import { + execGitWithRetry, + type GitExecResult, + isTransientGitFailure, +} from "./git-exec"; + +function result(partial: Partial): GitExecResult { + return { stdout: "", stderr: "", exitCode: 1, ...partial }; +} + +describe("git-exec retry", () => { + describe("isTransientGitFailure", () => { + it.each([ + { + name: "HTTP 502", + res: result({ + stderr: + "fatal: unable to access: The requested URL returned error: 502", + }), + expected: true, + }, + { + name: "timeout", + res: result({ error: "git timed out after 600000ms" }), + expected: true, + }, + { + name: "ECONNRESET", + res: result({ error: "read ECONNRESET" }), + expected: true, + }, + { + name: "unresolvable host", + res: result({ + stderr: "fatal: unable to access: Could not resolve host: github.com", + }), + expected: true, + }, + { + name: "interrupted transfer", + res: result({ stderr: "fetch-pack: unexpected disconnect, early EOF" }), + expected: true, + }, + { + name: "success", + res: result({ exitCode: 0, stderr: "early EOF" }), + expected: false, + }, + { + name: "auth failure", + res: result({ + stderr: "fatal: Authentication failed for 'https://github.com/x.git'", + }), + expected: false, + }, + { + name: "missing remote ref", + res: result({ stderr: "fatal: couldn't find remote ref missing" }), + expected: false, + }, + ])("$name -> $expected", ({ res, expected }) => { + expect(isTransientGitFailure(res)).toBe(expected); + }); + }); + + describe("execGitWithRetry", () => { + it("retries transient failures then succeeds", async () => { + const exec = vi + .fn() + .mockResolvedValueOnce(result({ stderr: "early EOF" })) + .mockResolvedValueOnce(result({ stdout: "ok", exitCode: 0 })); + const res = await execGitWithRetry(["fetch"], {}, { backoffMs: 0 }, exec); + expect(res.exitCode).toBe(0); + expect(exec).toHaveBeenCalledTimes(2); + }); + + it("stops after maxAttempts on persistent transient failure", async () => { + const exec = vi.fn().mockResolvedValue(result({ stderr: "early EOF" })); + const res = await execGitWithRetry( + ["fetch"], + {}, + { maxAttempts: 3, backoffMs: 0 }, + exec, + ); + expect(res.exitCode).toBe(1); + expect(exec).toHaveBeenCalledTimes(3); + }); + + it("does not retry deterministic failures", async () => { + const exec = vi + .fn() + .mockResolvedValue(result({ stderr: "fatal: Authentication failed" })); + const res = await execGitWithRetry(["fetch"], {}, { backoffMs: 0 }, exec); + expect(res.exitCode).toBe(1); + expect(exec).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/products/desktop/packages/git/src/git-exec.ts b/products/desktop/packages/git/src/git-exec.ts new file mode 100644 index 000000000000..af8d7b8479ba --- /dev/null +++ b/products/desktop/packages/git/src/git-exec.ts @@ -0,0 +1,146 @@ +// Namespace import (not `{ execFile }`) so the renderer's browser bundle can +// resolve this node-only module against vite's `__vite-browser-external` stub, +// which has no named exports. execGit never runs in the browser. +import * as childProcess from "node:child_process"; + +export interface GitExecResult { + stdout: string; + stderr: string; + exitCode: number; + error?: string; +} + +export interface GitExecOptions { + cwd?: string; + /** Merged over `process.env` rather than replacing it. */ + env?: Record; + /** + * Kill the `git` subprocess after this many ms, so a clone or fetch that + * stalls on the network can't hang the caller forever. Omit for no timeout. + */ + timeoutMs?: number; + maxBuffer?: number; +} + +const DEFAULT_MAX_BUFFER = 32 * 1024 * 1024; + +/** + * Runs a `git` subcommand and resolves with its result, mirroring `execGh`. + * + * This is the raw-subprocess counterpart to the simple-git client: no repo + * locking, no saga bookkeeping, and `GIT_CONFIG_*` in `env` is passed through + * untouched. It suits callers that own the whole checkout for the duration of + * the call, such as an agent cloning into its own scratch workspace. + */ +export function execGit( + args: string[], + options: GitExecOptions = {}, +): Promise { + const env = options.env ? { ...process.env, ...options.env } : process.env; + + return new Promise((resolve) => { + childProcess.execFile( + "git", + args, + { + cwd: options.cwd, + env, + timeout: options.timeoutMs ?? 0, + maxBuffer: options.maxBuffer ?? DEFAULT_MAX_BUFFER, + }, + (error, stdout, stderr) => { + if (!error) { + resolve({ stdout, stderr, exitCode: 0 }); + return; + } + + const err = error as Error & { + code?: number | string; + killed?: boolean; + stdout?: string; + stderr?: string; + }; + // execFile kills the child on timeout (`killed` set, `code` null), so + // report that as a timeout rather than an opaque signal death. + const timedOut = err.killed === true && !!options.timeoutMs; + const exitCode = + typeof err.code === "number" + ? err.code + : err.code === "ENOENT" + ? 127 + : 1; + + resolve({ + stdout: stdout ?? err.stdout ?? "", + stderr: stderr ?? err.stderr ?? "", + exitCode, + error: timedOut + ? `git timed out after ${options.timeoutMs}ms` + : err.message, + }); + }, + ); + }); +} + +// Failures worth retrying: server-side blips (5xx), interrupted transfers, +// our own timeout, and transport-level network errors. Deterministic failures +// (auth, missing refs, non-empty destination) are intentionally excluded — +// retrying them only wastes time. +const TRANSIENT_GIT_PATTERNS: readonly RegExp[] = [ + /The requested URL returned error: 5\d\d/, + /\btimed out\b/i, + /\bETIMEDOUT\b/, + /\bECONNRESET\b/, + /\bECONNREFUSED\b/, + /\bEAI_AGAIN\b/, + /connection reset/i, + /Could not resolve host/i, + /Failed to connect to/i, + /early EOF/i, + /RPC failed/i, + /GnuTLS recv error/i, +]; + +export function isTransientGitFailure(res: GitExecResult): boolean { + if (res.exitCode === 0) { + return false; + } + const text = `${res.stderr} ${res.error ?? ""} ${res.stdout}`; + return TRANSIENT_GIT_PATTERNS.some((re) => re.test(text)); +} + +export interface GitRetryOptions { + maxAttempts?: number; + /** Base backoff; attempt N waits `backoffMs * 2^(N-2)` before retrying. */ + backoffMs?: number; +} + +const sleep = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Runs `execGit`, retrying only on transient failures with exponential + * backoff, mirroring `execGhWithRetry`. `exec` is injectable for tests; + * production callers use the default. + */ +export async function execGitWithRetry( + args: string[], + options: GitExecOptions = {}, + retry: GitRetryOptions = {}, + exec: typeof execGit = execGit, +): Promise { + const maxAttempts = retry.maxAttempts ?? 3; + const backoffMs = retry.backoffMs ?? 500; + + let res = await exec(args, options); + for ( + let attempt = 2; + attempt <= maxAttempts && isTransientGitFailure(res); + attempt++ + ) { + await sleep(backoffMs * 2 ** (attempt - 2)); + res = await exec(args, options); + } + return res; +} diff --git a/products/desktop/packages/git/src/utils.test.ts b/products/desktop/packages/git/src/utils.test.ts index f25c355f2920..567789bb72d3 100644 --- a/products/desktop/packages/git/src/utils.test.ts +++ b/products/desktop/packages/git/src/utils.test.ts @@ -235,6 +235,11 @@ describe("parseGithubUrl", () => { "file:///path/to/repo", // Missing repo "https://github.com/PostHog", + // Path traversal + unsafe characters (scp-style paths skip WHATWG + // dot-segment normalization, and callers path.join the segments) + "git@github.com:PostHog/..", + "git@github.com:../../etc/passwd", + "git@github.com:PostHog/re po", // Multiple / leading slashes "https://github.com//PostHog/code.git", "https://github.com/PostHog//code.git", diff --git a/products/desktop/packages/git/src/utils.ts b/products/desktop/packages/git/src/utils.ts index 9e7694c25337..bc0512a38f4a 100644 --- a/products/desktop/packages/git/src/utils.ts +++ b/products/desktop/packages/git/src/utils.ts @@ -156,6 +156,13 @@ export async function forceRemove(target: string): Promise { await fs.rm(target, { recursive: true, force: true, maxRetries: 3 }); } +// GitHub owner and repo names only ever contain word characters, dots, and +// hyphens. Callers join the parsed segments into filesystem paths, and +// scp-style SSH inputs skip WHATWG dot-segment normalization, so a `..` +// segment would otherwise survive parsing and escape the caller's target +// directory via path.join. +const SAFE_SLUG_SEGMENT = /^(?!\.\.?$)[\w.-]+$/; + export function parseGithubUrl( url: string | null | undefined, ): GitHubUrl | null { @@ -180,6 +187,9 @@ export function parseGithubUrl( if (parts.length < 2 || parts.some((p) => p === "")) return null; const [owner, repoRaw, segment, num] = parts; const repo = repoRaw.replace(/\.git$/, ""); + if (!SAFE_SLUG_SEGMENT.test(owner) || !SAFE_SLUG_SEGMENT.test(repo)) { + return null; + } if (segment === "issues" || segment === "pull") { const number = Number(num);