From 481f122373bd24e7e5acdcaa7010e31a4073f9aa Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 04:40:34 -0700 Subject: [PATCH] fix(agent-actions): make the global kill-switch's fail-open observable, not silent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isGlobalAgentFrozen fails open (returns false / "not frozen") on a D1 read error or an absent singleton row — an intentional, already-tested tradeoff (a transient hiccup must not by itself halt the fleet). But the failure was completely silent: an operator flipping frozen=1 during an incident, while D1 happens to be degraded at that exact moment, or on a self-host instance whose singleton row was lost to a backup restore, would see "not frozen" with zero signal that the read actually failed rather than genuinely reporting unfrozen. Emit a structured warning distinguishing "confirmed unfrozen" from "read failed/row missing, assumed unfrozen" from both isGlobalAgentFrozen and the duplicate copy in the /status operator health surface (defaultOpsHealthDeps.isFrozen) — the fail-open VALUE is unchanged, only its silence is fixed. Also filed #2359: setGlobalAgentFrozen (the write side) has zero callers anywhere in src/ — there is currently no application-level way to actually flip this kill-switch, only direct SQL. Out of scope for this fix (a real admin route/MCP tool is a separate, larger addition), but worth tracking since it's the necessary next step to make the switch operable at all. Advances #1936. Closes #2125. --- src/db/repositories.ts | 12 ++++++++-- src/review/ops.ts | 10 +++++++-- test/unit/agent-action-executor.test.ts | 17 ++++++++++++++ test/unit/ops.test.ts | 30 ++++++++++++++++++++++++- 4 files changed, 64 insertions(+), 5 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 6214e6a602..e860703d48 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -163,7 +163,7 @@ import { DEFAULT_COMMAND_AUTHORIZATION_POLICY, normalizeCommandAuthorizationPoli import { normalizeContributorBlacklist } from "../settings/contributor-blacklist"; import { normalizeAutonomyPolicy, normalizeAutoMaintainPolicy, DEFAULT_AUTO_MAINTAIN_POLICY } from "../settings/autonomy"; import { decryptSecret, encryptSecret, sha256Hex } from "../utils/crypto"; -import { jsonString, nowIso, parseJson, repoParts } from "../utils/json"; +import { errorMessage, jsonString, nowIso, parseJson, repoParts } from "../utils/json"; import { PUBLIC_LOCAL_PATH_SCRUB_PATTERN } from "../signals/redaction"; const MAX_STORED_BODY_CHARS = 4000; @@ -2063,11 +2063,19 @@ export async function getProductUsageRollupStatus( // Global agent kill-switch (#audit-§5.2). A DB-backed emergency brake an operator flips with one row (no // redeploy), complementing the env-var AGENT_ACTIONS_PAUSED hard backstop. Fail-OPEN on a read error (return // false): a transient D1 hiccup must not by itself halt the whole fleet, and the env var is the hard backstop. +// The fail-open VALUE is an intentional, tested tradeoff — but it must never be SILENT: an operator who flips +// this during an incident concurrent with a D1 hiccup (or on a self-host instance that never ran migration +// 0059, or whose singleton row was later lost to a backup restore / manual cleanup) needs a visible signal that +// the kill-switch may not have actually engaged, not silent normal-looking operation. (#2125) export async function isGlobalAgentFrozen(env: Env): Promise { try { const row = await env.DB.prepare("SELECT frozen FROM global_agent_controls WHERE id = 'singleton'").first<{ frozen: number }>(); + if (!row) { + console.warn(JSON.stringify({ ev: "global_kill_switch_row_missing", message: "global_agent_controls has no singleton row — treating as unfrozen; re-run migrations or re-seed the row" })); + } return row?.frozen === 1; - } catch { + } catch (error) { + console.warn(JSON.stringify({ ev: "global_kill_switch_read_error", message: errorMessage(error).slice(0, 200) })); return false; } } diff --git a/src/review/ops.ts b/src/review/ops.ts index 0311b4815e..ace9877981 100644 --- a/src/review/ops.ts +++ b/src/review/ops.ts @@ -156,12 +156,18 @@ export interface OpsHealthDeps { export const defaultOpsHealthDeps: OpsHealthDeps = { validateAgentConfig: () => [], // The DB-backed global kill-switch (#audit-§5.2): /status now reports the REAL freeze state instead of a - // hardcoded false. Raw SQL keeps this module self-contained; fail-open on a read error. + // hardcoded false. Raw SQL keeps this module self-contained; fail-open on a read error — but this is the + // operator-facing health surface used to CONFIRM a freeze took effect, so a swallowed read failure must be + // visible, not silently reported as an ordinary "unfrozen" (#2125). isFrozen: async (env) => { try { const row = await env.DB.prepare("SELECT frozen FROM global_agent_controls WHERE id = 'singleton'").first<{ frozen: number }>(); + if (!row) { + console.warn(JSON.stringify({ ev: "global_kill_switch_row_missing", message: "global_agent_controls has no singleton row — /status will report unfrozen" })); + } return row?.frozen === 1; - } catch { + } catch (error) { + console.warn(JSON.stringify({ ev: "global_kill_switch_read_error", message: error instanceof Error ? error.message.slice(0, 200) : String(error).slice(0, 200) })); return false; } }, diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index b6a904f998..071c589b6b 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -166,6 +166,23 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(await isGlobalAgentFrozen(broken)).toBe(false); }); + it("isGlobalAgentFrozen's fail-open is never SILENT — a read error is observable, not indistinguishable from a genuine unfrozen state (#2125)", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const broken = { ...createTestEnv({}), DB: null } as unknown as Env; + expect(await isGlobalAgentFrozen(broken)).toBe(false); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("global_kill_switch_read_error")); + warn.mockRestore(); + }); + + it("isGlobalAgentFrozen also warns (but still fails open) when the table exists but the singleton row is absent (#2125)", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const env = createTestEnv({}); + await env.DB.prepare("DELETE FROM global_agent_controls WHERE id = 'singleton'").run(); + expect(await isGlobalAgentFrozen(env)).toBe(false); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("global_kill_switch_row_missing")); + warn.mockRestore(); + }); + it("auto_with_approval: stages the action (queued) instead of executing", async () => { const env = createTestEnv({}); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [{ ...merge, requiresApproval: true }]); diff --git a/test/unit/ops.test.ts b/test/unit/ops.test.ts index 5499c22d5e..60ce8e3f92 100644 --- a/test/unit/ops.test.ts +++ b/test/unit/ops.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { computeAgentHealth, computeCalibration, @@ -20,6 +20,34 @@ describe("defaultOpsHealthDeps.isFrozen — DB-backed global freeze (#audit-§5. const broken = { ...env, DB: null } as unknown as Env; expect(await defaultOpsHealthDeps.isFrozen(broken, "owner/repo")).toBe(false); // fail-open on a read error }); + + it("warns (but still fails open) on a read error and on an absent singleton row (#2125)", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const env = createTestEnv(); + const broken = { ...env, DB: null } as unknown as Env; + expect(await defaultOpsHealthDeps.isFrozen(broken, "owner/repo")).toBe(false); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("global_kill_switch_read_error")); + warn.mockClear(); + + await env.DB.prepare("DELETE FROM global_agent_controls WHERE id = 'singleton'").run(); + expect(await defaultOpsHealthDeps.isFrozen(env, "owner/repo")).toBe(false); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("global_kill_switch_row_missing")); + warn.mockRestore(); + }); + + it("formats a non-Error throw (e.g. a driver rejecting with a plain string) without crashing", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const thrown: Env = { + DB: { + prepare: () => { + throw "driver exploded"; // eslint-disable-line no-throw-literal -- exercising the non-Error catch arm + }, + } as unknown as Env["DB"], + } as unknown as Env; + expect(await defaultOpsHealthDeps.isFrozen(thrown, "owner/repo")).toBe(false); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("driver exploded")); + warn.mockRestore(); + }); }); // ── computeCalibration (ported from reviewbot test/calibration.test.ts) ──────────────────────────