Repository navigation
fix(tools): report deeply nested JSON as invalid input and run the reserved-name archive test - #12266
Conversation
read_report passes the file to json.loads, and a report nested a few hundred thousand levels deep raised RecursionError, which main does not catch. The command ended in a traceback with exit code 1, the code the tool documents as 'different outputs', so a corrupt file looked like a hardware difference. The error is now a ValueError naming the file, and main reports it with exit code 2 like every other invalid report.
…rt_audit read_registry and read_needed_registry catch OSError, UnicodeDecodeError and JSONDecodeError, but a registry nested a few hundred thousand levels deep raises RecursionError. main returns 2 for an AuditError and 1 for missing imports, so the traceback ended with exit code 1 and a corrupt registry looked like a failed import audit. RecursionError is now caught with the other read errors and reported as an AuditError, so the exit code is 2.
tools/test_web.py launched archive-root, archive-windows and archive.test but not archive-reserved.test.mjs, which covers the superscript Windows device names (COM1 with a superscript digit, LPT2 and so on) added with 3397e87. No workflow, script or CTest entry referenced it, so a regression in validPath for those names went unnoticed. It runs next to the other archive tests now.
…ser allows The command has to exit with 2 and say the file is nested too deeply, without a traceback, instead of exiting with the code that means different outputs.
… JSON parser allows A registry and a needed-libraries file of 200000 opening brackets have to end in an audit error with exit code 2 and the read-error message, not a traceback.
|
verdict: OK
Freshness envelope: head 70cd9c9; merge-base 0cd07dc; behind main: 0; git merge-tree --write-tree origin/main pull/12266: exit 0, clean. The conventions checker and git diff --check pass. Cross-PR ledger: open non-draft PR #7454 also changes tools/import_audit.py and tools/tests/test_import_audit.py. Pairwise git merge-tree reports conflicts in both shared files, so the two current heads do not combine cleanly. Draft PR #6171 was excluded. |
|
A Windows run, since the description lists none. Head
I didn't run AI-assisted: yes (Claude Code). |
|
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.
|
|
@MarcZX10 the two RecursionError fixes look right to me, and their tests cover both readers. One small thing in |
rooklz
left a comment
rooklz
left a comment
There was a problem hiding this comment.
Review of 70cd9c91c2, macOS arm64, run locally on this machine. The head merges cleanly onto main 3a40945e62.
JSON depth (does the fixture trigger on every supported Python?) json.loads("[" * n):
| Python | n=20000 | n=200000 |
|---|---|---|
| 3.9.6 | RecursionError | RecursionError |
| 3.13.1 | RecursionError | RecursionError |
| 3.14.8 | JSONDecodeError | RecursionError ("Stack overflow") |
| 3.15.1 | JSONDecodeError | RecursionError ("Stack overflow") |
At 20000 the input is caught on 3.14+ already. 200000 hits the uncaught path on all four versions, so the fixture depth is the right choice.
Python fixes
- With the head checked out,
test_compare_results(12 tests) andtest_import_audit(157 tests, 1 skipped) pass on 3.9.6, 3.13 and 3.14. - With only the two scripts set back to
main(3.9.6), each module fails exactly one test: the new one. - Mutants, all killed: dropping
RecursionErrorfromread_registryonly, dropping it fromread_needed_registryonly, and changing thenested too deeplymessage. The second half of the import_audit test does its job. - The three
json.loadscall sites in these two files are all covered now.
archive-reserved.test.mjs wiring (the body says this was not run, so I ran it). build_web.copy_sources copies demo/web as-is, so I put a scratch <build>/web copy together and fetched the pinned zip.js:
- Head:
node demo/web/tests/archive-reserved.test.mjs <build>/webexits 0 on Node 26.4.0. - When I set the scratch
archive.mjsback to the regex from before 3397e87 (com[1-9]|lpt[1-9]), it fails withAssertionError: Missing expected exception., so the newly wired test does catch the regression it was written for. - The whole
tools/test_web.py --build <scratch>run stops later, atarchive.test.mjs, because it needs the realnative/relinker_wasm.mjs. That is expected without a wasm build and is not part of this change. - Non-blocking:
web-tests.ymltriggers ontools/test_web.pybut never runs it; it only runsarchive-rootplusnode --testfor 4 files.test_web.pyruns only inrelease.yml, so the reserved-name test will run at release time, not on PRs. Adding a step toweb-tests.ymlthe wayarchive-rootis wired would catch it per PR. That could be a follow-up.
Gates (merged tree) check_conventions --base upstream/main passes. Relinker-only build 270/270, ctest 378/378. pr_overlap reports only #6171 and #7454 as not comparable (both conflicting) on the import_audit files; this diff adds lines to the except tuples and one test, so I don't see a topical overlap.
Approving.
AI-assisted: yes
THEMANnt
left a comment
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 70cd9c91c2afdb3348e644b83772346c7058acfc — approve. Deeply nested import registries are reported as invalid input instead of escaping as RecursionError. The Windows reserved-name archive case is enabled as part of this patch.
Windows/Python 3.11: head test_import_audit.py: 154 passed, 3 skipped (157 total); base test_import_audit.py: 153 passed, 3 skipped (156 total). The new nested-registry case fails against old production with the reproduced RecursionError. This control establishes the parsing fix; it does not imply that every platform-only archive case executed. Full command/output evidence is retained locally. Hosted CI and mergeability are rechecked before publication. No game or full product execution is claimed.
What
Three defects in repository tooling. One commit each; the two Python fixes have a test commit each:
tools/hw-oracle/compare_results.py:read_reportpasses the file tojson.loads, and a report nested a few hundred thousand levels deep raisesRecursionError, whichmaindoes not catch. The command ended in a traceback with exit code 1, the code the tool documents as "different outputs", so a corrupt file looked like a hardware difference. It now raises aValueErrornaming the file, andmainreports it with exit code 2 like every other invalid report.tools/import_audit.py:read_registryandread_needed_registrycatchOSError,UnicodeDecodeErrorandJSONDecodeErrorbut notRecursionError.mainreturns 2 for anAuditErrorand 1 for missing imports, so a deeply nested registry ended in a traceback with exit code 1 and looked like a failed import audit.RecursionErroris now reported as anAuditError(exit code 2).tools/test_web.pylaunchedarchive-root,archive-windowsandarchive.testbut notdemo/web/tests/archive-reserved.test.mjs, which covers the superscript Windows device names added with 3397e87. No workflow, script or CTest entry referenced it, so a regression invalidPathfor those names went unnoticed. It runs next to the other archive tests now. I did not run it (it needs a web build and the zip library download).Tested
python3 -m unittest test_compare_resultsintools/hw-oracle(12 tests) andpython3 -m unittest test_import_auditintools/tests(157 tests, 1 skipped) pass. Each of the two new tests fails againstmain's version of its script.tools/check_conventions.py --base upstream/mainandgit diff --checkpass.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)