Reference in New Issue
Block a user
Delete Branch "fix/2770-governance-pr-status-shadows"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #2770
Fixes #2767
Teaches status-reaper to compensate non-required
(pull_request)/(pull_request_review)governance shadows when the trusted(pull_request_target)variant is green. CI workflows with apush:trigger are excluded. Includes regression tests.REQUEST_CHANGES on head
47e63cac.Blocking masking-risk finding: the governance shadow predicate is broader than the contract. is_non_push_governance_context() returns true for any workflow known to have no push trigger and a (pull_request)/(pull_request_review) suffix. It does not restrict compensation to the named governance shadows (sop-checklist, qa-review, security-review, retired sop-tier-check).
I checked the workflow inventory on this head: there are many non-push PR-triggered workflows besides those governance gates, including gate-check-v3, reserved-path-review, lint-required-no-paths, lint-required-context-exists-in-bp, audit-force-merge, status-reaper, umbrella-reaper, and others. If any of those emits a failed (pull_request) or (pull_request_review) context and a matching (pull_request_target) context is green, this patch would auto-post success to the failed context. That violates the requested invariant: compensate ONLY the named non-required governance shadows, NEVER real required/merge-control lanes.
The new tests cover the happy governance case, target-missing/failed preservation, and a push-triggered CI preservation case. They do not cover a non-push non-governance workflow such as gate-check-v3 or reserved-path-review, which is the dangerous class.
Please add an explicit context/workflow allowlist for the intended shadow contexts/jobs and a regression test proving a non-push non-governance failure with a green pull_request_target equivalent is preserved, not compensated. Until then this can mask real failures.
REQUEST_CHANGES on head
47e63cac.The compensation predicate is broader than the safety contract.
is_non_push_governance_context()returns true for any failed(pull_request)or(pull_request_review)context whose workflow is known and has nopush:trigger (.gitea/scripts/status-reaper.py:510-529). The requested contract is narrower: compensate only the named non-required governance shadow contexts (sop-checklist / qa-review / security-review / retired sop-tier-check) when the trusted(pull_request_target)equivalent is green.As written, a future or unrelated no-push workflow that emits both a PR shadow and a target context would be auto-greened if the target context is green, even if its PR/review failure is a real signal. The new tests cover target-success, target-failed, and push-triggered CI preservation, but they do not pin an explicit governance allowlist or prove that an arbitrary no-push workflow is preserved.
Fix shape: make this an exact allowlist of compensatable workflow/job contexts (including any retired context names that must be supported), and add a regression test like
custom-audit / check (pull_request_review)withworkflow_trigger_map={"custom-audit": False}plus a green target equivalent, asserting no compensation. That keeps this from becoming a general no-push failure masker.REQUEST_CHANGES on head
2bd911a1.The production predicate is much safer now: GOVERNANCE_SHADOW_ALLOWLIST is exactly {sop-checklist, qa-review, security-review, sop-tier-check}, and is_governance_shadow_context() preserves unknown/non-allowlisted workflows and still excludes push-triggered workflows. The target-success requirement is also intact.
Blocking remaining issue is test coverage for the masking-risk class from my prior RC. The re-review request specifically called out proving preservation for the non-governance no-push workflows that can be dangerous to auto-green: gate-check-v3, reserved-path-review, lint-required-no-paths, audit-force-merge, status-reaper, and umbrella-reaper. The new regression test only uses a synthetic custom-audit workflow. That proves the generic allowlist behavior once, but it does not pin the actual workflow names that motivated the RC, and it would not catch a later edit that accidentally adds one of those real merge-control/lint workflows to the allowlist.
Please add a table-driven regression covering those named workflow contexts with a green pull_request_target equivalent and assert no compensating status is posted. Include representative pull_request and pull_request_review suffixes where relevant. After that, this should be approvable if CI remains green.
Status note: CI / all-required is green on 2bd911a1; gate-check-v3 is currently red separately.
REQUEST_CHANGES on head
2bd911a1.The over-broad masking predicate is fixed for active workflows: the allowlist is now only sop-checklist / qa-review / security-review / sop-tier-check, arbitrary non-governance PR/review shadows are preserved, target-green is required, and push-trigger workflows are excluded.
One blocker remains for the explicitly-listed retired
sop-tier-checkcase. In production,is_governance_shadow_context()still requiresworkflow_trigger_map.get(workflow_name) is False. The current tree intentionally has no.gitea/workflows/sop-tier-check.yml(test_no_tier_regression.shasserts it stays removed), so the real scan will not populatesop-tier-check: False. A failedsop-tier-check / ... (pull_request)with a green(pull_request_target)equivalent therefore returns false and falls through topreserved_non_push_suffix, meaning the retired context this PR claims to compensate is not actually compensated. The new retired test injects{"sop-tier-check": False}, so it does not exercise the production/deleted-workflow path.Fix shape: either treat explicitly-retired allowlisted names as known non-push even when absent from the workflow trigger map, or remove retired sop-tier-check from the promised scope. Add a regression where
workflow_trigger_maplackssop-tier-checkbut the failed retired shadow plus green target is present.APPROVED on head
11fcbb05.Verified the #11572 blocker is closed:
sop-tier-checkis still explicitly allowlisted, and the predicate now treats it as compensatable when it is absent fromworkflow_trigger_map, matching the production retired-workflow state. The new regression covers that absent-map case directly, not the synthetic{"sop-tier-check": false}setup.The scope is still narrow: only names in
GOVERNANCE_SHADOW_ALLOWLISTcan enter the governance-shadow path, push-triggered allowlisted workflows are preserved, and non-allowlisted names remain false even when absent/no-push. I spot-checked the named masking-risk workflows (gate-check-v3,reserved-path-review,lint-required-no-paths,lint-required-context-exists-in-bp,audit-force-merge,status-reaper,umbrella-reaper) and the parametrized test now pins them as preserved.CI / all-requiredis green on11fcbb05. Remaining red statuses are the expected governance review/SOP gates.REQUEST_CHANGES on head
11fcbb05.The #11571 test-coverage blocker is resolved: the new parametrized regression test explicitly covers the named non-governance no-push workflows (
gate-check-v3,reserved-path-review,lint-required-no-paths,lint-required-context-exists-in-bp,audit-force-merge,status-reaper,umbrella-reaper) and asserts no compensating post is made even when a pull_request_target equivalent is green. That part is not vacuous. The base allowlist also remains the intended exact set:sop-checklist,qa-review,security-review,sop-tier-check.New blocker: the retired-workflow missing-trigger-map handling is not scoped to retired
sop-tier-check. Inis_governance_shadow_context, once a workflow is inGOVERNANCE_SHADOW_ALLOWLIST,has_push = workflow_trigger_map.get(workflow_name)andreturn has_push is not Truemeans any allowlisted workflow missing from the trigger map is treated as compensatable. That includes activesop-checklist,qa-review, andsecurity-review, not only removedsop-tier-check. If trigger-map discovery/parser input ever misses an active governance workflow, the reaper now fails open and can auto-green its PR/review shadow when a target context is green.Please split active vs retired handling: active allowlisted workflows should require an explicit trigger-map entry of
False; only an explicit retired allowlist/set containingsop-tier-checkshould be compensatable when absent from the map. Add a regression that e.g. missingqa-reviewis preserved while missingsop-tier-checkis compensated.New commits pushed, approval review dismissed automatically according to repository settings
APPROVED on head
840fad88.Re-reviewed the #11579 fail-open fix. The production predicate now separates active and retired allowlists: active governance shadows are exactly
sop-checklist,qa-review, andsecurity-review, and they compensate only when the trigger map explicitly saysFalse. If an active allowlisted workflow is absent from the trigger map, the code now preserves it. The retired allowlist is a separate exact set containing onlysop-tier-check, and absent-from-map compensation applies only through that retired path.The new regression coverage is load-bearing: missing
qa-reviewwith a green pull_request_target equivalent posts nothing and keeps compensated counts at zero, while missing retiredsop-tier-checkstill compensates. The prior parametrized preservation test forgate-check-v3,reserved-path-review,lint-required-no-paths,lint-required-context-exists-in-bp,audit-force-merge,status-reaper, andumbrella-reaperremains in place, so arbitrary non-governance no-push workflows are still preserved.CI checked on
840fad88: Platform Go, Shellcheck, andCI / all-requiredare green. I could not rerun pytest locally because this container lacks pytest, but the current-head CI covers the script/test suite.