Skip to content

ci(gates): fire required checks on pull_request edited so retargets re-evaluate (backend#2701) - #886

Merged
aptracebloc merged 2 commits into
developfrom
fix/2701-retarget-required-checks
Aug 27, 2026
Merged

ci(gates): fire required checks on pull_request edited so retargets re-evaluate (backend#2701)#886
aptracebloc merged 2 commits into
developfrom
fix/2701-retarget-required-checks

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Closes tracebloc/backend#2701

What

A required status check filtered by branches: with notypes: uses GitHub's default pull_request activity types [opened, synchronize, reopened]. A base-branch change (a retarget) fires only edited, which the defaults omit — so a PR opened against a base a workflow never ran on and then retargeted onto a gated base never produces the check and sits at "Expected — waiting for status to be reported" forever. It's the same permanent-pending state the paths trap causes (which these very workflows are built to avoid), reached through the trigger instead of the paths filter.

This is the Bugbot Medium from the client#876 staging hop (client/pull/876#discussion_r3871750197), verified still-present on current develop.

Fix — edited added to the class, not just the instance

edited added (with the default types it replaces spelled out, since declaring types:replaces the default set) to every required / hard-fail, deliberately-not-path-filtered gate whose pull_request is branches:-filtered and was missing it:

WorkflowContextNote
helm-unit.yamlHelm unit teststhe Bugbot finding
drift-checks.yamlSource-of-truth driftrequired develop + main
standard-checks.ymlLint, Unit testsorg-standard required
version-bump-gate-caller.ymlversion-bump-gatehard-fail develop gate — added in review (thanks @saqlainsyed007)

Each file's own header already argues "a required/hard-fail check must report on every PR to its gated base or it sits at Expected-waiting forever" — the retarget gap is that same failure, so they're fixed together. fr-gate-caller.yml already carried edited with a load-bearing base-change comment (backend#1945/.github#237); this PR brings the rest of the class in line.

Guard — the rule is machine-enforced for helm-unit

scripts/tests/helm-unittest-gated.sh (run by the required Source-of-truth drift job) gains a third failure mode alongside the paths-trap and rename checks: it fails closed when helm-unit.yaml's pull_request types omit editedor drop a default. Verified: passes on the fix; fails on (a) no types:, (b) edited missing, (c) defaults dropped.

Class-completeness sweep

A repo-wide sweep for the exact trap signature — pull_request with a branches: filter, nopaths:/paths-ignore:, and edited absent from the effective types — now returns onlye2e-auth-proxy.yaml (deliberately deferred, below). Everything else is either fixed, path-filtered (∴ not a required check), or has no branches: filter (∴ not stranded by a retarget — opened fires regardless of base).

Scope notes

  • e2e-auth-proxy.yaml (E2E auth-proxy (squid)) is the same structural pattern but is a candidate required check (the staging+main required-flip is a pending manual step, client#867/backend#2350) and is a ~90s k3d job where blanket edited also fires on every title/body edit. Its retarget-safety fix should land with its required-flip, and likely wants an if: github.event.changes.base job gate rather than a blanket edited given the cost. Deliberately not touched here.
  • No-branches: gates (chart-version-guard.yml, bugbot-gate-caller.yml, code-quality-caller.yml, set-pr-status.yml) run on opened regardless of base, so a retarget never strands them. Where one compares against the base (set-pr-status's closing-ref), a retarget can leave a stale verdict — a different, non-blocking problem from the permanent-pending one this PR targets, and out of scope.
  • Guard coverage: only helm-unit.yaml is machine-guarded (the guard hardcodes that path by design — it can't read live branch-protection, so it can't enumerate which other workflows are required). The other gates are fixed but rely on the in-file convention comment; a shared guard would need a machine-readable required-workflow list this repo doesn't have.
  • Path-filtered workflows (helm-ci.yaml, installer-tests.yaml, envelope-contract-drift.yml) are intentionally not required, so the retarget trap doesn't strand them — out of scope.

Verification

  • helm-unittest-gated.sh: PASS in-tree; fails closed on all three regression shapes.
  • All edited workflows parse as valid YAML; the trap sweep passes (only the deferred e2e-auth-proxy.yaml remains).
  • Ran /code-review (xhigh) on the diff; findings fixed or documented above.

🤖 Generated with Claude Code

… re-evaluate (backend#2701)
A required status check filtered by `branches:` with no `types:` uses GitHub's
defaults [opened, synchronize, reopened]. A base-branch change (a RETARGET)
fires only `edited`, which the defaults omit -- so a PR opened against a base a
workflow never ran on and then retargeted onto a gated base never produces the
check and sits at "Expected - waiting for status to be reported" forever, the
same permanent-pending state the paths trap causes, reached via the trigger.
Add `edited` (and spell out the three defaults it replaces) to the three
documented-required, non-path-filtered workflows that share this trap:
- helm-unit.yaml (`Helm unit tests`) -- the Bugbot finding
- drift-checks.yaml (`Source-of-truth drift`)
- standard-checks.yml (`Lint`, `Unit tests`)
Encode the rule in helm-unit's own guard (helm-unittest-gated.sh, run by the
required `Source-of-truth drift` job): it now fails closed when helm-unit's
`pull_request` types omit `edited` or drop a default.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@aptraceblocaptracebloc self-assigned this Aug 27, 2026
@LukasWodka
LukasWodka requested review from saqlainsyed007 and removed request for saadqbalAugust 27, 2026 15:03

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at a0d7d33d. The change is correct and I'd own it — one class-completeness question before I approve, since the PR's own thesis is "fix the class."

What it does: adds types: [opened, synchronize, reopened, edited] to the pull_request trigger of three required, non-path-filtered workflows (helm-unit.yaml, drift-checks.yaml, standard-checks.yml), and extends helm-unittest-gated.sh to fail closed if helm-unit.yaml's types omit edited or drop a default. The gap: a base-branch retarget fires onlyedited, which the default set omits — so a PR opened against a non-gated base and retargeted onto a gated one sits at "Expected — waiting for status" forever.

I verified the two things that could go wrong and both are clean:

  • No trigger was narrowed. All three files had notypes: on develop, so they ran on the default [opened, synchronize, reopened]; the new lists are exact supersets — push/open still fire, edited is added.
  • The guard is mutation-proof.missing = [t for t in REQUIRED_PR_TYPES if t not in types] is derived and order-independent; I ran it against a reconstructed tree and it fails closed on all three regression shapes (no types:, edited missing, a default dropped). No expensive/side-effecting workflow gained edited.

The one thing to settle (class-completeness): the scope notes carefully justify excluding e2e-auth-proxy.yaml, the path-filtered workflows, and the guard's single-file coverage — but are silent on version-bump-gate-caller.yml, which matches the exact trap: branches: [develop], no paths:, and types: [opened, synchronize, reopened, ready_for_review, labeled, unlabeled]omits edited. Its sibling fr-gate-caller.yml already carries edited with a load-bearing comment citing this same base-change rationale. If version-bump-gate is a required check on develop, a retarget onto develop would leave it permanently pending exactly as this PR fixes elsewhere. Is it a member you meant to include, or an intentional exclusion? Happy to approve as soon as that's covered or explicitly ruled out.

(One clarification so nobody over-reads the scope: this PR doesn't touch set-pr-status.yml, so the separate "editing the body to add Closes doesn't re-fire set-status / closing-ref" gap is not addressed here — that gate has no branches: filter, so it's a stale-status problem, not the permanent-pending one this PR targets.)

Holding at Comment on the version-bump-gate-caller.yml question; CI is green and everything else is ready.

… (backend#2701)
version-bump-gate-caller.yml is a hard-fail (soft-fail: false) develop gate with
`branches: [develop]` and no `paths:`, but its types omitted `edited` -- the same
base-change trap this PR fixes on the other required, non-path-filtered gates. A
PR opened against a non-develop base and retargeted onto develop would never
re-run the gate (a retarget fires only `edited`, and `synchronize` needs a push).
Its sibling fr-gate-caller.yml already carries `edited` for the identical reason
(backend#1945/.github#237).
Raised in review by @saqlainsyed007 as a missed class member. A repo-wide sweep
now confirms every `branches:`-filtered, non-path-filtered gate carries `edited`
except e2e-auth-proxy.yaml, which stays deferred to its required-flip (documented
in the PR body).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@aptracebloc

Copy link
Copy Markdown
ContributorAuthor

Good catch, @saqlainsyed007 — that's a real class member, not an intentional exclusion. Fixed in da349c4.

version-bump-gate-caller.yml is a hard-fail develop gate (soft-fail: false, branches: [develop], no paths:), so a PR opened against a non-develop base and retargeted onto develop would leave it permanently pending exactly as you describe — the same trap fr-gate-caller.yml's edited already guards against. I added edited with a comment pointing at that convention (backend#1945/.github#237).

To make sure I wasn't missing a third one, I ran a repo-wide sweep for the exact trap signature (pull_request with a branches: filter, no paths:/paths-ignore:, edited absent from effective types). After this commit it returns onlye2e-auth-proxy.yaml, which stays deferred to its required-flip for the reasons in the scope notes (candidate check + 90s k3d job that wants an if: github.event.changes.base gate rather than blanket edited). Everything else is fixed, path-filtered (∴ not required), or has no branches: filter (∴ opened fires regardless of base, so a retarget can't strand it).

Agreed on set-pr-status.yml — no branches: filter, so it's a stale-status concern, not the permanent-pending one this PR targets. Out of scope here.

Re-requesting your review.

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at da349c45. The one question I raised is resolved with evidence and CI is green.

You added edited to version-bump-gate-caller.yml — its types: is now [opened, synchronize, reopened, ready_for_review, labeled, unlabeled, edited] — with an in-file comment documenting the retarget rationale and cross-referencing fr-gate-caller.yml. That closes the missed class member. I re-swept the class: among branches:-filtered, non-path-filtered pull_request gates (drift-checks, helm-unit, standard-checks, fr-gate-caller, version-bump-gate-caller) all now carry edited; the only holdout, e2e-auth-proxy.yaml, is your explicitly documented deferral (pending its required-flip, wants an if: github.event.changes.base gate), and the no-branches: gates are correctly excluded since opened fires regardless of base.

The delta is a pure superset — only edited appended, nothing narrowed — and introduces no new defect. LGTM.

@aptracebloc
aptracebloc merged commit e74207c into developAug 27, 2026
47 of 49 checks passed
@aptracebloc
aptracebloc deleted the fix/2701-retarget-required-checks branch August 27, 2026 15:50
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@aptracebloc@saqlainsyed007@LukasWodka