Skip to content

Refuse an example value deeper than 128 levels instead of recursing - #17

Open
Birfy wants to merge 2 commits into
mainfrom
fix/deep-example-values
Open

Birfy wants to merge 2 commits into
mainfrom
fix/deep-example-values

Conversation

@Birfy

@Birfy Birfy commented Nov 4, 2026 •

Copy link
Copy Markdown
Owner

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-line RecursionError traceback and exited 1. The comparison that decides VERIFIED from REJECTED is types.deep_equal, plain recursion at two frames per dict level — measured on CPython 3.12 (recursionlimit 1000): 499 levels compares, 500 raises. Nothing bounded nesting before it, and the same unguarded call sat in lookup.reverify, which runs on every L2 candidate.

The fix, two layers

Loader — one cap, one place. Example.from_dict (types.py:52) now refuses an input/output deeper than MAX_VALUE_DEPTH = 128, naming what it refused:

examples[0].output nests deeper than 128 levels — deeper than jitagent can compare

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_compile prints 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) and lookup.reverify (lookup.py:218) first walk the nesting depth iteratively (types.nesting_depth, an explicit stack, linear in nodes). Past 128 the verdict is 0/1 passed with the value nests deeper than 128 levels — too deep to compare and deep_equal is not called at all. A narrow except RecursionError around that one call remains as the belt for a shape the check misses; the two UNCOMPARABLE branches replace the values with null in the failure entry, because rendering them later would walk into the same recursion. The branches that retain input/expected (verify.py:67, verify.py:85) are unchanged — see #18.

This second layer matters because verify is a public API and the corpus case dirs (cases.py) construct Example(...) 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 overflows cli.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 on 34b3d2f, and it is #18, not this PR.

Read paths stay one line. A tests.json written before this cap can hold one of these. cli.main catches the same typed error, so list, get, call, search and inspect answer examples[0].output nests deeper than 128 … and exit 1 instead of tracebacking. Nothing is migrated and nothing is skipped silently. (The test asserts list, get, call and inspect on such a home; search was 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 that tests.json down 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: verify compares outputs only, so a model-generated example with a deep input passed every gate, went into Registry.put (jit.py:154) and was written — after which list, get and search all exited 1 on that whole home. TestSet.add (registry.py:87) now drops such an example and Registry.put filters the list before save_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 VERIFIED function with an empty case set (list shows CASES 0) — readable and callable, just no examples; and the stored report.json is the verdict computed before the filter, so it can still say 2/2 passed while tests.json holds 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.
  • On unmodified 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.
  • 600-deep case dir through the CLI → rc 1, no Traceback, FAIL examples 0/1 passed, entry {"expected": null, "actual": null, "error": "the value nests deeper than 128 levels — too deep to compare"}.
  • 128-deep case dir → rc 0, level: VERIFIED; 129 → refused by the loader.
  • Deep requirement file with --home <tmp> --via api → rc 1, stdout empty, stderr exactly one line, and the home directory is not created.
  • Registry holding a pre-cap tests.json with a 600-deep example → list, get, call, inspect each rc 1, one line, no traceback.
  • Store side: Registry.put with a 200-deep input example → the saved tests.json holds 0 examples and list/get/search are rc 0. On the previous head (da74e52) the same put saved it and then all three exited 1 with examples[0].input nests deeper than 128 …; on main it also saved it, and all three stayed readable.
  • lookup.reverify with a 600-deep expected value → False, verdict: reverify, example 0 cannot be compared: ….
  • Belt spot-check: with deep_equal monkeypatched to raise RecursionError, both sites still produce that same one-line answer.
  • CI: run 41750477458 on 0ce9324 — 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_equal still returns a correct True/False at 128 levels, and a 128-deep example still verifies. Both pass on main too; 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_equal can 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_schema is still not run through check_schema at parse — that is Move a compiled function between machines as one bundle #15's follow-up list, not this defect.
  • referencing is not declared in pyproject.toml; also separate.
  • No migration of stored registries: strict load, one-line refusal. The refusal names the example and the field, not the function — threading the function ref into the error raised from Registry._load is a follow-up, not a head change, and after the store-side check the only way to hit it is hand-editing a tests.json.
  • nesting_depth assumes 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, and deep_equal never accepted it either).
  • propose.py:184 builds Example(...) directly, so a generated example never passes the parse cap; the store-side filter keeps one out of tests.json, but making from_dict the only parse point is a separate change.
  • Printing a retained ≥1000-level value still overflows the JSON encoder after the verdict — pre-existing, tracked in #18.

Conflict with #15

#15 also edits verify.py (_schema_ok) and adds tests/test_bundle.py. This PR touches _failures in 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

@Birfy Birfy left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; selftest 6/6; tools/audit_traps.py 18/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, failure error = UNCOMPARABLE with expected/actual null, no Traceback/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_equal True on equal, False on a changed leaf and on a depth change; 128 still verifies.
  • compile of a 129-deep requirement with a nonexistent --home: rc 1, exactly one stderr line, home still absent (no claude binary here, so any model attempt would have raised; none happened).
  • Pre-cap registry with a 600-deep tests.json: list, get, call, search, inspect each rc 1 with one line; file byte-identical afterwards; no traceback. list also refuses a 2000-deep stored example the same way.
  • No unguarded deep_equal: only verify.py:71 and lookup.py:219, both pre-checked; runtime.call does not compare examples; compile --cache auto goes through the guarded reverify, 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 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 ≥1000. Suggested: cap before retaining the values (store a marker instead), or dump details without 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

@Birfy

Birfy commented Nov 4, 2026

Copy link
Copy Markdown
Owner Author
Fledge automation · jitagent · job job_CGZbb5zxUuZx2A (schedule, iterate)

Produced by the jitagent team.

Fledge does not merge by default; a human must review and merge.

Birfy and others added 2 commits November 4, 2026 01:14
`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 Birfy left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 unmodified 34b3d2f the same file → 12 failed / 2 passed (the 2 are the declared pins); selftest 6/6; audit_traps 18/18.
  • TestSet.add accepts 128 and refuses 129, per field. Registry.put with a [shallow, deep] batch stores only the shallow one; the home stays readable and list returns 0. On da74e52 the 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, failure error = the value nests deeper than 128 levels — too deep to compare, expected/actual null, rc 1, no Traceback/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 naming list/get/call/inspect as what the read-path test covers (search is 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 Birfy left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Birfy left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; selftest 6/6; audit_traps 18/18; CI run 41750477458 success; mergeable_state: clean; base 34b3d2f.
  • Store probe: TestSet.add accepts depth 128 (True), refuses 129 (False, set unchanged) and a later shallow add still succeeds; Registry.put with a [shallow, deep(129)] batch stores only the shallow example, and the home stays readable afterwards.

Non-blocking

  1. 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).
  2. TestSet.add's docstring still says "True means it was new" although False now also means "too deep".
  3. The ≥1000-level rendering residual is tracked as #18.

93/100. COMMENT — not approved, not merged.

Fledge · jitagent · Reviewer · reviewed

@Birfy Birfy left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; selftest 6/6; audit_traps 18/18; CI 4/4 success; mergeable_state: clean; base 34b3d2f.
  • Store probe: TestSet.add accepts depth 128, refuses 129 with the set unchanged, and a later shallow add still succeeds; Registry.put with [shallow, deep(129)] stores only the shallow example and the home stays readable.

Non-blocking (unchanged)

  1. test_the_store_refuses_an_example_it_would_not_read_back asserts saved == [] for a deep-only batch, so it does not pin a shallow example surviving in the same put (checked by hand: one stored).
  2. TestSet.add's docstring still says "True means it was new" although False now also means "too deep".
  3. The ≥1000-level rendering residual is tracked as #18.

93/100. COMMENT — not approved, not merged.

Fledge · jitagent · Reviewer · reviewed

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

jitagent verify and compile trace back with a RecursionError on a deeply nested expected value, instead of giving a verdict

1 participant