[PR #166] [MERGED] feat(pr-derisk): `/derisk` split plan with grader-computed floors (BE-7380) #175

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

Base: main ← Head: matt/be-7380-pr-derisk

📝 Commits (3)

5592eb3 feat(pr-derisk): /derisk split plan with grader-computed floors (BE-7380)
59e438b fix(pr-derisk): compare against the PATH floor, anchor the commenter gate, bound the model phase (BE-7380)
444fb6c fix(pr-derisk): drop grep -q from the suite's has/no helpers — it raced pipefail (BE-7380)

📊 Changes

15 files changed (+2547 additions, -10 deletions)

View changed files

📝 .github/bump-callers/README.md (+3 -2)
➕ .github/workflows/bump-pr-derisk-callers.yml (+268 -0)
➕ .github/workflows/pr-derisk.yml (+650 -0)
➕ .github/workflows/test-pr-derisk.yml (+64 -0)
📝 AGENTS.md (+21 -5)
📝 README.md (+1 -0)
📝 docs/callers/README.md (+2 -0)
➕ docs/callers/pr-derisk.md (+104 -0)
➕ scripts/pr-derisk/README.md (+117 -0)
➕ scripts/pr-derisk/collect-pr-inputs.sh (+152 -0)
➕ scripts/pr-derisk/plan-derisk.sh (+475 -0)
➕ scripts/pr-derisk/publish-derisk-comment.sh (+284 -0)
➕ scripts/pr-derisk/resolve-enabled.sh (+79 -0)
➕ scripts/pr-derisk/tests/test_plan_derisk.sh (+305 -0)
📝 scripts/pr-risk/README.md (+22 -3)

📄 Description

ELI-5

The risk grader already tells you how risky a PR is and which files hold the grade up. It does not tell you what to do about it. This adds the next rung: you comment /derisk on a pull request, and a bot replies with a concrete plan for splitting it into a chain of smaller PRs — one small one that carries all the risk, and the rest in cheap lanes. A model proposes which files go together; it is never allowed to say how risky anything is. Every risk tier in that comment is recomputed by the actual grader from the files the model assigned, so a model that hallucinates "this lands R0" cannot put that number in front of a reviewer. It is off everywhere by default, nothing is gated or filed, and the comment is the entire product.

What this adds

.github/workflows/pr-derisk.yml — new reusable workflow, issue_comment-triggered. A gate job (no token scopes) resolves the enablement switch and the commenter gate; a plan job re-grades the PR, makes ONE model call, computes the split floors, and posts one sticky comment. enabled: defaults to false, and vars.DERISK_CONFIG overrides it in both directions (kill switch with no PR), matching the pr-risk convention.
scripts/pr-derisk/ — collect-pr-inputs.sh (re-grade + fetch the diff, capped), plan-derisk.sh (the single model call, partition validation, floor computation), publish-derisk-comment.sh (the sticky renderer), resolve-enabled.sh, plus a README and a 42-assertion hermetic suite with the model layer stubbed via MODEL_RESPONSE_FILE — no network, no API key.
.github/workflows/bump-pr-derisk-callers.yml — the fleet entrypoint over the shared bump-callers.sh. It watches scripts/pr-risk/** as well as its own tree, because a /derisk run executes the pr-risk grader at the same pinned ref.
.github/workflows/test-pr-derisk.yml, docs/callers/pr-derisk.md, and the README / AGENTS.md / caller-index / bump-callers-README / pr-risk-README rows that keep the catalog in sync.

The invariant this is built around

grade = worst(path_floor, provenance, reversibility) is deterministic, and grade-pr-risk.sh --stdin grades synthetic records with no network. So each proposed step's path floor is computed by the actual judge over a synthetic scorecard record built from that step's real files, change types and line counts. The merge step rebuilds each step object field-by-field from a fixed list, so any tier/floor/risk key the model emitted despite the instruction is dropped rather than merged. The headline verdict is arithmetic over those grader floors, never over the model's prose — which is what makes the single-class-monolith honesty rule ("N smaller single-concern R3s, same lane") a property of the renderer rather than a request in a prompt.

Only axes.path_floor.tier is read back, deliberately: the other two axes of a synthetic record would be values this script invented, and printing an invented number beside a computed one is exactly the model-claimed floor the design exists to prevent. The comment says so, and names provenance and the future check rollup as the unknowns — the same wording discipline the v0 readout (BE-7378) landed with.

Security posture

issue_comment workflows run from the caller's default branch, so a PR cannot edit the workflow that serves it. On top of that: the commenter is gated on author_association (default OWNER,MEMBER,COLLABORATOR — a property the API attributes, not a string the commenter writes); no PR code is ever checked out (the diff is read over the API and handed to a model as text); the workflows_ref shape+ancestry guard is byte-identical to pr-risk.yml's and a test fails the build if the two drift; the only write in the whole run is one PR comment. Untrusted diff in, advisory comment out — and every PR-controlled string, the model's own output included, is markdown-escaped by the renderer (a filename can otherwise forge a table row).

Defects fixed in review of the draft

Three things I found reviewing my own working tree before committing, each worth calling out because a reader would otherwise take the comment at its word:

WATCHED_ASSETS listed only scripts/pr-derisk while the paths: filter watched scripts/pr-risk too — test_paths_contract.sh failed on it. Both trees are now listed, so deleting either pathspec line is a build failure rather than a silent narrowing.
WATCHED_EXEC listed apply-risk-label.sh and publish-risk-surfaces.sh, justified by a comment claiming grade-targets.sh hard-dies without them at source time. It does not — those checks live inside main(), which its BASH_SOURCE == $0 footer runs only on a direct invocation, and collect-pr-inputs.sh only sources it for two helpers. Left in, a pr-risk-side decommission of a file no /derisk run touches would have frozen this fleet behind a false ::warning::. Both entries removed and the comment corrected.
collect-pr-inputs.sh defined log/warn/die and then sourced grade-targets.sh, which redefines log/die at file scope — so every diagnostic after that point went out under a [grade-targets] prefix naming a script that did not emit it. They are re-declared after the source.

Verification

shellcheck -x scripts/pr-derisk/*.sh scripts/pr-derisk/tests/*.sh clean; bash scripts/pr-derisk/tests/test_plan_derisk.sh → 42 passed, 0 failed (covers the floor computation end-to-end through the real grader, the exact-coverage rejection + single re-prompt, the deterministic fallbacks, the single-class-monolith verdict, markdown-forgery resistance via a crafted filename and step name, the comment-length bound, the sticky marker, and the enablement switch in all five degrade directions). bash .github/bump-callers/tests/test_paths_contract.sh → 56/0; test_preflight.sh, scripts/pr-risk/tests/test_pin_contract.sh and check_workflow_pins.py all pass; all three new workflow files parse as YAML.

Judgment calls, and what I could NOT exercise

The ANTHROPIC_API_KEY secret's visibility is unverified. The ticket's decision 1 explicitly asks to "confirm at implementation time that the secret's visibility covers Comfy-Org/github-workflows and any repo whose caller runs the model step". I have no access to org secret settings from this environment, so I did not confirm it. It is the one named artifact in the ticket I could not exercise, and it is why this says Refs, not Closes — the enrolment half of the ticket is untested until someone with org-settings access checks it.
No live /derisk run happened. There is no enrolled caller (by design — decision 3 pins ship-off-everywhere), so the end-to-end path has only been exercised hermetically with the model stubbed. The first real run is the pilot flip, not this PR.
PR_DERISK_CALLERS is not set, and ALLOW_EMPTY: "true" is deliberate for now. With no enrolled callers, an empty roster is the accurate state and a hard-fail would redden main on every commit to this rung. The workflow header says in as many words that this flips to "false" in the same change as the first enrolment — left at "true" past that point it degrades into exactly the silent pin drift the fleet exists to prevent.
Comment identity is the plain GITHUB_TOKEN, per the ticket's pinned default (no workflow cascade), and enabled ships off everywhere per decision 3. Neither is revisited here.
AGENTS.md is 245 lines, over its own 200-line ceiling. It was already 229 on main and this adds 16; the integrity checker is not enrolled as a caller on this repo, so nothing gates it. Trimming it back under 200 is a rewrite of sections this change does not touch, so I left it — flagging rather than quietly widening it.
The plan job's model step holds the API key and no GitHub token; the publish step holds the token and no key. That separation is intentional and worth keeping if this file is edited later.

Refs BE-7380

_{🔄 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/166 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/14/2026 **Status:** ✅ Merged **Merged:** 8/14/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-7380-pr-derisk` --- ### 📝 Commits (3) - [`5592eb3`](https://github.com/Comfy-Org/github-workflows/commit/5592eb3d3d65e0ae63e8d076984d194cde17ad82) feat(pr-derisk): `/derisk` split plan with grader-computed floors (BE-7380) - [`59e438b`](https://github.com/Comfy-Org/github-workflows/commit/59e438b7ce98b3e7a8e0f0044e53e439ba2acc3f) fix(pr-derisk): compare against the PATH floor, anchor the commenter gate, bound the model phase (BE-7380) - [`444fb6c`](https://github.com/Comfy-Org/github-workflows/commit/444fb6c26dba06862577f52654e678b714a9dc85) fix(pr-derisk): drop `grep -q` from the suite's has/no helpers — it raced pipefail (BE-7380) ### 📊 Changes **15 files changed** (+2547 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+3 -2) ➕ `.github/workflows/bump-pr-derisk-callers.yml` (+268 -0) ➕ `.github/workflows/pr-derisk.yml` (+650 -0) ➕ `.github/workflows/test-pr-derisk.yml` (+64 -0) 📝 `AGENTS.md` (+21 -5) 📝 `README.md` (+1 -0) 📝 `docs/callers/README.md` (+2 -0) ➕ `docs/callers/pr-derisk.md` (+104 -0) ➕ `scripts/pr-derisk/README.md` (+117 -0) ➕ `scripts/pr-derisk/collect-pr-inputs.sh` (+152 -0) ➕ `scripts/pr-derisk/plan-derisk.sh` (+475 -0) ➕ `scripts/pr-derisk/publish-derisk-comment.sh` (+284 -0) ➕ `scripts/pr-derisk/resolve-enabled.sh` (+79 -0) ➕ `scripts/pr-derisk/tests/test_plan_derisk.sh` (+305 -0) 📝 `scripts/pr-risk/README.md` (+22 -3) </details> ### 📄 Description ## ELI-5 The risk grader already tells you *how* risky a PR is and *which files* hold the grade up. It does not tell you what to do about it. This adds the next rung: you comment `/derisk` on a pull request, and a bot replies with a concrete plan for splitting it into a chain of smaller PRs — one small one that carries all the risk, and the rest in cheap lanes. A model proposes *which files go together*; it is never allowed to say how risky anything is. Every risk tier in that comment is recomputed by the actual grader from the files the model assigned, so a model that hallucinates "this lands R0" cannot put that number in front of a reviewer. It is off everywhere by default, nothing is gated or filed, and the comment is the entire product. ## What this adds - **`.github/workflows/pr-derisk.yml`** — new reusable workflow, `issue_comment`-triggered. A `gate` job (no token scopes) resolves the enablement switch and the commenter gate; a `plan` job re-grades the PR, makes ONE model call, computes the split floors, and posts one sticky comment. `enabled:` defaults to **false**, and `vars.DERISK_CONFIG` overrides it in both directions (kill switch with no PR), matching the pr-risk convention. - **`scripts/pr-derisk/`** — `collect-pr-inputs.sh` (re-grade + fetch the diff, capped), `plan-derisk.sh` (the single model call, partition validation, floor computation), `publish-derisk-comment.sh` (the sticky renderer), `resolve-enabled.sh`, plus a README and a 42-assertion hermetic suite with the model layer stubbed via `MODEL_RESPONSE_FILE` — no network, no API key. - **`.github/workflows/bump-pr-derisk-callers.yml`** — the fleet entrypoint over the shared `bump-callers.sh`. It watches `scripts/pr-risk/**` as well as its own tree, because a `/derisk` run executes the pr-risk grader at the same pinned ref. - **`.github/workflows/test-pr-derisk.yml`**, `docs/callers/pr-derisk.md`, and the README / AGENTS.md / caller-index / bump-callers-README / pr-risk-README rows that keep the catalog in sync. ## The invariant this is built around `grade = worst(path_floor, provenance, reversibility)` is deterministic, and `grade-pr-risk.sh --stdin` grades synthetic records with no network. So each proposed step's path floor is computed by the *actual judge* over a synthetic scorecard record built from that step's real files, change types and line counts. The merge step rebuilds each step object field-by-field from a fixed list, so any `tier`/`floor`/`risk` key the model emitted despite the instruction is dropped rather than merged. The headline verdict is arithmetic over those grader floors, never over the model's prose — which is what makes the single-class-monolith honesty rule ("N smaller single-concern R3s, same lane") a property of the renderer rather than a request in a prompt. Only `axes.path_floor.tier` is read back, deliberately: the other two axes of a synthetic record would be values this script invented, and printing an invented number beside a computed one is exactly the model-claimed floor the design exists to prevent. The comment says so, and names provenance and the future check rollup as the unknowns — the same wording discipline the v0 readout (BE-7378) landed with. ## Security posture `issue_comment` workflows run from the caller's **default branch**, so a PR cannot edit the workflow that serves it. On top of that: the commenter is gated on `author_association` (default `OWNER,MEMBER,COLLABORATOR` — a property the API attributes, not a string the commenter writes); **no PR code is ever checked out** (the diff is read over the API and handed to a model as text); the `workflows_ref` shape+ancestry guard is byte-identical to `pr-risk.yml`'s and a test fails the build if the two drift; the only write in the whole run is one PR comment. Untrusted diff in, advisory comment out — and every PR-controlled string, the model's own output included, is markdown-escaped by the renderer (a filename can otherwise forge a table row). ## Defects fixed in review of the draft Three things I found reviewing my own working tree before committing, each worth calling out because a reader would otherwise take the comment at its word: 1. `WATCHED_ASSETS` listed only `scripts/pr-derisk` while the `paths:` filter watched `scripts/pr-risk` too — `test_paths_contract.sh` failed on it. Both trees are now listed, so deleting either pathspec line is a build failure rather than a silent narrowing. 2. `WATCHED_EXEC` listed `apply-risk-label.sh` and `publish-risk-surfaces.sh`, justified by a comment claiming `grade-targets.sh` hard-`die`s without them at source time. It does not — those checks live inside `main()`, which its `BASH_SOURCE == $0` footer runs only on a direct invocation, and `collect-pr-inputs.sh` only sources it for two helpers. Left in, a pr-risk-side decommission of a file no `/derisk` run touches would have frozen this fleet behind a false `::warning::`. Both entries removed and the comment corrected. 3. `collect-pr-inputs.sh` defined `log`/`warn`/`die` and then sourced `grade-targets.sh`, which redefines `log`/`die` at file scope — so every diagnostic after that point went out under a `[grade-targets]` prefix naming a script that did not emit it. They are re-declared after the source. ## Verification `shellcheck -x scripts/pr-derisk/*.sh scripts/pr-derisk/tests/*.sh` clean; `bash scripts/pr-derisk/tests/test_plan_derisk.sh` → **42 passed, 0 failed** (covers the floor computation end-to-end through the real grader, the exact-coverage rejection + single re-prompt, the deterministic fallbacks, the single-class-monolith verdict, markdown-forgery resistance via a crafted filename and step name, the comment-length bound, the sticky marker, and the enablement switch in all five degrade directions). `bash .github/bump-callers/tests/test_paths_contract.sh` → 56/0; `test_preflight.sh`, `scripts/pr-risk/tests/test_pin_contract.sh` and `check_workflow_pins.py` all pass; all three new workflow files parse as YAML. ## Judgment calls, and what I could NOT exercise - **The `ANTHROPIC_API_KEY` secret's visibility is unverified.** The ticket's decision 1 explicitly asks to "confirm at implementation time that the secret's visibility covers Comfy-Org/github-workflows and any repo whose caller runs the model step". I have no access to org secret settings from this environment, so I did not confirm it. It is the one named artifact in the ticket I could not exercise, and it is why this says `Refs`, not `Closes` — the enrolment half of the ticket is untested until someone with org-settings access checks it. - **No live `/derisk` run happened.** There is no enrolled caller (by design — decision 3 pins ship-off-everywhere), so the end-to-end path has only been exercised hermetically with the model stubbed. The first real run is the pilot flip, not this PR. - **`PR_DERISK_CALLERS` is not set, and `ALLOW_EMPTY: "true"` is deliberate for now.** With no enrolled callers, an empty roster is the accurate state and a hard-fail would redden `main` on every commit to this rung. The workflow header says in as many words that this flips to `"false"` in the same change as the first enrolment — left at `"true"` past that point it degrades into exactly the silent pin drift the fleet exists to prevent. - **Comment identity is the plain `GITHUB_TOKEN`**, per the ticket's pinned default (no workflow cascade), and **`enabled` ships off everywhere** per decision 3. Neither is revisited here. - **`AGENTS.md` is 245 lines, over its own 200-line ceiling.** It was already 229 on `main` and this adds 16; the integrity checker is not enrolled as a caller on this repo, so nothing gates it. Trimming it back under 200 is a rewrite of sections this change does not touch, so I left it — flagging rather than quietly widening it. - The `plan` job's model step holds the API key and no GitHub token; the publish step holds the token and no key. That separation is intentional and worth keeping if this file is edited later. Refs BE-7380 --- <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