fix(scenario): route the evaluator's report emits through write-report.js - #9
Merged
Merged
Conversation
…t.js scenario.js was the last emitter still handing its report to process.stdout.write() directly. Every other CLI adopted the guarantee in 0da6f20; this one could not ride along, because scripts/scenario.js is the evaluator file and the separation guard (EVALUATOR_EXACT) forbids mixing it with behavior changes in one PR. Both branches are merged now, so the block is gone. One helper, `out()`, backs all three emit sites -- out(scenario) in record, out(bundle) in replay, out(result) in selftest -- so a single line closes it. The path that can actually lose bytes today is selftest: `out(result)` is followed immediately by `process.exit(1)` when the run has errors, and process.exit() discards whatever libuv still had queued. The exit code survives, so this never fabricated a pass -- but the report explaining WHY the evaluator failed is exactly what got truncated. The replay and record paths return instead of exiting, so Node drains them; they are routed anyway because the guarantee belongs to the emitter, not to the caller's control flow, and a later refactor that adds an exit must not silently reintroduce the defect. Measured on this platform (Linux, node 22): file 673B, spawnSync pipe 673B, identical. NO loss is reproducible here at this size, and that is stated rather than dressed up -- the report is far below the 64KB Linux pipe buffer. The defect is not size-bounded (it produced a completely empty report in CI, meaning libuv's first try_write moved zero bytes) and macOS's effective pipe buffer is ~8KB, so a small report is not a safe report. This change is preventative on Linux and load-bearing elsewhere. scaffold.js copies only watchdog.js into generated projects, so unlike that file scenario.js can safely `require` the helper rather than inline it. Verified: scenario.js --selftest exit 0; gate.js --selftest exit 0; tests/scenario/run-tests.js green; tests/report-integrity 22 PASS / 0 FAIL with its positive control firing (110615 of 402967 bytes lost through spawnSync, so the suite's green result is meaningful on this platform, not inert); graphlint --selftest, graphlint scripts/, docs-lint all exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARvYLenCrqcDcr8VyefoxG
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Routes
scripts/scenario.js's report emits throughscripts/write-report.js, closing thelast instance of the report-loss class fixed across every other CLI in
0da6f20.Why this is a separate PR
scripts/scenario.jsis the evaluator file. The separation guard (EVALUATOR_EXACT) rejectsa PR that mixes it with behaviour changes, so this cannot ride along with the cleanup branch.
Guard verdict on this diff: pass, 1 file, no mixing.
What it fixes
One helper,
out(), backs all three emit sites —out(scenario)in record,out(bundle)inreplay,
out(result)in selftest — so a single line closes them.The path that can actually lose bytes today is selftest:
out(result)is followed immediatelyby
process.exit(1)when the run has errors, andprocess.exit()discards whatever libuvstill had queued. The exit code survives, so this never fabricated a pass — but the report
explaining why the evaluator failed is exactly what got truncated.
The replay and record paths
returninstead of exiting, so Node drains them. They are routedanyway, because the guarantee belongs to the emitter rather than to the caller's control flow,
and a later refactor that adds an exit must not silently reintroduce the defect.
Honest scope of the measurement
Measured on Linux / node 22: file 673B, spawnSync pipe 673B — identical. No loss is
reproducible on that platform at this size, and that is stated rather than dressed up: the
report is far below the 64KB Linux pipe buffer.
The change still matters. The defect is not size-bounded — it produced a completely empty
report in CI, meaning libuv's first
try_writemoved zero bytes — and macOS's effective pipebuffer is roughly 8KB. Preventative on Linux, load-bearing elsewhere.
A correction to an earlier write-up
An earlier note flagged line 216 as a second emit site. That was wrong. It sits inside
generateManager()'s template string — a log line in a generated fixture manager, not ascenario.js report emit. scenario.js derives verdicts from on-disk state, never from output
strings, so losing those lines cannot fabricate a verdict. It is also a generated standalone
file, so a
requirethere would hit the same MODULE_NOT_FOUND trap that keepswatchdog.jsinlined. Deliberately left alone.
scaffold.jscopies onlywatchdog.jsinto generated projects, so scenario.js can safelyrequirethe helper.Verification
scenario.js --selftestexit 0;gate.js --selftestexit 0 (gate consumes scenario output)tests/scenario/run-tests.jsgreentests/report-integrity22 PASS / 0 FAIL, with its positive control firing (110,615 of402,967 bytes lost through spawnSync), so the suite's green result is meaningful on this
platform rather than inert
graphlint --selftest,graphlint scripts/,docs-lintall exit 0Not included
The timing-fragility work tracked in
claude/graphsmith-ci-timing-fragility-decision.mdisunrelated to this change and lives on the other branch. This PR contains no timing-dependent
code.