fix(action): restore set +e so a findings exit (1) doesn't abort unde…#12
Closed
cemililik wants to merge 1 commit into
Closed
fix(action): restore set +e so a findings exit (1) doesn't abort unde…#12cemililik wants to merge 1 commit into
cemililik wants to merge 1 commit into
Conversation
…r bash -e GitHub runs `shell: bash` steps with -e (`bash --noprofile --norc -e -o pipefail`). leakwatch legitimately exits 1 when it reports findings, so the scan aborted the step *before* the exit-code mapping — meaning fail-on-findings: false was ignored and the action failed on any findings. (The pre-rewrite action had `set +e` here; it was dropped during the Marketplace rewrite.) - action.yml: `set +e` before the leakwatch call; the script maps 0/1/>=2 itself. - action-test.yml (cli-github-format): `|| true` on the `out=$(leakwatch …)` capture so it doesn't abort under -e either. Verified by reproducing GitHub's `bash -e -o pipefail`: without the fix the mapping is skipped and the step exits 1; with it the mapping runs and the step honors fail-on-findings. The run-action self-test (fail-on-findings: false) and cli-github-format job are the regression guards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR adjusts how the GitHub Action and its test workflow handle leakwatch’s non-zero exit code under GitHub’s default Sequence diagram for GitHub Action handling leakwatch exit codes under bash -esequenceDiagram
participant bash
participant leakwatch
bash->>bash: set_plus_e
bash->>leakwatch: leakwatch ARGS
leakwatch-->>bash: exit 0_or_1_or_ge_2
bash->>bash: EXIT_CODE=$?
alt [fail-on-findings is false]
bash->>bash: exit 0
else [fail-on-findings is true]
bash->>bash: exit EXIT_CODE
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGitHub Actions exit-code handling for the leakwatch binary is improved across two configuration files. The composite action disables errexit before invoking leakwatch, and the test workflow appends ChangesLeakwatch exit-code handling in GitHub Actions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 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)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
action.yml, consider re-enablingset -eafter theleakwatchinvocation so that failures in any subsequent commands in the step are not silently ignored now that errexit is globally disabled. - In the
action-test.ymljob, the|| truewrapper will swallow any non-1 failure modes fromleakwatchas well; if you care about distinguishing real errors from expected findings in the test, it may be safer to capture$?and explicitly allow only the expected exit codes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `action.yml`, consider re-enabling `set -e` after the `leakwatch` invocation so that failures in any subsequent commands in the step are not silently ignored now that errexit is globally disabled.
- In the `action-test.yml` job, the `|| true` wrapper will swallow any non-1 failure modes from `leakwatch` as well; if you care about distinguishing real errors from expected findings in the test, it may be safer to capture `$?` and explicitly allow only the expected exit codes.
## Individual Comments
### Comment 1
<location path="action.yml" line_range="292-297" />
<code_context>
# Do NOT echo the assembled args: path/extra-args may carry credentials
# (tokens, authenticated URLs) that GitHub log masking would not catch.
echo "Running leakwatch scan (type=${INPUT_SCAN_TYPE}, format=${INPUT_FORMAT})"
+ # GitHub invokes bash with -e; findings legitimately exit 1, so disable
+ # errexit around the scan and map the exit code ourselves below (otherwise
+ # the step would abort here and fail-on-findings: false would be ignored).
+ set +e
leakwatch "${ARGS[@]}"
EXIT_CODE=$?
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider limiting the scope of `set +e` or restoring `-e` after the scan to avoid changing error-handling semantics for subsequent commands.
`set +e` here affects all subsequent commands in this step, so any later failures that should be fail-fast will be ignored.
Either:
- Re-enable `errexit` right after capturing the exit code (e.g., `set -e`), or
- Avoid changing global options by wrapping the scan in a conditional, for example:
```bash
if ! leakwatch "${ARGS[@]}"; then
EXIT_CODE=$?
else
EXIT_CODE=0
fi
```
This preserves the special handling for the scan without weakening error handling for the rest of the script.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
Thanks @sourcery-ai — both points are valid and applied in #13:
Note: the underlying |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…r bash -e
GitHub runs
shell: bashsteps with -e (bash --noprofile --norc -e -o pipefail). leakwatch legitimately exits 1 when it reports findings, so the scan aborted the step before the exit-code mapping — meaning fail-on-findings: false was ignored and the action failed on any findings. (The pre-rewrite action hadset +ehere; it was dropped during the Marketplace rewrite.)set +ebefore the leakwatch call; the script maps 0/1/>=2 itself.|| trueon theout=$(leakwatch …)capture so it doesn't abort under -e either.Verified by reproducing GitHub's
bash -e -o pipefail: without the fix the mapping is skipped and the step exits 1; with it the mapping runs and the step honors fail-on-findings. The run-action self-test (fail-on-findings: false) and cli-github-format job are the regression guards.Summary by Sourcery
Ensure the GitHub Action correctly handles leakwatch's exit codes when running under bash -e so that fail-on-findings behaves as configured instead of always failing on findings.
Bug Fixes:
CI:
Summary by CodeRabbit