lib.sh #305

New issue

Closed

opened 2026-10-26 22:38:15 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:15 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/304
Author: @mattmillerai
Created: 9/18/2026
Status: ✅ Merged
Merged: 9/18/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15257-pr-risk-lib-sh

📝 Commits (2)

cc1ce8f refactor(pr-risk): extract the sourceable helpers into scripts/pr-risk/lib.sh
967b297 fix(pr-risk): own the scratch trio in the parent, anchor lib.sh to the checkout, refuse a path that leaves contents/

📊 Changes

16 files changed (+598 additions, -213 deletions)

View changed files

📝 .github/workflows/bump-pr-derisk-callers.yml (+15 -12)
📝 .github/workflows/bump-pr-risk-callers.yml (+4 -0)
📝 .github/workflows/pr-derisk.yml (+7 -2)
📝 .github/workflows/pr-risk.yml (+7 -2)
📝 .github/workflows/test-pr-derisk.yml (+11 -1)
📝 .github/workflows/test-pr-risk.yml (+1 -1)
📝 docs/callers/pr-derisk.md (+1 -1)
📝 docs/callers/pr-risk.md (+1 -1)
📝 scripts/pr-derisk/README.md (+18 -7)
📝 scripts/pr-derisk/collect-pr-inputs.sh (+61 -33)
📝 scripts/pr-derisk/plan-derisk.sh (+1 -1)
➕ scripts/pr-derisk/tests/test_collect_pr_inputs.sh (+161 -0)
📝 scripts/pr-risk/README.md (+13 -0)
📝 scripts/pr-risk/grade-targets.sh (+32 -152)
➕ scripts/pr-risk/lib.sh (+217 -0)
📝 scripts/pr-risk/tests/test_grade_targets.sh (+48 -0)

📄 Description

ELI-5

scripts/pr-risk/grade-targets.sh was two things at once: a script you run, and a library another script borrows from. pr-derisk's collect-pr-inputs.sh sourced all 619 lines of it just to reuse three helpers. That dual contract cost two workarounds — a GT_DIRECT flag with a lazily-installed EXIT trap (because installing the trap at file scope replaced the sourcing shell's own trap), and a second copy of log/warn/die in collect-pr-inputs.sh carrying two shellcheck suppressions (because grade-targets.sh defined its own at file scope and captured them). Neither is a property of the logic; both are properties of the file boundary. This PR moves the shared helpers into scripts/pr-risk/lib.sh and both workarounds go with the boundary. Nothing about grading, target parsing or the derisk plan changes.

What moved

Into the new scripts/pr-risk/lib.sh, verbatim: enc, enc_path, init_scratch, gherr, retryable_err, retry_read, resolve_base_ref, fetch_override, plus the config those helpers read (REPO, READ_RETRY_TRIES, READ_RETRY_DELAY_SECONDS, JOB_DEADLINE). parse_targets, settle_grade, process_target and main stay in grade-targets.sh — the cut deliberately skips parse_targets, which sits inside the same line range but is not part of the shared set.

Its diagnostics are gt_log / gt_die, prefixed so they cannot capture a sourcing script's own log/die. They print under GT_LOG_PREFIX, which defaults to grade-targets and which collect-pr-inputs.sh sets to its own name before sourcing. That variable is not decoration: the old arrangement had collect-pr-inputs.sh re-declare log after the source specifically so a retry line emitted during a /derisk run would not read [grade-targets], naming a script that did not emit it. Keeping the prefix settable is what makes both callers' run-log output byte-identical to before — a plain [grade-targets] in the library was my first cut and it regressed exactly that line, caught by diffing old against new (see Verified).

grade-targets.sh drops GT_DIRECT and installs trap 'rm -f "$ERRF" "$LABELF" "$OUTF"' EXIT unconditionally inside main(), which runs only on a direct invocation. collect-pr-inputs.sh now declares MAP_PATH / RB_PATH / FLEET_LOGINS / BOT_LOGINS itself — they are its own documented inputs and previously arrived only because the file it sourced happened to default them at its file scope, an inheritance nothing stated and nothing tested. Same defaults, so no behaviour change; without it the script would have died on an unbound variable under set -u.

The riskiest line, and why it is safe

. "$SELF_DIR/lib.sh" in grade-targets.sh — SELF_DIR, not TOOL_DIR. TOOL_DIR is an env-overridable pointer at the swappable tools (grade-pr-risk.sh, apply-risk-label.sh, …) and the suite overrides it with a stub grader directory that holds none of this repo's other files; sourcing the library from there would break those phases. lib.sh is this script's own implementation and travels with it. collect-pr-inputs.sh keeps using $TOOL_DIR/lib.sh, matching exactly how it used to reach $TOOL_DIR/grade-targets.sh.

Judgment call: `grade-targets.sh` leaves pr-derisk's `WATCHED_EXEC`

The issue and the task only asked that both bump entrypoints gain scripts/pr-risk/lib.sh. I also removed scripts/pr-risk/grade-targets.sh from bump-pr-derisk-callers.yml's list, because after this change a /derisk run never loads that file at all — collect-pr-inputs.sh sources the library directly and execs the grader. That list's own stated contract is "every file whose ABSENCE breaks a pinned caller at run time", and its own comment warns that an over-wide entry turns a pr-risk-side decommission into a false decommission that freezes this fleet behind a ::warning::. Leaving it would have been exactly the shape that comment already excludes apply-risk-label.sh for.

Empirically checked rather than assumed, since this is a removal: grep -rn "grade-targets\|pr-risk/" .github/workflows/pr-derisk.yml returns nothing, pr-derisk.yml invokes only resolve-enabled.sh, collect-pr-inputs.sh, plan-derisk.sh and publish-derisk-comment.sh out of a full (non-sparse) checkout, and no file under scripts/pr-derisk/ references grade-targets.sh in any executable position after this change. bump-pr-risk-callers.yml still watches it — that is the fleet whose callers run it. Happy to restore the entry if a reviewer would rather keep the list conservative.

Verified

shellcheck -x clean across both trees, with lib.sh added to the list in test-pr-risk.yml:51 (as relative lib.sh) and shellcheck -x .github/bump-callers/*.sh .github/bump-callers/tests/*.sh clean too.
All seven shell suites over the two trees green: test_grade_targets.sh 105/0 (including phase 21 — sourcing leaves the sourcing shell's EXIT trap intact and creates zero scratch files), test_plan_derisk.sh 63/0, test_grade_pr_risk.sh 138/0, test_apply_risk_label.sh 52/0, test_publish_risk_surfaces.sh 94/0, test_pin_contract.sh 28/0, test_resolve_enabled.sh 27/0.
Phase 21 proven non-vacuous against the new file: re-added a trap … EXIT plus an mktemp at lib.sh's file scope and the phase failed (got: trap -- 'echo BOOM' EXIT); reverted and it passes. Sourcing grade-targets.sh sources lib.sh, so that assertion now covers the library transitively.
test_paths_contract.sh 110/0 and test_preflight.sh 445/0 for the two edited bump entrypoints.
check_workflow_pins.py OK (11 workflows, none defaulting workflows_ref), its unit suite 341/0, and check_agents_md.py --root . passed (2 pre-existing warnings, unrelated).
Old-vs-new behavioural diff of collect-pr-inputs.sh, since nothing in the repo covers that file: ran origin/main's copy and this branch's copy under an identical stubbed gh, over both a happy path (base ref resolved from the API, both overrides 404 → generic defaults, diff fetched, tier=R2) and a failure path (a 502 on the runbooks override exercising the retry and the not-a-404-so-do-not-fall-back guard). diff of the two full stdout+stderr transcripts is empty in both cases, including the [collect-pr-inputs] read failed (attempt 1/2) retry line and exit codes. A caller-installed EXIT trap survived the source in both.
A direct grade-targets.sh run under a probe TMPDIR leaves 0 scratch files behind, including on the early-die path — the trap in main() covers every exit.
Read the linked issue Comfy-Org/github-workflows#245 directly (it is public): its helper list, its "no top-level side effects / prefixed diagnostics" requirement and its WATCHED_EXEC risk note all match what shipped.

Provenance

Authored by: agent-work loop
Verified: see the Verified section above — shellcheck clean on both trees; 7 shell suites 509 assertions green (phase 21 included, and proven non-vacuous); test_paths_contract.sh 110/0; test_preflight.sh 445/0; check_workflow_pins.py + 341 unit tests OK; old-vs-new transcript diff of collect-pr-inputs.sh empty on two paths
Deviations: removed scripts/pr-risk/grade-targets.sh from bump-pr-derisk-callers.yml's WATCHED_EXEC in addition to adding lib.sh — see the judgment-call section; every listed acceptance criterion is otherwise met

Residual

collect-pr-inputs.sh still leaks its three mktemp scratch files. init_scratch mints grade-targets-{err,label,out}.XXXXXX under $TMPDIR, and collect-pr-inputs.sh installs no EXIT trap over them — so a /derisk run leaves three temp files behind. This is pre-existing and is unchanged on purpose, because "behaviour is identical — this is a pure extraction" was an explicit acceptance criterion and adding a trap is a behaviour change. It is harmless on an ephemeral Actions runner and a nuisance for anyone driving the script locally. The fix is two lines in collect-pr-inputs.sh (trap 'rm -f "$ERRF" "$LABELF" "$OUTF"' EXIT after the source, guarded for the empty-string case before init_scratch has run), and it belongs in its own change where it can be reviewed as a behaviour change rather than smuggled into a refactor.
.github/bump-callers/README.md's illustrative WATCHED_EXEC block (around :466) is stale and I did not touch it. It shows four entries for the pr-risk fleet while the real bump-pr-risk-callers.yml now carries eight. That drift predates this PR — the real list had already grown past the example — and this change makes it one entry staler. It is documentation of an example rather than a contract nothing checks it against, so no test catches it. Worth a docs pass that either refreshes the excerpt or replaces it with a pointer to the live file.
No tests/test_lib.sh, by design. The ticket put a companion suite out of scope: the two existing suites cover these helpers through their current entrypoints. That remains true, but it is now true of a file that has no direct coverage of its own — test_grade_targets.sh reaches every helper only via grade-targets.sh, and collect-pr-inputs.sh (the other consumer) has no suite in the repo at all.
collect-pr-inputs.sh has no checked-in test. The old-vs-new transcript comparison above is the strongest evidence this PR carries for that file, and it was a throwaway harness in /tmp, not a committed suite. test-pr-derisk.yml shellchecks the file but runs nothing against it. Anyone changing the base-ref or override path through lib.sh in future has the pr-risk suite as a guard and nothing on the pr-derisk side.
Not exercised: the two bump fleets themselves. bump-pr-risk-callers.yml and bump-pr-derisk-callers.yml only run on push to main and mint an org-write token, so their WATCHED_EXEC edits are verified here only by test_paths_contract.sh / test_preflight.sh (which drive synthetic fixtures) and by reading preflight.sh's blob probe — resolve_blob requires a regular-file blob and not an exec bit, so lib.sh at mode 100644 probes present. The first real exercise is the push-to-main run after merge.

Closes #245

_{🔄 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/304 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/18/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15257-pr-risk-lib-sh` --- ### 📝 Commits (2) - [`cc1ce8f`](https://github.com/Comfy-Org/github-workflows/commit/cc1ce8f0d7dc454e3154cb238c58bdec9819eaa5) refactor(pr-risk): extract the sourceable helpers into scripts/pr-risk/lib.sh - [`967b297`](https://github.com/Comfy-Org/github-workflows/commit/967b297a6f5e33dce111e583eb924b682c0dcc72) fix(pr-risk): own the scratch trio in the parent, anchor lib.sh to the checkout, refuse a path that leaves contents/ ### 📊 Changes **16 files changed** (+598 additions, -213 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/bump-pr-derisk-callers.yml` (+15 -12) 📝 `.github/workflows/bump-pr-risk-callers.yml` (+4 -0) 📝 `.github/workflows/pr-derisk.yml` (+7 -2) 📝 `.github/workflows/pr-risk.yml` (+7 -2) 📝 `.github/workflows/test-pr-derisk.yml` (+11 -1) 📝 `.github/workflows/test-pr-risk.yml` (+1 -1) 📝 `docs/callers/pr-derisk.md` (+1 -1) 📝 `docs/callers/pr-risk.md` (+1 -1) 📝 `scripts/pr-derisk/README.md` (+18 -7) 📝 `scripts/pr-derisk/collect-pr-inputs.sh` (+61 -33) 📝 `scripts/pr-derisk/plan-derisk.sh` (+1 -1) ➕ `scripts/pr-derisk/tests/test_collect_pr_inputs.sh` (+161 -0) 📝 `scripts/pr-risk/README.md` (+13 -0) 📝 `scripts/pr-risk/grade-targets.sh` (+32 -152) ➕ `scripts/pr-risk/lib.sh` (+217 -0) 📝 `scripts/pr-risk/tests/test_grade_targets.sh` (+48 -0) </details> ### 📄 Description ## ELI-5 `scripts/pr-risk/grade-targets.sh` was two things at once: a script you run, and a library another script borrows from. pr-derisk's `collect-pr-inputs.sh` sourced all 619 lines of it just to reuse three helpers. That dual contract cost two workarounds — a `GT_DIRECT` flag with a lazily-installed EXIT trap (because installing the trap at file scope *replaced* the sourcing shell's own trap), and a second copy of `log`/`warn`/`die` in `collect-pr-inputs.sh` carrying two shellcheck suppressions (because `grade-targets.sh` defined its own at file scope and captured them). Neither is a property of the logic; both are properties of the file boundary. This PR moves the shared helpers into `scripts/pr-risk/lib.sh` and both workarounds go with the boundary. Nothing about grading, target parsing or the derisk plan changes. ## What moved Into the new `scripts/pr-risk/lib.sh`, verbatim: `enc`, `enc_path`, `init_scratch`, `gherr`, `retryable_err`, `retry_read`, `resolve_base_ref`, `fetch_override`, plus the config those helpers read (`REPO`, `READ_RETRY_TRIES`, `READ_RETRY_DELAY_SECONDS`, `JOB_DEADLINE`). `parse_targets`, `settle_grade`, `process_target` and `main` stay in `grade-targets.sh` — the cut deliberately skips `parse_targets`, which sits inside the same line range but is not part of the shared set. Its diagnostics are `gt_log` / `gt_die`, prefixed so they cannot capture a sourcing script's own `log`/`die`. They print under `GT_LOG_PREFIX`, which defaults to `grade-targets` and which `collect-pr-inputs.sh` sets to its own name before sourcing. That variable is not decoration: the old arrangement had `collect-pr-inputs.sh` re-declare `log` *after* the source specifically so a retry line emitted during a `/derisk` run would not read `[grade-targets]`, naming a script that did not emit it. Keeping the prefix settable is what makes both callers' run-log output byte-identical to before — a plain `[grade-targets]` in the library was my first cut and it regressed exactly that line, caught by diffing old against new (see Verified). `grade-targets.sh` drops `GT_DIRECT` and installs `trap 'rm -f "$ERRF" "$LABELF" "$OUTF"' EXIT` unconditionally *inside* `main()`, which runs only on a direct invocation. `collect-pr-inputs.sh` now declares `MAP_PATH` / `RB_PATH` / `FLEET_LOGINS` / `BOT_LOGINS` itself — they are its own documented inputs and previously arrived only because the file it sourced happened to default them at *its* file scope, an inheritance nothing stated and nothing tested. Same defaults, so no behaviour change; without it the script would have died on an unbound variable under `set -u`. ## The riskiest line, and why it is safe `. "$SELF_DIR/lib.sh"` in `grade-targets.sh` — `SELF_DIR`, not `TOOL_DIR`. `TOOL_DIR` is an env-overridable pointer at the *swappable* tools (`grade-pr-risk.sh`, `apply-risk-label.sh`, …) and the suite overrides it with a stub grader directory that holds none of this repo's other files; sourcing the library from there would break those phases. `lib.sh` is this script's own implementation and travels with it. `collect-pr-inputs.sh` keeps using `$TOOL_DIR/lib.sh`, matching exactly how it used to reach `$TOOL_DIR/grade-targets.sh`. ## Judgment call: `grade-targets.sh` leaves pr-derisk's `WATCHED_EXEC` The issue and the task only asked that both bump entrypoints *gain* `scripts/pr-risk/lib.sh`. I also removed `scripts/pr-risk/grade-targets.sh` from `bump-pr-derisk-callers.yml`'s list, because after this change a `/derisk` run never loads that file at all — `collect-pr-inputs.sh` sources the library directly and execs the grader. That list's own stated contract is "every file whose ABSENCE breaks a pinned caller at run time", and its own comment warns that an over-wide entry turns a pr-risk-side decommission into a false decommission that freezes *this* fleet behind a `::warning::`. Leaving it would have been exactly the shape that comment already excludes `apply-risk-label.sh` for. Empirically checked rather than assumed, since this is a removal: `grep -rn "grade-targets\|pr-risk/" .github/workflows/pr-derisk.yml` returns nothing, `pr-derisk.yml` invokes only `resolve-enabled.sh`, `collect-pr-inputs.sh`, `plan-derisk.sh` and `publish-derisk-comment.sh` out of a full (non-sparse) checkout, and no file under `scripts/pr-derisk/` references `grade-targets.sh` in any executable position after this change. `bump-pr-risk-callers.yml` still watches it — that is the fleet whose callers run it. Happy to restore the entry if a reviewer would rather keep the list conservative. ## Verified - `shellcheck -x` clean across both trees, with `lib.sh` added to the list in `test-pr-risk.yml:51` (as relative `lib.sh`) and `shellcheck -x .github/bump-callers/*.sh .github/bump-callers/tests/*.sh` clean too. - All seven shell suites over the two trees green: `test_grade_targets.sh` 105/0 (**including phase 21** — sourcing leaves the sourcing shell's EXIT trap intact and creates zero scratch files), `test_plan_derisk.sh` 63/0, `test_grade_pr_risk.sh` 138/0, `test_apply_risk_label.sh` 52/0, `test_publish_risk_surfaces.sh` 94/0, `test_pin_contract.sh` 28/0, `test_resolve_enabled.sh` 27/0. - **Phase 21 proven non-vacuous against the new file**: re-added a `trap … EXIT` plus an `mktemp` at `lib.sh`'s file scope and the phase failed (`got: trap -- 'echo BOOM' EXIT`); reverted and it passes. Sourcing `grade-targets.sh` sources `lib.sh`, so that assertion now covers the library transitively. - `test_paths_contract.sh` 110/0 and `test_preflight.sh` 445/0 for the two edited bump entrypoints. - `check_workflow_pins.py` OK (11 workflows, none defaulting `workflows_ref`), its unit suite 341/0, and `check_agents_md.py --root .` passed (2 pre-existing warnings, unrelated). - **Old-vs-new behavioural diff of `collect-pr-inputs.sh`**, since nothing in the repo covers that file: ran `origin/main`'s copy and this branch's copy under an identical stubbed `gh`, over both a happy path (base ref resolved from the API, both overrides 404 → generic defaults, diff fetched, `tier=R2`) and a failure path (a 502 on the runbooks override exercising the retry and the not-a-404-so-do-not-fall-back guard). `diff` of the two full stdout+stderr transcripts is **empty** in both cases, including the `[collect-pr-inputs] read failed (attempt 1/2)` retry line and exit codes. A caller-installed EXIT trap survived the source in both. - A direct `grade-targets.sh` run under a probe `TMPDIR` leaves **0** scratch files behind, including on the early-`die` path — the trap in `main()` covers every exit. - Read the linked issue Comfy-Org/github-workflows#245 directly (it is public): its helper list, its "no top-level side effects / prefixed diagnostics" requirement and its WATCHED_EXEC risk note all match what shipped. ## Provenance - **Authored by:** agent-work loop - **Verified:** see the Verified section above — shellcheck clean on both trees; 7 shell suites 509 assertions green (phase 21 included, and proven non-vacuous); `test_paths_contract.sh` 110/0; `test_preflight.sh` 445/0; `check_workflow_pins.py` + 341 unit tests OK; old-vs-new transcript diff of `collect-pr-inputs.sh` empty on two paths - **Deviations:** removed `scripts/pr-risk/grade-targets.sh` from `bump-pr-derisk-callers.yml`'s `WATCHED_EXEC` in addition to adding `lib.sh` — see the judgment-call section; every listed acceptance criterion is otherwise met ## Residual - **`collect-pr-inputs.sh` still leaks its three `mktemp` scratch files.** `init_scratch` mints `grade-targets-{err,label,out}.XXXXXX` under `$TMPDIR`, and `collect-pr-inputs.sh` installs no EXIT trap over them — so a `/derisk` run leaves three temp files behind. This is pre-existing and is **unchanged on purpose**, because "behaviour is identical — this is a pure extraction" was an explicit acceptance criterion and adding a trap is a behaviour change. It is harmless on an ephemeral Actions runner and a nuisance for anyone driving the script locally. The fix is two lines in `collect-pr-inputs.sh` (`trap 'rm -f "$ERRF" "$LABELF" "$OUTF"' EXIT` after the source, guarded for the empty-string case before `init_scratch` has run), and it belongs in its own change where it can be reviewed as a behaviour change rather than smuggled into a refactor. - **`.github/bump-callers/README.md`'s illustrative `WATCHED_EXEC` block (around `:466`) is stale and I did not touch it.** It shows four entries for the pr-risk fleet while the real `bump-pr-risk-callers.yml` now carries eight. That drift predates this PR — the real list had already grown past the example — and this change makes it one entry staler. It is documentation of an example rather than a contract nothing checks it against, so no test catches it. Worth a docs pass that either refreshes the excerpt or replaces it with a pointer to the live file. - **No `tests/test_lib.sh`, by design.** The ticket put a companion suite out of scope: the two existing suites cover these helpers through their current entrypoints. That remains true, but it is now true of a file that has no direct coverage of its own — `test_grade_targets.sh` reaches every helper only via `grade-targets.sh`, and `collect-pr-inputs.sh` (the other consumer) has no suite in the repo at all. - **`collect-pr-inputs.sh` has no checked-in test.** The old-vs-new transcript comparison above is the strongest evidence this PR carries for that file, and it was a throwaway harness in `/tmp`, not a committed suite. `test-pr-derisk.yml` shellchecks the file but runs nothing against it. Anyone changing the base-ref or override path through `lib.sh` in future has the `pr-risk` suite as a guard and nothing on the `pr-derisk` side. - **Not exercised: the two bump fleets themselves.** `bump-pr-risk-callers.yml` and `bump-pr-derisk-callers.yml` only run on `push` to `main` and mint an org-write token, so their `WATCHED_EXEC` edits are verified here only by `test_paths_contract.sh` / `test_preflight.sh` (which drive synthetic fixtures) and by reading `preflight.sh`'s blob probe — `resolve_blob` requires a regular-file blob and not an exec bit, so `lib.sh` at mode `100644` probes present. The first real exercise is the push-to-main run after merge. Closes #245 --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:15 +00:00