diff --git a/CHANGELOG.md b/CHANGELOG.md index d3cb057..ba56747 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## 0.8.1 - Unreleased +- Prevented oversized provider, validation, and PR-publishing timeout overrides from overflowing into one-millisecond deadlines. + - Preserved observed source edits in failed patch attempts when a provider writes files before exiting with an error. - Fixed nested-project repairs to ignore their own state and sibling changes, fingerprint project-relative source paths, and record both sides of renames. diff --git a/docs/configuration.md b/docs/configuration.md index 9314f99..3b1e9f3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -84,8 +84,10 @@ Environment overrides: - `CLAWPATCH_GH_PR_CREATE_TIMEOUT_MS` (default `300000`, or 5 minutes) - `CLAWPATCH_TASKKILL_TIMEOUT_MS` (Windows cleanup deadline; default `5000`, or 5 seconds) -The `open-pr` timeout overrides must be positive millisecond values. Invalid values fall back to -their defaults. +Provider, validation, and `open-pr` timeout overrides accept `1` through +`2147483647` milliseconds. Invalid or out-of-range values fall back to their +defaults; fractional values are truncated. This prevents Node from turning an +overflowing deadline into a one-millisecond timer. `CLAWPATCH_TASKKILL_TIMEOUT_MS` must be between `1` and `2147483647` milliseconds; invalid values fall back to 5 seconds. Fractional values are truncated. Each Windows diff --git a/docs/providers.md b/docs/providers.md index 5fcdbf6..f64d1bc 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -26,6 +26,10 @@ Provider names today: - `mock`: deterministic provider for tests and fixtures - `mock-fail`: failure provider for tests +Timeout overrides must be between `1` and `2147483647` milliseconds. Invalid +values use the default and fractional values are truncated. A provider-specific +timeout takes precedence over `CLAWPATCH_PROVIDER_TIMEOUT_MS`. + ## Codex Codex invocation: diff --git a/src/exec.ts b/src/exec.ts index 8ed2fb6..ea072c6 100644 --- a/src/exec.ts +++ b/src/exec.ts @@ -1,3 +1,4 @@ +import { parseTimeoutMs } from "./timeout.js"; import { spawn } from "node:child_process"; import { existsSync } from "node:fs"; import { delimiter, extname, join } from "node:path"; @@ -19,12 +20,7 @@ const abortHandlers = new Map void>(); const defaultTaskkillTimeoutMs = 5_000; export function taskkillTimeoutMs(): number { - const configured = Number( - process.env["CLAWPATCH_TASKKILL_TIMEOUT_MS"] ?? String(defaultTaskkillTimeoutMs), - ); - return Number.isFinite(configured) && configured >= 1 && configured <= 2_147_483_647 - ? Math.trunc(configured) - : defaultTaskkillTimeoutMs; + return parseTimeoutMs(process.env["CLAWPATCH_TASKKILL_TIMEOUT_MS"], defaultTaskkillTimeoutMs); } export async function runCommand( diff --git a/src/fix.ts b/src/fix.ts index 340ff9a..2086564 100644 --- a/src/fix.ts +++ b/src/fix.ts @@ -1,3 +1,4 @@ +import { parseTimeoutMs } from "./timeout.js"; import { loadProjectState, type AppContext } from "./app-context.js"; import { changedPathsBetweenSnapshots, @@ -116,7 +117,7 @@ export async function fixCommand( for (const command of validationCommands) { commandsRun.push( await runCommand(command, loaded.root, undefined, { - timeoutMs: validationTimeoutMs(), + timeoutMs: parseTimeoutMs(process.env["CLAWPATCH_VALIDATION_TIMEOUT_MS"], 600_000), maxOutputChars: 100_000, }), ); @@ -171,8 +172,3 @@ export async function fixCommand( : `clawpatch revalidate --finding ${finding.findingId}`, }; } - -function validationTimeoutMs(): number { - const configured = Number(process.env["CLAWPATCH_VALIDATION_TIMEOUT_MS"] ?? "600000"); - return Number.isFinite(configured) && configured > 0 ? configured : 600_000; -} diff --git a/src/mappers/go.ts b/src/mappers/go.ts index 28c0f8e..e9806e2 100644 --- a/src/mappers/go.ts +++ b/src/mappers/go.ts @@ -1,3 +1,4 @@ +import { parseTimeoutMs } from "../timeout.js"; import { readdir, readFile, realpath } from "node:fs/promises"; import { isAbsolute, join, relative } from "node:path"; import { runCommandArgs } from "../exec.js"; @@ -8,12 +9,7 @@ import { FeatureSeed, MapperContext, SeedFileRef, SeedTestRef } from "./types.js const defaultGoListTimeoutMs = 120_000; export function goListTimeoutMs(): number { - const configured = Number( - process.env["CLAWPATCH_GO_LIST_TIMEOUT_MS"] ?? String(defaultGoListTimeoutMs), - ); - return Number.isFinite(configured) && configured >= 1 && configured <= 2_147_483_647 - ? Math.trunc(configured) - : defaultGoListTimeoutMs; + return parseTimeoutMs(process.env["CLAWPATCH_GO_LIST_TIMEOUT_MS"], defaultGoListTimeoutMs); } export async function goSeeds(root: string, context: MapperContext): Promise { diff --git a/src/open-pr.test.ts b/src/open-pr.test.ts index 3501f49..9db9ab9 100644 --- a/src/open-pr.test.ts +++ b/src/open-pr.test.ts @@ -48,6 +48,13 @@ describe("open-pr command timeouts", () => { expect(ghPrCreateTimeoutMs()).toBe(300_000); }); + it("rejects overflowing publishing deadlines before passing them to Node", () => { + process.env["CLAWPATCH_GIT_PUSH_TIMEOUT_MS"] = "2147483648"; + process.env["CLAWPATCH_GH_PR_CREATE_TIMEOUT_MS"] = "1e100"; + expect(gitPushTimeoutMs()).toBe(600_000); + expect(ghPrCreateTimeoutMs()).toBe(300_000); + }); + it( "times out a hung git push instead of blocking open-pr", { timeout: HANG_TEST_TIMEOUT_MS }, diff --git a/src/open-pr.ts b/src/open-pr.ts index 4474625..bd83260 100644 --- a/src/open-pr.ts +++ b/src/open-pr.ts @@ -1,3 +1,4 @@ +import { parseTimeoutMs } from "./timeout.js"; import { lstat, realpath } from "node:fs/promises"; import { relative, resolve } from "node:path"; import { loadProjectState, type AppContext } from "./app-context.js"; @@ -505,13 +506,11 @@ function githubCli(): string { } export function gitPushTimeoutMs(): number { - const configured = Number(process.env["CLAWPATCH_GIT_PUSH_TIMEOUT_MS"] ?? "600000"); - return Number.isFinite(configured) && configured > 0 ? configured : 600_000; + return parseTimeoutMs(process.env["CLAWPATCH_GIT_PUSH_TIMEOUT_MS"], 600_000); } export function ghPrCreateTimeoutMs(): number { - const configured = Number(process.env["CLAWPATCH_GH_PR_CREATE_TIMEOUT_MS"] ?? "300000"); - return Number.isFinite(configured) && configured > 0 ? configured : 300_000; + return parseTimeoutMs(process.env["CLAWPATCH_GH_PR_CREATE_TIMEOUT_MS"], 300_000); } async function localBranchExists(gitRoot: string, branch: string): Promise { diff --git a/src/provider-runtime.test.ts b/src/provider-runtime.test.ts index d938177..ccea237 100644 --- a/src/provider-runtime.test.ts +++ b/src/provider-runtime.test.ts @@ -24,4 +24,18 @@ describe("provider runtime policy", () => { expect(providerTimeoutMs("CLAWPATCH_TEST_TIMEOUT_MS", 300)).toBe(300); expect(providerCheckTimeoutMs()).toBe(10_000); }); + + it.each(["2147483648", "1e100", "0.5"])("rejects unsafe timer delay %s", (value) => { + process.env["CLAWPATCH_TEST_TIMEOUT_MS"] = value; + process.env["CLAWPATCH_PROVIDER_CHECK_TIMEOUT_MS"] = value; + expect(providerTimeoutMs("CLAWPATCH_TEST_TIMEOUT_MS", 300)).toBe(300); + expect(providerCheckTimeoutMs()).toBe(10_000); + }); + + it("normalizes valid fractional delays without overflowing the timer range", () => { + process.env["CLAWPATCH_TEST_TIMEOUT_MS"] = "1234.9"; + expect(providerTimeoutMs("CLAWPATCH_TEST_TIMEOUT_MS", 300)).toBe(1234); + process.env["CLAWPATCH_TEST_TIMEOUT_MS"] = "2147483647"; + expect(providerTimeoutMs("CLAWPATCH_TEST_TIMEOUT_MS", 300)).toBe(2147483647); + }); }); diff --git a/src/provider-runtime.ts b/src/provider-runtime.ts index 9414333..2c34044 100644 --- a/src/provider-runtime.ts +++ b/src/provider-runtime.ts @@ -1,10 +1,8 @@ +import { parseTimeoutMs } from "./timeout.js"; + export function providerTimeoutMs(envName: string, defaultMs: number): number { const raw = process.env[envName] ?? process.env["CLAWPATCH_PROVIDER_TIMEOUT_MS"]; - if (raw === undefined) { - return defaultMs; - } - const parsed = Number(raw); - return Number.isFinite(parsed) && parsed > 0 ? parsed : defaultMs; + return parseTimeoutMs(raw, defaultMs); } export function providerCheckTimeoutMs(): number { diff --git a/src/timeout.ts b/src/timeout.ts new file mode 100644 index 0000000..41b1365 --- /dev/null +++ b/src/timeout.ts @@ -0,0 +1,6 @@ +export function parseTimeoutMs(value: string | undefined, fallback: number): number { + const milliseconds = Number(value); + return Number.isFinite(milliseconds) && milliseconds >= 1 && milliseconds <= 2_147_483_647 + ? Math.trunc(milliseconds) + : fallback; +}