From 272ea5499cf0b0c4f451c68647e386cd81f6cee1 Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Fri, 6 Nov 2026 13:36:37 +0800 Subject: [PATCH] ci: Add clearing-label recorder diagnostics --- .github/workflows/risk-gate.yml | 41 +++++- tests/test_risk_gate_dependabot_identity.py | 144 ++++++++++++++++++++ 2 files changed, 179 insertions(+), 6 deletions(-) diff --git a/.github/workflows/risk-gate.yml b/.github/workflows/risk-gate.yml index d5bff53..5aa68fe 100644 --- a/.github/workflows/risk-gate.yml +++ b/.github/workflows/risk-gate.yml @@ -53,17 +53,23 @@ jobs: with: script: | const label = context.payload.label?.name; - if (!['owner-approved', 'agent-approved'].includes(label)) return; + const expected = ['owner-approved', 'agent-approved'].includes(label); + const noRecord = reason => { + core.info(`clearing-label-recorder reason=${reason}`); + if (expected) core.warning(`clearing-label-recorder no_status_record reason=${reason}`); + }; + if (!expected) return noRecord('non_clearing_label'); // Only the configured approver's label carries a claim. - if (!process.env.APPROVAL_OWNER || context.payload.sender?.login !== process.env.APPROVAL_OWNER) return; + if (!process.env.APPROVAL_OWNER || context.payload.sender?.login !== process.env.APPROVAL_OWNER) return noRecord('approver_mismatch'); const sha = context.payload.pull_request?.head?.sha; // A closed pull request keeps no authority over a commit that // another pull request may now use. - if (!sha || context.payload.pull_request?.state !== 'open') return; + if (!sha) return noRecord('missing_head'); + if (context.payload.pull_request?.state !== 'open') return noRecord('pr_not_open'); // The pull request is part of the record: two PRs can share a // commit, and an approval given on one must not clear the other. const number = context.payload.pull_request?.number; - if (!number) return; + if (!number) return noRecord('missing_pr_number'); // The event id is part of the record too. A label can be revoked // and added again later; without the id, moving the head back would // revive the revoked approval, because the record and the live @@ -77,23 +83,46 @@ jobs: // to the addition's own time, and that snapshot arrived with the // head it records, so matching on that time names this addition. const addedAt = context.payload.pull_request?.updated_at; - if (!addedAt) return; + if (!addedAt) return noRecord('missing_updated_at'); const events = await github.paginate(github.rest.issues.listEvents, { ...context.repo, issue_number: number, per_page: 100, + }).catch(error => { + noRecord('event_read_failed'); + throw error; }); const matches = events.filter(event => event.event === 'labeled' && event.label?.name === label && event.actor?.login === process.env.APPROVAL_OWNER && event.created_at === addedAt); + // Log only correlation fields, never the payload, actor or credentials. + const evidence = { + payload_updated_at: addedAt, + match_count: matches.length, + head_equal: sha === context.sha, + pr_number_equal: number === context.issue.number, + }; + core.info(`clearing-label-recorder event_match=${JSON.stringify(evidence)}`); + for (const event of events.filter(event => event.event === 'labeled' + && event.label?.name === label + && event.actor?.login === process.env.APPROVAL_OWNER)) { + core.info(`clearing-label-recorder event_candidate=${JSON.stringify({ + ...evidence, issue_event_created_at: event.created_at ?? null, + })}`); + } // Two additions in the same second cannot be told apart; recording // either could credit the wrong head, so this delivery records // nothing and the gate stays closed until a later addition. - if (matches.length !== 1) return; + if (matches.length !== 1) return noRecord('match_count_ne_1'); const record = matches[0]; await github.rest.repos.createCommitStatus({ ...context.repo, sha, context: 'clearing-label', state: 'success', description: `${label} for PR ${number} at event ${record.id} on this head`, + }).catch(error => { + core.info('clearing-label-recorder reason=status_write_failed'); + core.warning('clearing-label-recorder status_write_unconfirmed reason=status_write_failed'); + throw error; }); + core.info('clearing-label-recorder reason=status_recorded'); evaluate: name: Evaluate PR governance diff --git a/tests/test_risk_gate_dependabot_identity.py b/tests/test_risk_gate_dependabot_identity.py index d5c2921..133546c 100644 --- a/tests/test_risk_gate_dependabot_identity.py +++ b/tests/test_risk_gate_dependabot_identity.py @@ -1,6 +1,7 @@ """Execute the trusted workflow predicate against synthetic commit metadata.""" import json +import copy from pathlib import Path import subprocess import unittest @@ -82,5 +83,148 @@ def test_transport_failure_remains_an_error(self): self.evaluate(transport_error=True) +class ClearingLabelRecorderTests(unittest.TestCase): + def fixture(self): + return { + "owner": "example-approver", "sha": "head-a", "number": 7, + "payload": { + "label": {"name": "agent-approved"}, + "sender": {"login": "example-approver"}, + "pull_request": { + "head": {"sha": "head-a"}, "state": "open", "number": 7, + "updated_at": "2026-01-01T00:00:00Z", + }, + "private_value": "synthetic-secret-must-not-appear", + }, + "events": [{ + "id": 11, "event": "labeled", "label": {"name": "agent-approved"}, + "actor": {"login": "example-approver"}, + "created_at": "2026-01-01T00:00:00Z", + "private_value": "synthetic-secret-must-not-appear", + }], + } + + def evaluate(self, fixture): + workflow = (ROOT / ".github/workflows/risk-gate.yml").read_text() + recorder = workflow.split(" script: |\n", 1)[1].split("\n evaluate:", 1)[0] + script = """ +const fixture = JSON.parse(process.argv[1]); +process.env.APPROVAL_OWNER = fixture.owner; +const context = {payload: fixture.payload, sha: fixture.sha, + issue: {number: fixture.number}, repo: {owner: 'example-org', repo: 'example-repo'}}; +const result = {info: [], warnings: [], writes: [], reads: []}; +const core = {info: value => result.info.push(value), warning: value => result.warnings.push(value)}; +const github = { + paginate: async (_, args) => { + result.reads.push(args); + if (fixture.readError) throw new Error('Synthetic failure'); + return fixture.events; + }, + rest: {issues: {listEvents: 'listEvents'}, repos: {createCommitStatus: async args => { + if (fixture.writeError) throw new Error('Synthetic failure'); + result.writes.push(args); + if (fixture.writeThenError) throw new Error('Synthetic response loss'); + }}}, +}; +(async () => { + try { await (async () => { +""" + recorder + """ + })(); } catch (_) { result.failed = true; } + process.stdout.write(JSON.stringify(result)); +})(); +""" + result = subprocess.run( + ["node", "-e", script, json.dumps(fixture)], capture_output=True, + text=True, timeout=10, check=True, + ) + self.assertNotIn("synthetic-secret-must-not-appear", result.stdout) + return json.loads(result.stdout) + + def test_noop_guards_log_reason_and_warn_only_for_clearing_labels(self): + cases = ( + ("non_clearing_label", ("label", "name"), "risk:high"), + ("approver_mismatch", ("sender", "login"), "another-actor"), + ("missing_head", ("pull_request", "head", "sha"), None), + ("pr_not_open", ("pull_request", "state"), "closed"), + ("missing_pr_number", ("pull_request", "number"), None), + ("missing_updated_at", ("pull_request", "updated_at"), None), + ) + for reason, path, value in cases: + with self.subTest(reason=reason): + fixture = self.fixture() + target = fixture["payload"] + for key in path[:-1]: + target = target[key] + target[path[-1]] = value + result = self.evaluate(fixture) + self.assertEqual(result["info"], [f"clearing-label-recorder reason={reason}"]) + self.assertEqual(len(result["warnings"]), reason != "non_clearing_label") + self.assertEqual(result["writes"], []) + self.assertEqual(result["reads"], []) + fixture = self.fixture() + fixture["owner"] = "" + self.assertIn("approver_mismatch", self.evaluate(fixture)["warnings"][0]) + + def test_exact_event_binding_is_preserved(self): + for change in ("timestamp", "actor", "label", "event", "duplicate", "empty"): + with self.subTest(change=change): + fixture = self.fixture() + if change == "timestamp": + fixture["events"][0]["created_at"] = "2026-01-01T00:00:01Z" + elif change == "actor": + fixture["events"][0]["actor"]["login"] = "another-actor" + elif change == "label": + fixture["events"][0]["label"]["name"] = "owner-approved" + elif change == "event": + fixture["events"][0]["event"] = "unlabeled" + elif change == "duplicate": + fixture["events"].append(copy.deepcopy(fixture["events"][0])) + else: + fixture["events"] = [] + result = self.evaluate(fixture) + self.assertEqual(result["writes"], []) + self.assertIn("no_status_record reason=match_count_ne_1", result["warnings"][0]) + evidence = json.loads(result["info"][0].split("event_match=", 1)[1]) + self.assertEqual(evidence["match_count"], 2 if change == "duplicate" else 0) + self.assertEqual(set(evidence), { + "payload_updated_at", "match_count", "head_equal", "pr_number_equal", + }) + if change == "timestamp": + candidate = json.loads(result["info"][1].split("event_candidate=", 1)[1]) + self.assertEqual(candidate["issue_event_created_at"], "2026-01-01T00:00:01Z") + self.assertEqual(set(candidate), set(evidence) | {"issue_event_created_at"}) + + def test_success_still_binds_payload_head_pr_and_event_id(self): + fixture = self.fixture() + # A newer event must never replace the event matching this delivery. + newer = copy.deepcopy(fixture["events"][0]) + newer.update(id=12, created_at="2026-01-01T00:00:01Z") + fixture["events"].extend([dict(newer, event="unlabeled"), newer]) + fixture.update(sha="base-head", number=8) + result = self.evaluate(fixture) + self.assertEqual(result["warnings"], []) + self.assertEqual(result["writes"], [{ + "owner": "example-org", "repo": "example-repo", "sha": "head-a", + "context": "clearing-label", "state": "success", + "description": "agent-approved for PR 7 at event 11 on this head", + }]) + evidence = json.loads(result["info"][0].split("event_match=", 1)[1]) + self.assertFalse(evidence["head_equal"]) + self.assertFalse(evidence["pr_number_equal"]) + + def test_api_failures_warn_and_remain_errors(self): + for failure, reason in (("readError", "event_read_failed"), + ("writeError", "status_write_failed"), + ("writeThenError", "status_write_failed")): + with self.subTest(failure=failure): + fixture = self.fixture() + fixture[failure] = True + result = self.evaluate(fixture) + self.assertTrue(result["failed"]) + self.assertEqual(len(result["writes"]), failure == "writeThenError") + outcome = "no_status_record" if failure == "readError" else "status_write_unconfirmed" + self.assertIn(f"{outcome} reason={reason}", result["warnings"][0]) + + if __name__ == "__main__": unittest.main()