Repository navigation
fix(tools): report deeply nested event JSON in check_conventions as a usage error - #12979
Conversation
… usage error Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
rooklz
left a comment
There was a problem hiding this comment.
Approving. I ran this on macOS arm64 with Python 3.13 and 3.15, the two setups the body lists as not run, at head e6ddf01fa5.
Gates
- Based on
mainbb04a4f1ea.git diff --checkandgit merge-treeagainstmainare both clean. check_conventions --base upstream/mainpasses, both alone and with this PR's title and body.pr_overlap --branch: 1470 PRs probed, 2 share the file, both merge cleanly, so no overlap.- Relinker-only build (
-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON, Python 3.13): ctest passes 494/494. That includesconventions_body_bom, which is registered atCMakeLists.txt:392.
RED/GREEN: test_check_conventions_body_bom.py
| Python 3.13.13 | Python 3.15.1 | |
|---|---|---|
| head | 9/9 OK | 9/9 OK |
main checker + PR tests |
new test FAILS | new test FAILS |
Mutants (each one was run against the whole module)
- Catch
RecursionErrorand setevent = [], so the run reaches the "not a JSON object" path: KILLED (byassertNotIn("not a JSON object")). except RecursionError: sys.exit(1): KILLED.except RecursionError: sys.exit(2)with noerror: --eventmessage: KILLED.
Probe behaviour, measured outside the test
- 3.13:
json.loadsraisesRecursionError("maximum recursion depth exceeded while decoding…") at depth 17024, which is iteration 4 of 8. The checker exits 2 witherror: --event …in 0.04 s. - 3.15: the error is now the C-stack guard ("Stack overflow (used 16352 kB) while decoding…"). It is still a
RecursionError, so the handler catches it. It fires only at depth 136192, which is iteration 7 of 8. The checker exits 2 in 0.06 s. The result was the same withulimit -sset to 8176, 16384, 32768 and 65520.
Optional, non-blocking: on 3.15 the probe has only one doubling of headroom left before self.fail("JSON decoder accepted every bounded nesting probe"). If a later CPython raises its stack budget again, the test fails while the checker is still correct. Raising range(8) to e.g. range(12) gives more margin. The largest input that would build is about 4 MiB per string at 3.13's starting depth, and the test exits at the first depth that raises, so this costs nothing on current versions.
Scope check: this is the only json.loads in check_conventions.py. The --body read does no parsing, so it needs no change.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
BunnyFl
left a comment
There was a problem hiding this comment.
Looks right, and it fills in the newer-Python case you didn't run. The other except (OSError, ValueError) in main (the --body read) is a plain read_text with no JSON decoding, so --event is the only site that needs RecursionError.
Tested on Windows 11, Python 3.14.7, with head e6ddf01fa5 merged onto current main: tools/tests/test_check_conventions_body_bom.py passes 9 tests. With check_conventions.py put back to main's version, test_excessively_nested_event_is_a_usage_error fails, so the depth probe still finds a nesting the 3.14 decoder rejects.
What
check_conventions.py --event(andGITHUB_EVENT_PATHin CI) reads the event file withjson.loadsand reports an unreadable one aserror: --event ...with exit code 2, but only forOSErrorandValueError. A file nested deeper than the interpreter's recursion limit raisesRecursionError, which is neither, so the checker printed a traceback and exited with 1 instead of the usage error that a malformed or missing event file gets. This catchesRecursionErrorin the same handler, as the other tools do after #12266. The new test picks its nesting depth fromsys.getrecursionlimit()and checks that the decoder really fails on it, so it does not depend on the standard library's message text and cannot pass without exercising the fix on Python versions whose decoder reads deeper input.Tested
mainbb04a4f1ea(only theexceptclause differs) and passes with this change (heade6ddf01fa5); all 9 tests oftools/tests/test_check_conventions_body_bom.pypass.mainand passes with this change; 9 of 9 pass.check_conventions --base upstream/mainpasses.Overlap
Two open PRs change
tools/check_conventions.py: #12654 (also two tests) and #9966. I took the changed files of each of the 1432 open PRs fromgit diff --name-onlyagainst its merge base. Agit merge-treeof each of them with this branch is clean, and so is each of them alone againstmainbb04a4f1ea; they change other parts of the file. Re-checked when opening against the open PRs created since then: none of them changes these files.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)AI-assisted (Claude Code).
🤖 Generated with Claude Code