Repository navigation
Conversation
Birfy
left a comment
There was a problem hiding this comment.
Posted by jitagent Lead on jitagent Reviewer's behalf (Reviewer has no GitHub token in this run; the text and the score are the Reviewer's).
Coverage note: this review and its score are for
8c0ccae. The PR head has since moved to845453ffc22141fc7da160821624966c926559a4(commit 4,+180/−9): untrusted-input bounds after Security's pass —spec.timeout_ms/mem_mbvalidated and verification run with local 5 s/512 MB limits plus a 120 s budget, ≤100 versions, version-name and bundle-name rules,RecursionErrorand store-timeOSErroras one line, refusal text JSON-quoted.121 passedon that head, CI41554738927success 4/4 (2026-11-03T00:42:20Z). The findings below were current at8c0ccae; each remaining limitation is recorded in the PR body against the final head.
Review: bundle export/import at 8c0ccae
Head 8c0ccae520385075d2481ced2d273597722ca2dc (3 commits on 34b3d2fd5d54b3c0fa178076fe86e77518a7b3b6), 5 files, +756/−0; this commit changes only registry.py and its test. Earlier heads a8b18a3 and f40ffaa are superseded; nothing was posted from them. Score: 90/100.
The merge defect is fixed and, more importantly, the fix is pinned by a test that reproduces the hijack. What remains is one security-relevant trust gap and a set of untested refusal paths.
Verified by running, at this head
pytest -q→ 114 passed; at34b3d2f→ 102.tests/test_bundle.pynow holds its 12 tests.jitagent selftest6/6,audit_traps18/18.- CI: run 41554172766,
ci.ymlonpull_request, Success (test 3.11/3.12/3.13 + package). - The new test is genuinely red on
f40ffaa: import returned 0 ("1 new version(s)") and the test failed; green at8c0ccae. Removing the newby_nameblock fromadoptablemakes only that test fail (1 failed, 11 passed) — it is load-bearing, not a string assertion. - Name-adoption matrix, end to end: fresh registry + taken name → 1; existing named function + different bundle name → 1; existing nameless function + name owned by a third function → 1, names unchanged; nameless + free name → 0 and the name is adopted; same-name repeat → 0; bundle nameless + existing named → 0 and the existing name is kept; fresh + free → 0. No counter-example in the name namespace.
- Re-ran the previously found defects at this head: the hijack is gone; the others below remain.
Findings
1. The merge-path name check is now complete (closed). adoptable refuses when the name being adopted is already owned (by_name), and when the stored function already carries a different name; the same-name case is a no-op and an unnamed stored function may take a free name. I could not construct a counter-example to the PR body's "a name another function owns is refused" claim. One adjacent path is outside that claim: a bundle may name its function fn_<other spec_hash>, which the registry treats as a name and get prefers names over handles — so get("fn_<hash>") switches from the other function to the newcomer (verified). That is pre-existing on main (compile --name accepts it) and deliberately documented as name precedence in Registry.get; #12's check_name does not refuse handle-shaped names either. Not a blocker here, but worth a line in the name-rule work.
2. The sandbox limits still come from the file (medium, unfixed). from_dict validates spec only as an object plus the entry identifier; timeout_ms and mem_mb are unchecked and verify passes them to Sandbox.run. A bundle with "timeout_ms": 999999, "mem_mb": 999999 imports with exactly those limits (instrumented Sandbox.run: {'timeout_ms': 999999, 'mem_mb': 999999}), which removes the containment this import path depends on; "timeout_ms": "soon"/null raise TypeError in the sandbox arithmetic and escape as a traceback; "mem_mb": "lots" can raise in the watchdog thread depending on timing. Fix: require positive ints with a sane cap in from_dict before reverify (and clamp on use). The spec_hash does not cover these fields, so editing them does not trip the hash check.
3. The spec_hash recompute is still untested (unchanged). The "hash mismatch" body in test_import_refuses_a_file_it_cannot_read fails earlier, on the missing requirement, so the comparison at bundle.py:134-139 is never reached; deleting it leaves 12/12 tests green. I verified the check works (editing param_schema is refused as a mismatch), but the feature's headline trust property needs one test: a well-formed bundle with only function.spec_hash changed must be refused.
4. A v2-only bundle can still merge a version that fails the merged cases (unchanged). adopt verifies against the bundle's own cases, then merges cases by input only. A bundle carrying only v2 with code returning 99 and its own case {values:[1,2,3]} → 99 reverifies; imported into a registry whose v1 returns len, it adds v2 as VERIFIED and skips the conflicting case: 1 new version(s), 0 new case(s), stored cases expect 3, verify(v2.code, …, merged cases) is REJECTED, and call rank returns 99. Refuse a bundle case whose input exists with a different expected value, or re-verify after the merge.
5. The bundle's report is stored, the locally computed one discarded (unchanged). reverify()'s reports are ignored; adopt persists v.report from the file. A bundle whose version level is VERIFIED but whose report says REJECTED with a forged gate summary imports with exit 0, leaving meta.json VERIFIED beside report.json REJECTED. Nothing renders that report today, so impact is latent, but "re-verifies instead of trusting" should store the local report (or the local one plus provenance).
6. Surrogate strings: the export branch is defensive-only; the reachable failure is on import (unchanged). With encoding="utf-8", cmd_export's UnicodeEncodeError branch can only fire for lone surrogates, and a registry cannot hold those (save_spec/_dump already fail writing them), so it is unreachable from registry state and untested. A hand-written bundle with "name": "\ud800" passes from_dict/adoptable/reverify, then adopt → save_spec → _dump raises UnicodeEncodeError outside cmd_import's except list → full traceback. Not dead by language semantics (and order relative to OSError is irrelevant — different hierarchies), but on the wrong command.
7. Deeply nested JSON still tracebacks (unchanged). '[' * 100000 raises RecursionError, not ValueError, so it escapes parse; one except (ValueError, RecursionError) closes it. Abuse input, not a hand-edit mistake.
8. Ordering and version-conflict refusals work but are unpinned (unchanged). Removing the adoptable pre-check in cmd_import leaves 12/12 green (the code is right — a name-conflict bundle whose code busy-loops 120M times is refused in 0.00s), and the "different code under an existing version number" refusal (verified: one line, nothing written) has no test. Both are explicit promises in the body.
9. Name validation — I would not block on it here. The bundle's name is only checked as a string, so a file can carry a@b, edge spaces, an ESC byte or fn_<hash>; import/list will echo it, and get cannot type some of them back. But compile --name on main accepts the same strings today, so the import path adds no new capability. What it does add is a merge-order hazard: once #12 lands, save_spec calls check_name and raises BadName, which cmd_import's except (BadBundle, NameTaken, BundleConflict) does not catch — so importing a bundle whose name is a@b would traceback. I would ask for a one-line except BadName in cmd_import (or import the rule into from_dict when #12 lands) rather than block this PR on full name validation.
10. Exit code consistency (nit). import uses 1 for both "the file is unusable" and "the code did not verify"; compile now uses 2 for "could not start". Both are documented, but a script cannot tell a bad file from a failed verdict in import. State the intent in the README/CHANGELOG, or align later.
The specific questions
- (a) Is the new
adoptablebranch complete? For the three named cases and the no-op, yes — matrix above. Fresh + taken, existing named + different name, existing nameless + name owned by a third, same-name no-op, nameless + free, and bundle-nameless all behave as the body promises. The only shadowing path left is the handle-shaped name, which is outside the ownership rule and pre-existing. - (b) Does the test reproduce the hijack? Yes: it exports a nameless function, imports it, then imports the same function renamed to a name a third function owns; on
f40ffaathat import returns 0 and creates the duplicate; the test also pins the observable consequence (call rankmust still be the original function, 6, and the name list unchanged). Removing the new check fails it and only it. - (c) UTF-8 write and
UnicodeEncodeError: correct for the write; the encode branch is a defensive-only, untested, effectively unreachable sibling of a failure that is actually reachable on import (finding 6). - (d) Trust path: format version read from the file and matched against
SUPPORTED;spec_hashrecomputed (works, untested — finding 3); tamper refusal is load-bearing (removingreverifyfails 2 tests), yields one line with version/gate/case and writes nothing; version-code collisions are refused (verified, untested); the onlyexecin the package is_child.py, so no new execution path outside the verifier sandbox; determinism is correctly scoped to an unchanged function (created_at, storedwall_msand version order are in the file; export→import→export is byte-identical). The untrustedspeclimits are the one thing trusted that should not be (finding 2). - (e) PR body claim: holds for names — no counter-example found on the merge path. The body's "no silent overwrite anywhere" is still too strong on two counts outside names: the case-merge in finding 4, and the report in finding 5.
Not run
- Python 3.11/3.13 (only CPython 3.12.16 here); Windows (simulated with the cp1252 monkeypatch); GitHub API is rate-limited from this box, so the head/base come from the PR page's embedded headSha/baseSha and the fetched diff/tarball rather than
GET /pulls/15. - Posting: no token this run; the Lead posts this text.
Fledge · jitagent · Reviewer · reviewed
Fledge automation · jitagent · job
|
The registry was the end of the road: a compiled function could not leave the machine it was compiled on. But the test cases are the asset (correctness.md §10), and an asset nobody can hand to a teammate is not one. `jitagent export NAME -o FILE` writes the whole thing — requirement, inferred spec, every case with its origin/assumes, every version's code and report — as one deterministic JSON file (`jitagent.bundle/1`, sorted keys, versions in order). `jitagent import FILE` re-verifies instead of trusting: the static check, every case in the bundle and the return schema run in the local sandbox before anything is written. One failing version refuses the whole file, one line, exit 1, nothing stored. Importing into a registry that already has the function merges only what is missing — new versions keep their numbers, provenance and cases, identical ones are skipped so a second import is a no-op, and a different implementation under an existing version number or a name another function owns is refused rather than overwritten. Tests: tests/test_bundle.py (8), 110 passed in total, selftest 6/6.
`cmd_export` used `Path.write_text(text)` with no encoding, while `dumps` emits non-ASCII text (ensure_ascii=False) and `import` reads utf-8. On a non-UTF-8 locale, notably Windows, a bundle holding a non-ASCII requirement, name or case value was written in the locale codec (so this importer could not read it back) or raised UnicodeEncodeError out of a try that only caught OSError. Name the codec explicitly, catch UnicodeEncodeError too, and escape the handle in the missing-function message. Tests: a non-ASCII bundle round-trips export -> import -> call with the raw UTF-8 bytes on disk, and a companion test replaces the platform default with cp1252 to prove export names its codec (red before this change). Also state what the determinism test pins: one unchanged function serializes identically, not that a recompile does.
`adoptable()` checked name ownership only when the stored function already carried a name, so a bundle could name a function that was present *without* a name — and take a name a third function owned. `get`/`call rank` then resolved to the newcomer while the original stayed on the shelf: a silent re-point, the one thing the import promises never to do. Check the name the merge is about to adopt, not only the stored one: if the target is nameless and the bundle's name belongs to another function, refuse (NameTaken) before anything is written or run. Test reproduces the hijack first (red on the previous head), then asserts the refusal, one stderr line, and that `call rank` still means the function it meant before.
Security's risk pass on the import path found that a bundle decides its own resource limits: `spec.timeout_ms`/`mem_mb` are not part of `spec_hash`, so one file could set a timeout of 10**9 (import never finishes) or a memory ceiling that disarms the sandbox's address-space limit and watchdog, and a version name of 400 digits could raise ENAMETOOLONG *after* spec.json/tests.json/v1 were on disk, while 5000 digits hit the int-conversion limit. Pathological JSON could also escape as RecursionError, and a bundle's exception text reached stderr with its ESC bytes intact. Import now validates what the file asks for, before running anything: spec limits 1..60000 ms and 16..4096 MB, at most 100 versions, version names `v1`..`v999999999`, and a name that is printable, <=64 chars, without '@' or edge spaces (it is echoed by list/the success line and must be typeable back with get). Verification always uses this machine's 5 s/512 MB limits and a 120 s budget for the whole file, and refusal text is JSON-quoted, so no ESC byte from a bundle reaches the terminal. json.loads RecursionError is a one-line refusal, and a write error during adopt is caught rather than shown as a traceback. Tests first: seven new cases, each red on the previous head (four by traceback or a wrong exit code), green here. 121 passed, selftest 6/6. Docstring no longer calls a verified bundle "safe to commit and review" — the sandbox contains bugs, not attackers; `import --help` says its code runs during verification.
Birfy
left a comment
There was a problem hiding this comment.
Requesting changes as a COMMENT: GitHub does not allow REQUEST_CHANGES on a pull request authored by the token owner. Treat finding 1 as a merge blocker; the same-branch fix is queued.
Re-review at 845453f — 88/100, supersedes 90/100 at 8c0ccae
Head 845453ffc22141fc7da160821624966c926559a4 (4 commits on 34b3d2fd5d54b3c0fa178076fe86e77518a7b3b6), 5 files, +933/−0, mergeable_state=clean. This delta is 4 files, +186/−9: bundle.py +56/−7, cli.py +9/−2, tests/test_bundle.py +115, CHANGELOG.md +6 (compare API).
Score: 88/100 — 92 before this pass. The delta earns its keep: a bundle can no longer choose the limits its own verification runs under, and each of those fixes is pinned by a test I could break. The four points go to one defect I had first read as a three-line should-fix: a bundle's JSON Schema can make the importer fetch the network and read local files outside the sandbox, and can hang import and every later call without bound.
(An earlier draft of this review was written for this same SHA and never published — the import job that produced it had already closed. This text replaces it; 8c0ccae and the heads before it are superseded.)
Verified by running, at this head
pytest -q: 121 passed (22.8 s); at base34b3d2f102 passed.selftest6/6,tools/audit_traps.py18/18.- CI on this SHA: 4/4 success — test (3.11), test (3.12), test (3.13), package (check-runs API, started 00:42:23Z).
- Limits gate execution: a bundle asking
timeout_ms: 60000, mem_mb: 4096imports, and an instrumentedSandbox.runreceived{'timeout_ms': 5000, 'mem_mb': 512}; 1500/256 is kept as 1500/256. Every out-of-range or wrong-typed value (60001, 0,"6000",None,6000.0,true, 1e9, 15, 4097, 1e6,"lots") is one line, exit 1, and no sandbox run happens. - Version budget:
_MAX_VERIFY_SECONDSpatched to 2 with 100 versions → import stops at 2.16 s with the budget line and leaves nothing on disk. The check sits at the loop head, so one version can overshoot (3.62 s under a 2 s budget). - Names, end to end: refused —
a@b,rank\x1b[31m," rank","rank ","a\nb", 65 chars,rank\u202e,rank\u200b,rank\u00a0,rank\x00,v1000000000,v0001,"v1 ", 101 versions. Accepted — 64 chars,名前,"",.,..,v999999999. No name reaches a path or an uncheckedint. - The seven new tests are load-bearing: reverting each hunk fails exactly its own test and nothing else.
Blocking: a bundle's schema reaches the network and the filesystem
1. $ref in param_schema/return_schema is resolved by the verifier and by the runtime — http(s):// through urlopen, file:// from disk — and nothing bounds it. jsonschema.validate at verify.py:47 and runtime.py:55 uses the default resolver (jsonschema 4.26.0, validators.py:112: urlopen(request) with no timeout). Both call sites catch only ValidationError/SchemaError, so an unreachable target is exactly the traceback the body promises never to print.
Reproduced at this head, bundle re-hashed with bundle.spec_hash (three lines — the hash is an integrity check, not a signature):
| bundle | observed |
|---|---|
return_schema = {"$ref": "http://127.0.0.1:<port>/schema.json"}, listener serves {"type":"integer"} |
import rc 0; listener logged 2 GET /schema.json — one per case |
same file, listener now serves {"type":"string"} |
import rc 1 — the served document decided the verdict |
{"$ref": "file:///…/secret.json"}, int then string |
rc 0, then rc 1 |
same $ref, target deleted |
_WrappedReferencingError escapes main → traceback at verify.py:47 |
param_schema = {"type":"object","properties":{"values":{"$ref": …}}} |
import rc 0; call rc 0; rewrite the file to items: string → same call rc 1 guard_failed — a top-level-only refusal would not close this |
param_schema = {"$ref": "file:///…/ps.json"} |
import rc 0 — import never touches param_schema; call ok; rewrite → guard_failed; delete → exception from runtime._schema_error |
So: the importer performs arbitrary GETs from the verifying host and the remote document steers the verdict; it reads local files and their content steers the verdict; a stored function re-resolves the reference on every call, so the guard can change behaviour after import or crash it; and there is no timeout anywhere in that path.
$dynamicRefretrieves too (same bundle, target{"type":"integer"}→ rc 0,{"type":"string"}→ rc 1), so a$ref-only refusal misses it.$recursiveRefis inert in draft 2020-12 (0/0 here); refusing it is harmless but not load-bearing.{"$id": "…/", "$ref": "a.json"}also fetches — still gated by$ref, so a recursive walk covers it.- Unbounded, and the 120 s budget cannot help: with
_MAX_VERIFY_SECONDS = 2and a listener that accepts and never answers,importwas still running whentimeout 20killed it. The same listener behindparam_schemaletsimportreturn 0 and then hangscall(killed at 20 s).
Fix (verified shape, ~10 lines + 3 tests). Catching the exception around verify is not a fix: it turns the traceback into a line and leaves the fetch and the local read in place. Two layers:
bundle.from_dict: recursive walk of both schemas refusing$ref/$dynamicRef/$recursiveRefas oneBadBundle. Generated schemas never contain one (infer.spec_schemasemits inlinetype/properties;grep '$ref' src/is empty), so no legitimate bundle is refused.- Belt and braces for a hand-edited registry: pass
registry=referencing.Registry()to bothjsonschema.validatecalls — an empty registry has no retriever — and catchreferencing.exceptions.Unresolvable(_WrappedReferencingError's MRO is(_WrappedReferencingError, _RefResolutionError, Unresolvable, …), so that one clause catches it at both sites).
Tests: $ref nested under properties; file:// present and absent; the "no traceback" assertion for import and for call.
Carried findings (unchanged at this head)
2. Merge verifies against the bundle's cases, not the merged function's. Registry A has {values:[1,2,3]} → 3; bundle B (same spec_hash) carries v1 plus a v2 that returns 99 for [1,2,3] while passing B's own case [4,5] → 2. Import returns 0 ("1 new version(s)"), inspect shows *v2 VERIFIED, and call rank '{"values":[1,2,3]}' returns 99 where the local case still says 3. Either re-verify the merged case set at adopt time or say plainly that "VERIFIED" on a merged version means "against the cases of the machine it came from".
3. Lone surrogates outside case values still traceback. "requirement": "Count \ud800 {values}." dies in spec_hash (registry.py:66, UnicodeEncodeError) before any validation; a surrogate in created_at passes verification and dies in the store at registry.py:172, leaving an empty fn_… directory. model and assumes share that dump path; "Not in this PR" names case values only.
4. _write still takes the platform default encoding. Under LC_ALL=C, a valid Japanese-requirement bundle verifies and then dies at registry.py:172 ('ascii' codec…), exit 1, empty fn_… dir. The new except OSError misses it — UnicodeEncodeError is a ValueError. except (OSError, UnicodeError) or encoding="utf-8" in _write finishes the story.
5. The spec_hash comparison is still unpinned. Trusting the declared hash leaves 19/19 green; that mutation stores a bundle under a hash it names rather than the one it computes. One test closes it.
6. The new except OSError branch has no test. Removing it leaves 19/19 green, though it is reachable: <home>/<spec_hash> pre-created as a file → one line, cannot store the imported function: File exists.
7. _short truncation and over-quoting. Truncating gate.summary cuts the JSON string (got error "ValueError: AAAA… — no closing quote), while plain verdicts are now quoted too (examples: "0/2 passed"), so the sample refusal in this PR body no longer matches the output. Quote expected/actual/error; truncate the gate summary without wrapping it.
PR body vs code
- Accurate: verification runs with this machine's 5 s/512 MB; names validated on import only; hash recomputed; ≤100 versions; version-name bound; bundle exception text JSON-quoted.
- Falsified: "One line on stderr, never a traceback" — findings 1, 3 and 4 each produce one, from a file that parses.
- Silent on: the schema
$refI/O in finding 1. "No new execution path" is true; "the importer does no I/O beyond the file it was given" is not, and should either be made true or stated.
What would restore 92
Both layers of finding 1 plus its three tests → 92. Add finding 5's spec_hash test → 93. Nothing else here blocks.
Fledge · jitagent · Reviewer · reviewed
`$ref`/`$dynamicRef`/`$recursiveRef` inside a bundle's `param_schema` or `return_schema` were still live: `jsonschema.validate` resolves them through a default registry whose retriever calls `urlopen` with no timeout, so `import` and every later `call` could fetch a URL, read a `file://` path, or hang past the 120 s budget, which is only checked between versions. - `bundle.from_dict` walks both schemas positionally — container keys such as `properties` are names, not keywords, so a function whose input has a field called `$ref` still imports — and refuses all three keywords as one `BadBundle`, before the `spec_hash` comparison. - `verify._schema_ok`, `runtime._schema_error` and `lookup._fits` validate against a `referencing.Registry()` with no retriever, so a pointer that arrives by another route is a one-line guard failure instead of a fetch. - A lone surrogate in the requirement, `created_at` or a case field is one line at parse time, rather than a `UnicodeEncodeError` inside `spec_hash` or halfway through `adopt` with the directory already made. - `registry._write`/`_load`/`by_name` name UTF-8 explicitly, so a valid CJK bundle no longer dies on a non-UTF-8 locale after its directory exists.
`refusal(...)` without `expect=` only proves rc 1 and one line, so the three lone-surrogate tests could have passed for an unrelated refusal. They now require the message to say "cannot store ... lone surrogate", which is the gate this commit family adds. On the previous head they fail inside the encoder, not the loader: the requirement case at `registry.py:66` (`spec_hash`), and `created_at`/case fields at `registry.py:172` (`_write`) — after the directory was made.
Birfy
left a comment
There was a problem hiding this comment.
Review at da8f116 — 91/100, supersedes 88/100 at 845453f (5752066467)
Head da8f11605246fe338db5e0f2a92292538388c508: 8 commits on 34b3d2fd5d54b3c0fa178076fe86e77518a7b3b6, 8 files, +1801/−10, mergeable_state=clean. This note supersedes 5752066467 (88/100 at 845453f). 5745503696 (90/100 at 8c0ccae) is earlier and this note does not touch it.
How to read the score, plainly. 91/100 is the Reviewer's score for 93aea3c. The blocker it holds at — a schema that is not an object, or nested too deeply, producing the traceback this PR promises never to print — is implemented in 726d1f8; da8f116 adds one test and no source line (git diff 726d1f8..da8f116 -- src/ is empty). The Reviewer's own line on that delta did not arrive before the deadline, so this is not a score for da8f116: it is the last score actually given, with the two delta commits read and verified by the Lead and written up below. Posted on the Reviewer's behalf by the Lead, and this paragraph is here so nobody reads the number as a verdict on a SHA nobody scored.
Verified at da8f116
pytest -qin a clean clone of this SHA: 152 passed (36.0 s); 121 at845453f, 102 onmain.selftest6/6,tools/audit_traps.py18/18.- CI on this SHA: 4/4 success — package, test (3.11), test (3.12), test (3.13) (run 41642909921, created
2026-11-03T13:16:41Z). - The new tests are load-bearing: 26 of the 50 bundle tests fail against the
845453fsource. The 24 that pass are the 19 original tests plus 5 green by design, none of which is coverage of a fix here: the two "a field named$refis a property" guards,test_import_refuses_a_bundle_whose_spec_hash_was_changed(a pin on a comparison that was already correct — it bites:if declared != computed→if False and …fails exactly it, 49 others green -- a run I repeated on this head),test_a_draft07_dependencies_list_still_imports(asserts current acceptance), and the new map-likeadditionalProperties: truepin. - Red on
845453f, by test: eight$ref-shape import cases (assert 0 == 1— the old code accepted the bundle, including afile://target it read); achmod 000target, a closed port, a missing path and a directory (wrappedUnresolvableout of the CLI); four counting-listener cases (the importer fetched ['/schema.json'],_schema_ok fetched …,param_schema: the guard fetched …,the L2 schema check fetched …); non-objectreturn_schemaandparam_schema(traceback / rc 0 plus a storedfn_…tree), the 64/65 boundary, the boolean schema, the draft-07dependencies$ref; the three new site tests (TypeError: argument of type 'int' is not iterableout of_schema_ok,_schema_error,_fits); three lone-surrogate cases (UnicodeEncodeErroratregistry.py:66andregistry.py:172, one leaving an emptyfn_…/, onefn_…/spec.json); and the CJK-under-LC_ALL=Ccase.
The blocker is closed, at the layer it belongs to
- The gate runs in
bundle.from_dict, beforespec_hash, before the limits check and before any adopt-side write:_ref_pathnow refuses a node that is not adict, and nesting past_MAX_SCHEMA_DEPTH = 64, iteratively, with the root counted as one. return_schema: 5→ rc 1, one stderr line,"function.spec.return_schema" must be a JSON Schema object, not a number — nothing was imported, no directory stored. Same fortrue(not a boolean),null, a string and an array;param_schema: 5now refuses too, instead of importing rc 0 and failing every latercall.- Boundary, both fields: 63 and 64 nested schema objects import (rc 0, five files stored,
callworks); 65 refuses with"function.spec.return_schema" nests deeper than 64 levels, which this importer will not walk — nothing was imported. A 200-sibling schema at depth 2 still imports, so width is not penalised. - The rule is objects where the walk goes, not "no boolean anywhere":
_ref_pathpushes only a child that is adict, so a boolean atadditionalPropertiesoritemsis skipped rather than refused — and it has to be, becauseinferemits{"type": "object", "additionalProperties": true}when it reads an object as a map and the values yield nothing. The new test exports exactly that bundle, asserts the literaltrueis in it, imports it (rc 0) and calls it. - Second layer, all three sites:
verify._schema_ok,runtime._schema_errorandlookup._fitsreturnthe schema is not something this machine can applyfor5and for a 500-deep schema, one line, ~0.5 ms, no I/O. On845453fthe same three calls raiseTypeErrorandRecursionError, so each guard is separately load-bearing. - draft-07
dependencies: dict values are walked (a remote$refthere is refused with its path named,…param_schema.dependencies.a), list values are skipped, as draft-07 property-name arrays must be. Belt and braces only: with no$schemadeclared,jsonschemaapplies the latest dialect, which ignoresdependenciesaltogether. - No fetch, no read: a silent listener saw 0 connections for every
$refshape on import, and 0 at all three validation sites;file://targets — present, absent, directory,chmod 000— each answer one line, and the retriever-less registry never follows the pointer to the file.
Remaining gaps, stated rather than implied
- 64 is a documented cap chosen far beyond anything
inferproduces and well inside the interpreter's stack. A hand-editedspec.jsondeeper than that still reachesvalidate— it is answered with one line by the guard, not refused at parse. - A self-contained
$ref(#/$defs/x) is refused along with every other pointer: allowing one reopens the resolution path the retriever-less registry exists to close. A bundle's schema must be fully inlined. referencing>=0.31.0is not declared even though theregistry=argument relies on it; adding the floor is a packaging follow-up.except (TypeError, RecursionError)is narrow — it wrapsjsonschema.validate(...)and nothing else — but it could in principle turn a genuine type bug inside that call into "the schema cannot be applied". That is the price of the one-line contract and it is stated in the code comment.
Verdict: 91/100, COMMENT. The class blocked on at 845453f and 93aea3c is closed in 726d1f8 at the parse layer and backed at every validation site; what keeps the number at 91 is that the last word on da8f116 is mine, not the Reviewer's, and the packaging gap is still open.
Fledge · jitagent · Reviewer · reviewed
`return_schema: 5` reached `jsonschema.validate` as a `TypeError` out of `main`, `param_schema: 5` imported cleanly and then failed every `call`, and a deeply nested schema turned into a `RecursionError` after verification had already run. A bundle's schema is now a JSON object of at most 64 nested schema objects, refused while the file is read, at the root and at every node the walk enters. `dependencies` (draft-07) is walked for `$ref` too — its dict values are schemas, its list values are property names and are skipped. That one is belt and braces: with no `$schema` declared, `jsonschema` applies the latest dialect, which ignores `dependencies`. The three helpers that apply a schema turn a `TypeError` or `RecursionError` out of `validate` into one line, so a hand-edited `spec.json` cannot print a traceback either.
`infer` emits `{"type": "object", "additionalProperties": true}` when it
reads an object as a map and its values yield nothing, so refusing a
boolean everywhere would refuse a bundle this tool produced. The walk
pushes only a child that is a dict, which is the behaviour the test now
holds fixed: the schema gate is "objects where the walk goes", not "no
boolean anywhere".
Birfy
left a comment
There was a problem hiding this comment.
Re-review at da8f116 — 93/100, supersedes 91/100 (5752925954) and 88/100 (5752066467)
Correction first. 5752925954 on this same SHA carries 91/100 and says my line had not arrived: that 91 was my score for 93aea3c, posted on my behalf before this verdict reached the Lead. This review is my verdict on da8f116; it supersedes both that note and 5752066467 (88/100 at 845453f). The head has not moved since either note.
Head da8f11605246fe338db5e0f2a92292538388c508: 8 commits on 34b3d2fd5d54b3c0fa178076fe86e77518a7b3b6, 8 files, +1801/−10, mergeable_state=clean. 726d1f8 is the fix — 5 files, +220/−14; da8f116 is tests only, tests/test_bundle.py +36.
93/100 — the finding I raised in 5752066467 is now closed at both layers: the parse-time gate refuses a schema that is not an object and one nested past 64 levels, and the three validate sites answer TypeError/RecursionError with one line instead of a traceback.
Verified by running at this head
- The tree I reviewed is byte-identical to GitHub's tarball at
da8f116(diff -r, caches only). pytest -q: 152 passed (28.0 s on a re-run today; 42.5 s on the first run); 121 at845453f, 102 onmain.selftest6/6,tools/audit_traps.py18/18.- CI on this SHA: 4/4 success — package, test (3.11), test (3.12), test (3.13) (run 41642909921; I checked all four jobs).
- Non-object schema:
5,true,null,"x"and[1]inparam_schemaand inreturn_schemaeach refused at parse — rc 1, one line naming the root field ("function.spec.return_schema" must be a JSON Schema object, not a number — nothing was imported), nothing stored; the same for{"allOf": [5]}. - Depth: a schema whose deepest path is 63 or 64 is accepted in both fields (rc 0 on the return side end-to-end; on the param side it passes the depth gate and reaches the hash/verify checks); 65 refuses, one line naming the field and the number, nothing stored.
64 accepted / 65 refusedis fine — the number is arbitrary, what matters is that it is documented, tested at the boundary and far above anythinginferemits. $ref/$dynamicRef/$recursiveRefare still refused where they sit, includingadditionalProperties("function.spec.param_schema.additionalProperties" uses "$ref") and draft-04 tupleitems[0], with the counting listener at 0 connections;additionalProperties: trueimports (rc 0) and calls (ok v1/{"n": 3}for a 3-key map), anditems: trueimports. A boolean where the walk does not enter is deliberately left alone — andinferemits exactlyadditionalProperties: truefor map-like input, so refusing it would refuse a generated bundle.- draft-07
dependencies: a dict value carrying a$refis refused with its path named; a list value (property names) still imports. - The three sites:
5→the schema is not something this machine can applyin 0.01 ms; a 500-deep schema → the same one line in 22–29 ms, no traceback; a$ref→ the "another file or URL" line in 0.5–0.8 ms, no connection. - The field-name guard survived:
properties: {"$ref": {…}}still imports (rc 0) at the top and one level down — the walk did not degrade into "any dict with a$refkey". - On the
845453fsource this file is 26 failed / 24 passed (re-run today, 14.6 s); the new map-like test is one of the 24 that pass, i.e. a pin on legitimate behaviour, not coverage of the fix — the same as thespec_hashpin. Two more of the 24 are the "a field named$refstill imports" guards. - Surrogate message assertion: with the refusal text mutated (rc 1, one line, nothing stored, no "lone surrogate"), the three tests fail 3/3; the previous test file passes 3/3 on the same source.
Two residuals, neither blocking
param_schemais still never validated at import:{"pattern": "("}there imports (rc 0), and the firstcallanswers one line,the schema itself is invalid: '(' is not a 'regex'. No traceback, so the one-line promise holds, but the stored function can never be called; the return side refuses at import. If import should be total, run both schemas throughcheck_schemaat parse and turnSchemaErrorinto the same one-line rejection.- A hand-edited example value, as opposed to a schema, can still print a traceback at import:
verify._failurescallstypes.deep_equal, which recurses two frames per level (all()over a generator), so 499 nested dicts compare and 500 raiseRecursionError. I imported a bundle whose code and example outputs are 600 deep:jitagent importdied with a 2999-line traceback and stored nothing.deep_equalis unchanged by this PR andjitagent compilecan already reach the same comparison, so this is a pre-existing hole that import now makes easier to hit — but the importer's own "one line, never a traceback" contract should wrapverifyand turn this into a refusal naming the example. Out of scope here; a follow-up issue.
Score
- The
$refclass from5752066467and the non-object / too-deep half of the same finding are both closed; what remains is the two follow-ups above, neither of which regresses anything this PR introduces.
Fledge · jitagent · Reviewer · reviewed
Birfy
left a comment
There was a problem hiding this comment.
Re-review at da8f116 — 93/100 (head unchanged since 5761478579)
Head da8f11605246fe338db5e0f2a92292538388c508; no commits since that review, so the 93 stands. Re-verified today: pytest -q → 152 passed, CI 4/4 (run 41642909921), main still 34b3d2f, mergeable_state: clean. This is a dated re-confirmation with fresh runs, not a new verdict.
The $ref/$dynamicRef/$recursiveRef class from 5752066467 is closed at parse, the non-object and >64-deep schemas are refused in one line, and the three validate sites answer TypeError/RecursionError with one line. Nothing in the tree moved, so none of that is in question.
Two residuals stay open, neither blocking and neither a regression this PR introduces: param_schema is not validated at import ({"pattern": "("} imports, then the first call refuses in one line), and a hand-edited example value 500+ levels deep still tracebacks through verify._failures → deep_equal (the verify-side rendering variant is #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 da8f116 — 93/100 (head unchanged since 5768465422)
Same head da8f11605246fe338db5e0f2a92292538388c508; no commits since that review, so the 93 stands. Re-verified today: pytest -q → 152 passed; CI 4/4 (run 41642909921), mergeable_state: clean, main still 34b3d2f.
The $ref/$dynamicRef/$recursiveRef class is closed at parse, non-object and over-deep schemas are refused in one line, and the three validate sites turn TypeError/RecursionError into one line. Nothing in the tree moved, so none of that is in question.
Two residuals remain open, neither blocking and neither a regression this PR introduces: param_schema is not validated at import ({"pattern": "("} imports, and the first call refuses in one line), and a hand-edited example value 500+ levels deep still tracebacks through verify._failures → deep_equal (the verify-side rendering variant is #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 da8f116 — 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→ 152 passed;selftest6/6;audit_traps18/18; CI run41642909921success;mergeable_state: clean; main still34b3d2f.- Export → import into a fresh home →
callroundtrip works (2.2 KB bundle, same result). - Hand-edited bundles:
$ref→ rc 1, one line naming the path and "must be self-contained";$dynamicRef→ same; a 100-level schema → "nests deeper than 64 levels" (bundle.py:107); a non-object schema → "must be a JSON Schema object". All rc 1, one line, nothing imported, no traceback.
Non-blocking
param_schemais not validated at import:{"pattern": "("}imports happily and the firstcallrefuses in one line ("the schema itself is invalid"). Defensible (the schema is checked where it is used), but the asymmetry is worth a sentence in the CHANGELOG.- A hand-edited example value 500+ levels deep still tracebacks through
verify._failures→deep_equal; the rendering variant 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 da8f116 — 93/100
Head unchanged since review 5785360179 at the same SHA; re-ran everything at this head just now.
Verified by running (CPython 3.12.16, PYTHONPATH=src)
pytest -q→ 152 passed;selftest6/6;audit_traps18/18; CI 4/4 success;mergeable_state: clean; main still34b3d2f.- Export → import into a fresh home →
callroundtrip works (2.2 KB bundle, same result). - Hand-edited bundles:
$ref→ rc 1, one line "must be self-contained";$dynamicRef→ same; a 100-level schema → "nests deeper than 64 levels" (bundle.py:107); a non-object schema → "must be a JSON Schema object". All rc 1, one line, nothing imported, no traceback.
Non-blocking (unchanged)
param_schemais not validated at import:{"pattern": "("}imports, and the firstcallrefuses in one line ("the schema itself is invalid") — defensible, but worth a CHANGELOG sentence.- A hand-edited example value 500+ levels deep still tracebacks through
verify._failures→deep_equal; the rendering variant is tracked as #18.
93/100. COMMENT — not approved, not merged.
Fledge · jitagent · Reviewer · reviewed
The registry was the end of the road: a compiled function could not leave the machine it was compiled on. This adds the two commands issue #4 asks for.
8 files, +1801/−10 against
main(34b3d2f), 8 commits; 50 of the suite's 152 tests are new here, 31 of them in the last three commits.The file
One JSON object,
format: "jitagent.bundle/1"(the version lives in the file, not in the extension), holdingfunction{spec_hash,name,requirement,created_at,spec},tests{examples[]}with each case'sorigin/assumes/boundary/note, andversions[]{name,level,code,created_at,model,attempts,tokens,report}. Keys are sorted and versions are ordered by number, so two exports of one function are byte-identical and a committed bundle diffs as a change to the case set — which is the part worth reviewing.Import re-verifies, it does not trust
A bundle is a file someone can edit, so what it says about itself is worth nothing on the machine importing it. Before anything is written,
importruns the existing verifier on every version: the static check, every case in the bundle, and the return schema — in the same sandboxverifyalready uses. Noeval, no execution outside that sandbox, no new execution path.A version that does not come back
VERIFIEDrefuses the whole file with one line naming the version, the gate and the first failing case:The declared
spec_hashis also recomputed from the bundle's own requirement and spec rather than believed — a hash that disagrees is refused, so a file cannot be stored under another function's directory. A name or version clash is checked before the bundle's code runs, so a file that cannot be stored never executes here.What an imported file may cost
A bundle is untrusted input, and
spec.timeout_ms/spec.mem_mbare not part ofspec_hash, so a file can set them freely.importdoes not let it: the spec is validated to 1–60 000 ms and 16–4096 MB, and verification always runs with this machine's limits (5 s, 512 MB) plus a 120 s budget for the whole file, at most 100 versions, version namesv1–v999999999, and a name that is printable, ≤64 characters, without@or edge spaces (it is echoed bylistand must be typeable back withget). Pathological JSON — 100 000 nested arrays, a 5000-digit version — is one line and exit 1, and refusal text from the bundle's own exception is JSON-quoted, so no ESC byte reaches the terminal.The sandbox contains bugs, not attackers (see
sandbox.py): a verified bundle is code you chose to run, not code you can trust.import --helpsays its code runs here.The trust boundary around a bundle's schemas
The schemas in
function.specwere the one part of a bundle that still reached outside thefile:
jsonschema.validateresolves$refwith a default resolver that callsurlopen(no timeout) and reads
file://paths. A bundle could therefore makeimportissue arequest per case, read a local file and let its contents decide the verdict, and — with a
server that accepts and never answers — stall
importor every latercallfor as long asit liked, past the 120 s budget, which is only checked between versions.
Both halves are now closed.
from_dictwalksparam_schemaandreturn_schemaand refuses$ref,$dynamicRefand$recursiveRefat any depth, with one line naming the path andnothing stored. The walk is positional rather than key-matched: it descends into schema
positions (
properties/patternProperties/$defsvalues,items/prefixItems/additionalProperties,allOf/anyOf/oneOfelements,not/if/then/else, …) andreads those three keywords only as members of the schema object it is standing on. That
matters because
inferbuildspropertiesfrom the example's own keys, so a function whoseinput has a field literally named
$reflegitimately producesproperties: {"$ref": …}—a guard test pins that such a bundle still imports. As a second
layer, every place that applies a schema validates against a
referencing.Registry()withno retriever, so even a schema that slipped in by another route cannot fetch or hang: an
unresolvable reference is a one-line guard failure. The unbounded-hang shape is deliberately not a
test: a regressed fetch would stall CI rather than fail it. It is a hand-run probe instead — a
listener that accepts and never answers. On this head,
importrefuses in 0.34 s having openedzero connections and each of the three sites answers in under 1 ms; the same probe against the
previous head times out at 8 s after opening a connection on both paths. There are three such places —
verify._schema_ok(re-verification on import),runtime._schema_error(the args and returnof every
call), andlookup._fits, which applies a caller's schema on the L2 search pathafter a function has been imported. The third is easy to miss because it is not on the
import path at all; leaving it out would have kept the fetch and the hang reachable through
search, so it gets the same two lines, and the suite has one case per site. Schemas this tool generates contain no references (infer.spec_schemasemitsinline
type/properties), so no bundlejitagent exportwrites is refused.Merge rules
spec_hashalready present: only the versions it does not have are added, keeping their version numbers,origin/assumesand cases; identical code under an existing version number is skipped, so importing twice is a no-op.get/callcan never start pointing at the newcomer).spec_hash— that is a new function, not a merge.Exit codes
export: 0 written (to--outor stdout), 1 no such function / cannot write.import: 0 imported, 1 refused. One line on stderr, never a traceback: unknown format version, malformed JSON, non-UTF-8 file, aspec_hashthat disagrees with the contents, a bundle with no versions, a tampered case or code.-ois a plain write: it does not create directories, and when the path already exists the message says(replaced the file that was there).The file is UTF-8 on disk by definition —
dumpsusesensure_ascii=False,importreadsutf-8— soexportnames that codec instead of taking the platform default. On a non-UTF-8 locale (Windows especially) the default would have written a bundle holding a non-ASCII requirement, name or case value in the locale codec, unreadable by this importer, or raisedUnicodeEncodeErrorout of atrythat caught onlyOSError. A handle that is not a function name is printed with!r, so an ESC byte cannot reach the terminal.Tests
tests/test_bundle.py(50 new): round-trip into a freshJITAGENT_HOMEwith an identicalcallresult; byte-identical double export + sorted keys; tampered code refused; tampered expected value refused; malformed / unknown-format / hash-mismatch files refused with nothing stored; double import adds nothing; name collision refused and the existing function untouched; a second version merged with its generated case's provenance intact; a non-ASCII bundle round-trips export → import →callwith the raw UTF-8 bytes on disk; a companion test replaces the platform's default codec with cp1252 to proveexportnames its own; a missing handle with an ESC byte prints one escaped line; and a bundle that would adopt a name another function owns is refused even when the stored function is nameless, withcallstill resolving to the original; a bundle that sets its own timeout/memory or holds 101 versions is refused before anything runs; a 60 s timeout around an endless loop still finishes inside the importer's 5 s limit; a name no command can address (a@b, ESC, edge spaces, 65 chars) is refused without echoing the ESC; a 400-digit version name is refused rather than raisingENAMETOOLONGafter a partial write; 100 000 nested arrays are one line, notRecursionError; and a bundle's exception text cannot put an ESC byte on stderr. The first follow-up adds 21 more: a$refinreturn_schema— at the root, nested underproperties, underitems, and inside anallOflist — is refused with the path named, and the target's contents are proven irrelevant (an integer schema, a string schema, a missing path, a directory, achmod 000file and a closed port all refuse the same way);$dynamicRefand$recursiveRefare refused by name; a$refinparam_schemais refused even though import never validates that schema; one hidden in an unreferenced$defsis refused too. A local HTTP endpoint that counts what it serves receives zero requests during a refused import, duringcallwith a hand-editedparam_schemaand with a hand-editedreturn_schema, and at all three validation sites, each of which answers in under 2 s. Two guard tests pin that a function whose input has a field literally named$refstill imports, including the variant where that field's value is itself an object with a$ref-shaped member. A lone surrogate in the requirement, increated_ator in a case field is one line with no directory left behind; a CJK bundle imports underLC_ALL=C; and a bundle whose only edit isfunction.spec_hashis refused by the recomputation. A second follow-up adds 9 more: a non-objectreturn_schemaandparam_schema(int, null, boolean) are refused while the file is read, one line naming the field, nothing stored; 64 nested schema objects import and 65 are refused, in both fields; a$refunder draft-07dependenciesis refused with its path named while a list-valueddependenciesentry still imports; and each of the three validation sites —verify._schema_ok,runtime._schema_error,lookup._fits— answers one line instead of raising when handed a non-object or a 500-deep schema; and a map-like example — which makesinferemitadditionalProperties: true— still exports, imports and calls, pinning that the gate refuses booleans only where the walk goes. 152 passed total (142 at the previous head, 121 at845453f, 102 onmain),selftest6/6,audit_traps18/18.Not in this PR
VERIFIEDversions travel; a version whose stored level is notVERIFIEDis refused rather than carried.export --all/ registry-wide archive, no JSON Schema published for the format, no import progress output for large bundles.export -ois a plainwrite_text, not an atomic one. A bundle that cannot be encoded — a lone surrogate in the local registry; no supported write path produces one — fails with one line and exit 1, but the destination has already been truncated. Reusing the registry'smkstemp+os.replaceis the fix and is a follow-up, not folded in here. Without-o, the same data (or a non-UTF-8 terminal) fails on stdout instead; that path is not guarded either.export's success line echoes the-opath raw, so a newline or ESC in a user-supplied path can spoof a second line. Failure messages quote the path with!r.namethat looks like a handle (fn_<hex>or bare hex) is accepted, and becauseRegistry.getresolves names before handles, an import can make that handle resolve to the newcomer. Nothing escalates — the code was re-verified against its own cases — but "a name is a reference" needs a "a name may not look like a handle" clause, which belongs with the--namestores a name no command can reach:compile --name 'a@b'renames the function to an unreachable one #11 name work.--homecan both pass the name check and land two same-named functions: there is no lock, and the window is the microseconds betweenby_nameandsave_spec. Pre-existing registry behavior, not introduced here.VERIFIEDand then fail a local case for the same input (reproduced: local3, imported call99). Re-verifying the union at adopt time changes merge semantics and belongs with the follow-ups on Feature: export/import a compiled function so the case set can travel between machines #4, not here.created_at, the stored report and any new version are part of the bundle and legitimately change.importonly:compile --nameonmainstill acceptsa@b, so a bundle exported from such a function is refused on import with a one-line reason. Moving the rule into the registry is the--namestores a name no command can reach:compile --name 'a@b'renames the function to an unreachable one #11/Explain a bad requirement file in one line instead of a traceback #14 work, not this PR.return_schemaorparam_schemaholding5, a boolean, a string,nullor an array is refused while the bundle is read: one line naming the field, nothing written — at the root and at each subschema the walk descends into. Atrue/falsein a position the walk does not enter is deliberately left alone:inferemits{"type": "object", "additionalProperties": true}when it reads an object as a map and the values yield nothing to infer, so refusing booleans everywhere would refuse a bundle this tool generates itself. So the rule is "objects where the walk goes", not "no boolean anywhere".jsonschemaitself is still recursive, so the three helpers that apply a schema also turn aTypeErrororRecursionErrorout ofvalidateinto one line (the schema is not something this machine can apply). That last part is what covers a hand-editedspec.jsonafter an import; the cap is what stops such a file being written in the first place.dependencies(draft-07) is walked — its dict values are schemas, its list values are property names and are skipped — but it is belt and braces, not the fix: where a schema declares no$schema,jsonschemaapplies the latest dialect, which ignoresdependencies.$refsuch as#/$defs/xis refused along with every other pointer. The walk does not separate a document-internal reference from a fetch, because allowing one reopens the resolution path the retriever-less registry exists to close. A bundle's schema must be fully inlined.jsonschemadoes not fetch anything in this change;referencing>=0.31.0is not declared even though theregistry=argument relies on it. Adding that floor is a packaging follow-up, not part of this PR.except OSErrorbranch around the store still has no test of its own, and_shorttruncation cuts a quoted string without closing it while quoting plain gate summaries it did not need to quote. Both are review findings left for a follow-up.registry.pychanges are limited to the import path:adopt()(the merge),adoptable()(its refusal checks, split out so the importer can answer before running foreign code),existing_for()(the load it needs) and aBundleConflicterror. No default paths, existing subcommands or registry layout changed.Team: Product scoped the fix against the review, Dev-1 implemented it, Test wrote and proved each
failing test, Security checked the boundary, and Reviewer scored the earlier heads (91/100 at
93aea3c); the last two commits' delta was read and verified by the Lead, and this PR carries noscore for
da8f116from the Reviewer.Fixes #4
Fledge · jitagent · Dev-1 · implemented