From 7306360b26331475af11ec4c3c1259b4c592d9ca Mon Sep 17 00:00:00 2001 From: Dong1017 <55723162+Dong1017@users.noreply.github.com> Date: Wed, 28 Oct 2026 16:59:49 +0900 Subject: [PATCH] fix: isolate PR admission writes and remove temporary capture Route PR and declaration-comment events to the trigger and affected head peers. Preserve full policy refresh, ownership checks and conservative fallback. Recognize the exact observed capacity response without claiming recovery; remove one-shot encrypted diagnostic instrumentation. Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com> Agent-assisted-by: OpenAI Codex --- .../diagnostics/issue203-capture-public.pem | 25 --- .github/workflows/knowledge-model-policy.yml | 16 -- docs/knowledge-model-policy.md | 52 ++--- tests/test_knowledge_model_policy.py | 34 ++- tests/test_knowledge_policy_http.py | 26 +++ tests/test_knowledge_policy_impact_routing.py | 66 ++++++ .../test_knowledge_policy_private_capture.py | 212 ------------------ tools/check_knowledge_model_policy.py | 148 ++++-------- 8 files changed, 183 insertions(+), 396 deletions(-) delete mode 100644 .github/diagnostics/issue203-capture-public.pem delete mode 100644 tests/test_knowledge_policy_private_capture.py diff --git a/.github/diagnostics/issue203-capture-public.pem b/.github/diagnostics/issue203-capture-public.pem deleted file mode 100644 index 09d541518..000000000 --- a/.github/diagnostics/issue203-capture-public.pem +++ /dev/null @@ -1,25 +0,0 @@ ------BEGIN CERTIFICATE----- -MIIEHTCCAoWgAwIBAgIUThz91QB45dG5J+qcjPo99amBVTEwDQYJKoZIhvcNAQEL -BQAwHjEcMBoGA1UEAwwTaXNzdWUyMDMtZGlhZ25vc3RpYzAeFw0yNjEwMjgwNzE5 -MDhaFw0yNjEwMzAwNzE5MDhaMB4xHDAaBgNVBAMME2lzc3VlMjAzLWRpYWdub3N0 -aWMwggGiMA0GCSqGSIb3DQEBAQUAA4IBjwAwggGKAoIBgQCcU91PJ/X2cNtGEONk -qv+6LB4ZjS0I7GqwcwpBB4ahKdkbvSx4a9TunBepoZlDCxxIqN+QJUk+P7oub9RC -MIcHbS1rhf/JTqpnilON2FpiAo4Ry1tVn+ri6HwGCgv9VTfPwFRGabijQG8mmv5F -vECBIopZyPDcIt6Tl7Katr7LNpQXPWbeVH39KhlAgPw48mhJS14/whgSlmsRBEcJ -Cfh+20GcsNOAEAzFiYuwcO5FN2z4z9I12Ak+qltl3DsT0PyNR/Kxb9JD08P097SM -YWcWkkhvr/T+Inaqb+PsXLps3Zgz3WDjBGP0ZuTUNGalUIEGwc2k57hW309kwyAN -XFI/MX/ulhg/JpY79FcXe9iUm48daEChlBTygQmrXe0JFYmsM628wY88I4qQ5qQ/ -wLq/TJhQ5/OqQXKZ2szsMTb7svOaKsGCUvyn9OWXY0mZAV3KKErpImF+TjqSvPcU -8zSpwUyz7GxnH+guL/6NM8RN5NHgR2Tkcwgx38BT4W2GXG0CAwEAAaNTMFEwHQYD -VR0OBBYEFOh1U0GpgaJd5/WEm1o6NMil7aLNMB8GA1UdIwQYMBaAFOh1U0GpgaJd -5/WEm1o6NMil7aLNMA8GA1UdEwEB/wQFMAMBAf8wDQYJKoZIhvcNAQELBQADggGB -AEH19kT5NHjq9Z5qEMPVuwWHs6XwGKslbEM+ElDJ129ykc6ZMOgNxG5jVG1FtRwD -tYKlvNB2qyAAqOMoP2FjIWObM/WyT7MIQ/PwN5tm+5FQ055mN0vlFmcK7OoS43V2 -Zc48mTId4stWscTGOiVoB1Z9cHWpSpRpLm6QYdcf2uqiUTEu4pUb+raaSLJOQ7NP -XKPvpcJauhWuvzn9wATN8YMXqePurL9XXoRz9dmQ+dhGFrKkHJJ6btZr2CYYgvmO -VeW/ZadSF8wYEtjX8XK2g5wTJX0pyvW1JDHaiZXzwvr+XxsFY3UrGtWOFJREF8IU -IDmG+Kc4h9Se+zVUhJ1hTC3gmfUeCqpAS8PI9pKzq8MIm7ImbxxmvRlmjgYtBhcS -ApZOqcoLiiwM3If3aFrsk9YHyft4TADmUudm9M2gRlwEbWULfKpc2IbOGZotnu7Z -G3NWFRLI657fNbQNmxi+VvkiiN4uRdeCWxr+tn88NN5GRkWYG+V8SGG1Gj56rGlI -2w== ------END CERTIFICATE----- diff --git a/.github/workflows/knowledge-model-policy.yml b/.github/workflows/knowledge-model-policy.yml index 068011a45..8c5ec515a 100644 --- a/.github/workflows/knowledge-model-policy.yml +++ b/.github/workflows/knowledge-model-policy.yml @@ -44,27 +44,11 @@ jobs: - uses: actions/checkout@v4 with: ref: main - # Temporary: prove the certificate's first introduction without diagnostic fetches. - fetch-depth: 0 persist-credentials: false - uses: actions/setup-python@v5 with: python-version: "3.11" - name: Evaluate declarations with trusted main code - id: admission env: GITHUB_TOKEN: ${{ github.token }} - # Temporary owner-approved capture: Python also enforces event/certificate/head gating. - ISSUE203_PRIVATE_CAPTURE: one-shot run: bash tools/run_knowledge_model_policy.sh - - - name: Retain only encrypted one-shot rejection diagnostic - if: >- - ${{ always() && steps.admission.outputs.private_capture == 'encrypted' && - github.event_name == 'push' && github.run_attempt == 1 }} - uses: actions/upload-artifact@v4 - with: - name: issue203-encrypted-rejection - path: ${{ runner.temp }}/issue203-422-errors.cms - retention-days: 1 - if-no-files-found: ignore diff --git a/docs/knowledge-model-policy.md b/docs/knowledge-model-policy.md index 5c591a07e..0aa25e083 100644 --- a/docs/knowledge-model-policy.md +++ b/docs/knowledge-model-policy.md @@ -326,8 +326,9 @@ The workflow handles PR open/reopen/close/commits/body-or-base edits on any targ branch (a PR against another base can share a main PR's head), approval-comment creation/edit/deletion, every main push and manual dispatch. All eligible triggers share one serialized status writer. Ordinary main pushes and closed-PR events use -the bounded impact routing below; policy changes, manual dispatch and other -eligible events retain the full open main-target PR refresh. It checks out `main`, +the bounded impact routing below; PR and approval-comment events select the +trigger and affected head peers. Policy changes, manual dispatch and unverified +routing evidence retain the full open main-target PR refresh. It checks out `main`, executes only trusted code, installs no PR dependencies and treats PR text/files as API data. Permissions are limited to contents/PR/issues read and commit-status write. Statuses explicitly target the @@ -378,7 +379,10 @@ terminal only for the current runner invocation: `status-rejected` remains an execution error, not declaration noncompliance or a successful invalidation. An explicit recognized status-capacity response is `budget-saturated`: the fixed full capacity message is recognized either directly in the `errors` string or in -a structured `Status`/`custom` array entry. An arbitrary string, substring match, +a structured `Status`/`custom` array entry. The exact string variant with the +`Validation failed: ` prefix is also recognized, based on the verified #203 sample; +this classification does not restore status capacity or replace an old remote status. +An arbitrary string, substring match, count of 900, or unknown 422 is not capacity proof. Other transient write failures retain the bounded outer retries. A fresh invocation may retry after remediation. The previous remote status may remain green or pending when a write is rejected; @@ -387,7 +391,7 @@ do not repair permissions, restore capacity, identify every unknown validation cause, or guarantee that historical red checks clear. -### Bounded impact routing for merge events +### Bounded impact routing for main and PR events An ordinary main push compares the event's `before` SHA to the **current fetched main SHA**, not merely the event's `after`. A changed trusted input forces a full @@ -405,10 +409,18 @@ base selects the PR; unchanged policy JSON alone is not enough, because another merge can change the protected paths bound by an exception approval. All open PRs, including other base branches, remain in the ownership scan. A shared -head is never skipped. A closed-PR event selects the closed trigger (reported N/A, -no new status) plus remaining open main-target peers on that same head. Other PR -and approval-comment events still perform full refreshes. This is deliberately not -a general event router or persistent admission cache. +head is never skipped by push-impact analysis. A closed-PR event selects the closed +trigger (reported N/A, no new status) plus remaining open main-target peers on that +same head. Other PR events select the trigger and open main-target peers sharing +its event head, current head, or (for synchronize) previous head. Missing previous +head evidence falls back to a full refresh. Approval-comment create/edit/delete +events select the trigger and current-head peers; edits and revocations remain +eligible. Retargeting outside main still refreshes main-target peers. The evaluator +continues checking ownership against all open PRs, including other base branches. +Unrelated PRs receive no history scan or status POST. This reduces duplicate writes; +it is not a persistent cache or deduplication of repeated events on the same PR. +Every selected PR still invalidates pending before assessment, and known saturated +heads remain blocked until a supported recovery changes the underlying condition. Missing objects, shallow/incomplete history, force pushes, malformed collections, repository/branch mismatches, fetch timeouts and unverified impact fall back to a @@ -473,27 +485,3 @@ python tools/check_knowledge_model_policy.py --repository --pull-re publishing statuses. It does not show the main workflow is operational. Production uses runtime-injected `GITHUB_TOKEN`; never pass tokens as CLI arguments. The existing ready lane runs unit tests on Python 3.10 and 3.11 in its full suite. - - - -### Temporary owner-authorized encrypted diagnostic (#203) - -The one-shot opt-in captures only the selected pending-status POST's HTTP 422 -`errors` string, from the response already being processed. It adds no request. -Activation requires the first run attempt of the main push that introduces the -exact owner-held public certificate blob, repository and exact head. Both commit -objects must be readable; the certificate must be absent in `before`, match the -pinned blob in `after`, and match the checked-out certificate. Complete ancestry -must prove `before` is an ancestor of `after` and that this certificate path never -existed before; removing and readding it cannot re-arm capture. This uses existing -Git objects only, with a ten-second aggregate bound; temporary full-depth trusted -main checkout supplies that ancestry without depending on a PR-head fetch. Missing/shallow objects or -force pushes disable collection. Other events and later pushes are disabled. An exclusive marker -prevents a second capture even if encryption fails. The string is passed in memory -to OpenSSL CMS using a temporary public certificate and a credential-free child -environment. Only ciphertext is written and uploaded, after a successful-capture -step output, with one-day artifact retention. The decryption key stays outside -GitHub. Neither plaintext, full response bodies, headers nor tokens are recorded. -Capture failure uses a fixed diagnostic and never changes the admission verdict. -Remove the opt-in, capture hook and public certificate after the authorized sample -is collected; this diagnostic is not a permanent telemetry channel or a 422 fix. diff --git a/tests/test_knowledge_model_policy.py b/tests/test_knowledge_model_policy.py index 64a6dffd8..ff7736640 100644 --- a/tests/test_knowledge_model_policy.py +++ b/tests/test_knowledge_model_policy.py @@ -347,13 +347,9 @@ def test_workflow_never_executes_pr_code_and_refreshes_metadata_and_base(): assert set(job["concurrency"]) == {"group", "cancel-in-progress", "queue"} assert job["concurrency"]["queue"] == "max" steps = value["jobs"]["declarations"]["steps"] - assert steps[0]["with"] == {"ref": "main", "fetch-depth": 0, "persist-credentials": False} + assert steps[0]["with"] == {"ref": "main", "persist-credentials": False} assert all("pip install" not in step.get("run", "") for step in steps) - assert steps[-2]["run"] == "bash tools/run_knowledge_model_policy.sh" - assert steps[-1]["uses"] == "actions/upload-artifact@v4" - assert steps[-1]["with"]["path"] == "${{ runner.temp }}/issue203-422-errors.cms" - assert "steps.admission.outputs.private_capture == 'encrypted'" in steps[-1]["if"] - assert steps[-1]["with"]["retention-days"] == 1 + assert steps[-1]["run"] == "bash tools/run_knowledge_model_policy.sh" runner = (ROOT / "tools/run_knowledge_model_policy.sh").read_text(encoding="utf-8") # The runner refetches trusted main and reruns only for the stale-base exit code. assert "git fetch --quiet origin main" in runner @@ -1627,3 +1623,29 @@ def request(route, payload=None, **kwargs): assert len(posts) == 2 # A changed head invalidates the carried terminal error. check_prs(api, policy, BASE, False, [123], plan={"completed": {}}) assert len(posts) == 3 # A new invocation can reassess after external remediation. + + +@pytest.mark.parametrize("event_name", ["pull_request_target", "issue_comment"]) +def test_targeted_event_does_not_read_history_or_publish_to_unrelated_pr( + api_fixture, policy, event_name): + from tools.check_knowledge_model_policy import admission_route + + api, state = api_fixture + other = copy.deepcopy(pr()) + other.update(number=124, head={"sha": "c" * 40}) + state["prs"][124] = other + event = {"repository": {"full_name": "example/repository"}, "action": "edited"} + if event_name == "pull_request_target": + event.update(number=123, pull_request=pr()) + else: + event["issue"] = {"number": 123, "pull_request": {}} + numbers, routing, _ = admission_route( + api, event, event_name, policy, BASE, "example/repository") + assert numbers == [123] + assert routing["mode"] == "pr-impact" + results = check_prs(api, policy, BASE, False, numbers) + assert results[0]["kind"] == "compliant" + assert [item["state"] for item in state["statuses"]] == ["pending", "success"] + assert all(item["sha"] == HEAD for item in state["statuses"]) + assert ("GET", "commits/" + "c" * 40 + "/statuses") not in state["requests"] + assert ("GET", "pulls/124/files") not in state["requests"] diff --git a/tests/test_knowledge_policy_http.py b/tests/test_knowledge_policy_http.py index 8b6ec0a8a..58e75d148 100644 --- a/tests/test_knowledge_policy_http.py +++ b/tests/test_knowledge_policy_http.py @@ -63,6 +63,32 @@ def test_exact_string_capacity_is_recognized_but_remains_execution_failure(self) self.assertEqual(policy.summarize([result])["execution"], "failed") self.assertEqual(policy.summarize([result])["noncompliant_pull_requests"], []) + def test_verified_prefixed_capacity_string_is_terminal_not_compliance(self): + message = ("Validation failed: This SHA and context has reached the maximum " + "number of statuses.") + body = {"message": "Validation Failed", "errors": message} + failure = policy.GitHubHTTPError(HTTPError("https://example.invalid", 422, "", {}, + io.BytesIO(json.dumps(body).encode()))) + self.assertEqual(failure.reason, "status-capacity") + self.assertNotIn(message, str(failure)) + result = policy.status_write_failure(failure, {"description": "fixture"}) + result.update(pull_request=1, rerun=result["kind"] in policy.RERUN_KINDS) + self.assertEqual(result["kind"], "budget-saturated") + self.assertEqual(result["state"], "failure") + self.assertFalse(result["rerun"]) + self.assertEqual(policy.exit_code([result]), policy.EXIT_NONCOMPLIANT) + self.assertEqual(policy.summarize([result])["noncompliant_pull_requests"], []) + for changed in (message + " FAKE_SECRET", "FAKE_SECRET " + message, + message.replace("Validation failed:", "Validation error:"), + message[:-1]): + with self.subTest(changed=changed): + body["errors"] = changed + rejected = policy.GitHubHTTPError(HTTPError("https://example.invalid", 422, "", {}, + io.BytesIO(json.dumps(body).encode()))) + self.assertEqual(rejected.reason, "unknown") + self.assertNotIn("FAKE_SECRET", str(rejected)) + self.assertNotIn("This SHA", str(rejected)) + def test_capacity_string_near_misses_remain_unknown_and_redacted(self): capacity = "This SHA and context has reached the maximum number of statuses." for errors in ("FAKE_SECRET", capacity + " FAKE_SECRET", "FAKE_SECRET " + capacity, diff --git a/tests/test_knowledge_policy_impact_routing.py b/tests/test_knowledge_policy_impact_routing.py index d43bc5dc8..b017f36d0 100644 --- a/tests/test_knowledge_policy_impact_routing.py +++ b/tests/test_knowledge_policy_impact_routing.py @@ -220,3 +220,69 @@ def test_graph_prepare_rejects_still_shallow_and_never_uses_candidate_remote(mon assert topology.git.call_args_list[1].args == ( "fetch", "--quiet", "--no-tags", "--unshallow", "https://github.com/example/repository.git", BEFORE, CURRENT, HEAD) + + +def lifecycle_client(prs, live): + client = api(prs) + client.get.side_effect = lambda path: ({"object": {"sha": CURRENT}} + if path.startswith("git/ref/") else live) + return client + + +def pr_event(action, head=HEAD): + return {"repository": {"full_name": REPOSITORY}, "number": 1, "action": action, + "pull_request": pr(head=head)} + + +def test_synchronize_selects_old_event_and_live_head_peers_only(): + old, new, latest = "d" * 40, "e" * 40, "f" * 40 + prs = [pr(1, latest), pr(2, old), pr(3, new), pr(4, latest), pr(5, HEAD)] + client = lifecycle_client(prs, prs[0]) + payload = {**pr_event("synchronize", new), "before": old} + numbers, routing, _ = route(client, payload, name="pull_request_target") + assert numbers == [1, 2, 3, 4] + assert routing["mode"] == "pr-impact" + client.history.assert_not_called() + client.status.assert_not_called() + + +@pytest.mark.parametrize("action", ["opened", "edited", "reopened", "ready_for_review"]) +def test_body_or_lifecycle_edit_routes_trigger_and_shared_other_base_ownership(action): + prs = [pr(), pr(2, base="release"), pr(3, head="d" * 40)] + client = lifecycle_client(prs, prs[0]) + assert route(client, pr_event(action), name="pull_request_target")[0] == [1] + # The unrelated-base peer stays in the ownership collection used by the actual evaluator. + assert policy.head_owner(prs, 1, HEAD) is not None + + +def test_retargeting_trigger_outside_main_still_refreshes_main_head_peer(): + prs = [pr(base="release"), pr(2), pr(3, head="d" * 40)] + client = lifecycle_client(prs, prs[0]) + assert route(client, pr_event("edited"), name="pull_request_target")[0] == [1, 2] + + +@pytest.mark.parametrize("action", ["created", "edited", "deleted"]) +def test_approval_comments_refresh_trigger_and_current_head_peer(action): + prs = [pr(), pr(2), pr(3, head="d" * 40)] + client = lifecycle_client(prs, prs[0]) + payload = {"repository": {"full_name": REPOSITORY}, "action": action, + "issue": {"number": 1, "pull_request": {}}} + assert route(client, payload, name="issue_comment")[0] == [1, 2] + + +@pytest.mark.parametrize("before", [None, "bad", "0" * 40]) +def test_synchronize_missing_or_invalid_old_head_keeps_conservative_full_sweep(before): + client = lifecycle_client([pr()], pr()) + payload = pr_event("synchronize") + if before is not None: + payload["before"] = before + assert route(client, payload, name="pull_request_target")[0] is None + + +def test_pr_event_identity_mismatch_and_head_race_fall_back(): + client = lifecycle_client([pr()], pr()) + payload = pr_event("edited") + payload["pull_request"]["number"] = 2 + assert route(client, payload, name="pull_request_target")[0] is None + client.pages.side_effect = [[pr()], [pr(head="d" * 40)]] + assert route(client, pr_event("edited"), name="pull_request_target")[0] is None diff --git a/tests/test_knowledge_policy_private_capture.py b/tests/test_knowledge_policy_private_capture.py deleted file mode 100644 index 12ff85f99..000000000 --- a/tests/test_knowledge_policy_private_capture.py +++ /dev/null @@ -1,212 +0,0 @@ -"""One-shot owner-approved capture: fixture transport and encryption, no live writes.""" - -import io -import json -import subprocess -from unittest.mock import Mock -from urllib.error import HTTPError - -import pytest - -from tools import check_knowledge_model_policy as policy - - -@pytest.fixture -def capture(tmp_path, monkeypatch): - event = tmp_path / "event.json" - event.write_text(json.dumps({"before": "a" * 40, "after": "b" * 40, - "ref": "refs/heads/main", "repository": {"full_name": "InferMatrixLab/InferRSI"}})) - root = tmp_path / "checkout" - cert = root / ".github/diagnostics/issue203-capture-public.pem" - cert.parent.mkdir(parents=True) - cert.write_text("synthetic public certificate") - output = tmp_path / "output" - output.mkdir() - for key, value in {"ISSUE203_PRIVATE_CAPTURE": "one-shot", "GITHUB_EVENT_NAME": "push", - "GITHUB_RUN_ATTEMPT": "1", "GITHUB_REPOSITORY": "InferMatrixLab/InferRSI", - "GITHUB_EVENT_PATH": str(event), "RUNNER_TEMP": str(output), - "GITHUB_OUTPUT": str(tmp_path / "step-output"), - "GITHUB_TOKEN": "FAKE_TOKEN", "EXTRA_SECRET": "FAKE_SECRET"}.items(): - monkeypatch.setenv(key, value) - monkeypatch.setattr(policy, "ROOT", root) - monkeypatch.setattr(policy, "capture_certificate_introduced", lambda event: True) - encrypt = Mock(return_value=Mock(stdout=b"synthetic ciphertext")) - monkeypatch.setattr(policy.subprocess, "run", encrypt) - payload = {"context": policy.CONTEXT, "state": "pending"} - return output, event, encrypt, f"statuses/{policy.PRIVATE_CAPTURE_HEAD}", payload - - -def test_capture_is_once_ciphertext_only_and_child_has_no_credentials(capture, capsys): - output, _, encrypt, route, payload = capture - for _ in range(2): - policy.capture_private_rejection("FAKE_PRIVATE_RESPONSE", route, payload) - encrypt.assert_called_once() - assert encrypt.call_args.kwargs["input"] == b"FAKE_PRIVATE_RESPONSE" - assert encrypt.call_args.kwargs["env"] == { - "PATH": "/usr/bin:/bin", "LANG": "C", "OPENSSL_CONF": "/dev/null"} - assert encrypt.call_args.kwargs["stderr"] is subprocess.DEVNULL - assert encrypt.call_args.kwargs["timeout"] == 10 - assert (output / policy.PRIVATE_CAPTURE_FILE).read_bytes() == b"synthetic ciphertext" - assert (output / "issue203-capture-attempted").read_bytes() == b"" - assert (output / policy.PRIVATE_CAPTURE_FILE).stat().st_mode & 0o777 == 0o600 - assert (output.parent / "step-output").read_text() == "private_capture=encrypted\n" - assert "FAKE_PRIVATE_RESPONSE" not in capsys.readouterr().out - - -@pytest.mark.parametrize("key,value", [ - ("ISSUE203_PRIVATE_CAPTURE", ""), ("GITHUB_EVENT_NAME", "pull_request_target"), - ("GITHUB_EVENT_NAME", "workflow_dispatch"), ("GITHUB_EVENT_NAME", "issue_comment"), - ("GITHUB_RUN_ATTEMPT", "2"), ("GITHUB_REPOSITORY", "other/repository"), -]) -def test_other_runs_default_off(capture, monkeypatch, key, value): - output, _, encrypt, route, payload = capture - monkeypatch.setenv(key, value) - policy.capture_private_rejection("FAKE_PRIVATE_RESPONSE", route, payload) - encrypt.assert_not_called() - assert not list(output.iterdir()) - - -@pytest.mark.parametrize("mutation", [ - {"ref": "refs/heads/other"}, - {"repository": {"full_name": "other/repository"}}, -]) -def test_unrelated_pushes_do_not_capture(capture, mutation): - _, event, encrypt, route, payload = capture - event.write_text(json.dumps({**json.loads(event.read_text()), **mutation})) - policy.capture_private_rejection("FAKE_PRIVATE_RESPONSE", route, payload) - encrypt.assert_not_called() - - -@pytest.mark.parametrize("route,payload", [ - ("statuses/" + "b" * 40, {"context": policy.CONTEXT, "state": "pending"}), - ("pulls/51", None), - (f"statuses/{policy.PRIVATE_CAPTURE_HEAD}", {"context": "other", "state": "pending"}), - (f"statuses/{policy.PRIVATE_CAPTURE_HEAD}", {"context": policy.CONTEXT, "state": "success"}), -]) -def test_other_requests_do_not_capture(capture, route, payload): - _, _, encrypt, _, _ = capture - policy.capture_private_rejection("FAKE_PRIVATE_RESPONSE", route, payload) - encrypt.assert_not_called() - - -def test_encryption_failure_is_fixed_diagnostic_and_not_retried(capture, capsys): - output, _, encrypt, route, payload = capture - encrypt.side_effect = subprocess.CalledProcessError(1, "FAKE_SECRET", stderr=b"FAKE_SECRET") - for _ in range(2): - policy.capture_private_rejection("FAKE_PRIVATE_RESPONSE", route, payload) - encrypt.assert_called_once() - assert not (output / policy.PRIVATE_CAPTURE_FILE).exists() - assert capsys.readouterr().out == '{"private_capture":"unavailable"}\n' - - -def test_destination_symlink_is_not_followed(capture, capsys): - output, _, _, route, payload = capture - victim = output / "untouched" - victim.write_bytes(b"untouched") - (output / policy.PRIVATE_CAPTURE_FILE).symlink_to(victim) - policy.capture_private_rejection("FAKE_PRIVATE_RESPONSE", route, payload) - assert victim.read_bytes() == b"untouched" - assert not (output.parent / "step-output").exists() - assert "FAKE_PRIVATE_RESPONSE" not in capsys.readouterr().out - - -@pytest.mark.parametrize("code,errors,expected", [(422, "FAKE_PRIVATE_RESPONSE", 1), - (422, [{"message": "FAKE_PRIVATE_RESPONSE"}], 0), (403, "FAKE_PRIVATE_RESPONSE", 0)]) -def test_only_errors_string_from_existing_422_is_offered(code, errors, expected): - callback = Mock() - response = HTTPError("https://example.invalid", code, "", {}, io.BytesIO(json.dumps({ - "message": "FAKE_OTHER_BODY", "errors": errors}).encode())) - failure = policy.GitHubHTTPError(response, capture=callback) - assert callback.call_count == expected - if expected: - callback.assert_called_once_with("FAKE_PRIVATE_RESPONSE") - result = policy.status_write_failure(failure, {"description": "fixture"}) - assert result["kind"] == "status-rejected" - assert result["state"] == "failure" - assert "FAKE_" not in str(failure) - - -def test_certificate_introduction_requires_known_commit_trees_and_exact_blob(monkeypatch): - certificate = ".github/diagnostics/issue203-capture-public.pem" - expected = f"100644 blob {policy.PRIVATE_CAPTURE_CERT_BLOB}\t{certificate}" - responses = ["false", "", "", "", "", "", expected, policy.PRIVATE_CAPTURE_CERT_BLOB] - transport = Mock(side_effect=[Mock(stdout=value) for value in responses]) - monkeypatch.setattr(policy.subprocess, "run", transport) - assert policy.capture_certificate_introduced({"before": "a" * 40, "after": "b" * 40}) - assert transport.call_count == 8 - for call in transport.call_args_list: - assert "GITHUB_TOKEN" not in call.kwargs["env"] - assert call.kwargs["stderr"] is subprocess.DEVNULL - assert call.kwargs["timeout"] <= 10 - - -@pytest.mark.parametrize("before_entry,after_entry", [ - ("existing", "unused"), ("", ""), ("", "120000 blob wrong symlink"), - ("", "100644 blob wrong certificate"), -]) -def test_later_push_absent_after_or_changed_certificate_never_activates( - monkeypatch, before_entry, after_entry): - transport = Mock(side_effect=[Mock(stdout=value) for value in - ["false", "", "", "", before_entry, "", after_entry]]) - monkeypatch.setattr(policy.subprocess, "run", transport) - assert not policy.capture_certificate_introduced({"before": "a" * 40, "after": "b" * 40}) - - -def test_missing_commit_is_not_treated_as_certificate_absence(monkeypatch): - transport = Mock(side_effect=[Mock(stdout="false"), subprocess.CalledProcessError(1, "git")]) - monkeypatch.setattr(policy.subprocess, "run", transport) - with pytest.raises(subprocess.CalledProcessError): - policy.capture_certificate_introduced({"before": "a" * 40, "after": "b" * 40}) - - -@pytest.mark.parametrize("event", [{"before": "0" * 40, "after": "b" * 40}, - {"before": "a" * 40, "after": "b" * 40, "forced": True}, - {"before": "--malicious", "after": "b" * 40}, {}]) -def test_invalid_or_forced_introduction_does_no_git_work(monkeypatch, event): - transport = Mock() - monkeypatch.setattr(policy.subprocess, "run", transport) - assert not policy.capture_certificate_introduced(event) - transport.assert_not_called() - - -def test_removed_and_readded_certificate_never_rearms(monkeypatch): - transport = Mock(side_effect=[Mock(stdout=value) for value in - ["false", "", "", "", "", "c" * 40]]) - monkeypatch.setattr(policy.subprocess, "run", transport) - assert not policy.capture_certificate_introduced({"before": "a" * 40, "after": "b" * 40}) - assert transport.call_args.args[0][-6:] == [ - "-1", "--full-history", "--format=%H", "a" * 40, "--", - ".github/diagnostics/issue203-capture-public.pem"] - - -def test_shallow_graph_never_captures_or_fetches(monkeypatch): - transport = Mock(return_value=Mock(stdout="true")) - monkeypatch.setattr(policy.subprocess, "run", transport) - assert not policy.capture_certificate_introduced({"before": "a" * 40, "after": "b" * 40}) - assert transport.call_count == 1 - - -def test_real_git_graph_accepts_first_certificate_introduction_only(tmp_path, monkeypatch): - certificate = ".github/diagnostics/issue203-capture-public.pem" - public = (policy.ROOT / certificate).read_bytes() - - def git(*args, stdin=None): - return subprocess.run(["git", *args], cwd=tmp_path, input=stdin, text=True, - capture_output=True, check=True).stdout.strip() - - git("init", "-q") - git("config", "user.name", "Fixture") - git("config", "user.email", "fixture@example.invalid") - empty = git("mktree", stdin="") - before = git("commit-tree", empty, stdin="before") - destination = tmp_path / certificate - destination.parent.mkdir(parents=True) - destination.write_bytes(public) - git("add", certificate) - tree = git("write-tree") - after = git("commit-tree", tree, "-p", before, stdin="first introduction") - monkeypatch.setattr(policy, "ROOT", tmp_path) - assert policy.capture_certificate_introduced({"before": before, "after": after}) - removed = git("commit-tree", empty, "-p", after, stdin="removed") - readded = git("commit-tree", tree, "-p", removed, stdin="readded") - assert not policy.capture_certificate_introduced({"before": removed, "after": readded}) diff --git a/tools/check_knowledge_model_policy.py b/tools/check_knowledge_model_policy.py index c825665ee..1e64998a4 100644 --- a/tools/check_knowledge_model_policy.py +++ b/tools/check_knowledge_model_policy.py @@ -304,106 +304,10 @@ def json_type(value): return "number" -# Owner-authorized, one-shot diagnostic. Remove after the encrypted artifact is collected. -PRIVATE_CAPTURE_CERT_BLOB = "09d541518a02bed5f6065fc3a48dc752f839675d" -PRIVATE_CAPTURE_HEAD = "3c1b2ff969537f3216d0125d034ae903d0743e7a" -PRIVATE_CAPTURE_FILE = "issue203-422-errors.cms" - - -def capture_certificate_introduced(event): - """Only the push introducing this exact owner-held certificate can capture.""" - before, after = event.get("before"), event.get("after") - if (not isinstance(before, str) or not isinstance(after, str) - or not SHA.fullmatch(before) or not SHA.fullmatch(after) - or before == "0" * 40 or after == "0" * 40 or event.get("forced", False)): - return False - deadline = time.monotonic() + 10 - environment = {"PATH": "/usr/bin:/bin", "LANG": "C", "GIT_CONFIG_NOSYSTEM": "1", - "GIT_CONFIG_GLOBAL": "/dev/null", "GIT_TERMINAL_PROMPT": "0"} - - def git(*arguments): - remaining = deadline - time.monotonic() - if remaining <= 0: - raise ValueError("Capture event proof deadline exhausted") - return subprocess.run( - ["/usr/bin/git", "--no-replace-objects", "-c", "credential.helper=", - "-c", "core.hooksPath=/dev/null", *arguments], cwd=ROOT, env=environment, - stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, check=True, - timeout=remaining, text=True).stdout.strip() - - # Use only existing complete Git evidence, normally prepared by push routing. - # No extra network operation: missing/shallow objects disable collection. - if git("rev-parse", "--is-shallow-repository") != "false": - return False - for revision in (before, after): - git("cat-file", "-e", revision + "^{commit}") - git("merge-base", "--is-ancestor", before, after) - certificate = ".github/diagnostics/issue203-capture-public.pem" - if (git("ls-tree", before, "--", certificate) - or git("log", "-1", "--full-history", "--format=%H", before, "--", certificate)): - # Removal followed by re-addition must not re-arm this diagnostic. - return False - expected = f"100644 blob {PRIVATE_CAPTURE_CERT_BLOB}\t{certificate}" - return (git("ls-tree", after, "--", certificate) == expected - and git("hash-object", "--no-filters", certificate) == PRIVATE_CAPTURE_CERT_BLOB) - - -def capture_private_rejection(value, route, payload): - """Encrypt only one selected response's errors string; never persist plaintext.""" - if (os.environ.get("ISSUE203_PRIVATE_CAPTURE") != "one-shot" - or os.environ.get("GITHUB_EVENT_NAME") != "push" - or os.environ.get("GITHUB_RUN_ATTEMPT") != "1" - or os.environ.get("GITHUB_REPOSITORY") != "InferMatrixLab/InferRSI" - or route != f"statuses/{PRIVATE_CAPTURE_HEAD}" - or not isinstance(payload, dict) or payload.get("context") != CONTEXT - or payload.get("state") != "pending" or not isinstance(value, str)): - return - try: - event = json.loads(Path(os.environ["GITHUB_EVENT_PATH"]).read_text(encoding="utf-8")) - if (event.get("ref") != "refs/heads/main" - or event.get("repository", {}).get("full_name") != "InferMatrixLab/InferRSI" - or not capture_certificate_introduced(event)): - return - directory = Path(os.environ["RUNNER_TEMP"]) - if not directory.is_absolute() or not directory.is_dir() or directory.is_symlink(): - raise ValueError("Unsafe diagnostic destination") - # A zero-byte exclusive marker prevents another process/retry in this run from - # collecting again, including when encryption fails. It is never uploaded. - flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW - try: - marker = os.open(directory / "issue203-capture-attempted", flags, 0o600) - except FileExistsError: - return - os.close(marker) - certificate = ROOT / ".github/diagnostics/issue203-capture-public.pem" - if certificate.is_symlink() or not certificate.is_file(): - raise ValueError("Unavailable diagnostic public certificate") - encrypted = subprocess.run( - ["/usr/bin/openssl", "cms", "-encrypt", "-binary", "-aes-256-cbc", - "-outform", "DER", str(certificate)], input=value.encode("utf-8"), - stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, check=True, timeout=10, - env={"PATH": "/usr/bin:/bin", "LANG": "C", "OPENSSL_CONF": "/dev/null"}).stdout - if not encrypted: - raise ValueError("Empty encrypted diagnostic") - # Encryption completes in memory before any content file is opened. Refuse - # preexisting files/symlinks; no plaintext, headers, token or full body is stored. - destination = os.open(directory / PRIVATE_CAPTURE_FILE, flags, 0o600) - with os.fdopen(destination, "wb") as stream: - stream.write(encrypted) - step_output = os.environ.get("GITHUB_OUTPUT") - if step_output: - with Path(step_output).open("a", encoding="utf-8") as stream: - stream.write("private_capture=encrypted\n") - print('{"private_capture":"encrypted"}') - except (OSError, ValueError, TypeError, KeyError, AttributeError, subprocess.SubprocessError): - # Never echo exception text: transport/encryption failures may contain inputs. - print('{"private_capture":"unavailable"}') - - class GitHubHTTPError(OSError): """An allowlisted diagnostic, never a raw response body or credential-bearing URL.""" - def __init__(self, error, capture=None): + def __init__(self, error): self.code = error.code headers = error.headers or {} self.delay = None @@ -447,12 +351,13 @@ def __init__(self, error, capture=None): # HTTP 422, an arbitrary validation message, or the locally counted history. capacity_message = "this sha and context has reached the maximum number of statuses." errors = body.get("errors", []) if isinstance(body, dict) else [] - if capture is not None and isinstance(errors, str): - capture(errors) # GitHub may return the same capacity diagnostic directly as `errors` # rather than in a Status/custom array. Match only this fixed full text. + # The verified service response can include this exact validation prefix. + # Do not accept arbitrary prefixes/suffixes or substring matches. + capacity_strings = {capacity_message, "validation failed: " + capacity_message} capacity = (isinstance(errors, str) and - errors.strip().lower() == capacity_message) or ( + errors.strip().lower() in capacity_strings) or ( isinstance(errors, list) and any( isinstance(item, dict) and item.get("resource") == "Status" and item.get("code") == "custom" @@ -529,8 +434,7 @@ def request(self, route, payload=None, timeout=30): with urlopen(request, timeout=max(0.001, deadline - time.monotonic())) as response: return json.load(response), response.headers.get("Link", "") except HTTPError as error: - failure = GitHubHTTPError( - error, capture=lambda value: capture_private_rejection(value, route, payload)) + failure = GitHubHTTPError(error) # Retry reads only, once, with explicit rate timing and within the existing # request budget. Never replay a status POST or guess a delay from HTTP 403. if (attempt == 0 and payload is None and failure.category == "rate-limit" @@ -1194,14 +1098,48 @@ def admission_route(api, event, event_name, policy, policy_sha, repository, grap """Select only proven affected targets; ambiguous evidence keeps the full sweep.""" full = {"mode": "full-sweep", "reason": "event-requires-refresh"} is_close = event_name == "pull_request_target" and event.get("action") == "closed" - if event_name != "push" and not is_close: + is_pr = event_name == "pull_request_target" and not is_close + is_comment = event_name == "issue_comment" + if event_name != "push" and not (is_close or is_pr or is_comment): return None, full, None try: if policy["base_branch"] != "main" or event["repository"]["full_name"] != repository: raise ValueError("Routing repository or branch mismatch") prs = api.pages("pulls?state=open") snapshot = routing_snapshot(prs) - if is_close: + if is_pr or is_comment: + if is_pr: + if event["action"] not in {"opened", "synchronize", "reopened", "edited", + "ready_for_review"}: + raise ValueError("Unverified PR action") + trigger = event["number"] + captured = event["pull_request"] + if captured["number"] != trigger: + raise ValueError("Inconsistent PR event identity") + heads = {captured["head"]["sha"]} + if event["action"] == "synchronize": + # The old head can still belong to another open PR. Missing old-head + # evidence cannot justify dropping that peer's ownership refresh. + heads.add(event["before"]) + else: + if (event["action"] not in {"created", "edited", "deleted"} + or "pull_request" not in event["issue"]): + raise ValueError("Unverified PR comment event") + trigger = event["issue"]["number"] + heads = set() + if type(trigger) is not int or trigger <= 0: + raise ValueError("Invalid trigger PR number") + live = api.get(f"pulls/{trigger}") + if live["number"] != trigger or live["state"] not in {"open", "closed"}: + raise ValueError("Unverified current PR identity") + heads.add(live["head"]["sha"]) + if any(not isinstance(head, str) or not SHA.fullmatch(head) + or head == "0" * 40 for head in heads): + raise ValueError("Unverified affected head") + numbers = sorted({trigger, *(pr["number"] for pr in prs + if pr["head"]["sha"] in heads and addressable(pr, policy))}) + route = {"mode": "pr-impact", "reason": "trigger-and-head-peers", "targets": numbers} + elif is_close: trigger = event["number"] live = api.get(f"pulls/{trigger}") captured = event["pull_request"] @@ -1323,7 +1261,7 @@ def report_exit(path, binding, sweep_exit): and not result["errors"] and SHA.fullmatch(result["head_sha"])): conclusion = 0 routing = report.get("routing", {"mode": "full-sweep"}) - if routing.get("mode") not in {"full-sweep", "push-impact", "closed-impact"}: + if routing.get("mode") not in {"full-sweep", "push-impact", "closed-impact", "pr-impact"}: raise ValueError("Invalid routing scope") impact = routing["mode"] != "full-sweep" summary = {"scope": ("trigger-pr" if trigger is not None else