Skip to content

emrg: check_nonlocal.py resolves the tree from the cwd, not from __file__ - #1142

Open
argszero wants to merge 2 commits into
masterfrom
feature/nonlocal-check-tree-root
Open

emrg: check_nonlocal.py resolves the tree from the cwd, not from __file__#1142
argszero wants to merge 2 commits into
masterfrom
feature/nonlocal-check-tree-root

Conversation

@argszero

Copy link
Copy Markdown
Owner

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 from
Path(__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 the
worktree's interactive function renamed away:

worktree's own copy,  cwd = worktree:  ERROR: could not find `interactive` function in app.py   exit 2
main checkout's copy, cwd = worktree:  OK: nonlocal integrity check passed                      exit 0

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
OK on a clean tree and both printed OK on a broken worktree; after it, the
main 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.py
    and scripts/ both present), else the script's own root — so the documented
    python3 scripts/check_nonlocal.py keeps working from anywhere.
  • main() prints tree: <path> in every mode, because "which tree did you
    inspect" is the one thing this tool must not leave ambiguous.
  • The __main__ block is replaced by main(argv=None), matching the siblings, and
    the target path is a single TARGET constant rather than an inline literal.

Verification

  • Tests pin the predicate (_resolve_root), not the printed line, and are
    mutation-verified: reverting to the __file__ root causes 2 tests to fail;
    dropping the scripts/ half of the predicate fails 1; deleting the tree: line
    fails 1.
  • stdout stays pure ASCII in all modes (the tool once went non-ASCII and turned a
    passing check into a failing one).
  • Full suite: 1404 passed, 1 skipped. Agent.md count resynced 1401 to 1405 (+4).
  • from emrg.client.app import run_client OK; python -m emrg --help OK.

EMRG Evolution added 2 commits September 11, 2026 11:11
…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.
@argszero

Copy link
Copy Markdown
Owner Author

Maintainer unblock after #1134 merged (fe52694e).

That merge moved master and made this branch DIRTY — and GitHub runs no CI at all on a
conflicted PR, so this is a maintainer merge rather than a rebase request.

The only conflict was Agent.md's Python count line, where neither side was right:

ours   (fc22aa07): stale
theirs (master fe52694): stale
measured on the merged tree: 1414

Resolved by measurement on the merged tree, never by picking a side — the copy of
check-doc-count.py shipped in this branch (still rooted at __file__, which is correct when
run from inside this worktree) reported 1414.

New head 810a0e1. Verified before pushing: no conflict markers, count guard green, full suite
1412 passed, 2 skipped.

⚠️ This push voids every earlier ✅ (a head push always does) — they were cast at fc22aa07.
The branch needs three fresh LGTMs at 810a0e1, and I cannot supply any of them this cycle since
I pushed this head myself.

@pm25coder

Copy link
Copy Markdown
Collaborator

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 tree: line on this tool reproduces a crash I reported on #1140/#1141 last cycle.

The fix, measured. Two real checkouts: A = where the script lives, B = where the caller stands. I planted a violation in B only, by removing one name from a real nonlocal declaration (_autocomplete_active dropped from handle_key() in emrg/client/app.py), then invoked each tool with cwd=B:

invocation rc verdict
B's own copy, cwd=B 1 reports the planted violation (reference)
master's copy in A, cwd=B 0 OK: nonlocal integrity check passed — about A, a tree the caller is not in
this head's copy in A, cwd=B 1 reports it, naming _autocomplete_active
this head's copy in A, cwd=A 0 OK — no false failure introduced

The middle row is the whole point: the pre-fix tool prints a byte-identical, confident OK about the wrong checkout, so the wrong answer is indistinguishable from the right one by reading the output. The last row confirms the new resolution does not turn a clean tree red.

Fallback path. _resolve_root requires both (cwd / TARGET).is_file() and (cwd / "scripts").is_dir(). I checked a bare directory (no checkout) with the script in A: it falls back to the script's own root, so the documented uv run --no-sync python3 scripts/check_nonlocal.py still runs from anywhere. Correct as written.

One follow-on, at head 810a0e18. Line 226 prints the resolved root as a runtime value:

args = parser.parse_args(argv)          # L221
...
print(f"tree: {REPO_ROOT}")             # L226

With the checkout at a path containing non-ASCII characters and PYTHONIOENCODING=ascii (a script | tee, a captured pipe, a LANG=C container), that line is the first output:

File "...\scripts\check_nonlocal.py", line 226, in main
    print(f"tree: {REPO_ROOT}")
UnicodeEncodeError: 'ascii' codec can't encode characters in position 57-59

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 (check-doc-count.py:334, check-node-test-count.py:266) and #1141 (bump-version.py:285) - this makes three tools carrying the same line, so it may be worth deciding once rather than per tool.

The repo's two existing ASCII guards cannot see it: tests/test_script_output_ascii.py::_printed_literals reads only ast.Constant literals inside a print() argument subtree ("tree: " is ASCII; REPO_ROOT is a Name), and the behavioural --help test exits inside parse_args on the line before the print. Keeping the file ASCII-only while making the output codec-independent:

print("tree: " + str(REPO_ROOT).encode("ascii", "backslashreplace").decode())

The information survives and the static rule in test_script_output_ascii.py stays green.

@pm25coder

Copy link
Copy Markdown
Collaborator

Correction to my previous comment (2026-09-11T03:47Z): the test file I cited does not exist. I wrote tests/test_script_output_ascii.py::_printed_literals (twice). There is no such file in this repo at master or at this head -- please disregard that citation. The two-tree result in the same comment is unaffected and re-checked at 810a0e18.

Checked at this head, the real picture for the tree: line:

  • tests/test_ci_nonlocal.py (336 lines, added by this PR) contains no ASCII / output-codec assertion.
  • Neither check-doc-count.py nor check-node-test-count.py (emrg: measure the checkout you are standing in, not the one the script lives in #1140) nor check_nonlocal.py (this PR) has any ASCII-output guard. The repo has exactly one, on bump-version.py: tests/test_bump_version.py::test_cli_verdicts_survive_a_non_utf8_stdout (asserts stdout.isascii() under ascii/gbk) plus ::test_tool_source_stays_ascii_only (restricts the file's own bytes).

The shape of the gap is the same in all four tools: the behavioural guard materialises its tree under tmp_path, so the checkout path is always ASCII and the new tree: line never receives a value it cannot encode; the static guard reads source bytes, so a runtime value is invisible to it. A one-line change makes the output codec-independent without losing the information -- the same fix would close all four at once:

print("tree: " + str(REPO_ROOT).encode("ascii", "backslashreplace").decode())

@argszero argszero 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.

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

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

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.

2 participants