diff --git a/.copilot/skills/triage-dependabot/README.md b/.copilot/skills/triage-dependabot/README.md index 80bbc73..06bc1d8 100644 --- a/.copilot/skills/triage-dependabot/README.md +++ b/.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 diff --git a/.copilot/skills/triage-dependabot/SKILL.md b/.copilot/skills/triage-dependabot/SKILL.md index ae1c65d..99d58e4 100644 --- a/.copilot/skills/triage-dependabot/SKILL.md +++ b/.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. diff --git a/.copilot/skills/triage-dependabot/tests.py b/.copilot/skills/triage-dependabot/tests.py index d7313fa..d7ff326 100644 --- a/.copilot/skills/triage-dependabot/tests.py +++ b/.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.""" diff --git a/.copilot/skills/triage-dependabot/triage_dependabot.py b/.copilot/skills/triage-dependabot/triage_dependabot.py index d8a6fd7..f535cdf 100644 --- a/.copilot/skills/triage-dependabot/triage_dependabot.py +++ b/.copilot/skills/triage-dependabot/triage_dependabot.py @@ -1183,17 +1183,60 @@ def do_add_label(repo: str, number: int, label: str, *, dry_run: bool) -> None: ) -def do_dependabot_close(repo: str, number: int, *, dry_run: bool) -> None: - """Post the ``@dependabot close`` comment to close a prerelease bump PR. +def _is_already_closed_stderr(stderr: str) -> bool: + """Detect gh's "already closed / not found" stderr signatures. + + Used to distinguish a benign race (the PR was closed between the + comment and the close call, e.g. by a parallel run or by Dependabot + itself acting on the comment) from a real failure (auth expiry, + rate limit, transient 5xx, branch-deletion failure) that must + propagate so the outer run loop records it and the next cron tick + retries instead of silently treating it as success. + """ + lowered = stderr.lower() + return ( + "already closed" in lowered + or "pull request is closed" in lowered + or "could not resolve to a pullrequest" in lowered + or "not found" in lowered + ) + - Dependabot treats this comment as a directive to close the PR (and the - backing branch) so the noise stops. If the upstream releases another - prerelease later Dependabot may open a new PR; the next run of this - tool will close that one too. For a permanent skip, add an - ``ignore`` rule for prereleases in the repo's ``.github/dependabot.yml``. +def do_dependabot_close(repo: str, number: int, *, dry_run: bool) -> None: + """Close a prerelease bump PR via both a directive comment and a direct close. + + Two-step closure: + + 1. Post ``@dependabot close`` so Dependabot's own tracking sees the + directive (helps it record that the PR was intentionally closed + and avoid immediately re-opening for the same prerelease). + 2. Call ``gh pr close --delete-branch`` to force-close the PR + directly via the API. Dependabot has historically ignored the + ``@dependabot close`` comment for hours (see + github-community-projects/contributors#496 where the cron posted + the directive 12+ times before the PR actually closed), so the + direct close guarantees the PR shuts on the first cron tick and + stops the comment spam. + + Only the narrow "already closed / not found" race is swallowed (e.g. + a parallel run, or Dependabot acting on the comment between the two + calls). Every other ``gh pr close`` failure - auth, rate limit, + timeout, transient API error, branch-deletion failure - propagates + so the outer run loop records it in ``stats.errors`` and skips the + ``mark_thread_done`` + cooldown that would otherwise hide the open + PR until the next cron tick. + + If the upstream releases another prerelease later Dependabot may + open a new PR; the next run of this tool will close that one too. + For a permanent skip, add an ``ignore`` rule for prereleases in the + repo's ``.github/dependabot.yml``. """ if dry_run: - logger.info("dry-run: would post '@dependabot close' on %s#%d", repo, number) + logger.info( + "dry-run: would post '@dependabot close' and force-close %s#%d", + repo, + number, + ) return run_gh( [ @@ -1207,6 +1250,28 @@ def do_dependabot_close(repo: str, number: int, *, dry_run: bool) -> None: ], timeout=30, ) + try: + run_gh( + [ + "pr", + "close", + str(number), + "--repo", + repo, + "--delete-branch", + ], + timeout=30, + ) + except subprocess.CalledProcessError as exc: + stderr = (exc.stderr or "") if hasattr(exc, "stderr") else "" + if _is_already_closed_stderr(stderr): + logger.info( + "do_dependabot_close: %s#%d already closed; nothing to do", + repo, + number, + ) + return + raise def mark_thread_done(thread_id: str, *, dry_run: bool) -> None: @@ -1219,6 +1284,42 @@ def mark_thread_done(thread_id: str, *, dry_run: bool) -> None: ) +def _safe_mark_thread_done( + thread_id: str, + *, + dry_run: bool, + stats: TriageStats, + context: str, +) -> bool: + """Best-effort wrapper around ``mark_thread_done`` for post-action cleanup. + + Used after a successful action (merge / label-and-merge / + close-prerelease / terminal-skip) where the action has already + landed and the per-PR cooldown has already been written. A flaky + notifications API call (``CalledProcessError`` / + ``TimeoutExpired``) must NOT unwind any of that - otherwise the + outer ``except`` would skip the cooldown set, and the next cron + tick would re-attempt the already-completed action (re-merge, + re-close, re-comment) producing the exact spam pathology this + module is built to avoid. + + Failures are recorded in ``stats.errors`` and logged. ``thread_id`` + of empty / falsy is a no-op (matches the existing convention at + the call sites that guard with ``if thread_id``). + + Returns True on success or no-op, False if the call raised. + """ + if not thread_id: + return True + try: + mark_thread_done(thread_id, dry_run=dry_run) + return True + except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as exc: + stats.errors.append(f"mark-done failed for {context}: {exc}") + logger.warning("mark_thread_done failed for %s: %s", context, exc) + return False + + # --------------------------------------------------------------------------- # todo.yml integration # --------------------------------------------------------------------------- @@ -1604,24 +1705,21 @@ def coverage_lookup(repo: str) -> int | None: pr_url = pr.get("url") or "" logger.info("%s#%d -> skipping archived repo %s", repo, number, repo) stats.skipped_archived += 1 + if pr_url: + state[pr_url] = now if thread_id: - try: - mark_thread_done(thread_id, dry_run=args.dry_run) + if _safe_mark_thread_done( + thread_id, + dry_run=args.dry_run, + stats=stats, + context=f"archived repo {pr_url}", + ): stats.stale_removed += _cleanup_stale_entries( data, thread_id=thread_id, pr_url=pr_url, dry_run=args.dry_run, ) - except ( - subprocess.CalledProcessError, - subprocess.TimeoutExpired, - ) as exc: - stats.errors.append( - f"mark-done failed for archived repo {pr_url}: {exc}" - ) - if pr_url: - state[pr_url] = now continue skipped_dep = skipped_dependency_match(pr) or skipped_repo_match(repo) if skipped_dep: @@ -1723,9 +1821,14 @@ def coverage_lookup(repo: str) -> int | None: my_login=my_login, head_sha=pr.get("headRefOid"), ) - mark_thread_done(thread_id, dry_run=args.dry_run) stats.merged += 1 state[pr_url] = now + _safe_mark_thread_done( + thread_id, + dry_run=args.dry_run, + stats=stats, + context=f"merged {pr_url}", + ) stats.stale_removed += _cleanup_stale_entries( data, thread_id=thread_id, pr_url=pr_url, dry_run=args.dry_run ) @@ -1740,9 +1843,14 @@ def coverage_lookup(repo: str) -> int | None: my_login=my_login, head_sha=pr.get("headRefOid"), ) - mark_thread_done(thread_id, dry_run=args.dry_run) stats.labeled_and_merged += 1 state[pr_url] = now + _safe_mark_thread_done( + thread_id, + dry_run=args.dry_run, + stats=stats, + context=f"labeled-and-merged {pr_url}", + ) stats.stale_removed += _cleanup_stale_entries( data, thread_id=thread_id, pr_url=pr_url, dry_run=args.dry_run ) @@ -1752,9 +1860,14 @@ def coverage_lookup(repo: str) -> int | None: state[pr_url] = now elif decision.outcome == OUTCOME_CLOSE_PRERELEASE: do_dependabot_close(repo, number, dry_run=args.dry_run) - mark_thread_done(thread_id, dry_run=args.dry_run) stats.closed_prerelease += 1 state[pr_url] = now + _safe_mark_thread_done( + thread_id, + dry_run=args.dry_run, + stats=stats, + context=f"closed-prerelease {pr_url}", + ) stats.stale_removed += _cleanup_stale_entries( data, thread_id=thread_id, pr_url=pr_url, dry_run=args.dry_run ) @@ -1767,7 +1880,14 @@ def coverage_lookup(repo: str) -> int | None: state[pr_url] = now else: if decision.terminal and thread_id: - mark_thread_done(thread_id, dry_run=args.dry_run) + if pr_url: + state[pr_url] = now + _safe_mark_thread_done( + thread_id, + dry_run=args.dry_run, + stats=stats, + context=f"terminal-skip {pr_url}", + ) stats.stale_removed += _cleanup_stale_entries( data, thread_id=thread_id, pr_url=pr_url, dry_run=args.dry_run ) @@ -1800,15 +1920,12 @@ def coverage_lookup(repo: str) -> int | None: BRANCH_PROTECTION_COOLDOWN_SECONDS - ACTION_COOLDOWN_SECONDS ) if thread_id: - try: - mark_thread_done(thread_id, dry_run=args.dry_run) - except ( - subprocess.CalledProcessError, - subprocess.TimeoutExpired, - ) as mark_exc: - stats.errors.append( - f"mark-done failed for branch-protected {pr_url}: {mark_exc}" - ) + _safe_mark_thread_done( + thread_id, + dry_run=args.dry_run, + stats=stats, + context=f"branch-protected {pr_url}", + ) except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as exc: stats.errors.append(f"action {decision.outcome} failed for {pr_url}: {exc}")