From 4d9173b261c600c51b969f5796ac0d138d9f67c1 Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 13:40:56 +0100 Subject: [PATCH 1/4] feat(checker): compare Harnessy proof with GitHub facts harnessy.work_proof v2 compares proof.json with workstream.github_facts.v1 facts from the request input when its binding configuration names a GitHub repository and base branch, one error finding per mismatch. Missing required facts fail. Publish real configuration/input schemas. Co-Authored-By: Claude Opus 5.5 --- .../WS-GHPROOF-001/WS-GHPROOF-001-03.md | 114 ++++++++++ .../external/test_harnessy_proof_checker.py | 212 ++++++++++++++++++ docs/engineering/local-agent-loop.md | 76 ++++++- external_checkers/harnessy_proof/checker.py | 196 +++++++++++++++- .../harnessy_proof/publish_local.py | 54 ++++- 5 files changed, 628 insertions(+), 24 deletions(-) create mode 100644 .commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md 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..dd768acb3 --- /dev/null +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md @@ -0,0 +1,114 @@ +# WS-GHPROOF-001-03 — Harnessy proof checker GitHub comparison mode + +- Initiative: WS-GHPROOF-001 +- Durable disposition: Complete +- Intended merge outcome: The networkless `harnessy.work_proof` checker compares `proof.json` with WorkStream-supplied `workstream.github_facts.v1` facts when its binding configuration names a GitHub repository and base branch, reporting one error finding per mismatch, and its v2 registry bodies validate that configuration. + +## Intent + +A hand-written `proof.json` passes today. Once WorkStream supplies GitHub facts +(chunk 04), this mode makes a faked proof fail, while the checker stays +networkless and credential-free. + +## 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 + `{"type": "object"}` configuration and input schemas, default version `v1`. +- `docs/engineering/local-agent-loop.md` documents shape mode 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` sections on publishing and on what the + proof checker checks. + +### 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 + +- **Input shape.** WorkStream places the facts object in the request `input` + under `github_facts`, beside the normalized POST input fields, so the request + digest binds them. The object is exactly chunk 02's + `GitHubPullRequestFacts.model_dump(mode="json")`: `schema_version`, + `repository`, `pull_request_number`, `state`, `merged`, `merge_commit_sha` + (null unless merged), `base_ref`, `head_sha`, `required_checks` + (`name`, `app_id`, `conclusion`), `fetched_at` and `responses`. +- **Mode selection.** Configuration `{}` with no facts is shape mode, unchanged. + Configuration `{"github_repository": "owner/name", "base_branch": ""}` + requires facts. Facts without configuration are compared except the + configured repository and base branch. +- **Findings.** Shape rules still run. Each mismatch is one `error` finding with + code `harnessy.proof.github.`: `repository`, `base_branch`, `pr_url`, + `pr_repository`, `pull_request_number`, `merged`, `merge_commit_sha`, one + `required_check` per non-success check, `facts_missing`, `facts_invalid` and + `configuration`. Repository comparisons are case-insensitive, as GitHub + names are; the commit SHA is compared in lower case. GitHub-provided check + names in messages are bounded to 200 printable characters. +- **Fail closed.** Required but missing or malformed facts produce an error + finding and a `failed` verdict, never a pass. Chunk 04 maps a facts-fetch + failure to an infrastructure failure before the checker runs, so a GitHub + outage does not become a contributor-visible failure. +- **Registry schemas.** POST `configuration_schema` accepts `{}` or both keys + together (`dependentRequired`, no other keys, repository pattern). PRE accepts + only `{}` because PRE never has facts. POST `input_schema` constrains + `github_facts.schema_version` when present; the checker validates the rest. +- **Version.** `publish_local.py` defaults to capability version `v2`, because + the registry entry for `v1` is immutable and the behavior and schemas changed. +- Rejected: a second ART mount for facts (owner decision), and making + `github_facts` required in the POST input schema (it would break shape-mode + bindings). + +## Acceptance criteria + +- [x] A proof matching the facts passes. +- [x] Each mismatch (repository, PR repository, PR number, PR URL form, merged, + merge commit SHA, base branch) reports exactly its finding. +- [x] Each failed, missing or pending required check reports its own finding. +- [x] Required facts that are missing or malformed fail; incomplete + configuration fails. +- [x] Shape mode is unchanged; `main()` passes configuration and input through. +- [x] Published v2 registry bodies validate through + `ExternalCheckerRegistrySpec` and accept or reject 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 on missing facts, and + strict `success` for required checks. + +## Evidence + +| Claim | Command or proof | Result | Remaining uncertainty | +|---|---|---|---| +| Comparison rules and result contract | `pytest tests/checkers/external/test_harnessy_proof_checker.py` | 56 passed | Facts shape agreement with chunk 02 is documented, not cross-tested; chunk 04 should add that test | +| 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 + +None recorded. + +## Reconciliation + +- Current-source reconciliation: based on WS-GHPROOF-001-01 over `main` after + PR #674; independent of chunk 02. +- Roadmap: no impact. The local agent loop keeps shape mode until chunk 04 + supplies facts, so its stated trust limit is unchanged. +- Next usable boundary: chunk 04 POST wiring. +- Remaining risks: until chunk 04, configuring comparison mode fails every POST + check 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 349db03b8..7b7eb6a5b 100644 --- a/backend/tests/checkers/external/test_harnessy_proof_checker.py +++ b/backend/tests/checkers/external/test_harnessy_proof_checker.py @@ -3,6 +3,7 @@ from __future__ import annotations import importlib.util +import io import json from pathlib import Path from uuid import uuid4 @@ -11,6 +12,7 @@ import yaml from app.modules.checkers.api.external import ( + ExternalCheckerContractError, ExternalCheckerExecutionResult, ExternalCheckerRegistrySpec, ) @@ -179,3 +181,213 @@ def test_published_service_paths_match_the_compose_overlay() -> None: ] mode = ":ro" if name in ("backend", "worker", "beat") else "" assert socket_mounts == [f"checker_socket:/run/ws-checker{mode}"], name + + +# GitHub comparison mode ------------------------------------------------------- + +CONFIGURATION = {"github_repository": "acme/widgets", "base_branch": "main"} +FACTS = { + "schema_version": "workstream.github_facts.v1", + "repository": "acme/widgets", + "pull_request_number": 12, + "state": "closed", + "merged": True, + "merge_commit_sha": VALID_PROOF["commit_sha"], + "base_ref": "main", + "head_sha": "e" * 40, + "required_checks": [ + {"name": "backend", "app_id": 15368, "conclusion": "success"}, + {"name": "lint", "app_id": None, "conclusion": "success"}, + ], + "fetched_at": "2026-10-24T12:00:00Z", + "responses": [{"request": "pull_request", "sha256": "sha256:" + "f" * 64}], +} + + +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 _compare( + proof: bytes | None = None, + *, + configuration: object = CONFIGURATION, + input_value: object = None, +) -> list[str]: + findings = checker.evaluate_proof( + _encoded() if proof is None else proof, + configuration=configuration, + input_value=_input() if input_value is None else input_value, + ) + return [item["code"] for item in findings] + + +def test_proof_matching_github_facts_passes() -> None: + assert _compare() == [] + upper = VALID_PROOF["commit_sha"].upper() + pr_url = "https://github.com/Acme/Widgets/pull/12/" + assert _compare(_encoded(commit_sha=upper, pr_url=pr_url)) == [] + + +@pytest.mark.parametrize( + ("proof_changes", "fact_changes", "code"), + [ + ( + {"pr_url": "https://github.com/acme/other/pull/12"}, + {"repository": "acme/other"}, + "github.repository", + ), + ({"pr_url": "https://github.com/acme/other/pull/12"}, {}, "github.pr_repository"), + ( + {"pr_url": "https://github.com/acme/widgets/pull/13"}, + {}, + "github.pull_request_number", + ), + ({"pr_url": "https://github.com/acme/widgets/issues/12"}, {}, "github.pr_url"), + ({}, {"merged": False, "state": "open"}, "github.merged"), + ({"commit_sha": "1" * 40}, {}, "github.merge_commit_sha"), + ({}, {"base_ref": "release"}, "github.base_branch"), + ], +) +def test_each_github_mismatch_reports_exactly_its_finding( + proof_changes: dict, fact_changes: dict, code: str +) -> None: + assert _compare(_encoded(**proof_changes), input_value=_input(**fact_changes)) == [ + f"harnessy.proof.{code}" + ] + + +def test_unmerged_facts_also_fail_the_merge_commit_comparison() -> None: + codes = _compare(input_value=_input(merged=False, merge_commit_sha=None)) + assert codes == ["harnessy.proof.github.merged", "harnessy.proof.github.merge_commit_sha"] + + +def test_each_unsuccessful_required_check_reports_its_own_finding() -> None: + checks = [ + {"name": "backend", "app_id": 15368, "conclusion": "failure"}, + {"name": "lint", "app_id": None, "conclusion": "success"}, + {"name": "docs", "app_id": None, "conclusion": "missing"}, + {"name": "e2e\nspoof", "app_id": None, "conclusion": "pending"}, + ] + findings = checker.evaluate_proof( + _encoded(), configuration=CONFIGURATION, input_value=_input(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] + + +@pytest.mark.parametrize( + "input_value", + [{}, {"schema_version": "workstream.external_checker_post_submit_input.v1"}, None], +) +def test_required_facts_missing_is_an_error_never_a_pass(input_value: object) -> None: + findings = checker.evaluate_proof( + _encoded(), configuration=CONFIGURATION, input_value=input_value + ) + assert [item["code"] for item in findings] == ["harnessy.proof.github.facts_missing"] + + +@pytest.mark.parametrize( + "facts_changes", + [ + {"schema_version": "workstream.github_facts.v2"}, + {"merged": "true"}, + {"pull_request_number": "12"}, + {"required_checks": []}, + {"required_checks": [{"name": "backend"}]}, + {"merge_commit_sha": "ABC"}, + {"head_sha": ...}, + ], +) +def test_malformed_facts_are_an_error(facts_changes: dict) -> None: + assert _compare(input_value=_input(**facts_changes)) == [ + "harnessy.proof.github.facts_invalid" + ] + + +@pytest.mark.parametrize( + "configuration", + [ + {"github_repository": "acme/widgets"}, + {"base_branch": "main"}, + {**CONFIGURATION, "extra": True}, + {"github_repository": "", "base_branch": "main"}, + ], +) +def test_incomplete_configuration_is_an_error(configuration: dict) -> None: + assert _compare(configuration=configuration) == ["harnessy.proof.github.configuration"] + + +def test_shape_mode_is_unchanged_without_configuration_or_facts() -> None: + assert checker.evaluate_proof(_encoded(), configuration={}, input_value={}) == [] + assert checker.evaluate_proof(_encoded(merged=False)) == [ + checker._finding("merged", "merged must be the JSON value true.") + ] + + +def test_unconfigured_facts_are_still_compared() -> None: + codes = _compare(configuration={}, input_value=_input(base_ref="other", merged=False)) + assert codes == ["harnessy.proof.github.merged"] + + +def test_shape_errors_and_github_mismatches_are_both_reported() -> None: + codes = _compare(_encoded(ci_conclusion="failure"), input_value=_input(merged=False)) + assert codes == ["harnessy.proof.ci_conclusion", "harnessy.proof.github.merged"] + + +def test_comparison_failure_produces_a_backend_valid_failed_result() -> None: + findings = checker.evaluate_proof( + _encoded(), configuration=CONFIGURATION, input_value=_input(base_ref="dev") + ) + parsed = ExternalCheckerExecutionResult.model_validate_json( + json.dumps(checker.build_result(_request(), findings)) + ) + assert parsed.verdict == "failed" + assert [item.code for item in parsed.findings] == ["harnessy.proof.github.base_branch"] + + +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"] + + +def test_registry_schemas_accept_only_supported_configuration_and_facts_input() -> None: + digest = "sha256:" + "d" * 64 + post = ExternalCheckerRegistrySpec.model_validate( + publish_local.registry_spec("post_submit", digest, "v2") + ) + pre = ExternalCheckerRegistrySpec.model_validate( + publish_local.registry_spec("pre_submit", digest, "v2") + ) + post.configuration_schema.validate_instance({}) + post.configuration_schema.validate_instance(CONFIGURATION) + post.input_schema.validate_instance(_input()) + post.input_schema.validate_instance( + {"schema_version": "workstream.external_checker_post_submit_input.v1"} + ) + pre.configuration_schema.validate_instance({}) + rejected = [ + (post.configuration_schema, {"github_repository": "acme/widgets"}), + (post.configuration_schema, {"base_branch": "main"}), + (post.configuration_schema, {**CONFIGURATION, "github_repository": "acme"}), + (post.configuration_schema, {**CONFIGURATION, "extra": 1}), + (pre.configuration_schema, CONFIGURATION), + (post.input_schema, _input(schema_version="workstream.github_facts.v2")), + ] + for schema, value in rejected: + with pytest.raises(ExternalCheckerContractError): + schema.validate_instance(value) diff --git a/docs/engineering/local-agent-loop.md b/docs/engineering/local-agent-loop.md index 5a51aee78..cde07efa5 100644 --- a/docs/engineering/local-agent-loop.md +++ b/docs/engineering/local-agent-loop.md @@ -120,7 +120,7 @@ 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 +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 two new bodies. Registering a new digest under an existing version returns `external_checker_registry_conflict`. @@ -231,14 +231,72 @@ error finding exists: | `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. +**Trust caveat.** The checker has no network access. With an empty binding +configuration (`{}`, shape mode) 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 shape mode alone to gate acceptance that +leads to compensation; pair it with human review, or use GitHub comparison +mode once WorkStream supplies GitHub facts, when the claim must be true. + +### GitHub comparison mode + +Capability version `v2` adds a comparison mode +([WS-GHPROOF-001](../../.commitrail/initiatives/WS-GHPROOF-001/OVERVIEW.md)). +The POST binding configuration names the repository and base branch: + +```json +{"github_repository": "Flow-Research/harnessy-v2", "base_branch": "main"} +``` + +The registry `configuration_schema` requires both keys together; the PRE +schema accepts only `{}`. WorkStream, not the checker, reads the facts from +GitHub and places them in the request `input` under `github_facts`, so the +request digest binds them: + +```json +{ + "github_facts": { + "schema_version": "workstream.github_facts.v1", + "repository": "Flow-Research/harnessy-v2", + "pull_request_number": 12, + "state": "closed", + "merged": true, + "merge_commit_sha": "<40 lowercase hex, or null unless merged>", + "base_ref": "main", + "head_sha": "<40 lowercase hex>", + "required_checks": [{"name": "backend", "app_id": 15368, "conclusion": "success"}], + "fetched_at": "2026-10-24T12:00:00Z", + "responses": [{"request": "pull_request", "sha256": "sha256:<64 hex>"}] + } +} +``` + +`required_checks` lists every check required by the base branch's protection, +with its result for the head commit (`missing` and `pending` included). In +this mode the shape rules above still apply, and each mismatch adds one `error` +finding: + +| Code (`harnessy.proof.github.*`) | Fails when | +|---|---| +| `repository` | the facts' repository is not `github_repository` | +| `base_branch` | the pull request's base branch is not `base_branch` | +| `pr_url` | `pr_url` is not `https://github.com///pull/` | +| `pr_repository` | `pr_url` names another repository than the facts | +| `pull_request_number` | `pr_url` names another pull request than the facts | +| `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` | +| `facts_missing` | the configuration requires facts and the input has none | +| `facts_invalid` | the facts do not have the `workstream.github_facts.v1` shape | +| `configuration` | the configuration is neither `{}` nor both keys | + +Facts present without configuration are still compared, except repository and +base branch. Until WorkStream's POST evaluation supplies facts (initiative +chunk 04), configuring comparison mode makes every POST check fail with +`facts_missing`; this stack therefore keeps shape mode. ## Create the manager project and the agent submitter diff --git a/external_checkers/harnessy_proof/checker.py b/external_checkers/harnessy_proof/checker.py index 66249f07b..1aba30d09 100644 --- a/external_checkers/harnessy_proof/checker.py +++ b/external_checkers/harnessy_proof/checker.py @@ -4,11 +4,19 @@ 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". +Shape mode (empty binding configuration, no facts in the request input): the +checker 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. A passing +verdict means "well-formed proof claims", not "independently verified work". + +GitHub comparison mode: the binding configuration names +``{"github_repository": "owner/name", "base_branch": "main"}``, and WorkStream +places ``workstream.github_facts.v1`` facts it read from GitHub in the request +input under ``github_facts``. The checker compares every claim with those facts +and reports one error finding per mismatch. Missing or malformed facts when the +configuration requires them are an error finding, never a pass. Facts present +without configuration are still compared, except the configured repository and +base branch. The checker itself never uses the network or a credential. """ from __future__ import annotations @@ -26,7 +34,14 @@ 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" _COMMIT_SHA = re.compile(r"[0-9a-fA-F]{40}") +_LOWER_SHA = re.compile(r"[0-9a-f]{40}") +_PULL_PATH = re.compile( + r"/([A-Za-z0-9][A-Za-z0-9-]{0,38})/([A-Za-z0-9._-]{1,100})/pull/([1-9][0-9]{0,9})" +) +_MAX_CHECK_NAME_CHARACTERS = 200 def _finding(code: str, message: str, *, level: str = "error") -> dict[str, object]: @@ -51,8 +66,17 @@ def _github_https_url(value: object) -> bool: 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 = None, + input_value: object = None, +) -> list[dict[str, object]]: + """Return error findings for one proof document; an empty list passes. + + ``configuration`` is the locked binding configuration and ``input_value`` the + request input. Together they select shape mode or GitHub comparison mode. + """ if raw is None: return [_finding("missing", "The submission must contain proof.json at its root.")] if len(raw) > MAX_PROOF_BYTES: @@ -76,6 +100,157 @@ def evaluate_proof(raw: bytes | None) -> list[dict[str, object]]: findings.append(_finding("ci_conclusion", "ci_conclusion must be 'success'.")) if proof.get("merged") is not True: findings.append(_finding("merged", "merged must be the JSON value true.")) + findings.extend(compare_github_facts(proof, configuration, input_value)) + return findings + + +def _github_configuration(configuration: object) -> tuple[str, str] | None | bool: + """Return (repository, base branch), None for shape mode, or False if invalid.""" + if not isinstance(configuration, dict) or not configuration: + return None + repository = configuration.get("github_repository") + base_branch = configuration.get("base_branch") + if ( + set(configuration) != {"github_repository", "base_branch"} + or not isinstance(repository, str) + or not isinstance(base_branch, str) + or not repository + or not base_branch + ): + return False + return repository, base_branch + + +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") + merge_sha = facts.get("merge_commit_sha") + return ( + isinstance(facts.get("repository"), str) + and type(facts.get("pull_request_number")) is int + and isinstance(facts.get("merged"), bool) + and ( + merge_sha is None + or (isinstance(merge_sha, str) and _LOWER_SHA.fullmatch(merge_sha) is not None) + ) + 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 isinstance(checks, list) + and len(checks) > 0 + and all( + isinstance(check, dict) + and isinstance(check.get("name"), str) + and isinstance(check.get("conclusion"), str) + 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 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.""" + configured = _github_configuration(configuration) + if configured is False: + return [ + _finding( + "github.configuration", + "The checker configuration must name github_repository and base_branch.", + ) + ] + facts = input_value.get(GITHUB_FACTS_INPUT_KEY) if isinstance(input_value, dict) else None + if facts is None: + if configured is None: + return [] + 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.")] + findings = [] + repository = facts["repository"] + if configured is not None: + expected_repository, expected_base = configured + if repository.casefold() != expected_repository.casefold(): + findings.append( + _finding( + "github.repository", + f"The pull request belongs to {_display(repository)}, " + f"not {_display(expected_repository)}.", + ) + ) + if facts["base_ref"] != expected_base: + findings.append( + _finding( + "github.base_branch", + f"The pull request targets {_display(facts['base_ref'])}, " + f"not {_display(expected_base)}.", + ) + ) + pr_url = proof.get("pr_url") + match = ( + _PULL_PATH.fullmatch(urlsplit(pr_url).path.rstrip("/")) + if _github_https_url(pr_url) + else None + ) + if match is None: + findings.append( + _finding( + "github.pr_url", + "pr_url must be https://github.com///pull/.", + ) + ) + else: + if f"{match.group(1)}/{match.group(2)}".casefold() != repository.casefold(): + findings.append( + _finding( + "github.pr_repository", + "pr_url names a different repository than the GitHub facts.", + ) + ) + if int(match.group(3)) != facts["pull_request_number"]: + findings.append( + _finding( + "github.pull_request_number", + "pr_url names a different pull request than the GitHub 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.", + ) + ) return findings @@ -118,7 +293,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 1f3647b8c..9c7b4dc8e 100644 --- a/external_checkers/harnessy_proof/publish_local.py +++ b/external_checkers/harnessy_proof/publish_local.py @@ -44,13 +44,50 @@ def material_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"} +# PRE has no GitHub facts, so only shape mode is valid there. POST accepts shape +# mode ({}) or GitHub comparison mode naming both repository and base branch. +CONFIGURATION_SCHEMAS: dict[str, dict[str, object]] = { + "pre_submit": {"type": "object", "additionalProperties": False, "maxProperties": 0}, + "post_submit": { + "type": "object", + "additionalProperties": False, + "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}, + }, + "dependentRequired": { + "github_repository": ["base_branch"], + "base_branch": ["github_repository"], + }, + }, +} +# WorkStream adds GitHub facts to the normalized POST input only for bindings in +# GitHub comparison mode. The checker validates the full facts shape itself. +INPUT_SCHEMAS: dict[str, dict[str, object]] = { + "pre_submit": {"type": "object"}, + "post_submit": { + "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", @@ -66,8 +103,10 @@ def registry_spec(phase: str, image_digest: str, capability_version: str) -> dic "capability_version": capability_version, "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_SCHEMAS[phase] + ), + "input_schema": _schema(INPUT_SCHEMA_IDS[phase], INPUT_SCHEMAS[phase]), "output_schema": _schema("external_checker_result"), "resources": { "cpu_millis": 500, @@ -147,8 +186,9 @@ def main(argv: list[str] | None = None) -> int: parser.add_argument("--sandbox-gid", type=int, required=True) parser.add_argument("--out-dir", type=Path, required=True) # 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 adds GitHub + # comparison mode and real configuration/input schemas. + 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) From 42d8b13abad0dd9d2c891d3d6159972c3625e72e Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 15:29:28 +0100 Subject: [PATCH 2/4] feat(checker): enforce GitHub proof integrity rules Owner decision 20: unbound required checks, CI/config path changes and a missing independent head-commit approval each fail with their own finding; operator-only approval adds a non-blocking harnessy.operator_approved warning. Safe defaults, configurable through the locked configuration. Co-Authored-By: Claude Opus 5.5 --- .../WS-GHPROOF-001/WS-GHPROOF-001-03.md | 29 +- .../external/test_harnessy_proof_checker.py | 257 +++++++++++++++++- docs/engineering/local-agent-loop.md | 30 +- external_checkers/harnessy_proof/checker.py | 231 +++++++++++++++- .../harnessy_proof/publish_local.py | 24 +- 5 files changed, 547 insertions(+), 24 deletions(-) diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md index dd768acb3..13265cec6 100644 --- a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md @@ -41,8 +41,10 @@ networkless and credential-free. digest binds them. The object is exactly chunk 02's `GitHubPullRequestFacts.model_dump(mode="json")`: `schema_version`, `repository`, `pull_request_number`, `state`, `merged`, `merge_commit_sha` - (null unless merged), `base_ref`, `head_sha`, `required_checks` - (`name`, `app_id`, `conclusion`), `fetched_at` and `responses`. + (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`, `commit_status_judged`), + `fetched_at` and `responses`. - **Mode selection.** Configuration `{}` with no facts is shape mode, unchanged. Configuration `{"github_repository": "owner/name", "base_branch": ""}` requires facts. Facts without configuration are compared except the @@ -54,6 +56,23 @@ networkless and credential-free. `configuration`. Repository comparisons are case-insensitive, as GitHub names are; the commit SHA is compared in lower case. GitHub-provided check names in messages are bounded to 200 printable characters. +- **Integrity rules (owner decision 20, 2026-10-24).** They apply whenever + facts are compared, with safe defaults, each with its own finding: + `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 `info` is used because contributor and + history reads count only blocking, warning and coverage findings, so an + `info` finding would be invisible. - **Fail closed.** Required but missing or malformed facts produce an error finding and a `failed` verdict, never a pass. Chunk 04 maps a facts-fetch failure to an infrastructure failure before the checker runs, so a GitHub @@ -76,6 +95,10 @@ networkless and credential-free. - [x] Each failed, missing or pending required check reports its own finding. - [x] Required facts that are missing or malformed fail; incomplete configuration fails. +- [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, operator-only approval, and missing integrity facts. - [x] Shape mode is unchanged; `main()` passes configuration and input through. - [x] Published v2 registry bodies validate through `ExternalCheckerRegistrySpec` and accept or reject configuration and input as @@ -93,7 +116,7 @@ networkless and credential-free. | Claim | Command or proof | Result | Remaining uncertainty | |---|---|---|---| -| Comparison rules and result contract | `pytest tests/checkers/external/test_harnessy_proof_checker.py` | 56 passed | Facts shape agreement with chunk 02 is documented, not cross-tested; chunk 04 should add that test | +| Comparison rules and result contract | `pytest tests/checkers/external/test_harnessy_proof_checker.py` | 91 passed | Facts shape agreement with chunk 02 is documented, not cross-tested; chunk 04 should add that test | | Lint | `ruff check` on the test module and `external_checkers/harnessy_proof/` | Pass | — | | Docs | Markdown link and stale wording checks | Pass | — | diff --git a/backend/tests/checkers/external/test_harnessy_proof_checker.py b/backend/tests/checkers/external/test_harnessy_proof_checker.py index 7b7eb6a5b..18777995f 100644 --- a/backend/tests/checkers/external/test_harnessy_proof_checker.py +++ b/backend/tests/checkers/external/test_harnessy_proof_checker.py @@ -186,6 +186,7 @@ def test_published_service_paths_match_the_compose_overlay() -> None: # GitHub comparison mode ------------------------------------------------------- CONFIGURATION = {"github_repository": "acme/widgets", "base_branch": "main"} +HEAD = "e" * 40 FACTS = { "schema_version": "workstream.github_facts.v1", "repository": "acme/widgets", @@ -194,10 +195,36 @@ def test_published_service_paths_match_the_compose_overlay() -> None: "merged": True, "merge_commit_sha": VALID_PROOF["commit_sha"], "base_ref": "main", - "head_sha": "e" * 40, + "head_sha": HEAD, + "author": {"login": "agent-bot", "id": 501, "type": "Bot"}, + "reviews": [ + { + "id": 61, + "reviewer": {"login": "reviewer", "id": 7, "type": "User"}, + "state": "APPROVED", + "commit_id": HEAD, + "submitted_at": "2026-10-24T11:00:00Z", + } + ], + "changed_files_count": 1, + "changed_files": [{"filename": "src/app.py", "previous_filename": None, "status": "modified"}], "required_checks": [ - {"name": "backend", "app_id": 15368, "conclusion": "success"}, - {"name": "lint", "app_id": None, "conclusion": "success"}, + { + "name": "backend", + "app_id": 15368, + "app_bound": True, + "conclusion": "success", + "check_run_app_id": 15368, + "commit_status_judged": False, + }, + { + "name": "lint", + "app_id": 15368, + "app_bound": True, + "conclusion": "success", + "check_run_app_id": 15368, + "commit_status_judged": False, + }, ], "fetched_at": "2026-10-24T12:00:00Z", "responses": [{"request": "pull_request", "sha256": "sha256:" + "f" * 64}], @@ -267,11 +294,12 @@ def test_unmerged_facts_also_fail_the_merge_commit_comparison() -> None: def test_each_unsuccessful_required_check_reports_its_own_finding() -> None: + bound = {"app_id": 1, "app_bound": True, "check_run_app_id": 1, "commit_status_judged": False} checks = [ - {"name": "backend", "app_id": 15368, "conclusion": "failure"}, - {"name": "lint", "app_id": None, "conclusion": "success"}, - {"name": "docs", "app_id": None, "conclusion": "missing"}, - {"name": "e2e\nspoof", "app_id": None, "conclusion": "pending"}, + {**bound, "name": "backend", "conclusion": "failure"}, + {**bound, "name": "lint", "conclusion": "success"}, + {**bound, "name": "docs", "conclusion": "missing"}, + {**bound, "name": "e2e\nspoof", "conclusion": "pending"}, ] findings = checker.evaluate_proof( _encoded(), configuration=CONFIGURATION, input_value=_input(required_checks=checks) @@ -391,3 +419,218 @@ def test_registry_schemas_accept_only_supported_configuration_and_facts_input() for schema, value in rejected: with pytest.raises(ExternalCheckerContractError): schema.validate_instance(value) + + + +# Integrity rules (owner decision 20) ------------------------------------------- + + +def _review(review_id: int, state: str, *, commit: str = HEAD, **reviewer: object) -> dict: + person = {"login": "reviewer", "id": 7, "type": "User", **reviewer} + return { + "id": review_id, + "reviewer": person, + "state": state, + "commit_id": commit, + "submitted_at": "2026-10-24T11:00:00Z", + } + + +def _integrity(configuration: object = CONFIGURATION, **fact_changes: object) -> list[dict]: + return [ + item + for item in checker.evaluate_proof( + _encoded(), configuration=configuration, input_value=_input(**fact_changes) + ) + ] + + +def _integrity_codes(configuration: object = CONFIGURATION, **fact_changes: object) -> list[str]: + return [item["code"] for item in _integrity(configuration, **fact_changes)] + + +def _check(name: str, *, bound: bool) -> dict: + return { + "name": name, + "app_id": 15368 if bound else None, + "app_bound": bound, + "conclusion": "success", + "check_run_app_id": 15368 if bound else None, + "commit_status_judged": not bound, + } + + +def test_unbound_required_check_is_an_error_unless_explicitly_allowed() -> None: + checks = [_check("backend", bound=True), _check("lint", bound=False)] + findings = _integrity(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 _integrity_codes(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 = _integrity(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 _integrity_codes(changed_files=files) == [] + extended = {**CONFIGURATION, "protected_paths": ["deploy/**"]} + assert _integrity_codes(extended, changed_files=files) == [ + "harnessy.proof.github.protected_path_changed" + ] + nested = [{"filename": "src/.github/x", "previous_filename": None, "status": "added"}] + assert _integrity_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 = _integrity(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 _integrity_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 _integrity_codes(reviews=reviews) == [] + out_of_order = [_review(70, "APPROVED"), _review(65, "CHANGES_REQUESTED")] + assert _integrity_codes(reviews=out_of_order) == [] + + +def test_required_independent_approvals_is_configurable() -> None: + two = {**CONFIGURATION, "required_independent_approvals": 2} + assert _integrity_codes(two) == ["harnessy.proof.github.independent_approval_missing"] + reviews = [_review(61, "APPROVED"), _review(62, "APPROVED", login="second", id=8)] + assert _integrity_codes(two, reviews=reviews) == [] + + +def test_operator_only_approval_is_a_non_blocking_warning() -> None: + operators = {**CONFIGURATION, "operator_logins": ["Reviewer"]} + findings = _integrity(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 _integrity_codes(operators, reviews=reviews) == [] + + +@pytest.mark.parametrize( + "missing", + ["author", "reviews", "changed_files", "changed_files_count"], +) +def test_integrity_rules_fail_closed_without_their_facts(missing: str) -> None: + assert _integrity_codes(**{missing: ...}) == ["harnessy.proof.github.facts_invalid"] + + +def test_file_count_mismatch_and_unlabelled_check_binding_fail_closed() -> None: + assert _integrity_codes(changed_files_count=2) == ["harnessy.proof.github.facts_invalid"] + check = {key: value for key, value in FACTS["required_checks"][0].items() if key != "app_bound"} + assert _integrity_codes(required_checks=[check]) == ["harnessy.proof.github.facts_invalid"] + + +def test_unconfigured_facts_still_apply_default_integrity_rules() -> None: + files = [{"filename": ".github/ci.yml", "previous_filename": None, "status": "added"}] + assert _integrity_codes({}, changed_files=files, reviews=[]) == [ + "harnessy.proof.github.protected_path_changed", + "harnessy.proof.github.independent_approval_missing", + ] + + +@pytest.mark.parametrize( + "configuration", + [ + {**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_invalid_integrity_options_are_a_configuration_error(configuration: dict) -> None: + assert _compare(configuration=configuration) == ["harnessy.proof.github.configuration"] + + +def test_registry_schema_accepts_integrity_options_only_in_comparison_mode() -> None: + post = ExternalCheckerRegistrySpec.model_validate( + publish_local.registry_spec("post_submit", "sha256:" + "d" * 64, "v2") + ) + full = { + **CONFIGURATION, + "allow_unbound_required_checks": False, + "protected_paths": ["deploy/**"], + "required_independent_approvals": 2, + "operator_logins": ["julian"], + } + post.configuration_schema.validate_instance(full) + for value in ( + {"operator_logins": ["julian"]}, + {**CONFIGURATION, "required_independent_approvals": 0}, + {**CONFIGURATION, "protected_paths": "deploy/**"}, + ): + with pytest.raises(ExternalCheckerContractError): + post.configuration_schema.validate_instance(value) diff --git a/docs/engineering/local-agent-loop.md b/docs/engineering/local-agent-loop.md index cde07efa5..f25e1dd11 100644 --- a/docs/engineering/local-agent-loop.md +++ b/docs/engineering/local-agent-loop.md @@ -252,7 +252,15 @@ The POST binding configuration names the repository and base branch: ``` The registry `configuration_schema` requires both keys together; the PRE -schema accepts only `{}`. WorkStream, not the checker, reads the facts from +schema accepts only `{}`. Comparison mode also accepts integrity options whose +defaults are the safe ones: + +| 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, not the checker, reads the facts from GitHub and places them in the request `input` under `github_facts`, so the request digest binds them: @@ -267,7 +275,14 @@ request digest binds them: "merge_commit_sha": "<40 lowercase hex, or null unless merged>", "base_ref": "main", "head_sha": "<40 lowercase hex>", - "required_checks": [{"name": "backend", "app_id": 15368, "conclusion": "success"}], + "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, + "commit_status_judged": false}], "fetched_at": "2026-10-24T12:00:00Z", "responses": [{"request": "pull_request", "sha256": "sha256:<64 hex>"}] } @@ -291,7 +306,16 @@ finding: | `required_check` | one finding per required check whose result is not `success` | | `facts_missing` | the configuration requires facts and the input has none | | `facts_invalid` | the facts do not have the `workstream.github_facts.v1` shape | -| `configuration` | the configuration is neither `{}` nor both keys | +| `configuration` | the configuration is neither `{}` nor both keys with valid options | +| `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 | + +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. These integrity rules also apply when +facts arrive without configuration, using the defaults. Facts present without configuration are still compared, except repository and base branch. Until WorkStream's POST evaluation supplies facts (initiative diff --git a/external_checkers/harnessy_proof/checker.py b/external_checkers/harnessy_proof/checker.py index 1aba30d09..ff47f939a 100644 --- a/external_checkers/harnessy_proof/checker.py +++ b/external_checkers/harnessy_proof/checker.py @@ -17,6 +17,13 @@ configuration requires them are an error finding, never a pass. Facts present without configuration are still compared, except the configured repository and base branch. The checker itself never uses the network or a credential. + +Integrity rules (owner decision 20) apply whenever facts are compared and fail +closed: 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); and approval only by configured operators adds a non-blocking +``harnessy.operator_approved`` warning. """ from __future__ import annotations @@ -104,21 +111,75 @@ def evaluate_proof( return findings -def _github_configuration(configuration: object) -> tuple[str, str] | None | bool: - """Return (repository, base branch), None for shape mode, or False if invalid.""" +DEFAULT_PROTECTED_PATHS = (".github/**", "**/CODEOWNERS", ".gitlab-ci.yml", ".circleci/**") +DEFAULT_REQUIRED_INDEPENDENT_APPROVALS = 1 +OPERATOR_APPROVED_CODE = "harnessy.operator_approved" +_APPROVAL_STATES = frozenset({"APPROVED", "CHANGES_REQUESTED", "DISMISSED"}) +_MAX_LISTED_PATHS = 5 +_CONFIGURATION_KEYS = frozenset( + { + "github_repository", + "base_branch", + "allow_unbound_required_checks", + "protected_paths", + "required_independent_approvals", + "operator_logins", + } +) + + +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 | bool: + """Return the normalized GitHub configuration, None for shape mode, or False. + + 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) or not configuration: 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 ( - set(configuration) != {"github_repository", "base_branch"} + 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 False - return repository, base_branch + 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 _valid_facts(facts: object) -> bool: @@ -127,6 +188,8 @@ def _valid_facts(facts: object) -> bool: return False checks = facts.get("required_checks") merge_sha = facts.get("merge_commit_sha") + reviews = facts.get("reviews") + files = facts.get("changed_files") return ( isinstance(facts.get("repository"), str) and type(facts.get("pull_request_number")) is int @@ -138,12 +201,35 @@ def _valid_facts(facts: object) -> bool: 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 (review.get("commit_id") is None or isinstance(review.get("commit_id"), str)) + 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 ( + item.get("previous_filename") is None + or isinstance(item.get("previous_filename"), str) + ) + for item in files + ) and isinstance(checks, list) and len(checks) > 0 and all( isinstance(check, dict) and isinstance(check.get("name"), str) and isinstance(check.get("conclusion"), str) + and isinstance(check.get("app_bound"), bool) for check in checks ) ) @@ -157,6 +243,129 @@ def _display(value: str) -> str: 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 _integrity_findings(facts: dict, configured: dict | None) -> list[dict[str, object]]: + """Integrity rules that apply whenever GitHub facts are compared.""" + settings = configured or { + "allow_unbound": False, + "protected_paths": DEFAULT_PROTECTED_PATHS, + "required_approvals": DEFAULT_REQUIRED_INDEPENDENT_APPROVALS, + "operators": frozenset(), + } + 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]]: @@ -166,7 +375,8 @@ def compare_github_facts( return [ _finding( "github.configuration", - "The checker configuration must name github_repository and base_branch.", + "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 @@ -184,21 +394,20 @@ def compare_github_facts( findings = [] repository = facts["repository"] if configured is not None: - expected_repository, expected_base = configured - if repository.casefold() != expected_repository.casefold(): + if repository.casefold() != configured["repository"].casefold(): findings.append( _finding( "github.repository", f"The pull request belongs to {_display(repository)}, " - f"not {_display(expected_repository)}.", + f"not {_display(configured['repository'])}.", ) ) - if facts["base_ref"] != expected_base: + if facts["base_ref"] != configured["base_branch"]: findings.append( _finding( "github.base_branch", f"The pull request targets {_display(facts['base_ref'])}, " - f"not {_display(expected_base)}.", + f"not {_display(configured['base_branch'])}.", ) ) pr_url = proof.get("pr_url") @@ -251,9 +460,11 @@ def compare_github_facts( "for the pull request head commit.", ) ) + findings.extend(_integrity_findings(facts, configured)) 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: diff --git a/external_checkers/harnessy_proof/publish_local.py b/external_checkers/harnessy_proof/publish_local.py index 9c7b4dc8e..1b722adee 100644 --- a/external_checkers/harnessy_proof/publish_local.py +++ b/external_checkers/harnessy_proof/publish_local.py @@ -50,7 +50,8 @@ def _hash(value: object) -> str: # PRE has no GitHub facts, so only shape mode is valid there. POST accepts shape -# mode ({}) or GitHub comparison mode naming both repository and base branch. +# mode ({}) or GitHub comparison mode naming both repository and base branch, +# optionally with integrity options whose defaults are the safe ones. CONFIGURATION_SCHEMAS: dict[str, dict[str, object]] = { "pre_submit": {"type": "object", "additionalProperties": False, "maxProperties": 0}, "post_submit": { @@ -62,10 +63,31 @@ def _hash(value: object) -> str: "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}, + }, }, + # Every integrity option belongs to GitHub comparison mode. "dependentRequired": { "github_repository": ["base_branch"], "base_branch": ["github_repository"], + "allow_unbound_required_checks": ["github_repository"], + "protected_paths": ["github_repository"], + "required_independent_approvals": ["github_repository"], + "operator_logins": ["github_repository"], }, }, } From 88f5e09b829a7456ba4667b4ae04c3f71101d25e Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 15:38:44 +0100 Subject: [PATCH 3/4] fix(checker): fail closed with one canonical GitHub identity Security review: remove the shape-only pass (configuration and facts are always required); compare pr_url and ci_run_url only by exact equality with canonical URLs built from the facts and GitHub's html_url; reject check entries bound to another commit. The checker is post-submission only. Co-Authored-By: Claude Opus 5.5 --- .../WS-GHPROOF-001/WS-GHPROOF-001-03.md | 168 ++-- .../external/test_harnessy_proof_checker.py | 731 ++++++++---------- docs/engineering/local-agent-loop.md | 122 ++- docs/roadmap_status.md | 6 +- external_checkers/harnessy_proof/checker.py | 292 +++---- .../harnessy_proof/publish_local.py | 113 ++- 6 files changed, 697 insertions(+), 735 deletions(-) diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md index 13265cec6..a9fb73ed2 100644 --- a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md @@ -1,22 +1,25 @@ -# WS-GHPROOF-001-03 — Harnessy proof checker GitHub comparison mode +# WS-GHPROOF-001-03 — Harnessy proof checker compares proofs with GitHub facts - Initiative: WS-GHPROOF-001 - Durable disposition: Complete -- Intended merge outcome: The networkless `harnessy.work_proof` checker compares `proof.json` with WorkStream-supplied `workstream.github_facts.v1` facts when its binding configuration names a GitHub repository and base branch, reporting one error finding per mismatch, and its v2 registry bodies validate that configuration. +- 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 mode makes a faked proof fail, while the checker stays -networkless and credential-free. +(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 - `{"type": "object"}` configuration and input schemas, default version `v1`. -- `docs/engineering/local-agent-loop.md` documents shape mode only. +- `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 @@ -26,8 +29,8 @@ networkless and credential-free. library only). - `backend/tests/checkers/external/test_harnessy_proof_checker.py` (already in the lane catalogue). -- `docs/engineering/local-agent-loop.md` sections on publishing and on what the - proof checker checks. +- `docs/engineering/local-agent-loop.md` publishing, registration and checker + sections; the two agent-loop mentions in `docs/roadmap_status.md`. ### Not allowed @@ -36,87 +39,104 @@ networkless and credential-free. ## Design and decisions -- **Input shape.** WorkStream places the facts object in the request `input` - under `github_facts`, beside the normalized POST input fields, so the request - digest binds them. The object is exactly chunk 02's - `GitHubPullRequestFacts.model_dump(mode="json")`: `schema_version`, - `repository`, `pull_request_number`, `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`, `commit_status_judged`), - `fetched_at` and `responses`. -- **Mode selection.** Configuration `{}` with no facts is shape mode, unchanged. - Configuration `{"github_repository": "owner/name", "base_branch": ""}` - requires facts. Facts without configuration are compared except the - configured repository and base branch. -- **Findings.** Shape rules still run. Each mismatch is one `error` finding with - code `harnessy.proof.github.`: `repository`, `base_branch`, `pr_url`, - `pr_repository`, `pull_request_number`, `merged`, `merge_commit_sha`, one - `required_check` per non-success check, `facts_missing`, `facts_invalid` and - `configuration`. Repository comparisons are case-insensitive, as GitHub - names are; the commit SHA is compared in lower case. GitHub-provided check - names in messages are bounded to 200 printable characters. -- **Integrity rules (owner decision 20, 2026-10-24).** They apply whenever - facts are compared, with safe defaults, each with its own finding: - `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); +- **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 `info` is used because contributor and - history reads count only blocking, warning and coverage findings, so an + 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. -- **Fail closed.** Required but missing or malformed facts produce an error - finding and a `failed` verdict, never a pass. Chunk 04 maps a facts-fetch - failure to an infrastructure failure before the checker runs, so a GitHub - outage does not become a contributor-visible failure. -- **Registry schemas.** POST `configuration_schema` accepts `{}` or both keys - together (`dependentRequired`, no other keys, repository pattern). PRE accepts - only `{}` because PRE never has facts. POST `input_schema` constrains - `github_facts.schema_version` when present; the checker validates the rest. -- **Version.** `publish_local.py` defaults to capability version `v2`, because - the registry entry for `v1` is immutable and the behavior and schemas changed. -- Rejected: a second ART mount for facts (owner decision), and making - `github_facts` required in the POST input schema (it would break shape-mode - bindings). +- **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. -- [x] Each mismatch (repository, PR repository, PR number, PR URL form, merged, - merge commit SHA, base branch) reports exactly its finding. -- [x] Each failed, missing or pending required check reports its own finding. -- [x] Required facts that are missing or malformed fail; incomplete - configuration fails. +- [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, operator-only approval, and missing integrity facts. -- [x] Shape mode is unchanged; `main()` passes configuration and input through. -- [x] Published v2 registry bodies validate through - `ExternalCheckerRegistrySpec` and accept or reject configuration and input as - documented. + 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 on missing facts, and - strict `success` for required checks. +- 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 | |---|---|---|---| -| Comparison rules and result contract | `pytest tests/checkers/external/test_harnessy_proof_checker.py` | 91 passed | Facts shape agreement with chunk 02 is documented, not cross-tested; chunk 04 should add that test | +| Rules and result contract | `pytest tests/checkers/external/test_harnessy_proof_checker.py` | 108 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 | | Lint | `ruff check` on the test module and `external_checkers/harnessy_proof/` | Pass | — | | Docs | Markdown link and stale wording checks | Pass | — | @@ -124,14 +144,20 @@ The image was not rebuilt or run: Docker was out of scope for this change. ## Review findings -None recorded. +- 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: no impact. The local agent loop keeps shape mode until chunk 04 - supplies facts, so its stated trust limit is unchanged. +- 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, configuring comparison mode fails every POST - check with `facts_missing`. +- 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 18777995f..948ab106e 100644 --- a/backend/tests/checkers/external/test_harnessy_proof_checker.py +++ b/backend/tests/checkers/external/test_harnessy_proof_checker.py @@ -1,4 +1,4 @@ -"""Harnessy proof checker logic and its published result contract.""" +"""Harnessy proof checker: fail-closed comparison with GitHub facts.""" from __future__ import annotations @@ -33,204 +33,76 @@ 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 _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 _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 _request(phase: str = "post_submit") -> dict: +def _review(review_id: int, state: str, *, commit: str = HEAD, **reviewer: object) -> dict: return { - "request_digest": "sha256:" + "b" * 64, - "registry": {"registry_entry_id": str(uuid4()), "entry_digest": "sha256:" + "c" * 64}, - "identity": {"phase": phase}, + "id": review_id, + "reviewer": {"login": "reviewer", "id": 7, "type": "User", **reviewer}, + "state": state, + "commit_id": commit, + "submitted_at": "2026-10-24T11:00:00Z", } -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"], - ) - - -@pytest.mark.parametrize( - ("changes", "code"), - [ - ({"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"), - ], -) -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( - ("raw", "code"), - [ - (None, "missing"), - (b"{not json", "invalid_json"), - (b"\xff\xfe", "invalid_json"), - (b"[]", "invalid_json"), - (b" " * (checker.MAX_PROOF_BYTES + 1), "too_large"), - ], -) -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" - - -def test_read_proof_rejects_links_and_bounds_bytes(tmp_path: Path) -> None: - assert checker.read_proof(tmp_path) is None - target = tmp_path / "elsewhere.json" - target.write_bytes(_encoded()) - (tmp_path / "proof.json").symlink_to(target) - assert checker.read_proof(tmp_path) is None - (tmp_path / "proof.json").unlink() - (tmp_path / "proof.json").write_bytes(b"x" * (checker.MAX_PROOF_BYTES + 10)) - assert len(checker.read_proof(tmp_path)) == checker.MAX_PROOF_BYTES + 1 - - -def test_published_registry_bodies_are_backend_valid_for_both_phases() -> 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", - ) - for registry in ("localhost:5000@registry.example", "localhost", "registry.example:5000"): - with pytest.raises(SystemExit): - publish_local.publish(registry, "harnessy-proof") - - -def test_published_service_paths_match_the_compose_overlay() -> None: - """The service config and the overlay must name the same daemon and socket paths.""" - overlay = yaml.safe_load( - (CHECKER_ROOT.parents[1] / "docker-compose.agent-loop.yml").read_text() - ) - environment = overlay["x-agent-loop-backend"]["environment"] - config = publish_local.service_config( - {"operating_system": "linux", "architecture": "arm64"}, - compose_project="${COMPOSE_PROJECT_NAME}", - sandbox_uid=501, - sandbox_gid=20, - ) - assert config["material_root"] == environment["WORKSTREAM_EXTERNAL_CHECKER_MATERIAL_ROOT"] - assert config["material_root"] == environment["WORKSTREAM_ARTIFACT_SCRATCH_ROOT"] - assert config["socket_path"] == environment["WORKSTREAM_EXTERNAL_CHECKER_SERVICE_SOCKET"] - assert len(config["socket_path"].encode()) <= 107 - for name in ("backend", "worker", "beat", "checker", "checker-volumes-init"): - socket_mounts = [ - mount - for mount in overlay["services"][name]["volumes"] - if (mount.split(":")[1] if isinstance(mount, str) else mount["target"]) - == "/run/ws-checker" - ] - mode = ":ro" if name in ("backend", "worker", "beat") else "" - assert socket_mounts == [f"checker_socket:/run/ws-checker{mode}"], name - - -# GitHub comparison mode ------------------------------------------------------- - -CONFIGURATION = {"github_repository": "acme/widgets", "base_branch": "main"} -HEAD = "e" * 40 FACTS = { "schema_version": "workstream.github_facts.v1", - "repository": "acme/widgets", + "repository": REPOSITORY, "pull_request_number": 12, + "html_url": PR_URL, "state": "closed", "merged": True, - "merge_commit_sha": VALID_PROOF["commit_sha"], + "merge_commit_sha": MERGE, "base_ref": "main", "head_sha": HEAD, "author": {"login": "agent-bot", "id": 501, "type": "Bot"}, - "reviews": [ - { - "id": 61, - "reviewer": {"login": "reviewer", "id": 7, "type": "User"}, - "state": "APPROVED", - "commit_id": HEAD, - "submitted_at": "2026-10-24T11:00:00Z", - } - ], + "reviews": [_review(61, "APPROVED")], "changed_files_count": 1, "changed_files": [{"filename": "src/app.py", "previous_filename": None, "status": "modified"}], - "required_checks": [ - { - "name": "backend", - "app_id": 15368, - "app_bound": True, - "conclusion": "success", - "check_run_app_id": 15368, - "commit_status_judged": False, - }, - { - "name": "lint", - "app_id": 15368, - "app_bound": True, - "conclusion": "success", - "check_run_app_id": 15368, - "commit_status_judged": False, - }, - ], + "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 { @@ -239,87 +111,106 @@ def _input(**changes: object) -> dict: } -def _compare( +def _findings( proof: bytes | None = None, *, configuration: object = CONFIGURATION, input_value: object = None, -) -> list[str]: - findings = checker.evaluate_proof( + **fact_changes: object, +) -> list[dict]: + return checker.evaluate_proof( _encoded() if proof is None else proof, configuration=configuration, - input_value=_input() if input_value is None else input_value, + input_value=_input(**fact_changes) if input_value is None else input_value, ) - return [item["code"] for item in findings] -def test_proof_matching_github_facts_passes() -> None: - assert _compare() == [] - upper = VALID_PROOF["commit_sha"].upper() - pr_url = "https://github.com/Acme/Widgets/pull/12/" - assert _compare(_encoded(commit_sha=upper, pr_url=pr_url)) == [] +def _codes(proof: bytes | None = None, **kwargs: object) -> list[str]: + return [item["code"] for item in _findings(proof, **kwargs)] -@pytest.mark.parametrize( - ("proof_changes", "fact_changes", "code"), - [ - ( - {"pr_url": "https://github.com/acme/other/pull/12"}, - {"repository": "acme/other"}, - "github.repository", - ), - ({"pr_url": "https://github.com/acme/other/pull/12"}, {}, "github.pr_repository"), - ( - {"pr_url": "https://github.com/acme/widgets/pull/13"}, - {}, - "github.pull_request_number", - ), - ({"pr_url": "https://github.com/acme/widgets/issues/12"}, {}, "github.pr_url"), - ({}, {"merged": False, "state": "open"}, "github.merged"), - ({"commit_sha": "1" * 40}, {}, "github.merge_commit_sha"), - ({}, {"base_ref": "release"}, "github.base_branch"), - ], -) -def test_each_github_mismatch_reports_exactly_its_finding( - proof_changes: dict, fact_changes: dict, code: str -) -> None: - assert _compare(_encoded(**proof_changes), input_value=_input(**fact_changes)) == [ - f"harnessy.proof.{code}" - ] +def _request(phase: str = "post_submit") -> dict: + return { + "request_digest": "sha256:" + "b" * 64, + "registry": {"registry_entry_id": str(uuid4()), "entry_digest": "sha256:" + "c" * 64}, + "identity": {"phase": phase}, + } -def test_unmerged_facts_also_fail_the_merge_commit_comparison() -> None: - codes = _compare(input_value=_input(merged=False, merge_commit_sha=None)) - assert codes == ["harnessy.proof.github.merged", "harnessy.proof.github.merge_commit_sha"] +# Matching proof and result contract -------------------------------------------- -def test_each_unsuccessful_required_check_reports_its_own_finding() -> None: - bound = {"app_id": 1, "app_bound": True, "check_run_app_id": 1, "commit_status_judged": False} - checks = [ - {**bound, "name": "backend", "conclusion": "failure"}, - {**bound, "name": "lint", "conclusion": "success"}, - {**bound, "name": "docs", "conclusion": "missing"}, - {**bound, "name": "e2e\nspoof", "conclusion": "pending"}, - ] - findings = checker.evaluate_proof( - _encoded(), configuration=CONFIGURATION, input_value=_input(required_checks=checks) +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 [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] + 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( + "configuration", + [ + 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_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"}, None], + [{}, {"schema_version": "workstream.external_checker_post_submit_input.v1"}, "x"], ) -def test_required_facts_missing_is_an_error_never_a_pass(input_value: object) -> None: - findings = checker.evaluate_proof( - _encoded(), configuration=CONFIGURATION, input_value=input_value - ) - assert [item["code"] for item in findings] == ["harnessy.proof.github.facts_missing"] +def test_missing_facts_never_pass(input_value: object) -> None: + assert _codes(input_value=input_value) == ["harnessy.proof.github.facts_missing"] @pytest.mark.parametrize( @@ -328,147 +219,190 @@ def test_required_facts_missing_is_an_error_never_a_pass(input_value: object) -> {"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 _compare(input_value=_input(**facts_changes)) == [ - "harnessy.proof.github.facts_invalid" - ] + assert _codes(**facts_changes) == ["harnessy.proof.github.facts_invalid"] @pytest.mark.parametrize( - "configuration", + "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( + ("raw", "code"), [ - {"github_repository": "acme/widgets"}, - {"base_branch": "main"}, - {**CONFIGURATION, "extra": True}, - {"github_repository": "", "base_branch": "main"}, + (None, "missing"), + (b"{not json", "invalid_json"), + (b"\xff\xfe", "invalid_json"), + (b"[]", "invalid_json"), + (b" " * (checker.MAX_PROOF_BYTES + 1), "too_large"), ], ) -def test_incomplete_configuration_is_an_error(configuration: dict) -> None: - assert _compare(configuration=configuration) == ["harnessy.proof.github.configuration"] +def test_unreadable_proof_reports_one_error(raw: bytes | None, code: str) -> None: + findings = checker.evaluate_proof(raw, configuration=CONFIGURATION, input_value=_input()) + assert [item["code"] for item in findings] == [f"harnessy.proof.{code}"] -def test_shape_mode_is_unchanged_without_configuration_or_facts() -> None: - assert checker.evaluate_proof(_encoded(), configuration={}, input_value={}) == [] - assert checker.evaluate_proof(_encoded(merged=False)) == [ - checker._finding("merged", "merged must be the JSON value true.") - ] +def test_read_proof_rejects_links_and_bounds_bytes(tmp_path: Path) -> None: + assert checker.read_proof(tmp_path) is None + target = tmp_path / "elsewhere.json" + target.write_bytes(_encoded()) + (tmp_path / "proof.json").symlink_to(target) + assert checker.read_proof(tmp_path) is None + (tmp_path / "proof.json").unlink() + (tmp_path / "proof.json").write_bytes(b"x" * (checker.MAX_PROOF_BYTES + 10)) + assert len(checker.read_proof(tmp_path)) == checker.MAX_PROOF_BYTES + 1 -def test_unconfigured_facts_are_still_compared() -> None: - codes = _compare(configuration={}, input_value=_input(base_ref="other", merged=False)) - assert codes == ["harnessy.proof.github.merged"] +# Claims and mismatches ----------------------------------------------------------- -def test_shape_errors_and_github_mismatches_are_both_reported() -> None: - codes = _compare(_encoded(ci_conclusion="failure"), input_value=_input(merged=False)) - assert codes == ["harnessy.proof.ci_conclusion", "harnessy.proof.github.merged"] +@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}"] -def test_comparison_failure_produces_a_backend_valid_failed_result() -> None: - findings = checker.evaluate_proof( - _encoded(), configuration=CONFIGURATION, input_value=_input(base_ref="dev") - ) - parsed = ExternalCheckerExecutionResult.model_validate_json( - json.dumps(checker.build_result(_request(), findings)) - ) - assert parsed.verdict == "failed" - assert [item.code for item in parsed.findings] == ["harnessy.proof.github.base_branch"] +@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", + ] -def test_main_passes_request_configuration_and_input( - monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +@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: - 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"] - - -def test_registry_schemas_accept_only_supported_configuration_and_facts_input() -> None: - digest = "sha256:" + "d" * 64 - post = ExternalCheckerRegistrySpec.model_validate( - publish_local.registry_spec("post_submit", digest, "v2") - ) - pre = ExternalCheckerRegistrySpec.model_validate( - publish_local.registry_spec("pre_submit", digest, "v2") - ) - post.configuration_schema.validate_instance({}) - post.configuration_schema.validate_instance(CONFIGURATION) - post.input_schema.validate_instance(_input()) - post.input_schema.validate_instance( - {"schema_version": "workstream.external_checker_post_submit_input.v1"} - ) - pre.configuration_schema.validate_instance({}) - rejected = [ - (post.configuration_schema, {"github_repository": "acme/widgets"}), - (post.configuration_schema, {"base_branch": "main"}), - (post.configuration_schema, {**CONFIGURATION, "github_repository": "acme"}), - (post.configuration_schema, {**CONFIGURATION, "extra": 1}), - (pre.configuration_schema, CONFIGURATION), - (post.input_schema, _input(schema_version="workstream.github_facts.v2")), + assert _codes(_encoded(**proof_changes), **fact_changes) == [ + f"harnessy.proof.github.{code}" for code in codes ] - for schema, value in rejected: - with pytest.raises(ExternalCheckerContractError): - schema.validate_instance(value) +@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"] + -# Integrity rules (owner decision 20) ------------------------------------------- +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" + ] -def _review(review_id: int, state: str, *, commit: str = HEAD, **reviewer: object) -> dict: - person = {"login": "reviewer", "id": 7, "type": "User", **reviewer} - return { - "id": review_id, - "reviewer": person, - "state": state, - "commit_id": commit, - "submitted_at": "2026-10-24T11:00:00Z", - } +@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 _integrity(configuration: object = CONFIGURATION, **fact_changes: object) -> list[dict]: - return [ - item - for item in checker.evaluate_proof( - _encoded(), configuration=configuration, input_value=_input(**fact_changes) - ) - ] +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 _integrity_codes(configuration: object = CONFIGURATION, **fact_changes: object) -> list[str]: - return [item["code"] for item in _integrity(configuration, **fact_changes)] +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] -def _check(name: str, *, bound: bool) -> dict: - return { - "name": name, - "app_id": 15368 if bound else None, - "app_bound": bound, - "conclusion": "success", - "check_run_app_id": 15368 if bound else None, - "commit_status_judged": not bound, - } +# Integrity rules (owner decision 20) ---------------------------------------------- def test_unbound_required_check_is_an_error_unless_explicitly_allowed() -> None: - checks = [_check("backend", bound=True), _check("lint", bound=False)] - findings = _integrity(required_checks=checks) + 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 _integrity_codes(allowed, required_checks=checks) == [] + assert _codes(configuration=allowed, required_checks=checks) == [] @pytest.mark.parametrize( @@ -480,14 +414,25 @@ def test_unbound_required_check_is_an_error_unless_explicitly_allowed() -> None: {"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"}, + { + "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"), + 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 = _integrity(changed_files=files, changed_files_count=2) + findings = _findings(changed_files=files, changed_files_count=2) assert [item["code"] for item in findings] == [ "harnessy.proof.github.protected_path_changed" ] @@ -496,13 +441,13 @@ def test_protected_path_change_requires_human_acceptance(changed: dict) -> None: def test_configured_protected_paths_extend_the_defaults() -> None: files = [{"filename": "deploy/prod.tf", "previous_filename": None, "status": "modified"}] - assert _integrity_codes(changed_files=files) == [] + assert _codes(changed_files=files) == [] extended = {**CONFIGURATION, "protected_paths": ["deploy/**"]} - assert _integrity_codes(extended, changed_files=files) == [ + assert _codes(configuration=extended, changed_files=files) == [ "harnessy.proof.github.protected_path_changed" ] nested = [{"filename": "src/.github/x", "previous_filename": None, "status": "added"}] - assert _integrity_codes(changed_files=nested) == [] + assert _codes(changed_files=nested) == [] def test_protected_path_finding_lists_a_bounded_number_of_paths() -> None: @@ -510,7 +455,7 @@ def test_protected_path_finding_lists_a_bounded_number_of_paths() -> None: {"filename": f".github/workflows/w{index}.yml", "previous_filename": None, "status": "added"} for index in range(20) ] - findings = _integrity(changed_files=files, changed_files_count=20) + findings = _findings(changed_files=files, changed_files_count=20) assert len(findings) == 1 assert "and 15 more" in findings[0]["message"] @@ -541,9 +486,7 @@ def test_protected_path_finding_lists_a_bounded_number_of_paths() -> None: ), ) def test_missing_independent_approval_on_head_commit_is_an_error(reviews: list) -> None: - assert _integrity_codes(reviews=reviews) == [ - "harnessy.proof.github.independent_approval_missing" - ] + assert _codes(reviews=reviews) == ["harnessy.proof.github.independent_approval_missing"] def test_latest_decisive_review_wins_and_comments_do_not_cancel() -> None: @@ -552,21 +495,23 @@ def test_latest_decisive_review_wins_and_comments_do_not_cancel() -> None: _review(62, "APPROVED"), _review(63, "COMMENTED"), ] - assert _integrity_codes(reviews=reviews) == [] + assert _codes(reviews=reviews) == [] out_of_order = [_review(70, "APPROVED"), _review(65, "CHANGES_REQUESTED")] - assert _integrity_codes(reviews=out_of_order) == [] + 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 _integrity_codes(two) == ["harnessy.proof.github.independent_approval_missing"] + assert _codes(configuration=two) == ["harnessy.proof.github.independent_approval_missing"] reviews = [_review(61, "APPROVED"), _review(62, "APPROVED", login="second", id=8)] - assert _integrity_codes(two, reviews=reviews) == [] + assert _codes(configuration=two, reviews=reviews) == [] def test_operator_only_approval_is_a_non_blocking_warning() -> None: operators = {**CONFIGURATION, "operator_logins": ["Reviewer"]} - findings = _integrity(operators) + findings = _findings(configuration=operators) assert [(item["code"], item["level"]) for item in findings] == [ ("harnessy.operator_approved", "warning") ] @@ -575,49 +520,20 @@ def test_operator_only_approval_is_a_non_blocking_warning() -> None: ) assert result.verdict == "passed" reviews = [_review(61, "APPROVED"), _review(62, "APPROVED", login="independent", id=8)] - assert _integrity_codes(operators, reviews=reviews) == [] + assert _codes(configuration=operators, reviews=reviews) == [] -@pytest.mark.parametrize( - "missing", - ["author", "reviews", "changed_files", "changed_files_count"], -) -def test_integrity_rules_fail_closed_without_their_facts(missing: str) -> None: - assert _integrity_codes(**{missing: ...}) == ["harnessy.proof.github.facts_invalid"] - - -def test_file_count_mismatch_and_unlabelled_check_binding_fail_closed() -> None: - assert _integrity_codes(changed_files_count=2) == ["harnessy.proof.github.facts_invalid"] - check = {key: value for key, value in FACTS["required_checks"][0].items() if key != "app_bound"} - assert _integrity_codes(required_checks=[check]) == ["harnessy.proof.github.facts_invalid"] - - -def test_unconfigured_facts_still_apply_default_integrity_rules() -> None: - files = [{"filename": ".github/ci.yml", "previous_filename": None, "status": "added"}] - assert _integrity_codes({}, changed_files=files, reviews=[]) == [ - "harnessy.proof.github.protected_path_changed", - "harnessy.proof.github.independent_approval_missing", - ] +# Published registry body ----------------------------------------------------------- -@pytest.mark.parametrize( - "configuration", - [ - {**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_invalid_integrity_options_are_a_configuration_error(configuration: dict) -> None: - assert _compare(configuration=configuration) == ["harnessy.proof.github.configuration"] - - -def test_registry_schema_accepts_integrity_options_only_in_comparison_mode() -> None: - post = ExternalCheckerRegistrySpec.model_validate( - publish_local.registry_spec("post_submit", "sha256:" + "d" * 64, "v2") +def test_published_registry_body_is_post_only_and_backend_valid() -> None: + digest = "sha256:" + "d" * 64 + 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, @@ -626,11 +542,48 @@ def test_registry_schema_accepts_integrity_options_only_in_comparison_mode() -> "required_independent_approvals": 2, "operator_logins": ["julian"], } - post.configuration_schema.validate_instance(full) - for value in ( - {"operator_logins": ["julian"]}, - {**CONFIGURATION, "required_independent_approvals": 0}, - {**CONFIGURATION, "protected_paths": "deploy/**"}, + 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): - post.configuration_schema.validate_instance(value) + 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") + + +def test_published_service_paths_match_the_compose_overlay() -> None: + """The service config and the overlay must name the same daemon and socket paths.""" + overlay = yaml.safe_load( + (CHECKER_ROOT.parents[1] / "docker-compose.agent-loop.yml").read_text() + ) + environment = overlay["x-agent-loop-backend"]["environment"] + config = publish_local.service_config( + {"operating_system": "linux", "architecture": "arm64"}, + compose_project="${COMPOSE_PROJECT_NAME}", + sandbox_uid=501, + sandbox_gid=20, + ) + assert config["material_root"] == environment["WORKSTREAM_EXTERNAL_CHECKER_MATERIAL_ROOT"] + assert config["material_root"] == environment["WORKSTREAM_ARTIFACT_SCRATCH_ROOT"] + assert config["socket_path"] == environment["WORKSTREAM_EXTERNAL_CHECKER_SERVICE_SOCKET"] + assert len(config["socket_path"].encode()) <= 107 + for name in ("backend", "worker", "beat", "checker", "checker-volumes-init"): + socket_mounts = [ + mount + for mount in overlay["services"][name]["volumes"] + if (mount.split(":")[1] if isinstance(mount, str) else mount["target"]) + == "/run/ws-checker" + ] + mode = ":ro" if name in ("backend", "worker", "beat") else "" + assert socket_mounts == [f"checker_socket:/run/ws-checker{mode}"], name diff --git a/docs/engineering/local-agent-loop.md b/docs/engineering/local-agent-loop.md index f25e1dd11..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 @@ -121,8 +121,8 @@ 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` (the default is `v2`; use, for example, `v3`), 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 +`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,51 +216,33 @@ 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. -| Field | Rule | -|---|---| -| `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. With an empty binding -configuration (`{}`, shape mode) 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 shape mode alone to gate acceptance that -leads to compensation; pair it with human review, or use GitHub comparison -mode once WorkStream supplies GitHub facts, when the claim must be true. - -### GitHub comparison mode - -Capability version `v2` adds a comparison mode -([WS-GHPROOF-001](../../.commitrail/initiatives/WS-GHPROOF-001/OVERVIEW.md)). -The POST binding configuration names the repository and base branch: +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"} ``` -The registry `configuration_schema` requires both keys together; the PRE -schema accepts only `{}`. Comparison mode also accepts integrity options whose -defaults are the safe ones: - | 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, not the checker, reads the facts from -GitHub and places them in the request `input` under `github_facts`, so the -request digest binds them: +| `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 { @@ -270,6 +250,7 @@ request digest binds them: "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>", @@ -282,45 +263,52 @@ request digest binds them: "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, - "commit_status_judged": false}], + "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>"}] } } ``` -`required_checks` lists every check required by the base branch's protection, -with its result for the head commit (`missing` and `pending` included). In -this mode the shape rules above still apply, and each mismatch adds one `error` -finding: +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.*`): -| Code (`harnessy.proof.github.*`) | Fails when | +| Code | Fails when | |---|---| -| `repository` | the facts' repository is not `github_repository` | +| `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 `https://github.com///pull/` | -| `pr_repository` | `pr_url` names another repository than the facts | -| `pull_request_number` | `pr_url` names another pull request than the facts | +| `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` | -| `facts_missing` | the configuration requires facts and the input has none | -| `facts_invalid` | the facts do not have the `workstream.github_facts.v1` shape | -| `configuration` | the configuration is neither `{}` nor both keys with valid options | | `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. These integrity rules also apply when -facts arrive without configuration, using the defaults. +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. -Facts present without configuration are still compared, except repository and -base branch. Until WorkStream's POST evaluation supplies facts (initiative -chunk 04), configuring comparison mode makes every POST check fail with -`facts_missing`; this stack therefore keeps shape mode. +**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 @@ -459,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 5a95474ab..259ed93db 100644 --- a/docs/roadmap_status.md +++ b/docs/roadmap_status.md @@ -546,7 +546,7 @@ cannot be reused as post-submission review-gate evidence. See the | Contribution evidence reads | **Public fixed-membership REST and complete CLI export** | PILOT-10A exposes current human self-read and one-project JSON/CSV export for covering Project Managers or exact-project Finance Authorities. Each page has fresh authority and one repeatable-read response transaction; selections through 100,000 contributions retain page-one membership behind signed scope/filter/format/limit-bound cursors, with complete zero/one/two immutable awards, exact decimals, recorded-time filters and formula-neutralized CSV. The CLI follows JSON or CSV continuation internally with unchanged filters, validates cross-page order/identity, and publishes only the complete JSON result or one private no-overwrite CSV | No recognition mutation, fulfillment, payout, deployed-service or published-binary claim | | 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 @@ -573,7 +573,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 ff47f939a..bf22604dd 100644 --- a/external_checkers/harnessy_proof/checker.py +++ b/external_checkers/harnessy_proof/checker.py @@ -1,28 +1,23 @@ -"""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. - -Shape mode (empty binding configuration, no facts in the request input): the -checker 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. A passing -verdict means "well-formed proof claims", not "independently verified work". - -GitHub comparison mode: the binding configuration names -``{"github_repository": "owner/name", "base_branch": "main"}``, and WorkStream -places ``workstream.github_facts.v1`` facts it read from GitHub in the request -input under ``github_facts``. The checker compares every claim with those facts -and reports one error finding per mismatch. Missing or malformed facts when the -configuration requires them are an error finding, never a pass. Facts present -without configuration are still compared, except the configured repository and -base branch. The checker itself never uses the network or a credential. - -Integrity rules (owner decision 20) apply whenever facts are compared and fail -closed: 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); and approval only by configured operators adds a non-blocking +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. """ @@ -35,7 +30,6 @@ import re import stat import sys -from urllib.parse import urlsplit PROOF_PATH = "proof.json" PROOF_SCHEMA = "harnessy.work.proof.v1" @@ -43,12 +37,24 @@ 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}") -_PULL_PATH = re.compile( - r"/([A-Za-z0-9][A-Za-z0-9-]{0,38})/([A-Za-z0-9._-]{1,100})/pull/([1-9][0-9]{0,9})" -) +_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]: @@ -62,28 +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, *, - configuration: object = None, - input_value: object = None, + configuration: object, + input_value: object, ) -> list[dict[str, object]]: - """Return error findings for one proof document; an empty list passes. - - ``configuration`` is the locked binding configuration and ``input_value`` the - request input. Together they select shape mode or GitHub comparison mode. - """ + """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: @@ -94,52 +85,36 @@ def evaluate_proof( 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: findings.append(_finding("merged", "merged must be the JSON value true.")) - findings.extend(compare_github_facts(proof, configuration, input_value)) return findings -DEFAULT_PROTECTED_PATHS = (".github/**", "**/CODEOWNERS", ".gitlab-ci.yml", ".circleci/**") -DEFAULT_REQUIRED_INDEPENDENT_APPROVALS = 1 -OPERATOR_APPROVED_CODE = "harnessy.operator_approved" -_APPROVAL_STATES = frozenset({"APPROVED", "CHANGES_REQUESTED", "DISMISSED"}) -_MAX_LISTED_PATHS = 5 -_CONFIGURATION_KEYS = frozenset( - { - "github_repository", - "base_branch", - "allow_unbound_required_checks", - "protected_paths", - "required_independent_approvals", - "operator_logins", - } -) - - 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 | bool: - """Return the normalized GitHub configuration, None for shape mode, or False. +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) or not configuration: + if not isinstance(configuration, dict): return None repository = configuration.get("github_repository") base_branch = configuration.get("base_branch") @@ -161,7 +136,7 @@ def _github_configuration(configuration: object) -> dict | None | bool: or approvals < 1 or not _strings(operators) ): - return False + return None return { "repository": repository, "base_branch": base_branch, @@ -182,22 +157,46 @@ def _actor(value: object) -> bool: ) -def _valid_facts(facts: object) -> bool: +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") - merge_sha = facts.get("merge_commit_sha") 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 ( - merge_sha is None - or (isinstance(merge_sha, str) and _LOWER_SHA.fullmatch(merge_sha) is not None) - ) + 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 @@ -208,7 +207,7 @@ def _valid_facts(facts: object) -> bool: 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 (review.get("commit_id") is None or isinstance(review.get("commit_id"), str)) + and _sha_or_none(review.get("commit_id")) for review in reviews ) and isinstance(files, list) @@ -217,19 +216,21 @@ def _valid_facts(facts: object) -> bool: and all( isinstance(item, dict) and isinstance(item.get("filename"), str) - and ( - item.get("previous_filename") is None - or isinstance(item.get("previous_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 isinstance(check.get("name"), str) - and isinstance(check.get("conclusion"), str) - and isinstance(check.get("app_bound"), bool) + 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 ) ) @@ -310,14 +311,57 @@ def qualifying_approvals(facts: dict) -> list[str]: return sorted(logins) -def _integrity_findings(facts: dict, configured: dict | None) -> list[dict[str, object]]: - """Integrity rules that apply whenever GitHub facts are compared.""" - settings = configured or { - "allow_unbound": False, - "protected_paths": DEFAULT_PROTECTED_PATHS, - "required_approvals": DEFAULT_REQUIRED_INDEPENDENT_APPROVALS, - "operators": frozenset(), +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"]: @@ -370,8 +414,8 @@ 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.""" - configured = _github_configuration(configuration) - if configured is False: + settings = github_configuration(configuration) + if settings is None: return [ _finding( "github.configuration", @@ -381,63 +425,36 @@ def compare_github_facts( ] facts = input_value.get(GITHUB_FACTS_INPUT_KEY) if isinstance(input_value, dict) else None if facts is None: - if configured is None: - return [] return [ _finding( "github.facts_missing", "GitHub facts are required for this check but were not provided.", ) ] - if not _valid_facts(facts): + 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 configured is not None: - if repository.casefold() != configured["repository"].casefold(): - findings.append( - _finding( - "github.repository", - f"The pull request belongs to {_display(repository)}, " - f"not {_display(configured['repository'])}.", - ) - ) - if facts["base_ref"] != configured["base_branch"]: - findings.append( - _finding( - "github.base_branch", - f"The pull request targets {_display(facts['base_ref'])}, " - f"not {_display(configured['base_branch'])}.", - ) - ) - pr_url = proof.get("pr_url") - match = ( - _PULL_PATH.fullmatch(urlsplit(pr_url).path.rstrip("/")) - if _github_https_url(pr_url) - else None - ) - if match is None: + if repository.casefold() != settings["repository"].casefold(): findings.append( _finding( - "github.pr_url", - "pr_url must be https://github.com///pull/.", + "github.repository", + f"The pull request belongs to {_display(repository)}, " + f"not {_display(settings['repository'])}.", ) ) - else: - if f"{match.group(1)}/{match.group(2)}".casefold() != repository.casefold(): - findings.append( - _finding( - "github.pr_repository", - "pr_url names a different repository than the GitHub facts.", - ) - ) - if int(match.group(3)) != facts["pull_request_number"]: - findings.append( - _finding( - "github.pull_request_number", - "pr_url names a different pull request than the GitHub facts.", - ) + 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.") @@ -460,11 +477,10 @@ def compare_github_facts( "for the pull request head commit.", ) ) - findings.extend(_integrity_findings(facts, configured)) + 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: diff --git a/external_checkers/harnessy_proof/publish_local.py b/external_checkers/harnessy_proof/publish_local.py index 1b722adee..874bac18f 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" @@ -49,61 +48,42 @@ def _hash(value: object) -> str: return "sha256:" + hashlib.sha256(encoded.encode("utf-8")).hexdigest() -# PRE has no GitHub facts, so only shape mode is valid there. POST accepts shape -# mode ({}) or GitHub comparison mode naming both repository and base branch, -# optionally with integrity options whose defaults are the safe ones. -CONFIGURATION_SCHEMAS: dict[str, dict[str, object]] = { - "pre_submit": {"type": "object", "additionalProperties": False, "maxProperties": 0}, - "post_submit": { - "type": "object", - "additionalProperties": False, - "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}, - }, +# 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}, }, - # Every integrity option belongs to GitHub comparison mode. - "dependentRequired": { - "github_repository": ["base_branch"], - "base_branch": ["github_repository"], - "allow_unbound_required_checks": ["github_repository"], - "protected_paths": ["github_repository"], - "required_independent_approvals": ["github_repository"], - "operator_logins": ["github_repository"], + "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 only for bindings in -# GitHub comparison mode. The checker validates the full facts shape itself. -INPUT_SCHEMAS: dict[str, dict[str, object]] = { - "pre_submit": {"type": "object"}, - "post_submit": { - "type": "object", - "properties": { - "github_facts": { - "type": "object", - "required": ["schema_version"], - "properties": {"schema_version": {"const": "workstream.github_facts.v1"}}, - } - }, +# 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"}}, + } }, } @@ -118,17 +98,17 @@ def _schema(schema_id: str, document: dict[str, object] | None = None) -> dict[s } -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", CONFIGURATION_SCHEMAS[phase] + "harnessy.work_proof.configuration", CONFIGURATION_SCHEMA ), - "input_schema": _schema(INPUT_SCHEMA_IDS[phase], INPUT_SCHEMAS[phase]), + "input_schema": _schema(INPUT_SCHEMA_ID, INPUT_SCHEMA), "output_schema": _schema("external_checker_result"), "resources": { "cpu_millis": 500, @@ -208,8 +188,8 @@ def main(argv: list[str] | None = None) -> int: parser.add_argument("--sandbox-gid", type=int, required=True) parser.add_argument("--out-dir", type=Path, required=True) # The registry holds one immutable entry per phase, capability and version, - # so a rebuilt image (new digest) needs a new version. v2 adds GitHub - # comparison mode and real configuration/input schemas. + # 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) @@ -221,11 +201,8 @@ def main(argv: list[str] | None = None) -> int: sandbox_uid=args.sandbox_uid, sandbox_gid=args.sandbox_gid, ), - "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(): From d54ebcfa6c38362a4606baf8c57531bd1454971e Mon Sep 17 00:00:00 2001 From: Julian Duru Date: Sat, 24 Oct 2026 15:49:14 +0100 Subject: [PATCH 4/4] fix(checker): report a non-regular proof.json instead of crashing Coverage review: a proof.json directory made read_proof raise; check the file type on the raw descriptor first. Add rejecting tests for bounded check names, single-segment globs, non-regular proof files and the isolated entrypoint. Branch coverage of checker.py is now 100%. Co-Authored-By: Claude Opus 5.5 --- .../WS-GHPROOF-001/WS-GHPROOF-001-03.md | 8 ++- .../external/test_harnessy_proof_checker.py | 54 +++++++++++++++++++ external_checkers/harnessy_proof/checker.py | 7 ++- 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md index a9fb73ed2..0465a1668 100644 --- a/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md +++ b/.commitrail/initiatives/WS-GHPROOF-001/WS-GHPROOF-001-03.md @@ -136,7 +136,8 @@ nothing the contributor could produce. | Claim | Command or proof | Result | Remaining uncertainty | |---|---|---|---| -| Rules and result contract | `pytest tests/checkers/external/test_harnessy_proof_checker.py` | 108 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 | +| 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 | — | @@ -144,6 +145,11 @@ 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 diff --git a/backend/tests/checkers/external/test_harnessy_proof_checker.py b/backend/tests/checkers/external/test_harnessy_proof_checker.py index 948ab106e..8d8bc80d0 100644 --- a/backend/tests/checkers/external/test_harnessy_proof_checker.py +++ b/backend/tests/checkers/external/test_harnessy_proof_checker.py @@ -5,7 +5,10 @@ import importlib.util import io import json +import os from pathlib import Path +import subprocess +import sys from uuid import uuid4 import pytest @@ -587,3 +590,54 @@ def test_published_service_paths_match_the_compose_overlay() -> None: ] mode = ":ro" if name in ("backend", "worker", "beat") else "" assert socket_mounts == [f"checker_socket:/run/ws-checker{mode}"], name + + +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/external_checkers/harnessy_proof/checker.py b/external_checkers/harnessy_proof/checker.py index bf22604dd..46994d8b0 100644 --- a/external_checkers/harnessy_proof/checker.py +++ b/external_checkers/harnessy_proof/checker.py @@ -487,9 +487,12 @@ def read_proof(input_root: Path) -> bytes | None: 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)