PR #558 follow-ups: test isolation, the notes bullet, and an unchecked write nf-wdku ← Beads

closed priority 2 task unassigned

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

Suggestions from ninefold-reviewer's review of #558. All are below the gate's severity floor — the PR converged at 8c60d6a with the python lane measured green on the Windows merge box (68/68) — so none of them blocked it. Filed unclaimed so they are not lost.

#562 held the previous round's residuals and closes on #558's merge; this is its successor, not a duplicate of it.

1. test_a_plain_resume_gets_past_the_flag_checks reaches the real box

tools/tests/test_spawn_dev.py:1012-1020. It has to get *past* the new --resume guard, so it calls sd.main(["widget", "--resume"]) with no fakes and no NF_DEVS_ROOT — the only test in the suite that does. Every other MainTest case refuses before any git or filesystem work, and test_a_spawn_with_an_empty_kickoff_is_refused's docstring names that as "what keeps this test from touching the real repo".

Measured on the Windows box: it runs the real git worktree list, then stops at ✗ tmux is not on PATH. On the office box, where tmux and claude are present and ~/nf/devs is real, it continues to worktree_at(~/nf/devs/widget) — and the assertion holds only because no dev is named widget. If one ever is and its session has died (the state --resume exists for), the preconditions pass and the test performs a real resume: tmux new-session -d plus a real claude -c --permission-mode auto. The test then also fails on assertEqual(code, 1).

LayoutTest already has the fix in the same file (test_spawn_dev.py:319): set NF_DEVS_ROOT to a tempdir for this test. "No worktree registered at <tmp>/widget" then becomes the deterministic reason for the exit-1 being asserted on — which also makes the test prove more than it does now, since today a missing tmux anywhere upstream satisfies it.

2. The new --resume refusal is not in the notes or the runbook text

8c60d6a makes --resume --issue 42 a hard refusal. That is the right call, but notes/spawn-dev.md's Gotchas list and the proposed runbooks/spawn-dev.md §2 text on [#557](https://github.com/cfsh/ninefold/issues/557#issuecomment-5190361510) both still describe --issue/--notes and --resume without mentioning that they are mutually exclusive. The CTO meets it as a surprise refusal. One bullet, same shape as the F10 one — and the #557 comment is the copy actually handed over, so it wants both.

3. Closes #562 is in the commit message only

The PR body's linkage is Refs #557. merge_pr.py merges with gh pr merge --squash and no explicit --body (merge_pr.py:447), so whether the Closes #562 trailer survives into the squash commit depends on the repo's squash_merge_commit_message setting. Adding one Closes #562 line to the PR body removes the dependency.

4. ensure_identity reports a write it does not check

tools/office/spawn_dev.py:619-625:

```python tree.raw(["config", "user.name", BOT_NAME], check=False) tree.raw(["config", "user.email", BOT_EMAIL], check=False) return (f"git identity was … — set to {BOT_NAME}. …") ```

Neither result is inspected. If either git config write fails — a locked or read-only shared config is the realistic way in — the report still says set to jcantsp-bot and the spawn proceeds, and the dev commits under whatever identity was actually there. That is the failure this constant exists to prevent (BOT_NAME's own comment: every session commits as jcantsp-bot so aedanpope can approve every PR), discovered at PR time rather than spawn time.

Same class as F4 and lower reachability, which is why it is here rather than in the gate. The function's docstring says it "asserts the guarantee"; re-reading the two values after writing would make that literally true, and it is the same verify-don't-assume discipline create_worktree is built on.

5. The guardrail block does not name the one write outside the dev workspace

spawn_dev.py:32-53 is the text #561's grant decision is read off, and it is scoped precisely: "authors exactly one file in the dev's workspace". True as written. But ensure_identity can write $GIT_COMMON_DIR/config — the clone's and every other worktree's — and that is the tool's only mutation outside the dev workspace. It is documented in ensure_identity's own docstring and surfaced in the closing report, so nothing is hidden; it is just not in the paragraph a grantor reads. F2's lesson was that this specific block has to be complete rather than merely accurate.

Dependencies

None.

Comments

No comments.

Add a comment