feat: Done cards stop alerting: re-check state before a block alert, and clear block/review labels on close

[mabry1985]

Summary

Done cards no longer generate stale block alerts, and close/cancel edges now clear all stale state labels.

• loop/drive.py — _notify_operator now re-reads the card's state immediately before writing the inbox row. If the card is done or cancelled, the alert is dropped and logged at debug; if the re-read raises, the alert is still delivered (fail open, so a real block is never suppressed).
• store.py — the close edges (record_merge, manual mark_done, and the shared cancel_feature path) now also strip any blocked-class:* label plus in-review, review-pending, and changes-requested, in addition to blocked. cancelled and audit labels (e.g. verified:*) are kept. Pulled the stale-label set into a small shared helper so all three edges stay consistent.
• tests/test_stale_alerts_536.py (new) — covers: alert suppressed for done/cancelled cards; alert still delivered when the re-read raises; record_merge and mark_done clear the full stale-label set while keeping audit labels; and a regression that a card carrying both blocked and blocked-class:terminal is still returned by list_features(state="blocked"), confirming the blocked-flag mapping is untouched.
• changelog.d/536.md (new) — changelog entry.

This retry only reformatted store.py and tests/test_stale_alerts_536.py with ruff format to satisfy the gate; no logic changed from the prior attempt.

Fixes #536

[mabry1985]

Review findings (blocking)

2 finding(s): 2 major.

Major

• loop/drive.py:508 [correctness] — confirmed: The new stale-alert re-read in sync _notify_operator makes a blocking br show call (store.get_feature, process-wide single-flight br lock, up to the 45s _BR_TIMEOUT_S), but async _recover_blocked in loop/reconcile.py calls it directly instead of via asyncio.to_thread. Every escalated blocked card on every sweep pass, including passes where the inbox dedups the alert, now stalls all coroutines and logs the 'blocking br on the event-loop thread' WARNING (flagged by the correctness, cross-file and removed-behavior reviews).
  • evidence: now = (store or self._store()).get_feature(fid)
  • verifier: At the PR head, async def _recover_blocked calls self._notify_operator(..., store=store) directly with no to_thread, while every other store call in that method is wrapped in asyncio.to_thread. _notify_operator is a plain def and now runs get_feature before the dedup key is built, so the re-read happens even when the inbox later dedups the alert. store.py defines _BR_TIMEOUT_S = 45.0, the process-wide _br_lock, and _warn_blocking_on_event_loop, which logs a WARNING whenever br runs on a thread with a running event loop.
• store.py:3729 [removed-behavior] — confirmed: record_merge (and mark_done at line 3779) now remove the in-review label (plus review-pending/changes-requested) BEFORE br close, with no rollback. If the close fails, the still-open card drops from in_review to plain in_progress and so drops out of the merge poll's retry set, which only scans in_review/blocked cards. Recovery then depends on the orphan sweep's pr_url_for_branch, and if that lookup misses merged PRs the card is requeued to ready (unverified). Before this PR only blocked was removed and the next poll retried.
  • evidence: stale = _terminal_stale_labels(f.get("labels"))
    if stale:
    self._run("update", fid, *[a for label in stale for a in ("--remove-label", label)])
  • verifier: Confirmed at the PR head. _terminal_stale_labels includes LABEL_IN_REVIEW, record_merge and mark_done remove those labels before self._run("close", ...), and neither has a rollback. Only cancel_feature got an undo. The _list_for_pass docstring says the PR reconcile scans 'in_review AND blocked'. One caveat: 'never retries' is somewhat overstated. The health sweep runs _reconcile_orphan on in_progress cards with no live drive, and that can re-adopt the card via pr_url_for_branch → open_review. I did not check whether that lookup returns merged PRs. If it doesn't, the card is requeued to ready instead, which is arguably worse. The regression is real either way.

The coder is re-driving a fix for these findings.

#544

[protoreview]

QA panel review — PASS

code-review-structural · head 4eb3274f68cb · formal

Low-risk PR: the stale-alert guard and terminal-label cleanup are well-structured and verified clean. The single finding is a pre-existing non-atomic sys.modules init in release_freeze.py (a file this PR does not modify), flagged by the structural engine and confirmed by the verifier. Practical risk is low because CPython's import lock serializes module init, but the setdefault fix is trivial and correct if you want to close it. No panel disagreement; verification confirmed the finding as-is with no severity change. No gaps.

Findings

	Severity	Location	Finding	Verified
🟠	major	release_freeze.py:102	Non-atomic process-stable state init in release_freeze.py allows two threads to hold different locks/dicts	confirmed

findings JSON (machine-readable)

[
  {
    "file": "release_freeze.py",
    "line": 102,
    "severity": "major",
    "category": "concurrency",
    "claim": "Non-atomic process-stable state init in release_freeze.py allows two threads to hold different locks/dicts",
    "evidence": "The module-level state in release_freeze.py uses a get-then-set pattern (lines 103-110): sys.modules.get(_SLOT) returns None, a new ModuleType is built, and sys.modules[_SLOT] = _holder assigns it. Two threads racing this path each build their own holder with its own Lock and dicts, and each keeps a reference to the one it built. The second assignment overwrites the first in sys.modules, so one th Fix: Replace the get-then-set with sys.modules.setdefault(_SLOT, _holder), matching the pattern already used in store._br_lock, review_coverage_hold, and work_snapshot. (protopatch confidence: high)",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "Code at head 4eb3274f68cb lines ~102-110 is exactly as quoted: `_holder = sys.modules.get(_SLOT)` / `if _holder is None:` / \u2026 / `sys.modules[_SLOT] = _holder`. The get-then-set is non-atomic. Practical risk is mitigated by CPython's import lock (module code runs single-threaded during import), but the pattern is as described and the suggested `setdefault` fix is correct."
  }
]

---

1 finding(s) excluded from the verdict by in-diff confinement (file not among this PR's changed paths):

• release_freeze.py (major) — Non-atomic process-stable state init in release_freeze.py allows two threads to hold different locks/dicts

[protoreview]

Promoting the PASS verdict for head 4eb3274f68cb: all checks terminal-green, zero unresolved review threads. (approve-on-green)

Open findings carried by this approval — non-blocking, but they did not go away:

• major release_freeze.py:102 — Non-atomic process-stable state init in release_freeze.py allows two threads to hold different locks/dicts

Approving a WARN does not resolve its findings (issue #22).

[protoreview]

QA panel review — PASS

code-review-structural · head c17ecba5222c · formal

No findings were raised this round, so there was nothing to verify. However, the prior major at release_freeze.py:102 (non-atomic process-stable state init) remains open — the panel's clean pass does not confirm it was fixed, and I cannot verify the fix without the diff. It is carried forward as a standing item. No panel disagreement; no Gaps.

Prior requests

	Prior finding	Disposition	Why
🔴	release_freeze.py:102	open	The panel returned zero findings this round; I cannot confirm from the empty result that the non-atomic init was fixed. Carrying forward until a round with dif…

Findings

	Severity	Location	Finding	Verified
🟠	major	release_freeze.py:102	Non-atomic process-stable state init allows two threads to hold different locks/dicts.	confirmed

findings JSON (machine-readable)

[
  {
    "file": "release_freeze.py",
    "line": 102,
    "severity": "major",
    "category": "correctness",
    "claim": "Non-atomic process-stable state init allows two threads to hold different locks/dicts.",
    "evidence": "Prior-round confirmed major; not re-verified this round (panel returned empty findings).",
    "since": "4eb3274f68cbcca739b980d4f041b86e83d32a2f",
    "verdict": "confirmed",
    "note": "re-listed without a fresh verifier ruling on a line unchanged since a verifier confirmed it \u2014 the earlier confirmation stands (#232)",
    "carried": true,
    "carried_by": "synthesizer"
  }
]

---

1 finding(s) excluded from the verdict by in-diff confinement (file not among this PR's changed paths):

• release_freeze.py (major) — Non-atomic process-stable state init allows two threads to hold different locks/dicts.

[protoreview]

Promoting the PASS verdict for head c17ecba5222c: all checks terminal-green, zero unresolved review threads. (approve-on-green)

Open findings carried by this approval — non-blocking, but they did not go away:

• major release_freeze.py:102 — Non-atomic process-stable state init allows two threads to hold different locks/dicts.

Approving a WARN does not resolve its findings (issue #22).