fix(ledger,drift): retry an unanchored tip, stop truncating the anchor log, and cap the third advisory (#9489, #9491) - #9514
Merged
Conversation
…r log, and cap the third advisory (#9489, #9491) 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 FAILURE at a QUIET tip unrecoverable. Rekor 429s at seq N, the ledger goes quiet, and every hourly tick then sees tipUnchanged and returns "unchanged", so the tip carries no valid external anchor indefinitely: exactly the unanchored window the feature exists to bound. It was also backend-blind, since the newest row won regardless of which backend wrote it, so git succeeding masked rekor failing at the same seq. The scheduler now asks per backend whether THIS rowHash has a successful anchor, and retries on the hourly tick when it does not. loadDecisionLedgerTip read COUNT(*) and the tip row as two statements under Promise.all. The seq/totalCount pair is precisely how a verifier detects truncation-and-rechaining, so a concurrent append between them produced totalCount = seq + 1 beside the OLD rowHash -- an internally inconsistent checkpoint that anchoring then signs and publishes to Rekor, unretractably, as a FALSE tamper signal about the maintainer's own ledger. One statement now reads both from a single snapshot. For a file between 1 MB and 100 MB the Contents API returns content:"" with encoding:"none", and because typeof "" === "string" the old code accepted that as the file's contents and rewrote the whole anchor log to a single line while reporting ok. At ~300 bytes per line hourly that lands 4-5 months out; git history keeps the commits but the file a skeptic is told to read shrinks, which is indistinguishable from tampering. It now refuses and records a failed attempt, leaving the log untouched and the gap visible on the public attempt log. The linked-issue satisfaction advisory was the one paid LLM call in its family with no per-PR commit cap -- ai_slop and ai_review both stop past auto_pause_after_reviewed_commits -- so a long-lived PR kept paying for a fresh assessment on every push after its siblings had stopped.
…over the new ledger paths queue-4 asserted /^ai-review-input:v6:/ against a literal, so the v6 -> v7 bump in #9513 broke it on main. It now asserts against AI_REVIEW_CACHE_INPUT_VERSION itself: the property that matters is that the fingerprint carries the CURRENT version, not which version that happens to be, so a future bump cannot break it again. Adds per-backend anchor-lookup coverage (a failed attempt is not anchored; one backend's success does not mask another's failure; a success at a different rowHash does not anchor this tip), the three Contents-API body shapes, and the commit-threshold cap regression. Two fail-open catches are annotated: an unreadable counter must not SUPPRESS the advisory, and an unreadable anchors table degrades to the pre-#9489 scheduling behaviour rather than forcing an anchor every tick.
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9514 +/- ##
===========================================
+ Coverage 75.51% 88.66% +13.15%
===========================================
Files 275 843 +568
Lines 58032 110190 +52158
Branches 6209 26227 +20018
===========================================
+ Hits 43820 97699 +53879
+ Misses 13942 11518 -2424
- Partials 270 973 +703
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three ledger-anchoring defects in the feature that exists to make the decision ledger externally verifiable, plus the third member of a sibling-guard family that never got its cap.
Also fixes
main, which is currently red — see below.Closes #9491
Partially addresses #9489 (three of four; see Scope)
mainis broken right nowtest/unit/queue-4.test.tsasserts/^ai-review-input:v6:/against a hardcoded literal, so thev6 → v7bump in #9513 broke it on merge. Verified by checking outorigin/maindirectly and running the test — it fails there, not just on my branch.It now asserts against
AI_REVIEW_CACHE_INPUT_VERSIONitself. The property that matters is "the fingerprint carries the current version", not which version that happens to be, so a future bump cannot break it again. That is the same class of brittleness as the constant the version fix was about.#9489a — a failed anchor at a quiet tip was never retried
loadLastLedgerAnchorAttemptdeliberately returns the newest attempt regardless of status, so the scheduler advances to newer tips rather than hammering a stale checkpoint. Sound on its own — but it made a failure at a quiet tip unrecoverable: Rekor 429s at seq N, the ledger goes quiet (a weekend), and every hourly tick then seestipUnchangedand returns"unchanged". The tip carries no valid external anchor indefinitely — precisely the unanchored window this feature exists to bound.It was also backend-blind: git succeeding at seq N masked rekor failing at the same seq, because the newest row won regardless of which backend wrote it.
The scheduler now asks, per backend, whether this exact rowHash has a successful anchor, and retries on the hourly tick when it does not.
otsis excluded from the required set — it is tracked-but-not-built (#9267), so requiring it would leave every tip permanently unanchored.#9489b — a race could publish a signed, false tamper signal
loadDecisionLedgerTipreadCOUNT(*)and the tip row as two statements underPromise.all. Theseq/totalCountpair is exactly how a verifier detects truncation-and-rechaining (perbuildLedgerAnchorPayload's own doc), so a concurrentappendDecisionLedgerlanding between them producedtotalCount = seq + 1beside the oldrowHash— an internally inconsistent checkpoint.Anchoring then signs that and publishes it to Rekor, unretractably, as a false tamper signal about the maintainer's own ledger. One statement now reads both from a single snapshot.
#9489c — the anchor log silently truncated past 1 MB
For a file between 1 MB and 100 MB the Contents API returns
content: ""withencoding: "none". Becausetypeof "" === "string", the old code accepted that as the file's contents and thePUTrewrote the whole log to a single line — while reportingstatus: "ok". At ~300 bytes per line on 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. It now refuses and records a failed attempt, so the log is untouched and the gap is visible on the public attempt log (#9271).
#9491 — the third sibling had no spend cap
The linked-issue satisfaction advisory is one of three paid LLM calls in the same family, and the other two already stop past a per-PR reviewed-commit cap —
ai_slopviacommitThresholdReached,ai_reviewviaauto_pause_after_reviewed_commits. This one had none, so a long-lived PR kept paying for a fresh assessment on every push after its siblings had stopped. Same drift class as #9399.Scope
#9489's fourth defect —
verifyDecisionLedger'sshort_tailbeing transiently false-positive between a record insert and its ledger append, and permanently blind to interior orphans once a newer row chains — is not here. It changes what a public verification endpoint reports about ledger integrity, and it interacts with #9474's retention-vs-verification question, so those two belong together in one deliberate change rather than split across PRs. #9489 stays open for it.Validation
npx tsc --noEmit,db:migrations:check(contiguous 0001..0197),selfhost:env-reference:check,git diff --check— all cleanRegressions: an hourly tick retries an unchanged tip no backend has anchored; a partially anchored tip still retries so one backend cannot mask another; a failed attempt does not count as anchored; a success at a different rowHash does not anchor this tip; the oversized-log refusal leaves the file untouched and records the failure; the advisory stops spending past the cap.
Invariants: a fully anchored unchanged tip stays quiet (no re-anchoring churn); an unanchored tip does not force a non-hourly tick; an empty ledger is still never anchored; omitting the new input preserves pre-#9489 behaviour exactly; all three Contents-API body shapes are distinguished — elided-with-encoding, elided-by-size, and a genuinely empty file that must still be appended to.
Two fail-open catches are annotated rather than tested: an unreadable counter must not suppress the advisory, and an unreadable anchors table degrades to the pre-#9489 scheduling behaviour rather than forcing an anchor on every tick.