From 7f48247f586c934356112719356954804fb3f2dc Mon Sep 17 00:00:00 2001 From: ahrazzle <38987266+ahrazzle@users.noreply.github.com> Date: Wed, 14 Oct 2026 13:22:31 -0500 Subject: [PATCH] Harden quorum counting, push guards, bus sync, config validation, policy fidelity, and resource hygiene Second audit pass (aska-digital/ion). Re-applies the still-valid fixes from the mimar audit that the rename did not carry over, plus one extension: - approve/core: count quorum actors case-insensitively so one identity cannot approve twice through case variants (relates to #27). - execute/base: validate every push refspec destination against the default branch and the ion/ prefix, not just the last; the broker only sees the last refspec's branch. - approve/bus: accept sync envelopes with seq null instead of crashing. - 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. - 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. --- ion/analyze/policy.py | 5 ++++- ion/approve/bus.py | 2 +- ion/approve/core.py | 2 +- ion/collect/github.py | 10 ++++++---- ion/config.py | 6 +++++- ion/execute/base.py | 10 ++++++---- ion/fleet.py | 12 +++++++----- tests/test_analyze.py | 10 ++++++++++ tests/test_approve.py | 23 +++++++++++++++++++++++ tests/test_collect.py | 20 ++++++++++++++++++-- tests/test_execute.py | 9 +++++++++ tests/test_fleet_planconfig.py | 20 ++++++++++++++++++++ tests/test_misc.py | 4 ++++ 13 files changed, 114 insertions(+), 19 deletions(-) diff --git a/ion/analyze/policy.py b/ion/analyze/policy.py index 6257b43..56f5e1d 100644 --- a/ion/analyze/policy.py +++ b/ion/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/ion/approve/bus.py b/ion/approve/bus.py index 7c3fc4e..6abf38e 100644 --- a/ion/approve/bus.py +++ b/ion/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/ion/approve/core.py b/ion/approve/core.py index 53a038d..e9f5885 100644 --- a/ion/approve/core.py +++ b/ion/approve/core.py @@ -60,7 +60,7 @@ 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"] + 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/ion/collect/github.py b/ion/collect/github.py index 227b409..6a1ed99 100644 --- a/ion/collect/github.py +++ b/ion/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("ion") + 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 } } ionYml: object(expression: "HEAD:.github/ion.yml") { ... on Blob { text } } legacyYml: object(expression: "HEAD:.github/mimar.yml") { ... on Blob { text } } parent { nameWithOwner pushedAt } @@ -67,8 +70,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("ionYml") or node.get("legacyYml") or {}).get("text"), "parent": node.get("parent"), "commits": _commits(node), @@ -114,7 +115,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/ion/config.py b/ion/config.py index 95d0545..eb1b43f 100644 --- a/ion/config.py +++ b/ion/config.py @@ -72,7 +72,11 @@ "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": { + "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/ion/execute/base.py b/ion/execute/base.py index 5505be6..3a984f1 100644 --- a/ion/execute/base.py +++ b/ion/execute/base.py @@ -221,12 +221,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}") - req = WriteRequest("push_branch", repo or "unknown", {"branch": dest, "base": default_branch or "main", + if not dest.startswith("ion/") and not allow_default: + raise SafetyError(f"refusing to push non-ion branch {dest}") + req = WriteRequest("push_branch", repo or "unknown", {"branch": dests[-1] if dests else "", + "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/ion/fleet.py b/ion/fleet.py index d51737d..fc79c67 100644 --- a/ion/fleet.py +++ b/ion/fleet.py @@ -209,11 +209,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 a7b3ffc..b58448c 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 85f6567..e8c0657 100644 --- a/tests/test_approve.py +++ b/tests/test_approve.py @@ -187,3 +187,26 @@ 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": ["ada"], + "approval_sources": ["github_label", "divan_bus"], + "bus": {"maintainer_seats": ["Ada"]}}) + 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", "Ada", "approve", FUTURE, "e1", True) + core.record(ctx.db, p["id"], "github_label", "ada", "approve", FUTURE, "e2", True) + ev = core.evaluate(ctx, proposals.get(ctx.db, p["id"])) + assert ev["state"] == "awaiting" and ev["approvers"] == ["ada"] + + +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"ion approve {p['id']}") + m["seq"] = None + bus.inbox = [m] + r = sync_approvals(ctx) + assert status(ctx, p) == "approved" and not r["errors"] diff --git a/tests/test_collect.py b/tests/test_collect.py index 9c3a802..2068ea0 100644 --- a/tests/test_collect.py +++ b/tests/test_collect.py @@ -53,9 +53,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="ion"): + 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 b8258a7..1a6634b 100644 --- a/tests/test_execute.py +++ b/tests/test_execute.py @@ -120,6 +120,15 @@ def test_safe_push_refuses_default_branch(ctx, tmp_path): ex.safe_push(["origin", "ion/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") + pushed = [] + monkeypatch.setattr(ex, "g", lambda argv, **kw: pushed.append(argv)) + with pytest.raises(SafetyError, match="non-ion branch"): + ex.safe_push(["origin", "docs:refs/heads/docs", "ion/x:refs/heads/ion/x"], tmp_path, "main") + assert pushed == [] + + def test_safe_write_refuses_symlinks_and_paths_outside_the_checkout(tmp_path): root, out = tmp_path / "clone", outside(tmp_path) root.mkdir() diff --git a/tests/test_fleet_planconfig.py b/tests/test_fleet_planconfig.py index 2f42a14..6fde02f 100644 --- a/tests/test_fleet_planconfig.py +++ b/tests/test_fleet_planconfig.py @@ -241,6 +241,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() + + def test_autofix_refuses_to_write_through_a_symlinked_github_dir(ctx, world, tmp_path): out = outside(tmp_path) plant_symlink(world, tmp_path, "alpha-core", ".github", str(out)) diff --git a/tests/test_misc.py b/tests/test_misc.py index cfd4f54..0f77cbd 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -36,6 +36,10 @@ def test_invalid_config_rejected(): load_config(None, {"org": "o", "notifiers": [{"type": "pager"}]}) with pytest.raises(ConfigError): load_config(None, {"org": None}).org + with pytest.raises(ConfigError): + 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"}}}) def test_no_org_specific_paths_in_code():