From 3fdab420cbe60ceadae2023c490f48ea9b72a801 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 15 Jun 2026 21:54:06 -0700 Subject: [PATCH 1/3] Fix audit findings: updater safety + config/CLI robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit B13: auto-updater no longer restarts on a failed update. checkForUpdate now returns a boolean (true only when the renameSync over process.execPath succeeds); tick() gates restartFn() on it and closes out the Slack "auto-updating" message on failure. Auto-update is also deferred when getQueueSize() > 0, mirroring cleanupStale. Updater integrity: reject truncated downloads (buffer.length !== Content-Length) and treat a missing checksum asset as a hard failure for auto-update (new requireChecksum opt, set by the auto-updater) so we never rename in an unverified binary. Manual `update` behavior is unchanged. B18: empty/comment-only config no longer throws a cryptic TypeError — parseYaml(raw) ?? {} in loadConfig, loadWorkDir, and loadCleanConfig (mirroring validate.ts). Dropped the redundant validateCritterType loop (parseCritterType already validates). B20: pr-status normalizes statusCheckRollup per-entry via conclusion ?? state, so legacy StatusContext nodes map to success/failure instead of perpetual pending. Added a 10s timeout to the gh pr view call (via runCommand timeoutMs, which also drains stderr) and process toFetch in sequential batches instead of dropping work beyond the cap (F6). B22: logs --follow interpolates a real ESC byte instead of literal "\x1b[36m". De-duped extractTimestamp/newestDir and the regex-escape helper by exporting them from log-resolver.ts. env.ts: loadEnvFallback strips a surrounding quote pair and supports a leading `export ` so quoted tokens no longer leak quotes into values. utils.ts: runCommand gains an optional timeoutMs (default unchanged). Tests: src/__tests__/audit-updater-misc.test.ts covers B18, B20, env quote stripping, and checkForUpdate returning false on truncated/checksum-failed/ missing-checksum downloads. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/__tests__/audit-updater-misc.test.ts | 255 +++++++++++++++++++++++ src/auto-updater.ts | 22 +- src/config.ts | 17 +- src/env.ts | 14 +- src/log-resolver.ts | 14 +- src/logs.ts | 17 +- src/pr-status.ts | 78 ++++--- src/updater.ts | 55 +++-- src/utils.ts | 28 ++- 9 files changed, 420 insertions(+), 80 deletions(-) create mode 100644 src/__tests__/audit-updater-misc.test.ts diff --git a/src/__tests__/audit-updater-misc.test.ts b/src/__tests__/audit-updater-misc.test.ts new file mode 100644 index 00000000..bccf2549 --- /dev/null +++ b/src/__tests__/audit-updater-misc.test.ts @@ -0,0 +1,255 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { mkdirSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { createTempDir } from "./helpers.js"; + +// node:os mock must be registered before importing env.ts (which reads homedir()). +let fakeHome = ""; +mock.module("node:os", () => ({ homedir: () => fakeHome })); + +const { loadWorkDir, loadCleanConfig } = await import("../config.js"); +const { normalizeCheckVerdict } = await import("../pr-status.js"); +const { loadEnvFallback } = await import("../env.js"); +const { checkForUpdate } = await import("../updater.js"); + +const RELEASES_URL = "https://api.github.com/repos/ack-ventures/critters/releases/latest"; +const BIN_URL = "https://github.com/ack-ventures/critters/releases/download/v2.0.0/bin"; +const CHECKSUM_URL = "https://github.com/ack-ventures/critters/releases/download/v2.0.0/checksums-sha256.txt"; + +const EXPECTED_ASSET = `critters-${process.platform}-${process.arch}`; + +// --------------------------------------------------------------------------- +// B18 — empty / comment-only config must not throw a cryptic TypeError +// --------------------------------------------------------------------------- +describe("B18: empty/comment-only config (config.ts null guard)", () => { + let tempDir: string; + let cleanup: () => void; + + beforeEach(() => { + const t = createTempDir(); + tempDir = t.path; + cleanup = t.cleanup; + }); + afterEach(() => cleanup()); + + function writeConfig(contents: string): string { + const p = join(tempDir, "critters.config.yaml"); + writeFileSync(p, contents); + return p; + } + + test("loadWorkDir returns the default for a comment-only file instead of throwing", () => { + const p = writeConfig("# only a comment\n"); + expect(() => loadWorkDir(p)).not.toThrow(); + // /tmp may resolve to /private/tmp on macOS; the point is the default workDir. + expect(loadWorkDir(p)).toMatch(/\/tmp\/critters-work$/); + }); + + test("loadWorkDir returns the default for an empty file", () => { + const p = writeConfig(""); + expect(loadWorkDir(p)).toMatch(/\/tmp\/critters-work$/); + }); + + test("loadCleanConfig returns defaults for a comment-only file instead of throwing", () => { + const p = writeConfig("# nothing here\n"); + expect(() => loadCleanConfig(p)).not.toThrow(); + const clean = loadCleanConfig(p); + expect(clean.workDir).toMatch(/\/tmp\/critters-work$/); + expect(clean.healthPort).toBe(3847); + expect(clean.tmuxSession).toBe("critters"); + }); +}); + +// --------------------------------------------------------------------------- +// B20 — legacy StatusContext nodes (state) must map to success/failure +// --------------------------------------------------------------------------- +describe("B20: normalizeCheckVerdict (pr-status.ts)", () => { + test("StatusContext SUCCESS (state, no conclusion) → success, not pending", () => { + expect(normalizeCheckVerdict({ state: "SUCCESS" })).toBe("success"); + }); + + test("StatusContext FAILURE/ERROR (state) → failure", () => { + expect(normalizeCheckVerdict({ state: "FAILURE" })).toBe("failure"); + expect(normalizeCheckVerdict({ state: "ERROR" })).toBe("failure"); + }); + + test("StatusContext PENDING (state) → pending", () => { + expect(normalizeCheckVerdict({ state: "PENDING" })).toBe("pending"); + }); + + test("CheckRun conclusion takes precedence and maps correctly", () => { + expect(normalizeCheckVerdict({ status: "COMPLETED", conclusion: "SUCCESS" })).toBe("success"); + expect(normalizeCheckVerdict({ status: "COMPLETED", conclusion: "FAILURE" })).toBe("failure"); + }); + + test("CheckRun in progress (no conclusion, not COMPLETED) → pending", () => { + expect(normalizeCheckVerdict({ status: "IN_PROGRESS" })).toBe("pending"); + }); +}); + +// --------------------------------------------------------------------------- +// env.ts — quote stripping and `export ` prefix in the fallback .env loader +// --------------------------------------------------------------------------- +describe("env.ts: loadEnvFallback quote stripping", () => { + let tempDir: string; + let cleanup: () => void; + let originalCwd: string; + const touched: string[] = []; + + beforeEach(() => { + const t = createTempDir(); + tempDir = t.path; + cleanup = t.cleanup; + fakeHome = join(tempDir, "home"); + mkdirSync(join(fakeHome, ".critters"), { recursive: true }); + const cwd = join(tempDir, "cwd"); + mkdirSync(cwd, { recursive: true }); + originalCwd = process.cwd(); + process.chdir(cwd); + }); + + afterEach(() => { + process.chdir(originalCwd); + for (const k of touched) delete process.env[k]; + touched.length = 0; + cleanup(); + }); + + function writeEnv(contents: string): void { + writeFileSync(join(fakeHome, ".critters", ".env"), contents); + } + + test("strips surrounding double quotes", () => { + writeEnv('QUOTED_DQ_TEST="lin_secret"\n'); + touched.push("QUOTED_DQ_TEST"); + loadEnvFallback(); + expect(process.env.QUOTED_DQ_TEST).toBe("lin_secret"); + }); + + test("strips surrounding single quotes", () => { + writeEnv("QUOTED_SQ_TEST='lin_secret'\n"); + touched.push("QUOTED_SQ_TEST"); + loadEnvFallback(); + expect(process.env.QUOTED_SQ_TEST).toBe("lin_secret"); + }); + + test("handles a leading `export ` prefix", () => { + writeEnv('export EXPORT_TEST="abc123"\n'); + touched.push("EXPORT_TEST"); + loadEnvFallback(); + expect(process.env.EXPORT_TEST).toBe("abc123"); + }); + + test("leaves unquoted values untouched", () => { + writeEnv("PLAIN_TEST=lin_plain\n"); + touched.push("PLAIN_TEST"); + loadEnvFallback(); + expect(process.env.PLAIN_TEST).toBe("lin_plain"); + }); + + test("does not strip mismatched/inner quotes", () => { + writeEnv("MISMATCH_TEST=\"unterminated\n"); + touched.push("MISMATCH_TEST"); + loadEnvFallback(); + expect(process.env.MISMATCH_TEST).toBe('"unterminated'); + }); +}); + +// --------------------------------------------------------------------------- +// B13 / updater integrity — checkForUpdate returns false on bad downloads +// --------------------------------------------------------------------------- +describe("checkForUpdate integrity (updater.ts)", () => { + let originalExecPath: string; + let originalFetch: typeof globalThis.fetch; + + beforeEach(() => { + originalExecPath = process.execPath; + originalFetch = globalThis.fetch; + // Pretend we are a compiled binary (basename !== bun) at a path that is never + // actually written to in the failure paths under test. + Object.defineProperty(process, "execPath", { value: "/tmp/critters-fake-binary", writable: true }); + }); + + afterEach(() => { + Object.defineProperty(process, "execPath", { value: originalExecPath, writable: true }); + globalThis.fetch = originalFetch; + }); + + function mockFetch(opts: { + includeChecksumAsset: boolean; + binaryBody: string; + contentLength: string; + checksumText?: string; + }): void { + const assets: Array<{ name: string; browser_download_url: string }> = [ + { name: EXPECTED_ASSET, browser_download_url: BIN_URL }, + ]; + if (opts.includeChecksumAsset) { + assets.push({ name: "checksums-sha256.txt", browser_download_url: CHECKSUM_URL }); + } + const releaseData = { tag_name: "v2.0.0", assets }; + + globalThis.fetch = (async (url: string | URL | Request) => { + const u = String(url); + if (u === RELEASES_URL) { + return new Response(JSON.stringify(releaseData), { status: 200 }); + } + if (u === BIN_URL) { + return new Response(opts.binaryBody, { + status: 200, + headers: { "Content-Length": opts.contentLength }, + }); + } + if (u === CHECKSUM_URL) { + return new Response(opts.checksumText ?? "", { status: 200 }); + } + throw new Error(`unexpected fetch: ${u}`); + }) as typeof globalThis.fetch; + } + + test("returns false on a truncated download (buffer.length !== Content-Length)", async () => { + const body = "x".repeat(100); + mockFetch({ + includeChecksumAsset: true, + binaryBody: body, + contentLength: "200", // advertises more than we received + checksumText: `0000 ${EXPECTED_ASSET}\n`, + }); + const result = await checkForUpdate("1.0.0"); + expect(result).toBe(false); + }); + + test("returns false on a SHA-256 checksum mismatch", async () => { + const body = "x".repeat(100); + mockFetch({ + includeChecksumAsset: true, + binaryBody: body, + contentLength: String(Buffer.byteLength(body)), + checksumText: `deadbeefdeadbeef ${EXPECTED_ASSET}\n`, + }); + const result = await checkForUpdate("1.0.0"); + expect(result).toBe(false); + }); + + test("auto-update returns false when the checksum asset is missing", async () => { + const body = "x".repeat(100); + mockFetch({ + includeChecksumAsset: false, + binaryBody: body, + contentLength: String(Buffer.byteLength(body)), + }); + // Auto-update path (requireChecksum) → missing checksum is a hard failure. + const result = await checkForUpdate("1.0.0", { requireChecksum: true }); + expect(result).toBe(false); + }); + + test("returns false (no-op) when already up to date", async () => { + mockFetch({ + includeChecksumAsset: true, + binaryBody: "x", + contentLength: "1", + }); + const result = await checkForUpdate("9.9.9"); + expect(result).toBe(false); + }); +}); diff --git a/src/auto-updater.ts b/src/auto-updater.ts index eda348c5..7f5842d5 100644 --- a/src/auto-updater.ts +++ b/src/auto-updater.ts @@ -52,8 +52,9 @@ export function startAutoUpdater( if (!result || !result.available) return; const activeCount = spawner.getActiveCount(); - if (activeCount > 0) { - log(`Auto-update: v${result.currentVersion} → v${result.latestVersion} available but deferred — ${activeCount} critter(s) active`); + const queueSize = spawner.getQueueSize(); + if (activeCount > 0 || queueSize > 0) { + log(`Auto-update: v${result.currentVersion} → v${result.latestVersion} available but deferred — ${activeCount} critter(s) active, ${queueSize} queued`); return; } @@ -65,8 +66,21 @@ export function startAutoUpdater( ); } - await checkForUpdate(version); - restartFn(); + // Only restart when the binary was actually replaced. checkForUpdate + // returns false on any failure (download/verify/rename), so a failed apply + // no longer triggers a futile restart loop onto an unchanged version. + const applied = await checkForUpdate(version, { requireChecksum: true }); + if (applied) { + restartFn(); + } else { + log(`Auto-update: apply failed — staying on v${result.currentVersion}, will retry next interval`); + if (slackNotifier.isConfigured) { + await slackNotifier.notify( + "__auto_update__", + `⚠️ Auto-update to v${result.latestVersion} failed — staying on v${result.currentVersion}. Will retry on the next check.`, + ); + } + } } catch (err) { log(`Auto-update: check failed — ${formatError(err)}`); } diff --git a/src/config.ts b/src/config.ts index 1f4fbf2b..5a906351 100644 --- a/src/config.ts +++ b/src/config.ts @@ -2,7 +2,7 @@ import { existsSync, mkdirSync, readFileSync, realpathSync } from "node:fs"; import { homedir } from "node:os"; import { parse as parseYaml } from "yaml"; import { assertValidCliAdapterName } from "./cli/adapter-names.js"; -import { type CritterTypeConfig, parseCritterTypes as parseCritterTypesFromYaml, synthesizeDefaultTypes, validateCritterType } from "./critter-type.js"; +import { type CritterTypeConfig, parseCritterTypes as parseCritterTypesFromYaml, synthesizeDefaultTypes } from "./critter-type.js"; import { log } from "./logger.js"; import type { AutoRetryConfig, AutoUpdateConfig, CircuitBreakerConfig, Config, RepoConfig, TunnelConfig } from "./types.js"; @@ -75,7 +75,9 @@ export function resolveConfigPath(configPath?: string): string { export function loadConfig(configPath?: string): Config { const resolved = resolveConfigPath(configPath); const raw = readFileSync(resolved, "utf-8"); - const yaml = parseYaml(raw) as Record; + // parseYaml returns null for an empty/comment-only file; coalesce to {} so the + // field reads below produce sensible defaults instead of a cryptic TypeError. + const yaml = (parseYaml(raw) ?? {}) as Record; const linearApiKey = process.env.LINEAR_API_KEY || undefined; const jiraHost = process.env.JIRA_HOST || undefined; @@ -295,7 +297,7 @@ export function loadWorkDir(configPath?: string): string { } const raw = readFileSync(resolved, "utf-8"); - const yaml = parseYaml(raw) as Record; + const yaml = (parseYaml(raw) ?? {}) as Record; const workDir = (yaml.workDir as string) ?? "/tmp/critters-work"; validateWorkDir(workDir); @@ -314,7 +316,7 @@ export function loadCleanConfig(configPath?: string): { workDir: string; cleanup return { workDir: "/tmp/critters-work", healthPort: 3847, tmuxSession: "critters" }; } const raw = readFileSync(resolved, "utf-8"); - const yaml = parseYaml(raw) as Record; + const yaml = (parseYaml(raw) ?? {}) as Record; const workDir = (yaml.workDir as string) ?? "/tmp/critters-work"; validateWorkDir(workDir); const tmuxSession = (yaml.tmuxSession as string) ?? "critters"; @@ -344,11 +346,10 @@ function parseCritterTypes(yaml: Record, config: Config): Critt const types: CritterTypeConfig[] = []; for (const [name, raw] of Object.entries(rawTypes)) { + // parseCritterTypesFromYaml → parseCritterType already validates each type, + // so no separate validateCritterType pass is needed here. const expanded = parseCritterTypesFromYaml(name, raw); - for (const ct of expanded) { - validateCritterType(ct); - types.push(ct); - } + types.push(...expanded); } if (types.length === 0) { diff --git a/src/env.ts b/src/env.ts index 4fc1ddfb..7cbdf323 100644 --- a/src/env.ts +++ b/src/env.ts @@ -11,12 +11,22 @@ export function loadEnvFallback(): void { if (!existsSync(cwdEnv) && existsSync(userEnv)) { const envContent = readFileSync(userEnv, "utf-8"); for (const line of envContent.split("\n")) { - const trimmed = line.trim(); + let trimmed = line.trim(); if (!trimmed || trimmed.startsWith("#")) continue; + // Support a leading `export ` prefix (common in shell-style .env files). + if (trimmed.startsWith("export ")) { + trimmed = trimmed.slice("export ".length).trim(); + } const eqIdx = trimmed.indexOf("="); if (eqIdx === -1) continue; const key = trimmed.slice(0, eqIdx).trim(); - const value = trimmed.slice(eqIdx + 1).trim(); + let value = trimmed.slice(eqIdx + 1).trim(); + // Strip a matching surrounding quote pair so a quoted token doesn't leak + // its quotes into the value (e.g. KEY="lin_..." → lin_...). Mirrors Bun's + // native CWD .env loader, which this fallback otherwise diverges from. + if (value.length >= 2 && (value[0] === '"' || value[0] === "'") && value[value.length - 1] === value[0]) { + value = value.slice(1, -1); + } if (!(key in process.env)) { process.env[key] = value; } diff --git a/src/log-resolver.ts b/src/log-resolver.ts index 5e138577..3fa276b5 100644 --- a/src/log-resolver.ts +++ b/src/log-resolver.ts @@ -21,21 +21,25 @@ export function stripAnsi(text: string): string { return text.replace(ANSI_RE, ""); } -function extractTimestamp(dirName: string): number { +export function extractTimestamp(dirName: string): number { const parts = dirName.split("-"); return parseInt(parts[parts.length - 1], 10); } -function newestDir(dirs: string[]): string { +export function newestDir(dirs: string[]): string { return dirs.sort((a, b) => extractTimestamp(b) - extractTimestamp(a))[0]; } +export function escapeRegExp(s: string): string { + return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + export function findWorkDirs(workDir: string, identifier: string): { critterDirs: string[]; reviewDirs: string[] } { if (!existsSync(workDir)) { return { critterDirs: [], reviewDirs: [] }; } - const escaped = identifier.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const escaped = escapeRegExp(identifier); const critterDirPattern = new RegExp(`^${escaped}-\\d+$`); const reviewDirPattern = new RegExp(`^review-${escaped}-\\d+$`); @@ -332,10 +336,6 @@ function extractReadableContent(jsonLines: string[], adapter: CliAdapter): strin return renderReadableLines(jsonLines, adapter).join("\n"); } -function escapeRegExp(s: string): string { - return s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); -} - export function resolvePhasesFromAttachments( identifier: string, attachments: Array<{ name: string; url: string }>, diff --git a/src/logs.ts b/src/logs.ts index eeafc503..193839ae 100644 --- a/src/logs.ts +++ b/src/logs.ts @@ -2,7 +2,7 @@ import { existsSync, readFileSync, statSync, writeFileSync } from "node:fs"; import { LinearClient } from "@linear/sdk"; import { loadWorkDir } from "./config.js"; import { STREAM_FILTER } from "./jq-filter.js"; -import { findWorkDirs, phaseFileTag, renderReadableLines, resolveCliAdapterForLog } from "./log-resolver.js"; +import { findWorkDirs, newestDir, phaseFileTag, renderReadableLines, resolveCliAdapterForLog } from "./log-resolver.js"; const IDENTIFIER_RE = /^[A-Z]+-\d+$/; const FILTER_TMP_PATH = "/tmp/critters-logs-filter.jq"; @@ -59,15 +59,6 @@ function parseArgs(args: string[]): ParsedArgs { return { identifier, phase, follow, configPath }; } -function extractTimestamp(dirName: string): number { - const parts = dirName.split("-"); - return parseInt(parts[parts.length - 1], 10); -} - -function newestDir(dirs: string[]): string { - return dirs.sort((a, b) => extractTimestamp(b) - extractTimestamp(a))[0]; -} - function writeFilterFile(): void { writeFileSync(FILTER_TMP_PATH, STREAM_FILTER, "utf-8"); } @@ -154,7 +145,11 @@ async function followLogs(logFile: string): Promise { } writeFilterFile(); - const proc = Bun.spawn(["sh", "-c", `tail -n +1 -f ${JSON.stringify(logFile)} | jq -cr --unbuffered --arg tool_color '\\x1b[36m' -f ${JSON.stringify(FILTER_TMP_PATH)}`], { + // Interpolate a real ESC byte (not the literal text "\x1b[36m") so jq receives + // an actual ANSI color code. A raw ESC inside sh single quotes is passed through + // verbatim, matching what displayWithJq does via --arg tool_color "\x1b[36m". + const toolColor = "\x1b[36m"; + const proc = Bun.spawn(["sh", "-c", `tail -n +1 -f ${JSON.stringify(logFile)} | jq -cr --unbuffered --arg tool_color '${toolColor}' -f ${JSON.stringify(FILTER_TMP_PATH)}`], { stdout: "inherit", stderr: "inherit", }); diff --git a/src/pr-status.ts b/src/pr-status.ts index 6a19ec6f..c76e0699 100644 --- a/src/pr-status.ts +++ b/src/pr-status.ts @@ -1,3 +1,5 @@ +import { runCommand } from "./utils.js"; + export interface PrStatus { ciStatus: "success" | "failure" | "pending" | "none"; reviewStatus: "approved" | "changes_requested" | "pending" | "none"; @@ -12,31 +14,52 @@ const cache = new Map(); const CACHE_TTL_MS = 60_000; // 60 seconds const MAX_CONCURRENT_FETCHES = 10; +// statusCheckRollup is a union of CheckRun nodes (status/conclusion) and legacy +// StatusContext nodes (state). Normalize each entry to a single verdict so that +// commit-status entries — which have no conclusion and never report COMPLETED — +// aren't read as perpetually pending. +export function normalizeCheckVerdict(check: { + status?: string; + conclusion?: string; + state?: string; +}): "success" | "failure" | "pending" { + const raw = (check.conclusion ?? check.state ?? "").toUpperCase(); + if (raw === "FAILURE" || raw === "ERROR" || raw === "TIMED_OUT" || raw === "CANCELLED" || raw === "ACTION_REQUIRED") { + return "failure"; + } + if (raw === "SUCCESS") { + return "success"; + } + // No verdict yet: CheckRuns are pending until status === COMPLETED; StatusContext + // entries with state PENDING (or any unrecognized value) are likewise pending. + if (check.conclusion === undefined && check.state === undefined) { + // CheckRun without a conclusion: trust its status field. + return check.status === "COMPLETED" ? "success" : "pending"; + } + return "pending"; +} + async function fetchPrStatus(prUrl: string): Promise { try { - const proc = Bun.spawn( - ["gh", "pr", "view", prUrl, "--json", "statusCheckRollup,reviewDecision"], - { stdout: "pipe", stderr: "pipe" }, + const { code, stdout } = await runCommand( + "gh", + ["pr", "view", prUrl, "--json", "statusCheckRollup,reviewDecision"], + { timeoutMs: 10_000 }, ); - const output = await new Response(proc.stdout).text(); - const exitCode = await proc.exited; - if (exitCode !== 0) { + if (code !== 0) { return { ciStatus: "none", reviewStatus: "none" }; } - const data = JSON.parse(output); + const data = JSON.parse(stdout); // Parse CI status from statusCheckRollup let ciStatus: PrStatus["ciStatus"] = "none"; - const checks: Array<{ status?: string; conclusion?: string }> = data.statusCheckRollup ?? []; + const checks: Array<{ status?: string; conclusion?: string; state?: string }> = data.statusCheckRollup ?? []; if (checks.length > 0) { - const hasFailure = checks.some( - (c) => c.conclusion === "FAILURE" || c.conclusion === "ERROR", - ); - const hasPending = checks.some((c) => c.status !== "COMPLETED"); - if (hasFailure) { + const verdicts = checks.map(normalizeCheckVerdict); + if (verdicts.some((v) => v === "failure")) { ciStatus = "failure"; - } else if (hasPending) { + } else if (verdicts.some((v) => v === "pending")) { ciStatus = "pending"; } else { ciStatus = "success"; @@ -78,22 +101,17 @@ export async function getPrStatuses(prUrls: string[]): Promise { - const status = await fetchPrStatus(url); - cache.set(url, { status, fetchedAt: Date.now() }); - result.set(url, status); - }); - - await Promise.all(fetches); - - // For URLs beyond the limit, return cached values if available, otherwise skip - for (const url of toFetch.slice(MAX_CONCURRENT_FETCHES)) { - const cached = cache.get(url); - if (cached) { - result.set(url, cached.status); - } + // Process all uncached URLs in sequential batches of MAX_CONCURRENT_FETCHES, + // pacing the work instead of dropping anything beyond the first batch. + for (let i = 0; i < toFetch.length; i += MAX_CONCURRENT_FETCHES) { + const batch = toFetch.slice(i, i + MAX_CONCURRENT_FETCHES); + await Promise.all( + batch.map(async (url) => { + const status = await fetchPrStatus(url); + cache.set(url, { status, fetchedAt: Date.now() }); + result.set(url, status); + }), + ); } return result; diff --git a/src/updater.ts b/src/updater.ts index af856069..47d59f46 100644 --- a/src/updater.ts +++ b/src/updater.ts @@ -176,11 +176,21 @@ async function downloadWithProgress( return Buffer.from(result); } +/** + * Download and apply the latest release over the running binary. + * Returns true ONLY when the renameSync over process.execPath actually + * succeeded (i.e. a new binary is now in place). Every failure or no-op + * (already up to date, download/verify error, etc.) returns false so callers + * like the auto-updater don't restart onto an unchanged version. + */ export async function checkForUpdate( currentVersion: string, - opts?: { force?: boolean }, -): Promise { + opts?: { force?: boolean; requireChecksum?: boolean }, +): Promise { const force = opts?.force ?? false; + // Auto-update opts in to strict verification: a missing checksum asset becomes + // a hard failure so we never rename in a binary we couldn't verify. + const requireChecksum = opts?.requireChecksum ?? false; const print = force ? console.log.bind(console) : log; const printError = force ? console.error.bind(console) : logError; @@ -191,17 +201,17 @@ export async function checkForUpdate( const execName = process.execPath.split("/").pop() ?? ""; if (execName === "bun" || execName === "bun.exe") { if (force) printError("Cannot update: running via bun, not as a compiled binary. Use install.sh to install."); - return; + return false; } if (currentVersion === "dev") { if (force) printError("Cannot check for updates: running a dev build."); - return; + return false; } if (!isAllowedApiUrl(RELEASES_URL)) { printError(`Update aborted: releases URL points to unexpected domain`); - return; + return false; } const tempPath = `${process.execPath}.update`; @@ -217,7 +227,7 @@ export async function checkForUpdate( if (!response.ok) { printError(`Update check failed: GitHub API returned ${response.status}`); - return; + return false; } const data = await response.json(); @@ -225,14 +235,14 @@ export async function checkForUpdate( if (typeof tag_name !== "string" || !Array.isArray(assets)) { printError("Update check failed: unexpected API response format"); - return; + return false; } const latestVersion = tag_name.replace(/^v/, "").replace(/-.*$/, ""); if (compareSemver(latestVersion, currentVersion) <= 0) { if (force) print(`Already up to date (v${currentVersion})`); - return; + return false; } print(`Update available: v${currentVersion} → v${latestVersion}`); @@ -244,14 +254,14 @@ export async function checkForUpdate( if (!asset || typeof asset.browser_download_url !== "string") { printError(`Update: no valid binary asset found for ${process.platform}-${process.arch}`); - return; + return false; } if (!isAllowedDownloadUrl(asset.browser_download_url)) { let hostname = "unknown"; try { hostname = new URL(asset.browser_download_url).hostname; } catch {} printError(`Update aborted: download URL points to unexpected domain: ${hostname}`); - return; + return false; } const downloadResponse = await fetch(asset.browser_download_url, { @@ -260,11 +270,20 @@ export async function checkForUpdate( if (!downloadResponse.ok) { printError(`Update download failed: HTTP ${downloadResponse.status}`); - return; + return false; } const buffer = await downloadWithProgress(downloadResponse, force); + // Reject a truncated download: if the server advertised a Content-Length and + // the received bytes don't match, the binary is incomplete — never install it. + const contentLengthHeader = downloadResponse.headers.get("Content-Length"); + const expectedBytes = contentLengthHeader !== null ? Number.parseInt(contentLengthHeader, 10) : null; + if (expectedBytes !== null && !Number.isNaN(expectedBytes) && buffer.length !== expectedBytes) { + printError(`Update aborted: download truncated (${buffer.length} of ${expectedBytes} bytes)`); + return false; + } + // Checksum verification const checksumAsset = assets.find( (a: { name?: string }) => a.name === "checksums-sha256.txt", @@ -275,7 +294,7 @@ export async function checkForUpdate( let hostname = "unknown"; try { hostname = new URL(checksumAsset.browser_download_url).hostname; } catch {} printError(`Update aborted: checksum URL points to unexpected domain: ${hostname}`); - return; + return false; } const checksumResponse = await fetch(checksumAsset.browser_download_url, { @@ -284,7 +303,7 @@ export async function checkForUpdate( if (!checksumResponse.ok) { printError(`Update: failed to download checksum file (HTTP ${checksumResponse.status}), aborting update`); - return; + return false; } const checksumContent = await checksumResponse.text(); @@ -293,15 +312,19 @@ export async function checkForUpdate( if (!expectedHash) { printError(`Update: checksum for ${expectedName} not found in checksums-sha256.txt, aborting update`); - return; + return false; } if (!verifyChecksum(buffer, expectedHash)) { printError("Update: SHA-256 checksum mismatch, aborting update (possible tampering or corruption)"); - return; + return false; } print("Checksum verified (SHA-256)"); + } else if (requireChecksum) { + // Auto-update must never rename in a binary it could not verify. + printError("Update aborted: checksums-sha256.txt not found in release, cannot verify binary for auto-update"); + return false; } else { print("Update: checksums-sha256.txt not found in release, skipping verification"); } @@ -313,6 +336,7 @@ export async function checkForUpdate( print(`Backup saved to ${backupPath}`); renameSync(tempPath, process.execPath); print(`Update applied (v${currentVersion} → v${latestVersion}). Restart the daemon manually to use the new version.`); + return true; } catch (err) { try { if (existsSync(tempPath)) { @@ -334,5 +358,6 @@ export async function checkForUpdate( } printError(`Update failed: ${formatError(err)}`); + return false; } } diff --git a/src/utils.ts b/src/utils.ts index 54e99fd8..9d8b8f37 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -6,18 +6,40 @@ import type { Config, SpawnResult } from "./types.js"; export function runCommand( command: string, args: string[], - options?: { cwd?: string }, + options?: { cwd?: string; timeoutMs?: number }, ): Promise<{ code: number; stdout: string; stderr: string }> { return new Promise((resolve) => { const proc = spawn(command, args, options?.cwd ? { cwd: options.cwd } : undefined); let stdout = ""; let stderr = ""; + let settled = false; + let timer: ReturnType | undefined; + + const finish = (result: { code: number; stdout: string; stderr: string }) => { + if (settled) return; + settled = true; + if (timer) clearTimeout(timer); + resolve(result); + }; + proc.stdout?.on("data", (d) => (stdout += d)); proc.stderr?.on("data", (d) => (stderr += d)); proc.on("error", (err) => { - resolve({ code: 1, stdout, stderr: stderr ? `${stderr}\n${err.message}` : err.message }); + finish({ code: 1, stdout, stderr: stderr ? `${stderr}\n${err.message}` : err.message }); }); - proc.on("close", (code) => resolve({ code: code ?? 1, stdout, stderr })); + proc.on("close", (code) => finish({ code: code ?? 1, stdout, stderr })); + + // Optional timeout: default behavior (no timeout) is preserved when timeoutMs is undefined. + if (options?.timeoutMs != null && options.timeoutMs > 0) { + timer = setTimeout(() => { + try { proc.kill("SIGKILL"); } catch {} + finish({ + code: 1, + stdout, + stderr: stderr ? `${stderr}\nCommand timed out after ${options.timeoutMs}ms` : `Command timed out after ${options.timeoutMs}ms`, + }); + }, options.timeoutMs); + } }); } From 5ce311b2239e89000625b5aa188039b60e4aee0c Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 15 Jun 2026 22:08:59 -0700 Subject: [PATCH 2/3] Fix pr-status verdict for non-failing terminal checks; tidy updater tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit normalizeCheckVerdict treated a COMPLETED CheckRun with conclusion SKIPPED/NEUTRAL/STALE/CANCELLED as 'pending' (hourglass forever), where the original code read it as success. Classify all recognized non-failing terminal conclusions as success, keep only in-flight/unknown states pending, and keep FAILURE/ERROR/TIMED_OUT/ACTION_REQUIRED (and STARTUP_FAILURE) as failure. StatusContext (state-based) handling from B20 is preserved. Tests: add coverage for SKIPPED/NEUTRAL/STALE/CANCELLED → success, and make the truncated-download test use a valid checksum so it isolates the truncation guard instead of leaning on a bad checksum. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/__tests__/audit-updater-misc.test.ts | 20 ++++++++++- src/pr-status.ts | 44 +++++++++++++++++++----- 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/__tests__/audit-updater-misc.test.ts b/src/__tests__/audit-updater-misc.test.ts index bccf2549..35e6932d 100644 --- a/src/__tests__/audit-updater-misc.test.ts +++ b/src/__tests__/audit-updater-misc.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { createHash } from "node:crypto"; import { mkdirSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { createTempDir } from "./helpers.js"; @@ -85,6 +86,19 @@ describe("B20: normalizeCheckVerdict (pr-status.ts)", () => { test("CheckRun in progress (no conclusion, not COMPLETED) → pending", () => { expect(normalizeCheckVerdict({ status: "IN_PROGRESS" })).toBe("pending"); }); + + test("COMPLETED CheckRun with a non-failing conclusion → success, not pending", () => { + // Regression: SKIPPED/NEUTRAL/STALE/CANCELLED used to read as perpetually pending. + expect(normalizeCheckVerdict({ status: "COMPLETED", conclusion: "SKIPPED" })).toBe("success"); + expect(normalizeCheckVerdict({ status: "COMPLETED", conclusion: "NEUTRAL" })).toBe("success"); + expect(normalizeCheckVerdict({ status: "COMPLETED", conclusion: "STALE" })).toBe("success"); + expect(normalizeCheckVerdict({ status: "COMPLETED", conclusion: "CANCELLED" })).toBe("success"); + }); + + test("CheckRun with a genuinely failing conclusion → failure", () => { + expect(normalizeCheckVerdict({ status: "COMPLETED", conclusion: "TIMED_OUT" })).toBe("failure"); + expect(normalizeCheckVerdict({ status: "COMPLETED", conclusion: "ACTION_REQUIRED" })).toBe("failure"); + }); }); // --------------------------------------------------------------------------- @@ -209,11 +223,15 @@ describe("checkForUpdate integrity (updater.ts)", () => { test("returns false on a truncated download (buffer.length !== Content-Length)", async () => { const body = "x".repeat(100); + // Use the VALID checksum of the received body so this test isolates the + // truncation guard — the download fails the length check before checksum + // verification, and a passing checksum proves nothing else rejected it. + const validHash = createHash("sha256").update(body).digest("hex"); mockFetch({ includeChecksumAsset: true, binaryBody: body, contentLength: "200", // advertises more than we received - checksumText: `0000 ${EXPECTED_ASSET}\n`, + checksumText: `${validHash} ${EXPECTED_ASSET}\n`, }); const result = await checkForUpdate("1.0.0"); expect(result).toBe(false); diff --git a/src/pr-status.ts b/src/pr-status.ts index c76e0699..98d5884a 100644 --- a/src/pr-status.ts +++ b/src/pr-status.ts @@ -23,19 +23,45 @@ export function normalizeCheckVerdict(check: { conclusion?: string; state?: string; }): "success" | "failure" | "pending" { - const raw = (check.conclusion ?? check.state ?? "").toUpperCase(); - if (raw === "FAILURE" || raw === "ERROR" || raw === "TIMED_OUT" || raw === "CANCELLED" || raw === "ACTION_REQUIRED") { + // Legacy StatusContext nodes (commit statuses) carry a `state` and no + // conclusion; they never report COMPLETED, so map their state directly. + if (check.conclusion === undefined && check.state !== undefined) { + const state = check.state.toUpperCase(); + if (state === "SUCCESS") return "success"; + if (state === "FAILURE" || state === "ERROR") return "failure"; + // PENDING, EXPECTED, or any unrecognized value is still in flight. + return "pending"; + } + + // CheckRun nodes carry a `conclusion` once they reach a terminal state. Until + // then conclusion is absent and we trust the `status` field. + const conclusion = (check.conclusion ?? "").toUpperCase(); + if (conclusion === "") { + return check.status === "COMPLETED" ? "success" : "pending"; + } + // Genuinely failing terminal conclusions. + if ( + conclusion === "FAILURE" || + conclusion === "ERROR" || + conclusion === "TIMED_OUT" || + conclusion === "ACTION_REQUIRED" || + conclusion === "STARTUP_FAILURE" + ) { return "failure"; } - if (raw === "SUCCESS") { + // Non-failing terminal conclusions: a completed CheckRun that didn't fail + // should not read as perpetually pending (the hourglass-forever bug). SKIPPED, + // NEUTRAL, STALE and CANCELLED all mean "not blocking" → treat as success. + if ( + conclusion === "SUCCESS" || + conclusion === "SKIPPED" || + conclusion === "NEUTRAL" || + conclusion === "STALE" || + conclusion === "CANCELLED" + ) { return "success"; } - // No verdict yet: CheckRuns are pending until status === COMPLETED; StatusContext - // entries with state PENDING (or any unrecognized value) are likewise pending. - if (check.conclusion === undefined && check.state === undefined) { - // CheckRun without a conclusion: trust its status field. - return check.status === "COMPLETED" ? "success" : "pending"; - } + // Unknown / non-terminal conclusion value: still in flight. return "pending"; } From daf2e13fa53e69c92a7f1cd4e9d2970d27742bee Mon Sep 17 00:00:00 2001 From: andrew Date: Tue, 16 Jun 2026 13:55:35 -0700 Subject: [PATCH 3/3] test: assert mcp config against homedir() to fix CI flakiness resolvePhaseMcpConfig expands ~ via node:os homedir(); the test compared it to process.env.HOME. Bun caches homedir() on first call and ignores later process.env.HOME mutations, so when a sibling test changes HOME they diverge on Linux CI and this test fails (works on macOS by execution-order luck). Assert against the same homedir() the implementation uses. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/__tests__/mcp.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/__tests__/mcp.test.ts b/src/__tests__/mcp.test.ts index 87ee7672..02c3f7e7 100644 --- a/src/__tests__/mcp.test.ts +++ b/src/__tests__/mcp.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { homedir } from "node:os"; import { ClaudeCodeAdapter } from "../cli/claude.js"; import { CodexAdapter } from "../cli/codex.js"; import { resolvePhaseMcpConfig } from "../cli/mcp.js"; @@ -25,7 +26,7 @@ describe("resolvePhaseMcpConfig", () => { } as Config, ); - expect(result.mcpConfig).toEqual([`${process.env.HOME}/.critters/review-mcp.json`]); + expect(result.mcpConfig).toEqual([`${homedir()}/.critters/review-mcp.json`]); expect(result.strictMcpConfig).toBe(true); });