GH#3541, GH#3683, GH#3550, GH#3535, GH#4155, GH#3551, GH#3533, GH#4153, GH#4154: medium quality-debt batch 9#4174
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughThis PR addresses 10+ quality-debt findings from prior code reviews across shell scripts, Python utilities, and documentation. Changes include replacing fixed delays with event-based waits, removing stderr suppression per style guide, eliminating dead code, improving regex portability, and correcting spelling inconsistencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a batch of quality-debt fixes across several scripts, enhancing reliability, security, and code maintainability. Key improvements include more robust font loading in accessibility checks, removal of dead code, refined error logging in email testing utilities, and critical GraphQL injection prevention measures with input validation in statistical functions. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
johnwaldo
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request delivers a valuable batch of quality improvements across several scripts. The changes enhance reliability by replacing a fixed timeout with an event-based wait in playwright-contrast.mjs, improve security by preventing GraphQL injection in stats-functions.sh, and increase observability by redirecting error streams to logs instead of /dev/null. The removal of dead code and unnecessary process.exit(0) calls also contributes to a cleaner codebase.
I've added a few suggestions in email-delivery-test-helper.sh to improve the accuracy of spam-related metric counting and ensure portability of grep commands. Overall, this is a solid set of fixes.
|
Added commit 4: Fix silent failure in secret-helper.sh
Additional closes: #3633 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/services/accessibility/accessibility-audit.md:
- Line 107: Update the font-size check in accessibility-helper.sh where the
regex (([0-9]|1[01])px|0\.[0-9]+em) is used so it enforces the documentation's
"below 14px" threshold instead of the current 12px one: change the px branch to
allow up to 13px (e.g., ([0-9]|1[0-3])px) and adjust the em branch to treat
values <= 0.875em as below-14px (replace the generic 0\.[0-9]+em check with one
that only matches <=0.875em), ensuring any conditional that uses this regex (the
font-size validation/location referenced by that pattern) flags fonts under
14px.
- Around line 237-241: The sed extraction fails because accessibility-helper.sh
emits ANSI color codes (via shared-constants.sh) so the pattern in the score
extraction (sed -E -n 's/.*Score: ([0-9]+).*/\1/p') won't match; update the
example to strip ANSI escapes from the helper output before running the
score-extraction sed (or invoke the helper in non-TTY/no-color mode), so
parse_lighthouse_a11y output is cleaned of escape sequences and the variable
score can be parsed reliably.
In `@tests/test-email-thread-reconstruction.sh`:
- Around line 234-235: The grep check against index_file uses a pattern that
requires two adjacent backslashes and thus misses Windows paths with a single
backslash; update the grep -qE pattern used in that check (the regex string
passed to grep -qE in the if statement that reads index_file) so the backslash
token uses a one-or-more quantifier instead of matching exactly two backslashes
(i.e., change the backslash part of the ERE to match one or more backslashes),
ensuring links like (..\email1.md) are detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 164f5a64-197d-4c52-9a28-f53981f0e606
📒 Files selected for processing (12)
.agents/scripts/accessibility/playwright-contrast.mjs.agents/scripts/commands/email-delivery-test.md.agents/scripts/email-delivery-test-helper.sh.agents/scripts/email-design-test-helper.sh.agents/scripts/email-thread-reconstruction.py.agents/scripts/secret-helper.sh.agents/scripts/stats-functions.sh.agents/scripts/wappalyzer-detect.mjs.agents/services/accessibility/accessibility-audit.md.agents/services/email/email-delivery-test.md.agents/services/email/email-delivery-testing.mdtests/test-email-thread-reconstruction.sh
💤 Files with no reviewable changes (2)
- .agents/scripts/email-design-test-helper.sh
- .agents/scripts/wappalyzer-detect.mjs
johnwaldo
force-pushed
the
bugfix/quality-debt-batch9
branch
from
872cf9b to
1f28d46
Compare
Rebase + Review Feedback FixesRebased onto main — dropped the New commit: address review feedback
PR now has 4 commits (was 4, dropped 1, added 1):
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/services/accessibility/accessibility-audit.md:
- Around line 236-237: Replace the GNU-only ANSI-strip sed expression used after
the accessibility-helper.sh lighthouse call (currently: sed
's/\x1b\[[0-9;]*m//g') with a POSIX/BSD-compatible approach that uses a literal
ESC byte (octal 033) or an equivalent portable method (or switch to perl -pe
's/\e\[[0-9;]*m//g') so the ANSI escape sequences are removed on macOS/BSD
runners; update the pipeline invocation that captures the Score to use that
portable escape-stripping expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ece65344-a2fe-4508-ba82-6dd2313a5510
📒 Files selected for processing (11)
.agents/scripts/accessibility/playwright-contrast.mjs.agents/scripts/commands/email-delivery-test.md.agents/scripts/email-delivery-test-helper.sh.agents/scripts/email-design-test-helper.sh.agents/scripts/email-thread-reconstruction.py.agents/scripts/secret-helper.sh.agents/scripts/wappalyzer-detect.mjs.agents/services/accessibility/accessibility-audit.md.agents/services/email/email-delivery-test.md.agents/services/email/email-delivery-testing.mdtests/test-email-thread-reconstruction.sh
💤 Files with no reviewable changes (2)
- .agents/scripts/wappalyzer-detect.mjs
- .agents/scripts/email-design-test-helper.sh
✅ Files skipped from review due to trivial changes (1)
- .agents/services/email/email-delivery-testing.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .agents/scripts/accessibility/playwright-contrast.mjs
- tests/test-email-thread-reconstruction.sh
- .agents/scripts/secret-helper.sh
- .agents/scripts/email-delivery-test-helper.sh
- .agents/services/email/email-delivery-test.md
- email-delivery-test-helper.sh: replace grep -ciE with grep -oiE | wc -l for high-risk and medium-risk phrase counting to accurately count occurrences (not just matching lines) — addresses Gemini review - accessibility-helper.sh: update font-size regex from 12px to 14px threshold (([0-9]|1[0-3])px) and tighten em branch to <=0.875em; update warning message to match — aligns code with docs (CodeRabbit) - accessibility-audit.md: update table entry from '12px' to '14px' to match helper script threshold; replace GNU-only sed '\x1b' ANSI-strip with portable bash $'\033' literal ESC byte for BSD/macOS compat - tests/test-email-thread-reconstruction.sh: change grep -qE backslash pattern from '\\' to '\\+' (one-or-more quantifier) to detect single Windows-style path separators in Markdown links
- playwright-contrast.mjs: replace waitForTimeout(1500) with document.fonts.ready for reliable font-dependent contrast analysis - wappalyzer-detect.mjs: remove unnecessary process.exit(0) — let Node exit naturally after async completion - email-design-test-helper.sh: remove dead eoa_auth_args() function (defined but never called — code uses EOA_AUTH_HEADER directly) - email-delivery-test-helper.sh: remove 2>/dev/null from grep calls (|| true already handles exit codes), redirect spamassassin stderr to log file, remove suppression on health-check call, fix Analyse → Analyze typo for consistency with function name Closes marcusquinn#3541 Closes marcusquinn#3683 Closes marcusquinn#3550 Closes marcusquinn#3535
- email-delivery-test.md, email-delivery-testing.md, email-delivery-test command: normalize Analyse → Analyze for consistency with function names - accessibility-audit.md: fix font size threshold inconsistency (12px → 14px to match checklist), replace grep -oP with portable sed in CI example, add error handling for missing score - test-email-thread-reconstruction.sh: replace grep -P (GNU PCRE) with grep -E for macOS portability - email-thread-reconstruction.py: use pathlib.as_posix() instead of manual os.sep replacement for clearer intent Closes marcusquinn#3551 Closes marcusquinn#3536 Closes marcusquinn#3534 Closes marcusquinn#3533 Closes marcusquinn#4153 Closes marcusquinn#4154
- Add || true to gopass show command so missing secrets don't cause silent exit under set -e (cmd_get's error message was unreachable) - Update stale comment: function is used by both cmd_get (stdout) and build_secret_env (subprocess injection), not just internally Closes marcusquinn#3633
… ANSI stripping, backslash pattern - email-delivery-test-helper.sh: replace grep -c with grep -o | wc -l for accurate multi-match-per-line counting (img, URL, JS, shortener counts) - accessibility-audit.md: correct font-size threshold to 12px matching accessibility-helper.sh implementation, add ANSI color stripping to CI/CD score extraction example - test-email-thread-reconstruction.sh: fix ERE backslash pattern to catch single Windows path separators (was requiring two consecutive backslashes)
- email-delivery-test-helper.sh: replace grep -ciE with grep -oiE | wc -l for high-risk and medium-risk phrase counting to accurately count occurrences (not just matching lines) — addresses Gemini review - accessibility-helper.sh: update font-size regex from 12px to 14px threshold (([0-9]|1[0-3])px) and tighten em branch to <=0.875em; update warning message to match — aligns code with docs (CodeRabbit) - accessibility-audit.md: update table entry from '12px' to '14px' to match helper script threshold; replace GNU-only sed '\x1b' ANSI-strip with portable bash $'\033' literal ESC byte for BSD/macOS compat - tests/test-email-thread-reconstruction.sh: change grep -qE backslash pattern from '\\' to '\\+' (one-or-more quantifier) to detect single Windows-style path separators in Markdown links
alex-solovyev
force-pushed
the
bugfix/quality-debt-batch9
branch
from
26cc012 to
2d82553
Compare
|
This PR is from an external contributor (@johnwaldo). Auto-merge is disabled for external PRs — a maintainer must review and approve manually. To approve or decline, comment on this PR:
|
alex-solovyev
added
external-contributor
Review Feedback AddressedAll CHANGES_REQUESTED findings from CodeRabbit and Gemini have been verified against the current branch code and fixed. CodeRabbit Findings (3 issues)1. Font-size threshold mismatch (accessibility-helper.sh vs accessibility-audit.md)
2. ANSI escape stripping — GNU-only sed not portable to BSD/macOS (accessibility-audit.md ~line 237)
3. Backslash ERE pattern requires two adjacent backslashes (tests/test-email-thread-reconstruction.sh ~line 235)
Gemini Findings (6 issues — all in email-delivery-test-helper.sh)All six grep -c / grep -co portability and accuracy issues were fixed by replacing with grep -oiE | wc -l:
The branch is up to date at commit 2d82553. Requesting re-review. |
|
@coderabbitai review Both CHANGES_REQUESTED findings have been addressed in commit Finding 1 — Font-size regex threshold (accessibility-helper.sh line 723):
Finding 2 — POSIX-compatible ANSI strip (accessibility-audit.md line 237):
Both changes verified against current code before applying. ShellCheck passes with no new violations (only pre-existing SC1091 info on sourced file). |
|
Kicking off a full re-review of the PR now. 🧠 Learnings used✅ Actions performedFull review triggered. |
|
Dispatching worker to address 7 unresolved review suggestions and rebase onto main.
|
|
Worker launch failed (at capacity 8/8 when dispatch attempted). PR #4174 still needs: rebase onto main + address 7 unresolved CodeRabbit suggestions. Will be re-dispatched next pulse cycle when a slot opens. |
- email-delivery-test-helper.sh: replace grep -ciE with grep -oiE | wc -l for high-risk and medium-risk phrase counting to accurately count occurrences (not just matching lines) — addresses Gemini review - accessibility-helper.sh: update font-size regex from 12px to 14px threshold (([0-9]|1[0-3])px) and tighten em branch to <=0.875em; update warning message to match — aligns code with docs (CodeRabbit) - accessibility-audit.md: update table entry from '12px' to '14px' to match helper script threshold; replace GNU-only sed '\x1b' ANSI-strip with portable bash $'\033' literal ESC byte for BSD/macOS compat - tests/test-email-thread-reconstruction.sh: change grep -qE backslash pattern from '\\' to '\\+' (one-or-more quantifier) to detect single Windows-style path separators in Markdown links
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Worker killed after 3.75h with 0 commits (struggle_ratio: infinite).
|
|
This PR has a merge conflict that automated workers have been unable to resolve (3.75h, 0 commits). Since this is an external contributor PR, please rebase onto main and resolve the conflicts manually: git fetch upstream
git rebase upstream/main
# resolve conflicts
git push --force-with-leaseOnce rebased, the PR will be auto-merged (it's already APPROVED with all CI checks passing). |
|
Dispatching worker (attempt 3 — escalated to opus after 2 failed sonnet attempts).
|
- email-delivery-test-helper.sh: replace grep -ciE with grep -oiE | wc -l for high-risk and medium-risk phrase counting to accurately count occurrences (not just matching lines) — addresses Gemini review - accessibility-helper.sh: update font-size regex from 12px to 14px threshold (([0-9]|1[0-3])px) and tighten em branch to <=0.875em; update warning message to match — aligns code with docs (CodeRabbit) - accessibility-audit.md: update table entry from '12px' to '14px' to match helper script threshold; replace GNU-only sed '\x1b' ANSI-strip with portable bash $'\033' literal ESC byte for BSD/macOS compat - tests/test-email-thread-reconstruction.sh: change grep -qE backslash pattern from '\\' to '\\+' (one-or-more quantifier) to detect single Windows-style path separators in Markdown links
- email-delivery-test-helper.sh: replace grep -ciE with grep -oiE | wc -l for high-risk and medium-risk phrase counting to accurately count occurrences (not just matching lines) — addresses Gemini review - accessibility-helper.sh: update font-size regex from 12px to 14px threshold (([0-9]|1[0-3])px) and tighten em branch to <=0.875em; update warning message to match — aligns code with docs (CodeRabbit) - accessibility-audit.md: update table entry from '12px' to '14px' to match helper script threshold; replace GNU-only sed '\x1b' ANSI-strip with portable bash $'\033' literal ESC byte for BSD/macOS compat - tests/test-email-thread-reconstruction.sh: change grep -qE backslash pattern from '\\' to '\\+' (one-or-more quantifier) to detect single Windows-style path separators in Markdown links
|
Rebased onto main to resolve conflicts. Branch is now clean — requesting re-review. |
|
Rebased onto main to resolve merge conflict. Ready to merge. |
- email-delivery-test-helper.sh: replace grep -ciE with grep -oiE | wc -l for high-risk and medium-risk phrase counting to accurately count occurrences (not just matching lines) — addresses Gemini review - accessibility-helper.sh: update font-size regex from 12px to 14px threshold (([0-9]|1[0-3])px) and tighten em branch to <=0.875em; update warning message to match — aligns code with docs (CodeRabbit) - accessibility-audit.md: update table entry from '12px' to '14px' to match helper script threshold; replace GNU-only sed '\x1b' ANSI-strip with portable bash $'\033' literal ESC byte for BSD/macOS compat - tests/test-email-thread-reconstruction.sh: change grep -qE backslash pattern from '\\' to '\\+' (one-or-more quantifier) to detect single Windows-style path separators in Markdown links
- email-delivery-test-helper.sh: replace grep -ciE with grep -oiE | wc -l for high-risk and medium-risk phrase counting to accurately count occurrences (not just matching lines) — addresses Gemini review - accessibility-helper.sh: update font-size regex from 12px to 14px threshold (([0-9]|1[0-3])px) and tighten em branch to <=0.875em; update warning message to match — aligns code with docs (CodeRabbit) - accessibility-audit.md: update table entry from '12px' to '14px' to match helper script threshold; replace GNU-only sed '\x1b' ANSI-strip with portable bash $'\033' literal ESC byte for BSD/macOS compat - tests/test-email-thread-reconstruction.sh: change grep -qE backslash pattern from '\\' to '\\+' (one-or-more quantifier) to detect single Windows-style path separators in Markdown links
- email-delivery-test-helper.sh: replace grep -ciE with grep -oiE | wc -l for high-risk and medium-risk phrase counting to accurately count occurrences (not just matching lines) — addresses Gemini review - accessibility-helper.sh: update font-size regex from 12px to 14px threshold (([0-9]|1[0-3])px) and tighten em branch to <=0.875em; update warning message to match — aligns code with docs (CodeRabbit) - accessibility-audit.md: update table entry from '12px' to '14px' to match helper script threshold; replace GNU-only sed '\x1b' ANSI-strip with portable bash $'\033' literal ESC byte for BSD/macOS compat - tests/test-email-thread-reconstruction.sh: change grep -qE backslash pattern from '\\' to '\\+' (one-or-more quantifier) to detect single Windows-style path separators in Markdown links
- email-delivery-test-helper.sh: replace grep -ciE with grep -oiE | wc -l for high-risk and medium-risk phrase counting to accurately count occurrences (not just matching lines) — addresses Gemini review - accessibility-helper.sh: update font-size regex from 12px to 14px threshold (([0-9]|1[0-3])px) and tighten em branch to <=0.875em; update warning message to match — aligns code with docs (CodeRabbit) - accessibility-audit.md: update table entry from '12px' to '14px' to match helper script threshold; replace GNU-only sed '\x1b' ANSI-strip with portable bash $'\033' literal ESC byte for BSD/macOS compat - tests/test-email-thread-reconstruction.sh: change grep -qE backslash pattern from '\\' to '\\+' (one-or-more quantifier) to detect single Windows-style path separators in Markdown links
- email-delivery-test-helper.sh: replace grep -ciE with grep -oiE | wc -l for high-risk and medium-risk phrase counting to accurately count occurrences (not just matching lines) — addresses Gemini review - accessibility-helper.sh: update font-size regex from 12px to 14px threshold (([0-9]|1[0-3])px) and tighten em branch to <=0.875em; update warning message to match — aligns code with docs (CodeRabbit) - accessibility-audit.md: update table entry from '12px' to '14px' to match helper script threshold; replace GNU-only sed '\x1b' ANSI-strip with portable bash $'\033' literal ESC byte for BSD/macOS compat - tests/test-email-thread-reconstruction.sh: change grep -qE backslash pattern from '\\' to '\\+' (one-or-more quantifier) to detect single Windows-style path separators in Markdown links
2 participants
Summary
Commit 1: Small fixes across 4 files
waitForTimeout(1500)withdocument.fonts.readyfor reliable font-dependent contrast analysis (quality-debt: .agents/scripts/accessibility/playwright-contrast.mjs — PR #937 review feedback (medium) #3541)process.exit(0)— let Node exit naturally after async completion (quality-debt: .agents/scripts/wappalyzer-detect.mjs — PR #1536 review feedback (medium) #3683)eoa_auth_args()function (defined but never called — code usesEOA_AUTH_HEADERdirectly in curl commands) (quality-debt: .agents/scripts/email-design-test-helper.sh — PR #930 review feedback (medium) #3550)2>/dev/nullfrom 7 grep calls (|| truealready handles exit codes), redirect spamassassin stderr to log file instead of/dev/null, remove suppression on health-check call, fix "Analyse" → "Analyze" typo for consistency with function name (quality-debt: .agents/scripts/email-delivery-test-helper.sh — PR #933 review feedback (medium) #3535)Commit 2: GraphQL injection prevention in stats-functions.sh
-F(typed) GraphQL variables instead of string interpolation forowner/namein_cleanup_stale_pinned_issuesrepository query and for search queries in quality-debt backlog stats_validate_repo_slug()helper to validateowner/repoformat (defense-in-depth —repos.jsonis trusted but validation prevents injection if config is corrupted)2>/dev/nullwith2>>$LOGFILEon GraphQL query for pinned issues (quality-debt: .agents/scripts/stats-functions.sh — PR #4110 review feedback (medium) #4155)Commit 3: Docs consistency, portability, and minor code improvements
grep -oPwith portablesedin CI example, add error handling for missing score (quality-debt: .agents/services/accessibility/accessibility-audit.md — PR #932 review feedback (medium) #3533)grep -P(GNU PCRE) withgrep -Efor macOS portability in test (quality-debt: tests/test-email-thread-reconstruction.sh — PR #4111 review feedback (medium) #4154)pathlib.as_posix()instead of manualos.sepreplacement in Python (quality-debt: .agents/scripts/email-thread-reconstruction.py — PR #4111 review feedback (medium) #4153)Closes #3541
Closes #3683
Closes #3550
Closes #3535
Closes #4155
Closes #3551
Closes #3536
Closes #3534
Closes #3533
Closes #4153
Closes #4154
Summary by CodeRabbit
Bug Fixes
Documentation
Refactor
Tests