fix(scenario): a certified halt-on-uncertainty is a PASS for a crash-resume scenario - #8
Conversation
…resume scenario tests/scenario check 5c was flaky, and the flakiness was the evaluator marking the component wrong for being right. A crash-resume scenario kills the manager the instant the target step's checkpoint lands. Where that instant falls is scheduling-dependent: it can land outside any side-effect window (resume runs to completion) or inside a concurrent fan-out sibling's window, leaving an intent recorded with no completion. In the second case the resumed run correctly HALTS with UNRESOLVED SIDE EFFECT -- it refuses to guess about external state instead of silently re-sending -- and the evaluator scored that as workflow_fault because expected.completed_steps was not reached. Reproduced under 2-core load: fanout-crash-resume, alternating sides, same commit. This also put the evaluator's own determinism claim at risk: checks 1a and 6b require the same seed to produce a byte-identical bundle, and pass/cause_code were depending on CPU scheduling. A halt is now an accepted outcome for a crash-resume scenario, and completed_steps is inapplicable to a run that deliberately stopped -- but ONLY when the on-disk state justifies the halt: a real unresolved intent in intents.log, and no duplicated effect in effects.log (the halt came INSTEAD of a re-send, not after one). This is the same PD-3 certification scripts/chaos.js applies, so a run cannot earn a pass by printing the magic string. Every other check still has to pass: the invariants, and no pre-crash step re-executed. Four selftest cases pin the rule, including the two gaming attempts. readRunState() also stops treating every *.json in the run directory as a completed-step checkpoint. Runtime state that happens to be JSON (budget-state, a watchdog capability snapshot) is not a step, and the resume check would demand a '"status":"skipped"' line for it -- the same mis-detection that broke chaos.js. Callers now pass the scenario's declared step ids. Verified on Linux: tests/scenario 20 PASS / 0 FAIL (was 19/1); scenario --selftest determinism_pass true; 6 consecutive full-corpus replays under 2-core contention all clean AND byte-identical bundle hashes. Green on Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARvYLenCrqcDcr8VyefoxG
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 575527b213
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const halt = certifyHaltOnUncertainty(state, result2.stdout + result2.stderr); | ||
| if (halt.halted) { |
There was a problem hiding this comment.
In the tree-directory replay path, a worker can append an unmatched intent, print UNRESOLVED SIDE EFFECT, and call process.exit(0) before producing the remaining checkpoints; this branch treats that successful early exit as a certified halt and returns pass: true, bypassing the expected-step check below. I reproduced this with a two-step crash-resume scenario whose second worker did exactly that, and both candidate and baseline were scored ok. Gate this alternative on result2.code !== 0 (as chaos.js does) so merely emitting the marker and exiting successfully cannot earn a safety pass.
Useful? React with 👍 / 👎.
| const cnt = {}; | ||
| for (const e of state.effects) cnt[e] = (cnt[e] || 0) + 1; | ||
| const dupes = Object.entries(cnt).filter(([, n]) => n > 1); |
There was a problem hiding this comment.
When an effect ID is __proto__—which is permitted by the scenario step-name pattern—assigning it on this ordinary object does not create an own numeric counter, so Object.entries(cnt) reports no duplicate even if effects.log contains that ID multiple times. A crash-resume run with duplicated __proto__ effects and another unresolved intent can therefore be certified and scored as a safe halt despite the explicit no-duplicate requirement. Use a Map or Object.create(null) for this counter (and the corresponding invariant counter) so all schema-valid IDs are counted.
Useful? React with 👍 / 👎.
| return { | ||
| pass: violations.length === 0, | ||
| cause_code: violations.length > 0 ? "workflow_fault" : "ok", |
There was a problem hiding this comment.
A scenario can legally contain fixture.crash_after_step while declaring expected.outcome as complete, halt, or budget-exceeded, because neither the published schema nor validateScenario() conditionally ties those fields together. If its restart genuinely halts with certified unresolved state, this unconditional pass ignores the already-computed outcome.pass and marks the scenario green even though the declared expected outcome was not observed. Only apply the alternate safety-pass semantics when the scenario explicitly expects crash-recovered (and/or is declared failure_mode: "crash-resume").
Useful? React with 👍 / 👎.
1 participant
One file,
scripts/scenario.js, +83/-5, no test changes. Cherry-picked fromfix/watchdog-deadman-resumeas commite50f284.What it does
Adds
certifyHaltOnUncertainty()and filtersreadRunState()to declared step ids, so that a correct halt-on-uncertainty is scored as a PASS for a crash-resume scenario instead of a failure. Certification requires a real unresolved intent and no duplicate effects — it does not accept a halt on faith.Why it is a separate PR
ci-check-pr-separation.jsrejects any PR that mixes evaluator/corpus files with behavior files (contracts/11, contracts/04 B17).scripts/scenario.jsis inEVALUATOR_EXACT: it is the evaluator that scores candidates, so changing it alongside the behavior it scores is exactly the conflict of interest that guard exists to prevent. There is no exemption mechanism in the guard, deliberately, and none was added.Verified on this branch:
pass (1 changed file, no mixing).Read this before looking at the red CI
This PR's CI is red because
mainis red, not because of this change.mainatb1df2f8fails all six Phase-A legs,verify (ubuntu-latest, 18), and the publication-hygiene job. Anything branched off it inherits that.So the change was measured directly instead. Full suite battery on Linux / node 18, against two trees differing only by this commit:
main— 11 failing gating suitesmain+ this commit — the same 11, minus one, plus oneAnd the runner's own re-run classifier resolved the newcomer:
The failing case was
concurrency.two-process-register-deregister+errors a=2 b=0— a contention-timing case, in a suitescenario.jscannot influence, onmain's unfixed copy of it. Commit08ee6bfon the behavior branch fixes that class.Net: this takes
tests/scenariofrom red to green onmainand introduces nothing. Its remaining CI failures aremain's own, pre-existing and untouched.Independently confirmed on Windows at
main+ this commit:scenario.js --selftestexit 0,tests/scenario20 PASS / 0 FAIL, plus graphlint,gate.js --selftestandpromote.js --selftestall clean.What happens after this merges
fix/watchdog-deadman-resumerebases onto the newmain.scripts/scenario.jsleaves that PR's diff, the separation guard goes green legitimately rather than by override, and every behavior fix on it is present so the suites follow.Known follow-up, not included here
scripts/scenario.jshas two stdout report-emit sites that still useprocess.stdout.write(...)followed byprocess.exit(), which can lose a queued report on a POSIX pipe (measured: 8190 bytes of an 11886-byte report on real macOS, exit code 0). Every other affected script was fixed on the behavior branch in0da6f20;scenario.jswas deliberately left out so this PR stays a single-purpose evaluator change. It should land as its own follow-up once both branches are in.