Skip to content

fix(governance): close the baseline/alert split and stop the SPDX check fighting the lockfile - #597

Merged
hyperpolymath merged 1 commit into
mainfrom
fix/structural-gate-corrections
Aug 7, 2026
Merged

fix(governance): close the baseline/alert split and stop the SPDX check fighting the lockfile#597
hyperpolymath merged 1 commit into
mainfrom
fix/structural-gate-corrections

Conversation

@hyperpolymath

Copy link
Copy Markdown
Owner

Three structural corrections — each removes a class of estate failure rather than another instance.

1. The baseline never reached the security tab

scan ──┬─> findings.json ─> apply-baseline.sh ─> the GATE (filtered)
└─> hypatia.sarif ────────────────────> code scanning (NOT filtered)

The code_scanning ruleset rule blocks on those alerts and knows nothing of the baseline, so acknowledging a finding silenced the gate while the same finding still blocked the merge — every required check green, nothing to point at.

Measured 2026-08-07: 147 open PRs across 91 repos, held by 1,777 alerts, zero introduced by the PR they blocked.

scripts/filter-sarif-by-baseline.sh filters the SARIF before upload. It contains no matcher of its own — it runs the caller's apply-baseline.sh (the same copy the gate runs) and uses its findings_suppressed list, so there is exactly one matcher by construction. It fails open in every error path; 4 of its 9 tests assert that.

2. The SPDX check fought gh actions-lock

The check was head -1 | grep, but gh actions-lock inserts its managed-by comment at line 1 on every mint — so the check re-failed on files whose licence sat on line 2. It reported 40 workflows estate-wide as missing a header they had, and prepending a default mis-licensed 3 files (PMPL shadowed by MPL) before it was caught.

Now reads the leading comment block, where REUSE puts the identifier. Validated against 984 real workflow files: 983 accepted, the single rejection genuinely has no declaration.

3. The pin check named the wrong thing

ERROR: Found unpinned actions: comes only from the no-lockfile branch but reads generically — indistinguishable from the stale-lockfile branch or an SPDX failure in the same job. It now says which tree lacked a lockfile and states the mutual exclusion, because inline-pinning to "fix" it removes actions from the lockfile (hypatia: 14 startup_failures from one commit).

Tests: 9 + 9, both in scripts/tests/ so self-test.yml discovers them as of #588.

🤖 Generated with Claude Code

…ck fighting the lockfile
Three structural corrections. Each removes a whole class of estate failure
rather than another instance of one.
## 1. The baseline never reached the security tab
A Hypatia finding travelled two paths that never met:
scan ──┬─> findings.json ─> apply-baseline.sh ─> the GATE (filtered)
└─> hypatia.sarif ────────────────────> code scanning (NOT filtered)
The `code_scanning` ruleset rule blocks on those alerts and has no knowledge of
the baseline. So acknowledging a finding silenced the gate while the identical
finding still blocked the merge — with every required check green and nothing
to point at.
Measured 2026-08-07: **147 open pull requests across 91 repositories** were held
this way by **1,777 alerts, ZERO of which were introduced by the pull request
they blocked**.
`scripts/filter-sarif-by-baseline.sh` now filters the SARIF before upload.
⚠ It contains NO matcher of its own. Baseline matching keys on severity as well
as rule_module/type, and its glob-to-regex translation has two jq binding traps
documented in apply-baseline.sh; a second implementation would drift and
reintroduce the split. The filter RUNS the caller's own `apply-baseline.sh` —
the same copy the gate runs — and uses its `findings_suppressed` list. One
matcher, by construction.
⚠ It FAILS OPEN in every error path. An unfiltered upload is a safe failure; a
silently over-filtered one hides real alerts. Four of the nine tests assert
exactly that.
Path relativisation is hoisted into its own step so the filter and the gate
share one definition of a finding's path.
## 2. The SPDX check fought the estate's own tool
The check was `head -1 | grep`. `gh actions-lock` INSERTS
`# This workflow is managed by gh actions-lock.` at line 1 whenever it mints a
lockfile — so the check re-failed every time a lockfile was refreshed, on files
that had a perfectly good licence declaration on line 2.
It reported 27 hypatia workflows and 13 more elsewhere as missing a header they
all had. Worse, "fixing" that by prepending a default MIS-LICENSED three files,
shadowing PMPL-1.0-or-later with MPL-2.0, before it was caught.
The check now reads the leading comment block, which is where REUSE puts the
identifier. Validated against 984 real workflow files across 58 repositories:
983 accepted, and the one rejection genuinely has no declaration.
## 3. The pin check's failure text named the wrong thing
`ERROR: Found unpinned actions:` is emitted only by the NO-LOCKFILE branch, but
reads as a generic pinning complaint — indistinguishable from the stale-lockfile
branch and from an unrelated SPDX failure in the same job. It now says which
tree lacked a lockfile and states the mutual exclusion explicitly, because
"fixing" it by inline-pinning REMOVES actions from the lockfile (measured on
hypatia: 14 startup_failures from one commit).
Tests: 9 for the SARIF filter, 9 for the SPDX check. Both suites live in
scripts/tests/ and so are discovered by self-test.yml as of #588.
@sonarqubecloud

Copy link
Copy Markdown

@hyperpolymath
hyperpolymath merged commit 13cb62a into mainAug 7, 2026
35 of 37 checks passed
@hyperpolymath
hyperpolymath deleted the fix/structural-gate-corrections branch August 7, 2026 13:27
Comment on lines +1063 to +1064
if ! awk '/^---[[:space:]]*$/ { next } /^#/ { print; next } { exit }' "$file" \
| grep -q "^# SPDX-License-Identifier:"; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Edge Case: SPDX header scan stops at a blank line in the block

The awk matcher /^#/ { print; next } { exit } exits on the first non-comment line, so a blank line anywhere in the leading comment block (e.g. # managed by gh actions-lock., blank line, then # SPDX-License-Identifier:) causes the identifier below it to be missed and the file to be reported as missing a header it has — the same false-positive class this PR set out to remove. Consider skipping blank lines within the leading block (e.g. add a /^[[:space:]]*$/ { next } rule) if such layouts occur in the estate.

Tolerate blank lines interspersed in the leading comment block.:

awk '/^---[[:space:]]*$/ { next } /^[[:space:]]*$/ { next } /^#/ { print; next } { exit }' "$file" \
| grep -q "^# SPDX-License-Identifier:"
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-botBot commented Aug 7, 2026

Copy link
Copy Markdown

Note

Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime.
Learn more

Code Review👍 Approved with suggestions0 resolved / 1 findings

Aligns the SARIF upload with the baseline to resolve merge blockers, fixes SPDX header detection for comment blocks, and improves pin-check error messages. Consider updating the awk matcher in the SPDX scan to handle internal blank lines without exiting prematurely.

Auto-approved and auto-merge armed: No blocking issues found.
Please see Auto-approve Docs for details on setting custom approval criteria. — merges when pipeline and required approvals pass.

💡 Edge Case: SPDX header scan stops at a blank line in the block

📄 .github/workflows/governance-reusable.yml:1063-1064📄 scripts/tests/spdx-header-block-test.sh:19-22

The awk matcher /^#/ { print; next } { exit } exits on the first non-comment line, so a blank line anywhere in the leading comment block (e.g. # managed by gh actions-lock., blank line, then # SPDX-License-Identifier:) causes the identifier below it to be missed and the file to be reported as missing a header it has — the same false-positive class this PR set out to remove. Consider skipping blank lines within the leading block (e.g. add a /^[[:space:]]*$/ { next } rule) if such layouts occur in the estate.

Tolerate blank lines interspersed in the leading comment block.
awk '/^---[[:space:]]*$/ { next } /^[[:space:]]*$/ { next } /^#/ { print; next } { exit }' "$file" \
| grep -q "^# SPDX-License-Identifier:"
🤖 Prompt for agents
Code Review: Aligns the SARIF upload with the baseline to resolve merge blockers, fixes SPDX header detection for comment blocks, and improves pin-check error messages. Consider updating the awk matcher in the SPDX scan to handle internal blank lines without exiting prematurely.
1. 💡 Edge Case: SPDX header scan stops at a blank line in the block
Files: .github/workflows/governance-reusable.yml:1063-1064, scripts/tests/spdx-header-block-test.sh:19-22
The awk matcher `/^#/ { print; next } { exit }` exits on the first non-comment line, so a blank line anywhere in the leading comment block (e.g. `# managed by gh actions-lock.`, blank line, then `# SPDX-License-Identifier:`) causes the identifier below it to be missed and the file to be reported as missing a header it has — the same false-positive class this PR set out to remove. Consider skipping blank lines within the leading block (e.g. add a `/^[[:space:]]*$/ { next }` rule) if such layouts occur in the estate.
Fix (Tolerate blank lines interspersed in the leading comment block.):
awk '/^---[[:space:]]*$/ { next } /^[[:space:]]*$/ { next } /^#/ { print; next } { exit }' "$file" \
| grep -q "^# SPDX-License-Identifier:"
Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose 

Important

Your trial ends in 3 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

@gitar-botgitar-botBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️This PR is blocked due to unresolved code review findings.

Configure merge blocking · Maintainers can dismiss this review.

hyperpolymath added a commit that referenced this pull request Aug 7, 2026
main moved under this branch. #597 ("close the baseline/alert split and
stop the SPDX check fighting the lockfile") rewrote both files this branch
touches, so the conflict is real rather than textual.
Both conflicts were additive and both sides are kept:
.gitleaks.toml -- #597 added a substantial measured finding to the
baseline: gitleaks 8.18.4 matches allowlist `regexes` against the whole
MATCH, not the extracted secret, and `regexTarget` does not change it, so
an anchored `^value$` entry is silently INERT for generic-api-key (the
estate's largest false-positive source). That documentation is preserved
verbatim -- it now lives in config/gitleaks/estate-baseline.toml, where it
reaches every repo that extends the baseline instead of only this one.
The root .gitleaks.toml remains the thin consumer.
secret-scanner-reusable.yml -- #597 added a comment explaining why
--verbose is required (without it the gate blocks a merge while telling
the author nothing, which is how 154 false positives sat untriaged across
eleven repositories). Kept, alongside the estate-baseline staging steps.
Verified in a clean worktree, same tree, both configs:
main's config: 4 findings
this branch: 3 findings
regressions: NONE
difference: the baseline file no longer flags itself
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hyperpolymath added a commit that referenced this pull request Aug 7, 2026
#599)
## The deadlock
`.github/CODEOWNERS` made `@hyperpolymath` the **sole** owner of every
path, and the Base ruleset sets `require_code_owner_review: true`.
GitHub will not let a pull request's author satisfy the code-owner
requirement themselves. With one owner who is also always the author,
**the rule could not be satisfied by anybody** — every
maintainer-authored PR sat permanently `BLOCKED`.
The evidence is in the merge history: #592, #593 and #597 all merged
with `DISMISSED` / `CHANGES_REQUESTED` and **zero approving reviews**,
via the admin bypass (`RepositoryRole id=2, mode=always`).
A rule that can never pass doesn't gate anything. It just makes bypass
the normal path — and once bypass is routine it stops carrying
information, which is how three *other* broken rules in this same
ruleset went unnoticed for months.
## The fix
`@JoshuaJewell` (repo admin, the only other org member) is named
co-owner, so either maintainer can approve the other's work and the
requirement becomes satisfiable for the first time.
**On every line, not just `*`.** In CODEOWNERS the last matching rule
wins, and a specific path line *replaces* the default rather than adding
to it. Co-owning only the `*` default would leave `SECURITY.md`,
`.github/workflows/`, `.machine_readable/`, `LICENSE`, `*.sh` and the
rest single-owned — and any PR touching them still deadlocked.
## Bootstrapping
The ruleset reads CODEOWNERS from the **base** branch, so this change
only takes effect once it is on `main`. This PR itself therefore still
needs one admin-bypass merge. After that, code-owner review works
normally.
## Context
This is the fourth of four independent deadlocks found in this ruleset
on 2026-08-07:
1. `code_quality` / `code_coverage` / `copilot_code_review` — features
producing no PR result — **removed**
2. `required_deployments: github-pages` — Pages deploys on `push` only,
so no PR head can ever have one — **removed** (18 other repos still
affected; see `scripts/clear-pages-deployment-deadlock.sh`)
3. `code_scanning` @ error threshold — 13 alerts from May–July, all
already excused in `.hypatia-baseline.json` — **resolved**, 12 dismissed
4. `require_code_owner_review` — **this PR**
🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.

1 participant

@hyperpolymath