From c8253fbc3e10f8a3a74e1b79dce7dd05b54caf80 Mon Sep 17 00:00:00 2001 From: Tim Stranske Date: Mon, 4 May 2026 21:44:51 -0500 Subject: [PATCH] fix: address sync review feedback --- .../agent-event-eligibility/action.yml | 13 +- .../scripts/__tests__/detect-changes.test.js | 26 ++++ .github/scripts/agents-guard.js | 6 + .github/scripts/detect-changes.js | 3 +- .github/workflows/agents-guard.yml | 2 +- .github/workflows/agents-verifier.yml | 4 +- scripts/state_fingerprint.py | 9 +- .../.github/scripts/agents-guard.js | 6 + .../.github/scripts/detect-changes.js | 3 +- .../.github/workflows/agents-guard.yml | 2 +- .../.github/workflows/agents-verifier.yml | 4 +- tests/scripts/test_state_fingerprint.py | 123 ++++++++++++++++++ tests/workflows/test_agents_guard.py | 26 ++++ 13 files changed, 216 insertions(+), 11 deletions(-) diff --git a/.github/actions/agent-event-eligibility/action.yml b/.github/actions/agent-event-eligibility/action.yml index adbe5c20f..928fba612 100644 --- a/.github/actions/agent-event-eligibility/action.yml +++ b/.github/actions/agent-event-eligibility/action.yml @@ -18,15 +18,22 @@ inputs: required: false default: '' expected-actions: - description: Comma-separated event action allow-list, or event names for events without an action. + description: >- + Comma-separated event action allow-list, or event names for events + without an action. required: false default: '' custom-predicate: - description: JMESPath-style predicate evaluated against the event payload. Must be truthy when supplied. + description: >- + Custom predicate evaluated against the event payload. Supports payload + paths, literals, comparisons, &&/||/!, and + contains/starts_with/ends_with/length/not_null functions. required: false default: '' mode: - description: Eligibility mode. Use enforce to skip denied events, or warning to report denials without skipping. + description: >- + Eligibility mode. Use enforce to skip denied events, or warning to report + denials without skipping. required: false default: enforce outputs: diff --git a/.github/scripts/__tests__/detect-changes.test.js b/.github/scripts/__tests__/detect-changes.test.js index 52979d055..8dc5936cf 100644 --- a/.github/scripts/__tests__/detect-changes.test.js +++ b/.github/scripts/__tests__/detect-changes.test.js @@ -183,3 +183,29 @@ test('detectChanges falls back to raw github when wrapper initialization fails', assert.equal(warnings.length, 1); assert.match(warnings[0], /Failed to enable rate-limit wrapper for detect-changes/); }); + +test('detectChanges preserves non-error wrapper initialization failures', async () => { + const warnings = []; + const github = {}; + Object.defineProperty(github, 'request', { + get() { + throw 'string boom'; + }, + }); + github.hook = {}; + + await detectChanges({ + github, + core: { + warning(message) { + warnings.push(String(message)); + }, + setOutput() {}, + }, + context: { eventName: 'pull_request' }, + files: ['src/app.py'], + }); + + assert.equal(warnings.length, 1); + assert.match(warnings[0], /string boom/); +}); diff --git a/.github/scripts/agents-guard.js b/.github/scripts/agents-guard.js index 3ac7a6471..601b40a52 100644 --- a/.github/scripts/agents-guard.js +++ b/.github/scripts/agents-guard.js @@ -32,6 +32,12 @@ const ALLOW_REMOVED_PATHS = new Set( // v1 verify-to-issue workflow deprecated; v2 is the active version. // Archived to archives/deprecated-workflows/ '.github/workflows/agents-verify-to-issue.yml', + // Consolidated event hub and gate followups replace these legacy template + // entry points after the 2026-02-15 removal deadline. + '.github/workflows/agents-autofix-loop.yml', + '.github/workflows/agents-bot-comment-handler.yml', + '.github/workflows/agents-keepalive-loop.yml', + '.github/workflows/agents-verify-to-issue-v2.yml', // The verify-to-new-pr autopilot bridge was collapsed into the main workflow. '.github/workflows/agents-verify-to-new-pr-autopilot.yml', ].map((entry) => entry.toLowerCase()), diff --git a/.github/scripts/detect-changes.js b/.github/scripts/detect-changes.js index cad319f59..8c98d17ea 100644 --- a/.github/scripts/detect-changes.js +++ b/.github/scripts/detect-changes.js @@ -345,7 +345,8 @@ module.exports = { try { github = await ensureRateLimitWrapped({ github: rawGithub, core, env: process.env }); } catch (error) { - core?.warning?.(`Failed to enable rate-limit wrapper for detect-changes: ${error.message}`); + const message = error instanceof Error ? error.message : String(error); + core?.warning?.(`Failed to enable rate-limit wrapper for detect-changes: ${message}`); } return detectChanges({ github, context, core, files, fetchFiles }); }, diff --git a/.github/workflows/agents-guard.yml b/.github/workflows/agents-guard.yml index 03f759c4a..953e52ebf 100644 --- a/.github/workflows/agents-guard.yml +++ b/.github/workflows/agents-guard.yml @@ -38,7 +38,7 @@ jobs: expected-labels: >- agent:auto,agent:codex,agent:claude,agent:copilot, agents:auto-pilot,agents:keepalive - expected-actions: opened,synchronize,labeled + expected-actions: opened,reopened,synchronize,ready_for_review,labeled,unlabeled custom-predicate: pull_request - name: Checkout base ref for safety validation diff --git a/.github/workflows/agents-verifier.yml b/.github/workflows/agents-verifier.yml index 2ad12ffa4..cc3f5bbc5 100644 --- a/.github/workflows/agents-verifier.yml +++ b/.github/workflows/agents-verifier.yml @@ -244,7 +244,8 @@ jobs: page = 1 while True: separator = "&" if "?" in path else "?" - batch = api(f"{path}{separator}{urllib.parse.urlencode({'per_page': 100, 'page': page})}") + query = urllib.parse.urlencode({"per_page": 100, "page": page}) + batch = api(f"{path}{separator}{query}") results.extend(batch) if len(batch) < 100: return results @@ -264,6 +265,7 @@ jobs: } for item in files ] + diff_surface.sort(key=lambda item: item["filename"]) diff_hash = hashlib.sha256( json.dumps(diff_surface, sort_keys=True, separators=(",", ":")).encode("utf-8") ).hexdigest() diff --git a/scripts/state_fingerprint.py b/scripts/state_fingerprint.py index 5c948b323..31c93363f 100644 --- a/scripts/state_fingerprint.py +++ b/scripts/state_fingerprint.py @@ -153,10 +153,15 @@ def request(self, method: str, path: str, body: dict[str, Any] | None = None) -> except urllib.error.HTTPError as exc: detail = exc.read().decode("utf-8", errors="replace") raise RuntimeError(f"GitHub API {method} {path} failed: {exc.code} {detail}") from exc + except (urllib.error.URLError, TimeoutError, OSError) as exc: + raise RuntimeError(f"GitHub API {method} {path} failed: {exc}") from exc if not payload: return None - return json.loads(payload) + try: + return json.loads(payload) + except json.JSONDecodeError as exc: + raise RuntimeError(f"GitHub API {method} {path} returned invalid JSON: {exc}") from exc def paged_get(self, path: str) -> list[dict[str, Any]]: page = 1 @@ -376,7 +381,7 @@ def main(argv: list[str] | None = None) -> int: args = parser.parse_args(argv) try: return args.func(args) - except RuntimeError as exc: + except Exception as exc: print(str(exc), file=sys.stderr) return 1 diff --git a/templates/consumer-repo/.github/scripts/agents-guard.js b/templates/consumer-repo/.github/scripts/agents-guard.js index 3ac7a6471..601b40a52 100644 --- a/templates/consumer-repo/.github/scripts/agents-guard.js +++ b/templates/consumer-repo/.github/scripts/agents-guard.js @@ -32,6 +32,12 @@ const ALLOW_REMOVED_PATHS = new Set( // v1 verify-to-issue workflow deprecated; v2 is the active version. // Archived to archives/deprecated-workflows/ '.github/workflows/agents-verify-to-issue.yml', + // Consolidated event hub and gate followups replace these legacy template + // entry points after the 2026-02-15 removal deadline. + '.github/workflows/agents-autofix-loop.yml', + '.github/workflows/agents-bot-comment-handler.yml', + '.github/workflows/agents-keepalive-loop.yml', + '.github/workflows/agents-verify-to-issue-v2.yml', // The verify-to-new-pr autopilot bridge was collapsed into the main workflow. '.github/workflows/agents-verify-to-new-pr-autopilot.yml', ].map((entry) => entry.toLowerCase()), diff --git a/templates/consumer-repo/.github/scripts/detect-changes.js b/templates/consumer-repo/.github/scripts/detect-changes.js index cad319f59..8c98d17ea 100644 --- a/templates/consumer-repo/.github/scripts/detect-changes.js +++ b/templates/consumer-repo/.github/scripts/detect-changes.js @@ -345,7 +345,8 @@ module.exports = { try { github = await ensureRateLimitWrapped({ github: rawGithub, core, env: process.env }); } catch (error) { - core?.warning?.(`Failed to enable rate-limit wrapper for detect-changes: ${error.message}`); + const message = error instanceof Error ? error.message : String(error); + core?.warning?.(`Failed to enable rate-limit wrapper for detect-changes: ${message}`); } return detectChanges({ github, context, core, files, fetchFiles }); }, diff --git a/templates/consumer-repo/.github/workflows/agents-guard.yml b/templates/consumer-repo/.github/workflows/agents-guard.yml index c1a369fc0..a13d58652 100644 --- a/templates/consumer-repo/.github/workflows/agents-guard.yml +++ b/templates/consumer-repo/.github/workflows/agents-guard.yml @@ -38,7 +38,7 @@ jobs: expected-labels: >- agent:auto,agent:codex,agent:claude,agent:copilot, agents:auto-pilot,agents:keepalive - expected-actions: opened,synchronize,labeled + expected-actions: opened,reopened,synchronize,ready_for_review,labeled,unlabeled custom-predicate: pull_request # Mint GitHub App token early to use for API calls (avoids rate limits) diff --git a/templates/consumer-repo/.github/workflows/agents-verifier.yml b/templates/consumer-repo/.github/workflows/agents-verifier.yml index 62e1e3d5f..4b44854a3 100644 --- a/templates/consumer-repo/.github/workflows/agents-verifier.yml +++ b/templates/consumer-repo/.github/workflows/agents-verifier.yml @@ -238,7 +238,8 @@ jobs: page = 1 while True: separator = "&" if "?" in path else "?" - batch = api(f"{path}{separator}{urllib.parse.urlencode({'per_page': 100, 'page': page})}") + query = urllib.parse.urlencode({"per_page": 100, "page": page}) + batch = api(f"{path}{separator}{query}") results.extend(batch) if len(batch) < 100: return results @@ -258,6 +259,7 @@ jobs: } for item in files ] + diff_surface.sort(key=lambda item: item["filename"]) diff_hash = hashlib.sha256( json.dumps(diff_surface, sort_keys=True, separators=(",", ":")).encode("utf-8") ).hexdigest() diff --git a/tests/scripts/test_state_fingerprint.py b/tests/scripts/test_state_fingerprint.py index b4525d372..9865fbb99 100644 --- a/tests/scripts/test_state_fingerprint.py +++ b/tests/scripts/test_state_fingerprint.py @@ -1,4 +1,5 @@ import json +import urllib.error import pytest from scripts import state_fingerprint @@ -17,6 +18,35 @@ def write_fingerprint(self, workflow_name: str, fingerprint_hash: str) -> None: self.writes.append(fingerprint_hash) +class FakeResponse: + def __init__(self, payload: bytes) -> None: + self.payload = payload + + def __enter__(self) -> "FakeResponse": + return self + + def __exit__(self, *args: object) -> None: + return None + + def read(self) -> bytes: + return self.payload + + +class FakeApi: + def __init__(self, values: dict[str, object] | None = None) -> None: + self.repo = "owner/repo" + self.values = values or {} + self.requests: list[tuple[str, str, dict | None]] = [] + + def request(self, method: str, path: str, body: dict | None = None) -> object: + self.requests.append((method, path, body)) + key = f"{method} {path}" + value = self.values.get(key) + if isinstance(value, Exception): + raise value + return value + + def test_compute_fingerprint_canonicalizes_key_order() -> None: first = state_fingerprint.compute_fingerprint("wf", {"b": 2, "a": {"d": 4, "c": 3}}) second = state_fingerprint.compute_fingerprint("wf", {"a": {"c": 3, "d": 4}, "b": 2}) @@ -98,3 +128,96 @@ def test_malformed_prior_marker_is_tolerated() -> None: assert decision.should_run is True assert decision.reason == "no-prior-fingerprint" assert decision.prior_hash is None + + +def test_extract_hash_accepts_raw_json_storage_value() -> None: + fingerprint_hash = "a" * 64 + + assert ( + state_fingerprint._extract_hash(json.dumps({"hash": fingerprint_hash}), "wf") + == fingerprint_hash + ) + + +def test_variable_name_is_stable_and_within_github_limit() -> None: + workflow_name = "Verifier " + ("very-long-name-" * 20) + + first = state_fingerprint._variable_name(workflow_name) + second = state_fingerprint._variable_name(workflow_name) + + assert first == second + assert first.startswith("STATE_FINGERPRINT_VERIFIER_") + assert len(first) <= 100 + + +def test_repo_variable_storage_reads_existing_variable() -> None: + fingerprint_hash = "b" * 64 + api = FakeApi( + { + "GET /repos/owner/repo/actions/variables/STATE_FINGERPRINT_TEST": { + "value": json.dumps({"hash": fingerprint_hash}) + } + } + ) + storage = state_fingerprint.RepoVariableStorage(api, "STATE_FINGERPRINT_TEST") # type: ignore[arg-type] + + assert storage.read_fingerprint("wf") == fingerprint_hash + + +def test_repo_variable_storage_creates_missing_variable() -> None: + api = FakeApi( + { + "PATCH /repos/owner/repo/actions/variables/STATE_FINGERPRINT_TEST": RuntimeError( + "GitHub API PATCH /repos/owner/repo/actions/variables/STATE_FINGERPRINT_TEST failed: 404 missing" + ) + } + ) + storage = state_fingerprint.RepoVariableStorage(api, "STATE_FINGERPRINT_TEST") # type: ignore[arg-type] + + storage.write_fingerprint("wf", "c" * 64) + + assert api.requests[0][0] == "PATCH" + assert api.requests[1][0] == "POST" + assert api.requests[1][1] == "/repos/owner/repo/actions/variables" + + +def test_github_api_wraps_url_errors(monkeypatch: pytest.MonkeyPatch) -> None: + def raise_url_error(*args: object, **kwargs: object) -> None: + raise urllib.error.URLError("connection refused") + + monkeypatch.setattr(state_fingerprint.urllib.request, "urlopen", raise_url_error) + + api = state_fingerprint.GitHubApi("owner/repo", "token") + with pytest.raises(RuntimeError, match=r"GitHub API GET /repos/owner/repo failed:"): + api.request("GET", "/repos/owner/repo") + + +def test_github_api_wraps_json_decode_errors(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr( + state_fingerprint.urllib.request, + "urlopen", + lambda *args, **kwargs: FakeResponse(b"{not json"), + ) + + api = state_fingerprint.GitHubApi("owner/repo", "token") + with pytest.raises( + RuntimeError, match=r"GitHub API GET /repos/owner/repo returned invalid JSON:" + ): + api.request("GET", "/repos/owner/repo") + + +def test_main_catches_unexpected_exceptions( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + def raise_value_error(_name: str, _workflow: str) -> MemoryStorage: + raise ValueError("storage exploded") + + monkeypatch.setattr(state_fingerprint, "_storage_from_name", raise_value_error) + + exit_code = state_fingerprint.main( + ["compare", "--workflow", "wf", "--inputs", "{}", "--storage", "pr-comment"] + ) + + captured = capsys.readouterr() + assert exit_code == 1 + assert captured.err.strip() == "storage exploded" diff --git a/tests/workflows/test_agents_guard.py b/tests/workflows/test_agents_guard.py index 4994bcda1..a994df09e 100644 --- a/tests/workflows/test_agents_guard.py +++ b/tests/workflows/test_agents_guard.py @@ -228,6 +228,32 @@ def test_issue_intake_deletion_allowed(): assert result["commentBody"] is None +@skip_if_no_node +@pytest.mark.parametrize( + "filename", + [ + ".github/workflows/agents-autofix-loop.yml", + ".github/workflows/agents-bot-comment-handler.yml", + ".github/workflows/agents-keepalive-loop.yml", + ".github/workflows/agents-verify-to-issue-v2.yml", + ], +) +def test_consolidated_agent_workflow_deletions_allowed(filename): + result = run_guard( + files=[ + { + "filename": filename, + "status": "removed", + } + ], + codeowners=CODEOWNERS_SAMPLE, + ) + + assert result["blocked"] is False + assert not result["failureReasons"] + assert result["commentBody"] is None + + @skip_if_no_node def test_rename_blocks_with_guidance(): result = run_guard(