From 2c715d3e73b16a95b7418757b0082971be0fc6ea Mon Sep 17 00:00:00 2001 From: jinon86 <247078695+jinon86@users.noreply.github.com> Date: Thu, 6 Aug 2026 01:37:23 +0000 Subject: [PATCH] fix(external-wait): normalize short head SHAs at registration, heal legacy records (#961) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 7-39 char head SHA passed format validation but could never equal GitHub's 40-char headRefOid, so the wait superseded on the first poll while register returned ok — the #949 silent promise loss. - register: resolve short SHAs to the full head via gh api; refuse registration (ok:false, exit 2) when the SHA does not resolve - monitor: a recorded short SHA that prefixes the live head is healed to the full SHA instead of superseded; a genuinely moved head still supersedes - registry: correct_head_sha() heals only monitoring records with a strict prefix match Closes #961 --- bridge/core/external_wait.py | 27 ++++++++++ bridge/core/external_wait_cli.py | 44 ++++++++++++++- bridge/core/external_wait_monitor.py | 16 ++++-- bridge/tests/test_external_wait_cli.py | 63 ++++++++++++++++++++++ bridge/tests/test_external_wait_monitor.py | 41 ++++++++++++++ 5 files changed, 186 insertions(+), 5 deletions(-) diff --git a/bridge/core/external_wait.py b/bridge/core/external_wait.py index a0ec6416..11e774c0 100644 --- a/bridge/core/external_wait.py +++ b/bridge/core/external_wait.py @@ -301,6 +301,33 @@ def _do(records): self._mutate(_do) + def correct_head_sha(self, wait_id: str, full_sha: str) -> bool: + """Heal a legacy short-SHA registration to the full 40-char head. + + Registration now normalizes short SHAs to the full head via ``gh`` + (#961), but records written before that fix may hold 7-39 hex chars + that can never equal GitHub's 40-char ``headRefOid``. The monitor + calls this when the live head *starts with* the recorded short form; + a genuine head change still supersedes. Returns True only when the + record was monitoring and actually healed. + """ + full_sha = validate_head_sha(full_sha) + if len(full_sha) != 40: + return False + + def _do(records): + rec = records.get(wait_id) + if rec is None or rec.get("state") != STATE_MONITORING: + return False + recorded = str(rec.get("head_sha") or "") + if not recorded or len(recorded) >= 40 or not full_sha.startswith(recorded): + return False + rec["head_sha"] = full_sha + rec["updated_at"] = _utc_now_iso() + return True + + return bool(self._mutate(_do)) + def finish(self, wait_id: str, terminal_status: str, *, now: Optional[float] = None) -> bool: """Terminal transition, journaled before any wake. First write wins. diff --git a/bridge/core/external_wait_cli.py b/bridge/core/external_wait_cli.py index 30d64f09..aaff124f 100644 --- a/bridge/core/external_wait_cli.py +++ b/bridge/core/external_wait_cli.py @@ -23,6 +23,7 @@ import json import os +import subprocess import sys from pathlib import Path from typing import Any, Optional, Sequence @@ -59,6 +60,47 @@ def _emit(payload: dict[str, Any]) -> None: print(json.dumps(payload, ensure_ascii=False, separators=(",", ":"))) +def resolve_full_head_sha(repo: str, head_sha: str) -> str: + """Normalize a 7-39 char short SHA to the full 40-char head (#961). + + The monitor compares the recorded SHA against GitHub's 40-char + ``headRefOid`` with exact equality, so a short SHA that passes format + validation can never match — registration would return ``ok`` for a wait + that supersedes on the first poll (the #949 silent promise loss). Resolve + through ``gh`` at registration; when the short SHA does not resolve to a + commit in the repo, registration fails closed instead of promising a + watch that can never fire. Full 40-char SHAs pass through untouched. + """ + if len(head_sha) == 40: + return head_sha + try: + proc = subprocess.run( + ["gh", "api", f"repos/{repo}/commits/{head_sha}", "--jq", ".sha"], + capture_output=True, + timeout=30, + ) + except (OSError, subprocess.TimeoutExpired): + raise ExternalWaitValidationError( + "short head SHA could not be resolved via gh (unavailable or timeout); " + "pass the full 40-char SHA" + ) + if proc.returncode != 0: + raise ExternalWaitValidationError( + "short head SHA does not resolve to a commit in the repo; " + "pass the full 40-char SHA from the PR head" + ) + full = proc.stdout.decode("utf-8", "replace").strip().lower() + try: + full = validate_head_sha(full) + except ExternalWaitValidationError: + full = "" + if len(full) != 40 or not full.startswith(head_sha): + raise ExternalWaitValidationError( + "head SHA resolution returned an unexpected value; registration refused" + ) + return full + + def _parse_args(argv: Sequence[str]) -> dict[str, Any]: """Tiny flag parser (no argparse dependency games in hook contexts).""" args: dict[str, Any] = {"_": []} @@ -92,7 +134,7 @@ def _cmd_register(home: Path, args: dict[str, Any]) -> int: try: repo = validate_repo(str(args.get("repo", ""))) pr_number = validate_pr_number(args.get("pr")) - head_sha = validate_head_sha(str(args.get("head_sha", ""))) + head_sha = resolve_full_head_sha(repo, validate_head_sha(str(args.get("head_sha", "")))) summary = validate_summary(args.get("summary")) timeout = float(args.get("timeout_seconds", DEFAULT_TIMEOUT_SECONDS)) except ExternalWaitValidationError as exc: diff --git a/bridge/core/external_wait_monitor.py b/bridge/core/external_wait_monitor.py index 83bb1466..71cf5f8e 100644 --- a/bridge/core/external_wait_monitor.py +++ b/bridge/core/external_wait_monitor.py @@ -301,10 +301,18 @@ async def _poll_one(self, record: Dict[str, Any]) -> None: self._reschedule(record) return self._transport_errors.pop(wait_id, None) - if state.head_sha != record.get("head_sha"): - # The PR moved on: never report the stale run as the watched one. - self._registry.finish(wait_id, TERMINAL_SUPERSEDED, now=now) - return + recorded = str(record.get("head_sha") or "") + if state.head_sha != recorded: + if recorded and len(recorded) < 40 and state.head_sha.startswith(recorded): + # Legacy short-SHA registration watching the same head: heal + # the record to the full SHA instead of dropping the promise + # as superseded (#961). A genuinely moved head still ends the + # wait below. + self._registry.correct_head_sha(wait_id, state.head_sha) + else: + # The PR moved on: never report the stale run as the watched one. + self._registry.finish(wait_id, TERMINAL_SUPERSEDED, now=now) + return terminal = _TERMINAL_BY_ROLLUP.get(state.rollup) if terminal is not None: self._registry.finish(wait_id, terminal, now=now) diff --git a/bridge/tests/test_external_wait_cli.py b/bridge/tests/test_external_wait_cli.py index 44537edc..511644ba 100644 --- a/bridge/tests/test_external_wait_cli.py +++ b/bridge/tests/test_external_wait_cli.py @@ -16,11 +16,25 @@ from telegram_bot.core import external_wait_cli from telegram_bot.core.external_wait import ( ExternalWaitRegistry, + ExternalWaitValidationError, default_registry_path, publish_active_turn, ) +_REAL_RESOLVE = external_wait_cli.resolve_full_head_sha + + +@pytest.fixture(autouse=True) +def _stub_sha_resolution(monkeypatch: pytest.MonkeyPatch) -> None: + """Keep ``gh`` out of unit tests: expand short SHAs deterministically.""" + + def _fake(repo: str, head_sha: str) -> str: + return head_sha if len(head_sha) == 40 else head_sha.ljust(40, "0") + + monkeypatch.setattr(external_wait_cli, "resolve_full_head_sha", _fake) + + @pytest.fixture def home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: monkeypatch.setenv("CCC_EXTERNAL_WAIT_HOME", str(tmp_path)) @@ -81,6 +95,55 @@ def test_register_is_idempotent_for_the_same_natural_key( assert first["wait_id"] == second["wait_id"] +def test_register_normalizes_a_short_sha_to_the_full_head( + home: Path, capsys: pytest.CaptureFixture +) -> None: + _publish(home) + + rc = external_wait_cli.main(_register_args(head_sha="a8ac2475")) + + assert rc == 0 + payload = json.loads(capsys.readouterr().out.strip()) + # Regression (#961/#949): a short SHA must never be stored as-is — the + # monitor compares against GitHub's 40-char headRefOid with exact + # equality, so a raw short SHA supersedes on the first poll and the + # "CI finishes -> auto-resume" promise is silently dropped. + assert len(payload["head_sha"]) == 40 + record = ExternalWaitRegistry(default_registry_path(home)).get(payload["wait_id"]) + assert record["head_sha"] == payload["head_sha"] + assert record["head_sha"].startswith("a8ac2475") + + +def test_register_fails_closed_when_the_short_sha_does_not_resolve( + home: Path, capsys: pytest.CaptureFixture, monkeypatch: pytest.MonkeyPatch +) -> None: + _publish(home) + + def _unresolvable(repo: str, head_sha: str) -> str: + raise ExternalWaitValidationError("short head SHA does not resolve to a commit") + + monkeypatch.setattr(external_wait_cli, "resolve_full_head_sha", _unresolvable) + + rc = external_wait_cli.main(_register_args(head_sha="deadbeef")) + + assert rc == 2 + payload = json.loads(capsys.readouterr().out.strip()) + assert payload["ok"] is False + assert payload["code"] == "validation" + # The honest failure must not leave a watch that can never fire. + assert ExternalWaitRegistry(default_registry_path(home)).records() == [] + + +def test_full_sha_passes_through_without_gh(monkeypatch: pytest.MonkeyPatch) -> None: + full = "a8ac2475" * 5 + + def _boom(*args, **kwargs): + raise AssertionError("gh must not run for an already-full SHA") + + monkeypatch.setattr(external_wait_cli.subprocess, "run", _boom) + assert _REAL_RESOLVE("jinwon-int/ccc-node", full) == full + + def test_register_fails_closed_without_an_active_route( home: Path, capsys: pytest.CaptureFixture ) -> None: diff --git a/bridge/tests/test_external_wait_monitor.py b/bridge/tests/test_external_wait_monitor.py index 4138b12b..8fd6ee92 100644 --- a/bridge/tests/test_external_wait_monitor.py +++ b/bridge/tests/test_external_wait_monitor.py @@ -178,6 +178,47 @@ async def test_failure_and_cancelled_are_terminal_results(tmp_path: Path) -> Non assert headline in recorder.notifications[0][1] +@pytest.mark.anyio +async def test_legacy_short_sha_heals_instead_of_superseding(tmp_path: Path) -> None: + # Regression (#961): a record written with a 7-char SHA watches the same + # head whose headRefOid merely *extends* it — that is not a moved head. + clock = Clock() + registry = _registry(tmp_path, clock) # seeds head_sha "abc1234" (7 chars) + full = "abc1234" + "f" * 33 + transport = FakeTransport([PrState(full, "pending"), PrState(full, "success")]) + recorder = Recorder() + monitor = _monitor(registry, transport, recorder, clock) + + await monitor._tick() + + record = registry.records()[0] + assert record["state"] == "monitoring" + assert record["head_sha"] == full + + clock.advance(120) + await monitor._tick() + + record = registry.records()[0] + assert record["terminal_status"] == TERMINAL_SUCCESS + assert len(recorder.notifications) == 1 + + +@pytest.mark.anyio +async def test_short_sha_prefix_of_a_different_head_still_supersedes(tmp_path: Path) -> None: + # Same length, different content: not the watched head, so the wait ends. + clock = Clock() + registry = _registry(tmp_path, clock) + transport = FakeTransport([PrState("abc9999" + "f" * 33, "success")]) + recorder = Recorder() + monitor = _monitor(registry, transport, recorder, clock) + + await monitor._tick() + + record = registry.records()[0] + assert record["terminal_status"] == TERMINAL_SUPERSEDED + assert recorder.resumes == [] + + @pytest.mark.anyio async def test_head_sha_mismatch_is_superseded_never_success(tmp_path: Path) -> None: clock = Clock()