diff --git a/README.md b/README.md index 957dd89..6f91bb1 100644 --- a/README.md +++ b/README.md @@ -45,11 +45,11 @@ that should follow it. You do not need to register either file here. The [fleet checker](docs/node-fleet-setup.md) inspects each default branch, honors local overrides, and reports failed or unverifiable setup in the host repository. -It reuses one issue per inventory, closes it when every static check passes, and -reopens it on recurrence. This repository checks ten public repositories weekly -on Mondays at 03:00 UTC. Keep private inventories and reports in a private host; -other hosts choose their own caller schedule. Checks run five repositories at a time in one -runner, with one combined report and issue. +It keeps one issue per failure episode, closes it when every static check passes, +and creates a new issue after a later regression. This repository checks Hydra +and OmegaConf weekly on Mondays at 03:00 UTC. Keep private inventories and reports +in a private host; other hosts choose their own caller schedule. Checks run five +repositories at a time in one runner, with one combined report and issue. ## Documentation diff --git a/docs/node-fleet-setup.md b/docs/node-fleet-setup.md index 0fc9029..3843d08 100644 --- a/docs/node-fleet-setup.md +++ b/docs/node-fleet-setup.md @@ -136,17 +136,29 @@ Shared defaults and pinned repository snapshots are reused across checks. The scan continues after an individual repository fails. Any failed or unverified result fails the scanner job and opens or updates one **Node maintenance setup check** issue in the host. The reporter still runs after scan failures. A fully passing scan -closes that issue; recurrence reopens it. Repeated failures update the body -without per-run comments. An invalid or unreadable inventory also reports against -that list when its repository/path identity is valid. +closes that issue and records recovery. A later regression creates a new issue. +Repeated failures, including changed findings, update the current issue body +without per-run comments. If you close an unresolved issue manually, the next +failing scan reopens that issue. An invalid or unreadable inventory also reports +against that list when its repository/path identity is valid. The issue starts with a results table and links to each repository. Exact commit identities are in the expandable **Checked revisions** section. Issue identity uses the inventory repository and path, so changing its contents -or ref keeps the same issue. Different paths have separate issues. Runs are -serialized in the host to avoid competing issue creation. Existing ambiguous -duplicate marked issues cause a reporting error instead of another issue. +or ref keeps the current incident. Different paths have separate incidents. Runs +are serialized in the host to avoid competing issue creation. The reporter reads +bot-created issues from newest to oldest and stops at the latest matching issue; +it does not scan older incidents or add comments on every run. Newer unrelated +bot issues still count toward the lookup limit of 100 pages of 100 entries. If +the lookup cannot finish, it fails without creating another issue. + +Recovery is recorded in a hidden marker in the issue body. Keep this marker when +editing an issue. Existing issues from earlier workflow versions are adopted as +unresolved until a scan with the new version confirms recovery, even if already +closed. Fully passing scans then leave the closed incident unchanged. No new +permissions or external state store are needed. + Invalid inventory location inputs that cannot identify a safe list fail the workflow without opening an issue. diff --git a/node/runtime/fleet_issue.py b/node/runtime/fleet_issue.py index a1d0a49..aee9124 100644 --- a/node/runtime/fleet_issue.py +++ b/node/runtime/fleet_issue.py @@ -1,4 +1,4 @@ -"""Maintain one central setup issue per inventory, without touching consumers.""" +"""Maintain one setup issue per failure episode, without touching consumers.""" import argparse import hashlib @@ -7,6 +7,7 @@ import os import re import runpy +import urllib.parse from pathlib import Path ISSUES = runpy.run_path(str(Path(__file__).with_name("configuration_issue.py"))) @@ -14,6 +15,8 @@ MAX_ISSUE_BYTES = ISSUES["MAX_ISSUE_BYTES"] DIAGNOSTIC_LIMIT = 1000 TRUNCATION_SUFFIX = "… [truncated]" +RESOLVED = "" +ACTIVE = "" def validate_report(report): @@ -97,6 +100,93 @@ def marker(source): ) +def latest_issue(repository, identity, request): + """Stop at the newest matching bot issue; older incidents are historical.""" + for page in range(1, 101): + query = urllib.parse.urlencode( + { + "state": "all", + "creator": ISSUES["BOT"], + "sort": "created", + "direction": "desc", + "per_page": 100, + "page": page, + } + ) + issues = request("GET", f"repos/{repository}/issues?{query}") + if not isinstance(issues, list): + raise ValueError("GitHub did not return an issue list; no issue created") + for issue in issues: + if not isinstance(issue, dict): + raise ValueError("GitHub returned an invalid issue; no issue created") + author = issue.get("user") + if ( + "pull_request" not in issue + and isinstance(author, dict) + and author.get("login") == ISSUES["BOT"] + and author.get("type") == "Bot" + and isinstance(issue.get("body"), str) + and issue["body"].startswith(identity + "\n") + ): + if ( + type(issue.get("number")) is not int + or issue["number"] <= 0 + or issue.get("state") not in ("open", "closed") + ): + raise ValueError("GitHub returned an invalid marked fleet issue") + return issue + if len(issues) < 100: + return None + raise ValueError( + "Cannot find latest fleet issue within lookup limit; no issue created" + ) + + +def maintain_incident(repository, *, identity, title, body, failing, request): + if not body.startswith(identity + "\n") or len(body.encode()) > MAX_ISSUE_BYTES: + raise ValueError("invalid marked fleet issue body") + issue = latest_issue(repository, identity, request) + recovered = issue is not None and issue["body"].startswith( + identity + "\n" + RESOLVED + "\n" + ) + if not failing and (issue is None or (recovered and issue["state"] == "closed")): + return {"operation": "unchanged", "number": issue["number"] if issue else None} + if issue is None or (failing and recovered): + result = request( + "POST", f"repos/{repository}/issues", {"title": title, "body": body} + ) + operation = "created" + else: + state = "open" if failing else "closed" + if issue["state"] == state and issue["body"] == body: + return {"operation": "unchanged", "number": issue["number"]} + result = request( + "PATCH", + f"repos/{repository}/issues/{issue['number']}", + {"body": body, "state": state}, + ) + operation = ( + "closed" + if not failing + else "reopened" + if issue["state"] == "closed" + else "updated" + ) + if not isinstance(result, dict) or result.get("number") != issue["number"]: + raise ValueError( + "Fleet issue update identity changed; no additional issue created" + ) + if ( + not isinstance(result, dict) + or type(result.get("number")) is not int + or result["number"] <= 0 + ): + raise ValueError( + "GitHub did not return a fleet issue identity; inspect the run before retrying" + ) + return {"operation": operation, "number": result["number"]} + + def update_issue(repository, report, run_url, request=ISSUES["github_request"]): validate_report(report) source = report["inventory"] @@ -150,6 +240,7 @@ def cell(value): ) lines = [ marker(source), + RESOLVED if report["status"] == "ok" else ACTIVE, "", "## Node maintenance setup check", "", @@ -195,7 +286,7 @@ def cell(value): "", f"[Workflow run]({run_url})", "", - "This issue is reused for this inventory. A fully passing scan closes it; a later failure or unverifiable setup reopens it.", + "This issue tracks one failure episode for this inventory. A fully passing scan closes it; a later regression creates a new issue. Closing it manually while problems persist causes it to reopen on the next scan.", ] ) return "\n".join(lines) @@ -217,9 +308,9 @@ def cell(value): "fleet issue body exceeds its fixed identity and status budget" ) body = best - return ISSUES["maintain_issue"]( + return maintain_incident( repository, - marker=marker(source), + identity=marker(source), title="Node maintenance setup check: " + source["path"][:180], body=body, failing=report["status"] != "ok", diff --git a/node/tests/test_fleet_setup.py b/node/tests/test_fleet_setup.py index 06e53e0..25207f1 100644 --- a/node/tests/test_fleet_setup.py +++ b/node/tests/test_fleet_setup.py @@ -9,6 +9,7 @@ from concurrent.futures import ThreadPoolExecutor from pathlib import Path from threading import Barrier, Event, Lock +from urllib.parse import parse_qs, urlparse import pytest import yaml @@ -405,10 +406,20 @@ def test_symlink_and_changed_blob_are_rejected(): class Issues: def __init__(self): self.items = [] + self.calls = [] def request(self, method, path, payload=None): + self.calls.append((method, path, copy.deepcopy(payload))) if method == "GET": - return copy.deepcopy(self.items) + query = parse_qs(urlparse(path).query) + assert query["state"] == ["all"] + assert query["creator"] == ["github-actions[bot]"] + assert query["sort"] == ["created"] + assert query["direction"] == ["desc"] + page = int(query["page"][0]) + assert query["per_page"] == ["100"] + items = sorted(self.items, key=lambda issue: issue["number"], reverse=True) + return copy.deepcopy(items[(page - 1) * 100 : page * 100]) if method == "POST": result = { **payload, @@ -423,7 +434,7 @@ def request(self, method, path, payload=None): return self.items[number - 1] -def test_one_issue_per_inventory_updates_closes_and_reopens(): +def test_one_issue_per_failure_episode_with_manual_closure_and_recovery(): github = GitHub() issues = Issues() report = FLEET["check_fleet"]( @@ -454,15 +465,153 @@ def update(): ) assert "| extra
[link]" in issues.items[0]["body"] assert "Checked revisions" in issues.items[0]["body"] + issues.items[0]["state"] = "closed" + assert update() == {"operation": "reopened", "number": 1} + report["results"][0]["diagnostic"] = "Different setup failure" + assert update() == {"operation": "updated", "number": 1} report["status"] = "ok" report["results"][0].update(status="ok", diagnostic="Static setup checks passed") assert update()["operation"] == "closed" + recovered = copy.deepcopy(issues.items[0]) + assert REPORTER["RESOLVED"] in recovered["body"] + assert update() == {"operation": "unchanged", "number": 1} + recovered_body = issues.items[0]["body"] + issues.items[0]["state"] = "open" report["status"] = "unverified" report["diagnostic"] = "Access unavailable" - assert update()["operation"] == "reopened" - assert len(issues.items) == 1 + assert update() == {"operation": "created", "number": 2} + assert issues.items[0]["body"] == recovered_body == recovered["body"] + assert update() == {"operation": "unchanged", "number": 2} report["inventory"]["path"] = "defaults/other.json" - assert update()["operation"] == "created" and len(issues.items) == 2 + assert update()["operation"] == "created" and len(issues.items) == 3 + + +@pytest.fixture +def fleet_report(): + return FLEET["check_fleet"]( + GitHub(), + {"repository": "owner/settings", "path": "defaults/repos.json", "ref": "main"}, + ) + + +def test_healthy_fleet_does_not_create_issue_or_rewrite_recovered_history(fleet_report): + issues = Issues() + + def update(): + return REPORTER["update_issue"]( + "owner/host", fleet_report, "run", issues.request + ) + + assert update() == {"operation": "unchanged", "number": None} + assert all(call[0] == "GET" for call in issues.calls) + fleet_report["status"] = "unverified" + assert update()["operation"] == "created" + fleet_report["status"] = "ok" + assert update()["operation"] == "closed" + issues.calls.clear() + assert update() == {"operation": "unchanged", "number": 1} + assert len(issues.calls) == 1 and issues.calls[0][0] == "GET" + + +@pytest.mark.parametrize("closed", [False, True]) +@pytest.mark.parametrize("healthy", [False, True]) +def test_legacy_issue_is_adopted_until_recovery_is_confirmed( + fleet_report, closed, healthy +): + issues = Issues() + fleet_report["status"] = "unverified" + REPORTER["update_issue"]("owner/host", fleet_report, "run", issues.request) + issue = issues.items[0] + issue["body"] = issue["body"].replace(REPORTER["ACTIVE"] + "\n", "") + issue["state"] = "closed" if closed else "open" + fleet_report["status"] = "ok" if healthy else "unverified" + result = REPORTER["update_issue"]("owner/host", fleet_report, "run", issues.request) + assert result == { + "operation": "closed" if healthy else "reopened" if closed else "updated", + "number": 1, + } + assert len(issues.items) == 1 + assert (REPORTER["RESOLVED"] if healthy else REPORTER["ACTIVE"]) in issue["body"] + + +def test_lookup_stops_at_newest_matching_incident_without_scanning_history( + fleet_report, +): + issues = Issues() + fleet_report["status"] = "unverified" + REPORTER["update_issue"]("owner/host", fleet_report, "run", issues.request) + historical = copy.deepcopy(issues.items[0]) + for number in range(2, 302): + issues.items.append({**historical, "number": number, "state": "closed"}) + # The newest incident wins even with many historical matches. PRs and + # user-authored lookalikes cannot take over the reporter's identity. + issues.items.extend( + [ + {**historical, "number": 302, "pull_request": {}}, + {**historical, "number": 303, "user": {"login": "someone", "type": "User"}}, + ] + ) + issues.calls.clear() + result = REPORTER["update_issue"]("owner/host", fleet_report, "run", issues.request) + assert result == {"operation": "reopened", "number": 301} + assert [call[0] for call in issues.calls] == ["GET", "PATCH"] + assert issues.items[0] == historical + + +def test_lookup_paginates_newer_unrelated_issues(fleet_report): + issues = Issues() + fleet_report["status"] = "unverified" + REPORTER["update_issue"]("owner/host", fleet_report, "run", issues.request) + for number in range(2, 102): + issues.items.append({"number": number, "body": "unrelated"}) + issues.calls.clear() + result = REPORTER["update_issue"]("owner/host", fleet_report, "run", issues.request) + assert result == {"operation": "unchanged", "number": 1} + assert len(issues.calls) == 2 + assert issues.calls[-1][1].endswith("page=2") + + +@pytest.mark.parametrize( + "response, message", + [ + ({}, "issue list"), + ([None], "invalid issue"), + ([{"number": 1, "state": "unknown"}], "invalid marked fleet issue"), + ([{"number": True, "state": "open"}], "invalid marked fleet issue"), + ], +) +def test_lookup_rejects_invalid_api_evidence_without_writes( + fleet_report, response, message +): + calls = [] + + def request(method, path, payload=None): + calls.append(method) + if isinstance(response, list) and response[0] is not None: + return [ + { + **response[0], + "body": REPORTER["marker"](fleet_report["inventory"]) + "\nold", + "user": {"login": "github-actions[bot]", "type": "Bot"}, + } + ] + return response + + with pytest.raises(ValueError, match=message): + REPORTER["update_issue"]("owner/host", fleet_report, "run", request) + assert calls == ["GET"] + + +def test_incomplete_lookup_does_not_create_duplicate(fleet_report): + calls = [] + + def request(method, path, payload=None): + calls.append(method) + return [{"body": "unrelated"}] * 100 + + with pytest.raises(ValueError, match="lookup limit"): + REPORTER["update_issue"]("owner/host", fleet_report, "run", request) + assert calls == ["GET"] * 100 def test_workflow_splits_read_access_and_host_issue_write_access(): @@ -799,5 +948,5 @@ def test_large_fleet_issue_is_bounded_and_retains_member_identity(): REPORTER["update_issue"]("owner/host", report, run_url, issues.request)[ "operation" ] - == "reopened" + == "created" )