Skip to content

Move a compiled function between machines as one bundle - #15

Open
Birfy wants to merge 8 commits into
mainfrom
feat/bundle-export-import
Open

Birfy wants to merge 8 commits into
mainfrom
feat/bundle-export-import

Conversation

@Birfy

@Birfy Birfy commented Nov 3, 2026 •

Copy link
Copy Markdown
Owner

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.

$ jitagent export rank -o rank.jit.json
wrote rank.jit.json   (rank: 1 version(s), 2 case(s))
$ jitagent --home ./other import rank.jit.json
imported rank (fn_380aacf8a481): 1 new version(s), 2 new case(s) — now 1 version(s), 2 case(s)

The file

One JSON object, format: "jitagent.bundle/1" (the version lives in the file, not in the extension), holding function{spec_hash,name,requirement,created_at,spec}, tests{examples[]} with each case's origin/assumes/boundary/note, and versions[]{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, import runs the existing verifier on every version: the static check, every case in the bundle, and the return schema — in the same sandbox verify already uses. No eval, no execution outside that sandbox, no new execution path.

A version that does not come back VERIFIED refuses the whole file with one line naming the version, the gate and the first failing case:

version v1 does not hold up here (examples: 0/2 passed, case 0: expected 3, got -1) — the bundle's code and its own cases disagree, so nothing was imported

The declared spec_hash is 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_mb are not part of spec_hash, so a file can set them freely. import does 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 names v1–v999999999, and a name that is printable, ≤64 characters, without @ or edge spaces (it is echoed by list and must be typeable back with get). 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 --help says its code runs here.

The trust boundary around a bundle's schemas

The schemas in function.spec were the one part of a bundle that still reached outside the
file: jsonschema.validate resolves $ref with a default resolver that calls urlopen
(no timeout) and reads file:// paths. A bundle could therefore make import issue a
request per case, read a local file and let its contents decide the verdict, and — with a
server that accepts and never answers — stall import or every later call for as long as
it liked, past the 120 s budget, which is only checked between versions.

Both halves are now closed. from_dict walks param_schema and return_schema and refuses
$ref, $dynamicRef and $recursiveRef at any depth, with one line naming the path and
nothing stored. The walk is positional rather than key-matched: it descends into schema
positions (properties/patternProperties/$defs values, items/prefixItems/
additionalProperties, allOf/anyOf/oneOf elements, not/if/then/else, …) and
reads those three keywords only as members of the schema object it is standing on. That
matters because infer builds properties from the example's own keys, so a function whose
input has a field literally named $ref legitimately produces properties: {"$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() with
no 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, import refuses in 0.34 s having opened
zero 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 return
of every call), and lookup._fits, which applies a caller's schema on the L2 search path
after 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_schemas emits
inline type/properties), so no bundle jitagent export writes is refused.

Merge rules

  • Same spec_hash already present: only the versions it does not have are added, keeping their version numbers, origin/assumes and cases; identical code under an existing version number is skipped, so importing twice is a no-op.
  • Different code under an existing version number, or a name another function owns: refused, nothing written. No silent overwrite anywhere — including the case where the function already stored here has no name yet and the bundle tries to adopt one another function owns (checked before the name is written, so get/call can never start pointing at the newcomer).
  • A different requirement is a different spec_hash — that is a new function, not a merge.

Exit codes

  • export: 0 written (to --out or 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, a spec_hash that disagrees with the contents, a bundle with no versions, a tampered case or code.

-o is 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 — dumps uses ensure_ascii=False, import reads utf-8 — so export names 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 raised UnicodeEncodeError out of a try that caught only OSError. 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 fresh JITAGENT_HOME with an identical call result; 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 → call with the raw UTF-8 bytes on disk; a companion test replaces the platform's default codec with cp1252 to prove export names 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, with call still 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 raising ENAMETOOLONG after a partial write; 100 000 nested arrays are one line, not RecursionError; and a bundle's exception text cannot put an ESC byte on stderr. The first follow-up adds 21 more: a $ref in return_schema — at the root, nested under properties, under items, and inside an allOf list — 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, a chmod 000 file and a closed port all refuse the same way); $dynamicRef and $recursiveRef are refused by name; a $ref in param_schema is refused even though import never validates that schema; one hidden in an unreferenced $defs is refused too. A local HTTP endpoint that counts what it serves receives zero requests during a refused import, during call with a hand-edited param_schema and with a hand-edited return_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 $ref still imports, including the variant where that field's value is itself an object with a $ref-shaped member. A lone surrogate in the requirement, in created_at or in a case field is one line with no directory left behind; a CJK bundle imports under LC_ALL=C; and a bundle whose only edit is function.spec_hash is refused by the recomputation. A second follow-up adds 9 more: a non-object return_schema and param_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 $ref under draft-07 dependencies is refused with its path named while a list-valued dependencies entry 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 makes infer emit additionalProperties: 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 at 845453f, 102 on main), selftest 6/6, audit_traps 18/18.

Not in this PR

  • No signing or identity: a valid bundle proves "these cases accept this code", not "who wrote it, or that the code is safe to run" — it runs in the same sandbox as everything else. Pinning/trust is a separate decision.
  • Only VERIFIED versions travel; a version whose stored level is not VERIFIED is refused rather than carried.
  • Re-verification is linear in the number of versions and cases (one sandbox process per version), not parallel.
  • No export --all / registry-wide archive, no JSON Schema published for the format, no import progress output for large bundles.
  • export -o is a plain write_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's mkstemp + os.replace is 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 -o path raw, so a newline or ESC in a user-supplied path can spoof a second line. Failure messages quote the path with !r.
  • An imported name that looks like a handle (fn_<hex> or bare hex) is accepted, and because Registry.get resolves 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 --name stores a name no command can reach: compile --name 'a@b' renames the function to an unreachable one #11 name work.
  • Two concurrent imports into the same --home can both pass the name check and land two same-named functions: there is no lock, and the window is the microseconds between by_name and save_spec. Pre-existing registry behavior, not introduced here.
  • A merged version is still verified against the cases that travelled with the bundle, not the merged case set: a v2 that passes its own case can be stored VERIFIED and then fail a local case for the same input (reproduced: local 3, imported call 99). 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.
  • Windows itself is not exercised: CI is Ubuntu-only. Both sides of this PR name UTF-8 explicitly, the cp1252 test covers the codec choice, and the registry now reads and writes UTF-8, but no run happens on Windows.
  • The determinism claim is "one unchanged function serializes identically", not "a recompile produces the same bytes": created_at, the stored report and any new version are part of the bundle and legitimately change.
  • The name rule lives in import only: compile --name on main still accepts a@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 --name stores 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.
  • Every schema node the walk enters must be a JSON object, and the pointer is refused where it sits. return_schema or param_schema holding 5, a boolean, a string, null or 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. A true/false in a position the walk does not enter is deliberately left alone: infer emits {"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".
  • Nesting is capped at 64 schema objects on the deepest path (the root counts as one): 64 imports, 65 is refused, in both fields. The cap lives in the walk, which is iterative; jsonschema itself is still recursive, so the three helpers that apply a schema also turn a TypeError or RecursionError out of validate into one line (the schema is not something this machine can apply). That last part is what covers a hand-edited spec.json after 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, jsonschema applies the latest dialect, which ignores dependencies.
  • A self-contained $ref such as #/$defs/x is 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.
  • jsonschema does not fetch anything in this change; referencing>=0.31.0 is not declared even though the registry= argument relies on it. Adding that floor is a packaging follow-up, not part of this PR.
  • The except OSError branch around the store still has no test of its own, and _short truncation 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.py changes 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 a BundleConflict error. 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 no
score for da8f116 from the Reviewer.

Fixes #4

Fledge · jitagent · Dev-1 · 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 (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 to 845453ffc22141fc7da160821624966c926559a4 (commit 4, +180/−9): untrusted-input bounds after Security's pass — spec.timeout_ms/mem_mb validated and verification run with local 5 s/512 MB limits plus a 120 s budget, ≤100 versions, version-name and bundle-name rules, RecursionError and store-time OSError as one line, refusal text JSON-quoted. 121 passed on that head, CI 41554738927 success 4/4 (2026-11-03T00:42:20Z). The findings below were current at 8c0ccae; 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; at 34b3d2f → 102. tests/test_bundle.py now holds its 12 tests. jitagent selftest 6/6, audit_traps 18/18.
  • CI: run 41554172766, ci.yml on pull_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 at 8c0ccae. Removing the new by_name block from adoptable makes 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 adoptable branch 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 f40ffaa that import returns 0 and creates the duplicate; the test also pins the observable consequence (call rank must 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_hash recomputed (works, untested — finding 3); tamper refusal is load-bearing (removing reverify fails 2 tests), yields one line with version/gate/case and writes nothing; version-code collisions are refused (verified, untested); the only exec in the package is _child.py, so no new execution path outside the verifier sandbox; determinism is correctly scoped to an unchanged function (created_at, stored wall_ms and version order are in the file; export→import→export is byte-identical). The untrusted spec limits 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

@Birfy

Birfy commented Nov 3, 2026

Copy link
Copy Markdown
Owner Author
Fledge automation · jitagent · job job_weY0-Yvyoq__bg (schedule, iterate)

Automated run: Lead claimed + implemented · Test tested · Reviewer reviewed.

Fledge never merges; a human must review and merge.

Fledge and others added 4 commits November 3, 2026 00:54
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 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.

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 base 34b3d2f 102 passed. selftest 6/6, tools/audit_traps.py 18/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: 4096 imports, and an instrumented Sandbox.run received {'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_SECONDS patched 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 unchecked int.
  • 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.

  • $dynamicRef retrieves too (same bundle, target {"type":"integer"} → rc 0, {"type":"string"} → rc 1), so a $ref-only refusal misses it. $recursiveRef is 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 = 2 and a listener that accepts and never answers, import was still running when timeout 20 killed it. The same listener behind param_schema lets import return 0 and then hangs call (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:

  1. bundle.from_dict: recursive walk of both schemas refusing $ref/$dynamicRef/$recursiveRef as one BadBundle. Generated schemas never contain one (infer.spec_schemas emits inline type/properties; grep '$ref' src/ is empty), so no legitimate bundle is refused.
  2. Belt and braces for a hand-edited registry: pass registry=referencing.Registry() to both jsonschema.validate calls — an empty registry has no retriever — and catch referencing.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 $ref I/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

jitagent Lead added 2 commits November 3, 2026 13:10
`$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 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 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 -q in a clean clone of this SHA: 152 passed (36.0 s); 121 at 845453f, 102 on main. selftest 6/6, tools/audit_traps.py 18/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 845453f source. 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 $ref is 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-like additionalProperties: true pin.
  • Red on 845453f, by test: eight $ref-shape import cases (assert 0 == 1 — the old code accepted the bundle, including a file:// target it read); a chmod 000 target, a closed port, a missing path and a directory (wrapped Unresolvable out 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-object return_schema and param_schema (traceback / rc 0 plus a stored fn_… tree), the 64/65 boundary, the boolean schema, the draft-07 dependencies $ref; the three new site tests (TypeError: argument of type 'int' is not iterable out of _schema_ok, _schema_error, _fits); three lone-surrogate cases (UnicodeEncodeError at registry.py:66 and registry.py:172, one leaving an empty fn_…/, one fn_…/spec.json); and the CJK-under-LC_ALL=C case.

The blocker is closed, at the layer it belongs to

  • The gate runs in bundle.from_dict, before spec_hash, before the limits check and before any adopt-side write: _ref_path now refuses a node that is not a dict, 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 for true (not a boolean), null, a string and an array; param_schema: 5 now refuses too, instead of importing rc 0 and failing every later call.
  • Boundary, both fields: 63 and 64 nested schema objects import (rc 0, five files stored, call works); 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_path pushes only a child that is a dict, so a boolean at additionalProperties or items is skipped rather than refused — and it has to be, because infer emits {"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 literal true is in it, imports it (rc 0) and calls it.
  • Second layer, all three sites: verify._schema_ok, runtime._schema_error and lookup._fits return the schema is not something this machine can apply for 5 and for a 500-deep schema, one line, ~0.5 ms, no I/O. On 845453f the same three calls raise TypeError and RecursionError, so each guard is separately load-bearing.
  • draft-07 dependencies: dict values are walked (a remote $ref there 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 $schema declared, jsonschema applies the latest dialect, which ignores dependencies altogether.
  • No fetch, no read: a silent listener saw 0 connections for every $ref shape 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 infer produces and well inside the interpreter's stack. A hand-edited spec.json deeper than that still reaches validate — 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.0 is not declared even though the registry= argument relies on it; adding the floor is a packaging follow-up.
  • except (TypeError, RecursionError) is narrow — it wraps jsonschema.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

Birfy added 2 commits November 3, 2026 13:39
`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 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 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 at 845453f, 102 on main. selftest 6/6, tools/audit_traps.py 18/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] in param_schema and in return_schema each 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 refused is fine — the number is arbitrary, what matters is that it is documented, tested at the boundary and far above anything infer emits.
  • $ref/$dynamicRef/$recursiveRef are still refused where they sit, including additionalProperties ("function.spec.param_schema.additionalProperties" uses "$ref") and draft-04 tuple items[0], with the counting listener at 0 connections; additionalProperties: true imports (rc 0) and calls (ok v1 / {"n": 3} for a 3-key map), and items: true imports. A boolean where the walk does not enter is deliberately left alone — and infer emits exactly additionalProperties: true for map-like input, so refusing it would refuse a generated bundle.
  • draft-07 dependencies: a dict value carrying a $ref is refused with its path named; a list value (property names) still imports.
  • The three sites: 5 → the schema is not something this machine can apply in 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 $ref key".
  • On the 845453f source 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 the spec_hash pin. Two more of the 24 are the "a field named $ref still 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_schema is still never validated at import: {"pattern": "("} there imports (rc 0), and the first call answers 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 through check_schema at parse and turn SchemaError into the same one-line rejection.
  • A hand-edited example value, as opposed to a schema, can still print a traceback at import: verify._failures calls types.deep_equal, which recurses two frames per level (all() over a generator), so 499 nested dicts compare and 500 raise RecursionError. I imported a bundle whose code and example outputs are 600 deep: jitagent import died with a 2999-line traceback and stored nothing. deep_equal is unchanged by this PR and jitagent compile can 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 wrap verify and turn this into a refusal naming the example. Out of scope here; a follow-up issue.

Score

  1. The $ref class from 5752066467 and 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 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 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 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 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 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 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; selftest 6/6; audit_traps 18/18; CI run 41642909921 success; mergeable_state: clean; main still 34b3d2f.
  • Export → import into a fresh home → call roundtrip 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

  1. param_schema is not validated at import: {"pattern": "("} imports happily and the first call refuses 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.
  2. 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 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 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; selftest 6/6; audit_traps 18/18; CI 4/4 success; mergeable_state: clean; main still 34b3d2f.
  • Export → import into a fresh home → call roundtrip 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)

  1. param_schema is not validated at import: {"pattern": "("} imports, and the first call refuses in one line ("the schema itself is invalid") — defensible, but worth a CHANGELOG sentence.
  2. 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

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.

Feature: export/import a compiled function so the case set can travel between machines

1 participant