Repository navigation
Conversation
Birfy
left a comment
There was a problem hiding this comment.
Posted by jitagent Lead on jitagent Reviewer's behalf.
Coverage of this review. The score below is Reviewer's, and it is a score for da74e52 — the first commit of this PR, which Reviewer rebuilt from the tarball and ran itself. The head when this was posted is 0ce9324: the same change plus 2 files, +82/−1 (src/jitagent/registry.py +12/−1, tests/test_example_depth.py +71) — the store-side depth filter and its two tests, added after Security showed that a deep input could be stored and then never read back. The Lead read that delta, ran it on 0ce9324 (full suite 116 passed; new file 14 passed; 12 failed / 2 passed on 34b3d2f; selftest 6/6; audit_traps 18/18; CI run 41750477458 4/4 success) and confirmed the filter only ever removes a case, never rewrites one. There is no Reviewer score for 0ce9324; this review states that gap rather than inventing a number for it.
Two PR-body corrections follow from this review: the "a deep case dir still has to end in a verdict" sentence is narrowed to values at or below the cap, with the ≥1000-level printing path moved to #18; and the body now says the read-path test asserts list/get/call/inspect while search was checked by hand.
Review at da74e52 — 92/100
Fixes #16. Scope matches the description: 6 files, +446/−9, base 34b3d2f, mergeable clean; CI run 41749320555 4/4 success (package + 3.11/3.12/3.13). The fix is the two layers claimed, and I reproduced the claims on the head rather than taking them from the PR body.
Verified on the head (CPython 3.12.16, PYTHONPATH=src)
pytest -q→ 114 passed;tests/test_example_depth.py→ 12 passed;selftest6/6;tools/audit_traps.py18/18.- Red claim: copied the new test file onto unmodified
34b3d2f→ 10 failed / 2 passed; the two are the declared pins (test_deep_equal_still_compares_a_hundred_and_twenty_eight_levels,test_a_hundred_and_twenty_eight_deep_example_still_verifies). - 600-deep case dir:
0/1 passed,REJECTED, failureerror= UNCOMPARABLE withexpected/actualnull, noTraceback/RecursionError, rc 1. - Cap: 128 accepted, 129 refused with
examples[0].output nests deeper than 128 levels — deeper than jitagent can compare; input is checked first;deep_equalTrue on equal, False on a changed leaf and on a depth change; 128 still verifies. compileof a 129-deep requirement with a nonexistent--home: rc 1, exactly one stderr line, home still absent (noclaudebinary here, so any model attempt would have raised; none happened).- Pre-cap registry with a 600-deep
tests.json:list,get,call,search,inspecteach rc 1 with one line; file byte-identical afterwards; no traceback.listalso refuses a 2000-deep stored example the same way. - No unguarded
deep_equal: onlyverify.py:71andlookup.py:219, both pre-checked;runtime.calldoes not compare examples;compile --cache autogoes through the guardedreverify, and caller examples are loader-capped.
One non-blocking finding (pre-existing, but the PR body promised otherwise)
verify.py:67 still keeps the full input/expected in a failure entry when the run itself failed; cli.py:27 then calls json.dumps(..., indent=2), which uses the pure-Python encoder (~1 frame per level) and raises at 1000. Repro: a case dir with a 1000-deep input, output 1000. Suggested: cap before retaining the values (store a marker instead), or dump details without 1, impl returning 2 (the run succeeds; this is just a mismatch) → jitagent verify prints 0/1 passed and then a RecursionError traceback; 900 is clean. Same with a raising impl and a 1000-deep expected. The mismatch branch (verify.py:85) also retains ex.input. So "deep values are deliberately left out … rendering them later would walk into the same recursion" holds only for the UNCOMPARABLE branches, and "a deep case dir still has to end in a verdict rather than a crash" fails at ≥indent.
Secondary, more exotic: propose.py:184 builds Example(...) directly, so generated examples bypass from_dict; verify's comparison guard covers them, but I verified the store side directly — an Example(origin="generated") with a 2000-deep input and output 1 verifies (only the output is compared) and then reg.put raises RecursionError in Registry._dump (registry.py:181, indent=2). propose.py:184 is how such an example would arrive. Pre-existing and unlikely, but the "single parse point" only covers the file and tests.json callers.
Minor: the read-path test covers list/get/call/inspect but not search, although the body names it (I ran it manually: one line, rc 1); and the except RecursionError belt phrases a stack-depth failure as "nests deeper than 128", which can overstate the cause.
Verdict
92/100. The reported defect is fixed at the claimed depth, the cap is enforced at the parse point, the comparison guards are in the right places, and the tests are unusually good (exact messages, red on main, pins that forbid "refuse everything", the registry-unchanged assertion). Nothing here blocks merging the primary fix; the ≥1000 retained-value rendering and the generated-example store path are follow-up material, not regressions. I did not approve, merge, push or modify code.
Fledge · jitagent · Reviewer · reviewed
Fledge automation · jitagent · job
|
`types.deep_equal` is plain recursion and nothing checked nesting before it: a case whose expected output nested 600 dicts printed a ~3,000-line RecursionError out of `verify._failures`, and `lookup.reverify` carried the same unguarded call on every L2 candidate. - `types`: `MAX_VALUE_DEPTH = 128`, the `UNCOMPARABLE` message, `ExampleTooDeep`, an iterative `nesting_depth`, and `Example.from_dict` refusing a deep `input`/`output` while naming `examples[i].field`. One parse point, reached by a requirement file and by a stored test set. - `verify._failures` / `lookup.reverify`: depth pre-check on both values, then a narrow `except RecursionError` belt around that one call; the deep values are left out of the failure entry so nothing downstream renders them. - `cli`: one-line refusal from `cmd_compile` before the client starts, and from `main` for read paths on a registry written before the cap. - `tests/test_example_depth.py`: 11 tests / 12 items, red on main (8 failed, 2 deliberate pins). Fixes #16
verify compares outputs only, so a generated example with a deep *input* can still pass every gate and reach `Registry.put` — where it was written without a depth check. The loader then refuses it, which makes the whole home unreadable: `list`, `get` and `search` all exit 1 on one stored case. `TestSet.add` now drops such an example (returns False like any case it refuses) and `Registry.put` filters the list before `save_tests`. Nothing is migrated: the read path stays strict, so a home written before this commit still refuses in one line.
Birfy
left a comment
There was a problem hiding this comment.
Review at 0ce9324 — 93/100
Supersedes 5761716868 (92/100, which was a score for da74e52 and said plainly that no Reviewer score existed for this head). This is the Reviewer's verdict on 0ce9324, re-run today; the delta it covers is the store-side depth filter, registry.py +12/−1 plus tests/test_example_depth.py +71.
Delta since da74e52 is exactly the store-side fix; PR total 6 files, +528/−10, base 34b3d2f, mergeable_state: clean; CI run 41750477458 4/4 (package + 3.11/3.12/3.13).
Verified on this head (CPython 3.12.16, PYTHONPATH=src):
pytest -q→ 116 passed; the depth test file → 14 passed; on unmodified34b3d2fthe same file → 12 failed / 2 passed (the 2 are the declared pins);selftest6/6;audit_traps18/18.TestSet.addaccepts 128 and refuses 129, per field.Registry.putwith a[shallow, deep]batch stores only the shallow one; the home stays readable andlistreturns 0. Onda74e52the same new test file is 2 failed / 12 passed, so the two new tests really pin this delta.- 600-deep case dir on this head:
0/1 passed,REJECTED, failureerror=the value nests deeper than 128 levels — too deep to compare,expected/actualnull, rc 1, noTraceback/RecursionError(re-ran today, not taken from the PR body). - The 1000-deep retained-value traceback is unchanged (
cli.py:27→json.dumps(indent=2)), now tracked as #18. The body correction — scoping the "deep case dir ends in a verdict" sentence to the 128 cap and that ~1000-level rendering limit, and naminglist/get/call/inspectas what the read-path test covers (searchis hand-checked: rc 1, one line) — matches what I found.
93/100: +1 for closing the store hole I flagged last pass. A deep input is not compared by verify, so it could be written and then poison every read of the home; the delta stops it at both TestSet.add and Registry.put. The ≥1000 rendering residual remains (pre-existing, filed as follow-up), so not higher.
One test nit (non-blocking): test_the_store_refuses_an_example_it_would_not_read_back offers a deep-only batch and asserts saved == [], so it does not show a shallow example surviving in the same put. I checked [shallow, deep] by hand (one stored, list rc 0); adding that would pin the behavior against a "drop the whole batch" regression. Also TestSet.add's docstring still says "True means it was new" although False now also means "too deep" — one-line fix.
No blocking findings. The delta is acceptable; I would not refuse it on this head.
Fledge · jitagent · Reviewer · reviewed
Birfy
left a comment
There was a problem hiding this comment.
Re-review at 0ce9324 — 93/100 (head unchanged since 5768464689)
Same head 0ce9324faef77660ae890fc4996af713df5b5508; no commits since that review, so the 93 stands. Re-verified today: pytest -q → 116 passed; CI 4/4 (run 41750477458), mergeable_state: clean, base 34b3d2f.
Fresh store probe today: TestSet.add accepts a value at nesting depth 128 and refuses 129 (False, set unchanged, shallow entry still added afterwards); Registry.put with a [shallow, deep(129)] batch stores only the shallow example, and the home directory stays readable. That is the hole this delta was meant to close — a deep input is never compared by verify, so a stored one would poison every read.
Open, unchanged, non-blocking: test_the_store_refuses_an_example_it_would_not_read_back offers a deep-only batch and asserts saved == [], so it does not pin a shallow example surviving in the same put (checked by hand: one stored); TestSet.add's docstring still says "True means it was new" although False now also means "too deep". The ≥1000-level rendering residual is tracked as #18.
93/100. COMMENT — not approved, not merged.
Fledge · jitagent · Reviewer · reviewed
Birfy
left a comment
There was a problem hiding this comment.
Re-review at 0ce9324 — 93/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→ 116 passed;selftest6/6;audit_traps18/18; CI run41750477458success;mergeable_state: clean; base34b3d2f.- Store probe:
TestSet.addaccepts depth 128 (True), refuses 129 (False, set unchanged) and a later shallow add still succeeds;Registry.putwith a[shallow, deep(129)]batch stores only the shallow example, and the home stays readable afterwards.
Non-blocking
test_the_store_refuses_an_example_it_would_not_read_backoffers a deep-only batch and assertssaved == [], so it does not pin a shallow example surviving in the sameput(checked by hand: one stored).TestSet.add's docstring still says "True means it was new" althoughFalsenow also means "too deep".- The ≥1000-level rendering residual is tracked as #18.
93/100. COMMENT — not approved, not merged.
Fledge · jitagent · Reviewer · reviewed
Birfy
left a comment
There was a problem hiding this comment.
Re-review at 0ce9324 — 93/100
Head unchanged since review 5785360429 at the same SHA; re-ran everything at this head just now.
Verified by running (CPython 3.12.16, PYTHONPATH=src)
pytest -q→ 116 passed;selftest6/6;audit_traps18/18; CI 4/4 success;mergeable_state: clean; base34b3d2f.- Store probe:
TestSet.addaccepts depth 128, refuses 129 with the set unchanged, and a later shallow add still succeeds;Registry.putwith[shallow, deep(129)]stores only the shallow example and the home stays readable.
Non-blocking (unchanged)
test_the_store_refuses_an_example_it_would_not_read_backassertssaved == []for a deep-only batch, so it does not pin a shallow example surviving in the sameput(checked by hand: one stored).TestSet.add's docstring still says "True means it was new" althoughFalsenow also means "too deep".- The ≥1000-level rendering residual is tracked as #18.
93/100. COMMENT — not approved, not merged.
Fledge · jitagent · Reviewer · reviewed
Fixes #16.
The defect
A case whose expected value nests a few hundred dicts deep did not get a verdict:
jitagent verify <case-dir>printed a ~3,000-lineRecursionErrortraceback and exited 1. The comparison that decides VERIFIED from REJECTED istypes.deep_equal, plain recursion at two frames per dict level — measured on CPython 3.12 (recursionlimit1000): 499 levels compares, 500 raises. Nothing bounded nesting before it, and the same unguarded call sat inlookup.reverify, which runs on every L2 candidate.The fix, two layers
Loader — one cap, one place.
Example.from_dict(types.py:52) now refuses aninput/outputdeeper thanMAX_VALUE_DEPTH = 128, naming what it refused:That is the single parse point both callers go through: a requirement file (
cli.py:80) and a stored test set (registry.py:100,tests.json).cmd_compileprints the line and exits 1 before the client is built or anything is written — the registry directory is never even created.Comparison — bounded, not crashing.
verify._failures(verify.py:69) andlookup.reverify(lookup.py:218) first walk the nesting depth iteratively (types.nesting_depth, an explicit stack, linear in nodes). Past 128 the verdict is0/1 passedwiththe value nests deeper than 128 levels — too deep to compareanddeep_equalis not called at all. A narrowexcept RecursionErroraround that one call remains as the belt for a shape the check misses; the twoUNCOMPARABLEbranches replace the values withnullin the failure entry, because rendering them later would walk into the same recursion. The branches that retaininput/expected(verify.py:67,verify.py:85) are unchanged — see #18.This second layer matters because
verifyis a public API and the corpus case dirs (cases.py) constructExample(...)directly — the loader cap deliberately does not cover them. Precisely what is fixed: an example at or below the cap (128) always ends in a verdict, and any value the comparison sees is bounded. A value the corpus path hands to the reporter at ~1000 levels still overflowscli.py:27 _print_details→json.dumps(indent=2)after the verdict is computed; that is a different door (printing, not comparing), it is pre-existing on34b3d2f, and it is #18, not this PR.Read paths stay one line. A
tests.jsonwritten before this cap can hold one of these.cli.maincatches the same typed error, solist,get,call,searchandinspectanswerexamples[0].output nests deeper than 128 …and exit 1 instead of tracebacking. Nothing is migrated and nothing is skipped silently. (The test assertslist,get,callandinspecton such a home;searchwas checked the same way by hand — rc 1, one line.)The honest consequence: such an entry is not openable through the CLI until its stored example is regenerated or its directory is removed by hand. The repair is
rm -rf $JITAGENT_HOME/registry/<hash>or editing thattests.jsondown to the examples you want — there is no delete command today. That only bites a registry written before this change, and no released version has ever written one. The one-line refusal names the example and the field but not the function whose registry it is.The store cannot hold what the loader refuses. The cap on load alone left a real asymmetry, reachable without editing any file:
verifycompares outputs only, so a model-generated example with a deep input passed every gate, went intoRegistry.put(jit.py:154) and was written — after whichlist,getandsearchall exited 1 on that whole home.TestSet.add(registry.py:87) now drops such an example andRegistry.putfilters the list beforesave_tests. Dropping, not raising: a generated case is a candidate, and failing a compile that already succeeded over one unusable case would trade one one-line failure for another. Nothing is migrated — the read path stays strict, so a home that already holds one still refuses in one line rather than quietly losing the case.Two consequences of dropping rather than refusing, both verified: a compile whose cases are all too deep stores a
VERIFIEDfunction with an empty case set (listshowsCASES 0) — readable and callable, just no examples; and the storedreport.jsonis the verdict computed before the filter, so it can still say2/2 passedwhiletests.jsonholds 1 case. The filter only ever removes cases, never rewrites one, so that pair can only disagree in the "fewer cases" direction.Evidence
Run on the head of this branch, CPython 3.12,
PYTHONPATH=src:pytest -q -p no:cacheprovider→ 116 passed (102 before this change, 14 new).pytest tests/test_example_depth.py→ 14 passed in 0.62s.main(34b3d2f) the same file is 12 failed, 2 passed — the 2 are deliberate pins (see below).jitagent selftest→ 6/6;python tools/audit_traps.py→ 18/18.Traceback,FAIL examples 0/1 passed, entry{"expected": null, "actual": null, "error": "the value nests deeper than 128 levels — too deep to compare"}.level: VERIFIED; 129 → refused by the loader.--home <tmp> --via api→ rc 1, stdout empty, stderr exactly one line, and the home directory is not created.tests.jsonwith a 600-deep example →list,get,call,inspecteach rc 1, one line, no traceback.Registry.putwith a 200-deep input example → the savedtests.jsonholds 0 examples andlist/get/searchare rc 0. On the previous head (da74e52) the sameputsaved it and then all three exited 1 withexamples[0].input nests deeper than 128 …; onmainit also saved it, and all three stayed readable.lookup.reverifywith a 600-deep expected value →False,verdict: reverify,example 0 cannot be compared: ….deep_equalmonkeypatched to raiseRecursionError, both sites still produce that same one-line answer.41750477458on0ce9324— 4/4 success:test(3.11),test(3.12),test(3.13),package.Two tests are pins, not coverage, and say so in their docstrings:
deep_equalstill returns a correctTrue/Falseat 128 levels, and a 128-deep example still verifies. Both pass onmaintoo; they exist so a later "tighten the cap until the red goes away" change cannot pass this suite.Why 128, and what it costs
The cliff is 499 on this interpreter, so 128 is ~4× below it, which leaves room for a caller already deep in the stack and for shapes that cost more frames per level. The cost is real: a 400-deep caller example that verifies today will be refused after this change. That is deliberate —
deep_equalcan compare 400, but a comparison whose success depends on the stack is exactly how this defect got here. The number is one constant; change it and the message follows.Not in this PR
param_schemais still not run throughcheck_schemaat parse — that is Move a compiled function between machines as one bundle #15's follow-up list, not this defect.referencingis not declared inpyproject.toml; also separate.Registry._loadis a follow-up, not a head change, and after the store-side check the only way to hit it is hand-editing atests.json.nesting_depthassumes an acyclic value, which every JSON path guarantees. A hand-built cyclic Python object passed straight to the Python API is not a shape this change supports (it is unrepresentable in JSON, anddeep_equalnever accepted it either).propose.py:184buildsExample(...)directly, so a generated example never passes the parse cap; the store-side filter keeps one out oftests.json, but makingfrom_dictthe only parse point is a separate change.Conflict with #15
#15 also edits
verify.py(_schema_ok) and addstests/test_bundle.py. This PR touches_failuresin the same file and does not touch_schema_ok, so the textual overlap is expected to be nil; whichever lands second needs a trivial rebase, not a rewrite.Fledge · jitagent · Lead · implemented