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/.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/.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/.prettierignore b/.prettierignore index 04920dd78..9e6f8354c 100644 --- a/.prettierignore +++ b/.prettierignore @@ -7,6 +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 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/** 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 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/cli/__tests__/integration/claim-disposition-characterisation.test.ts b/packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts new file mode 100644 index 000000000..51218169d --- /dev/null +++ b/packages/cli/__tests__/integration/claim-disposition-characterisation.test.ts @@ -0,0 +1,171 @@ +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. + * + * 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; + + 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, + }); + }); +}); 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-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..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')) @@ -2709,10 +2851,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/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. // 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/__tests__/runbook/session-release.test.ts b/packages/core/__tests__/runbook/session-release.test.ts new file mode 100644 index 000000000..bed3e1a3a --- /dev/null +++ b/packages/core/__tests__/runbook/session-release.test.ts @@ -0,0 +1,297 @@ +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. + * + * 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_${index.toString(16).padStart(32, '0')}`), + 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); + 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 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 + // 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'); + }); +}); + +/** + * 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 + // `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(), + seeds, + (ids, roles, pick, order) => { + 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()); + 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', + ); + }, + ), + ); + }); + + 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 }), + seeds, + (ids, roles, order) => { + 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)); + expect(apply(permute(releases, order))).toEqual(apply(releases)); + }, + ), + ); + }); +}); 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/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..fdd36d7fb 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,32 @@ 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. 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 + * @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 722e51576..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, @@ -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 @@ -688,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}. */ diff --git a/packages/core/src/runbook/session-release.ts b/packages/core/src/runbook/session-release.ts new file mode 100644 index 000000000..400eb2f7c --- /dev/null +++ b/packages/core/src/runbook/session-release.ts @@ -0,0 +1,142 @@ +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. + * + * `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', + 'collateral', + '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. + * + * `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'; + // Stryker disable next-line ConditionalExpression,BlockStatement: unreachable — exhaustive `never` arm + 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; +} 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,