mirror of
https://github.com/GoodOlClint/PSProxmoxVE.git
synced 2026-09-03 18:55:33 +00:00
docs: record the branch protection that is actually configured (#133)
* docs: record the branch protection that is actually configured CLAUDE.md has claimed since March that main had 'required build checks, required review, admin enforced'. Two of those three were wrong: direct pushes to main were still permitted until 2026-09-02, and admin bypass is now deliberately available rather than enforced. Records what is actually set: required checks, required PR with an approving review, stale reviews dismissed on push, and review from Code Owners required. Explains the narrow CODEOWNERS, so a future session knows why a workflow or ADR change needs the operator while ordinary work does not. Admin bypass is available on purpose. GitHub does not let an author approve their own PR, so without it an operator-authored governance change would deadlock. It is not a hole here: App installations do not get admin bypass, so it does nothing for a compromised or injected bot. ADR 0025 previously said enabling code-owner review was the operator's action and not done by that decision. It has been done, so the dismissal gate is now defence-in-depth rather than the load-bearing control - GitHub enforces the property directly, including against approvals from claude.yml, the ungated path the gate cannot see. * docs: trim a clause ADR 0025 stated twice in four lines Review nit on #133. The 'claude.yml is an ungated second path the dismissal step cannot see' clause appeared in consecutive paragraphs. The first states it; the second now refers back rather than restating. --------- Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
parent
c38ca95987
commit
09a2384cd6
@@ -11,8 +11,28 @@ Tests: xUnit (`tests/PSProxmoxVE.Core.Tests/`) and Pester (`tests/PSProxmoxVE.Te
|
||||
|
||||
## Development Workflow
|
||||
|
||||
**All changes go through pull requests.** The `main` branch has branch protection enabled
|
||||
(required build checks, required review, admin enforced). Never push directly to main.
|
||||
**All changes go through pull requests.** Never push directly to `main`.
|
||||
|
||||
Protection on `main`, as configured 2026-09-02:
|
||||
|
||||
- required status checks
|
||||
- required pull request, with an approving review
|
||||
- stale reviews dismissed on push — a new commit invalidates the existing approval, and the
|
||||
automated review re-runs on `synchronize`
|
||||
- **review from Code Owners required**
|
||||
|
||||
`CODEOWNERS` is deliberately narrow. It names only the paths that govern review or publishing —
|
||||
workflows, `.claude/`, `.mcp.json`, any `CLAUDE.md`/`AGENTS.md`, `DECISIONS.md`,
|
||||
`docs/decisions/`, the test fixtures, the module manifest and `CHANGELOG.md`. A PR touching any
|
||||
of those needs the operator's approval and cannot be merged on an automated one. Everything else
|
||||
has no code owner, so the automated review still merges it.
|
||||
|
||||
Admin bypass is **available** to the operator, deliberately. GitHub does not let an author
|
||||
approve their own pull request, so without it an operator-authored change to a governance path
|
||||
would deadlock. It is not a hole in the threat model this protects against: App installations
|
||||
do not get admin bypass, so it does nothing for a compromised or prompt-injected bot.
|
||||
|
||||
See [ADR 0025](docs/decisions/0025-review-instructions-come-from-the-default-branch-and-review-governing-prs-cannot-self-approve.md).
|
||||
|
||||
```bash
|
||||
# Create a feature branch
|
||||
|
||||
+5
-3
@@ -65,12 +65,14 @@ Operator ruling 2026-09-02: narrow the ownership rather than accept the weakness
|
||||
|
||||
This inverts the design. With the setting enabled, GitHub enforces the property, and it enforces it far better than the workflow step can: it is not a one-shot check, it has no pagination limit, it does not depend on matching the right bot identity, it needs no dismissal permission, and it applies to an approval from **any** source — including `.github/workflows/claude.yml`, the ungated second path noted below, which the dismissal step cannot see.
|
||||
|
||||
The workflow gate stays, as defence-in-depth and as the only control while the setting is off. **Enabling the setting is the operator's action and is not done by this decision.** Until it is, the dismissal step is still load-bearing and should be read that way.
|
||||
**Enabled 2026-09-02**, together with required-pull-request, required-approving-review and dismiss-stale-reviews-on-push. Enabling it also closed a gap nobody had noticed: direct pushes to `main` were still permitted until that moment, despite `CLAUDE.md` having claimed otherwise since March.
|
||||
|
||||
So the workflow gate is now **defence-in-depth rather than the load-bearing control**, and the `claude.yml` limit described below is closed in practice by the same move. The gate still earns its place: it applies if the setting is ever turned off, and it catches an automated approval early with a clear message rather than leaving one sitting on the pull request.
|
||||
|
||||
Admin bypass is deliberately left **available**. GitHub does not permit an author to approve their own pull request, so without bypass an operator-authored change to a governance path would deadlock with no eligible reviewer. It does not weaken the threat model: App installations do not receive admin bypass, so it is unavailable to a compromised or prompt-injected bot, which is the adversary this decision is about.
|
||||
|
||||
The two lists must be kept in sync; both files say so. Drift is the failure mode, and it is silent in the direction that matters — a path in the detector but not in `CODEOWNERS` is protected only by the weaker mechanism.
|
||||
|
||||
One edge case is worth knowing before enabling the setting: **GitHub does not let an author approve their own pull request.** An operator-authored PR touching a governance path would have no eligible code-owner reviewer, and with admin enforcement on there is no bypass. In practice every recent PR is authored by `goodolclint-claude[bot]`, so the operator is free to approve; but an operator-authored governance change would need admin enforcement toggled, or to go through the bot.
|
||||
|
||||
Three limits remain, and are not closed by this decision:
|
||||
|
||||
- **`.github/workflows/claude.yml` is a second, ungated path to a binding approval.** It runs the same action on any `@claude` mention with `contents: write` and no `allowedTools` restriction, no materialized prompt, and no governance detection — so its standing instructions are the repository `CLAUDE.md` *from the PR's checkout*, the exact file this workflow materializes from the default branch to avoid. An approval produced there is never dismissed, because the gate here is a one-shot check inside this job and `pull_request` does not fire on review submission. Filed separately; not fixed here.
|
||||
|
||||
Reference in New Issue
Block a user