From 9f66142faf0285c2081ba5f44b188be8297f2304 Mon Sep 17 00:00:00 2001 From: Dominikus Nold Date: Tue, 14 Apr 2026 22:27:57 +0200 Subject: [PATCH] Updating docs and fix code review findings --- README.md | 2 +- docs/authoring/module-signing.md | 12 +- docs/authoring/publishing-modules.md | 2 +- .../test_sign_modules_on_approval.py | 144 ++++++++++++++---- 4 files changed, 125 insertions(+), 35 deletions(-) diff --git a/README.md b/README.md index 7e456b26..ed4e049b 100644 --- a/README.md +++ b/README.md @@ -50,7 +50,7 @@ hatch run specfact code review run --json --out .specfact/code-review.json **Module signatures:** `pr-orchestrator` enforces `--require-signature` only for events targeting **`main`**; for **`dev`** (and feature branches) CI checks checksums and version bumps without requiring a cryptographic signature yet. Add `--require-signature` to the `verify-modules-signature` command when you want the same bar as **`main`** (for example before merging to `main`). Pre-commit runs `scripts/pre-commit-verify-modules-signature.sh`, which mirrors that policy (signatures required on branch `main`, or when `GITHUB_BASE_REF=main` in Actions). -**CI signing:** Approved PRs to `dev` or `main` from **this repository** (not forks) run `.github/workflows/sign-modules-on-approval.yml`, which can commit signed manifests using repository secrets. See [Module signing](/authoring/module-signing/). +**CI signing:** Approved PRs to `dev` or `main` from **this repository** (not forks) run `.github/workflows/sign-modules-on-approval.yml`, which can commit signed manifests using repository secrets. See [Module signing](./docs/authoring/module-signing.md). To mirror CI locally with git hooks, enable pre-commit: diff --git a/docs/authoring/module-signing.md b/docs/authoring/module-signing.md index aec84e0e..9f02de93 100644 --- a/docs/authoring/module-signing.md +++ b/docs/authoring/module-signing.md @@ -89,9 +89,17 @@ hatch run python scripts/sign-modules.py \ --base-ref origin/dev \ --bump-version patch -# Verify after signing (must match sign payload mode). For a dev-targeting branch, CI omits -# --require-signature; add it when checking as for main: +# Verify after signing (must match sign payload mode). This matches dev-targeting CI: checksum + +# version policy only—dev CI omits --require-signature: +hatch run python scripts/verify-modules-signature.py --payload-from-filesystem --enforce-version-bump --version-check-base origin/dev + +# Main-equivalent (strict) verification: dev CI does not run this, but use it locally when you want +# cryptographic signatures enforced like on main. Same verifier flags as above, plus +# --require-signature. Example with --version-check-base origin/dev (typical feature → dev PR); +# before merging to main, point --version-check-base at origin/main so version policy matches the +# integration target: hatch run python scripts/verify-modules-signature.py --require-signature --payload-from-filesystem --enforce-version-bump --version-check-base origin/dev +hatch run python scripts/verify-modules-signature.py --require-signature --payload-from-filesystem --enforce-version-bump --version-check-base origin/main ``` Wrapper for single manifest: diff --git a/docs/authoring/publishing-modules.md b/docs/authoring/publishing-modules.md index f8166bfe..0f280ea9 100644 --- a/docs/authoring/publishing-modules.md +++ b/docs/authoring/publishing-modules.md @@ -82,7 +82,7 @@ Repository workflow `.github/workflows/publish-modules.yml`: - Bump module `version` in `module-package.yaml` whenever payload or manifest content changes; keep versions immutable for published artifacts. - Use `namespace/name` for any module you publish to a registry. -- Before releasing from **`main`**, run `scripts/verify-modules-signature.py --require-signature --payload-from-filesystem` (or your registry’s policy). On **`dev`**, CI may accept checksum-only manifests until promotion; align with your registry’s requirements. +- Before releasing from **`main`**, run `scripts/verify-modules-signature.py --require-signature --enforce-version-bump --payload-from-filesystem` (or your registry’s policy). On **`dev`**, only signature strictness differs: CI may accept checksum-only manifests (omit `--require-signature`), but when signed module assets or manifests are in play you should still run `scripts/verify-modules-signature.py --enforce-version-bump --payload-from-filesystem` so version bumps stay enforced; add `--require-signature` when you want the same bar as **`main`** before promotion. - Prefer `--download-base-url` and `--index-fragment` when integrating with a custom registry index. ## See also diff --git a/tests/unit/workflows/test_sign_modules_on_approval.py b/tests/unit/workflows/test_sign_modules_on_approval.py index fa2993b6..c4d0cf49 100644 --- a/tests/unit/workflows/test_sign_modules_on_approval.py +++ b/tests/unit/workflows/test_sign_modules_on_approval.py @@ -1,6 +1,9 @@ from __future__ import annotations from pathlib import Path +from typing import Any, cast + +import yaml REPO_ROOT = Path(__file__).resolve().parents[3] @@ -11,19 +14,70 @@ def _workflow_text() -> str: return path.read_text(encoding="utf-8") +def _parsed_workflow() -> dict[Any, Any]: + loaded = yaml.safe_load(_workflow_text()) + assert isinstance(loaded, dict) + return cast(dict[Any, Any], loaded) + + +def _workflow_on_section(doc: dict[Any, Any]) -> dict[str, Any]: + """Top-level workflow triggers. PyYAML 1.1 parses the key ``on`` as bool ``True``.""" + section = doc.get(True) + if isinstance(section, dict): + return cast(dict[str, Any], section) + raw = doc.get("on") + assert isinstance(raw, dict) + return cast(dict[str, Any], raw) + + +def _sign_modules_job(doc: dict[Any, Any]) -> dict[str, Any]: + jobs = doc["jobs"] + assert isinstance(jobs, dict) + job = jobs["sign-modules"] + assert isinstance(job, dict) + return cast(dict[str, Any], job) + + +def _step_by_field(steps: list[Any], field: str, value: str) -> dict[str, Any]: + for raw in steps: + if isinstance(raw, dict) and raw.get(field) == value: + return cast(dict[str, Any], raw) + raise AssertionError(f"No workflow step with {field}={value!r}") + + +def _assert_pull_request_review_submitted(doc: dict[Any, Any]) -> None: + on = _workflow_on_section(doc) + pr_review = on["pull_request_review"] + assert isinstance(pr_review, dict) + assert pr_review["types"] == ["submitted"] + + +def _assert_sign_job_branch_filters(doc: dict[Any, Any]) -> None: + job = _sign_modules_job(doc) + job_if = job["if"] + assert isinstance(job_if, str) + assert "github.event.review.state == 'approved'" in job_if + assert "github.event.pull_request.base.ref == 'dev'" in job_if + assert "github.event.pull_request.base.ref == 'main'" in job_if + assert "github.event.pull_request.head.repo.full_name == github.repository" in job_if + + +def _assert_concurrency_and_permissions(doc: dict[Any, Any]) -> None: + conc = doc["concurrency"] + assert isinstance(conc, dict) + assert conc["cancel-in-progress"] is True + assert "${{ github.event.pull_request.number }}" in conc["group"] + + perms = doc["permissions"] + assert isinstance(perms, dict) + assert perms["contents"] == "write" + + def test_sign_modules_on_approval_trigger_and_job_filter() -> None: - workflow = _workflow_text() - assert "pull_request_review:" in workflow - assert "types:" in workflow - assert "submitted" in workflow - assert "github.event.review.state == 'approved'" in workflow - assert "github.event.pull_request.base.ref == 'dev'" in workflow - assert "github.event.pull_request.base.ref == 'main'" in workflow - assert "github.event.pull_request.head.repo.full_name == github.repository" in workflow - assert "concurrency:" in workflow - assert "cancel-in-progress: true" in workflow - assert "permissions:" in workflow - assert "contents: write" in workflow + doc = _parsed_workflow() + _assert_pull_request_review_submitted(doc) + _assert_sign_job_branch_filters(doc) + _assert_concurrency_and_permissions(doc) def test_sign_modules_on_approval_checkout_and_python() -> None: @@ -47,15 +101,17 @@ def test_sign_modules_on_approval_dependencies_and_discover() -> None: def test_sign_modules_on_approval_secrets_guard() -> None: - workflow = _workflow_text() - assert "Guard signing secrets" in workflow - assert '[ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY:-}" ]' in workflow - assert '[ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE:-}" ]' in workflow - assert "Missing secret: SPECFACT_MODULE_PRIVATE_SIGN_KEY" in workflow - assert "Missing secret: SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE" in workflow - assert "exit 1" in workflow - assert "SPECFACT_MODULE_PRIVATE_SIGN_KEY" in workflow - assert "SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE" in workflow + job = _sign_modules_job(_parsed_workflow()) + steps = job["steps"] + assert isinstance(steps, list) + guard = _step_by_field(steps, "name", "Guard signing secrets") + run = guard["run"] + assert isinstance(run, str) + assert '[ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY:-}" ]' in run + assert '[ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE:-}" ]' in run + assert 'echo "::error::Missing secret: SPECFACT_MODULE_PRIVATE_SIGN_KEY"' in run + assert 'echo "::error::Missing secret: SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE"' in run + assert run.count("exit 1") >= 2 def test_sign_modules_on_approval_sign_step_merge_base() -> None: @@ -72,14 +128,40 @@ def test_sign_modules_on_approval_sign_step_merge_base() -> None: assert "--payload-from-filesystem" in workflow +def _assert_discover_step_writes_outputs(steps: list[Any]) -> None: + discover = _step_by_field(steps, "id", "discover") + discover_run = discover["run"] + assert isinstance(discover_run, str) + assert "manifests_count=" in discover_run + assert "GITHUB_OUTPUT" in discover_run + + +def _assert_commit_and_push_step(steps: list[Any]) -> None: + commit_step = _step_by_field(steps, "name", "Commit and push signed manifests") + assert commit_step.get("id") == "commit" + commit_run = commit_step["run"] + assert isinstance(commit_run, str) + assert 'git commit -m "chore(modules): ci sign changed modules [skip ci]"' in commit_run + assert 'git push origin "HEAD:${PR_HEAD_REF}"' in commit_run + assert "Push to ${PR_HEAD_REF} failed" in commit_run + + +def _assert_job_summary_step(steps: list[Any]) -> None: + summary = _step_by_field(steps, "name", "Write job summary") + assert summary.get("if") == "always()" + env = summary["env"] + assert isinstance(env, dict) + assert env["COMMIT_CHANGED"] == "${{ steps.commit.outputs.changed }}" + assert env["MANIFESTS_COUNT"] == "${{ steps.discover.outputs.manifests_count }}" + summary_run = summary["run"] + assert isinstance(summary_run, str) + assert "GITHUB_STEP_SUMMARY" in summary_run + + def test_sign_modules_on_approval_commit_push_and_summary() -> None: - workflow = _workflow_text() - assert "github-actions[bot]" in workflow - assert "chore(modules): ci sign changed modules [skip ci]" in workflow - assert "git push origin" in workflow - assert "Push to ${PR_HEAD_REF} failed" in workflow - assert "HEAD:${PR_HEAD_REF}" in workflow - assert "GITHUB_STEP_SUMMARY" in workflow - assert "COMMIT_CHANGED:" in workflow - assert "MANIFESTS_COUNT:" in workflow - assert "steps.discover.outputs.manifests_count" in workflow + job = _sign_modules_job(_parsed_workflow()) + steps = job["steps"] + assert isinstance(steps, list) + _assert_discover_step_writes_outputs(steps) + _assert_commit_and_push_step(steps) + _assert_job_summary_step(steps)