From 10bdb683cb64a0ff3b5c1e87ddb9488f86f1d922 Mon Sep 17 00:00:00 2001 From: tegwick Date: Tue, 25 Aug 2026 10:32:51 +0200 Subject: [PATCH] fix(registrar): make --confirm-primary assert authority, not liveness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _check_primary accepted any instance reporting status=ok and db=connected. A local cache and the central hub both satisfied that for seven weeks while every registration went to the cache — a liveness check wearing an authority check's name. It now requires the hub to declare instance_role=primary. An instance that declares nothing is refused with a message naming what to set; proceeding anyway requires an explicit --allow-unverified-primary rather than a silent default. Four tests cover the logic directly; the existing suite stubbed _check_primary and never exercised it. Refs CUST-WP-0067-T03 Co-Authored-By: Claude Opus 5 Assistant: claude-code Assistant-Model: opus Assistant-Process: 2583210@bnt-lap001 Assistant-Session: f2bff2d5-e9b2-4338-92ca-10282a927006 --- src/repo_manager/cli.py | 7 ++ .../commands/registrar_reconcile.py | 35 ++++++++- tests/test_registrar_reconcile.py | 74 +++++++++++++++++-- 3 files changed, 105 insertions(+), 11 deletions(-) diff --git a/src/repo_manager/cli.py b/src/repo_manager/cli.py index fa1df44..6ea6ff4 100644 --- a/src/repo_manager/cli.py +++ b/src/repo_manager/cli.py @@ -64,6 +64,12 @@ def main(argv: list[str] | None = None) -> int: help="Confirm that --api-base is the authoritative hub", ) p_registrar.add_argument("--push", action="store_true", help="Push the registrar commit") + p_registrar.add_argument( + "--allow-unverified-primary", + action="store_true", + help="Proceed when the hub declares no instance role. Explicit on purpose: " + "the alternative is silently trusting whatever answered.", + ) registrar_mode = p_registrar.add_mutually_exclusive_group() registrar_mode.add_argument( "--repair-workplan", @@ -517,6 +523,7 @@ def main(argv: list[str] | None = None) -> int: api_base=args.api_base, statehub_bin=args.statehub_bin, confirm_primary=args.confirm_primary, + allow_unverified_primary=args.allow_unverified_primary, push=args.push, repair_workplan=args.repair_workplan, bootstrap_empty_projection=args.bootstrap_empty_projection, diff --git a/src/repo_manager/commands/registrar_reconcile.py b/src/repo_manager/commands/registrar_reconcile.py index 6703270..d6b470c 100644 --- a/src/repo_manager/commands/registrar_reconcile.py +++ b/src/repo_manager/commands/registrar_reconcile.py @@ -176,7 +176,16 @@ def _check_git(repo: Path) -> tuple[dict[str, Any], str | None]: }, None -def _check_primary(api_base: str) -> tuple[dict[str, Any], str | None]: +def _check_primary( + api_base: str, *, allow_unverified: bool = False +) -> tuple[dict[str, Any], str | None]: + """Verify the target is the authoritative hub, not merely a healthy one. + + This used to assert only status/db, which every instance satisfies — a + liveness check wearing an authority check's name. A local cache and central + both passed it for seven weeks while onboarding went to the cache + (CUST-WP-0067-T03, ADR-010). + """ try: response = httpx.get(f"{api_base.rstrip('/')}/state/health", timeout=10.0) response.raise_for_status() @@ -185,7 +194,24 @@ def _check_primary(api_base: str) -> tuple[dict[str, Any], str | None]: return {}, f"primary State Hub health check failed: {exc}" if payload.get("status") != "ok" or payload.get("db") != "connected": return payload, "State Hub is not healthy and database-connected" - return payload, None + + role = payload.get("instance_role") + label = payload.get("instance_label") or api_base + if role == "primary": + return payload, None + if role in (None, "unknown"): + if allow_unverified: + return payload, None + return payload, ( + f"{api_base} does not declare an instance role, so it cannot be " + "confirmed as the authoritative hub. Set STATE_HUB_INSTANCE_ROLE=" + "primary on the central deployment. To proceed without that " + "guarantee, pass --allow-unverified-primary explicitly." + ) + return payload, ( + f"{api_base} reports instance_role={role!r} ({label}); refusing to treat " + "it as the authoritative hub" + ) def _workplan_projection_id(repo: Path, canonical_id: str) -> str | None: @@ -410,6 +436,7 @@ def registrar_reconcile( api_base: str = "http://127.0.0.1:8000", statehub_bin: str | None = None, confirm_primary: bool = False, + allow_unverified_primary: bool = False, push: bool = False, repair_workplan: str | None = None, bootstrap_empty_projection: bool = False, @@ -502,7 +529,9 @@ def registrar_reconcile( "rejected", evidence, {"code": "git_precondition_failed", "message": git_error}, cid ) - health, health_error = _check_primary(api_base) + health, health_error = _check_primary( + api_base, allow_unverified=allow_unverified_primary + ) evidence["state_hub_health"] = health if health_error: return RegistrarResult( diff --git a/tests/test_registrar_reconcile.py b/tests/test_registrar_reconcile.py index 56c6ffd..1aa016a 100644 --- a/tests/test_registrar_reconcile.py +++ b/tests/test_registrar_reconcile.py @@ -172,7 +172,7 @@ def test_linked_closed_workplan_does_not_reopen_historical_task_gaps(tmp_path: P def test_rejects_dirty_or_unsynced_repository(tmp_path: Path, monkeypatch) -> None: repo = _fixture(tmp_path) - monkeypatch.setattr(rr, "_check_primary", lambda _api: ({"status": "ok", "db": "connected"}, None)) + monkeypatch.setattr(rr, "_check_primary", lambda _api, **_: ({"status": "ok", "db": "connected", "instance_role": "primary"}, None)) (repo / "note.txt").write_text("dirty", encoding="utf-8") dirty = rr.registrar_reconcile(repo, confirm_primary=True) assert dirty.status == "rejected" @@ -242,7 +242,7 @@ status: accepted def test_scopes_registrar_env_and_commits_assigned_ids(tmp_path: Path, monkeypatch) -> None: repo = _fixture(tmp_path) - monkeypatch.setattr(rr, "_check_primary", lambda _api: ({"status": "ok", "db": "connected"}, None)) + monkeypatch.setattr(rr, "_check_primary", lambda _api, **_: ({"status": "ok", "db": "connected", "instance_role": "primary"}, None)) def fake_run(command, *, env): assert command[1:3] == ["fix-consistency", "--path"] @@ -282,7 +282,7 @@ def test_accepts_unrelated_assessment_fail_after_exact_requested_verification( monkeypatch.setattr( rr, "_check_primary", - lambda _api: ({"status": "ok", "db": "connected"}, None), + lambda _api, **_: ({"status": "ok", "db": "connected", "instance_role": "primary"}, None), ) def fake_run(command, *, env): @@ -335,7 +335,7 @@ def test_statehub_timeout_returns_structured_failure(tmp_path: Path, monkeypatch monkeypatch.setattr( rr, "_check_primary", - lambda _api: ({"status": "ok", "db": "connected"}, None), + lambda _api, **_: ({"status": "ok", "db": "connected", "instance_role": "primary"}, None), ) monkeypatch.setattr( rr, @@ -379,7 +379,7 @@ def test_repairs_an_already_identified_workplan_projection(tmp_path: Path, monke monkeypatch.setattr( rr, "_check_primary", - lambda _api: ({"status": "ok", "db": "connected"}, None), + lambda _api, **_: ({"status": "ok", "db": "connected", "instance_role": "primary"}, None), ) monkeypatch.setattr( rr, @@ -428,7 +428,7 @@ def test_repair_fails_when_exact_projection_remains_absent(tmp_path: Path, monke monkeypatch.setattr( rr, "_check_primary", - lambda _api: ({"status": "ok", "db": "connected"}, None), + lambda _api, **_: ({"status": "ok", "db": "connected", "instance_role": "primary"}, None), ) monkeypatch.setattr( rr, @@ -476,7 +476,7 @@ def test_bootstraps_and_verifies_a_completely_empty_projection(tmp_path: Path, m monkeypatch.setattr( rr, "_check_primary", - lambda _api: ({"status": "ok", "db": "connected"}, None), + lambda _api, **_: ({"status": "ok", "db": "connected", "instance_role": "primary"}, None), ) monkeypatch.setattr( rr, @@ -530,7 +530,7 @@ def test_empty_projection_bootstrap_refuses_existing_rows(tmp_path: Path, monkey monkeypatch.setattr( rr, "_check_primary", - lambda _api: ({"status": "ok", "db": "connected"}, None), + lambda _api, **_: ({"status": "ok", "db": "connected", "instance_role": "primary"}, None), ) monkeypatch.setattr( rr, @@ -554,3 +554,61 @@ def test_empty_projection_bootstrap_refuses_existing_rows(tmp_path: Path, monkey assert result.status == "rejected" assert result.error and result.error["code"] == "bootstrap_precondition_failed" + + +class TestCheckPrimary: + """_check_primary must assert authority, not just liveness (CUST-WP-0067-T03). + + The previous version accepted any instance reporting status=ok and + db=connected. A local cache and the central hub both satisfied that for + seven weeks while every registration went to the cache. + """ + + @staticmethod + def _resp(payload): + class R: + def raise_for_status(self): + return None + + def json(self): + return payload + + return R() + + def test_accepts_declared_primary(self, monkeypatch): + monkeypatch.setattr( + rr.httpx, "get", + lambda *a, **k: self._resp( + {"status": "ok", "db": "connected", "instance_role": "primary"} + ), + ) + _, err = rr._check_primary("http://hub") + assert err is None + + def test_rejects_declared_cache(self, monkeypatch): + monkeypatch.setattr( + rr.httpx, "get", + lambda *a, **k: self._resp( + {"status": "ok", "db": "connected", "instance_role": "cache", + "instance_label": "workstation"} + ), + ) + _, err = rr._check_primary("http://hub") + assert err and "cache" in err + + def test_rejects_healthy_instance_that_declares_nothing(self, monkeypatch): + """A healthy hub is not thereby the authoritative one.""" + monkeypatch.setattr( + rr.httpx, "get", + lambda *a, **k: self._resp({"status": "ok", "db": "connected"}), + ) + _, err = rr._check_primary("http://hub") + assert err and "does not declare an instance role" in err + + def test_unverified_requires_an_explicit_opt_in(self, monkeypatch): + monkeypatch.setattr( + rr.httpx, "get", + lambda *a, **k: self._resp({"status": "ok", "db": "connected"}), + ) + _, err = rr._check_primary("http://hub", allow_unverified=True) + assert err is None