diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index d199dd36a0..0fdfb7e7e2 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -18,7 +18,15 @@ # adopts the dynamic detection as the common, more-correct behavior for # every caller, so no per-repo public/private input is needed. # -# Example caller (.github/workflows/dependency-review.yml in a product repo): +# Example caller (.github/workflows/dependency-review.yml in a product repo). +# Pin `uses:` to this file's exact commit SHA, not @main: an unpinned mutable +# ref would run an unreviewed central change against every PR check in the +# calling repo (Devin flagged this on the first four callers; fixed in all of +# them). If the calling repo's branch protection requires a status check +# literally named after the old standalone job, converting to `uses:` here +# will rename the published check to " / dependency-review" and +# silently break that required check -- update the branch protection's +# required-check name to match before or immediately after merging a caller. # # name: Dependency Review # on: @@ -28,7 +36,7 @@ # cancel-in-progress: true # jobs: # dependency-review: -# uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@main +# uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@ # with: # fail_on_severity: high # allow_ghsas: "GHSA-69w3-r845-3855" diff --git a/docs/adr/0024-dependency-review-reusable-workflow-consolidation.md b/docs/adr/0024-dependency-review-reusable-workflow-consolidation.md index 51bc203c37..156e4d7ef1 100644 --- a/docs/adr/0024-dependency-review-reusable-workflow-consolidation.md +++ b/docs/adr/0024-dependency-review-reusable-workflow-consolidation.md @@ -117,4 +117,12 @@ drift sources, not repo-specific contracts" principle uses, closing that drift too. See `docs/doctoring/dependency-review-reusable-workflow-consolidation.md` for -the full per-repo audit and the exact diffs each caller received. +the full per-repo audit and the exact diffs each caller received, including +two post-merge corrections found by Devin's review on the caller PRs: (1) +every caller now pins `uses:` to this file's exact commit SHA rather than +the mutable `@main`, since a mutable central-workflow reference runs +unreviewed against every caller's PR checks; (2) converting a job to +`uses: ` renames its published check-run to a combined +` / ` name, which broke `newsdom-api`'s branch +protection (it required the old standalone name) until that required-check +name was updated to match. diff --git a/docs/doctoring/dependency-review-reusable-workflow-consolidation.md b/docs/doctoring/dependency-review-reusable-workflow-consolidation.md index 7b63da83a6..d24c2a30a3 100644 --- a/docs/doctoring/dependency-review-reusable-workflow-consolidation.md +++ b/docs/doctoring/dependency-review-reusable-workflow-consolidation.md @@ -59,7 +59,7 @@ keeps that repository's original `on:` trigger block (argos keeps its `branches: [main, developmental]` restriction — a `workflow_call` target cannot itself be what GitHub triggers on pull_request), gains a `concurrency` block if it lacked one, and adds one job: -`uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@main` +`uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@0bcd22d8bb07650aafb0a8f116e4c2bbb8744f03` with only that repository's non-default `with:` values. ### argos caller @@ -77,7 +77,7 @@ concurrency: jobs: dependency-review: - uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@main + uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@0bcd22d8bb07650aafb0a8f116e4c2bbb8744f03 with: fail_on_severity: moderate continue_on_error: true @@ -97,7 +97,7 @@ concurrency: jobs: dependency-review: - uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@main + uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@0bcd22d8bb07650aafb0a8f116e4c2bbb8744f03 with: fail_on_severity: high ``` @@ -116,7 +116,7 @@ concurrency: jobs: dependency-review: - uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@main + uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@0bcd22d8bb07650aafb0a8f116e4c2bbb8744f03 with: fail_on_severity: low allow_ghsas: "GHSA-69w3-r845-3855" @@ -137,7 +137,7 @@ concurrency: jobs: dependency-review: - uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@main + uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@0bcd22d8bb07650aafb0a8f116e4c2bbb8744f03 with: fail_on_severity: moderate ``` @@ -153,6 +153,44 @@ job-level `if:` to reproduce it — `workflow_dispatch` stays in the trigger list and the job still runs, harmlessly skipping the gate exactly as the original did. +## Post-merge corrections (2026-09-02, same day) + +Two real problems surfaced after the four caller PRs opened, both caught +before any of them merged (except argos, fixed retroactively): + +**1. Mutable `@main` reference (Devin, security finding).** The original +callers referenced `uses: .../dependency-review.yml@main` — the example +above now shows the corrected pattern. A mutable branch ref means an +unreviewed change to `.github`'s `main` (or a reference-tampering attack) +runs directly against every caller's PR checks with zero review in the +calling repo. Fixed by pinning every caller to the exact commit SHA that +added the file, `0bcd22d8bb07650aafb0a8f116e4c2bbb8744f03` (unchanged since +it merged) — `argos` retroactively (a follow-up PR after its original +merge), the other three before their first merge. This is now the +documented pattern in the reusable workflow's own header comment: pin +`uses:` to a commit SHA for every caller, the same way every *action* step +inside the reusable workflow itself is already SHA-pinned. + +**2. Required-status-check name collision (Devin, bug finding on +newsdom-api).** Converting a job from inline steps to `uses: ` changes the check-run name GitHub publishes, from the caller +job's own name (e.g. `dependency-review`) to a combined +` / ` (here, +`dependency-review / dependency-review`). `newsdom-api`'s `develop` branch +protection required a status check named literally `dependency-review` — +after conversion, that exact name is never published again, so the +required check stays pending forever and blocks every future merge. +Verified live: `argos` and `mightyETL` have no branch protection at all +(nothing to break); `scopeweave`'s required checks don't include +`dependency-review`; only `newsdom-api` was affected. Fixed by updating +`newsdom-api`'s branch protection required-status-checks list directly +(`gh api -X PATCH repos/.../branches/develop/protection/required_status_checks`), +replacing `dependency-review` with the actual published name +`dependency-review / dependency-review`. This is a general gotcha for any +future "convert a standalone job to a reusable-workflow caller" change — +check the target repo's branch protection for a required check matching the +job's *old* name before or immediately after merging the conversion. + ## Verified before merge - `python3 -c "import yaml; yaml.safe_load(open(...))"` on all five files