fix(triage-dependabot): force-close prerelease PRs + preserve cooldown on mark-done flakes

.copilot/skills/triage-dependabot/README.md

@@ -28,7 +28,7 @@ by `dependabot[bot]` or `dependabot-preview[bot]`:
 | Title or body references an excluded dependency AND notification reason is `mention`, `team_mention`, `author`, or `manual` | skip, leave notification in inbox for direct response |
 | Draft PR | flag-for-review |
 | Any non-bot human (other than me) reviewed or commented | flag-for-review |
-| Target version is a prerelease (alpha / beta / rc / dev / preview) | close-prerelease (comment `@dependabot close`) |
+| Target version is a prerelease (alpha / beta / rc / dev / preview) | close-prerelease (comment `@dependabot close` and force-close via `gh pr close`) |
 | `mergeStateStatus` is `behind` or `dirty` | rebase (suppressed if my last rebase comment is newer than the latest dependabot push) |
 | Bump is major / minor / unknown AND repo coverage threshold below 90 | flag-for-review |
 | CI status pending | skip this run; next hour retries |
@@ -52,10 +52,19 @@ unstable release. Recognized prerelease forms:
 
 Docker build variants (`-slim`, `-alpine`, `-bookworm`) and PEP 440
 post-releases (`1.0.0.post1`) are NOT treated as prereleases. The action
-is `@dependabot close`; Dependabot may open a new PR if the upstream
-releases another prerelease, and the next run closes that one too. For
-a permanent skip, add an `ignore` rule in the repo's
-`.github/dependabot.yml`.
+posts `@dependabot close` (so Dependabot's tracking records the
+directive) and then immediately calls `gh pr close --delete-branch` to
+force the PR shut via the API. The direct close exists because
+Dependabot has historically ignored the comment for hours (see
+`github-community-projects/contributors#496`, where the hourly cron
+posted the directive 12+ times before the PR actually closed). Only a
+narrow "already closed / not found" race on the close call is
+swallowed; any other failure (auth, rate limit, timeout, branch
+deletion) propagates so the outer run loop records it and the next
+cron tick retries instead of silently treating the PR as handled.
+Dependabot may open a new PR if the upstream releases another
+prerelease, and the next run closes that one too. For a permanent
+skip, add an `ignore` rule in the repo's `.github/dependabot.yml`.
 
 ### Excluded dependencies
 
@@ -85,8 +94,10 @@ conservative default.
   notification stays open so the next push triggers another evaluation.
 - **Label-and-merge**: `gh pr edit --add-label release` (only if the
   repo defines a `release` label) followed by auto-merge.
-- **Close-prerelease**: `gh pr comment --body "@dependabot close"` for
-  PRs whose target version is an alpha / beta / rc / dev / preview;
+- **Close-prerelease**: `gh pr comment --body "@dependabot close"`
+  followed by `gh pr close --delete-branch` (force-close via the API,
+  so we do not depend on Dependabot acting on the comment) for PRs
+  whose target version is an alpha / beta / rc / dev / preview;
   notification is marked done and the cooldown is applied.
 - **Flag-for-review**: a Q1 entry in
   `~/repos/zkoppert-todo/todo.yml` under

.copilot/skills/triage-dependabot/SKILL.md

@@ -31,10 +31,12 @@ upgrades or seeing a backlog of dependabot notifications.
    - `label-and-merge` - add the `release` label (if the repo defines
      one) and enable auto-merge, for changes the Copilot sub-agent or
      fallback regex classifies as security-related.
-   - `close-prerelease` - comment `@dependabot close` when the target
+   - `close-prerelease` - comment `@dependabot close` and then
+     force-close via `gh pr close --delete-branch` when the target
      version is a prerelease (alpha / beta / rc / dev / preview).
      Catches PRs like `bump python from 3.14.5-slim to 3.15.0b2-slim`
-     that should never auto-merge.
+     that should never auto-merge. The direct close exists because
+     Dependabot has historically ignored the comment for hours.
    - `flag-for-review` - write a Q1 entry to
      `~/repos/zkoppert-todo/todo.yml` for human attention.
 4. Marks the notification done on GitHub when an action runs.

.copilot/skills/triage-dependabot/tests.py

@@ -10,6 +10,7 @@
 import argparse
 import datetime
 import json
+import subprocess
 from pathlib import Path
 from typing import Any
 from unittest import mock
@@ -2989,15 +2990,84 @@ def test_do_dependabot_close_dry_run_no_subprocess() -> None:
 def test_do_dependabot_close_posts_correct_comment() -> None:
     with mock.patch.object(td, "run_gh") as mocked:
         td.do_dependabot_close("github-community-projects/stale-repos", 520, dry_run=False)
-    mocked.assert_called_once()
-    args = mocked.call_args[0][0]
-    assert args[:2] == ["pr", "comment"]
-    assert "520" in args
-    assert "--repo" in args
-    assert "github-community-projects/stale-repos" in args
-    assert "--body" in args
-    body_idx = args.index("--body")
-    assert args[body_idx + 1] == "@dependabot close"
+    assert mocked.call_count == 2
+    comment_call, close_call = mocked.call_args_list
+    comment_args = comment_call[0][0]
+    assert comment_args[:2] == ["pr", "comment"]
+    assert "520" in comment_args
+    assert "--repo" in comment_args
+    assert "github-community-projects/stale-repos" in comment_args
+    assert "--body" in comment_args
+    body_idx = comment_args.index("--body")
+    assert comment_args[body_idx + 1] == "@dependabot close"
+    close_args = close_call[0][0]
+    assert close_args[:2] == ["pr", "close"]
+    assert "520" in close_args
+    assert "--repo" in close_args
+    assert "github-community-projects/stale-repos" in close_args
+    assert "--delete-branch" in close_args
+
+
+def test_do_dependabot_close_swallows_already_closed_error() -> None:
+    """If ``gh pr close`` fails with an 'already closed' stderr (a benign
+    race where Dependabot or a parallel run closed the PR between the
+    comment and the close), the comment has still landed and the tool
+    must not crash."""
+
+    def _run_gh_side_effect(args: list[str], *, timeout: int = 60) -> str:
+        if args[:2] == ["pr", "close"]:
+            raise subprocess.CalledProcessError(
+                1,
+                ["gh", *args],
+                stderr="pull request is closed",
+            )
+        return ""
+
+    with mock.patch.object(td, "run_gh", side_effect=_run_gh_side_effect) as mocked:
+        td.do_dependabot_close(
+            "github-community-projects/contributors", 496, dry_run=False
+        )
+    assert mocked.call_count == 2
+
+
+def test_do_dependabot_close_propagates_real_close_failure() -> None:
+    """If ``gh pr close`` fails for a real reason (auth, rate limit,
+    branch deletion, network), the error MUST propagate so the outer
+    run loop records it and skips the cooldown / mark-done that would
+    otherwise hide the still-open PR. Without this, the fix would
+    silently re-introduce the spam pathology it is trying to escape."""
+
+    def _run_gh_side_effect(args: list[str], *, timeout: int = 60) -> str:
+        if args[:2] == ["pr", "close"]:
+            raise subprocess.CalledProcessError(
+                1,
+                ["gh", *args],
+                stderr="HTTP 403: API rate limit exceeded for installation",
+            )
+        return ""
+
+    with mock.patch.object(td, "run_gh", side_effect=_run_gh_side_effect):
+        with pytest.raises(subprocess.CalledProcessError):
+            td.do_dependabot_close(
+                "github-community-projects/contributors", 496, dry_run=False
+            )
+
+
+def test_do_dependabot_close_propagates_timeout() -> None:
+    """A timeout on the close call leaves the PR in unknown state -
+    propagate so the outer loop retries on the next cron tick instead
+    of silently marking the notification done."""
+
+    def _run_gh_side_effect(args: list[str], *, timeout: int = 60) -> str:
+        if args[:2] == ["pr", "close"]:
+            raise subprocess.TimeoutExpired(cmd=["gh", *args], timeout=timeout)
+        return ""
+
+    with mock.patch.object(td, "run_gh", side_effect=_run_gh_side_effect):
+        with pytest.raises(subprocess.TimeoutExpired):
+            td.do_dependabot_close(
+                "github-community-projects/contributors", 496, dry_run=False
+            )
 
 
 def test_run_closes_prerelease_and_marks_notification_done(tmp_path: Path) -> None:
@@ -3048,6 +3118,213 @@ def test_run_closes_prerelease_and_marks_notification_done(tmp_path: Path) -> No
     )
 
 
+def test_run_merge_cooldown_set_when_mark_done_fails(tmp_path: Path) -> None:
+    """If mark_thread_done throws after a successful merge, the cooldown
+    state MUST still be written so the next cron tick does not try to
+    re-merge the already-merged PR. The mark-done failure is recorded
+    in stats.errors and processing continues.
+
+    Regression guard for a latent bug at the run-loop call site where
+    a notifications-API hiccup mid-tick would jump to the outer except
+    and leave state[pr_url] unset, causing the next tick to re-run
+    do_merge against a closed PR."""
+    notif = {
+        "id": "thread-merge-flaky-mark",
+        "reason": "subscribed",
+        "subject": {
+            "type": "PullRequest",
+            "url": "https://api.github.com/repos/o/r1/pulls/1",
+        },
+    }
+    pr = _base_pr(number=1, url="https://github.com/o/r1/pull/1")
+    args = _make_args(tmp_path)
+
+    with mock.patch.object(
+        td, "get_my_login", return_value="zkoppert"
+    ), mock.patch.object(
+        td, "fetch_notifications", return_value=[notif]
+    ), mock.patch.object(
+        td, "fetch_pr", return_value=pr
+    ), mock.patch.object(
+        td, "detect_repo_coverage", return_value=95
+    ), mock.patch.object(
+        td, "do_merge"
+    ), mock.patch.object(
+        td,
+        "mark_thread_done",
+        side_effect=subprocess.CalledProcessError(1, ["gh"], stderr="HTTP 500"),
+    ):
+        stats = td.run(args)
+
+    assert stats.merged == 1
+    assert any("mark-done failed for merged" in e for e in stats.errors)
+    state = td.load_state(args.state_file)
+    assert "https://github.com/o/r1/pull/1" in state
+
+
+def test_run_label_and_merge_cooldown_set_when_mark_done_fails(
+    tmp_path: Path,
+) -> None:
+    """Same cooldown-preservation invariant for the label-and-merge path."""
+    notif = {
+        "id": "thread-lm-flaky-mark",
+        "reason": "subscribed",
+        "subject": {
+            "type": "PullRequest",
+            "url": "https://api.github.com/repos/o/r1/pulls/7",
+        },
+    }
+    pr = _base_pr(
+        number=7,
+        url="https://github.com/o/r1/pull/7",
+        title="chore(deps): bump cryptography from 41.0.0 to 41.0.7 (CVE-2024-1234)",
+        body="Fixes CVE-2024-1234, a security advisory.",
+    )
+    args = _make_args(tmp_path)
+
+    with mock.patch.object(
+        td, "get_my_login", return_value="zkoppert"
+    ), mock.patch.object(
+        td, "fetch_notifications", return_value=[notif]
+    ), mock.patch.object(
+        td, "fetch_pr", return_value=pr
+    ), mock.patch.object(
+        td, "detect_repo_coverage", return_value=95
+    ), mock.patch.object(
+        td, "fetch_repo_labels", return_value={"release"}
+    ), mock.patch.object(
+        td, "is_security_change", return_value=True
+    ), mock.patch.object(
+        td, "do_add_label"
+    ), mock.patch.object(
+        td, "do_merge"
+    ), mock.patch.object(
+        td,
+        "mark_thread_done",
+        side_effect=subprocess.TimeoutExpired(cmd=["gh"], timeout=20),
+    ):
+        stats = td.run(args)
+
+    assert stats.labeled_and_merged == 1
+    assert any("mark-done failed for labeled-and-merged" in e for e in stats.errors)
+    state = td.load_state(args.state_file)
+    assert "https://github.com/o/r1/pull/7" in state
+
+
+def test_run_close_prerelease_cooldown_set_when_mark_done_fails(
+    tmp_path: Path,
+) -> None:
+    """Same cooldown-preservation invariant for the close-prerelease path.
+
+    Without this guard a notifications-API hiccup right after the
+    direct close would leave the cooldown unset, and the next cron
+    tick (within 1 hour) could re-encounter the still-listed
+    notification, re-post the @dependabot close comment, and reproduce
+    the contributors#496 spam pattern."""
+    notif = {
+        "id": "thread-prerelease-flaky-mark",
+        "reason": "subscribed",
+        "subject": {
+            "type": "PullRequest",
+            "url": "https://api.github.com/repos/o/r1/pulls/520",
+        },
+    }
+    pr = _base_pr(
+        number=520,
+        url="https://github.com/o/r1/pull/520",
+        title="chore(deps): bump python from 3.14.5-slim to 3.15.0b2-slim",
+    )
+    args = _make_args(tmp_path)
+
+    with mock.patch.object(
+        td, "get_my_login", return_value="zkoppert"
+    ), mock.patch.object(
+        td, "fetch_notifications", return_value=[notif]
+    ), mock.patch.object(
+        td, "fetch_pr", return_value=pr
+    ), mock.patch.object(
+        td, "do_dependabot_close"
+    ), mock.patch.object(
+        td,
+        "mark_thread_done",
+        side_effect=subprocess.CalledProcessError(
+            1, ["gh"], stderr="HTTP 403: API rate limit exceeded"
+        ),
+    ):
+        stats = td.run(args)
+
+    assert stats.closed_prerelease == 1
+    assert any("mark-done failed for closed-prerelease" in e for e in stats.errors)
+    state = td.load_state(args.state_file)
+    assert "https://github.com/o/r1/pull/520" in state
+
+
+def test_safe_mark_thread_done_no_thread_is_noop() -> None:
+    """Empty thread_id is the documented no-op (matches the prior
+    ``if thread_id:`` guard at the call sites)."""
+    stats = td.TriageStats()
+    with mock.patch.object(td, "mark_thread_done") as mocked:
+        result = td._safe_mark_thread_done(
+            "", dry_run=False, stats=stats, context="ctx"
+        )
+    assert result is True
+    mocked.assert_not_called()
+    assert stats.errors == []
+
+
+def test_safe_mark_thread_done_records_failure() -> None:
+    """A flaky mark-done returns False and appends to stats.errors so
+    callers can branch on it if they need to."""
+    stats = td.TriageStats()
+    with mock.patch.object(
+        td,
+        "mark_thread_done",
+        side_effect=subprocess.CalledProcessError(1, ["gh"], stderr="HTTP 500"),
+    ):
+        result = td._safe_mark_thread_done(
+            "thread-x", dry_run=False, stats=stats, context="merged https://x/y"
+        )
+    assert result is False
+    assert len(stats.errors) == 1
+    assert "mark-done failed for merged https://x/y" in stats.errors[0]
+
+
+def test_run_terminal_skip_cooldown_set_when_mark_done_fails(
+    tmp_path: Path,
+) -> None:
+    """Same cooldown-preservation invariant for the terminal-skip path
+    (closed/merged PRs). Without this guard, a flaky mark-done on a
+    closed PR would leave the cooldown unset, and the next cron tick
+    would re-fetch the same closed PR and retry mark-done forever."""
+    notif = {
+        "id": "thread-closed-flaky-mark",
+        "subject": {
+            "type": "PullRequest",
+            "url": "https://api.github.com/repos/o/r/pulls/9",
+        },
+    }
+    pr = _base_pr(number=9, url="https://github.com/o/r/pull/9", state="closed")
+    args = _make_args(tmp_path)
+
+    with mock.patch.object(
+        td, "get_my_login", return_value="zkoppert"
+    ), mock.patch.object(
+        td, "fetch_notifications", return_value=[notif]
+    ), mock.patch.object(
+        td, "fetch_pr", return_value=pr
+    ), mock.patch.object(
+        td,
+        "mark_thread_done",
+        side_effect=subprocess.CalledProcessError(1, ["gh"], stderr="HTTP 500"),
+    ):
+        stats = td.run(args)
+
+    assert stats.skipped == 1
+    assert any("mark-done failed for terminal-skip" in e for e in stats.errors)
+    state = td.load_state(args.state_file)
+    assert "https://github.com/o/r/pull/9" in state
+
+
 def test_run_closes_prerelease_dry_run_does_not_post(tmp_path: Path) -> None:
     """Dry-run prerelease close: count the action, don't actually post the
     comment, don't mark the thread done, don't write state."""