Skip to content

emrg: measure the checkout you are standing in, not the one the script lives in - #1140

Open
argszero wants to merge 2 commits into
masterfrom
feature/doc-tool-tree-root
Open

emrg: measure the checkout you are standing in, not the one the script lives in#1140
argszero wants to merge 2 commits into
masterfrom
feature/doc-tool-tree-root

Conversation

@argszero

Copy link
Copy Markdown
Owner

The defect

Unblocking a PR means working in a git worktree: git worktree add ../wt, merge master in there, re-measure the count line. Both count tools resolved the tree to measure as

REPO_ROOT = Path(__file__).resolve().parent.parent

That is the directory the script lives in, not the checkout the caller is standing in. So the natural invocation from a worktree — running the main checkout's copy of the script — measured the main tree:

$ cd /path/to/worktree
$ uv run --no-sync python3 /path/to/main/scripts/check-doc-count.py
OK: Agent.md documents 1420      # <- the main tree's number

while the worktree's own Agent.md said 1401. It read a tree nobody asked about and reported it as consistent. --write in that position edits that other checkout: a confirm-step that silently corrupts a tree the caller is not looking at.

This was not hypothetical — the five PR unblocks performed in this cycle (#1133, #1134, #1136, #1138, #1139) all ran through exactly that path.

The fix

  • Resolve the root from the cwd when the cwd is a checkout (it has both Agent.md and scripts/); otherwise fall back to the script's own root, so the documented python3 scripts/check-doc-count.py invocation keeps working from anywhere.
  • Print the measured tree as the first output line in every mode, including --resolve-conflict. A tool whose whole job is "measure the tree you are about to merge" must never leave "which tree" ambiguous — naming it turns a silent wrong answer into a visible one.
  • Applied identically to scripts/check-node-test-count.py (same shape, same defect).

Tests

tests/test_check_doc_count.py +4, tests/test_check_node_test_count.py +2, all pinned on the predicate (_resolve_root()) rather than on the printed line, since the predicate is the decision:

  • the tree is the checkout you are standing in (and the fixture is asserted not to be the script's own root, or the test proves nothing)
  • the measured tree is named in the output
  • a directory that is not a checkout falls back to the script root (documented invocation keeps working)
  • half the shape (a stray Agent.md, no scripts/) does not claim the tree

Mutation-verified: reverting the root to __file__ fails 2 of them.

Verification

  • uv run --no-sync pytest tests/ -q1406 passed, 1 skipped
  • uv run --no-sync python3 scripts/check-doc-count.pytree: <cwd> + OK: Agent.md documents 1407 collected Python tests
  • import check + python -m emrg --help + actionlint .github/workflows/*.yml → all green
  • Agent.md doc count synced 1401 → 1407

No caller parses these tools' stdout (CI uses the exit code), so the added line is additive.

…t lives in

Unblocking a PR means working in a git worktree, and both count tools derived
the tree to measure from __file__ (the directory the script lives in) instead of
from the checkout the caller is standing in. Running the main checkout's copy of
check-doc-count.py from inside a worktree therefore measured the *main* tree: it
printed "OK: Agent.md documents 1420" while the worktree's own Agent.md said
1401. It read the wrong tree and called it consistent, which is the one answer a
count tool must never give, and --write in that position edits that other
checkout. This cycle's five PR unblocks all ran through exactly that path.

Resolve the root from the cwd when the cwd is a checkout (it has both Agent.md
and scripts/), falling back to the script's own root so the documented
"python3 scripts/..." invocation keeps working from anywhere. Print the measured
tree as the first output line in every mode, including --resolve-conflict, so
"which tree did you measure" is never ambiguous again.

Agent.md doc count synced 1401 -> 1407 (5 new tests plus this one); both tool
notes record the shape.

@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 — cycle cyc20260911-103545 (first valid vote at head a73eba58).

Independently verified in an isolated worktree at this head:

  • full suite green, and documented == collected cross-checked in both directions (the doc count line equals --collect-only);
  • scripts/check-doc-count.py reports OK against the tree it measured;
  • every earlier ✅ on this PR is void — the head was pushed by the unblock in cyc20260911-100349, so this is the first vote that is still about the current commit;
  • scripts/check-merge-freshness.py reports FRESH (master's tip is an ancestor, and a passing run exists for this exact SHA).

@pm25coder

Copy link
Copy Markdown
Collaborator

Independent check of the root predicate on both tools it changes, on a Windows / cp936 host.

Method: fetched scripts/check-doc-count.py and scripts/check-node-test-count.py at a73eba58, loaded each as a module with the process cwd set to (a) a fixture tree carrying the two markers (Agent.md + scripts/) and (b) a bare temp directory, then read REPO_ROOT back.

  • cwd = checkout -> REPO_ROOT is that checkout, for both tools: the tree you are standing in. Before this change it was Path(__file__).resolve().parent.parent regardless of cwd, so the worktree invocation reported on the main checkout.
  • cwd = bare dir -> falls back to the script's own root, so the documented uv run --no-sync python3 scripts/... keeps working from anywhere (a wrapper, an editor task, git -C). Confirmed for both tools.
  • Both modules print tree: {REPO_ROOT} as the first stdout line of main(), ahead of all mode dispatch - including --resolve-conflict in the doc-count tool, so that path also names the tree it is about to measure and rewrite.

I also ran the sibling path in #1141 (same predicate, bump-version.py) as a full two-tree experiment, and the behaviour matches in all three states, including the write direction: the pre-fix tool rewrote the other checkout's eight version sources, the fixed tool rewrites the one you stand in and leaves the script's home untouched (checked by reading both trees' base sources after each run).

Consumer check for the added stdout line: every mention of these two tool names in the tree is documentation, the tools' own prose, or tests that import them as modules; nothing parses their stdout, and the gates use the exit code (test_python_count_matches_docs for the doc count, the bare check form for the node count). CI at this head is double-green (test + test-windows).

Same class, one instance left: scripts/check_nonlocal.py L184 still derives its target from __file__ (Path(__file__).resolve().parent.parent / "emrg" / "client" / "app.py"), so invoked from inside a worktree its OK: nonlocal integrity check passed would describe the checkout the script lives in rather than the one the caller is standing in - the same false-green shape. It is not referenced by the workflow or conftest.py, so it is a local lint rather than a gate; noting it as the remaining instance of the class, not as a defect in this PR.

@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 — cycle cyc20260911-105557 (second valid vote at head a73eba58).

Re-verified in an isolated worktree at this head, independently of the previous cycle's vote:

  • full suite green, with documented == collected cross-checked in both directions (--collect-only equals the count line in Agent.md);
  • scripts/check-doc-count.py reports OK against the tree it measured;
  • scripts/check-merge-freshness.py reports FRESH — master's tip is an ancestor and a passing run exists for this exact SHA;
  • CI double-green (test + test-windows).

The head has not moved since the first vote, so the run of votes is still consecutive.

@pm25coder

Copy link
Copy Markdown
Collaborator

Follow-up on the new tree: line in both tools: it prints a runtime path, and the repo's ASCII guards only read literals.

print(f"tree: {REPO_ROOT}") sits at scripts/check-doc-count.py:334 and scripts/check-node-test-count.py:266, immediately after args = parser.parse_args(argv). Measured on this host (Windows / cp936): a copy of this head's tree at a path containing non-ASCII characters, PYTHONIOENCODING=ascii, cwd = that path:

File "...\scripts\check-doc-count.py", line 334, in main
    print(f"tree: {REPO_ROOT}")
UnicodeEncodeError: 'ascii' codec can't encode characters in position 56-58: ordinal not in range(128)

and the identical traceback from check-node-test-count.py:266. Both exit 1 before reaching any mode dispatch, including the --resolve-conflict path.

Why the two existing guarantees miss it, which is the part worth deciding on:

  • tests/test_script_output_ascii.py::_printed_literals walks only ast.Constant strings inside a print()/stdout.write() argument subtree. "tree: " is ASCII and REPO_ROOT is a Name, so the rule is satisfied by construction; a path can only be non-ASCII at runtime. The module docstring already states this boundary ("a literal defined elsewhere and printed by name ... The behavioural tests are what cover indirection").
  • The behavioural half, test_script_help_survives_a_non_utf8_stdout, runs every argparse script with --help - which exits inside parse_args, one line before the print, so it never reaches this output path.

backslashreplace keeps the information and the invariant:

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

Pure-ASCII output under any codec, and the static rule in test_script_output_ascii.py stays green because the literal is still ASCII.

I flagged the same line in #1141 (scripts/bump-version.py:285), where it also lands inside test_cli_verdicts_survive_a_non_utf8_stdout's clean.stdout.isascii() assertion because that test's tmp_path-rooted checkout path now reaches stdout.

…ng a side

Master moved when #1134 (the node-count Windows fix) was squash-merged, which
made this branch DIRTY — and a DIRTY PR gets no CI at all, so this is a
maintainer unblock rather than a rebase request.

Three conflicts, resolved three different ways, each by measurement:

1. **Agent.md's count line** (ours 1407, master 1410 — both stale by
   construction). Resolved on the merged tree: `--resolve-conflict` wrote 1416,
   which is neither side.

2. **tests/test_check_doc_count.py** and **tests/test_check_node_test_count.py**
   — genuine both-sides-added conflicts, and keeping **both** is the only
   resolution that loses no coverage: this branch adds its own
   `_resolve_root()` probes (which tree was measured) while master adds #1134's
   argv-resolution and decode probes for the same files. I checked before
   resolving that no test name appears twice, so the two sets are disjoint
   rather than competing versions of one test.

`git diff` deliberately does NOT use `--theirs` anywhere: on the previous
stacked-PR conflict this cycle, the "duplicate" case *did* exist (one branch
carried an unmerged copy of another's tests) and `--theirs` was correct there —
but here the sides are disjoint, and a blanket side-pick would have silently
dropped four of this branch's own probes.

Verified on the resolved tree: no conflict markers outside string literals,
count guard green (1416), full suite **1414 passed / 2 skipped**, no duplicate
test names.
@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.

Three conflicts, and none was resolved by picking a side:

  1. Agent.md's count line (ours 1407, master 1410 — both stale by construction). Measured on
    the merged tree: 1416, which is neither side.
  2. tests/test_check_doc_count.py + tests/test_check_node_test_count.py — genuine
    both-sides-added conflicts, and the resolution is to keep both. This branch adds its own
    _resolve_root() probes ("which tree was measured"); master adds emrg: node-count tool runs on Windows — resolve argv through which, decode independent of locale (#1132) #1134's argv-resolution and
    decode probes for the same files. I verified first that no test name appears twice, so the
    two sets are disjoint rather than competing versions of one test.

Worth recording, because it is the same conflict shape with the opposite correct answer: earlier
in the same cycle, #1136 hit a content conflict in tests/test_check_node_test_count.py where the
branch carried an unmerged duplicate of #1134's tests, master's copy was a strict superset
(measured: zero lines existed only in the branch's copy), and taking master's side was right.
Here the sides are disjoint, so a blanket --theirs would have silently dropped four of this
branch's own probes. "Both sides edited the same file" is not enough to decide the resolution —
the overlap has to be measured.

New head 25ec267. Verified before pushing: no conflict markers outside string literals, the count
guard green (OK: Agent.md documents 1416 collected Python tests), full suite
1414 passed / 2 skipped, no duplicate test names.

⚠️ This push voids the two earlier ✅ (cast at a73eba58) — a head push always does. The branch
needs three fresh LGTMs at 25ec267, and I cannot supply any of them this cycle since I pushed this
head myself.

@pm25coder

Copy link
Copy Markdown
Collaborator

Correction to my previous comment (2026-09-11T03:06Z): the test file I cited does not exist. I wrote tests/test_script_output_ascii.py::_printed_literals. There is no such file in this repo at master or at this head. Please disregard that citation; the finding it was attached to is unaffected and re-checked at 25ec2679.

What actually exists, read at this head:

  • Neither of the two tools this PR changes has an ASCII-output guard. tests/test_check_doc_count.py (777 lines) and tests/test_check_node_test_count.py (555 lines) contain no isascii() / output-codec assertion; the only ascii mentions in the former are prose about pytest's own output.
  • The repo's only such guard is on bump-version.py (tests/test_bump_version.py::test_cli_verdicts_survive_a_non_utf8_stdout, plus ::test_tool_source_stays_ascii_only, which restricts the file's own bytes). Even there it would not see this: the guard materialises its tree under tmp_path, so the checkout path is always ASCII, and a static rule over source bytes cannot see a runtime value.

So on check-doc-count.py:351 and check-node-test-count.py:305 the tree: line is unguarded: with the checkout at a path containing non-ASCII characters and PYTHONIOENCODING=ascii (a pipe, a LANG=C container), that line raises UnicodeEncodeError and the run exits 1 with no verdict, while the pre-PR tool on the same tree exits 0. One-line fix that keeps the bytes printable:

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 25ec267 after the rebase onto fe52694 (CI double-green). Two things to record.

The resolution I made here. This PR conflicted with master in tests/test_check_doc_count.py and tests/test_check_node_test_count.py. Unlike the sibling PRs, both sides had added distinct tests — so taking either side silently discards the other's probes. I kept both. The distinction matters because these two conflicting PRs in the same cycle required opposite answers: #1136's conflict was an unmerged duplicate (take master's superset), this one is a genuine disjoint addition (keep both), and at the marker level they look the same. That is why I built scripts/classify-conflict.py this cycle — its disjoint / duplicate split is exactly this case, and this PR's merge state is pinned in that tool's test suite as ground truth.

The change itself. Verified in both directions, which is the only way a root-resolution fix can be checked. Running this worktree's copy from inside the worktree reports tree: <worktree> and OK: Agent.md documents 1416; running that same script file from the main checkout reports tree: /Users/argszero/.emrg/evolution/emrg and OK: Agent.md documents 1431. Two different answers, each about the tree the caller was standing in, and the tree is now printed rather than left to be inferred. That is the defect fixed: previously the answer was byte-identical in both positions, so the wrong one was unreadable from the output. --write in the old position edited a checkout the caller was not looking at — a confirm-step that silently corrupts the wrong tree.

Guard tests at this head: tests/test_check_doc_count.py and tests/test_check_node_test_count.py → 63 passed, 1 skipped.

argszero pushed a commit that referenced this pull request Sep 11, 2026
…fects)

Adversarial probing of classify-conflict.py (#1143) found three ways it could
recommend a resolution that silently loses work. All three are latent in the
predicate, not the plumbing, so none was visible from the tool's own suite.

1. duplicate compared declared NAMES only. "theirs declares every name ours
   does" was read as "theirs contains ours", but when both sides declare
   test_alpha with different bodies, taking the superset discards ours' edit to
   it. That is the data loss this tool exists to prevent, hidden behind the one
   verdict that recommends a side-pick. Shared symbols' bodies are now compared;
   a mismatch escalates to overlapping instead of guessing.

2. count-line fired on any one-line-vs-one-line integer difference, so
   x = compute(1) vs x = compute(2) was answered "MEASURE ... never pick a side"
   with exit 0 - wrong advice, and it closed the only case a human must read.
   The rule now requires a parenthesised, non-call count on both lines, which is
   the Agent.md shape.

3. With no symbols and no count, a single differing line was called disjoint
   (KEEP BOTH), which concatenates into nonsense if it is really one line edited.
   Ambiguous now escalates.

Verification: both real historical cases still reproduce exactly against
reconstructed merges - #1140 -> disjoint=2 + count-line=1, #1136 -> duplicate=1
+ count-line=1, zero false escalations. Six new tests, all three defects
mutation-verified (disabling each fix reds exactly the tests meant to pin it).
Full suite 1435 passed / 2 skipped.

@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 25ec267 (CI double-green, MERGEABLE, FRESH). I verified the central claim — that the root resolves from the cwd, not from __file__ — and mutation-checked that the tests actually pin it. Replacing the cwd predicate with if False: (i.e. reverting to the script's own root) reds exactly two tests, test_the_tree_is_the_checkout_you_are_standing_in and test_the_measured_tree_is_named_in_the_output, and nothing else. The guard is load-bearing, not decorative.

The second test name is the more valuable half. The original defect was that the output was byte-identical in both positions, so a wrong tree was unreadable from the result — a count tool that misreports certifies a tree you are not looking at. Printing tree: <path> makes the failure checkable at all, and that is the part I would keep even if the root logic were later rewritten.

This is also the PR whose conflict I resolved by keeping both sides rather than picking one: the two sides added disjoint probes for the same test files, so either side-pick would have silently dropped four of them — the case that motivated building scripts/classify-conflict.py. At this head: 34 passed. The count line was re-measured on the merged tree.

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