Skip to content

Pop claims, the OUTPUTS seam, and the STEP_ENTERED characterisation (#788, #790, #799, #816) - #826

Merged
tobyhede merged 11 commits into
mainfrom
788-pop-claims-and-outputs-seam
Aug 19, 2026
Merged

Pop claims, the OUTPUTS seam, and the STEP_ENTERED characterisation (#788, #790, #799, #816)#826
tobyhede merged 11 commits into
mainfrom
788-pop-claims-and-outputs-seam

Conversation

@tobyhede

Copy link
Copy Markdown
Owner

Summary

@coderabbitai summary

Four tickets, in dependency order. The three behaviour-bearing commits carry
changesets; the rest is characterisation and docs.

#790ReleaseRole vocabulary, plus characterisation of today's terminal
claim disposition.
Core gains ReleaseRole / ClaimDisposition /
claimDisposition / projectRunRelease in session-release.ts, with no
callers. The session release primitive currently asks each of sixteen call sites
for a conclusion (retainClaimsAsTerminal), which is domain logic performed
independently; the new vocabulary asks for the fact the caller already holds
and owns the conclusion itself, so the omission bug has no option left to omit.
A property test pins that a run's disposition depends only on its own role,
which is what lets claimDisposition(role) widen to claimDisposition(role, claim) later without touching a caller.

#788 — undoing an inline-child activation no longer revokes the child's
claim.
popRunbookIfActive undid a stack push through the general release
primitive, revoking every claim controlling the run. The undo disposed of
authority the push never created, irrecoverably: adoptRunControlClaim refuses
to re-mint once the child has issued a delegation, so the child ran unarmed and
nothing addressed the run again. Replaced with a narrow projectStackPop that
takes the stack array alone — it cannot revoke a claim, today or after a later
edit — and removes the topmost occurrence only, since session_stack has no
uniqueness constraint and cannot gain one.

#799 — derive the OUTPUTS scope behind the state machine. The CLI used to
decide which OUTPUTS an execution unit captures and where those channels live,
then ship both conclusions to the machine on EXECUTE_COMMAND. Both derivations
move into the commandExecActor invoke-input closure, entering through the two
doors CLAUDE.md's actor-dependency rule prescribes: nakedOutputs is
compile-time-bound and closed over by the leaf-state builder, outputScope is
event-time-bound and read from context.forStack at fire time. The event fields
are gone, so a re-added CLI derivation fails to compile.

#816 — characterise today's STEP_ENTERED divergence between run and
collect.
Two functions build the StepEntryMetadata behind a STEP_ENTERED
payload and they disagree; all four rendered fields are optional on the type,
which is what lets the disagreement compile. Pinned against unmodified code so
#799's remaining half reads as an assertion flipping. Details below.

The #816 divergences

Each assertion states which value is the correct one, so the later flip is
unambiguous.

  • Rendered fields, end to end. step-entered-divergence-characterisation.test.ts
    enters one substep of one runbook twice — first by rundown run, then by the
    RETRY re-entry rundown collect drives. The run payload carries the
    description and prompt; the collect payload carries neither, while position,
    stepName, isSubstep, prompted, hasCommand and runbookId all agree.
    That agreement is what makes the omission a divergence rather than a different
    event. Correct: the run payload — a substep's description does not depend on
    which command entered it.
  • prompted. The loop ORs currentStep.kind === 'prompted-for' into the
    flag it was called with; collect reads !!advanced.prompted alone and reports
    false for the same cursor on the same step. Correct: the loop's value. The
    field documents whether execution is prompted rather than automatic, and the
    loop returns 'waiting' on that same term.
  • substepId / isSubstep. Inside one builder rather than between two: the
    loop takes substepId off the raw cursor and isSubstep off the resolved
    execution unit, so a cursor naming no live substep yields a populated
    substepId alongside isSubstep: false. Correct: substepId absent. Both
    answer one question, so both belong to the resolved unit — and this is not
    tidiness, since the frontier seams gate credential disclosure on isSubstep
    while deriveStepEnteredEffect's cursor guard fires on substepId.

The two unit-level halves capture the entry off the observeExecutionUnitEntry
argument rather than off the emitted event, because that argument is the
builder's output and the payload is a lossy projection of it — substepId never
reaches the event at all. Nothing already pinned in the collection-service suite
was edited.

Testing

  • pnpm run verify

Targeted tests or checks:

  • pnpm run test:integration:cli — 668/668, including the new
    step-entered-divergence-characterisation suite.
  • pnpm --filter @rundown-org/core exec jest --testPathPatterns collection-service.test — 83/83.
  • pnpm --filter @rundown-org/cli exec jest --testPathPatterns execution-loop — 103/103.
  • /code-review high over the branch: one finding, fixed in 88aeab2dc (see
    below).

Review Notes

Check all that apply:

  • Public CLI behavior changed; docs and JSON schema output were checked.
  • Runbook syntax or parser behavior changed; docs/spec/language.md,
    docs/spec/grammar.md, and runbook fixtures were checked.
  • Persisted runbook state changed; stale-state handling follows the
    no-migration rule.
  • Security policy, sandbox, path resolution, or command execution changed;
    traversal, symlink escape, deny precedence, env leakage, and fail-closed
    behavior were checked.
  • GitHub Actions changed; actions remain SHA-pinned with version comments.

Notes for the reviewer:

  • Persisted state. The EXECUTE_COMMAND event drops outputScope and
    nakedOutputs. That event never reaches persisted context (no assign on the
    __execute-command target), so no run state changes shape — but the leaf-state
    builder does, and an in-flight run on the old shape should be finished, stopped
    or pruned rather than adapted.
  • popRunbookIfActive stays mutateGuarded, so its execution_in_progress
    / recovery_required refusals are now unreachable through a stack-only
    projection. Deliberately held back: both come from one loop in
    mutateSessionGuarded, and removing them wants a stale-lease test and a
    multi-process test rather than more argument. This projection is what makes
    that removal a no-op later rather than a lossy edit.
  • 88aeab2dc narrows a TSDoc claim on extractUnitOutputs, which 526ea4485
    exported from core's barrel. The doc promised that a substepId naming no
    substep never falls back to the parent's OUTPUTS; that holds only when the step
    defines substeps, and execution-units.test.ts:131 pins the other branch
    deliberately. Behaviour unchanged — the fallback is correct, because
    resolveCurrentExecutionUnit resolves the same cursor to the parent step.
  • No changeset for Characterise today's STEP_ENTERED divergence between run and collect #816 or the doc fix. Both are test-only / docs-only with
    no released behaviour to note.

.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.
The CLI decided which OUTPUTS an execution unit captures and where those
channels live, then shipped both conclusions to the machine on
EXECUTE_COMMAND. Every input to that derivation was already machine-owned,
and OUTPUTS capture is Category B by name in CLAUDE.md's side-effect table.

deriveOutputScope and extractUnitOutputs leave the CLI for core, and
outputScope/nakedOutputs leave the event. The two halves now enter through
different doors: nakedOutputs is compile-time-bound, resolved once by the
leaf-state builder from the unit's own declarations, while outputScope is
event-time-bound, read from context.forStack at fire time because its
iteration tier changes per FOR iteration. This is the split
buildArtifactResolveInput already applies one function away for ARTIFACTS
over the same forStack — core carried two parallel derivations of one
concept and drove only one of them from the machine.

The scope is built from the leaf state's own stepName/substepId rather than
from a reported cursor, which closes a real gap: a persisted substep naming
a substep that no longer exists used to fall back to a step-level scope
while the machine sat wherever it actually sat. A leaf state exists only
for a substep the compiled runbook defines, so the two can no longer
disagree.

Both moved functions drop the CLI versions' separate isSubstep boolean — a
defined substepId IS the substep tier, so the two can no longer be passed
in disagreement. deriveOutputScope also takes forStack non-optionally,
matching RunbookContext, which deletes a branch the old optional chain
carried over from RunbookState.

commandExecActor is untouched; only the source of its input changed. No
source-text guard accompanies this: the event fields are gone, so a
re-added CLI derivation has nowhere to send its result and fails to
compile.
Killing a Stryker run with workers live leaves a partially-written
stryker-incremental.json, and every subsequent scoped run then stalls
rather than failing — measured at 4/20 mutants with the ETA climbing past
17m on a scope that had just completed in seconds, reproducible across two
scopes and at both concurrency 1 and 2. --force does not rescue it: the
report is still read first. The symptom is a hang with no error, so the
recovery has to be written down.
popRunbookIfActive undid a push by calling projectRunbookRelease with no
options, so retainClaimsAsTerminal was falsy and every claim controlling
the run was revoked. The operation being undone is defaultStack.push(id).
It mints nothing and never reads session.claims, so the undo disposed of
authority the push never created.

That was irrecoverable. The pop's one caller is the inline launch
rollback, reached only when a process reclaims an interrupted launch from
a dead owner and the consume then throws. The child survives the rollback
and the next attempt resumes it, but adoptRunControlClaim refuses to
re-mint once that child has issued a delegation — so the child ran
unarmed and nothing addressed the run again. The holder was told
claim-rotated, a rotation that never happened.

projectStackPop takes the stack array rather than SessionData, and that
narrowness is the guarantee: it cannot revoke a claim or clear a stash
slot even after an edit that forgets why it must not. An option can be
omitted and omission reads as the destructive direction; deleting the
parameter is stronger than defaulting it. It removes the topmost
occurrence only, because session_stack has no uniqueness constraint and
cannot gain one under the no-migration rule.

The method stays mutateGuarded. A stack-only projection issues no guarded
statement, so both ownership refusals are now unreachable here, but they
come from one loop in mutateSessionGuarded and cannot be dropped
singly — and recovery_required is the arm the push's symmetry argument
covers least well. Removing them wants a stale-lease test and a
multi-process test, in their own commit, which this projection makes a
no-op rather than a lossy edit.
Two functions build the `StepEntryMetadata` behind a STEP_ENTERED payload
and they disagree. The CLI execution loop renders description, prompt,
commandCode and commandLang; core's `prepareCollectReEntryFrontier` fills
ids, position, name and flags and leaves every rendered field absent. All
four are optional on the type, which is what lets the disagreement compile.

Pinned against unmodified code, so #799's move reads as an assertion
flipping rather than as a new test. Three divergences, each stating which
value is the correct one:

- The rendered fields, end to end. One substep of one runbook entered
  twice — first by `rundown run`, then by the RETRY re-entry `rundown
  collect` drives — with position, stepName, isSubstep, prompted,
  hasCommand and runbookId asserted to AGREE. That agreement is what makes
  the missing description a divergence rather than a different event. The
  collect payload under-fills: a substep's description does not depend on
  which command entered it.
- `prompted`. The loop ORs `currentStep.kind === 'prompted-for'` into the
  flag it was called with; collect reads `!!advanced.prompted` alone and
  reports `false` for the same cursor on the same step. The loop's value
  is the correct one — the field documents whether execution is prompted
  rather than automatic, and the loop returns 'waiting' on that same term.
- `substepId` / `isSubstep`, inside one builder rather than between two.
  The loop takes `substepId` off the raw cursor and `isSubstep` off the
  resolved execution unit, so a cursor naming no live substep yields a
  populated `substepId` alongside `isSubstep: false`. Both answer one
  question, so both belong to the resolved unit: correct is `substepId`
  absent. This is not tidiness — the frontier seams gate credential
  disclosure on `isSubstep` while `deriveStepEnteredEffect`'s cursor guard
  fires on `substepId`.

The two unit-level halves capture the entry off the
`observeExecutionUnitEntry` argument rather than off the emitted event,
because that argument IS the builder's output and the payload is a lossy
projection of it — `substepId` never reaches the event at all.

Additive: every payload already pinned in the collection-service suite
stays exactly as it is.
The TSDoc promised that "a `substepId` that names no substep on
`currentStep` yields no declarations rather than silently falling back to
the parent's". That holds only when the step DEFINES substeps. When it
defines none, a defined `substepId` falls straight through to
`currentStep.outputs` — the parent's declarations — which is the fallback
the sentence says cannot happen. `execution-units.test.ts:131` pins that
branch deliberately, so the doc and the test contradicted each other.

The behaviour is right and stays: `resolveCurrentExecutionUnit` resolves
the same cursor to the parent step, so the unit being entered really is
the step and the step's OUTPUTS are the ones in scope. Only the promise
was too wide, and `extractUnitOutputs` is now exported from core's barrel,
so it is a promise a caller could hold the function to.

Found by review of #816.
@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your current included review allowance is based on your included PR review attempts over the past 7 days.

Next review available in: 16 minutes

Limit details: You’ve used all 2 included reviews currently available. Your 55 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.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d38ceeca-b114-4d26-a038-7331da2651e9

📥 Commits

Reviewing files that changed from the base of the PR and between 7e3fc64 and 88aeab2.

📒 Files selected for processing (29)
  • .changeset/machine-derives-output-scope.md
  • .changeset/pop-runbook-keeps-claims.md
  • .changeset/release-role-vocabulary.md
  • .prettierignore
  • CLAUDE.md
  • cspell-dictionary.txt
  • packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts
  • packages/cli/__tests__/integration/step-entered-divergence-characterisation.test.ts
  • packages/cli/__tests__/services/execution-helpers.test.ts
  • packages/cli/__tests__/services/execution-loop.test.ts
  • packages/cli/src/services/execution.ts
  • packages/core/__tests__/runbook/actor-service-compute-commit-equivalence.test.ts
  • packages/core/__tests__/runbook/actor-service-pending-effects.test.ts
  • packages/core/__tests__/runbook/actor-service.test.ts
  • packages/core/__tests__/runbook/collection-service.test.ts
  • packages/core/__tests__/runbook/compiler-command-exec.test.ts
  • packages/core/__tests__/runbook/compiler.test.ts
  • packages/core/__tests__/runbook/execution-recovery-service.test.ts
  • packages/core/__tests__/runbook/execution-units.test.ts
  • packages/core/__tests__/runbook/output-channels.test.ts
  • packages/core/__tests__/runbook/persisted-context-hygiene.properties.test.ts
  • packages/core/__tests__/runbook/session-release.test.ts
  • packages/core/__tests__/runbook/session-service.test.ts
  • packages/core/src/runbook/compiler.ts
  • packages/core/src/runbook/execution-units.ts
  • packages/core/src/runbook/index.ts
  • packages/core/src/runbook/output-channels.ts
  • packages/core/src/runbook/session-release.ts
  • packages/core/src/runbook/session-service.ts

Warning

.coderabbit.yaml has a parsing error

The CodeRabbit configuration file in this repository has a parsing error and default settings were used instead. Please fix the error(s) in the configuration file. You can initialize chat with CodeRabbit to get help with the configuration file.

💥 Parsing errors (1)
Validation error: Too big: expected string to have <=250 characters at "tone_instructions"
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

🧬 Mutation score (advisory)

Hybrid mutation analysis: source changes use changed-line scopes (dedicated tests by default; mutation:related opts into related tests), while test-only changes use native incremental comparison with the main baseline. This check is advisory and never blocks merge. Trend & full reports: the Stryker Dashboard. See issue #485.

ℹ️ Mutation scope plan

Source test selection: dedicated.

  • ⚠️ core: source and test changes are mixed; the dedicated source tier ran, but test-only regression attribution was not independently evaluated
  • ⚠️ cli/src/services/execution.ts: not scored — source change has no new-side lines; there is no changed code to mutate

⚠️ cli — test-only mutation result incomplete

Changed tests: __tests__/integration/claim-disposition-characterisation.test.ts __tests__/integration/step-entered-divergence-characterisation.test.ts __tests__/services/execution-loop.test.ts __tests__/services/execution-helpers.test.ts

No valid main-branch dashboard baseline was available. A cold full-package run was intentionally not substituted.

⚠️ core — changed-scope mutants (floor 70% shown as score context)

File Score Status
src/runbook/compiler.ts 0.00% ❌ 4 mutants
  • src/runbook/compiler.ts line 4775: 3 NoCoverage — ArrayDeclaration → ["Stryker was here"]
  • src/runbook/compiler.ts line 4775: 1 Survived — ConditionalExpression → false
  • src/runbook/compiler.ts line 4775: 0 Survived — ConditionalExpression → true
  • src/runbook/compiler.ts line 4775: 2 Survived — EqualityOperator → owningStep !== undefined

core — changed-scope mutants (floor 70% shown as score context)

File Score Status
src/runbook/execution-units.ts 100.00%

core — changed-scope mutants (floor 70% shown as score context)

File Score Status
src/runbook/index.ts ⏭️ not mutated

core — changed-scope mutants (floor 70% shown as score context)

File Score Status
src/runbook/output-channels.ts 100.00%

core — changed-scope mutants (floor 70% shown as score context)

File Score Status
src/runbook/session-release.ts 100.00%

core — changed-scope mutants (floor 70% shown as score context)

File Score Status
src/runbook/session-service.ts 100.00%

@tobyhede
tobyhede merged commit 898e6cb into main Aug 19, 2026
50 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant