Route ruleset_id()'s Per-Page Guard Through jqr - #1972
Merged
Merged
Conversation
## Summary Next in the #1123/#1234/#1246/#1247/#1253/#1254/#1880/#1881/#1882 chain. `repo-config/configure.sh`'s `ruleset_id()` guards its own single-fetch assumption with a plain `jq`, whose output on a native Windows jq carries a trailing `\r` that breaks the guard's own integer comparison. ## The defect ```sh if [ "$(jq 'length' <<<"$out")" -eq 100 ]; then ``` On a native Windows jq, the substitution yields `100\r`. `[ "100\r" -eq 100 ]` reaches bash's arithmetic comparison with a non-numeric string, which prints `integer expression expected` to stderr and evaluates false. `[`'s non-zero status here is the condition of an `if`, which bash never treats as a triggering failure under `set -e`, so the guard silently reads as false at exactly the boundary it exists to catch, and its own "100 rulesets returned" stderr line never fires. A repo that has hit the 100-ruleset per_page cap would then fall through to the single-fetch id lookup instead of aborting, which can make `apply_ruleset()` create a duplicate ruleset by name. ## Fix Route the extraction through the existing `jqr()` helper (`jqr() { jq -r "$@" | sed $'s/\r$//'; }`), matching the treatment the rest of `ruleset_id()` already gets after #1253, and matching the existing `count="$(jqr 'length' <<<"$entries")"` pattern elsewhere in this file. No behavior change on a CR-free `jq` (Linux/macOS). ## Verification - `shellcheck` and `shfmt -d` both clean on `repo-config/configure.sh`. - `python3 scripts/prose_lint.py .`, `python3 scripts/repo_gate.py`, `python3 spec/validate.py`, and `python3 -m unittest discover -s tests` (1770 tests) all pass. - A standalone probe (bash, no network) shims `jq` to append a trailing CR (the documented native-Windows behavior), then shows the unfixed `jq 'length'` extraction fails the `-eq` comparison with a silent "integer expression expected" (the guard never fires), while the `jqr`-routed extraction strips the CR and the guard fires correctly, and that a real (CR-free) `jq` is unaffected by the fix either way. **This defect and its fix are Windows-only, and I cannot reproduce or verify the actual Windows-jq CRLF behavior on this (Linux) host.** The verification above proves the CR-stripping mechanism is correct and unchanged on Linux/macOS; it does not and cannot prove the fix resolves the issue on a real Windows host. That remains unverified here, same as #1123/#1229/#1234/#1246/#1247/#1253/#1254. Closes on promotion: #1880 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ptr727/ProjectTemplate/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesRuleset lookup
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to The change addresses carriage returns in the ruleset count without introducing a new failure; the existing 100-item rejection is unchanged. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1972 +/- ##
==========================================
Coverage ? 55.41%
==========================================
Files ? 16
Lines ? 7247
Branches ? 0
==========================================
Hits ? 4016
Misses ? 3231
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is a minimal, targeted swap from jq to the existing jqr() normalization helper in a guard path, with no functional change expected on CR-free platforms.
Review effort: Lite
Findings: None
What changed in this PR
This PR fixes a Windows-specific failure mode in repo-config/configure.sh where ruleset_id()'s per_page=100 guard could silently fail due to a native Windows jq emitting a trailing \r, causing the numeric -eq comparison to misbehave.
Changes:
- Route the
ruleset_id()lengthextraction through the existingjqr()helper so trailing\ris stripped before the integer comparison.
| File | Description |
|---|---|
| repo-config/configure.sh | Uses jqr() for the length guard to avoid CRLF-tainted numeric comparisons on Windows. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Sep 28, 2026
ptr727
added a commit
that referenced
this pull request
Sep 28, 2026
…1988) ## Summary `repo-config/configure.sh`'s `ruleset_id()` per_page guard fails open on any non-array 2xx response from the rulesets API, not only the CR-mangled case #1880/#1972 fixed, because the guard's own comparison failure inside its `$(...)` subshell never reaches `set -e` (no `inherit_errexit`). ## The defect On a non-array response, `jqr 'length'` prints nothing, so the `[ "" -eq 100 ]` guard fails silently rather than tripping, and the following `ids=` extraction likewise falls through empty rather than aborting. `ruleset_id` returns 0, which both call sites read as "not found": the apply site would create a duplicate ruleset by name, and the check site would misreport "not found" instead of aborting. ## Fix Validate `$out` is a JSON array (`jq_has 'type == "array"' <<<"$out"`, matching the `entries` guard's own pattern) before either the per_page guard or the `ids=` extraction reads it, aborting with a "could not read live ruleset state" message on failure. ## Verification - New `tests/test_configure_ruleset_id.py` lifts `ruleset_id()`'s own lines and exercises it through a stubbed `gh`, invoked the way both real call sites invoke it (`id="$(ruleset_id ...)"`), which is what exposes the subshell-errexit defect. - Confirmed by reverting the fix: every non-array-response case then returns exit 0 with empty stdout (the "not found" misread), and the pre-existing per_page-cap, gh-failure, and duplicate-name cases stay passing throughout. - `shellcheck` and `shfmt -d` clean on `repo-config/configure.sh`. - `python3 -m unittest discover -s tests` (1777 tests), `python3 scripts/repo_gate.py`, and `python3 spec/validate.py` all pass. - A local-strict-review pass over the full diff found no blocking defects. It flagged one comment-wording inaccuracy (fixed) and one pre-existing gap in the `ids=` extraction for a malformed array element, filed separately as #1987 rather than expanding this fix's scope. Closes on promotion: #1971 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
ptr727
added a commit
that referenced
this pull request
Sep 28, 2026
… and Setup Tooling (#2000) ## Summary Promotes develop to main, carrying the 16 pull requests merged to develop since #1943. - **Wait-loop guard (requirement 7):** [#1999](#1999) credits a comparison bound only inside a `[`, `[[`, or `test` invocation, and [#1996](#1996) reconciles the requirement's README count and diagram with its hook. - **Registry:** [#1994](#1994) and [#1976](#1976) record Vantage-Config's line endings and description. - **Scripts and tooling:** - [#1988](#1988) and [#1972](#1972) harden `ruleset_id()`. - [#1974](#1974) and [#1949](#1949) fix `pr_review.py` `reply --match` and `wait`. - [#1969](#1969), [#1964](#1964), [#1954](#1954) and [#1951](#1951) fix host-setup tool shadowing, shims, hook ownership and dpkg ownership checks. - **Gates and audit:** - [#1980](#1980) triages a path collision. - [#1967](#1967) and [#1962](#1962) tighten sha-pin and version-literal checks. - [#1956](#1956) keeps a folded `if:` visible to the interface audit. Closes #1636 Closes #1639 Closes #1948 Closes #1971 Closes #1718 Closes #1934 Closes #1877 Closes #1880 Closes #1865 Closes #1889 Closes #1966 Closes #1906 Closes #1935 Closes #1901 Closes #1905 Closes #1866 Closes #1897 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
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.
Summary
Next in the #1123/#1234/#1246/#1247/#1253/#1254/#1880/#1881/#1882 chain.
repo-config/configure.sh'sruleset_id()guards its own single-fetchassumption with a plain
jq, whose output on a native Windows jq carries atrailing
\rthat breaks the guard's own integer comparison.The defect
On a native Windows jq, the substitution yields
100\r.[ "100\r" -eq 100 ]reaches bash's arithmetic comparison with a non-numeric string, which prints
integer expression expectedto stderr and evaluates false.['s non-zerostatus here is the condition of an
if, which bash never treats as atriggering failure under
set -e, so the guard silently reads as false atexactly the boundary it exists to catch, and its own "100 rulesets returned"
stderr line never fires. A repo that has hit the 100-ruleset per_page cap
would then fall through to the single-fetch id lookup instead of aborting,
which can make
apply_ruleset()create a duplicate ruleset by name.Fix
Route the extraction through the existing
jqr()helper(
jqr() { jq -r "$@" | sed $'s/\r$//'; }), matching the treatment the restof
ruleset_id()already gets after #1253, and matching the existingcount="$(jqr 'length' <<<"$entries")"pattern elsewhere in this file. Nobehavior change on a CR-free
jq(Linux/macOS).Verification
shellcheckandshfmt -dboth clean onrepo-config/configure.sh.python3 scripts/prose_lint.py .,python3 scripts/repo_gate.py,python3 spec/validate.py, andpython3 -m unittest discover -s tests(1770 tests) all pass.
jqto append a trailing CR(the documented native-Windows behavior), then shows the unfixed
jq 'length'extraction fails the-eqcomparison with a silent"integer expression expected" (the guard never fires), while the
jqr-routed extraction strips the CR and the guard fires correctly, andthat a real (CR-free)
jqis unaffected by the fix either way.introduced by it, and one pre-existing defect in the same guard's own
failure-propagation (a non-array
gh apiresponse fails the guard openregardless of the CR), filed separately as ruleset_id()'s per_page Guard Fails Open on a Non-Array gh api Response #1971 rather than expanding
this fix's scope.
This defect and its fix are Windows-only, and I cannot reproduce or verify
the actual Windows-jq CRLF behavior on this (Linux) host. The verification
above proves the CR-stripping mechanism is correct and unchanged on
Linux/macOS; it does not and cannot prove the fix resolves the issue on a
real Windows host. That remains unverified here, same as
#1123/#1229/#1234/#1246/#1247/#1253/#1254.
Closes on promotion: #1880
🤖 Generated with Claude Code
Summary by CodeRabbit