[PR #165] [MERGED] fix(pr-risk): let the reducibility clause speak on a fully-attributed reversibility tie (BE-7419) #174

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/165
Author: @mattmillerai
Created: 8/13/2026
Status: ✅ Merged
Merged: 8/13/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-7419-reducibility-rev-tie

📝 Commits (2)

04a0d07 fix(pr-risk): let the reducibility clause speak on a fully-attributed reversibility tie (BE-7419)
5d3ad09 fix(pr-risk): bound the reducibility exception by where the axis actually lands (BE-7419)

📊 Changes

5 files changed (+317 additions, -33 deletions)

View changed files

📝 scripts/pr-risk/README.md (+40 -8)
📝 scripts/pr-risk/grade-pr-risk.sh (+25 -1)
📝 scripts/pr-risk/publish-risk-surfaces.sh (+118 -15)
📝 scripts/pr-risk/tests/test_grade_pr_risk.sh (+35 -1)
📝 scripts/pr-risk/tests/test_publish_risk_surfaces.sh (+99 -8)

📄 Description

ELI-5

The risk comment sometimes offers "split this PR and the leftovers grade cheaper." It refused to make that offer whenever another axis matched the headline tier. For provenance that refusal is right — provenance is about who wrote the PR, so it follows the leftovers into the split and the split buys nothing. For reversibility it was often wrong: reversibility is about specific files, and those files are frequently the exact ones the split proposes peeling off. So one R3 migration under 600 lines of R0 docs got no offer at all, while the identical file set with a lower reversibility tier got the full offer. Same peel, two different answers. Now the two ties are judged separately, and a reversibility tie steps aside when the grader's own attribution proves the peel takes it with it.

What changed

scripts/pr-risk/publish-risk-surfaces.sh, all three surfaces in one commit (they share the predicate today precisely so they cannot disagree):

Tie classification split per axis. $prov_tie / $rev_tie replace the single $drivers list where gating is concerned. $drivers itself stays as the combined list — the $conc_tail attribution's wording ("the provenance and reversibility axis proposes R3") is about which axes tied and is correct either way.
$rev_removable — $rev_tie AND axes.reversibility.files is a non-empty array AND every entry is in [$topf[].path].
$path_decided becomes graded AND headline == floor AND (NOT $prov_tie) AND (NOT $rev_tie OR $rev_removable). The clause keeps its existing $comp_tier / $below_pct guards untouched.
Headline $driver gains a first branch: under $path_decided AND $rev_tie it names path and reversibility and carries reversibility's reason, so the <details> split pitch never sits under a headline crediting reversibility alone. $conc_short's gate fires for that combined driver as well as plain path.
The clause's caveat ("final grade still depends on the provenance and reversibility axes at PR time") stays — it remains honest, since the remainder re-derives reversibility on its own classes and its own checks.

scripts/pr-risk/README.md's reducibility-clause paragraph now documents the per-axis tie reading, the full-subset requirement and why it is load-bearing, the null fail-safe, and the combined headline driver.

Why the full-subset test, and not "does reversibility name any peeled file?"

A consumer map override can put an irreversible-class file below the path floor — remap migrations to path R1 while leaving it in irreversible_classes — and there peeling $topf leaves the reversibility reason exactly where it was. A weaker "any" test would speak wrongly on precisely that record. There is a dedicated fixture for it.

files absent or null — the R2 rung ("no green rollup", a property of the head commit) and the R1 rung ("no test touched", a property of the whole change set), plus every record graded before BE-7418 added the field — reads as not removable. That is a deliberate fail-safe back to today's unconditional suppression, and it is pinned as a test rather than left implicit.

Verification

Per .github/workflows/test-pr-risk.yml, run from scripts/pr-risk/:

step	result
`shellcheck -x` (all 11 files)	clean
default map + registry validate	pass
`tests/test_grade_pr_risk.sh`	131 passed, 0 failed
`tests/test_apply_risk_label.sh`	48 passed, 0 failed
`tests/test_grade_targets.sh`	105 passed, 0 failed
`tests/test_pin_contract.sh`	28 passed, 0 failed
`tests/test_publish_risk_surfaces.sh`	91 passed, 0 failed (was 84)
`tests/test_resolve_enabled.sh`	27 passed, 0 failed

Red-before-green, measured: the new suite was run against HEAD:scripts/pr-risk/publish-risk-surfaces.sh (the pre-change script) and returned 87 passed, 4 failed — failing exactly the four positive assertions (clause prints, caveat attached, headline names both axes, $conc_short fires) and passing every silence assertion. That is the ticket's reproduction confirmed against the shipped code, and it also proves the new SILENT assertions pin pre-existing behavior rather than behavior this PR introduces.

Malformed-input probe. axes.reversibility.files was driven with "a string", 42, true, {"a":1}, [null], [42], and a partial-subset ["…", null]. Every one renders silently at exit 0 with no clause and no jq error — jq's and short-circuits ahead of the $rf[] iteration, so a non-array can never reach it.

Test coverage added

The record / file_entry helpers now carry axes.reversibility.files and per-file classes, both defaulting to what every pre-existing fixture used (null and ["cls"]), so no existing assertion changes meaning. New cases:

migration + docs, reversibility R3 tie with files: ["migrations/0042_drop.sql"] ⊆ $topf → clause prints with the remainder's floor, caveat attached, headline path and reversibility, $conc_short fires
the consumer-override case (irreversible-class file below the floor) → silent, headline still credits reversibility alone, no above-the-fold fragment
files: null → silent (backward compatibility pinned)
files: [] → silent (an empty list is a subset of everything; rejected by the non-empty test)
a partial subset → silent
provenance tie and a removable reversibility tie → silent (the provenance half survives any peel)

The existing provenance-tie assertion is untouched; only the comment above it was narrowed to say "provenance tie", since the blanket claim is no longer true of both axes.

Judgment calls

$drivers kept as the combined list rather than removed. The ticket allowed either; keeping it means $conc_tail is byte-identical, and $conc_tail and the new clause are mutually exclusive by construction anyway ($conc_tail needs tier > floor, $path_decided needs tier == floor).
The t key was dropped from $drivers entries now that membership is decided by the pre-computed booleans. Verified by grep that nothing reads .t — the two consumers use .a and [0].r only.
ties($t; $headline) takes the headline as a parameter instead of closing over $tier: the def block is compiled before the record is destructured, so a lexical reference would not resolve.
Headline carries reversibility's reason, not the path floor's. Reversibility is the axis a reader would otherwise assume the split cannot touch, so it is the one worth naming; the path floor's reason is already the subject of the concentration sentence directly below.
Negative-claim falsification (self-review clause e): not triggered. The diff adds no capability denial — it removes a false-negative suppression, so the user-facing outcome strictly grants a readout that was previously withheld. No not supported / unavailable / STOP strings, no throw/deny path, and no test was flipped to assert a dead-end (the new SILENT assertions pin behavior that is green on both the old and new script, as the red-before-green run above shows).

Provenance

Ticket BE-7419, deferred from #162 review thread r3774678317; origin spike BE-7414. Depends on #162 (merged, 14ea7d5) for $path_decided / $drivers / the clause, and on BE-7418 / #163 (merged, c67a2a4) for axes.reversibility.files. Both are on main, so this is a plain PR against main, not a stacked one.

_{🔄 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/165 **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-7419-reducibility-rev-tie` --- ### 📝 Commits (2) - [`04a0d07`](https://github.com/Comfy-Org/github-workflows/commit/04a0d07c3d8c884de2fa99d5a89b186e5ab3ca4c) fix(pr-risk): let the reducibility clause speak on a fully-attributed reversibility tie (BE-7419) - [`5d3ad09`](https://github.com/Comfy-Org/github-workflows/commit/5d3ad090a52f166ce19f786a0e02f6d505bc3ad1) fix(pr-risk): bound the reducibility exception by where the axis actually lands (BE-7419) ### 📊 Changes **5 files changed** (+317 additions, -33 deletions) <details> <summary>View changed files</summary> 📝 `scripts/pr-risk/README.md` (+40 -8) 📝 `scripts/pr-risk/grade-pr-risk.sh` (+25 -1) 📝 `scripts/pr-risk/publish-risk-surfaces.sh` (+118 -15) 📝 `scripts/pr-risk/tests/test_grade_pr_risk.sh` (+35 -1) 📝 `scripts/pr-risk/tests/test_publish_risk_surfaces.sh` (+99 -8) </details> ### 📄 Description ## ELI-5 The risk comment sometimes offers "split this PR and the leftovers grade cheaper." It refused to make that offer whenever *another* axis matched the headline tier. For **provenance** that refusal is right — provenance is about who wrote the PR, so it follows the leftovers into the split and the split buys nothing. For **reversibility** it was often wrong: reversibility is about specific *files*, and those files are frequently the exact ones the split proposes peeling off. So one R3 migration under 600 lines of R0 docs got no offer at all, while the identical file set with a lower reversibility tier got the full offer. Same peel, two different answers. Now the two ties are judged separately, and a reversibility tie steps aside when the grader's own attribution proves the peel takes it with it. ## What changed `scripts/pr-risk/publish-risk-surfaces.sh`, all three surfaces in one commit (they share the predicate today precisely so they cannot disagree): - **Tie classification split per axis.** `$prov_tie` / `$rev_tie` replace the single `$drivers` list *where gating is concerned*. `$drivers` itself stays as the combined list — the `$conc_tail` attribution's wording ("the provenance and reversibility axis proposes R3") is about which axes tied and is correct either way. - **`$rev_removable`** — `$rev_tie` AND `axes.reversibility.files` is a **non-empty array** AND **every** entry is in `[$topf[].path]`. - **`$path_decided`** becomes `graded AND headline == floor AND (NOT $prov_tie) AND (NOT $rev_tie OR $rev_removable)`. The clause keeps its existing `$comp_tier` / `$below_pct` guards untouched. - **Headline `$driver`** gains a first branch: under `$path_decided AND $rev_tie` it names `path and reversibility` and carries reversibility's reason, so the `<details>` split pitch never sits under a headline crediting reversibility alone. `$conc_short`'s gate fires for that combined driver as well as plain `path`. - The clause's caveat ("final grade still depends on the provenance and reversibility axes at PR time") **stays** — it remains honest, since the remainder re-derives reversibility on its own classes and its own checks. `scripts/pr-risk/README.md`'s reducibility-clause paragraph now documents the per-axis tie reading, the full-subset requirement and why it is load-bearing, the `null` fail-safe, and the combined headline driver. ## Why the full-subset test, and not "does reversibility name any peeled file?" A consumer map override can put an irreversible-class file **below** the path floor — remap `migrations` to path R1 while leaving it in `irreversible_classes` — and there peeling `$topf` leaves the reversibility reason exactly where it was. A weaker "any" test would speak wrongly on precisely that record. There is a dedicated fixture for it. `files` absent or `null` — the R2 rung ("no green rollup", a property of the head commit) and the R1 rung ("no test touched", a property of the whole change set), plus every record graded before BE-7418 added the field — reads as **not** removable. That is a deliberate fail-safe back to today's unconditional suppression, and it is pinned as a test rather than left implicit. ## Verification Per `.github/workflows/test-pr-risk.yml`, run from `scripts/pr-risk/`: | step | result | |---|---| | `shellcheck -x` (all 11 files) | clean | | default map + registry validate | pass | | `tests/test_grade_pr_risk.sh` | 131 passed, 0 failed | | `tests/test_apply_risk_label.sh` | 48 passed, 0 failed | | `tests/test_grade_targets.sh` | 105 passed, 0 failed | | `tests/test_pin_contract.sh` | 28 passed, 0 failed | | `tests/test_publish_risk_surfaces.sh` | **91 passed, 0 failed** (was 84) | | `tests/test_resolve_enabled.sh` | 27 passed, 0 failed | **Red-before-green, measured:** the new suite was run against `HEAD:scripts/pr-risk/publish-risk-surfaces.sh` (the pre-change script) and returned **87 passed, 4 failed** — failing exactly the four *positive* assertions (clause prints, caveat attached, headline names both axes, `$conc_short` fires) and passing every silence assertion. That is the ticket's reproduction confirmed against the shipped code, and it also proves the new SILENT assertions pin pre-existing behavior rather than behavior this PR introduces. **Malformed-input probe.** `axes.reversibility.files` was driven with `"a string"`, `42`, `true`, `{"a":1}`, `[null]`, `[42]`, and a partial-subset `["…", null]`. Every one renders silently at exit 0 with no clause and no jq error — jq's `and` short-circuits ahead of the `$rf[]` iteration, so a non-array can never reach it. ## Test coverage added The `record` / `file_entry` helpers now carry `axes.reversibility.files` and per-file `classes`, both defaulting to what every pre-existing fixture used (`null` and `["cls"]`), so no existing assertion changes meaning. New cases: - migration + docs, reversibility R3 tie with `files: ["migrations/0042_drop.sql"]` ⊆ `$topf` → clause **prints** with the remainder's floor, caveat attached, headline `path and reversibility`, `$conc_short` fires - the consumer-override case (irreversible-class file below the floor) → **silent**, headline still credits reversibility alone, no above-the-fold fragment - `files: null` → **silent** (backward compatibility pinned) - `files: []` → **silent** (an empty list is a subset of everything; rejected by the non-empty test) - a **partial** subset → **silent** - provenance tie *and* a removable reversibility tie → **silent** (the provenance half survives any peel) The existing provenance-tie assertion is untouched; only the comment above it was narrowed to say "provenance tie", since the blanket claim is no longer true of both axes. ## Judgment calls - **`$drivers` kept as the combined list** rather than removed. The ticket allowed either; keeping it means `$conc_tail` is byte-identical, and `$conc_tail` and the new clause are mutually exclusive by construction anyway (`$conc_tail` needs `tier > floor`, `$path_decided` needs `tier == floor`). - **The `t` key was dropped from `$drivers` entries** now that membership is decided by the pre-computed booleans. Verified by grep that nothing reads `.t` — the two consumers use `.a` and `[0].r` only. - **`ties($t; $headline)` takes the headline as a parameter** instead of closing over `$tier`: the `def` block is compiled before the record is destructured, so a lexical reference would not resolve. - **Headline carries reversibility's reason, not the path floor's.** Reversibility is the axis a reader would otherwise assume the split cannot touch, so it is the one worth naming; the path floor's reason is already the subject of the concentration sentence directly below. - **Negative-claim falsification (self-review clause e): not triggered.** The diff adds no capability denial — it *removes* a false-negative suppression, so the user-facing outcome strictly grants a readout that was previously withheld. No `not supported` / `unavailable` / `STOP` strings, no throw/deny path, and no test was flipped to assert a dead-end (the new SILENT assertions pin behavior that is green on both the old and new script, as the red-before-green run above shows). ## Provenance Ticket BE-7419, deferred from #162 review thread `r3774678317`; origin spike BE-7414. Depends on #162 (merged, `14ea7d5`) for `$path_decided` / `$drivers` / the clause, and on BE-7418 / #163 (merged, `c67a2a4`) for `axes.reversibility.files`. Both are on `main`, so this is a plain PR against `main`, not a stacked one. --- <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