stormtest: fix stale params starving bind_event/logs_read; log every abort reason (#634)#639
Conversation
Two op generators sent parameter names the tools no longer accept, so
those paths always errored and got zero real stress coverage:
- bind_event sent event_type=key with `key="A"`; the op requires
`keycode`. 26/26 bind_event calls failed MISSING_REQUIRED_PARAM in a
heavy run.
- logs_read drew source from a list containing "combined", which is not
a valid source (plugin|game|editor|all). ~35% of logs_read calls
failed ToolError, and the `all` path was never exercised.
Separately, several early-exit failure paths set STOP[0] silently, so a
truncated run gave no reason — a heavy run ended at 6115/~9000 calls with
the summary pointing at "the log above", which said nothing. Route every
genuine abort through a new _abort(reason) helper that prints the cause
and sets an ABORTED flag distinct from STOP (which the normal end-of-run
teardown and a user Ctrl-C also set). The report and JSON snapshot now
distinguish a clean finish from an early abort and name the reason.
Closes #634
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds centralized early-abort tracking in ChangesStormtest harness updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Related issues: Suggested labels: bug, testing, script Suggested reviewers: maintainers familiar with 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/stormtest.py`:
- Line 965: The verdict logic in the editor status check is using M["ok"], which
can cause aborted runs to still report the editor as alive after an earlier
successful tool call. Update the verdict calculation in the stormtest flow so it
keys off ABORTED[0] or the final liveness probe result instead of any prior
success, and keep the existing "EDITOR ALIVE"/"EDITOR DEAD/UNREACHABLE" symbols
consistent with the surrounding status handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
CodeRabbit review: the verdict used `not ABORTED[0] or M["ok"]`, so any run with at least one earlier successful call reported "EDITOR ALIVE" even if the editor was dead at the end — masking a real death behind prior success. Key the verdict off STOP[0] as it stands at report() time instead. In concurrent mode main()'s final liveness probe sets STOP[0]=False when the editor answers editor_state (True when unreachable); in isolated mode STOP[0] stays False unless a path aborted. A mid-run abort the editor recovered from now correctly reads ALIVE (it is alive at report time), with the separate ABORTED line explaining the truncation. Verified across all four (STOP, ABORTED, ok) combinations by driving report() directly: prior-success + dead-final-probe now reports DEAD/UNREACHABLE (previously ALIVE). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed in e2fd055: the verdict now keys off the end-of-run liveness probe ( |
There was a problem hiding this comment.
Pull request overview
This PR updates the script/stormtest.py stress-test harness to (1) stop generating invalid/stale parameters that were causing input_map_manage.bind_event and logs_read calls to fail (and therefore not provide real coverage), and (2) ensure any early termination is explicitly logged with a clear abort reason and an aborted flag in the JSON snapshot.
Changes:
- Fix stormtest-generated params to match current tool expectations (
bind_eventnow useskeycode;logs_readnow uses a validsource, includingall). - Add a centralized
_abort(reason)helper andABORTEDflag so every genuine early failure logs a reason and the report/JSON can distinguish “aborted early” from normal teardown / SIGINT. - Include
abortedin the persisted report JSON and adjust the final verdict/report messaging accordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes #634. Two classes of problem in the stormtest harness, both found while reconciling error buckets during a live smoke.
1. Stale params starved two ops of coverage
op_input_mapsent{"event_type": "key", "key": "A"}forbind_event, but the op requireskeycode. 26/26 bind_event calls failedMISSING_REQUIRED_PARAMin a heavy run — zero real coverage.op_logsdrewsourcefrom["plugin", "editor", "combined"];"combined"is not a valid source (plugin|game|editor|all). ~50/148 logs_read calls failedToolError, and theallpath was never exercised.Both fixed (
keycode,all). Verified live:bind_eventandlogs_readnow report 0 errors.2. Silent aborts
Several early-exit paths set
STOP[0]with no reason logged, so a truncated run gave no cause — a heavy run ended at 6115/~9000 calls with the summary literally saying "see log above for why" while the log said nothing. Every genuine abort now routes through a new_abort(reason)helper that prints the cause and sets a distinctABORTEDflag.STOPalone is also set by the normal end-of-run teardown and by a user Ctrl-C (SIGINT), so the report/JSON now distinguish:aborted: false, no "aborted early" lineaborted: true,!!! stormtest aborting: <reason>printed, reason named in the summaryNote on parallel work
A separate local session (spun off as a task chip during the smoke) began the two param-name edits but stopped without committing — its 2-line change sat uncommitted in a worktree. This PR includes those same two fixes plus the abort-logging (item 2 of #634), so it supersedes that unpushed work as the complete fix.
Testing
python -m py_compile+ruff check: clean.input_map_manage.bind_event1/0,logs_read9/0,final verdict EDITOR ALIVE, JSONstopped:false aborted:false.🤖 Generated with Claude Code
Summary by CodeRabbit
abortedstatus for easier result tracking.