From 0d5b849d85b1a33f0ed5185fec541a2b75c0e821 Mon Sep 17 00:00:00 2001 From: Matthew P Munger Date: Wed, 26 Aug 2026 12:30:47 -0500 Subject: [PATCH] fix: make main green again after F4 landed on S3/S6/S7 (R2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F4 (#82) branched before S3/S6/S7 (#81) and was merged without a rebase. It renamed `TransitionOptions.actor` to `by: Caller` and `HistoryEntry.actor` to `by`, and never saw the S6/S7 tests that pass `actor`. The files are disjoint, so git reported no conflict and `main` went red on merge: 8 typecheck errors and 4 failing test files. Every branch cut since has inherited them, which is worse than the four tests — a red suite is how everyone learns to stop reading the output. Three files are the rename F4 would have made had it seen them. `digest.test.ts`, `digest-arrival.test.ts` and `webhook.test.ts` each declare a local `PERSON: Caller`, which is the convention F4 established in the four test files it did update. None of the three renders an attribution — the digest is about cases, not about who moved them — so each says in a line that the caller exists to satisfy the transition guard and is never asserted on. The fourth was F4's own guard failing on `personActionsFor`, and that one is the guard's defect rather than the function's. `transition.actor` is a permission set: a list of classes. Two things may legitimately be tested against it and both are classes — a caller's `kind`, which is the check `applyAction` performs, and a class named outright, which is how a caller asks a question about the permission set rather than about a caller. `personActionsFor` does the second: "which actions may a person fire" is exactly what a permission set is for, and the answer decides which buttons exist. What F4 set out to forbid is comparing an IDENTITY against the list — a user id, an agent name — because that guard answers for the strings it lists and guesses for the rest. The regex could not tell the two apart, so it flagged the legitimate use. Rewriting `personActionsFor` to satisfy it would have been making the code worse to keep a check green. The guard is narrowed instead: the argument must be a `kind` or a class the registry declares. The permitted literals are read off `vocabulary.json`, so a third class is permitted the moment it is declared and an agent name — `checkpoint`, `grouping`, `migration` — never is. Narrowing a check earns the obligation to show it still bites, so the rule is now a named predicate with its own test: `kind` expressions and every declared class pass, and identities of both shapes are rejected. A class held in a variable is still flagged, deliberately — it is indistinguishable from an identity by reading, and both call sites here have a better form available. The scanner reads prose as well as code, so the file may not spell the pattern it looks for. It did, briefly, and reported itself; there is now a comment saying why it must not. No production code changed. --- src/lib/__tests__/caller.test.ts | 79 ++++++++++++++++++++---- src/lib/__tests__/digest-arrival.test.ts | 18 ++++-- src/lib/__tests__/digest.test.ts | 13 +++- src/lib/__tests__/webhook.test.ts | 9 ++- 4 files changed, 98 insertions(+), 21 deletions(-) diff --git a/src/lib/__tests__/caller.test.ts b/src/lib/__tests__/caller.test.ts index 3c4212a..52e7388 100644 --- a/src/lib/__tests__/caller.test.ts +++ b/src/lib/__tests__/caller.test.ts @@ -120,20 +120,73 @@ describe("attribution", () => { }); }); -/* ── The guard is on the class, and only on the class ───────────────────── */ +/* ── The guard is on the class, and only on the class ─────────────────── */ + +/** + * Every comparison against a permission set in a file, with the argument it was + * handed. + * + * This is a text scan, so it reads comments as well as code and cannot tell the + * difference. Nothing in this file may spell the pattern out in prose — doing so + * makes the guard report itself, which is a failure that looks exactly like a + * real one and wastes the next reader's afternoon. + */ +const COMPARISON = /\.actor\b\s*\.\s*includes\(([^)]*)\)/g; + +/** + * Whether one such comparison is testing something that is not a class. + * + * `transition.actor` is a PERMISSION SET — a list of classes. Two things may + * legitimately be tested against it, and they are both classes: + * + * - a caller's `kind`, which is the runtime check `applyAction` performs; + * - a class named outright, which is how a caller asks a question ABOUT the + * permission set rather than about a caller. `personActionsFor` does this: + * "which actions may a person fire" is precisely what a permission set is + * for, and the answer drives which buttons exist. + * + * What is forbidden is comparing an IDENTITY against it — a user id, an agent + * name, any string naming who rather than which class. That guard answers for + * the strings it lists and has to guess for the rest, so an unlisted caller + * either gets in or is locked out for having a new name, and a caller that picks + * a listed name gets in on the strength of the name alone. F4 exists because + * `actor` conflated the two; a check that cannot tell them apart either is the + * same defect one level up. + * + * The permitted literals are read off the registry rather than written here, so + * a third class added to `vocabulary.json` is permitted the moment it is + * declared, and an agent name — `"checkpoint"`, `"grouping"`, `"migration"` — + * is never one of them. + * + * A class held in a variable is still flagged. That is deliberate: it is rare, + * it is indistinguishable from an identity by reading, and the two call sites in + * this codebase both have a better form available. Erring tight on a check like + * this costs an author one line of justification; erring loose costs the guard. + */ +function comparesANonClass(argument: string): boolean { + const text = argument.trim(); + if (/\.kind\b/.test(text)) return false; + return !REGISTRY_CLASSES.some((className) => text === `"${className}"` || text === `'${className}'`); +} describe("no identity string is ever compared against a permission list", () => { + it("knows an identity from a class", () => { + // The guard was narrowed in R2, so what it still catches is asserted rather + // than assumed. A check that was loosened without this is a check nobody can + // tell from a deleted one. + expect(comparesANonClass("options.by.kind")).toBe(false); + expect(comparesANonClass("by.kind")).toBe(false); + for (const className of REGISTRY_CLASSES) { + expect(comparesANonClass(`"${className}"`), `${className} is a class the registry declares`).toBe(false); + } + // Identities, every one of which the old field could hold and none of which + // is a class. + for (const identity of ['"checkpoint"', '"grouping"', '"migration"', '"rae@webflow.com"', "rec.owner", "userId", "entry.by.userId"]) { + expect(comparesANonClass(identity), `${identity} is an identity, not a class`).toBe(true); + } + }); + it("is true of every file under src/", () => { - /** - * The shape this chunk exists to prevent: a guard that asks whether an - * identity string is in a list of permitted names. It answers for the - * strings it lists and has to guess for the rest, so an unlisted caller - * either gets in or is locked out for having a new name — and a caller that - * picks a listed name gets in on the strength of the name alone. - * - * `transition.actor` may only ever be tested against a caller's `kind`. - * This reads every source file and checks that. - */ const files: string[] = []; const walk = (dir: string) => { for (const entry of readdirSync(dir, { withFileTypes: true })) { @@ -147,8 +200,8 @@ describe("no identity string is ever compared against a permission list", () => const offenders: string[] = []; for (const file of files) { const source = readFileSync(file, "utf8"); - for (const match of source.matchAll(/\.actor\b\s*\.\s*includes\(([^)]*)\)/g)) { - if (!/\.kind\b/.test(match[1])) offenders.push(`${path.relative(srcDir, file)}: ${match[0]}`); + for (const match of source.matchAll(COMPARISON)) { + if (comparesANonClass(match[1])) offenders.push(`${path.relative(srcDir, file)}: ${match[0]}`); } } expect(offenders).toEqual([]); diff --git a/src/lib/__tests__/digest-arrival.test.ts b/src/lib/__tests__/digest-arrival.test.ts index c30c526..742bcca 100644 --- a/src/lib/__tests__/digest-arrival.test.ts +++ b/src/lib/__tests__/digest-arrival.test.ts @@ -21,6 +21,7 @@ import { type IssueCase, type IssueState, } from "../issue-case"; +import type { Caller } from "../caller"; import { recordCheckpointReading } from "../checkpoint-evaluation"; import { normalizePerformanceThresholds } from "../performanceThresholds"; import { ISSUE_TRANSITIONS, QUEUES, WORK_STATES, type IssueAction } from "../vocabulary"; @@ -43,6 +44,15 @@ const AT = "2026-08-25T06:00:00.000Z"; const DATE = "2026-08-25"; const APP = "https://watch.example.com/page-watch"; +/** + * Whoever walked the case down the lifecycle below. + * + * The address is the case's id and nothing else, so who moved it is exactly the + * kind of fact the link is not allowed to carry. This exists to satisfy the + * transition guard and is never asserted on. + */ +const PERSON: Caller = { kind: "person", userId: "rae@webflow.com" }; + function caseOf(overrides: Partial = {}): IssueCase { return { id: "PW-2291", @@ -129,9 +139,9 @@ describe("the case route", () => { */ const link = digestOf([caseOf()]).sections[0].lines[0].href; let issue = caseOf(); - issue = accept(issue, { actor: "person", at: AT }); - issue = start(issue, { actor: "person", at: AT }); - issue = markFixed(issue, { actor: "person", at: AT }); + issue = accept(issue, { by: PERSON, at: AT }); + issue = start(issue, { by: PERSON, at: AT }); + issue = markFixed(issue, { by: PERSON, at: AT }); issue = recordCheckpointReading(issue, { interval: "7d", outcome: "disagreed", at: AT }).issue; expect(issue.state).toBe("reopened"); expect(issue.id).toBe("PW-2291"); @@ -187,7 +197,7 @@ describe("the context banner", () => { it("repeats the line the message wrote, because both come from one writer", () => { const reopened = recordCheckpointReading( - markFixed(caseOf({ state: "in_progress" }), { actor: "person", at: AT }), + markFixed(caseOf({ state: "in_progress" }), { by: PERSON, at: AT }), { interval: "7d", outcome: "disagreed", at: AT }, ).issue; const digest = digestOf([reopened]); diff --git a/src/lib/__tests__/digest.test.ts b/src/lib/__tests__/digest.test.ts index 2326138..d1b08f5 100644 --- a/src/lib/__tests__/digest.test.ts +++ b/src/lib/__tests__/digest.test.ts @@ -14,6 +14,7 @@ import { digestLinks, renderDigestMessage } from "../digest-email"; import { DIGEST_CADENCE_LABEL } from "../digestCadence"; import { formatImpact } from "../impact-format"; import { markFixed, scheduleCheckpoints, type IssueCase } from "../issue-case"; +import type { Caller } from "../caller"; import { recordCheckpointReading } from "../checkpoint-evaluation"; import { normalizePerformanceThresholds } from "../performanceThresholds"; import { casePath } from "../paths"; @@ -61,15 +62,21 @@ function caseOf(overrides: Partial = {}): IssueCase { }; } +/** + * Whoever marked the fix. The digest never renders them — it is about cases, not + * about who moved them — so this exists only to satisfy the transition guard. + */ +const PERSON: Caller = { kind: "person", userId: "rae@webflow.com" }; + /** A case the system brought back, produced by the evaluator rather than posed. */ function cameBackCase(overrides: Partial = {}): IssueCase { - const fixed = markFixed(caseOf({ state: "in_progress", ...overrides }), { actor: "person", at: AT }); + const fixed = markFixed(caseOf({ state: "in_progress", ...overrides }), { by: PERSON, at: AT }); return recordCheckpointReading(fixed, { interval: "7d", outcome: "disagreed", at: AT }).issue; } /** A fixed case still waiting: three checkpoints scheduled, nothing read. */ function heldCase(overrides: Partial = {}): IssueCase { - return markFixed(caseOf({ state: "in_progress", ...overrides }), { actor: "person", at: AT }); + return markFixed(caseOf({ state: "in_progress", ...overrides }), { by: PERSON, at: AT }); } /** A fixed case whose three checks all failed to read — evaluation rule 4. */ @@ -81,7 +88,7 @@ function unreadableCase(overrides: Partial = {}): IssueCase { attempts: 2, result: "unavailable" as const, })), - history: [{ at: AT, from: "in_progress", to: "fixed", actor: "person" }], + history: [{ at: AT, from: "in_progress", to: "fixed", by: PERSON }], ...overrides, }); } diff --git a/src/lib/__tests__/webhook.test.ts b/src/lib/__tests__/webhook.test.ts index a3e3887..5a04545 100644 --- a/src/lib/__tests__/webhook.test.ts +++ b/src/lib/__tests__/webhook.test.ts @@ -7,6 +7,7 @@ import { import { buildDigest, type Digest } from "../digest"; import { renderDigestMessage } from "../digest-email"; import { markFixed, type IssueCase } from "../issue-case"; +import type { Caller } from "../caller"; import { recordCheckpointReading } from "../checkpoint-evaluation"; import { normalizePerformanceThresholds } from "../performanceThresholds"; import { pendingPage } from "../mutations"; @@ -18,6 +19,12 @@ afterEach(() => { const AT = "2026-08-04T06:00:00.000Z"; +/** + * Whoever marked the fix. The payload is the digest message and the digest is + * about cases, so this only ever satisfies the transition guard. + */ +const PERSON: Caller = { kind: "person", userId: "rae@webflow.com" }; + function caseOf(overrides: Partial = {}): IssueCase { return { id: "PW-1", @@ -71,7 +78,7 @@ describe("alert webhook", () => { * sections, same sentences. */ const digest = digestOf([ - recordCheckpointReading(markFixed(caseOf(), { actor: "person", at: AT }), { + recordCheckpointReading(markFixed(caseOf(), { by: PERSON, at: AT }), { interval: "7d", outcome: "disagreed", at: AT,