# Engine final fixes — findings F1-F5 of `FINAL-REVIEW.en.md`

Scope: the Engine copy in `engine/` (`vl-workflow-engine` 4.21.9), the authoritative
baseline for this snapshot. Independent implementation and regression testing of the
five findings in `FINAL-REVIEW.en.md`. Every behaviour claimed below is backed by an
executed test, not by assertion in this report: the pre-fix failures were recorded by
running the new suite against the unmodified baseline before any source edit.

This is not a release claim and not Matrix L05 acceptance. Flow's integration is not
touched: `flow-readonly/` was read for evidence only.

---

## 1. Disposition of the five findings

| # | Review claim | Disposition | Reproduced? |
| --- | --- | --- | --- |
| **F1** | The resolved-record check is echo-able | **Partly upheld, fixed differently.** The premise ("any validator that answers from the descriptor") does not hold for the real Flow validator, which is host-authoritative (§2). But the engine did accept a resolved verdict that proved only *identity*, and identity is exactly what the descriptor publishes. Fixed by requiring the answer **content** on both resolved paths, not by removing descriptor fields. | Yes — `F1` |
| **F2** | A handler that returns on abort/pause destroys the wait | **Upheld, fixed.** The record survived only a `throw`; a cooperative return deleted it *and* closed the step. | Yes — `F2a`, `F2b` |
| **F3** | A wait step reached but not re-entered leaves a stale record and a live verdict | **Upheld for `if:` and the result cache, fixed. Not reproducible for the breakpoint** (§4). | Yes — `F3a`, `F3b`, `F3c` |
| **F4** | Purity is a denylist over engine event types | **Upheld, fixed** by the inversion the review recommended. | Yes — `F4a`, `F4b`, `F4c` |
| **F5** | No run identity on the record; branch identity not re-checked at bind | **Defence in depth, not a product bug.** The real host already binds the exact run and refuses a foreign one (§2). Implemented as an additive, compare-when-present `runId` plus a bind-time owner assertion; the host store remains the single identity authority. | `F5a` reproduces the engine-side gap; `F5b` is fault injection — no natural trigger found |

### Concrete bug vs. defence in depth

* **Concrete bugs, reproduced without touching a checkpoint or misusing the API:**
  F2 (both variants), F3 (`if:` skip, result-cache hit, leftover verdict), F4 (both
  spans). Each of these silently loses or misdirects a human decision, or wedges a
  run permanently.
* **Contract tightening on a real (if narrow) hazard:** F1. No product bug against
  the shipped Flow validator; a defensible, compatible improvement (§3).
* **Defence in depth:** F5. Both halves guard against a divergence that the current
  code paths do not produce.

---

## 2. What the real Flow validator actually does (evidence for F1 and F5)

`flow-readonly/app-instance-manager.js:1710-1738`, `_validateDurableHumanWait`, read
only. It is host-authoritative on every axis the review worried about:

* **Run identity is already enforced by the host.** It refuses unless
  `instance.state.workflow.runId === descriptor.runId`, and additionally refuses a
  different live executor (`different_live_executor`).
* **It never echoes the descriptor's resolution.** It reads SQLite
  (`store.listApprovalTasks({instanceId, runId})`), requires **exactly one** task
  matching `nodeId === descriptor.stepID && waitToken === descriptor.token`, and takes
  `requestId` from `task.metadata.requestId`, `payload` from
  `task.metadata.humanWaitResponse` and `payloadDigest` from a digest computed over
  that stored answer. `descriptor.resolution` is not read at all.
* **It re-derives the request digest** from the persisted request and cross-checks the
  step digest, and it enforces status monotonicity from its own side
  (`descriptor.status === 'resolved' && task.status !== 'resolved'` → refuse).

So F1's "even a correct validator reports from `d.resolution` because the engine
offered them" does not describe the shipped host, and F5's run-identity fork is
already refused there. Both findings are therefore treated as hardening, and the fix
for F1 was chosen so that this validator needs **no change**: it already sends
`payload` on every resolved verdict.

---

## 3. Exact source edits

Only the three source/script files permitted by the task changed. Verified against
`baseline-manifest.json`: `CHANGED: [scripts/run-all-tests.mjs, lib/human-wait.js,
lib/engine.js]`, nothing else, nothing missing. `lib/types.js` needed no change.
Checkpoint `_version` stays 4 — `runId` is an additive field inside the already
additive `humanWaits` record.

### `engine/lib/human-wait.js`

1. **F4 — explicit neutral classification (`HUMAN_WAIT_CLEAN_ENGINE_EVENTS`).** New
   exported set holding only run-level lifecycle and diagnostic types
   (`workflow_start|done|failed|cancelled|paused`, `breakpoint_hit`, `step_skipped`,
   `step_print`, `workflow_preflight_start|done|failed`,
   `step_guard_passed|failed`). `taintHumanWait`'s engine arm changes from
   `!HUMAN_WAIT_EFFECT_EVENTS.has(type)` to `HUMAN_WAIT_CLEAN_ENGINE_EVENTS.has(type)`.
   The host arm is unchanged: `allowedHostEvents` still governs the host's own event
   types, and deliberately does **not** whitelist an engine type — that would be the
   "broad replay permission" the task rules out, and would weaken today's
   unconditional taint on `step_start`, `channel_send` and friends.
   `HUMAN_WAIT_EFFECT_EVENTS` is kept (documentation of the obvious cases) but the
   rule no longer depends on it being complete.
2. **F1 — the answer content is required on both resolved paths.** In
   `humanWaitVerdictRefusal`, the `payload === undefined → 'payload_missing'` check
   moved out of the pending-only branch to after the record/verdict comparisons, so it
   now applies to a resolved record too. Order is deliberate: `request_id_missing`,
   `payload_digest_missing`, `payload_mismatch`, `request_id_mismatch`,
   `payload_digest_mismatch` all still win first, so no existing refusal detail
   changed.
3. **F5 — `runId` on the record.** `createHumanWaitRecord` takes and stores
   `runId` (normalised to `null`); `humanWaitRecordDefect` adds a `run_id_shape`
   check; `humanWaitReentryDefect` adds `if (record.runId && record.runId !==
   expectation.runId) return 'run'` — compare-when-present, so records written before
   this field behave exactly as before and no checkpoint migration is needed.

### `engine/lib/engine.js`

4. **F2 — a cooperative return does not destroy the wait.** `_releaseHumanWait` lost
   its `handlerReturned` argument: how the handler left is not evidence, only whether
   the run was interrupted is. It now keeps the record whenever
   `ctx.aborted || ctx.status !== Running` and **returns whether it kept it**. In
   `executeStep`'s custom-handler arm, if the record was kept the step returns
   immediately after the `finally` (`ctx.currentStepID = step.id; _emitCheckpoint;
   return`), mirroring the Loop arm's existing interrupt check — so no `step_done`, no
   `completedSteps` entry, no `next`, no join, and the frame stays `started` with its
   handler nesting.
5. **F3 — a validated re-entry is not re-gated.** New predicate `_humanWaitReentry(ctx,
   step)`: true only when this `executeFrom`'s validator produced a verdict for the
   step **and** the record is still present. `executeStep` computes it once and uses it
   to skip (a) the `step.if` re-evaluation and (b) the result-cache hit. Both are
   *entry* gates that decided whether the step runs when it first started; re-deciding
   them ends the step without reaching the handler. The cache is still **written**
   after the re-entered handler succeeds. The breakpoint gate is untouched (§4).
6. **F3 — the verdict dies with the record.** `_releaseHumanWait` also calls
   `this._humanWaitVerdicts.delete(stepID)` when it drops the record, so a spent
   one-shot permission can never authorize a *new* request at the same step id (the
   request digest is over `{tool, stepId, contract}` and is identical for the same
   question, so the `HUMAN_WAIT_LIVE` guard would otherwise be bypassed).
7. **F5 — bind-time identities.** `bindHumanWait` passes `runId: ctx.workflowID` when
   creating a record, and on the re-entry branch asserts
   `sameHumanWaitOwner(existing.owner, this._recoveryOwner(ctx))` next to the existing
   request-digest check (`HUMAN_WAIT_SPEC`). `_planHumanWait` fills
   `runId: ctx.workflowID` into the expectation. `sameHumanWaitOwner` added to the
   existing import list. The descriptor is unchanged: `describeHumanWait` still reports
   the **resuming** run's id, which is what Flow compares against its instance, so
   there is still exactly one identity authority.

### `engine/test/host-human-wait-final.js` (new) and `engine/scripts/run-all-tests.mjs`

13 cases, registered as the 21st suite. `test/host-human-wait.js` was **not** edited:
all 16 of its cases still pass unchanged, which is the main evidence that these fixes
do not move the contract they pinned.

### Documentation

`engine/docs/HUMAN-WAIT-RESULT.en.md` and `engine/docs/cold-recovery-host-contract.md`
updated: verdict table, taint classification, record lifecycle, the re-entry rule for
entry gates, `runId`, the new refusal details, and an explicit residual-trust note.

---

## 4. Host-visible contract changes (read this before integrating)

Three changes are visible to a host. Two are strictly additive; one can refuse a
verdict that used to be accepted.

1. **Breaking-ish, verdict side (F1).** A resolved verdict for an **already resolved**
   record must now include `payload`, digesting to `payloadDigest`. It was optional.
   A validator that sends only `{requestId, payloadDigest}` now gets
   `human_wait_refused` / `payload_missing` and the resume pauses
   `cold_resume_unresolved` instead of re-entering.
   *Effect on the shipped host: none.* `_validateDurableHumanWait` always sends
   `payload: cloneJson(answer, null)` when `task.status === 'resolved'`
   (`app-instance-manager.js:1736-1738`). The descriptor shape is unchanged, so no
   validator input has to be adapted. This is the only rule that got stricter.
2. **Additive, event side (F2/F3).** A run interrupted while a pure wait was open no
   longer emits `step_done` for that step, and the step no longer appears in
   `completedSteps`. A host that inferred "the wait finished" from `step_done` after an
   abort was inferring something false; a host that watches `humanWaits` in the
   checkpoint sees the record where it previously vanished. On a validated re-entry the
   host will no longer see `step_skipped` / `step_cached` for that step.
3. **Additive, checkpoint side (F5).** `checkpoint.humanWaits[stepID].runId` is a new
   string field. Old records without it are accepted unchanged. A checkpoint whose
   `workflowID` was rewritten (a fork) is now refused with
   `human_wait_invalid` / `detail: 'run'` before the validator is consulted.

Nothing here requires a new checkpoint `_version`, a new gating state, a token
replacement, or any issuer/quota machinery.

---

## 5. Reproductions (how each defect was provoked)

No checkpoint was edited to create a defect, except where explicitly marked as fault
injection.

**F2a / F2b — cooperative return.** A handler in the documented shape whose wait
promise resolves `null` on `ctx.signal` (F2a, abort, root/legacy path) or on an
explicit `ctx.pause()` (F2b, inside a fan-out) and then `return`s. Pre-fix: the record
was deleted, `step_done` was emitted, and — in the fan-out case — the branch's frame
moved on while the human request stayed open at the host. Post-fix: the record and the
`started` + `handler` cursor survive, the join stays open with only the sibling
completed, and a new Engine re-attaches the **original** token on the same run.

**F3a — `if:` skip.** Branch A binds the wait; the parallel sibling sets
`$cancelled = true`; the process dies. On resume the planner admits the record, the
validator accepts it — and then `Tool_Ask`'s `if: '=!$cancelled'` evaluated false, the
step was skipped, and the record was never released. The run completed carrying a
stale record; every later resume then paused `cold_resume_unresolved` /
`human_wait_orphan` — permanently unresumable — while the human request stayed open.

**F3b — result-cache hit.** `Tool_Ask` (`cacheKey: '=$k'`) is answered once under
`$k = 'k1'` inside a single-child fan-out; `$k` moves to `'k2'`; a second fan-out asks
the human a **second** question, which is still open when the process dies; the
parallel sibling has meanwhile set `$k` back to `'k1'`. On resume the recomputed key
hits the first pass's cached entry: pre-fix the step reported `step_cached` +
`step_done` and `$j` became `'first'` — the answer to a *different* question — while
the second request stayed open at the host and the record was orphaned exactly as in
F3a. Post-fix the handler re-attaches, the human answers, and `$j` is `'second'`.

**F3c — leftover verdict.** After a record is dropped, `engine._humanWaitVerdicts` is
empty, so a later arrival at the same step id cannot spend a stale permission.

**F4a / F4b — effectful engine events.** For each of `channel_recv`, `actor_done`,
`child_run_done`, `interact_resolved`, `segment_verification_receipt`, a handler that
declares `allowedHostEvents: ['tool_start','tool_done']` emits the event while the wait
is **pending** (F4a) and, separately, **after `resolveHumanWait` and before the handler
exits** (F4b). The state that carries the effect is captured from
`engine.activeCtx.checkpoint()` *after* the event — the bind-time checkpoint is written
before it and proves nothing, which is why an earlier draft fixture that inspected the
bind-time checkpoint could not distinguish fixed from unfixed. Pre-fix `tainted` stayed
`false` in both spans and the resume was accepted; post-fix both spans yield
`tainted:<type>` and the resume is refused before the validator is consulted.

**F4c — classification.** Unit-level sweep over the whole `RunEventType` table:
every engine type is clean iff it is in `HUMAN_WAIT_CLEAN_ENGINE_EVENTS` (or
`var_changed`); 17 named effectful engine types still taint even when the binding
declares them in `allowedHostEvents`; declared/undeclared **host** types keep their
old meaning exactly.

**F4d — no over-tainting.** A clean pure wait under a fan-out still shows
`tainted: false` and completes through a normal cold resume.

**F1.** A validator that answers from the descriptor
(`requestId: d.resolution.requestId, payloadDigest: d.resolution.payloadDigest`, no
payload) was accepted pre-fix and is refused post-fix with `payload_missing`, while the
store-reading validator still re-enters and maps the real answer.

**F5a.** The crash checkpoint's `workflowID` is changed to `wf_fork_1` — the "duplicate
run / sandbox replay" the review describes, at the level a host actually does it. The
token-keyed store would hand the same approval to the fork; the engine now refuses with
`human_wait_invalid` / `run` before asking the validator. A record with `runId` deleted
(a pre-field checkpoint) still re-attaches normally.

**F5b — fault injection, stated as such.** No natural path reaches `bindHumanWait` on a
branch the planner did not validate, so the record's `owner` is changed inside the
handler's `before` hook, i.e. after planning. The guard refuses the hand-over
(`HUMAN_WAIT_SPEC`) and the wait is never handed over; the legitimate shape still
re-attaches.

### The breakpoint variant of F3 is not reproducible — and was therefore not "fixed"

The review asks that a re-entry skipped by a breakpoint not strand the record. It
cannot be, for two independent structural reasons, both verified by running the engine:

1. `ChildExecutionContext` exposes no `breakpoints` getter (`lib/types.js:883-935`), so
   `ctx.breakpoints` is `undefined` on any parallel branch context and the gate at
   `lib/engine.js:1066` never fires there. A fresh run with
   `breakpoints: ['Tool_Ask']` on a fan-out branch enters the handler and emits no
   `breakpoint_hit`.
2. On the legacy (no-frame) path a record is admitted **only** for `currentStepID`
   (`_planLegacyHumanWaits`), and `executeFrom` one-shot-releases exactly that step
   (`ctx._breakpointReleased.add(startStepID)`). Resuming a wait checkpoint with
   `runParams.breakpoints = ['Tool_Ask']` re-attaches and completes; no
   `breakpoint_hit`. The same holds for a direct single-child branch at the root, which
   runs on the root context whose `currentStepID` *is* the wait step.

Fixing an unreachable path would have meant either bypassing a debugging gate that
never fires, or relaxing `humanWaitReentryDefect`'s `nested_steps` rule so the planner
admits `entered` frames — a fail-closed check I am not willing to weaken for a
hypothetical. Recorded as a known edge in `HUMAN-WAIT-RESULT.en.md` (unresolved edge
11) instead, with the exact condition that would make it live.

---

## 6. Test results

All suites are local Node builtins: no network, no credentials, no provider calls.

### New suite, before the source edits (baseline `engine/`, unmodified)

```
node test/host-human-wait-final.js
  ✗ F2a  ✗ F2b  ✓ F2c  ✗ F3a  ✗ F3b  ✗ F3c  ✗ F4a  ✗ F4b  ✗ F4c  ✓ F4d
  ✗ F1   ✗ F5a  ✗ F5b
  2 passed, 11 failed
```

The two pre-fix passes are the deliberate no-regression pins: `F2c` (a custom handler
holding **no** wait record must keep its old lifecycle) and `F4d` (the taint inversion
must not start refusing clean waits). They pass before and after, by design.

Representative pre-fix failures:

```
F2a  an interrupted wait keeps its record however the handler left
F3a  timeout waiting for re-entered the accepted wait     (the step was skipped instead)
F3b  timeout waiting for re-entered the accepted wait     (the cache answered instead)
F4a  channel_recv must taint a pending wait: expected true, got false
F4b  channel_recv must taint after resolve: expected true, got false
F1   paused unresolved: expected "paused", got "stopped"  (the echoing verdict was accepted)
F5a  the record names its run: expected "wf_1789285353466", got undefined
```

### New suite, after the source edits

```
node test/host-human-wait-final.js      13 passed, 0 failed
```

Re-run 5× consecutively together with `test/host-human-wait.js`: 13/13 and 16/16 every
time (these fixtures are timing-driven, so repetition was checked deliberately).

### Full suite

Baseline before any edit: **20 suites, 0 failed** (`test/host-human-wait.js` 16/16).

After the edits, with the new suite registered:

```
node scripts/run-all-tests.mjs
  ✓ test/run.js  ✓ test/host-sdk.js  ✓ test/test-editor.js  ✓ test/test-bundle.js
  ✓ test/iteration.js  ✓ test/runtime-graph-failed-step.js  ✓ test/score-grow.js
  ✓ test/meta-flow.js  ✓ test/channel.js  ✓ test/topology.js  ✓ test/retry-cache.js
  ✓ test/segment-contracts.js  ✓ test/trusted-dag-v2.js  ✓ test/breakpoint.js
  ✓ test/strict-contracts.js  ✓ test/cache-resume.js  ✓ test/cold-recovery.js
  ✓ test/recovery-validation.js  ✓ test/parallel-loop-joins.js
  ✓ test/host-human-wait.js  ✓ test/host-human-wait-final.js
Total: 21 suites, 0 failed, 16.7s
```

`test/host-human-wait.js` (16), `test/cold-recovery.js`, `test/recovery-validation.js`,
`test/cache-resume.js`, `test/retry-cache.js`, `test/breakpoint.js` and
`test/parallel-loop-joins.js` all pass unchanged — the Fable cold-recovery/human-wait
work, the strict pre-mutation recovery validation, cancellation, cached provenance and
parallel-loop join isolation are preserved.

---

## 7. Remaining limitations

1. **F5b is untriggered defence.** No natural execution path reaches `bindHumanWait`
   on an unvalidated branch; the guard is proven only by fault injection. If it ever
   fires in production it means a planner/executor divergence that itself needs
   investigation, not a host error.
2. **The breakpoint edge is documented, not closed** (§5). Two structural facts make it
   dead today; either changing (branch contexts gaining breakpoints, or the legacy path
   admitting a record for a non-`currentStepID` step) would revive it.
3. **`runId` is not a run identity proof.** It is written from `ctx.workflowID`; a fork
   that keeps the same `workflowID` is indistinguishable to the engine. The host store
   remains the authority (Flow compares `instance.state.workflow.runId` and requires
   exactly one matching task).
4. **F1 does not sandbox a hostile validator.** The validator is CODE registered by the
   host and can fabricate a payload. Requiring the content removes the *accidental*
   echo — a well-meaning validator reporting values the engine handed it — and nothing
   more. Removing `requestId`/`payloadDigest` from the descriptor, as the review
   suggested, would break the descriptor contract for a gain that malicious code
   ignores; it was deliberately not done. Documented as residual trust.
5. **Purity is still asserted by host code.** A native action that emits no event stays
   invisible to the taint rule; the validator's tool allowlist is the second check.
   Unchanged.
6. **`var_changed` is still neutral**, so variable writes before/during a wait are
   repeated on re-entry.
7. **Untracked contexts still cannot bind** (parallel-Loop branches, strict-segment
   candidate contexts): `HUMAN_WAIT_UNTRACKED`, unchanged.
8. **New engine event types default to tainting.** That is the intended direction of
   the F4 inversion, but it means adding a genuinely neutral run-level event type in
   future requires adding it to `HUMAN_WAIT_CLEAN_ENGINE_EVENTS`, or clean waits will
   start being refused on resume. `F4c` will flag the omission as an unclassified type.
9. **Two executors on one checkpoint** remain the host store's problem; the engine
   still only refuses a second attach within one `executeFrom`. Flow's
   `different_live_executor` check covers this on its side.
10. **Fixtures, not product acceptance.** These are engine fixtures driving `Engine`
    directly against an in-memory host store. No L05 claim and no release-readiness
    claim is made here; Flow's durable approval integration and its own tests are
    outside this snapshot and were not run.
