Repository navigation
ci: share gosec policy across local and CI scans - #2153
Conversation
cristim
added
triaged
|
Warning Review limit reached
This review includes 7 billable files and costs up to $1.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 54 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 78 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe pull request adds a shared gosec runner for package scans, Makefile targets, and CI. The runner validates options, installs and verifies gosec v2.28.0, and handles report output. CI checks the generated SARIF structure. Changesgosec scanning
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~22 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions CI workflow
participant Runner as scripts/run-gosec.sh
participant Gosec as gosec
participant SARIF as SARIF validation
CI->>Runner: request SARIF scan
Runner->>Gosec: scan targets and write report
Gosec-->>Runner: report and exit status
Runner-->>CI: report and exit status
CI->>SARIF: validate version, runs, drivers, and results
Merge Risk: 🔵 Low · up to The change appears mergeable. Local contract-test runs retain temporary files, but no material scanning failure was established. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments (7)
scripts/run-gosec.sh (1)
1-65: LGTM!scripts/gosec-hook.sh (1)
4-18: LGTM!Makefile (1)
87-87: LGTM!Also applies to: 106-106
.pre-commit-config.yaml (1)
160-161: LGTM!.github/workflows/pre-commit.yml (1)
47-53: LGTM!Also applies to: 149-149
.github/workflows/ci.yml (1)
378-388: LGTM!scripts/run-gosec-test.sh-5-5 (1)
5-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Clean up the scratch directory with an
EXITtrap.The script creates
scratchwithmktemp -dbut never removes it. This happens on success and on failure. Every local or CI run leaves behind a fakegobinary, a scanner, a gosec cache, and the reports. Register cleanup right after you create the directory. If you want to keep the directory for debugging, delete it only on success, or gate the deletion behind an environment variable.Proposed fix
scratch=$(mktemp -d) +trap 'rm -rf "$scratch"' EXITBased on learnings: "any temporary file or directory created with mktemp or similar must be cleaned up via a trap ... set immediately after creation."
Source: Learnings
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
- Review profile: CHILL
- Plan: Essentials
- Run ID:
6d86066c-a5f3-4339-9d98-af1ebc1c724e
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/pre-commit.yml.pre-commit-config.yamlMakefilescripts/gosec-hook.shscripts/run-gosec-test.shscripts/run-gosec.sh
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.
|
@coderabbitai full review |
|
|
Independent review is clean at Independent real-path checks confirm the Bash 3.2 contract, directory-output rejection, and exact CI block's report/failure behavior. Removing the guard makes the regression fail. Actual scanner G304 fixture outputs agree across hook/Make/CI; strict repository JSON/SARIF scans agree. Fixture report-error probes are labeled separately from real scans. Pinned workflow commit hooks passed normally. Fresh exact-head CI runs37728064787 and37728064679 both succeeded, including unit/integration/security/workflow checks. Working tree is clean and PR merge state is CLEAN. CodeRabbit explicitly returned quota exhaustion, so its success check is not a clean review. Owner-authorized quota waiver applies with independent adversarial review, local verification, and green CI; retrospective review stays on the coordinator's existing cadence. Scope remains the three standalone callers. Embedded golangci policy assessment is tracked in #2152; existing local full-suite capture deadlock remains #2150. No protection bypass or live-cloud purchase. |
|
Merged normally as Issue1394 is closed. All source branches, tests, metadata, and verification artifacts were preserved. CodeRabbit review was quota-waived with independent evidence; retrospective full review is recorded for the coordinator's existing cadence. Embedded gosec assessment remains #2152. Local act simulation hit GitHub408 twice; actual CI and exact local run-block evidence passed. |
1 participant
The changed-package hook suppressed 15 gosec rules, Make suppressed eight, and CI suppressed none. All three now use one wrapper that owns the pinned scanner and strict default policy. The hook retains selected-package coverage; Make and CI scan the root module and retain JSON/SARIF reports and failure statuses.
The wrapper verifies cached module build metadata and rejects missing reports and directory outputs. A Bash 3.2-compatible contract harness runs in CI. Unused duplicate installation/version declarations and obsolete multi-module SARIF merging are removed.
Validation: two independent implementation and two staged reviews; normal commit hooks including pinned actionlint and zizmor; shellcheck; realistic G304 fixture through the actual hook, Make target, and exact CI block. Before: hook/Make passed while CI failed. After: all three reject the same
(G304, read.go, 3)finding. Clean fixture and full repository JSON/SARIF scans passed with matching empty findings. Pinned Go build and scoped SDK race tests passed.The existing local full-suite failure #2150 remains separate. Embedded golangci-lint gosec settings are outside this three-caller change and tracked in #2152. No live cloud account was required.
Closes #1394
Summary by CodeRabbit