fix(compliance-audit): suppress gh_api stdout on failure to fix false CODEOWNERS positive#212
fix(compliance-audit): suppress gh_api stdout on failure to fix false CODEOWNERS positive#212don-petry wants to merge 62 commits into
Conversation
…se positives When gh api returns a 404, it outputs the error JSON to stdout (not stderr). The previous gh_api() forwarded all stdout unconditionally, causing callers like check_codeowners() to receive concatenated 404 JSON blobs instead of empty strings. This made found=true for a missing file and treated the error JSON as CODEOWNERS content, triggering a false codeowners-org-leads-not-first finding. Fix: capture output into a variable and only echo it when the exit code is 0, so failed API calls produce no stdout. Closes #208 Co-authored-by: Don Petry <don-petry@users.noreply.github.com>
|
Warning Review limit reached
More reviews will be available in 59 minutes and 23 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates the ChangesGitHub CLI API Response Handling
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@petry-projects/org-leads — this PR is ready for review and merge. It fixes a false positive in the compliance audit script. |
There was a problem hiding this comment.
Pull request overview
This PR updates the gh_api() retry wrapper in scripts/compliance-audit.sh to prevent gh api stdout (including error JSON bodies from failed requests like 404s) from being forwarded to callers, which previously caused false positives in checks like check_codeowners().
Changes:
- Capture
gh apioutput insidegh_api()and only emit it when the command succeeds (exit code 0).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i in $(seq 1 $retries); do | ||
| if gh api "$@" 2>/dev/null; then | ||
| output=$(gh api "$@" 2>/dev/null) | ||
| rc=$? | ||
| if [ $rc -eq 0 ]; then | ||
| echo "$output" | ||
| return 0 | ||
| fi |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: a191bfcfa6fc014ce4d88ea3ba70860a890233d4
Review mode: triage-approved (single reviewer)
Summary
Single-line CI workflow change pinning petry-projects/.github/.github/workflows/agent-shield-reusable.yml from the mutable @v1 tag to the exact commit SHA 0cb4bba11d7563bf197ad805f12fb8639e4879e4, with the # v1 human-readable comment retained and the with: required-files: AGENTS.md input preserved. Verified via gh api repos/petry-projects/.github/git/refs/tags/v1 that the pinned SHA matches the actual commit currently behind the v1 tag. Conforms to the org action-pinning policy and follows the same pattern as the recently merged #127 (auto-rebase-reusable.yml SHA pin).
Linked issue analysis
Closes #114 — a compliance-audit finding for unpinned-actions-agent-shield.yml flagging that agent-shield.yml had 1 action not pinned to SHA. The PR addresses exactly that line; no other unpinned references remain in this workflow.
Findings
No issues found.
- SHA pin verified against upstream tag
v1(matches0cb4bba11d7563bf197ad805f12fb8639e4879e4). - Repo-specific
with:inputs preserved unchanged. - No secrets, permissions, or trigger surface modified.
- Pre-existing missing trailing newline on the file is unchanged by this PR; out of scope.
CI status
All required checks green: AgentShield, Claude Code, CodeQL (Analyze actions), Dependency audit (ecosystem detect), SonarCloud / SonarCloud Code Analysis (Quality Gate passed, 0 new issues), CodeRabbit. Dependabot auto-merge and ecosystem-specific audit jobs correctly skipped (no matching ecosystems / not a Dependabot PR). CodeRabbit posted a rate-limit notice but its status check reports SUCCESS; gemini-code-assist skipped due to unsupported file type. Mergeable: yes; merge state BLOCKED only on the required human review.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/compliance-audit.sh (1)
545-551: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUpdate comment to reflect the fixed
gh_api()wrapper behavior.The comment explains that the
gh_api()retry wrapper should not be used here because it "loops 3 times without suppressing stdout, so the captured output ends up as 3 concatenated error bodies." However, the wrapper now suppresses stdout on failure (lines 115-118), so this explanation is outdated.The directive to avoid the wrapper is still correct, but for a different reason: this function needs direct access to the error response body to distinguish 403 permission errors from other failures, and the wrapper now suppresses error output on failure.
📝 Suggested comment update
- # IMPORTANT: Do NOT use the gh_api() retry wrapper here. When gh api gets a - # 403 it outputs the error JSON body to stdout before exiting non-zero. The - # retry wrapper loops 3 times without suppressing stdout, so the captured - # output ends up as 3 concatenated error bodies — indistinguishable from a - # real "not-configured" state and causing persistent false-positive findings. - # We capture the response body and exit code separately so we can detect a - # 403 permission error and skip without filing a spurious finding. + # IMPORTANT: Do NOT use the gh_api() retry wrapper here. This function needs + # direct access to the error response body to distinguish 403 permission errors + # from other failures (404, 500, etc.). The gh_api() wrapper suppresses error + # output on failure to prevent false positives elsewhere, but here we must + # inspect the error JSON to detect "Resource not accessible by personal access + # token" (403) and skip the check rather than filing a spurious finding. + # We capture the response body and exit code separately for this purpose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/compliance-audit.sh` around lines 545 - 551, Update the block comment that explains why we must not use the gh_api() retry wrapper: change the outdated claim that the wrapper "loops 3 times without suppressing stdout" to the current behavior (the wrapper now suppresses stdout on failure) and instead state the real reason we avoid it here — the script needs direct access to the raw HTTP error response body (and exit code) so it can detect a 403 permission response versus other failures; reference gh_api() in the comment and instruct readers to call gh directly to capture the response body and exit status separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/compliance-audit.sh`:
- Around line 545-551: Update the block comment that explains why we must not
use the gh_api() retry wrapper: change the outdated claim that the wrapper
"loops 3 times without suppressing stdout" to the current behavior (the wrapper
now suppresses stdout on failure) and instead state the real reason we avoid it
here — the script needs direct access to the raw HTTP error response body (and
exit code) so it can detect a 403 permission response versus other failures;
reference gh_api() in the comment and instruct readers to call gh directly to
capture the response body and exit status separately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a448ef2-a7ce-44f7-a366-a5fb886703ab
📒 Files selected for processing (1)
scripts/compliance-audit.sh
|
@dev-lead - please fix this PR |
|
Summary
codeowners-org-leads-not-firstcompliance finding inscripts/compliance-audit.shgh_api()retry wrapper was forwardinggh apistdout unconditionally — including the error JSON body emitted on a 404 responsecheck_codeowners()received concatenated 404 JSON blobs instead of an empty string, makingfound=truefor a missing file and treating the error JSON as CODEOWNERS contentThe CODEOWNERS file at
.github/CODEOWNERSwas already correct (* @petry-projects/org-leads); no CODEOWNERS change is needed.Test plan
codeowners-org-leads-not-firstfinding should be absentgh_apicall sites are unaffected (callers that use|| echo ""or redirect stdout to/dev/nullstill work correctly)Closes #208
Generated with Claude Code
Summary by CodeRabbit