feat(scripts): SHA-pin tooling Suggested-tier follow-up (#176 review)#177
Conversation
- REPO_LIMIT now reads from environment with default 1000:
`REPO_LIMIT="${REPO_LIMIT:-1000}"`. Allows tests to exercise the
saturation WARN with a small stub-friendly value and gives operators
an escape hatch for fleets that grow beyond the default.
- New STRICT_AUDIT=1 mode: track an audit_incomplete flag during the
run. When STRICT_AUDIT=1 is set in the environment and any repo
emitted a `repo,error` row OR REPO_LIMIT saturated, exit 2 with a
diagnostic stderr line. The default flow remains report-only (exit 0)
so existing CI consumers do not break; CI gates that need fail-closed
semantics opt in by setting STRICT_AUDIT=1.
- Header comment updated with both env vars under a new
"Environment overrides" block.
Addresses Suggested findings S-7 (STRICT_AUDIT fail-closed mode) and
S-1 partial (env override for REPO_LIMIT) from the /pr-review of #176.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When the GitHub API returns a tag-ref response missing `.object.type` or `.object.sha`, jq -r emits the literal string "null". The previous code accepted that as a successful resolution and produced patterns like `actions/checkout@null` which the caller then wrote into the workflow file (replacing the original tag-pinned ref with a broken SHA pin). After both the initial composite fetch and the second annotated-tag dereferencing call, validate that the returned fields are non-empty and not the literal "null". Return empty (the existing failure convention) so the caller marks the action SKIPPED rather than writing junk. Also clarify the sed-delimiter assumption in the safe_tag escape block: the comment now explicitly names the three replacement-side specials (\, &, |) and notes that the | escape is tied to the sed delimiter choice; if the delimiter ever changes, this set must be updated. Addresses Suggested findings S-6 (annotated-tag .object.sha guard) and S-9 (sed-delimiter documentation) from the /pr-review of #176. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds nine new bats cases guarding the production fixes in this branch and the residual coverage gaps the /pr-review of #176 surfaced: fleet-audit-sha-pins.bats: - "audit warns when gh repo list saturates REPO_LIMIT": exercises the saturation WARN with REPO_LIMIT=3 + a 3-repo stub - "STRICT_AUDIT=1 exits non-zero on REPO_LIMIT saturation": verifies the new fail-closed exit-2 path on saturation - "STRICT_AUDIT=1 exits non-zero when any repo emits error sentinel": verifies fail-closed exit-2 path on per-repo API failure - "STRICT_AUDIT=1 remains exit 0 on a fully clean run": regression guard so STRICT_AUDIT does not break healthy audits - "--skip-owners with a single owner (no commas)": verifies the case framing handles a single-value SKIP_OWNERS (",actions,") - "--skip-owners with empty string does not skip anything": empty SKIP_OWNERS produces "," framing that matches no owner - "--skip-owners with double internal commas still matches listed owners": noisy empty segments do not break the match update-pinned-actions.bats: - "--pin-tags reports branch refs that carry an inline comment": extract_branch_pins must surface a comment-annotated branch ref (previously only the tag-extraction path was tested with comments) - "--pin-tags --apply on a workflow with no third-party refs succeeds without warnings": first-party-only fixture exercises the rc-aware extract_*_pins zero-match path under set -euo pipefail - "--pin-tags --apply skips when annotated-tag dereferencing returns missing .object.sha": stubs the second-step call to return an object without .sha; asserts the new null guard marks the action SKIPPED and never writes "actions/checkout@null" into the workflow file Addresses Suggested findings S-1 (REPO_LIMIT test), S-2 (branch-pin inline-comment test), S-3 (zero-match fixture), S-5 (SKIP_OWNERS edges), S-6 test (annotated-tag null), and S-7 tests (STRICT_AUDIT) from the /pr-review of #176. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Add ### Added entry for the new STRICT_AUDIT=1 environment toggle and the env-overridable REPO_LIMIT - Add ### Fixed entry for the resolve_tag_sha guard against jq's literal "null" output when a tag-ref response is missing object fields - Add ### Fixed entry summarizing the nine new bats cases added in this branch covering Suggested-tier regression vectors Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 39 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Follow-up hardening for the SHA-pin tooling scripts, addressing Suggested-tier review findings by adding fail-closed audit behavior, tightening tag resolution correctness, and expanding Bats regression coverage.
Changes:
- Add
STRICT_AUDIT=1fail-closed mode and env-overridableREPO_LIMITtofleet-audit-sha-pins.sh, plus new regression tests for saturation and skip-owner edge cases. - Prevent
update-pinned-actions.shfrom acceptingjq -r"null"values during tag SHA resolution, plus add tests for annotated-tag edge cases and zero third-party workflows. - Document the new behaviors in
CHANGELOG.mdand clarify sed delimiter escape semantics.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/fleet-audit-sha-pins.sh | Add STRICT_AUDIT and env-overridable REPO_LIMIT, track incompleteness to optionally fail closed. |
| scripts/update-pinned-actions.sh | Add "null" guards in resolve_tag_sha, clarify sed replacement escaping documentation. |
| tests/fleet-audit-sha-pins.bats | Add coverage for REPO_LIMIT saturation warnings, STRICT_AUDIT exit behavior, and skip-owner edge cases. |
| tests/update-pinned-actions.bats | Add coverage for branch refs with inline comments, zero third-party refs, and annotated-tag dereference missing .object.sha. |
| CHANGELOG.md | Document STRICT_AUDIT and REPO_LIMIT behavior and new regression tests. |
| if [[ "$api_error" == true ]]; then | ||
| emit "$full,error" | ||
| audit_incomplete=true | ||
| else | ||
| emit "$full,$violations" |
| REPO_LIMIT=1000 | ||
| # REPO_LIMIT defaults to 1000 but may be overridden via environment for tests | ||
| # and for fleets that grow beyond the default. | ||
| REPO_LIMIT="${REPO_LIMIT:-1000}" |
PR ReviewComprehensive Critical
Important
Suggested + Informational (10 additional findings)Test gaps for the first-call null-guard path, Other reviewer signals
Recommended sequencingFix Critical #1 and #2 together. Fix #2 alone and the existing STRICT_AUDIT error-sentinel test will start failing CI, confirming Critical #1. Triggering 🤖 Generated with Claude Code |
… and CI Critical - scripts/fleet-audit-sha-pins.sh: the workflows-listing failure path emitted a "$full,error" sentinel and continued without setting audit_incomplete=true, so STRICT_AUDIT=1 exited 0 even when error rows were present. Verified by manual reproduction with a 429-stub before the fix. New bats case "exits non-zero on workflows-listing failure (Critical #1 regression)" is the regression guard. - .github/workflows/shell-tests.yml: bats runner only executed tests/update-pinned-actions.bats; the 13 cases in tests/fleet-audit-sha-pins.bats (including all STRICT_AUDIT and REPO_LIMIT tests from PR #176 and the new ones in PR #177) never ran in CI. Switched to find + xargs over tests/*.bats, which is glob-injection-safe and discovers new test files automatically. This is the reason Critical #1 escaped detection until self-review. Important - scripts/fleet-audit-sha-pins.sh: mapfile -t repos < <(gh repo list ...) discarded the gh exit code via process substitution; auth-loss or rate-limit mid-enumeration silently contributed zero rows. Now captures stdout to a tmpfile and surfaces the rc, flagging audit_incomplete on failure. - scripts/fleet-audit-sha-pins.sh: REPO_LIMIT validated as positive integer at script entry; previously a non-numeric value crashed inside [[ -eq ]] under set -euo pipefail. Exits 1 with a clear error message instead. - scripts/update-pinned-actions.sh: null-guard paths in resolve_tag_sha now emit a stderr WARN before returning empty so operators can distinguish a malformed GitHub API response from a missing tag. - scripts/fleet-audit-sha-pins.sh: STRICT_AUDIT accepts truthy spellings 1, true, yes (case-insensitive). Previously only "1" worked and STRICT_AUDIT=true silently degraded to report-only. Suggested - scripts/fleet-audit-sha-pins.sh: strip trailing CR from each line before regex matching so workflow files with CRLF endings do not leak \r into BASH_REMATCH and inflate violation counts. - tests/fleet-audit-sha-pins.bats: 8 new regression tests covering the above paths. - tests/update-pinned-actions.bats: 1 new test for the first-call null guard in resolve_tag_sha (the second-call path was already covered); strengthened branch-with-comment assertion to anchor on the "WARN: branch ref detected" header; added positive controls to the empty-SKIP_OWNERS test. - scripts/update-pinned-actions.sh: maintenance note that changing the sed delimiter requires updating both the replacement-side escape set and the pattern-side escape_sed_pat() function. - .claude/CLAUDE.md: corrected stale statement that bats coverage is limited to scripts/update-pinned-actions.sh. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
PR Fix SummaryAddressed all Critical and Important findings from the Critical (both fixed)
Important (all fixed)
Suggested (6 of 7 fixed)
Test coverage
CI re-run triggered by push. The 🤖 Generated with Claude Code |
Summary
Addresses the 9 Suggested-tier findings from the
/pr-reviewof #176 that were deferred from the Important-tier scope. All changes are mechanical hardening or test additions; no breaking changes to the default flow.Builds directly on the merged
v1.4.2(PR #176).Findings addressed
Added
scripts/fleet-audit-sha-pins.sh): the audit script now supportsSTRICT_AUDIT=1to fail closed (exit 2) when the run is incomplete (per-reporepo,errorrow or REPO_LIMIT saturation).REPO_LIMITis now env-overridable (REPO_LIMIT=\"\${REPO_LIMIT:-1000}\") so tests can exercise the saturation WARN without a 1000-repo stub. Default flow remains report-only (exit 0).Fixed
scripts/update-pinned-actions.sh):resolve_tag_shanow validates that.object.typeand.object.shaare non-empty and not the literal string\"null\"(whichjq -remits for missing fields). The previous code accepted\"null\"as a successful resolution and produced patterns likeactions/checkout@nullthat the caller wrote into the workflow file. Guard fires after both the composite first-call and the annotated-tag second-call.scripts/update-pinned-actions.sh): thesafe_tagescape block's inline comment now explicitly names the three replacement-side specials (\\,&,|) and notes that the|escape is tied to the sed delimiter choice. The pattern-side escape lives inescape_sed_pat(added in PR fix(scripts): SHA-pin tooling robustness follow-up (#175 review) #176), already documented.Tests
REPO_LIMIT=3+ 3-repo stubextract_branch_pinsreports a comment-annotated branch ref (previously only the tag path was tested with comments)--pin-tags --applysucceeds with a fixture containing only first-party refs (verifies the rc-aware extractor zero-match path underset -euo pipefail).object.shaFindings re-scoped after PR #176 merge
{ ...; } || truewrapper): the|| truewrappers were removed by PR fix(scripts): SHA-pin tooling robustness follow-up (#175 review) #176 in favor of explicit grep rc handling. The Suggested-tier test now verifies the new rc-aware path returns 0 cleanly when the first grep matches lines but the awk filter zeroes them all (all-first-party fixture). Same coverage intent, different mechanism.${VAR:+\"\$VAR\"}expansion). Any existing legacy-path test exercises the trap; an explicit leak-counter test was judged net-negative because temp-file inspection is fragile across bats environments.detect-secrets(and TruffleHog) per.pre-commit-config.yaml;# pragma: allowlist secretis the correct annotation. No code change needed; not in scope of this PR.Test plan
pre-commit run --files <changed>passes (trailing whitespace, EOL, secrets, em-dash all clean)shellcheckclean on both modified scripts (only the pre-existing SC2001 style note on line 250 remains, unchanged by this branch)Notes
tests/fleet-audit-sha-pins.bats, 3 intests/update-pinned-actions.bats, plus the 2 STRICT_AUDIT diagnostic variants). Full suite is now 52 -> 61.🤖 Generated with Claude Code