diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md new file mode 100644 index 000000000..0465a1668 --- /dev/null +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md @@ -0,0 +1,169 @@ +# WS-GHPROOF-001-03 — Harnessy proof checker compares proofs with GitHub facts + +- Initiative: WS-GHPROOF-001 +- Durable disposition: Complete +- Intended merge outcome: The networkless, post-submission `harnessy.work_proof` checker passes a proof only when it matches WorkStream-supplied `workstream.github_facts.v1` facts and the integrity rules of owner decision 20, failing closed on missing configuration or facts, with one finding per mismatch and a v2 registry body that validates its configuration. + +## Intent + +A hand-written `proof.json` passes today. Once WorkStream supplies GitHub facts +(chunk 04), this checker makes a faked or unreviewed proof fail, while the +checker stays networkless and credential-free. Verification integrity is +non-negotiable (owner decision 20, 24 October 2026): fail closed and trust +nothing the contributor could produce. + +## Current behavior + +- `external_checkers/harnessy_proof/checker.py` checks only proof shape and + ignores the request `configuration` and `input`. +- `external_checkers/harnessy_proof/publish_local.py` publishes PRE and POST + bodies with `{"type": "object"}` configuration and input schemas, default + version `v1`. +- `docs/engineering/local-agent-loop.md` documents shape checking only. + +## Bounded change + +### Allowed + +- `external_checkers/harnessy_proof/checker.py`, `publish_local.py` (standard + library only). +- `backend/tests/checkers/external/test_harnessy_proof_checker.py` (already in + the lane catalogue). +- `docs/engineering/local-agent-loop.md` publishing, registration and checker + sections; the two agent-loop mentions in `docs/roadmap_status.md`. + +### Not allowed + +- Backend runtime, POST wiring, the GitHub adapter (chunk 02), checker network + access, the checker service, ART custody, or Harnessy's proof format. + +## Design and decisions + +- **One fail-closed mode (security review, fail-open finding).** The binding + configuration must name `github_repository` and `base_branch`; a missing, + empty or invalid configuration is `github.configuration`. Facts must be + present (`github.facts_missing`) and have the exact v1 shape + (`github.facts_invalid`). There is no shape-only pass. The local agent loop + does not need one: until chunk 04 supplies facts every evaluation fails with + `facts_missing`, which the runbook states. Workstream keeps no compatibility + mode. +- **Post-submission only.** PRE checking never has facts, so `publish_local.py` + writes only `registry-post.json`; the runbook registers POST only. +- **Input shape.** WorkStream places chunk 02's + `GitHubPullRequestFacts.model_dump(mode="json")` in the request `input` under + `github_facts`, so the request digest binds it. The fields are + `schema_version`, `repository`, `pull_request_number`, `html_url`, `state`, + `merged`, `merge_commit_sha` (null unless merged), `base_ref`, `head_sha`, + `author`, `reviews`, `changed_files_count`, `changed_files`, + `required_checks` (`name`, `app_id`, `app_bound`, `conclusion`, + `check_run_app_id`, `check_run_head_sha`, `check_run_html_url`, + `check_run_details_url`, `workflow_run_id`, `commit_status_judged`, + `commit_status_sha`), `fetched_at` and `responses`. Required-check entries + must have exactly these keys. +- **One canonical identity (security review, parser differential).** The + checker parses no URL. `pr_url` must equal + `https://github.com/{facts.repository}/pull/{facts.pull_request_number}` + exactly, and GitHub's `html_url` must equal the same string; otherwise + `github.pr_url`. Comparison is case-sensitive against the repository name + GitHub returns, so a trailing slash, query, fragment, port, user information, + percent encoding, backslash or non-ASCII character fails. `ci_run_url` must + equal `https://github.com/{repository}/actions/runs/{id}` (the canonical form, + without `/job/{id}`) for the `workflow_run_id` of at least one judged required + check; otherwise `github.ci_run`. The fetcher (chunk 04) takes the + pull-request number from chunk 02's single `parse_pull_request_url`, and this + checker re-asserts equality, so the two cannot disagree. +- **Commit binding (security review, time-of-check).** A required-check entry + whose `check_run_head_sha` or `commit_status_sha` names a commit other than + `head_sha` fails as `github.facts_binding`. Reviews keep their own + `commit_id`: a review of an earlier commit is normal history, so it is not a + failure, but only reviews of `head_sha` can approve. +- **Comparisons.** `repository` (case-insensitive against configuration), + `base_branch`, `merged`, `merge_commit_sha` (lower-cased proof SHA) and one + `required_check` per non-success check. The proof's own `schema`, + `commit_sha`, `ci_conclusion` and `merged` claims keep their rules. The former + "any `https://github.com/` URL" rules for `pr_url` and `ci_run_url` are + replaced by the exact-equality rules. +- **Integrity rules (owner decision 20).** `unbound_required_check` (a + requirement accepting any source; config `allow_unbound_required_checks`, + default false); `protected_path_changed` (one finding listing up to five paths + that match `.github/**`, `**/CODEOWNERS`, `.gitlab-ci.yml`, `.circleci/**` or + configured `protected_paths`, checking both names of a rename; it states that + the work requires explicit human acceptance and cannot be auto-accepted); + `independent_approval_missing` (fewer than `required_independent_approvals`, + default 1, reviewers whose latest `APPROVED`/`CHANGES_REQUESTED`/`DISMISSED` + review by id approves the head SHA, excluding the author by id or login and + every non-`User` account; comments do not cancel an approval). When all + qualifying approvals come from `operator_logins`, the checker adds code + `harnessy.operator_approved` at level `warning`, which does not fail the + verdict. `warning` rather than the lower `info` is used because contributor + and history reads count only blocking, warning and coverage findings, so an + `info` finding would be invisible. +- **Registry schemas.** The POST `configuration_schema` requires both keys, + allows only the integrity options, and bounds them. The POST `input_schema` + constrains `github_facts.schema_version`; a missing facts object reaches the + checker as `facts_missing`. `publish_local.py` defaults to capability version + `v2` because `v1` is immutable and both behavior and schemas changed. +- Rejected: a shape-only mode, PRE registration, URL parsing in the checker, + a second ART mount for facts (owner decision), and requiring `github_facts` + in the input schema (that would turn a missing fact into a request contract + error instead of a clear finding). + +## Acceptance criteria + +- [x] A proof matching the facts passes; missing or invalid configuration and + missing or malformed facts never pass. +- [x] Each mismatch reports its finding: repository, base branch, merged, merge + commit SHA, each non-success required check, and commit binding. +- [x] Every rejected `pr_url` variant, a differing `html_url`, and every + `ci_run_url` that is not a judged required workflow run fail. +- [x] Each integrity rule has its own finding and edge-case tests: stale + approval on an old commit, approval then changes requested or dismissed, bot + approval, author self-approval, rename into or out of `.github`, unbound + check, and operator-only approval. +- [x] The published v2 body is post-submission only and validates through + `ExternalCheckerRegistrySpec`, accepting and rejecting configuration and + input as documented. + +## Risk and review routing + +- Risk class: L1, it changes acceptance evidence for automatic FinalAcceptance + once wired. +- Required reviewers: security, QA, documentation. +- Human review focus: the finding set, failing closed everywhere, strict + `success` for required checks, and `warning` for operator-only approval. + +## Evidence + +| Claim | Command or proof | Result | Remaining uncertainty | +|---|---|---|---| +| Rules and result contract | `pytest tests/checkers/external/test_harnessy_proof_checker.py` | 116 passed | Facts agreement with chunk 02 was checked in a scratch run against that branch, not by a committed cross-branch test; chunk 04 should add one | +| Every fail-closed branch rejects | `coverage run` with `branch = true`, `patch = subprocess`, `include = */harnessy_proof/checker.py`, then `coverage combine` and `coverage report -m` | 100% line and branch on `checker.py` (before: 96%) | The isolated `python -I` entrypoint is measured through coverage's subprocess patch | +| Lint | `ruff check` on the test module and `external_checkers/harnessy_proof/` | Pass | — | +| Docs | Markdown link and stale wording checks | Pass | — | + +The image was not rebuilt or run: Docker was out of scope for this change. + +## Review findings + +- Coverage review found a defect: a `proof.json` directory in the submitted + archive made `read_proof` raise `IsADirectoryError`, crashing the checker + instead of reporting `harnessy.proof.missing`. `read_proof` now checks the + file type on the raw descriptor before wrapping it; directory and FIFO + regressions cover it. +- Security review, fail-open integrity check: removed the shape-only pass and + the facts-without-configuration path. +- Security review, parser differential: replaced URL parsing with exact + equality to canonical URLs built from the facts, and bound `ci_run_url` to a + judged required workflow run. +- Security review, time-of-check binding: added the commit-binding finding. + +## Reconciliation + +- Current-source reconciliation: based on WS-GHPROOF-001-01 over `main` after + PR #674; independent of chunk 02. +- Roadmap: the agent-loop entries now state that the checker compares proofs + with GitHub facts and that FinalAcceptance through it also waits for facts + wiring (chunk 04). +- Next usable boundary: chunk 04 POST wiring. +- Remaining risks: until chunk 04, every evaluation with this checker fails + with `facts_missing`. diff --git a/backend/tests/checkers/external/test_harnessy_proof_checker.py b/backend/tests/checkers/external/test_harnessy_proof_checker.py index f1cd2cd8b..cdbfe13fd 100644 --- a/backend/tests/checkers/external/test_harnessy_proof_checker.py +++ b/backend/tests/checkers/external/test_harnessy_proof_checker.py @@ -1,16 +1,21 @@ -"""Harnessy proof checker logic and its published result contract.""" +"""Harnessy proof checker: fail-closed comparison with GitHub facts.""" from __future__ import annotations import importlib.util +import io import json +import os from pathlib import Path +import subprocess +import sys from uuid import uuid4 import pytest import yaml from app.modules.checkers.api.external import ( + ExternalCheckerContractError, ExternalCheckerExecutionResult, ExternalCheckerRegistrySpec, ) @@ -31,21 +36,102 @@ def _load(name: str): checker = _load("checker") publish_local = _load("publish_local") +REPOSITORY = "Acme/widgets" +HEAD = "e" * 40 +MERGE = "0123456789abcdef0123456789abcdef01234567" +PR_URL = f"https://github.com/{REPOSITORY}/pull/12" +RUN_URL = f"https://github.com/{REPOSITORY}/actions/runs/345" +CONFIGURATION = {"github_repository": "acme/widgets", "base_branch": "main"} + VALID_PROOF = { "schema": "harnessy.work.proof.v1", - "commit_sha": "0123456789abcdef0123456789abcdef01234567", - "pr_url": "https://github.com/acme/widgets/pull/12", - "ci_run_url": "https://github.com/acme/widgets/actions/runs/345", + "commit_sha": MERGE, + "pr_url": PR_URL, + "ci_run_url": RUN_URL, "ci_conclusion": "success", "merged": True, } +def _check(name: str = "backend", **changes: object) -> dict: + check = { + "name": name, + "app_id": 15368, + "app_bound": True, + "conclusion": "success", + "check_run_app_id": 15368, + "check_run_head_sha": HEAD, + "check_run_html_url": f"https://github.com/{REPOSITORY}/runs/10", + "check_run_details_url": f"{RUN_URL}/job/678", + "workflow_run_id": 345, + "commit_status_judged": False, + "commit_status_sha": None, + } + check.update(changes) + return check + + +def _review(review_id: int, state: str, *, commit: str = HEAD, **reviewer: object) -> dict: + return { + "id": review_id, + "reviewer": {"login": "reviewer", "id": 7, "type": "User", **reviewer}, + "state": state, + "commit_id": commit, + "submitted_at": "2026-10-24T11:00:00Z", + } + + +FACTS = { + "schema_version": "workstream.github_facts.v1", + "repository": REPOSITORY, + "pull_request_number": 12, + "html_url": PR_URL, + "state": "closed", + "merged": True, + "merge_commit_sha": MERGE, + "base_ref": "main", + "head_sha": HEAD, + "author": {"login": "agent-bot", "id": 501, "type": "Bot"}, + "reviews": [_review(61, "APPROVED")], + "changed_files_count": 1, + "changed_files": [{"filename": "src/app.py", "previous_filename": None, "status": "modified"}], + "required_checks": [_check("backend"), _check("lint", workflow_run_id=346)], + "fetched_at": "2026-10-24T12:00:00Z", + "responses": [{"request": "pull_request", "sha256": "sha256:" + "f" * 64}], +} + + def _encoded(**changes: object) -> bytes: proof = {**VALID_PROOF, **changes} return json.dumps({key: value for key, value in proof.items() if value is not ...}).encode() +def _input(**changes: object) -> dict: + facts = {**FACTS, **changes} + return { + "schema_version": "workstream.external_checker_post_submit_input.v1", + "github_facts": {key: value for key, value in facts.items() if value is not ...}, + } + + +def _findings( + proof: bytes | None = None, + *, + configuration: object = CONFIGURATION, + input_value: object = None, + **fact_changes: object, +) -> list[dict]: + return checker.evaluate_proof( + _encoded() if proof is None else proof, + configuration=configuration, + input_value=_input(**fact_changes) if input_value is None else input_value, + ) + + +def _codes(proof: bytes | None = None, **kwargs: object) -> list[str]: + return [item["code"] for item in _findings(proof, **kwargs)] + + def _request(phase: str = "post_submit") -> dict: return { "request_digest": "sha256:" + "b" * 64, @@ -54,51 +140,114 @@ def _request(phase: str = "post_submit") -> dict: } -def _codes(raw: bytes | None) -> list[str]: - return [item["code"] for item in checker.evaluate_proof(raw)] - - -def test_valid_proof_passes_with_backend_valid_result() -> None: - assert checker.evaluate_proof(_encoded()) == [] - for phase in ("pre_submit", "post_submit"): - request = _request(phase) - result = checker.build_result(request, []) - parsed = ExternalCheckerExecutionResult.model_validate_json(json.dumps(result)) - assert (parsed.verdict, parsed.findings, parsed.phase) == ("passed", (), phase) - assert ( - parsed.request_digest, - str(parsed.registry_entry_id), - parsed.registry_entry_digest, - ) == ( - request["request_digest"], - request["registry"]["registry_entry_id"], - request["registry"]["entry_digest"], - ) +# Matching proof and result contract -------------------------------------------- + + +def test_proof_matching_github_facts_passes_with_backend_valid_result() -> None: + assert _findings() == [] + assert _codes(_encoded(commit_sha=MERGE.upper())) == [] + request = _request() + parsed = ExternalCheckerExecutionResult.model_validate_json( + json.dumps(checker.build_result(request, [])) + ) + assert (parsed.verdict, parsed.findings, parsed.phase) == ("passed", (), "post_submit") + assert ( + parsed.request_digest, + str(parsed.registry_entry_id), + parsed.registry_entry_digest, + ) == ( + request["request_digest"], + request["registry"]["registry_entry_id"], + request["registry"]["entry_digest"], + ) + + +def test_failed_verdict_exactly_when_an_error_finding_exists() -> None: + findings = _findings(merged=False) + parsed = ExternalCheckerExecutionResult.model_validate_json( + json.dumps(checker.build_result(_request(), findings)) + ) + assert parsed.verdict == "failed" + info = [{**findings[0], "level": "info"}] + assert checker.build_result(_request(), info)["verdict"] == "passed" + + +def test_main_passes_request_configuration_and_input( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + request = {**_request(), "configuration": CONFIGURATION, "input": _input(merged=False)} + monkeypatch.setattr(checker, "read_proof", lambda _root: _encoded()) + monkeypatch.setattr(checker.sys, "stdin", io.StringIO(json.dumps(request))) + assert checker.main() == 0 + result = json.loads(capsys.readouterr().out) + assert result["verdict"] == "failed" + assert [item["code"] for item in result["findings"]] == ["harnessy.proof.github.merged"] + + +# One fail-closed mode ------------------------------------------------------------ @pytest.mark.parametrize( - ("changes", "code"), + "configuration", [ - ({"schema": "harnessy.work.proof.v2"}, "schema"), - ({"schema": ...}, "schema"), - ({"commit_sha": "0123456789abcdef0123456789abcdef0123456"}, "commit_sha"), - ({"commit_sha": "g123456789abcdef0123456789abcdef01234567"}, "commit_sha"), - ({"commit_sha": 1234}, "commit_sha"), - ({"pr_url": "http://github.com/acme/widgets/pull/12"}, "pr_url"), - ({"pr_url": "https://github.com.evil.example/acme/pull/12"}, "pr_url"), - ({"pr_url": "https://user@github.com/acme/widgets/pull/12"}, "pr_url"), - ({"pr_url": "https://github.com:8443/acme/widgets/pull/12"}, "pr_url"), - ({"ci_run_url": "https://github.com/"}, "ci_run_url"), - ({"ci_run_url": "https://gitlab.com/acme/widgets/-/pipelines/1"}, "ci_run_url"), - ({"ci_conclusion": "failure"}, "ci_conclusion"), - ({"ci_conclusion": ...}, "ci_conclusion"), - ({"merged": "true"}, "merged"), - ({"merged": 1}, "merged"), - ({"merged": False}, "merged"), + None, + {}, + {"github_repository": "acme/widgets"}, + {"base_branch": "main"}, + {**CONFIGURATION, "extra": True}, + {"github_repository": "", "base_branch": "main"}, + {**CONFIGURATION, "allow_unbound_required_checks": "yes"}, + {**CONFIGURATION, "protected_paths": ".github/**"}, + {**CONFIGURATION, "protected_paths": [""]}, + {**CONFIGURATION, "required_independent_approvals": 0}, + {**CONFIGURATION, "required_independent_approvals": True}, + {**CONFIGURATION, "operator_logins": [1]}, ], ) -def test_each_invalid_claim_reports_exactly_its_error(changes: dict, code: str) -> None: - assert _codes(_encoded(**changes)) == [f"harnessy.proof.{code}"] +def test_missing_or_invalid_configuration_never_passes(configuration: object) -> None: + assert _codes(configuration=configuration) == ["harnessy.proof.github.configuration"] + + +@pytest.mark.parametrize( + "input_value", + [{}, {"schema_version": "workstream.external_checker_post_submit_input.v1"}, "x"], +) +def test_missing_facts_never_pass(input_value: object) -> None: + assert _codes(input_value=input_value) == ["harnessy.proof.github.facts_missing"] + + +@pytest.mark.parametrize( + "facts_changes", + [ + {"schema_version": "workstream.github_facts.v2"}, + {"merged": "true"}, + {"pull_request_number": "12"}, + {"html_url": ...}, + {"required_checks": []}, + {"required_checks": [{"name": "backend"}]}, + {"required_checks": [{**_check(), "extra": 1}]}, + {"required_checks": [_check(workflow_run_id="345")]}, + {"merge_commit_sha": "ABC"}, + {"head_sha": ...}, + {"author": ...}, + {"reviews": ...}, + {"reviews": [_review(61, "APPROVED", commit="not-a-sha")]}, + {"changed_files": ...}, + {"changed_files_count": ...}, + {"changed_files_count": 2}, + ], +) +def test_malformed_facts_are_an_error(facts_changes: dict) -> None: + assert _codes(**facts_changes) == ["harnessy.proof.github.facts_invalid"] + + +@pytest.mark.parametrize( + "check", + [_check(check_run_head_sha="1" * 40), _check(commit_status_sha="2" * 40)], + ids=("check-run-other-commit", "status-other-commit"), +) +def test_check_entry_naming_another_commit_fails_the_binding(check: dict) -> None: + assert _codes(required_checks=[check]) == ["harnessy.proof.github.facts_binding"] @pytest.mark.parametrize( @@ -112,18 +261,8 @@ def test_each_invalid_claim_reports_exactly_its_error(changes: dict, code: str) ], ) def test_unreadable_proof_reports_one_error(raw: bytes | None, code: str) -> None: - assert _codes(raw) == [f"harnessy.proof.{code}"] - - -def test_failed_verdict_exactly_when_an_error_finding_exists() -> None: - findings = checker.evaluate_proof(_encoded(merged=False, ci_conclusion="cancelled")) - parsed = ExternalCheckerExecutionResult.model_validate_json( - json.dumps(checker.build_result(_request(), findings)) - ) - assert parsed.verdict == "failed" - assert [item.path for item in parsed.findings] == ["proof.json", "proof.json"] - info = [{**findings[0], "level": "info"}] - assert checker.build_result(_request(), info)["verdict"] == "passed" + findings = checker.evaluate_proof(raw, configuration=CONFIGURATION, input_value=_input()) + assert [item["code"] for item in findings] == [f"harnessy.proof.{code}"] def test_read_proof_rejects_links_and_bounds_bytes(tmp_path: Path) -> None: @@ -137,18 +276,290 @@ def test_read_proof_rejects_links_and_bounds_bytes(tmp_path: Path) -> None: assert len(checker.read_proof(tmp_path)) == checker.MAX_PROOF_BYTES + 1 -def test_published_registry_bodies_are_backend_valid_for_both_phases() -> None: +# Claims and mismatches ----------------------------------------------------------- + + +@pytest.mark.parametrize( + ("changes", "code"), + [ + ({"schema": "harnessy.work.proof.v2"}, "schema"), + ({"schema": ...}, "schema"), + ({"ci_conclusion": "failure"}, "ci_conclusion"), + ({"ci_conclusion": ...}, "ci_conclusion"), + ({"merged": "true"}, "merged"), + ({"merged": 1}, "merged"), + ({"merged": False}, "merged"), + ], +) +def test_each_invalid_claim_reports_exactly_its_error(changes: dict, code: str) -> None: + assert _codes(_encoded(**changes)) == [f"harnessy.proof.{code}"] + + +@pytest.mark.parametrize( + "commit_sha", + ["0123456789abcdef0123456789abcdef0123456", "g" * 40, 1234], +) +def test_malformed_commit_sha_fails_shape_and_merge_comparison(commit_sha: object) -> None: + assert _codes(_encoded(commit_sha=commit_sha)) == [ + "harnessy.proof.commit_sha", + "harnessy.proof.github.merge_commit_sha", + ] + + +@pytest.mark.parametrize( + ("proof_changes", "fact_changes", "codes"), + [ + ({}, {"repository": "acme/other"}, ["repository", "pr_url", "ci_run"]), + ({}, {"base_ref": "release"}, ["base_branch"]), + ({}, {"merged": False, "state": "open"}, ["merged"]), + ({"commit_sha": "1" * 40}, {}, ["merge_commit_sha"]), + ({}, {"merged": False, "merge_commit_sha": None}, ["merged", "merge_commit_sha"]), + ], +) +def test_each_github_mismatch_reports_its_finding( + proof_changes: dict, fact_changes: dict, codes: list[str] +) -> None: + assert _codes(_encoded(**proof_changes), **fact_changes) == [ + f"harnessy.proof.github.{code}" for code in codes + ] + + +@pytest.mark.parametrize( + "pr_url", + [ + f"{PR_URL}/", + f"{PR_URL}?x=1", + f"{PR_URL}#top", + "https://github.com:443/Acme/widgets/pull/12", + "https://user@github.com/Acme/widgets/pull/12", + "https://github.com/Acme/widgets/pull/%31%32", + "https://github.com/Acme\\widgets/pull/12", + "https://github.com/acme/widgets/pull/12", + "https://github.com/Acme/widgets/pull/012", + "https://github.com/Acme/widgets/pull/13", + "https://github.com/Acme/widgéts/pull/12", + "http://github.com/Acme/widgets/pull/12", + "https://github.com/Acme/widgets/issues/12", + "https://github.com/Acme/other/pull/12", + None, + 12, + ], +) +def test_pr_url_must_equal_the_canonical_github_url_exactly(pr_url: object) -> None: + assert _codes(_encoded(pr_url=pr_url)) == ["harnessy.proof.github.pr_url"] + + +def test_pr_url_must_also_equal_githubs_own_html_url() -> None: + assert _codes(html_url="https://github.com/Acme/widgets/pull/12/") == [ + "harnessy.proof.github.pr_url" + ] + + +@pytest.mark.parametrize( + "ci_run_url", + [ + f"{RUN_URL}/job/678", + f"{RUN_URL}/", + f"{RUN_URL}?check_suite_focus=true", + "https://github.com/Acme/widgets/actions/runs/999", + "https://github.com/acme/widgets/actions/runs/345", + "https://github.com/Acme/other/actions/runs/345", + f"https://github.com/{REPOSITORY}/runs/10", + None, + ], +) +def test_ci_run_url_must_be_a_judged_required_workflow_run(ci_run_url: object) -> None: + assert _codes(_encoded(ci_run_url=ci_run_url)) == ["harnessy.proof.github.ci_run"] + + +def test_ci_run_url_may_name_any_judged_required_workflow_run() -> None: + other_run = f"https://github.com/{REPOSITORY}/actions/runs/346" + assert _codes(_encoded(ci_run_url=other_run)) == [] + no_runs = [_check(workflow_run_id=None)] + assert _codes(required_checks=no_runs) == ["harnessy.proof.github.ci_run"] + + +def test_each_unsuccessful_required_check_reports_its_own_finding() -> None: + checks = [ + _check("backend", conclusion="failure"), + _check("lint", workflow_run_id=None), + _check("docs", conclusion="missing", workflow_run_id=None), + _check("e2e\nspoof", conclusion="pending", workflow_run_id=None), + ] + findings = _findings(required_checks=checks) + assert [item["code"] for item in findings] == ["harnessy.proof.github.required_check"] * 3 + messages = [item["message"] for item in findings] + assert "backend is failure" in messages[0] + assert "docs is missing" in messages[1] + assert "e2e?spoof is pending" in messages[2] + + +# Integrity rules (owner decision 20) ---------------------------------------------- + + +def test_unbound_required_check_is_an_error_unless_explicitly_allowed() -> None: + checks = [_check("backend"), _check("lint", app_id=None, app_bound=False)] + findings = _findings(required_checks=checks) + assert [item["code"] for item in findings] == [ + "harnessy.proof.github.unbound_required_check" + ] + assert "lint" in findings[0]["message"] + allowed = {**CONFIGURATION, "allow_unbound_required_checks": True} + assert _codes(configuration=allowed, required_checks=checks) == [] + + +@pytest.mark.parametrize( + "changed", + [ + {"filename": ".github/workflows/ci.yml", "previous_filename": None, "status": "modified"}, + {"filename": "docs/CODEOWNERS", "previous_filename": None, "status": "added"}, + {"filename": "CODEOWNERS", "previous_filename": None, "status": "modified"}, + {"filename": ".gitlab-ci.yml", "previous_filename": None, "status": "added"}, + {"filename": ".circleci/config.yml", "previous_filename": None, "status": "removed"}, + {"filename": ".github/workflows/x.yml", "previous_filename": "x.yml", "status": "renamed"}, + { + "filename": "moved.yml", + "previous_filename": ".github/workflows/x.yml", + "status": "renamed", + }, + ], + ids=( + "workflow", + "nested-codeowners", + "root-codeowners", + "gitlab", + "circleci", + "rename-into-github", + "rename-out-of-github", + ), +) +def test_protected_path_change_requires_human_acceptance(changed: dict) -> None: + files = [FACTS["changed_files"][0], changed] + findings = _findings(changed_files=files, changed_files_count=2) + assert [item["code"] for item in findings] == [ + "harnessy.proof.github.protected_path_changed" + ] + assert "cannot be auto-accepted" in findings[0]["message"] + + +def test_configured_protected_paths_extend_the_defaults() -> None: + files = [{"filename": "deploy/prod.tf", "previous_filename": None, "status": "modified"}] + assert _codes(changed_files=files) == [] + extended = {**CONFIGURATION, "protected_paths": ["deploy/**"]} + assert _codes(configuration=extended, changed_files=files) == [ + "harnessy.proof.github.protected_path_changed" + ] + nested = [{"filename": "src/.github/x", "previous_filename": None, "status": "added"}] + assert _codes(changed_files=nested) == [] + + +def test_protected_path_finding_lists_a_bounded_number_of_paths() -> None: + files = [ + {"filename": f".github/workflows/w{index}.yml", "previous_filename": None, "status": "added"} + for index in range(20) + ] + findings = _findings(changed_files=files, changed_files_count=20) + assert len(findings) == 1 + assert "and 15 more" in findings[0]["message"] + + +@pytest.mark.parametrize( + "reviews", + [ + [], + [_review(61, "APPROVED", commit="1" * 40)], + [_review(61, "APPROVED"), _review(62, "CHANGES_REQUESTED")], + [_review(61, "APPROVED"), _review(62, "DISMISSED")], + [_review(61, "APPROVED", type="Bot", login="ci-bot[bot]")], + [_review(61, "APPROVED", login="agent-bot", id=501)], + [_review(61, "APPROVED", login="Agent-Bot", id=999)], + [_review(61, "COMMENTED")], + [{**_review(61, "APPROVED"), "reviewer": None}], + ], + ids=( + "no-review", + "stale-approval-on-old-commit", + "approval-then-changes-requested", + "approval-then-dismissed", + "bot-approval", + "author-self-approval", + "author-login-different-case", + "comment-only", + "deleted-reviewer", + ), +) +def test_missing_independent_approval_on_head_commit_is_an_error(reviews: list) -> None: + assert _codes(reviews=reviews) == ["harnessy.proof.github.independent_approval_missing"] + + +def test_latest_decisive_review_wins_and_comments_do_not_cancel() -> None: + reviews = [ + _review(61, "CHANGES_REQUESTED"), + _review(62, "APPROVED"), + _review(63, "COMMENTED"), + ] + assert _codes(reviews=reviews) == [] + out_of_order = [_review(70, "APPROVED"), _review(65, "CHANGES_REQUESTED")] + assert _codes(reviews=out_of_order) == [] + old_then_new = [_review(60, "APPROVED", commit="1" * 40), _review(61, "APPROVED")] + assert _codes(reviews=old_then_new) == [] + + +def test_required_independent_approvals_is_configurable() -> None: + two = {**CONFIGURATION, "required_independent_approvals": 2} + assert _codes(configuration=two) == ["harnessy.proof.github.independent_approval_missing"] + reviews = [_review(61, "APPROVED"), _review(62, "APPROVED", login="second", id=8)] + assert _codes(configuration=two, reviews=reviews) == [] + + +def test_operator_only_approval_is_a_non_blocking_warning() -> None: + operators = {**CONFIGURATION, "operator_logins": ["Reviewer"]} + findings = _findings(configuration=operators) + assert [(item["code"], item["level"]) for item in findings] == [ + ("harnessy.operator_approved", "warning") + ] + result = ExternalCheckerExecutionResult.model_validate_json( + json.dumps(checker.build_result(_request(), findings)) + ) + assert result.verdict == "passed" + reviews = [_review(61, "APPROVED"), _review(62, "APPROVED", login="independent", id=8)] + assert _codes(configuration=operators, reviews=reviews) == [] + + +# Published registry body ----------------------------------------------------------- + + +def test_published_registry_body_is_post_only_and_backend_valid() -> None: digest = "sha256:" + "d" * 64 - for phase in ("pre_submit", "post_submit"): - spec = ExternalCheckerRegistrySpec.model_validate( - publish_local.registry_spec(phase, digest, "v2") - ) - assert (spec.phase, spec.image_digest, spec.capability_id, spec.capability_version) == ( - phase, - digest, - "harnessy.work_proof", - "v2", - ) + spec = ExternalCheckerRegistrySpec.model_validate(publish_local.registry_spec(digest, "v2")) + assert (spec.phase, spec.image_digest, spec.capability_id, spec.capability_version) == ( + "post_submit", + digest, + "harnessy.work_proof", + "v2", + ) + full = { + **CONFIGURATION, + "allow_unbound_required_checks": False, + "protected_paths": ["deploy/**"], + "required_independent_approvals": 2, + "operator_logins": ["julian"], + } + spec.configuration_schema.validate_instance(CONFIGURATION) + spec.configuration_schema.validate_instance(full) + spec.input_schema.validate_instance(_input()) + for schema, value in ( + (spec.configuration_schema, {}), + (spec.configuration_schema, {"github_repository": "acme/widgets"}), + (spec.configuration_schema, {"base_branch": "main"}), + (spec.configuration_schema, {**CONFIGURATION, "github_repository": "acme"}), + (spec.configuration_schema, {**CONFIGURATION, "extra": 1}), + (spec.configuration_schema, {**CONFIGURATION, "required_independent_approvals": 0}), + (spec.configuration_schema, {**CONFIGURATION, "protected_paths": "deploy/**"}), + (spec.input_schema, _input(schema_version="workstream.github_facts.v2")), + ): + with pytest.raises(ExternalCheckerContractError): + schema.validate_instance(value) for registry in ("localhost:5000@registry.example", "localhost", "registry.example:5000"): with pytest.raises(SystemExit): publish_local.publish(registry, "harnessy-proof") @@ -239,3 +650,54 @@ def test_published_service_paths_match_the_compose_overlay() -> None: sandbox_gid=20, enable_model_proxy=True, ) + + +def test_long_or_control_check_names_are_bounded_in_messages() -> None: + name = "x" * 300 + findings = _findings(required_checks=[_check(name, conclusion="failure")]) + assert [item["code"] for item in findings] == ["harnessy.proof.github.required_check"] + assert "x" * 200 + "..." in findings[0]["message"] + assert "x" * 201 not in findings[0]["message"] + + +@pytest.mark.parametrize( + ("path", "protected"), + [ + ("deploy/prod.tf", True), + ("deploy/eu/prod.tf", False), + ("keys/secret1.pem", True), + ("keys/secret12.pem", False), + ], +) +def test_configured_globs_keep_single_segment_wildcards_within_a_directory( + path: str, protected: bool +) -> None: + configuration = {**CONFIGURATION, "protected_paths": ["deploy/*.tf", "keys/secret?.pem"]} + files = [{"filename": path, "previous_filename": None, "status": "modified"}] + expected = ["harnessy.proof.github.protected_path_changed"] if protected else [] + assert _codes(configuration=configuration, changed_files=files) == expected + + +@pytest.mark.parametrize("kind", ["directory", "fifo"]) +def test_read_proof_rejects_non_regular_files(tmp_path: Path, kind: str) -> None: + if kind == "directory": + (tmp_path / "proof.json").mkdir() + else: + os.mkfifo(tmp_path / "proof.json") + assert checker.read_proof(tmp_path) is None + + +def test_entrypoint_fails_closed_without_proof_material() -> None: + """The image entrypoint runs isolated and reports the missing proof.""" + request = {**_request(), "configuration": CONFIGURATION, "input": _input()} + completed = subprocess.run( + [sys.executable, "-I", str(CHECKER_ROOT / "checker.py")], + input=json.dumps(request), + capture_output=True, + text=True, + check=True, + timeout=30, + ) + result = json.loads(completed.stdout) + assert result["verdict"] == "failed" + assert [item["code"] for item in result["findings"]] == ["harnessy.proof.missing"] diff --git a/docs/engineering/local-agent-loop.md b/docs/engineering/local-agent-loop.md index 5a51aee78..92f050993 100644 --- a/docs/engineering/local-agent-loop.md +++ b/docs/engineering/local-agent-loop.md @@ -94,7 +94,7 @@ docker compose up --wait checker-registry ``` Build and publish the proof checker, then write the checker-service -configuration and the two registry bodies. Run this on the Docker host from the +configuration and the post-submission registry body. Run this on the Docker host from the repository root: ```bash @@ -120,9 +120,9 @@ with an empty client configuration, for example Rebuilding the proof checker changes its digest. The registry holds one immutable entry per phase, capability and version, so rerun `publish_local.py` -with a new `--capability-version` (for example `v2`), recreate the checker with -`docker compose up --wait --force-recreate checker`, and register the two new -bodies. Registering a new digest under an existing version returns +with a new `--capability-version` (the default is `v2`; use, for example, `v3`), recreate the checker with +`docker compose up --wait --force-recreate checker`, and register the new +body. Registering a new digest under an existing version returns `external_checker_registry_conflict`. ## Create operators and service identities @@ -195,16 +195,14 @@ docker compose exec -T backend python scripts/provision_local_service_actors.py ## Register the proof checker -Registration needs a current system `operator` grant. Each body is an -`ExternalCheckerRegistrySpec` for one phase with the exact platform-manifest -digest. Keep each key with its file; replaying the same key and body returns the -same immutable entry: +Registration needs a current system `operator` grant. The body is an +`ExternalCheckerRegistrySpec` for the post-submission phase with the exact +platform-manifest digest. The checker needs GitHub facts, which pre-submission +checking never has, so it is not registered for PRE. Keep the key with its file; +replaying the same key and body returns the same immutable entry: ```bash -PRE_KEY="$(uuidgen | tr 'A-Z' 'a-z')" POST_KEY="$(uuidgen | tr 'A-Z' 'a-z')" -ws_as local-operator operations checker registry register \ - --input .local-agent-loop/registry-pre.json --idempotency-key "$PRE_KEY" -o json ws_as local-operator operations checker registry register \ --input .local-agent-loop/registry-post.json --idempotency-key "$POST_KEY" -o json ``` @@ -218,27 +216,99 @@ run the checker for any project. ### What the proof checker checks -`external_checkers/harnessy_proof/checker.py` reads `proof.json` at the root of -the submitted ZIP and returns `external_checker_result.v1`. Each failed rule is -one `error` finding on `proof.json`, and the verdict is `failed` exactly when an -error finding exists: +`external_checkers/harnessy_proof/checker.py` (capability version `v2`, +[WS-GHPROOF-001](../../.commitrail/initiatives/WS-GHPROOF-001/OVERVIEW.md)) is a +post-submission checker with one mode, and it fails closed. It reads +`proof.json` at the root of the submitted ZIP, compares it with GitHub facts +that WorkStream read with its read-only GitHub App, and returns +`external_checker_result.v1`. The checker has no network access and no +credential. Every failed rule is one `error` finding on `proof.json`, and the +verdict is `failed` exactly when an error finding exists. A proof never passes +on its own claims. + +The POST binding configuration must name the repository and base branch, and +may set integrity options whose defaults are the safe ones: + +```json +{"github_repository": "Flow-Research/harnessy-v2", "base_branch": "main"} +``` + +| Option | Default | Effect | +|---|---|---| +| `allow_unbound_required_checks` | `false` | When false, a required check that accepts any source is an error | +| `protected_paths` | `[]` | Globs added to `.github/**`, `**/CODEOWNERS`, `.gitlab-ci.yml`, `.circleci/**` | +| `required_independent_approvals` | `1` | Independent approvals needed on the head commit (1 to 10) | +| `operator_logins` | `[]` | Logins whose approval alone adds the `harnessy.operator_approved` warning | + +WorkStream places the facts in the request `input` under `github_facts`, so the +request digest binds them. The facts are bound to one pull-request state +(`head_sha`, `merge_commit_sha`): + +```json +{ + "github_facts": { + "schema_version": "workstream.github_facts.v1", + "repository": "Flow-Research/harnessy-v2", + "pull_request_number": 12, + "html_url": "https://github.com/Flow-Research/harnessy-v2/pull/12", + "state": "closed", + "merged": true, + "merge_commit_sha": "<40 lowercase hex, or null unless merged>", + "base_ref": "main", + "head_sha": "<40 lowercase hex>", + "author": {"login": "agent-bot", "id": 501, "type": "Bot"}, + "reviews": [{"id": 61, "reviewer": {"login": "reviewer", "id": 7, "type": "User"}, + "state": "APPROVED", "commit_id": "", "submitted_at": "..."}], + "changed_files_count": 1, + "changed_files": [{"filename": "src/app.py", "previous_filename": null, "status": "modified"}], + "required_checks": [{"name": "backend", "app_id": 15368, "app_bound": true, + "conclusion": "success", "check_run_app_id": 15368, + "check_run_head_sha": "", + "check_run_html_url": "...", "check_run_details_url": "...", + "workflow_run_id": 345, "commit_status_judged": false, + "commit_status_sha": null}], + "fetched_at": "2026-10-24T12:00:00Z", + "responses": [{"request": "pull_request", "sha256": "sha256:<64 hex>"}] + } +} +``` + +Rules on the claims themselves (`harnessy.proof.*`): `schema` equals +`harnessy.work.proof.v1`; `commit_sha` is 40 hexadecimal characters; +`ci_conclusion` equals `success`; `merged` is the JSON value `true`. Rules +against the facts (`harnessy.proof.github.*`): -| Field | Rule | +| Code | Fails when | |---|---| -| `schema` | equals `harnessy.work.proof.v1` | -| `commit_sha` | 40 hexadecimal characters | -| `pr_url`, `ci_run_url` | `https://github.com/...` without credentials or port | -| `ci_conclusion` | equals `success` | -| `merged` | the JSON value `true` | - -**Trust caveat.** The checker has no network access. It checks only what -`proof.json` claims. It does not confirm that the pull request, commit or CI -run exist, that the CI run belongs to the commit, or that the pull request was -merged. A passing verdict means well-formed claims, not independently verified -work. With `human_review_required:false`, a well-formed but false proof -would reach automatic FinalAcceptance. Do not use this checker alone to gate -acceptance that leads to compensation; pair it with human review, or with a -checker that has trusted access to the evidence, when the claim must be true. +| `configuration` | the configuration is missing, lacks either required key, or has an invalid option | +| `facts_missing` | the input has no facts | +| `facts_invalid` | the facts do not have the exact `workstream.github_facts.v1` shape | +| `facts_binding` | a required-check entry names a commit other than `head_sha` | +| `repository` | the facts' repository is not `github_repository` (case-insensitive) | +| `base_branch` | the pull request's base branch is not `base_branch` | +| `pr_url` | `pr_url` is not exactly `https://github.com/{repository}/pull/{number}` built from the facts, or GitHub's `html_url` differs from it | +| `ci_run` | `ci_run_url` is not exactly `https://github.com/{repository}/actions/runs/{id}` for the workflow run of a judged required check | +| `merged` | GitHub reports the pull request as not merged | +| `merge_commit_sha` | `commit_sha` is not the merge commit (case-insensitive) | +| `required_check` | one finding per required check whose result is not `success` | +| `unbound_required_check` | a required check accepts any source and `allow_unbound_required_checks` is false | +| `protected_path_changed` | a changed file, or a rename's previous name, matches a protected glob; the work needs explicit human acceptance and cannot be auto-accepted | +| `independent_approval_missing` | fewer than `required_independent_approvals` reviewers approve the head commit, counting each reviewer's latest approve, request-changes or dismissed review, and excluding the author and non-`User` accounts | + +URL comparison is exact and case-sensitive against the repository name GitHub +returns. A trailing slash, query, fragment, port, user information, percent +encoding, backslash or non-ASCII character fails. The canonical CI run form is +the workflow run URL without `/job/{id}`. + +When every qualifying approval comes from `operator_logins`, the checker adds a +non-blocking `warning` finding with code `harnessy.operator_approved`: the work +may earn points but may not be paid in money until an independent reviewer +confirms it. The verdict does not change. + +**Until facts are wired.** WorkStream's POST evaluation does not yet supply +facts (initiative chunk 04). Until it does, every evaluation with this checker +fails with `facts_missing`, so this stack cannot reach automatic +FinalAcceptance through it. There is deliberately no shape-only mode. ## Create the manager project and the agent submitter @@ -377,8 +447,8 @@ version that selects the new published policy is activated. ## Placeholder: compile, approve and activate the guide rules > **Not covered here.** Selecting `harnessy.work_proof` into the locked -> pre-submission and post-submission policies is guide compilation, followed by -> the manager's separate PRE and POST approvals and guide activation. How rule +> post-submission policy is guide compilation, followed by the manager's +> separate PRE and POST approvals and guide activation. How rule > setup works for this stack is being decided separately. Until it is, no task > can be created for the project, and the contributor flow below cannot run. > Provider-backed compilation needs `OPENROUTER_API_KEY` (for the default OpenRouter provider); see the diff --git a/docs/roadmap_status.md b/docs/roadmap_status.md index c43265073..a04822367 100644 --- a/docs/roadmap_status.md +++ b/docs/roadmap_status.md @@ -578,7 +578,7 @@ cannot be reused as post-submission review-gate evidence. See the | CLI and terminal interface | **Public CLI clients and opt-in read-only contributor workbench** | Existing explicit commands retain text/JSON/noninteractive behavior. [WS-CLI-001-48](../.commitrail/initiatives/WS-CLI-001/WS-CLI-001-48.md) adds exact-grant project, Ready/active Mine/own Submission browsing and fresh details/context/intake through nine existing typed reads, with explicit pagination and fresh-denial handling | Detailed findings/Evidence remain manual CLI handoffs; a shared typed single-page contribution port precedes in-TUI Evidence. TUI writes/recovery and management remain separate. Protected first binary publication and hosted journey proof remain outstanding | | Fulfillment, reconciliation, and audit | **Planned** | Shared audit foundations, provider-neutral adapter convention, AUTH-OUTBOX-02 live dispatcher authority, retained phase audit decisions, Celery delivery/recovery scans and CON-02B custody | Feature-specific handlers and authority, conditional award fulfillment, callbacks, idempotent recovery, reconciliation, bounded operational reads, and release controls | | Runtime diagnostics | **Implemented foundation; release configuration prepared** | Closed structured logging, explicit API/Celery tracing, bounded metrics, safe correlation and optional typed OTLP export; hosted composition accepts only an optional HTTPS collector origin and keeps exporter outage outside product correctness | Restrict broker publishers; configure and secure collector/log access, egress, encryption and finite deletion; prove privacy, correlation and collector outage during the deployed release drill; no deployed monitoring claim | -| Frontend and pilot | **Local runtime foundation; release configuration prepared; project inputs prepared** | An original [Terminal Systems guide, example and five task briefs](../pilots/terminal_systems/README.md) provide inputs for fresh project onboarding. The fixed five-task sample, its checker, private runtime continuation and sample-only proof workflow are retired. Checkout-isolated Docker Compose API, prefork Celery, Beat, PostgreSQL, Redis and MinIO remain available, alongside digest-only hosted release composition and the generic external image service. The generic image command tests project samples and publishes a verified digest to ECR without registering or activating it. The opt-in [local agent-loop overlay](engineering/local-agent-loop.md) adds that service, a separately built networkless proof checker and operator scripts; Docker Desktop (Apple Silicon) proof covers registration, checker execution and lifecycle activation, not this project’s requirements. Setup supports inference and explicitly manager-authored rules through the same governed proposal path. Inference is limited to PRE/POST specifications, with strict JSON and bounded correction; PM policy choices remain separate. The retained real Terminal Systems proposal preserves null project limits on the unchanged guide; its independent review predates the PRE/POST-only scope, with ten explicit implementation/runtime gaps and no bindings in that retained proposal. The [project-owned PRE image](../pilots/terminal_systems/checkers/README.md) now implements layout and metadata intake with local rule and networkless container samples; it has no task-name allowlist or shared-runner project code. The independent offline gVisor feasibility spike remains historical mechanism evidence | Obtain a fresh PRE/POST-only correction of the retained proposal, then reconcile the implemented PRE image, implement the remaining POST checks and resolve their runtime/material gaps; register the required images, obtain corrected policy bindings and separate approvals, then activate, import, screen and release tasks. Then prove the real contribution journey, runtime limits and interruption recovery on the intended local/hosted stack. Guide assets, image builds and former sample results are not live-project evidence. Hosted identity/AWS configuration, isolation, diagnostics, backup/restore and broader macOS runtime proof remain; the agent-loop overlay still needs approved guide bindings and a persistent-stack FinalAcceptance drill | +| Frontend and pilot | **Local runtime foundation; release configuration prepared; project inputs prepared** | An original [Terminal Systems guide, example and five task briefs](../pilots/terminal_systems/README.md) provide inputs for fresh project onboarding. The fixed five-task sample, its checker, private runtime continuation and sample-only proof workflow are retired. Checkout-isolated Docker Compose API, prefork Celery, Beat, PostgreSQL, Redis and MinIO remain available, alongside digest-only hosted release composition and the generic external image service. The generic image command tests project samples and publishes a verified digest to ECR without registering or activating it. The opt-in [local agent-loop overlay](engineering/local-agent-loop.md) adds that service, a separately built networkless proof checker and operator scripts; Docker Desktop (Apple Silicon) proof covers registration, checker execution and lifecycle activation, not this project’s requirements. Setup supports inference and explicitly manager-authored rules through the same governed proposal path. Inference is limited to PRE/POST specifications, with strict JSON and bounded correction; PM policy choices remain separate. The retained real Terminal Systems proposal preserves null project limits on the unchanged guide; its independent review predates the PRE/POST-only scope, with ten explicit implementation/runtime gaps and no bindings in that retained proposal. The [project-owned PRE image](../pilots/terminal_systems/checkers/README.md) now implements layout and metadata intake with local rule and networkless container samples; it has no task-name allowlist or shared-runner project code. The independent offline gVisor feasibility spike remains historical mechanism evidence | Obtain a fresh PRE/POST-only correction of the retained proposal, then reconcile the implemented PRE image, implement the remaining POST checks and resolve their runtime/material gaps; register the required images, obtain corrected policy bindings and separate approvals, then activate, import, screen and release tasks. Then prove the real contribution journey, runtime limits and interruption recovery on the intended local/hosted stack. Guide assets, image builds and former sample results are not live-project evidence. Hosted identity/AWS configuration, isolation, diagnostics, backup/restore and broader macOS runtime proof remain; the agent-loop overlay still needs approved guide bindings, GitHub facts supplied to POST evaluation ([WS-GHPROOF-001](../.commitrail/initiatives/WS-GHPROOF-001/OVERVIEW.md)) and a persistent-stack FinalAcceptance drill | ## What Has Been Completed @@ -605,7 +605,9 @@ cannot be reused as post-submission review-gate evidence. See the grant operations remain the sole local authority path. The opt-in [local agent loop overlay](engineering/local-agent-loop.md) adds the existing checker service in `docker-dev` mode with a networkless - proof-claim checker; selecting it into guide rules remains separate work. + proof checker that compares proofs with GitHub facts and fails closed until + POST evaluation supplies them; selecting it into guide rules remains + separate work. - AWS S3 is the hosted artifact target, MinIO proves the storage protocol in development and CI, and all storage access stays behind `ArtifactStore`. Development/CI MinIO uses a shared pinned-source build; existing artifact diff --git a/external_checkers/harnessy_proof/checker.py b/external_checkers/harnessy_proof/checker.py index 66249f07b..46994d8b0 100644 --- a/external_checkers/harnessy_proof/checker.py +++ b/external_checkers/harnessy_proof/checker.py @@ -1,14 +1,24 @@ -"""Networkless Harnessy work-proof checker for the local agent loop. +"""Networkless Harnessy work-proof checker for post-submission evaluation. The checker reads one ``external_checker_request.v1`` envelope on stdin and the -verified, read-only Submission material under ``/work/input``. It validates the -claims in ``proof.json`` and writes one ``external_checker_result.v1`` envelope. - -Trust caveat: the checker runs without network access, so it validates only the -shape and values that ``proof.json`` claims. It does not confirm that the pull -request, commit or CI run exist on GitHub, that the CI run belongs to that -commit, or that the pull request was really merged. A passing verdict means -"well-formed proof claims", not "independently verified work". +verified, read-only Submission material under ``/work/input``. It compares the +claims in ``proof.json`` with ``workstream.github_facts.v1`` facts that +WorkStream read from GitHub and placed in the request input under +``github_facts``, and writes one ``external_checker_result.v1`` envelope. + +There is one mode and it fails closed (owner decision 20): the binding +configuration must name ``github_repository`` and ``base_branch``, and the facts +must be present and well formed. Otherwise the verdict is ``failed``. A proof is +never passed on its own claims. The checker itself never uses the network or a +credential, and it derives nothing from URLs beyond exact equality with the +canonical URLs built from the facts. + +Integrity rules: every required check must be bound to an App; a change to CI +or repository configuration paths needs explicit human acceptance; the head +commit needs an independent approval (latest decisive review, not the author, +not a bot); every fact entry that names a commit must name the head commit; and +approval only by configured operators adds a non-blocking +``harnessy.operator_approved`` warning. """ from __future__ import annotations @@ -20,13 +30,31 @@ import re import stat import sys -from urllib.parse import urlsplit PROOF_PATH = "proof.json" PROOF_SCHEMA = "harnessy.work.proof.v1" MAX_PROOF_BYTES = 65_536 CHECK_ID = "harnessy.proof" +GITHUB_FACTS_INPUT_KEY = "github_facts" +GITHUB_FACTS_SCHEMA = "workstream.github_facts.v1" +DEFAULT_PROTECTED_PATHS = (".github/**", "**/CODEOWNERS", ".gitlab-ci.yml", ".circleci/**") +DEFAULT_REQUIRED_INDEPENDENT_APPROVALS = 1 +OPERATOR_APPROVED_CODE = "harnessy.operator_approved" _COMMIT_SHA = re.compile(r"[0-9a-fA-F]{40}") +_LOWER_SHA = re.compile(r"[0-9a-f]{40}") +_APPROVAL_STATES = frozenset({"APPROVED", "CHANGES_REQUESTED", "DISMISSED"}) +_MAX_CHECK_NAME_CHARACTERS = 200 +_MAX_LISTED_PATHS = 5 +_CONFIGURATION_KEYS = frozenset( + { + "github_repository", + "base_branch", + "allow_unbound_required_checks", + "protected_paths", + "required_independent_approvals", + "operator_logins", + } +) def _finding(code: str, message: str, *, level: str = "error") -> dict[str, object]: @@ -40,19 +68,13 @@ def _finding(code: str, message: str, *, level: str = "error") -> dict[str, obje } -def _github_https_url(value: object) -> bool: - """Accept only an https://github.com/... URL without credentials or port.""" - if not isinstance(value, str): - return False - try: - parts = urlsplit(value) - except ValueError: - return False - return parts.scheme == "https" and parts.netloc == "github.com" and parts.path not in ("", "/") - - -def evaluate_proof(raw: bytes | None) -> list[dict[str, object]]: - """Return error findings for one proof document; an empty list passes.""" +def evaluate_proof( + raw: bytes | None, + *, + configuration: object, + input_value: object, +) -> list[dict[str, object]]: + """Return findings for one proof; it passes only when it matches GitHub facts.""" if raw is None: return [_finding("missing", "The submission must contain proof.json at its root.")] if len(raw) > MAX_PROOF_BYTES: @@ -63,15 +85,17 @@ def evaluate_proof(raw: bytes | None) -> list[dict[str, object]]: return [_finding("invalid_json", "proof.json must be valid UTF-8 JSON.")] if not isinstance(proof, dict): return [_finding("invalid_json", "proof.json must contain a JSON object.")] + return claim_findings(proof) + compare_github_facts(proof, configuration, input_value) + + +def claim_findings(proof: dict) -> list[dict[str, object]]: + """Check the claims that need no facts; URLs are checked against the facts.""" findings = [] if proof.get("schema") != PROOF_SCHEMA: findings.append(_finding("schema", f"schema must be {PROOF_SCHEMA!r}.")) commit_sha = proof.get("commit_sha") if not isinstance(commit_sha, str) or _COMMIT_SHA.fullmatch(commit_sha) is None: findings.append(_finding("commit_sha", "commit_sha must be 40 hexadecimal characters.")) - for field in ("pr_url", "ci_run_url"): - if not _github_https_url(proof.get(field)): - findings.append(_finding(field, f"{field} must be an https://github.com/ URL.")) if proof.get("ci_conclusion") != "success": findings.append(_finding("ci_conclusion", "ci_conclusion must be 'success'.")) if proof.get("merged") is not True: @@ -79,15 +103,396 @@ def evaluate_proof(raw: bytes | None) -> list[dict[str, object]]: return findings +def _strings(value: object) -> bool: + """Return whether a value is a list of non-empty strings.""" + return isinstance(value, list) and all(isinstance(item, str) and item for item in value) + + +def github_configuration(configuration: object) -> dict | None: + """Return the normalized configuration, or None when it is absent or invalid. + + Integrity options take safe defaults: unbound required checks are errors, + the default protected paths apply, and one independent approval is needed. + """ + if not isinstance(configuration, dict): + return None + repository = configuration.get("github_repository") + base_branch = configuration.get("base_branch") + allow_unbound = configuration.get("allow_unbound_required_checks", False) + protected = configuration.get("protected_paths", []) + approvals = configuration.get( + "required_independent_approvals", DEFAULT_REQUIRED_INDEPENDENT_APPROVALS + ) + operators = configuration.get("operator_logins", []) + if ( + not set(configuration) <= _CONFIGURATION_KEYS + or not isinstance(repository, str) + or not isinstance(base_branch, str) + or not repository + or not base_branch + or not isinstance(allow_unbound, bool) + or not _strings(protected) + or type(approvals) is not int + or approvals < 1 + or not _strings(operators) + ): + return None + return { + "repository": repository, + "base_branch": base_branch, + "allow_unbound": allow_unbound, + "protected_paths": (*DEFAULT_PROTECTED_PATHS, *protected), + "required_approvals": approvals, + "operators": frozenset(login.casefold() for login in operators), + } + + +def _actor(value: object) -> bool: + """Return whether a value has the GitHub actor fact shape.""" + return ( + isinstance(value, dict) + and isinstance(value.get("login"), str) + and type(value.get("id")) is int + and isinstance(value.get("type"), str) + ) + + +def _optional(value: object, kind: type) -> bool: + """Return whether a value is null or a non-boolean instance of ``kind``.""" + return value is None or (isinstance(value, kind) and type(value) is not bool) + + +def _sha_or_none(value: object) -> bool: + """Return whether a value is null or a lowercase 40-character commit SHA.""" + return value is None or (isinstance(value, str) and _LOWER_SHA.fullmatch(value) is not None) + + +_CHECK_KEYS = frozenset( + { + "name", + "app_id", + "app_bound", + "conclusion", + "check_run_app_id", + "check_run_head_sha", + "check_run_html_url", + "check_run_details_url", + "workflow_run_id", + "commit_status_judged", + "commit_status_sha", + } +) + + +def valid_facts(facts: object) -> bool: + """Accept only the exact ``workstream.github_facts.v1`` field shape.""" + if not isinstance(facts, dict) or facts.get("schema_version") != GITHUB_FACTS_SCHEMA: + return False + checks = facts.get("required_checks") + reviews = facts.get("reviews") + files = facts.get("changed_files") + return ( + isinstance(facts.get("repository"), str) + and type(facts.get("pull_request_number")) is int + and isinstance(facts.get("html_url"), str) + and isinstance(facts.get("merged"), bool) + and _sha_or_none(facts.get("merge_commit_sha")) + and isinstance(facts.get("base_ref"), str) + and isinstance(facts.get("head_sha"), str) + and _LOWER_SHA.fullmatch(facts["head_sha"]) is not None + and _actor(facts.get("author")) + and isinstance(reviews, list) + and all( + isinstance(review, dict) + and type(review.get("id")) is int + and (review.get("reviewer") is None or _actor(review.get("reviewer"))) + and isinstance(review.get("state"), str) + and _sha_or_none(review.get("commit_id")) + for review in reviews + ) + and isinstance(files, list) + and type(facts.get("changed_files_count")) is int + and facts["changed_files_count"] == len(files) + and all( + isinstance(item, dict) + and isinstance(item.get("filename"), str) + and _optional(item.get("previous_filename"), str) + for item in files + ) + and isinstance(checks, list) + and len(checks) > 0 + and all( + isinstance(check, dict) + and set(check) == _CHECK_KEYS + and isinstance(check["name"], str) + and isinstance(check["conclusion"], str) + and isinstance(check["app_bound"], bool) + and isinstance(check["commit_status_judged"], bool) + and _sha_or_none(check["check_run_head_sha"]) + and _sha_or_none(check["commit_status_sha"]) + and _optional(check["workflow_run_id"], int) + for check in checks + ) + ) + + +def _display(value: str) -> str: + """Bound a GitHub-provided name for a contributor-visible message.""" + printable = "".join(character if character.isprintable() else "?" for character in value) + if len(printable) > _MAX_CHECK_NAME_CHARACTERS: + return printable[:_MAX_CHECK_NAME_CHARACTERS] + "..." + return printable + + +def _glob_regex(pattern: str) -> re.Pattern[str]: + """Translate a path glob: ``**`` spans directories, ``*`` and ``?`` do not. + + A leading ``**/`` also matches at the repository root. + """ + parts = [] + index = 0 + while index < len(pattern): + if pattern.startswith("**/", index): + parts.append("(?:.*/)?") + index += 3 + elif pattern.startswith("**", index): + parts.append(".*") + index += 2 + elif pattern[index] == "*": + parts.append("[^/]*") + index += 1 + elif pattern[index] == "?": + parts.append("[^/]") + index += 1 + else: + parts.append(re.escape(pattern[index])) + index += 1 + return re.compile("".join(parts)) + + +def protected_changes(files: list[dict], patterns: tuple[str, ...]) -> list[str]: + """Return changed paths, including rename sources, that match a protected glob.""" + compiled = [_glob_regex(pattern) for pattern in patterns] + matched = [] + for item in files: + for path in (item.get("previous_filename"), item["filename"]): + if path and any(regex.fullmatch(path) for regex in compiled): + matched.append(path) + return sorted(set(matched)) + + +def qualifying_approvals(facts: dict) -> list[str]: + """Return logins whose latest decisive review approves the head commit. + + Each reviewer's latest ``APPROVED``, ``CHANGES_REQUESTED`` or ``DISMISSED`` + review (by review id) decides; comments do not. The approval must name the + head SHA and come from a ``User`` who is not the pull request author. + """ + author = facts["author"] + latest: dict[int, dict] = {} + for review in facts["reviews"]: + reviewer = review["reviewer"] + if reviewer is None or review["state"] not in _APPROVAL_STATES: + continue + current = latest.get(reviewer["id"]) + if current is None or review["id"] > current["id"]: + latest[reviewer["id"]] = review + logins = [] + for review in latest.values(): + reviewer = review["reviewer"] + if ( + review["state"] == "APPROVED" + and review["commit_id"] == facts["head_sha"] + and reviewer["type"] == "User" + and reviewer["id"] != author["id"] + and reviewer["login"].casefold() != author["login"].casefold() + ): + logins.append(reviewer["login"]) + return sorted(logins) + + +def _binding_findings(facts: dict) -> list[dict[str, object]]: + """Every check entry that names a commit must name the head commit.""" + head = facts["head_sha"] + for check in facts["required_checks"]: + for sha in (check["check_run_head_sha"], check["commit_status_sha"]): + if sha is not None and sha != head: + return [ + _finding( + "github.facts_binding", + "The GitHub facts mix results from different commits.", + ) + ] + return [] + + +def _url_findings(proof: dict, facts: dict) -> list[dict[str, object]]: + """Require the claimed URLs to equal the canonical URLs built from the facts. + + ``pr_url`` must equal ``https://github.com/{repository}/pull/{number}`` and + GitHub's own ``html_url``. ``ci_run_url`` must equal + ``https://github.com/{repository}/actions/runs/{id}`` for the workflow run of + at least one judged required check. Nothing else is derived from either URL. + """ + findings = [] + repository = facts["repository"] + canonical_pull = f"https://github.com/{repository}/pull/{facts['pull_request_number']}" + if proof.get("pr_url") != canonical_pull or facts["html_url"] != canonical_pull: + findings.append( + _finding( + "github.pr_url", + f"pr_url must be exactly {canonical_pull}, the pull request GitHub reports.", + ) + ) + run_urls = { + f"https://github.com/{repository}/actions/runs/{check['workflow_run_id']}" + for check in facts["required_checks"] + if check["workflow_run_id"] is not None + } + if proof.get("ci_run_url") not in run_urls: + findings.append( + _finding( + "github.ci_run", + "ci_run_url must be https://github.com//actions/runs/ for a " + "workflow run that produced a required check on the head commit.", + ) + ) + return findings + + +def _integrity_findings(facts: dict, settings: dict) -> list[dict[str, object]]: + """Integrity rules of owner decision 20.""" + findings = [] + if not settings["allow_unbound"]: + for check in facts["required_checks"]: + if not check["app_bound"]: + findings.append( + _finding( + "github.unbound_required_check", + f"Required check {_display(check['name'])} accepts a result from " + "any source; it must be bound to the App that produces it.", + ) + ) + protected = protected_changes(facts["changed_files"], settings["protected_paths"]) + if protected: + listed = ", ".join(_display(path) for path in protected[:_MAX_LISTED_PATHS]) + more = len(protected) - _MAX_LISTED_PATHS + suffix = f" and {more} more" if more > 0 else "" + findings.append( + _finding( + "github.protected_path_changed", + f"The pull request changes CI or repository configuration ({listed}{suffix}); " + "it requires explicit human acceptance and cannot be auto-accepted.", + ) + ) + approvals = qualifying_approvals(facts) + if len(approvals) < settings["required_approvals"]: + findings.append( + _finding( + "github.independent_approval_missing", + f"The head commit needs {settings['required_approvals']} approval(s) from " + "reviewers who are neither the author nor a bot; " + f"GitHub shows {len(approvals)}.", + ) + ) + if approvals and all(login.casefold() in settings["operators"] for login in approvals): + findings.append( + { + **_finding( + "operator_approved", + "Only operator accounts approved this work. It may earn points but may " + "not be paid in money until an independent reviewer confirms it.", + level="warning", + ), + "code": OPERATOR_APPROVED_CODE, + } + ) + return findings + + +def compare_github_facts( + proof: dict, configuration: object, input_value: object +) -> list[dict[str, object]]: + """Compare proof claims with WorkStream-read GitHub facts; one finding per mismatch.""" + settings = github_configuration(configuration) + if settings is None: + return [ + _finding( + "github.configuration", + "The checker configuration must name github_repository and base_branch, " + "with only supported integrity options.", + ) + ] + facts = input_value.get(GITHUB_FACTS_INPUT_KEY) if isinstance(input_value, dict) else None + if facts is None: + return [ + _finding( + "github.facts_missing", + "GitHub facts are required for this check but were not provided.", + ) + ] + if not valid_facts(facts): + return [_finding("github.facts_invalid", "The provided GitHub facts are malformed.")] + binding = _binding_findings(facts) + if binding: + return binding + findings = [] + repository = facts["repository"] + if repository.casefold() != settings["repository"].casefold(): + findings.append( + _finding( + "github.repository", + f"The pull request belongs to {_display(repository)}, " + f"not {_display(settings['repository'])}.", + ) + ) + if facts["base_ref"] != settings["base_branch"]: + findings.append( + _finding( + "github.base_branch", + f"The pull request targets {_display(facts['base_ref'])}, " + f"not {_display(settings['base_branch'])}.", + ) + ) + findings.extend(_url_findings(proof, facts)) + if facts["merged"] is not True: + findings.append( + _finding("github.merged", "GitHub reports that the pull request is not merged.") + ) + commit_sha = proof.get("commit_sha") + if not isinstance(commit_sha, str) or facts["merge_commit_sha"] != commit_sha.lower(): + findings.append( + _finding( + "github.merge_commit_sha", + "commit_sha does not equal the pull request's merge commit on GitHub.", + ) + ) + for check in facts["required_checks"]: + if check["conclusion"] != "success": + findings.append( + _finding( + "github.required_check", + f"Required check {_display(check['name'])} is " + f"{_display(check['conclusion'])}, not success, " + "for the pull request head commit.", + ) + ) + findings.extend(_integrity_findings(facts, settings)) + return findings + + def read_proof(input_root: Path) -> bytes | None: """Read at most one byte past the limit from a regular, non-linked proof file.""" try: descriptor = os.open(input_root / PROOF_PATH, os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK) except OSError: return None + # Check the type before wrapping: ``fdopen`` raises for a directory, and a + # contributor-controlled archive must yield a finding, not a crash. + if not stat.S_ISREG(os.fstat(descriptor).st_mode): + os.close(descriptor) + return None with os.fdopen(descriptor, "rb") as handle: - if not stat.S_ISREG(os.fstat(handle.fileno()).st_mode): - return None return handle.read(MAX_PROOF_BYTES + 1) @@ -118,7 +523,12 @@ def build_result(request: dict, findings: list[dict[str, object]]) -> dict[str, def main() -> int: request = json.load(sys.stdin) - result = build_result(request, evaluate_proof(read_proof(Path("/work/input")))) + findings = evaluate_proof( + read_proof(Path("/work/input")), + configuration=request.get("configuration"), + input_value=request.get("input"), + ) + result = build_result(request, findings) sys.stdout.write(_canonical(result)) return 0 diff --git a/external_checkers/harnessy_proof/publish_local.py b/external_checkers/harnessy_proof/publish_local.py index 4521a6a00..e74bbf06b 100644 --- a/external_checkers/harnessy_proof/publish_local.py +++ b/external_checkers/harnessy_proof/publish_local.py @@ -6,8 +6,9 @@ platform manifest, pulls that digest so the daemon records it, and writes: - ``checker-service.json``: the Rust checker-service configuration; -- ``registry-pre.json`` and ``registry-post.json``: ``ExternalCheckerRegistrySpec`` - bodies for ``POST /api/v1/operations/external-checker-registry``. +- ``registry-post.json``: the ``ExternalCheckerRegistrySpec`` body for + ``POST /api/v1/operations/external-checker-registry``. The checker is + post-submission only: it needs GitHub facts, which PRE never has. It uses only the Python standard library and the Docker CLI. """ @@ -29,10 +30,8 @@ "application/vnd.docker.distribution.manifest.v2+json", ) CAPABILITY_ID = "harnessy.work_proof" -INPUT_SCHEMA_IDS = { - "pre_submit": "external_checker_pre_submit_input", - "post_submit": "external_checker_post_submit_input", -} +PHASE = "post_submit" +INPUT_SCHEMA_ID = "external_checker_post_submit_input" # The socket stays on a named volume: Docker Desktop host-shared paths refuse # chmod on Unix sockets, and AF_UNIX paths must stay within 107 bytes. SOCKET_PATH = "/run/ws-checker/checker.sock" @@ -50,13 +49,53 @@ def model_relay_root(compose_project: str) -> str: def _hash(value: object) -> str: - """Canonical JSON digest for the string-only schema documents used here.""" + """Canonical JSON digest for the string/integer/object schema documents used here.""" encoded = json.dumps(value, ensure_ascii=False, sort_keys=True, separators=(",", ":")) return "sha256:" + hashlib.sha256(encoded.encode("utf-8")).hexdigest() -def _schema(schema_id: str) -> dict[str, object]: - document = {"type": "object"} +# One fail-closed mode: the repository and base branch are required. Integrity +# options are optional and default to the safe values. +CONFIGURATION_SCHEMA: dict[str, object] = { + "type": "object", + "additionalProperties": False, + "required": ["github_repository", "base_branch"], + "properties": { + "github_repository": { + "type": "string", + "pattern": "^[A-Za-z0-9][A-Za-z0-9-]{0,38}/[A-Za-z0-9._-]{1,100}$", + }, + "base_branch": {"type": "string", "minLength": 1, "maxLength": 255}, + "allow_unbound_required_checks": {"type": "boolean"}, + "protected_paths": { + "type": "array", + "maxItems": 100, + "items": {"type": "string", "minLength": 1, "maxLength": 1000}, + }, + "required_independent_approvals": {"type": "integer", "minimum": 1, "maximum": 10}, + "operator_logins": { + "type": "array", + "maxItems": 100, + "items": {"type": "string", "minLength": 1, "maxLength": 100}, + }, + }, +} +# WorkStream adds GitHub facts to the normalized POST input. A missing facts +# object reaches the checker, which reports github.facts_missing. +INPUT_SCHEMA: dict[str, object] = { + "type": "object", + "properties": { + "github_facts": { + "type": "object", + "required": ["schema_version"], + "properties": {"schema_version": {"const": "workstream.github_facts.v1"}}, + } + }, +} + + +def _schema(schema_id: str, document: dict[str, object] | None = None) -> dict[str, object]: + document = {"type": "object"} if document is None else document return { "schema_id": schema_id, "schema_version": "v1", @@ -65,15 +104,17 @@ def _schema(schema_id: str) -> dict[str, object]: } -def registry_spec(phase: str, image_digest: str, capability_version: str) -> dict[str, object]: - """One immutable registry body; PRE and POST share the image and limits.""" +def registry_spec(image_digest: str, capability_version: str) -> dict[str, object]: + """The immutable post-submission registry body.""" return { "capability_id": CAPABILITY_ID, "capability_version": capability_version, - "phase": phase, + "phase": PHASE, "image_digest": image_digest, - "configuration_schema": _schema("harnessy.work_proof.configuration"), - "input_schema": _schema(INPUT_SCHEMA_IDS[phase]), + "configuration_schema": _schema( + "harnessy.work_proof.configuration", CONFIGURATION_SCHEMA + ), + "input_schema": _schema(INPUT_SCHEMA_ID, INPUT_SCHEMA), "output_schema": _schema("external_checker_result"), "resources": { "cpu_millis": 500, @@ -177,8 +218,9 @@ def main(argv: list[str] | None = None) -> int: help="emit fixed local proxy and daemon-visible relay paths", ) # The registry holds one immutable entry per phase, capability and version, - # so a rebuilt image (new digest) needs a new version. - parser.add_argument("--capability-version", default="v1") + # so a rebuilt image (new digest) needs a new version. v2 compares the + # proof with GitHub facts and is post-submission only. + parser.add_argument("--capability-version", default="v2") args = parser.parse_args(argv) image = publish(args.registry, args.repository) args.out_dir.mkdir(parents=True, exist_ok=True) @@ -190,11 +232,8 @@ def main(argv: list[str] | None = None) -> int: sandbox_gid=args.sandbox_gid, enable_model_proxy=args.enable_model_proxy, ), - "registry-pre.json": registry_spec( - "pre_submit", image["platform_manifest_digest"], args.capability_version - ), "registry-post.json": registry_spec( - "post_submit", image["platform_manifest_digest"], args.capability_version + image["platform_manifest_digest"], args.capability_version ), } for name, value in outputs.items():