diff --git a/packages/agent/src/adapters/claude/claude-agent.refresh.test.ts b/packages/agent/src/adapters/claude/claude-agent.refresh.test.ts index a70aab87d5..e75287e18b 100644 --- a/packages/agent/src/adapters/claude/claude-agent.refresh.test.ts +++ b/packages/agent/src/adapters/claude/claude-agent.refresh.test.ts @@ -16,6 +16,7 @@ type SdkQueryHandle = { interrupt: ReturnType; setModel: ReturnType; setMcpServers: ReturnType; + mcpServerStatus: ReturnType; supportedCommands: ReturnType; initializationResult: ReturnType; close: ReturnType; @@ -33,6 +34,7 @@ function makeQueryHandle(): SdkQueryHandle { interrupt: vi.fn().mockResolvedValue(undefined), setModel: vi.fn().mockResolvedValue(undefined), setMcpServers: vi.fn().mockResolvedValue(undefined), + mcpServerStatus: vi.fn().mockResolvedValue([]), supportedCommands: vi.fn().mockResolvedValue([]), initializationResult: vi.fn().mockImplementation(() => nextInitPromise), close: vi.fn(), @@ -55,9 +57,12 @@ vi.mock("@anthropic-ai/claude-agent-sdk", () => ({ })); const fetchMcpToolMetadataMock = vi.fn().mockResolvedValue(undefined); +const clearMcpToolMetadataCacheMock = vi.fn(); vi.mock("./mcp/tool-metadata", () => ({ fetchMcpToolMetadata: fetchMcpToolMetadataMock, getConnectedMcpServerNames: vi.fn().mockReturnValue([]), + getCachedMcpTools: vi.fn().mockReturnValue([]), + clearMcpToolMetadataCache: clearMcpToolMetadataCacheMock, })); // Import after the mocks so ClaudeAcpAgent resolves the mocked SDK @@ -82,6 +87,16 @@ function installFakeSession( const endSpy = vi.spyOn(input, "end"); const abortController = new AbortController(); + // Distinguishable fresh instance per call so tests can prove a rebuild. + let freshInstanceCounter = 0; + const buildInProcessMcpServers = vi.fn(() => ({ + "posthog-code-tools": { + type: "sdk" as const, + name: "posthog-code-tools", + instance: { fresh: ++freshInstanceCounter }, + }, + })); + const session = { query: oldQuery, queryOptions: { @@ -93,11 +108,13 @@ function installFakeSession( "posthog-code-tools": { type: "sdk", name: "posthog-code-tools", - instance: {}, + instance: { stale: true }, }, }, abortController, }, + buildInProcessMcpServers, + localToolsServerNames: ["posthog-code-tools"], input, cancelled: false, settingsManager: { dispose: vi.fn() }, @@ -123,7 +140,13 @@ function installFakeSession( (agent as unknown as { session: unknown }).session = session; (agent as unknown as { sessionId: string }).sessionId = sessionId; - return { session, oldQuery, endSpy, abortController }; + return { + session, + oldQuery, + endSpy, + abortController, + buildInProcessMcpServers, + }; } const freshMcpServers = [ @@ -145,6 +168,7 @@ describe("ClaudeAcpAgent.extMethod refresh_session", () => { models: [], }); fetchMcpToolMetadataMock.mockClear(); + clearMcpToolMetadataCacheMock.mockClear(); }); it("returns methodNotFound for unknown extension methods", async () => { @@ -238,7 +262,7 @@ describe("ClaudeAcpAgent.extMethod refresh_session", () => { expect(endSpy).toHaveBeenCalledTimes(1); // New query: resume identity (not sessionId), http server refreshed, and - // the in-process local-tools server preserved. + // the in-process local-tools server rebuilt fresh. expect(lastQueryCall.options).toMatchObject({ resume: "s-2", forkSession: false, @@ -364,24 +388,192 @@ describe("ClaudeAcpAgent.extMethod refresh_session", () => { expect(lastQueryCall.options?.model).toBe(expected); }); - it("preserves the in-process local-tools server across refresh", async () => { + it("rebuilds a FRESH in-process local-tools server across refresh", async () => { const agent = makeAgent(); - installFakeSession(agent, "s-inprocess"); + const { session, buildInProcessMcpServers } = installFakeSession( + agent, + "s-inprocess", + ); + const staleInstance = ( + session as unknown as { + queryOptions: { mcpServers: Record }; + } + ).queryOptions.mcpServers["posthog-code-tools"].instance; - // freshMcpServers carries only external (http) servers, so the sdk server - // must be carried over from the previous session options. + // freshMcpServers carries only external servers; the sdk server is rebuilt. await agent.extMethod(POSTHOG_METHODS.REFRESH_SESSION, { mcpServers: freshMcpServers, }); + expect(buildInProcessMcpServers).toHaveBeenCalledTimes(1); const servers = lastQueryCall.options?.mcpServers as Record< string, - { type?: string } + { type?: string; name?: string; instance?: unknown } >; - expect(servers["posthog-code-tools"]).toEqual({ + expect(servers["posthog-code-tools"]).toMatchObject({ type: "sdk", name: "posthog-code-tools", - instance: {}, }); + // A brand-new instance object, never the stale reused one. + expect(servers["posthog-code-tools"].instance).not.toBe(staleInstance); + expect(servers["posthog-code-tools"].instance).toEqual({ fresh: 1 }); + }); + + it("clears the MCP tool metadata cache on refresh", async () => { + const agent = makeAgent(); + installFakeSession(agent, "s-cache"); + + await agent.extMethod(POSTHOG_METHODS.REFRESH_SESSION, { + mcpServers: freshMcpServers, + }); + + expect(clearMcpToolMetadataCacheMock).toHaveBeenCalledTimes(1); + }); +}); + +const DISCONNECTED_STATUS = [{ name: "posthog-code-tools", status: "failed" }]; + +describe("ClaudeAcpAgent self-heal: ensureLocalToolsConnected", () => { + beforeEach(() => { + clearMcpToolMetadataCacheMock.mockClear(); + fetchMcpToolMetadataMock.mockClear(); + }); + + function callHeal(agent: Agent, trigger = "test"): Promise { + return ( + agent as unknown as { + ensureLocalToolsConnected: (t: string) => Promise; + } + ).ensureLocalToolsConnected(trigger); + } + + it("is a no-op when the signed-commit server is connected", async () => { + const agent = makeAgent(); + const { oldQuery } = installFakeSession(agent, "s-healthy"); + oldQuery.mcpServerStatus.mockResolvedValue([ + { name: "posthog-code-tools", status: "connected" }, + ]); + + await expect(callHeal(agent)).resolves.toBe(true); + expect(oldQuery.setMcpServers).not.toHaveBeenCalled(); + }); + + it("rebuilds and reconnects a fresh server when disconnected", async () => { + const agent = makeAgent(); + const { session, oldQuery, buildInProcessMcpServers } = installFakeSession( + agent, + "s-down", + ); + oldQuery.mcpServerStatus.mockResolvedValue(DISCONNECTED_STATUS); + + await expect(callHeal(agent)).resolves.toBe(true); + + expect(buildInProcessMcpServers).toHaveBeenCalledTimes(1); + expect(oldQuery.setMcpServers).toHaveBeenCalledTimes(1); + const arg = oldQuery.setMcpServers.mock.calls[0][0] as Record< + string, + { type?: string; instance?: unknown } + >; + // External http server passed through unchanged; sdk server is fresh. + expect(arg.posthog).toMatchObject({ type: "http" }); + expect(arg["posthog-code-tools"]).toMatchObject({ type: "sdk" }); + expect(arg["posthog-code-tools"].instance).toEqual({ fresh: 1 }); + expect(clearMcpToolMetadataCacheMock).toHaveBeenCalledTimes(1); + // queryOptions is updated so later heals/refresh see the fresh server set. + expect( + (session as unknown as { queryOptions: { mcpServers: unknown } }) + .queryOptions.mcpServers, + ).toBe(arg); + }); + + it("passes every external server through when reconnecting", async () => { + const agent = makeAgent(); + const { session, oldQuery } = installFakeSession(agent, "s-multi"); + ( + session as unknown as { + queryOptions: { mcpServers: Record }; + } + ).queryOptions.mcpServers = { + posthog: { type: "http", url: "https://old" }, + sentry: { type: "sse", url: "https://sse" }, + "posthog-code-tools": { + type: "sdk", + name: "posthog-code-tools", + instance: { stale: true }, + }, + }; + oldQuery.mcpServerStatus.mockResolvedValue(DISCONNECTED_STATUS); + + await expect(callHeal(agent)).resolves.toBe(true); + + const arg = oldQuery.setMcpServers.mock.calls[0][0] as Record< + string, + { type?: string } + >; + expect(Object.keys(arg).sort()).toEqual([ + "posthog", + "posthog-code-tools", + "sentry", + ]); + expect(arg.posthog).toMatchObject({ type: "http" }); + expect(arg.sentry).toMatchObject({ type: "sse" }); + expect(arg["posthog-code-tools"]).toMatchObject({ type: "sdk" }); + }); + + it("treats a server missing from status as disconnected", async () => { + const agent = makeAgent(); + const { oldQuery } = installFakeSession(agent, "s-missing"); + oldQuery.mcpServerStatus.mockResolvedValue([ + { name: "some-other", status: "connected" }, + ]); + + await expect(callHeal(agent)).resolves.toBe(true); + expect(oldQuery.setMcpServers).toHaveBeenCalledTimes(1); + }); + + it("does not block the turn when the status RPC fails", async () => { + const agent = makeAgent(); + const { oldQuery } = installFakeSession(agent, "s-statuserr"); + oldQuery.mcpServerStatus.mockRejectedValue(new Error("rpc down")); + + await expect(callHeal(agent)).resolves.toBe(true); + expect(oldQuery.setMcpServers).not.toHaveBeenCalled(); + }); + + it("does not block the turn when the status RPC hangs", async () => { + vi.useFakeTimers(); + try { + const agent = makeAgent(); + const { oldQuery } = installFakeSession(agent, "s-statushang"); + oldQuery.mcpServerStatus.mockReturnValue(new Promise(() => {})); + + const healPromise = callHeal(agent); + await vi.advanceTimersByTimeAsync(5_001); + + await expect(healPromise).resolves.toBe(true); + expect(oldQuery.setMcpServers).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + + it("returns false when reconnect fails", async () => { + const agent = makeAgent(); + const { oldQuery } = installFakeSession(agent, "s-reconnect-fail"); + oldQuery.mcpServerStatus.mockResolvedValue(DISCONNECTED_STATUS); + oldQuery.setMcpServers.mockRejectedValue(new Error("connect boom")); + + await expect(callHeal(agent)).resolves.toBe(false); + }); + + it("is a no-op when no in-process server is enabled", async () => { + const agent = makeAgent(); + const { session, oldQuery } = installFakeSession(agent, "s-none"); + ( + session as unknown as { localToolsServerNames: string[] } + ).localToolsServerNames = []; + + await expect(callHeal(agent)).resolves.toBe(true); + expect(oldQuery.mcpServerStatus).not.toHaveBeenCalled(); }); }); diff --git a/packages/agent/src/adapters/claude/claude-agent.slash-command.test.ts b/packages/agent/src/adapters/claude/claude-agent.slash-command.test.ts index 776663417e..0dc8536af8 100644 --- a/packages/agent/src/adapters/claude/claude-agent.slash-command.test.ts +++ b/packages/agent/src/adapters/claude/claude-agent.slash-command.test.ts @@ -46,6 +46,8 @@ function installFakeSession( const session = { query, queryOptions: { sessionId, cwd: "/tmp/repo", abortController }, + buildInProcessMcpServers: () => ({}), + localToolsServerNames: [] as string[], input, cancelled: false, interruptReason: undefined, diff --git a/packages/agent/src/adapters/claude/claude-agent.streamed-text.test.ts b/packages/agent/src/adapters/claude/claude-agent.streamed-text.test.ts index da450eacac..b7f8d0a201 100644 --- a/packages/agent/src/adapters/claude/claude-agent.streamed-text.test.ts +++ b/packages/agent/src/adapters/claude/claude-agent.streamed-text.test.ts @@ -14,6 +14,8 @@ vi.mock("@anthropic-ai/claude-agent-sdk", () => ({ vi.mock("./mcp/tool-metadata", () => ({ fetchMcpToolMetadata: vi.fn().mockResolvedValue(undefined), getConnectedMcpServerNames: vi.fn().mockReturnValue([]), + getCachedMcpTools: vi.fn().mockReturnValue([]), + clearMcpToolMetadataCache: vi.fn(), setMcpToolApprovalStates: vi.fn(), isMcpToolReadOnly: vi.fn().mockReturnValue(false), getMcpToolMetadata: vi.fn().mockReturnValue(undefined), @@ -48,6 +50,8 @@ function installFakeSession( const session = { query, queryOptions: { sessionId, cwd: "/tmp/repo", abortController }, + buildInProcessMcpServers: () => ({}), + localToolsServerNames: [] as string[], input, cancelled: false, interruptReason: undefined, @@ -79,6 +83,27 @@ function installFakeSession( return { query, input }; } +// Mark the session as having an enabled (but currently disconnected) in-process +// signed-commit server so the pre-prompt heal has something to reconnect. +function enableSignedCommitServer(agent: Agent): void { + const session = ( + agent as unknown as { + session: { + buildInProcessMcpServers: () => Record; + localToolsServerNames: string[]; + }; + } + ).session; + session.localToolsServerNames = ["posthog-code-tools"]; + session.buildInProcessMcpServers = () => ({ + "posthog-code-tools": { + type: "sdk", + name: "posthog-code-tools", + instance: {}, + }, + }); +} + function tick(): Promise { return new Promise((resolve) => setImmediate(resolve)); } @@ -216,4 +241,55 @@ describe("ClaudeAcpAgent.prompt — streamed assistant text wiring", () => { "gateway answer", ]); }); + + it("reconnects a disconnected signed-commit server before the turn", async () => { + const { agent } = makeAgent(); + const sessionId = "s-heal"; + const { query, input } = installFakeSession(agent, sessionId); + + // Signed-commit server is enabled but the live query reports it absent. + enableSignedCommitServer(agent); + (query.mcpServerStatus as ReturnType).mockResolvedValue([ + { name: "posthog-code-tools", status: "failed" }, + ]); + + const promptPromise = agent.prompt({ + sessionId, + prompt: [{ type: "text", text: "commit this" }], + }); + await tick(); + + // The pre-prompt heal fired before the model turn began. + expect(query.mcpServerStatus).toHaveBeenCalled(); + expect(query.setMcpServers).toHaveBeenCalledTimes(1); + const arg = (query.setMcpServers as ReturnType).mock + .calls[0][0] as Record; + expect(arg["posthog-code-tools"]).toMatchObject({ type: "sdk" }); + + await echoUserMessage(query, input); + await send(query, assistantMessage(sessionId, "msg_h", "done")); + await send(query, resultSuccess(sessionId)); + const result = await promptPromise; + expect(result.stopReason).toBe("end_turn"); + }); + + it("skips the pre-prompt heal for local-only commands", async () => { + const { agent } = makeAgent(); + const sessionId = "s-local-only"; + const { query } = installFakeSession(agent, sessionId); + + enableSignedCommitServer(agent); + + const promptPromise = agent.prompt({ + sessionId, + prompt: [{ type: "text", text: "/context" }], + }); + await tick(); + + expect(query.mcpServerStatus).not.toHaveBeenCalled(); + expect(query.setMcpServers).not.toHaveBeenCalled(); + + await send(query, resultSuccess(sessionId)); + await promptPromise; + }); }); diff --git a/packages/agent/src/adapters/claude/claude-agent.ts b/packages/agent/src/adapters/claude/claude-agent.ts index 8df465ae26..c266dd950e 100644 --- a/packages/agent/src/adapters/claude/claude-agent.ts +++ b/packages/agent/src/adapters/claude/claude-agent.ts @@ -34,6 +34,7 @@ import { type CanUseTool, getSessionMessages, listSessions, + type McpSdkServerConfigWithInstance, type McpServerConfig, type Options, type Query, @@ -94,6 +95,7 @@ import { import type { EnrichedReadCache } from "./hooks"; import { createLocalToolsMcpServer } from "./mcp/local-tools"; import { + clearMcpToolMetadataCache, fetchMcpToolMetadata, getCachedMcpTools, getConnectedMcpServerNames, @@ -138,11 +140,29 @@ import type { const SESSION_VALIDATION_TIMEOUT_MS = 30_000; +// Pre-prompt self-heal runs on every cloud turn; bound the status RPC so a +// wedged control channel can't stall the turn. +const MCP_STATUS_TIMEOUT_MS = 5_000; + const DEFAULT_FORCE_CANCEL_GRACE_MS = 30_000; const MAX_TITLE_LENGTH = 256; const LOCAL_ONLY_COMMANDS = new Set(["/context", "/heapdump", "/extra-usage"]); +function isSdkMcpServer( + cfg: McpServerConfig, +): cfg is McpSdkServerConfigWithInstance { + return cfg.type === "sdk"; +} + +function externalMcpServers( + servers: Record | undefined, +): Record { + return Object.fromEntries( + Object.entries(servers ?? {}).filter(([, cfg]) => !isSdkMcpServer(cfg)), + ); +} + // Best-effort: silent on ENOENT, logs other errors so permission failures // aren't masked. function readClaudeMdQuietly(cwd: string, logger: Logger): string | undefined { @@ -443,6 +463,10 @@ export class ClaudeAcpAgent extends BaseAcpAgent { } promptReplayed = true; } else { + // Reconnect the signed-commit server before the turn (guard hook backstops). + if (!isLocalOnlyCommand) { + await this.ensureLocalToolsConnected("pre-prompt"); + } this.session.input.push(userMessage); } @@ -1254,17 +1278,19 @@ export class ClaudeAcpAgent extends BaseAcpAgent { const newAbortController = new AbortController(); const { sessionId: _drop, ...rest } = prev.queryOptions; - // parseMcpServers yields only http/sse/stdio – carry over any in-process - // ("sdk") server so the local-tools server (signed commits) survives. - const preservedInProcess = Object.fromEntries( - Object.entries(prev.queryOptions.mcpServers ?? {}).filter( - ([, cfg]) => (cfg as { type?: string }).type === "sdk", - ), - ); + // Rebuild the in-process ("sdk") server fresh; reusing the prior instance + // throws "Already connected to a transport" and drops the signed-commit tools. + const freshInProcess = prev.buildInProcessMcpServers(); + if (Object.keys(freshInProcess).length > 0) { + this.logger.info("Rebuilt in-process MCP servers on refresh", { + sessionId: this.sessionId, + servers: Object.keys(freshInProcess), + }); + } const newOptions: Options = { ...rest, - mcpServers: { ...mcpServers, ...preservedInProcess }, + mcpServers: { ...mcpServers, ...freshInProcess }, resume: this.sessionId, forkSession: false, abortController: newAbortController, @@ -1294,8 +1320,71 @@ export class ClaudeAcpAgent extends BaseAcpAgent { ); } - // Re-fetch MCP tool metadata + slash commands — the server list changed. - this.deferBackgroundFetches(newQuery); + this.refreshMcpMetadata(newQuery); + } + + /** + * Best-effort self-heal: if the in-process signed-commit server is enabled but + * the live Query reports it disconnected, rebuild a fresh instance and + * reconnect via setMcpServers. Returns whether the tooling is usable after. + */ + private async ensureLocalToolsConnected(trigger: string): Promise { + const names = this.session.localToolsServerNames; + if (names.length === 0) { + return true; + } + + const status = await withTimeout( + this.session.query.mcpServerStatus(), + MCP_STATUS_TIMEOUT_MS, + ).catch((error) => { + this.logger.debug("ensureLocalToolsConnected: status check failed", { + trigger, + error: error instanceof Error ? error.message : String(error), + }); + return { result: "timeout" as const }; + }); + // A slow or failed status RPC must not block the turn; assume healthy. + if (status.result !== "success") { + return true; + } + + const allConnected = names.every((name) => + status.value.some((s) => s.name === name && s.status === "connected"), + ); + if (allConnected) { + return true; + } + + const logCtx = { trigger, sessionId: this.sessionId, servers: names }; + this.logger.warn( + "Signed-commit MCP server unhealthy; reconnecting", + logCtx, + ); + + try { + const next = { + ...externalMcpServers(this.session.queryOptions.mcpServers), + ...this.session.buildInProcessMcpServers(), + }; + await this.session.query.setMcpServers(next); + this.session.queryOptions.mcpServers = next; + this.refreshMcpMetadata(this.session.query); + this.logger.info("Reconnected signed-commit MCP server", logCtx); + return true; + } catch (error) { + this.logger.error("Failed to reconnect signed-commit MCP server", { + ...logCtx, + error: error instanceof Error ? error.message : String(error), + }); + return false; + } + } + + /** Clear stale MCP tool metadata, then re-fetch it for the new server set. */ + private refreshMcpMetadata(q: Query): void { + clearMcpToolMetadataCache(); + this.deferBackgroundFetches(q); } async setSessionMode( @@ -1531,32 +1620,44 @@ export class ClaudeAcpAgent extends BaseAcpAgent { const earlyModelId = settingsManager.getSettings().model || meta?.model || ""; - const mcpServers = supportsMcpInjection(earlyModelId) - ? parseMcpServers(params, this.logger) - : {}; // Register the in-process general local-tools MCP server. Tools self-gate // via the registry (e.g. signed-commit is cloud-only and needs a GH token), // so adding a tool needs no change here. In cloud runs `git commit`/`git // push` are blocked by the PreToolUse guard (and the sandbox git shim), so // the agent commits via the signed-commit tool instead. - const localToolsServer = createLocalToolsMcpServer( - { - cwd, - token: resolveGithubToken(), - taskId, - baseBranch: meta?.baseBranch, - }, - meta, - ); - if (localToolsServer) { - mcpServers[LOCAL_TOOLS_MCP_NAME] = localToolsServer; - } else if (cloudRun) { + // + // A closure so refresh/self-heal can rebuild a fresh instance (reusing one + // throws "Already connected to a transport"). Capture only the fields it + // needs so the session doesn't pin the whole meta object. + const baseBranch = meta?.baseBranch; + const environment = meta?.environment; + const buildInProcessMcpServers = (): Record< + string, + McpSdkServerConfigWithInstance + > => { + const server = createLocalToolsMcpServer( + { cwd, token: resolveGithubToken(), taskId, baseBranch }, + { environment }, + ); + return server ? { [LOCAL_TOOLS_MCP_NAME]: server } : {}; + }; + + const initialInProcess = buildInProcessMcpServers(); + const localToolsServerNames = Object.keys(initialInProcess); + if (localToolsServerNames.length === 0 && cloudRun) { this.logger.warn( - "Cloud run registered no local tools — missing GH_TOKEN/GITHUB_TOKEN? signed commits unavailable", + "Cloud run registered no local tools (missing GH_TOKEN/GITHUB_TOKEN?); signed commits unavailable", ); } + const mcpServers: Record = { + ...(supportsMcpInjection(earlyModelId) + ? parseMcpServers(params, this.logger) + : {}), + ...initialInProcess, + }; + const systemPrompt = buildSystemPrompt(meta?.systemPrompt); if (meta?.mcpToolApprovals) { @@ -1612,6 +1713,8 @@ export class ClaudeAcpAgent extends BaseAcpAgent { enrichmentDeps: this.enrichment?.deps, enrichedReadCache: this.enrichedReadCache, cloudMode: cloudRun, + onEnsureLocalToolsConnected: () => + this.ensureLocalToolsConnected("guard-hook"), taskState, onTaskStateChange: async () => { await this.client.sessionUpdate({ @@ -1632,6 +1735,8 @@ export class ClaudeAcpAgent extends BaseAcpAgent { const session: Session = { query: q, queryOptions: options, + buildInProcessMcpServers, + localToolsServerNames, input, cancelled: false, settingsManager, diff --git a/packages/agent/src/adapters/claude/hooks.test.ts b/packages/agent/src/adapters/claude/hooks.test.ts index 2e3dc5ab4e..0c6415b4ce 100644 --- a/packages/agent/src/adapters/claude/hooks.test.ts +++ b/packages/agent/src/adapters/claude/hooks.test.ts @@ -366,6 +366,47 @@ describe("createSignedCommitGuardHook", () => { ); expect(result).toEqual({ continue: true }); }); + + test("attempts a heal and keeps the standard message when tools are available", async () => { + const onHeal = vi.fn().mockResolvedValue(true); + const healingGuard = createSignedCommitGuardHook(logger, onHeal); + + const result = await healingGuard( + bashInput("git commit -m x"), + undefined, + opts, + ); + + expect(onHeal).toHaveBeenCalledTimes(1); + expect(result).toMatchObject({ + hookSpecificOutput: { + permissionDecision: "deny", + permissionDecisionReason: expect.stringContaining("git_signed_commit"), + }, + }); + }); + + test.each([ + ["resolves false", vi.fn().mockResolvedValue(false)], + ["throws", vi.fn().mockRejectedValue(new Error("reconnect boom"))], + ])("reassures the model when the heal %s", async (_label, onHeal) => { + const healingGuard = createSignedCommitGuardHook(logger, onHeal); + + const result = await healingGuard( + bashInput("git commit -m x"), + undefined, + opts, + ); + + expect(result).toMatchObject({ + hookSpecificOutput: { + permissionDecision: "deny", + permissionDecisionReason: expect.stringContaining( + "safe in the working tree", + ), + }, + }); + }); }); describe("createTaskHook", () => { diff --git a/packages/agent/src/adapters/claude/hooks.ts b/packages/agent/src/adapters/claude/hooks.ts index e840391dbe..c3d21423a3 100644 --- a/packages/agent/src/adapters/claude/hooks.ts +++ b/packages/agent/src/adapters/claude/hooks.ts @@ -341,7 +341,10 @@ function blocksUnsignedGit(command: string): boolean { * which creates GitHub-signed (Verified) commits via the API. */ export const createSignedCommitGuardHook = - (logger: Logger): HookCallback => + ( + logger: Logger, + onEnsureLocalToolsConnected?: () => Promise, + ): HookCallback => async (input: HookInput, _toolUseID: string | undefined) => { if (input.hook_event_name !== "PreToolUse") return { continue: true }; if (input.tool_name !== "Bash") return { continue: true }; @@ -355,16 +358,34 @@ export const createSignedCommitGuardHook = logger.info( `[SignedCommitGuard] Blocking unsigned git command: ${command}`, ); + + // Try to restore the server before denying; tailor the message to the result. + let toolsAvailable = true; + if (onEnsureLocalToolsConnected) { + try { + toolsAvailable = await onEnsureLocalToolsConnected(); + } catch { + toolsAvailable = false; + } + } + + const reason = toolsAvailable + ? "Commits must be signed: `git commit` and `git push` are disabled here. " + + "Stage changes with `git add`, then call the `git_signed_commit` tool " + + `(${SIGNED_COMMIT_QUALIFIED_TOOL_NAME}) with a \`message\` to create a signed ` + + "commit on the branch." + : "Commits must be signed, and the signed-commit tooling is momentarily " + + "reconnecting, so it isn't available this instant. Your staged and unstaged " + + "changes are safe in the working tree — nothing is lost. Wait a moment, then " + + `call the \`git_signed_commit\` tool (${SIGNED_COMMIT_QUALIFIED_TOOL_NAME}) with a ` + + "`message`; raw `git commit`/`git push` stay disabled."; + return { continue: true, hookSpecificOutput: { hookEventName: "PreToolUse" as const, permissionDecision: "deny" as const, - permissionDecisionReason: - "Commits must be signed: `git commit` and `git push` are disabled here. " + - "Stage changes with `git add`, then call the `git_signed_commit` tool " + - `(${SIGNED_COMMIT_QUALIFIED_TOOL_NAME}) with a \`message\` to create a signed ` + - "commit on the branch.", + permissionDecisionReason: reason, }, }; }; diff --git a/packages/agent/src/adapters/claude/session/options.test.ts b/packages/agent/src/adapters/claude/session/options.test.ts index 031253e45b..f214caa8cb 100644 --- a/packages/agent/src/adapters/claude/session/options.test.ts +++ b/packages/agent/src/adapters/claude/session/options.test.ts @@ -1,11 +1,32 @@ import * as os from "node:os"; import * as path from "node:path"; -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import type { HookInput, Options } from "@anthropic-ai/claude-agent-sdk"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { Logger } from "../../../utils/logger"; import { SUBAGENT_REWRITES } from "../hooks"; import { buildSessionOptions } from "./options"; import { SettingsManager } from "./settings"; +const GIT_COMMIT_HOOK_INPUT = { + session_id: "s", + transcript_path: "/tmp/t", + cwd: "/tmp", + hook_event_name: "PreToolUse", + tool_name: "Bash", + tool_use_id: "toolu_1", + tool_input: { command: "git commit -m x" }, +} as HookInput; + +async function runPreToolUseHooks(options: Options): Promise { + const opts = { signal: new AbortController().signal }; + const hooks = (options.hooks?.PreToolUse ?? []).flatMap( + (entry) => entry.hooks ?? [], + ); + for (const hook of hooks) { + await hook(GIT_COMMIT_HOOK_INPUT, undefined, opts); + } +} + function makeParams() { const cwd = path.join(os.tmpdir(), `options-test-${Date.now()}`); return { @@ -71,6 +92,32 @@ describe("buildSessionOptions", () => { expect(options.agents?.["ph-explore"]).toEqual(override); }); + it("threads onEnsureLocalToolsConnected into the signed-commit guard (cloud)", async () => { + const healSpy = vi.fn().mockResolvedValue(true); + await runPreToolUseHooks( + buildSessionOptions({ + ...makeParams(), + cloudMode: true, + onEnsureLocalToolsConnected: healSpy, + }), + ); + + expect(healSpy).toHaveBeenCalledTimes(1); + }); + + it("omits the signed-commit guard outside cloud mode", async () => { + const healSpy = vi.fn().mockResolvedValue(true); + await runPreToolUseHooks( + buildSessionOptions({ + ...makeParams(), + cloudMode: false, + onEnsureLocalToolsConnected: healSpy, + }), + ); + + expect(healSpy).not.toHaveBeenCalled(); + }); + describe("CLAUDE_CODE_EXECUTABLE", () => { const originalClaudeExecutable = process.env.CLAUDE_CODE_EXECUTABLE; diff --git a/packages/agent/src/adapters/claude/session/options.ts b/packages/agent/src/adapters/claude/session/options.ts index 75a379ad40..64e3a37b77 100644 --- a/packages/agent/src/adapters/claude/session/options.ts +++ b/packages/agent/src/adapters/claude/session/options.ts @@ -62,6 +62,9 @@ export interface BuildOptionsParams { onPostHogResourceUsed?: (subTool: string, commandText?: string) => void; /** Cloud task session — enables the signed-commit guard. */ cloudMode?: boolean; + /** Reactive self-heal invoked when the guard blocks a raw git commit/push. + * Returns whether signed-commit tooling is usable after the attempt. */ + onEnsureLocalToolsConnected?: () => Promise; /** Per-session task state populated by createTaskHook from SDK Task* events. */ taskState: TaskState; /** Called after createTaskHook mutates taskState so callers can emit a plan @@ -171,6 +174,7 @@ function buildHooks( enrichedReadCache: EnrichedReadCache | undefined, registeredAgents: ReadonlySet, cloudMode: boolean, + onEnsureLocalToolsConnected: (() => Promise) | undefined, taskState: TaskState, onTaskStateChange: (() => Promise) | undefined, ): Options["hooks"] { @@ -191,7 +195,9 @@ function buildHooks( createSubagentRewriteHook(logger, registeredAgents), ]; if (cloudMode) { - preToolUseHooks.push(createSignedCommitGuardHook(logger)); + preToolUseHooks.push( + createSignedCommitGuardHook(logger, onEnsureLocalToolsConnected), + ); } const taskHook = createTaskHook(taskState, onTaskStateChange); @@ -415,6 +421,7 @@ export function buildSessionOptions(params: BuildOptionsParams): Options { params.enrichedReadCache, registeredAgentNames, params.cloudMode ?? false, + params.onEnsureLocalToolsConnected, params.taskState, params.onTaskStateChange, ), diff --git a/packages/agent/src/adapters/claude/types.ts b/packages/agent/src/adapters/claude/types.ts index 6246da57b8..f44a7a5314 100644 --- a/packages/agent/src/adapters/claude/types.ts +++ b/packages/agent/src/adapters/claude/types.ts @@ -4,6 +4,7 @@ import type { TerminalOutputResponse, } from "@agentclientprotocol/sdk"; import type { + McpSdkServerConfigWithInstance, Options, Query, SDKUserMessage, @@ -46,6 +47,15 @@ export type Session = BaseSession & { query: Query; /** The Options object passed to query() — mutating it affects subsequent prompts */ queryOptions: Options; + /** Rebuilds the in-process ("sdk") signed-commit server with a fresh instance + * each call (reusing one throws "Already connected"); {} when none is enabled. */ + buildInProcessMcpServers: () => Record< + string, + McpSdkServerConfigWithInstance + >; + /** Names of the in-process servers registered at session start. Lets the + * self-heal check status without rebuilding instances on every prompt. */ + localToolsServerNames: string[]; input: Pushable; settingsManager: SettingsManager; permissionMode: CodeExecutionMode;