From e83048ae55eb0cd1fe2dbf61d806923ff1cebe9b Mon Sep 17 00:00:00 2001 From: Jim Cresswell Date: Thu, 18 Jun 2026 13:42:19 +0100 Subject: [PATCH 1/3] feat(release): continuous release on merge via the Oak Semantic Release Bot Replace the release-PR pattern with continuous semantic-release. After CI passes on main, the Release workflow computes the Conventional Commits increment and the Oak Semantic Release Bot (a main-ruleset bypass actor) bumps pyproject + uv.lock + CHANGELOG, commits + tags, and pushes straight to protected main, then builds and publishes the GitHub Release with the wheel + sdist. So every qualifying merge advances the version, which is what the owner asked for. Mechanics: - trigger on workflow_run after CI success on main (release only when green), plus workflow_dispatch for a manual major; - authenticate with actions/create-github-app-token (RELEASE_APP_ID / RELEASE_APP_PRIVATE_KEY), checkout with that token so the push bypasses the ruleset; - the bump commit carries [skip ci] so it does not re-trigger CI -> Release (an infinite loop). A secondary guard: a re-run finds no commits since the fresh tag and stands down. Majors stay manual (owner decision): a breaking marker makes the auto-release stand down, and the new prevent-accidental-major commit-msg hook (tools/prevent_accidental_major.py, reusing release_increment.is_breaking) rejects the marker at commit time so it cannot land by accident and silently halt releases. The hook is protected from SKIP in the agent guardrail policy. audit_release_workflow now requires the workflow_run trigger and the [skip ci] loop guard; docs (README, dev-tooling, governance) updated, and the stale coverage-doc figure corrected to the branch-aware floor of 86. Co-Authored-By: Claude Opus 4.8 (1M context) --- .agent/hooks/policy.json | 3 +- .github/workflows/release.yml | 195 ++++++++++++------------- .gitignore | 2 + .pre-commit-config.yaml | 5 + README.md | 28 ++-- docs/dev-tooling.md | 40 ++--- docs/repository-governance.md | 13 +- tests/test_agent_hooks.py | 6 +- tests/test_prevent_accidental_major.py | 48 ++++++ tests/test_repo_audit.py | 29 +++- tools/prevent_accidental_major.py | 49 +++++++ tools/repo_audit.py | 22 ++- 12 files changed, 289 insertions(+), 151 deletions(-) create mode 100644 tests/test_prevent_accidental_major.py create mode 100644 tools/prevent_accidental_major.py diff --git a/.agent/hooks/policy.json b/.agent/hooks/policy.json index d259efa..b9073e8 100644 --- a/.agent/hooks/policy.json +++ b/.agent/hooks/policy.json @@ -69,7 +69,8 @@ ], "blocked_pre_commit_skip_ids": [ "quality-gates", - "commitizen-commit-msg" + "commitizen-commit-msg", + "prevent-accidental-major" ], "blocked_pre_commit_skip_reason": "SKIP is prohibited here when it bypasses the repo's quality-gates or commitizen-commit-msg hooks.", "blocked_dynamic_git_config_reason": "Dynamic git config is prohibited here for git commit and git push because it can hide hook bypasses or force pushes." diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 078b6b9..9a93c24 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,24 +1,25 @@ name: Release -# Release automation via the release-PR pattern, so the committed version in -# pyproject.toml can advance under the protected `main` ruleset (no direct push, -# no bypass token). Two phases, selected by whether the committed version already -# has a matching tag: -# - prepare: committed version IS tagged -> compute the next version from -# Conventional Commits (Commitizen custom bump map) and open/refresh a -# "release" PR that bumps pyproject + CHANGELOG. Breaking changes stand down -# for a manual major. -# - publish: committed version is NOT tagged (a release PR just merged) -> -# tag it, build, and create the GitHub Release with the wheel + sdist. -# Major releases are manual: run this workflow via "Run workflow" with -# increment = MAJOR. +# Continuous release on merge to main. After CI passes on main, compute the +# Conventional Commits increment (custom policy via tools/release_increment.py), +# bump pyproject + uv.lock + CHANGELOG with Commitizen, then commit + tag and push +# straight to the protected `main` branch using the Oak Semantic Release Bot +# GitHub App token (a ruleset bypass actor). The bump commit carries `[skip ci]` +# so it does not re-trigger CI and loop. Every qualifying merge therefore advances +# the version and publishes a GitHub Release with the wheel + sdist. +# +# Major releases stay manual: a `!`/`BREAKING CHANGE` marker makes the auto-release +# stand down (and tools/prevent_accidental_major.py blocks the marker at commit +# time); cut the major deliberately via "Run workflow" with increment = MAJOR. on: - push: + workflow_run: + workflows: [CI] + types: [completed] branches: [main] workflow_dispatch: inputs: increment: - description: "Release increment to prepare (use MAJOR for a deliberate major release)" + description: "Release increment to force (use MAJOR for a deliberate major release)" type: choice options: [MAJOR, MINOR, PATCH] default: MAJOR @@ -28,23 +29,38 @@ on: permissions: contents: read -# Serialise releases on a ref; never cancel a release mid-flight. +# Serialise releases so a rapid second merge never races the in-flight bump push. concurrency: - group: release-${{ github.ref }} + group: release cancel-in-progress: false jobs: release: name: Release + # Only release after CI succeeded on main, or on a deliberate manual dispatch. + if: >- + github.event_name == 'workflow_dispatch' + || github.event.workflow_run.conclusion == 'success' runs-on: ubuntu-latest permissions: contents: write - pull-requests: write steps: + # Mint a short-lived installation token for the release bot. The bot is a + # bypass actor on the `main` ruleset, so the bump commit + tag push through. + - name: Generate the release-bot token + id: app-token + uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1 + with: + app-id: ${{ secrets.RELEASE_APP_ID }} + private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }} + - name: Check out full history and tags uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: fetch-depth: 0 + # Keep the bot token in the git config so `git push` to protected main + # is authenticated as the bypass actor. + token: ${{ steps.app-token.outputs.token }} - name: Install uv uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0 @@ -57,34 +73,75 @@ jobs: - name: Configure the bot git identity run: | - git config user.name "github-actions[bot]" - git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git config user.name "oak-semantic-release-bot[bot]" + git config user.email "oak-semantic-release-bot[bot]@users.noreply.github.com" - - name: Determine the release phase - id: phase + - name: Compute the release increment + id: increment + env: + FORCED_INCREMENT: ${{ inputs.increment }} + EVENT_NAME: ${{ github.event_name }} run: | set -euo pipefail version=$(uv run python -c "import tomllib, pathlib; print(tomllib.loads(pathlib.Path('pyproject.toml').read_text())['project']['version'])") - echo "version=${version}" >> "$GITHUB_OUTPUT" - if git rev-parse -q --verify "refs/tags/v${version}" >/dev/null; then - phase=prepare + prev="v${version}" + echo "previous=${prev}" >> "$GITHUB_OUTPUT" + if [ "${EVENT_NAME}" = "workflow_dispatch" ]; then + increment="${FORCED_INCREMENT}" + elif git rev-parse -q --verify "refs/tags/${prev}" >/dev/null; then + # cz_conventional_commits ignores [tool.commitizen].bump_map, so we + # compute the increment from that single policy source ourselves. + increment="$(git log "${prev}..HEAD" --pretty=format:'%B%x00' | uv run python tools/release_increment.py)" else - phase=publish + # The committed version has no tag yet (bootstrap) — release it as-is. + increment="PATCH" fi - echo "phase=${phase}" >> "$GITHUB_OUTPUT" - echo "Committed version v${version} -> phase ${phase}" + case "${increment}" in + BREAKING) + echo "::warning::Breaking change since ${prev}; cut a major via Run workflow (increment = MAJOR). Auto-release stood down." + echo "release=false" >> "$GITHUB_OUTPUT" + ;; + NONE) + echo "No releasable commits since ${prev}." + echo "release=false" >> "$GITHUB_OUTPUT" + ;; + MAJOR | MINOR | PATCH) + echo "increment=${increment}" >> "$GITHUB_OUTPUT" + echo "release=true" >> "$GITHUB_OUTPUT" + ;; + *) + echo "unexpected increment '${increment}'" + exit 1 + ;; + esac - # --- PUBLISH: the committed version has no tag, so a release PR just merged. - - name: Tag and publish the GitHub Release - if: steps.phase.outputs.phase == 'publish' + - name: Bump, tag, and push to main + id: bump + if: steps.increment.outputs.release == 'true' env: - GH_TOKEN: ${{ github.token }} - VERSION: ${{ steps.phase.outputs.version }} + INCREMENT: ${{ steps.increment.outputs.increment }} + PREV: ${{ steps.increment.outputs.previous }} + run: | + set -euo pipefail + uv run cz bump --increment "${INCREMENT}" --changelog --files-only --yes + version=$(uv run python -c "import tomllib, pathlib; print(tomllib.loads(pathlib.Path('pyproject.toml').read_text())['project']['version'])") + if [ -z "${version}" ]; then echo "could not read the bumped version"; exit 1; fi + # `[skip ci]` stops the bump push from re-triggering CI -> Release (a loop). + git add pyproject.toml uv.lock CHANGELOG.md + git commit -m "bump: version ${PREV} -> v${version} [skip ci]" + git tag -a "v${version}" -m "v${version}" + git push origin HEAD:main + git push origin "v${version}" + echo "version=${version}" >> "$GITHUB_OUTPUT" + + - name: Build and publish the GitHub Release + if: steps.increment.outputs.release == 'true' + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + VERSION: ${{ steps.bump.outputs.version }} run: | set -euo pipefail tag="v${VERSION}" - git tag -a "${tag}" -m "${tag}" - git push origin "${tag}" uv build notes="$(mktemp)" if uv run cz changelog --dry-run "${VERSION}" > "${notes}" 2>/dev/null && [ -s "${notes}" ]; then @@ -92,73 +149,3 @@ jobs: else gh release create "${tag}" --title "${tag}" --generate-notes ./dist/* fi - - - name: Note an ignored manual dispatch - if: steps.phase.outputs.phase == 'publish' && github.event_name == 'workflow_dispatch' - env: - VERSION: ${{ steps.phase.outputs.version }} - run: | - echo "::warning::A manual release was requested, but committed version v${VERSION} is not yet tagged, so the pending release was published instead. Re-run after it completes to prepare the next release." - - # --- PREPARE: the committed version is tagged, so look for the next release. - - name: Compute the next release and bump the files - id: prepare - if: steps.phase.outputs.phase == 'prepare' - env: - PREV_VERSION: ${{ steps.phase.outputs.version }} - INCREMENT: ${{ inputs.increment }} - EVENT_NAME: ${{ github.event_name }} - run: | - set -euo pipefail - prev="v${PREV_VERSION}" - if [ "${EVENT_NAME}" = "workflow_dispatch" ]; then - increment="${INCREMENT}" - else - # cz_conventional_commits ignores [tool.commitizen].bump_map, so we - # compute the increment from that policy ourselves and pass it - # explicitly below (see tools/release_increment.py). - increment="$(git log "${prev}..HEAD" --pretty=format:'%B%x00' | uv run python tools/release_increment.py)" - case "${increment}" in - BREAKING) - echo "::warning::Breaking change detected since ${prev}. Cut a major manually via Run workflow (increment = MAJOR); auto-release stood down." - echo "prepared=false" >> "$GITHUB_OUTPUT" - exit 0 - ;; - NONE) - echo "No releasable commits since ${prev}." - echo "prepared=false" >> "$GITHUB_OUTPUT" - exit 0 - ;; - MINOR | PATCH) ;; - *) - echo "unexpected increment '${increment}'" - exit 1 - ;; - esac - fi - uv run cz bump --increment "${increment}" --files-only --changelog --yes - next=$(uv run python -c "import tomllib, pathlib; print(tomllib.loads(pathlib.Path('pyproject.toml').read_text())['project']['version'])") - if [ -z "${next}" ]; then echo "could not read the bumped version"; exit 1; fi - echo "version=${next}" >> "$GITHUB_OUTPUT" - echo "prepared=true" >> "$GITHUB_OUTPUT" - - # Pinned to a commit SHA (this action runs with the job's - # contents/pull-requests write token); Dependabot bumps it via the comment. - - name: Open or refresh the release PR - if: steps.phase.outputs.phase == 'prepare' && steps.prepare.outputs.prepared == 'true' - uses: peter-evans/create-pull-request@5f6978faf089d4d20b00c7766989d076bb2fc7f1 # v8.1.1 - with: - branch: release/next - base: main - title: "chore(release): v${{ steps.prepare.outputs.version }}" - commit-message: "chore(release): v${{ steps.prepare.outputs.version }}" - labels: release - body: | - Automated release PR. Merging this cuts release **v${{ steps.prepare.outputs.version }}**. - - It lands the version bump in `pyproject.toml`, `uv.lock`, and - `CHANGELOG.md`; the Release workflow then tags `v${{ steps.prepare.outputs.version }}` - and publishes the GitHub Release with the built wheel + sdist. - - Note: this PR is opened with the default token, so CI does not run on - it automatically. diff --git a/.gitignore b/.gitignore index a21d8c6..80cabd2 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,8 @@ __pycache__/ .pytest_cache/ .hypothesis/ .ruff_cache/ +.sonarlint/ +.vscode/ .coverage .coverage.* coverage.xml diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 028322e..2de0375 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -12,6 +12,11 @@ repos: language: system stages: [pre-commit, pre-push] pass_filenames: false + - id: prevent-accidental-major + name: prevent accidental major version + entry: uv run python -m tools.prevent_accidental_major + language: system + stages: [commit-msg] - id: commitizen-commit-msg name: commit message (commitizen) entry: uv run cz check --allow-abort --commit-msg-file diff --git a/README.md b/README.md index 2080f11..8acb415 100644 --- a/README.md +++ b/README.md @@ -174,15 +174,12 @@ uv run cz check --message "feat: add truthful commit-msg enforcement" ## Releases -Releases are automated from Conventional Commits, with the version kept -**committed** in `pyproject.toml`. Because `main` is protected, the bump flows -through a one-click **release PR**: - -1. Merging changes to `main` makes the Release workflow open or refresh a - `chore(release): vX.Y.Z` PR that bumps `pyproject.toml`, `uv.lock`, and - `CHANGELOG.md`. -2. Merging that release PR tags `vX.Y.Z` and publishes a GitHub Release with the - built wheel + sdist attached. +Releases are **continuous**: every merge to `main` advances the version. After +CI passes on `main`, the Release workflow computes the Conventional Commits +increment, and the **Oak Semantic Release Bot** (a `main`-ruleset bypass actor) +bumps `pyproject.toml`, `uv.lock`, and `CHANGELOG.md`, commits + tags `vX.Y.Z`, +pushes straight to `main`, and publishes a GitHub Release with the built wheel + +sdist attached. The bump commit carries `[skip ci]` so it does not loop. The bump level is computed by Commitizen with this repo's policy: @@ -192,9 +189,16 @@ The bump level is computed by Commitizen with this repo's policy: | everything else (`chore`, `docs`, `perf`, `refactor`, `build`, `ci`, …) | patch | | `!` / `BREAKING CHANGE` | no auto-release — a **major** is required | -**Major versions are manual.** A breaking change makes the automation stand -down; cut the major deliberately via the Release workflow's *Run workflow* -button (`increment = MAJOR`). Releases publish to GitHub Releases only (no PyPI). +**Major versions are manual.** A breaking marker makes the auto-release stand +down; cut the major deliberately via the Release workflow's *Run workflow* button +(`increment = MAJOR`). To stop a breaking marker landing by accident (which would +silently halt the auto-release), the `prevent-accidental-major` commit-msg hook +rejects `type!:` / `BREAKING CHANGE` in commits. Releases publish to GitHub +Releases only (no PyPI). + +The workflow needs the `RELEASE_APP_ID` / `RELEASE_APP_PRIVATE_KEY` secrets and +the bot added as a ruleset bypass actor — see +[docs/repository-governance.md](docs/repository-governance.md). ## Governance diff --git a/docs/dev-tooling.md b/docs/dev-tooling.md index bfc3d83..fcd4f44 100644 --- a/docs/dev-tooling.md +++ b/docs/dev-tooling.md @@ -140,13 +140,15 @@ Its package identity follows the Oak Python convention: ## Coverage reporting -- the `coverage` gate runs `pytest` under `coverage.py`; locally it prints the - `term-missing` report and fails under the threshold in `[tool.coverage.report]` - (`fail_under = 85`, an honest floor below the ~88% the suite achieves) +- the `coverage` gate runs `pytest` under `coverage.py` with **branch coverage** + on; locally it prints the `term-missing` report and fails under the threshold + in `[tool.coverage.report]` (`fail_under = 86`, an honest floor below the + ~87.6% the suite achieves with branches counted) - the `coverage-contract` `repo-audit` check guards what the coverage gate - itself cannot: it fails if `fail_under` drops below 85, or if - `[tool.coverage.run].omit` excludes any file beyond the justified set - (`devtools.py`) — so the threshold cannot be quietly lowered and code cannot be + itself cannot: it fails if `fail_under` drops below 86, if branch coverage is + switched off, or if `[tool.coverage.run].omit` excludes any file beyond the + justified set (`devtools.py`) — so the threshold cannot be quietly lowered, + branch coverage cannot be disabled to inflate the number, and code cannot be hidden from the coverage denominator. Raising `fail_under` is always allowed - CI additionally derives a Cobertura report from the same run with the `coverage xml` subcommand — the `coverage` gate's `pytest --cov` run writes the @@ -185,25 +187,29 @@ uv run cz check --message "docs: explain the Commitizen workflow" ## Releases -- automated by `.github/workflows/release.yml` using the **release-PR pattern**, - so the committed `pyproject.toml` version advances under the protected `main` - ruleset with no direct push and no bypass token +- automated by `.github/workflows/release.yml` as **continuous release on merge**: + it triggers on `workflow_run` after CI succeeds on `main`, so a release only + cuts once the gates are green - the bump policy is `feat`/`fix` → minor, everything else → patch, and breaking markers are **not** auto-mapped to major; it lives in `[tool.commitizen].bump_map` but, because `cz_conventional_commits` ignores that map, `tools/release_increment.py` reads it and computes the increment, which the workflow applies via `cz bump --increment` -- on a push to `main` the workflow either *prepares* (opens/refreshes a - `chore(release)` PR that bumps `pyproject.toml`, `uv.lock`, and `CHANGELOG.md`) - or *publishes* (when the committed version has no tag yet — i.e. a release PR - just merged — it tags `vX.Y.Z`, runs `uv build`, and creates the GitHub Release - with the wheel + sdist attached) +- the **Oak Semantic Release Bot** GitHub App (a `main`-ruleset bypass actor) + authenticates via `actions/create-github-app-token` (secrets `RELEASE_APP_ID` + - `RELEASE_APP_PRIVATE_KEY`); the workflow bumps `pyproject.toml`, `uv.lock`, + and `CHANGELOG.md`, commits + tags `vX.Y.Z`, pushes straight to `main`, then + `uv build`s and creates the GitHub Release with the wheel + sdist attached +- the bump commit is marked `[skip ci]` so pushing it to `main` does not + re-trigger CI → Release (an infinite loop) - **major releases are manual**: a `!`/`BREAKING CHANGE` marker makes the auto-release stand down; cut the major via the workflow's `workflow_dispatch` - (`increment = MAJOR`) + (`increment = MAJOR`). The `prevent-accidental-major` commit-msg hook + (`tools/prevent_accidental_major.py`) rejects the marker at commit time so it + cannot land by accident and silently halt the auto-release - the version stays committed in the tree; releases publish to GitHub Releases - only (no PyPI). `audit_release_workflow` keeps the workflow and the bump policy - honest + only (no PyPI). `audit_release_workflow` keeps the workflow (trigger, `cz bump`, + the increment tool, the `[skip ci]` loop guard) and the bump policy honest ## Hydration guidance diff --git a/docs/repository-governance.md b/docs/repository-governance.md index 3f2cf69..f08421b 100644 --- a/docs/repository-governance.md +++ b/docs/repository-governance.md @@ -29,12 +29,13 @@ gap the in-repo gates structurally cannot. CodeQL check, but **not** these two — so `main` can go red and a PR can still merge. This is the single biggest enforcement gap. -2. **Give the release PR a token.** - The standing release PR is opened by `GITHUB_TOKEN`, so `ci.yml` does not run - on it and it sits `UNSTABLE` indefinitely (merge it with - `gh pr merge --squash --auto`). Provide `create-pull-request` with a PAT - or GitHub App token so `ci.yml` runs on the release PR; it then goes `CLEAN` - and merges without `--auto`. See [Releases](dev-tooling.md#releases). +2. **Release bot (done — keep it wired).** + Continuous release pushes the bump commit + tag straight to protected `main` + via the **Oak Semantic Release Bot** GitHub App. This needs the + `RELEASE_APP_ID` / `RELEASE_APP_PRIVATE_KEY` repo secrets **and** the app added + as a **bypass actor** on the `main` ruleset (both are in place). If the app's + key is rotated or it is removed as a bypass actor, the release push will be + rejected. See [Releases](dev-tooling.md#releases). 3. **Enable GitHub Code Quality (organisation preview).** Coverage is uploaded as Cobertura on every PR, but the upload runs with diff --git a/tests/test_agent_hooks.py b/tests/test_agent_hooks.py index 0c82f8e..07ac3fb 100644 --- a/tests/test_agent_hooks.py +++ b/tests/test_agent_hooks.py @@ -38,7 +38,11 @@ def test_load_policy_reads_the_canonical_repo_policy() -> None: == "Read .agent/commands/start-right-quick.md before substantive work." ) assert policy.quality_gate_message == QUALITY_GATE_MESSAGE - assert policy.blocked_pre_commit_skip_ids == ("quality-gates", "commitizen-commit-msg") + assert policy.blocked_pre_commit_skip_ids == ( + "quality-gates", + "commitizen-commit-msg", + "prevent-accidental-major", + ) assert policy.blocked_pre_commit_skip_reason == SKIP_BYPASS_REASON assert policy.blocked_hook_bypass_env_var_prefixes == ( subject.BlockedPrefix( diff --git a/tests/test_prevent_accidental_major.py b/tests/test_prevent_accidental_major.py new file mode 100644 index 0000000..cb2dfe8 --- /dev/null +++ b/tests/test_prevent_accidental_major.py @@ -0,0 +1,48 @@ +from __future__ import annotations + +from pathlib import Path + +import pytest + +import tools.prevent_accidental_major as subject + + +def _commit_msg(tmp_path: Path, text: str) -> str: + path = tmp_path / "COMMIT_EDITMSG" + path.write_text(text, encoding="utf-8") + return str(path) + + +def test_allows_conventional_non_breaking_commits(tmp_path: Path) -> None: + allowed = [ + "feat: add a thing", + "fix(parser): handle empty input", + "chore: tidy the tree", + # A prose mention of a breaking change (not a marker) must pass. + "docs: explain the breaking change semantics for adopters", + ] + for message in allowed: + assert subject.main([_commit_msg(tmp_path, message)]) == 0, message + + +def test_rejects_a_bang_breaking_subject( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + code = subject.main([_commit_msg(tmp_path, "feat!: drop the legacy API")]) + + assert code == 1 + assert "MAJOR" in capsys.readouterr().err + + +def test_rejects_a_scoped_bang_subject(tmp_path: Path) -> None: + assert subject.main([_commit_msg(tmp_path, "refactor(core)!: rename the entry point")]) == 1 + + +def test_rejects_a_breaking_change_footer(tmp_path: Path) -> None: + message = "feat: a new surface\n\nBREAKING CHANGE: removes the old one\n" + + assert subject.main([_commit_msg(tmp_path, message)]) == 1 + + +def test_errors_without_a_path_argument(tmp_path: Path) -> None: + assert subject.main([]) == 1 diff --git a/tests/test_repo_audit.py b/tests/test_repo_audit.py index 430f24e..34eb1ee 100644 --- a/tests/test_repo_audit.py +++ b/tests/test_repo_audit.py @@ -852,11 +852,16 @@ def test_audit_ci_workflow_reports_missing_gate_command_independently(tmp_path: def _release_workflow_yaml( - *, triggers: str = "both", with_cz_bump: bool = True, with_increment_tool: bool = True + *, + triggers: str = "both", + with_cz_bump: bool = True, + with_increment_tool: bool = True, + with_skip_ci: bool = True, ) -> str: on_block = { - "both": "on:\n push:\n branches: [main]\n workflow_dispatch:\n", - "push-only": "on:\n push:\n branches: [main]\n", + "both": "on:\n workflow_run:\n workflows: [CI]\n types: [completed]\n" + " workflow_dispatch:\n", + "run-only": "on:\n workflow_run:\n workflows: [CI]\n types: [completed]\n", "dispatch-only": "on:\n workflow_dispatch:\n", }[triggers] steps = "" @@ -870,6 +875,8 @@ def _release_workflow_yaml( if with_cz_bump else " - run: uv build\n" ) + if with_skip_ci: + steps += ' - run: git commit -m "bump v0.1.1 [skip ci]"\n' return ( "name: Release\n" + on_block @@ -918,22 +925,30 @@ def test_audit_release_workflow_rejects_a_non_mapping_workflow(tmp_path: Path) - assert ".github/workflows/release.yml must define a top-level mapping" in joined -def test_audit_release_workflow_requires_a_push_trigger(tmp_path: Path) -> None: +def test_audit_release_workflow_requires_a_workflow_run_trigger(tmp_path: Path) -> None: _write_release_world(tmp_path, workflow=_release_workflow_yaml(triggers="dispatch-only")) joined = "\n".join(subject.audit_release_workflow(tmp_path)) - assert "must trigger on push to main" in joined + assert "must trigger on workflow_run" in joined assert "must offer workflow_dispatch" not in joined def test_audit_release_workflow_requires_workflow_dispatch_for_manual_major(tmp_path: Path) -> None: - _write_release_world(tmp_path, workflow=_release_workflow_yaml(triggers="push-only")) + _write_release_world(tmp_path, workflow=_release_workflow_yaml(triggers="run-only")) joined = "\n".join(subject.audit_release_workflow(tmp_path)) assert "must offer workflow_dispatch for manual major releases" in joined - assert "must trigger on push to main" not in joined + assert "must trigger on workflow_run" not in joined + + +def test_audit_release_workflow_requires_the_skip_ci_loop_guard(tmp_path: Path) -> None: + _write_release_world(tmp_path, workflow=_release_workflow_yaml(with_skip_ci=False)) + + joined = "\n".join(subject.audit_release_workflow(tmp_path)) + + assert "[skip ci]" in joined def test_audit_release_workflow_requires_a_cz_bump_step(tmp_path: Path) -> None: diff --git a/tools/prevent_accidental_major.py b/tools/prevent_accidental_major.py new file mode 100644 index 0000000..caf4783 --- /dev/null +++ b/tools/prevent_accidental_major.py @@ -0,0 +1,49 @@ +#!/usr/bin/env python3 +"""Block accidental major-version markers in commit messages (a commit-msg hook). + +Major releases are deliberate in this repo: the release workflow stands down on a +breaking-change marker, and a major is cut manually via "Run workflow" +(increment = MAJOR). A ``type!:`` subject or a ``BREAKING CHANGE`` footer would +therefore *not* cut a major automatically — it would silently stop the +auto-release until a human intervenes. This hook rejects the marker so it cannot +land by accident: describe the breaking change in prose and cut the major +deliberately from the workflow. + +Run as a module (``python -m tools.prevent_accidental_major ``) +so it can reuse ``release_increment.is_breaking`` — keeping one definition of +"what counts as breaking", shared with the release workflow. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +from tools.release_increment import is_breaking + +_REJECTION = ( + "Breaking-change markers (`type!:` or a `BREAKING CHANGE` footer) are not " + "allowed in commits here.\n" + "Major releases are cut manually: use the Release workflow (Run workflow -> " + "increment = MAJOR).\n" + "Describe the breaking change in prose without the marker, then cut the major " + "deliberately.\n" +) + + +def main(argv: list[str] | None = None) -> int: + """Reject a commit message that carries a breaking-change marker.""" + + args = sys.argv[1:] if argv is None else argv + if not args: + sys.stderr.write("prevent_accidental_major: expected a commit-message file path\n") + return 1 + message = Path(args[0]).read_text(encoding="utf-8") + if is_breaking([message]): + sys.stderr.write(_REJECTION) + return 1 + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tools/repo_audit.py b/tools/repo_audit.py index 70f765c..54dcd01 100644 --- a/tools/repo_audit.py +++ b/tools/repo_audit.py @@ -79,7 +79,11 @@ "bypasses or force pushes." ), } -REQUIRED_PRE_COMMIT_SKIP_IDS = ("quality-gates", "commitizen-commit-msg") +REQUIRED_PRE_COMMIT_SKIP_IDS = ( + "quality-gates", + "commitizen-commit-msg", + "prevent-accidental-major", +) REQUIRED_PRE_COMMIT_SKIP_REASON = ( "SKIP is prohibited here when it bypasses the repo's quality-gates or " "commitizen-commit-msg hooks." @@ -692,8 +696,11 @@ def audit_release_workflow(root: Path) -> list[str]: require( failures, check, - "push" in triggers, - f".github/workflows/release.yml must trigger on push to main (found: {found})", + "workflow_run" in triggers, + ( + ".github/workflows/release.yml must trigger on workflow_run (release " + f"after CI succeeds on main) (found: {found})" + ), ) require( failures, @@ -721,6 +728,15 @@ def audit_release_workflow(root: Path) -> list[str]: "the custom bump_map)" ), ) + require( + failures, + check, + any("[skip ci]" in command for command in run_commands), + ( + ".github/workflows/release.yml must mark the bump commit `[skip ci]` so " + "pushing it to main does not re-trigger CI -> Release (an infinite loop)" + ), + ) else: require( failures, From f48a85f0d1b4f4b077d9b9664aa8c56cc6817369 Mon Sep 17 00:00:00 2001 From: Jim Cresswell Date: Thu, 18 Jun 2026 13:54:51 +0100 Subject: [PATCH 2/3] fix(release): harden the release workflow and guard from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopted the substantive pre-merge review findings: - bootstrap (a fresh fork whose committed version is untagged) now releases that version as-is instead of forcing a PATCH bump, so an adopter's first version is the one they chose; - run `uv lock` after the bump so uv.lock's project version always matches pyproject, even if a future uv stops updating it during `cz bump`; - the publish step now gates on the release step having set its version output, so it can never run against a half-applied release; - prevent_accidental_major handles an unreadable commit-message file with a clean error instead of a traceback, with a test for the missing-file path; - the blocked-pre-commit-skip reason now names prevent-accidental-major (kept in lockstep across policy.json, repo_audit, and the hooks test). Rejected, with reasoning recorded: pinning checkout to workflow_run.head_sha (would make the bump push non-fast-forward whenever a second merge landed — incompatible with direct-push; releasing main's protected tip is intended), explicit token repo-scoping (create-github-app-token already defaults to the current repo), and loosening BREAKING_MARKER (over-blocking is the safe direction for a guard). Co-Authored-By: Claude Opus 4.8 (1M context) --- .agent/hooks/policy.json | 2 +- .github/workflows/release.yml | 48 ++++++++++++++++++-------- tests/test_agent_hooks.py | 4 +-- tests/test_prevent_accidental_major.py | 6 ++++ tools/prevent_accidental_major.py | 6 +++- tools/repo_audit.py | 4 +-- 6 files changed, 49 insertions(+), 21 deletions(-) diff --git a/.agent/hooks/policy.json b/.agent/hooks/policy.json index b9073e8..b31eb4e 100644 --- a/.agent/hooks/policy.json +++ b/.agent/hooks/policy.json @@ -72,6 +72,6 @@ "commitizen-commit-msg", "prevent-accidental-major" ], - "blocked_pre_commit_skip_reason": "SKIP is prohibited here when it bypasses the repo's quality-gates or commitizen-commit-msg hooks.", + "blocked_pre_commit_skip_reason": "SKIP is prohibited here when it bypasses the repo's quality-gates, commitizen-commit-msg, or prevent-accidental-major hooks.", "blocked_dynamic_git_config_reason": "Dynamic git config is prohibited here for git commit and git push because it can hide hook bypasses or force pushes." } diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 9a93c24..dff6c05 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -93,8 +93,13 @@ jobs: # compute the increment from that single policy source ourselves. increment="$(git log "${prev}..HEAD" --pretty=format:'%B%x00' | uv run python tools/release_increment.py)" else - # The committed version has no tag yet (bootstrap) — release it as-is. - increment="PATCH" + # The committed version is not tagged yet (a fresh fork of this + # template): release it as-is so the adopter's first version is the + # one they chose, rather than bumping past it. + echo "No tag for committed ${prev}; releasing it as the bootstrap version." + echo "mode=bootstrap" >> "$GITHUB_OUTPUT" + echo "release=true" >> "$GITHUB_OUTPUT" + exit 0 fi case "${increment}" in BREAKING) @@ -107,6 +112,7 @@ jobs: ;; MAJOR | MINOR | PATCH) echo "increment=${increment}" >> "$GITHUB_OUTPUT" + echo "mode=bump" >> "$GITHUB_OUTPUT" echo "release=true" >> "$GITHUB_OUTPUT" ;; *) @@ -115,30 +121,42 @@ jobs: ;; esac - - name: Bump, tag, and push to main - id: bump + - name: Apply the release + id: release_step if: steps.increment.outputs.release == 'true' env: + MODE: ${{ steps.increment.outputs.mode }} INCREMENT: ${{ steps.increment.outputs.increment }} PREV: ${{ steps.increment.outputs.previous }} run: | set -euo pipefail - uv run cz bump --increment "${INCREMENT}" --changelog --files-only --yes - version=$(uv run python -c "import tomllib, pathlib; print(tomllib.loads(pathlib.Path('pyproject.toml').read_text())['project']['version'])") - if [ -z "${version}" ]; then echo "could not read the bumped version"; exit 1; fi - # `[skip ci]` stops the bump push from re-triggering CI -> Release (a loop). - git add pyproject.toml uv.lock CHANGELOG.md - git commit -m "bump: version ${PREV} -> v${version} [skip ci]" - git tag -a "v${version}" -m "v${version}" - git push origin HEAD:main - git push origin "v${version}" + if [ "${MODE}" = "bump" ]; then + uv run cz bump --increment "${INCREMENT}" --changelog --files-only --yes + # Regenerate the lock so its project version matches the bump. + uv lock + version=$(uv run python -c "import tomllib, pathlib; print(tomllib.loads(pathlib.Path('pyproject.toml').read_text())['project']['version'])") + if [ -z "${version}" ]; then echo "could not read the bumped version"; exit 1; fi + # `[skip ci]` stops the bump push re-triggering CI -> Release (a loop). + git add pyproject.toml uv.lock CHANGELOG.md + git commit -m "bump: version ${PREV} -> v${version} [skip ci]" + git tag -a "v${version}" -m "v${version}" + git push origin HEAD:main + git push origin "v${version}" + else + # bootstrap: tag the committed version as-is (no bump, no commit). + version=$(uv run python -c "import tomllib, pathlib; print(tomllib.loads(pathlib.Path('pyproject.toml').read_text())['project']['version'])") + git tag -a "v${version}" -m "v${version}" + git push origin "v${version}" + fi + # Set only on full success (set -e aborts earlier) — the publish step + # gates on this, so it never runs against a half-applied release. echo "version=${version}" >> "$GITHUB_OUTPUT" - name: Build and publish the GitHub Release - if: steps.increment.outputs.release == 'true' + if: steps.release_step.outputs.version != '' env: GH_TOKEN: ${{ steps.app-token.outputs.token }} - VERSION: ${{ steps.bump.outputs.version }} + VERSION: ${{ steps.release_step.outputs.version }} run: | set -euo pipefail tag="v${VERSION}" diff --git a/tests/test_agent_hooks.py b/tests/test_agent_hooks.py index 07ac3fb..7289439 100644 --- a/tests/test_agent_hooks.py +++ b/tests/test_agent_hooks.py @@ -12,8 +12,8 @@ "oaknational.python_repo_template.devtools check before you stop." ) SKIP_BYPASS_REASON = ( - "SKIP is prohibited here when it bypasses the repo's quality-gates or " - "commitizen-commit-msg hooks." + "SKIP is prohibited here when it bypasses the repo's quality-gates, " + "commitizen-commit-msg, or prevent-accidental-major hooks." ) HOOKS_PATH_BYPASS_REASON = ( "core.hooksPath overrides are prohibited here because they bypass repo git hooks." diff --git a/tests/test_prevent_accidental_major.py b/tests/test_prevent_accidental_major.py index cb2dfe8..b8293a8 100644 --- a/tests/test_prevent_accidental_major.py +++ b/tests/test_prevent_accidental_major.py @@ -46,3 +46,9 @@ def test_rejects_a_breaking_change_footer(tmp_path: Path) -> None: def test_errors_without_a_path_argument(tmp_path: Path) -> None: assert subject.main([]) == 1 + + +def test_errors_cleanly_on_a_missing_file(tmp_path: Path) -> None: + missing = str(tmp_path / "does-not-exist") + + assert subject.main([missing]) == 1 diff --git a/tools/prevent_accidental_major.py b/tools/prevent_accidental_major.py index caf4783..50ab0b0 100644 --- a/tools/prevent_accidental_major.py +++ b/tools/prevent_accidental_major.py @@ -38,7 +38,11 @@ def main(argv: list[str] | None = None) -> int: if not args: sys.stderr.write("prevent_accidental_major: expected a commit-message file path\n") return 1 - message = Path(args[0]).read_text(encoding="utf-8") + try: + message = Path(args[0]).read_text(encoding="utf-8") + except OSError as error: + sys.stderr.write(f"prevent_accidental_major: cannot read {args[0]!r}: {error}\n") + return 1 if is_breaking([message]): sys.stderr.write(_REJECTION) return 1 diff --git a/tools/repo_audit.py b/tools/repo_audit.py index 54dcd01..3398248 100644 --- a/tools/repo_audit.py +++ b/tools/repo_audit.py @@ -85,8 +85,8 @@ "prevent-accidental-major", ) REQUIRED_PRE_COMMIT_SKIP_REASON = ( - "SKIP is prohibited here when it bypasses the repo's quality-gates or " - "commitizen-commit-msg hooks." + "SKIP is prohibited here when it bypasses the repo's quality-gates, " + "commitizen-commit-msg, or prevent-accidental-major hooks." ) REQUIRED_DYNAMIC_GIT_CONFIG_REASON = ( "Dynamic git config is prohibited here for git commit and git push because " From efe9bee4bee70cb7db0fd0d28ca22700258a904e Mon Sep 17 00:00:00 2001 From: Jim Cresswell Date: Thu, 18 Jun 2026 14:03:58 +0100 Subject: [PATCH 3/3] fix(security): confine the commit-msg hook to the git directory SonarCloud flagged prevent_accidental_major reading a path straight from argv as path injection (pythonsecurity:S8707). The impact is negligible for a commit-msg hook (git supplies the path; the tool emits one bit), but fix-at-source beats suppression: canonicalise the path and confine it to the worktree-aware git directory before reading, so a stray argument cannot make the hook read an arbitrary file. An allowed_base seam keeps it testable without polluting .git, with tests for the traversal refusal and the git-directory resolution. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_prevent_accidental_major.py | 40 +++++++++++++++++++++----- tools/prevent_accidental_major.py | 33 ++++++++++++++++++--- 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/tests/test_prevent_accidental_major.py b/tests/test_prevent_accidental_major.py index b8293a8..9d7d81c 100644 --- a/tests/test_prevent_accidental_major.py +++ b/tests/test_prevent_accidental_major.py @@ -22,33 +22,59 @@ def test_allows_conventional_non_breaking_commits(tmp_path: Path) -> None: "docs: explain the breaking change semantics for adopters", ] for message in allowed: - assert subject.main([_commit_msg(tmp_path, message)]) == 0, message + assert subject.main([_commit_msg(tmp_path, message)], allowed_base=tmp_path) == 0, message def test_rejects_a_bang_breaking_subject( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: - code = subject.main([_commit_msg(tmp_path, "feat!: drop the legacy API")]) + code = subject.main( + [_commit_msg(tmp_path, "feat!: drop the legacy API")], allowed_base=tmp_path + ) assert code == 1 assert "MAJOR" in capsys.readouterr().err def test_rejects_a_scoped_bang_subject(tmp_path: Path) -> None: - assert subject.main([_commit_msg(tmp_path, "refactor(core)!: rename the entry point")]) == 1 + message = _commit_msg(tmp_path, "refactor(core)!: rename the entry point") + + assert subject.main([message], allowed_base=tmp_path) == 1 def test_rejects_a_breaking_change_footer(tmp_path: Path) -> None: - message = "feat: a new surface\n\nBREAKING CHANGE: removes the old one\n" + message = _commit_msg(tmp_path, "feat: a new surface\n\nBREAKING CHANGE: removes the old one\n") - assert subject.main([_commit_msg(tmp_path, message)]) == 1 + assert subject.main([message], allowed_base=tmp_path) == 1 def test_errors_without_a_path_argument(tmp_path: Path) -> None: - assert subject.main([]) == 1 + assert subject.main([], allowed_base=tmp_path) == 1 def test_errors_cleanly_on_a_missing_file(tmp_path: Path) -> None: missing = str(tmp_path / "does-not-exist") - assert subject.main([missing]) == 1 + assert subject.main([missing], allowed_base=tmp_path) == 1 + + +def test_refuses_a_path_outside_the_allowed_base( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + # A path that escapes the commit-message directory must be refused, not read. + outside = tmp_path.parent / "elsewhere.txt" + outside.write_text("feat!: sneak a breaking marker via traversal", encoding="utf-8") + + code = subject.main([str(outside)], allowed_base=tmp_path) + + assert code == 1 + assert "refusing to read outside" in capsys.readouterr().err + + +def test_git_directory_resolves_inside_the_repository() -> None: + # The default base (no allowed_base) is the worktree-aware git directory. + # A variable attribute name dodges ruff B009 + pyright reportPrivateUsage. + attribute = "_git_directory" + git_directory = getattr(subject, attribute) + + assert git_directory().is_dir() diff --git a/tools/prevent_accidental_major.py b/tools/prevent_accidental_major.py index 50ab0b0..916dc6e 100644 --- a/tools/prevent_accidental_major.py +++ b/tools/prevent_accidental_major.py @@ -16,6 +16,7 @@ from __future__ import annotations +import subprocess import sys from pathlib import Path @@ -31,16 +32,40 @@ ) -def main(argv: list[str] | None = None) -> int: - """Reject a commit message that carries a breaking-change marker.""" +def _git_directory() -> Path: + """Resolve the git directory (worktree-aware) that holds the commit message.""" + + result = subprocess.run( + ["git", "rev-parse", "--git-dir"], + capture_output=True, + text=True, + check=True, + ) + return Path(result.stdout.strip()).resolve() + + +def main(argv: list[str] | None = None, *, allowed_base: Path | None = None) -> int: + """Reject a commit message that carries a breaking-change marker. + + The message path comes from git via the commit-msg hook. It is canonicalised + and confined to the git directory (``allowed_base`` overrides this in tests) + so a stray argument cannot make the hook read an arbitrary file. + """ args = sys.argv[1:] if argv is None else argv if not args: sys.stderr.write("prevent_accidental_major: expected a commit-message file path\n") return 1 try: - message = Path(args[0]).read_text(encoding="utf-8") - except OSError as error: + base = (allowed_base or _git_directory()).resolve() + commit_message_path = Path(args[0]).resolve() + if not commit_message_path.is_relative_to(base): + sys.stderr.write( + f"prevent_accidental_major: refusing to read outside {base}: {args[0]!r}\n" + ) + return 1 + message = commit_message_path.read_text(encoding="utf-8") + except (OSError, subprocess.CalledProcessError) as error: sys.stderr.write(f"prevent_accidental_major: cannot read {args[0]!r}: {error}\n") return 1 if is_breaking([message]):