From 0519c19e3c683849b4fac532825fcc672c92d0c0 Mon Sep 17 00:00:00 2001 From: Tag Date: Wed, 25 Mar 2026 01:38:20 -0400 Subject: [PATCH] feat(agents): wire Copilot SDK session hooks for observability --- src/agents/copilot-runner.test.ts | 55 ++++++++++ src/agents/copilot-runner.ts | 4 + src/agents/copilot-sdk.ts | 5 + src/agents/copilot-session-hooks.test.ts | 122 +++++++++++++++++++++++ src/agents/copilot-session-hooks.ts | 101 +++++++++++++++++++ 5 files changed, 287 insertions(+) create mode 100644 src/agents/copilot-session-hooks.test.ts create mode 100644 src/agents/copilot-session-hooks.ts diff --git a/src/agents/copilot-runner.test.ts b/src/agents/copilot-runner.test.ts index c4cc7291f34ee..2699b08248fef 100644 --- a/src/agents/copilot-runner.test.ts +++ b/src/agents/copilot-runner.test.ts @@ -9,6 +9,12 @@ vi.mock("./copilot-sdk.js", () => ({ runCopilotAgent: (...args: unknown[]) => runCopilotAgentMock(...args), })); +// Mock copilot-session-hooks.js to verify it's called and hooks are passed through +const buildCopilotSessionHooksMock = vi.fn(); +vi.mock("./copilot-session-hooks.js", () => ({ + buildCopilotSessionHooks: (...args: unknown[]) => buildCopilotSessionHooksMock(...args), +})); + // Stub out bootstrap/docs resolution to avoid filesystem side effects vi.mock("./bootstrap-files.js", () => ({ resolveBootstrapContextForRun: vi.fn(async () => ({ contextFiles: [] })), @@ -25,6 +31,8 @@ describe("runCopilotCliAgent", () => { beforeEach(() => { checkCopilotAvailableMock.mockReset(); runCopilotAgentMock.mockReset(); + buildCopilotSessionHooksMock.mockReset(); + buildCopilotSessionHooksMock.mockReturnValue({ onSessionStart: vi.fn() }); }); it("throws FailoverError when copilot is not available", async () => { @@ -174,4 +182,51 @@ describe("runCopilotCliAgent", () => { expect(sdkArgs.model).toBeUndefined(); expect(result.meta?.agentMeta?.model).toBe("default"); }); + + it("builds session hooks and passes them to runCopilotAgent", async () => { + const fakeHooks = { onSessionStart: vi.fn(), onPreToolUse: vi.fn() }; + buildCopilotSessionHooksMock.mockReturnValue(fakeHooks); + checkCopilotAvailableMock.mockReturnValue({ available: true }); + runCopilotAgentMock.mockResolvedValueOnce({ + text: "hooked", + sessionId: "copilot-session-hooks", + }); + + await runCopilotCliAgent({ + sessionId: "s1", + sessionFile: "/tmp/session.jsonl", + workspaceDir: "/tmp", + prompt: "hello", + timeoutMs: 5_000, + runId: "run-hooks", + }); + + // Verify buildCopilotSessionHooks was called + expect(buildCopilotSessionHooksMock).toHaveBeenCalledTimes(1); + + // Verify hooks were passed through to runCopilotAgent + const sdkArgs = runCopilotAgentMock.mock.calls[0]?.[0]; + expect(sdkArgs.hooks).toBe(fakeHooks); + }); + + it("does not pass hooks when buildCopilotSessionHooks returns undefined", async () => { + buildCopilotSessionHooksMock.mockReturnValue(undefined); + checkCopilotAvailableMock.mockReturnValue({ available: true }); + runCopilotAgentMock.mockResolvedValueOnce({ + text: "no hooks", + sessionId: "copilot-session-no-hooks", + }); + + await runCopilotCliAgent({ + sessionId: "s1", + sessionFile: "/tmp/session.jsonl", + workspaceDir: "/tmp", + prompt: "hello", + timeoutMs: 5_000, + runId: "run-no-hooks", + }); + + const sdkArgs = runCopilotAgentMock.mock.calls[0]?.[0]; + expect(sdkArgs.hooks).toBeUndefined(); + }); }); diff --git a/src/agents/copilot-runner.ts b/src/agents/copilot-runner.ts index 252708ccd43c0..c1e598ac56ce1 100644 --- a/src/agents/copilot-runner.ts +++ b/src/agents/copilot-runner.ts @@ -7,6 +7,7 @@ import { makeBootstrapWarn, resolveBootstrapContextForRun } from "./bootstrap-fi import { resolveCliBackendConfig } from "./cli-backends.js"; import { buildSystemPrompt, normalizeCliModel } from "./cli-runner/helpers.js"; import { checkCopilotAvailable, runCopilotAgent } from "./copilot-sdk.js"; +import { buildCopilotSessionHooks } from "./copilot-session-hooks.js"; import { resolveOpenClawDocsPath } from "./docs-path.js"; import { FailoverError, resolveFailoverStatus } from "./failover-error.js"; import { classifyFailoverReason, isFailoverErrorMessage } from "./pi-embedded-helpers.js"; @@ -114,6 +115,8 @@ export async function runCopilotCliAgent(params: { try { log.info(`copilot-cli exec: model=${modelId} promptChars=${params.prompt.length}`); + const hooks = buildCopilotSessionHooks(); + const result = await runCopilotAgent({ prompt: params.prompt, model: modelId === "default" ? undefined : modelId, @@ -121,6 +124,7 @@ export async function runCopilotCliAgent(params: { systemPrompt, timeoutMs: params.timeoutMs, sessionId: params.cliSessionId, + hooks, }); const text = result.text?.trim(); diff --git a/src/agents/copilot-sdk.ts b/src/agents/copilot-sdk.ts index 067dffbb9c0b9..9719880c48be2 100644 --- a/src/agents/copilot-sdk.ts +++ b/src/agents/copilot-sdk.ts @@ -5,6 +5,9 @@ import type { ModelInfo, SessionConfig, } from "@github/copilot-sdk"; + +/** Derive SessionHooks from SessionConfig since it's not directly exported. */ +export type SessionHooks = NonNullable; import { createSubsystemLogger } from "../logging/subsystem.js"; const log = createSubsystemLogger("agents/copilot-sdk"); @@ -116,6 +119,7 @@ export type CopilotAgentRunOptions = { systemPrompt?: string; timeoutMs?: number; sessionId?: string; + hooks?: SessionHooks; }; export type CopilotAgentRunResult = { @@ -143,6 +147,7 @@ export async function runCopilotAgent( model: options.model, workingDirectory: options.workspaceDir, streaming: true, + hooks: options.hooks, // Deny tool-use permission requests by default (security: callers must // opt-in to specific capabilities through session configuration). onPermissionRequest: async () => ({ diff --git a/src/agents/copilot-session-hooks.test.ts b/src/agents/copilot-session-hooks.test.ts new file mode 100644 index 0000000000000..546cea415a089 --- /dev/null +++ b/src/agents/copilot-session-hooks.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, it, vi } from "vitest"; + +// Stub logging to avoid side effects +vi.mock("../logging/subsystem.js", () => ({ + createSubsystemLogger: () => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }), +})); + +import { buildCopilotSessionHooks } from "./copilot-session-hooks.js"; + +describe("buildCopilotSessionHooks", () => { + const invocation = { sessionId: "test-session" }; + + it("returns hooks with all expected handlers", () => { + const hooks = buildCopilotSessionHooks(); + expect(hooks.onSessionStart).toBeTypeOf("function"); + expect(hooks.onSessionEnd).toBeTypeOf("function"); + expect(hooks.onPreToolUse).toBeTypeOf("function"); + expect(hooks.onPostToolUse).toBeTypeOf("function"); + expect(hooks.onUserPromptSubmitted).toBeTypeOf("function"); + expect(hooks.onErrorOccurred).toBeTypeOf("function"); + }); + + it("onPreToolUse denies dangerous tools by default", () => { + const hooks = buildCopilotSessionHooks(); + const result = hooks.onPreToolUse!( + { + toolName: "delete_file", + toolArgs: { path: "/etc/hosts" }, + timestamp: Date.now(), + cwd: "/tmp", + }, + invocation, + ); + expect(result).toEqual(expect.objectContaining({ permissionDecision: "deny" })); + }); + + it("onPreToolUse allows non-dangerous tools", () => { + const hooks = buildCopilotSessionHooks(); + const result = hooks.onPreToolUse!( + { toolName: "read_file", toolArgs: { path: "foo.ts" }, timestamp: Date.now(), cwd: "/tmp" }, + invocation, + ); + expect(result).toBeUndefined(); + }); + + it("onPreToolUse respects custom denyTools", () => { + const hooks = buildCopilotSessionHooks({ denyTools: ["my_custom_tool"] }); + const result = hooks.onPreToolUse!( + { toolName: "my_custom_tool", toolArgs: {}, timestamp: Date.now(), cwd: "/tmp" }, + invocation, + ); + expect(result).toEqual(expect.objectContaining({ permissionDecision: "deny" })); + }); + + it("onPreToolUse skips deny list when disableToolDenyList is true", () => { + const hooks = buildCopilotSessionHooks({ disableToolDenyList: true }); + const result = hooks.onPreToolUse!( + { toolName: "delete_file", toolArgs: {}, timestamp: Date.now(), cwd: "/tmp" }, + invocation, + ); + expect(result).toBeUndefined(); + }); + + it("onErrorOccurred returns retry for recoverable errors", () => { + const hooks = buildCopilotSessionHooks(); + const result = hooks.onErrorOccurred!( + { + error: "timeout", + errorContext: "model_call", + recoverable: true, + timestamp: Date.now(), + cwd: "/tmp", + }, + invocation, + ); + expect(result).toEqual(expect.objectContaining({ errorHandling: "retry", retryCount: 1 })); + }); + + it("onErrorOccurred returns nothing for non-recoverable errors", () => { + const hooks = buildCopilotSessionHooks(); + const result = hooks.onErrorOccurred!( + { + error: "fatal", + errorContext: "system", + recoverable: false, + timestamp: Date.now(), + cwd: "/tmp", + }, + invocation, + ); + expect(result).toBeUndefined(); + }); + + it("onUserPromptSubmitted does not expose prompt content", () => { + const hooks = buildCopilotSessionHooks(); + // Should return void (no modifications) + const result = hooks.onUserPromptSubmitted!( + { prompt: "secret password is abc123", timestamp: Date.now(), cwd: "/tmp" }, + invocation, + ); + expect(result).toBeUndefined(); + }); + + it("onSessionStart and onSessionEnd return void", () => { + const hooks = buildCopilotSessionHooks(); + const startResult = hooks.onSessionStart!( + { source: "new", timestamp: Date.now(), cwd: "/tmp" }, + invocation, + ); + expect(startResult).toBeUndefined(); + + const endResult = hooks.onSessionEnd!( + { reason: "complete", timestamp: Date.now(), cwd: "/tmp" }, + invocation, + ); + expect(endResult).toBeUndefined(); + }); +}); diff --git a/src/agents/copilot-session-hooks.ts b/src/agents/copilot-session-hooks.ts new file mode 100644 index 0000000000000..98759d61fb3bb --- /dev/null +++ b/src/agents/copilot-session-hooks.ts @@ -0,0 +1,101 @@ +import { createSubsystemLogger } from "../logging/subsystem.js"; +import type { SessionHooks } from "./copilot-sdk.js"; + +const log = createSubsystemLogger("agents/copilot-hooks"); + +/** + * Tools that are denied by default in hook-managed sessions. + * These are high-risk tools that should require explicit opt-in. + */ +const DENIED_TOOLS = new Set(["delete_file", "delete_directory", "execute_command", "run_shell"]); + +/** + * Build observability hooks for Copilot SDK sessions. + * Logs all hook events via the subsystem logger and optionally denies + * dangerous tool invocations. + */ +export function buildCopilotSessionHooks(options?: { + /** Additional tool names to deny. Merged with the built-in deny list. */ + denyTools?: string[]; + /** Disable the built-in tool deny list entirely. */ + disableToolDenyList?: boolean; +}): SessionHooks { + const startedAt = Date.now(); + const denySet = options?.disableToolDenyList + ? new Set(options?.denyTools ?? []) + : new Set([...DENIED_TOOLS, ...(options?.denyTools ?? [])]); + + return { + onSessionStart: (input, { sessionId }) => { + log.info("session started", { + sessionId, + source: input.source, + }); + }, + + onSessionEnd: (input, { sessionId }) => { + const durationMs = Date.now() - startedAt; + log.info("session ended", { + sessionId, + reason: input.reason, + durationMs, + ...(input.error ? { error: input.error } : {}), + }); + }, + + onPreToolUse: (input, { sessionId }) => { + log.info("pre-tool-use", { + sessionId, + tool: input.toolName, + argsKeys: + input.toolArgs && typeof input.toolArgs === "object" + ? Object.keys(input.toolArgs as Record) + : undefined, + }); + + if (denySet.has(input.toolName)) { + log.warn("tool denied by hook policy", { + sessionId, + tool: input.toolName, + }); + return { + permissionDecision: "deny" as const, + permissionDecisionReason: `Tool "${input.toolName}" is denied by session hook policy.`, + }; + } + }, + + onPostToolUse: (input, { sessionId }) => { + const resultStr = input.toolResult != null ? JSON.stringify(input.toolResult) : ""; + log.info("post-tool-use", { + sessionId, + tool: input.toolName, + resultLength: resultStr.length, + }); + }, + + onUserPromptSubmitted: (input, { sessionId }) => { + // Log prompt length only — not content, for privacy + log.info("user prompt submitted", { + sessionId, + promptLength: input.prompt.length, + }); + }, + + onErrorOccurred: (input, { sessionId }) => { + log.error("error occurred", { + sessionId, + error: input.error, + context: input.errorContext, + recoverable: input.recoverable, + }); + + if (input.recoverable) { + return { + errorHandling: "retry" as const, + retryCount: 1, + }; + } + }, + }; +}