Skip to content

emrg: the guard matches the rule, not the operator that wrote it - #2494

Merged
argszero merged 1 commit into
masterfrom
fix/the-guard-matches-the-components
Oct 22, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/the-guard-matches-the-components

Conversation

@pm25coder

Copy link
Copy Markdown
Collaborator

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_directory walked ast.BinOp with op=Div and flattened only left-hand operands, so it saw a / chain and nothing else. Five spellings of the same four components came back clean:

Spelling Before
cwd / f".emrg/sessions/{sid}" not flagged
cwd.joinpath(".emrg", "sessions", sid) not flagged
os.path.join(cwd, ".emrg", "sessions", sid) not flagged
Path(cwd, ".emrg", "sessions", sid) not flagged
(cwd / ".emrg") / ("sessions" / sid) not flagged

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-hand Div nodes only, so "sessions" / sid landed 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_components normalises any expression to its ordered components — a literal segment, or _ANY for anything this scan cannot resolve — and the match is on the sequence .emrg, sessions, then anything. It understands / in any grouping, Path(...) / PurePath(...) / Path as a method, <x>.joinpath(...), os.path.join(...), and an f-string with literal segments. _slash_chain is gone, because walking one operator was the defect.

Two behaviours are deliberate:

  • A line is reported once. Several nested expressions on one line are one site; the old predicate listed three lines twice.
  • The id-less root stays quiet. 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:

  • Same tree, two predicates (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.
  • This tree (9f5ca50d): producer 1 (session.py:357), offenders 0 — unchanged.
  • The spellings: nine must-be-flagged forms, all nine flagged, including the five above.
  • Non-copies that must stay quiet: seven, all quiet — the root in three spellings, .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):

Arm Result
os.path.join(cwd, ".emrg", "sessions", sid) appended to emrg/client/app.py KILLED — rc 1, "these build <cwd>/.emrg/sessions/<id> themselves"
(cwd / ".emrg") / ("sessions" / sid) appended to emrg/server/daemon.py KILLED — rc 1
Path(cwd) / f".emrg/sessions/{sid}" inside a function in emrg/session.py KILLED — rc 1, and the module still imports, so the failure is the guard's rather than an import error
untouched tree (control) passed

The 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 skipped
  • uv run python -c "from emrg.client.app import run_client" — imports
  • uv run python -m emrg --help — exits 0

CI on this head is pending.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ 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.

@argszero
argszero merged commit 16ab710 into master Oct 22, 2026
2 checks passed
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.

A session's directory has ten spellings and one rule

2 participants