feat(core): add the ReleaseRole vocabulary and characterise today's claim disposition (#790) - #815
Merged
Merged
Conversation
.agents/skills/ holds skills installed from upstream repos and pinned by computedHash in skills-lock.json. Prettier reflowed all 44 of their markdown files, which failed check:md and blocked pnpm run verify at step 3 of 11 -- and formatting them would have invalidated every hash in the lock. .claude/skills/* are symlinks into the same tree; Prettier does not follow symlinked directories, so the single .agents/skills/** entry covers both.
Pins the answer an orchestrator gets back at the resolution seam, before anything in the #781 cluster moves. Three cases: - An already-terminal loop entry resolves `superseded` / `claim-rotated`. This is the #781 defect, asserted deliberately: `rundown run` mints a run-control claim for every default-stack root, and the terminal loop entry releases that root through the stack-pop derivation, which still encodes "this run is unclaimed". The assertion flips to `terminal` in the ticket that fixes it, so it must be green first for the flip to read as a one-line diff. - A run completing through a fenced command resolves `terminal`. The control, which must not move at any point in the cluster. - Both dispositions survive a process boundary, resolved from a third process sharing only the database. The trigger for the first case is a delegation whose child runbook is not discoverable, which stops the run during initialization and re-enters the loop already terminal. A plain completion would not do: the fence releases with retention and the loop returns before the entry-time terminal check, so a test built on one passes under both old and new code. The control resolving `terminal` in the same harness is what proves the two paths are actually distinct. Assertions are at the resolution seam rather than the session projection, because the projection is what the follow-up rewrites and the resolution is what a caller observes. The precise supersession reason comes from core; the CLI envelope carries only the code, which `claim-rotated` shares with `parent-unreadable`, so the message text is what separates them.
`ReleaseRole` (addressed | collateral | discarded), `ClaimDisposition` (retain-as-terminal-evidence | revoke), `claimDisposition(role)`, `RunRelease` and `projectRunRelease(session, release)`. The primitive this will replace asks each caller for a conclusion — `retainClaimsAsTerminal`, "should this claim survive?" — which is domain logic, performed independently at sixteen sites. Fifteen agree on a rule none of them states: the run the caller acted ON keeps its claim as terminal evidence; a run swept up so the addressed run could close does not. The sixteenth omits the option, and omission reads as the destructive direction. This asks for the fact the caller already holds instead, and owns the conclusion. There is no option left to omit, so that bug class becomes unrepresentable. `discarded` is its own arm rather than a synonym for `collateral`: a destroy path spelled `addressed` would retain claims over a run about to stop existing. `claimDisposition` takes the role alone, and a property test pins what makes that safe — a run's disposition depends only on its own role, never on ordering, never on the other members of a batch. That is what lets it widen to `claimDisposition(role, claim)` later without touching a caller. `projectRunRelease` is synchronous and in-place by requirement, not preference: several dispositions reach the projection through a session callback that accepts nothing else. It preserves the replaced primitive's found/not-found answer exactly, including counting a retained claim as found, so the migration that moves callers onto it can be behaviour-neutral. Nothing calls any of it yet. Wiring is the next ticket, deliberately separate so a regression there stays traceable to the move rather than to the vocabulary.
`projectRunRelease` reported found/not-found correctly in every existing case, but never because of the default stack: each of those cases carried a claim or a stash entry as well, so replacing the stack comparison with `false` changed no assertion. Adds the case where stack membership is the only evidence. The exhaustive `never` arm of `claimDisposition` takes the repo's existing Stryker suppression for unreachable code, matching lifecycle-command-service.ts:2344. `session-release.ts` now detects all 41 of its valid mutants.
.agents/plugins/ comes from the same upstream install as .agents/skills/ and is pinned by the same lockfile. It ships no Markdown today, so the narrower pattern passed -- and the first upstream plugin that does would reintroduce the check:md blocker this exclusion exists to prevent. Deliberately not extended to cspell: check:spell is `cspell ... .`, whose traversal does not descend into dot-directories, so .agents is already unreachable from it. Pointing cspell at the path directly flags pocock, travelling, lossiness and theorise, none of which are in the dictionary -- yet check:spell reports nothing there. An ignorePaths entry would be dead config. Prettier needs one only because its **/*.md glob does traverse them.
Review of 132c9d2..HEAD. All four were latent rather than live, and each would have degraded a gate silently rather than failing it. session-release.ts: RELEASE_ROLES claimed a compile error would follow from adding a ReleaseRole arm without listing it. It would not -- `as const satisfies readonly ReleaseRole[]` checks assignability, not exhaustiveness, so the constant would have gone silently short and the tests iterating it silently partial, while only claimDisposition's `never` arm complained. Made the claim true with an AssertNever-constrained UnlistedReleaseRole; adding a fourth role now fails the type check at the constant as well. session-release.test.ts: both order-independence properties were named "under any permutation" but compared forward against reversed only -- two of up to 120 orderings, blind to cross-talk that depends on an interior ordering. They now also compare a seed-driven Fisher-Yates permutation. Separately, claimFor built its key as `key.repeat(32).slice(0, 32)`, which collides for indices whose hex digits repeat to the same 32 characters (1 and 17); zero-padded instead, so a larger session cannot quietly assert against fewer claims than it created. claim-disposition-characterisation.test.ts: dropped the third test. #790 asks for a multi-process assertion, and the two remaining tests already carry it -- the run is one subprocess and the `rundown status --claim-id` assertion is another, sharing only the database. The third re-ran both scenarios to assert strictly weaker versions, for two extra CLI subprocesses and no coverage, and duplicated the assertion #793 has to flip where a reader would not look. The rationale it documented is folded into the file docblock. Unchanged: 100% mutation score on session-release.ts (39 killed, 2 ignored, 0 survived, 0 no-coverage) and `pnpm run verify` green.
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes Limit details: You’ve used all 2 included reviews currently available. Your 50 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Warning
|
Deploying rundown with
|
| Latest commit: |
463c511
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dceb6052.rundown-7hl.pages.dev |
| Branch Preview URL: | https://790-claim-disposition-charac.rundown-7hl.pages.dev |
🧬 Mutation score (advisory)Hybrid mutation analysis: source changes use changed-line scopes (dedicated tests by default; ℹ️ Mutation scope planSource test selection:
|
| File | Score | Status |
|---|---|---|
src/runbook/index.ts |
— | ⏭️ not mutated |
✅ core — changed-scope mutants (floor 70% shown as score context)
| File | Score | Status |
|---|---|---|
src/runbook/session-release.ts |
100.00% | ✅ |
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
1 participant
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.
Closes #790. Step 1 and step 3 of the #781 implementation order.
Behaviour-neutral. Nothing calls the new vocabulary yet, and the characterisation tests assert today's behaviour unmodified. The point is to make the change that follows visible as a one-line diff.
What lands
The vocabulary, in core, with no callers (
packages/core/src/runbook/session-release.ts):The session release primitive currently asks each caller for a conclusion —
retainClaimsAsTerminal, a boolean meaning "should this claim survive?" — which is domain logic performed independently at sixteen call sites. Fifteen agree, on a rule none of them states. The sixteenth omits the option, and omission reads as the destructive direction, which is #781.The new vocabulary asks for the fact the caller already holds ("I addressed this run") and owns the conclusion itself. That makes the bug class unrepresentable: there is no option left to omit.
Two constraints are load-bearing and are pinned by tests, not just documented:
projectRunReleaseis synchronous and mutates in place, because six dispositions reach the projection throughcommitOwnedState({ updateSession }), which accepts a sync in-place callback and nothing else.claimDispositiontakes the role alone. A property test pins the invariant that makes that safe — a run's disposition depends only on its own role, never on ordering and never on the other members of a batch — which is what lets it widen toclaimDisposition(role, claim)later without touching a caller. The property shuffles up to 5 releases across all orderings rather than testing reversal alone, because cross-talk on an interior ordering survives a plain reversal.discardedis a distinct arm rather than a synonym forcollateral: the destroy paths must never be spelledaddressed, which would retain claims over a run about to stop existing. And the retained case is namedretain-as-terminal-evidence, nottombstone— the tombstone is thesupersededrow that revoking produces (runbook-store.ts:1558-1585), and that collision is what the vocabulary exists to remove.Characterisation tests at the resolution seam (
packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts) assert the answer an orchestrator actually gets, not the shape stored in the session projection:superseded/claim-rotated— this assertion flips in the ticket that fixes Terminal dispositions disagree on claim retention: stack-pop deletes, release-runbook and the fence retain #781;terminal— the control, which must not move at any point.The trigger is a delegation whose child runbook is not discoverable. A plain completion does not reach the entry check at all: the fenced command mutation releases with retention and the loop returns first.
#790's third acceptance item — the multi-process case — is discharged inline in both tests rather than as a third case, and the test file says so: the run is one subprocess and
resolveClaimreads the database from another, so both already cross a process boundary. A unit suite mocking the session boundary cannot observe the tombstone crossing that boundary, which is the whole reason the requirement exists.Acceptance
ReleaseRole/ClaimDisposition/claimDisposition/projectRunReleasein core, unit-tested, no production callerstest:mutate:changedreports no survivors in the changed ranges3ed3d00af)Also included
Two Prettier-exclusion chores (
132c9d298,9aeecbd24). They are not incidental: without thempnpm run verifystops atcheck:mdon the untracked vendored skills tree, which is the failure #791 hit and #532 tracks. They belong on the branch that has to be green to merge.Testing
pnpm run verify— green, exit 0.What this unblocks
#792, #793 and #794 are blocked in sequence behind #790. Nothing else in the #781 chain can land until this does.