[PR #163] [MERGED] feat(pr-risk): record which files supplied the reversibility tier (BE-7418) #172

New issue

Closed

opened 2026-10-26 22:37:01 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:01 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/163
Author: @mattmillerai
Created: 8/13/2026
Status: ✅ Merged
Merged: 8/13/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-7418-reversibility-files

📝 Commits (2)

c73a829 feat(pr-risk): record which files supplied the reversibility tier (BE-7418)
aea46cd fix(pr-risk): attribute reversibility across BOTH R3 rungs, not just the first (BE-7418)

📊 Changes

3 files changed (+198 additions, -10 deletions)

View changed files

📝 scripts/pr-risk/README.md (+13 -1)
📝 scripts/pr-risk/grade-pr-risk.sh (+79 -9)
📝 scripts/pr-risk/tests/test_grade_pr_risk.sh (+106 -0)

📄 Description

ELI-5

The risk grader already tells you which files made the path-floor axis land where it did. The reversibility axis just said "R3 — touches migrations" without naming the migration. This adds axes.reversibility.files: the changed paths that actually supplied that tier. It's a label on the record, nothing more — no tier moves.

What changed

grade-pr-risk.sh AXIS 3 now emits files in its record ({tier, status, reason, files, flag_gated, deleted_files}), populated on the two rungs where the tier is attributable to specific files:

irreversible class — the $A1.files rows whose per-file classes intersect the matched irreversible classes. The rows already carry classes, matched on destination and origin, so this is a pure derivation over what the path axis already computed — no second matching pass.
removal under a sensitive class — attributed by row, not by intersecting a row's classes with the sensitive list. A MODIFIED auth/ file also carries class auth and removed nothing; naming it here would send the publisher peeling a file that cannot change the branch. A row qualifies when the path it removed (.path for DELETED, previous_path for RENAMED — the same two halves that build $deleted) matches a rule in a delete_sensitive class. The destination path is recorded, so the publisher's subset test against axes.path_floor.files[].path compares like with like.

null on the other three rungs. "No green rollup" is a property of the head commit and "no test touched" of the whole change set — neither is removable by dropping files, so null reads as "not attributable, keep suppressing", which an empty array would not.

Nothing existing was renamed or moved: publish-risk-surfaces.sh and apply-risk-label.sh read this record and their suites are unchanged and green.

Why it's safe (the report-only contract)

Same guard the per-file path floors use. The list is derived after the tier decision $d and is read nowhere else in the grader, so it cannot become a second input to grading. To make that mechanical rather than aspirational, each branch of the decision ladder now carries a k tag naming itself, and the attribution keys on $d.k instead of re-testing the same conditions — a re-test is a second copy of the ladder that can drift out of step with it, and a files list that disagrees with the reason printed beside it is worse than no list at all. k never reaches $d.t.

Empirically pinned: graded a 10-record corpus (both R3 branches, all three null rungs, a fork, an unknown path axis, an uncollected rollup) with the pre-change grader from origin/main and with this one, deleted graded_at and the new files key from both, and diffed — byte-identical. No tier moved anywhere.

Judgment calls

A DELETED file that is not under a sensitive class is not attributed. The ticket's wording ("the .path of rows with change_type == DELETED, or RENAMED whose origin matched a class in delete_sensitive_classes") can be read as attributing every deleted row. I read the class filter as applying to both, because the field's purpose is "which files supplied the tier": deleting README.md alongside src/auth/x.go does not supply the R3 — peeling the README leaves the reason standing, peeling the auth file removes it. This also gives the branch a useful invariant: the list is empty exactly when $del_sensitive is false, i.e. never on this branch. Pinned by a test fixture (README.md DELETED alongside a DELETED auth file → only the auth file is attributed). Easy to flip if the phase-2 consumer wants the broader set.

Tests

New phase 23 in tests/test_grade_pr_risk.sh (122 assertions total, all green):

irreversible-class fixture (migrations/** + a docs file) → only the migration is attributed
delete-sensitive fixture (DELETED auth file + MODIFIED auth file + DELETED README) → only the DELETED auth file; deleted_files count untouched
RENAMED-out-of-a-sensitive-dir fixture → attributed under its DESTINATION path
R2 / R1 / R0 fixtures → files is null
unknown path axis → the axis is unknown before attribution runs, so no list is emitted from a file list nobody read
report-only pin: every fixture's overall tier and axes.reversibility.tier asserted against the values the grader emitted before the field existed
subset property: every attributed path equals some axes.path_floor.files[].path
shape pin: the reversibility record still carries every field it had

Also ran the rest of the pr-risk suites (apply_risk_label, grade_targets, resolve_enabled, pin_contract, publish_risk_surfaces) plus the CI shellcheck invocation and the default map/registry validation — all green.

Provenance

Ticket BE-7418. Deferred from review of #162 (discussion r3774678317); phase 1 of the BE-7414 spike's plan. Base is current main — this does not depend on #162.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/163 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/13/2026 **Status:** ✅ Merged **Merged:** 8/13/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-7418-reversibility-files` --- ### 📝 Commits (2) - [`c73a829`](https://github.com/Comfy-Org/github-workflows/commit/c73a82923f9a6923e2cb508a19a3397bb4964e47) feat(pr-risk): record which files supplied the reversibility tier (BE-7418) - [`aea46cd`](https://github.com/Comfy-Org/github-workflows/commit/aea46cdbff149f4cb6f5babf140fcfa69475289a) fix(pr-risk): attribute reversibility across BOTH R3 rungs, not just the first (BE-7418) ### 📊 Changes **3 files changed** (+198 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `scripts/pr-risk/README.md` (+13 -1) 📝 `scripts/pr-risk/grade-pr-risk.sh` (+79 -9) 📝 `scripts/pr-risk/tests/test_grade_pr_risk.sh` (+106 -0) </details> ### 📄 Description ## ELI-5 The risk grader already tells you *which files* made the path-floor axis land where it did. The reversibility axis just said "R3 — touches migrations" without naming the migration. This adds `axes.reversibility.files`: the changed paths that actually supplied that tier. It's a label on the record, nothing more — no tier moves. ## What changed `grade-pr-risk.sh` AXIS 3 now emits `files` in its record (`{tier, status, reason, files, flag_gated, deleted_files}`), populated on the two rungs where the tier is attributable to specific files: - **irreversible class** — the `$A1.files` rows whose per-file `classes` intersect the matched irreversible classes. The rows already carry `classes`, matched on destination *and* origin, so this is a pure derivation over what the path axis already computed — no second matching pass. - **removal under a sensitive class** — attributed **by row**, not by intersecting a row's classes with the sensitive list. A MODIFIED `auth/` file also carries class `auth` and removed nothing; naming it here would send the publisher peeling a file that cannot change the branch. A row qualifies when the path it *removed* (`.path` for `DELETED`, `previous_path` for `RENAMED` — the same two halves that build `$deleted`) matches a rule in a `delete_sensitive` class. The **destination** path is recorded, so the publisher's subset test against `axes.path_floor.files[].path` compares like with like. `null` on the other three rungs. "No green rollup" is a property of the head *commit* and "no test touched" of the *whole change set* — neither is removable by dropping files, so `null` reads as "not attributable, keep suppressing", which an empty array would not. Nothing existing was renamed or moved: `publish-risk-surfaces.sh` and `apply-risk-label.sh` read this record and their suites are unchanged and green. ## Why it's safe (the report-only contract) Same guard the per-file path floors use. The list is derived **after** the tier decision `$d` and is read nowhere else in the grader, so it cannot become a second input to grading. To make that mechanical rather than aspirational, each branch of the decision ladder now carries a `k` tag naming itself, and the attribution keys on `$d.k` instead of re-testing the same conditions — a re-test is a second copy of the ladder that can drift out of step with it, and a `files` list that disagrees with the `reason` printed beside it is worse than no list at all. `k` never reaches `$d.t`. Empirically pinned: graded a 10-record corpus (both R3 branches, all three null rungs, a fork, an unknown path axis, an uncollected rollup) with the pre-change grader from `origin/main` and with this one, deleted `graded_at` and the new `files` key from both, and diffed — **byte-identical**. No tier moved anywhere. ## Judgment calls **A DELETED file that is *not* under a sensitive class is not attributed.** The ticket's wording ("the `.path` of rows with `change_type == DELETED`, or `RENAMED` whose origin matched a class in `delete_sensitive_classes`") can be read as attributing *every* deleted row. I read the class filter as applying to both, because the field's purpose is "which files supplied the tier": deleting `README.md` alongside `src/auth/x.go` does not supply the R3 — peeling the README leaves the reason standing, peeling the auth file removes it. This also gives the branch a useful invariant: the list is empty exactly when `$del_sensitive` is false, i.e. never on this branch. Pinned by a test fixture (`README.md` DELETED alongside a DELETED auth file → only the auth file is attributed). Easy to flip if the phase-2 consumer wants the broader set. ## Tests New phase 23 in `tests/test_grade_pr_risk.sh` (122 assertions total, all green): - irreversible-class fixture (`migrations/**` + a docs file) → only the migration is attributed - delete-sensitive fixture (DELETED auth file + MODIFIED auth file + DELETED README) → only the DELETED auth file; `deleted_files` count untouched - RENAMED-out-of-a-sensitive-dir fixture → attributed under its DESTINATION path - R2 / R1 / R0 fixtures → `files` is `null` - unknown path axis → the axis is `unknown` before attribution runs, so no list is emitted from a file list nobody read - report-only pin: every fixture's overall tier and `axes.reversibility.tier` asserted against the values the grader emitted before the field existed - subset property: every attributed path equals some `axes.path_floor.files[].path` - shape pin: the reversibility record still carries every field it had Also ran the rest of the pr-risk suites (`apply_risk_label`, `grade_targets`, `resolve_enabled`, `pin_contract`, `publish_risk_surfaces`) plus the CI shellcheck invocation and the default map/registry validation — all green. ## Provenance Ticket BE-7418. Deferred from review of #162 (discussion r3774678317); phase 1 of the BE-7414 spike's plan. Base is current `main` — this does not depend on #162. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:01 +00:00