[Bram910]

What

Report JSON recursion failures through the performance-comparison tool's normal error path instead of an uncaught traceback. An excessively nested summary currently makes the CLI exit 1 without its usual file/line diagnostic. Wrap decoder RecursionError as the existing location-qualified ValueError, and handle the same exception if JSON report serialization fails. Errors exit 2 without partial report output.

No new nesting limit or interpreter-limit change; metadata the interpreter already supports is preserved. Timing calculations, comparison semantics and PR performance-label validation are unchanged. This is CLI error handling, not a title-performance change.

Tested

• Based on main 00198bfcdf; previous head b1db8bb5fd changed tests only relative to 4be5952b83. Current follow-up head 8ffac634b69dd595e391403ff1e47484a3c45680 adds JSON-report diagnostic context and the test cleanup described below. Python tooling only; no title, full-runtime, GPU or local macOS run.

• Latest review cleanup: a RecursionError specifically from JSON report serialization is wrapped as ValueError("JSON report: ...") with its cause retained. Existing exit 2 and no-partial-output behavior are unchanged. The metadata-preservation test now exercises only actual JSON round-tripping, with assertions inside the capture block; existing separate Markdown tests retain rendering coverage. The unused nesting-probe variable is replaced by _.

• Latest validation: Windows Python 3.13.5 comparison suite 23/23 and labeling consumer 17/17 pass. Registered Windows performance_compare CTest passes five runs (7.62-8.29 seconds each), under the unchanged 20-second timeout. Linux Python 3.10.12 and 3.11.0rc1 each pass the full 23-test suite five times.

• New diagnostic regression fails against previous head b1db8bb5fd for the missing JSON report: context. A real input with extra nested 2,997 array levels loads on Windows Python 3.13.5 but cannot serialize the enclosing report; the updated tool exits 2 with JSON report: and empty stdout. This independently reproduces the reviewer's natural encoder-only failure rather than generalizing the earlier Python 3.10 probe.

• Red/green against main's unchanged tool: the three rejecting methods produce four failed CLI subcases and three unhandled errors (two decoder cases and the injected encoder case). All four new methods pass with this fix.

• Windows x64, Python 3.13.5, MinGW-w64 GCC 14.1.0 UCRT, Release, ANYPS5_RELINKER_ONLY=ON, BUILD_TESTING=ON: cmake --build build-relinker --parallel 4 and ctest --test-dir build-relinker --output-on-failure --parallel 4; 240/240 CTest entries pass. Direct comparison suite: 23/23; its PR-labeling consumer: 17/17.

• Ubuntu 22.04 under WSL, Python 3.10.12: comparison suite 23/23 and PR-labeling suite 17/17. All four new methods also pass five consecutive repetitions. The installed alternate Python 3.11.0rc1 passes the full 23-test comparison suite. The registered Linux CTest entry also passes five consecutive runs in 3.85–4.09 seconds each, with its existing timeout.

• Review follow-up: replace the 100,000-level fixtures with cached, bounded probes of the actual decoder limit plus a small safety margin. Local fixtures use 1,192 levels on Linux Python 3.10 and 4,384 on Windows Python 3.13, whose C decoder accepts values beyond the Python recursion limit. All four array/object × baseline/candidate CLI cases remain, with Markdown and JSON output represented. Accepted nested-metadata entrypoint checks run in-process, reducing the new subprocess launches from ten to four. No timeout increase or test skips.

• The CLI still reports the correct input path and line 3 after blank lines, exits 2, writes no report, and leaves both inputs unchanged. API checks retain the original RecursionError as the diagnostic's cause. The updated tests still fail against unchanged main (four CLI failures and three API/injected-encoder errors).

• The report-serialization error path uses an injected RecursionError; no natural encoder-only failure was found in an additional Python 3.10 boundary probe. Supported nested metadata is round-tripped unchanged through a real JSON report, and Markdown still succeeds.

• python tools/check_conventions.py --base origin/main --head HEAD and git diff --check origin/main...HEAD pass. Open-PR searches found no duplicate; python tools/pr_overlap.py --branch fix/performance-summary-nesting reports no overlap. That scan ran on this identical patch before rebasing onto its fetched base 00198bfcdf.

• Hosted rerun at b1db8bb5fd: Ubuntu 244/244, Windows 241/241, macOS arm64 235/235 and macOS x86-64 235/235 all pass. The comparison entry takes 4.03, 4.78, 4.08 and 2.70 seconds respectively, under its unchanged 20-second timeout. ASan, eligibility, conventions and labels also pass. Full product builds, write-watch and register-context jobs are skipped, not passing product tests. Local full Windows CTest passes 240/240; its comparison entry takes 8.00 seconds under parallel-suite load.

Checklist

• Based on current main; no other open PR implements the same functions
• One topic per PR; follow-ups go in a new PR
• No comments in code except technical debt
• Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
• No notes, investigation .md files or images added to the repository (attach them to this PR)
• New third-party code is a submodule built from source
• Generic behaviour, not specific to one title
• Depends on: none
• Fixes: none
• Unblocks: none
• GPUs tested: n/a
• AI-assisted: yes, Codex

[MarcZX10]

The new nesting regressions are functionally useful, and the focused Python module passes locally (19/19). However, the full hosted relinker CTest run now times out its existing 20-second performance_compare test on Ubuntu and both macOS architectures; in each case this is the only failed CTest entry. The new CLI regression launches eight subprocess cases with 100,000 nested delimiters each, which makes this test too expensive under the parallel full-suite load. Please reduce the nesting to the smallest depth that still reliably raises RecursionError (for example, just above the interpreter recursion limit), retain both array/object and input-file coverage, and rerun the full relinker CTest matrix. Hosted eligibility, conventions, intake, labels, and ASan pass; Windows relinker CTest passes. I ran the focused Python module locally: 19 tests passed.

[MarcZX10]

Reviewed updated head b1db8bb. The nesting fixture now probes the decoder boundary once and caches a bounded value, while preserving array/object, baseline/candidate, Markdown/JSON coverage and reducing subprocess runs. I independently ran python3 -m unittest tools.tests.test_performance_compare -q on this exact head: 23/23 passed. The hosted Ubuntu, Windows, macOS ARM64/x86_64 relinker jobs and ASan all pass; eligibility, conventions, and labels pass. The prior hosted performance_compare timeouts are resolved; product build jobs remain skipped by eligibility. No blocking issue found.

[Bram910]

Thanks @MarcZX10 — addressed in b1db8bb5fd, and thanks for re-reviewing the update.

• Replaced the 100,000-level fixtures with cached, bounded probes of the interpreter's actual decoder limit plus a safety margin. Local depths are 1,192 on Linux Python 3.10 and 4,384 on Windows Python 3.13, whose C decoder accepts values beyond sys.getrecursionlimit().
• Retained arrays and objects in both input files, both output formats, file/line diagnostics, empty stdout and input preservation. Accepted nested-metadata entrypoint checks now run in-process; the new subprocess count drops from ten to four.
• Production code and the 20-second CTest timeout are unchanged. Revised negative controls still fail against main with four CLI failures and three decoder/injected-encoder errors.

The full relinker matrix rerun passes: Ubuntu 244/244, Windows 241/241, and both macOS architectures 235/235 each. performance_compare passes in 4.03 s, 4.78 s, 4.08 s (arm64) and 2.70 s (x86-64), respectively. ASan, conventions, eligibility and labels also pass; full product builds remain skipped.

Local full Windows CTest passes 240/240, and the registered Linux comparison test passes five consecutive runs in 3.85–4.09 s. The PR description now records these follow-up results.

[THEMANnt]

Reviewed head b1db8bb5fdbe901091f403ac9cb14e71612d46d2 against pinned base 00198bfcdfc4ca42f5d7519281786758dfdc688a. No actionable findings in this focused review.

In tools/performance_compare.py, load_summary now wraps decoder RecursionError in the existing path/line-qualified ValueError, preserving its cause. main also handles report-serialization recursion before writing stdout, producing exit 2 without a partial report. The tests exercise real excessive array/object nesting in either input, line numbering after blank lines, Markdown/JSON errors, and unchanged input bytes. Supported nested metadata still round-trips through JSON. Existing assertions remain intact and fixtures are isolated; the encoder-error case uses an injected exception.

The controller's local execution of tools/tests/test_performance_compare.py used Windows AMD64 and Python 3.11.9: baseline passed 19 test methods; pristine PR and candidate each passed 23. Candidate tests against baseline production code produced four CLI assertion failures and three errors across subcases, confirming the old traceback/exit-1 behavior. The recorded local whitespace and repository-conventions checks also passed.

GitHub-reported effective checks passed: conventions, labels, eligibility, relinker-asan, and relinker jobs for windows-latest/g++, ubuntu-latest/g++, and both listed macos-latest/clang++ variants. Superseded labels/conventions executions were excluded using matching workflow identities. windows, linux, write-watch, and register-context were skipped, providing no passing product-test evidence. Broader author-reported build/interpreter results were not independently reproduced, and this review does not validate game or GPU behavior. The evidence supports approval.

[roboterspinne]

Reviewed head b1db8bb merged onto upstream/main befe264 (2026-10-24); the merge is clean, check_conventions.py passes and pr_overlap.py reports no overlap with open pull requests.

Environment: Windows 11 Home 10.0.26200, Python 3.13.9 (Anaconda); Python only, no build needed.

Results

• python -m unittest tools.tests.test_performance_compare on the merged tree: 23 tests pass in 8.2 s. Red/green: against main's performance_compare.py the location test errors in both openings, the CLI test fails in all four cases and the report-recursion test errors (seven subcases in all), while test_supported_nested_metadata_is_preserved passes on main too; all pass on the branch.
• The probe is deterministic and cheap here: starting at the recursion limit plus 64 and doubling, it settles at 4384 levels on 3.12 and 3.13 (the decoder gives up at 2998 and 2999) and at 1192 on 3.11 (994), about 1 ms per probe, so the 20 s ctest budget the first head exceeded is no longer at risk.
• The encoder handler is reachable with a real input, not only through the injected test: a summary whose extra is nested 2997 levels deep loads and renders as Markdown on 3.13.9, but --json overflows in json.dumps, because the report wraps the record in two more containers. main dies with a traceback and exit 1, this branch exits 2 without one. So the body's "no natural encoder-only failure was found" holds for the 3.10 probe it names, not in general; the handler is justified either way, and the test's injected exception enters at the same site with the same type.
• load_summary (line 73) is the only untrusted decode in this tool; cell() only dumps validated scalars.

Nits

• performance_compare.py:184: for that natural --json overflow the exit-2 line is just performance_compare.py: maximum recursion depth exceeded ..., naming neither the input nor the report step, while the decoder path says before.jsonl:3:. Wrapping the dump (raise ValueError(f"JSON report: {error}") from error, or a prefix in the handler) would make it actionable.
• test_performance_compare.py:244: the assertions of test_supported_nested_metadata_is_preserved sit outside the with self.subTest(...) block, so a failure is reported without its args label and ends the loop; and the Markdown leg never looks at the nested metadata, since markdown() does not render extra. The test also passes on main, and any depth cap above the fixture's 33 levels would pass it too, so it guards against an absurdly low cap rather than this fix. Moving the assertions into the block and keeping the depth check on the JSON leg would make it say what its name says.
• test_performance_compare.py:35: attempt is never used; for _ in range(8):, as in check_conventions.py:123.

Notes, nothing to change here: the outer except now turns any RecursionError from compare() or markdown(), including one from a future programming bug, into the same terse exit 2 without a traceback; that is what the tuple already does for ValueError and OverflowError, and pr_labels.py:61 catches the pair the same way. trace_digest.py decodes the same aps5-summary.jsonl and trace JSON (lines 93, 100, 287) and its entry-point tuple at line 386 has no RecursionError, so the same input still gives a traceback there; a separate PR. Four CLI runs instead of eight lose no coverage, since decoding fails before the output format matters.

Not checked: Linux and macOS runs; Python 3.14, whose C recursion check is stack-based rather than counted.

I found no blocking issue: the two handlers sit exactly at the two places where deep nesting can raise, the counter-checks hold, the fixture is bounded and fast, and supported nesting is preserved. For disclosure: I have open PRs (#10667 on tools/tests/test_suggest_patches.py, and relinker PRs #10679, #10658, #10646, #10607, #10596, #10564, #10403, #10402, #10401, #10391, #10390); none touches this tool.

Written with AI assistance (Claude Code). I ran the tests above on my Windows 11 machine.

[roboterspinne]

Nit: these assertions sit outside the with self.subTest(...) block, so a failure is reported without its args label and ends the loop; and the Markdown leg never looks at the nested metadata, since markdown() does not render extra. The test also passes on main, and any depth cap above the fixture's 33 levels would pass it too. Moving the assertions into the block and keeping the depth check on the JSON leg would make it say what its name says.

[roboterspinne]

Nit: attempt is never used; for _ in range(8):, as in check_conventions.py:123.

[roboterspinne]

Nit: a summary whose extra is nested 2997 levels deep loads and renders as Markdown on 3.13.9 here, but --json overflows in this dumps (the report adds two container levels). The new handler turns that into exit 2, good, but the line is just performance_compare.py: maximum recursion depth exceeded ..., naming neither the input nor the report step, while the decoder path says before.jsonl:3:. raise ValueError(f"JSON report: {error}") from error around the dump, or a prefix in the handler, would make it actionable.

[Bram910]

Addressed all three nits in 8ffac634b69dd595e391403ff1e47484a3c45680:

• Wrap recursion specifically at JSON report serialization as ValueError(f"JSON report: {error}") from error, so the existing exit-2 handler names the failing stage. Decoder path/line diagnostics are unchanged. The revised injected regression requires the stage prefix and fails against the previous PR head.
• The supported-metadata test now runs only the JSON round-trip it actually inspects, and both assertions live inside the output-capture block. Removed the unnecessary format/subtest loop; separate existing Markdown tests retain format coverage. This remains positive coverage of supported metadata, not a claim to distinguish main by itself.
• Replace unused attempt with _ in the bounded decoder probe.

I also reproduced your natural encoder-only case on Windows Python 3.13.5: a summary with extra nested 2,997 array levels loads, then the enclosing JSON report overflows. It now exits 2 with JSON report: and empty stdout. The description distinguishes this from the earlier Python 3.10 probe that did not find such an input.

Validation: Windows comparison suite 23/23 and labeling consumer 17/17 pass; registered Windows comparison CTest passes five consecutive runs (7.62-8.29 seconds, unchanged 20-second timeout). On Linux Python 3.10.12 and 3.11.0rc1, the full 23-test suite passes five runs per interpreter. Conventions and whitespace checks pass. Existing completed matrix results in the description are explicitly associated with the previous head; fresh CI is needed for this follow-up.

[MarcZX10]

Re-reviewed exact head 8ffac634b69dd595e391403ff1e47484a3c45680. The change now reports decoder recursion with the input location and wraps report-serialization recursion as a clean exit-2 CLI error; tests cover both paths, preserve input files, and retain supported nested metadata. Local test_performance_compare passes (19 tests), and diff-check and conventions pass. The previously failing Linux/macOS relinker checks now pass along with Windows and ASan.

[roboterspinne]

@Bram910 Thanks, checked 8ffac63 merged onto upstream/main 2dc8560 (2026-10-24), Windows 11, Python 3.13.9: tools.tests.test_performance_compare 23 tests pass. With the previous head's performance_compare.py under the new tests exactly test_json_report_recursion_produces_only_a_cli_error fails (the JSON report: prefix), and against main the four CLI cases fail and the location and report tests error (seven subcases), as before. The natural case behaves as you describe here too: extra nested 2996 levels deep passes both formats (the --json report is 18 MB), 2997 renders as Markdown and exits 2 with performance_compare.py: JSON report: maximum recursion depth exceeded while encoding a JSON object for --json, 2998 exits 2 with the before.jsonl:3: decoder location in both formats, no traceback anywhere. All three nits are done from my side.

Written with AI assistance (Claude Code).