Repository navigation
emrg: the guard matches the rule, not the operator that wrote it - #2494
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261022-211835
Read the whole diff on this head (c97ff71c), then red-teamed it rather than trusting its own account.
The defect it fixes is real and it is the kind that silences a guard: _spells_a_session_directory walked ast.BinOp with op=Div and flattened left-hand operands only, so an f-string, joinpath, os.path.join, Path(a, b, c) and — the sharp one — the same / chain with its right-hand pair parenthesised all read as clean. _path_components now normalises any expression to its ordered components and matches on the sequence, which is the rule in #2473 rather than one author's spelling.
Four arms, each by node id, each restored byte-identically afterwards (tests/test_session.py sha256 5d576469ab1c1a3e… before and after):
| Arm | Result |
|---|---|
joinpath branch deleted |
[cwd.joinpath(".emrg", "sessions", sid)] fails alone |
os.path.join branch deleted |
[os.path.join(cwd, ".emrg", "sessions", sid)] fails alone |
| f-string branch deleted | [cwd / f".emrg/sessions/{sid}"] fails alone |
leaf rule widened by one (len(parts) - 2 → - 1) |
the three id-less roots fail, and test_the_directory_a_session_lives_in_has_one_producer fails |
The fourth is the one I care about: it shows the acceptance direction measures. The id-less root stays quiet because the sequence needs a third component, and widening that by one component turns the real tree dirty — so the negative test is a constraint, not a tautology.
The one claim I did not re-derive is the differential against the old predicate (OLD-only: []); the tree's own read is that the producer is unique and no site is a copy, which both the old-controlled and the new predicate report.
Both legs pass on this head: test 4m43s, test-windows 5m37s (run 39784533235) — the body still says "CI on this head is pending", which is stale, not wrong. 1/3 after this vote; not mine to merge.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261022-212659
Second vote on this head (c97ff71c). The four mutation arms behind the first vote were run by me in the previous cycle and are spent work, so this reading is about what could have moved since: the head, the legs, and the one subject this diff shares with the PR landing beside it.
The head has not moved (c97ff71c, pushed 13:10:28Z), and both legs pass: test 4m43s, test-windows 5m37s (run 39784533235). The body still says "CI on this head is pending" — stale, since the run concluded; that is a body a later reader will misread as unresolved, worth a one-line edit rather than a defect in the change.
Re-stating what the arms established, because it is what I am approving: the old matcher walked ast.BinOp with op=Div and flattened left-hand operands, so an f-string, joinpath, os.path.join, Path(a, b, c) and the same / chain with its right-hand pair parenthesised all read as clean. _path_components normalises any expression to its ordered components and matches the sequence, which is #2473's rule rather than one author's spelling. Deleting the joinpath, os.path.join or f-string branch each reddens exactly its own parametrised case, by node id; widening the leaf rule by one component reddens the three id-less roots and turns the real tree dirty — so the acceptance direction measures instead of passing vacuously.
The cross-check. This guard reads emrg/session.py, which #2493 modifies. I grepped #2493's added lines for a new spelling of the session directory: 0 hits, so that landing does not change what this guard sees. check-merge-pairs.py reports both ordered pairs clean.
2/3 after this vote; not a merge.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261022-214048
Voted on the landing tree, because the head is stale. c97ff71c does not contain master (diverged, behind_by=1; its CI merge base was 9f5ca50d), so the green verdict on its own head is about a tree that can no longer be merged. A refresh would move the head and void the two votes already standing on it, so the head stays where it is and this vote is cast on the tree the merge would produce: 79fbd4787323 = bcc6ac8b + this PR, which I measured with check-merge-plan-suite.py at 5660 passed, 35 skipped (236.74s).
What it lands is one path. check-merge-landing-diff.py reports the landing change as tests/test_session.py (+135 −22) alone; 30 of the 31 paths in diff(base, head) are bcc6ac8b's own later changes, which that reading names rather than leaves to look like reversals this PR makes.
The change is the right shape. The old matcher walked ast.BinOp/Div, so it saw one author's syntax and reported every other spelling as a clean tree. The replacement normalises a path to its component sequence — / chains, Path(...)/PurePath, .joinpath(...), os.path.join(...), f-strings with literal segments, and the parenthesised right-hand pair — and matches .emrg, sessions, then anything. The negative direction is pinned by its own parametrised test (cwd / ".emrg" / "sessions", os.path.join(cwd, ".emrg", "sessions"), and a non-path call that merely takes those strings), so a matcher that flagged the sessions root would redden rather than teach readers to ignore the guard. That is the direction these guards usually get wrong.
The pair was measured, not assumed. The file lists do not overlap — this PR is tests/test_session.py, #2496 is five other files — but this guard's subject is tree-wide and #2496 edits emrg/session.py, which is exactly what such a guard scans. So I merged the two heads together on top of bcc6ac8b in a detached worktree and ran both subjects: 131 passed. Landing this cannot redden #2496's guard, and #2496 cannot redden this one.
Not claimed: #2496's own CI is pending on its head, and nothing here is evidence about it. That trial merge is a local measurement on this checkout, not a CI verdict for the pair.
Closes #2473
The guard added for this issue reads the syntax tree, and that part is right — but it matches one joining operator.
_spells_a_session_directorywalkedast.BinOpwithop=Divand flattened only left-hand operands, so it saw a/chain and nothing else. Five spellings of the same four components came back clean:cwd / f".emrg/sessions/{sid}"cwd.joinpath(".emrg", "sessions", sid)os.path.join(cwd, ".emrg", "sessions", sid)Path(cwd, ".emrg", "sessions", sid)(cwd / ".emrg") / ("sessions" / sid)The last one is not another author's taste: it is the same
/chain with its right-hand pair parenthesised, and it dropped out because the flattener walked left-handDivnodes only, so"sessions" / sidlanded where the"sessions"constant was expected. An instrument that reports zero offenders over a tree containing one teaches its reader to stop reading it, and every later "the guard reads clean" inherits the under-count.What changed
_path_componentsnormalises any expression to its ordered components — a literal segment, or_ANYfor anything this scan cannot resolve — and the match is on the sequence.emrg,sessions, then anything. It understands/in any grouping,Path(...)/PurePath(...)/Pathas a method,<x>.joinpath(...),os.path.join(...), and an f-string with literal segments._slash_chainis gone, because walking one operator was the defect.Two behaviours are deliberate:
cwd / ".emrg" / "sessions"names the directory sessions live in, which several callers may legitimately ask for. A guard that flagged it would report a clean tree as dirty — the same failure in the other direction.How it was measured
Four controls, each run rather than asserted:
0cccf946, before the one-producer change): old predicate 15 entries / 11 distinct lines, new 11 entries / 11 distinct lines,OLD-only: [],NEW-only: []. No copy the old one found is lost; the entry difference is only the duplicate lines.9f5ca50d): producer 1 (session.py:357), offenders 0 — unchanged..emrg/history.jsonl,.emrg/sessions.json, a call that is not a path, and a bare name.Three mutation arms on the real tree, each restored byte-identically afterwards (sha256 compared):
os.path.join(cwd, ".emrg", "sessions", sid)appended toemrg/client/app.py<cwd>/.emrg/sessions/<id>themselves"(cwd / ".emrg") / ("sessions" / sid)appended toemrg/server/daemon.pyPath(cwd) / f".emrg/sessions/{sid}"inside a function inemrg/session.pyThe third arm was run twice on purpose: a module-level version of it failed at import (rc 2), which looks like a kill and proves nothing. Moving it inside a function body is what makes it a reading about the guard.
Verification
uv run pytest tests/— 5166 passed, 508 skippeduv run python -c "from emrg.client.app import run_client"— importsuv run python -m emrg --help— exits 0CI on this head is pending.