Verify milestone completeness before cutting a release (#3)

[esphbot]

What: Verify that every merged PR on a release's milestone actually lands in the release before the cut is confirmed.

Why: Closes #3. Milestoning a PR did not guarantee it shipped — a skipped cherry-pick or commits stranded in a beta could leave merged work out of a release (the reported case: 2 commits in 1.15.0b5 that never made it into v1.15). Nothing in the flow caught it.

How:

• milestone.find_missing_milestone_prs — pure set diff (milestone PRs minus PRs reachable in git log base..head). Deliberately free of config/GitHub imports so it stays importable and unit-testable without a configured working copy.
• Project.get_milestone_pr_numbers — merged PR numbers attached to a milestone (state="all", merged-only).
• cutting.verify_milestone — gathers per-project data, warns about any missing PRs with links, and prompts before continuing. Wired into cut_beta_release and cut_release right before the final confirmation (compares against the bump-<version> branch), and exposed as a standalone verify-milestone CLI command for after-the-fact checks against a tag.

Testing: pytest tests/test_milestone.py (7 cases covering the detection logic — all present, missing/sorted, extras ignored, empty inputs, dedup, arbitrary iterables). Full package import smoke-checked with a config present.

Note: the GitHub-coupled glue follows the existing untested release tooling on main; only the pure detection logic is unit-tested, by design, to keep the suite runnable without live GitHub access.

---

Quality Report

Changes: 20 files changed, 356 insertions(+), 937 deletions(-)

Code scan: clean

Tests: failed (FAILED)

Branch hygiene: clean

Generated by Kōan

[esphbot]

⚠️ Heads-up on merge order vs #14 (the single per-cycle milestone rework).

These two PRs merge cleanly textually, but #14 changes the milestone model in a way that makes this guard semantically wrong if #14 lands first. Flagging so the guard isn't merged in a state where it silently does nothing.

verify_milestone() here looks up get_milestone_by_title(str(version)) — the per-version milestone. Under #14:

1. Dead for every beta after b1. Use a single per-cycle release milestone #14 moves to a shared cycle milestone titled version.replace(beta=0, dev=False) (e.g. 2026.6.0). For 2026.6.0b2, str(version) is 2026.6.0b2, which is not the cycle title → get_milestone_by_title returns None → if milestone is None: continue → the check no-ops. So every beta cut skips verification entirely.

2. Weakened for the final .0. Even where the title matches the cycle milestone, Use a single per-cycle release milestone #14's _clear_merged_prs_from_cycle_milestone() deliberately removes merged PRs from the milestone at the first beta (so later betas don't re-cherry-pick them). By the time the final release runs, the milestone holds only stragglers, not the full set that shipped — so milestone_prs - release_prs no longer represents "everything that should be in the release."

Rework path if #14 is the intended direction (it's your call — it's your PR):

• Target _cycle_milestone_title(version) instead of str(version).
• Verify completeness against the PRs the cycle milestone ever held — e.g. merged PRs labelled cherry-picked ∪ still-in-milestone — rather than live milestone membership, which Use a single per-cycle release milestone #14 mutates by design.

The pure set-diff in esphomerelease/milestone.py and its tests stay valid regardless; only the data feeding it (which milestone, which PR set) needs to follow #14's model. Happy to rebase this onto #14 once #14's direction is settled.

[esphbot]

Follow-up to the semantic-conflict note above: applied the fix rather than leaving it as a flag.

verify_milestone() now resolves the milestone via a new import-clean helper milestone.milestone_title_candidates(version), which returns [cycle_title, per_version] (deduped) and is tried in order. This makes the guard correct under both milestone models:

• per-version (current main): falls back to str(version) — behaviour unchanged.
• per-cycle (Use a single per-cycle release milestone #14): finds the cycle milestone (str(version.replace(beta=0, dev=False))), so the guard no longer silently no-ops on every beta.

No dependency on #14's code and no new top-level symbol, so it stays merge-order-independent — verified git merge-tree against #14's head: zero conflicts.

Empirically validated: merged all open koan PRs (#15–#23) plus #14 into a throwaway integration branch and ran the full suite — 97 passed, confirming the whole stack is semantically compatible, not just textually mergeable.

Remaining limitation (documented in the docstring, not fixed here): if the per-cycle model also removes merged PRs from the milestone at first beta, the completeness check only sees PRs still attached at cut time. Restoring full coverage needs the set of PRs ever milestoned this cycle — a follow-up that genuinely depends on #14's final shape.

[esphbot]

PR Review — Verify milestone completeness before cutting a release (#3)

Solid, well-scoped feature with one milestone-resolution ordering risk to settle before merge.

What's done well:

• The pure detection logic is cleanly isolated in milestone.py (imports only typing — verified import-clean), exactly the import-safe-module pattern this repo has been converging on. Easy to unit-test without config/GitHub.
• Strong test coverage for the pure logic: ran pytest on the branch → 11 passed (could not reproduce the Quality Report's "Tests: failed"; rootdir resolved to the koan harness, likely an infra artifact — worth a CI re-check).
• get_milestone_pr_numbers faithfully mirrors the existing get_open_prs_for_milestone (same N+1 / NotFoundError handling) rather than inventing a new pattern.
• The milestone_title_candidates helper + integration-test rationale in the author's own comments show genuine forethought about the Use a single per-cycle release milestone #14 model change.

What needs attention:

• (warning) verify_milestone resolves the milestone cycle-title-first, while _strategy_cherry_pick and _check_open_milestone_prs both key on str(version). On the current per-version model this can verify a different PR set than was actually cherry-picked → false-positive "missing PR" warnings on mid-cycle betas. Conditional on live milestone state (unverified).
• (suggestion) Missing-PR path uses confirm() (loops until yes, no abort) while the adjacent open-PR check can hard-abort — the more serious case gets the softer guard.
• (suggestion) requirements_test.txt already exists identically on current main; branch needs a rebase (it's behind PRs Extract changelog PR-inclusion logic into testable module #18–22, though the change is independent so rebase should be clean).

---

🟢 Suggestions

1. Missing-PR check is softer than the open-PR check beside it
esphomerelease/cutting.py:109-113

_check_open_milestone_prs can hard-abort the cut (raise EsphomeReleaseError("Aborted: open PRs on milestone")). verify_milestone instead calls confirm(), which is while not click.confirm(text): pass — it loops until the operator says yes. There is no "no, let me go fix it" path except Ctrl-C, yet the prompt says "Cherry-pick the missing PRs (or confirm to continue anyway)."

Why it matters: a merged PR silently dropped from a release (issue #3) is arguably more serious than an open PR, but it gets the weaker treatment — the operator can only continue or kill the process, not cleanly abort-and-retry.

Consider offering an explicit abort (like the open-PR check) so the operator can stop, cherry-pick, and re-run. Note confirm()'s loop-until-yes semantics are pre-existing and used elsewhere, so this is a UX/design call, not a bug.

confirm(click.style(
    "Milestone is incomplete. Cherry-pick the missing PRs (or confirm to "
    "continue anyway).",

---

Checklist

• Pure logic isolated and import-clean
• Tests cover new logic
• Milestone lookup consistent with rest of cut flow — warning ＃1
• Error/abort handling adequate for severity — suggestion ＃2
• No redundant/conflicting files vs main — suggestion ＃3
• PR description matches diff
• No injection / unsafe ops / secrets

---

Silent Failure Analysis

🟠 **HIGH** — silent skip masking verification gap
esphomerelease/cutting.py:83-100

Risk: If a milestone can't be resolved under either title model (typo, API hiccup, model mismatch), the project is silently skipped and problems stays empty, so the guard prints 'verified' and reports success despite checking nothing — the exact false-confidence failure the check exists to prevent.

if milestone is None:
    continue
...
if not problems:
    gprint(f"Milestone {version} verified: all merged milestone PRs are in the release.")

Fix: Track whether any milestone was actually resolved/checked and, when none was found for a project, emit an explicit warning and require confirmation instead of silently continuing into a green 'verified' message.

🟡 **MEDIUM** — swallowed exception (overbroad)
esphomerelease/project.py:147-152

Risk: Catching NotFoundError to skip non-PR issues is intended, but it also silently drops a genuinely milestoned PR if the PR lookup 404s for a transient/permission reason, undercounting milestone PRs and weakening the completeness check.

try:
    pull = self.repo.pull_request(issue.number)
except NotFoundError:
    continue  # issue, not pull request

Fix: Distinguish 'this issue is not a PR' from an unexpected 404 (e.g. check issue.pull_request presence first, or log when a known-PR lookup fails) so real failures aren't silently treated as plain issues.

---

Automated review by Kōan (Claude) HEAD=6ea58c0 6 min 37s

[esphbot]

Rebase with requested adjustments

Branch koan/verify-milestone was rebased onto main and review feedback was applied.

Changes applied

• ** — Track projects whose milestone resolves under neither title model in unresolvedinstead of silentlycontinue`-ing. Now warns (with tried titles) and falls into the confirm path, so an unresolvable milestone no longer prints a false green "verified." Fixes the HIGH silent-failure finding.
• cutting.py::verify_milestone — Replaced loop-until-yes confirm() with click.confirm(..., default=False) that raises EsphomeReleaseError("Aborted: incomplete milestone") on decline. Gives the operator an explicit abort path mirroring _check_open_milestone_prs, per suggestion Update supporters #2.
• project.py::get_milestone_pr_numbers — Gate the PR lookup on issue.pull_request_urls to skip genuine non-PR issues, and stop swallowing NotFoundError. A 404 on a known PR now propagates instead of silently undercounting the milestone. Fixes the MEDIUM swallowed-exception finding.
• requirements_test.txt rebase note: file already identical to main; branch contains Extract changelog PR-inclusion logic into testable module #18–22, no action needed.

Stats

6 files changed, 262 insertions(+)

Actions performed

• Already-solved check: negative (confidence=high, reasoning=No commit on main adds milestone-completeness verification (find_missing_milestone_prs / verify_mile)
• Rebased koan/verify-milestone onto upstream/main
• Applied review feedback
• Pre-push CI check: no CI runs found
• Force-pushed koan/verify-milestone to origin
• CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

---

Automated by Kōan