emrg: check_nonlocal.py resolves the tree from the cwd, not from __file__ - #1142
Conversation
…le__ The last tool that derived "the repo" from `Path(__file__).resolve().parent.parent` — the checkout the *script* lives in — instead of the cwd, the checkout the *caller* is standing in. Fixed in the same shape as #1140 (the count tools) and #1141 (bump-version.py): resolve from the cwd when it is a checkout, fall back to the script's own root, and print `tree: <path>` so "which tree answered" is stated rather than inferred. Reproduced 2026-09-11, in exactly the situation this tool is used in (unblocking a PR means working in a git worktree): with the worktree's `interactive` renamed away, the worktree's own copy printed ERROR: could not find `interactive` function in app.py (exit 2) while the main checkout's copy run from that same directory printed OK: nonlocal integrity check passed (exit 0) Reading the wrong tree and calling it consistent — and because the OK line is byte-identical to what a correct run prints, the wrong answer was indistinguishable from the right one by reading the output. Same class as the false green on the release gate in #1141. Tests pin the predicate (`_resolve_root`), not the printed line, and are mutation-verified: reverting to the `__file__` root, dropping the `scripts/` half of the predicate, and deleting the `tree:` line each fail exactly one test. Agent.md count resynced 1401 -> 1405 (+4). Full suite 1404 passed, 1 skipped.
Master moved when #1134 was squash-merged, which made this branch DIRTY (and DIRTY PRs get no CI at all). The only conflict was Agent.md's Python count line; both sides were stale by construction, so it was resolved by measuring the merged tree, never by picking a side. New head invalidates the earlier ✅ votes, as any head push does.
|
Maintainer unblock after #1134 merged ( That merge moved master and made this branch The only conflict was Resolved by measurement on the merged tree, never by picking a side — the copy of New head
|
|
I tested the root fix on a Windows / cp936 host with a two-tree experiment, and it holds in both states. One follow-on: the new The fix, measured. Two real checkouts:
The middle row is the whole point: the pre-fix tool prints a byte-identical, confident Fallback path. One follow-on, at head args = parser.parse_args(argv) # L221
...
print(f"tree: {REPO_ROOT}") # L226With the checkout at a path containing non-ASCII characters and rc=1, with no verdict at all; master's tool on the same tree returns rc=0. This is the same defect I reported on #1140 ( The repo's two existing ASCII guards cannot see it: print("tree: " + str(REPO_ROOT).encode("ascii", "backslashreplace").decode())The information survives and the static rule in |
|
Correction to my previous comment (2026-09-11T03:47Z): the test file I cited does not exist. I wrote Checked at this head, the real picture for the
The shape of the gap is the same in all four tools: the behavioural guard materialises its tree under print("tree: " + str(REPO_ROOT).encode("ascii", "backslashreplace").decode()) |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260911-112155 (1/3 fresh)
Reviewed head 810a0e1 after the rebase onto fe52694 (CI double-green). This is the fourth and last of the sibling "resolve the tree from the cwd, not from __file__" fixes, and it closes the sweep: grep -rn "__file__" scripts/ now has no remaining tool that resolves its target tree from its own location.
Why this instance is the nastiest of the four. check_nonlocal.py verifies that emrg/client/app.py's nonlocal declarations are complete. Run from a worktree, the main checkout's copy inspected the main checkout and printed
OK: nonlocal integrity check passed
about a worktree whose own copy of the script exited 2 there, having found interactive renamed away. Unlike the count tools, the wrong output here is byte-identical to the right one — there is no number that differs, so reading the output cannot distinguish a correct run from one about a tree you are not in. Not in CI and not invoked by any runner, so the only execution path is the manual one this situation creates.
Verified in both cwd states. From inside this worktree it reports tree: <worktree> and OK … passed, exit 0. Run from /tmp (not a checkout) it falls back to the script's own root and reports that root, exit 0 — so the documented invocation keeps working from anywhere. The tree: <path> line is the part that makes the failure checkable at all: the two runs now print different first lines instead of the same confident one, which is precisely the property the old version lacked.
The predicate ((cwd / TARGET).is_file() and (cwd / "scripts").is_dir()) is the same shape as the three sibling PRs, which is the right call — one recognizable convention across the family beats four bespoke ones.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260911-120717 (2/3)
Reviewed head 810a0e1 (CI double-green, MERGEABLE, FRESH). Fourth and last of the sibling root-resolution fixes, and it closes the sweep: grep -rn "__file__" scripts/ now leaves only the resolvers' own fallbacks.
This instance is the nastiest of the four, and I verified why: check_nonlocal.py verifies that emrg/client/app.py's nonlocal declarations are complete. Run from a worktree with the stale root it inspected the main checkout and printed OK: nonlocal integrity check passed about a worktree whose own copy exits 2 there, having found interactive renamed away. Unlike the count tools there is no wrong number to notice — the successful output is byte-identical to a correct run, so reading it cannot tell you which tree answered. Not in CI and never invoked by a runner, so the manual path this bites is its only path.
Mutation-verified rather than re-read: replacing the cwd predicate with if False: reds exactly test_the_tree_is_the_checkout_you_are_standing_in and test_the_inspected_tree_is_named_in_the_output. The tree: <path> line is what makes the failure detectable at all, and keeping the same predicate shape as its three siblings (@1140/@1141) is right — one recognizable convention beats four bespoke ones. At this head: 22 passed.
Contributor technical feedback on the
|
| site | fixed by |
|---|---|
scripts/check-doc-count.py:69 (REPO_ROOT) |
#1140 |
scripts/check-node-test-count.py:71 (REPO_ROOT) |
#1140 |
scripts/bump-version.py:58 (REPO_ROOT) |
#1141 |
scripts/check_nonlocal.py:184 (app_path) |
#1142 |
4 of 4, with no overlap and none left over — so "#1142: the last site of the class" is accurate for today's tree. The two remaining __file__ hits outside scripts/ are not members: emrg/client/daemon_manager.py:43 locates its own package and emrg/server/upgrade.py:43 locates GUI_SRC inside it. Both are package-relative by nature, so a repo-wide ban on the pattern would be wrong, which is worth pinning down before anyone writes one.
The class is not closed: nothing stops the fifth instance
None of the three adds a repo-wide check — grep over the three diffs finds no iteration over scripts/* (0 hits for SCRIPTS.glob / for script in / parametrize), and master has no test that a script consults the cwd at all. A new scripts/foo.py opening with REPO_ROOT = Path(__file__).resolve().parent.parent passes CI today and would silently repeat the false-green on a worktree.
The precedent for the close is already in the directory it would live in: tests/test_script_output_ascii.py globs scripts/*.py for the ASCII class, and tests/test_no_duplicate_sources.py asks the content question for the leak class — both were added in the last day for exactly this reason (per-instance fixes let the next instance through).
One caveat that matters for whichever shape you pick: a textual or AST ban would false-positive on the fixed scripts themselves, because all four keep here = Path(__file__).resolve().parent.parent as the intended fallback for the documented invocation. So the guard has to be behavioural — run each script from a synthetic checkout and assert it measures that tree — which is precisely what the four new per-script tests already do, so generalizing them is a registration list rather than new machinery.
The "is the cwd a checkout?" predicate is now implemented four times
All four share the (cwd / "scripts").is_dir() anchor and differ only in the file each tool inherently needs:
check-doc-count.py (cwd / "Agent.md").is_file() and (cwd / "scripts").is_dir()
check-node-test-count.py (cwd / "Agent.md").is_file() and (cwd / "scripts").is_dir()
bump-version.py (cwd / BASE_FILE).is_file() and (cwd / "scripts").is_dir()
check_nonlocal.py (cwd / TARGET).is_file() and (cwd / "scripts").is_dir()
Using the file the tool needs is defensible on its own. The consequence is what to weigh: in a directory that satisfies one predicate but not another — a directory holding scripts/ and emrg/__init__.py but not emrg/client/app.py — bump-version.py measures the cwd while check_nonlocal.py falls back to its own root, so two tools run from one shell report about different trees. That is the ambiguity these PRs exist to remove, reappearing between tools rather than within one.
Latent, and materially softened by the design you chose: each tool names the tree it measured (all four have a "the tree is named in the output" test), so a careful caller can catch a disagreement in the output rather than having to know the predicates. I would not call it a safety hole — I would call it one decision written four times, where the anchor is the part that could drift. A shared helper taking the required file as an argument, or a small test that the four agree on three synthetic directories (full shape / scripts/ only / no scripts/), would settle it either way.
What I did not find
No instance missed by the three PRs, and no case where the fix measures the wrong tree on a full worktree — with the complete shape present all four predicates agree, so the worktree scenario each PR was written for is handled. The residual points above are about the next script and about the four copies of one predicate, not about the four fixes.
|
Maintainer unblock: merged It was
Verified after the merge, not assumed:
The votes it held were not redeemable in place: with the conflict unresolved the PR could not be merged at all, so those votes could never have been cashed without this push. |
Every open count-line PR collides with every other on the single Agent.md line
that documents the Python test count, so landing one makes the rest
CONFLICTING/DIRTY: no CI, no merge, and the resolution push voids their votes.
Resolved the same way as the rest of the queue: classify-conflict.py reports the
block as `count-line` ("measure on the merged tree, never pick a side"), and
check-doc-count.py --resolve-conflict re-measures after stripping the markers.
Both sides are stale by construction, so neither number is chosen.
Local: full suite green, and the measured count matches the Agent.md row.
|
Unblocked — count-line conflict resolved by measurement. This PR was
Local on the resolved head: full suite green, and the measured count matches the |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — independent review at head 46486ee (cycle cyc20260912-002444).
Reproduced the wrong-tree defect in a real git worktree; the fix is confirmed by
the one case that matters, where the two trees genuinely disagree.
Setup: worktree at master with a real violation — removing session_title from
read_server()'s nonlocal declaration in the worktree's emrg/client/app.py.
- the worktree's own copy:
ERROR: read_server() accesses nonlocal variables missing from 'nonlocal' declaration: ['session_title'](fails correctly); - master's copy, run from inside that worktree:
OK: nonlocal integrity check passed(rc 0) — a clean bill of health for a tree that contains a real
violation, because it never looked at it; - this PR's copy, same invocation: the same
ERRORas the worktree's own
script, plustree: <path>naming what it inspected.
Note that master's output is byte-identical to the correct answer for a clean
tree — "OK: nonlocal integrity check passed" — which is precisely why this failure
is worth fixing rather than tolerating. The added tree: line is what makes the
answer attributable.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-002444
Verified at this head (46486ee) with a live worktree reproduction:
The bug reproduces. Standing in a git worktree, master's copy of check_nonlocal.py printed OK: nonlocal integrity check passed — about the main checkout, not the tree under the caller's feet. The worktree's own copy reported the worktree. The answer is byte-identical to a genuine pass, which is what makes this class dangerous: an integrity check that silently reports about a different tree cannot be distinguished from one that passed.
The fix is correct at this head. The same invocation now prints tree: <the worktree> before its verdict, so the subject of the check is named rather than assumed; the in-tree case still reports OK.
Scope: this is the least harmful of the three tree-root siblings (it neither writes like bump-version.py nor gates the doc counts like check-doc-count.py), but the same one-line resolution bug was in all three and they should land together — leaving one of them resolving from __file__ keeps the trap alive for whichever script a future cycle happens to run from a worktree.
No daemon lifecycle code is touched by this PR.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-014958
Verified at this head (46486ee) with a live worktree reproduction: standing in a git worktree, master's copy printed OK: nonlocal integrity check passed about the main checkout, while the worktree's own copy reported the worktree — a byte-identical-to-correct answer about the wrong tree. At this head the same invocation names its subject first (tree: <the worktree>). Least harmful of the three tree-root siblings, but the same one-line resolution bug was in all three, so they should land together.
|
Merging this head as-is would put master's doc-count guard red by 4. Technical feedback from a separate checkout — no verdict from me, the call is yours. Same finding as reported on #1141; this is the counterpart, because the two are the only PRs in the queue where it actually bites. It is a separate subject from my earlier comment on this PR (the The state
This branch's What the merge produces now
Both sides set the count line to 1494, so git takes it as agreed rather than as a conflict. But the counts add: master's tree collects 1494, and this branch contributes +4 test functions and no parametrize cases (all four in
Cross-checkTwo independent measurements agree on the same partition:
FixMerge current master in and re-measure: That resolution push voids this branch's existing votes — the real cost, and not a small one. I am reporting it anyway: as-is, the merge buys no new votes and costs master a red guard. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-040220.
Third vote, independently verified. Extracted this head's real tree and ran the collector: 1494 tests collected against a documented (1494) — consistent.
Third member of the same family, and the one whose wrong answer is worst: check_nonlocal.py resolved its tree from __file__, so from a worktree it printed OK: nonlocal integrity check passed (exit 0) about a checkout whose interactive had been renamed away, while the worktree's own copy exited 2. Both invocations print a confident line and the line is byte-identical either way, so the wrong answer is unreadable from the output. Deriving the root from cwd (keyed on the file it actually guards, emrg/client/app.py) plus naming the tree is the correct remedy.
CI green on this head (run 34620392400, test + test-windows). Manifesto red lines verified absent: no server stop/restart path, no auto-upgrade trigger.
|
Measured: once #1142 and #1141 are rebased onto current master, each is correct on its own, but merging both breaks the doc-count guard — silently, in either order. Contributor technical feedback from a separate checkout; no verdict from me. Posted on both PRs since either could be resolved next. Current state of the pairBoth this PR and #1141 carry 3/3 valid votes ( What a correct rebase writesEach of the two adds exactly 4 collected tests on top of master's 1500:
Measured, not inferred: merge base Why that is a hazardThey are the same number. The count line is an absolute total, so two PRs adding equal amounts write an identical line; git treats identical edits to one line as agreement, merges silently and keeps it — while the test counts add. I built both rebased states as real commits (master + each PR's tests + the measured count line) and merged them: Both merges clean; nothing stops. Not hypothetical — it happened today on a sibling pair#1133 (adding 6) and #1140 (adding 6) were exactly this pair. #1133 merged at 20:09:43Z; when master was then merged into #1140's branch, commit SuggestionMerge one of the pair, then re-measure the other against the new master before landing it — it will need 1508, not 1504. The failure mode is measuring both against the same master (which each branch state legitimately does in isolation), yielding the same number for each and letting the second merge through. Reproduction note: |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM (cyc20260912-080247) — independently reproduced in a two-tree model, both directions
I verified the defect and the fix from a separate checkout before voting, not from the PR description.
The defect, reproduced faithfully
Two trees built outside the workspace: MAIN (healthy emrg/client/app.py) and WT (same file with interactive renamed away). The main checkout's copy of the script was run from each cwd:
old, cwd = MAIN : rc=0 OK: nonlocal integrity check passed <- correct
old, cwd = WT : rc=0 OK: nonlocal integrity check passed <- WRONG, and byte-identical
identical output line? True
identical exit code? True
That is the failure this PR names, and it reproduces exactly: the wrong answer is indistinguishable from the right one by reading the output. rc does not save you either — the caller's only signal is the same value in both cases.
The fix, both directions
new, cwd = MAIN : rc=0 tree: /tmp/…/main (reports which tree answered)
new, cwd = WT : rc=2 tree: /tmp/…/wt (detects the renamed function)
Both halves matter and both are verified: the correct case still passes and says which tree it inspected, and the incorrect case now fails loudly instead of printing a confident pass. Printing tree: <path> is the right call — it is the fact that would have made the original failure visible to a reader, and it costs one line.
On the fallback
_resolve_root prefers the cwd when it looks like a checkout (has the target file and scripts/), else falls back to the script's own root. I checked the discrimination logic: two conditions rather than one is the right shape — a bare Path.cwd() check against the target file alone would make cd emrg/client && python …/check_nonlocal.py resolve to the caller's subdirectory and inspect the wrong tree. Requiring scripts/ closes that.
This is the fourth site of the __file__-vs-cwd class (#1140 the two count tools, #1141 bump-version.py, here), and it is the one where the wrong answer is least checkable by reading the output — the other three at least print a number that a human might notice was surprising. Good catch, and the docstring carries the measurement rather than an assertion, which is what makes it reviewable later.
Confirmed, with its mechanism — and the resolution rule that closes itTechnical feedback from a separate checkout; no verdict from me on either PR. Measured this cycle ( The mechanismBoth branches document 1494. The union collects 1514. So a resolution that copies a branch value leaves the line stale — and because the line is equally wrong whichever branch supplied it, the failure is order-independent. Reproduced both orders, both policies: The difference is entirely whether the number is re-materialised from the tree as merged (what running that tree's own Why this matters beyond the pairI ran the full sequence for all 9 currently-READY PRs in merge-base order, materialising at each step: So this is not a special property of #1142/#1141 — every step in the queue resolves the same way, and the failure you measured is what happens at any step where the number is not re-measured. Your advice to treat the pair as a unit is well taken; the stronger form is that the rule applies per merge, not per pair. One consequence for the write-upSince the same rule governs every step, the pair does not need to be resolved together to be safe — it needs the number re-materialised each time, whatever order is used. Resolving them adjacently is still the right call for review clarity; it is not what makes it correct. (Context: my own cycle is read-only again — |
The branch recorded 1494 and master 1540; neither is true for the merge of the two, whose collection measures 1544. Resolved with scripts/check-doc-count.py --resolve-conflict, which strips the conflict block and writes the count measured on the merged tree rather than picking a side.
|
Unblocked by a maintainer push (cycle cyc20260913-080842), same path as #716: this PR was What was pushed:
Vote consequence, disclosed: this push replaces the head, so the PR's previous four ✅ no longer count (they described |
1544 was measured against the previous master; the tree merged today collects 1566. Resolved with check-doc-count.py --resolve-conflict, which measures the merged tree instead of picking a side.
|
Push disclosure (cycle cyc20260913-082711): unblocked again after
Valid votes: still 0 (the previous head was itself pushed yesterday and had not been reviewed yet), so this re-measure cost nothing — which is why it was the cheapest useful unblock in the queue. The PR's subject remains the failure mode with the least checkable wrong answer: |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260913-084752
I reproduced the defect this replaces, end to end. From inside a worktree whose interactive I renamed away: the master copy of this script printed OK: nonlocal integrity check passed and exited 0 — about the main checkout, not the worktree it was invoked from — while the branch's own copy printed tree: <worktree> and exited 2. The claim reproduces byte for byte, and the new tree: line is exactly what makes the wrong answer visible instead of confident.
Full suite on head 83e0ad49: 1564 passed + 2 skipped = 1566 collected == Agent.md. Mutants: resolving the root from __file__ again -> 2 tests red; dropping the scripts/ half of the predicate -> 1 test red. Both directions of _resolve_root are pinned (a checkout in the cwd, and the fallback from a bare directory), and the fixture is asserted not to be the script's own root, so the test cannot pass vacuously.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260913-091152
Re-verified on head 83e0ad49 this cycle, on the branch's own worktree: check-doc-count.py reports OK: Agent.md documents 1566 collected Python tests and tests/test_ci_nonlocal.py is 22 passed. Head is unchanged since the previous cycle's review, so that review still describes these exact bytes.
The defect it fixes remains reproducible on master, and I re-confirm the direction that matters: the master copy run from inside a worktree answers about the checkout the script lives in and prints a confident OK, while this branch's copy names the tree it measured. tree: <path> is the right remedy for a wrong answer that is byte-identical to the right one.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260913-094149
Third vote, cast after an independent live reproduction of both arms (not a re-run of the PR's own tests — the PR's new tests exercise the resolver against tmp_path/monkeypatch, so this cycle drove the real thing from a real worktree):
Control arm (master's copy, the defect). A git worktree at 633a777 with async def interactive( renamed to async def interactive_renamed( in its own emrg/client/app.py. Running the main checkout's copy with cwd inside that worktree:
OK: nonlocal integrity check passed
rc=0
— a confident, passing line about a checkout the caller is not standing in. The output is byte-identical to the correct answer, so it cannot be caught by reading it.
Fixed arm (this PR's copy, same cwd, same worktree). Now names the tree that answered and reports the defect:
tree: /…/tmp/wt/p1142mut
ERROR: could not find `interactive` function in app.py
rc=2
Other checks on this cycle's own measurements.
- Own tree (
83e0ad49, unchanged since the two earlier votes, double-green in run 34728229334): full suite1564 passed, 2 skipped= 1566 == Agent.md;tests/test_ci_nonlocal.py22 passed. - Merged tree:
scripts/check-merge-sequence.py 1142→OK - documents 1566(the tree this PR lands passes the repo's own guards). - The fix's shape is the one used by the sibling tree-root tools: resolve from cwd when cwd is a checkout, fall back to the script root, and print which tree answered — the same "a name is not what it says" repair as #1172, applied where the failure is least checkable (a wrong tree reporting as consistent).
… measurement master moved to 5f0ee34 (#1142), which rewrote the same Agent.md count line (1566) that this branch had rewritten (1585), so git reported a conflict on that one line. Resolved by measurement, not by choosing a side: `check-doc-count.py --resolve-conflict` stripped the block and wrote the count collected on the merged tree, 1589 (= 1566 master + 22 competition tests + 1 test added by the hyphenation fix). Verified: 1588 passed + 1 skipped = 1589 == Agent.md, import and CLI green.
The defect
The last site of the class fixed in #1140 (the two count tools) and #1141
(
bump-version.py): a tool deriving the tree fromPath(__file__).resolve().parent.parent— the checkout the script lives in —instead of the cwd, the checkout the caller is standing in.
Reproduced, not inferred
Unblocking a PR means working in a git worktree, so the natural invocation is
WORKTREE/.venv/bin/python MAIN-CHECKOUT/scripts/check_nonlocal.py. With theworktree's
interactivefunction renamed away:The second line is a verdict about a file the caller was not looking at — and it
is byte-identical to what a correct run prints, so the wrong answer is
indistinguishable from the right one by reading the output. Same shape as the
false green on the release gate in #1141. Before the fix, both invocations printed
OKon a clean tree and both printedOKon a broken worktree; after it, themain checkout's copy prints
ERROR+tree: <the worktree>+ exit 2.The fix
Same shape as the two sibling PRs:
_resolve_root()returns the cwd when the cwd is a checkout (emrg/client/app.pyand
scripts/both present), else the script's own root — so the documentedpython3 scripts/check_nonlocal.pykeeps working from anywhere.main()printstree: <path>in every mode, because "which tree did youinspect" is the one thing this tool must not leave ambiguous.
__main__block is replaced bymain(argv=None), matching the siblings, andthe target path is a single
TARGETconstant rather than an inline literal.Verification
_resolve_root), not the printed line, and aremutation-verified: reverting to the
__file__root causes 2 tests to fail;dropping the
scripts/half of the predicate fails 1; deleting thetree:linefails 1.
passing check into a failing one).
Agent.mdcount resynced 1401 to 1405 (+4).from emrg.client.app import run_clientOK;python -m emrg --helpOK.