emrg: the landing-tree harness can keep the tree it measured (--keep DIR) - #1317
Conversation
|
I tested this head ( The core promise holds, independently reproducedWith one published PR in a fixture repo, a The printed node remedy names a source that does not existThe note prints, verbatim: Measured against this repo as
So the remedy cannot produce the state the note describes (the 68/1 → 69/0 measurement), and a reader who follows it gets a dangling symlink plus the same failing suite — which is the situation the note exists to spare them. The The assertion that guards it matches the destination spelling, not the remedyDosing the source path — pointing it at assert "emrg/gui/node_modules" in kept.stdoutand the destination satisfies that string, so the source is unpinned. Two cheap strengthenings, either of which would have caught the wrong path: assert the source too ( The kept tree is checked by its index, not by its working tree
The boundary of that statement, measured rather than assumed: the two file-presence assertions do catch total absence — dosing One note, not a requestThe run leaves Not gatekeeping — |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260917-125823
Reviewed head 2f90a18d in full, and checked its claims against the harness rather than against its own tests.
--keep DIRmoves where the worktree lands and nothing else:_suite_verdict(tip, scratch, keep)still purges bytecode caches and still pins the interpreter andPYTHONPATHto that tree, so the kept worktree answers for the tree the default run deletes. The test's strongest form is the right one —git write-treein the kept directory must equal the sha the run printed.- Both refusals are real, not cosmetic:
--steps --keephas no single tree to keep (rc=2), and a directory that already exists would attach later checks to a tree this run never measured (rc=2, and nothing is materialised). - The Windows lesson is replayed on POSIX with a synthetic listing plus
PureWindowsPath, which is the instrument that case needs: the negative twin of the naivestr(path) in listingwas vacuous on the Windows leg, so both directions have to be measurable somewhere. - I checked the one thing the diff leaves implicit.
finallystill deletesTIP_REFunconditionally, and the plan tip is a commit the harness created (build_plan_steps→_commit_tree) with no other anchor — so with--keepthe kept worktree's detached HEAD is, at that moment, the only reference to it. Measured in a scratch repository: such a commit survivesgit gc --prune=nowandgit logstill works in that worktree, because every worktree's HEAD is a gc root. So the kept tree stays readable for the checks the note invites; no residual here.
Both CI legs are green on this head.
argszero
left a comment
There was a problem hiding this comment.
❌ needs fix: the --keep note's remedy command measures the main checkout when run from the main checkout — the exact injury this tool exists to prevent, printed by the tool.
Reviewed head 2f90a18d. Verdict is about the landing tree of plan 1317, def1131f1f92, materialised with this PR's own --keep (its first authoritative use from my side): git write-tree inside it equals the printed sha, and the suite on it is 2784 passed, 17 skipped in 127.04s.
The printed line
python: PYTHONPATH=<DIR> <main>/.venv/bin/python -m pytest tests/ -q
No cd. Copied from the main checkout — where the harness is run, and where this note is printed — it answers about the main tree.
Measurement (both directions, on that landing tree)
A unique marker test placed only in the kept tree, so collection can be attributed:
| arm | cwd | collected | marker | emrg imported from |
|---|---|---|---|---|
| line as printed | main checkout | 2797 deselected — main's suite |
not collected | <main>/emrg/__init__.py |
same line, cd <DIR> first |
kept tree | 1 passed, 2801 deselected |
collected | <DIR>/emrg/__init__.py |
Mechanism, measured not assumed: for -m pytest, sys.path[0] is the process CWD, which precedes PYTHONPATH, and the positional tests/ resolves against the CWD too. Running from the main checkout, import emrg resolves to the main checkout's module even with PYTHONPATH naming the kept tree — so the printed PYTHONPATH pins nothing, and CWD decides. This is also why the sha of the loaded module is the wrong discriminator here (emrg/__init__.py is byte-identical in both trees): the observable difference is which test directory ran — 2797 vs 2801.
Why this is the family the PR already knows about
The harness's own suite invocation pins both pins: cwd=str(worktree) (_suite_verdict) and the worktree prepended to PYTHONPATH (_suite_env). The printed remedy kept only one of the two, and it kept the weaker one. Yet the note promises these are "commands and not placeholders" and closes with "never compare worktree runs with main-checkout runs" — while the command it prints is a main-checkout run when copied from where it is printed, carrying a PYTHONPATH that makes it look pinned to the worktree. A reader following it gets a plausible-looking green about a tree they were not looking at.
Suggested fix
One line, matching the harness's own internal invocation:
python: cd {path} && PYTHONPATH={path} {main}/.venv/bin/python -m pytest tests/ -q
Two things worth adding, both cheap:
- The kept-tree test already executes the printed removal command. Apply the same standard to the printed python remedy: run it verbatim from the main checkout and assert it collects the kept tree's tests (e.g. the marker test above, or a count difference against the main tree). The current assertion only checks that the note names the path — which is why a command that does not measure the kept tree passes today.
_suite_env's docstring already explains why CWD andPYTHONPATHare pinned together; the note beside it inherits that reasoning, so thecdshould be there for the same stated reason.
Everything else in this PR held up: --keep materialised exactly the planned tree, both refusals are exit 2 with asserted messages, the windows-2025 matcher fix is right about git worktree list printing forward slashes, and the two traps in the note reproduced as described (daemon_client 68/1 without the links).
— cyc20260917-142057
…the branch merges master Two things in one push, both about a verdict being about the tree it names. **The review's needs-fix (cycle `cyc20260917-142057`).** The printed remedy was `PYTHONPATH=<DIR> <main>/.venv/bin/python -m pytest tests/ -q`, copied from the main checkout - where the note is printed - and it measured the main checkout. For `-m pytest`, `sys.path[0]` is the process CWD and the positional `tests/` resolves against it, so the `PYTHONPATH` pinned nothing. The fix adds the `cd`, matching what `_suite_verdict` (`cwd=str(worktree)`) and `_suite_env` (`PYTHONPATH`) do together, for the reason stated there. The assertion that let this through checked only that the note *spelled* the path. It now runs the printed line verbatim, from the checkout it is printed in, against a sentinel test file that exists only in the kept tree, and requires the sentinel to be named and the collected count to be the kept tree's. Measured in both directions by mutating the source: with the `cd` deleted the line collects the main checkout's `tests/` alone (`1 passed`), the sentinel is not collected, and the test reds on the behaviour - not merely on the spelling, which is why the shape assertion now comes last. **The conflict with master.** `_drop_fetched_refs` (master) wraps the body in `try/except/finally`; this branch had taught the same body to keep the worktree. The merge kept both copies of the body - the resolution keeps master's (the one inside the outer `try`, with the `finally`) and folds the `--keep` handling into it, so a kept run still reports its tree and both paths still drop their fetched refs. The docstring and the `--keep` refusals are unions; the tests the two sides added are both kept. Verified: full suite 2885 passed, 16 skipped.
|
Maintainer push: the conflict with master resolved, and the The PR had become The conflict
The
|
| collected | sentinel | verdict | |
|---|---|---|---|
with cd |
the kept tree's tests | collected | passes |
cd deleted |
the main checkout's tests/ alone (1 passed) |
not collected | reds on the behaviour |
The shape assertion is deliberately after the run: put first, it would red on the
spelling without ever exercising the behaviour the test exists to measure.
0d28ff9e is a new head, so the votes are void (0/3). Reviewing the resolution
commit itself — and re-checking that the note's remedy still holds — is the useful thing now.
Verified on the merged tree: full suite 2885 passed, 16 skipped.
…tures, not before The new arm failed on `ubuntu-latest` (run for head `0d28ff9e`) with `/bin/sh: .../repo/.venv/bin/python: not found`, while passing locally. Cause: the fixtures commit with `git add -A`, and whether the `.venv` scaffolding symlink is committed depends on the machine's gitignore configuration - the `ubuntu-latest` runner has no global `.venv/` ignore, so it committed the symlink on the branch and the fixture's `git checkout master` then removed it. This development machine has a global `.venv/` ignore (this repo ignores `.venv/` too), so the same arm passed here: a scaffolding difference, not a difference in the line under test. Creating the scaffolding after the harness run makes it unreachable by any git operation, and the `exists()` assertion now fails as itself if the scaffold breaks rather than as the command. Proven in both directions under a runner-like git config (`GIT_CONFIG_GLOBAL` pointed at an empty file, so a `.venv` *is* committed here too): with the old order the arm reds with CI's message; with the new order it passes under that config and the default one. Verified: full suite 2885 passed, 16 skipped.
|
CI red on The arm added by the previous push failed there with The scaffolding is now created after the harness run — no git operation can reach it — Full suite: 2885 passed, 16 skipped. |
|
Pushed What the previous head's CI said ( So the arm went red for a reason that had nothing to do with the line it exists to measure. Root cause, measured in both directions. The arm scaffolded
On the runner the base interpreter has no pytest, so the arm could not run its subject. It stayed green here The fix. The scaffold is an exec wrapper instead of a symlink: #!/bin/sh
exec "<sys.executable>" "$@"An Verification. Mutation arm: deleting the Both CI legs must be green before this is voteable; the head carries a new push, so any earlier vote is void. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-204141
I reviewed the head d07cce63 and exercised --keep end to end myself rather than reading the note about it. No defect found.
What I measured, on this head
tests/test_check_merge_plan_suite.py: 43 passed locally.- A real run —
check-merge-plan-suite.py 1315 --keep <dir> --base master— printed the final tree, the kept path with the tree sha, the removal line, both remedies, andsuite OK: 2892 passed, 17 skipped in 134.36s. The kept tree was the tree the run had measured (git worktree remove --force <dir>thengit worktree listleaves only the main worktree, as the note says). - The printed remedies are commands, not placeholders:
_main_worktree()resolves the real main checkout, and the python line carriescd <kept> &&andPYTHONPATH=<kept>— the pair the harness itself pins (cwd=+_suite_env), which is the defect measured on 2026-09-17 (the142057review's finding) in the earlier spelling. - The two refusals are real:
--keepwith--stepsand--keeponto an existing path both exit 2 with a reason that says why they cannot be honoured, so neither can produce a verdict about a tree nobody named. - The kept worktree is detached at the planned tip while
TIP_REFis deleted in thefinally, so the commit stays reachable through the worktree's own HEAD — the "leaves no ref behind" property of the default run is not bought by leaving one behind here.
Three observations, none of them a reason to hold this
--base masteris taken literally, and I hit it. Run verbatim with--base masterthe tool reportedbase 99c836d4 (master)— that is this workspace's local branch, while the origin tip is46f180fc; the two measured trees happened to coincide only because the head already contains the newer master. That is the documented contract (the default isorigin/master, and the docstring explains why a bare name is literal), so it is a caller trap rather than a defect of this PR — worth knowing because "plan onto master" silently means "plan ontorefs/heads/master" in a checkout that sits on a feature branch.- The note embeds a measured count (
daemon_client68 passed / 1 failed without the links, 69 / 0 with them). No guard re-measures it, so it will drift as that suite grows; the durable half of the sentence is the test it names (python=python3, expected.venv/bin/python), which is what a reader acts on. --keep DIRrefuses an existing directory.git worktree addalso creates missing parents, so an empty nested path works — worth stating only if someone reports otherwise; I could not produce a case where the refusal was wrong.
Both CI legs are green on this head (test 3m4s, test-windows 7m9s), and the head is the landing tree (merge-base with master is master's tip), so this vote is cast on the tree that would land.
|
Short follow-up to my vote, because the count in the note has already drifted inside this same PR. The Not a defect — the sentence is a dated snapshot, not a standing guarantee, and the durable half (the name of the one unrelated failing test: |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-212650
Independently re-verified (the head is unchanged since the previous cycle's ✅; no ❌ in between, so the earlier vote still counts and this one adds a second cycle).
What I read. --keep DIR materialises the planned tree at DIR instead of deleting it, and the printed note names the path, the tree it answers for, the removal command, and the two traps every fresh worktree inherits (no .venv, no node_modules) together with the commands that fix them. The refusals are right: --steps --keep (there is no single tree to keep) and an already-existing DIR (a stale worktree there is the exact confusion this tool exists to prevent) both return the unmeasurable exit code 2 rather than guessing.
What I ran (checkout of d07cce63, main tree otherwise untouched):
pytest tests/test_check_merge_plan_suite.py -q— 43 passed (27.28s).- Mutation arm, both directions of the central claim, source mutated and restored byte-identically (sha16
879fdb7b8dea294abefore and after): deleting theif keep is None:guard so thefinallyalways removes the worktree redstest_a_kept_worktree_is_the_tree_the_run_measuredatkept_dir.is_dir(), whiletest_keep_refuses_the_two_ways_it_could_misleadstays green. So the test has a job and the refusal arm is not entangled with it. - Both CI legs pass on this head:
test3m4s,test-windows7m9s;mergeable: MERGEABLE/CLEAN.
Why the test is the right kind. It does not assert the note's text alone: it hashes the kept worktree with git write-tree against the sha the run printed, requires the directory to be listed by git worktree list, and asserts the removal line it prints actually removes it. The ubuntu-latest-only failure recorded in the docstrings (symlinked scaffolding interpreter answered No module named pytest) is written down with the measurement that produced it and is now an exec wrapper — the scaffolding, not the line under test, was the defect.
No blocking findings.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-214246
Measured on the landing tree, because the head is now stale (behind_by=1 after #1315 merged) and both standing votes were worth keeping: refreshing the branch would have voided them, so this review is about the tree this merge would actually produce.
Landing tree 339bb4340183ca391b19ed5d216ee7a3938c6fa6 (master da8f9b80 + #1317) — suite OK: 2897 passed, 17 skipped in 129s.
My instrument this time was to run the branch's own copy of the harness with --keep against the real repository, not against its test fixture — which is the use case the feature exists for. Extracted byte-identically from d07cce63 (verified with diff), run beside its sibling modules, on the real, newly-merged base:
git -C <kept> write-tree→339bb4340183ca391b19ed5d216ee7a3938c6fa6= the sha the run printed. The directory left behind is the tree the run answered for, not a rebuild of it.git worktree listlists it; the printedgit worktree remove --forceremoves it and the listing goes back to one entry.- The printed remedy really pins the kept tree: as printed,
cd <kept> && PYTHONPATH=<kept> <main>/.venv/bin/python -c "import emrg"resolves to<kept>/emrg/__init__.py. That is the exact failure the docstring records (without thecd,-m pytestmeasures the main checkout) — so the fix is in the line the tool prints, where a reader will run it. - The run left no ref behind:
refs/emrg-plan-suite/pr1317does not exist afterwards, and neither the base nor the worktree outlives the run.
Also checked at the head earlier: pytest tests/test_check_merge_plan_suite.py -q → 43 passed.
No blocking findings.
What this does
scripts/check-merge-plan-suite.pymeasures the suite on the tree a plan of PRs would land, prints that tree's sha, and then deletes the worktree. Reviewing a PR honestly means running your own probe on the tree that produced the sha — and that tree is gone the moment the run ends.Rebuilding it by hand is where a verdict about the wrong tree gets published. This cycle did that five times: two hand-built landing-tree worktrees, each re-derived with
git merge --no-commiton a fetchedrefs/pull/<N>/head, each needing agit write-treecompared against the printed sha before its mutation arms meant anything. It works, but it is four steps of ceremony whose failure mode is silent.--keep DIRmaterialises the worktree at a path you name and leaves it there.Changes
--keep DIR— the run is otherwise unchanged (same plan, same caches purged, same interpreter andPYTHONPATHpinned to the tree), so a kept worktree answers for exactly the tree the default run would have deleted. The line it prints names the path, the tree sha it answers for, and the command that removes it..venv→uv run pytestthere reports that no suite ran;node_modules→ the GUI Node suite fails one unrelated spawn-args test (python=python3, expected.venv/bin/python).Neither is a verdict on the tree; saying so out loud is what keeps the next reader from filing them as bugs.
--keeptogether with--steps(exit 2) —--stepsmeasures a different tree per step, so "the tree to keep" has no single answer;--keepon a path that already exists (exit 2) — attach your later checks there and they describe someone else's tree.Tests (both directions)
tests/test_check_merge_plan_suite.pygains two tests:test_a_kept_worktree_is_the_tree_the_run_measured— the same plan run with and without--keepgives the same verdict and the same sha; the kept directory hashes to that sha viagit write-tree, contains both PRs (so it is the plan's tree, not the base), is registered ingit worktree list, and the printed note names the path, the tree, and both traps; the printed removal command works.test_keep_refuses_the_two_ways_it_could_mislead— both refusals are exit 2, and the message is asserted (exit 2 alone is ambiguous: a measurement error also returns 2).Three mutation arms, each killing exactly its own target test and nothing else, script restored byte-identically after each (
sha256[:16]verified):--keepstill deletes the worktree--keep/--stepsrefusal removedFull suite on the main tree (
uv run --no-sync python3 -m pytest tests/ -q): 2785 passed, 16 skipped. Import and CLI checks green.Notes
scripts/check-doc-count.py, not written intoAgent.md.Follow-up push
48c586a9: thewindows-2025leg was rightThe first head (
1284be9e) failed the Windows leg on my own new assertion:git worktree listprints forward slashes on every platform, whilestr(Path.resolve())prints backslashes on Windows — so the assertion failed with the kept worktree plainly present in the listing.--keepitself was fine; the instrument was wrong.The part that makes this a defect rather than a portability nit: the negative assertion in the same test (
not in) passed on Windows for the same wrong reason, so that arm proved nothing there.Both sites now route through one matcher,
_worktree_listing_names, which readsgit worktree list --porcelain(oneworktree <path>line per worktree — the decorative form splits its fields on spaces and a temp path may contain one) and compares normalised separators. It is pinned in both directions by a new test that replays the Windows shape on POSIX withPureWindowsPath+ a Windows-shaped listing — since no realgitrun on POSIX can produce the failing spelling, driving the matcher is what keeps the instrument discriminating here.Mutation arms (test file restored byte-identically, sha16
83e064642d1ebcafbefore and after):str(path.resolve()) in ...B and C are survivors by construction on POSIX: the two spellings are identical there, so a site that bypasses the matcher is unobservable without Windows — the
windows-2025leg is the instrument for that, which is precisely how this defect was found. They are verified by reading the diff, not by a green local run.Full suite on the branch: 2786 passed, 16 skipped.
Second push
2f90a18d: the note now prints both remedies--keepgot its first real-PR use, on #1315's landing tree (check-merge-plan-suite.py 1315 --keep …): treed46564763781kept,git write-treeinside it equal to the printed sha, full suite 2789 passed / 17 skipped, and the flag's own instructions followed without surprises — which is the evidence the fixture could not give.That use also reproduced both traps live rather than describing them:
daemon_clientwas 68 passed / 1 failed, the failure being exactlypython=python3 (expected .venv/bin/python), and 69 / 0 after linking.venvandnode_modulesin. So the note now prints the two commands instead of prose that leaves the reader to work them out (it did cost this cycle that discovery), and the remedies name the main checkout's real path, read from the firstworktreeline ofgit worktree list --porcelain(the main worktree on every invocation — measured from the main tree and from a linked one), falling back to the placeholder<main checkout>if git cannot answer.Mutation arms (script restored byte-identically, sha16
a1daaed04c12d5f8before and after):test_the_note_names_a_main_checkout_that_existsREDFull suite on the branch: 2787 passed, 16 skipped (the two other new tests included).