diff --git a/README.md b/README.md index eb4c642..17f47e3 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,14 @@ for builds/checks that will never run — e.g. on Azure DevOps: on GitHub: `PR #42 ('feat: widgets') has merge conflicts with 'main' (mergeable=CONFLICTING)`. Exit code 1 either way. +With `--wait`, a build/check that's paused on a manual approval (an Azure +Pipelines stage's Checkpoint.Approval, or a GitHub Actions deployment +protection rule) ends the wait instead of polling forever — it prints which +stage/check needs a reviewer plus a link to act on it, and exits **3** +(`bdt pr watch-deploy --wait`, below, behaves the same way). If another +build/check has already failed, that's reported instead (exit 1) even when +one is also waiting on approval. + **Azure DevOps**: org/project/repo are auto-detected from `git remote get-url origin` (handles SSH, `dev.azure.com` HTTPS, and `*.visualstudio.com` HTTPS forms). Auth is an explicit PAT (`--pat` or diff --git a/bmsdna/devtools/cli.py b/bmsdna/devtools/cli.py index db9420c..3a1a02f 100644 --- a/bmsdna/devtools/cli.py +++ b/bmsdna/devtools/cli.py @@ -254,7 +254,7 @@ def pr_publish( @pr_app.command("status") def pr_status( target_branch: str = typer.Option("main", "--target-branch", help="Target branch of the PR (Azure DevOps only — gh has no equivalent filter, it always resolves the PR for the current branch)"), - wait: bool = typer.Option(False, "--wait", help="Poll until all pipelines/checks are completed"), + wait: bool = typer.Option(False, "--wait", help="Poll until all pipelines/checks are completed; stops early and reports status if one needs manual approval"), pat: str | None = typer.Option(None, "--pat", envvar=["AZURE_DEVOPS_EXT_PAT", "AZURE_DEVOPS_PAT"], help="Azure DevOps PAT (else falls back to `az` login)"), ) -> None: """Show build/check status for the PR opened from the current branch (Azure DevOps or GitHub, auto-detected).""" @@ -288,7 +288,7 @@ def pr_retry( @pr_app.command("watch-deploy") def pr_watch_deploy( target_branch: str = typer.Option("main", "--target-branch", help="Branch to watch for a directly-triggered build/workflow run (e.g. a post-merge deployment pipeline)"), - wait: bool = typer.Option(False, "--wait", help="Poll until the build/workflow run(s) are completed"), + wait: bool = typer.Option(False, "--wait", help="Poll until the build/workflow run(s) are completed; stops early and reports status if one needs manual approval"), pat: str | None = typer.Option( None, "--pat", diff --git a/bmsdna/devtools/cli_tools.py b/bmsdna/devtools/cli_tools.py index 4468d59..028b785 100644 --- a/bmsdna/devtools/cli_tools.py +++ b/bmsdna/devtools/cli_tools.py @@ -18,6 +18,13 @@ GH_INSTALL_HINT = "Install the GitHub CLI: https://cli.github.com" PSQL_INSTALL_HINT = "Install the PostgreSQL client tools (psql): https://www.postgresql.org/download/" +# `pr status --wait` exits with this code (not 0=success, not 1=CI failure) when it stops +# because a build/check needs a human to approve it — there's nothing more the CLI can do +# but wait indefinitely, which defeats the point of --wait. Deliberately not 2: that's +# Click/Typer's own exit code for a CLI usage error (e.g. typer.BadParameter elsewhere in +# this tool), and callers branching on exit code shouldn't confuse the two. +EXIT_NEEDS_APPROVAL = 3 + def is_claude_code() -> bool: """True if this process is running as a subprocess of Claude Code. diff --git a/bmsdna/devtools/gh_pr.py b/bmsdna/devtools/gh_pr.py index 07cc7d9..ba3fffc 100644 --- a/bmsdna/devtools/gh_pr.py +++ b/bmsdna/devtools/gh_pr.py @@ -18,7 +18,7 @@ import time from pathlib import Path -from .cli_tools import detect_agent_session, ensure_agent_session_note, is_claude_code +from .cli_tools import EXIT_NEEDS_APPROVAL, detect_agent_session, ensure_agent_session_note, is_claude_code from .pr_markdown import build_attachments_section, build_comment_content, build_screenshots_section PR_VIEW_FIELDS = "number,title,baseRefName,mergeable,statusCheckRollup,isDraft" @@ -78,6 +78,11 @@ def get_pr(gh: str) -> dict: def check_bucket(check: dict) -> str: if check.get("__typename") == "StatusContext": return _STATUS_CONTEXT_BUCKET.get(check.get("state"), "pending") + # CheckRun.status "WAITING" is GitHub's distinct state for a run paused on a deployment + # protection rule (e.g. a required reviewer on the target environment) — unlike ordinary + # "still running" states, nothing here resolves on its own without a human. + if check.get("status") == "WAITING": + return "waiting_approval" if check.get("status") != "COMPLETED": return "pending" return _CHECK_RUN_BUCKET.get(check.get("conclusion"), "fail") @@ -224,10 +229,23 @@ def deploy_run_hint(gh: str, target_branch: str) -> str | None: def print_check(check: dict) -> None: bucket = check_bucket(check) - icon = {"pass": "✓", "fail": "✗", "cancel": "⊘"}.get(bucket, "…") + icon = {"pass": "✓", "fail": "✗", "cancel": "⊘", "waiting_approval": "⏸"}.get(bucket, "…") print(f" [{icon} {bucket.upper()}] {check_label(check)}") +def exit_needs_approval(msg: str, item_lines: list[str], rerun_cmd: str) -> None: + """Print the "Waiting for approval" report for already-formatted `item_lines` and exit + EXIT_NEEDS_APPROVAL -- shared by `run()` and `run_watch_deploy()`, which differ only in + what a "blocked" item is (a check vs. a workflow run) and how to label one. + """ + print(msg) + print("\nWaiting for approval:") + for line in item_lines: + print(f" {line}") + print(f"\nApprove at the link(s) above, then re-run `{rerun_cmd}`.") + sys.exit(EXIT_NEEDS_APPROVAL) + + def run(gh: str, wait: bool) -> None: last_line = "" draft_notice_shown = False @@ -271,7 +289,22 @@ def run(gh: str, wait: bool) -> None: buckets = [check_bucket(c) for c in checks] msg += " | " + ", ".join(f"{check_label(c)}: {check_bucket(c)}" for c in checks) - if "pending" in buckets and wait: + waiting_approval = [c for c, b in zip(checks, buckets) if b == "waiting_approval"] + # A failed check anywhere in the PR is reported as such even when another check is + # separately waiting on approval — a human shouldn't be sent to go approve a + # deployment gate while staying unaware that CI has already failed elsewhere. + if wait and waiting_approval and "fail" not in buckets: + item_lines = [] + for c in waiting_approval: + details_url = c.get("detailsUrl") + suffix = f" — {details_url}" if details_url else "" + item_lines.append(f"{check_label(c)} needs a reviewer to approve the deployment{suffix}") + exit_needs_approval(msg, item_lines, "bdt pr status --wait") + + # A check waiting on approval never resolves on its own -- if some other still-pending + # check only looks pending because it's downstream of that same approval gate, --wait + # must not keep polling forever waiting for it to become unstuck. + if "pending" in buckets and wait and not waiting_approval: if msg != last_line: print(msg, end="", flush=True) last_line = msg @@ -287,9 +320,10 @@ def run(gh: str, wait: bool) -> None: print(f"\n{retry_hint()}") sys.exit(1) # Only worth suggesting `pr watch-deploy` once this PR's own checks are actually - # settled (not still pending because the caller ran without --wait) -- otherwise - # it'd claim a merge/deploy is underway before the PR has even finished its own CI. - if "pending" not in buckets: + # settled (not still pending, and not sitting on an unresolved approval prompt, + # because the caller ran without --wait) -- otherwise it'd claim a merge/deploy is + # underway before the PR has even finished its own CI. + if "pending" not in buckets and "waiting_approval" not in buckets: hint = deploy_run_hint(gh, base) if hint: print(hint) @@ -346,7 +380,25 @@ def run_watch_deploy(gh: str, target_branch: str, wait: bool) -> None: for r in latest_runs ) - all_done = all(r.get("status") == "completed" for r in latest_runs) + already_failed = any(r.get("conclusion") == "failure" for r in latest_runs) + waiting_approval = [r for r in latest_runs if r.get("status") == "waiting"] + # WorkflowRun.status "waiting" is a distinct state for a run paused on a deployment + # protection rule (e.g. a required reviewer on the target environment) -- unlike + # ordinary in-progress runs, nothing here resolves on its own without a human. A run + # that's already failed elsewhere is reported as such (exit 1) instead, same priority + # as `run()` -- a human shouldn't be sent to go approve a deployment while staying + # unaware CI already failed. + if wait and waiting_approval and not already_failed: + item_lines = [ + f"{r.get('workflowName') or r.get('name') or '?'} #{r['databaseId']} needs a reviewer to approve the deployment — {r.get('url', '?')}" + for r in waiting_approval + ] + exit_needs_approval(msg, item_lines, "bdt pr watch-deploy --wait") + + # A run stuck on approval must not be treated as "still in progress" once something + # else has already failed -- otherwise --wait would poll forever for a run that can + # never resolve on its own instead of reporting the failure. + all_done = already_failed or all(r.get("status") == "completed" for r in latest_runs) if all_done or not wait: print(msg) print("\nDetails:") diff --git a/bmsdna/devtools/pr_build.py b/bmsdna/devtools/pr_build.py index 0effa16..6207c77 100644 --- a/bmsdna/devtools/pr_build.py +++ b/bmsdna/devtools/pr_build.py @@ -11,13 +11,18 @@ import requests from .ado_auth import auth_header -from .cli_tools import detect_agent_session, ensure_agent_session_note, is_claude_code +from .cli_tools import EXIT_NEEDS_APPROVAL, detect_agent_session, ensure_agent_session_note, is_claude_code from .gitrepo import AdoRemote, current_branch from .pr_markdown import build_attachments_section, build_comment_content, build_screenshots_section # Matches an ISO 8601 timestamp at the start of a log line, e.g. 2024-03-21T15:01:23.1234567Z TIMESTAMP_RE = re.compile(r"^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d+)?Z\s*") +# Timeline record name for a YAML pipeline stage's manual-approval check. Its `state` stays +# "inProgress" (like an ordinary running step) until someone approves/rejects it or it times +# out — indistinguishable from "still building" unless you look at the timeline specifically. +CHECKPOINT_APPROVAL_NAME = "Checkpoint.Approval" + # GitPullRequest.mergeStatus values (PullRequestAsyncStatus) that mean the PR # can't be merged as-is — build status is moot until this is resolved. BAD_MERGE_STATUSES = { @@ -369,6 +374,59 @@ def latest_per_pipeline(builds: list) -> list: return sorted(latest.values(), key=lambda b: b["id"], reverse=True) +def build_web_url(remote: AdoRemote, build_id: int) -> str: + """The browsable web page for a build, where a pending approval can actually be acted on.""" + return f"{_base_url(remote)}/_build/results?buildId={build_id}&view=results" + + +def get_timeline_records(session: requests.Session, remote: AdoRemote, build_id: int) -> list: + r = session.get(f"{_base_url(remote)}/_apis/build/builds/{build_id}/timeline", params={"api-version": "7.1"}) + r.raise_for_status() + return r.json().get("records") or [] + + +def pending_approval_records(records: list) -> list: + """Timeline records for still-open `Checkpoint.Approval` gates (manual stage approvals).""" + return [rec for rec in records if rec.get("state") == "inProgress" and rec.get("name") == CHECKPOINT_APPROVAL_NAME] + + +def approval_stage_name(records: list, approval_record: dict) -> str: + """Human-readable stage name for an approval record. + + Resolved by walking the timeline's parent chain: Checkpoint.Approval -> Checkpoint -> Stage. + Falls back to the approval record's own name if that chain is missing (unexpected shape). + """ + by_id = {rec.get("id"): rec for rec in records} + checkpoint = by_id.get(approval_record.get("parentId")) + stage = by_id.get(checkpoint.get("parentId")) if checkpoint else None + return (stage or {}).get("name") or approval_record.get("name") or "?" + + +def find_pending_approvals(session: requests.Session, remote: AdoRemote, builds: list) -> list: + """(build, timeline records, pending approval records) for each build that's actually + blocked on a stage approval, not just still running. + + Only builds with status "inProgress" have a timeline at all — one that's "notStarted" + (queued, waiting on agent capacity) or "postponed" gets a 404 from the timeline endpoint, + and a Checkpoint.Approval gate can only exist mid-run anyway. A build can also briefly + report "inProgress" before its timeline document exists yet -- that 404 (like any other + request failure here) fails open rather than crashing the --wait loop over it; the next + poll, 30s later, tries again. + """ + result = [] + for build in builds: + if build.get("status") != "inProgress": + continue + try: + records = get_timeline_records(session, remote, build["id"]) + except requests.RequestException: + continue + approvals = pending_approval_records(records) + if approvals: + result.append((build, records, approvals)) + return result + + def get_failed_step_logs(session: requests.Session, remote: AdoRemote, build_id: int) -> None: r = session.get(f"{_base_url(remote)}/_apis/build/builds/{build_id}/timeline", params={"api-version": "7.1"}) r.raise_for_status() @@ -450,7 +508,9 @@ def retry(remote: AdoRemote, pat: str | None, target_branch: str, source_branch: def print_build(session: requests.Session, remote: AdoRemote, build: dict) -> None: build_id = build["id"] status = build.get("status", "unknown") - result = build.get("result", "—") + # ADO's build resource always includes a `result` key, explicitly `null` (-> None) until + # the build completes — `.get(..., "—")`'s default only covers a missing key, not this. + result = build.get("result") or "—" name = build.get("definition", {}).get("name", "?") number = build.get("buildNumber", "?") start = build.get("startTime", "?") @@ -472,21 +532,70 @@ def print_build(session: requests.Session, remote: AdoRemote, build: dict) -> No get_failed_step_logs(session, remote, build_id) +def exit_if_blocked_on_approval( + session: requests.Session, + remote: AdoRemote, + msg: str, + pipeline_builds: list, + wait: bool, + rerun_cmd: str, + show_retry_hint: bool = False, +) -> None: + """If any pipeline is blocked on a stage approval, print details and exit -- it never + resolves on its own, so --wait must stop instead of polling forever. If another pipeline + has already failed, that's the more urgent, more actionable fact: report it (exit 1, not + the approval code) instead of just telling the user to go approve a stage while staying + unaware CI already failed elsewhere -- with the same `retry_hint()` a plain failure report + would get, when `show_retry_hint` says that applies here (it doesn't for `watch-deploy`, + where `bdt pr retry` has nothing to act on -- there's no PR whose checks it retries). + A no-op (returns normally) if nothing is blocked. + """ + already_failed = any(b.get("result") == "failed" for b in pipeline_builds) + pending_approvals = find_pending_approvals(session, remote, pipeline_builds) if wait else [] + if not pending_approvals: + return + print(msg) + if already_failed: + print("\nNote: another pipeline has already failed — see details below.") + print("\nWaiting for approval:") + for build, records, approvals in pending_approvals: + pipeline_name = build.get("definition", {}).get("name", "?") + for rec in approvals: + stage = approval_stage_name(records, rec) + print(f" {pipeline_name} #{build['id']}: stage '{stage}' needs approval — {build_web_url(remote, build['id'])}") + if already_failed: + print("\nDetails:") + for b in pipeline_builds: + print_build(session, remote, b) + if show_retry_hint: + print(f"\n{retry_hint()}") + sys.exit(1) + print(f"\nApprove at the link(s) above, then re-run `{rerun_cmd}`.") + sys.exit(EXIT_NEEDS_APPROVAL) + + def run(remote: AdoRemote, pat: str | None, target_branch: str, wait: bool, source_branch: str | None = None) -> None: source_branch = source_branch or current_branch() session = requests.Session() session.headers.update(auth_header(pat)) - # When waiting, a pipeline's "latest" build may already be a completed run - # from before this invocation. Only accept builds newer than whatever was - # already there when we started, so --wait actually waits for the build(s) - # triggered by the current HEAD instead of immediately reporting a stale result. - baseline_ids: dict[int, int] = {} + # When waiting, a pipeline's "latest" build may already be a *completed* run from before + # this invocation (CI hasn't registered a new build for the current push yet). Only accept + # a fresh build in that case, so --wait doesn't immediately report that stale old result. + # Only builds already completed at this snapshot go in here -- one that's inProgress here + # (whether just-started or long-running) is genuinely the current build for the current + # HEAD, not a stale leftover, and must be watched rather than waited past: recording it too + # would make the loop below treat "still the same build, same id" as "stale" forever, + # since a running build keeps the same id for its whole life -- reintroducing the hang + # this function exists to avoid, for any --wait invoked after the build had already started. + baseline_completed_ids: dict[int, int] = {} if wait: pr = get_pr(session, remote, source_branch, target_branch) for b in get_builds_for_pr(session, remote, source_branch, pr["pullRequestId"]): + if b.get("status") != "completed": + continue def_id = b.get("definition", {}).get("id") - baseline_ids[def_id] = max(baseline_ids.get(def_id, 0), b["id"]) + baseline_completed_ids[def_id] = max(baseline_completed_ids.get(def_id, 0), b["id"]) draft_notice_shown = False last_line = "" @@ -507,7 +616,7 @@ def run(remote: AdoRemote, pat: str | None, target_branch: str, wait: bool, sour if builds: pipeline_builds = latest_per_pipeline(builds) if wait: - stale = [b for b in pipeline_builds if b["id"] <= baseline_ids.get(b.get("definition", {}).get("id"), 0)] + stale = [b for b in pipeline_builds if b["id"] <= baseline_completed_ids.get(b.get("definition", {}).get("id"), 0)] if stale: msg += " | waiting for new build(s) to start: " + ", ".join( b.get("definition", {}).get("name", "?") for b in stale @@ -518,10 +627,12 @@ def run(remote: AdoRemote, pat: str | None, target_branch: str, wait: bool, sour time.sleep(30) continue msg += " | " + ", ".join( - f"{b.get('definition', {}).get('name', '?')} #{b['id']} {b.get('status')} ({b.get('result', '—')})" + f"{b.get('definition', {}).get('name', '?')} #{b['id']} {b.get('status')} ({b.get('result') or '—'})" for b in pipeline_builds ) + exit_if_blocked_on_approval(session, remote, msg, pipeline_builds, wait, "bdt pr status --wait", show_retry_hint=True) + all_done = all(b.get("status") == "completed" for b in pipeline_builds) if all_done or not wait: print(msg) @@ -586,10 +697,12 @@ def run_watch_deploy(remote: AdoRemote, pat: str | None, target_branch: str, wai pipeline_builds = latest_per_pipeline(builds) msg = f"\rBranch '{target_branch}' | " + ", ".join( - f"{b.get('definition', {}).get('name', '?')} #{b['id']} {b.get('status')} ({b.get('result', '—')})" + f"{b.get('definition', {}).get('name', '?')} #{b['id']} {b.get('status')} ({b.get('result') or '—'})" for b in pipeline_builds ) + exit_if_blocked_on_approval(session, remote, msg, pipeline_builds, wait, "bdt pr watch-deploy --wait") + all_done = all(b.get("status") == "completed" for b in pipeline_builds) if all_done or not wait: print(msg) diff --git a/pyproject.toml b/pyproject.toml index 29ef807..2c6632f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -10,7 +10,7 @@ packages = ["bmsdna"] [project] name = "bmsdna-devtools" -version = "0.17.0" +version = "0.18.0" description = "Shared Azure DevOps / GitHub / git / Azure Monitor developer tooling for BMS projects" readme = "README.md" requires-python = ">=3.14" # pgdevkit>=0.7.1 requires 3.14; was >=3.11 before adding it as a dependency diff --git a/tests/test_gh_pr.py b/tests/test_gh_pr.py index 6ad3f7e..8e9fcef 100644 --- a/tests/test_gh_pr.py +++ b/tests/test_gh_pr.py @@ -3,6 +3,7 @@ import pytest +from bmsdna.devtools.cli_tools import EXIT_NEEDS_APPROVAL from bmsdna.devtools.gh_pr import ( add_attachments, add_files, @@ -65,6 +66,7 @@ (COMPLETED_SKIPPED_CHECK_RUN, "skipping"), ({"__typename": "CheckRun", "status": "IN_PROGRESS"}, "pending"), ({"__typename": "CheckRun", "status": "QUEUED"}, "pending"), + ({"__typename": "CheckRun", "status": "WAITING"}, "waiting_approval"), ({"__typename": "CheckRun", "status": "COMPLETED", "conclusion": "FAILURE"}, "fail"), ({"__typename": "CheckRun", "status": "COMPLETED", "conclusion": "TIMED_OUT"}, "fail"), ({"__typename": "CheckRun", "status": "COMPLETED", "conclusion": "CANCELLED"}, "cancel"), @@ -78,6 +80,56 @@ def test_check_bucket(check: dict, expected_bucket: str) -> None: assert check_bucket(check) == expected_bucket +def test_run_wait_exits_1_not_2_when_a_check_already_failed_and_another_needs_approval(monkeypatch) -> None: + """Regression: an already-failed check elsewhere in the PR must still end --wait even when + another check is separately waiting on a deployment approval — must report the failure + (exit 1), not silently prioritize the approval prompt (exit 2) or hang forever. + """ + pr = { + "number": 42, + "title": "feat: widgets", + "baseRefName": "main", + "mergeable": "MERGEABLE", + "isDraft": False, + "statusCheckRollup": [ + {"__typename": "CheckRun", "name": "deploy", "status": "WAITING", "workflowName": "Deploy"}, + {"__typename": "CheckRun", "name": "build", "status": "COMPLETED", "conclusion": "FAILURE", "workflowName": "CI"}, + ], + } + monkeypatch.setattr("bmsdna.devtools.gh_pr.get_pr", lambda gh: pr) + + with pytest.raises(SystemExit) as exc_info: + run("gh", wait=True) + + assert exc_info.value.code == 1 + + +def test_run_wait_does_not_hang_when_a_stuck_pending_check_also_exists(monkeypatch) -> None: + """Regression: a genuinely-stuck pending check (e.g. downstream of the blocked deployment + gate, so it can never leave "pending" on its own) combined with an already-failed check + and a waiting-approval check must not send --wait into an infinite poll loop. + """ + pr = { + "number": 42, + "title": "feat: widgets", + "baseRefName": "main", + "mergeable": "MERGEABLE", + "isDraft": False, + "statusCheckRollup": [ + {"__typename": "CheckRun", "name": "deploy", "status": "WAITING", "workflowName": "Deploy"}, + {"__typename": "CheckRun", "name": "build", "status": "COMPLETED", "conclusion": "FAILURE", "workflowName": "CI"}, + {"__typename": "CheckRun", "name": "downstream", "status": "QUEUED", "workflowName": "CI"}, + ], + } + monkeypatch.setattr("bmsdna.devtools.gh_pr.get_pr", lambda gh: pr) + monkeypatch.setattr("bmsdna.devtools.gh_pr.time.sleep", lambda s: pytest.fail("must not poll — would hang --wait forever")) + + with pytest.raises(SystemExit) as exc_info: + run("gh", wait=True) + + assert exc_info.value.code == 1 + + def test_check_label_prefixes_workflow_when_distinct() -> None: assert check_label(COMPLETED_SUCCESS_CHECK_RUN) == "PR Triaging / label-external / label_issues" @@ -544,6 +596,32 @@ def test_run_watch_deploy_exits_1_on_failed_run(monkeypatch) -> None: assert exc_info.value.code == 1 +def test_run_watch_deploy_wait_stops_and_reports_pending_approval(monkeypatch, capsys) -> None: + """--wait must not poll forever when the only deploy workflow run is paused on a + deployment protection rule (status "waiting") -- it never completes on its own. + """ + waiting_run = {**DEPLOY_RUN, "databaseId": 557, "status": "waiting", "conclusion": None} + monkeypatch.setattr("bmsdna.devtools.gh_pr.subprocess.run", _fake_run_and_view([waiting_run])) + + with pytest.raises(SystemExit) as exc_info: + run_watch_deploy("gh", "main", wait=True) + + assert exc_info.value.code == EXIT_NEEDS_APPROVAL + assert "needs a reviewer" in capsys.readouterr().out + + +def test_run_watch_deploy_wait_exits_1_not_2_when_another_run_already_failed(monkeypatch) -> None: + """A run stuck on approval must not mask an already-failed run in the same batch.""" + failed_run = {**DEPLOY_RUN, "databaseId": 556, "workflowName": "CI", "conclusion": "failure"} + waiting_run = {**DEPLOY_RUN, "databaseId": 557, "workflowName": "Deploy", "status": "waiting", "conclusion": None} + monkeypatch.setattr("bmsdna.devtools.gh_pr.subprocess.run", _fake_run_and_view([failed_run, waiting_run])) + + with pytest.raises(SystemExit) as exc_info: + run_watch_deploy("gh", "main", wait=True) + + assert exc_info.value.code == 1 + + def test_run_prints_deploy_hint_after_checks_pass(monkeypatch, capsys) -> None: pr_view = { "number": 7, diff --git a/tests/test_pr_build.py b/tests/test_pr_build.py index a75cb37..5040077 100644 --- a/tests/test_pr_build.py +++ b/tests/test_pr_build.py @@ -1,12 +1,21 @@ +from unittest.mock import MagicMock + import pytest +import requests +from bmsdna.devtools.cli_tools import EXIT_NEEDS_APPROVAL from bmsdna.devtools.gitrepo import AdoRemote from bmsdna.devtools.pr_build import ( + approval_stage_name, + build_web_url, draft_notice, + find_pending_approvals, merge_conflict_message, + pending_approval_records, policy_configs_include_branch, pr_web_url, retry_hint, + run, ) REPO_ID = "0cd3a822-389e-416e-a4fa-b73f988c2930" @@ -145,3 +154,186 @@ def test_policy_configs_include_branch_matches_default_branch_scope() -> None: def test_pr_web_url_is_the_browsable_page_not_the_rest_api_url() -> None: remote = AdoRemote("bmeurope", "BMS - CCMT2", "BMS - CCMT2") assert pr_web_url(remote, 123) == "https://dev.azure.com/bmeurope/BMS%20-%20CCMT2/_git/BMS%20-%20CCMT2/pullrequest/123" + + +class FakeTimelineResponse: + def __init__(self, records: list) -> None: + self._records = records + + def json(self) -> dict: + return {"records": self._records} + + def raise_for_status(self) -> None: + pass + + +def test_build_web_url_is_the_browsable_results_page() -> None: + remote = AdoRemote("bmeurope", "BMS - CCMT2", "BMS - CCMT2") + assert build_web_url(remote, 456) == "https://dev.azure.com/bmeurope/BMS%20-%20CCMT2/_build/results?buildId=456&view=results" + + +# Shape captured from a real `.../_apis/build/builds/{id}/timeline` response for a YAML +# pipeline paused on a stage's manual approval check. +STAGE_RECORD = {"id": "stage-1", "type": "Stage", "name": "Deploy to Production", "state": "inProgress"} +CHECKPOINT_RECORD = {"id": "checkpoint-1", "type": "Checkpoint", "parentId": "stage-1", "state": "inProgress"} +PENDING_APPROVAL_RECORD = { + "id": "approval-1", + "type": "Checkpoint.Approval", + "name": "Checkpoint.Approval", + "parentId": "checkpoint-1", + "state": "inProgress", +} +APPROVED_APPROVAL_RECORD = {**PENDING_APPROVAL_RECORD, "id": "approval-2", "state": "completed"} +TASK_RECORD = {"id": "task-1", "type": "Task", "name": "npm install", "state": "inProgress"} + + +def test_pending_approval_records_finds_open_checkpoint_approval() -> None: + records = [STAGE_RECORD, CHECKPOINT_RECORD, PENDING_APPROVAL_RECORD, TASK_RECORD] + assert pending_approval_records(records) == [PENDING_APPROVAL_RECORD] + + +def test_pending_approval_records_ignores_completed_approval() -> None: + records = [STAGE_RECORD, CHECKPOINT_RECORD, APPROVED_APPROVAL_RECORD] + assert pending_approval_records(records) == [] + + +def test_pending_approval_records_ignores_ordinary_in_progress_steps() -> None: + assert pending_approval_records([TASK_RECORD]) == [] + + +def test_approval_stage_name_walks_parent_chain() -> None: + records = [STAGE_RECORD, CHECKPOINT_RECORD, PENDING_APPROVAL_RECORD] + assert approval_stage_name(records, PENDING_APPROVAL_RECORD) == "Deploy to Production" + + +def test_approval_stage_name_falls_back_when_chain_is_missing() -> None: + orphan = {"id": "approval-1", "name": "Checkpoint.Approval", "parentId": "missing", "state": "inProgress"} + assert approval_stage_name([orphan], orphan) == "Checkpoint.Approval" + + +def test_find_pending_approvals_skips_completed_builds() -> None: + remote = AdoRemote("myorg", "MyProj", "myrepo") + session = MagicMock() + session.get.return_value = FakeTimelineResponse([PENDING_APPROVAL_RECORD]) + + result = find_pending_approvals(session, remote, [{"id": 1, "status": "completed"}]) + + assert result == [] + session.get.assert_not_called() + + +@pytest.mark.parametrize("status", ["notStarted", "postponed", "none"]) +def test_find_pending_approvals_skips_builds_with_no_timeline_yet(status: str) -> None: + """A build that hasn't started running yet has no timeline — fetching it would 404.""" + remote = AdoRemote("myorg", "MyProj", "myrepo") + session = MagicMock() + session.get.return_value = FakeTimelineResponse([PENDING_APPROVAL_RECORD]) + + result = find_pending_approvals(session, remote, [{"id": 1, "status": status}]) + + assert result == [] + session.get.assert_not_called() + + +def test_find_pending_approvals_reports_blocked_build() -> None: + remote = AdoRemote("myorg", "MyProj", "myrepo") + session = MagicMock() + session.get.return_value = FakeTimelineResponse([STAGE_RECORD, CHECKPOINT_RECORD, PENDING_APPROVAL_RECORD]) + + build = {"id": 1, "status": "inProgress", "definition": {"name": "deploy"}} + result = find_pending_approvals(session, remote, [build]) + + assert len(result) == 1 + found_build, records, approvals = result[0] + assert found_build is build + assert approvals == [PENDING_APPROVAL_RECORD] + assert approval_stage_name(records, approvals[0]) == "Deploy to Production" + + +def test_find_pending_approvals_fails_open_on_404_from_timeline_not_ready_yet() -> None: + """A build can briefly report "inProgress" before Azure DevOps has created its timeline + document yet -- that request failure must not crash --wait, just skip this build for now. + """ + remote = AdoRemote("myorg", "MyProj", "myrepo") + session = MagicMock() + session.get.side_effect = requests.HTTPError("404 Not Found") + + build = {"id": 1, "status": "inProgress", "definition": {"name": "deploy"}} + result = find_pending_approvals(session, remote, [build]) + + assert result == [] + + +class _BuildsSequence: + """First call (baseline, before the polling loop starts) returns builds one id behind the + ones returned on every later call — so `run()`'s staleness check ("only accept builds newer + than baseline") doesn't itself treat the loop's builds as stale and keep --wait spinning. + """ + + def __init__(self, baseline: list, polled: list) -> None: + self._baseline = baseline + self._polled = polled + self._calls = 0 + + def __call__(self, *args, **kwargs) -> list: + self._calls += 1 + return self._baseline if self._calls == 1 else self._polled + + +def test_run_wait_detects_approval_when_build_was_already_in_progress_at_invocation(monkeypatch) -> None: + """Regression: if the pipeline was already inProgress (and blocked on approval) *before* + `--wait` was invoked -- not just-started -- the baseline snapshot sees that same build, + still inProgress, and must not treat it as "stale, waiting for a new build to start": a + running build keeps the same id for its whole life, so that would make the staleness gate + block forever, never reaching the approval check at all. + """ + remote = AdoRemote("myorg", "MyProj", "myrepo") + pr = {"pullRequestId": 42, "title": "feat: widgets", "status": "active", "isDraft": False} + blocked_build = {"id": 200, "status": "inProgress", "result": None, "definition": {"id": 2, "name": "deploy"}} + + monkeypatch.setattr("bmsdna.devtools.pr_build.current_branch", lambda: "feature-x") + monkeypatch.setattr("bmsdna.devtools.pr_build.auth_header", lambda pat: {}) + monkeypatch.setattr("bmsdna.devtools.pr_build.get_pr", lambda *a, **k: pr) + # Same build, same id, on every call -- baseline capture and every poll iteration alike. + monkeypatch.setattr("bmsdna.devtools.pr_build.get_builds_for_pr", lambda *a, **k: [blocked_build]) + monkeypatch.setattr( + "bmsdna.devtools.pr_build.find_pending_approvals", + lambda session, remote, builds: [(blocked_build, [PENDING_APPROVAL_RECORD], [PENDING_APPROVAL_RECORD])], + ) + monkeypatch.setattr("bmsdna.devtools.pr_build.time.sleep", lambda s: pytest.fail("must not poll — would hang --wait forever")) + + with pytest.raises(SystemExit) as exc_info: + run(remote, pat=None, target_branch="main", wait=True) + + assert exc_info.value.code == EXIT_NEEDS_APPROVAL + + +def test_run_wait_exits_1_not_2_when_a_pipeline_already_failed_and_another_needs_approval(monkeypatch, capsys) -> None: + """Regression: an already-failed pipeline elsewhere in the PR must still end --wait even + when another pipeline is separately blocked on approval — and must report the failure + (exit 1), not silently prioritize the approval prompt (exit 2) or hang waiting for the + blocked pipeline to complete on its own (which it never will without a human). + """ + remote = AdoRemote("myorg", "MyProj", "myrepo") + pr = {"pullRequestId": 42, "title": "feat: widgets", "status": "active", "isDraft": False} + failed_build = {"id": 100, "status": "completed", "result": "failed", "definition": {"id": 1, "name": "build"}} + blocked_build = {"id": 200, "status": "inProgress", "result": None, "definition": {"id": 2, "name": "deploy"}} + + monkeypatch.setattr("bmsdna.devtools.pr_build.current_branch", lambda: "feature-x") + monkeypatch.setattr("bmsdna.devtools.pr_build.auth_header", lambda pat: {}) + monkeypatch.setattr("bmsdna.devtools.pr_build.get_pr", lambda *a, **k: pr) + monkeypatch.setattr( + "bmsdna.devtools.pr_build.get_builds_for_pr", + _BuildsSequence(baseline=[{**failed_build, "id": 99}, {**blocked_build, "id": 199}], polled=[failed_build, blocked_build]), + ) + monkeypatch.setattr( + "bmsdna.devtools.pr_build.find_pending_approvals", + lambda session, remote, builds: [(blocked_build, [PENDING_APPROVAL_RECORD], [PENDING_APPROVAL_RECORD])], + ) + monkeypatch.setattr("bmsdna.devtools.pr_build.print_build", lambda session, remote, build: None) + + with pytest.raises(SystemExit) as exc_info: + run(remote, pat=None, target_branch="main", wait=True) + + assert exc_info.value.code == 1 + assert "bdt pr retry" in capsys.readouterr().out diff --git a/tests/test_pr_build_flow.py b/tests/test_pr_build_flow.py index b642892..b07d7ff 100644 --- a/tests/test_pr_build_flow.py +++ b/tests/test_pr_build_flow.py @@ -9,6 +9,7 @@ import pytest import requests +from bmsdna.devtools.cli_tools import EXIT_NEEDS_APPROVAL from bmsdna.devtools.gitrepo import AdoRemote from bmsdna.devtools.pr_build import ( add_attachments, @@ -24,6 +25,7 @@ run_watch_deploy, update, ) +from tests.test_pr_build import CHECKPOINT_RECORD, PENDING_APPROVAL_RECORD, STAGE_RECORD REMOTE = AdoRemote(org="myorg", project="MyProj", repo="myrepo") PR = {"pullRequestId": 42, "title": "feat: widgets", "description": "existing description"} @@ -339,6 +341,54 @@ def test_run_watch_deploy_exits_1_on_failed_build(monkeypatch) -> None: assert exc_info.value.code == 1 +def make_builds_session_with_timeline(builds: list[dict], timeline_records: list[dict]) -> MagicMock: + session = MagicMock() + + def fake_get(url, params=None, **kwargs): + if url.endswith("/_apis/build/builds"): + return FakeResponse({"value": builds}) + if "/timeline" in url: + return FakeResponse({"records": timeline_records}) + raise AssertionError(f"unexpected GET {url}") + + session.get.side_effect = fake_get + return session + + +def test_run_watch_deploy_wait_stops_and_reports_pending_approval(monkeypatch, capsys) -> None: + """--wait must not poll forever when the only deploy pipeline is paused on a stage + approval -- it never completes on its own. + """ + blocked_build = {**DEPLOY_BUILD, "id": 200, "status": "inProgress", "result": None} + session = make_builds_session_with_timeline([blocked_build], [STAGE_RECORD, CHECKPOINT_RECORD, PENDING_APPROVAL_RECORD]) + monkeypatch.setattr("bmsdna.devtools.pr_build.requests.Session", lambda: session) + + with pytest.raises(SystemExit) as exc_info: + run_watch_deploy(REMOTE, "fake-pat", "main", wait=True) + + assert exc_info.value.code == EXIT_NEEDS_APPROVAL + out = capsys.readouterr().out + assert "Deploy to Production" in out + + +def test_run_watch_deploy_wait_exits_1_not_2_when_another_pipeline_already_failed(monkeypatch, capsys) -> None: + """A pipeline stuck on approval must not mask an already-failed pipeline in the same batch. + + Unlike `run()`, no `bdt pr retry` hint here -- watch-deploy isn't watching a PR's own + retryable checks, so that command has nothing to act on. + """ + failed_build = {**DEPLOY_BUILD, "id": 100, "definition": {"id": 1, "name": "CI"}, "result": "failed"} + blocked_build = {**DEPLOY_BUILD, "id": 200, "definition": {"id": 2, "name": "Deploy"}, "status": "inProgress", "result": None} + session = make_builds_session_with_timeline([failed_build, blocked_build], [STAGE_RECORD, CHECKPOINT_RECORD, PENDING_APPROVAL_RECORD]) + monkeypatch.setattr("bmsdna.devtools.pr_build.requests.Session", lambda: session) + + with pytest.raises(SystemExit) as exc_info: + run_watch_deploy(REMOTE, "fake-pat", "main", wait=True) + + assert exc_info.value.code == 1 + assert "bdt pr retry" not in capsys.readouterr().out + + def test_run_prints_deploy_hint_after_reporting_pr_success(monkeypatch, capsys) -> None: pr = {"pullRequestId": 1, "title": "feat: x", "status": "active", "isDraft": False} ci_build = {**DEPLOY_BUILD, "id": 5, "definition": {"id": 2, "name": "CI"}} diff --git a/uv.lock b/uv.lock index c2cd7f4..3fef825 100644 --- a/uv.lock +++ b/uv.lock @@ -22,7 +22,7 @@ wheels = [ [[package]] name = "bmsdna-devtools" -version = "0.17.0" +version = "0.18.0" source = { editable = "." } dependencies = [ { name = "pgdevkit", extra = ["db"] },