Comfy-Org / Comfy-Org/github-workflows
[groom] grade-targets.sh is a script and a library at once, and the seam leaks into collect-pr-inputs.sh
- Dominant language
- Shell
- Stars
- 6
- Forks
- 1
- Avg merge
- 14h 4m
- Merged PRs (30d)
- 98
Description
**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.
Contributor guide
Assessment
This issue has not been assessed yet.