feat: fix(loop): re-check source_issue before opening PR — skip if already closed (#166)

[mabry1985]

Summary

Adds a _source_issue_still_open guard in the PR-open path of _drive (after fixups/gate, before gh pr create). When a card's source_issue is set and gh api reports it is no longer open, the loop skips the PR, marks the card superseded via cancel_feature, reaps the worktree, and returns — preventing a duplicate PR from wasting reviewer/CI cycles. All error paths (gh failure, timeout, unparseable reference, no slug, bare number with unresolvable remote) fail-open so a broken network read never blocks a legitimate PR. Cards without source_issue are unaffected.

Requirements

• r1: done

Fixes #166

[protoreview]

QA panel review — FAIL

code-review-structural · head 4c68facfac9d · formal

[review-synthesizer completed: workflow code-review-structural:report]

Moderate overall risk: the PR's headline guard (skip duplicate PRs when the source issue is already closed) fires only on an explicit source_issue field, so the common feature-text fallback path still opens duplicate PRs with a Fixes #N for a closed issue — the exact #166 waste this PR claims to prevent. Fix that guard first; the verification confirms both surviving findings byte-for-byte and upgraded the scope gap to major since it sits at the center of the change's purpose. The panel disagreed on the repo_slug monkeypatch in test_drive_opens_pr_when_source_issue_still_open: verification refuted the "dead test setup" nit because _with_source_issue_ref (which this test's drive reaches) consumes worktree.repo_slug(cwd=wt), so that patched symbol is live and those lines will not be touched. Note two coverage gaps: no structural/tool-sourced (protopatch) findings were submitted this round, and after the refute tests/test_loop.py has zero surviving findings — its coverage came only from the removed-behavior lane, so keep the untested except → flag_blocked branch in sight.

Findings

	Severity	Location	Finding	Verified
🟠	major	loop.py:1846	The closed-source-issue guard never runs for cards whose source issue is resolved by _source_issue()'s feature-text fallback (no explicit source_issue fiel…	confirmed
🟡	minor	loop.py:1858	The new except Exception → store.flag_blocked(fid, reason) fallback added to _drive's source-issue guard is entirely untested — no test makes `store.cancel…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "loop.py",
    "line": 1846,
    "severity": "major",
    "category": "correctness",
    "claim": "The closed-source-issue guard never runs for cards whose source issue is resolved by `_source_issue()`'s feature-text fallback (no explicit `source_issue` field), so a card whose source issue appears only in description/spec/design/title still opens a duplicate PR with a 'Fixes #N' for an already-closed issue — the exact #166 waste this PR claims to prevent (flagged by both correctness and removed-behavior review).",
    "evidence": "si_raw = str(feature.get(\"source_issue\") or \"\").strip()\n                    if si_raw and not await _source_issue_still_open(si_raw, wt):\n\n(the guard is gated purely on the explicit field, while the immediately following `_with_source_issue_ref(feature, …)` stamps the PR body via `_source_issue`, which documents: \"Precedence: an explicit `source_issue` field … wins; otherwise the FIRST GitHub issue URL in the feature text.\")",
    "verdict": "confirmed",
    "note": "Guard lines are verbatim in the diff+; _source_issue (read at base) falls back to description/spec/design/acceptance_criteria/title exactly as claimed, and _with_source_issue_ref stamps via it — so text-only-issue cards bypass the guard yet still get Fixes #N."
  },
  {
    "file": "loop.py",
    "line": 1858,
    "severity": "minor",
    "category": "tests",
    "claim": "The new `except Exception → store.flag_blocked(fid, reason)` fallback added to `_drive`'s source-issue guard is entirely untested — no test makes `store.cancel_feature` raise, so this new branch (warning + flag_blocked + continued cleanup) can regress silently.",
    "evidence": "+                        try:\n+                            store.cancel_feature(fid, reason)\n+                        except Exception:  # noqa: BLE001\n+                            log.warning(\n+                                \"[project_board] %s cancel_feature failed — flagging blocked instead\",\n+                                fid,\n+                                exc_info=True,\n+                            )\n+                            store.flag_blocked(fid, reason)",
    "verdict": "confirmed",
    "note": "try/except + log.warning + flag_blocked present verbatim in the diff+. FakeLoopStore.cancel_feature (added this PR) appends and returns without raising, and no test in tests/test_loop.py ever makes it raise — the new fallback branch is indeed uncovered."
  }
]

[protoreview]

QA panel review — FAIL

code-review-structural · head 4c68facfac9d · formal

[review-synthesizer completed: workflow code-review-structural:report]

All five findings came back confirmed; nothing was refuted and nothing is marked uncertain, so the panel stands intact. The verifier's only adjustments are framing qualifications (RB#1's "violates fail-open" wording — the code matches its own documented rc=0 contract, the defect is that fail-open is scoped to gh failure only; and Conv#3's test does execute the fake remove_worktree, it's just never asserted). Prior requests block is empty, so no dispositions block is emitted. No skipped structural pass and no coverage gaps: the verifier normalized every quote against the actual repo text, so I carry the verdict/note/evidence fields byte-for-byte. Ranking: the single major first, then minors ordered by load-bearing impact (the correctness-in-skip-path one before the convention nits, tests last).

Overall risk is moderate: the #166 source-issue re-check adds a destructive skip path, and the one thing to fix first is RB#1 (loop.py:414) — any rc=0 output that isn't literally "open" (empty stdout from a null jq .state, a broken pipe that still exits 0) is treated as closed and permanently cancels the feature card, which undercuts the fail-open intent the docstring reserves for gh failure only. The verifier confirmed all five findings and refuted none; it qualified RB#1 (the code matches its own documented rc=0 contract — fail-open is scoped to gh failure, not garbled rc=0) and clarified that Conv#3's test does execute the fake remove_worktree, it's just never asserted or forced to fail. Remaining minors: a bare remove_worktree that can strand inflight bookkeeping on a git timeout (1856), silent exception swallowing in _source_issue_still_open (405/382), and missing fallback/reap test coverage (1853). No skipped structural pass and no coverage gaps — the heavily-changed loop.py was read in full.

Findings

	Severity	Location	Finding	Verified
🟠	major	loop.py:414	The new guard treats any rc=0 gh output that is not exactly the string 'open' (including empty stdout from a null .state or a broken pipe that still exits 0) a…	confirmed
🟡	minor	loop.py:1856	In the new skip path, await worktree.remove_worktree(...) runs bare immediately after a guarded cancel_feature: if it raises WorktreeError the self._inflight.p…	confirmed
🟡	minor	loop.py:405	The new _source_issue_still_open swallows every exception from repo_slug/_gh and returns True with no logging at all, so a persistent gh/config failure silentl…	confirmed
🟡	minor	loop.py:382	The name _source_issue_still_open asserts a fact about issue state it doesn't guarantee: it returns True on empty/unparseable source_issue, gh failure, and unr…	confirmed
🟡	minor	loop.py:1853	The skip path's changed fallback behavior — store.flag_blocked(fid, reason) when cancel_feature raises, plus the remove_worktree reap — has zero test coverage:…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "loop.py",
    "line": 414,
    "severity": "major",
    "category": "correctness",
    "claim": "The new guard treats any rc=0 gh output that is not exactly the string 'open' (including empty stdout from a null .state or a broken pipe that still exits 0) as 'closed', and the skip path then permanently cancels the feature card and reaps its worktree — a fail-closed outcome that directly violates the fail-open contract the docstring ('a broken network read must never block a legitimate PR') and every other error branch establish (flag for both correctness/removed-behavior and conventions review).",
    "evidence": "    state = out.strip().strip('\"').lower()\n    return state == \"open\"",
    "verdict": "confirmed",
    "note": "Read verbatim in _source_issue_still_open: `state = out.strip().strip('\"').lower(); return state == \"open\"`. Any rc==0 output that isn't exactly \"open\" (\"closed\", \"null\", \"\") → False → skip path runs permanent store.cancel_feature + remove_worktree. One qualification for the synthesizer: the function's OWN docstring documents False as 'gh exits 0 and reports a state other than \"open\"', so the rc==0 branch matches its stated contract — the defect is that fail-open is scoped to gh *failure* (rc!=0 / raise) only, leaving rc==0+garbled routed to the destructive branch."
  },
  {
    "file": "loop.py",
    "line": 1856,
    "severity": "minor",
    "category": "correctness",
    "claim": "In the new skip path, await worktree.remove_worktree(...) runs bare immediately after a guarded cancel_feature: if it raises WorktreeError the self._inflight.pop and return never execute, leaving the already-cancelled card with a stale inflight entry and dropping control into the enclosing except(WorktreeError) handler written for the old all-or-nothing flow, which then runs its own bookkeeping on a card cancel_feature already removed.",
    "evidence": "                        try:\n                            store.cancel_feature(fid, reason)\n                        except Exception:  # noqa: BLE001\n                            log.warning(\n                                \"[project_board] %s cancel_feature failed — flagging blocked instead\",\n                                fid,\n                                exc_info=True,\n                            )\n                            store.flag_blocked(fid, reason)\n                        await worktree.remove_worktree(repo, wt, branch or \"\")\n                        self._inflight.pop(fid, None)\n                        return",
    "verdict": "confirmed",
    "note": "Skip block read verbatim: `await worktree.remove_worktree(repo, wt, branch or \"\")` is bare, followed by `self._inflight.pop(fid, None)` + `return`. remove_worktree calls _git, which raises WorktreeError on a git timeout, so it CAN raise; the whole body sits in the `except (worktree.NoChangesError, worktree.WorktreeError)` handler whose terminal branch runs flag_blocked on the already-cancelled card + a second remove_worktree/pop (the retryable branch would even re-dispatch a cancelled card). pop/return are indeed skipped if it raises."
  },
  {
    "file": "loop.py",
    "line": 405,
    "severity": "minor",
    "category": "conventions",
    "claim": "The new _source_issue_still_open swallows every exception from repo_slug/_gh and returns True with no logging at all, so a persistent gh/config failure silently disables the #166 re-check with zero trace in production logs, unlike every other error path in this same PR which logs with exc_info=True.",
    "evidence": "    except Exception:  # noqa: BLE001 — timeout / infra error → fail-open\n        return True",
    "verdict": "confirmed",
    "note": "Both except blocks in _source_issue_still_open are verbatim bare `return True` (repo_slug except and _gh except), and the `rc != 0: return True` branch logs nothing either. Sibling error paths in this same PR's skip path (cancel_feature except) log warning(...) with exc_info=True. No logging exists on any silent-fail-open path."
  },
  {
    "file": "loop.py",
    "line": 382,
    "severity": "minor",
    "category": "conventions",
    "claim": "The name _source_issue_still_open asserts a fact about issue state it doesn't guarantee: it returns True on empty/unparseable source_issue, gh failure, and unresolvable slug — cases where the state is unknown or unread, not 'still open' — so the identifier misleads callers reading its truth value as an issue-state predicate when it actually means 'proceed / don't block'.",
    "evidence": "    parsed = _source_issue({\"source_issue\": source_issue_raw})\n    if parsed is None:\n        return True  # unparseable reference → fail-open",
    "verdict": "confirmed",
    "note": "Function read: returns True on empty raw, unparseable ref (this line, verbatim), repo_slug exception, blank slug, _gh exception, and rc!=0 — all 'state unknown' cases, so the identifier overstates an issue-state fact. Accurate as a naming/convention observation; the docstring mitigates the confusion by spelling out semantics."
  },
  {
    "file": "loop.py",
    "line": 1853,
    "severity": "minor",
    "category": "tests",
    "claim": "The skip path's changed fallback behavior — store.flag_blocked(fid, reason) when cancel_feature raises, plus the remove_worktree reap — has zero test coverage: the fake store's cancel_feature never raises and test_drive_skips_pr_when_source_issue_closed asserts only the cancel_feature / no-open_review outcome.",
    "evidence": "                        except Exception:  # noqa: BLE001\n                            log.warning(\n                                \"[project_board] %s cancel_feature failed — flagging blocked instead\",\n                                fid,\n                                exc_info=True,\n                            )\n                            store.flag_blocked(fid, reason)",
    "verdict": "confirmed",
    "note": "Re-read tests/test_loop.py: FakeLoopStore.cancel_feature never raises; test_drive_skips_pr_when_source_issue_closed asserts opened==[], 'cancel_feature' in names, no open_review, 'superseded' in the reason, _inflight=={} — no test stubs cancel_feature to raise or asserts the flag_blocked fallback, and no assertion on the remove_worktree reap args. Minor nuance: the happy-path test DOES execute the fake remove_worktree (it's only un-asserted and never made to fail), so phrase as 'fallback + reap never asserted / fallback branch unreachable' rather than literally zero execution."
  }
]

---

2 panel step(s) hit their time budget and were skipped this round: find_correctness, find_crossfile. The verdict stands on the remaining angles; a finding only that step would have caught could be missed — the next push re-runs the full panel.