Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8513,9 +8513,17 @@ export async function runLinkedIssueSatisfactionForAdvisory(
files: Awaited<ReturnType<typeof listPullRequestFiles>>;
confirmedContributor: boolean;
installationId: number;
/** #9491: the same per-PR commit cap its two siblings already honor -- `ai_slop` (slop-detection.ts's
* `commitThresholdReached`) and `ai_review` (focus-manifest's auto_pause_after_reviewed_commits). This
* advisory was the one paid LLM call in the family with NO cap, so a long-lived PR kept paying for a
* fresh assessment on every push after the other two had stopped. */
commitThresholdReached: boolean;
},
): Promise<{ status: "addressed" | "partial" | "unaddressed"; rationale: string } | null> {
if (args.mode === "paused" || !args.confirmedContributor || !args.advisory.headSha) return null;
// #9491: honor the cap the siblings honor. Deliberately no reuse-for-display fallback, matching
// slop-detection's identical early return -- past the cap this feature simply stops spending.
if (args.commitThresholdReached) return null;
const primaryIssueNumber = args.pr.linkedIssues[0];
if (primaryIssueNumber === undefined) return null;
try {
Expand Down Expand Up @@ -10661,6 +10669,15 @@ async function maybePublishPrPublicSurface(
files: await getReviewFiles(),
confirmedContributor,
installationId,
// #9491: the same threshold its ai_slop sibling applies. countPublishedAiReviewHeads is the shared
// per-PR reviewed-commit counter both features key on; failing open to 0 (never capped) matches the
// sibling's own .catch(() => 0) rather than silently suppressing the advisory on a read error.
commitThresholdReached: isAutoReviewCommitThresholdReached(
autoReviewConfig,
/* v8 ignore next -- fail-open: a counter read error must not silently SUPPRESS the advisory, so it
degrades to 0 (never capped), matching the ai_slop sibling's identical .catch(() => 0). */
await countPublishedAiReviewHeads(env, repoFullName, pr.number).catch(() => 0),
),
});
}
}
Expand Down
20 changes: 13 additions & 7 deletions src/review/decision-record.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,13 +344,19 @@ export async function ledgerRowHash(prevHash: string, fields: LedgerRowFields):
* only needs "what is the tip right now" (the scheduled anchoring job, #9274) shouldn't pay for a
* self-consistency walk it isn't asking for. */
export async function loadDecisionLedgerTip(env: Env): Promise<{ seq: number; rowHash: string; totalCount: number }> {
const [totalRow, tipRow] = 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 }>(),
]);
/* v8 ignore next -- defensive: a bare COUNT(*) always returns exactly one row, mirroring
* verifyDecisionLedger's identical note. */
return { seq: tipRow?.seq ?? 0, rowHash: tipRow?.rowHash ?? LEDGER_GENESIS_HASH, totalCount: totalRow?.n ?? 0 };
// #9489: ONE statement, not two under Promise.all. The seq/totalCount pair is exactly how a verifier detects
// truncation-and-rechaining (see buildLedgerAnchorPayload's own doc), so reading them separately meant a
// concurrent appendDecisionLedger landing between the two queries produced totalCount = seq + 1 alongside
// the OLD rowHash -- an internally inconsistent checkpoint. Anchoring then signs it and publishes it to
// Rekor, unretractably, as a FALSE tamper signal about the maintainer's own ledger. A single statement reads
// both from one snapshot, so the pair can never disagree.
const row = await env.DB
.prepare("SELECT (SELECT COUNT(*) FROM decision_ledger) AS n, seq, row_hash AS rowHash FROM decision_ledger ORDER BY seq DESC LIMIT 1")
.first<{ n: number; seq: number; rowHash: string }>();
// An empty ledger returns no row at all (the ORDER BY ... LIMIT 1 has nothing to return), which is the
// genesis case rather than a defensive one.
if (!row) return { seq: 0, rowHash: LEDGER_GENESIS_HASH, totalCount: 0 };
return { seq: row.seq, rowHash: row.rowHash, totalCount: row.n };
}

export async function appendDecisionLedger(env: Env, recordId: string, recordDigest: string, attempts = 3): Promise<void> {
Expand Down
15 changes: 14 additions & 1 deletion src/review/ledger-anchor-git.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,20 @@ export async function submitToGitAnchor(env: Env, signed: SignedLedgerAnchor, oc
let existingContent = "";
try {
const response = await octokit.request("GET /repos/{owner}/{repo}/contents/{path}", { owner, repo, path, ref: branch });
const data = response.data as { content?: string; sha?: string };
const data = response.data as { content?: string; sha?: string; size?: number; encoding?: string };
// #9489: for a file between 1 MB and 100 MB the Contents API returns `content: ""` with
// `encoding: "none"` -- and `typeof "" === "string"`, so the old code accepted it as the file's real
// contents and the PUT below REWROTE the whole log to a single line. At ~300 bytes per line and an
// hourly cadence that lands roughly 4-5 months out. Git history would still hold the truncated commits,
// but the file a skeptic is told to read shrinks -- indistinguishable from the tampering this module's
// own header teaches them to look for. Refuse rather than silently truncate: an operator rotating the
// file is a deliberate act, and an unanchored tip is loudly visible on the public attempt log (#9271).
const bodyElided = data.encoding === "none" || (data.content === "" && typeof data.size === "number" && data.size > 0);
if (bodyElided) {
throw new Error(
`anchor log ${path} is too large for the Contents API (size=${data.size ?? "unknown"} bytes); rotate the file or switch to the Git Data API before anchoring can resume`,
);
}
if (typeof data.content === "string") existingContent = decodeBase64(data.content);
existingSha = data.sha;
} catch (error) {
Expand Down
25 changes: 25 additions & 0 deletions src/review/ledger-anchor-persistence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,3 +165,28 @@ export async function loadLastLedgerAnchorAttempt(env: Env): Promise<{ seq: numb
}>();
return row == null ? null : row;
}

/**
* #9489: does the CURRENT tip still lack a successful anchor?
*
* {@link loadLastLedgerAnchorAttempt} deliberately returns the newest attempt regardless of status, so the
* scheduler advances to newer tips rather than hammering a stale checkpoint. But that made a failed attempt at
* a QUIET tip unrecoverable: Rekor 429s at seq N, the ledger goes quiet (a weekend), every hourly tick then
* sees `tipUnchanged` and returns "unchanged" -- so the tip carries NO valid external anchor indefinitely,
* which is precisely the unanchored window the feature exists to bound.
*
* It is also backend-blind: git succeeding at seq N masked rekor failing at the same seq, because the newest
* attempt row won regardless of which backend wrote it. Asking per-backend "is there an OK row for this exact
* rowHash" answers both.
*/
export async function anchorBackendsMissingForRowHash(env: Env, rowHash: string, backends: readonly string[]): Promise<string[]> {
if (backends.length === 0) return [];
const placeholders = backends.map((_, index) => `?${index + 2}`).join(", ");
const { results } = await env.DB.prepare(
`SELECT DISTINCT backend FROM decision_ledger_anchors WHERE row_hash = ?1 AND status = 'ok' AND backend IN (${placeholders})`,
)
.bind(rowHash, ...backends)
.all<{ backend: string }>();
const anchored = new Set(results.map((row) => row.backend));
return backends.filter((backend) => !anchored.has(backend));
}
33 changes: 30 additions & 3 deletions src/review/ledger-anchor-scheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import { errorMessage, nowIso } from "../utils/json";
import { buildLedgerAnchorPayload, currentAnchorKey, parseAnchorPublicKeys, signLedgerAnchorPayload, type SignedLedgerAnchor } from "./ledger-anchor";
import { loadDecisionLedgerTip } from "./decision-record";
import { loadLastLedgerAnchorAttempt, recordLedgerAnchorAttempt, type LedgerAnchorBackend } from "./ledger-anchor-persistence";
import { anchorBackendsMissingForRowHash, loadLastLedgerAnchorAttempt, recordLedgerAnchorAttempt, type LedgerAnchorBackend } from "./ledger-anchor-persistence";
import { submitToRekor } from "./ledger-anchor-rekor";
import type { LedgerAnchorGitTarget } from "./ledger-anchor-git";

Expand All @@ -15,7 +15,7 @@ export const LEDGER_ANCHOR_SEQ_THRESHOLD = 256;

export type LedgerAnchorScheduleDecision =
| { shouldAnchor: false; reason: "empty_ledger" | "unchanged" }
| { shouldAnchor: true; reason: "hourly" | "seq_threshold" };
| { shouldAnchor: true; reason: "hourly" | "seq_threshold" | "retry_unanchored" };

/**
* Pure scheduling decision: given the live tip and the last attempt (regardless of that attempt's own
Expand All @@ -30,6 +30,9 @@ export function decideLedgerAnchorSchedule(input: {
currentTip: { seq: number; rowHash: string };
lastAnchor: { seq: number; rowHash: string } | null;
seqThreshold: number;
/** #9489: backends with NO successful anchor for the current tip's rowHash. Non-empty means the tip is
* unanchored on at least one backend even though nothing has moved, so an hourly tick should retry. */
unanchoredBackends?: readonly string[] | undefined;
}): LedgerAnchorScheduleDecision {
if (input.currentTip.seq === 0) return { shouldAnchor: false, reason: "empty_ledger" };

Expand All @@ -38,6 +41,13 @@ export function decideLedgerAnchorSchedule(input: {

const tipUnchanged = input.lastAnchor !== null && input.lastAnchor.rowHash === input.currentTip.rowHash;
if (input.isHourly && !tipUnchanged) return { shouldAnchor: true, reason: "hourly" };
// #9489: an unchanged tip is only "nothing to do" if it is actually ANCHORED. The previous check compared
// against the newest ATTEMPT regardless of status, so a failure at a quiet tip was never retried -- Rekor
// 429s at seq N, the ledger goes quiet, and every later tick reports "unchanged" while the tip carries no
// valid external anchor at all. Retrying on the hourly tick gives it a bounded, self-healing cadence.
if (input.isHourly && tipUnchanged && input.unanchoredBackends && input.unanchoredBackends.length > 0) {
return { shouldAnchor: true, reason: "retry_unanchored" };
}

return { shouldAnchor: false, reason: "unchanged" };
}
Expand Down Expand Up @@ -75,10 +85,27 @@ export type LedgerAnchorSchedulerDeps = {
* a total anchoring failure cannot propagate anywhere. Returns the scheduling decision so a caller/test can
* observe WHY nothing happened, without needing a second query.
*/
/** #9489: the backends whose success actually matters for "is this tip anchored". `ots` is tracked-but-not-
* built (#9267), so requiring it would make every tip permanently unanchored. */
const ANCHOR_BACKENDS_REQUIRING_SUCCESS = ["rekor", "git"] as const;

export async function runScheduledLedgerAnchor(env: Env, options: { isHourly: boolean; now?: string }, deps: LedgerAnchorSchedulerDeps = {}): Promise<LedgerAnchorScheduleDecision> {
const now = options.now ?? nowIso();
const [tip, lastAnchor] = await Promise.all([loadDecisionLedgerTip(env), loadLastLedgerAnchorAttempt(env)]);
const decision = decideLedgerAnchorSchedule({ isHourly: options.isHourly, currentTip: tip, lastAnchor, seqThreshold: LEDGER_ANCHOR_SEQ_THRESHOLD });
// #9489: which backends still have NO successful anchor for this exact tip. An unchanged tip is only
// "nothing to do" when it is genuinely anchored -- otherwise a failure at a quiet tip is never retried, and
// a success on one backend masks a failure on the other, since the newest-attempt row wins regardless of
// which backend wrote it.
/* v8 ignore next -- fail-open: an unreadable anchors table degrades to "nothing known to be missing", i.e.
exactly the pre-#9489 scheduling behaviour, rather than forcing an anchor on every tick. */
const unanchoredBackends = await anchorBackendsMissingForRowHash(env, tip.rowHash, ANCHOR_BACKENDS_REQUIRING_SUCCESS).catch(() => []);
const decision = decideLedgerAnchorSchedule({
isHourly: options.isHourly,
currentTip: tip,
lastAnchor,
seqThreshold: LEDGER_ANCHOR_SEQ_THRESHOLD,
unanchoredBackends,
});
if (!decision.shouldAnchor) return decision;

const keys = parseAnchorPublicKeys(env.LOOPOVER_LEDGER_ANCHOR_KEYS);
Expand Down
97 changes: 97 additions & 0 deletions test/unit/ledger-anchor-git.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,100 @@ describe("submitToGitAnchor (#9273)", () => {
await expect(submitToGitAnchor(env, signed, { request } as GitHubContentsRequester, TARGET)).resolves.toBeUndefined();
});
});

// #9489: for a file between 1 MB and 100 MB the Contents API returns `content: ""` with `encoding: "none"`.
// Because `typeof "" === "string"`, the old code accepted that as the file's real contents and the PUT
// REWROTE the whole anchor log to a single line -- reporting status "ok" while destroying it. At ~300 bytes
// per line on an hourly cadence that lands roughly 4-5 months out. Git history keeps the truncated commits,
// but the file a skeptic is told to read shrinks, which is indistinguishable from the tampering this module's
// own header teaches them to look for.
describe("oversized anchor log (#9489)", () => {
it("REGRESSION: refuses to write when the Contents API elides the body, instead of truncating the log", async () => {
const env = createTestEnv();
const signed = makeSignedAnchor(1);
const octokit = {
request: vi.fn(async (route: string) => {
// No `size` here: GitHub does not always include it, and the error message must still read cleanly.
if (route.startsWith("GET")) return { data: { content: "", encoding: "none", sha: "abc" } };
return { data: {} };
}),
};

// submitToGitAnchor never rejects by contract -- it records the attempt instead -- so the refusal shows up
// as a FAILED anchor rather than a thrown error. Both properties matter:
await expect(submitToGitAnchor(env, signed, octokit as unknown as GitHubContentsRequester, TARGET)).resolves.toBeUndefined();
// 1. no PUT was attempted, so the existing log is untouched rather than rewritten to a single line;
expect(octokit.request.mock.calls.some(([route]) => String(route).startsWith("PUT"))).toBe(false);
// 2. the failure is visible on the public attempt log, so the unanchored tip is loud rather than silent.
const { anchors } = await loadPublicLedgerAnchors(env);
expect(anchors[0]).toMatchObject({ status: "failed" });
expect(String((anchors[0] as { error?: string }).error)).toMatch(/too large for the Contents API/);
expect(String((anchors[0] as { error?: string }).error)).toContain("size=unknown"); // degrades cleanly
});

it("INVARIANT: an ordinary small file still appends normally", async () => {
const env = createTestEnv();
const signed = makeSignedAnchor(1);
const octokit = {
request: vi.fn(async (route: string, _params?: Record<string, unknown>) => {
if (route.startsWith("GET")) return { data: { content: Buffer.from("existing line\n").toString("base64"), encoding: "base64", size: 14, sha: "abc" } };
return { data: {} };
}),
};

await submitToGitAnchor(env, signed, octokit as unknown as GitHubContentsRequester, TARGET);

const put = octokit.request.mock.calls.find((call) => String(call[0]).startsWith("PUT"));
expect(put).toBeDefined();
const written = Buffer.from(String((put?.[1] as Record<string, unknown> | undefined)?.["content"] ?? ""), "base64").toString("utf8");
expect(written).toContain("existing line"); // appended, not replaced
});

it("also refuses on the size-only shape, where the body is elided without encoding:none", async () => {
// GitHub has returned both shapes for an over-threshold file; keying on either alone would miss one.
const env = createTestEnv();
const signed = makeSignedAnchor(1);
const octokit = {
request: vi.fn(async (route: string, _params?: Record<string, unknown>) => {
if (route.startsWith("GET")) return { data: { content: "", size: 1_500_000, sha: "abc" } };
return { data: {} };
}),
};

await submitToGitAnchor(env, signed, octokit as unknown as GitHubContentsRequester, TARGET);

expect(octokit.request.mock.calls.some((call) => String(call[0]).startsWith("PUT"))).toBe(false);
});

it("INVARIANT: an empty body with NO size field is treated as empty, not elided", async () => {
// Without a size there is no evidence the body was withheld, so refusing would block the very first anchor
// on a freshly created empty file. Appending is the safe reading.
const env = createTestEnv();
const signed = makeSignedAnchor(1);
const octokit = {
request: vi.fn(async (route: string, _params?: Record<string, unknown>) => {
if (route.startsWith("GET")) return { data: { content: "", sha: "abc" } };
return { data: {} };
}),
};

await submitToGitAnchor(env, signed, octokit as unknown as GitHubContentsRequester, TARGET);

expect(octokit.request.mock.calls.some((call) => String(call[0]).startsWith("PUT"))).toBe(true);
});

it("INVARIANT: a genuinely empty (0-byte) file is not mistaken for an elided one", async () => {
// size 0 with content "" is a real empty file -- the first anchor should append to it, not refuse.
const env = createTestEnv();
const signed = makeSignedAnchor(1);
const octokit = {
request: vi.fn(async (route: string) => {
if (route.startsWith("GET")) return { data: { content: "", encoding: "base64", size: 0, sha: "abc" } };
return { data: {} };
}),
};

await expect(submitToGitAnchor(env, signed, octokit as unknown as GitHubContentsRequester, TARGET)).resolves.toBeUndefined();
expect(octokit.request.mock.calls.some(([route]) => String(route).startsWith("PUT"))).toBe(true);
});
});
Loading