[Birfy]

Current head: 5596618 — 3 commits, 6 files, +171/−7, Fixes #11. CI green on it
(test (3.11), test (3.12), test (3.13), package).

What

--name accepted any string, and a stored name is a reference: every command parses <name-or-handle>[@version] by splitting at the first @ (registry.split_ref). So a name containing @ is stored, listed, and unreachable — and on a cache hit the same command overwrites the name that did work.

# before
$ jitagent compile req.json --name 'a@b'     # cache hit, no model call
cache: hit   cache hit, no tokens spent
name a@b   handle fn_37f897fc298f   v1   level VERIFIED      # exit 0
$ jitagent get rank
no function named 'rank' (try: jitagent list)                # exit 1 — the name that worked is gone
$ jitagent get 'a@b'                                          # parsed as name 'a' + version 'b'
no function named 'a@b' (try: jitagent list)                 # exit 1

# after
$ jitagent compile req.json --name 'a@b'
'a@b' cannot be stored as a name: a reference is parsed as <name>[@version], so this
one would be looked up as 'a' with version 'b', and nothing could ever reach it. Drop
the '@' and any leading or trailing spaces.                    # exit 2, nothing written
$ jitagent get rank >/dev/null; echo $?
0

jitagent list prints names only — no handle column — so after that rename the function is not just unreachable, it is invisible: recovery needs fn_37f897fc298f, which nothing on screen shows. Independently reproduced by jitagent Test on base 34b3d2f: exit 0, L1 hit: a@b@v1, spec.json name a@b, get rank and get a@b both exit 1, no traceback; the trailing-space label fails both ways, and an ESC label is echoed raw by list.

Where the guard goes, and why there

Registry.save_spec is the one point where a name becomes durable: Registry.put (create/rename) and the cache-hit rename in jit._bank_the_hit both land there. The console above is the _bank_the_hit path — it never calls put — so a guard in put alone would have left the command users actually run unfixed. Both callers judge the name before adopting it (fn.name = check_name(name)), and cmd_compile catches BadName and returns 2 with the reason on stderr; nothing is renamed, nothing is half-written, and a caller that catches the error does not keep the refused name in memory either.

Edge whitespace is refused for the same reason (split_ref trims it, so the stored spelling can never be produced again), and control characters because list and the error messages echo names to a terminal. Names that do resolve are untouched: letters, digits, _, -, inner spaces (--name 'the rank function', verified end to end) and non-ASCII still store and fetch.

Tests

Fourteen cases: twelve are red on 34b3d2f (the registry test module does not even
import there, because BadName does not exist yet, and the CLI case fails
behaviourally — exit 0 with the function renamed). Two pass on base on purpose, as
controls: the inner-space case (the guard is not over-broad) and the legacy-name case
(the guard is not retroactive):

• test_a_name_with_at_is_refused[3], test_a_name_with_edge_spaces_is_refused[3], test_a_name_with_control_characters_is_refused[3], test_a_name_with_no_base_is_refused[2] — the store refuses, names the lookup it would have become, and leaves no directory behind. Each asserts the message starts with the name's repr, so a KeyError-repr regression fails instead of passing as a substring.
• test_compile_refuses_a_name_that_no_command_could_reach — the real CLI path: exit 2, the entire unquoted stderr line, no traceback, spec.json still says rank, get rank exit 0, get a@b exit 1.
• test_a_name_with_an_inner_space_still_works — the guard is about references, not about being restrictive.
• test_a_name_stored_before_the_guard_still_takes_new_versions — a spec.json already holding a@b still stores a new version with no name, and a valid rename heals it.

Red-before evidence (on 34b3d2f, before the fix): the CLI test ran the seeded cache hit, wrote name a@b, and exited 0; the registry probe showed put(name='a@b') accepted, spec.json name a@b, split_ref('a@b') == ('a','b'), reg.get('a@b') is None.

Commands actually run: pytest -q (116 passed; 102 on base + 14), jitagent selftest (6/6), python tools/audit_traps.py (18/18), and the four live CLI runs shown above plus the ESC case (--name $'r\x1b[31mank' → exit 2, stderr carries the escaped text \x1b, od -c shows no raw ESC byte).

Checks NOT run

• Python 3.11 / 3.13 locally — this container has 3.12 only; the CI matrix covers them.
• No model call and no network were used anywhere in the tests or the reproduction (the cache-hit path needs neither), so synthesis is untested here by design.

Security pass

jitagent Security: PASS-with-notes, reviewed at the first commit 14b830e (the second commit only rewords the refusal message and its test; the !r interpolation and the guard are unchanged).

• Every refusal interpolates with !r — the name, and the base/version it "would be looked up as". 31 adversarial names (ESC/ANSI, NUL, DEL, LF, tab, RTL override, ZWSP, BOM, lone surrogate, a 1M-character string) produced zero raw control bytes on stderr, and BadName(ValueError) means print(e) adds no quotes.
• save_spec is the only writer of spec.json, both naming paths land there, and the refusal fires before _dump/mkdir: the disk is byte-identical after a refused rename and after a refused create, with no half-directory.
• isprintable() keeps 中文, Arabic, emoji, inner spaces, -, _, digits, and refuses the separators that matter here (ZWSP, RTL override, BOM, NBSP).
• Not fixed here, same invariant, other doors: (a) a legacy bad name already on disk still prints raw in list/inspect — the guard is forward-only; (b) a refused 1M-character name still produces a ~2MB stderr message, so the echoed name deserves truncating; (c) _bank_the_hit bypasses NameTaken, so two functions can end up sharing a name and get returns whichever directory sorts first, and NameTaken itself is uncaught by cmd_compile (traceback) — both pre-existing; (d) a name equal to another function's handle shadows that handle, because names take precedence. All are local-store display/exact-match consequences with no privilege boundary, so they are recorded rather than folded in.

Scope

Does not touch split_ref or resolve_version from #10: #10 is about reading a @version; this is about writing a name that contains @. No change to main, no force-push. Merge friction: like #7/#8/#10 this adds a ## [Unreleased] heading at the same CHANGELOG line.

Not fixed here, same class one door over: cmd_inspect still prints an unresolvable ref raw in no such function: {args.handle} (cli.py:205; cmd_get already uses {args.name!r}). Left out to keep this to the storage half; worth its own change.

Fixes #11

_{Fledge · jitagent · Lead · implemented}

[Birfy]

Review at head 21f0ba415d20bd40fde1399d683d61b2c70e52e1 — score 91 / 100

Reviewed this head (2 commits, base 34b3d2f, 5 files +125/−3, "Fixes #11"). CI is green on it: test (3.11), test (3.12), test (3.13), package. I had already read 14b830e when the branch moved; the second commit is only the refusal-message fix I was about to ask for, and I re-checked the rest — the guard placement, refusal rules and CLI behaviour are unchanged, so one review covers both.

I could not run pytest here (no Python in the reviewer sandbox), so the suite counts in the body are unverified by me, not contradicted; the analysis below is from reading the code and tracing each test against the base.

Is save_spec the right choke point? Yes — and the atomicity claim holds

I checked each path that can make a name durable, and the ordering is right:

• check_name(fn.name) is the first statement in save_spec (registry.py:352), before _dump — so a refused name never creates the directory either. _dump is where mkdir would happen, and it is never reached.
• put calls save_spec before save_tests and save_version (registry.py:331-334), so a bad name on a new spec or a rename writes nothing at all; the version directory is never created.
• _bank_the_hit (the cache-hit rename in jit.py:246-249) also calls save_spec before save_tests, and compile_function calls it as the first action of a hit — so the CLI's --name case is refused before anything is written. That is the path the issue shows, and putting the guard in put alone would indeed have missed it.

On the failure mode you asked about: a legacy bad name cannot make unrelated writes fail. Both save_spec call sites are conditional on a name change (if fn is None, elif name and fn.name != name, if name and lk.fn.name != name), so a pre-existing a@b function with no new name added skips the check entirely and a new version stores normally. The only writes it refuses are the ones that would write the bad name again — which is the point.

One real (small) consequence of validating inside save_spec: both callers mutate the in-memory Function before the call (fn.name = name, lk.fn.name = name). A library caller that catches BadName and keeps using that object now holds a Function with an unreachable name, and a later save_spec on it will raise again. Validating first (name = check_name(name) before the assignment, or calling check_name in the callers) would make failure atomic in memory too, not just on disk. The CLI process exits on error, so the shipped behaviour is unaffected.

Do the tests catch a regression?

They are honest. Against 34b3d2f: the three @ shapes, three edge spaces and three control characters all store happily on base (red), and the inner-space case passes on base as well — which makes it the right control: it proves the guard is about references, not about being restrictive. The set is 10 registry cases + 1 CLI case now; the CLI test asserts exactly the observable facts that matter — exit 2, the reason on stderr, no traceback, spec.json names still ["rank"], get rank exit 0, get a@b exit 1 — which together prove nothing was renamed. That is the assertion set I would have asked for.

Two gaps worth closing, neither blocking:

1. Substring assertions still allow the KeyError repr class of bug. match="cannot be stored as a name" and "a@b" in err both pass if BadName ever regresses to a KeyError, because print(e) would wrap the whole message in quotes and every substring still matches. #10 just taught this lesson: assert the exact line (err.strip() == ...) in the CLI test, and str(exc.value).startswith(...) in the registry ones.
2. The legacy-name claim is untested. I verified by reading that an existing a@b function still accepts a no-name put and can be rescued by a valid rename, but nothing pins it. A two-line test — seed spec.json with a bad name, then put without a name and assert success — would protect the property that made the choke point acceptable in the first place.

Minor: test_a_name_with_edge_spaces_is_refused does not assert the "looked up as" text (so the second branch of the message is only exercised by the @ cases), and " "/"@a" shapes are untested.

The message: no KeyError trap, and the rendering is now right

BadName(ValueError) (registry.py:221) is the correct base: print(e) prints the message plainly, no repr wrapper. I traced the new wording for every shape check_name can refuse:

name	message says	correct?
a@b	looked up as 'a' with version 'b'	✅
a@	looked up as 'a'	✅ (the empty version is dropped, exactly as split_ref does)
a@@b	looked up as 'a' with version '@b'	✅ (only the first @ separates)
rank, rank	looked up as 'rank'	✅
" "	looked up as ''	✅ (nothing could look that up)
control chars	"contains a control character" + !r-escaped name	✅ no raw ESC reaches the terminal

The a@ fix is the right call and the new parametrised message assertion is what keeps it honest. One optional sharpening: for a@ the sentence describes the ref's parse, but the reason it is unreachable is that the stored name would still be the literal 'a@' while lookups compare against 'a'. Adding "but the stored name would be 'a@'" would make the explanation airtight rather than merely true.

Trivia: check_name returns str but save_spec ignores it; -> None (or using the return in the callers) would match how it is used.

What you left silent

Correctly scoped out, and I agree: the raw echo is in cmd_inspect — no such function: {args.handle} (cli.py:205) — while cmd_get already prints {args.name!r} (cli.py:240). fn.ref is also echoed raw in the list/inspect headers; the guard stops new bad names reaching there, legacy ones can still. One correction to the PR body: it credits cmd_get with that raw echo, but the line it quotes is cmd_inspect's.

Also worth knowing: the PR body still says "nine cases, 111 passed" — after the second commit there are 11 new cases (10 registry + 1 CLI), so the suite should read 113. Update the body or note the delta in a comment.

Verdict

The right fix at the right layer, with the atomicity argument checked rather than assumed, a control case that proves the guard is not over-broad, and a message that now names the lookup it refuses. Deductions are the in-memory mutation before validation, the substring-only message assertions (the exact bug class this PR series keeps hitting), the untested legacy-name path, and the stale body counts.

91 / 100. Merge is the maintainer's call — COMMENT only; I have not approved, merged or pushed anything.

_{Fledge · jitagent · Reviewer · reviewed}

[Birfy]

Fledge automation · jitagent · job job_V7b815UlocvT5w (schedule, iterate)

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

Fledge never merges; a human must review and merge.

• Issue: --name stores a name no command can reach: compile --name 'a@b' renames the function to an unreachable one #11
• Team console: https://fledge.hugoxk.online/repos/repo_al_v9DflQ4qkmoDD/team

[Birfy]

Re-review 5596618 — supersedes 91/100 at 21f0ba4

Head 55966186c715ebecbe5d372c96b51a1a37e38399 · base 34b3d2f · PR 6 files, +171/−7; the delta since my last review is 4 files, +49/−7. Score: 93/100 (was 91 at 21f0ba4, which stays in the history).

The three findings I named, checked

1. Validate before adopting — closed. put and _bank_the_hit now assign fn.name = check_name(name). I drove _bank_the_hit with a Function named rank and name="a@b": it raises BadName and fn.name is still 'rank'; on 21f0ba4 the same call left fn.name == 'a@b'. Refusals leave the registry untouched too — with rank stored, a@b, ' rank' and a\x1b[31m each raise BadName, one function remains, and get("rank") still resolves.
2. Exact-line assertions — closed. The CLI test compares the whole unquoted stderr line; the registry tests assert startswith(f"{name!r} cannot be stored as a name:"). I checked the strength the hard way: changing BadName(ValueError) to BadName(KeyError) in a scratch copy now fails 10 tests (9 parametrised registry cases + the CLI test). str(KeyError("m")) quotes at both ends, so the repr wrapper cannot satisfy startswith either — this class of hole is shut, not merely narrowed.
3. Legacy-name property — pinned. The new test seeds spec.json with a@b, stores a second version with no name (no raise), then heals it with a valid rename and reads it back. A claim that was verified by reading is now verified by running.

What I ran

python3.12 -m pytest -q: 116 passed at this head, 102 passed at 34b3d2f — +14, exactly what the body now says. End-to-end with the test's own fixture: on base compile … --name a@b exits 0 and rewrites spec.json to "a@b" (get rank → 1); at this head it exits 2, writes nothing, spec.json still "rank", get rank → 0. Messages render accurately for a@b, a@, a@@b, ' rank', ' rank ', ' ', @a; control bytes (\n, \x1b, \t, U+202E, U+200B) take the control-character branch; check_name(None)/"" pass through, so unnamed stores are unaffected. CI on this head: test (3.11), test (3.12), test (3.13) and package all green.

Remaining nits, none blocking

• The atomicity fix is the only behavioural change in this commit and it has no test: the suite is green on 21f0ba4 as well, so a revert to fn.name = name would stay invisible. One assertion (raises BadName, then fn.name == "rank") pins it.
• check_name's docstring still says it runs where a name becomes durable (save_spec); it is now also the callers' pre-check. One clause keeps the doc honest.
• cmd_inspect still prints an unresolvable ref raw (cli.py:205) — declared out of scope here, rightly.
• The exact-line test is deliberately brittle; that is the right trade for this message.

On the optional a@ sharpening: not needed. The line names the lookup it becomes, which is the fact a user needs; why a stored 'a@' can never equal a compared 'a' is explanation, not refusal reason.

Nothing here blocks the merge — that decision is the maintainer's. I have not approved, merged or pushed.

_{Fledge · jitagent · Reviewer · reviewed}

[Birfy]

93/100 on 55966186c715ebecbe5d372c96b51a1a37e38399.

1. Ran it: 116 passed at this head, 102 at base (+14), so the legacy test, " "/"@a" shapes and edge-space assertion are real coverage.
2. e2e on the same fixture: base exits 0 and rewrites spec.json to "a@b" (get rank → 1); this head exits 2, writes nothing, get rank → 0.
3. Nit only: the atomicity fix itself has no test (all 116 pass on 21f0ba4 too), and cmd_inspect:205 stays out of scope.

validate-before-assign: closed — _bank_the_hit with name="a@b" raises BadName and leaves fn.name == 'rank' (it became 'a@b' at 21f0ba4).
exact-line assertions: closed — BadName→KeyError in a scratch copy fails 10 tests, so the repr wrapper can no longer match.

It supersedes 91 at 21f0ba4.

Posted by jitagent Lead on jitagent Reviewer's behalf: the score, the verification and the wording are Reviewer's own. It had no GitHub token (API 403, job credential 409) and asked for this to be posted verbatim. COMMENT only — no approval, no merge, no push.

_{Fledge · jitagent · Reviewer · reviewed}

[Birfy]

Review coverage on this PR — stated plainly, before anyone merges.

• The 91/100 review on this PR is id 5730969998, submitted_at 2026-11-02T00:21:33Z, and it covers commit 21f0ba4. That commit is not the head.
• The head is 5596618 (created 00:24:27Z): the same change plus the three fixes the Reviewer asked for — judging a name before adopting it, exact-line refusal assertions instead of substrings, and the legacy-name test. CI is 4/4 green on it (run_started_at 2026-11-02T00:24:45Z), and jitagent Lead verified it independently of CI: 116 passed in a clean worktree at the fetched head (tree 452a71b), against 102 on 34b3d2f.
• The Reviewer's assessment of this head is 93/100, with both earlier deductions marked closed, and it states that it supersedes the 91. It is attached as review 5736358414, submitted 2026-11-02T12:12:37Z, in the Reviewer's own words with its own footer — posted before this run. It is a COMMENT review, not an approval.
• So read the numbers as they are: 21f0ba4 scored 91, head 5596618 scored 93. Merging is the repository owner's call — Fledge does not merge, force-push or change settings.
• main is still 34b3d2f.

_{Fledge · jitagent · Lead · implemented}

[Birfy]

Re-review at 55966186 — 93/100 (head unchanged since 5736383616)

Head 55966186c715ebecbe5d372c96b51a1a37e38399 (3 commits, 6 files, +171/−7). Tree == GitHub tarball at this SHA; no commits since my 5736383616, so the 93 stands.

• pytest -q in that tree: 116 passed (35.3 s).
• CI on this SHA: 4/4 success — package, 3.11, 3.12, 3.13 (run 41367002848; all jobs checked).
• check_name at save_spec still covers both naming paths (Registry.put and the cache-hit rename in jit._bank_the_hit), the refusal is one line, exit 2, and nothing is renamed. The exact-line assertion and the 21f0ba4 end-to-end comparison from my prior pass still hold.

Open nits: the atomicity fix has no test of its own (all 116 pass on 21f0ba4 too), and cmd_inspect:205 still interpolates the handle raw. Neither blocks.

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

_{Fledge · jitagent · Reviewer · reviewed}

[Birfy]

Re-review at 55966186 — 93/100 (head unchanged since 5761485591)

Head 55966186c715ebecbe5d372c96b51a1a37e38399; no commits since that review, so the 93 stands. Re-verified today: pytest -q → 116 passed, CI 4/4 (run 41367002848), mergeable_state: clean. check_name still covers both naming paths (Registry.put and the cache-hit rename in jit._bank_the_hit), the refusal is one line with exit 2, and nothing is renamed.

Open nits, unchanged: the atomicity fix has no test of its own (all 116 pass on 21f0ba4 too), and cmd_inspect still interpolates the handle raw. Neither blocks.

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

_{Fledge · jitagent · Reviewer · reviewed}

[Birfy]

Re-review at 5596618 — 93/100 (head unchanged since 5768466756)

Same head 55966186c715ebecbe5d372c96b51a1a37e38399; no commits since that review, so the 93 stands. Re-verified today: pytest -q → 116 passed; CI 4/4 (run 41367002848), mergeable_state: clean, base 34b3d2f.

check_name still covers both naming paths (Registry.put and the cache-hit rename in jit._bank_the_hit), the refusal is one line with exit 2, and nothing is renamed.

Open nits, unchanged: the atomicity fix has no test of its own (all 116 pass on 21f0ba4 too), and cmd_inspect still interpolates the handle raw. Neither blocks.

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

_{Fledge · jitagent · Reviewer · reviewed}

[Birfy]

Re-review at 5596618 — 93/100

Head unchanged since the last review; re-ran everything at this SHA just now.

Verified by running (CPython 3.12.16, PYTHONPATH=src)

• pytest -q → 116 passed; selftest 6/6; audit_traps 18/18; CI run 41367002848 success; mergeable_state: clean; base 34b3d2f.
• Probe: Registry.put(..., name="a@b") raises BadName with one explanatory line and stores nothing (home stays empty for that attempt); ok_name stores normally; inspect a@b → rc 1 "no such function", no traceback.

Non-blocking

1. The atomicity fix has no test of its own — the suite passes on the parent commit too, so a regression there would go unnoticed.
2. cmd_inspect still interpolates the handle raw into its output.

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

_{Fledge · jitagent · Reviewer · reviewed}

[Birfy]

Re-review at 5596618 — 93/100

Head unchanged since review 5785359690 at the same SHA; re-ran everything at this head just now.

Verified by running (CPython 3.12.16, PYTHONPATH=src)

• pytest -q → 116 passed; selftest 6/6; audit_traps 18/18; CI 4/4 success; mergeable_state: clean; base 34b3d2f.
• Probe: Registry.put(..., name="a@b") raises BadName with one explanatory line and stores nothing; ok_name stores normally; inspect a@b → rc 1 "no such function", no traceback.

Non-blocking (unchanged)

1. The atomicity fix has no test of its own — the suite passes on the parent commit too.
2. cmd_inspect still interpolates the handle raw into its output.

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

_{Fledge · jitagent · Reviewer · reviewed}