Render the execution unit behind the machine (#817, #819, #820)

.changeset/collect-emits-a-complete-step-entered.md

@@ -0,0 +1,81 @@
+---
+'@rundown-org/core': minor
+---
+
+# `rundown collect` emits the same `STEP_ENTERED` as `rundown run`
+
+Entering substep `1.1` via `rundown run` produced a `STEP_ENTERED` carrying its
+description and prompt. Entering the same substep of the same runbook via the
+RETRY re-entry `rundown collect` drives produced one carrying neither. Two
+functions built the payload's `StepEntryMetadata` and they disagreed: the CLI
+execution loop rendered every field, and the collection service hand-built ids,
+position, name and flags with every rendered field absent. All four are optional
+on the type, which is what let the disagreement compile.
+
+The collect path now enters through the same core seam the loop does. The
+hand-built entry is gone, along with the Stryker equivalence annotations that
+existed only because half the fields it built were never observed.
+
+Three assertions flip with it, each pinned by #816 against the old behaviour:
+
+- The rendered fields. `description` and `prompt` are present on the collect
+  payload, end to end, and equal to the run payload's for the same unit.
+- `prompted`. The collect path read `!!state.prompted` alone; it now composes
+  the persisted flag with the step kind, so a prompted-FOR step reports `true`
+  on both paths.
+- `substepId`. It came off the raw cursor while `isSubstep` came off the
+  resolved unit, so a cursor naming no live substep produced a populated
+  `substepId` beside `isSubstep: false`. Both answer one question and both now
+  come off the resolved unit — which matters beyond tidiness, because the
+  frontier seams gate credential disclosure on `isSubstep`.
+
+**The fenced frontier seam sheds its `entry` parameter**, as its unfenced twin
+already had. `prepareReEntryFrontierConsume` derives the substep question from
+the state it holds and returns the projected bearers rather than an entry, so
+there is no longer any route by which a caller can hand either seam an entry
+that disagrees with the run.
+
+**Two guards are deleted rather than left unreachable.**
+`deriveStepEnteredEffect` refused an entry whose `stepId` / `substepId`
+disagreed with the snapshot. Those existed because the entry was a parameter;
+with one producer that reads the cursor and the snapshot off the same
+`RunbookState`, the mismatch is unrepresentable.
+`RunbookActorService.observeExecutionUnitEntry` goes with them — its last caller
+was the collect path — and `StepEntryMetadata` becomes a local passed between
+two core functions rather than a parameter of anything.
+
+**A new failure surface, named.** A collect that has committed can now fail to
+RENDER the entry its bearers ride on — typically a `--helpers` helper raising —
+where before it emitted a thinner event that needed no rendering. Nothing
+recovers the bearers (the consume is durable, so a retry answers the idempotent
+no-op), so the collect still rejects rather than reporting a phantom success
+with an empty observation list. It rejects with a code of its own,
+`DELEGATION_FRONTIER_DISCLOSURE_FAILED` (RD-833), instead of escaping bare as
+RD-999 "Unknown error" — an envelope that cannot carry this condition's
+recovery, which is "fix the helper, then re-delegate". A render refusal that is
+`InvalidRunbookStateError` keeps its own class, so the CLI's RD-309 arm still
+prints finish/stop/prune for a run that cannot describe itself.
+
+Bearer-disclosure ordering is unchanged and still asserted: the commit lands
+before the entry is derived, so a refused transaction consumes nothing and
+discloses nothing.
+
+**The persisted-snapshot guards are now typed too, and RD-833 depends on it.**
+`assertFreshSnapshotValue` and `compileMachineFromState` refuse an unreadable
+`snapshot.value`, a transient parent-entry state, a cursor naming a step the
+runbook no longer declares, and a missing `frontmatterOutputs` — all one run's
+corrupt persisted state, and every message already spelled RD-309's remediation
+("Prune invalid runbook state and restart execution"). They threw a bare
+`Error`, which reached the CLI as RD-999. They now raise
+`InvalidRunbookStateError` with a typed reason, which is what keeps them off
+RD-833: without it, a collect whose committed target carried an unparseable
+`stateValue` would have told the operator to fix a helper and re-issue
+delegations when the real recovery is prune/restart. Every other caller of those
+guards gains the RD-309 envelope with them.
+
+One narrowing worth knowing. The artifact-path projection used to fall back to
+`WORK_DIR` when a run carried no `WorkPath`, while the render context the same
+entry expands helper paths against refused a missing `WorkPath` outright — so
+the fallback could only ever produce an entry whose artifact paths and helper
+paths named different roots. There is one read now, and a run with no `WorkPath`
+is refused as corrupt persisted state on both paths.

.changeset/core-entry-seam.md

@@ -0,0 +1,174 @@
+---
+'@rundown-org/core': minor
+'@rundown-org/cli': minor
+---
+
+# Enter an execution unit through core, not by rendering it in the CLI
+
+The CLI execution loop used to render the unit it was about to enter — merge
+effective variables, build the step frame, pick which expander applied to which
+field, assemble a `StepEntryMetadata` — and then read its own rendered command
+back out to decide what to do next. `expandedCommandCode === undefined` was the
+loop's control-flow signal for "nothing to run". Rendering precedence is a
+language-level concern the spec owns, so it belongs behind the machine (#799);
+`undefined`-as-signal is a missing type.
+
+`RunbookActorService.enterExecutionUnit({ state, steps })` now does all three —
+render, observe, classify — and returns `ExecutionUnitEntry`, a three-arm union:
+
+- **`awaiting`** — nothing for this process to run. One arm for the three
+  conditions the loop used to spell out itself: a prompted run, a prompted-FOR
+  step, and a unit that declares no command.
+- **`runnable`** — carries a `RenderedUnitCommand`: the expanded code, its
+  display projection, and the `RD_*` environment for the child process.
+- **`inline-launch`** — carries the one-shot intent the machine prepared.
+
+The loop sends state and steps and reads back a classified entry. It renders
+nothing, and it derives exactly one fact for itself — whether the cursor is on a
+substep — because the missing-deriver authority precondition has to answer that
+before any entry exists.
+
+**The command is one value, and that is the point.** The string announced in
+`STEP_ENTERED.commandCode` and the string handed to `EXECUTE_COMMAND` come from
+one expansion, so a non-deterministic `--helpers` helper cannot make a runbook
+run something other than what it announced. That property used to be held by
+statement ordering in the loop; it is now held by construction.
+
+The comment that ordering carried was wrong and is not carried forward. It
+claimed artifact-producing helpers "append a manifest row per call, so a second
+expansion would duplicate the entries". `expandLoopVariablesForCommand` is
+synchronous and reduces to `substituteText`, which imports neither `fs` nor the
+manifest module; the manifest append is idempotent by identity anyway. The real
+constraint is helper determinism, which is what the docs now say.
+
+`RenderedUnitCommand` is nominally branded — tier 1 of the doctrine in
+`effective-vars.ts`, a `declare const` `unique symbol`, minted only inside
+`deriveExecutionUnitEntry`. Tier 1 is right here because the record is consumed
+by typed functions and never round-trips through JSON: `EXECUTE_COMMAND` targets
+`__execute-command`, whose `invoke.input` reads the event with no `assign`, so a
+rendered command never reaches persisted context.
+
+The brand is load-bearing rather than decorative, and two things keep it that
+way. `RenderedUnitCommand` is **not** re-exported from `@rundown-org/core`, so
+outside core the type cannot be named — and a type that cannot be named cannot
+be asserted to, aliased, or reached through a namespace import. Inside core,
+where a relative import puts the name back in scope, a type-aware ESLint rule
+(`local/no-rendered-unit-command-cast`,
+`eslint-rules/no-rendered-unit-command-cast.mjs`) bans every assertion that can
+mint one. Tier 1 means the assertion IS the mint, so the set of assertion
+SYNTAXES is the whole surface — but a selector that matches syntax has to
+enumerate every spelling, and an import rename (`RenderedUnitCommand as Local`)
+produces a spelling no enumeration anticipates. The rule instead resolves the
+asserted-to TYPE through the checker and walks its symbol, base types, and
+union/intersection members, so a rename, an alias two hops away, or an interface
+that inherits the brand all resolve to the same declared symbol and get caught
+the same as a direct cast. `scripts/__tests__/eslint-brand-cast-guard.test.mjs`
+lints one committed fixture per laundering route through the real config,
+because a bug in the rule's type resolution matches nothing and otherwise reads
+as passing. The fixtures are real `.ts` files under
+`packages/core/__tests__/fixtures/brand-cast/` — a type-aware rule needs ESLint
+and the checker to be reading the same bytes, which `lintText` against a
+borrowed `filePath` does not guarantee, and a file this test writes and sweeps
+is visible to everything else that reads the working tree while it exists.
+Separately, the CLI, MCP and plugin `src/**` may no longer import
+`buildStepVariables`, `expandLoopVariables`, `expandLoopVariablesForCommand`, or
+`deriveExecutionUnitEntry` from core at all.
+
+**The entry seam's internals came off the public barrel** on the same reasoning.
+`deriveStepEnteredEffect` used to carry two cursor-mismatch guards, refusing an
+entry whose `stepId` / `substepId` disagreed with the snapshot; they are deleted
+because the entry now has exactly ONE producer, which reads the cursor and the
+snapshot off the same `RunbookState`. That argument only holds while a front end
+cannot reach the deriver with a hand-built entry, and a wildcard
+`export * from './execution-observation.js'` was putting the deriver,
+`StepEntryMetadata` and `StepEntryObservationInput` on `@rundown-org/core`
+without any file naming them. The barrel names its exports now.
+
+**`hasCommand` is now a field on the entry, derived from the parsed unit.** It
+used to be computed as `commandCode !== undefined` inside
+`deriveStepEnteredEffect`, which made a payload flag an accident of which
+builder produced the entry — the collect-side builder renders nothing, so every
+entry it produced reported `hasCommand: false` regardless of the unit. A command
+that renders to the empty string is now correctly `hasCommand: true`.
+
+**Both re-entry frontier seams shed their `entry` parameter.** Each read exactly
+one field off it — `isSubstep` — and both now derive that from the state they
+already hold, through the same `resolveCurrentExecutionUnit` the entry seam
+uses. A caller-supplied entry was the wrong shape for it anyway: the field
+describes the cursor, so taking it from the caller let an entry describing one
+cursor decide a question about another.
+
+The unfenced seam (`projectAndConsumeReEntryFrontier`) enters through
+`enterExecutionUnit` with the verified bearers attached, and its `projected` arm
+returns the whole classified entry rather than bare observations, so the caller
+gets the same classification on the re-entry path as on an ordinary one. The
+ordering guarantee is untouched: the consume still commits before the entry is
+returned, so a failed consume discloses no bearers. The fenced twin
+(`prepareReEntryFrontierConsume`, which `rundown collect` drives) returns the
+prepared state and the projected frontier, leaving the commit and the disclosure
+to the caller's transaction — `RunbookCollectionService` enters through
+`enterExecutionUnit` after its commit lands.
+
+**`enterExecutionUnit` is declared `async`.** Its body is synchronous today, but
+three refusals run before the derivation returns — the snapshot freshness gate,
+the machine compile, and the render itself — and without the keyword all three
+threw in the CALLER's tick rather than rejecting the promise the signature
+advertises. A caller that attached `.catch(...)` to the returned promise, or
+collected the call in `Promise.all`, observed none of them. `await` callers are
+unaffected.
+
+**Behaviour notes.** Helper path containment now resolves against `manager.cwd`
+rather than the `cwd` argument threaded into the loop — the canonicalised
+directory the actor service already used for artifact path projection, so the
+two can no longer disagree. (The CLI always passes `process.cwd()`, which Node
+returns already resolved, so the two values are identical in production; the
+canonicalisation only bites a caller that supplies a symlinked path, and
+containment wants the resolved one anyway.)
+
+Three refusals are now typed `InvalidRunbookStateError` rather than bare, which
+is what routes each onto the CLI's existing RD-309 finish/stop/prune recovery
+rather than an envelope carrying the wrong instruction:
+
+- A run whose `templateVars` carry no string `ContextId` or `WorkPath`
+  (`reason: 'missing_render_context'`).
+- A cursor naming a step the parsed runbook does not define
+  (`reason: 'cursor_step_not_in_runbook'`, raised by `findStepOrThrow`, which
+  now takes the run id for the defect). This one was a live misclassification:
+  the collect path wraps any non-`InvalidRunbookStateError` rejection out of the
+  entry seam as RD-833, whose recovery reads "fix the helper and re-delegate" —
+  the wrong instruction entirely for corrupt persisted state.
+- A persisted row carrying no `prompted` (`reason: 'missing_prompted'`).
+  `RunbookState.prompted` is required now and `create` always writes it, exactly
+  as `templateVars` already worked, so the `?? false` at each read site is gone
+  rather than unreachable. The field decides whether a run announces its
+  commands or executes them, and is the value a composing parent inherits down
+  into a fresh inline child, so defaulting it silently adapted an incompatible
+  row into an executing run.
+
+**Five branches came out as provably dead** while mutation-testing the new
+module to 100%, and each was a second spelling of a fact the types already
+carried: `currentStep.kind === 'command'` and `currentStep.kind === 'for'`
+(`command` is declared on `Substep` and `StepWithCommand` only, `forClause` on
+`ResolvedStepWithFor` only — both are now structural `in` checks); two of the
+five identity checks in the inline-intent projection (`entry.stepId` IS
+`state.step` by construction, and the entry's `substepId` check subsumes the raw
+cursor's); and an outer `typeof state.snapshot` guard the optional chain already
+answered. The cursor overlay that used to sit in `snapshotForEntry` went with
+them — it existed to satisfy `deriveStepEnteredEffect`'s guards, which this work
+deletes, so nothing read it any more.
+
+**The #816 divergence is closed rather than characterised.** `rundown collect`
+used to build its own partial entry — ids, position, name and flags, and none of
+the four rendered fields — while the CLI execution loop's builder filled all of
+them, so the same cursor produced two different `STEP_ENTERED` payloads
+depending on which command reached it. There is one builder now and nothing left
+to disagree, so the characterisation assertions are inverted rather than
+deleted: what was `toBeUndefined()` is the rendered value, and what was `false`
+is the composed one. They read the emitted payload, because the argument they
+used to capture is core-private. The end-to-end contrast is pinned in the CLI's
+`integration/step-entered-run-collect-agreement.test.ts`, and the two loop-half
+assertions moved from the CLI's mocked loop onto the real derivation in
+`packages/core/__tests__/runbook/execution-unit-entry.test.ts`, asserting the
+same values on the same fixtures.
+
+Behaviour is otherwise unchanged.

.changeset/loop-prompted-off-the-parameter.md

@@ -0,0 +1,35 @@
+---
+'@rundown-org/core': patch
+'@rundown-org/cli': patch
+---
+
+# `runExecutionLoop` reads `prompted` off the run, not off a parameter
+
+`runExecutionLoop` took `prompted: boolean` as its fifth argument. Every one of
+its six call sites passed the run's own persisted flag — four of them spelled
+`!!state.prompted` or the equivalent, and the other two (`transitions.ts`,
+`runbook-pipeline.ts`) passed a value core had already derived from
+`Boolean(state.prompted)` or written to the row with `manager.create`. The loop
+loads that same state on its first line.
+
+So the parameter was never a way to configure the loop. It was a way for a
+caller to disagree with state about a fact state owns, and nothing in the tree
+used it that way. It is gone; the loop derives `prompted` once from the state it
+loads, above the `while`, because the flag is fixed at run creation and cannot
+vary across iterations.
+
+`launchInlineChildFromIntent` keeps its own `prompted` parameter, and that is
+not the same fact: on the fresh-child branch it is the value the composing
+parent _inherits down_ into a child run that does not exist yet and therefore
+has no persisted flag to read. The resumed-child branch beside it already read
+`!!existingChild.prompted` rather than the parameter.
+
+Behaviour-neutral prefactor for #799: the entry seam that follows derives
+`prompted` from state, so the parameter had to go either way, and removing it
+first keeps that change to one concern.
+
+`LifecycleLoopDirective`'s `prompted` field goes with it. It existed only to
+feed that argument from `runSeamTransition`; with the argument gone it is a
+second copy of a persisted flag, and a second copy is a way to disagree. The
+directive now says only whether to run the loop, which is the one thing the
+frontend cannot decide for itself.

.changeset/reentry-frontier-commit-before-render.md

@@ -0,0 +1,30 @@
+---
+'@rundown-org/core': patch
+'@rundown-org/cli': patch
+---
+
+Fix the re-entry frontier seam's render ordering, deduplicate `findStepOrThrow`,
+let the entry seam accept a caller-precomputed position, and correct a
+misleading comment (code review follow-up on #817/#819/#820).
+
+- **`projectAndConsumeReEntryFrontier` (core)**: rendering the execution unit —
+  which can invoke non-idempotent `--helpers` JS — now happens AFTER
+  `DELEGATE_FRONTIER_CONSUMED` commits, not before. Previously a failed commit
+  still ran the render's side effects; the next retry re-projects the
+  still-persisted frontier and would run them again. The render now runs against
+  the committed state, mirroring the pattern `collection-service.ts`'s
+  `finishCollection` already used for the fenced twin.
+- **`findStepOrThrow` (core, cli)**: the CLI (`services/execution.ts`) and two
+  core modules (`collection-service.ts`, `completion-service.ts`) each carried
+  their own copy of this lookup. All three now import the canonical
+  implementation from `execution-units.ts`.
+- **`deriveExecutionUnitEntry` (core)**: accepts an optional caller-precomputed
+  `position`, used instead of re-deriving one via `countNumberedSteps` +
+  `buildStepPosition`. The CLI execution loop already computes this value once
+  per iteration for its own error-reporting events; it now forwards it to
+  `enterExecutionUnit` instead of paying for the identical derivation twice.
+- **`runExecutionLoop`'s `prompted` fallback comment (cli)**: corrected.
+  `RunbookState.prompted` and `CreateOptions.prompted` are genuinely optional at
+  the type level (unlike `templateVars`, `load()` carries no fail-closed guard
+  for a missing one). The fallback is unreachable only because of call-site
+  discipline, not because the type forbids `undefined`.