diff --git a/docs/branch-review-records/44c2668e0a184a3856e8f614a4358bb33d033ffc179a15ef4b69248c4df83669.record.md b/docs/branch-review-records/44c2668e0a184a3856e8f614a4358bb33d033ffc179a15ef4b69248c4df83669.record.md new file mode 100644 index 0000000000..c391fcd9bb --- /dev/null +++ b/docs/branch-review-records/44c2668e0a184a3856e8f614a4358bb33d033ffc179a15ef4b69248c4df83669.record.md @@ -0,0 +1 @@ +| 2026-08-15 | claude/db-remediation-phase-0-wfaiyl | f390d6cd5faddd8b30320f60a082374688dc1f71 | Ledger reconciliation and push guard: final current-base merge | Verified parallel-cancellation and rewritten-history handling with isolated Git reproductions; merged latest required base with no conflicts or new P0-P2 findings | git diff --check; ledger-inbox self-test; ineffective-cancellation assertion; rewritten-history guard assertion; ledger-inbox; outstanding-issues; branch-review-ledger; ledger-write-discipline | diff --git a/docs/branch-review-records/7bd50bd9c4093cf764692231945db6e88a27e21ebb7545745fa66facf618d208.record.md b/docs/branch-review-records/7bd50bd9c4093cf764692231945db6e88a27e21ebb7545745fa66facf618d208.record.md new file mode 100644 index 0000000000..fe77034cc8 --- /dev/null +++ b/docs/branch-review-records/7bd50bd9c4093cf764692231945db6e88a27e21ebb7545745fa66facf618d208.record.md @@ -0,0 +1 @@ +| 2026-08-15 | PR #1978 | ef1bb2eac059d1c3ad0d4cae756bb069b5b8edb2 | review-and-fix | Fixed applied cancel-of-cancel acceptance and restored guard-push executable mode; verified rewritten-history and parallel-reconcile semantics; merged latest main | focused Vitest 39 passed, 1 Windows skip; guard self-tests; ledger and issue guards; format; manual adversarial review (CodeRabbit CLI unavailable) | diff --git a/docs/branch-review-records/7ce860ee652bd3f4e91c56cab26f002c8fd5e5b18eee189d01baaaa18ffd432c.record.md b/docs/branch-review-records/7ce860ee652bd3f4e91c56cab26f002c8fd5e5b18eee189d01baaaa18ffd432c.record.md new file mode 100644 index 0000000000..d2dc82c71f --- /dev/null +++ b/docs/branch-review-records/7ce860ee652bd3f4e91c56cab26f002c8fd5e5b18eee189d01baaaa18ffd432c.record.md @@ -0,0 +1 @@ +| 2026-08-15 | claude/db-remediation-phase-0-wfaiyl | becdb68610b27e7c38e9607c8bb0210911107581 | PR #1978 base sync | Merged main 6f7b7deefaf7e0cd062b748f18fc6ca8988093f6 into the reviewed PR head; merge tree was clean. | git merge-tree --write-tree exact-head main: clean; git diff --check; ledger guards. | diff --git a/docs/branch-review-records/b2482dceb2ee840589af0dfbe5f66b5f406c49c01ccd942a2c3f16bca8ae8ade.record.md b/docs/branch-review-records/b2482dceb2ee840589af0dfbe5f66b5f406c49c01ccd942a2c3f16bca8ae8ade.record.md new file mode 100644 index 0000000000..fd9e8eddb2 --- /dev/null +++ b/docs/branch-review-records/b2482dceb2ee840589af0dfbe5f66b5f406c49c01ccd942a2c3f16bca8ae8ade.record.md @@ -0,0 +1 @@ +| 2026-08-15 | claude/db-remediation-phase-0-wfaiyl | 1a8f7ae02c2bd7aa22e51edfb3b4a7b249ede578 | required base sync through main 17402395 | Approved — required main update merged; prior ledger guard remediation review remains applicable with no PR-path conflict | git diff --check; ci-change-scope self-test; ledger/inbox/outstanding/discipline guards passed | diff --git a/docs/branch-review-records/fb53209f910afc80ba5fb207e1a2f36d30bc14b98d5c069c3e7fa8b7f89edee4.record.md b/docs/branch-review-records/fb53209f910afc80ba5fb207e1a2f36d30bc14b98d5c069c3e7fa8b7f89edee4.record.md new file mode 100644 index 0000000000..9b39d22d9e --- /dev/null +++ b/docs/branch-review-records/fb53209f910afc80ba5fb207e1a2f36d30bc14b98d5c069c3e7fa8b7f89edee4.record.md @@ -0,0 +1 @@ +| 2026-08-15 | claude/db-remediation-phase-0-wfaiyl | b55f7a4b5c02c8a0ca8e59fd2d9edfb7eaac0234 | Required base sync through main d301d8f4 | approved | git diff --check; guard self-test; ledger and issue guards | diff --git a/scripts/guard-push.mjs b/scripts/guard-push.mjs index 4d754e4d62..6857372e93 100755 --- a/scripts/guard-push.mjs +++ b/scripts/guard-push.mjs @@ -116,17 +116,46 @@ export function pushedBranchNames(ranges, fallbackBranch = "") { return [...branches]; } -/** Exported for tests: existing branches compare from their remote tip; new - * branches compare from the PR merge base so newer main-only commits are out of - * scope for transaction guards that accept explicit base/head commits. */ -export function guardBaseForRange(range, cwd = PROJECT_ROOT) { - if (range.remoteSha && range.remoteSha !== ZERO_SHA) return range.remoteSha; +/** True when `ancestor` is reachable from `descendant`, i.e. the push fast-forwards. */ +function isAncestor(ancestor, descendant, cwd = PROJECT_ROOT) { + try { + execFileSync("git", ["merge-base", "--is-ancestor", ancestor, descendant], { cwd, stdio: "ignore" }); + return true; + } catch { + return false; + } +} + +/** Merge base with origin/main — the base a PR is actually evaluated against, and + * the same one CI passes as LEDGER_WRITE_BASE_SHA (.github/workflows/ci.yml). */ +function mainMergeBase(range, cwd = PROJECT_ROOT) { if (!tryGit(["rev-parse", "--verify", "--quiet", MAIN_REMOTE_REF], cwd)) return undefined; return tryGit(["merge-base", MAIN_REMOTE_REF, range.localSha], cwd); } +/** Exported for tests: a fast-forward push compares from its remote tip; a new + * branch, or one whose history was rewritten, compares from the PR merge base so + * newer main-only commits are out of scope for transaction guards that accept + * explicit base/head commits. + * + * The rewritten-history case matters: after a force-push the old remote tip is an + * abandoned line, so every request it carried reads as deleted and the ledger + * transaction guard can never pass — no matter how clean the rebuild is. Falling + * back to the merge base asks the question CI asks instead of an unanswerable one. */ +export function guardBaseForRange(range, cwd = PROJECT_ROOT) { + if (range.remoteSha && range.remoteSha !== ZERO_SHA) { + if (isAncestor(range.remoteSha, range.localSha, cwd)) return range.remoteSha; + return mainMergeBase(range, cwd); + } + return mainMergeBase(range, cwd); +} + export function changedFilesForRange(range, cwd = PROJECT_ROOT) { - const existingRemote = range.remoteSha && range.remoteSha !== ZERO_SHA; + // A rewritten history is treated like a new branch here for the same reason as + // guardBaseForRange: `..` is not the set of files this push + // actually introduces relative to main. + const existingRemote = + range.remoteSha && range.remoteSha !== ZERO_SHA && isAncestor(range.remoteSha, range.localSha, cwd); const hasOriginMain = !existingRemote && tryGit(["rev-parse", "--verify", "--quiet", MAIN_REMOTE_REF], cwd); const spec = existingRemote ? `${range.remoteSha}..${range.localSha}` diff --git a/scripts/ledger-inbox.mjs b/scripts/ledger-inbox.mjs index 7a856fd00c..05c74844e2 100644 --- a/scripts/ledger-inbox.mjs +++ b/scripts/ledger-inbox.mjs @@ -99,12 +99,43 @@ function mutationConflicts(requests) { return [...byIssue.entries()].filter(([, requestIds]) => requestIds.length > 1); } +/** + * Ids of requests that a previous reconciliation already applied. Used to tell a + * cancellation that lost a race (its target landed via another branch's batch) + * apart from one that names a request which never existed. + */ +function appliedRequestsById() { + try { + return new Map( + loadRequestsIn(APPLIED_DIR) + .map((entry) => entry.request) + .filter((request) => request?.id) + .map((request) => [request.id, request]), + ); + } catch { + return new Map(); + } +} + /** * Resolve immutable cancellation decisions before applying a pending request batch. * A cancellation request is itself retained in the applied audit trail, while the * targeted request is moved unchanged but deliberately not applied to the ledger. + * + * Parallel reconciliations are normal here: several branches queue requests and one + * of them reconciles first. A cancellation whose target was applied by that earlier + * batch has therefore *failed* — but throwing would wedge every consumer of this + * function (check:docs-links, check:ledger-write-discipline and reconcile itself) + * with no legal way out, because write discipline forbids deleting the queued file. + * So an already-applied target is reported loudly and skipped rather than fatal; a + * genuinely unknown target still throws. + * + * @param {Array} requests pending requests in the batch + * @param {{ appliedRequests?: Map, warn?: (message: string) => void }} [options] */ -export function planRequestBatch(requests) { +export function planRequestBatch(requests, options = {}) { + const applied = options.appliedRequests ?? appliedRequestsById(); + const warn = options.warn ?? ((message) => console.warn(message)); const byId = new Map(); for (const request of requests) { const problems = validateRequest(request); @@ -115,10 +146,27 @@ export function planRequestBatch(requests) { const cancelledIds = new Set(); const cancellations = []; + const ineffective = []; for (const request of requests) { if (request.action !== "cancel") continue; const target = byId.get(request.payload.requestId); if (!target) { + const appliedTarget = applied.get(request.payload.requestId); + if (appliedTarget) { + if (appliedTarget.action === "cancel") { + throw new Error(`cancel request ${request.id} cannot cancel another cancellation request`); + } + // Lost the race. Say so plainly: the correction this cancellation was + // protecting did NOT take effect, and whoever queued it needs to fix the + // row with a fresh update rather than assume the cancel did its job. + ineffective.push({ requestId: request.id, targetId: request.payload.requestId }); + warn( + `ledger inbox: cancel request ${request.id} did not take effect — its target ` + + `${request.payload.requestId} was already applied by an earlier reconciliation. ` + + `The cancellation is recorded but changed nothing; correct the affected row with a new update request.`, + ); + continue; + } throw new Error(`cancel request ${request.id} targets missing pending request ${request.payload.requestId}`); } if (target.action === "cancel") { @@ -145,7 +193,7 @@ export function planRequestBatch(requests) { ); } - return { active, cancellations, cancelledIds: [...cancelledIds] }; + return { active, cancellations, cancelledIds: [...cancelledIds], ineffectiveCancellations: ineffective }; } export function applyRequestBatch(markdown, requests) { diff --git a/tests/guard-push.test.ts b/tests/guard-push.test.ts index 2dffb6d0a5..82ae1b70d5 100644 --- a/tests/guard-push.test.ts +++ b/tests/guard-push.test.ts @@ -154,6 +154,49 @@ describe("push-range parsing", () => { expect(parsePushRanges("\n \n")).toHaveLength(0); }); + it("compares a fast-forward push from its remote tip", () => { + const { root, git } = gitFixture(); + git("update-ref", "refs/remotes/origin/main", "HEAD"); + git("switch", "--quiet", "-c", "feature"); + writeFileSync(join(root, "one.md"), "one\n"); + git("add", "one.md"); + git("commit", "--quiet", "-m", "one"); + const remoteSha = git("rev-parse", "HEAD"); + writeFileSync(join(root, "two.md"), "two\n"); + git("add", "two.md"); + git("commit", "--quiet", "-m", "two"); + const localSha = git("rev-parse", "HEAD"); + + // Ordinary push: the remote tip is reachable, so it stays the base and only + // the newly pushed commit is in scope. + expect(guardBaseForRange({ localSha, remoteSha }, root)).toBe(remoteSha); + expect(changedFilesForRange({ localSha, remoteSha }, root)).toEqual(["two.md"]); + }); + + // A force-push abandons the old remote tip. Comparing against it makes every + // file the discarded history carried look deleted, which is unanswerable for + // transaction guards; the merge base is the question CI actually asks. + it("falls back to the merge base when the remote tip was discarded by a force-push", () => { + const { root, git, baseSha } = gitFixture(); + git("update-ref", "refs/remotes/origin/main", "HEAD"); + git("switch", "--quiet", "-c", "feature"); + writeFileSync(join(root, "abandoned.md"), "abandoned\n"); + git("add", "abandoned.md"); + git("commit", "--quiet", "-m", "abandoned"); + const discardedSha = git("rev-parse", "HEAD"); + + git("reset", "--quiet", "--hard", baseSha); + writeFileSync(join(root, "rebuilt.md"), "rebuilt\n"); + git("add", "rebuilt.md"); + git("commit", "--quiet", "-m", "rebuilt"); + const localSha = git("rev-parse", "HEAD"); + + expect(discardedSha).not.toBe(localSha); + expect(guardBaseForRange({ localSha, remoteSha: discardedSha }, root)).toBe(baseSha); + // abandoned.md must not read as a deletion introduced by this push. + expect(changedFilesForRange({ localSha, remoteSha: discardedSha }, root)).toEqual(["rebuilt.md"]); + }); + it("keeps a Windows new-branch static command scoped to the PR side of an advanced main", () => { const { root, git, baseSha } = gitFixture(); git("switch", "--quiet", "-c", "feature"); diff --git a/tests/ledger-inbox-cancellation.test.ts b/tests/ledger-inbox-cancellation.test.ts new file mode 100644 index 0000000000..06948733c6 --- /dev/null +++ b/tests/ledger-inbox-cancellation.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it } from "vitest"; + +import { planRequestBatch } from "../scripts/ledger-inbox.mjs"; + +const UPDATE_ID = "11111111-1111-4111-8111-111111111111"; +const CANCEL_ID = "22222222-2222-4222-8222-222222222222"; +const APPLIED_ID = "33333333-3333-4333-8333-333333333333"; +const UNKNOWN_ID = "44444444-4444-4444-8444-444444444444"; + +function update(id: string, issueId = "#316") { + return { + version: 1, + id, + createdOn: "2026-08-14", + action: "update", + payload: { id: issueId, detail: `detail for ${issueId}` }, + }; +} + +function cancel(id: string, requestId: string) { + return { + version: 1, + id, + createdOn: "2026-08-14", + action: "cancel", + payload: { requestId, reason: "superseded" }, + }; +} + +// planRequestBatch reads the applied directory itself by default; every test here +// injects the requests so the assertions do not depend on the repo's real inbox. +function plan(requests: object[], appliedRequests: ReturnType[] = []) { + const warnings: string[] = []; + const result = planRequestBatch(requests, { + appliedRequests: new Map(appliedRequests.map((request) => [request.id, request])), + warn: (message: string) => warnings.push(message), + }); + return { ...result, warnings }; +} + +describe("ledger inbox cancellation planning", () => { + it("cancels a pending request in the same batch", () => { + const result = plan([update(UPDATE_ID), cancel(CANCEL_ID, UPDATE_ID)]); + + expect(result.active).toHaveLength(0); + expect(result.cancelledIds).toEqual([UPDATE_ID]); + expect(result.cancellations).toEqual([{ requestId: CANCEL_ID, targetId: UPDATE_ID, reason: "superseded" }]); + expect(result.warnings).toHaveLength(0); + expect(result.ineffectiveCancellations).toHaveLength(0); + }); + + // The race this exists for: another branch reconciled first and applied the + // target, so this cancellation can no longer take effect. Throwing here wedges + // check:docs-links, check:ledger-write-discipline and reconcile at once, with no + // legal way out because write discipline forbids deleting the queued request. + it("does not throw when the cancelled target was already applied elsewhere", () => { + const result = plan([update(UPDATE_ID), cancel(CANCEL_ID, APPLIED_ID)], [update(APPLIED_ID)]); + + expect(result.active).toHaveLength(1); + expect(result.cancelledIds).toHaveLength(0); + expect(result.ineffectiveCancellations).toEqual([{ requestId: CANCEL_ID, targetId: APPLIED_ID }]); + }); + + it("says loudly that an already-applied cancellation changed nothing", () => { + const result = plan([cancel(CANCEL_ID, APPLIED_ID)], [update(APPLIED_ID)]); + + expect(result.warnings).toHaveLength(1); + expect(result.warnings[0]).toContain(CANCEL_ID); + expect(result.warnings[0]).toContain(APPLIED_ID); + expect(result.warnings[0]).toContain("did not take effect"); + // The operator must be told to fix the row another way, not left assuming the + // cancellation did its job. + expect(result.warnings[0]).toContain("new update request"); + }); + + it("still throws when the target never existed", () => { + expect(() => plan([cancel(CANCEL_ID, UNKNOWN_ID)], [update(APPLIED_ID)])).toThrow( + /targets missing pending request/, + ); + }); + + it("still refuses to cancel another cancellation", () => { + expect(() => plan([update(UPDATE_ID), cancel(CANCEL_ID, UPDATE_ID), cancel(APPLIED_ID, CANCEL_ID)])).toThrow( + /cannot cancel another cancellation/, + ); + }); + + it("still refuses to cancel a cancellation that was already applied", () => { + expect(() => plan([cancel(CANCEL_ID, APPLIED_ID)], [cancel(APPLIED_ID, UPDATE_ID)])).toThrow( + /cannot cancel another cancellation/, + ); + }); + + it("still refuses to cancel the same request twice", () => { + expect(() => plan([update(UPDATE_ID), cancel(CANCEL_ID, UPDATE_ID), cancel(APPLIED_ID, UPDATE_ID)])).toThrow( + /cancelled more than once/, + ); + }); + + it("keeps requiring a decision when two mutations target one row", () => { + expect(() => plan([update(UPDATE_ID), update(CANCEL_ID)])).toThrow( + /multiple pending mutations require an explicit cancellation decision/, + ); + }); + + it("an ineffective cancellation does not resolve a competing-mutation conflict", () => { + // The cancellation lost its race, so both updates are still active and the + // conflict must still be raised rather than silently half-applied. + expect(() => + plan([update(UPDATE_ID), update(CANCEL_ID), cancel(UNKNOWN_ID, APPLIED_ID)], [update(APPLIED_ID)]), + ).toThrow(/multiple pending mutations require an explicit cancellation decision/); + }); +});