Skip to content

Commit 56ccbb5

Browse files
committed
refactor(queue): assert the delivery-id prefixes are distinguishable, not longest-match
The longest-prefix tie-break could only run if one prefix were a prefix of another, which none is -- untestable defensive code standing in for a guarantee nobody stated. The guarantee is now an invariant test, so adding an ambiguous prefix fails with the real problem named rather than being silently absorbed by a scan.
1 parent 46060b3 commit 56ccbb5

2 files changed

Lines changed: 16 additions & 14 deletions

File tree

src/queue/delivery-id.ts

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -43,17 +43,12 @@ export function deliveryIdFor(origin: DeliveryIdOrigin, suffix: string): string
4343
/**
4444
* Which producer minted this delivery id, or null for a raw GitHub webhook delivery id.
4545
*
46-
* Longest prefix wins, so an origin whose prefix is a prefix of another's still resolves to the more
47-
* specific one rather than to whichever happens to be declared first.
46+
* First match wins, which is unambiguous ONLY because no prefix here is a prefix of another -- an
47+
* invariant asserted directly in decision-record.test.ts rather than worked around with a
48+
* longest-match scan. A tie-break that can never run is untestable defensive code; a failing
49+
* invariant test names the real problem (two origins that cannot be told apart) to whoever adds one.
4850
*/
4951
export function deliveryIdOrigin(deliveryId: string | null | undefined): DeliveryIdOrigin | null {
5052
if (typeof deliveryId !== "string") return null;
51-
let match: DeliveryIdOrigin | null = null;
52-
for (const origin of DELIVERY_ID_ORIGINS) {
53-
if (!deliveryId.startsWith(DELIVERY_ID_PREFIXES[origin])) continue;
54-
if (match === null || DELIVERY_ID_PREFIXES[origin].length > DELIVERY_ID_PREFIXES[match].length) {
55-
match = origin;
56-
}
57-
}
58-
return match;
53+
return DELIVERY_ID_ORIGINS.find((origin) => deliveryId.startsWith(DELIVERY_ID_PREFIXES[origin])) ?? null;
5954
}

test/unit/decision-record.test.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {
1515
sha256Hex,
1616
type DecisionRecord,
1717
} from "../../src/review/decision-record";
18-
import { DELIVERY_ID_ORIGINS, deliveryIdFor, deliveryIdOrigin } from "../../src/queue/delivery-id";
18+
import { DELIVERY_ID_ORIGINS, DELIVERY_ID_PREFIXES, deliveryIdFor, deliveryIdOrigin } from "../../src/queue/delivery-id";
1919
import { appendDecisionLedger, LEDGER_GENESIS_HASH, loadDecisionLedgerTip, loadDecisionRecordCollapsible, loadPublicDecisionRecord, verifyDecisionLedger } from "../../src/review/decision-record";
2020
import { createTestEnv } from "../helpers/d1";
2121

@@ -1015,9 +1015,16 @@ describe("deriveReevaluationReason", () => {
10151015
expect(DELIVERY_ID_ORIGINS.length).toBeGreaterThan(0);
10161016
});
10171017

1018-
it("resolves the LONGEST matching prefix, so one origin cannot shadow another", () => {
1019-
// `regate-sweep:` and `regate-repair:` share a stem; a first-match scan could mis-attribute a repair
1020-
// as routine maintenance, which is exactly the distinction the record exists to preserve.
1018+
it("keeps every origin distinguishable: no prefix may be a prefix of another", () => {
1019+
// This is what makes a first-match scan unambiguous. If it ever fails, two producers have become
1020+
// indistinguishable and one would silently inherit the other's reason -- `regate-sweep:` read as
1021+
// `regate-repair:` would file a repair as routine maintenance, the exact distinction this preserves.
1022+
for (const a of DELIVERY_ID_ORIGINS) {
1023+
for (const b of DELIVERY_ID_ORIGINS) {
1024+
if (a === b) continue;
1025+
expect(DELIVERY_ID_PREFIXES[a].startsWith(DELIVERY_ID_PREFIXES[b]), `${a} vs ${b}`).toBe(false);
1026+
}
1027+
}
10211028
expect(deliveryIdOrigin(deliveryIdFor("regateRepair", "o/r#7"))).toBe("regateRepair");
10221029
expect(deliveryIdOrigin(deliveryIdFor("regateSweep", "o/r#7"))).toBe("regateSweep");
10231030
expect(deliveryIdOrigin("not-a-known-prefix:o/r#7")).toBeNull();

0 commit comments

Comments
 (0)