From 17fbca71e49d744383a174a8c12ec3d997832ad1 Mon Sep 17 00:00:00 2001
From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com>
Date: Tue, 20 Oct 2026 15:55:48 -0300
Subject: [PATCH] feat(go-pr-analysis): let callers gate on govulncheck instead
 of only logging it
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The "Run govulncheck" step carried a bare `continue-on-error: true`, so a known
vulnerability in a consumer's dependency graph left the Security job green and
the finding sat in a job log nobody opens on a passing check.

Removing the flag outright would be a break for every repository on the channel:
the Go vulnerability database moves independently of the pull request, so a newly
published CVE in a common dependency would turn them all red at once, on pull
requests touching nothing related — the same shape of breakage x/vuln v1.8.0
already caused here.

So the step now branches on the exit code instead. Findings are always annotated:
a warning by default, an error — failing the job — when the new
`fail_on_govulncheck` input is true. The default keeps today's behaviour, and a
repository that has cleaned its graph can opt into the gate on its own schedule.

Closes #910
---
 .github/workflows/go-pr-analysis.yml   | 29 ++++++++++++++++++++++++--
 .github/workflows/go-pr-validation.yml | 12 +++++++++++
 docs/go-pr-analysis.md                 |  7 +++++++
 docs/go-pr-validation.md               |  1 +
 4 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/go-pr-analysis.yml b/.github/workflows/go-pr-analysis.yml
index 6f54339a..b2f2362d 100644
--- a/.github/workflows/go-pr-analysis.yml
+++ b/.github/workflows/go-pr-analysis.yml
@@ -108,6 +108,17 @@ on:
         description: 'Enable security scanning (gosec, govulncheck)'
         type: boolean
         default: true
+      fail_on_govulncheck:
+        description: >-
+          Fail the Security job when govulncheck reports findings. Default false preserves the
+          advisory behaviour this step has always had: the Go vulnerability database changes
+          independently of the pull request, so a newly published CVE in a common dependency would
+          otherwise turn every repository on this channel red at once, on pull requests that touch
+          nothing related. Findings are reported either way — as a warning annotation when false, as
+          an error when true — so the scan is never silent. Turn it on per repository once its
+          dependency graph is clean.
+        type: boolean
+        default: false
       enable_tests:
         description: 'Enable unit tests'
         type: boolean
@@ -438,8 +449,22 @@ jobs:
 
       - name: Run govulncheck
         working-directory: ${{ matrix.app.working_dir }}
-        run: govulncheck ./...
-        continue-on-error: true
+        env:
+          # Data, not script: substituted into the environment, never into the line the shell parses.
+          FAIL_ON_FINDINGS: ${{ inputs.fail_on_govulncheck }}
+          APP_NAME: ${{ matrix.app.name }}
+        # A bare `continue-on-error: true` swallowed the result entirely — the finding stayed in the
+        # job log, which nobody opens on a green check. Branching on the exit code instead reports it
+        # as an annotation in both modes, and lets a caller that has cleaned its graph gate on it.
+        run: |
+          if govulncheck ./...; then
+            exit 0
+          fi
+          if [ "$FAIL_ON_FINDINGS" = "true" ]; then
+            echo "::error::govulncheck reported findings for $APP_NAME"
+            exit 1
+          fi
+          echo "::warning::govulncheck reported findings for $APP_NAME (advisory: fail_on_govulncheck is false)"
 
   # ============================================
   # UNIT TESTS
diff --git a/.github/workflows/go-pr-validation.yml b/.github/workflows/go-pr-validation.yml
index 39bf3da7..cf6c133a 100644
--- a/.github/workflows/go-pr-validation.yml
+++ b/.github/workflows/go-pr-validation.yml
@@ -248,6 +248,17 @@ on:
         description: 'Extra arguments passed to golangci-lint (e.g. --timeout=5m)'
         type: string
         default: '--timeout=5m'
+      fail_on_govulncheck:
+        description: >-
+          Fail the Security job when govulncheck reports findings. Default false preserves the
+          advisory behaviour this step has always had: the Go vulnerability database changes
+          independently of the pull request, so a newly published CVE in a common dependency would
+          otherwise turn every repository on this channel red at once, on pull requests that touch
+          nothing related. Findings are reported either way — as a warning annotation when false, as
+          an error when true — so the scan is never silent. Turn it on per repository once its
+          dependency graph is clean.
+        type: boolean
+        default: false
       app_name_prefix:
         description: 'Prefix used to namespace coverage/build artifacts'
         type: string
@@ -528,6 +539,7 @@ jobs:
       go_version: ${{ inputs.go_version }}
       golangci_lint_version: ${{ inputs.golangci_lint_version }}
       govulncheck_version: ${{ inputs.govulncheck_version }}
+      fail_on_govulncheck: ${{ inputs.fail_on_govulncheck }}
       golangci_lint_args: ${{ inputs.golangci_lint_args }}
       app_name_prefix: ${{ inputs.app_name_prefix }}
       filter_paths: ${{ inputs.filter_paths }}
diff --git a/docs/go-pr-analysis.md b/docs/go-pr-analysis.md
index 798ca248..19196849 100644
--- a/docs/go-pr-analysis.md
+++ b/docs/go-pr-analysis.md
@@ -108,6 +108,7 @@ jobs:
 | `go_version` | Go version to use | No | `1.23` |
 | `golangci_lint_version` | GolangCI-Lint version | No | `v1.62.2` |
 | `govulncheck_version` | govulncheck version to install. Pinned rather than `@latest` so an upstream release cannot change what runs here unchosen. Works at any `go_version` — the install step overrides `GOTOOLCHAIN=auto`, the scan still runs under the pinned toolchain | No | `v1.7.0` |
+| `fail_on_govulncheck` | Fail the Security job when govulncheck reports findings. Default `false` keeps the scan advisory: the Go vulnerability database changes independently of the pull request, so a newly published CVE in a common dependency would otherwise turn every repository on the channel red at once, on pull requests that touch nothing related. Findings are annotated either way — warning when `false`, error when `true` — so the scan is never silent. Turn it on per repository once its dependency graph is clean | No | `false` |
 | `golangci_lint_args` | Additional golangci-lint arguments | No | `--timeout=5m` |
 | `coverage_threshold` | Minimum coverage percentage (0-100) | No | `80` |
 | `fail_on_coverage_threshold` | Fail if coverage below threshold | No | `true` |
@@ -177,6 +178,12 @@ Runs security scanners per changed app:
 - **GoSec**: Static analysis for security issues (uploads SARIF to GitHub Security tab)
 - **govulncheck**: Official Go vulnerability database check
 
+GoSec runs in report-only mode (`-no-fail`) and publishes its findings to the Security tab.
+govulncheck is advisory by default as well, but always annotates what it finds: a warning when
+`fail_on_govulncheck` is `false`, an error — and a failed job — when it is `true`. Set it to `true`
+once the repository's dependency graph is clean, so a reachable vulnerability stops being something
+only a reader of the job log would notice.
+
 ### tests
 Runs unit tests per changed app with:
 - Race detection (`-race`) with `CGO_ENABLED=1`
diff --git a/docs/go-pr-validation.md b/docs/go-pr-validation.md
index 1bd344d8..acd4563d 100644
--- a/docs/go-pr-validation.md
+++ b/docs/go-pr-validation.md
@@ -76,6 +76,7 @@ This distinguishes the two skips that used to look identical: a pipeline the cal
 | `go_version` | Go version | string | `1.23` |
 | `golangci_lint_version` | GolangCI-Lint version | string | `v1.62.2` |
 | `govulncheck_version` | govulncheck version to install. Pinned rather than `@latest` so an upstream release cannot change what runs here unchosen. Works at any `go_version` — the install step overrides `GOTOOLCHAIN=auto`, the scan still runs under the pinned toolchain | string | `v1.7.0` |
+| `fail_on_govulncheck` | Fail the Security job when govulncheck reports findings. Default `false` keeps the scan advisory: the Go vulnerability database changes independently of the pull request, so a newly published CVE in a common dependency would otherwise turn every repository on the channel red at once, on pull requests that touch nothing related. Findings are annotated either way — warning when `false`, error when `true` — so the scan is never silent. Turn it on per repository once its dependency graph is clean | boolean | `false` |
 | `golangci_lint_args` | Extra arguments passed to golangci-lint (e.g. `--timeout=5m`) | string | `--timeout=5m` |
 | `app_name_prefix` | Prefix used to namespace coverage/build artifacts | string | `''` |
 | `filter_paths` | Newline-separated component path prefixes for monorepo per-component analysis (lint/tests/coverage) **and** security scanning; empty = single-app root run. When using it for security, leave `dockerfile_path` empty so each component Dockerfile is discovered | string | `''` |