R2 — make main green again after F4 landed on S3/S6/S7 (Blocking) - #86
Merged
Conversation
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.
This was referenced Aug 26, 2026
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.
Blocking.
mainat728eb6cdoes not typecheck and does not test green. This restores it. No production code changes — four test files.What happened
F4 (#82) branched before S3/S6/S7 (#81) and was merged without a rebase. F4 renamed
TransitionOptions.actor→by: CallerandHistoryEntry.actor→by; it never saw the S6/S7 test files that passactor. The files are disjoint, so git reported no conflict and merged cleanly into a broken tree.Measured on
728eb6cin a clean worktree:npm run typechecknpm test -- --runnpm run lintnpm run buildEvery branch cut since inherits the failures. That is the part worth fixing quickly — a red suite is how everyone learns to stop reading the output.
Three files: the rename F4 would have made
digest.test.ts,digest-arrival.test.tsandwebhook.test.tseach get a localconst PERSON: Caller, which is the convention F4 established in the four test files it did update (issue-case,checkpoint-evaluation,case-applicability,watch-notation). I followed that rather than inventing a shared helper.None of the three renders an attribution — the digest is about cases, not about who moved them — so each
PERSONcarries a line saying it exists only to satisfy the transition guard and is never asserted on. That is worth writing down, because the next reader will otherwise wonder whether the identity matters to the assertion.The fourth: F4's guard, not
personActionsForF4's
caller.test.tsasserts that no identity string is ever compared against a permission list. It was flaggingpersonActionsForinissue-case.ts:That is the legitimate use, and it follows from F4's own reasoning.
transition.actoris a permission set — a list of classes. Two things may be tested against it, and both are classes:kind, which is the runtime checkapplyActionperforms;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 has to guess for the rest, so an unlisted caller either gets in or is locked out for having a new name. The regex could not tell a class literal from an identity, so it caught the wrong one.
Rewriting
personActionsForto satisfy an over-broad guard would be making the code worse to keep a check green, which is the thing R1 spent a session forbidding. So the guard is narrowed instead: the argument must be akindexpression, or a class the registry declares.The permitted literals are read off
vocabulary.jsonvia theREGISTRY_CLASSESthe file already computes — so a third class is permitted the moment it is declared, and an agent name (checkpoint,grouping,migration) never is."system"is permitted, because "may the system fire this" is the same legitimate question as "may a person".Narrowing a check earns the obligation to show it still bites
The rule is now a named predicate with its own test rather than an inline regex condition.
kindexpressions pass, every registry-declared class passes, and identities of both shapes — quoted agent names, quoted emails, bare variables likerec.ownerandentry.by.userId— are rejected.Without that test, a loosened check is indistinguishable from a deleted one.
A class held in a variable is still flagged. That is deliberate rather than an oversight: it is indistinguishable from an identity by reading, it is rare, and both call sites in this codebase have a better form available. Erring tight costs an author one line of justification; erring loose costs the guard.
One thing worth knowing about the guard
It is a text scan, so it reads comments as well as code and cannot tell the difference. My first draft spelled the pattern out in its own doc comment and the guard reported itself — a failure that looks exactly like a real one. There is now a comment in the file saying the pattern must not appear in prose. Cheap to hit, annoying to diagnose.
Verification
npm run lint,npm run typecheck,npm test -- --run(97 files, 1210 tests),npm run build— all clean.npx --yes npm@10.9.7 ci --dry-run --ignore-scripts --no-audit --no-fundperAGENTS.md; lockfile untouched.Merge order
This is based directly on
mainand touches only test files. S5 (#84) is based onmaintoo and its write set does not overlap — it changes no test file this one touches. Either order works; landing R2 first means S5's CI reports only S5.