fix(iterate-pr): say when the rebase upstream was a guess

[thecodedrift]

#301 described a silent loss: a rewritten parent fetched into a fresh checkout, tier 3 falling back to the parent, and the balloon guard blind to it because the expectation and the outcome share the upstream. I wrote that issue. Reproduced against real repositories, it is wrong on both halves, and this PR fixes what actually reproduces.

What the reproduction showed

The recommended pattern already works. --fork-point reads the parent branch's reflog, and a worktree shares refs and reflogs with its clone. So several agents in worktrees of one repository — exactly what CLAUDE.md recommends — keep tier 2 functioning. One worktree amends the parent; another reads the pre-amend tip out of the shared reflog and replays the child from the right place:

✓ child rebased onto parent (+1 own)
p.txt → v2-FIXED          # the parent's fix survived
child log → only the child's own commit

Reaching tier 3 with a rewritten parent needs a separate CLONE that never saw the old tip: a fresh CI checkout, a second machine, someone else's copy. Measured there, both outcomes are safe:

stale copies	outcome
patch-compatible (pure rebase, or an amend that only adds)	dropped by git rebase; result correct, parent's fix intact
genuinely divergent	CONFLICT, rebase aborted, child untouched, nothing pushed, exit 2

No silent loss, and no guard reading as satisfied.

What is actually wrong

The tool explained itself badly. The conflict arrived as:

✗ CONFLICT: child onto parent (from parent). Needs manual reconcile:
      parent.txt

parent.txt is a file the child never touched, and nothing marks the upstream as inferred. (from parent) is the only tell and reads as information rather than a warning.

The danger is the next step, not the abort. Someone reconciling by hand sees an inexplicable conflict and resolves toward the child's copy — which is the superseded side, and taking it puts the parent's old work back. The tool stops safely; the person then undoes the fix.

The change

forkUpstream now reports where the upstream came from (recorded, fork-point, guessed). A guess is announced twice: in the plan, before the rebase, so a clean replay is still attributable, and again at a conflict, naming which side is superseded.

! child: no fork point is known, so the upstream is a GUESS (parent). That is
  correct if parent was only appended to, and wrong if it was rewritten
  somewhere this clone never saw.
✗ CONFLICT: child onto parent (from parent, guessed). Needs manual reconcile:
      parent.txt
      ^ the upstream above was a GUESS. A conflict in files child never touched
        usually means parent was rewritten elsewhere and this clone cannot see
        where child forked. Do NOT resolve toward child's copy without checking
        what parent changed: that side is the superseded one, and taking it puts
        the old work back.

It warns only when the parent is not already an ancestor of the child. When it is, the child contains the current parent, parent..child is exactly the child's own commits, and the fallback is right by construction — warning every time would train the reader to skip the line that matters. The worktree case above stays silent, because it knows its fork point.

Rejected: failing closed

Refusing whenever tier 3 fires and the parent is not an ancestor would also refuse the case tier 3 exists for — a parent that was only appended to, where parent..child is the child's own commits and the replay is correct. That would break working behaviour to guard against a case that already stops safely.

Rejected: recording pre-rewrite tips in a pushed ref

The obvious "make it actually work" fix, and it is worse than it looks. The record has to be written by whatever rewrites the branch, which is ordinary git commit --amend and git push --force, not this script — so it would need a hook installed per clone, and a fresh clone is precisely the one least likely to have hooks. The mechanism would be absent where it is needed.

Worse, a stale record beats --fork-point in the tier order, so it is trusted. One surviving a later legitimate rebase makes the tool confidently replay the wrong range — manufacturing the silent, confident failure this issue wrongly attributed to the current code.

Tests

Six new cases, mutation-checked in both directions:

• never warning fails the two announcement tests
• warning even when the parent is already an ancestor fails the exemption test

The docblock is rewritten to describe what reproduces rather than what was assumed, including why failing closed and ref-recording were rejected, so neither gets re-proposed.

Fixes #301

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 3m 26s —— View job

---

Review mode: incremental — read 0 prior review thread(s) before reviewing.

.prior-review.json contains no reviewThreads, no reviews, and its only comments entry is the @claude /review trigger itself. There is nothing previously raised to classify as addressed or still-open — this is effectively the first substantive review of this PR.

• Read .prior-review.json
• Fetch PR diff (gh pr diff 317) and metadata
• Read both changed files in full (propagate_stack.cjs, propagate_stack.test.cjs)
• Read adjacent files for cross-file impact (shared.cjs, stack_status.cjs)
• Analyze correctness, tests, clarity
• Post findings

---

Assessment

The change does what the PR body says: forkUpstream now returns { upstream, source } instead of a bare string, guessIsRisky gates the new warning on the parent not already being an ancestor of the child, and the warning is emitted both pre-rebase (plan) and at conflict time. forkUpstream has no other callers in the repo, so the return-shape change is safe. The six new tests exercise both the "announce" and "don't train the reader to skip it" branches, plus the conflict-message wording, and line up with the code paths they claim to cover. I did not find a correctness bug in the core logic change.

[New] Reuse — guessIsRisky re-implements a helper that already exists next door.
.agents/skills/iterate-pr/scripts/propagate_stack.cjs:122-124:

const guessIsRisky = (git, parent, child, source) =>
  source === "guessed" &&
  git("merge-base", "--is-ancestor", parent, child).code !== 0;

.agents/skills/iterate-pr/scripts/stack_status.cjs:47-48 already defines the exact same primitive:

const isAncestor = (git, ancestor, descendant) =>
  git("merge-base", "--is-ancestor", ancestor, descendant).code === 0;

Both files already share shared.cjs for countRange, orderedDescendants, etc. — orderedDescendants even has a comment noting it's shared "rather than a second copy that has to be kept in sync by hand" (stack_status.cjs:53). isAncestor should move to shared.cjs and be reused (negated) here instead of being reimplemented inline with the polarity flipped, which is exactly the kind of near-duplicate the codebase already avoids for its other primitives.

[New] Documentation gap — the PR body promises a rationale the code doesn't contain.
The PR description states: "The docblock is rewritten to describe what reproduces rather than what was assumed, including why failing closed and ref-recording were rejected, so neither gets re-proposed." The docblock in propagate_stack.cjs:74-101 does explain why failing closed was rejected (lines 99-101), but contains no mention of the "recording pre-rewrite tips in a pushed ref" alternative or why it's worse than it looks (the per-clone-hook problem, and a stale record outranking --fork-point in the tier order). That reasoning currently lives only in the PR body. Since the stated purpose is to keep that alternative from being re-proposed later, and future readers of the script will see the code comment, not this PR description, it's worth folding that rejection into the docblock too.

[New] Test-coverage gap — the "not risky" combination is only tested pre-rebase, not at conflict time.
risky is computed once (propagate_stack.cjs:203) and gates both the plan warning (line 204) and the conflict annotation (line 241). Tests cover guessed+risky (both plan and conflict), guessed+not-risky (plan only, propagate_stack.test.cjs:434-440), and fork-point+conflict (no warning). There's no test for guessed+not-risky+conflict (parent already an ancestor, but the rebase still conflicts) to confirm the conflict block also stays silent. The code path shares the same risky variable so it's very likely correct, but that specific combination — arguably the one case where a false-positive "GUESS" annotation would be most confusing, since the tool is confident about the upstream — isn't pinned by a test.

No blocking correctness issues found. The two non-blocking items above are cleanup/completeness suggestions, not bugs.

[thecodedrift]

Re: @claude — "Claude finished @thecodedrift's task in 3m 26s … Review mode: incremental"
#317 (comment)

Fixed in 905c1ff. You were right that shared.cjs is where this belongs, and the precedent you cited is the one that settled it: orderedDescendants carries a comment saying it was consolidated so there would not be "a second copy that has to be kept in sync by hand," after the same finding one review earlier. Writing the third copy of a primitive in a pair of files that already share a library is the thing that comment exists to prevent.

isAncestor now lives in shared.cjs; stack_status.cjs imports it instead of defining it, and guessIsRisky reads as source === "guessed" && !isAncestor(git, parent, child).

Two tests came with it. One pins the exit-code mapping, and one pins the argument order — (ancestor, descendant) is the half that is easy to get backwards, and a reversed pair still returns a boolean, so nothing would have reported it. 377 script tests pass, lint clean.

Thanks also for checking that forkUpstream has no other callers before signing off on the return-shape change. That was the right thing to verify, and it is the kind of check that only shows up in a review when someone actually goes looking.

— AI Coding Agent