From c73a808575abc874026552326e3d9269ce5b45d6 Mon Sep 17 00:00:00 2001 From: ahrazzle <38987266+ahrazzle@users.noreply.github.com> Date: Tue, 13 Oct 2026 05:30:05 -0500 Subject: [PATCH] Harden quorum counting, push guards, bus/label sync, config validation, policy fidelity, and resource hygiene - approve/core: count quorum actors case-insensitively so one identity cannot approve twice through case variants. - execute/base: validate every push refspec destination, not just the last. - approve/bus: accept sync envelopes with seq null. - approve/labels: snapshot the timeline before adding the mirrored label so an intervening maintainer label is not misattributed. - config: validate policy.branch_protection types at load instead of crashing mid-run. - analyze/policy: record unknown (not silence) for secret_scanning on private repos under a paid plan; the free-plan blocked_by_plan finding is unchanged. - collect/github: drop the collaborators field from the GraphQL query and normalise; nothing consumes it. - collect/github: log a warning when the security-extras REST read fails instead of degrading silently. - fleet: clean the preview workdir in a finally block so a failing fixer does not leak full clones. Each fix carries a regression test that fails with the fix reverted. --- mimar/analyze/policy.py | 5 ++++- mimar/approve/bus.py | 2 +- mimar/approve/core.py | 3 ++- mimar/approve/labels.py | 9 ++++++--- mimar/collect/github.py | 10 ++++++---- mimar/config.py | 8 +++++++- mimar/execute/base.py | 9 ++++++--- mimar/fleet.py | 12 +++++++----- tests/test_analyze.py | 10 ++++++++++ tests/test_approve.py | 34 ++++++++++++++++++++++++++++++++++ tests/test_collect.py | 20 ++++++++++++++++++-- tests/test_execute.py | 7 +++++++ tests/test_fleet_planconfig.py | 20 ++++++++++++++++++++ tests/test_misc.py | 4 ++++ 14 files changed, 132 insertions(+), 21 deletions(-) diff --git a/mimar/analyze/policy.py b/mimar/analyze/policy.py index f162f9c..87750a3 100644 --- a/mimar/analyze/policy.py +++ b/mimar/analyze/policy.py @@ -100,7 +100,10 @@ def add(cls, status, detail, fix=None): ss = pol.get("secret_scanning") if ss and ss != "off": if private and ss == "public_only": - add("secret_scanning", "blocked_by_plan", "secret scanning on private repos needs a paid plan") if free else None + if free: + add("secret_scanning", "blocked_by_plan", "secret scanning on private repos needs a paid plan") + else: + add("secret_scanning", "unknown", "secret scanning status not collected for private repos") elif snap.get("fork"): add("secret_scanning", "skipped", "fork") else: diff --git a/mimar/approve/bus.py b/mimar/approve/bus.py index 3ea1f54..04d72aa 100644 --- a/mimar/approve/bus.py +++ b/mimar/approve/bus.py @@ -79,7 +79,7 @@ def sync_bus(ctx) -> dict: seats = set(cfg["bus"]["maintainer_seats"]) top = since for e in envs: - top = max(top, int(e.get("seq", 0))) + top = max(top, int(e.get("seq") or 0)) for e in cands: res["seen"] += 1 for m in MSG_RE.finditer(e["body"]): diff --git a/mimar/approve/core.py b/mimar/approve/core.py index 53a038d..26872b9 100644 --- a/mimar/approve/core.py +++ b/mimar/approve/core.py @@ -60,7 +60,8 @@ def evaluate(ctx, p: dict, ignore_ttl: bool = False, at: dt.datetime | None = No approvers = {} for (src, actor), v in actions.items(): if v["action"] == "approve" and (src == "divan_bus" or actor.lower() in maint): - approvers[actor] = v["event_ts"] + # one person, one vote: a bus seat and a GitHub login that differ only by case are the same approver + approvers[actor.lower()] = v["event_ts"] need = quorum_for(cfg, p["kind"]) if len(approvers) < need: why = f"{len(approvers)}/{need} approvals" diff --git a/mimar/approve/labels.py b/mimar/approve/labels.py index 4b6754b..c9782fa 100644 --- a/mimar/approve/labels.py +++ b/mimar/approve/labels.py @@ -107,9 +107,12 @@ def mirror_label(ctx, p: dict, action: str) -> None: n = p.get("issue_number") if not n or ctx.cfg.is_paused(): return - ctx.writes.tracker("add_label", number=int(n), labels=[label]) - events = ctx.gh.paginate(f"{tracker.repo_path(ctx)}/issues/{n}/timeline") - mine = [e for e in events if e.get("event") == "labeled" and (e.get("label") or {}).get("name") == label] + path = f"{tracker.repo_path(ctx)}/issues/{n}/timeline" + before = {str(e.get("id")) for e in ctx.gh.paginate(path)} # a maintainer's own label event must never be + ctx.writes.tracker("add_label", number=int(n), labels=[label]) # mistaken for the one mimar just added + events = ctx.gh.paginate(path) + mine = [e for e in events if e.get("event") == "labeled" and (e.get("label") or {}).get("name") == label + and str(e.get("id")) not in before] if mine: last = max(mine, key=lambda e: e.get("created_at") or "") ctx.db.x("insert or ignore into mirrored_labels(proposal,label,event_id) values(?,?,?)", (p["id"], label, str(last.get("id")))) diff --git a/mimar/collect/github.py b/mimar/collect/github.py index 125099d..b3a8f59 100644 --- a/mimar/collect/github.py +++ b/mimar/collect/github.py @@ -1,8 +1,12 @@ """Batched GraphQL collection of org repo metadata.""" from __future__ import annotations +import logging + from ..ghclient import GH +log = logging.getLogger("mimar") + QUERY = """ query($org: String!, $after: String, $n: Int!, $page: Int!) { rateLimit { cost remaining resetAt } @@ -27,7 +31,6 @@ vulnerabilityAlerts(first: 1, states: OPEN) { totalCount } branchProtectionRules(first: 10) { nodes { pattern requiresApprovingReviews requiredApprovingReviewCount allowsForcePushes allowsDeletions isAdminEnforced } } - collaborators(first: 20) { nodes { login } } mimarYml: object(expression: "HEAD:.github/mimar.yml") { ... on Blob { text } } parent { nameWithOwner pushedAt } } @@ -66,8 +69,6 @@ def normalise(node: dict) -> dict: "vuln_alerts": ((node.get("vulnerabilityAlerts") or {}).get("totalCount")), "vuln_enabled": node.get("hasVulnerabilityAlertsEnabled"), "protection": (node.get("branchProtectionRules") or {}).get("nodes") or [], - "collaborators": [c["login"] for c in ((node.get("collaborators") or {}).get("nodes") or [])] - if node.get("collaborators") else None, "override_text": (node.get("mimarYml") or {}).get("text"), "parent": node.get("parent"), "commits": _commits(node), @@ -113,7 +114,8 @@ def fetch_security_extras(gh: GH, org: str, snap: dict) -> dict | None: """Secret scanning status is not in GraphQL; one REST read per public non-fork repo.""" try: data = gh.rest("GET", f"/repos/{org}/{snap['name']}") - except Exception: + except Exception as e: + log.warning("security extras for %s unreadable: %s", snap["name"], e) return None sa = (data or {}).get("security_and_analysis") if sa is None: diff --git a/mimar/config.py b/mimar/config.py index bd58b8c..6979cb0 100644 --- a/mimar/config.py +++ b/mimar/config.py @@ -71,7 +71,13 @@ "backend": {"enum": ["claude_cli", "anthropic_api", "none"]}, "max_calls_per_run": {"type": "integer", "minimum": 0}}}, "budget": {"type": "object", "properties": {"graphql_points_per_run": {"type": "integer", "minimum": 1}}}, - "policy": {"type": "object"}, + "policy": {"type": "object", "properties": { + "default_branch": {"type": "string"}, + # a typo here used to crash mid-run in lowers_protection; fail fast at load instead + "branch_protection": {"type": "object", "properties": { + "require_pr": {"type": "boolean"}, "required_approvals": {"type": "integer", "minimum": 0}, + "allow_force_pushes": {"type": "boolean"}, "allow_deletions": {"type": "boolean"}, + "enforce_admins": {"type": "boolean"}}}}}, "auto_apply": {"type": "array", "items": {"enum": ["branch_protection", "dependabot_alerts", "secret_scanning", "health_fix"]}}, "policy_actions": {"type": "object", "additionalProperties": {"enum": ["log", "issue", "fix"]}, diff --git a/mimar/execute/base.py b/mimar/execute/base.py index 136d4d9..9777a6f 100644 --- a/mimar/execute/base.py +++ b/mimar/execute/base.py @@ -194,11 +194,14 @@ def safe_push(self, argv_tail: list[str], cwd, default_branch: str | None, allow brand new empty repo.""" full = ["push", *argv_tail] refs = [a for a in argv_tail if not a.startswith("-")] - dest = "" - for r in refs[1:]: - dest = r.split(":")[-1].replace("refs/heads/", "") + dests = [r.split(":")[-1].replace("refs/heads/", "") for r in refs[1:]] + for dest in dests: if default_branch and dest == default_branch and not allow_default: raise SafetyError(f"refusing to push to default branch {default_branch}") + if not dest.startswith("mimar/") and not allow_default: + # the broker only sees the last refspec, so every destination is checked here + raise SafetyError(f"refusing to push to non-mimar branch {dest}") + dest = dests[-1] if dests else "" req = WriteRequest("push_branch", repo or "unknown", {"branch": dest, "base": default_branch or "main", **({"initial": True} if allow_default else {})}) self.ctx.writes.submit(req, lambda: self.g(full, cwd=cwd)) diff --git a/mimar/fleet.py b/mimar/fleet.py index 898288a..a852bdf 100644 --- a/mimar/fleet.py +++ b/mimar/fleet.py @@ -205,11 +205,13 @@ def preview(ctx, fixer: Autofixer, repo_glob: str | None = None) -> dict[str, st repos = fleet_repos(ctx, repo_glob) root = ctx.work_dir("preview") / fixer.name out = {} - for name in fixer.find_repos(ctx, repos): - d = diff_of(ctx, fixer, name, repos[name], root / name) - if d.strip(): - out[name] = d - shutil.rmtree(root, ignore_errors=True) + try: + for name in fixer.find_repos(ctx, repos): + d = diff_of(ctx, fixer, name, repos[name], root / name) + if d.strip(): + out[name] = d + finally: + shutil.rmtree(root, ignore_errors=True) return out diff --git a/tests/test_analyze.py b/tests/test_analyze.py index c0401ac..02dce53 100644 --- a/tests/test_analyze.py +++ b/tests/test_analyze.py @@ -63,6 +63,16 @@ def test_policy_drift_and_plan_limits(collected): assert pol[("alpha-core", "required_file:README.md")] == "ok" +def test_policy_secret_scanning_private_repo_on_paid_plan_is_unknown(collected): + snap = {"name": "r", "private": True, "fork": False, "archived": False, "default_branch": "main", + "protection": [], "vuln_enabled": True} + paid = dict(collected.cfg) + paid["plan"] = "team" + fs = policy.check(snap, {"README.md"}, paid, None) + ss = [f for f in fs if f["class"] == "secret_scanning"] + assert len(ss) == 1 and ss[0]["status"] == "unknown" + + def test_policy_per_repo_override_changes_requirements(collected): snap = {"name": "r", "private": False, "fork": False, "archived": False, "default_branch": "main", "protection": [], "vuln_enabled": True, "secret_scanning": "enabled"} diff --git a/tests/test_approve.py b/tests/test_approve.py index 7595f72..33caa34 100644 --- a/tests/test_approve.py +++ b/tests/test_approve.py @@ -176,3 +176,37 @@ def test_bus_down_is_reported_not_fatal(tmp_path, home, world): setup(ctx) r = sync_approvals(ctx) assert r["errors"] and "bus" in r["errors"][0] + + +def test_quorum_counts_case_variants_as_one_approver(tmp_path, home, world): + ctx = make_ctx(tmp_path, world, {"quorum": 2, "maintainers": ["mozi"], + "approval_sources": ["github_label", "divan_bus"], + "bus": {"maintainer_seats": ["Mozi"]}}) + p = setup(ctx) + ctx.db.set_state(p["id"], hash_log=[["2026-01-01T00:00:00Z", p["plan_hash"]]]) + core.record(ctx.db, p["id"], "divan_bus", "Mozi", "approve", FUTURE, "e1", True) + core.record(ctx.db, p["id"], "github_label", "mozi", "approve", FUTURE, "e2", True) + ev = core.evaluate(ctx, proposals.get(ctx.db, p["id"])) + assert ev["state"] == "awaiting" and ev["approvers"] == ["mozi"] + + +def test_bus_envelope_without_seq_does_not_break_sync(tmp_path, home, world): + bus = FakeBus() + ctx = bus_ctx(tmp_path, world, bus) + p = setup(ctx) + m = msg(1, "saga-divan", f"mimar approve {p['id']}") + m["seq"] = None + bus.inbox = [m] + r = sync_approvals(ctx) + assert status(ctx, p) == "approved" and not r["errors"] + + +def test_mirror_label_does_not_swallow_maintainer_event(tmp_path, home, world): + bus = FakeBus() + ctx = bus_ctx(tmp_path, world, bus, approval_sources=["github_label", "divan_bus"], quorum=2, + maintainers=["maint"]) + p = setup(ctx) + ctx.gh.add_label_event(p["issue_number"], "mimar:approved", "maint", ts="2099-01-02T00:00:00Z") + bus.inbox = [msg(1, "saga-divan", f"mimar approve {p['id']}")] + sync_approvals(ctx) + assert status(ctx, p) == "approved" diff --git a/tests/test_collect.py b/tests/test_collect.py index 915684e..493f611 100644 --- a/tests/test_collect.py +++ b/tests/test_collect.py @@ -30,9 +30,25 @@ def test_budget_stops_cleanly(tmp_path, home, world): def test_normalise_handles_missing_optional_fields(): - n = github.normalise({"name": "x", "isPrivate": True, "isFork": False, "isArchived": False, "collaborators": None, + n = github.normalise({"name": "x", "isPrivate": True, "isFork": False, "isArchived": False, "defaultBranchRef": None}) - assert n["collaborators"] is None and n["commits"] == [] and n["default_branch"] is None + assert "collaborators" not in n and n["commits"] == [] and n["default_branch"] is None + + +def test_collect_query_does_not_request_unused_collaborators(): + assert "collaborators" not in github.QUERY + + +def test_fetch_security_extras_logs_api_errors(caplog): + import logging + + class BoomGH: + def rest(self, *a, **k): + raise RuntimeError("403 Forbidden") + + with caplog.at_level(logging.WARNING, logger="mimar"): + assert github.fetch_security_extras(BoomGH(), "acme", {"name": "x"}) is None + assert "security extras for x unreadable" in caplog.text def test_override_file_parsed(tmp_path, home, world): diff --git a/tests/test_execute.py b/tests/test_execute.py index 31dead8..5337bb8 100644 --- a/tests/test_execute.py +++ b/tests/test_execute.py @@ -119,6 +119,13 @@ def test_safe_push_refuses_default_branch(ctx, tmp_path): ex.safe_push(["origin", "mimar/x:refs/heads/main"], tmp_path, "main") +def test_safe_push_checks_every_refspec_not_just_the_last(ctx, tmp_path, monkeypatch): + ex = executor_for(ctx, "merge") + monkeypatch.setattr(ex, "g", lambda *a, **k: (_ for _ in ()).throw(AssertionError("git must not run"))) + with pytest.raises(SafetyError, match="non-mimar"): + ex.safe_push(["origin", "docs:refs/heads/docs", "mimar/x:refs/heads/mimar/x"], tmp_path, "main") + + # ---- merge: dry run + end to end --------------------------------------------------------- def test_merge_dry_run_shows_exact_commands(ctx): diff --git a/tests/test_fleet_planconfig.py b/tests/test_fleet_planconfig.py index 861d0e8..45e7ce8 100644 --- a/tests/test_fleet_planconfig.py +++ b/tests/test_fleet_planconfig.py @@ -240,6 +240,26 @@ def test_readme_skeleton_finds_repos_without_readme(collected, world, tmp_path): assert "+# mono" in d["mono"] and "TODO: describe" in d["mono"] +def test_preview_cleans_workdir_when_fixer_raises(collected): + class Boom(Autofixer): + name = "boom-test" + description = "x" + commit_message = "t" + pr_title = "t" + pr_body = "t" + + def find_repos(self, ctx, repos): + return ["old-tool"] + + def apply(self, workdir, repo, snap): + raise RuntimeError("boom") + + root = collected.work_dir("preview") / "boom-test" + with pytest.raises(RuntimeError, match="boom"): + preview(collected, Boom()) + assert not root.exists() + + # ---- feature 9: plan-config --------------------------------------------------------------------- def analysed(ctx): diff --git a/tests/test_misc.py b/tests/test_misc.py index 09f62ee..4fe6d8c 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -34,6 +34,10 @@ def test_invalid_config_rejected(): load_config(None, {"org": "o", "auto_apply": ["delete_repos"]}) with pytest.raises(ConfigError): load_config(None, {"org": "o", "notifiers": [{"type": "pager"}]}) + with pytest.raises(ConfigError): # a typo here used to crash mid-run in lowers_protection + load_config(None, {"org": "o", "policy": {"branch_protection": {"required_approvals": "many"}}}) + with pytest.raises(ConfigError): + load_config(None, {"org": "o", "policy": {"branch_protection": {"require_pr": "yes"}}}) with pytest.raises(ConfigError): load_config(None, {"org": None}).org