[IAmJSD]

Root cause

gosec ran with three different rule sets in three places:

• scripts/gosec-hook.sh (pre-commit) excluded 15 rules (G101,G104,G115,G117,G118,G122,G204,G301,G304,G402,G505,G702,G703,G705,G706).
• make security-scan-go excluded 8 different rules, scanned only the root module, and exited 0 when gosec was not on PATH.
• The CI "Run gosec Security Scanner" step excluded nothing.

So a clean pre-commit or make security-scan-go run predicted nothing about CI.

Fix

This mirrors LeanerCloud/cloud-commitments-go#485.

• New scripts/run-gosec.sh is the single source of truth. It holds the gosec pin (v2.28.0), the module list (every go.work module: . and tests/e2e) and the rule set: gosec defaults with no -exclude, which is what CI already enforced. It takes --module MOD [PKG...], --sarif-dir DIR (CI) and --list-modules (hook). It rejects any other argument.
• scripts/gosec-hook.sh now only maps staged files to module/package pairs and execs the script. Per-changed-package scanning is unchanged.
• make security-scan-go and the CI gosec step call the script. CI keeps the same SARIF file names (gosec-root.sarif, gosec-tests-e2e.sarif), the same automationDetails ids (gosec-root/, gosec-tests-e2e/, checked by running the step locally), the same "scan every module, then fail" behavior (#1717), and the same merge and upload steps.
• scripts/security-scan.sh (a standalone helper that nothing invokes) ran a fourth, unpinned, root-only gosec. It now calls the script too.
• The gosec binary is built with the root go.mod toolchain (CI's GOTOOLCHAIN=local is respected) and cached under ~/.cache/gosec/<gosec>-<go env GOVERSION>/. It is installed into a temp dir and then moved into place. If the toolchain doesn't match, gosec can't read the export data and reports 0 issues without scanning anything, so the key includes the Go version.
• The Makefile no longer writes gosec-report.json. Nothing reads it; only clean mentions it.
• New scripts/test-run-gosec.sh runs in the security-scan job before the gosec step. It uses a fake gosec, so it is fast and offline. It pins:
  • the exact gosec calls (module dirs, packages, flags, no -exclude) for full, SARIF and per-package runs
  • the SARIF names
  • finding propagation, including that later modules are still scanned after a finding
  • a missing SARIF file failing the run
  • how the hook maps files to packages and filters deleted files, testdata and paths with whitespace
  • that make -n security-scan-go runs only the script
  • that the CI step calls the script and never installs or runs gosec itself
  • that no -exclude=G… list and no direct gosec -flag call exists in the Makefile, workflows or scripts/*.sh outside the script

New findings from unifying: none. gosec v2.28.0 (go1.26.9 build) with no excludes reports 0 issues on both modules (root: 244 files, 78,333 lines, 43 existing #nosec). So nothing needed fixing and no #nosec was added. This matches CI, which already ran without excludes and is green on main.

Closes #340

Verification

All of this is local evidence on macOS (bash 5 and /bin/bash 3.2): shell tests with a fake gosec, plus real gosec runs on scratch copies. CI on this PR is the remaining check of the workflow change on Linux.

• bash scripts/test-run-gosec.sh: 17 PASS, exit 0 (also under /bin/bash 3.2).
• Fail before: the same test run against an origin/main copy fails 15 checks and passes only "hook runs no scan when nothing survives filtering". The script is missing (exit 127), the old hook tries to install its own gosec, make -n security-scan-go shows the -exclude=G101,… recipe, the CI step runs gosec -fmt sarif directly, and the guards list the stray rule sets (Makefile:165 and GOSEC_EXCLUDE in scripts/gosec-hook.sh) and the direct gosec calls (Makefile, ci.yml and scripts/security-scan.sh).
• Mutation probes on the test: adding -exclude=G104 to the script's args fails 5 cases. Dropping tests/e2e from MODULES fails 5 cases.
• Real-gosec probe (scratch copy): added a package with an unchecked os.Remove("x") (G104) and os.ReadFile(p) (G304), both of which the old hook excluded.
  • New hook: exit 1, both findings reported.
  • Old hook on an origin/main copy: exit 0.
  • New make security-scan-go: fails (gosec exit 1).
  • Old make security-scan-go: printed "gosec not installed" and exited 0.
  • CI step body run locally against the copy: exit 1, merged SARIF has gosec-root/ with 2 results.
  • security-scan.sh gosec section: OVERALL_STATUS=1.
• Clean tree:
  • make security-scan-go: exit 0.
  • scripts/run-gosec.sh --sarif-dir …: exit 0, 0 results in both SARIF files.
  • CI step body run locally with RUNNER_TEMP set: exit 0, "Merged 2 SARIF runs", ids gosec-root/ and gosec-tests-e2e/.
• Linters and scanners:
  • shellcheck 0.11.0 on all changed scripts: clean.
  • actionlint .github/workflows/ci.yml: clean.
  • zizmor==1.29.0 --offline --persona=pedantic --min-severity=medium: no findings.
  • git diff --check: clean. YAML parses.
  • git-secrets (--register-aws, fresh repo, isolated HOME) over all tracked files: clean. scripts/test-git-secrets-allowlist.sh: 42 PASS.
• No Go code changed, so golangci-lint, gocyclo and the Go tests are unaffected. pre-commit itself isn't installed on this machine, so I ran its hooks individually as listed above.

Out of scope / follow-ups

• .github/workflows/pre-commit.yml still installs gosec@v2.28.0 into ~/go/bin, but its gosec hook is SKIPped in CI, so the binary is never used. That pin is dead but still duplicated. The Makefile's GOSEC_VERSION for install-dev-tools also remains. go#485 left the equivalent in place.
• .golangci.yml configures golangci-lint's own gosec linter (excludes: [G104]). That is a different tool and gate, and I didn't touch it.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/internal, effort/s, type/chore, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: e0215b0
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5336973e-13bb-4109-a9bf-c23c9d5dd4b0

📥 Commits

Reviewing files that changed from the base of the PR and between e0215b0 and f25ca0c.

📒 Files selected for processing: 7

• .github/workflows/ci.yml
• .pre-commit-config.yaml
• Makefile
• scripts/gosec-hook.sh
• scripts/run-gosec.sh
• scripts/security-scan.sh
• scripts/test-run-gosec.sh

+-------------------------------------------------------------------------------------------------------------------+
| Work with a user to think like a user. It's the best way to gain insight into how the system will really be used. |
+-------------------------------------------------------------------------------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

Fixed the red Security Scanning job in f25ca0c. The new self-test compared the output of make -C <root> -n security-scan-go exactly. GNU make, which the CI runner uses, wraps that output in Entering directory/Leaving directory lines; macOS make doesn't, so the check only failed in CI. The test now passes --no-print-directory. I confirmed the difference on ubuntu:24.04 GNU make, and the self-test passes locally. No change to the gosec script or the rule set.

[cristim]

VERDICT: APPROVE f25ca0c

Base: merge-base e0215b0 is stale, but git merge-tree --write-tree origin/main(ca1e8a82) f25ca0c6 is clean (tree c6b012fe). Everything below ran on that merged tree.

Findings (none blocking)

• coordination: open PR ci: single-source the gosec version in .gosec-version #1227 conflicts with this PR in 4 files (merge-tree of ci: single-source the gosec version in .gosec-version #1227 46cbf54 onto main plus this PR): .github/workflows/ci.yml, .pre-commit-config.yaml, scripts/gosec-hook.sh, scripts/security-scan.sh. ci: single-source the gosec version in .gosec-version #1227's check-gosec-pin.sh would also flag the literal GOSEC_VERSION="v2.28.0" at scripts/run-gosec.sh:31, and the sed of that pin at scripts/test-run-gosec.sh:26. Whichever PR lands second has to switch run-gosec.sh, and the test, to reading .gosec-version. Both PRs carry the same version (v2.28.0), so neither disagrees on the version today.
• minor .github/workflows/ci.yml:769 (scripts/run-gosec.sh --sarif-dir "$RUNNER_TEMP" || status=1): changing it to || true leaves scripts/test-run-gosec.sh at 17/17 PASS. The self-test checks that the step calls the script, but not that a failure reaches exit "$status". A later edit could quietly stop gosec findings from failing CI.
• nit scripts/run-gosec.sh:84: a cached binary is trusted on -x alone. The old hook re-checked go version -m against the pin. The cache key includes the version, so this only matters for a corrupted cache.
• nit process: commit 7ecc3f3 has 514 changed lines (cap 400).

Rule set before vs after (effective)

• CI before: defaults, no -exclude, no -exclude-dir. After: defaults, no -exclude. -exclude-dir=.legacy/.dev-notes/vendor is now added in CI as well. None of those directories exist, and ./... skips dot and vendor directories anyway, so CI loses no coverage. gosec logs "Excluding rules: default" for both modules.
• Hook before: 15 excludes. Makefile before: 8 excludes and exit 0 when gosec is absent. Both now use the CI set, which is stricter. No rule is dropped compared with CI.
• All three callers run scripts/run-gosec.sh (Makefile:165, gosec-hook.sh:103 via exec, ci.yml:769) at the same pin.

Attacked / local results

• bash scripts/test-run-gosec.sh: 17 PASS. shellcheck on run-gosec, test-run-gosec, gosec-hook and security-scan: clean. actionlint ci.yml: clean.
• Real gosec v2.28.0 (go1.26.9, fresh XDG cache) run on the full merged tree with --sarif-dir: rc 0, 0 results in both gosec-root.sarif and gosec-tests-e2e.sarif.
• Real hook on a planted package containing os.Remove("x") and os.ReadFile(p): G104 and G304 reported, rc 1. The old hook excluded both rules.
• Mutation probes (FAIL count from the self-test):
  • -exclude=G104 added to the args: 5
  • tests/e2e dropped from MODULES: 5
  • the hook swallows the script's exit: 1
  • exit 0 in place of exit $fail: 3
  • the Makefile calls gosec directly with -exclude: 3
  • the CI step gets || true: 0 (survivor, listed above)

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE f25ca0c

Findings (none blocking):

• [non-blocking] scripts/run-gosec.sh:54-61 - Any token after --module MOD that does not start with -- is taken as a package and passed to gosec after the flags, so -exclude=... can get in through the package slot.
  • The body's "rejects any other argument" is true only at the top level.
  • Reproduced with real gosec on a scratch package with a G104 and a G304:
    • run-gosec.sh --module . -exclude=G104,G304 ./internal/zzprobe exits 0 and logs "scanning -exclude=G104,G304 ./internal/zzprobe".
    • Without the flag it exits 1.
  • Nothing calls the script this way, and the hook always prefixes ./, so this weakens the single-rule-set guarantee without breaking it today.
  • Fix: reject PKG values that start with -, and add a test case.
• [non-blocking] scripts/test-run-gosec.sh, "hook maps staged files" - The testdata filter is never exercised. scripts/testdata/none.go does not exist, so [[ -f ]] at scripts/gosec-hook.sh:49 drops it before the */testdata/* case runs. Breaking that pattern still gives 17/17.
• [non-blocking] test-run-gosec.sh, CI-step checks - The only check is that the step calls run-gosec.sh and has no direct gosec call. Two probes passed 17/17:
  • Removing || status=1 (.github/workflows/ci.yml:744). Under the runner's bash -e, a finding would then abort the step before the SARIF merge.
  • Changing the automationDetails id (--arg id "${tag}/"), which would silently move the Code Scanning categories.
• Out of scope and admitted in the body: the gosec pin is still duplicated in .github/workflows/pre-commit.yml and the Makefile's GOSEC_VERSION. Fine as follow-ups.

Against #340: all four acceptance criteria are met, and this fixes the root cause.

• There is one script, and the hook, the Makefile, CI and security-scan.sh all call it.
• Nothing is excluded anywhere.
• make security-scan-go now scans . and tests/e2e and fails on findings, instead of exiting 0.
• Real gosec reports 0 findings on head and on the merged tree, so nothing new goes red.

Attacked:

• Blast radius:
  • security-scan.sh has set -euo pipefail, so | tee keeps gosec's exit code.
  • Nothing reads gosec-report.json.
  • The new -exclude-dir entries name directories that do not exist, and ./... already skips vendor, so CI coverage is unchanged.
• Other checks:
  • Arrays under bash 3.2 with set -u are fine, because the hook always passes at least one package.
  • The install uses an atomic rename, and the cache key includes the Go version.
  • The merged SARIF file is created after the glob runs, so the glob never picks it up.
• Mutation probes on a scratch copy, each restored, with git status clean:
  • MODULES without tests/e2e: 5 FAIL.
  • fail=1 on a finding removed: 2 FAIL.
  • fail=1 on missing SARIF removed: 1 FAILS.
  • -exclude=G104 added: 5 FAIL.
  • Hook testdata filter broken: 0 fail. Gap.
  • CI || status=1 removed: 0 fail. Gap.
  • CI id changed: 0 fail. Gap.
• Drift: main changed only the terraform job in ci.yml plus go.mod bumps, and merge-tree --write-tree origin/main <head> is clean. No migrations.

Verified locally, on git-initialised scratch copies with no cloud credentials:

• bash scripts/test-run-gosec.sh: 17 PASS. That holds on head with bash 5, on head with bash 3.2, and on the merged tree.
• Fail-before: the head's test on base gives rc 1, with 15 FAIL and 1 PASS, matching the PR body.
• shellcheck on the 4 changed scripts, actionlint on ci.yml and git diff --check: clean.
• Under the heavy lock, with real gosec v2.28.0:
  • run-gosec.sh --sarif-dir on head and on the merged tree: rc 0, with 0 results in both modules.
  • The CI step body on the merged tree: rc 0, "Merged 2 SARIF runs" (gosec-root/ and gosec-tests-e2e/).
  • A probe package with G104 and G304: the new hook exits 1 and reports both, and the old hook exits 0.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Local verification at f25ca0c638979d9000dc44559ea4917a76bc4ca2 merged onto current main (039698da; merge-tree d3df6527; macOS):

• go build ./..., go vet ./... and go vet -tags integration ./... pass.
• No Go packages changed, so no package tests were run for this step.
• The full go test -race -short ./... suite passes on the tree with every PR in this merge train applied (tree 99ee50e2).
• Nothing was applied to any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent, at IAmJSD's request, after 2 independent reviews and local verification (see the review and verification comments above, all at f25ca0c638979d9000dc44559ea4917a76bc4ca2).