fix(ci): unmask RE005 exit-masking steps; annotate fail-closed (#942) - #1077
Open
hyperpolymath wants to merge 2 commits into
Open
hyperpolymath wants to merge 2 commits into
hyperpolymath wants to merge 2 commits into
Conversation
Hypatia RE005 flagged 20 workflow steps across 10 files for silently swallowing non-zero exit via `|| true` / `continue-on-error: true`. Each instance was read in context (the step itself plus what runs after it) and classified as a real mask (A, fixed) or a deliberate fail-closed design (B, left alone and annotated with an inline `# hypatia: allow` pragma). 1. affinescript-verify.yml "Checkout AffineScript compiler" -- advisory annotation added (fails OPEN, not B: best-effort checkout while BLOCKING is false; "Verify changed .affine files" surfaces an explicit ::warning:: instead of a silent green claim). 2. changelog-reusable.yml "Mode = check-only -- verify no drift" -- A: removed inert `|| true` (pipe ends in `head -60`; the following `exit 1` fails the job either way). 3. ci-pipeline.yml "Checkout the pinned Standards Deno ledger" -- B: annotated fail-closed; "Refuse Deno unless this repository is ledgered" checks for the ledger file and exits 1 with ::error:: if missing. 4. echidna-verify.yml "Type-check proofs" -- A: rewrote so a real `agda --safe` failure is detected (was previously unreachable under `tee`'s exit status plus a redundant `|| true`) and surfaced via ::warning::, kept advisory since the proof corpus is currently evicted (issue #748) and re-entry criteria aren't set. 5-14. governance-reusable.yml (ten instances): - "Check banned-language files" -- A: removed inert `|| true` on `git ls-files`-terminated captures (RES/GO/SWIFT/DART/VMOD), narrowed to `|| [ $? -eq 1 ]` on `grep`-terminated captures (PY/MAKE/JAVA) so a real grep error (exit 2) still trips `-e`. - "Check for npm/yarn artifacts" -- A: removed 4 inert `|| true` (`git ls-files`/`find`-terminated). - "Security checks" -- A: removed 3 inert `|| true` (`head`-terminated). - "Check file permissions" / "Check TODO/FIXME" / "Check for large files" -- A: dropped `continue-on-error: true` entirely (each step already always exits 0) and now emit an explicit ::warning:: instead of a silent printout. - "EditorConfig check" -- advisory annotation added (fails OPEN, not B: a follow-up step surfaces ::warning:: on failure; repos opt into blocking locally). - "Mixed content check" -- A: removed inert `|| true` (`head`-terminated). - "Checkout the pinned Standards policy helpers" -- B: annotated fail-closed; "Duplicate YAML keys in workflows" refuses to run (::error:: + exit 1) when neither script copy is present. - "Check locked or SHA-pinned actions" -- A: narrowed `|| true` to `|| [ $? -eq 1 ]` (`grep -cve`-terminated; an empty ledger file is an expected, not error, case). 15. hypatia-scan-reusable.yml "Check out standards for the SARIF baseline filter" -- advisory annotation added (fails OPEN, not B: the fallback uploads the SARIF unfiltered, which can only show more alerts). 16. readme-derive-reusable.yml "Freshness check (fail-and-tell)" -- A: narrowed `|| true` to `|| [ $? -eq 1 ]` on a bare `diff` display command; the following regen instructions + `exit 1` still fire. 17. scorecard-enforcer.yml "Check for pinned dependencies" -- A: removed inert `|| true` (`head`-terminated; already emits ::warning:: itself). 18-19. secret-scanner-reusable.yml "Check for hardcoded secrets in Rust" / "... in shell scripts" -- A: narrowed `grep`-terminated captures to `|| [ $? -eq 1 ]`, removed one inert `sed -n`-terminated `|| true`. 20. security-gate-pr-target.yml "Extract PR branch for safe checkout" -- A: the most severe finding. Removed the same-named-base-repo-branch fallback (which could let a malicious fork PR's real content go unscanned while reporting success) and converted silent tolerance of fetch/checkout failure into explicit ::error:: + exit 1. Validation: `ruby -ryaml -e 'YAML.load_file(...)'` and `yq .` both pass cleanly on all 10 files. `actionlint` exits 1 but every finding (shellcheck info/style/warning notes and pre-existing `job.workflow_sha` property errors) is confirmed pre-existing and unrelated to these edits -- zero new findings introduced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFphKkDVB9pUDSCD4bkz65
The Nickel import scan's `|| true` absorbs grep's exit 1 on a file with no imports; the ledger checkout's continue-on-error feeds a Verdict step that treats unreadable == empty == blocked. Local scan: RE005 22 -> 0, total findings 150 -> 128. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFphKkDVB9pUDSCD4bkz65
Contributor
|
Warning Review limit reachedNext included review available in 27 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (10)
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 |
4 tasks
This branch has not been deployed
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.
Closes #942.
Hypatia RE005 flags steps that silently swallow a non-zero exit (
|| true,continue-on-error: true). I read every flagged step in context, together with the step that runs after it, and sorted each one into one of three groups:# hypatia: allow research_extensions/RE005 -- <reason>pragma explaining why.The per-step reasoning is in the commit message of b2f688c.
Measured locally (hypatia escript from hypatia#883, no Actions minutes)
bd9313a6Most important fix
security-gate-pr-target.yml"Extract PR branch for safe checkout" had two problems:It now fails closed on both, using
::error::+exit 1.Checked for regression risk
governance-reusable.ymlruns across the whole estate, so I checked its changes against this risk: dropping|| truewould break any step running underpipefailwhose pipeline ends ingrep -v(exit 1 when nothing matches) orfind | head(findgets SIGPIPE).None of the changed steps sets a
shell:key, there is nodefaults:block, and none sets pipefail in its body. They all run under GitHub's defaultbash -e {0}, where a pipeline's status is that of its last command (head→ 0), so those|| truewere inert. Where a pipeline ends ingrep, the mask is narrowed to|| [ $? -eq 1 ]rather than removed. That way a real grep error (exit 2) still fails the step.Two more checks:
actionlint: base and branch report the same 4 pre-existing warnings (SC2317, SC2153, and 2×job.workflow_sha, which this actionlint version doesn't know). No new warnings.ci-pipeline.ymlare annotated in 8e0f96b:|| trueonly to absorb grep's exit 1 on a file with no imports.continue-on-errorfeeds a Verdict step that treats unreadable, empty and blocked the same way.For the owner: advisory steps that fail open by design
Each of these is annotated rather than changed. Say if you want any of them made blocking.
affinescript-verify.yml"Checkout AffineScript compiler": a best-effort checkout whileBLOCKINGis false. It surfaces a::warning::.governance-reusable.yml"EditorConfig check": a new follow-up step now surfaces a::warning::when it fails. Repos opt into blocking locally.hypatia-scan-reusable.yml"Check out standards for the SARIF baseline filter": the fallback uploads the SARIF unfiltered, which can only show more alerts.echidna-verify.yml"Type-check proofs": before this change, a realagda --safefailure was unreachable (hidden behindtee's exit status plus|| true). It is now detected and reported as a::warning::. It stays advisory while the proof corpus is evicted (echidna-verify: required contextIdris2 — a2ml proofsis vacuously green on every PR and red on every cron #748).🤖 Generated with Claude Code
https://claude.ai/code/session_01QFphKkDVB9pUDSCD4bkz65