ums: separate merge-tree's two forms, stale vs diverged main, untracked R/ files

[d-morrison]

Three lessons banked from the post-merge run on ucdavis/bcs#536.
Each was hit live during that run, and each is measured rather than recalled.

1. shared/workflow/batch-merge-and-resolve.md --- the two merge-tree forms carry opposite signals

The fragment already documents each form separately: the legacy three-arg form always exits 0 (so grep its output), and --write-tree reports by exit status.
It does not say what happens when they are crossed, which is the likeliest mistake, because each half is separately recommended --- --write-tree by resolve-conflicts, ardi, post-merge, and wrap-up, and the marker grep by this fragment.

--write-tree emits no <<<<<<< markers at all.
On a conflict it prints the tree OID, three stage entries, and a CONFLICT (content): line, so the marker grep returns 0 unconditionally, and the existing "match unanchored" fix does not help --- there is nothing to match.

Measured on git 2.50.1 against a two-commit synthetic conflict:

form	exit status	<<<<<<< occurrences
merge-tree --write-tree a b	1	0
merge-tree $base a b (legacy)	0	1, as +<<<<<<< .our

Adds the cross-product trap, a Do/Don't pair, and the correct grep for --write-tree (^CONFLICT).

2. skills/post-merge/SKILL.md --- stale main is not diverged main

A blocked git checkout main reads as the diverged case the skill already warns about, since the symptom is identical.
Stale is far commoner: local main behind origin/main and 0 ahead, carrying nobody's work.
The existing remedy ("skip the pull and delete the branch only") is wrong for that case --- it leaves main stale for no reason.

What blocks the switch is usually an unrelated dirty file whose content differs between the stale main and HEAD, but which is identical between HEAD and origin/main.
git fetch origin main:main fast-forwards the ref by refspec with no checkout, after which the switch carries the dirty edit across untouched.
The refspec form refuses a non-fast-forward rather than clobbering, so it is safe even if the diagnosis is wrong.

On bcs#536: local main was 54 behind, 0 ahead, and blocked on a locally-modified renv.lock.

3. memories/r-quarto.md --- an untracked file in R/ poisons the generated NAMESPACE

document() reads the working tree, not the index, so an untracked .R file still gets roxygenized and its @export lands in the tracked NAMESPACE.
git status then shows one innocuous modification while the source backing it is invisible to every other checkout; local R CMD check passes because the file is on disk.

On bcs#536 an untracked R/prep_adherence_by_month.R put export(prep_adherence_by_month) into a document() commit.
It never reached main only because that commit was never pushed.

Verification

• check-new-line-breaks (gha): clean over added lines --- it flagged 6 multi-sentence lines in the first draft of item 3, fixed by a reflow before pushing.
• Banned-glyph scan over added lines: 0 hits, with a positive control confirming the detector fires.
• The --write-tree numbers above come from a purpose-built synthetic conflict, not from recollection.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XcGPxpt5ZqyWvajP8Nse3v

[d-morrison]

Driving this PR to clean --- back off until done.

[claude]

Code review

Reviewed this UMS PR (three lessons banked into shared/workflow/batch-merge-and-resolve.md, skills/post-merge/SKILL.md, and memories/r-quarto.md) against the SERG lab manual conventions and this corpus's own CLAUDE.md/shared/ conventions, and independently verified the technical claims (git merge-tree exit-code/output-shape behavior, git pull --ff-only/fast-forward semantics, git fetch <branch>:<branch> refspec safety, and roxygen2's document() reading the working tree rather than the index). All of the substantive git and R/roxygen2 tool-behavior claims check out against documented behavior.

Three issues found, each posted inline with detail:

1. shared/workflow/batch-merge-and-resolve.md:137 — the [ardi](ardi.md) citation resolves to shared/workflow/ardi.md, which contains no merge-tree/write-tree content; the claim is actually supported by skills/ardi/SKILL.md. (Also posted a follow-up correcting my own first suggestion block there, which mistakenly spanned an extra line — sorry for the noise, the second suggestion is the one to apply.)
2. shared/workflow/batch-merge-and-resolve.md:168-169 — "the positive control" (new text) vs. "the negative control" (pre-existing section heading and case record, same file, same technique) — an internal terminology inconsistency, plus a note that the pre-existing "both failure modes above" a few lines down is now stale since this insertion adds a third.
3. skills/post-merge/SKILL.md:345 — (ahead is non-zero) is not the actual condition under which the described git pull --ff-only failure occurs; that requires both ahead > 0 and behind > 0. Low practical impact (the prescribed remedy is harmless in the mislabeled quadrant), but it undercuts the new section's own claim of giving an exact two-count test.

Considered and set aside as not worth blocking on: a missing explicit Do/Don't pair in the new memories/r-quarto.md entry (per CLAUDE.md's "Record both the pattern and the anti-pattern") — but that file's own established convention almost never uses the labeled-pair format (only 1 of ~30 existing entries does, including a directly comparable pre-existing entry two lines above this one), so flagging it here would be holding this entry to a stricter standard than the file it lives in.

Verdict

Needs minor changes — three findings above, none blocking on correctness/safety (the prescribed remedies in items 2 and 3 are harmless even where the prose is imprecise), but items 1 and 3 are factual/citation errors worth fixing before merge since this corpus is instructional content that future AI agent sessions will follow directly.

[github-actions]

Claude finished review — View run

---

I checked the PR state first, since the review workflow requires stopping if Claude has already reviewed and nothing has changed.

Findings:

• PR ums: separate merge-tree's two forms, stale vs diverged main, untracked R/ files #1077 is open, not a draft.
• The current head commit is f5985597c3d9d5e87f3d127de7f5975fe0100431 — identical to the commit already reviewed in the prior round (the review comment's own blob links all point to this same SHA).
• No commits have been pushed since that review (gh pr view shows exactly one commit on the branch, and it's the one already reviewed).
• Claude (claude[bot]/claude) has already posted a full review on this PR, including three inline findings and a summary verdict, at this exact head.

Since the diff is unchanged since the last review, there is nothing new to evaluate. Per this review workflow's own stop condition ("Claude has already commented on this PR"), and since no --comment flag was supplied, I'm not posting any new GitHub comments — just reporting status here.

Summary

No new commits since the prior review. The three findings from the previous review (posted 2026-08-03T03:40:10Z) remain outstanding and unaddressed at the current head (f5985597):

1. shared/workflow/batch-merge-and-resolve.md:137 — the [ardi](ardi.md) citation resolves to shared/workflow/ardi.md, which has no merge-tree/write-tree content; should point to ../../skills/ardi/SKILL.md.
2. shared/workflow/batch-merge-and-resolve.md:168-169 — new text says "the positive control" where the rest of the file (heading and case record) established "the negative control" for the same technique; plus a stale "both failure modes above" count.
3. skills/post-merge/SKILL.md:345 — (ahead is non-zero) mislabels the actual failure condition, which requires both ahead > 0 and behind > 0.

No new issues to add; the prior review already covered this diff in full.

Verdict

Needs minor changes — same as the prior verdict, since the diff has not changed. The three findings above are still open.

[github-actions]

💰 Cost: $2.6037 (review) — run

[d-morrison]

ARD round 1

All three findings Addressed in 40649bbd. Each was verified independently before applying, rather than taken on the reviewer's word.

#	Finding	Disposition	Verification
1	[ardi](ardi.md) resolves to shared/workflow/ardi.md, which does not support the claim	✅ Address	shared/workflow/ardi.md: 0 merge-tree/write-tree hits; skills/ardi/SKILL.md: 2. Repointed.
2	"positive control" vs the file's own "negative control"; plus "both failure modes" now stale	✅ Address	negative control used this way in 3 other corpus files. Adopted it; count corrected to "all three".
3	(ahead is non-zero) is not the condition for --ff-only to fail	✅ Address	Reproduced: ahead=1, behind=0 → Already up to date., rc=0. Replaced with a three-way table over both counts.

Two notes on the round.

On finding 2, I set aside a real counter-argument rather than silently agreeing: under standard experimental usage a case known to exhibit the effect is the positive control, so the corpus's term is arguably the imprecise one. Internal consistency still wins — one file introducing a competing term is worse than a term with an odd etymology — but changing it belongs in its own PR across all four files, not here. Detail in the thread.

Finding 2's second half is the one worth flagging as a process gap: "both failure modes above" is pre-existing text outside my diff, falsified by my insertion. No added-lines-scoped check can see that class of defect. It is the same shape as the deleted-line hazard this very PR documents, arriving from the other direction — an insertion that invalidates neighbouring prose rather than a deletion that removes it.

Self-review before pushing: check-new-line-breaks clean over added lines, banned-glyph scan clean, and I caught one forward reference ("the negative control below") in my own fix and removed it.

The push carries the re-review; not posting a trigger mention.

[claude]

Code review (round 2 — confirming)

Re-reviewed against the prior verdict (#1077 (comment)), which flagged three items. All three are fixed in 40649bbd, and I independently re-verified each rather than taking the author's ARD summary on trust:

1. Citation target (shared/workflow/batch-merge-and-resolve.md:137) — now reads [ardi](../../skills/ardi/SKILL.md). Confirmed skills/ardi/SKILL.md, skills/resolve-conflicts/SKILL.md, skills/post-merge/SKILL.md, and skills/wrap-up/SKILL.md (the four files the sentence names) all exist and contain write-tree occurrences (2, 3, 2, 1 respectively), so the citation now supports the claim it's attached to.
2. Control terminology (shared/workflow/batch-merge-and-resolve.md) — "positive control" no longer appears anywhere in the file; the new section now says "negative control," consistent with the pre-existing section heading and case record. The stale "both failure modes" was also corrected to "all three failure modes," matching the count after this PR's insertion.
3. ahead/behind condition (skills/post-merge/SKILL.md) — the diverged-checkout bullet now reads "(ahead and behind both non-zero)" and is fed by a three-row decision table keyed on both counts, matching actual git pull --ff-only behavior (verified: ahead>0, behind=0 → Already up to date., exit 0).

No new issues in the fix commit itself. memories/r-quarto.md is byte-identical to the previously-reviewed version (unchanged, and was already clean). One cosmetic note, not flagged as a finding per this review's scope: the reworded "Diverged main checkout" bullet splits bold markup oddly across a parenthetical (**Diverged main checkout** (...)**:**) — renders correctly, just visually unusual; not worth a round over.

Verdict

Ready for merge.

[github-actions]

Claude finished review — View run

---

Code review (round 2 — confirming)

Re-reviewed against the prior verdict (#1077 (comment)), which flagged three items. All three are fixed in 40649bbd, and I independently re-verified each rather than taking the author's ARD summary on trust:

1. Citation target (shared/workflow/batch-merge-and-resolve.md:137) — now reads [ardi](../../skills/ardi/SKILL.md). Confirmed skills/ardi/SKILL.md, skills/resolve-conflicts/SKILL.md, skills/post-merge/SKILL.md, and skills/wrap-up/SKILL.md (the four files the sentence names) all exist and contain write-tree occurrences (2, 3, 2, 1 respectively), so the citation now supports the claim it's attached to.
2. Control terminology (shared/workflow/batch-merge-and-resolve.md) — "positive control" no longer appears anywhere in the file; the new section now says "negative control," consistent with the pre-existing section heading and case record. The stale "both failure modes" was also corrected to "all three failure modes," matching the count after this PR's insertion.
3. ahead/behind condition (skills/post-merge/SKILL.md) — the diverged-checkout bullet now reads "(ahead and behind both non-zero)" and is fed by a three-row decision table keyed on both counts, matching actual git pull --ff-only behavior (verified: ahead>0, behind=0 → Already up to date., exit 0).

No new issues in the fix commit itself. memories/r-quarto.md is byte-identical to the previously-reviewed version (unchanged, and was already clean). One cosmetic note, not flagged as a finding per this review's scope: the reworded "Diverged main checkout" bullet splits bold markup oddly across a parenthetical (**Diverged main checkout** (...)**:**) — renders correctly, just visually unusual; not worth a round over.

Verdict

Ready for merge.

[github-actions]

💰 Cost: $7.2494 (review) — run

[d-morrison]

ARD round 2

Round 2's verdict was Ready for merge with no findings, plus one item explicitly marked "not flagged as a finding." Addressing it anyway, in 631f7e39.

Item	Disposition	Detail
Bold markup split oddly across the diverged-checkout parenthetical	✅ Address	Whole label and its colon now sit in one span, matching the sibling bullets in the same file.

Two reasons this wasn't left as-is. shared/workflow/address-every-comment.md treats severity labels as prioritization rather than a pass, and ardi's own guidance is to read "not a finding" items as candidate violations — that phrasing retires an item more firmly than "nit" does, so it is the one most likely to be skimmed past. And the line is mine: the odd split predates this PR in structure, but I edited it, which under this corpus's own rule makes it mine to fix.

It also cost nothing, because a main sync was owed regardless.

Sync

Merged origin/main (12 commits). Verified rather than assumed:

• None of those 12 commits touch this PR's three files (git log HEAD..origin/main -- <files> is empty), and merge-tree was clean beforehand — so the lag was stale, not defective, which is why I let round 2 run against the un-synced head instead of cancelling it to re-sync.
• Spliced-bullet count delta across the merge: 13/35/33 before, 13/35/33 after. Unchanged, so the merge collapsed no blank lines between list items.
• Merge-status hedge scan over the touched files, whitespace-normalized: one hit, not yet merged, which is quoted git output inside a fenced block, pre-existing, and must stay verbatim. No stale hedges.

Self-review before pushing: check-new-line-breaks clean, banned-glyph scan clean.

Correcting one thing I got wrong mid-round: I first ran the behind-check in the same command as a push, so the count arrived too late to act on, and then misread git diff HEAD origin/main as showing main's changes when it also showed my own. The three-dot/two-dot distinction is documented in this corpus and I used the wrong one.

The push carries the re-review.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.

_{2026-08-03 04:15 UTC}

[claude]

Code review (round 3 — confirming)

Re-reviewed against the prior verdict (#1077 (comment)), which found the PR ready for merge after all three round-1 findings were fixed.

Since then, exactly one new commit landed that's part of this PR's own diff: 631f7e39 ("keep the diverged-checkout label inside one bold span"), which fixes the cosmetic bold-span nit I noted as non-blocking in the round-2 review. Verified directly:

• **Diverged main checkout (aheadandbehind both non-zero):** now opens and closes its bold span on one line, matching the file's other bullet conventions.
• The intervening Merge remote-tracking branch 'origin/main' commit brought in unrelated changes from other already-merged PRs (177 mechanical codex-skills/ wrapper updates, a new rct/refresh-claude-token skill pair, and edits to several shared/workflow/* fragments) — none of that is part of this PR's actual diff against main, which is still exactly the three files reviewed before: memories/r-quarto.md, shared/workflow/batch-merge-and-resolve.md, skills/post-merge/SKILL.md (132 additions, 3 deletions, per gh pr view).
• Re-confirmed all three round-1 fixes are still present: the ardi citation correctly points to ../../skills/ardi/SKILL.md, "positive control" no longer appears anywhere in the file (0 matches), and the ahead/behind divergence table and its three-way decision rule are intact and correct.

No new issues.

Verdict

Ready for merge.

[github-actions]

Claude finished review — View run

---

Posted a confirming round-3 review. Summary: the only change since the last "Ready for merge" verdict was a single cosmetic commit that fixed the bold-markup nit I'd flagged as non-blocking in round 2 — everything else in the branch history since then came from merging main and isn't part of this PR's own diff (still exactly 3 files, 132/-3). All three prior findings remain fixed; no new issues.

Verdict

Ready for merge.

[github-actions]

💰 Cost: $12.1210 (review) — run

[d-morrison]

Done --- PR is free.

Fully clean at 631f7e39: 10/10 check runs completed successfully, round-3 verdict Ready for merge with zero findings and no suppressed-comments block, 0 unresolved inline threads, synced to current main.

One thing named rather than papered over: Copilot ran and posted nothing. Its copilot-pull-request-reviewer check ran 03:55:51Z to 04:00:02Z (4m11s) and concluded success, while get_reviews returns zero Copilot reviews under either login spelling. It is reachable --- it posted a real 2548-byte review on #1079 in this repo at 03:46Z --- so this is silence specific to this PR, not an outage. The clean call therefore rests on claude-review's verdict alone.