From 44500daaa059be57fae08908206e342da3d2c2cd Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Thu, 18 Jun 2026 21:32:46 +0800 Subject: [PATCH] refactor(copilot): remove dormant bridge scaffolding --- docs/plugins/copilot.md | 7 +- extensions/copilot/src/attempt.test.ts | 3 +- extensions/copilot/src/attempt.ts | 12 +- .../copilot/src/telemetry-bridge.test.ts | 253 ------------------ extensions/copilot/src/telemetry-bridge.ts | 219 --------------- .../copilot/src/user-input-bridge.test.ts | 231 ---------------- extensions/copilot/src/user-input-bridge.ts | 244 ----------------- scripts/deadcode-unused-files.allowlist.mjs | 2 - 8 files changed, 5 insertions(+), 966 deletions(-) delete mode 100755 extensions/copilot/src/telemetry-bridge.test.ts delete mode 100755 extensions/copilot/src/telemetry-bridge.ts delete mode 100755 extensions/copilot/src/user-input-bridge.test.ts delete mode 100755 extensions/copilot/src/user-input-bridge.ts diff --git a/docs/plugins/copilot.md b/docs/plugins/copilot.md index 259e633981c..c838e84d05c 100755 --- a/docs/plugins/copilot.md +++ b/docs/plugins/copilot.md @@ -179,8 +179,7 @@ The harness reads its config from per-attempt input registered with `overridesBuiltInTool: true` and `skipPermission: true` so 100% of tool calls flow through OpenClaw's wrapped `execute()`. See [Permissions and ask_user](#permissions-and-ask_user). -- `enableSessionTelemetry` — opt-in OpenTelemetry routing via - `telemetry-bridge.ts`. +- `enableSessionTelemetry` — optional SDK session telemetry flag. Nothing in the rest of OpenClaw needs to know about these fields. Other plugins, channels, and core code only see the standard @@ -267,9 +266,7 @@ real Copilot CLI or touch the host fs. decisions from the initial prompt rather than asking clarifying questions mid-turn. A follow-up will port the codex pattern at `extensions/codex/src/app-server/user-input-bridge.ts` to route SDK - `UserInputRequest`s through the OpenClaw channel/TUI prompt path; the - dormant scaffolding in `extensions/copilot/src/user-input-bridge.ts` - is the surface that follow-up will wire. + `UserInputRequest`s through the OpenClaw channel/TUI prompt path. ## Permissions and ask_user diff --git a/extensions/copilot/src/attempt.test.ts b/extensions/copilot/src/attempt.test.ts index 337a39050e5..60596b89273 100644 --- a/extensions/copilot/src/attempt.test.ts +++ b/extensions/copilot/src/attempt.test.ts @@ -983,8 +983,7 @@ describe("runCopilotAttempt", () => { // Per the SDK contract (types.d.ts: `When provided, enables the // ask_user tool allowing the agent to ask questions`), omitting the // handler hides ask_user from the model entirely. The MVP keeps it - // hidden; a follow-up will port the codex user-input-bridge to wire - // ask_user to the OpenClaw channel/TUI path. + // hidden until a real channel/TUI prompt bridge exists. expect("onUserInputRequest" in cfg).toBe(false); }); diff --git a/extensions/copilot/src/attempt.ts b/extensions/copilot/src/attempt.ts index 2447bba419f..59dce825701 100644 --- a/extensions/copilot/src/attempt.ts +++ b/extensions/copilot/src/attempt.ts @@ -763,14 +763,8 @@ function createSessionConfig( onPermissionRequest: createPermissionBridge(permissionPolicy), // `onUserInputRequest` is intentionally NOT registered: per the SDK // contract, omitting the handler hides the `ask_user` tool from the - // model entirely. This is the MVP posture — interactive ask_user - // requires routing the request to the OpenClaw channel/TUI prompt - // path (mirroring extensions/codex/src/app-server/user-input-bridge.ts), - // which is tracked as a follow-up. With the handler absent, agents - // running under this harness must make best-judgment decisions from - // the initial prompt rather than asking clarifying questions - // mid-turn. See user-input-bridge.ts for the dormant policy - // scaffolding the follow-up will reuse. + // model entirely. Interactive ask_user will need a real channel/TUI + // prompt bridge before this runtime can expose the handler. // SessionHooks: only set when the host actually supplied handlers. // createHooksBridge returns undefined for an empty config so we // never install an empty hooks subsystem. See hooks-bridge.ts for @@ -779,8 +773,6 @@ function createSessionConfig( // Session-level telemetry opt-out: only propagate when the host // explicitly set a boolean. undefined means "use SDK default" // (enabled for GitHub auth; disabled when a BYOK provider is set). - // Client-level OTel config is plumbed via runtime.ts / - // telemetry-bridge.ts. ...(typeof params.enableSessionTelemetry === "boolean" ? { enableSessionTelemetry: params.enableSessionTelemetry } : {}), diff --git a/extensions/copilot/src/telemetry-bridge.test.ts b/extensions/copilot/src/telemetry-bridge.test.ts deleted file mode 100755 index f92f78b1f11..00000000000 --- a/extensions/copilot/src/telemetry-bridge.test.ts +++ /dev/null @@ -1,253 +0,0 @@ -// Copilot tests cover telemetry bridge plugin behavior. -import { describe, expect, it, vi } from "vitest"; -import { - createTelemetryConfig, - createTraceContextProvider, - type CopilotTraceContextErrorInfo, -} from "./telemetry-bridge.js"; - -describe("createTelemetryConfig", () => { - it("returns undefined for undefined input", () => { - expect(createTelemetryConfig()).toBeUndefined(); - }); - - it("returns undefined when every field is undefined", () => { - expect(createTelemetryConfig({})).toBeUndefined(); - expect( - createTelemetryConfig({ - otlpEndpoint: undefined, - filePath: undefined, - }), - ).toBeUndefined(); - }); - - it("includes only the fields that were explicitly set", () => { - expect(createTelemetryConfig({ otlpEndpoint: "https://otel.example/v1/traces" })).toEqual({ - otlpEndpoint: "https://otel.example/v1/traces", - }); - expect(createTelemetryConfig({ sourceName: "openclaw" })).toEqual({ - sourceName: "openclaw", - }); - }); - - it("round-trips a fully populated config", () => { - const result = createTelemetryConfig({ - otlpEndpoint: "https://otel.example/v1/traces", - filePath: "/tmp/openclaw-traces.jsonl", - exporterType: "otlp-http", - sourceName: "openclaw", - captureContent: true, - }); - expect(result).toEqual({ - otlpEndpoint: "https://otel.example/v1/traces", - filePath: "/tmp/openclaw-traces.jsonl", - exporterType: "otlp-http", - sourceName: "openclaw", - captureContent: true, - }); - }); - - it("preserves captureContent: false (explicit disable, not undefined)", () => { - expect(createTelemetryConfig({ captureContent: false })).toEqual({ - captureContent: false, - }); - }); - - it("preserves empty-string values (caller chose to set them)", () => { - expect(createTelemetryConfig({ otlpEndpoint: "" })).toEqual({ otlpEndpoint: "" }); - }); -}); - -describe("createTraceContextProvider", () => { - it("returns an empty context when no sources are configured", async () => { - const provider = createTraceContextProvider(); - await expect(provider()).resolves.toEqual({}); - }); - - it("prefers getTraceContext over the convenience sources", async () => { - const getTraceContext = vi.fn().mockResolvedValue({ - traceparent: "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - tracestate: "vendor=value", - }); - const getTraceparent = vi.fn().mockResolvedValue("00-ffff-ffff-01"); - const provider = createTraceContextProvider({ getTraceContext, getTraceparent }); - const ctx = await provider(); - expect(ctx).toEqual({ - traceparent: "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - tracestate: "vendor=value", - }); - expect(getTraceparent).not.toHaveBeenCalled(); - }); - - it("falls back to getTraceparent when getTraceContext returns undefined", async () => { - const getTraceContext = vi.fn().mockResolvedValue(undefined); - const getTraceparent = vi - .fn() - .mockResolvedValue("00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01"); - const provider = createTraceContextProvider({ getTraceContext, getTraceparent }); - await expect(provider()).resolves.toEqual({ - traceparent: "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - }); - expect(getTraceContext).toHaveBeenCalledTimes(1); - expect(getTraceparent).toHaveBeenCalledTimes(1); - }); - - it("includes tracestate when both convenience sources return non-empty values", async () => { - const provider = createTraceContextProvider({ - getTraceparent: () => "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - getTracestate: () => "vendor=value", - }); - await expect(provider()).resolves.toEqual({ - traceparent: "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - tracestate: "vendor=value", - }); - }); - - it("omits empty/undefined tracestate even when traceparent is present", async () => { - const providerUndef = createTraceContextProvider({ - getTraceparent: () => "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - getTracestate: () => undefined, - }); - await expect(providerUndef()).resolves.toEqual({ - traceparent: "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - }); - const providerEmpty = createTraceContextProvider({ - getTraceparent: () => "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - getTracestate: () => "", - }); - await expect(providerEmpty()).resolves.toEqual({ - traceparent: "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - }); - }); - - it("does not propagate tracestate without traceparent (W3C requirement)", async () => { - const getTracestate = vi.fn().mockResolvedValue("vendor=value"); - const provider = createTraceContextProvider({ - getTraceparent: () => undefined, - getTracestate, - }); - await expect(provider()).resolves.toEqual({}); - expect(getTracestate).not.toHaveBeenCalled(); - }); - - it("re-reads sources on every invocation (so caching the provider is safe)", async () => { - let parent = "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01"; - const provider = createTraceContextProvider({ getTraceparent: () => parent }); - await expect(provider()).resolves.toEqual({ traceparent: parent }); - parent = "00-cccccccccccccccccccccccccccccccc-dddddddddddddddd-01"; - await expect(provider()).resolves.toEqual({ traceparent: parent }); - }); - - it("getTraceContext failure → empty context + notifier called with the original error", async () => { - const onError = vi.fn(); - const provider = createTraceContextProvider({ - getTraceContext: () => { - throw new Error("ctx-boom"); - }, - getTraceparent: () => "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - onError, - }); - await expect(provider()).resolves.toEqual({}); - expect(onError).toHaveBeenCalledTimes(1); - const info = onError.mock.calls[0]?.[0] as CopilotTraceContextErrorInfo; - expect(info.part).toBe("traceContext"); - expect(info.error.message).toBe("ctx-boom"); - }); - - it("getTraceparent failure → empty context + notifier called", async () => { - const onError = vi.fn(); - const provider = createTraceContextProvider({ - getTraceparent: async () => { - throw new Error("parent-boom"); - }, - getTracestate: () => "vendor=value", - onError, - }); - await expect(provider()).resolves.toEqual({}); - expect(onError).toHaveBeenCalledTimes(1); - expect((onError.mock.calls[0][0] as CopilotTraceContextErrorInfo).part).toBe("traceparent"); - }); - - it("getTracestate failure → partial success (traceparent kept) + notifier called", async () => { - const onError = vi.fn(); - const provider = createTraceContextProvider({ - getTraceparent: () => "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - getTracestate: () => { - throw new Error("state-boom"); - }, - onError, - }); - await expect(provider()).resolves.toEqual({ - traceparent: "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01", - }); - expect(onError).toHaveBeenCalledTimes(1); - expect((onError.mock.calls[0][0] as CopilotTraceContextErrorInfo).part).toBe("tracestate"); - }); - - it("default notifier uses console.warn", async () => { - const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); - try { - const provider = createTraceContextProvider({ - getTraceparent: () => { - throw new Error("default-warn-path"); - }, - }); - await expect(provider()).resolves.toEqual({}); - expect(warnSpy).toHaveBeenCalledTimes(1); - expect(String(warnSpy.mock.calls[0]?.[0])).toContain("traceparent"); - expect(String(warnSpy.mock.calls[0]?.[0])).toContain("default-warn-path"); - } finally { - warnSpy.mockRestore(); - } - }); - - it("normalizes non-Error throws into Error before notifying", async () => { - const onError = vi.fn(); - const provider = createTraceContextProvider({ - getTraceparent: () => { - throw toLintErrorObject("string-boom", "Non-Error thrown"); - }, - onError, - }); - await expect(provider()).resolves.toEqual({}); - const info = onError.mock.calls[0]?.[0] as CopilotTraceContextErrorInfo; - expect(info.error).toBeInstanceOf(Error); - expect(info.error.message).toBe("string-boom"); - }); - - it("notifier throws are swallowed (provider always resolves)", async () => { - const provider = createTraceContextProvider({ - getTraceparent: () => { - throw new Error("boom"); - }, - onError: () => { - throw new Error("notifier-boom"); - }, - }); - await expect(provider()).resolves.toEqual({}); - }); - - it("treats only-traceContext source returning empty object as a valid context (no fallback)", async () => { - const getTraceparent = vi.fn(); - const provider = createTraceContextProvider({ - getTraceContext: () => ({}), - getTraceparent, - }); - await expect(provider()).resolves.toEqual({}); - expect(getTraceparent).not.toHaveBeenCalled(); - }); -}); - -function toLintErrorObject(value: unknown, fallbackMessage: string): Error { - if (value instanceof Error) { - return value; - } - if (typeof value === "string") { - return new Error(value); - } - const error = new Error(fallbackMessage, { cause: value }); - if ((typeof value === "object" && value !== null) || typeof value === "function") { - Object.assign(error, value); - } - return error; -} diff --git a/extensions/copilot/src/telemetry-bridge.ts b/extensions/copilot/src/telemetry-bridge.ts deleted file mode 100755 index 8cc4e78f14d..00000000000 --- a/extensions/copilot/src/telemetry-bridge.ts +++ /dev/null @@ -1,219 +0,0 @@ -// Copilot plugin module implements telemetry bridge behavior. -import type { CopilotClientOptions } from "@github/copilot-sdk"; - -// Telemetry bridge for the GitHub Copilot agent runtime. -// -// SDK surface: -// - `CopilotClientOptions.telemetry?: TelemetryConfig` — OpenTelemetry -// configuration applied to the spawned CLI process via env vars. -// - `CopilotClientOptions.onGetTraceContext?: TraceContextProvider` — -// async callback returning a W3C `{traceparent?, tracestate?}` that the -// SDK injects into `session.create`, `session.resume`, and -// `session.send` RPCs for distributed trace propagation. -// -// Host-side back-pointers (NOT imported here to keep the package boundary -// clean — the wiring layer injects these via callbacks): -// - `src/infra/diagnostic-trace-context.ts` — `getActiveDiagnosticTraceContext`, -// `formatDiagnosticTraceparent`, `DiagnosticTraceContext`. -// - `src/infra/diagnostic-events.ts` — `formatDiagnosticTraceparentForPropagation` -// for trusted-only propagation. -// -// IMPORTANT — pool reuse caveat: -// `CopilotClientPool` keys on `{agentId, copilotHome, authMode, -// authProfileId, authProfileVersion}`. Client-level telemetry and -// `onGetTraceContext` are NOT part of the pool key. Two callers that -// share a pool key but supply different telemetry options will get the -// first-acquire's options ("first wins"). Mitigation: -// - The trace-context provider returned by `createTraceContextProvider` -// reads the active context **on every invocation**, so even when the -// provider function is cached the propagated `traceparent` reflects -// the current scope at RPC time. Per-call accuracy is preserved. -// - `TelemetryConfig` (OTel env vars) is genuinely first-wins because -// the CLI subprocess is spawned once per pool entry. Wire telemetry -// as a process-wide / per-agent setting, not per-attempt. - -type SdkTraceContext = NonNullable< - Awaited>> ->; -type SdkTraceContextProvider = NonNullable; -type SdkTelemetryConfig = NonNullable; - -export type { SdkTraceContext as CopilotTraceContext }; -export type { SdkTelemetryConfig as CopilotTelemetryConfig }; - -export type CopilotTraceContextSource = () => - | SdkTraceContext - | undefined - | Promise; -export type CopilotTraceparentSource = () => string | undefined | Promise; -export type CopilotTracestateSource = () => string | undefined | Promise; - -export interface CopilotTraceContextErrorInfo { - readonly part: "traceContext" | "traceparent" | "tracestate"; - readonly error: Error; -} - -export interface CopilotTraceContextOptions { - /** - * Primary source: a single callback returning the full SDK trace context - * (`{traceparent?, tracestate?}`). Use this when the host has one - * authoritative source of trace context so that traceparent and tracestate - * always reflect the same logical scope. - */ - getTraceContext?: CopilotTraceContextSource; - /** - * Convenience source: returns just the W3C `traceparent` header. Used - * when {@link getTraceContext} is not supplied OR returns undefined. - */ - getTraceparent?: CopilotTraceparentSource; - /** - * Convenience source: returns the W3C `tracestate` header. Only used - * when {@link getTraceContext} is not supplied AND a non-empty - * `traceparent` was obtained via {@link getTraceparent}. (Per W3C, - * `tracestate` is meaningless without an accompanying `traceparent`.) - */ - getTracestate?: CopilotTracestateSource; - /** - * Notifier for errors thrown by any source. Defaults to `console.warn`. - * Notifier failures are themselves swallowed. - */ - onError?: (info: CopilotTraceContextErrorInfo) => void; -} - -const EMPTY_TRACE_CONTEXT: SdkTraceContext = Object.freeze({}) as SdkTraceContext; - -function toError(error: unknown): Error { - if (error instanceof Error) { - return error; - } - return new Error(String(error)); -} - -function defaultOnTraceContextError(info: CopilotTraceContextErrorInfo): void { - console.warn(`[copilot:telemetry-bridge] ${info.part} source failed: ${info.error.message}`); -} - -function safeNotify( - notifier: (info: CopilotTraceContextErrorInfo) => void, - info: CopilotTraceContextErrorInfo, -): void { - try { - notifier(info); - } catch { - // Notifier failures are swallowed: telemetry is best-effort. - } -} - -function isNonEmptyString(value: unknown): value is string { - return typeof value === "string" && value.length > 0; -} - -/** - * Build a TraceContextProvider suitable for `CopilotClientOptions.onGetTraceContext`. - * - * Resolution order on each invocation: - * 1. If `getTraceContext` is supplied and returns a non-undefined value, - * return it as-is. Errors from this source → return `{}` and notify. - * 2. Otherwise call `getTraceparent` (if supplied). On error → return - * `{}` and notify (no traceparent = no propagation). - * 3. If traceparent is non-empty, call `getTracestate` (if supplied) - * and attach the result. Errors on tracestate are partial-success: - * notify and return `{traceparent}` (do not lose the parent). - * 4. If no source provided OR all return undefined, return `{}` so the - * SDK behaves as if no provider were configured. - */ -export function createTraceContextProvider( - options?: CopilotTraceContextOptions, -): SdkTraceContextProvider { - const onError = options?.onError ?? defaultOnTraceContextError; - const getTraceContext = options?.getTraceContext; - const getTraceparent = options?.getTraceparent; - const getTracestate = options?.getTracestate; - - return async () => { - if (getTraceContext) { - try { - const ctx = await getTraceContext(); - if (ctx !== undefined) { - return ctx; - } - } catch (error) { - safeNotify(onError, { part: "traceContext", error: toError(error) }); - return EMPTY_TRACE_CONTEXT; - } - } - - if (!getTraceparent) { - return EMPTY_TRACE_CONTEXT; - } - - let traceparent: string | undefined; - try { - traceparent = await getTraceparent(); - } catch (error) { - safeNotify(onError, { part: "traceparent", error: toError(error) }); - return EMPTY_TRACE_CONTEXT; - } - if (!isNonEmptyString(traceparent)) { - return EMPTY_TRACE_CONTEXT; - } - - if (!getTracestate) { - return { traceparent } as SdkTraceContext; - } - - let tracestate: string | undefined; - try { - tracestate = await getTracestate(); - } catch (error) { - safeNotify(onError, { part: "tracestate", error: toError(error) }); - return { traceparent } as SdkTraceContext; - } - - return isNonEmptyString(tracestate) - ? ({ traceparent, tracestate } as SdkTraceContext) - : ({ traceparent } as SdkTraceContext); - }; -} - -export interface CopilotTelemetryOptions { - otlpEndpoint?: string; - filePath?: string; - exporterType?: string; - sourceName?: string; - captureContent?: boolean; -} - -/** - * Shape a `TelemetryConfig` for `CopilotClientOptions.telemetry`. Returns - * `undefined` when no fields are supplied so callers can spread - * conditionally without producing an empty telemetry object that would - * still partially configure the CLI's OTel env layout. - * - * Any explicitly-set value (including `false` for `captureContent`) is - * preserved — only `undefined` is treated as "no opinion". - */ -export function createTelemetryConfig( - options?: CopilotTelemetryOptions, -): SdkTelemetryConfig | undefined { - if (!options) { - return undefined; - } - const result: SdkTelemetryConfig = {}; - if (options.otlpEndpoint !== undefined) { - result.otlpEndpoint = options.otlpEndpoint; - } - if (options.filePath !== undefined) { - result.filePath = options.filePath; - } - if (options.exporterType !== undefined) { - result.exporterType = options.exporterType; - } - if (options.sourceName !== undefined) { - result.sourceName = options.sourceName; - } - if (options.captureContent !== undefined) { - result.captureContent = options.captureContent; - } - return Object.keys(result).length > 0 ? result : undefined; -} diff --git a/extensions/copilot/src/user-input-bridge.test.ts b/extensions/copilot/src/user-input-bridge.test.ts deleted file mode 100755 index 3dd339ad8ac..00000000000 --- a/extensions/copilot/src/user-input-bridge.test.ts +++ /dev/null @@ -1,231 +0,0 @@ -// Copilot tests cover user input bridge plugin behavior. -import type { SessionConfig } from "@github/copilot-sdk"; -import { describe, expect, it, vi } from "vitest"; - -type UserInputHandler = NonNullable; -type SdkUserInputRequest = Parameters[0]; -type SdkUserInputResponse = Awaited>; - -import { - composeUserInputPolicies, - createUserInputBridge, - delegatingUserInputPolicy, - denyAllUserInputPolicy, - firstChoicePolicy, - staticAnswerPolicy, - DENY_ALL_ANSWER, - type CopilotUserInputContext, - type CopilotUserInputPolicy, -} from "./user-input-bridge.js"; - -function makeRequest(overrides: Partial = {}): SdkUserInputRequest { - return { - question: "what is your name?", - ...overrides, - }; -} - -function makeCtx(overrides: Partial = {}): CopilotUserInputContext { - return { - request: makeRequest(), - sessionId: "sess-1", - ...overrides, - }; -} - -describe("denyAllUserInputPolicy", () => { - it("returns the fail-closed DENY_ALL_ANSWER as a freeform answer", async () => { - const result = await denyAllUserInputPolicy(makeCtx()); - expect(result).toEqual({ answer: DENY_ALL_ANSWER, wasFreeform: true }); - }); -}); - -describe("firstChoicePolicy", () => { - it("returns the first choice (wasFreeform: false) when choices are present", async () => { - const result = await firstChoicePolicy( - makeCtx({ request: makeRequest({ choices: ["yes", "no"] }) }), - ); - expect(result).toEqual({ answer: "yes", wasFreeform: false }); - }); - - it("falls back to DENY_ALL_ANSWER when choices are empty", async () => { - const result = await firstChoicePolicy(makeCtx({ request: makeRequest({ choices: [] }) })); - expect(result).toEqual({ answer: DENY_ALL_ANSWER, wasFreeform: true }); - }); - - it("falls back to DENY_ALL_ANSWER when choices are absent", async () => { - const result = await firstChoicePolicy(makeCtx()); - expect(result).toEqual({ answer: DENY_ALL_ANSWER, wasFreeform: true }); - }); -}); - -describe("staticAnswerPolicy", () => { - it("returns the configured answer for every request", async () => { - const policy = staticAnswerPolicy({ answer: "Alice" }); - for (const question of ["a?", "b?", "c?"]) { - const result = await policy(makeCtx({ request: makeRequest({ question }) })); - expect(result).toEqual({ answer: "Alice", wasFreeform: true }); - } - }); - - it("respects wasFreeform=false override", async () => { - const policy = staticAnswerPolicy({ answer: "yes", wasFreeform: false }); - const result = await policy(makeCtx()); - expect(result).toEqual({ answer: "yes", wasFreeform: false }); - }); -}); - -describe("delegatingUserInputPolicy", () => { - it("forwards the request and returns the host response", async () => { - const onRequest = vi - .fn() - .mockResolvedValue({ answer: "Bob", wasFreeform: true } satisfies SdkUserInputResponse); - const policy = delegatingUserInputPolicy({ onRequest }); - const ctx = makeCtx({ sessionId: "sess-xyz" }); - const result = await policy(ctx); - expect(result).toEqual({ answer: "Bob", wasFreeform: true }); - expect(onRequest).toHaveBeenCalledTimes(1); - expect(onRequest).toHaveBeenCalledWith(ctx); - }); - - it("returns DENY_ALL_ANSWER when host callback returns undefined", async () => { - const onRequest = vi.fn().mockResolvedValue(undefined); - const policy = delegatingUserInputPolicy({ onRequest }); - const result = await policy(makeCtx()); - expect(result).toEqual({ answer: DENY_ALL_ANSWER, wasFreeform: true }); - }); - - it("converts thrown errors into a DENY_ALL_ANSWER with the error message appended", async () => { - const policy = delegatingUserInputPolicy({ - onRequest: () => { - throw new Error("prompt timeout"); - }, - }); - const result = await policy(makeCtx()); - expect(result).toBeDefined(); - expect(result!.wasFreeform).toBe(true); - expect(result!.answer).toContain(DENY_ALL_ANSWER); - expect(result!.answer).toContain("prompt timeout"); - }); - - it("falls back to onError policy when onRequest throws", async () => { - const onError = vi - .fn() - .mockResolvedValue({ answer: "fallback", wasFreeform: true }); - const policy = delegatingUserInputPolicy({ - onRequest: () => { - throw new Error("host boom"); - }, - onError, - }); - const result = await policy(makeCtx()); - expect(result).toEqual({ answer: "fallback", wasFreeform: true }); - expect(onError).toHaveBeenCalledTimes(1); - }); - - it("falls through to error-message response when onError also throws", async () => { - const policy = delegatingUserInputPolicy({ - onRequest: () => { - throw new Error("host boom"); - }, - onError: () => { - throw new Error("fallback boom"); - }, - }); - const result = await policy(makeCtx()); - expect(result).toBeDefined(); - expect(result!.answer).toContain("host boom"); - }); - - it("formats non-Error throws via JSON.stringify", async () => { - const policy = delegatingUserInputPolicy({ - onRequest: () => { - throw { code: 7, msg: "weird" } as unknown as Error; - }, - }); - const result = await policy(makeCtx()); - expect(result).toBeDefined(); - expect(result!.answer).toContain('"code":7'); - }); -}); - -describe("composeUserInputPolicies", () => { - it("returns the first non-undefined result and skips subsequent policies", async () => { - const a: CopilotUserInputPolicy = () => undefined; - const b: CopilotUserInputPolicy = () => ({ answer: "from-b", wasFreeform: true }); - const c = vi.fn(() => ({ answer: "from-c", wasFreeform: true })); - const policy = composeUserInputPolicies(a, b, c); - const result = await policy(makeCtx()); - expect(result).toEqual({ answer: "from-b", wasFreeform: true }); - expect(c).not.toHaveBeenCalled(); - }); - - it("falls through to DENY_ALL_ANSWER when all policies return undefined", async () => { - const policy = composeUserInputPolicies( - () => undefined, - () => undefined, - ); - const result = await policy(makeCtx()); - expect(result).toEqual({ answer: DENY_ALL_ANSWER, wasFreeform: true }); - }); - - it("short-circuits to error-message response when any policy throws", async () => { - const later = vi.fn(() => ({ answer: "later", wasFreeform: true })); - const policy = composeUserInputPolicies(() => { - throw new Error("compose boom"); - }, later); - const result = await policy(makeCtx()); - expect(result).toBeDefined(); - expect(result!.answer).toContain("compose boom"); - expect(later).not.toHaveBeenCalled(); - }); -}); - -describe("createUserInputBridge", () => { - it("adapts a policy to the SDK UserInputHandler shape", async () => { - const handler = createUserInputBridge(staticAnswerPolicy({ answer: "Alice" })); - const result = await handler(makeRequest(), { sessionId: "sess-1" }); - expect(result).toEqual({ answer: "Alice", wasFreeform: true }); - }); - - it("defaults to denyAllUserInputPolicy when no policy is passed", async () => { - const handler = createUserInputBridge(); - const result = await handler(makeRequest(), { sessionId: "sess-1" }); - expect(result).toEqual({ answer: DENY_ALL_ANSWER, wasFreeform: true }); - }); - - it("forwards the SDK sessionId into the policy context", async () => { - const policy = vi.fn(() => ({ answer: "x", wasFreeform: true })); - const handler = createUserInputBridge(policy); - await handler(makeRequest({ question: "q?", choices: ["a"] }), { sessionId: "sess-xyz" }); - expect(policy).toHaveBeenCalledTimes(1); - expect(policy.mock.calls[0]?.[0]).toEqual({ - sessionId: "sess-xyz", - request: { question: "q?", choices: ["a"] }, - }); - }); - - it("never throws when policy throws; returns DENY_ALL_ANSWER with the error message", async () => { - const handler = createUserInputBridge(() => { - throw new Error("policy boom"); - }); - const result = await handler(makeRequest(), { sessionId: "sess-1" }); - expect(result.answer).toContain(DENY_ALL_ANSWER); - expect(result.answer).toContain("policy boom"); - expect(result.wasFreeform).toBe(true); - }); - - it("never returns undefined: a policy returning undefined yields fail-closed answer", async () => { - const handler = createUserInputBridge(() => undefined); - const result = await handler(makeRequest(), { sessionId: "sess-1" }); - expect(result).toEqual({ answer: DENY_ALL_ANSWER, wasFreeform: true }); - }); - - it("preserves wasFreeform=false from a policy that picked from choices", async () => { - const handler = createUserInputBridge(firstChoicePolicy); - const result = await handler(makeRequest({ choices: ["one", "two"], allowFreeform: false }), { - sessionId: "sess-1", - }); - expect(result).toEqual({ answer: "one", wasFreeform: false }); - }); -}); diff --git a/extensions/copilot/src/user-input-bridge.ts b/extensions/copilot/src/user-input-bridge.ts deleted file mode 100755 index 71e4b2a1d34..00000000000 --- a/extensions/copilot/src/user-input-bridge.ts +++ /dev/null @@ -1,244 +0,0 @@ -/** - * User-input bridge for the copilot agent runtime. - * - * STATUS — MVP DORMANT: This module is intentionally NOT registered with - * the SDK in the current harness (see `attempt.ts` / `side-question.ts`). - * The SDK contract is "When `onUserInputRequest` is provided, enables the - * `ask_user` tool allowing the agent to ask questions" (see - * `node_modules/@github/copilot-sdk/dist/types.d.ts` `SessionConfig`); - * by omitting the handler we hide `ask_user` from the model entirely. - * Agents under the MVP must make best-judgment decisions from the - * initial prompt rather than asking clarifying questions mid-turn. - * - * FOLLOW-UP: The scaffolding below stays in tree so the follow-up that - * ports the codex user-input-bridge pattern - * (`extensions/codex/src/app-server/user-input-bridge.ts`) has a stable - * surface to wire — that change will route SDK `UserInputRequest`s - * through `params.onBlockReply` / `onPartialReply` and resolve the - * pending promise from the next inbound channel message, then register - * `createUserInputBridge(delegatingUserInputPolicy(...))` from - * `createSessionConfig`. - * - * BACK-POINTER: The host-side channel/TUI prompt flow lives outside - * this package boundary in `commitments/` and the channel plugins - * (slack/discord/cli/tui). Per proposal §50, this bridge does NOT - * import that flow directly (the package boundary - * `tsconfig.package-boundary.base.json` only allows - * `openclaw/plugin-sdk/*` and `@github/copilot-sdk`). Instead, this - * module: - * - * 1. Defines a small `CopilotUserInputPolicy` contract that the - * core wiring layer implements to forward `UserInputRequest`s to - * the host's channel/TUI prompt path. - * 2. Provides built-in policies for common defaults (deny-all with a - * synthetic answer, auto-first-choice, static-answer). - * 3. Provides a `delegatingUserInputPolicy({ onRequest })` so the - * core wiring layer can plug in a host-side callback that calls - * into `commitments/` and returns the SDK-shaped response. - * 4. Adapts the resulting policy into the SDK's `UserInputHandler` - * shape via `createUserInputBridge(policy)`. - * - * SDK contract note: unlike `PermissionHandler` (which has a - * `no-result` escape hatch), `UserInputHandler` MUST resolve with a - * `UserInputResponse`. The bridge therefore never returns `undefined` - * to the SDK; if a policy returns `undefined` or throws, the default - * fail-closed answer is used so the model sees a real string rather - * than a generic RPC failure. - * - * If the host's prompt contract changes materially, the contract here - * must be revisited in lockstep. The unit tests in - * `user-input-bridge.test.ts` exercise the SDK-shaped response envelope - * so any silent drift in the SDK type is caught at typecheck. - */ - -import type { SessionConfig } from "@github/copilot-sdk"; - -type UserInputHandler = NonNullable; -type SdkUserInputRequest = Parameters[0]; -type SdkUserInputResponse = Awaited>; - -/** Request shape forwarded to host-implemented user-input policies. */ -export interface CopilotUserInputContext { - /** SDK session id that originated the request. */ - sessionId: string; - /** Original SDK request payload. */ - request: SdkUserInputRequest; -} - -/** - * Policy contract. Implementors return an SDK-shaped response (or a - * Promise of one). - * - * Returning `undefined` is treated as "no opinion" and falls through - * to the default fail-closed response (`DENY_ALL_ANSWER`). This keeps - * composition trivial without requiring explicit responses from every - * code path. - */ -export type CopilotUserInputPolicy = ( - ctx: CopilotUserInputContext, -) => SdkUserInputResponse | undefined | Promise; - -/** - * Default answer used when no host policy provides one. The string is - * intentionally explicit so the model can detect the missing-prompt - * condition rather than treating it as a real user answer. - */ -export const DENY_ALL_ANSWER = - "[copilot agent runtime: no user-input policy installed; request declined]"; - -export const denyAllUserInputPolicy: CopilotUserInputPolicy = () => ({ - answer: DENY_ALL_ANSWER, - wasFreeform: true, -}); - -/** - * Auto-pick the first choice if the request offers choices; otherwise - * fall back to `DENY_ALL_ANSWER` as a freeform answer. Useful for - * non-interactive test runs. - */ -export const firstChoicePolicy: CopilotUserInputPolicy = ({ request }) => { - if (request.choices && request.choices.length > 0) { - return { answer: request.choices[0], wasFreeform: false }; - } - return { answer: DENY_ALL_ANSWER, wasFreeform: true }; -}; - -export interface StaticAnswerPolicyOptions { - /** Answer returned for every request. */ - answer: string; - /** - * Whether the answer should be flagged as a freeform response. - * Defaults to `true` (caller did not pick from `choices`). - */ - wasFreeform?: boolean; -} - -/** Always return a fixed answer. Useful for deterministic tests. */ -export function staticAnswerPolicy(options: StaticAnswerPolicyOptions): CopilotUserInputPolicy { - const wasFreeform = options.wasFreeform ?? true; - return () => ({ answer: options.answer, wasFreeform }); -} - -export interface DelegatingUserInputPolicyOptions { - /** - * Host-supplied callback. Returning `undefined` falls through to the - * fail-closed default. Throwing falls back to the configured - * `onError` policy if provided; otherwise the throw is converted to - * a `DENY_ALL_ANSWER` response so the SDK never sees an exception - * (which would surface as a generic RPC failure to the model). - */ - onRequest: CopilotUserInputPolicy; - /** - * Optional fallback when `onRequest` throws. If omitted, throws are - * converted to a `DENY_ALL_ANSWER` response with the error message - * appended. If supplied and `onError` also throws, fall through to - * the error-message response. - */ - onError?: CopilotUserInputPolicy; -} - -/** - * Wrap a host callback into a policy, catching synchronous throws and - * async rejections so the SDK never sees an exception. - */ -export function delegatingUserInputPolicy( - options: DelegatingUserInputPolicyOptions, -): CopilotUserInputPolicy { - const { onRequest, onError } = options; - return async (ctx) => { - try { - const result = await onRequest(ctx); - if (result !== undefined) { - return result; - } - return { answer: DENY_ALL_ANSWER, wasFreeform: true }; - } catch (error) { - if (onError) { - try { - const fallback = await onError(ctx); - if (fallback !== undefined) { - return fallback; - } - } catch { - // fall through to error-message response - } - } - return { - answer: `${DENY_ALL_ANSWER} (host policy threw: ${formatError(error)})`, - wasFreeform: true, - }; - } - }; -} - -/** - * Compose policies in order. The first policy to return a non-undefined - * result wins. If all return undefined, a fail-closed `DENY_ALL_ANSWER` - * response is produced. Throws inside any policy short-circuit to the - * error-message response; downstream policies are not consulted after a - * throw. - */ -export function composeUserInputPolicies( - ...policies: CopilotUserInputPolicy[] -): CopilotUserInputPolicy { - return async (ctx) => { - for (const policy of policies) { - try { - const result = await policy(ctx); - if (result !== undefined) { - return result; - } - } catch (error) { - return { - answer: `${DENY_ALL_ANSWER} (host policy threw: ${formatError(error)})`, - wasFreeform: true, - }; - } - } - return { answer: DENY_ALL_ANSWER, wasFreeform: true }; - }; -} - -/** - * Adapt a `CopilotUserInputPolicy` to the SDK's `UserInputHandler` - * shape. The returned handler always resolves with a valid - * `UserInputResponse` (never throws, never returns undefined), - * defaulting to `DENY_ALL_ANSWER` when the policy returns undefined or - * throws. - */ -export function createUserInputBridge( - policy: CopilotUserInputPolicy = denyAllUserInputPolicy, -): UserInputHandler { - return async ( - request: SdkUserInputRequest, - invocation: { sessionId: string }, - ): Promise => { - const ctx: CopilotUserInputContext = { - request, - sessionId: invocation.sessionId, - }; - try { - const result = await policy(ctx); - if (result !== undefined) { - return result; - } - } catch (error) { - return { - answer: `${DENY_ALL_ANSWER} (host policy threw: ${formatError(error)})`, - wasFreeform: true, - }; - } - return { answer: DENY_ALL_ANSWER, wasFreeform: true }; - }; -} - -function formatError(error: unknown): string { - if (error instanceof Error) { - return error.message; - } - try { - return JSON.stringify(error); - } catch { - return String(error); - } -} diff --git a/scripts/deadcode-unused-files.allowlist.mjs b/scripts/deadcode-unused-files.allowlist.mjs index 5d5ceae0cbc..b9597ab9b4a 100644 --- a/scripts/deadcode-unused-files.allowlist.mjs +++ b/scripts/deadcode-unused-files.allowlist.mjs @@ -14,8 +14,6 @@ export const KNIP_OPTIONAL_UNUSED_FILE_ALLOWLIST = [ "extensions/canvas/src/host/a2ui-app/bootstrap.js", "extensions/canvas/src/host/a2ui-app/rolldown.config.mjs", "extensions/copilot/src/doctor-probes.ts", - "extensions/copilot/src/telemetry-bridge.ts", - "extensions/copilot/src/user-input-bridge.ts", "extensions/diffs/src/viewer-client.ts", "extensions/diffs/src/viewer-payload.ts", "extensions/matrix/src/plugin-entry.runtime.js",