Skip to content

Commit 5a4f364

Browse files
777genius777genius
committed
fix(review): fence publication on command ledger
1 parent e425e0c commit 5a4f364

6 files changed

Lines changed: 149 additions & 59 deletions

File tree

‎__tests__/unit/github/ledger.test.ts‎

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
import { ReviewLedger } from '../../../src/github/ledger';
1+
import {
2+
ReviewLedger,
3+
commandLedgerWatermark,
4+
} from '../../../src/github/ledger';
25
import { GitHubClient } from '../../../src/github/client';
36

47
function makeClient(
@@ -23,6 +26,59 @@ function makeClient(
2326
}
2427

2528
describe('ReviewLedger', () => {
29+
it('derives a stable watermark from accepted command comment ids', () => {
30+
expect(
31+
commandLedgerWatermark({
32+
version: 1,
33+
repo: 'test-owner/test-repo',
34+
pr: 123,
35+
entries: [
36+
{
37+
action: 'skip',
38+
fingerprint: 'abc123',
39+
severity: 'major',
40+
actor: 'maintainer',
41+
actorRole: 'maintain',
42+
parentCommentId: 99,
43+
commandCommentId: 101,
44+
createdAt: '2026-05-01T00:00:00.000Z',
45+
},
46+
{
47+
action: 'unskip',
48+
fingerprint: 'abc123',
49+
severity: 'major',
50+
actor: 'maintainer',
51+
actorRole: 'maintain',
52+
parentCommentId: 99,
53+
commandCommentId: 105,
54+
createdAt: '2026-05-01T00:01:00.000Z',
55+
},
56+
],
57+
})
58+
).toBe(105);
59+
});
60+
61+
it('uses the parent id for legacy signed entries without a command id', () => {
62+
expect(
63+
commandLedgerWatermark({
64+
version: 1,
65+
repo: 'test-owner/test-repo',
66+
pr: 123,
67+
entries: [
68+
{
69+
action: 'skip',
70+
fingerprint: 'abc123',
71+
severity: 'major',
72+
actor: 'maintainer',
73+
actorRole: 'maintain',
74+
parentCommentId: 99,
75+
createdAt: '2026-05-01T00:00:00.000Z',
76+
},
77+
],
78+
})
79+
).toBe(99);
80+
});
81+
2682
it('creates a signed ledger comment and can load it back', async () => {
2783
const { client, octokit } = makeClient([]);
2884
const ledger = new ReviewLedger(client, 'test-secret');

‎dist/index.js‎

Lines changed: 41 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -32637,6 +32637,18 @@ var import_crypto4 = require("crypto");
3263732637
var LEDGER_MARKER = "reviewrouter-ledger:v1";
3263832638
var LEDGER_RE = /<!--\s*reviewrouter-ledger:v1\s+payload=([A-Za-z0-9_-]+)\s+signature=([a-f0-9]{64})\s*-->/;
3263932639
var MAX_LEDGER_ENTRIES = 200;
32640+
function commandLedgerWatermark(payload) {
32641+
return Math.max(
32642+
0,
32643+
...payload.entries.map((entry) => {
32644+
const value = entry.commandCommentId ?? entry.parentCommentId;
32645+
if (!Number.isSafeInteger(value) || value <= 0) {
32646+
throw new Error("review_router_command_ledger_watermark_invalid");
32647+
}
32648+
return value;
32649+
})
32650+
);
32651+
}
3264032652
var ReviewLedger = class {
3264132653
constructor(client, secret, dryRun = false) {
3264232654
this.client = client;
@@ -71093,19 +71105,24 @@ var GitHubReviewRevisionGuard = class {
7109371105
}
7109471106
};
7109571107
var FreshGitHubLifecycleInventory = class {
71096-
loader;
71097-
constructor(client) {
71108+
constructor(client, ledger) {
71109+
this.ledger = ledger;
7109871110
this.loader = new ReviewThreadInventoryLoader(client);
7109971111
}
71112+
loader;
7110071113
async loadCurrent(query) {
71101-
return mapFreshInventory(
71102-
await this.loader.load(query.scope.pullRequestNumber),
71103-
query.scope.reviewedHeadSha
71104-
);
71114+
const [raw, ledger] = await Promise.all([
71115+
this.loader.load(query.scope.pullRequestNumber),
71116+
this.ledger.load(query.scope.pullRequestNumber)
71117+
]);
71118+
return mapFreshInventory(raw, query.scope.reviewedHeadSha, ledger);
7110571119
}
7110671120
async loadForPrompt(pullRequestNumber, expectedHeadSha) {
71107-
const raw = await this.loader.load(pullRequestNumber);
71108-
const inventory = mapFreshInventory(raw, expectedHeadSha);
71121+
const [raw, ledger] = await Promise.all([
71122+
this.loader.load(pullRequestNumber),
71123+
this.ledger.load(pullRequestNumber)
71124+
]);
71125+
const inventory = mapFreshInventory(raw, expectedHeadSha, ledger);
7110971126
return Object.freeze({
7111071127
inventory,
7111171128
promptTargets: Object.freeze([
@@ -71115,7 +71132,7 @@ var FreshGitHubLifecycleInventory = class {
7111571132
});
7111671133
}
7111771134
};
71118-
function mapFreshInventory(raw, expectedHeadSha) {
71135+
function mapFreshInventory(raw, expectedHeadSha, ledger) {
7111971136
if (raw.failed) {
7112071137
throw new Error("review_action_v2_lifecycle_inventory_unavailable");
7112171138
}
@@ -71138,13 +71155,10 @@ function mapFreshInventory(raw, expectedHeadSha) {
7113871155
if (new Set(rawTargets.map(({ target }) => target.targetId)).size !== rawTargets.length) {
7113971156
throw new Error("review_action_v2_lifecycle_inventory_duplicate_target");
7114071157
}
71141-
const missingDatabaseId = rawTargets.some(
71142-
({ target }) => target.parentCommentDatabaseId === void 0
71143-
);
71144-
const warnings = [
71145-
...raw.warnings,
71146-
...missingDatabaseId ? ["review thread comment watermark is incomplete"] : []
71147-
].sort();
71158+
if (!ledger.valid) {
71159+
throw new Error("review_action_v2_command_ledger_unavailable");
71160+
}
71161+
const warnings = [...raw.warnings].sort();
7114871162
const targets = rawTargets.map(({ target, manual }) => ({
7114971163
targetId: target.targetId,
7115071164
threadId: target.threadId,
@@ -71169,16 +71183,12 @@ function mapFreshInventory(raw, expectedHeadSha) {
7116971183
}
7117071184
} : {}
7117171185
}));
71172-
const commandLedgerWatermark = String(
71173-
Math.max(
71174-
0,
71175-
...rawTargets.map(({ target }) => target.parentCommentDatabaseId ?? 0)
71176-
)
71177-
);
71186+
const commandWatermark = commandLedgerWatermark(ledger.payload);
71187+
const commandLedgerWatermarkValue = String(commandWatermark);
7117871188
const lifecycleStateHash = sha2568(
7117971189
canonicalJson9({
71180-
commandLedgerWatermark,
71181-
complete: !missingDatabaseId,
71190+
commandLedgerWatermark: commandLedgerWatermarkValue,
71191+
complete: true,
7118271192
loadedForHeadSha,
7118371193
targets,
7118471194
warnings
@@ -71188,8 +71198,8 @@ function mapFreshInventory(raw, expectedHeadSha) {
7118871198
inventoryVersion: "review_lifecycle_inventory.v1",
7118971199
loadedForHeadSha,
7119071200
lifecycleStateHash,
71191-
commandLedgerWatermark,
71192-
complete: !missingDatabaseId,
71201+
commandLedgerWatermark: commandLedgerWatermarkValue,
71202+
complete: true,
7119371203
warnings: Object.freeze(warnings),
7119471204
targets: Object.freeze(targets)
7119571205
});
@@ -71303,7 +71313,7 @@ var BuildCurrentReviewProjection = class {
7130371313
occurrences = applyPlacementDecisions(occurrences, presentation.placements);
7130471314
const coverageOnly = coverage.state === "partial" /* Partial */;
7130571315
const allClear = !coverageOnly && canClaimAllClear(coverage, inventory, occurrences, gate);
71306-
const lifecycleFacts = coverageOnly ? [] : buildLifecycleFacts(
71316+
const lifecycleFacts = buildLifecycleFacts(
7130771317
inventory,
7130871318
lifecycleDecisions,
7130971319
command.priorLineageHints,
@@ -73591,7 +73601,10 @@ var ProductionT0ReviewRunner = class {
7359173601
if (pr2.baseSha.toLowerCase() !== authorization.facts.baseSha || pr2.headSha.toLowerCase() !== authorization.facts.headSha) {
7359273602
return { outcome: "superseded" /* Superseded */ };
7359373603
}
73594-
const lifecycleInventory = new FreshGitHubLifecycleInventory(github);
73604+
const lifecycleInventory = new FreshGitHubLifecycleInventory(
73605+
github,
73606+
new ReviewLedger(github, process.env.REVIEW_ROUTER_LEDGER_KEY)
73607+
);
7359573608
const initialLifecycle = await lifecycleInventory.loadForPrompt(
7359673609
pr2.number,
7359773610
authorization.facts.headSha

‎dist/index.js.map‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/github/ledger.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,19 @@ export interface ActiveLedgerSkip extends LedgerEntry {
5151
action: 'skip';
5252
}
5353

54+
export function commandLedgerWatermark(payload: ReviewLedgerPayload): number {
55+
return Math.max(
56+
0,
57+
...payload.entries.map((entry) => {
58+
const value = entry.commandCommentId ?? entry.parentCommentId;
59+
if (!Number.isSafeInteger(value) || value <= 0) {
60+
throw new Error('review_router_command_ledger_watermark_invalid');
61+
}
62+
return value;
63+
})
64+
);
65+
}
66+
5467
export class ReviewLedger {
5568
constructor(
5669
private readonly client: GitHubClient,

‎src/review-orchestration/infrastructure/github-review-state-adapter.ts‎

Lines changed: 31 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
import { createHash } from 'crypto';
22
import { GitHubClient } from '../../github/client';
3+
import {
4+
commandLedgerWatermark,
5+
type LoadedLedger,
6+
type ReviewLedger,
7+
} from '../../github/ledger';
38
import {
49
ReviewThreadInventoryLoader,
510
type ReviewThreadInventory,
@@ -91,17 +96,21 @@ export class GitHubReviewRevisionGuard implements ReviewRevisionGuardPort {
9196
export class FreshGitHubLifecycleInventory implements CurrentLifecycleInventoryPort {
9297
private readonly loader: ReviewThreadInventoryLoader;
9398

94-
constructor(client: GitHubClient) {
99+
constructor(
100+
client: GitHubClient,
101+
private readonly ledger: ReviewLedger
102+
) {
95103
this.loader = new ReviewThreadInventoryLoader(client);
96104
}
97105

98106
async loadCurrent(query: {
99107
readonly scope: ReviewProjectionScope;
100108
}): Promise<CurrentLifecycleInventory> {
101-
return mapFreshInventory(
102-
await this.loader.load(query.scope.pullRequestNumber),
103-
query.scope.reviewedHeadSha
104-
);
109+
const [raw, ledger] = await Promise.all([
110+
this.loader.load(query.scope.pullRequestNumber),
111+
this.ledger.load(query.scope.pullRequestNumber),
112+
]);
113+
return mapFreshInventory(raw, query.scope.reviewedHeadSha, ledger);
105114
}
106115

107116
async loadForPrompt(
@@ -111,8 +120,11 @@ export class FreshGitHubLifecycleInventory implements CurrentLifecycleInventoryP
111120
readonly inventory: CurrentLifecycleInventory;
112121
readonly promptTargets: readonly LifecycleTarget[];
113122
}> {
114-
const raw = await this.loader.load(pullRequestNumber);
115-
const inventory = mapFreshInventory(raw, expectedHeadSha);
123+
const [raw, ledger] = await Promise.all([
124+
this.loader.load(pullRequestNumber),
125+
this.ledger.load(pullRequestNumber),
126+
]);
127+
const inventory = mapFreshInventory(raw, expectedHeadSha, ledger);
116128
return Object.freeze({
117129
inventory,
118130
promptTargets: Object.freeze([
@@ -125,7 +137,8 @@ export class FreshGitHubLifecycleInventory implements CurrentLifecycleInventoryP
125137

126138
function mapFreshInventory(
127139
raw: ReviewThreadInventory,
128-
expectedHeadSha: string
140+
expectedHeadSha: string,
141+
ledger: LoadedLedger
129142
): CurrentLifecycleInventory {
130143
if (raw.failed) {
131144
throw new Error('review_action_v2_lifecycle_inventory_unavailable');
@@ -154,15 +167,10 @@ function mapFreshInventory(
154167
throw new Error('review_action_v2_lifecycle_inventory_duplicate_target');
155168
}
156169

157-
const missingDatabaseId = rawTargets.some(
158-
({ target }) => target.parentCommentDatabaseId === undefined
159-
);
160-
const warnings = [
161-
...raw.warnings,
162-
...(missingDatabaseId
163-
? ['review thread comment watermark is incomplete']
164-
: []),
165-
].sort();
170+
if (!ledger.valid) {
171+
throw new Error('review_action_v2_command_ledger_unavailable');
172+
}
173+
const warnings = [...raw.warnings].sort();
166174
const targets = rawTargets.map(({ target, manual }) => ({
167175
targetId: target.targetId,
168176
threadId: target.threadId,
@@ -197,16 +205,12 @@ function mapFreshInventory(
197205
}
198206
: {}),
199207
}));
200-
const commandLedgerWatermark = String(
201-
Math.max(
202-
0,
203-
...rawTargets.map(({ target }) => target.parentCommentDatabaseId ?? 0)
204-
)
205-
);
208+
const commandWatermark = commandLedgerWatermark(ledger.payload);
209+
const commandLedgerWatermarkValue = String(commandWatermark);
206210
const lifecycleStateHash = sha256(
207211
canonicalJson({
208-
commandLedgerWatermark,
209-
complete: !missingDatabaseId,
212+
commandLedgerWatermark: commandLedgerWatermarkValue,
213+
complete: true,
210214
loadedForHeadSha,
211215
targets,
212216
warnings,
@@ -216,8 +220,8 @@ function mapFreshInventory(
216220
inventoryVersion: 'review_lifecycle_inventory.v1',
217221
loadedForHeadSha,
218222
lifecycleStateHash,
219-
commandLedgerWatermark,
220-
complete: !missingDatabaseId,
223+
commandLedgerWatermark: commandLedgerWatermarkValue,
224+
complete: true,
221225
warnings: Object.freeze(warnings),
222226
targets: Object.freeze(targets),
223227
});

‎src/review-orchestration/infrastructure/production-t0-review-runner.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { ReviewActionV2Client } from '../../control-plane/review-action-v2-clien
1212
import { BatchOrchestrator } from '../../core/batch-orchestrator';
1313
import { prioritizeFilesByRisk } from '../../review-execution/domain/file-risk-priority';
1414
import { GitHubClient } from '../../github/client';
15+
import { ReviewLedger } from '../../github/ledger';
1516
import { PullRequestLoader } from '../../github/pr-loader';
1617
import { CodexProvider } from '../../providers/codex';
1718
import { recoverDiffForFiles } from '../../utils/diff';
@@ -104,7 +105,10 @@ export class ProductionT0ReviewRunner implements CodexOAuthV2ReviewRunnerPort {
104105
) {
105106
return { outcome: CodexOAuthV2ReviewOutcome.Superseded };
106107
}
107-
const lifecycleInventory = new FreshGitHubLifecycleInventory(github);
108+
const lifecycleInventory = new FreshGitHubLifecycleInventory(
109+
github,
110+
new ReviewLedger(github, process.env.REVIEW_ROUTER_LEDGER_KEY)
111+
);
108112
const initialLifecycle = await lifecycleInventory.loadForPrompt(
109113
pr.number,
110114
authorization.facts.headSha

0 commit comments

Comments
 (0)