Repository navigation
Conversation
Birfy
left a comment
There was a problem hiding this comment.
Review at 9ffb51c — no Reviewer score; this body is the Lead's verification
I asked the Reviewer twice for a 0–100 score on this head and it did not return one before the job deadline. I am not going to let the final head sit unreviewed or put a number on it that nobody gave, so this body records what was actually checked, by whom, and states plainly that no Reviewer score exists for this SHA.
Verified on 9ffb51c (CPython 3.12.16, PYTHONPATH=src), by the Lead:
pytest -q -p no:cacheprovider tests→ 109 passed (102 on base, 7 new); the new file → 7 passed; on unmodified34b3d2fthe same file → 5 failed, 2 passed, the 2 being the declared pins (shallow entry unchanged; the report keeps the real value). On #17's head0ce9324it is also 5 failed, 2 passed, with the 129-deep test red on behaviour (assert marker in outwhile the full value prints), so this PR is not repaired by #17.- 1,000-deep
input,return 2vsoutput: 1→ rc 1, 18 stdout lines, 0 stderr, 0Traceback,level: REJECTED, one marker; on34b3d2fthe same case is 7 stdout + 37 stderr with oneTraceback. 900-deep control → 18 lines. 1,000 vs 10,000 through the printer → 13 lines each, byte-identical. - Report untouched: after
_print_details,report.gate("examples").detailstill carries the real 1,000-deep input, walked to the leaf; the printed copy is a new dict per entry. Boundary per field: 128 renders, 129 elides. Non-dict details and non-dict entries pass through unchanged; a 1,000-deep list elides like a dict; theexcept RecursionErrorwraps only the dump and prints one line. jitagent selftest6/6;tools/audit_traps.py18/18; CI run418359464874/4 (package, 3.11, 3.12, 3.13).
Known coverage gap, stated rather than hidden: the four 1,000-deep cases are skipped on 3.11, where the C json scanner/encoder recurse so a 1,000-deep case file cannot be loaded at all; the 129-deep test still runs there.
Security reviewed the same design independently on this branch: PASS-with-notes, zero material findings — no new crash class, the belt wraps only the dump and cannot move a verdict, boundaries correct per field (128/129, dict and list), seven shallow detail shapes byte-identical to the old printer, and the report still holds the identical 1,000-deep object after printing. Notes it raised, none blocking: DEEP_FIELDS omits value (the key return_schema violations use, unreachable today and degraded to one line by the belt); the _dump invariant is guarded by level, not by failures; and _deeper_than's "no more than limit steps" holds for deep values, while a wide shallow value is walked whole (783 ms / +61 MB on a 1M-key value that was already parsed and sandboxed).
No blocking findings from me. Two things I would want a Reviewer to look at if it gets to it: the except RecursionError is a belt for shapes the depth check misses, and cli.cmd_call (cli.py:254) has the same unbounded-render shape and is out of scope here.
Fledge · jitagent · Lead · reviewed
Fledge automation · jitagent · job
|
A gate's detail keeps the caller's own values, and a failing examples gate retains a 1000-deep input. _print_details then ran json.dumps(indent=2) — the recursive Python encoder — so verify printed 'level: REJECTED' and then a RecursionError traceback; the [:1800] slice came after dumps and could not bound it. _printable now copies the detail and replaces a retained entry value (input/expected/actual) that nests deeper than MAX_RENDER_DEPTH with one ELIDED line, so the rest of the entry still prints and output stays proportional to the entry. The walk is iterative (recursion is the thing being avoided), the data is untouched (the report still carries the real value), and a narrow except RecursionError keeps an unanticipated shape from crashing the printer. Fixes #18
…boundary The first CI run of this file failed on 3.11 only: case_dir wrote a 1000-deep case.json with json.dumps, and on 3.11 that takes the recursive Python encoder, so the fixture raised RecursionError before the code under test ran. depth 1000 is now written by splicing a literal (deep_text), which changes only the writer. Adds the primary red through main(): a 129-deep input authored with a plain json.dumps (under every supported version's cliff) must print one marker line on the fixed tree and the whole value on the unfixed one. No product code: the fixture and the cap's boundary only.
3.11 cannot even load a 1000-deep case file (json decoding recurses in that interpreter), so the reported reproduction cannot be set up there with or without this fix. The skip states that reason and leaves the parser alone; the 129-deep test carries the contract on every supported version.
…guess The skip said 3.11's json 'takes the pure-python paths'; that is a claim about CPython internals the log does not support. What the log shows is the frame: Case.load's json.loads raising RecursionError at json/decoder.py:353 before the printer exists. Wording only, no test behaviour changes.
…r recurse decoder.py:353 is raw_decode's call to scan_once, which is _json.make_scanner; a RecursionError raised inside the C scanner surfaces with that as its last Python frame. 3.12 replaced both the C scanner and the C encoder with non-recursive implementations, which is why the writer died on 3.11 and not on 3.12/3.13. Conclusion unchanged: a 1000-deep case cannot be set up on 3.11, so the four deep cases are skipped there and the 129-deep test carries the contract on every version.
Birfy
left a comment
There was a problem hiding this comment.
Re-review at 9ffb51c — 92/100 (head unchanged since 5769068865)
Same head 9ffb51c1296677b905b773ae91371c161be25ac0; no commits since that review, so the 92 stands. Re-verified today: pytest -q → 109 passed; CI 4/4 at this SHA (run 41835946487), mergeable_state: clean, base still 34b3d2f.
Fresh printer probe today: a failure entry whose input is 128 deep renders in full (40 lines, no marker); at 129, 1000 and 10 000 deep it renders 13 lines with the marker, byte-identical at 1000 vs 10 000 (287 bytes). On 34b3d2f the same 1000-deep entry raises RecursionError, and 129 prints the whole value — so the bound is behavioural, not an absent constant. The original detail is unmutated after printing (entry input still 129 deep).
Open, unchanged and non-blocking: tests/test_detail_rendering.py:166 is named for a deep expected, but case_dir (:93, :102) puts the depth in input and keeps output: 1, so expected/actual are never deep; the :259 boundary test varies only input; the belt at cli.py:83-84 has no test and _printable only understands detail["failures"] (the return_schema value at verify.py:136 would fall through to it); 3.11 was not run locally (3.12.16 only here) — the CI logs show that skip covers a pre-existing parser limit, not a regression.
92/100. COMMENT — not approved, not merged, no code changed.
Fledge · jitagent · Reviewer · reviewed
Birfy
left a comment
There was a problem hiding this comment.
Re-review at 9ffb51c — 92/100
Head unchanged since the last review; re-ran everything at this SHA just now.
Verified by running (CPython 3.12.16, PYTHONPATH=src)
pytest -q→ 109 passed;selftest6/6;audit_traps18/18; CI run41835946487success;mergeable_state: clean; base34b3d2f.- Printer probe: a failure entry whose input is 128 deep renders in full (40 lines, no marker); at 129, 1000 and 10 000 deep it renders 13 lines with the marker, byte-identical at 1000 vs 10 000 (287 bytes both). The original
detailis unmutated after printing (the 129-deep input is still 129 deep).
Non-blocking
tests/test_detail_rendering.py:166is named for a deep expected, butcase_dir(:93,:102) puts the depth ininputand keepsoutput: 1, soexpected/actualare never deep; the boundary test (:259) varies onlyinputalthough the PR body says "input and output alike".- The belt at
cli.py:84has no test, and_printableonly understandsdetail["failures"]— thereturn_schemavalue atverify.py:136would fall through to it. - 3.11 was not run locally (this machine has only 3.12.16); the CI logs show the skip covers a pre-existing parser limit, not a regression.
92/100. COMMENT — not approved, not merged.
Fledge · jitagent · Reviewer · reviewed
Birfy
left a comment
There was a problem hiding this comment.
Re-review at 9ffb51c — 92/100
Head unchanged since review 5785360719 at the same SHA; re-ran everything at this head just now.
Verified by running (CPython 3.12.16, PYTHONPATH=src)
pytest -q→ 109 passed;selftest6/6;audit_traps18/18; CI 4/4 success;mergeable_state: clean; base34b3d2f.- Printer probe: a 128-deep failure entry renders in full (40 lines, no marker); at 129, 1000 and 10 000 deep it renders 13 lines with the marker, byte-identical at 1000 vs 10 000 (287 bytes). The original
detailis unmutated after printing.
Non-blocking (unchanged)
tests/test_detail_rendering.py:166is named for a deep expected, butcase_dir(:93,:102) puts the depth ininputand keepsoutput: 1; the boundary test (:259) varies onlyinput.- The belt at
cli.py:84has no test and_printableonly understandsdetail["failures"]; thereturn_schemavalue (verify.py:136) would fall through to it. - 3.11 was not run locally (3.12.16 only); the CI logs show the skip covers a pre-existing parser limit.
92/100. COMMENT — not approved, not merged.
Fledge · jitagent · Reviewer · reviewed
Fixes #18.
The defect
jitagent verifycomputed the verdict and printed it, then died while printing the failure detail. A case dir with a 1,000-deepinput,output: 1andreturn 2:44 lines total (7 stdout / 37 stderr), exactly one
Traceback, rc 1. The verdict was already correct and already printed.Cause
cli._print_detailsrenders a failing gate's detail withjson.dumps(..., indent=2), which recurses once per nesting level.verify._failureskeeps the whole value in the entry it returns —verify.py:67-68on a failed run (input,expected),verify.py:70-71on a mismatch (input,actual) — so the value is retained and rendered, never compared, and nothing upstream bounds it. Case dirs buildExample(**e)directly (cases.py:31), outside the parse cap, and the[:1800]slice sits afterdumps, so it can never bound the encoder.The fix
The bound goes where the printing happens, and only there (
src/jitagent/cli.py, +60/−2):cli.MAX_RENDER_DEPTH = 128,cli.ELIDED = "<nests deeper than 128 levels — not shown>"(f-string over the constant),cli.DEEP_FIELDS = ("input", "expected", "actual").cli._deeper_than(value, limit)— iterative, explicit stack, returns as soon as the limit is passed. Recursion is the thing being avoided, so walking it recursively would be self-defeating.cli._printable(detail)— a copy of the detail dict in which each failure entry'sinput/expected/actualbecomesELIDEDwhen deeper than the cap;i,origin,errorand anything added later stay exactly as they are. Each entry is a new dict, so nothing the report holds is mutated._print_detailsdumps that copy and keeps today's indentation and[:1800]width slice, which is safe again: a too-deep value is now one short line instead of ~17,000 characters, so the slice cannot cut the marker off.except RecursionErroraround the dump prints one line,<detail too deep to render>, instead of a traceback — a belt for a shape the check misses, never a silent pass.The verdict and the exit code do not move.
Evidence (measured on this branch, CPython 3.12,
PYTHONPATH=src)The code under test is unchanged since
b6cc9d9; the test logic since2baae15;9ffb51cchanges one skip-reason string.34b3d2f)071c2be)input,return 2vsoutput: 1TracebackTraceback,level: REJECTED, marker 1input(control)inputvia ajson.dumps-authored case dircase.jsonby splicing the deep literal (deep_text) instead of encoding it, because 3.11's writer recurses at that depth; nothing else about them changes. The end-to-end path is not fixture-only: the 129-deep test — the contract test, and the one that runs on every supported version — writes its case dir with a plainjson.dumpsand drives it throughmain(), and the 900-deep control above was run by hand on this head.i,origin,error,expected,actualall print; only the too-deep field becomes the marker.report.gate("examples").detailstill carries the real 1,000-deep value after_print_detailsruns (walked to the leaf, not string-compared); the printed copy is a new dict at every level.MAX_RENDER_DEPTHrenders in full, one level past it becomesELIDED, oninputandoutputalike.0ce9324) the same file is also 5 failed, 2 passed, with the 129-deep test red on behaviour (assert marker in outon a full value) — this PR is not repaired by Refuse an example value deeper than 128 levels instead of recursing #17, and no test here goes green because Refuse an example value deeper than 128 levels instead of recursing #17 nullsexpected/actual.pytest -q -p no:cacheprovider tests→ 109 passed (102 on34b3d2f, 7 new). The same new file on unmodified34b3d2f: 5 failed, 2 passed — the 2 are declared pins (shallow entry unchanged,the report keeps the value). Of the 5 red, four fail on behaviour (traceback present, or the whole value printed with no marker);test_the_marker_appears_only_past_the_render_capfails because the constants it references do not exist on base — it pins the boundary, it is not proof of the defect.jitagent selftest→ 6/6;tools/audit_traps.py→ 18/18.41833662563: package success, 3.12 success, 3.13 success, 3.11 failed.The 3.11 story
On 3.11 the first CI run of this branch failed inside the fixture writer:
case_dir's plainjson.dumpsof a 1,000-deep value raisedRecursionErrorbefore the code under test ran (run41833662563).507c16fstopped authoring the fixture that way (deep_textsplices the literal; writer only,src/untouched), and the next 3.11 job then failed earlier still, inCase.load'sjson.loads:RecursionErroratjson/decoder.py:353(run41834451731). That line israw_decode's call toscan_once, which is the C scanner — 3.11's C scanner and C encoder are recursive, and 3.12 replaced both with non-recursive implementations, which is why 3.12/3.13 passed throughout. So on 3.11 a 1,000-deep case file cannot even be set up, with or without this fix, and the value never reaches the printer there: a parser limit, not a printer one.9ffb51cstates exactly that in the skip reason for the four 1,000-deep cases; the 129-deep case carries the contract on every supported version. No product code is skipped and none changes.CI run
41835946487on9ffb51c: 4/4 success — package, 3.11, 3.12, 3.13.Why a render cap of 128
It is the same number the comparison work uses for example values (#17), so the tool has one depth story instead of two. It is a rendering limit only: nothing is refused, stored or compared differently, and a case that prints the marker still gets the verdict and exit code it got before.
Not in this PR
cli.cmd_calldumps a returned result withjson.dumps(indent=2)(cli.py:254) — the same unbounded-render shape. Not reproduced here: reaching it needs a stored handle plus a function that builds a ~1,000-deep result, and Security found only a 1–3-level band where the child's_jsonableand the parent's indented dump disagree (992 works, 993 dies in the child, 996 in the parent on this build), so it is build dependent. The argument path has its own pre-existing limits, untouched by this diff: an inline argument ≥ 256 bytes dies atcli.py:304(Path.exists()→ENAMETOOLONG) and a 10,000-deep argument dies in_json.loadsatcli.py:306. Out of scope for the printer fix; it needs a different shape (a result is not a gate detail).registry._dumpwrites reports withindent=2(registry.py:180), but onlyVERIFIEDreports are stored (CACHEABLE,registry.py:42/281) and a passing verify builds{"failures": []}(verify.py:126with an emptybad), so it cannot hold this entry. Safe by invariant, not by guard; left alone deliberately so nobody "fixes"jitagent verifytracebacks while printing a failure entry that retains a 1000-level value #18 by starting to store rejected reports.prompts.pyserialises the sameinput/expected/actualinto the repair prompt (noindent) on the compile path, where examples come throughExample.from_dictand the parse cap.Fledge · jitagent · Lead · implemented