PR #626 follow-ups: adoption refusal ordering, and the dry run's gh line nf-zjd ← Beads

open priority 2 task unassigned

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

Suggestions from ninefold-reviewer's review of #626 (--adopt, at 3494d32). All advisory — the PR converged and none of these blocked it. Unclaimed.

1. The adoption refusal is unreachable when the topic word matches the outgoing session's

tools/office/spawn_dev.py — in spawn, the generic existing is not None branch runs before the if adoption is not None: block. A handover that reuses the outgoing topic word collides on the worktree path (nf-dev-publish-header/root/nf/devs/publish-header, and spawn_dev.py publish-header --adopt 553 computes the same path), so worktree_at matches first and the refusal that fires is the one recommending --resume — a flag --adopt refuses two frames later. The handover-specific message, including its "if it is still live, this is not a handover" warning, never appears. #626's live demo avoided it only by using the topic demo-adopt.

Fix: hoist the worktree_on check above the generic branch when adoption is not None, or make that refusal adoption-aware. Worth a test that spawns an adoption on the *colliding* topic word, since that is the case the existing suite does not cover.

2. The dry run's gh line omits -R, unlike every read the tool actually makes

tools/office/spawn_dev.py's dry-run block prints gh pr view {n} --json {PR_FIELDS}; Gh.pr_view (tools/nflib/gh.py:149) issues gh pr view <n> -R cfsh/ninefold --json <fields>. The module's own rule one command down is "a dry run that paraphrases is a dry run you cannot paste", and test_the_dry_run_fetch_matches_the_one_the_spawn_really_issues pins that property for the fetch. Pasted from outside a ninefold checkout the printed form resolves against the wrong repo or errors.

Fix: print -R {gh.repo}, ideally by building the line from the same argv pr_view builds, and extend the fetch's matching test to cover it.

3. read_pr does not require a non-empty headRefOid

read_pr refuses an empty headRefName but not an empty headRefOid, and the divergence precondition reads if local and adoption.head and local != adoption.head. A blank head silently disables the check worktree_add_argv's docstring calls load-bearing ("removing one without the other is a live regression"), and renders the kickoff's "cut from origin/<branch> at " with a blank sha. Not reachable through the real Gh today, so this is hardening rather than a live bug — but it is the guard the -B safety argument rests on, and it fails open.

4. The head is measured before the fetch

read_pr runs in main; git fetch origin --prune -q runs later in spawn. If origin/<branch> advances in between, -B puts the branch on the new head while the kickoff paragraph and report's adopted PR #n at <sha> both state the older one — the "one immutable measurement" the design leans on. Seconds-wide race; a post-add re-read of the created worktree's HEAD would close it, or drop it as not worth the code.

Not a code item, but it outlives this PR

#626's body says the cfsh/nf-office runbook (runbooks/spawn-dev.md) still describes --standing-slot plus a hand-written kickoff, with replacement text to be posted on the PR "once the flag's spelling is settled". A PR-body promise does not survive the merge; if it is not written, the runbook keeps prescribing the hand-assembly #625 exists to delete. Worth its own issue in whichever repo can act on it.

Dependencies

None.

Comments

No comments.

Add a comment