diff --git a/.copilot/skills/triage-dependabot/README.md b/.copilot/skills/triage-dependabot/README.md index 06bc1d8..7c6f4ea 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` and force-close via `gh pr close`) | +| Target version is a prerelease (alpha / beta / rc / dev / preview) | close-prerelease (force-close via `gh pr close --delete-branch`) | | `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,19 +52,20 @@ 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 -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 +calls `gh pr close --delete-branch` to force the PR shut via the API. +The script used to also post `@dependabot close` first, but Dependabot +has historically ignored that 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`. +posted the directive 12+ times before the PR actually closed) and once +the direct close lands the comment is pure noise on the PR timeline, so +the comment was dropped. 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 @@ -94,10 +95,9 @@ 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"` - 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; +- **Close-prerelease**: `gh pr close --delete-branch` (force-close via + the API, so we do not depend on Dependabot acting on a 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 99d58e4..64d76a6 100644 --- a/.copilot/skills/triage-dependabot/SKILL.md +++ b/.copilot/skills/triage-dependabot/SKILL.md @@ -31,12 +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` 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. The direct close exists because - Dependabot has historically ignored the comment for hours. + - `close-prerelease` - 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. The direct API close exists because Dependabot + has historically ignored `@dependabot close` comments 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 d7ff326..52d64a5 100644 --- a/.copilot/skills/triage-dependabot/tests.py +++ b/.copilot/skills/triage-dependabot/tests.py @@ -2889,7 +2889,7 @@ def test_load_and_write_todo_roundtrips_ruby_method_and_backticks( # --------------------------------------------------------------------------- -# Bug 5 / FR: prerelease (alpha/beta/rc/dev/preview) bumps -> @dependabot close +# Bug 5 / FR: prerelease (alpha/beta/rc/dev/preview) bumps -> force-close via gh pr close # --------------------------------------------------------------------------- @@ -2987,20 +2987,11 @@ def test_do_dependabot_close_dry_run_no_subprocess() -> None: mocked.assert_not_called() -def test_do_dependabot_close_posts_correct_comment() -> None: +def test_do_dependabot_close_force_closes_via_api() -> None: with mock.patch.object(td, "run_gh") as mocked: td.do_dependabot_close("github-community-projects/stale-repos", 520, dry_run=False) - 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 mocked.call_count == 1 + close_args = mocked.call_args_list[0][0][0] assert close_args[:2] == ["pr", "close"] assert "520" in close_args assert "--repo" in close_args @@ -3008,11 +2999,25 @@ def test_do_dependabot_close_posts_correct_comment() -> None: assert "--delete-branch" in close_args +def test_do_dependabot_close_does_not_post_dependabot_close_comment() -> None: + """Regression guard for the comment-spam pathology. Posting + ``@dependabot close`` is pure noise on the PR timeline now that + ``gh pr close --delete-branch`` closes the PR directly, so the + comment must never be sent.""" + with mock.patch.object(td, "run_gh") as mocked: + td.do_dependabot_close("github-community-projects/stale-repos", 520, dry_run=False) + for call in mocked.call_args_list: + args = call[0][0] + assert args[:2] != ["pr", "comment"], ( + "do_dependabot_close must not post any PR comment; got " + f"{args!r}" + ) + + 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.""" + race where Dependabot or a parallel run closed the PR before we + got there), the tool must not crash.""" def _run_gh_side_effect(args: list[str], *, timeout: int = 60) -> str: if args[:2] == ["pr", "close"]: @@ -3027,7 +3032,7 @@ def _run_gh_side_effect(args: list[str], *, timeout: int = 60) -> str: td.do_dependabot_close( "github-community-projects/contributors", 496, dry_run=False ) - assert mocked.call_count == 2 + assert mocked.call_count == 1 def test_do_dependabot_close_propagates_real_close_failure() -> None: @@ -3071,9 +3076,10 @@ def _run_gh_side_effect(args: list[str], *, timeout: int = 60) -> str: def test_run_closes_prerelease_and_marks_notification_done(tmp_path: Path) -> None: - """End-to-end: a Dependabot PR targeting a beta version posts - '@dependabot close', marks the notification done, and applies the - cooldown so the next run skips it.""" + """End-to-end: a Dependabot PR targeting a beta version is + force-closed via ``gh pr close --delete-branch``, the notification + is marked done, and the cooldown is applied so the next run skips + it.""" notif = { "id": "thread-prerelease", "reason": "subscribed", @@ -3219,8 +3225,9 @@ def test_run_close_prerelease_cooldown_set_when_mark_done_fails( 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.""" + notification and re-run the close path against an already-closed + PR (reproducing the contributors#496 churn pattern from a + different angle).""" notif = { "id": "thread-prerelease-flaky-mark", "reason": "subscribed", diff --git a/.copilot/skills/triage-dependabot/triage_dependabot.py b/.copilot/skills/triage-dependabot/triage_dependabot.py index f535cdf..d0e80fe 100644 --- a/.copilot/skills/triage-dependabot/triage_dependabot.py +++ b/.copilot/skills/triage-dependabot/triage_dependabot.py @@ -457,7 +457,7 @@ def is_prerelease_target(pr: dict[str, Any]) -> bool: Scans both the title and body so single-package PRs and grouped PRs are both detected. Conservative: any prerelease target anywhere in - the PR text routes the PR to ``@dependabot close`` rather than + the PR text routes the PR to the close-prerelease branch rather than auto-merging an unstable release. """ title = pr.get("title") or "" @@ -1203,28 +1203,23 @@ def _is_already_closed_stderr(stderr: str) -> bool: 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. + """Force-close a prerelease bump PR via the GitHub API. + + Calls ``gh pr close --delete-branch`` to shut the PR directly. We + used to also post ``@dependabot close`` first, but Dependabot + historically ignored that comment for hours (see + github-community-projects/contributors#496 where the cron posted + the directive 12+ times before the PR actually closed) and once the + direct close lands the comment is pure noise on the PR timeline, so + the comment was dropped. + + Only the narrow "already closed / not found" race is swallowed + (e.g. a parallel run, or Dependabot closing the PR itself between + cron ticks). 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. @@ -1232,24 +1227,8 @@ def do_dependabot_close(repo: str, number: int, *, dry_run: bool) -> None: repo's ``.github/dependabot.yml``. """ if dry_run: - logger.info( - "dry-run: would post '@dependabot close' and force-close %s#%d", - repo, - number, - ) + logger.info("dry-run: would force-close %s#%d", repo, number) return - run_gh( - [ - "pr", - "comment", - str(number), - "--repo", - repo, - "--body", - "@dependabot close", - ], - timeout=30, - ) try: run_gh( [