Skip to content

Runbook gotcha: any fix to squad-repo-health.yml is unverifiable in CI until it merges #1800

Description

@bradygaster

The property

.github/workflows/squad-repo-health.yml is permanently self-blocking: a pull request that fixes this workflow is always tested against the old, unfixed copy.

Two mechanisms combine:

  1. on: pull_request_target — GitHub takes the workflow definition from the base branch, not from the PR head. This is inherent to the trigger.
  2. The workflow then deliberately checks out base-branch scripts as well. There is no ref: override on any of its four checkout steps (only fetch-depth: 0), and there is a # SAFETY: comment explaining that this is intentional — running untrusted PR code under pull_request_target's elevated token would be a security hole.

Both choices are correct. The consequence is not a bug to fix, it is a property to know about.

Why it matters

The only pre-merge signal available for a change to this workflow is "the job didn't crash."

That is exactly the weak signal that let #1770 live undetected. A crashed github-script reporter is indistinguishable from a clean run to anyone skimming the checks list — it fails the class of finding it exists to catch, silently.

How it showed up

#1770 fixed three repo-health reporters that crashed on any finding message containing a backtick. On the PR that fixed it (#1786), Repo Health / Security Review — Permissions & Secrets was red at head with:

##[error]Unhandled error: SyntaxError: Unexpected identifier 'git'
    at new AsyncFunction (<anonymous>)
    at callAsyncFunction (.../github-script/dist/index.js:64949:16)

The fix was verified complete on the branch — zero remaining backtick-literal interpolation sites, all three reporters moved to the env:/process.env boundary. It simply had no effect, because dev's copy is what ran.

FIDO hit the same wall independently on #1792.

What to do when you touch this workflow

  • Expect the check to be red or misleading. Do not read a green Repo Health on such a PR as evidence the fix works; do not read a red one as evidence it doesn't.
  • Verify locally instead. Run the scanner directly (node scripts/security-review.mjs) and compile the script: bodies out of the real YAML — test/scripts/repo-health-comment-transport.test.ts (added in fix(ci): stop splicing repo-health report content into github-script bodies #1786) does exactly this and is the intended substitute for the missing CI signal.
  • Satisfy the gate on its own terms. If your PR's own diff trips a repo-health rule, reword the diff. Do not add an ignore entry, lower a severity, or add a path exclusion to get green.
  • Re-verify immediately after merge, since that is the first moment the fix is actually live.

Placement

Belongs in the runbook family alongside gotcha #10.

Related: #1770, #1786, #1792.

Metadata

Metadata

Assignees

No one assigned

    Labels

    squadSquad triage inbox — Lead will assign to a membersquad:boosterAssigned to Booster (CI/CD Engineer)

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions