diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index 0fdfb7e7e2..1bb83c2baf 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -1,6 +1,6 @@ -# Reusable Dependency Review (workflow_call), consolidating the four -# near-identical dependency-review.yml files argos, mightyETL, newsdom-api, -# and scopeweave each carried independently. See +# Reusable Dependency Review (workflow_call), consolidating the near- +# identical dependency-review.yml files argos, mightyETL, newsdom-api, +# scopeweave, and naruon each carried independently. See # docs/adr/0024-dependency-review-reusable-workflow-consolidation.md and # docs/doctoring/dependency-review-reusable-workflow-consolidation.md for the # per-repo field audit behind these inputs. @@ -68,6 +68,17 @@ on: required: false type: boolean default: false + comment_summary_in_pr: + description: >- + Value forwarded to dependency-review-action's comment-summary-in-pr + input. Default "on-failure" (scopeweave's original choice, applied + uniformly when this input was still hardcoded); naruon explicitly + opts out with "never" -- an explicit per-repo choice, not + accidental drift, so it must stay an input rather than being + flattened to one value. + required: false + type: string + default: "on-failure" permissions: contents: read @@ -83,6 +94,11 @@ jobs: # since it is a forward-compatibility setting, not a per-repo policy. FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true steps: + - name: Harden the runner (Audit all outbound calls) + uses: step-security/harden-runner@b09bb98e06d4d774595224525879c09bc6e98c40 # v2.20.1 + with: + egress-policy: audit + - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -138,7 +154,7 @@ jobs: with: fail-on-severity: ${{ inputs.fail_on_severity }} allow-ghsas: ${{ inputs.allow_ghsas }} - comment-summary-in-pr: on-failure + comment-summary-in-pr: ${{ inputs.comment_summary_in_pr }} - name: Dependency graph unavailable note if: steps.dependency_graph.outputs.available != 'true' && github.event_name == 'pull_request' diff --git a/docs/adr/0024-dependency-review-reusable-workflow-consolidation.md b/docs/adr/0024-dependency-review-reusable-workflow-consolidation.md index 156e4d7ef1..8746db8b23 100644 --- a/docs/adr/0024-dependency-review-reusable-workflow-consolidation.md +++ b/docs/adr/0024-dependency-review-reusable-workflow-consolidation.md @@ -3,7 +3,8 @@ - **Status:** Accepted - **Date:** 2026-09-02 - **Scope:** `.github/workflows/dependency-review.yml` (new, central, `workflow_call`); - thin callers in `argos`, `mightyETL`, `newsdom-api`, `scopeweave` + thin callers in `argos`, `mightyETL`, `newsdom-api`, `scopeweave`, `naruon` + (`naruon` added same-day, see "Addendum: naruon" below) ## Context @@ -17,18 +18,19 @@ on every PR run. A field-by-field audit of all four files (2026-09-02) found: -| Field | argos | mightyETL | newsdom-api | scopeweave | -| --- | --- | --- | --- | --- | -| `fail-on-severity` | `moderate` | `high` | unset (action default `low`) | `moderate` | -| `allow-ghsas` | none | none | `GHSA-69w3-r845-3855` | none | -| `comment-summary-in-pr` | unset | unset | unset | `on-failure` | -| step-level `continue-on-error` | `true` | unset (blocking) | unset (blocking) | unset (blocking) | -| Dependency Graph availability handling | none (always runs, no fallback) | static `github.event.repository.private` branch to a separate no-op job | none | dynamic API preflight (`dependency-graph/compare` HTTP status): 200 → run the gate, 403/404 → warn and skip, any other status → hard-fail the job | -| trigger scope | `pull_request: branches: [main, developmental]` | `pull_request` (all branches) | `pull_request` (all branches) | `pull_request` + `workflow_dispatch` | -| concurrency group | none | `${{ github.workflow }}-${{ github.event.pull_request.number \|\| github.ref }}` | none | `dependency-review-${{ github.event.pull_request.number \|\| github.ref }}` | -| `actions/checkout` pin | unpinned `@v4` | n/a (action doesn't need checkout) | SHA `3d3c42e5...` | SHA `9c091bb2...` (v7.0.0) | -| `dependency-review-action` pin | unpinned `@v4` | SHA `a1d282b3...` (v5.0.0) | SHA `a1d282b3...` | SHA `a1d282b3...` | -| `FORCE_JAVASCRIPT_ACTIONS_TO_NODE24` | unset | unset | `true` | unset | +| Field | argos | mightyETL | newsdom-api | scopeweave | naruon | +| --- | --- | --- | --- | --- | --- | +| `fail-on-severity` | `moderate` | `high` | unset (action default `low`) | `moderate` | `moderate` | +| `allow-ghsas` | none | none | `GHSA-69w3-r845-3855` | none | none | +| `comment-summary-in-pr` | unset | unset | unset | `on-failure` | `never` (explicit) | +| step-level `continue-on-error` | `true` | unset (blocking) | unset (blocking) | unset (blocking) | unset (blocking) | +| Dependency Graph availability handling | none (always runs, no fallback) | static `github.event.repository.private` branch to a separate no-op job | none | dynamic API preflight (`dependency-graph/compare` HTTP status): 200 → run the gate, 403/404 → warn and skip, any other status → hard-fail the job | none | +| `step-security/harden-runner` | absent | absent | absent | absent | present (egress audit) | +| trigger scope | `pull_request: branches: [main, developmental]` | `pull_request` (all branches) | `pull_request` (all branches) | `pull_request` + `workflow_dispatch` | `pull_request: branches: [develop, master, release/**]` + `workflow_dispatch` | +| concurrency group | none | `${{ github.workflow }}-${{ github.event.pull_request.number \|\| github.ref }}` | none | `dependency-review-${{ github.event.pull_request.number \|\| github.ref }}` | `dependency-review-${{ github.event.pull_request.number \|\| github.ref }}` | +| `actions/checkout` pin | unpinned `@v4` | n/a (action doesn't need checkout) | SHA `3d3c42e5...` | SHA `9c091bb2...` (v7.0.0) | SHA `3d3c42e5...` (v7.0.1) | +| `dependency-review-action` pin | unpinned `@v4` | SHA `a1d282b3...` (v5.0.0) | SHA `a1d282b3...` | SHA `a1d282b3...` | SHA `a1d282b3...` | +| `FORCE_JAVASCRIPT_ACTIONS_TO_NODE24` | unset | unset | `true` | unset | unset | Two findings changed the design from a naive copy-paste consolidation: @@ -126,3 +128,40 @@ unreviewed against every caller's PR checks; (2) converting a job to ` / ` name, which broke `newsdom-api`'s branch protection (it required the old standalone name) until that required-check name was updated to match. + +## Addendum: naruon (2026-09-02, later the same day) + +A peer session's fresh org-wide workflow-duplication survey (63 repos, 255 +workflow files) found a fifth repository, `naruon`, independently carrying +its own `dependency-review.yml` — missed by the original survey this ADR's +consolidation was based on, which never covered `naruon`. Auditing it found +two real, non-cosmetic differences from the four originals above: + +1. **A `step-security/harden-runner` step (egress audit), present in none + of the original four.** Not a per-repo policy — it is a uniformly + beneficial security-hardening practice already standard elsewhere in + this org's own workflows (e.g. `pr-review-autofix.yml`), so it is added + to the reusable workflow itself, as its first step, applying to every + caller including the four already migrated (no caller-side change + needed for this one). +2. **`comment-summary-in-pr: never`, an explicit opt-out**, conflicting + with the earlier decision (see item 3 above) to hardcode + `comment-summary-in-pr: on-failure` uniformly for every caller. That + earlier decision was made when only scopeweave's original set the + field at all, so "hardcode it uniformly" cost no caller its own choice. + naruon proves that assumption wrong: hardcoding it now would silently + overturn an explicit, deliberate choice naruon's original workflow + made. Corrected by making `comment_summary_in_pr` a proper + `workflow_call` input (default `"on-failure"`, preserving current + behavior for the four already-migrated callers with no changes needed + on their side; `naruon`'s caller explicitly sets `"never"`). + +`naruon`'s other fields (`fail-on-severity: moderate`, no `allow-ghsas`, +multi-branch trigger `develop`/`master`/`release/**` plus +`workflow_dispatch`, its own `concurrency` group, job-level `permissions:` +redundant with the workflow-level block, and an informational "Log +dependency review policy" step) either match an existing input, are +caller-side triggers/concurrency untouched by this ADR's design, or (the +informational logging step, and the redundant job-level `permissions:`) +are dropped as they add no policy value the central workflow or the +underlying action doesn't already provide. diff --git a/docs/doctoring/dependency-review-reusable-workflow-consolidation.md b/docs/doctoring/dependency-review-reusable-workflow-consolidation.md index d24c2a30a3..eeadf746d7 100644 --- a/docs/doctoring/dependency-review-reusable-workflow-consolidation.md +++ b/docs/doctoring/dependency-review-reusable-workflow-consolidation.md @@ -15,18 +15,25 @@ repository's own file. See Reading all four files' full bodies (not just the job name and action used) found real, repo-specific policy differences, not accidental copy drift: -| Field | argos | mightyETL | newsdom-api | scopeweave | -| --- | --- | --- | --- | --- | -| `fail-on-severity` | `moderate` | `high` | unset → action default `low` | `moderate` | -| `allow-ghsas` | none | none | `GHSA-69w3-r845-3855` | none | -| `comment-summary-in-pr` | unset | unset | unset | `on-failure` | -| step `continue-on-error` | `true` | unset (blocking) | unset (blocking) | unset (blocking) | -| availability handling | none | static `repository.private` branch to a separate no-op job | none | dynamic `dependency-graph/compare` HTTP-status preflight: 200 → run, 403/404 → warn+skip, other → hard-fail | -| trigger | `pull_request: branches: [main, developmental]` | `pull_request` | `pull_request` | `pull_request`, `workflow_dispatch` | -| concurrency group | none | workflow+PR/ref group, cancel-in-progress | none | `dependency-review-`+PR/ref group, cancel-in-progress | -| `actions/checkout` pin | unpinned `@v4` | not used | SHA `3d3c42e5aac5ba805825da76410c181273ba90b1` | SHA `9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0` (v7.0.0) | -| `dependency-review-action` pin | unpinned `@v4` | SHA `a1d282b36b6f3519aa1f3fc636f609c47dddb294` (v5.0.0) | same SHA | same SHA | -| `FORCE_JAVASCRIPT_ACTIONS_TO_NODE24` | unset | unset | `true` | unset | +| Field | argos | mightyETL | newsdom-api | scopeweave | naruon | +| --- | --- | --- | --- | --- | --- | +| `fail-on-severity` | `moderate` | `high` | unset → action default `low` | `moderate` | `moderate` | +| `allow-ghsas` | none | none | `GHSA-69w3-r845-3855` | none | none | +| `comment-summary-in-pr` | unset | unset | unset | `on-failure` | `never` (explicit) | +| step `continue-on-error` | `true` | unset (blocking) | unset (blocking) | unset (blocking) | unset (blocking) | +| availability handling | none | static `repository.private` branch to a separate no-op job | none | dynamic `dependency-graph/compare` HTTP-status preflight: 200 → run, 403/404 → warn+skip, other → hard-fail | none | +| `harden-runner` (egress audit) | absent | absent | absent | absent | present | +| trigger | `pull_request: branches: [main, developmental]` | `pull_request` | `pull_request` | `pull_request`, `workflow_dispatch` | `pull_request: branches: [develop, master, release/**]`, `workflow_dispatch` | +| concurrency group | none | workflow+PR/ref group, cancel-in-progress | none | `dependency-review-`+PR/ref group, cancel-in-progress | `dependency-review-`+PR/ref group, cancel-in-progress | +| `actions/checkout` pin | unpinned `@v4` | not used | SHA `3d3c42e5aac5ba805825da76410c181273ba90b1` | SHA `9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0` (v7.0.0) | SHA `3d3c42e5aac5ba805825da76410c181273ba90b1` (v7.0.1) | +| `dependency-review-action` pin | unpinned `@v4` | SHA `a1d282b36b6f3519aa1f3fc636f609c47dddb294` (v5.0.0) | same SHA | same SHA | same SHA | +| `FORCE_JAVASCRIPT_ACTIONS_TO_NODE24` | unset | unset | `true` | unset | unset | + +naruon was found later the same day by a peer session's fresh org-wide survey +-- missed by the original 4-repo survey this consolidation started from. See +"Addendum: naruon" below for the two real design changes it required +(`comment_summary_in_pr` becoming an input instead of a hardcoded uniform +value, and adding `harden-runner` uniformly). Two decisions this audit drove (see ADR-0024 for the full reasoning): @@ -50,10 +57,11 @@ Two decisions this audit drove (see ADR-0024 for the full reasoning): ## Mechanism -`.github/workflows/dependency-review.yml` (this repository) takes three +`.github/workflows/dependency-review.yml` (this repository) takes four `workflow_call` inputs (`fail_on_severity`, `allow_ghsas`, -`continue_on_error`) and always runs the checkout → availability-preflight → -conditional dependency-review → conditional unavailability-note sequence. +`continue_on_error`, `comment_summary_in_pr`) and always runs the +harden-runner → checkout → availability-preflight → conditional +dependency-review → conditional unavailability-note sequence. Each calling repository's own thin `.github/workflows/dependency-review.yml` keeps that repository's original `on:` trigger block (argos keeps its `branches: [main, developmental]` restriction — a `workflow_call` target @@ -153,6 +161,61 @@ 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. +### naruon caller + +```yaml +name: Dependency Review + +on: + pull_request: + branches: + - develop + - master + - "release/**" + workflow_dispatch: + +concurrency: + group: dependency-review-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + dependency-review: + uses: ContextualWisdomLab/.github/.github/workflows/dependency-review.yml@ + with: + fail_on_severity: moderate + comment_summary_in_pr: never +``` + +naruon's original also had a job-level `permissions:` block duplicating the +workflow-level one, and an informational "Log dependency review policy" step +that only printed the policy text and base/head refs -- neither is carried +into the caller: the job-level `permissions:` was redundant, and the log +step added no policy value beyond what `actions/dependency-review-action` +itself already reports on failure. + +## Addendum: naruon (2026-09-02, later the same day) + +A peer session's fresh org-wide workflow-duplication survey (63 repos, 255 +workflow files) found `naruon` independently carrying its own +`dependency-review.yml` -- missed by the original 4-repo survey. Auditing it +found two real differences, not cosmetic ones: + +1. **`step-security/harden-runner` (egress audit)**, absent from all four + original callers. Not a per-repo policy choice -- a uniformly beneficial + hardening practice already standard elsewhere in this org (e.g. + `pr-review-autofix.yml`). Added to the reusable workflow itself as its + first step, so every caller (the four already migrated included) gets it + with no caller-side change required. +2. **`comment-summary-in-pr: never`**, an explicit opt-out that directly + conflicts with the earlier decision to hardcode + `comment-summary-in-pr: on-failure` uniformly (made when only scopeweave's + original set the field, so hardcoding it cost no caller its own choice). + Silently applying that hardcoded value to naruon would overturn a + deliberate choice its original workflow made. Fixed by making + `comment_summary_in_pr` a proper `workflow_call` input, default + `"on-failure"` (no change for the four already-migrated callers), + `naruon`'s caller explicitly setting `"never"`. + ## Post-merge corrections (2026-09-02, same day) Two real problems surfaced after the four caller PRs opened, both caught diff --git a/tests/test_dependency_review_reusable_workflow_contract.py b/tests/test_dependency_review_reusable_workflow_contract.py index 3a856b4e2b..cadefcb8ac 100644 --- a/tests/test_dependency_review_reusable_workflow_contract.py +++ b/tests/test_dependency_review_reusable_workflow_contract.py @@ -24,22 +24,29 @@ def _workflow_text() -> str: return _WORKFLOW.read_text(encoding="utf-8") -def test_declares_workflow_call_with_three_inputs_and_recorded_defaults() -> None: - """Every genuinely-varying field found while auditing the four originals is an input.""" +def test_declares_workflow_call_with_four_inputs_and_recorded_defaults() -> None: + """Every genuinely-varying field found while auditing the five originals is an input.""" workflow = _workflow_text() assert "on:\n workflow_call:\n inputs:" in workflow - for name in ("fail_on_severity:", "allow_ghsas:", "continue_on_error:"): + for name in ( + "fail_on_severity:", + "allow_ghsas:", + "continue_on_error:", + "comment_summary_in_pr:", + ): assert name in workflow assert 'default: "moderate"' in workflow assert 'default: ""' in workflow assert "default: false" in workflow + assert 'default: "on-failure"' in workflow -def test_step_order_is_checkout_then_preflight_then_gated_steps() -> None: - """checkout -> dependency-graph preflight -> conditional gate/note, in that order.""" +def test_step_order_is_harden_then_checkout_then_preflight_then_gated_steps() -> None: + """harden-runner -> checkout -> dependency-graph preflight -> conditional gate/note.""" workflow = _workflow_text() order = [ + "Harden the runner", "actions/checkout@", "Check dependency graph availability", "Dependency review", @@ -61,10 +68,18 @@ def test_dependency_review_and_note_steps_are_mutually_exclusive_on_availability def test_inputs_are_forwarded_to_the_dependency_review_action() -> None: - """fail_on_severity and allow_ghsas must reach the underlying action untouched.""" + """fail_on_severity, allow_ghsas, and comment_summary_in_pr must reach the action untouched.""" workflow = _workflow_text() assert "fail-on-severity: ${{ inputs.fail_on_severity }}" in workflow assert "allow-ghsas: ${{ inputs.allow_ghsas }}" in workflow + assert "comment-summary-in-pr: ${{ inputs.comment_summary_in_pr }}" in workflow + + +def test_harden_runner_audits_egress() -> None: + """naruon's harden-runner step applies uniformly, not only to that one caller.""" + workflow = _workflow_text() + assert "step-security/harden-runner@" in workflow + assert "egress-policy: audit" in workflow def test_action_pins_are_current_and_uniform() -> None: @@ -113,7 +128,15 @@ def test_availability_check_only_runs_the_gate_for_pull_request_events() -> None assert '"${{ github.event_name }}" != "pull_request"' in workflow -def test_dependency_review_posts_a_pr_comment_on_failure() -> None: - """scopeweave's PR-comment-on-failure UX applies uniformly, not only to that one caller.""" +def test_dependency_review_comment_summary_defaults_to_on_failure() -> None: + """scopeweave's PR-comment-on-failure UX applies uniformly by default, overridable per caller. + + naruon explicitly overrides it to "never" -- see + test_declares_workflow_call_with_four_inputs_and_recorded_defaults for + the default assertion and test_inputs_are_forwarded_to_the_dependency_review_action + for the forwarding assertion; this test just pins the specific default + value chosen (scopeweave's original, not naruon's or some other value). + """ workflow = _workflow_text() - assert "comment-summary-in-pr: on-failure" in workflow + assert 'comment_summary_in_pr:\n' in workflow + assert 'default: "on-failure"' in workflow