diff --git a/packages/core/src/services/copilot.ts b/packages/core/src/services/copilot.ts index 939901c..132c8cb 100644 --- a/packages/core/src/services/copilot.ts +++ b/packages/core/src/services/copilot.ts @@ -14,6 +14,32 @@ export function logCopilotDebug(message: string): void { process.stderr.write(`[agentrc:copilot] ${message}\n`); } +/** + * Parse a positive-integer environment variable. Accepts only a base-10 + * integer literal (after trimming surrounding whitespace) so surprising + * inputs such as `"1.5"`, `"1e3"`, or `"0x10"` are rejected rather than + * silently coerced. Returns the parsed value when it is a safe integer > 0; + * otherwise returns `undefined` so callers can apply their own default. + * + * An unset, empty, or whitespace-only value is treated as "not configured" + * and returns `undefined` silently. A value that is present and non-blank + * but not a valid positive integer is rejected and logged via + * `logCopilotDebug`, so users running with `AGENTRC_DEBUG_COPILOT=1` can see + * when their override was ignored. + */ +export function parsePositiveIntEnv(name: string): number | undefined { + const raw = process.env[name]; + if (raw === undefined) return undefined; + const trimmed = raw.trim(); + if (trimmed === "") return undefined; + if (/^\d+$/u.test(trimmed)) { + const parsed = Number(trimmed); + if (Number.isSafeInteger(parsed) && parsed > 0) return parsed; + } + logCopilotDebug(`ignoring invalid ${name}=${JSON.stringify(raw)}`); + return undefined; +} + export type CopilotCliConfig = { cliPath: string; cliArgs?: string[]; @@ -28,6 +54,25 @@ let cachedCliConfig: CopilotCliConfig | null = null; let cachedCliConfigTimestamp = 0; const CLI_CACHE_TTL_MS = 5 * 60 * 1000; // 5 minutes +/** + * Timeout for the `--headless --version` readiness probe. npx may need to + * download `@github/copilot` on first run, so it gets a longer budget; the + * `.bat`-shim / Node cold start on Windows still routinely exceeds 5 s, so + * the non-npx path uses 20 s. Overridable via `AGENTRC_COPILOT_PROBE_TIMEOUT_MS`. + * + * @internal Exported only for unit testing of the selection logic; not part of + * the public API and intentionally excluded from the `@agentrc/core` barrel. + */ +export function getHeadlessProbeTimeoutMs(config: CopilotCliConfig): number { + const override = parsePositiveIntEnv("AGENTRC_COPILOT_PROBE_TIMEOUT_MS"); + if (override !== undefined) return override; + const isNpx = + config.cliArgs?.some( + (arg) => arg === "@github/copilot" || arg.startsWith("@github/copilot@") + ) ?? false; + return isNpx ? 30000 : 20000; +} + function cacheConfig(config: CopilotCliConfig): CopilotCliConfig { cachedCliConfig = config; cachedCliConfigTimestamp = Date.now(); @@ -40,8 +85,7 @@ export async function assertCopilotCliReady(): Promise { logCopilotDebug(`validating CLI compatibility with ${desc}`); try { - const isNpx = config.cliArgs?.includes("@github/copilot") ?? false; - const timeout = isNpx ? 30000 : 5000; + const timeout = getHeadlessProbeTimeoutMs(config); const [cmd, args] = buildExecArgs(config, ["--headless", "--version"]); await execFileAsync(cmd, args, { timeout }); } catch { @@ -285,9 +329,7 @@ async function findFirstCompatibleCandidate( } async function isHeadlessCompatible(config: CopilotCliConfig): Promise { - // npx may need to download the package on first run, so allow a longer timeout - const isNpx = config.cliArgs?.includes("@github/copilot") ?? false; - const timeout = isNpx ? 30000 : 5000; + const timeout = getHeadlessProbeTimeoutMs(config); try { const [cmd, args] = buildExecArgs(config, ["--headless", "--version"]); await execFileAsync(cmd, args, { timeout }); diff --git a/packages/core/src/services/copilotSdk.ts b/packages/core/src/services/copilotSdk.ts index 20711fd..f1502c3 100644 --- a/packages/core/src/services/copilotSdk.ts +++ b/packages/core/src/services/copilotSdk.ts @@ -6,6 +6,36 @@ import { buildExecArgs, logCopilotDebug, type CopilotCliConfig } from "./copilot export type CopilotSdkModule = typeof CopilotSdk; +/** + * Permission response kinds the bundled Copilot CLI 1.0.x accepts. + * Mirrors the SDK 0.3.0 vocabulary (https://github.com/github/copilot-sdk/releases/tag/v0.3.0). + * + * Note: this is the *response* kind sent back from a permission handler, + * not the *request* kind (`"read"`, `"shell"`, ...) the SDK passes in. + */ +export type WirePermissionResponseKind = + | "approve-once" + | "approve-for-session" + | "approve-for-location" + | "reject" + | "user-not-available"; + +/** + * Build the wire-shape permission response the Copilot CLI expects. + * + * The bundled CLI 1.0.x speaks the SDK 0.3.0+ vocabulary, but agentrc pins + * `@github/copilot-sdk` to ^0.2.0 whose type declarations still describe the + * older `"approved"` / `"denied-..."` kinds. This helper isolates the + * necessary unchecked cast in one place so the rest of the codebase can stay + * strict; remove it and use `{ kind } as const` once the SDK dependency is + * bumped. + */ +export function wirePermissionResponse( + kind: WirePermissionResponseKind +): ReturnType { + return { kind } as unknown as ReturnType; +} + let cachedSdkModule: Promise | null = null; function normalizeSdkLoadError(error: unknown): Error { @@ -156,7 +186,7 @@ export type PatchedCopilotClient = Omit< export function attachDefaultPermissionHandler( client: InstanceType ): void { - const approveAll: CopilotSdk.PermissionHandler = () => ({ kind: "approved" as const }); + const approveAll: CopilotSdk.PermissionHandler = () => wirePermissionResponse("approve-once"); const originalCreateSession = client.createSession.bind(client); // Override createSession so onPermissionRequest is optional at call sites. // The cast targets our PatchedCopilotClient createSession signature which diff --git a/packages/core/src/services/instructions.ts b/packages/core/src/services/instructions.ts index f9f107a..bdc4cdd 100644 --- a/packages/core/src/services/instructions.ts +++ b/packages/core/src/services/instructions.ts @@ -8,8 +8,8 @@ import { ensureDir, safeWriteFile } from "../utils/fs"; import type { Area, InstructionStrategy } from "./analyzer"; import { sanitizeAreaName } from "./analyzer"; -import { assertCopilotCliReady } from "./copilot"; -import { createCopilotClient, loadCopilotSdk } from "./copilotSdk"; +import { assertCopilotCliReady, parsePositiveIntEnv } from "./copilot"; +import { createCopilotClient, loadCopilotSdk, wirePermissionResponse } from "./copilotSdk"; import type { FileAction } from "./generator"; import { getSkillDirectory } from "./skills"; @@ -224,14 +224,27 @@ const INSTRUCTION_GENERATION_EXCLUDED_TOOLS = [ "str_replace_editor" ]; +/** + * Upper bound for a single `session.sendAndWait` call during instruction + * generation. Generation drives an agent loop with many tool calls and on + * slower endpoints or larger repos the total run can exceed the SDK's default + * 3-minute budget. This is a ceiling, not an idle wait — fast runs complete + * well under a minute. Overridable via `AGENTRC_INSTRUCTION_TIMEOUT_MS` for + * environments with slow networks or very large repositories. + * + * Implemented as a function (rather than a module-load constant) so test code + * can stub the env var via `vi.stubEnv` after the module has been imported. + */ +function getInstructionGenerationTimeoutMs(): number { + return parsePositiveIntEnv("AGENTRC_INSTRUCTION_TIMEOUT_MS") ?? 600_000; +} + const READ_ONLY_PERMISSION_HANDLER: PermissionHandler = (request) => { if (request.kind === "read" || request.kind === "custom-tool") { - return { kind: "approved" }; + return wirePermissionResponse("approve-once"); } - return { - kind: "denied-no-approval-rule-and-could-not-request-from-user" - }; + return wirePermissionResponse("user-not-available"); }; function getSessionError(errorMsg: string): Error { @@ -338,6 +351,8 @@ export async function generateCopilotInstructions( skillDirectories: [rootSkillDir] }); + const timeoutMs = getInstructionGenerationTimeoutMs(); + await trySetAutopilot(session); let content = ""; @@ -368,7 +383,7 @@ ${existingSection}`; progress("Analyzing codebase..."); let sendError: unknown; try { - await session.sendAndWait({ prompt }, 180000); + await session.sendAndWait({ prompt }, timeoutMs); } catch (err) { sendError = err; } finally { @@ -439,6 +454,8 @@ export async function generateAreaInstructions( skillDirectories: [areaSkillDir] }); + const timeoutMs = getInstructionGenerationTimeoutMs(); + await trySetAutopilot(session); let content = ""; @@ -471,7 +488,7 @@ ${existingSection ? `\nDo NOT duplicate content already covered by existing inst progress(`Analyzing area "${area.name}"...`); let sendError: unknown; try { - await session.sendAndWait({ prompt }, 180000); + await session.sendAndWait({ prompt }, timeoutMs); } catch (err) { sendError = err; } finally { @@ -699,6 +716,7 @@ async function generateNestedHub( area?: Area; childAreas?: Area[]; model?: string; + timeoutMs: number; onProgress?: (message: string) => void; } ): Promise { @@ -772,7 +790,7 @@ ${existingSection ? `\nDo NOT duplicate content from existing instruction files\ let sendError: unknown; try { - await session.sendAndWait({ prompt }, 180000); + await session.sendAndWait({ prompt }, options.timeoutMs); } catch (err) { sendError = err; } finally { @@ -799,6 +817,7 @@ async function generateNestedDetail( topic: NestedTopic; area?: Area; model?: string; + timeoutMs: number; onProgress?: (message: string) => void; } ): Promise { @@ -856,7 +875,7 @@ Description: ${options.topic.description}`; let sendError: unknown; try { - await session.sendAndWait({ prompt }, 180000); + await session.sendAndWait({ prompt }, options.timeoutMs); } catch (err) { sendError = err; } finally { @@ -895,6 +914,8 @@ export async function generateNestedInstructions( progress("Starting Copilot SDK..."); const client = await createCopilotClient(cliConfig); + const timeoutMs = getInstructionGenerationTimeoutMs(); + try { // Step 1: Generate hub const { hubContent, topics } = await generateNestedHub(client, { @@ -903,6 +924,7 @@ export async function generateNestedInstructions( area: options.area, childAreas: options.childAreas, model: options.model, + timeoutMs, onProgress: options.onProgress }); @@ -931,6 +953,7 @@ export async function generateNestedInstructions( topic, area: options.area, model: options.model, + timeoutMs, onProgress: options.onProgress }); if (detailContent) { diff --git a/src/services/__tests__/copilot.test.ts b/src/services/__tests__/copilot.test.ts index 16f1a3c..4749ecc 100644 --- a/src/services/__tests__/copilot.test.ts +++ b/src/services/__tests__/copilot.test.ts @@ -1,5 +1,9 @@ -import { extractModelChoices } from "@agentrc/core/services/copilot"; -import { describe, expect, it } from "vitest"; +import { + extractModelChoices, + getHeadlessProbeTimeoutMs, + parsePositiveIntEnv +} from "@agentrc/core/services/copilot"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; describe("extractModelChoices", () => { it("extracts model names from a single-line --help output", () => { @@ -35,3 +39,134 @@ describe("extractModelChoices", () => { expect(extractModelChoices(stderr)).toEqual(["gpt-5", "gpt-4.1", "claude-sonnet-4.5"]); }); }); + +describe("parsePositiveIntEnv", () => { + const NAME = "AGENTRC_TEST_TIMEOUT_MS"; + + beforeEach(() => { + vi.stubEnv(NAME, undefined); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("returns undefined when the variable is unset", () => { + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + }); + + it("parses a plain positive integer", () => { + vi.stubEnv(NAME, "5000"); + expect(parsePositiveIntEnv(NAME)).toBe(5000); + }); + + it("trims surrounding whitespace", () => { + vi.stubEnv(NAME, " 42 "); + expect(parsePositiveIntEnv(NAME)).toBe(42); + }); + + it("accepts leading zeros as base-10 (no octal)", () => { + vi.stubEnv(NAME, "007"); + expect(parsePositiveIntEnv(NAME)).toBe(7); + }); + + it("rejects non-integer decimals like 1.5", () => { + vi.stubEnv(NAME, "1.5"); + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + }); + + it("rejects scientific notation like 1e3", () => { + vi.stubEnv(NAME, "1e3"); + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + }); + + it("rejects hex literals like 0x10", () => { + vi.stubEnv(NAME, "0x10"); + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + }); + + it("rejects signed values", () => { + vi.stubEnv(NAME, "+5"); + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + }); + + it("rejects zero and negatives", () => { + vi.stubEnv(NAME, "0"); + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + vi.stubEnv(NAME, "-5"); + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + }); + + it("rejects empty and whitespace-only values", () => { + vi.stubEnv(NAME, ""); + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + vi.stubEnv(NAME, " "); + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + }); + + it("rejects non-numeric junk", () => { + vi.stubEnv(NAME, "5px"); + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + }); + + it("rejects values beyond the safe-integer range", () => { + vi.stubEnv(NAME, "99999999999999999999"); + expect(parsePositiveIntEnv(NAME)).toBeUndefined(); + }); +}); + +describe("getHeadlessProbeTimeoutMs", () => { + const ENV = "AGENTRC_COPILOT_PROBE_TIMEOUT_MS"; + + beforeEach(() => { + vi.stubEnv(ENV, undefined); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("uses 30s for the npx candidate", () => { + expect( + getHeadlessProbeTimeoutMs({ cliPath: "npx", cliArgs: ["--yes", "@github/copilot"] }) + ).toBe(30000); + }); + + it("uses 30s for a versioned npx spec (e.g. @github/copilot@latest)", () => { + expect( + getHeadlessProbeTimeoutMs({ cliPath: "npx", cliArgs: ["--yes", "@github/copilot@latest"] }) + ).toBe(30000); + expect( + getHeadlessProbeTimeoutMs({ cliPath: "npx", cliArgs: ["-y", "@github/copilot@0.3.0"] }) + ).toBe(30000); + }); + + it("uses 20s for a bare-path (non-npx) candidate", () => { + expect(getHeadlessProbeTimeoutMs({ cliPath: "/usr/bin/copilot" })).toBe(20000); + }); + + it("uses 20s when cliArgs do not include @github/copilot", () => { + expect( + getHeadlessProbeTimeoutMs({ cliPath: process.execPath, cliArgs: ["/path/to/npm-loader.js"] }) + ).toBe(20000); + }); + + it("does not treat a substring match (e.g. @github/copilot-foo) as npx", () => { + expect( + getHeadlessProbeTimeoutMs({ cliPath: "npx", cliArgs: ["--yes", "@github/copilot-foo"] }) + ).toBe(20000); + }); + + it("honors a valid override for both npx and non-npx candidates", () => { + vi.stubEnv(ENV, "12345"); + expect(getHeadlessProbeTimeoutMs({ cliPath: "/usr/bin/copilot" })).toBe(12345); + expect( + getHeadlessProbeTimeoutMs({ cliPath: "npx", cliArgs: ["--yes", "@github/copilot"] }) + ).toBe(12345); + }); + + it("ignores an invalid override and falls back to the default", () => { + vi.stubEnv(ENV, "not-a-number"); + expect(getHeadlessProbeTimeoutMs({ cliPath: "/usr/bin/copilot" })).toBe(20000); + }); +}); diff --git a/src/services/__tests__/copilotSdk.test.ts b/src/services/__tests__/copilotSdk.test.ts index b83efcf..f9046ae 100644 --- a/src/services/__tests__/copilotSdk.test.ts +++ b/src/services/__tests__/copilotSdk.test.ts @@ -36,13 +36,14 @@ describe("attachDefaultPermissionHandler", () => { expect(passedConfig.config).toHaveProperty("onPermissionRequest"); expect(typeof passedConfig.config.onPermissionRequest).toBe("function"); - // The injected handler should approve all request kinds + // The injected handler should approve all request kinds. + // SDK 0.3.0+ / CLI 1.0.x renamed "approved" -> "approve-once". const handler = passedConfig.config.onPermissionRequest as (req: unknown) => { kind: string }; - expect(handler({ kind: "shell" })).toEqual({ kind: "approved" }); - expect(handler({ kind: "write" })).toEqual({ kind: "approved" }); - expect(handler({ kind: "read" })).toEqual({ kind: "approved" }); - expect(handler({ kind: "url" })).toEqual({ kind: "approved" }); - expect(handler({ kind: "mcp" })).toEqual({ kind: "approved" }); + expect(handler({ kind: "shell" })).toEqual({ kind: "approve-once" }); + expect(handler({ kind: "write" })).toEqual({ kind: "approve-once" }); + expect(handler({ kind: "read" })).toEqual({ kind: "approve-once" }); + expect(handler({ kind: "url" })).toEqual({ kind: "approve-once" }); + expect(handler({ kind: "mcp" })).toEqual({ kind: "approve-once" }); }); it("preserves a caller-supplied onPermissionRequest", async () => { diff --git a/src/services/__tests__/instructions.test.ts b/src/services/__tests__/instructions.test.ts index cda3623..45d1bcb 100644 --- a/src/services/__tests__/instructions.test.ts +++ b/src/services/__tests__/instructions.test.ts @@ -816,17 +816,20 @@ describe("instruction generation sessions", () => { const expectReadOnlyPermissions = async ( onPermissionRequest: (request: { kind: string }) => Promise<{ kind: string }> | { kind: string } ) => { + // SDK 0.3.0+ / CLI 1.0.x renamed the permission response vocabulary: + // "approved" -> "approve-once" + // "denied-no-approval-rule-and-could-not-request-from-user" -> "user-not-available" await expect(Promise.resolve(onPermissionRequest({ kind: "read" }))).resolves.toEqual({ - kind: "approved" + kind: "approve-once" }); await expect(Promise.resolve(onPermissionRequest({ kind: "custom-tool" }))).resolves.toEqual({ - kind: "approved" + kind: "approve-once" }); await expect(Promise.resolve(onPermissionRequest({ kind: "shell" }))).resolves.toEqual({ - kind: "denied-no-approval-rule-and-could-not-request-from-user" + kind: "user-not-available" }); await expect(Promise.resolve(onPermissionRequest({ kind: "write" }))).resolves.toEqual({ - kind: "denied-no-approval-rule-and-could-not-request-from-user" + kind: "user-not-available" }); };