diff --git a/roboco/config.py b/roboco/config.py index b0680a28..1ab6a1c8 100644 --- a/roboco/config.py +++ b/roboco/config.py @@ -594,6 +594,19 @@ class Settings(BaseSettings): ge=60, description="Seconds between release-readiness assessment passes.", ) + release_ci_workflow: str = Field( + default="ci.yml", + description=( + "GitHub Actions workflow file name the release fail-closed CI gate " + "scopes to. Decoupled from self_heal_ci_workflow — that setting " + "documents an empty-string mode for single-workflow repos which, " + "inherited here, would degrade the release gate to the " + "all-workflows mode git.py itself flags as unreliable (a green " + "secondary workflow masking a red primary CI). The release gate " + "always resolves a NAMED workflow; empty falls back to 'ci.yml', " + "never None." + ), + ) # Organizational-memory loop — distill a high-signal lesson at task # completion, index journal reflections, and auto-inject similar past diff --git a/roboco/services/git.py b/roboco/services/git.py index f40ad7c4..bf0bd7df 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -237,6 +237,18 @@ def _select_ci_head_run(runs: list[dict[str, Any]]) -> dict[str, Any]: return max(same_head, key=lambda r: int(r.get("run_attempt") or 0)) +@dataclass(frozen=True) +class _CiRunQuery: + """Bundle of per-project inputs to a CI-run fetch (owner/repo, branch, token, + slug for logging) so ``_fetch_latest_ci_run`` stays under the arg-count gate — + owner_repo alone was already bundled for the same reason.""" + + project_slug: str + owner_repo: tuple[str, str] + branch: str + git_token: str + + class GitService(BaseService): """ Service for git operations on agent workspaces. @@ -1933,7 +1945,11 @@ class GitService(BaseService): } async def get_latest_ci_conclusion( - self, project_slug: str, *, workflow: str | None = None + self, + project_slug: str, + *, + workflow: str | None = None, + head_sha: str | None = None, ) -> dict[str, Any] | None: """Latest completed CI (GitHub Actions) run on a project's default branch. @@ -1944,10 +1960,12 @@ class GitService(BaseService): Resolves owner/repo and the git token PER PROJECT. ``workflow`` (a workflow file name like ``ci.yml``) scopes the signal to one workflow — without it the latest run across ALL workflows is used, which is - imprecise on a multi-workflow repo. Returns ``None`` on a missing token, - unparseable remote, GitHub error, or a repo with no matching Actions runs - (a repo that doesn't use GitHub Actions yields no signal, not a false - one). It never raises into the poll loop. + imprecise on a multi-workflow repo. ``head_sha`` further scopes the run + window to a specific commit (the release gate uses this so a later push + to the default branch can't mask the release commit's own CI). Returns + ``None`` on a missing token, unparseable remote, GitHub error, or a repo + with no matching Actions runs (a repo that doesn't use GitHub Actions + yields no signal, not a false one). It never raises into the poll loop. """ project = await get_project_service(self.session).get_by_slug(project_slug) if project is None or not project.git_url: @@ -1960,9 +1978,13 @@ class GitService(BaseService): if not git_token: return None branch = project.default_branch or "master" - run = await self._fetch_latest_ci_run( - project_slug, (owner, repo), branch, git_token, workflow + query = _CiRunQuery( + project_slug=project_slug, + owner_repo=(owner, repo), + branch=branch, + git_token=git_token, ) + run = await self._fetch_latest_ci_run(query, workflow, head_sha) if run is None: return None return { @@ -2013,42 +2035,47 @@ class GitService(BaseService): async def _fetch_latest_ci_run( self, - project_slug: str, - owner_repo: tuple[str, str], - branch: str, - git_token: str, + query: _CiRunQuery, workflow: str | None = None, + head_sha: str | None = None, ) -> dict[str, Any] | None: """Resolve ``branch``'s current-HEAD CI conclusion; None on error. Scopes to ``workflow`` (a workflow file name) when given — the precise signal — otherwise reads across ALL workflows, which on a multi-workflow - repo is unreliable (an unrelated green run can mask a red CI run). Pulls a - WINDOW of recent completed runs and selects the newest commit's latest - attempt (see ``_select_ci_head_run``) rather than the single - most-recently-completed run, so a green run on an older commit can't mask - the HEAD's failure and a green re-run correctly supersedes it. ``branch`` - filters by head branch, so only pushes to the default branch (not - pull-request runs, whose head is a feature branch) count — exactly the - "is the default branch red" signal self-heal needs. Transient network / - 429 / 5xx errors are retried a few times before giving up so a single - blip doesn't silently skip the cycle. + repo is unreliable (an unrelated green run can mask a red CI run). + ``head_sha`` further scopes the window to one commit so the release gate + can wait on a SPECIFIC release commit's CI without a later push to the + default branch masking it. Pulls a WINDOW of recent completed runs and + selects the newest commit's latest attempt (see + ``_select_ci_head_run``) rather than the single most-recently-completed + run, so a green run on an older commit can't mask the HEAD's failure and + a green re-run correctly supersedes it. ``branch`` filters by head + branch, so only pushes to the default branch (not pull-request runs, + whose head is a feature branch) count — exactly the "is the default + branch red" signal self-heal needs. Transient network / 429 / 5xx errors + are retried a few times before giving up so a single blip doesn't + silently skip the cycle. """ - owner, repo = owner_repo + owner, repo = query.owner_repo api_base = settings.github_api_base_url.rstrip("/") base = f"{api_base}/repos/{owner}/{repo}/actions" url = f"{base}/workflows/{workflow}/runs" if workflow else f"{base}/runs" headers = { - "Authorization": f"Bearer {git_token}", + "Authorization": f"Bearer {query.git_token}", "Accept": "application/vnd.github+json", "X-GitHub-Api-Version": "2022-11-28", } params: dict[str, str | int] = { - "branch": branch, + "branch": query.branch, "status": "completed", "per_page": _CI_RUN_WINDOW, } - resp = await self._get_ci_runs_response(project_slug, url, headers, params) + if head_sha: + params["head_sha"] = head_sha + resp = await self._get_ci_runs_response( + query.project_slug, url, headers, params + ) if resp is None or not resp.is_success: return None data = resp.json() diff --git a/roboco/services/release_executor.py b/roboco/services/release_executor.py index 15feff70..408c5a95 100644 --- a/roboco/services/release_executor.py +++ b/roboco/services/release_executor.py @@ -50,6 +50,8 @@ class ReleaseOps(Protocol): async def is_already_published(self, version: str) -> bool: ... + async def release_commit_sha(self, version: str) -> str | None: ... + async def apply_version_bumps( self, plan: list[str], new_version: str ) -> list[str]: ... @@ -72,7 +74,14 @@ class ReleaseExecutor: self._ops = ops async def execute(self, report: ReleaseReadinessReport) -> ReleaseResult: - """Bump → gate → commit/push → CI → publish, aborting on any red step.""" + """Bump → gate → commit/push → CI → publish, aborting on any red step. + + A half-landed (publish_failed) retry — a prior ``chore(release): {ver}`` + commit already on the branch but no tag — skips the bump/changelog/gate/ + commit pipeline (which would duplicate the changelog entry and land a + second release commit) and rejoins the shared CI → publish tail on the + existing commit. + """ version = report.proposed_version if await self._ops.is_already_published(version): return ReleaseResult( @@ -84,38 +93,52 @@ class ReleaseExecutor: detail=f"v{version} is already published; nothing to do.", ) - files = await self._ops.apply_version_bumps(report.version_bump_plan, version) - await self._ops.write_changelog_entry(report.drafted_changelog) - - if not await self._ops.run_gate(): - return ReleaseResult( - status="gate_failed", - version=version, - files_changed=files, - commit_sha=None, - release_url=None, - detail="make quality failed — aborted before commit (fail-closed).", + # Half-landed detection (publish_failed retry): a prior release commit + # on the branch means the pipeline already ran — only the publish step + # failed. Re-running it would re-insert the changelog entry (duplicate, + # above the already-present heading) and land a SECOND release commit. + # Reuse the existing commit and rejoin the shared CI → publish tail. + # None when no prior release commit exists (fresh release). + existing_sha = await self._ops.release_commit_sha(version) + if existing_sha is not None: + commit_sha = existing_sha + files: list[str] = [] + else: + files = await self._ops.apply_version_bumps( + report.version_bump_plan, version ) + await self._ops.write_changelog_entry(report.drafted_changelog) - try: - commit_sha = await self._ops.commit_and_push(version) - except RuntimeError as exc: - # The ops layer raises RuntimeError on a failed add/commit/push - # (gpgsign/pre-commit reject/no-op bump/non-fast-forward push). That - # is the correct fail-closed abort at the ops layer; the EXECUTOR - # turns it into a structured outcome so the CEO sees the cause - # instead of a 500 bubbling out of ``approve``. - logger.error("release commit/push failed", error=str(exc)[:300]) - return ReleaseResult( - status="commit_failed", - version=version, - files_changed=files, - commit_sha=None, - release_url=None, - detail=( + if not await self._ops.run_gate(): + return ReleaseResult( + status="gate_failed", + version=version, + files_changed=files, + commit_sha=None, + release_url=None, + detail="make quality failed — aborted before commit (fail-closed).", + ) + + try: + commit_sha = await self._ops.commit_and_push(version) + except RuntimeError as exc: + # The ops layer raises RuntimeError on a failed add/commit/push + # (gpgsign/pre-commit reject/no-op bump/non-fast-forward push). + # That is the correct fail-closed abort at the ops layer; the + # EXECUTOR turns it into a structured outcome so the CEO sees + # the cause instead of a 500 bubbling out of ``approve``. + logger.error("release commit/push failed", error=str(exc)[:300]) + detail = ( f"release commit/push failed — not published (fail-closed): {exc}" - )[:280], - ) + )[:280] + return ReleaseResult( + status="commit_failed", + version=version, + files_changed=files, + commit_sha=None, + release_url=None, + detail=detail, + ) if not await self._ops.wait_for_ci(commit_sha): return ReleaseResult( @@ -238,6 +261,29 @@ class _GitReleaseOps: rc, out = await self._git("ls-remote", "--tags", "origin", f"v{version}") return rc == 0 and bool(out.strip()) + async def release_commit_sha(self, version: str) -> str | None: + """Detect a half-landed release: a ``chore(release): {version}`` commit + already on the branch (a publish_failed retry — commit pushed, no tag + yet). Returns that commit's sha, or None when the clone is not yet at + ``version`` (no bump happened) or no such commit exists. + + The clone is fresh per execute(), so if the working version already + equals ``version`` AND the release commit is in history, the pipeline + ran in a prior attempt and must not re-run (it would duplicate the + changelog entry and land a second release commit). + """ + if self._current_version() != version: + return None + rc, out = await self._git("log", "-n", "50", "--format=%H%x00%s") + if rc != 0: + return None + wanted = f"chore(release): {version}" + for line in out.splitlines(): + sha, _, subject = line.partition("\x00") + if sha and subject.strip() == wanted: + return sha.strip() + return None + def _current_version(self) -> str: text = (self._root / "pyproject.toml").read_text(encoding="utf-8") match = re.search(r'^version\s*=\s*"([^"]+)"', text, re.MULTILINE) @@ -316,7 +362,7 @@ class _GitReleaseOps: git = get_git_service(self._session) for _ in range(_CI_MAX_POLLS): ci = await git.get_latest_ci_conclusion( - self._slug, workflow=self._ci_workflow + self._slug, workflow=self._ci_workflow, head_sha=commit_sha ) if ci and ci.get("head_sha") == commit_sha: conclusion = (ci.get("conclusion") or "").lower() @@ -378,6 +424,20 @@ def _insert_changelog_entry(existing: str, entry: str) -> str: return existing.rstrip() + "\n\n" + block +def _resolve_release_ci_workflow() -> str: + """The release CI gate's workflow, decoupled from self_heal_ci_workflow. + + ``self_heal_ci_workflow`` documents an empty-string mode for single-workflow + repos; inheriting that here would degrade the release fail-closed gate to + the all-workflows mode ``_fetch_latest_ci_run`` itself flags as unreliable + (a green secondary workflow masking a red primary CI). The release gate + always uses a NAMED workflow — empty falls back to ``ci.yml``, never None. + """ + from roboco.config import settings + + return settings.release_ci_workflow or "ci.yml" + + async def get_release_executor(session: AsyncSession) -> ReleaseExecutor: """Build a ReleaseExecutor with a production ops over a fresh writable clone.""" from roboco.config import settings @@ -399,7 +459,7 @@ async def get_release_executor(session: AsyncSession) -> ReleaseExecutor: default_branch=default_branch, root=root, auth_url=auth_url, - ci_workflow=(settings.self_heal_ci_workflow or None), + ci_workflow=_resolve_release_ci_workflow(), ) return ReleaseExecutor(_GitReleaseOps(session, ctx)) diff --git a/tests/unit/services/test_release_executor.py b/tests/unit/services/test_release_executor.py index cd8c4f58..1a9af263 100644 --- a/tests/unit/services/test_release_executor.py +++ b/tests/unit/services/test_release_executor.py @@ -8,10 +8,25 @@ call sequence; the production git/gh ops is exercised live (CEO-gated). from __future__ import annotations +from types import SimpleNamespace +from typing import TYPE_CHECKING +from unittest.mock import MagicMock + import pytest -from roboco.services.release_executor import ReleaseExecutor, ReleaseResult +from roboco.config import settings +from roboco.services import release_executor as re +from roboco.services.release_executor import ( + ReleaseExecutor, + ReleaseResult, + _GitReleaseOps, + _ReleaseContext, + _resolve_release_ci_workflow, +) from roboco.services.release_readiness import ReleaseReadinessReport +if TYPE_CHECKING: + from pathlib import Path + _PLAN = ["pyproject.toml", "roboco/__init__.py", "CHANGELOG.md"] _VERSION = "0.13.0" _ONE = 1 @@ -49,14 +64,28 @@ class _FakeOps: self._ci = ci self._commit_raises = commit_raises self._publish_raises = publish_raises + # Half-landed (publish_failed retry) detection: a prior + # ``chore(release): {version}`` commit already on the branch. Set on the + # instance (not via __init__ — keeps the constructor under the arg-count + # gate) by tests that exercise the retry path. + self._existing_sha: str | None = None self.calls: list[str] = [] self.bumped_plan: list[str] | None = None self.bumped_version: str | None = None + self.halflanded_check = False async def is_already_published(self, _version: str) -> bool: self.calls.append("check") return self._already + async def release_commit_sha(self, _version: str) -> str | None: + # Half-landed detection: a prior `chore(release): {version}` commit + # already on the branch means a publish_failed retry must NOT re-run the + # bump→changelog→gate→commit pipeline. Recorded via a flag (not calls) + # so the green-path call-sequence assertion is unaffected. + self.halflanded_check = True + return self._existing_sha + async def apply_version_bumps(self, plan: list[str], new_version: str) -> list[str]: self.calls.append("bump") self.bumped_plan = list(plan) @@ -186,3 +215,103 @@ def test_release_result_carries_outcome_fields() -> None: assert result.version == _VERSION assert result.files_changed == _PLAN assert result.release_url is not None + + +@pytest.mark.asyncio +async def test_half_landed_retry_skips_bump_and_republishes_only() -> None: + """#87: a publish_failed retry (commit pushed + CI green, no tag yet) must + NOT re-run bump/changelog/gate/commit — that would re-insert the changelog + entry above the already-present ``## [X.Y.Z]`` heading (duplicate) and land a + second ``chore(release): X.Y.Z`` commit. The executor detects the + half-landed state via ``release_commit_sha`` (a prior release commit already + on the branch) and jumps straight to wait_for_ci + publish.""" + ops = _FakeOps() + ops._existing_sha = "existingbeef" + result = await ReleaseExecutor(ops).execute(_report()) + assert result.status == "published" + assert result.commit_sha == "existingbeef" + assert result.release_url is not None + assert ops.halflanded_check is True + assert "bump" not in ops.calls + assert "changelog" not in ops.calls + assert "gate" not in ops.calls + assert "commit" not in ops.calls + assert ops.calls == ["check", "ci", "publish"] + + +@pytest.mark.asyncio +async def test_wait_for_ci_scoped_to_release_commit_not_branch_latest( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """#318: a later commit landing on master during the ~40min wait must not + mask the release commit's green CI. ``wait_for_ci`` scopes the GitHub query + to the release commit_sha (``head_sha=``), so the branch-latest run (a later + sha) can't make the gate poll forever and false-fail as ci_failed.""" + commit_sha = "release_commit_abc" + later_sha = "later_landed_def" + + async def _fake_get_ci(_slug: str, **_kwargs: object) -> dict[str, str]: + # Mimic GitHub's head_sha filter: a run for the release sha only when + # asked for it (head_sha=commit_sha); the branch-latest (later commit) + # run otherwise. The release gate MUST scope to commit_sha to see green. + if _kwargs.get("head_sha") == commit_sha: + return { + "head_sha": commit_sha, + "conclusion": "success", + "run_url": "u", + "run_name": "n", + "branch": "master", + "completed_at": "t", + } + return { + "head_sha": later_sha, + "conclusion": "success", + "run_url": "u2", + "run_name": "n2", + "branch": "master", + "completed_at": "t2", + } + + monkeypatch.setattr( + "roboco.services.git.get_git_service", + lambda _session: SimpleNamespace(get_latest_ci_conclusion=_fake_get_ci), + ) + monkeypatch.setattr(re, "_CI_MAX_POLLS", 2) + + async def _no_sleep(_secs: float) -> None: + return None + + monkeypatch.setattr(re.asyncio, "sleep", _no_sleep) + + ctx = _ReleaseContext( + slug="roboco-api", + default_branch="master", + root=tmp_path, + auth_url="x", + ci_workflow="ci.yml", + ) + ops = _GitReleaseOps(session=MagicMock(), ctx=ctx) + ok = await ops.wait_for_ci(commit_sha) + assert ok is True + + +def test_release_ci_workflow_decoupled_from_self_heal_setting( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """#402: the release CI gate must not inherit ``self_heal_ci_workflow``'s + empty-string tuning (documented valid for single-workflow repos), which would + degrade the fail-closed gate to the all-workflows mode git.py itself flags as + unreliable. The release gate always resolves a named workflow (default + ``ci.yml``), never None.""" + # The dangerous tuning an operator might apply for self-heal on a + # single-workflow repo — must NOT leak into the release gate. + monkeypatch.setattr(settings, "self_heal_ci_workflow", "") + monkeypatch.setattr(settings, "release_ci_workflow", "ci.yml") + assert _resolve_release_ci_workflow() == "ci.yml" + + monkeypatch.setattr(settings, "release_ci_workflow", "release.yml") + assert _resolve_release_ci_workflow() == "release.yml" + + # An empty release setting never falls through to None — always the default. + monkeypatch.setattr(settings, "release_ci_workflow", "") + assert _resolve_release_ci_workflow() == "ci.yml"