rbrooks/Iris-WLED
Watch
1
0
Fork
You've already forked Iris-WLED
 0
Code Issues 21 Pull requests Projects Releases 2 Packages 1 Wiki Activity Actions

Enable branch protection on main: require green CI before merge #56

New issue
Closed
opened 2026-07-15 15:38:29 +00:00 by claude-bot · 1 comment
claude-bot commented 2026-07-15 15:38:29 +00:00
Contributor
Copy link

From the 2026-07-15 review follow-ups. The Forgejo Actions migration PR merged with a red pipeline and nothing prevented it — CI is currently advisory. Once #41 makes the pipeline green, add a branch protection rule on main:

  • Require status checks (the CI workflow's jobs) to pass before merge
  • Block direct pushes to main (PRs only) — including for the bot account
  • Decide whether Renovate PRs may auto-merge minor/patch updates once checks pass (renovate.json automerge — pairs nicely with protection)

Depends on: #41 (protection with a permanently-red pipeline would block all merges).

From the 2026-07-15 review follow-ups. The Forgejo Actions migration PR merged with a red pipeline and nothing prevented it — CI is currently advisory. Once #41 makes the pipeline green, add a branch protection rule on `main`: - [ ] Require status checks (the CI workflow's jobs) to pass before merge - [ ] Block direct pushes to `main` (PRs only) — including for the bot account - [ ] Decide whether Renovate PRs may auto-merge minor/patch updates once checks pass (renovate.json `automerge` — pairs nicely with protection) Depends on: #41 (protection with a permanently-red pipeline would block all merges).
claude-bot added this to the v1.0.0 milestone 2026-07-15 15:38:29 +00:00
claude-bot commented 2026-08-31 04:24:13 +00:00
Author
Contributor
Copy link

Done — all three items

Require status checks before merge

enable_status_check   = true
status_check_contexts = ["CI / *"]
required_approvals    = 0

The contexts were not guessed. A wrong context string would have blocked every merge permanently, so I read what the sibling repos on this instance already use — Quest-Board has exactly ["CI / *"]. This workflow is also named CI, so the glob covers all five jobs.

It deliberately does not match the new Dependency audit workflow (#81), so advisories report without gating merges — the coupling that made this issue un-actionable until today.

required_approvals: 0 is intentional: on a single-maintainer repo, requiring one would deadlock on self-approval.

Block direct pushes to main

enable_push: false. Verified by behaviour, not by reading the config back — pushed a probe commit at main and got:

remote: Forgejo: Not allowed to push to protected branch main
 ! [remote rejected] main -> main (pre-receive hook declined)

The probe was then rolled back locally; nothing reached the remote.

Renovate automerge

Decided yes for minor/patch, extended from github-actions to the batched npm (non-major) group — landed in #82 (bd83fee), which was itself the first change to go through the new protection, validating the whole path end to end.

A security review then flagged automerge without a release-age quarantine, which was correct and exposed a flaw in my justification. I had argued automerge was safe because Renovate waits for green CI — but green CI is the wrong threat model. A hijacked package's postinstall does not fail ruff, mypy, vitest or Playwright; npm ci runs on the shared runner, release.yml holds registry push credentials, and the image reaches the dev server.

So minimumReleaseAge: "3 days" now guards both automerge rules — npm and github-actions. Actions execute directly in the runner, so guarding only npm would have half-closed it. Majors are unaffected: still dependencyDashboardApproval and hand-merged.

Worth recording for whoever reads this next: this issue depended on #41, but what actually unblocked it was #81 splitting the dependency audit out. continue-on-error: true does not suppress run failure on this runner, so the "advisory" audit was a hard gate in practice — enabling protection before that would have blocked every merge, and re-blocked on every newly published transitive CVE.

## Done — all three items ### Require status checks before merge ``` enable_status_check = true status_check_contexts = ["CI / *"] required_approvals = 0 ``` The contexts were **not** guessed. A wrong context string would have blocked every merge permanently, so I read what the sibling repos on this instance already use — `Quest-Board` has exactly `["CI / *"]`. This workflow is also named `CI`, so the glob covers all five jobs. It deliberately does **not** match the new `Dependency audit` workflow (#81), so advisories report without gating merges — the coupling that made this issue un-actionable until today. `required_approvals: 0` is intentional: on a single-maintainer repo, requiring one would deadlock on self-approval. ### Block direct pushes to `main` `enable_push: false`. **Verified by behaviour, not by reading the config back** — pushed a probe commit at `main` and got: ``` remote: Forgejo: Not allowed to push to protected branch main ! [remote rejected] main -> main (pre-receive hook declined) ``` The probe was then rolled back locally; nothing reached the remote. ### Renovate automerge Decided **yes** for minor/patch, extended from `github-actions` to the batched `npm (non-major)` group — landed in #82 (`bd83fee`), which was itself the first change to go through the new protection, validating the whole path end to end. A security review then flagged automerge without a release-age quarantine, which was correct and exposed a flaw in my justification. I had argued automerge was safe because Renovate waits for green CI — but green CI is the wrong threat model. A hijacked package's `postinstall` does not fail `ruff`, `mypy`, `vitest` or Playwright; `npm ci` runs on the shared runner, `release.yml` holds registry push credentials, and the image reaches the dev server. So `minimumReleaseAge: "3 days"` now guards **both** automerge rules — npm and github-actions. Actions execute directly in the runner, so guarding only npm would have half-closed it. Majors are unaffected: still `dependencyDashboardApproval` and hand-merged. --- Worth recording for whoever reads this next: this issue depended on #41, but what actually unblocked it was #81 splitting the dependency audit out. `continue-on-error: true` does not suppress run failure on this runner, so the "advisory" audit was a hard gate in practice — enabling protection before that would have blocked every merge, and re-blocked on every newly published transitive CVE.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
renovate/lock-file-maintenance
v1.1.0
v1.0.0
Labels
Clear labels   
area/ai
AI suggestion pipeline

  
area/backend
FastAPI / Python backend

  
area/frontend
React / Vite frontend

  
area/infra
Docker, deployment, and infrastructure

  
area/scheduler
Scheduler and time envelope

  
area/wled
WLED controller integration

  
good-first-issue
Good for newcomers

  
priority/high
Blocks release or high impact

  
priority/low
Nice to have

  
priority/medium
Should be done soon

  
type/bug
Something isn't working

  
type/chore
Maintenance, release, or housekeeping

  
type/ci-cd
CI/CD pipeline, build, or release automation

  
type/docs
Documentation

  
type/feature
New capability or enhancement

  
type/qa
Testing, validation, or quality assurance

  
v1.0.0
Target release: v1.0.0

  
v1.1.0
Target release: v1.1.0

  
v1.2.0
Target release: v1.2.0

  
v2.0.0
Target release: v2.0.0

No labels
area/ai
area/backend
area/frontend
area/infra
area/scheduler
area/wled
good-first-issue
priority/high
priority/low
priority/medium
type/bug
type/chore
type/ci-cd
type/docs
type/feature
type/qa
v1.0.0
v1.1.0
v1.2.0
v2.0.0
Milestone
Clear milestone
No items
No milestone
v1.0.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rbrooks/Iris-WLED#56
No description provided.