[GH-ISSUE #245] [groom] grade-targets.sh is a script and a library at once, and the seam leaks into collect-pr-inputs.sh #4

New issue

Closed

opened 2026-10-26 22:34:25 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-26 22:34:25 +00:00

Owner

Originally created by @cloud-code-bot[bot] on GitHub (Aug 28, 2026).
Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/245

Groom auto-builder — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, or build/test config that executes in pre-review CI) — a human must author these changes), so it is filed for a human. · run

Verified facts

scripts/pr-derisk/collect-pr-inputs.sh:77 sources the 619-line scripts/pr-risk/grade-targets.sh for exactly three helpers - resolve_base_ref, fetch_override, retry_read - as its own comment at :72-76 states. Making that safe cost two workarounds, both properties of the file boundary rather than of the logic:

scripts/pr-risk/grade-targets.sh:114-132 - a GT_DIRECT flag plus lazy init_scratch, added (per its own comment) because at file scope the EXIT trap replaced the SOURCING shell's trap, so a suite that sourced these helpers silently lost its sandbox cleanup and leaked both the sandbox and the temp files.
scripts/pr-derisk/collect-pr-inputs.sh:50-63 and :78-86 - log/warn/die defined TWICE, before and after the source, because grade-targets.sh:111-112 defines its own log/die at file scope and captures them. The first copy carries ten lines of comment plus a shellcheck disable for SC2317 and SC2329 to keep two linter versions quiet.

Both files are already WATCHED_EXEC entries of their fleets (bump-pr-risk-callers.yml:181, bump-pr-derisk-callers.yml:219), and bump-pr-derisk-callers.yml:195-203 already documents the sourcing relationship, so the coupling is known and load-bearing rather than accidental.

Fix

Move the sourceable helpers - enc, enc_path, init_scratch/gherr, retryable_err, retry_read, resolve_base_ref, fetch_override (grade-targets.sh:125-289) - into a new scripts/pr-risk/lib.sh with NO top-level side effects and PREFIXED diagnostic names (e.g. gt_log / gt_die) so they cannot capture a caller's log/die. The precedent already exists here: scripts/area-label/lib.sh. Then:

grade-targets.sh sources it and keeps main() and the per-target logic. With nothing else sourcing it, GT_DIRECT and its lazy-trap dance can go and the trap can be installed unconditionally.
collect-pr-inputs.sh sources lib.sh instead; the duplicated log/warn/die block and both shellcheck suppressions disappear.

Behaviour must be identical. scripts/pr-risk/tests/test_grade_targets.sh and scripts/pr-derisk/tests/test_plan_derisk.sh must stay green, and the shellcheck -x list in .github/workflows/test-pr-risk.yml:51 must gain the new file.

Risk

Medium and concrete: scripts/pr-risk/lib.sh becomes a run-time dependency of BOTH fleets, so WATCHED_EXEC in bump-pr-risk-callers.yml AND bump-pr-derisk-callers.yml must gain it in the same commit - a missing entry could re-point pinned callers at a SHA where the file is absent, which is exactly what those lists exist to prevent. Scope the move to the helpers listed above; do not opportunistically move parse_targets, settle_grade, process_target or main.

Steelman

The current arrangement works, is documented in painful detail and is pinned by tests; a lib.sh adds a third file to a two-file relationship and one more WATCHED_EXEC entry a caller breaks without. If the sourcing were a one-off it would not be worth touching. The counter is that the file is now maintained under two contradictory contracts at once, and every future helper added to it inherits both hazards - the trap one already bit.

Originally created by @cloud-code-bot[bot] on GitHub (Aug 28, 2026). Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/245 **Groom auto-builder** — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, or build/test config that executes in pre-review CI) — a human must author these changes), so it is filed for a human. · [run](https://github.com/Comfy-Org/github-workflows/actions/runs/33174435147) ## Verified facts `scripts/pr-derisk/collect-pr-inputs.sh:77` sources the 619-line `scripts/pr-risk/grade-targets.sh` for exactly three helpers - `resolve_base_ref`, `fetch_override`, `retry_read` - as its own comment at `:72-76` states. Making that safe cost two workarounds, both properties of the file boundary rather than of the logic: 1. `scripts/pr-risk/grade-targets.sh:114-132` - a GT_DIRECT flag plus lazy `init_scratch`, added (per its own comment) because at file scope the EXIT trap replaced the SOURCING shell's trap, so a suite that sourced these helpers silently lost its sandbox cleanup and leaked both the sandbox and the temp files. 2. `scripts/pr-derisk/collect-pr-inputs.sh:50-63` and `:78-86` - `log`/`warn`/`die` defined TWICE, before and after the source, because `grade-targets.sh:111-112` defines its own `log`/`die` at file scope and captures them. The first copy carries ten lines of comment plus a shellcheck disable for SC2317 and SC2329 to keep two linter versions quiet. Both files are already WATCHED_EXEC entries of their fleets (`bump-pr-risk-callers.yml:181`, `bump-pr-derisk-callers.yml:219`), and `bump-pr-derisk-callers.yml:195-203` already documents the sourcing relationship, so the coupling is known and load-bearing rather than accidental. ## Fix Move the sourceable helpers - `enc`, `enc_path`, `init_scratch`/`gherr`, `retryable_err`, `retry_read`, `resolve_base_ref`, `fetch_override` (`grade-targets.sh:125-289`) - into a new `scripts/pr-risk/lib.sh` with NO top-level side effects and PREFIXED diagnostic names (e.g. gt_log / gt_die) so they cannot capture a caller's `log`/`die`. The precedent already exists here: `scripts/area-label/lib.sh`. Then: - `grade-targets.sh` sources it and keeps `main()` and the per-target logic. With nothing else sourcing it, GT_DIRECT and its lazy-trap dance can go and the trap can be installed unconditionally. - `collect-pr-inputs.sh` sources `lib.sh` instead; the duplicated log/warn/die block and both shellcheck suppressions disappear. Behaviour must be identical. `scripts/pr-risk/tests/test_grade_targets.sh` and `scripts/pr-derisk/tests/test_plan_derisk.sh` must stay green, and the `shellcheck -x` list in `.github/workflows/test-pr-risk.yml:51` must gain the new file. ## Risk Medium and concrete: `scripts/pr-risk/lib.sh` becomes a run-time dependency of BOTH fleets, so WATCHED_EXEC in `bump-pr-risk-callers.yml` AND `bump-pr-derisk-callers.yml` must gain it in the same commit - a missing entry could re-point pinned callers at a SHA where the file is absent, which is exactly what those lists exist to prevent. Scope the move to the helpers listed above; do not opportunistically move `parse_targets`, `settle_grade`, `process_target` or `main`. ## Steelman The current arrangement works, is documented in painful detail and is pinned by tests; a lib.sh adds a third file to a two-file relationship and one more WATCHED_EXEC entry a caller breaks without. If the sourcing were a one-off it would not be worth touching. The counter is that the file is now maintained under two contradictory contracts at once, and every future helper added to it inherits both hazards - the trap one already bit.

bsakaguchi

2026-10-26 22:34:25 +00:00

closed this issue
added the
groom
label

bsakaguchi commented

2026-10-26 22:34:27 +00:00

Author

Owner

@mattmillerai commented on GitHub (Sep 18, 2026):

Triage: every claim still holds. Leaving this open, tracked internally.

Verified against origin/main @ cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b. Most of the line numbers are still exact — these files have not moved.

collect-pr-inputs.sh:77 still sources grade-targets.sh, with the comment at :72-76 naming the three helpers. (Two are called directly — resolve_base_ref at :93, fetch_override at :95/:97; retry_read is a transitive dependency of both. "Exactly three helpers" is the transitive set, not the call set.) grade-targets.sh is still 619 lines.
Workaround 1 is intact at grade-targets.sh:114-132 — the comment block, GT_DIRECT, the lazy init_scratch and the conditional trap install at :130.
Workaround 2 is intact at collect-pr-inputs.sh:50-63 and :78-86, with both SC2317 and SC2329 suppressions still present, on two separate lines.
scripts/pr-risk/lib.sh does not exist; the only lib.sh in these trees is the scripts/area-label/lib.sh precedent you cite, whose header states exactly the pattern proposed here.
The shellcheck -x list is still at .github/workflows/test-pr-risk.yml:51 and has no lib.sh.
No commit has touched either file since filing (the only one in these paths is 1567edf, human risk overrides, which touched neither), and no open PR overlaps — all nine open PRs were checked.

Two line numbers have shifted: the WATCHED_EXEC entries are now bump-pr-risk-callers.yml:186 (body says :181) and bump-pr-derisk-callers.yml:221/:224 (body says :219); the documenting comment cited at :195-203 is now at :197-213.

One constraint the body does not mention, and it is the biggest one on this refactor. scripts/pr-risk/tests/test_grade_targets.sh:405-413 is "phase 21: sourcing the script has NO side effects" — it spawns a subshell with its own EXIT trap, sources the file, and asserts both that the trap survived and that zero scratch files were created. It targets grade-targets.sh itself, not a future lib.sh. So after the extraction grade-targets.sh must still pass it: lib.sh has to be genuinely side-effect-free, and if GT_DIRECT goes, the direct-invocation cleanup has to move into main() rather than back to file scope. Worth reasoning about explicitly before writing the diff.

A scoping detail: parse_targets (:198-215) sits inside the :125-289 range but is not in your extraction list, and it calls die/log — so the cut is not contiguous and needs an explicit skip.

And a gap worth confirming: collect-pr-inputs.sh is not in the test-pr-risk.yml shellcheck list at all (it lives under scripts/pr-derisk). Check the pr-derisk side has its own lint job before assuming those two suppressions are load-bearing for a job that actually runs.

Your risk note is right and I confirmed the mechanic behind it: .github/bump-callers/tests/test_preflight.sh has decommission-warning tests keyed on grade-targets.sh (:1097, :1106, :1146, :1157), so "absent file → ::warning:: → fleet no-ops" is real and tested. Both fleets' WATCHED_EXEC must gain lib.sh in the same commit.

Reopen or comment if anything here reads wrong.

Created by Claude Code

@mattmillerai commented on GitHub (Sep 18, 2026): **Triage: every claim still holds. Leaving this open, tracked internally.** Verified against `origin/main` @ `cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b`. Most of the line numbers are still exact — these files have not moved. - `collect-pr-inputs.sh:77` still sources `grade-targets.sh`, with the comment at `:72-76` naming the three helpers. (Two are called directly — `resolve_base_ref` at `:93`, `fetch_override` at `:95`/`:97`; `retry_read` is a transitive dependency of both. "Exactly three helpers" is the transitive set, not the call set.) `grade-targets.sh` is still 619 lines. - Workaround 1 is intact at `grade-targets.sh:114-132` — the comment block, `GT_DIRECT`, the lazy `init_scratch` and the conditional trap install at `:130`. - Workaround 2 is intact at `collect-pr-inputs.sh:50-63` and `:78-86`, with both `SC2317` and `SC2329` suppressions still present, on two separate lines. - `scripts/pr-risk/lib.sh` **does not exist**; the only `lib.sh` in these trees is the `scripts/area-label/lib.sh` precedent you cite, whose header states exactly the pattern proposed here. - The `shellcheck -x` list is still at `.github/workflows/test-pr-risk.yml:51` and has no `lib.sh`. - No commit has touched either file since filing (the only one in these paths is `1567edf`, human risk overrides, which touched neither), and **no open PR overlaps** — all nine open PRs were checked. **Two line numbers have shifted:** the WATCHED_EXEC entries are now `bump-pr-risk-callers.yml:186` (body says `:181`) and `bump-pr-derisk-callers.yml:221`/`:224` (body says `:219`); the documenting comment cited at `:195-203` is now at `:197-213`. **One constraint the body does not mention, and it is the biggest one on this refactor.** `scripts/pr-risk/tests/test_grade_targets.sh:405-413` is "phase 21: sourcing the script has NO side effects" — it spawns a subshell with its own EXIT trap, sources the file, and asserts both that the trap survived and that zero scratch files were created. It targets `grade-targets.sh` itself, **not** a future `lib.sh`. So after the extraction `grade-targets.sh` must still pass it: `lib.sh` has to be genuinely side-effect-free, and if `GT_DIRECT` goes, the direct-invocation cleanup has to move into `main()` rather than back to file scope. Worth reasoning about explicitly before writing the diff. **A scoping detail:** `parse_targets` (`:198-215`) sits inside the `:125-289` range but is not in your extraction list, and it calls `die`/`log` — so the cut is not contiguous and needs an explicit skip. **And a gap worth confirming:** `collect-pr-inputs.sh` is not in the `test-pr-risk.yml` shellcheck list at all (it lives under `scripts/pr-derisk`). Check the pr-derisk side has its own lint job before assuming those two suppressions are load-bearing for a job that actually runs. Your risk note is right and I confirmed the mechanic behind it: `.github/bump-callers/tests/test_preflight.sh` has decommission-warning tests keyed on `grade-targets.sh` (`:1097`, `:1106`, `:1146`, `:1157`), so "absent file → `::warning::` → fleet no-ops" is real and tested. Both fleets' `WATCHED_EXEC` must gain `lib.sh` in the same commit. Reopen or comment if anything here reads wrong. *Created by Claude Code*

bsakaguchi referenced this issue

2026-10-26 22:35:25 +00:00

[PR #4] [MERGED] feat: add assign-prs-to-author reusable workflow #15