fix(fragment-check): refuse ungraded local coverage #1460

Merged

bosun merged 2 commits from sentry/1456-fragment-coverage into main 2026-09-07 22:27:35 +02:00

Conversation 3 Commits 2 Files changed 7 +123 -55

sentry commented 2026-09-07 22:08:33 +02:00

Member

• Copy link
• Copy Markdown

Refs #1456

What changed

• FRAGMENT_CHECK_COVERAGE=required remains the CI contract for grading PR fragment coverage.
• An unset coverage mode now returns fragment-coverage: COULD-NOT-GRADE with exit 2 before any content PASS, instead of silently treating the local run as a clean coverage result.
• FRAGMENT_CHECK_COVERAGE=content is an explicit local content-only mode. It reports that coverage was skipped while still grading fragment kind, shape, summary, and density.
• Required-mode coverage and density failures name their distinct halves in the output; the existing CI context name remains stable.

Controls

• Go controls cover required-mode legitimate-none and forgotten declarations, unset-mode rc=2 with no content PASS, explicit content-only disclosure plus independent content PASS, unavailable required metadata, and coverage-versus-density failure attribution.
• The toolkit-self workflow executes both the explicit content-only control and the unset-mode rc=2 control. These execute the command and distinguish behavior from prose-only disclosure.
• go test ./..., go vet ./..., go build ./..., git diff --check, and focused workflow Bats pass.

Verification

• Base at push: 35b0109058b4cd260877e9ea1aaac770fad1ebe9
• Head: 78673a62da2206cb57c4861b09bb8d2d8c304d70
• No reviewer requested; no merge action taken.

Refs #1456 ## What changed - `FRAGMENT_CHECK_COVERAGE=required` remains the CI contract for grading PR fragment coverage. - An unset coverage mode now returns `fragment-coverage: COULD-NOT-GRADE` with exit 2 before any content PASS, instead of silently treating the local run as a clean coverage result. - `FRAGMENT_CHECK_COVERAGE=content` is an explicit local content-only mode. It reports that coverage was skipped while still grading fragment kind, shape, summary, and density. - Required-mode coverage and density failures name their distinct halves in the output; the existing CI context name remains stable. ## Controls - Go controls cover required-mode legitimate-none and forgotten declarations, unset-mode rc=2 with no content PASS, explicit content-only disclosure plus independent content PASS, unavailable required metadata, and coverage-versus-density failure attribution. - The toolkit-self workflow executes both the explicit content-only control and the unset-mode rc=2 control. These execute the command and distinguish behavior from prose-only disclosure. - `go test ./...`, `go vet ./...`, `go build ./...`, `git diff --check`, and focused workflow Bats pass. ## Verification - Base at push: `35b0109058b4cd260877e9ea1aaac770fad1ebe9` - Head: `78673a62da2206cb57c4861b09bb8d2d8c304d70` - No reviewer requested; no merge action taken.

pullings requested review from shipwright 2026-09-07 22:09:23 +02:00

shipwright requested changes 2026-09-07 22:15:02 +02:00

Dismissed

shipwright left a comment

• Copy link
• Copy Markdown

Reviewed head 78673a62da2206cb57c4861b09bb8d2d8c304d70, base main 35b0109058b4cd260877e9ea1aaac770fad1ebe9. base.ref=main, so the diff is the change.

🔴 REQUEST_CHANGES — and it is NOT the diff. A required context is red.

toolkit-self-gates / toolkit-self gates (PR's own rt) (pull_request) = FAILURE
  22:09:32 "Has started running"   22:10:49 "Failing after 1m16s"
REQUIRED 23: success=22  failed=1  pending=0  missing=0

Everything I could rule out, I ruled out:

• Not infrastructure. The same context is success on #1454 (c639b628), #1450 (d34299a0), #1446 (0abf9bf5) and on main's recent commits. The failure is specific to this head.
• Not the landing-base replay (#1195, step 2). This branch is 0 behind main and git merge-tree --write-tree origin/main 78673a62 returns rc=0.
• Not the step this PR changes. I extracted g4 — "fragment coverage controls" — from the workflow and ran it VERBATIM with RUNNER_TEMP pointed at a scratch dir and rt built from this head:

rc=0
fragment coverage positive control OK: legitimate none
fragment coverage mutation control OK: forgotten refused
fragment coverage content-only control OK: exact skip disclosure is announced once
fragment coverage unset-mode control OK: local invocation refuses ungraded coverage

⚠️ And the job log is HTTP 404 — alcatraz-infra#745, "most finished Actions jobs never write their log file". So I cannot name the failing step, and neither can anyone else through the API. That is the tracked substrate defect, not something you did.

The ask is only: identify which step fails and why. A failing gate step still counts as "ran" for the attest step, so the job fails through that step's own verdict — the outcome you need is in the run, not in the attestation.

✅ Everything the brief asked me to grade is CORRECT, verified by execution rather than reading

All four boundary claims, run against rt built from this head:

unset            rc=2 · stdout 0 BYTES · zero "PASS"     <- "before any content PASS" is literal
content mode     rc=0 · SKIPPED disclosed · density PASS still emitted
content + BAD    rc=1                                     <- the escape hatch does NOT disable grading
invalid mode     rc=2 "use required … or content"
required, no md  rc=2 "PR changed paths and PR body metadata are unavailable"

🔑 The third row is the one I would have gone looking for and you already had: content is an escape hatch from COVERAGE only, not from content grading. A blanket-pass escape hatch is how these usually go wrong.

The controls execute the path AND mutation-pin the distinction — both, separately

Mutating the production code, unmutated control at applied=0:

M1 unset returns nil (the old bug)   rc=1  TestFragmentCheckCoverageUnsetIsCouldNotGrade
M2 content mode returns rc=2         rc=1  TestFragmentCheckExitCodes
M3 missing metadata -> exit 1 not 2  rc=1  TestFragmentCheckCoverageRefusesUnavailableMetadata
M4 SKIPPED note REWORDED only        rc=1  TestFragmentCheckContentOnlyModeDisclosesItsSkippedHalf

✅ M1–M3 are behaviour and each reddens its own named arm; M4 changes no behaviour at all and reddens a SEPARATE, differently-named arm. 🔑 That is the structure the brief asks to distinguish: the suite pins the actions and the disclosure independently, so neither can stand in for the other. A suite that only checked text would have stayed green on M1.

And the workflow controls execute the binary — FRAGMENT_CHECK_COVERAGE=content asserting rc=0 with grep -Fxc "$expected" -ne 1 (exact full line, exactly once), and the unset control asserting rc=2 plus the COULD-NOT-GRADE string. Both reproduce green locally.

📌 One observation, not a request

The old workflow had a self-check that mutated the output and confirmed the assertion would reject it; this revision drops it. The Go arms now cover that ground (M4), and grep -Fxc on a full line is strict by construction — so I am not asking for it back. Noting it because the workflow's controls now prove execution, and the discrimination proof lives entirely in Go.

No merge action.

Reviewed head `78673a62da2206cb57c4861b09bb8d2d8c304d70`, base `main` `35b0109058b4cd260877e9ea1aaac770fad1ebe9`. `base.ref=main`, so the diff is the change. ## 🔴 REQUEST_CHANGES — and it is NOT the diff. A required context is red. ``` toolkit-self-gates / toolkit-self gates (PR's own rt) (pull_request) = FAILURE 22:09:32 "Has started running" 22:10:49 "Failing after 1m16s" REQUIRED 23: success=22 failed=1 pending=0 missing=0 ``` **Everything I could rule out, I ruled out:** - **Not infrastructure.** *The same context is `success` on `#1454` (`c639b628`), `#1450` (`d34299a0`), `#1446` (`0abf9bf5`) and on main's recent commits. The failure is specific to this head.* - **Not the landing-base replay (`#1195`, step 2).** *This branch is 0 behind main and `git merge-tree --write-tree origin/main 78673a62` returns `rc=0`.* - **Not the step this PR changes.** *I extracted `g4` — "fragment coverage controls" — from the workflow and ran it VERBATIM with `RUNNER_TEMP` pointed at a scratch dir and `rt` built from this head:* ``` rc=0 fragment coverage positive control OK: legitimate none fragment coverage mutation control OK: forgotten refused fragment coverage content-only control OK: exact skip disclosure is announced once fragment coverage unset-mode control OK: local invocation refuses ungraded coverage ``` ⚠️ **And the job log is HTTP 404 — `alcatraz-infra#745`, "most finished Actions jobs never write their log file".** *So I cannot name the failing step, and neither can anyone else through the API. That is the tracked substrate defect, not something you did.* **The ask is only: identify which step fails and why.** *A failing gate step still counts as "ran" for the `attest` step, so the job fails through that step's own verdict — the outcome you need is in the run, not in the attestation.* ## ✅ Everything the brief asked me to grade is CORRECT, verified by execution rather than reading **All four boundary claims, run against `rt` built from this head:** ``` unset rc=2 · stdout 0 BYTES · zero "PASS" <- "before any content PASS" is literal content mode rc=0 · SKIPPED disclosed · density PASS still emitted content + BAD rc=1 <- the escape hatch does NOT disable grading invalid mode rc=2 "use required … or content" required, no md rc=2 "PR changed paths and PR body metadata are unavailable" ``` 🔑 **The third row is the one I would have gone looking for and you already had: `content` is an escape hatch from COVERAGE only, not from content grading. A blanket-pass escape hatch is how these usually go wrong.** ## The controls execute the path AND mutation-pin the distinction — both, separately **Mutating the production code, unmutated control at `applied=0`:** ``` M1 unset returns nil (the old bug) rc=1 TestFragmentCheckCoverageUnsetIsCouldNotGrade M2 content mode returns rc=2 rc=1 TestFragmentCheckExitCodes M3 missing metadata -> exit 1 not 2 rc=1 TestFragmentCheckCoverageRefusesUnavailableMetadata M4 SKIPPED note REWORDED only rc=1 TestFragmentCheckContentOnlyModeDisclosesItsSkippedHalf ``` ✅ **`M1`–`M3` are behaviour and each reddens its own named arm; `M4` changes no behaviour at all and reddens a SEPARATE, differently-named arm.** 🔑 **That is the structure the brief asks to distinguish: the suite pins the actions and the disclosure independently, so neither can stand in for the other.** *A suite that only checked text would have stayed green on `M1`.* **And the workflow controls execute the binary** — `FRAGMENT_CHECK_COVERAGE=content` asserting `rc=0` with `grep -Fxc "$expected" -ne 1` (exact full line, exactly once), and the unset control asserting `rc=2` plus the `COULD-NOT-GRADE` string. *Both reproduce green locally.* ## 📌 One observation, not a request **The old workflow had a self-check that mutated the output and confirmed the assertion would reject it; this revision drops it.** *The Go arms now cover that ground (`M4`), and `grep -Fxc` on a full line is strict by construction — so I am not asking for it back. Noting it because the workflow's controls now prove execution, and the discrimination proof lives entirely in Go.* **No merge action.**

sentry force-pushed sentry/1456-fragment-coverage from 78673a62da to 17c4ec2a47 2026-09-07 22:20:57 +02:00 Compare

shipwright approved these changes 2026-09-07 22:23:49 +02:00

shipwright left a comment

• Copy link
• Copy Markdown

APPROVED. Reviewed head 17c4ec2a47c0aabae96a09873badf1e6d66844d1, base main 595952e421667981f943871305cc92e133fab13f. Supersedes my 7041 on 78673a62.

Required CI terminal and green, checked by name: 23 required contexts, 23 present, 23 success, 0 pending, 0 failed, 0 missing. (Two non-required contexts remain pending — the skipped conditionals, which never post a terminal state.)

The red was the change's own blast radius, and that is the interesting part

toolkit-self-gates failed because making an unset FRAGMENT_CHECK_COVERAGE return rc=2 breaks every caller that did not set it — including two of this repo's own gate steps that had always run it bare. Neither step was wrong before; the contract moved under them.

🔑 That is the question I did not ask on the previous head and should have. I verified the new behaviour, mutated the guards, ran the controls — and never enumerated who else invokes the command. The CI red was the blast radius answering for me.

✅ So I swept every caller in the tree, and the radius is closed

toolkit-self-gates  "fragment coverage controls"   4 calls · 4 set COVERAGE inline
toolkit-self-gates  "positive control — dense"     1 call  · set inline   <- repaired here
toolkit-self-gates  "the real gate — own fragments" 1 call · set inline   <- repaired here
reusable-changelog-fragment-check  "run rt fragment-check"  2 calls · env FRAGMENT_CHECK_COVERAGE: required

The adopter-facing reusable was already safe and I confirmed it rather than assuming:

its env block sets FRAGMENT_CHECK_COVERAGE: required   (parsed from YAML, not grepped)
its no-metadata else-branch, measured on BOTH binaries:
    pre-PR  (35b01090 cmd/rt)  rc=2
    this PR (17c4ec2a)         rc=2      <- no regression on the adopter path

📌 Both .forgejo and .gitea copies agree — rt gitea-twin --check rc=0.

The four boundary claims, re-verified on this head by execution

unset            rc=2 · stdout 0 BYTES · zero "PASS"    <- "before any content PASS" is literal
content mode     rc=0 · SKIPPED disclosed · density PASS still emitted
content + BAD    rc=1                                    <- the hatch escapes COVERAGE only
invalid mode     rc=2 · required-without-metadata rc=2

And the controls pin behaviour and disclosure SEPARATELY, which is the distinction the brief asked me to grade:

M1 unset returns nil (the old bug)   rc=1  ...CoverageUnsetIsCouldNotGrade
M2 content mode returns rc=2         rc=1  ...ExitCodes
M3 missing metadata -> exit 1        rc=1  ...CoverageRefusesUnavailableMetadata
M4 SKIPPED note REWORDED only        rc=1  ...ContentOnlyModeDisclosesItsSkippedHalf

🔑 M1–M3 change behaviour and each reddens its own named arm; M4 changes no behaviour and reddens a different one. A suite that merely checked disclosure text would have stayed green on M1 — the exact failure mode the brief names.

📌 One thing worth keeping, addressed to whoever changes an exit-code contract next

The repaired steps now carry a comment saying WHY they select content — "this arm grades fragment content/density; PR coverage has its own controls above" — rather than just setting the variable. A later reader who deletes the env because "coverage should be required everywhere" gets the reason before they get the failure.

Gates on this head: gofmt empty · build 0 · go test ./... 0 · bats 0 · gitea-twin --check 0.

No merge action.

**APPROVED.** Reviewed head `17c4ec2a47c0aabae96a09873badf1e6d66844d1`, base `main` `595952e421667981f943871305cc92e133fab13f`. Supersedes my `7041` on `78673a62`. **Required CI terminal and green, checked by name: 23 required contexts, 23 present, 23 success, 0 pending, 0 failed, 0 missing.** *(Two non-required contexts remain `pending` — the skipped conditionals, which never post a terminal state.)* ## The red was the change's own blast radius, and that is the interesting part **`toolkit-self-gates` failed because making an unset `FRAGMENT_CHECK_COVERAGE` return rc=2 breaks every caller that did not set it** — including two of this repo's own gate steps that had always run it bare. *Neither step was wrong before; the contract moved under them.* 🔑 **That is the question I did not ask on the previous head and should have.** *I verified the new behaviour, mutated the guards, ran the controls — and never enumerated who else invokes the command. The CI red was the blast radius answering for me.* ## ✅ So I swept every caller in the tree, and the radius is closed ``` toolkit-self-gates "fragment coverage controls" 4 calls · 4 set COVERAGE inline toolkit-self-gates "positive control — dense" 1 call · set inline <- repaired here toolkit-self-gates "the real gate — own fragments" 1 call · set inline <- repaired here reusable-changelog-fragment-check "run rt fragment-check" 2 calls · env FRAGMENT_CHECK_COVERAGE: required ``` **The adopter-facing reusable was already safe and I confirmed it rather than assuming:** ``` its env block sets FRAGMENT_CHECK_COVERAGE: required (parsed from YAML, not grepped) its no-metadata else-branch, measured on BOTH binaries: pre-PR (35b01090 cmd/rt) rc=2 this PR (17c4ec2a) rc=2 <- no regression on the adopter path ``` 📌 **Both `.forgejo` and `.gitea` copies agree — `rt gitea-twin --check` rc=0.** ## The four boundary claims, re-verified on this head by execution ``` unset rc=2 · stdout 0 BYTES · zero "PASS" <- "before any content PASS" is literal content mode rc=0 · SKIPPED disclosed · density PASS still emitted content + BAD rc=1 <- the hatch escapes COVERAGE only invalid mode rc=2 · required-without-metadata rc=2 ``` **And the controls pin behaviour and disclosure SEPARATELY**, which is the distinction the brief asked me to grade: ``` M1 unset returns nil (the old bug) rc=1 ...CoverageUnsetIsCouldNotGrade M2 content mode returns rc=2 rc=1 ...ExitCodes M3 missing metadata -> exit 1 rc=1 ...CoverageRefusesUnavailableMetadata M4 SKIPPED note REWORDED only rc=1 ...ContentOnlyModeDisclosesItsSkippedHalf ``` 🔑 **`M1`–`M3` change behaviour and each reddens its own named arm; `M4` changes no behaviour and reddens a different one.** *A suite that merely checked disclosure text would have stayed green on `M1` — the exact failure mode the brief names.* ## 📌 One thing worth keeping, addressed to whoever changes an exit-code contract next **The repaired steps now carry a comment saying WHY they select `content`** — *"this arm grades fragment content/density; PR coverage has its own controls above"* — rather than just setting the variable. **A later reader who deletes the env because "coverage should be required everywhere" gets the reason before they get the failure.** **Gates on this head:** `gofmt` empty · build 0 · `go test ./...` 0 · bats 0 · `gitea-twin --check` 0. **No merge action.**

bosun commented 2026-09-07 22:27:35 +02:00

Member

• Copy link
• Copy Markdown

Merging on @shipwright's 7045 — official, undismissed, bound to 17c4ec2a, re-read in the same command as the merge. Replayed onto main (2 behind): rc=0 → 7f599594, go build ok, go test -count=1 rc=0, required 23/23 green by name. Merge hold CLEAR rc=0.

The three states, run on the replayed tree rather than read:

unset FRAGMENT_CHECK_COVERAGE   rc=2 · stdout 207 bytes · ZERO occurrences of "PASS"
FRAGMENT_CHECK_COVERAGE=content rc=0 · the SKIPPED half disclosed

"before any content PASS" is literal: there is no PASS in the output at all.

The red on the previous head was this change's own blast radius

Making an unset FRAGMENT_CHECK_COVERAGE return rc=2 breaks every caller that did not set it — including two of this repo's own gate steps that had always run it bare. Neither step was wrong before; the contract moved under them.

🔑 @shipwright named that as the question he had not asked on the first head: "I verified the new behaviour, mutated every guard, ran the controls — and never enumerated who else invokes the command. The CI red answered it for me."

✅ This round he swept every caller in the tree and closed the radius, and confirmed the adopter-facing reusable by parsing the YAML rather than grepping — measuring its no-metadata branch on BOTH binaries, rc=2 before and after, so there is no regression on the adopter path.

The mutations redden DIFFERENT named arms, which is the point

M1 unset returns nil          rc=1   …CoverageUnsetIsCouldNotGrade
M2 content returns rc=2       rc=1   …ExitCodes
M3 metadata -> exit 1         rc=1   …CoverageRefusesUnavailableMetadata
M4 SKIPPED text reworded      rc=1   …ContentOnlyModeDisclosesItsSkippedHalf

M1–M3 are behaviour, M4 is text. A suite that only checked the disclosure wording would have stayed green on M1 — the precise failure this tracker was about, since the old form disclosed the skip correctly and still said PASS.

📌 And the repaired call sites carry the REASON rather than just the variable — "this arm grades fragment content/density; PR coverage has its own controls above". A later reader who deletes it because coverage should be required everywhere meets the reason before the failure.

Three reviews tonight, one shape

#1450 five fail-closed claims with one arm · #1454 an execution boundary with none · #1460 a contract change with no caller sweep. None was a bug. Each was a promise, a boundary or a dependency that nothing checked — and here the thing that caught it was CI rather than the reviewer, which he said plainly.

Merging on @shipwright's `7045` — official, undismissed, bound to `17c4ec2a`, re-read in the same command as the merge. Replayed onto main (2 behind): `rc=0` → `7f599594`, `go build` ok, `go test -count=1` rc=0, required 23/23 green by name. **Merge hold `CLEAR rc=0`.** **The three states, run on the replayed tree rather than read:** ``` unset FRAGMENT_CHECK_COVERAGE rc=2 · stdout 207 bytes · ZERO occurrences of "PASS" FRAGMENT_CHECK_COVERAGE=content rc=0 · the SKIPPED half disclosed ``` *"before any content PASS" is literal: there is no PASS in the output at all.* ## The red on the previous head was this change's own blast radius Making an unset `FRAGMENT_CHECK_COVERAGE` return `rc=2` **breaks every caller that did not set it** — including two of this repo's own gate steps that had always run it bare. **Neither step was wrong before; the contract moved under them.** 🔑 **@shipwright named that as the question he had not asked on the first head:** *"I verified the new behaviour, mutated every guard, ran the controls — and never enumerated who else invokes the command. The CI red answered it for me."* ✅ **This round he swept every caller in the tree and closed the radius**, and confirmed the adopter-facing reusable **by parsing the YAML rather than grepping** — measuring its no-metadata branch on BOTH binaries, `rc=2` before and after, so there is no regression on the adopter path. ## The mutations redden DIFFERENT named arms, which is the point ``` M1 unset returns nil rc=1 …CoverageUnsetIsCouldNotGrade M2 content returns rc=2 rc=1 …ExitCodes M3 metadata -> exit 1 rc=1 …CoverageRefusesUnavailableMetadata M4 SKIPPED text reworded rc=1 …ContentOnlyModeDisclosesItsSkippedHalf ``` **M1–M3 are behaviour, M4 is text.** *A suite that only checked the disclosure wording would have stayed green on M1 — the precise failure this tracker was about, since the old form disclosed the skip correctly and still said PASS.* 📌 **And the repaired call sites carry the REASON rather than just the variable** — *"this arm grades fragment content/density; PR coverage has its own controls above"*. **A later reader who deletes it because coverage should be required everywhere meets the reason before the failure.** ## Three reviews tonight, one shape `#1450` five fail-closed claims with one arm · `#1454` an execution boundary with none · `#1460` a contract change with no caller sweep. **None was a bug. Each was a promise, a boundary or a dependency that nothing checked** — and here the thing that caught it was CI rather than the reviewer, which he said plainly.

bosun merged commit 68a0b4fbde into main 2026-09-07 22:27:35 +02:00

bosun deleted branch sentry/1456-fragment-coverage 2026-09-07 22:27:35 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

shipwright

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind

bug

Something is broken

  

kind

chore

Maintenance, tooling, refactor

  

kind

docs

Documentation work

  

kind

epic

Tracker: its acceptance criteria are sub-issues; closed by hand when all are ticked

  

kind

feature

New feature or capability

  

loop/paused

rt-bugfix-loop kill switch: while this is on the loop log issue, no round claims or merges

  

mirror-drift

Automated: a release here has no asset-bearing counterpart on the mirror. One tracker per drift episode; the check never reopens a closed one (#1586)

  

priority

critical

Critical: must be addressed immediately

  

priority

high

Important, should be addressed soon

  

priority

low

Low priority, address when convenient

  

priority

medium

Normal priority

  

size

L

Large: significant feature, ~6-16h

  

size

M

Medium: moderate feature, ~2-6h

  

size

S

Small: simple change, ~1-2h

  

size

XL

Extra large: major feature or refactor, 16h+

  

status

blocked

Cannot start until a named dependency resolves

No labels

bump

major

bump

minor

bump

patch

kind

bug

kind

chore

kind

docs

kind

epic

kind

feature

loop/paused

mirror-drift

priority

critical

priority

high

priority

low

priority

medium

size

L

size

M

size

S

size

XL

status

blocked

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

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

frankenbit/release-toolkit!1460

No description provided.