From 132c9d2983f02822d3ef0f96a280eaf0e498b35e Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 18 Aug 2026 19:35:25 +1000 Subject: [PATCH 01/11] chore: exclude vendored upstream skills from Prettier .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. --- .prettierignore | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.prettierignore b/.prettierignore index 04920dd78..b8f50f823 100644 --- a/.prettierignore +++ b/.prettierignore @@ -7,6 +7,10 @@ # Skills are functional LLM-facing instructions, content-pinned by tests — # reflowing their prose breaks substring assertions (skills/*.test.ts). packages/claude-code-plugin/skills/** +# Vendored skills installed from upstream repos and pinned by content hash in +# skills-lock.json — reformatting them invalidates the lock. (.claude/skills/* +# are symlinks into this tree; Prettier does not follow them.) +.agents/skills/** **/dist/** **/coverage/** packages/parser/fixtures/** From 3e026220719cfe3c47858e0b15c914674fc04c0a Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 18 Aug 2026 19:40:39 +1000 Subject: [PATCH 02/11] test(cli): characterise today's terminal claim disposition (#790) 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. --- ...claim-disposition-characterisation.test.ts | 192 ++++++++++++++++++ 1 file changed, 192 insertions(+) create mode 100644 packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts diff --git a/packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts b/packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts new file mode 100644 index 000000000..c576af4e2 --- /dev/null +++ b/packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts @@ -0,0 +1,192 @@ +import { describe, it, expect, beforeEach, afterEach } from '@jest/globals'; +import { writeFile } from 'node:fs/promises'; +import { join } from 'node:path'; +import { RunbookStateManager, SessionService, assertClaimId } from '@rundown-org/core'; +import { + createTestWorkspace, + createRunbook, + runCli, + parseJsonEvents, + parseCliJsonObject, + type TestWorkspace, +} from '../helpers/test-utils.js'; + +/** + * Characterisation of today's terminal claim disposition (#781). + * + * These tests pin CURRENT behaviour, including the defect. They assert the + * answer an orchestrator gets back at the *resolution* seam — the status a + * presented run-control bearer resolves to — rather than the shape stored in + * the session projection, because the projection is what #792 rewrites and the + * resolution is what a caller actually observes. + * + * Two of the three assertions are deliberately asymmetric: + * + * - The already-terminal loop entry currently resolves `superseded` / + * `claim-rotated`. That is the #781 defect. The assertion flips to `terminal` + * in #793, and it must be green *before* that change so the flip is visible + * as a one-line diff rather than as a new test. + * - The fenced completion currently resolves `terminal`, and must keep doing so + * at every point in the cluster. It is the control. + * + * The trigger for the defective path has to be an already-terminal *loop + * entry*. A plain completion does not reach the revoking release at all: the + * fenced command mutation releases with retention and the loop returns before + * the entry-time terminal check, so a characterisation test built on a plain + * completion passes under both the old and the new code — for the wrong reason. + * A delegation whose child runbook is not discoverable is the cheapest trigger: + * it stops the run during initialization and re-enters the loop already + * terminal. + */ +describe('Terminal claim disposition at the resolution seam (#781 characterisation)', () => { + let workspace: TestWorkspace; + + beforeEach(async () => { + workspace = await createTestWorkspace(); + }); + + afterEach(async () => { + await workspace.cleanup(); + }); + + /** The run-control bearer and run id a `rundown run` invocation emitted. */ + interface StartedRun { + readonly claimId: string; + readonly runId: string; + } + + /** + * Read the run-control bearer off `runbook_started`. + * + * That event is the sole delivery surface for the bearer — it is never + * recoverable from persisted state — so every test here starts by capturing + * it from the subprocess that minted it. + */ + function requireStartedRun(stdout: string): StartedRun { + const started = parseJsonEvents(stdout).find((event) => event.type === 'runbook_started'); + if (started === undefined) throw new Error(`no runbook_started event in: ${stdout}`); + const { claim_id: claimId, runbookId } = started; + if (typeof claimId !== 'string') throw new Error('runbook_started carried no claim_id'); + if (typeof runbookId !== 'string') throw new Error('runbook_started carried no runbookId'); + return { claimId, runId: runbookId }; + } + + /** Resolve a bearer through core, in this process, against the workspace db. */ + async function resolveClaim(claimId: string) { + const session = new SessionService(new RunbookStateManager(workspace.cwd)); + return session.getActiveForClaimId(assertClaimId(claimId)); + } + + /** A parent whose only substep delegates to a runbook that does not exist. */ + async function writeUnresolvableDelegation(): Promise { + const content = [ + '# Parent', + '', + '## 1. Fan-out', + '', + '- DELEGATE', + '- PASS ALL COMPLETE', + '- FAIL ANY STOP', + '', + '### 1.1 Task A', + '', + '- missing-child.runbook.md', + '', + ].join('\n'); + await writeFile(join(workspace.cwd, 'parent.runbook.md'), content); + } + + /** A single-step runbook that completes through a fenced command. */ + async function writeFencedCompletion(): Promise { + const content = createRunbook({ + title: 'Control', + steps: [{ title: 'Do a thing', pass: 'COMPLETE', fail: 'STOP', command: 'true' }], + }); + await writeFile(join(workspace.cwd, 'control.runbook.md'), content); + } + + it('leaves an already-terminal loop entry resolving superseded / claim-rotated', async () => { + // THE DEFECT. `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 encodes "this run is unclaimed" — a premise + // the minting falsified. The claim is revoked, and its holder is told to + // claim a delegation that does not exist. + await writeUnresolvableDelegation(); + + const run = runCli('run parent.runbook.md', workspace); + const started = requireStartedRun(run.stdout); + expect(parseJsonEvents(run.stdout).map((event) => event.type)).toContain('runbook_stopped'); + expect( + parseJsonEvents(run.stdout).find((event) => event.type === 'runbook_stopped'), + ).toMatchObject({ reason: 'delegation_resolution_failed' }); + + // The precise reason, which the CLI envelope does not carry. + await expect(resolveClaim(started.claimId)).resolves.toMatchObject({ + status: 'superseded', + reason: 'claim-rotated', + }); + + // And what the orchestrator is actually told. `claim-rotated` and + // `parent-unreadable` share this code, so the message is what separates + // them — it names a delegation to re-claim that was never issued. + const status = runCli(`status --claim-id ${started.claimId}`, workspace); + expect(status.exitCode).toBe(1); + const envelope = parseCliJsonObject(status.stdout || status.stderr); + expect(envelope).toMatchObject({ kind: 'error', code: 'CLAIMED_RUNBOOK_UNAVAILABLE' }); + expect(String(envelope.error)).toContain('was released or replaced and is no longer authority'); + }); + + it('leaves a run that completed through a fenced command resolving terminal', async () => { + // THE CONTROL. This assertion must not move at any point in the #781 + // cluster: the fence already releases with retention, and #793 makes the + // loop-entry path agree with it rather than the other way round. + await writeFencedCompletion(); + + const run = runCli('run control.runbook.md --allow-all', workspace); + const started = requireStartedRun(run.stdout); + expect(parseJsonEvents(run.stdout).map((event) => event.type)).toContain('runbook_completed'); + + await expect(resolveClaim(started.claimId)).resolves.toMatchObject({ + status: 'terminal', + lifecycle: 'completed', + }); + + const status = runCli(`status --claim-id ${started.claimId}`, workspace); + expect(status.exitCode).toBe(0); + expect(parseCliJsonObject(status.stdout)).toMatchObject({ + status: 'completed', + active: false, + runId: started.runId, + }); + }); + + it('carries both dispositions across a process boundary', async () => { + // Every assertion above already crosses one boundary (the run is a + // subprocess, the resolution is in-process), but the resolution itself must + // also work from a *third* process that shares nothing but the database: + // a unit suite mocking the session boundary cannot observe either the + // retained claim or the tombstone surviving persistence. + await writeUnresolvableDelegation(); + await writeFencedCompletion(); + + const revoked = requireStartedRun(runCli('run parent.runbook.md', workspace).stdout); + const retained = requireStartedRun( + runCli('run control.runbook.md --allow-all', workspace).stdout, + ); + + // Separate process, separate connection, no shared memory with either run. + const revokedStatus = runCli(`status --claim-id ${revoked.claimId}`, workspace); + expect(revokedStatus.exitCode).toBe(1); + expect(parseCliJsonObject(revokedStatus.stdout || revokedStatus.stderr)).toMatchObject({ + kind: 'error', + code: 'CLAIMED_RUNBOOK_UNAVAILABLE', + }); + + const retainedStatus = runCli(`status --claim-id ${retained.claimId}`, workspace); + expect(retainedStatus.exitCode).toBe(0); + expect(parseCliJsonObject(retainedStatus.stdout)).toMatchObject({ + status: 'completed', + runId: retained.runId, + }); + }); +}); From 529c1f532677c0dd994d88bf9787578d7dd4f40d Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 18 Aug 2026 19:52:43 +1000 Subject: [PATCH 03/11] feat(core): add the ReleaseRole vocabulary, with no callers (#790) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .changeset/release-role-vocabulary.md | 45 ++++ cspell-dictionary.txt | 1 + .../__tests__/runbook/session-release.test.ts | 255 ++++++++++++++++++ packages/core/src/runbook/index.ts | 1 + packages/core/src/runbook/session-release.ts | 127 +++++++++ 5 files changed, 429 insertions(+) create mode 100644 .changeset/release-role-vocabulary.md create mode 100644 packages/core/__tests__/runbook/session-release.test.ts create mode 100644 packages/core/src/runbook/session-release.ts diff --git a/.changeset/release-role-vocabulary.md b/.changeset/release-role-vocabulary.md new file mode 100644 index 000000000..fa2ec7a58 --- /dev/null +++ b/.changeset/release-role-vocabulary.md @@ -0,0 +1,45 @@ +--- +'@rundown-org/core': minor +'@rundown-org/cli': patch +--- + +# Add the ReleaseRole vocabulary, and characterise today's terminal claim disposition + +New in core, with no callers yet: `ReleaseRole` +(`addressed | collateral | discarded`), `ClaimDisposition` +(`retain-as-terminal-evidence | revoke`), `claimDisposition(role)`, `RunRelease` +and `projectRunRelease(session, release)`, in +`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 of them +agree, and they agree on a rule none of them states: the run the caller acted +_on_ keeps its claim as terminal evidence, and a run swept up so that the +addressed run could close does not. The sixteenth omits the option, and omission +reads as the destructive direction, so a run that reached terminal has its +run-control claim revoked and its holder is told to re-claim a delegation that +was never issued. + +The new vocabulary asks for the **fact** the caller already holds instead — "I +addressed this run" — and owns the conclusion itself, which makes that whole bug +class unrepresentable: there is no option left to omit. `discarded` is a +distinct arm rather than a synonym for `collateral` because the destroy paths +must never be spelled `addressed`, which would retain claims over a run that is +about to stop existing. + +`claimDisposition` takes the role alone, and 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. That is what lets it widen +to `claimDisposition(role, claim)` later — when a run-control claim and a +delegated bearer over the same run want different treatment — without touching a +caller. `projectRunRelease` is synchronous and mutates in place by requirement, +because several dispositions reach the projection through a session callback +that accepts nothing else. + +Also adds CLI integration tests characterising today's disposition at the +resolution seam, so the behaviour change that follows is visible as a one-line +diff. They record that an already-terminal loop entry resolves `superseded` / +`claim-rotated`, that a run completing through a fenced command resolves +`terminal`, and that both survive a process boundary. Nothing calls the new +vocabulary yet, so no behaviour changes here. diff --git a/cspell-dictionary.txt b/cspell-dictionary.txt index 0799ba7cb..c9b8cceae 100644 --- a/cspell-dictionary.txt +++ b/cspell-dictionary.txt @@ -476,3 +476,4 @@ lstart unaddressable decorrelate decorrelates +characterisation diff --git a/packages/core/__tests__/runbook/session-release.test.ts b/packages/core/__tests__/runbook/session-release.test.ts new file mode 100644 index 000000000..003526929 --- /dev/null +++ b/packages/core/__tests__/runbook/session-release.test.ts @@ -0,0 +1,255 @@ +import { describe, expect, it } from '@jest/globals'; +import fc from 'fast-check'; +import { + RELEASE_ROLES, + claimDisposition, + projectRunRelease, + type ReleaseRole, + type RunRelease, +} from '../../src/runbook/session-release.js'; +import { assertRunId, type RunId } from '../../src/runbook/run-id.js'; +import { assertClaimLookupKey } from '../../src/runbook/claim-id.js'; +import { makeClaimRecord } from '../../src/testing/claim-fixtures.js'; +import type { SessionData } from '../../src/runbook/state.js'; + +/** Distinct, canonical run ids. `n` must stay single-digit-hex wide. */ +function runId(n: number): RunId { + return assertRunId(`rd_${n.toString(16).repeat(32)}`); +} + +/** A claim controlling `run`, keyed distinctly so several can coexist. */ +function claimFor(run: RunId, key: string) { + return makeClaimRecord({ + claimKey: assertClaimLookupKey(`rdclk_${key.repeat(32).slice(0, 32)}`), + controlledRunId: run, + }); +} + +/** A session with `runs` stacked bottom-to-top and one claim over each. */ +function sessionOver(runs: readonly RunId[]): SessionData { + const claims: SessionData['claims'] = {}; + runs.forEach((run, index) => { + const record = claimFor(run, index.toString(16)); + claims[record.claimKey] = record; + }); + return { defaultStack: [...runs], claims }; +} + +describe('claimDisposition', () => { + // The latent rule the codebase already followed at fifteen of sixteen sites, + // stated once: the run the caller acted ON keeps its claim as terminal + // evidence; a run swept up so the addressed run could close, or one being + // destroyed outright, does not. + it('retains an addressed run’s claims as terminal evidence', () => { + expect(claimDisposition('addressed')).toBe('retain-as-terminal-evidence'); + }); + + it('revokes a collateral run’s claims', () => { + expect(claimDisposition('collateral')).toBe('revoke'); + }); + + it('revokes a discarded run’s claims', () => { + // Distinct from `collateral` despite agreeing today: a destroy path must + // never be spelled `addressed`, which would retain claims for a run that is + // being deleted. + expect(claimDisposition('discarded')).toBe('revoke'); + }); + + it('is total over every role', () => { + for (const role of RELEASE_ROLES) { + expect(['retain-as-terminal-evidence', 'revoke']).toContain(claimDisposition(role)); + } + }); + + it('names exactly the three roles', () => { + expect([...RELEASE_ROLES]).toEqual(['addressed', 'collateral', 'discarded']); + }); +}); + +describe('projectRunRelease', () => { + it('removes the run from the default stack', () => { + const [a, b] = [runId(1), runId(2)]; + const session = sessionOver([a, b]); + + expect(projectRunRelease(session, { runId: b, role: 'addressed' })).toBe(true); + + expect(session.defaultStack).toEqual([a]); + }); + + it('removes every occurrence of the run from the default stack', () => { + // A duplicate entry is reachable (§6.3 of the release-cause note refuses to + // add UNIQUE(run_id) to session_stack), and a release must not leave one + // behind for the next read to resolve as still-active. + const [a, b] = [runId(1), runId(2)]; + const session = sessionOver([a, b]); + session.defaultStack = [a, b, a]; + + projectRunRelease(session, { runId: a, role: 'addressed' }); + + expect(session.defaultStack).toEqual([b]); + }); + + it('leaves an addressed run’s claims in the session', () => { + const a = runId(1); + const session = sessionOver([a]); + const key = Object.keys(session.claims)[0]; + + projectRunRelease(session, { runId: a, role: 'addressed' }); + + expect(session.claims[key]).toBeDefined(); + expect(session.claims[key].controlledRunId).toBe(a); + }); + + it.each(['collateral', 'discarded'] as const)('deletes a %s run’s claims', (role) => { + const a = runId(1); + const session = sessionOver([a]); + + projectRunRelease(session, { runId: a, role }); + + expect(session.claims).toEqual({}); + }); + + it('never touches a claim over a different run', () => { + const [a, b] = [runId(1), runId(2)]; + const session = sessionOver([a, b]); + const other = Object.values(session.claims).find((c) => c.controlledRunId === b); + + projectRunRelease(session, { runId: a, role: 'discarded' }); + + expect(Object.values(session.claims)).toEqual([other]); + }); + + it('clears the stash slot when it names the released run', () => { + const a = runId(1); + const session = sessionOver([a]); + session.stashedRunbookId = a; + + expect(projectRunRelease(session, { runId: a, role: 'addressed' })).toBe(true); + + expect(session.stashedRunbookId).toBeUndefined(); + }); + + it('leaves a stash slot naming a different run alone', () => { + const [a, b] = [runId(1), runId(2)]; + const session = sessionOver([a, b]); + session.stashedRunbookId = b; + + projectRunRelease(session, { runId: a, role: 'addressed' }); + + expect(session.stashedRunbookId).toBe(b); + }); + + it('reports false when the run appears nowhere in the session', () => { + const session = sessionOver([runId(1)]); + + expect(projectRunRelease(session, { runId: runId(9), role: 'addressed' })).toBe(false); + }); + + it('reports true when only a retained claim matched', () => { + // The retained claim still counts as "found". Preserved deliberately from + // the primitive this replaces: a repeated `addressed` release reports + // found, not not-found, because the claim it retained is still there. + const a = runId(1); + const session = sessionOver([a]); + session.defaultStack = []; + + expect(projectRunRelease(session, { runId: a, role: 'addressed' })).toBe(true); + }); + + it('changes nothing on a second application', () => { + const [a, b] = [runId(1), runId(2)]; + for (const role of ['addressed', 'collateral', 'discarded'] as const) { + const session = sessionOver([a, b]); + session.stashedRunbookId = a; + const release: RunRelease = { runId: a, role }; + + projectRunRelease(session, release); + const afterFirst = structuredClone(session); + projectRunRelease(session, release); + + expect(session).toEqual(afterFirst); + } + }); + + it('mutates the caller’s session object in place, synchronously', () => { + // Load-bearing, and not merely "pure inside a mutateState build callback": + // six dispositions reach this projection through a synchronous in-place + // session callback that accepts nothing else. + const a = runId(1); + const session = sessionOver([a]); + const before = session; + + const result = projectRunRelease(session, { runId: a, role: 'addressed' }); + + expect(session).toBe(before); + expect(typeof result).toBe('boolean'); + }); +}); + +describe('projectRunRelease order-independence (property)', () => { + const role = fc.constantFrom('addressed', 'collateral', 'discarded'); + + it('gives a member the same outcome under any permutation of the others', () => { + // The invariant that lets `claimDisposition(role)` widen to + // `claimDisposition(role, claim)` later without touching a caller: a run's + // disposition depends only on its OWN role, never on ordering and never on + // the other members of the batch. + fc.assert( + fc.property( + fc.uniqueArray(fc.integer({ min: 1, max: 8 }), { minLength: 2, maxLength: 5 }), + fc.array(role, { minLength: 5, maxLength: 5 }), + fc.nat(), + (ids, roles, pick) => { + const runs = ids.map(runId); + const releases: RunRelease[] = runs.map((run, index) => ({ + runId: run, + role: roles[index % roles.length], + })); + const subject = releases[pick % releases.length]; + + const apply = (order: readonly RunRelease[]): SessionData => { + const session = sessionOver(runs); + for (const release of order) projectRunRelease(session, release); + return session; + }; + + const forward = apply(releases); + const reversed = apply([...releases].reverse()); + + // The subject's own claim survives, or does not, identically. + const survives = (session: SessionData): boolean => + Object.values(session.claims).some((c) => c.controlledRunId === subject.runId); + + expect(survives(reversed)).toBe(survives(forward)); + expect(survives(forward)).toBe( + claimDisposition(subject.role) === 'retain-as-terminal-evidence', + ); + }, + ), + ); + }); + + it('reaches the same whole session under any permutation', () => { + fc.assert( + fc.property( + fc.uniqueArray(fc.integer({ min: 1, max: 8 }), { minLength: 2, maxLength: 5 }), + fc.array(role, { minLength: 5, maxLength: 5 }), + (ids, roles) => { + const runs = ids.map(runId); + const releases: RunRelease[] = runs.map((run, index) => ({ + runId: run, + role: roles[index % roles.length], + })); + + const apply = (order: readonly RunRelease[]): SessionData => { + const session = sessionOver(runs); + for (const release of order) projectRunRelease(session, release); + return session; + }; + + expect(apply([...releases].reverse())).toEqual(apply(releases)); + }, + ), + ); + }); +}); diff --git a/packages/core/src/runbook/index.ts b/packages/core/src/runbook/index.ts index 722e51576..889685c0e 100644 --- a/packages/core/src/runbook/index.ts +++ b/packages/core/src/runbook/index.ts @@ -120,6 +120,7 @@ export * from './claim-activity.js'; export * from './claim-id.js'; export * from './duration.js'; export * from './last-action.js'; +export * from './session-release.js'; export * from './transition-kernel.js'; // `manual-delegation-machine.js` is deliberately NOT barrelled. Publishing // `prepareManualDelegation` would invite a front end to drive delegation around diff --git a/packages/core/src/runbook/session-release.ts b/packages/core/src/runbook/session-release.ts new file mode 100644 index 000000000..d8da64ee2 --- /dev/null +++ b/packages/core/src/runbook/session-release.ts @@ -0,0 +1,127 @@ +import type { RunId } from './run-id.js'; +import type { SessionData } from './state.js'; + +/** + * Why a run is being released from the session. + * + * A **fact the caller holds**, not a policy it chooses. The caller knows which + * run it acted on; converting that into "should this claim survive?" is domain + * logic, and this module owns it. Sixteen call sites each performing that + * conversion for themselves is what let one of them convert differently. + * + * - `addressed` — the caller acted on this run. It reached terminal, or was + * commanded terminal, and the release is the caller finishing with it. + * - `collateral` — the run was swept up so that an addressed run could close. + * An inline descendant forced terminal under its root, for instance. + * - `discarded` — the run is being destroyed. `prune` and the cleanup paths. + * Required as its own arm: spelling a destroy path `addressed` would retain + * claims over a run that is about to stop existing. + */ +export type ReleaseRole = 'addressed' | 'collateral' | 'discarded'; + +/** + * Every {@link ReleaseRole}, for exhaustive iteration in tests and callers. + * + * Declared `as const` and typed by its own members, so adding an arm to + * `ReleaseRole` without adding it here is a compile error rather than a silently + * shorter loop. + */ +export const RELEASE_ROLES = [ + 'addressed', + 'collateral', + 'discarded', +] as const satisfies readonly ReleaseRole[]; + +/** + * What a release does to the claims a run controls. + * + * `retain-as-terminal-evidence` writes nothing: the claim record stays in the + * session and its row stays active, so a holder presenting the bearer afterwards + * resolves `terminal` and learns the run finished. `revoke` deletes the record, + * and the store tombstones the row `superseded` — a holder presenting the bearer + * is then told its authority was rotated. + * + * Deliberately not called `tombstone`. The tombstone is the artefact **revoking** + * produces; naming the retained case after it is the collision this vocabulary + * exists to remove. + */ +export type ClaimDisposition = 'retain-as-terminal-evidence' | 'revoke'; + +/** + * Decide what a release does to one run's claims. + * + * The whole policy, in one place. Retention is the recoverable direction — a + * retained claim is garbage-collected when its run is pruned, whereas a + * revocation cannot be reconstructed — and it is also the majority case, so the + * fail-safe answer and the common answer coincide. + * + * Takes the role alone. That a run's disposition depends only on its own role, + * never on ordering and never on the other members of a batch, is the invariant + * that lets this widen to `claimDisposition(role, claim)` later — when a + * run-control claim and a delegated bearer over the same run want different + * treatment — without touching a caller. + * + * @param role - Why the run is being released. + * @returns What to do with the claims that run controls. + */ +export function claimDisposition(role: ReleaseRole): ClaimDisposition { + switch (role) { + case 'addressed': + return 'retain-as-terminal-evidence'; + case 'collateral': + case 'discarded': + return 'revoke'; + default: { + const _exhaustive: never = role; + return _exhaustive; + } + } +} + +/** One run's release, and the fact that explains it. */ +export interface RunRelease { + /** Run leaving the session's targeting structures. */ + readonly runId: RunId; + /** Why, which decides the claim disposition. */ + readonly role: ReleaseRole; +} + +/** + * Project one release onto an in-memory session snapshot, in place. + * + * Removes the run from every session structure that targets it — the default + * stack (all occurrences, since a duplicate entry is reachable), the stash slot, + * and, when the role revokes, the claims it controls. + * + * **Synchronous and in-place** by requirement, not by preference. Several + * dispositions reach this projection through a session callback that accepts a + * synchronous in-place mutation and nothing else, so this must never become + * async or start returning a new snapshot. + * + * @param session - Session snapshot, mutated in place. + * @param release - The run to release, and why. + * @returns Whether the run was present in any session structure. A retained + * claim counts as present, so re-applying an `addressed` release still + * reports `true` — it finds the claim it retained. Nothing in the tree + * branches on this beyond distinguishing "released" from "not found". + */ +export function projectRunRelease(session: SessionData, release: RunRelease): boolean { + const { runId, role } = release; + + const stackLengthBefore = session.defaultStack.length; + session.defaultStack = session.defaultStack.filter((id) => id !== runId); + const removedFromDefaultStack = session.defaultStack.length !== stackLengthBefore; + + const revoking = claimDisposition(role) === 'revoke'; + let matchedClaim = false; + for (const [claimKey, claim] of Object.entries(session.claims)) { + if (claim.controlledRunId !== runId) continue; + matchedClaim = true; + if (revoking) delete session.claims[claimKey]; + } + + const removedFromStash = session.stashedRunbookId === runId; + if (removedFromStash) session.stashedRunbookId = undefined; + + return removedFromDefaultStack || matchedClaim || removedFromStash; +} From 3ed3d00afae19e1cea23031d95ce1f5dbcc217bb Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 18 Aug 2026 20:13:21 +1000 Subject: [PATCH 04/11] test(core): close the two mutation gaps in session-release (#790) `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. --- .../core/__tests__/runbook/session-release.test.ts | 12 ++++++++++++ packages/core/src/runbook/session-release.ts | 1 + 2 files changed, 13 insertions(+) diff --git a/packages/core/__tests__/runbook/session-release.test.ts b/packages/core/__tests__/runbook/session-release.test.ts index 003526929..91b32a762 100644 --- a/packages/core/__tests__/runbook/session-release.test.ts +++ b/packages/core/__tests__/runbook/session-release.test.ts @@ -145,6 +145,18 @@ describe('projectRunRelease', () => { expect(projectRunRelease(session, { runId: runId(9), role: 'addressed' })).toBe(false); }); + it('reports true when only the default stack matched', () => { + // Stack membership alone is enough to have "found" the run. Without this + // the other cases all carry a claim or a stash entry as well, so the stack + // arm of the answer is never the one deciding it. + const a = runId(1); + const session: SessionData = { defaultStack: [a], claims: {} }; + + expect(projectRunRelease(session, { runId: a, role: 'addressed' })).toBe(true); + + expect(session.defaultStack).toEqual([]); + }); + it('reports true when only a retained claim matched', () => { // The retained claim still counts as "found". Preserved deliberately from // the primitive this replaces: a repeated `addressed` release reports diff --git a/packages/core/src/runbook/session-release.ts b/packages/core/src/runbook/session-release.ts index d8da64ee2..66c34bb6f 100644 --- a/packages/core/src/runbook/session-release.ts +++ b/packages/core/src/runbook/session-release.ts @@ -71,6 +71,7 @@ export function claimDisposition(role: ReleaseRole): ClaimDisposition { case 'collateral': case 'discarded': return 'revoke'; + // Stryker disable next-line ConditionalExpression,BlockStatement: unreachable — exhaustive `never` arm default: { const _exhaustive: never = role; return _exhaustive; From 9aeecbd24774978829d88d847a436ffeddf9c3df Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 18 Aug 2026 20:37:15 +1000 Subject: [PATCH 05/11] chore: widen the Prettier exclusion to the whole vendored root .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. --- .prettierignore | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/.prettierignore b/.prettierignore index b8f50f823..9e6f8354c 100644 --- a/.prettierignore +++ b/.prettierignore @@ -7,10 +7,12 @@ # Skills are functional LLM-facing instructions, content-pinned by tests — # reflowing their prose breaks substring assertions (skills/*.test.ts). packages/claude-code-plugin/skills/** -# Vendored skills installed from upstream repos and pinned by content hash in -# skills-lock.json — reformatting them invalidates the lock. (.claude/skills/* -# are symlinks into this tree; Prettier does not follow them.) -.agents/skills/** +# Vendored agent tooling installed from upstream repos and pinned by content +# hash in skills-lock.json — reformatting it invalidates the lock. Scoped to the +# whole install root, not just skills/, so a plugin shipping Markdown does not +# reintroduce the blocker. (.claude/skills/* are symlinks into this tree; +# Prettier does not follow them.) +.agents/** **/dist/** **/coverage/** packages/parser/fixtures/** From 463c511aa61e71737fb8bc6d9fc424060adf8e3b Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 18 Aug 2026 20:37:16 +1000 Subject: [PATCH 06/11] test(core,cli): close four review findings on the #790 vocabulary Review of 132c9d298..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. --- ...claim-disposition-characterisation.test.ts | 39 ++++------------- .../__tests__/runbook/session-release.test.ts | 42 ++++++++++++++++--- packages/core/src/runbook/session-release.ts | 20 +++++++-- 3 files changed, 62 insertions(+), 39 deletions(-) diff --git a/packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts b/packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts index c576af4e2..51218169d 100644 --- a/packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts +++ b/packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts @@ -37,6 +37,15 @@ import { * A delegation whose child runbook is not discoverable is the cheapest trigger: * it stops the run during initialization and re-enters the loop already * terminal. + * + * Both tests discharge #790's multi-process requirement inline rather than in a + * third case: the run is one subprocess, `resolveClaim` reads the database from + * this one, and the `rundown status --claim-id` assertion is a further + * subprocess sharing nothing with the run but the database. A unit suite that + * mocks the session boundary observes neither the retained claim nor the + * tombstone surviving persistence; these do. A separate combined test was + * removed as a strictly weaker restatement of the two below — it duplicated the + * assertion #793 has to flip, in a place a reader would not think to look. */ describe('Terminal claim disposition at the resolution seam (#781 characterisation)', () => { let workspace: TestWorkspace; @@ -159,34 +168,4 @@ describe('Terminal claim disposition at the resolution seam (#781 characterisati runId: started.runId, }); }); - - it('carries both dispositions across a process boundary', async () => { - // Every assertion above already crosses one boundary (the run is a - // subprocess, the resolution is in-process), but the resolution itself must - // also work from a *third* process that shares nothing but the database: - // a unit suite mocking the session boundary cannot observe either the - // retained claim or the tombstone surviving persistence. - await writeUnresolvableDelegation(); - await writeFencedCompletion(); - - const revoked = requireStartedRun(runCli('run parent.runbook.md', workspace).stdout); - const retained = requireStartedRun( - runCli('run control.runbook.md --allow-all', workspace).stdout, - ); - - // Separate process, separate connection, no shared memory with either run. - const revokedStatus = runCli(`status --claim-id ${revoked.claimId}`, workspace); - expect(revokedStatus.exitCode).toBe(1); - expect(parseCliJsonObject(revokedStatus.stdout || revokedStatus.stderr)).toMatchObject({ - kind: 'error', - code: 'CLAIMED_RUNBOOK_UNAVAILABLE', - }); - - const retainedStatus = runCli(`status --claim-id ${retained.claimId}`, workspace); - expect(retainedStatus.exitCode).toBe(0); - expect(parseCliJsonObject(retainedStatus.stdout)).toMatchObject({ - status: 'completed', - runId: retained.runId, - }); - }); }); diff --git a/packages/core/__tests__/runbook/session-release.test.ts b/packages/core/__tests__/runbook/session-release.test.ts index 91b32a762..bed3e1a3a 100644 --- a/packages/core/__tests__/runbook/session-release.test.ts +++ b/packages/core/__tests__/runbook/session-release.test.ts @@ -17,10 +17,17 @@ function runId(n: number): RunId { return assertRunId(`rd_${n.toString(16).repeat(32)}`); } -/** A claim controlling `run`, keyed distinctly so several can coexist. */ -function claimFor(run: RunId, key: string) { +/** + * A claim controlling `run`, keyed distinctly so several can coexist. + * + * The index is zero-padded rather than repeated: `key.repeat(32).slice(0, 32)` + * collides for any two indices whose hex digits repeat to the same 32 characters + * (1 and 17, say), which would silently overwrite one claim with another and + * leave a larger session asserting against fewer claims than it created. + */ +function claimFor(run: RunId, index: number) { return makeClaimRecord({ - claimKey: assertClaimLookupKey(`rdclk_${key.repeat(32).slice(0, 32)}`), + claimKey: assertClaimLookupKey(`rdclk_${index.toString(16).padStart(32, '0')}`), controlledRunId: run, }); } @@ -29,7 +36,7 @@ function claimFor(run: RunId, key: string) { function sessionOver(runs: readonly RunId[]): SessionData { const claims: SessionData['claims'] = {}; runs.forEach((run, index) => { - const record = claimFor(run, index.toString(16)); + const record = claimFor(run, index); claims[record.claimKey] = record; }); return { defaultStack: [...runs], claims }; @@ -198,8 +205,26 @@ describe('projectRunRelease', () => { }); }); +/** + * Permute `items` by a seed array, Fisher-Yates. + * + * Reversal alone is two of up to 120 orderings, and cross-talk that depends on + * an interior ordering rather than a full reversal survives it. Driving the + * shuffle from generated seeds tests the invariant the properties actually + * state. + */ +function permute(items: readonly T[], seeds: readonly number[]): T[] { + const out = [...items]; + for (let i = out.length - 1; i > 0; i--) { + const j = seeds[i % seeds.length] % (i + 1); + [out[i], out[j]] = [out[j], out[i]]; + } + return out; +} + describe('projectRunRelease order-independence (property)', () => { const role = fc.constantFrom('addressed', 'collateral', 'discarded'); + const seeds = fc.array(fc.nat(), { minLength: 5, maxLength: 5 }); it('gives a member the same outcome under any permutation of the others', () => { // The invariant that lets `claimDisposition(role)` widen to @@ -211,7 +236,8 @@ describe('projectRunRelease order-independence (property)', () => { fc.uniqueArray(fc.integer({ min: 1, max: 8 }), { minLength: 2, maxLength: 5 }), fc.array(role, { minLength: 5, maxLength: 5 }), fc.nat(), - (ids, roles, pick) => { + seeds, + (ids, roles, pick, order) => { const runs = ids.map(runId); const releases: RunRelease[] = runs.map((run, index) => ({ runId: run, @@ -227,12 +253,14 @@ describe('projectRunRelease order-independence (property)', () => { const forward = apply(releases); const reversed = apply([...releases].reverse()); + const shuffled = apply(permute(releases, order)); // The subject's own claim survives, or does not, identically. const survives = (session: SessionData): boolean => Object.values(session.claims).some((c) => c.controlledRunId === subject.runId); expect(survives(reversed)).toBe(survives(forward)); + expect(survives(shuffled)).toBe(survives(forward)); expect(survives(forward)).toBe( claimDisposition(subject.role) === 'retain-as-terminal-evidence', ); @@ -246,7 +274,8 @@ describe('projectRunRelease order-independence (property)', () => { fc.property( fc.uniqueArray(fc.integer({ min: 1, max: 8 }), { minLength: 2, maxLength: 5 }), fc.array(role, { minLength: 5, maxLength: 5 }), - (ids, roles) => { + seeds, + (ids, roles, order) => { const runs = ids.map(runId); const releases: RunRelease[] = runs.map((run, index) => ({ runId: run, @@ -260,6 +289,7 @@ describe('projectRunRelease order-independence (property)', () => { }; expect(apply([...releases].reverse())).toEqual(apply(releases)); + expect(apply(permute(releases, order))).toEqual(apply(releases)); }, ), ); diff --git a/packages/core/src/runbook/session-release.ts b/packages/core/src/runbook/session-release.ts index 66c34bb6f..400eb2f7c 100644 --- a/packages/core/src/runbook/session-release.ts +++ b/packages/core/src/runbook/session-release.ts @@ -22,9 +22,11 @@ export type ReleaseRole = 'addressed' | 'collateral' | 'discarded'; /** * Every {@link ReleaseRole}, for exhaustive iteration in tests and callers. * - * Declared `as const` and typed by its own members, so adding an arm to - * `ReleaseRole` without adding it here is a compile error rather than a silently - * shorter loop. + * `satisfies readonly ReleaseRole[]` checks only that the members listed are + * *assignable* to the union, not that they *exhaust* it. The coverage half is + * asserted by {@link UnlistedReleaseRole} below; without it, an arm added to + * `ReleaseRole` and not to this constant would leave the constant silently + * short and every test iterating it silently partial. */ export const RELEASE_ROLES = [ 'addressed', @@ -32,6 +34,18 @@ export const RELEASE_ROLES = [ 'discarded', ] as const satisfies readonly ReleaseRole[]; +/** Resolves to `T` only while `T` is `never`; a compile error otherwise. */ +type AssertNever = T; + +/** + * Compile-time proof that {@link RELEASE_ROLES} names every {@link ReleaseRole}. + * + * `never` while the constant is complete. Add an arm to `ReleaseRole` without + * adding it here and this alias stops satisfying {@link AssertNever}, so the + * omission fails the type check instead of silently shortening a loop. + */ +export type UnlistedReleaseRole = AssertNever>; + /** * What a release does to the claims a run controls. * From 526ea44856f91fdaaadb6533f6fa819f3dc5bc46 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Wed, 19 Aug 2026 08:22:51 +1000 Subject: [PATCH 07/11] refactor(core,cli): derive the OUTPUTS scope behind the state machine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .changeset/machine-derives-output-scope.md | 54 ++++ .../services/execution-helpers.test.ts | 300 ------------------ .../__tests__/services/execution-loop.test.ts | 8 +- packages/cli/src/services/execution.ts | 72 ----- ...service-compute-commit-equivalence.test.ts | 4 - .../actor-service-pending-effects.test.ts | 2 - .../__tests__/runbook/actor-service.test.ts | 12 - .../runbook/compiler-command-exec.test.ts | 87 ++++- .../core/__tests__/runbook/compiler.test.ts | 4 - .../execution-recovery-service.test.ts | 2 - .../__tests__/runbook/execution-units.test.ts | 85 ++++- .../__tests__/runbook/output-channels.test.ts | 55 ++++ ...rsisted-context-hygiene.properties.test.ts | 2 - packages/core/src/runbook/compiler.ts | 45 ++- packages/core/src/runbook/execution-units.ts | 24 ++ packages/core/src/runbook/index.ts | 3 +- packages/core/src/runbook/output-channels.ts | 39 +++ 17 files changed, 381 insertions(+), 417 deletions(-) create mode 100644 .changeset/machine-derives-output-scope.md delete mode 100644 packages/cli/__tests__/services/execution-helpers.test.ts diff --git a/.changeset/machine-derives-output-scope.md b/.changeset/machine-derives-output-scope.md new file mode 100644 index 000000000..4d47723d3 --- /dev/null +++ b/.changeset/machine-derives-output-scope.md @@ -0,0 +1,54 @@ +--- +'@rundown-org/core': minor +'@rundown-org/cli': patch +--- + +# Derive the OUTPUTS scope behind the state machine, not in the CLI + +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`. +`deriveOutputScope` and `extractUnitOutputs` are gone from +`packages/cli/src/services/execution.ts`; `outputScope` and `nakedOutputs` are +gone from the event. Core now derives both inside the `commandExecActor` +invoke-input closure. `commandExecActor` itself is untouched — only the source +of its input changed. + +Every input to that derivation was already machine-owned, and OUTPUTS capture is +Category B by name in CLAUDE.md's side-effect table, so nothing external had to +move with it. The two halves enter through different doors, which is the whole +point of the placement: `nakedOutputs` is **compile-time-bound** — which names a +unit captures is fixed by the parsed runbook, so the leaf-state builder resolves +it once and closes over it — while `outputScope` is **event-time-bound**, +because its iteration tier comes from `context.forStack` and changes per FOR +iteration, so it is read from context at fire time. This is the same split +`buildArtifactResolveInput` already applies one function away in `compiler.ts`, +for the sibling ARTIFACTS directive over the same `forStack`; core carried two +parallel derivations of one concept and drove only one of them from the machine. + +The scope is now built from the leaf state's own `stepName`/`substepId` rather +than from a cursor the sender reports, which closes a real gap: a persisted +`substep` naming a substep that no longer exists on the step used to fall back +through `resolveCurrentExecutionUnit` to a step-level scope, while the machine +sat wherever the machine actually sat. A leaf state exists only for a substep +the compiled runbook defines, so the position and the scope can no longer +disagree. + +Core gains `deriveOutputScope(stepId, substepId, forStack)` from +`output-channels.js`, beside the `OutputScope` type it constructs, and +`extractUnitOutputs(step, substepId)` from `execution-units.js`, beside +`resolveCurrentExecutionUnit`. Both drop the CLI versions' separate `isSubstep` +boolean: a defined `substepId` **is** the substep tier, so the two can no longer +be passed in disagreement, and the two test cases that exercised the +contradictory combinations are unrepresentable rather than deleted. + +Two mutants survive on the leaf builder's `owningStep === undefined` guard. They +are equivalent, not a coverage gap: `config.stepName` always names a step in +`steps`, so the arm is unreachable, and the guard mirrors the shape the adjacent +`needsIteration` line already uses on the same variable. The two mutants on that +line that do encode real behaviour are killed by +`compiler-command-exec.test.ts`. + +No new source-text guard accompanies this. The three `readFile`-plus-regex tests +in the CLI exist because those seams cannot state their invariant in types; this +one now can — the event fields are gone, so a re-added CLI derivation has +nowhere to send its result and fails to compile. diff --git a/packages/cli/__tests__/services/execution-helpers.test.ts b/packages/cli/__tests__/services/execution-helpers.test.ts deleted file mode 100644 index 42ea4d59d..000000000 --- a/packages/cli/__tests__/services/execution-helpers.test.ts +++ /dev/null @@ -1,300 +0,0 @@ -// packages/cli/__tests__/services/execution-helpers.test.ts -// -// Unit tests for the scope-tier derivation helpers exported from execution.ts: -// - deriveOutputScope(currentState, isSubstep, substepId?) -// - extractUnitOutputs(currentStep, isSubstep, substepId?) -// -// Both helpers are pure (no I/O), so they are tested directly without mocking -// the full execution loop. - -import { brandInitialTemplateVarsForTest } from '../helpers/brand-helpers.js'; -import { describe, it, expect } from '@jest/globals'; -import { deriveOutputScope, extractUnitOutputs } from '../../src/services/execution.js'; -import type { RunbookState, ForContext } from '@rundown-org/core'; -import type { ResolvedStep, OutputDeclaration, Substep } from '@rundown-org/parser'; - -// --------------------------------------------------------------------------- -// Minimal RunbookState factory -// --------------------------------------------------------------------------- - -function makeState(step: string, forStack: readonly ForContext[] = []): RunbookState { - return { - templateVars: brandInitialTemplateVarsForTest({}), - id: 'test-run' as RunbookState['id'], - runbook: { source: 'project', path: 'test.md' }, - runbookPath: '/test.md', - step, - stepName: `Step ${step}`, - retryCount: 0, - variables: {} as RunbookState['variables'], - steps: [], - forStack, - startedAt: '2024-01-01T00:00:00Z', - updatedAt: '2024-01-01T00:00:00Z', - }; -} - -// --------------------------------------------------------------------------- -// Minimal ResolvedStep factories -// --------------------------------------------------------------------------- - -function makeOutputDecl(name: string, value?: string): OutputDeclaration { - return value !== undefined ? { name, value } : { name }; -} - -function makeCommandStep(name: string, outputs?: readonly OutputDeclaration[]): ResolvedStep { - const step = { - kind: 'command', - name, - description: `Step ${name}`, - command: { code: 'echo hello', lang: 'sh' }, - transitions: { - pass: { kind: 'pass' as const, retry: 0, action: { type: 'CONTINUE' as const } }, - fail: { kind: 'fail' as const, retry: 0, action: { type: 'STOP' as const } }, - }, - ...(outputs !== undefined ? { outputs } : {}), - } satisfies ResolvedStep; - - return step; -} - -function makeSubstepsStep( - name: string, - substeps: Array<{ id: string; outputs?: readonly OutputDeclaration[] }>, - stepOutputs?: readonly OutputDeclaration[], -): ResolvedStep { - const resolvedSubsteps: readonly Substep[] = substeps.map((sub) => ({ - id: sub.id, - description: `Substep ${sub.id}`, - transitions: { - pass: { kind: 'pass' as const, retry: 0, action: { type: 'CONTINUE' as const } }, - fail: { kind: 'fail' as const, retry: 0, action: { type: 'STOP' as const } }, - }, - ...(sub.outputs !== undefined ? { outputs: sub.outputs } : {}), - })); - - const step = { - kind: 'substeps', - name, - description: `Step ${name}`, - substeps: resolvedSubsteps, - transitions: { - pass: { kind: 'pass' as const, retry: 0, action: { type: 'CONTINUE' as const } }, - fail: { kind: 'fail' as const, retry: 0, action: { type: 'STOP' as const } }, - }, - ...(stepOutputs !== undefined ? { outputs: stepOutputs } : {}), - } satisfies ResolvedStep; - - return step; -} - -// --------------------------------------------------------------------------- -// deriveOutputScope tests -// --------------------------------------------------------------------------- - -describe('deriveOutputScope', () => { - describe('step only (isSubstep=false)', () => { - it('returns only stepId when no substep and no FOR stack', () => { - const state = makeState('1'); - const scope = deriveOutputScope(state, false); - expect(scope).toEqual({ stepId: '1' }); - }); - - it('ignores substepId argument when isSubstep=false', () => { - const state = makeState('1'); - const scope = deriveOutputScope(state, false, '2'); - expect(scope).toEqual({ stepId: '1' }); - expect(scope).not.toHaveProperty('substep'); - }); - - it('uses state.step as stepId for named steps', () => { - const state = makeState('ErrorHandler'); - const scope = deriveOutputScope(state, false); - expect(scope).toEqual({ stepId: 'ErrorHandler' }); - }); - }); - - describe('substep tier', () => { - it('includes substep when isSubstep=true and substepId is provided', () => { - const state = makeState('1'); - const scope = deriveOutputScope(state, true, '2'); - expect(scope).toEqual({ stepId: '1', substep: { id: '2' } }); - }); - - it('omits substep when isSubstep=true but substepId is undefined', () => { - const state = makeState('1'); - const scope = deriveOutputScope(state, true, undefined); - expect(scope).toEqual({ stepId: '1' }); - expect(scope).not.toHaveProperty('substep'); - }); - }); - - describe('iteration tier', () => { - it('includes iteration nested inside substep when non-implicit FOR frame matches', () => { - const forStack: readonly ForContext[] = [ - { - stepId: '1', - iteration: 3, - start: 1, - end: 5, - implicit: false, - source: { kind: 'range' }, - }, - ]; - const state = makeState('1', forStack); - const scope = deriveOutputScope(state, true, '2'); - expect(scope).toEqual({ stepId: '1', substep: { id: '2', iteration: 3 } }); - }); - - it('omits iteration when the top FOR frame is implicit', () => { - const forStack: readonly ForContext[] = [ - { - stepId: '1', - iteration: 3, - start: 1, - end: 1, - implicit: true, - source: { kind: 'range' }, - }, - ]; - const state = makeState('1', forStack); - const scope = deriveOutputScope(state, true, '2'); - expect(scope).toEqual({ stepId: '1', substep: { id: '2' } }); - expect(scope.substep).not.toHaveProperty('iteration'); - }); - - it('omits iteration when the top FOR frame is for a different step', () => { - const forStack: readonly ForContext[] = [ - { - stepId: '99', - iteration: 3, - start: 1, - end: 5, - implicit: false, - source: { kind: 'range' }, - }, - ]; - const state = makeState('1', forStack); - const scope = deriveOutputScope(state, true, '2'); - expect(scope).toEqual({ stepId: '1', substep: { id: '2' } }); - expect(scope.substep).not.toHaveProperty('iteration'); - }); - - it('omits substep and iteration when isSubstep=false even with a matching FOR frame', () => { - // FOR loops always execute inside substeps. With isSubstep=false, the - // helper returns only { stepId } — iteration is gated on the isSubstep - // precondition, making { stepId, iteration } unrepresentable at runtime. - const forStack: readonly ForContext[] = [ - { - stepId: '1', - iteration: 2, - start: 1, - end: 3, - implicit: false, - source: { kind: 'range' }, - }, - ]; - const state = makeState('1', forStack); - const scope = deriveOutputScope(state, false); - expect(scope).toEqual({ stepId: '1' }); - expect(scope).not.toHaveProperty('substep'); - }); - - it('uses the last (top) frame when multiple frames are stacked', () => { - const forStack: readonly ForContext[] = [ - { - stepId: '1', - iteration: 1, - start: 1, - end: 3, - implicit: false, - source: { kind: 'range' }, - }, - { - stepId: '1', - iteration: 5, - start: 1, - end: 10, - implicit: false, - source: { kind: 'range' }, - }, - ]; - const state = makeState('1', forStack); - const scope = deriveOutputScope(state, true, '2'); - expect(scope.substep?.iteration).toBe(5); - }); - }); -}); - -// --------------------------------------------------------------------------- -// extractUnitOutputs tests -// --------------------------------------------------------------------------- - -describe('extractUnitOutputs', () => { - describe('step-level outputs (isSubstep=false)', () => { - it('returns step outputs when step has outputs and isSubstep=false', () => { - const outputs: readonly OutputDeclaration[] = [makeOutputDecl('Result')]; - const step = makeCommandStep('1', outputs); - const result = extractUnitOutputs(step, false); - expect(result).toEqual(outputs); - }); - - it('returns empty array when step has no outputs field and isSubstep=false', () => { - const step = makeCommandStep('1'); - const result = extractUnitOutputs(step, false); - expect(result).toEqual([]); - }); - - it('ignores substepId when isSubstep=false, returns step outputs', () => { - const stepOutputs: readonly OutputDeclaration[] = [makeOutputDecl('StepOut')]; - const step = makeSubstepsStep( - '1', - [{ id: '1.1', outputs: [makeOutputDecl('SubOut')] }], - stepOutputs, - ); - // Even if substepId is provided, isSubstep=false routes to step.outputs - const result = extractUnitOutputs(step, false, '1.1'); - expect(result).toEqual(stepOutputs); - }); - }); - - describe('substep-level outputs (isSubstep=true)', () => { - it('returns the matching substep outputs when isSubstep=true', () => { - const substepOutputs: readonly OutputDeclaration[] = [makeOutputDecl('SubResult')]; - const step = makeSubstepsStep('1', [{ id: '1.1', outputs: substepOutputs }]); - const result = extractUnitOutputs(step, true, '1.1'); - expect(result).toEqual(substepOutputs); - }); - - it('does NOT return step-level outputs when routing to a substep', () => { - const stepOutputs: readonly OutputDeclaration[] = [makeOutputDecl('StepOut')]; - const substepOutputs: readonly OutputDeclaration[] = [makeOutputDecl('SubOut')]; - const step = makeSubstepsStep('1', [{ id: '1.1', outputs: substepOutputs }], stepOutputs); - const result = extractUnitOutputs(step, true, '1.1'); - expect(result).toEqual(substepOutputs); - expect(result).not.toEqual(stepOutputs); - }); - - it('returns empty array when substep id does not match any substep', () => { - const step = makeSubstepsStep('1', [{ id: '1.1', outputs: [makeOutputDecl('Sub')] }]); - const result = extractUnitOutputs(step, true, '9.9'); - expect(result).toEqual([]); - }); - - it('returns empty array when the matching substep has no outputs field', () => { - // substep without outputs key - const step = makeSubstepsStep('1', [{ id: '1.1' }]); - const result = extractUnitOutputs(step, true, '1.1'); - expect(result).toEqual([]); - }); - - it('returns step outputs when isSubstep=true but step has no substeps (falls back to step.outputs)', () => { - // A command step (no substeps) — resolvedStepHasSubsteps returns false, - // so the helper falls through to return currentStep.outputs. - const stepOutputs: readonly OutputDeclaration[] = [makeOutputDecl('StepOut')]; - const step = makeCommandStep('1', stepOutputs); - const result = extractUnitOutputs(step, true, '1.1'); - expect(result).toEqual(stepOutputs); - }); - }); -}); diff --git a/packages/cli/__tests__/services/execution-loop.test.ts b/packages/cli/__tests__/services/execution-loop.test.ts index c7076536b..987bf9335 100644 --- a/packages/cli/__tests__/services/execution-loop.test.ts +++ b/packages/cli/__tests__/services/execution-loop.test.ts @@ -2709,10 +2709,10 @@ describe('runExecutionLoop', () => { expect(result).not.toBe('stopped'); const events = mockActorService.sendAndSync.mock.calls.map((call: unknown[]) => call[2]); expect(events).toEqual([ - expect.objectContaining({ - type: 'EXECUTE_COMMAND', - nakedOutputs: [{ name: 'Version' }], - }), + // The event names the command only. Which OUTPUTS this unit captures + // is derived inside the machine from the leaf's own declarations, so + // the CLI has nothing to assert about them here. + expect.objectContaining({ type: 'EXECUTE_COMMAND' }), ]); expect(events).not.toContainEqual(expect.objectContaining({ type: 'SET_VARIABLES' })); expect(events).not.toContainEqual(expect.objectContaining({ type: 'PASS' })); diff --git a/packages/cli/src/services/execution.ts b/packages/cli/src/services/execution.ts index 0783c4895..af776e31b 100644 --- a/packages/cli/src/services/execution.ts +++ b/packages/cli/src/services/execution.ts @@ -47,9 +47,7 @@ import { ErrorCodes, type ErrorCodeKey, getErrorMessage, - partitionOutputDeclarations, resolveCurrentExecutionUnit, - type OutputScope, deriveTransitionObservation, asTerminalSnapshotOrDefault, isRunbookStopped, @@ -60,7 +58,6 @@ import { createEffectfulActorMutationRunner, type EffectfulActorMutationRunner, } from '@rundown-org/core'; -import { resolvedStepHasSubsteps, type OutputDeclaration } from '@rundown-org/parser'; import { isInternalRdCommand, executeRdCommandInternal } from './internal-commands.js'; import { inlineLinkageFromIntent, @@ -120,68 +117,6 @@ export function findStepOrThrow(steps: ResolvedStep[], stepName: string): Resolv return step; } -/** - * Derive the output-channel scope for the unit currently being executed. - * - * Produces one of three tier compositions: - * - `{ stepId }` — step-level (no substep, no iteration) - * - `{ stepId, substep: { id } }` — substep-level, no FOR loop - * - `{ stepId, substep: { id, iteration } }` — substep inside a FOR loop - * - * Tier population: - * - substep tier: set from `substepId` when both `isSubstep` is true and - * `substepId` is defined — the `isSubstep` guard is a belt-and-suspenders - * check; the nested type makes iteration-without-substep unrepresentable - * - iteration tier: set from `top.iteration` when `isSubstep` is true AND - * the top FOR frame is non-implicit and its `stepId` matches - * `currentState.step` - * - * Implicit FOR frames contribute no iteration tier — implicit frames have no - * user-visible counter to segment the path with. - * - * @param currentState - The runbook state at the moment of execution - * @param isSubstep - Whether the current execution unit is a substep - * @param substepId - The substep id when isSubstep is true - * @returns OutputScope suitable for `outputChannelPath` / `prepareOutputChannels` - */ -export function deriveOutputScope( - currentState: RunbookState, - isSubstep: boolean, - substepId?: string, -): OutputScope { - const stepId = currentState.step; - if (!isSubstep || substepId === undefined) { - return { stepId }; - } - const top = currentState.forStack?.at(-1); - if (top && !top.implicit && top.stepId === stepId) { - return { stepId, substep: { id: substepId, iteration: top.iteration } }; - } - return { stepId, substep: { id: substepId } }; -} - -/** - * Extract the OUTPUTS declarations attached to the execution unit currently - * being run. For a substep, return the substep's OUTPUTS; for a step-level - * command, return the parent step's OUTPUTS. - * - * @param currentStep - The resolved parent step - * @param isSubstep - Whether a substep is being executed - * @param substepId - The substep id when isSubstep is true - * @returns Output declarations or empty array - */ -export function extractUnitOutputs( - currentStep: ResolvedStep, - isSubstep: boolean, - substepId?: string, -): readonly OutputDeclaration[] { - if (isSubstep && substepId !== undefined && resolvedStepHasSubsteps(currentStep)) { - const sub = currentStep.substeps.find((s) => s.id === substepId); - return sub?.outputs ?? []; - } - return currentStep.outputs ?? []; -} - type TransitionApplicationResult = | { status: 'continue'; state: RunbookState } | { status: 'done' } @@ -1699,11 +1634,6 @@ export async function runExecutionLoop( return 'waiting'; } - const substepId = isSubstep ? itemToRender.id : undefined; - const unitOutputs = extractUnitOutputs(currentStep, isSubstep, substepId); - const { naked: nakedOutputs } = partitionOutputDeclarations(unitOutputs); - const outputScope = deriveOutputScope(currentState, isSubstep, substepId); - // Build rundown-injected environment variables (RD_WORK_PATH, RD_RUN_ID, etc.) // Keys come from BUILTIN_VARIABLES so a rename in variable-discovery.ts // surfaces here as a typecheck error instead of silently breaking injection. @@ -1750,8 +1680,6 @@ export async function runExecutionLoop( command: expandedCommandCode, displayCommand, runbookPath: capturedState.runbookPath, - outputScope, - nakedOutputs, rdInjected, }, { issueDelegationCredential: options.delegationRuntime?.issueDelegationCredential }, diff --git a/packages/core/__tests__/runbook/actor-service-compute-commit-equivalence.test.ts b/packages/core/__tests__/runbook/actor-service-compute-commit-equivalence.test.ts index 33bac9b58..9a5e805c6 100644 --- a/packages/core/__tests__/runbook/actor-service-compute-commit-equivalence.test.ts +++ b/packages/core/__tests__/runbook/actor-service-compute-commit-equivalence.test.ts @@ -277,8 +277,6 @@ describe('prepareActorMutation / sendAndSync equivalence', () => { command: 'true', displayCommand: 'true', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: runId }, }; const { computed, computedEffects, persisted, persistedEffects } = await bothHalves( @@ -320,8 +318,6 @@ describe('prepareActorMutation / sendAndSync equivalence', () => { command: 'curl https://example.test', displayCommand: 'curl https://example.test', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: runId }, }; const { computed, computedEffects, persisted, persistedEffects } = await bothHalves( diff --git a/packages/core/__tests__/runbook/actor-service-pending-effects.test.ts b/packages/core/__tests__/runbook/actor-service-pending-effects.test.ts index a5b12f875..aa3c8e01f 100644 --- a/packages/core/__tests__/runbook/actor-service-pending-effects.test.ts +++ b/packages/core/__tests__/runbook/actor-service-pending-effects.test.ts @@ -215,8 +215,6 @@ describe('RunbookActorService pending machine effects', () => { type: 'EXECUTE_COMMAND', command: 'true', displayCommand: 'true', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: state.id }, }); await waitUntil(() => effectStarted === 1); diff --git a/packages/core/__tests__/runbook/actor-service.test.ts b/packages/core/__tests__/runbook/actor-service.test.ts index f6ef47398..cdbf5f031 100644 --- a/packages/core/__tests__/runbook/actor-service.test.ts +++ b/packages/core/__tests__/runbook/actor-service.test.ts @@ -774,8 +774,6 @@ echo ok command: 'true', displayCommand: 'true', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: runId }, }); @@ -827,8 +825,6 @@ echo ok command: 'sleep-longer-than-machine-effect-timeout', displayCommand: 'sleep-longer-than-machine-effect-timeout', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: runId }, }); @@ -863,8 +859,6 @@ echo ok command: 'false', displayCommand: 'false', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: runId }, }); @@ -902,8 +896,6 @@ echo ok command: 'curl https://example.test', displayCommand: 'curl https://example.test', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: runId }, }); @@ -942,8 +934,6 @@ echo ok command: 'npm test', displayCommand: 'npm test', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: runId }, }); @@ -979,8 +969,6 @@ echo ok command: 'true', displayCommand: 'true', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: runId }, }); diff --git a/packages/core/__tests__/runbook/compiler-command-exec.test.ts b/packages/core/__tests__/runbook/compiler-command-exec.test.ts index 97343dd64..631a061df 100644 --- a/packages/core/__tests__/runbook/compiler-command-exec.test.ts +++ b/packages/core/__tests__/runbook/compiler-command-exec.test.ts @@ -1,4 +1,7 @@ import { describe, expect, it } from '@jest/globals'; +import { promises as fs } from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; import { createActor } from 'xstate'; import { compileRunbookToMachine, type CommandExecutionServices } from '../../src/runbook/index.js'; import type { ResolvedStep } from '../../src/runbook/types.js'; @@ -50,8 +53,6 @@ describe('compiled machine command execution', () => { command: 'npm test', displayCommand: 'npm test', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: 'rd_11111111111111111111111111111111' }, }); @@ -84,8 +85,6 @@ describe('compiled machine command execution', () => { command: 'npm test', displayCommand: 'npm test', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: 'rd_66666666666666666666666666666666' }, }); @@ -123,8 +122,6 @@ describe('compiled machine command execution', () => { command: 'curl https://example.test', displayCommand: 'curl https://example.test', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: 'rd_22222222222222222222222222222222' }, }); @@ -170,8 +167,6 @@ describe('compiled machine command execution', () => { command: 'npm test', displayCommand: 'npm test', runbookPath: 'workflow.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: 'rd_77777777777777777777777777777777' }, }); @@ -195,3 +190,79 @@ describe('compiled machine command execution', () => { }); }); }); + +describe('compiled machine OUTPUTS derivation', () => { + // The scope and the naked declarations used to be derived by the CLI and + // shipped on EXECUTE_COMMAND. They are now derived behind the machine: the + // declarations are compile-time-bound from the leaf's own definition, and + // the scope is built from the leaf's identity. `RD_OUTPUTS_*` is where both + // become observable, since the channel path encodes every scope tier. + const substepOutputsStep = { + kind: 'substeps', + name: '1', + description: 'Release', + outputs: [{ name: 'ParentOnly' }], + substeps: [ + { + id: '1', + description: 'Capture version', + command: { code: 'printf v1', lang: 'bash' }, + outputs: [{ name: 'Version' }], + transitions: { + pass: { kind: 'pass', retry: 0, action: { type: 'COMPLETE' } }, + fail: { kind: 'fail', retry: 0, action: { type: 'STOP' } }, + }, + }, + ], + transitions: { + pass: { kind: 'pass', retry: 0, action: { type: 'COMPLETE' } }, + fail: { kind: 'fail', retry: 0, action: { type: 'STOP' } }, + }, + } as unknown as ResolvedStep; + + it('scopes channels to the executing substep and captures only its declarations', async () => { + const cwd = await fs.mkdtemp(path.join(os.tmpdir(), 'rundown-outputs-scope-')); + let injected: Record = {}; + const runId = 'rd_22222222222222222222222222222222'; + + try { + const machine = compileRunbookToMachine([substepOutputsStep], { + commandServices: { + runExternalCommand: async (runnerInput) => { + injected = runnerInput.rdInjected; + return { success: true, exitCode: 0 }; + }, + }, + evaluationOptions: { cwd }, + templateVars: { + RunId: runId, + WorkPath: '.rundown/work', + ContextId: 'ctx', + RunbookRef: { source: 'project', path: 'workflow.runbook.md' }, + } as never, + }); + const actor = createActor(machine).start(); + + actor.send({ + type: 'EXECUTE_COMMAND', + command: 'printf v1', + displayCommand: 'printf v1', + runbookPath: 'workflow.runbook.md', + rdInjected: { RD_RUN_ID: runId }, + }); + + await waitForDone(actor); + + // Substep tier present, from the leaf's own identity — not from a + // cursor the sender reported. + expect(injected.RD_OUTPUTS_Version).toBe( + path.join(cwd, '.rundown', 'runs', runId, 'outputs', '1', '1', 'Version'), + ); + // The parent step's OUTPUTS belong to a different channel path, so a + // substep unit must not capture them. + expect(injected).not.toHaveProperty('RD_OUTPUTS_ParentOnly'); + } finally { + await fs.rm(cwd, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/core/__tests__/runbook/compiler.test.ts b/packages/core/__tests__/runbook/compiler.test.ts index 9690f8b58..ba26ec971 100644 --- a/packages/core/__tests__/runbook/compiler.test.ts +++ b/packages/core/__tests__/runbook/compiler.test.ts @@ -9958,8 +9958,6 @@ echo hi type: 'EXECUTE_COMMAND', command: 'echo first', displayCommand: 'echo first', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: {}, }); await waitFor(actor, (snapshot) => snapshot.hasTag(PENDING_COMMAND_EXECUTION_TAG), { @@ -9970,8 +9968,6 @@ echo hi type: 'EXECUTE_COMMAND', command: 'echo second', displayCommand: 'echo second', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: {}, }); expect(executeCalls).toBe(1); diff --git a/packages/core/__tests__/runbook/execution-recovery-service.test.ts b/packages/core/__tests__/runbook/execution-recovery-service.test.ts index f0223ab0e..276513a75 100644 --- a/packages/core/__tests__/runbook/execution-recovery-service.test.ts +++ b/packages/core/__tests__/runbook/execution-recovery-service.test.ts @@ -201,8 +201,6 @@ describe('ExecutionRecoveryService', () => { command: 'npm test', displayCommand: 'npm test', runbookPath: 'command.runbook.md', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: persistedTemplateVars.RunId }, }); await commandStarted; diff --git a/packages/core/__tests__/runbook/execution-units.test.ts b/packages/core/__tests__/runbook/execution-units.test.ts index e12e4789f..30fd82675 100644 --- a/packages/core/__tests__/runbook/execution-units.test.ts +++ b/packages/core/__tests__/runbook/execution-units.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from '@jest/globals'; -import { resolveCurrentExecutionUnit } from '../../src/runbook/execution-units.js'; +import { + extractUnitOutputs, + resolveCurrentExecutionUnit, +} from '../../src/runbook/execution-units.js'; +import type { OutputDeclaration } from '@rundown-org/parser'; import type { ResolvedStep, Substep } from '../../src/runbook/types.js'; import { makeBaseStep } from '../helpers/step-factories.js'; @@ -52,3 +56,82 @@ describe('resolveCurrentExecutionUnit', () => { expect(resolveCurrentExecutionUnit(step, 'missing')).toBe(step); }); }); + +function makeOutputsSubstep(id: string, outputs?: readonly OutputDeclaration[]): Substep { + return { ...makeSubstep(id), ...(outputs !== undefined ? { outputs } : {}) }; +} + +function makeSubstepsStep( + substeps: readonly Substep[], + stepOutputs?: readonly OutputDeclaration[], +): ResolvedStep { + return { + kind: 'substeps', + name: '1', + description: 'Parent', + substeps, + transitions: { + pass: { kind: 'pass', retry: 0, action: { type: 'CONTINUE' } }, + fail: { kind: 'fail', retry: 0, action: { type: 'STOP' } }, + }, + ...(stepOutputs !== undefined ? { outputs: stepOutputs } : {}), + }; +} + +describe('extractUnitOutputs', () => { + it('returns the step OUTPUTS for a step-level unit', () => { + const outputs: readonly OutputDeclaration[] = [{ name: 'Result' }]; + expect( + extractUnitOutputs(makeBaseStep({ name: '1', description: 'S', outputs }), undefined), + ).toEqual(outputs); + }); + + it('returns no declarations when a step-level unit declares none', () => { + expect(extractUnitOutputs(makeBaseStep({ name: '1', description: 'S' }), undefined)).toEqual( + [], + ); + }); + + it('returns the substep OUTPUTS, not the parent step OUTPUTS', () => { + const stepOutputs: readonly OutputDeclaration[] = [{ name: 'StepOut' }]; + const substepOutputs: readonly OutputDeclaration[] = [{ name: 'SubOut' }]; + const step = makeSubstepsStep([makeOutputsSubstep('1.1', substepOutputs)], stepOutputs); + + expect(extractUnitOutputs(step, '1.1')).toEqual(substepOutputs); + }); + + it('returns the parent step OUTPUTS for a step-level unit on a step that has substeps', () => { + // A step with substeps still owns OUTPUTS of its own, and `substepId` + // alone decides which tier is being captured — the presence of substeps + // on the step does not route a step-level unit into the substep branch. + const stepOutputs: readonly OutputDeclaration[] = [{ name: 'ParentOnly' }]; + const step = makeSubstepsStep([makeOutputsSubstep('1.1', [{ name: 'SubOut' }])], stepOutputs); + + expect(extractUnitOutputs(step, undefined)).toEqual(stepOutputs); + }); + + it('returns no declarations when the named substep declares none', () => { + const step = makeSubstepsStep([makeOutputsSubstep('1.1')]); + + expect(extractUnitOutputs(step, '1.1')).toEqual([]); + }); + + it('returns no declarations when the substep id names no substep', () => { + // Not a fallback to the parent's OUTPUTS: those belong to a different + // channel path, so capturing them under a substep scope would misfile them. + const step = makeSubstepsStep( + [makeOutputsSubstep('1.1', [{ name: 'Sub' }])], + [{ name: 'StepOut' }], + ); + + expect(extractUnitOutputs(step, '9.9')).toEqual([]); + }); + + it('returns the step OUTPUTS when a substep id is named on a step that has none', () => { + const outputs: readonly OutputDeclaration[] = [{ name: 'StepOut' }]; + + expect( + extractUnitOutputs(makeBaseStep({ name: '1', description: 'S', outputs }), '1.1'), + ).toEqual(outputs); + }); +}); diff --git a/packages/core/__tests__/runbook/output-channels.test.ts b/packages/core/__tests__/runbook/output-channels.test.ts index 45c88c19c..5ce98b325 100644 --- a/packages/core/__tests__/runbook/output-channels.test.ts +++ b/packages/core/__tests__/runbook/output-channels.test.ts @@ -3,6 +3,7 @@ import * as path from 'node:path'; import { promises as fs } from 'node:fs'; import * as os from 'node:os'; import { + deriveOutputScope, partitionOutputDeclarations, outputsDirForRun, outputChannelPath, @@ -11,6 +12,60 @@ import { readCapturedOutputs, type OutputScope, } from '../../src/runbook/output-channels.js'; +import type { ForContext } from '../../src/runbook/types.js'; + +function rangeFrame(stepId: string, iteration: number, implicit: boolean): ForContext { + return { stepId, iteration, start: 1, end: 10, implicit, source: { kind: 'range' } }; +} + +describe('deriveOutputScope', () => { + it('returns only the step tier for a step-level unit', () => { + expect(deriveOutputScope('1', undefined, [])).toEqual({ stepId: '1' }); + expect(deriveOutputScope('ErrorHandler', undefined, [])).toEqual({ + stepId: 'ErrorHandler', + }); + }); + + it('omits the substep tier for a step-level unit even under a matching FOR frame', () => { + // FOR loops always execute their commands inside substeps, so a step-level + // unit contributes no iteration: `{ stepId, iteration }` stays + // unrepresentable because iteration nests inside the substep tier. + const scope = deriveOutputScope('1', undefined, [rangeFrame('1', 2, false)]); + expect(scope).toEqual({ stepId: '1' }); + expect(scope).not.toHaveProperty('substep'); + }); + + it('adds the substep tier when a substep id names the unit', () => { + expect(deriveOutputScope('1', '2', [])).toEqual({ stepId: '1', substep: { id: '2' } }); + }); + + it('nests the iteration inside the substep tier for a matching non-implicit frame', () => { + expect(deriveOutputScope('1', '2', [rangeFrame('1', 3, false)])).toEqual({ + stepId: '1', + substep: { id: '2', iteration: 3 }, + }); + }); + + it('omits the iteration tier when the top FOR frame is implicit', () => { + const scope = deriveOutputScope('1', '2', [rangeFrame('1', 3, true)]); + expect(scope).toEqual({ stepId: '1', substep: { id: '2' } }); + expect(scope.substep).not.toHaveProperty('iteration'); + }); + + it('omits the iteration tier when the top FOR frame belongs to another step', () => { + const scope = deriveOutputScope('1', '2', [rangeFrame('99', 3, false)]); + expect(scope).toEqual({ stepId: '1', substep: { id: '2' } }); + expect(scope.substep).not.toHaveProperty('iteration'); + }); + + it('reads the innermost frame when frames are stacked', () => { + const scope = deriveOutputScope('1', '2', [ + rangeFrame('1', 1, false), + rangeFrame('1', 5, false), + ]); + expect(scope.substep?.iteration).toBe(5); + }); +}); describe('partitionOutputDeclarations', () => { it('separates naked and expression entries preserving order', () => { diff --git a/packages/core/__tests__/runbook/persisted-context-hygiene.properties.test.ts b/packages/core/__tests__/runbook/persisted-context-hygiene.properties.test.ts index 55c141b9e..e2e9fc233 100644 --- a/packages/core/__tests__/runbook/persisted-context-hygiene.properties.test.ts +++ b/packages/core/__tests__/runbook/persisted-context-hygiene.properties.test.ts @@ -188,8 +188,6 @@ describe('persisted context hygiene properties', () => { type: 'EXECUTE_COMMAND', command: 'true', displayCommand: 'true', - outputScope: { stepId: '1' }, - nakedOutputs: [], rdInjected: { RD_RUN_ID: 'rd_eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' }, }); await waitFor(actor, (snap) => !snap.hasTag(PENDING_MACHINE_EFFECT_TAG)); diff --git a/packages/core/src/runbook/compiler.ts b/packages/core/src/runbook/compiler.ts index db451b5d0..c6110c3f3 100644 --- a/packages/core/src/runbook/compiler.ts +++ b/packages/core/src/runbook/compiler.ts @@ -29,7 +29,9 @@ import type { StepId } from './step-id.js'; import type { DelegationTokenHash } from './delegation-token.js'; import type { ArtifactDeclaration, ForClause, OutputDeclaration } from '@rundown-org/parser'; import { MAX_FOR_BOUND } from '@rundown-org/parser'; -import type { NakedOutput, OutputScope, PreparedChannel } from './output-channels.js'; +import { deriveOutputScope, partitionOutputDeclarations } from './output-channels.js'; +import type { NakedOutput, PreparedChannel } from './output-channels.js'; +import { extractUnitOutputs } from './execution-units.js'; import { artifactResolveActor, type ArtifactResolveInput, @@ -825,11 +827,37 @@ function requireCommandCwd(evaluationOptions: EvaluateOutputOptions | undefined) return evaluationOptions.cwd; } +/** + * Assemble the command actor's input for one execution unit. + * + * The OUTPUTS half of this input is derived here rather than carried on the + * event, and its two halves enter through different doors for the reasons in + * CLAUDE.md § Actor dependencies. `nakedOutputs` is compile-time-bound — the + * unit's OUTPUTS declarations are fixed by the parsed runbook, so the leaf + * builder resolves them once and closes over them. `outputScope` is + * event-time-bound: its iteration tier comes from `context.forStack`, which + * changes per FOR iteration, so it is read from context at fire time. + * + * `stepName`/`substepId` name this leaf, so the scope is derived against the + * machine's own position rather than against a cursor a caller reports. + * + * @param event - The dispatched EXECUTE_COMMAND event + * @param context - Machine context at fire time + * @param evaluationOptions - Compile-time evaluation options supplying `cwd` + * @param commandServices - DI'd command execution callables + * @param stepName - Step owning this leaf state + * @param substepId - Substep owning this leaf state, when it is a substep + * @param nakedOutputs - Compile-time-resolved naked OUTPUTS for the unit + * @returns Fully assembled command actor input + */ function buildCommandExecutionInput( event: Extract, context: RunbookContext, evaluationOptions: EvaluateOutputOptions | undefined, commandServices: CommandExecutionServices | undefined, + stepName: string, + substepId: string | undefined, + nakedOutputs: readonly NakedOutput[], ): CommandExecutionInput { return { services: requireCommandServices(commandServices), @@ -839,8 +867,8 @@ function buildCommandExecutionInput( runId: assertRunId(requireStringTemplateVar(context.templateVars, 'RunId')), runbookPath: event.runbookPath, runbook: requireRunbookRef(context.templateVars), - outputScope: event.outputScope, - nakedOutputs: event.nakedOutputs, + outputScope: deriveOutputScope(stepName, substepId, context.forStack), + nakedOutputs, rdInjected: event.rdInjected, }; } @@ -1195,8 +1223,6 @@ export type RunbookEvent = command: string; displayCommand: string; runbookPath?: string; - outputScope: OutputScope; - nakedOutputs: readonly NakedOutput[]; rdInjected: Record; } | { @@ -4742,6 +4768,12 @@ export function compileRunbookToMachine( ]; const owningStep = steps.find((step) => step.name === config.stepName); const needsIteration = owningStep !== undefined && leafNeedsIterationResolution(owningStep); + // Compile-time-bound: which names this unit captures is fixed by the parsed + // runbook, so it is resolved once here and closed over by the invoke input + // rather than re-derived (or supplied by a caller) on every execution. + const { naked: unitNakedOutputs } = partitionOutputDeclarations( + owningStep === undefined ? [] : extractUnitOutputs(owningStep, config.substepId), + ); const afterArtifactsTarget = shouldIssueDelegations ? '__issue-delegations' : shouldPrepareInlineLaunch @@ -5027,6 +5059,9 @@ export function compileRunbookToMachine( context, evaluationOptions, options?.commandServices, + config.stepName, + config.substepId, + unitNakedOutputs, ); }, onDone: [ diff --git a/packages/core/src/runbook/execution-units.ts b/packages/core/src/runbook/execution-units.ts index 58e5f256d..8093f32e9 100644 --- a/packages/core/src/runbook/execution-units.ts +++ b/packages/core/src/runbook/execution-units.ts @@ -1,4 +1,5 @@ import { resolvedStepHasSubsteps } from '@rundown-org/parser'; +import type { OutputDeclaration } from '@rundown-org/parser'; import type { ResolvedStep, Substep } from './types.js'; /** @@ -20,3 +21,26 @@ export function resolveCurrentExecutionUnit( } return currentStep.substeps.find((substep) => substep.id === substepId) ?? currentStep; } + +/** + * Extract the OUTPUTS declarations attached to one execution unit. + * + * For a substep, return the substep's OUTPUTS; for a step-level command, + * return the parent step's OUTPUTS. A `substepId` that names no substep on + * `currentStep` yields no declarations rather than silently falling back to + * the parent's — the parent's OUTPUTS belong to a different channel path, so + * capturing them under a substep scope would misfile them. + * + * @param currentStep - The resolved parent step + * @param substepId - Substep identifier, or undefined for a step-level unit + * @returns Output declarations for the unit, or an empty list + */ +export function extractUnitOutputs( + currentStep: ResolvedStep, + substepId: string | undefined, +): readonly OutputDeclaration[] { + if (substepId !== undefined && resolvedStepHasSubsteps(currentStep)) { + return currentStep.substeps.find((s) => s.id === substepId)?.outputs ?? []; + } + return currentStep.outputs ?? []; +} diff --git a/packages/core/src/runbook/index.ts b/packages/core/src/runbook/index.ts index 889685c0e..919968c72 100644 --- a/packages/core/src/runbook/index.ts +++ b/packages/core/src/runbook/index.ts @@ -75,7 +75,7 @@ export { type ExecutionEpoch, type GuardedMutationResult, } from './storage/mutation-result.js'; -export { resolveCurrentExecutionUnit } from './execution-units.js'; +export { extractUnitOutputs, resolveCurrentExecutionUnit } from './execution-units.js'; export { buildContextVars, buildStepVariables, @@ -689,6 +689,7 @@ export { type VariableValue, } from './effective-vars.js'; export { + deriveOutputScope, partitionOutputDeclarations, outputsDirForRun, outputChannelPath, diff --git a/packages/core/src/runbook/output-channels.ts b/packages/core/src/runbook/output-channels.ts index 9de058e27..5da7fb63b 100644 --- a/packages/core/src/runbook/output-channels.ts +++ b/packages/core/src/runbook/output-channels.ts @@ -7,6 +7,7 @@ import { RUNDOWN_DIR, assertSafeId } from '../paths.js'; import { logger } from '../logger.js'; import { isNodeError } from '../errors.js'; import type { VariableValue } from './effective-vars.js'; +import type { ForContext } from './types.js'; import { parseRuntimeVariableValue } from './runtime-variable-value.js'; import { openVerifiedRegularFile, UnsafeFileError } from './safe-fs.js'; @@ -59,6 +60,44 @@ export interface OutputScope { }; } +/** + * Derive the output-channel scope for one execution unit. + * + * Produces one of the three tier compositions {@link OutputScope} allows: + * - `{ stepId }` — step-level (no substep, no iteration) + * - `{ stepId, substep: { id } }` — substep-level, outside a FOR loop + * - `{ stepId, substep: { id, iteration } }` — substep inside a FOR loop + * + * The iteration tier is set from the top FOR frame only when that frame is + * non-implicit and names this step. Implicit frames contribute no iteration + * tier — they have no user-visible counter to segment the path with. + * + * `substepId` is the sole substep discriminant: a defined id IS the substep + * tier, so the impossible iteration-without-substep shape stays + * unrepresentable without a second boolean to keep in agreement. + * + * @param stepId - Owning step identifier for the unit being executed + * @param substepId - Substep identifier, or undefined for a step-level unit + * @param forStack - FOR frames active at the moment of execution, innermost last. + * Always present: `RunbookContext.forStack` is non-optional, so an absent + * stack is an empty one. + * @returns OutputScope suitable for `outputChannelPath` / `prepareOutputChannels` + */ +export function deriveOutputScope( + stepId: string, + substepId: string | undefined, + forStack: readonly ForContext[], +): OutputScope { + if (substepId === undefined) { + return { stepId }; + } + const top = forStack.at(-1); + if (top && !top.implicit && top.stepId === stepId) { + return { stepId, substep: { id: substepId, iteration: top.iteration } }; + } + return { stepId, substep: { id: substepId } }; +} + /** * Arguments for {@link prepareOutputChannels}. */ From 30e6b8f2c724634fe6991c7df4bb2e3bc30b8464 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Wed, 19 Aug 2026 08:23:25 +1000 Subject: [PATCH 08/11] docs: record that a killed mutation run poisons the incremental report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CLAUDE.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 50856821c..1da7e8834 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -739,6 +739,24 @@ All package scripts live in `package.json` — run `pnpm run` to list them changed file; that workflow is advisory (`continue-on-error` throughout, no required check), so it reports but never blocks. + **A killed run leaves that report poisoned, and the next run hangs rather than + failing.** If you `pkill` a run mid-flight (or it dies with workers live), the + partially-written `stryker-incremental.json` makes every subsequent scoped run + stall — measured on `output-channels.ts`: a scope that had just completed in + seconds stopped dead at 4/20 mutants with the ETA climbing past 17m, + reproducibly, across two different scopes and at both concurrency 1 and 2. + `--force` does not rescue this; it forces re-execution but the report is still + read first. The fix is to delete the report and re-run: + + ```bash + rm -f packages//reports/stryker-incremental.json + ``` + + It is gitignored and regenerated by the next run, so deleting it costs only + the incremental reuse of a baseline the kill had already corrupted. Reach for + this whenever a scoped run that should take seconds is still going after a + minute — the symptom is a stall, not an error, so nothing tells you. + - **The changed-code gates are the whole day-to-day signal.** `pnpm run test:mutate:changed` locally and the advisory per-PR check (`.github/workflows/mutation-pr.yml`) are what you act on. The full-fidelity From 2a6073d5fc6ad3a80d4efd2d07ad60a227a36a09 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Wed, 19 Aug 2026 09:36:25 +1000 Subject: [PATCH 09/11] fix(core): stop the stack-push undo from revoking the run's claim (#788) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .changeset/pop-runbook-keeps-claims.md | 52 ++++++++ .../__tests__/runbook/session-service.test.ts | 117 +++++++++++++++++- packages/core/src/runbook/session-service.ts | 74 +++++++++-- 3 files changed, 235 insertions(+), 8 deletions(-) create mode 100644 .changeset/pop-runbook-keeps-claims.md diff --git a/.changeset/pop-runbook-keeps-claims.md b/.changeset/pop-runbook-keeps-claims.md new file mode 100644 index 000000000..61c369502 --- /dev/null +++ b/.changeset/pop-runbook-keeps-claims.md @@ -0,0 +1,52 @@ +--- +'@rundown-org/core': patch +--- + +# Undoing an inline-child activation no longer revokes the child's claim + +`SessionService.popRunbookIfActive` undid a stack push by calling the general +release primitive, `projectRunbookRelease`, with no options — so +`retainClaimsAsTerminal` was falsy and every claim controlling the run was +revoked. But the operation being undone is `defaultStack.push(id)`, which mints +nothing and never reads `session.claims`. The undo disposed of authority the +push never created (#788). + +That was irrecoverable, not merely wrong. The pop's one caller is the inline +launch rollback in the CLI, reached only when a process reclaims an interrupted +launch from a dead owner and the intent consume then throws. The child at that +moment is live, non-terminal, and survives the rollback — the next attempt +resumes it. But `adoptRunControlClaim` refuses to re-mint once that child has +issued a delegation, because the replacement could not reproduce the +credentials. So the child ran unarmed, the machine's `actor_context_required` +refusal stood permanently, and nothing addressed the run again. The holder was +not even told the truth about it: a revoked claim is tombstoned `superseded` and +resolves as `claim-rotated`, a rotation that never happened. + +The fix is a new `projectStackPop` beside `projectRunbookRelease`, and its +narrowness is the guarantee rather than a style choice. It takes the stack array +alone, not `SessionData`, so it cannot revoke a claim or clear a stash slot — +today, or after a later edit that forgets why it must not. A release policy +carried as an option can be omitted, and omission reads as the destructive +direction; that is the same failure mode the `ReleaseRole` vocabulary removes +from the sixteen terminal-release call sites, applied here by deleting the +parameter instead of defaulting it. + +It removes the **topmost** occurrence only. `session_stack` has no uniqueness +constraint and cannot gain one — an existing session carrying a duplicate would +become impossible to load, with `prune` as the only recovery, which the +no-migration rule forbids — so a run can legitimately sit lower in the stack, +and undoing one push must leave that entry alone. `projectRunbookRelease` +filters every occurrence, which is right for a release and wrong for an undo. + +The method stays `mutateGuarded`. A stack-only projection issues no guarded +statement, so `execution_in_progress` and `recovery_required` are now +unreachable through this path, and the argument for deleting them is strong: the +preflight refuses on `exec_token IS NOT NULL` with no liveness probe, and this +method's only caller is the crash-recovery path where the child provably holds a +lease naming a dead pid — so the guard can only refuse where the undo must run, +leaving the child pushed after a failed launch. It is held back deliberately. +Both refusals come from one loop in `mutateSessionGuarded`, so the seam cannot +keep one and drop the other, and `recovery_required` is the arm the symmetry +argument covers least well. It wants a stale-lease test and a multi-process +test, not more argument. This projection is what makes that removal a no-op +rather than a lossy edit. diff --git a/packages/core/__tests__/runbook/session-service.test.ts b/packages/core/__tests__/runbook/session-service.test.ts index f949ce29f..fb0cc1833 100644 --- a/packages/core/__tests__/runbook/session-service.test.ts +++ b/packages/core/__tests__/runbook/session-service.test.ts @@ -5,7 +5,11 @@ import { join } from 'node:path'; import { DatabaseSync } from 'node:sqlite'; import { RunbookStateManager, type SessionData } from '../../src/runbook/state.js'; import { RunbookActorService } from '../../src/runbook/actor-service.js'; -import { SessionService, projectRunbookRelease } from '../../src/runbook/session-service.js'; +import { + SessionService, + projectRunbookRelease, + projectStackPop, +} from '../../src/runbook/session-service.js'; import { makeClaimRecord } from '../../src/testing/claim-fixtures.js'; import { assertClaimId, @@ -289,6 +293,64 @@ describe('SessionService', () => { expect(session.claims[minted.claim.claimKey]).toBeDefined(); }); + // #788. The undo of a push must dispose of nothing the push created, and + // the push creates one stack entry. Revoking the claim here was + // irrecoverable: 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 nothing addressed the run again. + it("popRunbookIfActive leaves the popped run's run-control claim intact", async () => { + const parent = await manager.create({ source: 'project', path: 'parent.md' }, mockRunbook, { + runbookPath: 'parent.md', + }); + const child = await manager.create({ source: 'project', path: 'child.md' }, mockRunbook, { + runbookPath: 'child.md', + }); + await sessionService.pushRunbook(parent.id); + // Pushed and claimed the way an inline launch does it, so the claim under + // test is a real run-control bearer an orchestrator could still hold. + const minted = unwrapSessionMutation( + await sessionService.pushRunbookWithRunControlClaim(child.id), + ); + + const result = unwrapSessionMutation(await sessionService.popRunbookIfActive(child.id)); + + expect(result).toEqual({ + status: 'popped', + runbookId: child.id, + nextDefaultRunbookId: parent.id, + }); + const session = await manager.loadSession(); + expect(session.defaultStack).toEqual([parent.id]); + // The whole point: the run left the stack, the authority over it did not. + expect(session.claims[minted.claim.claimKey]).toBeDefined(); + expect(session.claims[minted.claim.claimKey].controlledRunId).toBe(child.id); + }); + + // `session_stack` has no uniqueness constraint, and cannot gain one without + // making an existing session impossible to load, so a run can sit lower + // in the stack as well. Undoing one push must leave that entry alone — + // `releaseRunbook` filters every occurrence, which is why it is not the fix. + it('popRunbookIfActive removes only the topmost entry for a repeated run', async () => { + const outer = await manager.create({ source: 'project', path: 'outer.md' }, mockRunbook, { + runbookPath: 'outer.md', + }); + const mid = await manager.create({ source: 'project', path: 'mid.md' }, mockRunbook, { + runbookPath: 'mid.md', + }); + await sessionService.pushRunbook(outer.id); + await sessionService.pushRunbook(mid.id); + await sessionService.pushRunbook(outer.id); + + const result = unwrapSessionMutation(await sessionService.popRunbookIfActive(outer.id)); + + expect(result).toEqual({ + status: 'popped', + runbookId: outer.id, + nextDefaultRunbookId: mid.id, + }); + expect((await manager.loadSession()).defaultStack).toEqual([outer.id, mid.id]); + }); + it('popRunbookIfActive reports an empty stack as not-active', async () => { const state = await manager.create({ source: 'project', path: 'gone.md' }, mockRunbook, { runbookPath: 'gone.md', @@ -4577,6 +4639,59 @@ describe('SessionService', () => { }); }); +describe('projectStackPop', () => { + // The undo of a bare `defaultStack.push`. Exercised directly because its + // narrow input is the point: it takes the stack array, so a session field it + // must not touch is not reachable from inside it. + const RUN_ID = brandRunIdForTest(`rd_${'a'.repeat(32)}`); + const OTHER_RUN_ID = brandRunIdForTest(`rd_${'b'.repeat(32)}`); + + it('removes the topmost entry for the run', () => { + const stack = [OTHER_RUN_ID, RUN_ID]; + + projectStackPop(stack, RUN_ID); + + expect(stack).toEqual([OTHER_RUN_ID]); + }); + + it('leaves a lower entry for the same run in place', () => { + const stack = [RUN_ID, OTHER_RUN_ID, RUN_ID]; + + projectStackPop(stack, RUN_ID); + + expect(stack).toEqual([RUN_ID, OTHER_RUN_ID]); + }); + + // Without the `index === -1` guard, `splice(-1, 1)` removes the LAST entry — + // so an absent run would cost whoever holds the top their stack entry. + it('removes nothing when the run is not on the stack', () => { + const stack = [OTHER_RUN_ID]; + + projectStackPop(stack, RUN_ID); + + expect(stack).toEqual([OTHER_RUN_ID]); + }); + + it('removes nothing from an empty stack', () => { + const stack: RunId[] = []; + + projectStackPop(stack, RUN_ID); + + expect(stack).toEqual([]); + }); + + it('mutates the array in place rather than replacing it', () => { + const stack = [OTHER_RUN_ID, RUN_ID]; + const same = stack; + + projectStackPop(stack, RUN_ID); + + // The caller passes `session.defaultStack` and reads the new top back off + // the same reference, so a reassignment would leave the session unchanged. + expect(same).toEqual([OTHER_RUN_ID]); + }); +}); + describe('projectRunbookRelease', () => { // The in-memory half of a terminal release. The fence applies this projection // to a session snapshot INSIDE the same transaction as the state write, so it diff --git a/packages/core/src/runbook/session-service.ts b/packages/core/src/runbook/session-service.ts index 04a8849ad..7932a50ee 100644 --- a/packages/core/src/runbook/session-service.ts +++ b/packages/core/src/runbook/session-service.ts @@ -176,6 +176,44 @@ export function projectRunbookRelease( }; } +/** + * Remove the topmost stack entry for a run, and nothing else. + * + * The undo of a bare `defaultStack.push`, and deliberately not a variant of + * {@link projectRunbookRelease}. That primitive revokes every claim + * controlling the run and clears a matching stash slot, which is correct when + * a run is genuinely released, and wrong as the undo of a push: the push mints + * no claim and never reads `session.claims`, so an undo that disposes of + * authority destroys what the operation being undone never created (#788). + * + * The input is the stack array rather than the whole `SessionData`, and that + * narrowness IS the guarantee. A release policy expressed as an option can be + * omitted, and omission reads as the destructive direction; a function holding + * no reference to `claims` or `stashedRunbookId` cannot revoke or clear either, + * today or after an edit that forgets why it must not. + * + * Only the topmost occurrence goes. `session_stack` carries no uniqueness + * constraint — adding one would make an existing session with a duplicate + * impossible to load, which the no-migration rule forbids — so a run can + * appear lower in the stack, and an undo of one push must leave that entry + * alone. `projectRunbookRelease` filters every occurrence, which is right for + * a release and wrong here. + * + * Returns nothing: the sole caller reads the new top back off the stack it + * passed in, and a `not-on-stack` status it cannot reach would be dead code + * rather than defence. + * + * @param defaultStack - The session's active-run stack, mutated in place + * @param runbookId - Run whose topmost stack entry is removed + */ +export function projectStackPop(defaultStack: RunId[], runbookId: RunId): void { + const index = defaultStack.lastIndexOf(runbookId); + // Guarded because `splice(-1, 1)` would remove the top — a run that is absent + // must cost nothing, not somebody else's entry. + if (index === -1) return; + defaultStack.splice(index, 1); +} + /** Force-terminal command kind that drives inline-root resolution. */ export type InlineForceTerminalKind = 'complete' | 'stop'; @@ -2000,11 +2038,30 @@ export class SessionService { * `execution_in_progress` refusal naming a foreign run this call was never * going to touch. * + * What it removes is one stack entry. Claims controlling the run survive, and + * a stash slot naming it survives, because the push this undoes created + * neither (#788). Revoking the run-control claim here was irrecoverable: the + * child is still live and still resumable, and `adoptRunControlClaim` refuses + * to re-mint once that child has issued a delegation, so nothing addressed + * the run again — and the holder read the revocation as `claim-rotated`, a + * rotation that never happened. + * + * Still {@link mutateGuarded}. A stack-only projection issues no guarded + * statement, so the two ownership refusals below are now unreachable through + * this path, and the caller's arms for them are unreachable with them. They + * stay until a test drives a run holding a stale lease: the preflight refuses + * on `exec_token IS NOT NULL` with no liveness probe, and this method's one + * caller is the crash-recovery path where the child provably holds a lease + * naming a dead pid — so the guard can only refuse where the undo must run. + * Removing it is a separate change, and the projection above is what makes it + * a no-op rather than a lossy edit. + * * @param expected - The only run this may pop. * @returns `popped` with the new top when `expected` was still active, or * `not-active` naming whatever holds the top instead. Refused * `execution_in_progress` or `recovery_required` instead when `expected` is - * execution-owned or awaiting recovery; the value is absent then. + * execution-owned or awaiting recovery; the value is absent then. Claims + * and the stash slot are untouched on every arm. */ async popRunbookIfActive(expected: RunId): Promise> { return this.mutateGuarded( @@ -2023,12 +2080,15 @@ export class SessionService { if (topId !== expected) { return { status: 'not-active', activeRunbookId: topId }; } - // The release mutates `ctx.session` in place, and `expected` is provably - // on the stack here, so the new top is read back rather than taken from - // the result's `released` arm — the `not-found` arm that branch would - // guard against cannot be reached from inside this `if`, and an - // unreachable guard is dead code, not defence. - this.releaseFromSession(ctx.session, expected); + // A stack-only projection, never `releaseFromSession`: this undoes a + // push, and the push minted no claim and wrote no stash slot, so the + // undo must dispose of neither. It takes the stack array alone, so it + // could not reach `claims` even if a later edit asked it to. + // + // It mutates in place, and `expected` is provably on the stack here, so + // the new top is read back rather than returned — a status arm that + // cannot be reached from inside this `if` is dead code, not defence. + projectStackPop(ctx.session.defaultStack, expected); return { status: 'popped', runbookId: expected, From 519df2cd428e010b77c207cf8145b5f95dc5a888 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Wed, 19 Aug 2026 17:55:32 +1000 Subject: [PATCH 10/11] test(core,cli): characterise today's STEP_ENTERED divergence (#816) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ...ntered-divergence-characterisation.test.ts | 195 ++++++++++++++++++ .../__tests__/services/execution-loop.test.ts | 142 +++++++++++++ .../runbook/collection-service.test.ts | 142 ++++++++++++- 3 files changed, 478 insertions(+), 1 deletion(-) create mode 100644 packages/cli/__tests__/integration/step-entered-divergence-characterisation.test.ts diff --git a/packages/cli/__tests__/integration/step-entered-divergence-characterisation.test.ts b/packages/cli/__tests__/integration/step-entered-divergence-characterisation.test.ts new file mode 100644 index 000000000..40909ce86 --- /dev/null +++ b/packages/cli/__tests__/integration/step-entered-divergence-characterisation.test.ts @@ -0,0 +1,195 @@ +import { describe, expect, it, beforeEach, afterEach } from '@jest/globals'; +import { writeFile } from 'node:fs/promises'; +import { join } from 'node:path'; +import { + createTestWorkspace, + findActionOutput, + parseConcatenatedJson, + requireFrontierToken, + runCliInProcess, + withRunTarget, + type TestWorkspace, +} from '../helpers/test-utils.js'; + +/** + * Characterisation of today's `STEP_ENTERED` divergence between `rundown run` + * and `rundown collect` (#816, part of #799). + * + * These tests pin CURRENT behaviour, including the defect. Two functions build + * the `StepEntryMetadata` that becomes a `STEP_ENTERED` payload and they + * disagree: the CLI execution loop renders `description`, `prompt`, + * `commandCode` and `commandLang`; core's collection service 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. + * + * The divergence is asserted end to end, on ONE substep of ONE runbook entered + * twice — first by `rundown run`, then by the RETRY re-entry that `rundown + * collect` drives — because that is the level at which an orchestrator observes + * it. Everything except the rendered fields is asserted to AGREE, which is what + * makes the missing description a divergence rather than a different event. + * + * CORRECT VALUE: the run payload. A substep's description and prompt do not + * depend on which command entered it, so `rundown collect` under-fills. When + * #799 moves the rendering behind the machine, the two `toBeUndefined()` + * assertions below become the run payload's values and this reads as a + * one-line diff. + */ +describe('STEP_ENTERED divergence between run and collect (#816 characterisation)', () => { + let workspace: TestWorkspace; + + beforeEach(async () => { + workspace = await createTestWorkspace(); + }); + + afterEach(async () => { + await workspace.cleanup(); + }); + + /** The H3 title of substep 1.1, which the parser stores as its description. */ + const SUBSTEP_DESCRIPTION = 'Task A'; + /** The prose under substep 1.1, which the parser stores as its prompt. */ + const SUBSTEP_PROMPT = 'Describe task A in prose.'; + + /** + * Every `step_entered` event in a command's stdout, in emission order. + * + * Events arrive as concatenated JSON, sometimes nested inside arrays, so the + * flatten is not optional. + * + * @param stdout - Raw stdout from one CLI invocation. + * @returns The `step_entered` payloads that invocation emitted. + */ + function stepEnteredEvents(stdout: string): Record[] { + const flat: Record[] = []; + const walk = (nodes: unknown[]): void => { + for (const node of nodes) { + if (Array.isArray(node)) walk(node); + else if (node && typeof node === 'object') flat.push(node as Record); + } + }; + walk(parseConcatenatedJson(stdout)); + return flat.filter((event) => event.type === 'step_entered'); + } + + /** + * A parent whose substeps both DELEGATE, aggregating `FAIL ANY RETRY 1 STOP`. + * + * The RETRY is what makes the SAME substep reachable from both paths: the + * first entry is `rundown run`'s, and the re-entry the retry produces is + * projected by `rundown collect` through core's re-entry frontier seam. + */ + async function writeRunbooks(): Promise { + const passChild = [ + '# Child Pass', + '', + '## 1. Do work', + '', + '- PASS COMPLETE', + '- FAIL STOP', + '', + '```bash', + 'rd echo --result pass', + '```', + '', + ].join('\n'); + const failChild = [ + '# Child Fail', + '', + '## 1. Do work', + '', + '- PASS COMPLETE', + '- FAIL STOP', + '', + '```bash', + 'rd echo --result fail', + '```', + '', + ].join('\n'); + const parent = [ + '# Parent', + '', + '## 1. Fan-out', + '', + '- DELEGATE', + '- PASS ALL CONTINUE', + '- FAIL ANY RETRY 1 STOP', + '', + `### 1.1 ${SUBSTEP_DESCRIPTION}`, + '', + SUBSTEP_PROMPT, + '', + '- child.runbook.md', + '', + '### 1.2 Task B', + '', + '- child-fail.runbook.md', + '', + '## 2. Done', + '', + '- PASS COMPLETE', + '- FAIL STOP', + '', + 'Finished.', + '', + ].join('\n'); + + await writeFile(join(workspace.cwd, 'runbooks', 'child.runbook.md'), passChild); + await writeFile(join(workspace.runbooksDir(), 'child.runbook.md'), passChild); + await writeFile(join(workspace.cwd, 'runbooks', 'child-fail.runbook.md'), failChild); + await writeFile(join(workspace.runbooksDir(), 'child-fail.runbook.md'), failChild); + await writeFile(join(workspace.cwd, 'runbooks', 'parent.runbook.md'), parent); + } + + /** Claim a delegated substep's token and report its result under that bearer. */ + async function reportUnder(token: string, result: 'pass' | 'fail'): Promise { + const claim = await runCliInProcess(`claim ${token}`, workspace); + expect(claim.exitCode).toBe(0); + const claimId = String(findActionOutput(claim.stdout)!.claim_id); + await runCliInProcess([result, '--claim-id', claimId], workspace); + } + + it('drops the rendered description and prompt on the collect path and keeps every other field', async () => { + await writeRunbooks(); + + // ---- Path 1: `rundown run` enters substep 1.1 for the first time. ------- + const start = await runCliInProcess('run --prompted runbooks/parent.runbook.md', workspace); + expect(start.exitCode).toBe(0); + const runEntries = stepEnteredEvents(start.stdout); + expect(runEntries).toHaveLength(1); + const runEntry = runEntries[0]; + + // The loop's builder renders both fields off the resolved substep. + expect(runEntry.description).toBe(SUBSTEP_DESCRIPTION); + expect(runEntry.prompt).toBe(SUBSTEP_PROMPT); + + // ---- Drive the aggregation to RETRY so collect re-enters 1.1. ---------- + await reportUnder(requireFrontierToken(start.stdout, '1.1'), 'pass'); + await reportUnder(requireFrontierToken(start.stdout, '1.2'), 'fail'); + + // ---- Path 2: `rundown collect` re-enters the SAME substep. ------------- + const collected = await runCliInProcess(await withRunTarget(['collect'], workspace), workspace); + expect(collected.exitCode).toBe(0); + const collectEntries = stepEnteredEvents(collected.stdout); + expect(collectEntries).toHaveLength(1); + const collectEntry = collectEntries[0]; + + // THE DIVERGENCE. Same substep, same runbook, same cursor — and core's + // builder carries neither rendered field. + expect(collectEntry.description).toBeUndefined(); + expect(collectEntry.prompt).toBeUndefined(); + + // ...and everything the two builders both fill agrees, which is what makes + // the two payloads comparable at all. Asserted field by field rather than + // by diffing the objects, because the frontier tokens are freshly minted on + // re-entry (that is the RETRY working) and the envelope carries a `seq`. + expect(collectEntry.position).toEqual(runEntry.position); + expect(collectEntry.stepName).toBe(runEntry.stepName); + expect(collectEntry.isSubstep).toBe(runEntry.isSubstep); + expect(collectEntry.prompted).toBe(runEntry.prompted); + expect(collectEntry.hasCommand).toBe(runEntry.hasCommand); + expect(collectEntry.runbookId).toBe(runEntry.runbookId); + // Both entries are the delegating substep, not the parent step. + expect(collectEntry.isSubstep).toBe(true); + expect(collectEntry.position).toMatchObject({ current: '1', substep: '1' }); + }, 30_000); +}); diff --git a/packages/cli/__tests__/services/execution-loop.test.ts b/packages/cli/__tests__/services/execution-loop.test.ts index 987bf9335..368d4653e 100644 --- a/packages/cli/__tests__/services/execution-loop.test.ts +++ b/packages/cli/__tests__/services/execution-loop.test.ts @@ -1176,6 +1176,148 @@ describe('runExecutionLoop', () => { expect(result).toBe('waiting'); }); + // --------------------------------------------------------------------------- + // #816 characterisation — the LOOP half of the STEP_ENTERED divergence. + // + // Two builders produce the `StepEntryMetadata` behind a STEP_ENTERED payload: + // this loop's (`execution.ts`, every field filled) and core's collect-side one + // (`collection-service.ts`, ids/position/name/flags only). These pin what the + // loop builder does TODAY on the two axes the end-to-end contrast in + // `integration/step-entered-divergence-characterisation.test.ts` cannot reach, + // so #799's move reads as an assertion flipping rather than as a new test. + // + // The entry is captured off `observeExecutionUnitEntry` 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. + // --------------------------------------------------------------------------- + describe('STEP_ENTERED entry metadata (#816 characterisation)', () => { + type ObserveEntryMock = jest.Mock< + (id: string, steps: unknown, entry: Record) => Promise + >; + + /** + * The entry metadata the loop handed core for the unit it entered. + * + * @returns The single captured `StepEntryMetadata`-shaped argument. + */ + function capturedEntry(): Record { + const { calls } = (mockActorService.observeExecutionUnitEntry as ObserveEntryMock).mock; + expect(calls).toHaveLength(1); + return calls[0][2]; + } + + it('composes prompted from the loop flag OR the prompted-FOR step kind', async () => { + // A FOR step whose bounds did not resolve is demoted to `prompted-for`: + // substeps, no iteration machinery, the original FOR text kept as the + // step prompt. + const promptedForSteps: LooseStep[] = [ + { + kind: 'prompted-for', + name: '1', + description: 'Fan out over an unresolved source', + prompt: 'FOR item IN {{ items }}', + substeps: [ + { + id: '1', + description: 'Handle one item', + transitions: { + pass: { kind: 'pass', retry: 0, action: { type: 'CONTINUE' }, next: 'CONTINUE' }, + fail: { kind: 'fail', retry: 0, action: { type: 'STOP' }, next: 'STOP' }, + }, + }, + ], + transitions: { + pass: { kind: 'pass', retry: 0, action: { type: 'CONTINUE' }, next: 'CONTINUE' }, + fail: { kind: 'fail', retry: 0, action: { type: 'STOP' }, next: 'STOP' }, + }, + }, + ]; + mockManager.load.mockResolvedValue(makeLoopState('1', { substep: '1' })); + + const result = await runExecutionLoop( + asManager(mockManager), + runbookId, + asSteps(promptedForSteps), + '/tmp', + // The persisted/CLI prompted flag, explicitly FALSE. Everything below + // is about the second term. + false, + asEmitter(mockEmitter), + ); + + expect(result).toBe('waiting'); + // THE DIVERGENCE. The loop ORs `currentStep.kind === 'prompted-for'` into + // the flag it was called with; core's collect-side builder reads + // `!!advanced.prompted` alone and would report `false` for this same + // cursor on this same step. + // + // CORRECT VALUE: `true`. The payload field documents whether execution is + // prompted rather than automatic, and a prompted-FOR step IS prompted — + // the loop returns 'waiting' on exactly this term, as asserted above. So + // the collect path under-reports, and #799's move makes the composed + // value the one both paths derive. + expect(capturedEntry().prompted).toBe(true); + }); + + it('takes substepId from the raw cursor and isSubstep from the resolved unit', async () => { + // A cursor naming a substep the current step does not define. + // `resolveCurrentExecutionUnit` falls back to the parent step for it, so + // the two fields are derived from different sources and disagree. + const substepSteps: LooseStep[] = [ + { + kind: 'substeps', + name: '1', + description: 'Fan out', + aggregation: { strategy: 'ALL' }, + substeps: [ + { + id: '1', + description: 'The only live substep', + transitions: { + pass: { kind: 'pass', retry: 0, action: { type: 'CONTINUE' }, next: 'CONTINUE' }, + fail: { kind: 'fail', retry: 0, action: { type: 'STOP' }, next: 'STOP' }, + }, + }, + ], + transitions: { + pass: { kind: 'pass', retry: 0, action: { type: 'CONTINUE' }, next: 'CONTINUE' }, + fail: { kind: 'fail', retry: 0, action: { type: 'STOP' }, next: 'STOP' }, + }, + }, + ]; + mockManager.load.mockResolvedValue(makeLoopState('1', { substep: '9' })); + + const result = await runExecutionLoop( + asManager(mockManager), + runbookId, + asSteps(substepSteps), + '/tmp', + false, + asEmitter(mockEmitter), + ); + + expect(result).toBe('waiting'); + const entry = capturedEntry(); + // THE DIVERGENCE, inside one builder rather than between two: `substepId` + // comes straight off the raw cursor while `isSubstep` comes off the + // resolved execution unit, so a cursor naming no live substep yields a + // populated `substepId` alongside `isSubstep: false`. + // + // CORRECT VALUE: `substepId: undefined` with `isSubstep: false`. Both + // describe the same question — is the unit being entered a substep? — so + // both must come from the resolved unit. This matters beyond tidiness: + // the frontier seams gate credential disclosure on `isSubstep`, and + // `deriveStepEnteredEffect`'s cursor guard fires on `substepId`, so the + // two fields answering differently splits one decision across two seams. + expect(entry.substepId).toBe('9'); + expect(entry.isSubstep).toBe(false); + // The name confirms the fallback landed on the parent step: the substep + // arm would have used the substep's own id. + expect(entry.stepName).toBe('1'); + expect(entry.description).toBe('Fan out'); + }); + }); + it('executes command and advances to next step', async () => { mockManager.load .mockResolvedValueOnce(makeLoopState('1')) diff --git a/packages/core/__tests__/runbook/collection-service.test.ts b/packages/core/__tests__/runbook/collection-service.test.ts index ac4bcb59a..8d554cec0 100644 --- a/packages/core/__tests__/runbook/collection-service.test.ts +++ b/packages/core/__tests__/runbook/collection-service.test.ts @@ -44,7 +44,10 @@ import type { CollectionSessionService, RunbookCollectionServiceDependencies, } from '../../src/runbook/collection-service.js'; -import type { ExecutionObservationEffect } from '../../src/events/execution-observation.js'; +import type { + ExecutionObservationEffect, + StepEntryMetadata, +} from '../../src/events/execution-observation.js'; import type { RecoveryActor } from '../../src/runbook/execution-recovery-service.js'; import { ExecutionLifecycleService } from '../../src/runbook/execution-lifecycle-service.js'; import { brandCurrentCursorResolvedCompletionForTest } from '../../src/runbook/completion-service.js'; @@ -2598,6 +2601,143 @@ describe('RunbookCollectionService', () => { }); }); + // --------------------------------------------------------------------------- + // #816 characterisation — the COLLECT half of the STEP_ENTERED divergence. + // + // `prepareCollectReEntryFrontier` and the CLI execution loop both build a + // `StepEntryMetadata`, and they disagree about what it carries. These pin what + // THIS builder does today. The loop half is pinned in the CLI's + // `services/execution-loop.test.ts`, and the two paths are contrasted end to + // end in `integration/step-entered-divergence-characterisation.test.ts`. + // + // Additive by construction: every payload pinned above stays exactly as it is, + // so the fix that follows shows up as an assertion flipping here rather than + // as churn across the suite. + // --------------------------------------------------------------------------- + describe('STEP_ENTERED entry metadata (#816 characterisation)', () => { + /** + * Run a no-op collect that projects a valid frontier and return the entry it + * handed `observeExecutionUnitEntry`. + * + * A local twin of `projectFrontierAndCapture` above, which closes over the + * shared `steps` fixture. This one takes the step graph, because the + * `prompted` characterisation below needs a step KIND the shared fixture + * does not carry. + * + * @param collectSteps - Step graph the collect resolves its target step in. + * @param overrides - Target-state overrides applied on top of the fixture. + * @returns The single captured entry metadata. + */ + async function captureCollectEntry( + collectSteps: ResolvedStep[], + overrides: Partial = {}, + ): Promise { + const frameKey = buildFrameKey('1'); + const retry = frontierEntry(); + const target = state({ + retryCount: 1, + snapshot: { context: { delegateFrontier: [retry.persisted] } }, + ...overrides, + }); + await manager.save(target); + jest.spyOn(completionService, 'prepareResolvedCompletionDrain').mockResolvedValue({ + status: 'continue', + state: target, + unresolved: 1, + applied: [], + }); + const observeEntrySpy = jest + .spyOn(actorService, 'observeExecutionUnitEntry') + .mockResolvedValue([]); + jest + .spyOn(actorService, 'prepareActorMutation') + .mockResolvedValue(preparedMutation(target, { context: {} })); + + const outcome = await collectionService.collectDelegationOutcomes({ + targetState: target, + steps: collectSteps, + callerEvidence: ORCHESTRATOR_EVIDENCE, + frame: activeFrame(frameKey, 1), + }); + + expect(outcome.kind).toBe('collection_applied'); + expect(observeEntrySpy).toHaveBeenCalledTimes(1); + return observeEntrySpy.mock.calls[0][2]; + } + + it('carries none of the four rendered fields, though the substep it names has a description', async () => { + // The shared fixture gives substep '1' a description, so a builder that + // rendered anything at all would have something to render here. + expect(steps[0]).toMatchObject({ + substeps: expect.arrayContaining([expect.objectContaining({ id: '1', description: 'A' })]), + }); + + const entry = await captureCollectEntry(steps); + + // THE DIVERGENCE. All four rendered fields are optional on + // `StepEntryMetadata`, which is what lets this builder omit every one of + // them and still compile against a type the CLI loop fills completely. + // `hasCommand` on the derived event is computed as + // `commandCode !== undefined`, so the omission does not stop at the + // absent fields — it decides a flag too. + // + // CORRECT VALUE: the rendered fields, as the loop supplies them. A + // substep's description does not depend on which command entered it, so + // this path under-fills. + expect(entry.description).toBeUndefined(); + expect(entry.prompt).toBeUndefined(); + expect(entry.commandCode).toBeUndefined(); + expect(entry.commandLang).toBeUndefined(); + // The fields it DOES fill, so the omission reads as a gap in one builder + // rather than as an entry naming some other unit. + expect(entry).toMatchObject({ + stepId: '1', + substepId: '1', + stepName: '1', + isSubstep: true, + }); + }); + + it('reads prompted off the persisted flag alone, never off the step kind', async () => { + // The same two delegate substeps, hung off a step whose FOR bounds did + // not resolve. `resolvedStepHasSubsteps` accepts `prompted-for`, so the + // collect reaches its frontier exactly as it does for the shared fixture. + const promptedForSteps: ResolvedStep[] = [ + { + kind: 'prompted-for', + name: '1', + description: 'Delegate work', + prompt: 'FOR item IN {{ items }}', + substeps: [ + { id: '1', description: 'A', delegate: true, transitions: tx('CONTINUE', 'STOP') }, + { id: '2', description: 'B', delegate: true, transitions: tx('CONTINUE', 'STOP') }, + ], + transitions: tx('CONTINUE', 'STOP'), + }, + { + kind: 'base', + name: '2', + description: 'After collection', + transitions: tx('CONTINUE', 'STOP'), + }, + ]; + + const entry = await captureCollectEntry(promptedForSteps, { prompted: undefined }); + + // THE DIVERGENCE. This builder never looks at the step graph, so a + // prompted-FOR step reports `prompted: false` on the persisted flag + // alone. The CLI loop ORs `currentStep.kind === 'prompted-for'` into the + // same field and reports `true` for this exact cursor and step — pinned + // in the CLI's `execution-loop.test.ts`. + // + // CORRECT VALUE: `true`. The field documents whether execution is + // prompted rather than automatic, and a prompted-FOR step is: the loop + // returns 'waiting' on that same term. So this path under-reports, and + // the fix makes the composed value the one both paths derive. + expect(entry.prompted).toBe(false); + }); + }); + // --------------------------------------------------------------------------- // All-or-none: the transaction this change exists to create. // From 88aeab2dc72a7d5cc9a3792151fec2677411bec5 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Wed, 19 Aug 2026 18:15:17 +1000 Subject: [PATCH 11/11] docs(core): narrow the extractUnitOutputs no-substep claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/core/src/runbook/execution-units.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/core/src/runbook/execution-units.ts b/packages/core/src/runbook/execution-units.ts index 8093f32e9..fdd36d7fb 100644 --- a/packages/core/src/runbook/execution-units.ts +++ b/packages/core/src/runbook/execution-units.ts @@ -26,10 +26,16 @@ export function resolveCurrentExecutionUnit( * Extract the OUTPUTS declarations attached to one execution unit. * * For a substep, return the substep's OUTPUTS; for a step-level command, - * return the parent step's OUTPUTS. A `substepId` that names no substep on - * `currentStep` yields no declarations rather than silently falling back to - * the parent's — the parent's OUTPUTS belong to a different channel path, so - * capturing them under a substep scope would misfile them. + * return the parent step's OUTPUTS. On a step that DEFINES substeps, a + * `substepId` naming none of them yields no declarations rather than silently + * falling back to the parent's — the parent's OUTPUTS belong to a different + * channel path, so capturing them under a substep scope would misfile them. + * + * A `substepId` on a step that defines NO substeps is the other case, and it + * DOES return the step's own OUTPUTS. That is not the misfiling above: + * {@link resolveCurrentExecutionUnit} resolves the same cursor to the parent + * step, so the unit being entered really is the step and its declarations are + * the ones in scope. * * @param currentStep - The resolved parent step * @param substepId - Substep identifier, or undefined for a step-level unit