PR #613 follow-ups: the sweep's hardcoded tree basename, and one stale self-sync comment nf-mie ← Beads

open priority 2 task unassigned

Twinned from https://github.com/cfsh/ninefold/issues/636 by tools/beads/import_github.py's reconcile pass.

_Suggestions from ninefold-reviewer's review of #613._ Both are below the severity floor — filed rather than cycled. Unclaimed.

1. sweep_stale_trees reconstructs the worktree path with the OLD basename

tools/reviewer/dispatch.py — the sweep deregisters with:

```python run(["git", "worktree", "remove", "--force", str(entry / "tree")], check=False, cwd=clone) ```

12caefc changed the producer: ReviewTree.__enter__ now builds self._container / f"tree-{rand}", not tree, so the admin name is unique per invocation. The sweep was not updated with it, so on a real leaked tree that path does not exist, git worktree remove fails (not a working tree), and check=False swallows it.

The outcome still holds, but by accident rather than by design: shutil.rmtree(entry) reclaims the disk, swept increments, and the trailing if swept: git worktree prune drops the admin record — legitimately, because by then the directory HAS vanished. So nothing leaks today. What is wrong is that the git-aware half of the reclaimer is dead code, and the outcome now rests on a prune call that reads as a belt-and-braces afterthought.

Invisible to the suite, because the fixture pins the old shape too: SweepStaleTreesTest._tree creates (d / "tree").mkdir(parents=True), and the one test that reaches the removal fakes D.run without asserting the argv. So the sweep tests exercise a layout the producer can no longer create.

Fix is two lines — derive the inner name the way __enter__ does (or take the single child directory), and build the fixture through the same helper.

2. review_now.py still credits the reset with the review's code/CHARTER freshness

tools/pr/review_now.py, the comment above the setdefault:

> ⚠ Self-sync DEFAULTS on, and find_clone is what makes that safe — it hard-resets the clone to origin/main so the review runs against current code and the current CHARTER.

That clause is the one #613 F7 corrected in README.md:220, same class as F2/F9: the reset's job is deploying the dispatcher's own code, and the review's code context and CHARTER come from the per-invocation worktree. The banner ~15 lines above and the module docstring were both updated in that pass; this comment was missed.

Refs #613, #576.

Dependencies

None.

Comments

No comments.

Add a comment