diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index 9b71b72fba..3f55e9a5e9 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -19217,10 +19217,10 @@ "summary": "Verify a window of the hash-chained decision ledger (resumable via afterSeq)", "responses": { "200": { - "description": "Window verified clean; nextAfterSeq is the resume cursor (null at the tip)" + "description": "Window verified clean; nextAfterSeq is the resume cursor (null at the tip). Every response also carries tipSeq/tipHash/totalCount for third-party checkpointing." }, "409": { - "description": "First break found: sequence_gap | predecessor_mismatch | row_hash_mismatch" + "description": "First break found: sequence_gap | predecessor_mismatch | row_hash_mismatch | short_tail (a record exists past the verified tip with no chain entry)" } }, "security": [ @@ -19232,6 +19232,56 @@ } ] } + }, + "/v1/public/decision-records/{owner}/{repo}/{pull}": { + "get": { + "summary": "Fetch the latest published decision record for a PR, verbatim, plus its content digest", + "responses": { + "200": { + "description": "The latest DecisionRecord for this PR + its recordDigest" + }, + "400": { + "description": "Invalid pull number" + }, + "404": { + "description": "No decision record persisted yet for this PR" + } + }, + "security": [ + { + "LoopOverBearer": [] + }, + { + "LoopOverSessionCookie": [] + } + ], + "parameters": [ + { + "schema": { + "type": "string" + }, + "required": true, + "name": "owner", + "in": "path" + }, + { + "schema": { + "type": "string" + }, + "required": true, + "name": "repo", + "in": "path" + }, + { + "schema": { + "type": "string" + }, + "required": true, + "name": "pull", + "in": "path" + } + ] + } } }, "servers": [ diff --git a/src/api/routes.ts b/src/api/routes.ts index 48373a434f..1522a2fd4d 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -306,7 +306,7 @@ import { getContributorTrustProfile } from "../review/contributor-trust-profile" import { backfillContributorGateHistory } from "../review/contributor-gate-history-backfill"; import { isFairnessAnalyticsEnabled, resolveFairnessAnalyticsManifestOverride } from "../review/contributor-trust-profile-wire"; import { isRagEnabled } from "../review/rag-wire"; -import { verifyDecisionLedger } from "../review/decision-record"; +import { loadPublicDecisionRecord, verifyDecisionLedger } from "../review/decision-record"; import { getPublicStats, isPublicStatsEnabled, resolvePublicStatsManifestOverride } from "../review/public-stats"; import { loadPublicAccuracyTrend } from "../services/public-accuracy-trend"; import { loadPublicRulePrecision } from "../review/public-rule-precision"; @@ -1263,9 +1263,17 @@ export function createApp() { } }); - // #8837: public chain-verification for the decision ledger. Hashes/ids only — no record contents — so it - // is safe unauthenticated; any observer can confirm no decision was deleted, reordered, or rewritten. - // Resumable: pass afterSeq from the previous response's nextAfterSeq until it returns null. + // #8837: public chain-verification for the decision ledger. Hashes/ids only — no record contents — so it is + // unauthenticated by design (fixed in #9120: this route was missing from the requiresApiToken exemption + // list below and 401'd in prod despite this comment always having claimed otherwise). #9122 correction: this + // only proves SELF-consistency (no reorder/rewrite/gap WITHIN what verify examined) plus — since this + // route's own fix — that no decision_records row exists past the verified tip with no chain entry over it + // (catches a truncated tail, see verifyDecisionLedger's own doc comment). It does NOT prove the operator + // never deleted the chain wholesale and started fresh from genesis; that needs an external anchor the + // operator does not control, which is tracked but not yet built (decision-record.ts's module header has the + // full honest-limit paragraph). Resumable: pass afterSeq from the previous response's nextAfterSeq until it + // returns null; every response also carries the CURRENT tipSeq/tipHash/totalCount so a third party can keep + // its own checkpoint independent of pagination position. app.get("/v1/public/decision-ledger/verify", async (c) => { const afterSeq = Math.max(0, Number(c.req.query("afterSeq")) || 0); const limit = Number(c.req.query("limit")) || 500; @@ -1273,6 +1281,25 @@ export function createApp() { return c.json(result, result.ok ? 200 : 409); }); + // #9123: the decision record itself was persisted (decision_records) but never published anywhere a + // contributor or a third party could fetch the full body — the only prior public surface was + // renderDecisionRecordSection's bounded review-comment summary (12-char digest prefixes, and it omits + // decidedAt/baseSha/salvageability/repoFullName/pullNumber entirely). DecisionRecord is public-safe BY + // CONSTRUCTION (its own type doc: counts/digests/enums only, no diffs, no private config contents, no + // wallet/hotkey/trust-score/reward fields) — no field-level redaction needed before exposing it verbatim, + // unlike a route touching a type that carries any of those. Unauthenticated by design, mirroring the + // ledger-verify route immediately above; excluded from requiresApiToken below. + app.get("/v1/public/decision-records/:owner/:repo/:pull", async (c) => { + const owner = c.req.param("owner"); + const repo = c.req.param("repo"); + const pullNumber = Number(c.req.param("pull")); + if (!Number.isInteger(pullNumber) || pullNumber <= 0) return c.json({ error: "invalid_pull_number" }, 400); + const published = await loadPublicDecisionRecord(c.env, `${owner}/${repo}`, pullNumber); + if (!published) return c.json({ error: "not_found" }, 404); + c.header("Cache-Control", "public, max-age=60, stale-while-revalidate=300"); + return c.json({ record: published.record, recordDigest: published.recordDigest }); + }); + app.get("/v1/public/github/repos/:owner/:repo/stats", async (c) => { try { const stats = await fetchPublicRepoStats(c.env, c.req.param("owner"), c.req.param("repo")); @@ -6639,6 +6666,15 @@ function requiresApiToken(path: string): boolean { if (/^\/v1\/public\/repos\/[^/]+\/[^/]+\/quality$/.test(path)) return false; if (path === "/v1/public/subnet-interface") return false; if (path === "/v1/public/stats") return false; + // #9120: this route's own doc comment always claimed "safe unauthenticated" but was missing from this exact + // list, so it 401'd in prod — verified live: subnet-interface/stats/quality answered 200, this one 401'd. + if (path === "/v1/public/decision-ledger/verify") return false; + // #9123: the new public decision-record read route — same unauthenticated posture as its ledger-verify + // sibling immediately above, added in the SAME PR so the two can never drift apart the way #9120 did. The + // pull segment matches any non-slash text (not just digits): an invalid pull number is the ROUTE's 400 to + // return, not the auth gate's 401 — mirrors every other dynamic-segment exemption below (owner/repo use the + // same unvalidated [^/]+). + if (/^\/v1\/public\/decision-records\/[^/]+\/[^/]+\/[^/]+$/.test(path)) return false; if (path === "/openapi.json") return false; if (path === "/mcp") return false; // Public OAuth draft-submission flow (LOOPOVER_REVIEW_DRAFT): the submission entry points are unauthenticated diff --git a/src/openapi/spec.ts b/src/openapi/spec.ts index 5c53eb4c4a..3c606361c2 100644 --- a/src/openapi/spec.ts +++ b/src/openapi/spec.ts @@ -1001,8 +1001,19 @@ export function buildOpenApiSpec() { path: "/v1/public/decision-ledger/verify", summary: "Verify a window of the hash-chained decision ledger (resumable via afterSeq)", responses: { - 200: { description: "Window verified clean; nextAfterSeq is the resume cursor (null at the tip)" }, - 409: { description: "First break found: sequence_gap | predecessor_mismatch | row_hash_mismatch" }, + 200: { description: "Window verified clean; nextAfterSeq is the resume cursor (null at the tip). Every response also carries tipSeq/tipHash/totalCount for third-party checkpointing." }, + 409: { description: "First break found: sequence_gap | predecessor_mismatch | row_hash_mismatch | short_tail (a record exists past the verified tip with no chain entry)" }, + }, + }); + registry.registerPath({ + method: "get", + path: "/v1/public/decision-records/{owner}/{repo}/{pull}", + summary: "Fetch the latest published decision record for a PR, verbatim, plus its content digest", + request: { params: z.object({ owner: z.string(), repo: z.string(), pull: z.string() }) }, + responses: { + 200: { description: "The latest DecisionRecord for this PR + its recordDigest" }, + 400: { description: "Invalid pull number" }, + 404: { description: "No decision record persisted yet for this PR" }, }, }); registry.registerPath({ diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ad8c8bd624..e289f8a67d 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2907,6 +2907,12 @@ async function maybeCloseForContributorCapOnOpen( moderationWarningLabel: settings.moderationWarningLabel, moderationBannedLabel: settings.moderationBannedLabel, }, + // #9134: this early cap-on-open close previously wrote NO decision record at all — the exact bias the + // issue flagged (a contributor most likely to dispute a cap close had nothing joinable in the + // risk-control calibration set). The generic policy_close:contributor_cap reasonCode derivation + // (agent-action-executor.ts's defaultDecisionRecordReasonCode) is exactly right here — this path has + // no gate evaluation to derive a richer blockerClass-based code from. + decisionRecord: { configDigest: await contentDigest(settings) }, }, planned, ); @@ -3438,11 +3444,18 @@ async function runAgentMaintenancePlanAndExecute( // #8836/#8834: the content-addressed decision record — the public commitment that lets a contributor // argue "clause X of config abc123 decided this". Persist-only here (best-effort inside); the same // reasonCode expression as recordNativeGateDecision above so the record and the calibration row can never - // disagree about WHY. When an AI JUDGMENT shaped this decision (an AI_JUDGMENT_BLOCKER_CODES finding is - // present), the record carries the prompt-template commitment and the finding's calibrated confidence — - // the confidence is what joins every decision to the risk-control calibration set (#8835). modelId stays - // null at this site: the finding does not carry which concrete models ran, and recording a guess would be - // worse than recording nothing (the reviewDiagnostics ledger holds the per-run model identities). + // disagree about WHY. Built and persisted UNCONDITIONALLY for whatever disposition.actionClass this pass + // resolved to (merge/close/HOLD) — a hold never reaches executeAgentMaintenanceActions at all (this function + // returns early below once holdoutOnPlan is empty), so this is the only place a hold's decision record can + // be written; merge/close records built here stay independent of whether the ACTUAL mutation below ends up + // completing (#9134: this site's own ctx sets `decisionRecord: { managedByCaller: true }` on its + // executeAgentMaintenanceActions call, deliberately opting OUT of that executor's own completed-action + // record hook, to avoid double-recording the same decision here and there). When an AI JUDGMENT shaped this + // decision (an AI_JUDGMENT_BLOCKER_CODES finding is present), the record carries the prompt-template + // commitment and the finding's calibrated confidence — the confidence is what joins every decision to the + // risk-control calibration set (#8835). modelId stays null at this site: the finding does not carry which + // concrete models ran, and recording a guess would be worse than recording nothing (the reviewDiagnostics + // ledger holds the per-run model identities). { const aiJudgment = gate.blockers.find((blocker) => AI_JUDGMENT_BLOCKER_CODES.has(blocker.code)); // #8962: recomputed here (two cheap reads, only when an AI judgment shaped the decision) rather than @@ -3465,11 +3478,13 @@ async function runAgentMaintenancePlanAndExecute( aiConfidence: aiJudgment?.confidence ?? null, salvageability, }); - await persistDecisionRecord(env, record, recordDigest); + const recordId = await persistDecisionRecord(env, record, recordDigest); // #8838: persist the evaluation's own exact inputs beside the record (PRIVATE sibling, migration 0181) // so the replay harness can re-derive this decision bit-exactly. Best-effort, like the record itself; - // the no-replay no-op (synthetic content-lane/bridge evaluations) lives inside the helper. - await persistDecisionReplayInputForGate(env, `record:${record.repoFullName}#${record.pullNumber}@${record.headSha}`.slice(0, 250), gate, policyCloseKind ?? null); + // the no-replay no-op (synthetic content-lane/bridge evaluations) lives inside the helper. Keyed to the + // id persistDecisionRecord actually wrote (#9123: a supersession at the same head gets a revisioned id, + // not the base one this used to always recompute independently). + if (recordId !== null) await persistDecisionReplayInputForGate(env, recordId, gate, policyCloseKind ?? null); } // #2349 (PR 1): additive per-contributor calibration data, gated identically to recordNativeGateDecision // above -- see src/review/contributor-calibration.ts's doc comment. Currently write-only; nothing reads @@ -3661,6 +3676,11 @@ async function runAgentMaintenancePlanAndExecute( // #3472 split-brain: the executor's own live manual-review hold guard (immediately before approve/merge) // must check the SAME configured label the planner itself resolves labels.manualReview from. manualReviewLabel: settings.manualReviewLabel, + // #9134: this site already builds + persists its own decision record UNCONDITIONALLY, above (for every + // disposition — hold included, since a hold never even reaches this call) — see that block's own doc + // comment for why. `managedByCaller: true` deliberately opts OUT of the executor's generic + // completed-action record hook here, so the same decision is never recorded twice. + decisionRecord: { managedByCaller: true }, }, holdoutOnPlan, ); @@ -4020,6 +4040,9 @@ async function prReadyForReview( agentDryRun: settings.agentDryRun, installationPermissions: installation?.permissions ?? null, authorLogin: pr.authorLogin, + // #9134: required on every ctx, though this path only ever plans `update_branch` (never merge/close), + // so the decision-record path inside the executor is never actually exercised for it. + decisionRecord: { configDigest: await contentDigest(settings) }, }, [ { @@ -4464,6 +4487,9 @@ async function maybeForceFreshRebase( /* v8 ignore next -- an installed-App PR webhook always carries an installation record; the null is defensive (mirrors runAgentMaintenancePlanAndExecute's own identical merge-time read). */ installationPermissions: installation?.permissions ?? null, authorLogin: pr.authorLogin, + // #9134: required on every ctx, though this path only ever plans `update_branch` (never merge/close), so + // the decision-record path inside the executor is never actually exercised for it. + decisionRecord: { configDigest: await contentDigest(settings) }, }, [ { @@ -14008,6 +14034,9 @@ async function maybeThrottleReviewNagPing( installationPermissions: installation?.permissions ?? null, authorLogin: pr.authorLogin, moderationSettings: { moderationGateMode: settings.moderationGateMode, moderationRules: settings.moderationRules, moderationWarningLabel: settings.moderationWarningLabel, moderationBannedLabel: settings.moderationBannedLabel }, + // #9134: this review-nag close previously wrote NO decision record at all -- exactly the kind of + // contributor-disputable close the issue flagged as biasing the risk-control calibration join. + decisionRecord: { configDigest: await contentDigest(settings) }, }, planned, ); @@ -14199,6 +14228,9 @@ async function maybeThrottleMonitoredMentions( installationPermissions: installation?.permissions ?? null, authorLogin: pr.authorLogin, moderationSettings: { moderationGateMode: settings.moderationGateMode, moderationRules: settings.moderationRules, moderationWarningLabel: settings.moderationWarningLabel, moderationBannedLabel: settings.moderationBannedLabel }, + // #9134: this review-nag close (the @loopover-mention variant) previously wrote NO decision record at + // all -- the same gap as its comment-thread-cooldown sibling immediately above. + decisionRecord: { configDigest: await contentDigest(settings) }, }, planned, ); diff --git a/src/review/decision-record.ts b/src/review/decision-record.ts index 57d9e466f9..88c1dcfa50 100644 --- a/src/review/decision-record.ts +++ b/src/review/decision-record.ts @@ -11,6 +11,15 @@ // This record is also the input schema the golden-corpus replay (#8832) and the deterministic replay harness // (#8838) consume — one schema, three consumers, so drift between "what we published" and "what we can // replay" is structurally impossible. +// +// HONEST LIMIT (#9122, mirrored from migrations/0180_decision_ledger.sql's own header): the hash-chained +// ledger below makes this instance's history tamper-EVIDENT, not tamper-PROOF — an operator with direct DB +// access can still rewrite the chain wholesale (delete every row, recompute a fresh one from genesis) and +// nothing here can detect that from first principles. External anchoring (a signed checkpoint published +// somewhere the operator does not control — a git commit, a transparency log, an on-chain commitment) is the +// tracked follow-up once tenants exist, not yet built. That gap does not reduce the value against every OTHER +// actor (a maintainer quietly deleting one disputed decision, or an unprivileged bug), or against accidental +// corruption — both of which the chain below still catches deterministically. import { errorMessage, nowIso } from "../utils/json"; /** Bump when the record's FIELD SET changes meaning — consumers compare records only within a version. */ @@ -109,51 +118,68 @@ export async function buildDecisionRecord( } /** - * Persist the record (decision_records, migration 0179), keyed per (target, head sha) with - * latest-finalize-wins — the same replacement contract as the gate_decision row it explains. Best-effort: - * recording legibility must never break finalization (mirrors recordNativeGateDecision's posture). + * Persist the record (decision_records, migration 0179), one row per (target, head sha) UNLESS this exact + * head was already decided before — a re-gate that lands a SECOND verdict for a head decision_records + * already has a row for (#9123's "compounding bug": a live fleet carried 51 chain rows referencing a digest + * an UPDATE had already overwritten, permanently unreconcilable). The FIRST record for a (repo, pull, head) + * keeps the plain `record:#@` id every existing consumer (the replay CLI's extract query, + * decision-replay.ts) already expects; a SUPERSESSION gets its OWN row at `:rev` instead of + * overwriting it, so the digest the ledger already chained for the first decision keeps a live preimage + * forever — the ledger's own append-only "supersessions are visible history" promise now actually holds for + * the record body too, not just the chain pointer. Best-effort: recording legibility must never break + * finalization (mirrors recordNativeGateDecision's posture). Returns the id actually written (null on a + * swallowed failure) so a caller needing to key a private sibling row (e.g. decision-replay.ts's replay + * input) targets the SAME row this call produced, including a supersession's revisioned id. */ -export async function persistDecisionRecord(env: Env, record: DecisionRecord, recordDigest: string): Promise { +export async function persistDecisionRecord(env: Env, record: DecisionRecord, recordDigest: string, attempts = 3): Promise { + const baseId = `record:${record.repoFullName}#${record.pullNumber}@${record.headSha}`.slice(0, 250); try { - await env.DB.prepare( - `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) - ON CONFLICT(id) DO UPDATE SET action = excluded.action, reason_code = excluded.reason_code, - record_digest = excluded.record_digest, record_json = excluded.record_json, created_at = excluded.created_at`, - ) - .bind( - `record:${record.repoFullName}#${record.pullNumber}@${record.headSha}`.slice(0, 250), - record.repoFullName.slice(0, 200), - record.pullNumber, - record.headSha, - record.action, - record.reasonCode.slice(0, 200), - recordDigest, - canonicalJson(record), - record.decidedAt, - ) - .run(); - // #8837: every write appends a chain row — including latest-finalize-wins rewrites of the same id, so - // supersessions are visible history rather than silent replacement. - await appendDecisionLedger(env, `record:${record.repoFullName}#${record.pullNumber}@${record.headSha}`.slice(0, 250), recordDigest); + for (let attempt = 1; ; attempt += 1) { + const prior = await env.DB.prepare(`SELECT COUNT(*) AS n FROM decision_records WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ?`) + .bind(record.repoFullName.slice(0, 200), record.pullNumber, record.headSha) + .first<{ n: number }>(); + /* v8 ignore next -- defensive: a bare COUNT(*) always returns exactly one row (even {n: 0} against an + * empty table); the `?? 0` only satisfies .first()'s optional-by-signature TS return type. */ + const priorCount = prior?.n ?? 0; + const id = priorCount === 0 ? baseId : `${baseId}:rev${priorCount + 1}`; + try { + await env.DB.prepare( + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + ) + .bind(id, record.repoFullName.slice(0, 200), record.pullNumber, record.headSha, record.action, record.reasonCode.slice(0, 200), recordDigest, canonicalJson(record), record.decidedAt) + .run(); + // #8837: every write appends a chain row — including a supersession's OWN new row, so re-decisions + // are visible history rather than silent replacement. + await appendDecisionLedger(env, id, recordDigest); + return id; + } catch (error) { + if (attempt >= attempts) throw error; + // A concurrent supersession at the exact same (repo, pull, head) raced the count-then-insert above and + // collided on the PK — re-count and retry with the next revision id (mirrors appendDecisionLedger's + // own PK-collision retry for the ledger tip immediately above). + } + } } catch (error) { console.warn(JSON.stringify({ event: "decision_record_persist_error", target: `${record.repoFullName}#${record.pullNumber}`, message: errorMessage(error).slice(0, 160) })); + return null; } } /** Bounded, human-readable markdown body for the public review surface: the claim ("clause X of config - * abc123…") plus the digests a challenger needs. Digests are truncated for display; the full values live in - * the persisted record. Returned WITHOUT a details wrapper — the unified-comment bridge renders the - * collapsible chrome itself (UnifiedCollapsible). */ + * abc123…") plus the digests a challenger needs. #9123: digests print in FULL (64 hex chars) — a truncated + * prefix is not a commitment a challenger can actually compare against a re-hashed config/prompt/record, only + * a hint. The head sha keeps its conventional 7-char git-abbreviation (a display convention for a commit-ish, + * not a digest commitment; the full value is the record's own `headSha` field). Returned WITHOUT a details + * wrapper — the unified-comment bridge renders the collapsible chrome itself (UnifiedCollapsible). */ export function renderDecisionRecordSection(record: DecisionRecord, recordDigest: string): string { - const short = (digest: string): string => digest.slice(0, 12); const lines = [ `- **action**: ${record.action} · **clause**: \`${record.reasonCode}\``, - `- **config**: \`${short(record.configDigest)}\`${record.gatePack ? ` · **pack**: ${record.gatePack}` : ""}${record.ciState ? ` · **ci**: ${record.ciState}` : ""}`, + `- **config**: \`${record.configDigest}\`${record.gatePack ? ` · **pack**: ${record.gatePack}` : ""}${record.ciState ? ` · **ci**: ${record.ciState}` : ""}`, ...(record.modelId !== null || record.promptDigest !== null - ? [`- **model**: ${record.modelId ?? "n/a"}${record.promptDigest !== null ? ` · **prompt**: \`${short(record.promptDigest)}\`` : ""}${record.aiConfidence !== null ? ` · **confidence**: ${record.aiConfidence}` : ""}`] + ? [`- **model**: ${record.modelId ?? "n/a"}${record.promptDigest !== null ? ` · **prompt**: \`${record.promptDigest}\`` : ""}${record.aiConfidence !== null ? ` · **confidence**: ${record.aiConfidence}` : ""}`] : []), - `- **record**: \`${short(recordDigest)}\` (schema v${record.schemaVersion}, head \`${record.headSha.slice(0, 7)}\`)`, + `- **record**: \`${recordDigest}\` (schema v${record.schemaVersion}, head \`${record.headSha.slice(0, 7)}\`)`, ]; return lines.join("\n"); } @@ -178,6 +204,34 @@ export async function loadDecisionRecordCollapsible(env: Env, repoFullName: stri } } +/** + * #9123: the record was persisted but never PUBLISHED anywhere — the only thing that ever reached a + * contributor was renderDecisionRecordSection's bounded markdown summary (12-char digest prefixes, no + * decidedAt/baseSha/salvageability/repoFullName/pullNumber at all). This is the raw material for a public + * `GET /v1/public/decision-records/:owner/:repo/:pull` route: the LATEST record for a PR, verbatim, plus its + * digest — DecisionRecord is already public-safe by construction (its own type doc: "counts/digests/enums + * only — no diffs, no private config contents, no author identity"), so no field-level redaction is needed + * here, unlike a route that touches a wallet/hotkey/trust-score-bearing type. Same latest-wins query + * loadDecisionRecordCollapsible already uses (ORDER BY created_at DESC — a supersession's revisioned id sorts + * correctly by creation time regardless of its id suffix). Returns null on no-row-yet OR unreadable JSON, + * mirroring loadDecisionRecordCollapsible's own fail-safe posture — a route caller renders 404 either way. + */ +export async function loadPublicDecisionRecord(env: Env, repoFullName: string, pullNumber: number): Promise<{ record: DecisionRecord; recordDigest: string } | null> { + try { + const row = await env.DB.prepare( + `SELECT record_digest AS recordDigest, record_json AS recordJson FROM decision_records + WHERE repo_full_name = ? AND pull_number = ? ORDER BY created_at DESC LIMIT 1`, + ) + .bind(repoFullName, pullNumber) + .first<{ recordDigest: string; recordJson: string }>(); + if (!row) return null; + return { record: JSON.parse(row.recordJson) as DecisionRecord, recordDigest: row.recordDigest }; + } catch (error) { + console.warn(JSON.stringify({ event: "decision_record_public_load_error", target: `${repoFullName}#${pullNumber}`, message: errorMessage(error).slice(0, 160) })); + return null; + } +} + // ── Hash-chained ledger (#8837) ───────────────────────────────────────────────────────────────────────────── /** Genesis predecessor: the chain's first row links to 64 zero nibbles. */ @@ -223,18 +277,47 @@ export async function appendDecisionLedger(env: Env, recordId: string, recordDig export type LedgerBreak = | { kind: "sequence_gap"; atSeq: number; expectedSeq: number } | { kind: "predecessor_mismatch"; atSeq: number } - | { kind: "row_hash_mismatch"; atSeq: number }; + | { kind: "row_hash_mismatch"; atSeq: number } + // #9122: a self-consistent chain that stops short of every decision_records row it should account for — see + // the reconciliation check at the end of verifyDecisionLedger below for exactly what this catches and why. + | { kind: "short_tail"; atSeq: number }; + +/** #9122: the exact shape a scheduled external-anchoring job (git-commit checkpoint, transparency log, or an + * on-chain commitment — the actual publishing mechanism is a genuinely open infra/protocol decision tracked + * on the issue, deliberately NOT built here) would publish for a given tip: enough for a third party to later + * prove "the ledger's tip really was this, at this time" against whatever anchor eventually receives it. Pure + * and synchronous — this module has no scheduler and calls this from nowhere yet; a future cron handler is + * the natural caller, using the tipSeq/tipHash verifyDecisionLedger already returns on every call. */ +export function buildLedgerAnchorPayload(tip: { seq: number; rowHash: string }, at: string = nowIso()): { seq: number; rowHash: string; at: string } { + return { seq: tip.seq, rowHash: tip.rowHash, at }; +} /** * Verify a window of the chain, resumable via `afterSeq` (0 = genesis). Reports the FIRST break with its - * class — a gap, a broken predecessor link, or a rewritten row — and the cursor for the next window. Pure + * class — a gap, a broken predecessor link, a rewritten row, or a short tail (see below) — and the cursor for + * the next window. Always returns the CURRENT global tip (`tipSeq`/`tipHash`) and total row count, regardless + * of where this window's pagination stopped, so a third-party checkpoint-keeper can compare it against + * whatever tip it last observed (#9122 — the exact shape a future external-anchoring job would need). Pure * read; safe on a public route (hashes and ids only, no record contents). */ -export async function verifyDecisionLedger(env: Env, afterSeq = 0, limit = 500): Promise<{ ok: boolean; checked: number; nextAfterSeq: number | null; break?: LedgerBreak }> { +export async function verifyDecisionLedger( + env: Env, + afterSeq = 0, + limit = 500, +): Promise<{ ok: boolean; checked: number; nextAfterSeq: number | null; tipSeq: number; tipHash: string; totalCount: number; break?: LedgerBreak }> { const bounded = Math.max(1, Math.min(1000, limit)); - const prior = afterSeq > 0 ? await env.DB.prepare("SELECT seq, row_hash AS rowHash FROM decision_ledger WHERE seq = ?").bind(afterSeq).first<{ seq: number; rowHash: string }>() : null; + const [totalRow, globalTip, prior] = await Promise.all([ + env.DB.prepare("SELECT COUNT(*) AS n FROM decision_ledger").first<{ n: number }>(), + env.DB.prepare("SELECT seq, row_hash AS rowHash FROM decision_ledger ORDER BY seq DESC LIMIT 1").first<{ seq: number; rowHash: string }>(), + afterSeq > 0 ? env.DB.prepare("SELECT seq, row_hash AS rowHash, created_at AS createdAt FROM decision_ledger WHERE seq = ?").bind(afterSeq).first<{ seq: number; rowHash: string; createdAt: string }>() : Promise.resolve(null), + ]); + /* v8 ignore next -- defensive: a bare COUNT(*) always returns exactly one row (even {n: 0} against an empty + * table); the `?? 0` only satisfies .first()'s optional-by-signature TS return type. */ + const totalCount = totalRow?.n ?? 0; + const tipSeq = globalTip?.seq ?? 0; + const tipHash = globalTip?.rowHash ?? LEDGER_GENESIS_HASH; // `== null` deliberately: D1 drivers disagree on .first() returning null vs undefined for no-row. - if (afterSeq > 0 && prior == null) return { ok: false, checked: 0, nextAfterSeq: null, break: { kind: "sequence_gap", atSeq: afterSeq, expectedSeq: afterSeq } }; + if (afterSeq > 0 && prior == null) return { ok: false, checked: 0, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "sequence_gap", atSeq: afterSeq, expectedSeq: afterSeq } }; let prevHash = prior?.rowHash ?? LEDGER_GENESIS_HASH; let expectedSeq = afterSeq + 1; const { results } = await env.DB.prepare( @@ -243,14 +326,39 @@ export async function verifyDecisionLedger(env: Env, afterSeq = 0, limit = 500): .bind(afterSeq, bounded) .all<{ seq: number; recordId: string; recordDigest: string; prevHash: string; rowHash: string; createdAt: string }>(); let checked = 0; + // Tracks the created_at of the last row this call actually verified clean — the anchor the tail-truncation + // reconciliation below compares decision_records against. Seeded from `prior` (the checkpoint we resumed + // from) so a call that finds ZERO new rows still has an anchor to reconcile against. + let lastVerifiedCreatedAt = prior?.createdAt ?? null; for (const row of results) { - if (row.seq !== expectedSeq) return { ok: false, checked, nextAfterSeq: null, break: { kind: "sequence_gap", atSeq: row.seq, expectedSeq } }; - if (row.prevHash !== prevHash) return { ok: false, checked, nextAfterSeq: null, break: { kind: "predecessor_mismatch", atSeq: row.seq } }; + if (row.seq !== expectedSeq) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "sequence_gap", atSeq: row.seq, expectedSeq } }; + if (row.prevHash !== prevHash) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "predecessor_mismatch", atSeq: row.seq } }; const recomputed = await ledgerRowHash(prevHash, { seq: row.seq, recordId: row.recordId, recordDigest: row.recordDigest, createdAt: row.createdAt }); - if (recomputed !== row.rowHash) return { ok: false, checked, nextAfterSeq: null, break: { kind: "row_hash_mismatch", atSeq: row.seq } }; + if (recomputed !== row.rowHash) return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "row_hash_mismatch", atSeq: row.seq } }; prevHash = row.rowHash; + lastVerifiedCreatedAt = row.createdAt; expectedSeq = row.seq + 1; checked += 1; } - return { ok: true, checked, nextAfterSeq: results.length === bounded ? results[results.length - 1]!.seq : null }; + const nextAfterSeq = results.length === bounded ? results[results.length - 1]!.seq : null; + // #9122 — TAIL TRUNCATION: everything above only ever detects a break BETWEEN rows that still exist; deleting + // the newest rows outright (`DELETE FROM decision_ledger WHERE seq > N`) leaves every remaining row's + // gap/predecessor/hash checks passing clean, since there is nothing left in the window to disagree with. + // But decision_ledger and decision_records are written together, in the SAME call (persistDecisionRecord + // appends its ledger row immediately after inserting the record) — deleting ledger rows never touches + // decision_records. So a record created strictly AFTER this window's verified tip, with no chain entry + // covering it, is exactly the signature a truncated tail leaves behind in the one place the deletion could + // not reach. Only checked once we've reached what this call believes is the current end of the chain + // (`nextAfterSeq === null`; a paginated window still has more to verify first) and only when there is an + // actual tip to anchor the comparison on (an entirely empty, never-yet-populated ledger has nothing to + // truncate FROM, and predates this reconciliation by definition). + if (nextAfterSeq === null && lastVerifiedCreatedAt !== null) { + const orphaned = await env.DB.prepare("SELECT COUNT(*) AS n FROM decision_records WHERE created_at > ?").bind(lastVerifiedCreatedAt).first<{ n: number }>(); + /* v8 ignore next -- defensive: a bare COUNT(*) always returns exactly one row (even {n: 0}); the `?? 0` + * only satisfies .first()'s optional-by-signature TS return type. */ + if ((orphaned?.n ?? 0) > 0) { + return { ok: false, checked, nextAfterSeq: null, tipSeq, tipHash, totalCount, break: { kind: "short_tail", atSeq: expectedSeq - 1 } }; + } + } + return { ok: true, checked, nextAfterSeq, tipSeq, tipHash, totalCount }; } diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 66f3a56ee1..18e5dc4b5c 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -43,6 +43,7 @@ import { incr } from "../selfhost/metrics"; import { shouldWaitForOlderSiblings } from "../review/merge-train"; import { capturePostHogError } from "../selfhost/posthog"; import { claimContributorCapLock, releaseContributorCapLock } from "../queue/transient-locks"; +import { buildDecisionRecord, persistDecisionRecord, type DecisionRecord } from "../review/decision-record"; // The agent actor name on every audit record — the App acts on the maintainer's behalf per their configured // autonomy (the config IS the authorization; there is no human commenter to authorize, unlike #824). @@ -226,8 +227,101 @@ export type AgentActionExecutionContext = { // `pull_request_files` cache already populated). Absent/undefined degrades the merge-train overlap check to // linked-issue-only for this PR, never to "no overlap possible". pullRequestChangedFiles?: readonly string[] | undefined; + // #9134: every completed merge/close MUST emit a decision record + chained ledger row -- resolved by the + // CALLER (same "the executor has no settings access" shape as contributorCapCancelCi/manualReviewLabel + // above) and REQUIRED (not optional), so a NEW call site cannot compile without deciding what its record + // should carry. Before this field existed, buildDecisionRecord/persistDecisionRecord ran at exactly ONE of + // this executor's seven call sites (the gate-evaluation plan-and-execute path) -- and there UNCONDITIONALLY, + // for the disposition it PLANNED regardless of hold/merge/close and regardless of whether the plan actually + // executes cleanly (that site's own doc comment explains why: a hold never even reaches this executor, so + // its record has to be built before that early return). The other six call sites -- a contributor-cap close + // on PR open, two review-nag closes, an approval-queue accept, and two forced-rebase update_branch calls -- + // wrote no record and no ledger row at all, silently biasing the risk-control calibration join + // (loadCalibrationPairs) away from exactly the closes a contributor is most likely to dispute. Set + // `managedByCaller: true` ONLY for a call site that already builds its own record independently of this + // executor's completed-action outcome (today: just the one gate-evaluation site, for the hold reason above) + // -- passing it silently is a deliberate, self-documenting override, not an accidental omission, which is + // the actual property this field being required protects. + decisionRecord: DecisionRecordContext; }; +/** See `AgentActionExecutionContext.decisionRecord`'s doc comment for why this exists, is required, and has an + * explicit opt-out. */ +export type DecisionRecordContext = + | { managedByCaller: true } + | { + managedByCaller?: false | undefined; + /** Digest of the RESOLVED effective settings in force for this decision (canonical JSON) -- the ONE + * field every caller must actually resolve; every other field defaults to null/a generic derivation + * below when a caller has nothing richer to say. Compute via `contentDigest(settings)` + * (decision-record.ts) the SAME way the gate-evaluation call site already does. */ + configDigest: string; + gatePack?: string | null | undefined; + ciState?: string | null | undefined; + baseSha?: string | null | undefined; + modelId?: string | null | undefined; + promptDigest?: string | null | undefined; + aiConfidence?: number | null | undefined; + salvageability?: { score: number; factors: string[] } | null | undefined; + /** Override the generic reasonCode derivation (see `defaultDecisionRecordReasonCode` below) -- a caller + * with richer blockerClass/gate.conclusion context this executor has no way to derive generically + * passes its own `deriveDecisionReasonCode` result here instead. */ + reasonCode?: string | undefined; + /** Called once, best-effort, immediately after a completed merge/close action's record is persisted -- + * lets a caller with a sibling private row to write (e.g. a decision-replay input, keyed to the exact + * SAME record id) act on the id this call actually wrote, including a supersession's revisioned id + * (#9123) rather than recomputing a possibly-stale base id independently. Never invoked when the + * persist itself failed (persistDecisionRecord already swallows that and warns; recordCompletedDecision + * below treats a null id the same way). */ + afterPersist?: ((recordId: string, record: DecisionRecord) => Promise | void) | undefined; + }; + +/** Generic reasonCode when the caller has no richer derivation of its own (see DecisionRecordContext.reasonCode + * above). A policy-tagged close (contributor_cap / blacklist / review_nag / heuristic / whatever closeKind the + * planner attached) publishes as `policy_close:`, the SAME convention deriveDecisionReasonCode + * (decision-replay.ts) already uses for a policy close's blockerClass-less case; a plain merge, or a close + * with no closeKind at all, publishes as "success" -- there is no blocker to name and the action itself IS + * the verdict. Exported for direct unit testing. */ +export function defaultDecisionRecordReasonCode(action: PlannedAgentAction): string { + return action.closeKind !== undefined ? `policy_close:${action.closeKind}` : "success"; +} + +/** + * #9134: emit the decision record + chained ledger row for a JUST-COMPLETED merge/close action, by + * construction -- called from the SAME "completed" branch every call site's mutation funnels through (step 9 + * below), so a new call site cannot add a merge/close outcome without also going through this (unless it + * opted out via `managedByCaller: true` — see DecisionRecordContext's doc comment for the one site that does). + * Best-effort, mirroring persistDecisionRecord's own posture: recording legibility must never fail a mutation + * that already succeeded on GitHub -- callers wrap this in `.catch(() => undefined)`. + */ +async function recordCompletedDecision(env: Env, ctx: AgentActionExecutionContext, action: PlannedAgentAction): Promise { + if (ctx.decisionRecord.managedByCaller === true) return; + const dr = ctx.decisionRecord; + /* v8 ignore next -- the step-5 freshness guard above already denies a merge/close (this function only ever + * runs for those two classes) whenever action.expectedHeadSha ?? ctx.headSha is falsy, so this is always a + * truthy string by the time a completed merge/close reaches here; the "unknown" fallback only satisfies + * buildDecisionRecord's required string headSha field, mirroring the exact same defensive pattern the + * merge/approve cases in performAction already use for this identical expression. */ + const headSha = action.expectedHeadSha ?? ctx.headSha ?? "unknown"; + const { record, recordDigest } = await buildDecisionRecord({ + repoFullName: ctx.repoFullName, + pullNumber: ctx.pullNumber, + headSha, + baseSha: dr.baseSha ?? null, + action: action.actionClass, + reasonCode: dr.reasonCode ?? defaultDecisionRecordReasonCode(action), + configDigest: dr.configDigest, + gatePack: dr.gatePack ?? null, + ciState: dr.ciState ?? null, + modelId: dr.modelId ?? null, + promptDigest: dr.promptDigest ?? null, + aiConfidence: dr.aiConfidence ?? null, + salvageability: dr.salvageability ?? null, + }); + const recordId = await persistDecisionRecord(env, record, recordDigest); + if (recordId !== null && dr.afterPersist) await dr.afterPersist(recordId, record); +} + export type ModerationContextSettings = { moderationGateMode?: "inherit" | "off" | "enabled" | undefined; moderationRules?: ModerationRuleType[] | undefined; @@ -598,6 +692,12 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE try { const detailOverride = await performAction(env, ctx, action); await audit("completed", detailOverride ?? action.reason); + // #9134: every completed merge/close emits a decision record + chained ledger row, by construction -- + // see DecisionRecordContext's doc comment on why this lives HERE instead of at each call site. Never + // throws: recording legibility must never retroactively fail a mutation that already succeeded. + if (action.actionClass === "merge" || action.actionClass === "close") { + await recordCompletedDecision(env, ctx, action).catch(() => undefined); + } // CI-run cancellation on an anti-abuse close (#2462 contributor_cap; extended to blacklist #6659): stop // burning CI minutes on a PR that was just closed for exceeding the contributor cap, or for a banned // login. contributor_cap stays opt-in (contributorCapCancelCi) since a repo may want the cap to bite diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index d9ebe45cfb..f04a0a3bfb 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -13,6 +13,7 @@ import { resolveRepositorySettings } from "../settings/repository-settings"; import { createInstallationToken } from "../github/app"; import { loadLinkedIssueHardRules, resolveLinkedIssueHardRule } from "../review/linked-issue-hard-rules"; import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent-action-executor"; +import { contentDigest } from "../review/decision-record"; import { downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, type PlannedAgentAction } from "../settings/agent-actions"; import { findBlacklistEntry } from "../settings/contributor-blacklist"; import { isCloseHoldOnly, isHoldOnly, readUntrustworthyRuleCodes } from "../review/outcomes-wire"; @@ -449,6 +450,13 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // manual-review hold on this same PR/head before the maintainer accepts — the executor's own live guard // (step 7b of executeAgentMaintenanceActions) needs the configured label to check for. manualReviewLabel: settings.manualReviewLabel, + // #9134: the approval-queue accept path previously wrote NO decision record at all for a staged + // merge/close — every approval-gated action was invisible to the risk-control calibration join. closeKind + // round-trips through actionParams/pendingActionToPlanned (see their doc comments), so the executor's + // generic policy_close: reasonCode derivation still applies here — coarser than the immediate + // gate-evaluation path's blockerClass-based reasonCode for a replayed HEURISTIC close (this path has no + // `gate` object to re-derive that from), an accepted, documented gap rather than a re-run evaluation. + decisionRecord: { configDigest: await contentDigest(settings) }, }, plan, ); diff --git a/test/integration/public-decision-ledger-routes.test.ts b/test/integration/public-decision-ledger-routes.test.ts new file mode 100644 index 0000000000..ffce3e0802 --- /dev/null +++ b/test/integration/public-decision-ledger-routes.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, it } from "vitest"; +import { createApp } from "../../src/api/routes"; +import { createTestEnv } from "../helpers/d1"; +import { buildDecisionRecord, contentDigest, persistDecisionRecord } from "../../src/review/decision-record"; + +// #9120: the public decision-ledger verify endpoint required an API token in prod despite its own doc comment +// (and every sibling /v1/public/* route's own doc comment) always claiming "unauthenticated by design" -- +// verified live: /v1/public/subnet-interface and /v1/public/stats answered 200 with no credentials, +// /v1/public/decision-ledger/verify 401'd. This file pins the fix AND, per the issue's own ask, table-drives +// the full requiresApiToken /v1/public/* exemption list so the next public route added can't silently regress +// the same way -- each entry below is exercised as a real anonymous HTTP request, not a unit test of the +// (unexported) requiresApiToken function itself. +describe("public decision-ledger/decision-records routes answer WITHOUT credentials (#9120)", () => { + it("GET /v1/public/decision-ledger/verify answers 200 with no Authorization header (the exact route that 401'd in prod)", async () => { + const app = createApp(); + const env = createTestEnv(); + const response = await app.request("/v1/public/decision-ledger/verify", {}, env); + expect(response.status).toBe(200); + const body = (await response.json()) as { ok: boolean; tipSeq: number; tipHash: string; totalCount: number }; + expect(body.ok).toBe(true); + // #9122: every response now carries the current tip + total row count for third-party checkpointing. + expect(body).toHaveProperty("tipSeq"); + expect(body).toHaveProperty("tipHash"); + expect(body).toHaveProperty("totalCount"); + }); + + it("GET /v1/public/decision-records/:owner/:repo/:pull answers 200 with no Authorization header and returns the published record verbatim + a digest that re-hashes to the exact stored value (#9123)", async () => { + const app = createApp(); + const env = createTestEnv(); + const { record, recordDigest } = await buildDecisionRecord({ + repoFullName: "acme/widgets", + pullNumber: 42, + headSha: "abc1234def", + baseSha: null, + action: "close", + reasonCode: "policy_close:contributor_cap", + configDigest: await contentDigest({ gatePack: "oss-anti-slop" }), + gatePack: "oss-anti-slop", + ciState: null, + modelId: null, + promptDigest: null, + aiConfidence: null, + salvageability: null, + }); + await persistDecisionRecord(env, record, recordDigest); + + const response = await app.request("/v1/public/decision-records/acme/widgets/42", {}, env); + expect(response.status).toBe(200); + const body = (await response.json()) as { record: typeof record; recordDigest: string }; + expect(body.recordDigest).toBe(recordDigest); + expect(body.record).toEqual(record); + // The digest is an honest commitment to the exact published body -- an observer can re-hash and compare. + expect(await contentDigest(body.record)).toBe(body.recordDigest); + }); + + it("GET /v1/public/decision-records/:owner/:repo/:pull answers 404 (not 401) for a PR with no persisted record yet", async () => { + const app = createApp(); + const env = createTestEnv(); + const response = await app.request("/v1/public/decision-records/acme/widgets/999", {}, env); + expect(response.status).toBe(404); + }); + + it("GET /v1/public/decision-records/:owner/:repo/:pull answers 400 (not 401) for a non-numeric pull segment", async () => { + const app = createApp(); + const env = createTestEnv(); + const response = await app.request("/v1/public/decision-records/acme/widgets/not-a-number", {}, env); + expect(response.status).toBe(400); + }); + + // #9120's own ask: "a companion test for each currently-exempt /v1/public/* route" -- table-driven, so a + // future public route that forgets its requiresApiToken exemption fails HERE, not in prod. Every path answers + // something other than 401 (the specific status varies -- 200/404/503 depending on seed data/feature flags -- + // the only invariant under test is that the auth gate itself never fires). + const exemptPublicRoutes: Array<[string, string]> = [ + ["subnet-interface", "/v1/public/subnet-interface"], + ["stats", "/v1/public/stats"], + ["decision-ledger/verify", "/v1/public/decision-ledger/verify"], + ["decision-records/:owner/:repo/:pull", "/v1/public/decision-records/acme/widgets/1"], + ["github/repos/:owner/:repo/stats", "/v1/public/github/repos/acme/widgets/stats"], + ["repos/:owner/:repo/badge.svg", "/v1/public/repos/acme/widgets/badge.svg"], + ["repos/:owner/:repo/badge.json", "/v1/public/repos/acme/widgets/badge.json"], + ["repos/:owner/:repo/quality", "/v1/public/repos/acme/widgets/quality"], + ]; + + it.each(exemptPublicRoutes)("%s answers without credentials (never 401)", async (_name, path) => { + const app = createApp(); + const env = createTestEnv(); + const response = await app.request(path, {}, env); + expect(response.status).not.toBe(401); + }); + + it("a genuinely protected /v1/* route (no exemption) DOES 401 with no credentials -- proves the table above is discriminating, not vacuously passing", async () => { + const app = createApp(); + const env = createTestEnv(); + const response = await app.request("/v1/internal/audit-labels", {}, env); + expect(response.status).toBe(401); + }); +}); diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index c8154bb40f..58f82dcc69 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -72,6 +72,7 @@ import { clearInstallationHealthRefreshCooldownForTest, clearWritePermissionDenialCooldownForTest, writePermissionDenialCooldownSizeForTest, + defaultDecisionRecordReasonCode, executeAgentMaintenanceActions, executeIssueMaintenanceActions, pendingActionToPlanned, @@ -81,6 +82,7 @@ import { type IssueActionExecutionContext, } from "../../src/services/agent-action-executor"; import type { PlannedAgentAction } from "../../src/settings/agent-actions"; +import type { DecisionRecord } from "../../src/review/decision-record"; import { STRUCTURED_CLOSE_REASONS_MAX_COUNT } from "../../src/settings/agent-execution"; import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules"; import { clearProcessLocalGlobalAgentFrozenCacheForTest, getGlobalContributorBlacklist, isGlobalAgentFrozen, setGlobalAgentFrozen, upsertGlobalModerationConfig, upsertPullRequestFile, upsertPullRequestFromGitHub } from "../../src/db/repositories"; @@ -100,6 +102,9 @@ function ctx(over: Partial = {}): AgentActionExecut agentPaused: false, agentDryRun: false, installationPermissions: { pull_requests: "write", contents: "write", issues: "write" }, + // #9134: required on every context — default keeps every pre-existing test byte-identical; tests that + // care about the decision-record side effect itself override this explicitly. + decisionRecord: { configDigest: "test-config-digest" }, ...over, }; } @@ -2298,3 +2303,99 @@ describe("pre-merge contributor-cap re-check (#7284-fix, TOCTOU race)", () => { expect(mergePullRequest).toHaveBeenCalled(); }); }); + +// #9134: six of seven executeAgentMaintenanceActions call sites wrote no decision record and no ledger row at +// all for a real, completed merge/close — silently biasing the risk-control calibration join away from +// exactly the closes a contributor is most likely to dispute (cap, review-nag) plus every approval-gated +// merge. The fix hoists record-building into this SHARED executor rather than patching each call site, so it +// cannot be bypassed by a new call site that forgets. This is the structural test for that guarantee: it +// exercises the executor DIRECTLY (not any one caller) across every action class, and asserts the +// decision_records/decision_ledger invariant holds for ALL of them at once -- a future call site that adds a +// new action class without updating this table would fail here rather than silently shipping unrecorded. +describe("decision record emission is structural, not per-call-site (#9134)", () => { + async function decisionRecordCount(env: Env): Promise<{ records: number; ledgerRows: number }> { + const records = (await env.DB.prepare("select count(*) as n from decision_records").first<{ n: number }>())?.n ?? 0; + const ledgerRows = (await env.DB.prepare("select count(*) as n from decision_ledger").first<{ n: number }>())?.n ?? 0; + return { records, ledgerRows }; + } + + const everyActionClass: PlannedAgentAction[] = [label, requestChanges, approve, merge, close, updateBranch, { actionClass: "assign", requiresApproval: false, reason: "auto-assign PR opener", assignee: "alice" }]; + + it.each(everyActionClass.map((action) => [action.actionClass, action] as const))( + "a completed %s action emits a decision record + ledger row IF AND ONLY IF it is merge/close", + async (actionClass, action) => { + const env = createTestEnv({}); + // "assign" has no entry in ctx()'s default autonomy map (unlike the other six classes) -- every OTHER + // "LIVE assign" test in this file grants it explicitly the same way. + const autonomyOverride = actionClass === "assign" ? { assign: "auto" as const } : undefined; + const outcomes = await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest" }, ...(autonomyOverride ? { autonomy: autonomyOverride } : {}) }), [action]); + expect(outcomes[0]?.outcome).toBe("completed"); + const { records, ledgerRows } = await decisionRecordCount(env); + const shouldRecord = actionClass === "merge" || actionClass === "close"; + expect(records).toBe(shouldRecord ? 1 : 0); + expect(ledgerRows).toBe(shouldRecord ? 1 : 0); + }, + ); + + it("a completed merge persists a record whose action/configDigest/reasonCode reflect the ctx, chained into the ledger", async () => { + const env = createTestEnv({}); + await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-abc", gatePack: "oss-anti-slop", reasonCode: "custom_reason" } }), [merge]); + const row = await env.DB.prepare("select action, reason_code, record_json from decision_records").first<{ action: string; reason_code: string; record_json: string }>(); + expect(row).toMatchObject({ action: "merge", reason_code: "custom_reason" }); + const parsed = JSON.parse(row!.record_json) as { configDigest: string; gatePack: string | null }; + expect(parsed.configDigest).toBe("cfg-abc"); + expect(parsed.gatePack).toBe("oss-anti-slop"); + const ledgerRow = await env.DB.prepare("select record_digest from decision_ledger").first<{ record_digest: string }>(); + expect(ledgerRow).toBeTruthy(); + }); + + it("afterPersist is called once with the persisted record's id + body — the hook a richer caller (e.g. a decision-replay input) uses to key its own sibling row", async () => { + const env = createTestEnv({}); + const afterPersist = vi.fn(async (_recordId: string, _record: DecisionRecord) => undefined); + await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest", afterPersist } }), [close]); + expect(afterPersist).toHaveBeenCalledTimes(1); + const [recordId, record] = afterPersist.mock.calls[0]!; + expect(recordId).toMatch(/^record:owner\/repo#7@/); + expect(record).toMatchObject({ repoFullName: "owner/repo", pullNumber: 7, action: "close" }); + }); + + it("a NON-completed merge/close (denied) emits NO decision record — only a real, completed mutation counts", async () => { + const env = createTestEnv({}); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest" }, agentPaused: true }), [merge]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect((await decisionRecordCount(env)).records).toBe(0); + }); + + it("`managedByCaller: true` is a deliberate opt-out — a caller that already records its own decision independently is never double-recorded", async () => { + const env = createTestEnv({}); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { managedByCaller: true } }), [merge]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect((await decisionRecordCount(env)).records).toBe(0); + }); + + it("a persist failure inside the hook never surfaces as the action's own outcome (best-effort, mirrors persistDecisionRecord's own posture)", async () => { + const env = createTestEnv({}); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const realPrepare = env.DB.prepare.bind(env.DB); + vi.spyOn(env.DB, "prepare").mockImplementation((sql: string) => { + if (sql.includes("INSERT INTO decision_records")) throw new Error("db down"); + return realPrepare(sql); + }); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest" } }), [merge]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalled(); + vi.restoreAllMocks(); + warn.mockRestore?.(); + }); +}); + +describe("defaultDecisionRecordReasonCode (#9134)", () => { + it("a policy-tagged close (any closeKind) publishes policy_close:; anything else publishes success", () => { + expect(defaultDecisionRecordReasonCode({ actionClass: "close", requiresApproval: false, reason: "r", closeKind: "contributor_cap" })).toBe("policy_close:contributor_cap"); + expect(defaultDecisionRecordReasonCode({ actionClass: "close", requiresApproval: false, reason: "r", closeKind: "review_nag" })).toBe("policy_close:review_nag"); + expect(defaultDecisionRecordReasonCode({ actionClass: "close", requiresApproval: false, reason: "r", closeKind: "heuristic" })).toBe("policy_close:heuristic"); + // A close with no closeKind at all, and a plain merge, both publish "success" -- there is no blocker to name. + expect(defaultDecisionRecordReasonCode({ actionClass: "close", requiresApproval: false, reason: "r" })).toBe("success"); + expect(defaultDecisionRecordReasonCode({ actionClass: "merge", requiresApproval: false, reason: "r", mergeMethod: "squash" })).toBe("success"); + }); +}); diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 06c626ade4..7f2b4b06aa 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -93,6 +93,8 @@ function ctx(over: Partial = {}): AgentActionExecut agentPaused: false, agentDryRun: false, installationPermissions: { contents: "write", pull_requests: "write", issues: "write" }, + // #9134: required on every context — default keeps every pre-existing test byte-identical. + decisionRecord: { configDigest: "test-config-digest" }, ...over, }; } @@ -166,6 +168,12 @@ describe("agent approval queue (#779)", () => { expect((await getPendingAgentAction(env, action.id))?.status).toBe("accepted"); const audit = await env.DB.prepare("select outcome, actor from audit_events where event_type = ?").bind("agent.pending_action.accepted").first<{ outcome: string; actor: string }>(); expect(audit).toMatchObject({ outcome: "completed", actor: "owner" }); + // #9134 REGRESSION: an approval-queue-accepted merge previously wrote NO decision record at all -- every + // approval-gated merge was invisible to the risk-control calibration join. + const decisionRecord = await env.DB.prepare("select action, reason_code from decision_records where repo_full_name = ? and pull_number = 7").bind("owner/repo").first<{ action: string; reason_code: string }>(); + expect(decisionRecord).toMatchObject({ action: "merge", reason_code: "success" }); + const ledgerRows = await env.DB.prepare("select count(*) as n from decision_ledger").first<{ n: number }>(); + expect(ledgerRows?.n).toBeGreaterThanOrEqual(1); }); it("accept supersedes a staged merge when the live head moved after staging (force-push fail-safe)", async () => { @@ -341,6 +349,11 @@ describe("agent approval queue (#779)", () => { expect(result.executionOutcome).toBe("completed"); const { closePullRequest: closeStillBlacklisted } = await import("../../src/github/pr-actions"); expect(closeStillBlacklisted).toHaveBeenCalledWith(env, 5, "owner/repo", 7); + // #9134 REGRESSION: an approval-queue-accepted close previously wrote NO decision record at all. + const decisionRecord = await env.DB.prepare("select action, reason_code from decision_records where repo_full_name = ? and pull_number = 7").bind("owner/repo").first<{ action: string; reason_code: string }>(); + expect(decisionRecord).toMatchObject({ action: "close", reason_code: "policy_close:blacklist" }); + const ledgerRows = await env.DB.prepare("select count(*) as n from decision_ledger").first<{ n: number }>(); + expect(ledgerRows?.n).toBeGreaterThanOrEqual(1); }); it("REGRESSION: accept rechecks blacklist closes against the effective global blacklist", async () => { diff --git a/test/unit/decision-record.test.ts b/test/unit/decision-record.test.ts index f91fa4b858..0e2ee980c8 100644 --- a/test/unit/decision-record.test.ts +++ b/test/unit/decision-record.test.ts @@ -9,7 +9,7 @@ import { sha256Hex, type DecisionRecord, } from "../../src/review/decision-record"; -import { appendDecisionLedger, LEDGER_GENESIS_HASH, loadDecisionRecordCollapsible, verifyDecisionLedger } from "../../src/review/decision-record"; +import { appendDecisionLedger, buildLedgerAnchorPayload, LEDGER_GENESIS_HASH, loadDecisionRecordCollapsible, loadPublicDecisionRecord, verifyDecisionLedger } from "../../src/review/decision-record"; import { createTestEnv } from "../helpers/d1"; // #8836: the digests are commitments a contributor can challenge — key-order invariance and unicode @@ -83,50 +83,114 @@ describe("buildDecisionRecord / persistDecisionRecord", () => { expect(withConf.aiConfidence).toBe(0); }); - it("persists with latest-finalize-wins per (target, head sha)", async () => { + it("a re-persist at the SAME (repo, pull, head) is a NEW revisioned row, never an overwrite (#9123)", async () => { const env = createTestEnv(); const first = await buildDecisionRecord(recordInput({ action: "merge", reasonCode: "success", decidedAt: "2026-07-26T00:00:00Z" } as never)); - await persistDecisionRecord(env, first.record, first.recordDigest); + const firstId = await persistDecisionRecord(env, first.record, first.recordDigest); + // The FIRST record for a head keeps the plain, no-suffix id every existing consumer (the replay CLI's + // documented extract query) already expects. + expect(firstId).toBe(`record:o/r#7@abc1234def`); const second = await buildDecisionRecord(recordInput({ action: "close", reasonCode: "policy_close:contributor_cap", decidedAt: "2026-07-26T01:00:00Z" } as never)); - await persistDecisionRecord(env, second.record, second.recordDigest); - const rows = await env.DB.prepare("SELECT action, reason_code, record_digest, record_json FROM decision_records").all<{ action: string; reason_code: string; record_digest: string; record_json: string }>(); - expect(rows.results).toHaveLength(1); - expect(rows.results![0]).toMatchObject({ action: "close", reason_code: "policy_close:contributor_cap", record_digest: second.recordDigest }); - // The stored JSON is the canonical form — re-digesting it reproduces the stored digest (the replay check). - expect(await sha256Hex(rows.results![0]!.record_json)).toBe(second.recordDigest); + const secondId = await persistDecisionRecord(env, second.record, second.recordDigest); + expect(secondId).toBe(`record:o/r#7@abc1234def:rev2`); + const rows = ( + await env.DB.prepare("SELECT id, action, reason_code, record_digest, record_json FROM decision_records ORDER BY id").all<{ id: string; action: string; reason_code: string; record_digest: string; record_json: string }>() + ).results!; + // BOTH rows exist -- the compounding bug this fixes was the second write silently overwriting the first. + expect(rows).toHaveLength(2); + const firstRow = rows.find((row) => row.id === firstId)!; + expect(firstRow).toMatchObject({ action: "merge", reason_code: "success", record_digest: first.recordDigest }); + // The FIRST record's preimage is still fully intact and re-hashes to the digest the ledger chained for it. + expect(await sha256Hex(firstRow.record_json)).toBe(first.recordDigest); + const secondRow = rows.find((row) => row.id === secondId)!; + expect(secondRow).toMatchObject({ action: "close", reason_code: "policy_close:contributor_cap", record_digest: second.recordDigest }); + // Both writes chained into the ledger as their OWN rows (two appends, not a rewrite of one). + const ledgerRecordIds = (await env.DB.prepare("SELECT record_id AS recordId FROM decision_ledger ORDER BY seq").all<{ recordId: string }>()).results!.map((row) => row.recordId); + expect(ledgerRecordIds).toEqual([firstId, secondId]); + }); + + it("a third persist at the same head keeps counting revisions (:rev2, :rev3, ...)", async () => { + const env = createTestEnv(); + const one = await buildDecisionRecord(recordInput({ decidedAt: "2026-07-26T00:00:00Z" } as never)); + const two = await buildDecisionRecord(recordInput({ decidedAt: "2026-07-26T01:00:00Z" } as never)); + const three = await buildDecisionRecord(recordInput({ decidedAt: "2026-07-26T02:00:00Z" } as never)); + const idOne = await persistDecisionRecord(env, one.record, one.recordDigest); + const idTwo = await persistDecisionRecord(env, two.record, two.recordDigest); + const idThree = await persistDecisionRecord(env, three.record, three.recordDigest); + expect([idOne, idTwo, idThree]).toEqual([`record:o/r#7@abc1234def`, `record:o/r#7@abc1234def:rev2`, `record:o/r#7@abc1234def:rev3`]); }); - it("a persist failure is swallowed (legibility must never break finalization)", async () => { + it("a persist failure is swallowed (legibility must never break finalization) and resolves null", async () => { const env = createTestEnv(); const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); vi.spyOn(env.DB, "prepare").mockImplementation(() => { throw new Error("db down"); }); const { record, recordDigest } = await buildDecisionRecord(recordInput()); - await expect(persistDecisionRecord(env, record, recordDigest)).resolves.toBeUndefined(); + await expect(persistDecisionRecord(env, record, recordDigest)).resolves.toBeNull(); + expect(warn).toHaveBeenCalled(); + vi.restoreAllMocks(); + }); + + it("an INSERT collision (a concurrent supersession racing the count-then-insert) retries and lands on the next revision", async () => { + const env = createTestEnv(); + const realPrepare = env.DB.prepare.bind(env.DB); + let insertAttempts = 0; + vi.spyOn(env.DB, "prepare").mockImplementation((sql: string) => { + if (sql.includes("INSERT INTO decision_records")) { + insertAttempts += 1; + if (insertAttempts === 1) { + return { bind: () => ({ run: async () => { throw new Error("UNIQUE constraint failed: decision_records.id"); } }) } as never; + } + } + return realPrepare(sql); + }); + const { record, recordDigest } = await buildDecisionRecord(recordInput()); + const id = await persistDecisionRecord(env, record, recordDigest); + expect(id).toBe(`record:o/r#7@abc1234def`); + expect(insertAttempts).toBe(2); + vi.restoreAllMocks(); + }); + + it("an exhausted INSERT retry budget rethrows, swallowed by the outer best-effort catch (resolves null, warns)", async () => { + const env = createTestEnv(); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const realPrepare = env.DB.prepare.bind(env.DB); + vi.spyOn(env.DB, "prepare").mockImplementation((sql: string) => { + if (sql.includes("INSERT INTO decision_records")) { + return { bind: () => ({ run: async () => { throw new Error("UNIQUE constraint failed: decision_records.id"); } }) } as never; + } + return realPrepare(sql); + }); + const { record, recordDigest } = await buildDecisionRecord(recordInput()); + await expect(persistDecisionRecord(env, record, recordDigest, 2)).resolves.toBeNull(); expect(warn).toHaveBeenCalled(); vi.restoreAllMocks(); }); }); describe("renderDecisionRecordSection", () => { - it("renders the claim + truncated digests; model line only when an AI review contributed", async () => { + it("renders the claim + FULL 64-hex digests (#9123 -- a prefix is not a commitment); model line only when an AI review contributed", async () => { const { record, recordDigest } = await buildDecisionRecord(recordInput()); const body = renderDecisionRecordSection(record, recordDigest); // The section body carries no
chrome — the bridge's UnifiedCollapsible renders the title. expect(body).not.toContain("
"); expect(body).toContain("`ci_readiness`"); - expect(body).toContain(record.configDigest.slice(0, 12)); - expect(body).toContain(recordDigest.slice(0, 12)); + // Full digests, not a 12-char prefix -- a challenger must be able to re-hash and compare the WHOLE value. + expect(body).toContain(`\`${record.configDigest}\``); + expect(record.configDigest).toHaveLength(64); + expect(body).toContain(`\`${recordDigest}\``); expect(body).not.toContain("**model**"); + // The head sha keeps its conventional 7-char git-abbreviation — a display convention, not a digest. + expect(body).toContain(`\`${record.headSha.slice(0, 7)}\``); const ai = await buildDecisionRecord(recordInput({ modelId: "claude-sonnet-5", promptDigest: "p".repeat(64), aiConfidence: 0.97 })); const aiBody = renderDecisionRecordSection(ai.record, ai.recordDigest); expect(aiBody).toContain("**model**: claude-sonnet-5"); - expect(aiBody).toContain("`pppppppppppp`"); + expect(aiBody).toContain(`\`${"p".repeat(64)}\``); expect(aiBody).toContain("**confidence**: 0.97"); - // Bounded: a record section must stay a small fixed-size block. - expect(aiBody.length).toBeLessThan(700); + // Bounded: a record section must stay a small fixed-size block even with three full 64-hex digests inline. + expect(aiBody.length).toBeLessThan(900); // Null pack/ci render nothing for those segments; a prompt digest without a model id renders "n/a". const bare = await buildDecisionRecord(recordInput({ gatePack: null, ciState: null, modelId: null, promptDigest: "q".repeat(64) })); @@ -134,7 +198,7 @@ describe("renderDecisionRecordSection", () => { expect(bareBody).not.toContain("**pack**"); expect(bareBody).not.toContain("**ci**"); expect(bareBody).toContain("**model**: n/a"); - expect(bareBody).toContain("`qqqqqqqqqqqq`"); + expect(bareBody).toContain(`\`${"q".repeat(64)}\``); // Model id present with NO prompt digest: the model line renders without a prompt segment. const modelOnly = await buildDecisionRecord(recordInput({ modelId: "claude-sonnet-5" })); expect(renderDecisionRecordSection(modelOnly.record, modelOnly.recordDigest)).toContain("**model**: claude-sonnet-5"); @@ -161,6 +225,58 @@ describe("loadDecisionRecordCollapsible", () => { }); }); +describe("loadPublicDecisionRecord (#9123)", () => { + it("returns the latest record verbatim + its digest for the public route; null when none exists yet", async () => { + const env = createTestEnv(); + expect(await loadPublicDecisionRecord(env, "o/r", 7)).toBeNull(); + const { record, recordDigest } = await buildDecisionRecord(recordInput()); + await persistDecisionRecord(env, record, recordDigest); + const published = await loadPublicDecisionRecord(env, "o/r", 7); + expect(published!.recordDigest).toBe(recordDigest); + // Verbatim -- every field survives, not the bounded/truncated markdown summary. + expect(published!.record).toEqual(record); + expect(published!.record.decidedAt).toBeTruthy(); + expect(published!.record.baseSha).toBe("base999"); + }); + + it("returns the LATEST revision after a supersession, not the superseded first record", async () => { + const env = createTestEnv(); + const first = await buildDecisionRecord(recordInput({ action: "merge", decidedAt: "2026-07-26T00:00:00Z" } as never)); + await persistDecisionRecord(env, first.record, first.recordDigest); + const second = await buildDecisionRecord(recordInput({ action: "close", decidedAt: "2026-07-26T01:00:00Z" } as never)); + await persistDecisionRecord(env, second.record, second.recordDigest); + const published = await loadPublicDecisionRecord(env, "o/r", 7); + expect(published!.recordDigest).toBe(second.recordDigest); + expect(published!.record.action).toBe("close"); + }); + + it("fails safe (null) on unreadable stored JSON rather than throwing", async () => { + const env = createTestEnv(); + const { record, recordDigest } = await buildDecisionRecord(recordInput()); + await persistDecisionRecord(env, record, recordDigest); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + await env.DB.prepare("UPDATE decision_records SET record_json = '{not json' WHERE pull_number = 7").run(); + expect(await loadPublicDecisionRecord(env, "o/r", 7)).toBeNull(); + expect(warn).toHaveBeenCalled(); + vi.restoreAllMocks(); + }); +}); + +describe("buildLedgerAnchorPayload (#9122)", () => { + it("returns exactly {seq, rowHash, at} from a tip, using a caller-supplied timestamp", () => { + const payload = buildLedgerAnchorPayload({ seq: 5, rowHash: "a".repeat(64) }, "2026-01-01T00:00:00.000Z"); + expect(payload).toEqual({ seq: 5, rowHash: "a".repeat(64), at: "2026-01-01T00:00:00.000Z" }); + }); + + it("defaults `at` to the current time when the caller omits it", () => { + const beforeMs = Date.now(); + const payload = buildLedgerAnchorPayload({ seq: 1, rowHash: LEDGER_GENESIS_HASH }); + expect(payload.seq).toBe(1); + expect(payload.rowHash).toBe(LEDGER_GENESIS_HASH); + expect(Date.parse(payload.at)).toBeGreaterThanOrEqual(beforeMs); + }); +}); + describe("decision ledger (#8837)", () => { const persist = async (env: Env, pull: number, action = "close") => { const { record, recordDigest } = await buildDecisionRecord(recordInput({ pullNumber: pull, action })); @@ -181,6 +297,52 @@ describe("decision ledger (#8837)", () => { expect(rows[2]!.prevHash).toBe(rows[1]!.rowHash); const verified = await verifyDecisionLedger(env); expect(verified).toMatchObject({ ok: true, checked: 3, nextAfterSeq: null }); + // #9122: the current global tip + total row count are always returned, so a third party can checkpoint. + expect(verified.tipSeq).toBe(3); + expect(verified.tipHash).toBe(rows[2]!.rowHash); + expect(verified.totalCount).toBe(3); + }); + + it("verifying a completely empty ledger returns ok:true with a zero tip, and skips the tail-truncation check (nothing to anchor against yet)", async () => { + const env = createTestEnv(); + const verified = await verifyDecisionLedger(env); + expect(verified).toEqual({ ok: true, checked: 0, nextAfterSeq: null, tipSeq: 0, tipHash: LEDGER_GENESIS_HASH, totalCount: 0 }); + }); + + it("TAIL TRUNCATION now breaks verify instead of passing clean (#9122): dropping the newest ledger rows leaves an orphaned decision_records tail", async () => { + const env = createTestEnv(); + // Force well-separated, deterministic timestamps via fake system time (not a post-hoc created_at UPDATE -- + // that would desync row_hash, which is computed OVER createdAt at write time, from the stored value and + // make every row after it look like a row_hash_mismatch instead of exercising the real, timing-independent + // tail-truncation bug this test pins). Five persists in a tight loop could otherwise land in the same + // millisecond on a fast in-memory harness, which would make the reconciliation's strict `created_at >` + // comparison flaky. + vi.useFakeTimers(); + try { + for (let i = 1; i <= 5; i += 1) { + vi.setSystemTime(new Date(2026, 0, 1, 0, 0, i)); + await persist(env, i); + } + } finally { + vi.useRealTimers(); + } + // Drop the newest 2 ledger rows (the most disputable, per the issue) -- their decision_records rows + // (pulls 4 and 5) are untouched, since the DELETE only targets decision_ledger. + await env.DB.prepare("DELETE FROM decision_ledger WHERE seq > 3").run(); + const verified = await verifyDecisionLedger(env); + expect(verified.ok).toBe(false); + expect(verified.break).toEqual({ kind: "short_tail", atSeq: 3 }); + expect(verified.checked).toBe(3); // rows 1-3 verify perfectly clean on their own + expect(verified.totalCount).toBe(3); // the ledger itself only knows about what's left + }); + + it("a genuinely short ledger with NO orphaned records still verifies clean (both sides of the reconciliation)", async () => { + const env = createTestEnv(); + await persist(env, 1); + await persist(env, 2); + const verified = await verifyDecisionLedger(env); + expect(verified.ok).toBe(true); + expect(verified.break).toBeUndefined(); }); it("verification is RESUMABLE: a full window returns the cursor, the next call continues from it", async () => { diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 19a1de8968..e1683172b2 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -2886,6 +2886,13 @@ describe("queue processors", () => { expect(mergeAudit?.n).toBe(0); // The close comment states the cap + current count (public, unlike the blacklist's static-only comment). expect(seen.comments.some((c) => c.includes("@farmer99") && c.includes("3 open pull requests") && c.includes("limit of 2"))).toBe(true); + // #9134 REGRESSION: this early cap-on-open close path used to write NO decision record and NO ledger row + // at all -- exactly the contributor-disputable close the issue flagged as biasing the risk-control + // calibration join. It must now emit both, by construction (the executor's shared record hook). + const decisionRecord = await env.DB.prepare("select action, reason_code from decision_records where repo_full_name = ? and pull_number = 55").bind("JSONbored/gittensory").first<{ action: string; reason_code: string }>(); + expect(decisionRecord).toMatchObject({ action: "close", reason_code: "policy_close:contributor_cap" }); + const ledgerRows = await env.DB.prepare("select count(*) as n from decision_ledger").first<{ n: number }>(); + expect(ledgerRows?.n).toBeGreaterThanOrEqual(1); }); it("pre-merge contributor-cap re-check (#7284-fix): a contributor well UNDER a configured cap merges normally through the full pipeline", async () => { diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 85febed06b..3b37d87a3f 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -420,6 +420,11 @@ describe("queue processors", () => { expect(seen.comments.some((c) => c.includes("chatty") && c.includes("4 times"))).toBe(true); const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.close'").first<{ n: number }>(); expect(closeAudit?.n).toBeGreaterThanOrEqual(1); + // #9134 REGRESSION: this review-nag close previously wrote NO decision record at all. + const decisionRecord = await env.DB.prepare("select action, reason_code from decision_records where repo_full_name = ? and pull_number = 203").bind("JSONbored/gittensory").first<{ action: string; reason_code: string }>(); + expect(decisionRecord).toMatchObject({ action: "close", reason_code: "policy_close:review_nag" }); + const ledgerRows = await env.DB.prepare("select count(*) as n from decision_ledger").first<{ n: number }>(); + expect(ledgerRows?.n).toBeGreaterThanOrEqual(1); }); it("REGRESSION (#review-nag-cross-pr-carryover): a contributor who exhausted their pings on PR A carries the count over to a BRAND-NEW PR B instead of resetting to a clean 0/maxPings slate", async () => { @@ -985,6 +990,13 @@ describe("queue processors", () => { expect(seen.closed).toBe(true); expect(seen.labels).toContain("too-chatty"); // #label-scoping: close: "auto" alone (no broad label: "auto") is sufficient for the label AND the close. + // #9134 REGRESSION: this monitored-mentions close (the @mention-triggered review-nag variant) previously + // wrote NO decision record at all -- same closeKind ("review_nag") as its ping-count sibling above, + // since planAgentMaintenanceActions tags both through the same reviewNagMatch field. + const decisionRecord = await env.DB.prepare("select action, reason_code from decision_records where repo_full_name = ? and pull_number = 305").bind("JSONbored/gittensory").first<{ action: string; reason_code: string }>(); + expect(decisionRecord).toMatchObject({ action: "close", reason_code: "policy_close:review_nag" }); + const ledgerRows = await env.DB.prepare("select count(*) as n from decision_ledger").first<{ n: number }>(); + expect(ledgerRows?.n).toBeGreaterThanOrEqual(1); }); it("REGRESSION (#review-nag-cross-pr-carryover): a contributor who exhausted their @-mention pings for ONE login on PR A carries that login's count over to a BRAND-NEW PR B", async () => {