Fix bin/ci-run-failed-specs to handle bare spec paths

[justin808]

Summary

Fixes the bin/ci-run-failed-specs script to handle bare spec paths that users might copy directly from their terminal, like spec/system/integration_spec.rb[1:1:6:1:2]%.

Previously, the script only recognized the format rspec ./spec/... and would fail with "No specs found!" when given bare spec paths.

Changes

• Added regex pattern to match bare spec paths (with or without ./ prefix)
• Strip trailing % characters from spec paths (common in shell output)
• Normalize all paths to ./spec/ format for consistency
• Improved TTY detection to avoid spurious error messages when piping input
• Auto-confirm when TTY is unavailable instead of failing

Test Plan

The script now correctly handles all these input formats:

• spec/foo.rb[1:2:3]% (bare with trailing %)
• ./spec/foo.rb[1:2:3] (with ./ prefix)
• rspec ./spec/foo.rb[1:2:3] (full rspec command format)

Tested with:

echo "spec/system/integration_spec.rb[1:1:6:1:2]%" | bin/ci-run-failed-specs

🤖 Generated with Claude Code

---

This change is 

Summary by CodeRabbit

• Chores
  • Enhanced CI test runner to better handle test file path formats
  • Improved user confirmation prompts for automated CI environments

[coderabbitai]

Walkthrough

The bin/ci-run-failed-specs script is enhanced to parse bare spec paths with bracket notation (e.g., "spec/foo.rb[1:2:3]") by stripping status modifiers and normalizing paths. Additionally, confirmation prompts now check for /dev/tty availability when stdin is not a TTY, reading user input from it or auto-confirming with an informational message if unavailable.

Changes

Cohort / File(s)	Summary
CI script input handling and confirmation logic bin/ci-run-failed-specs	Added parsing for bare spec paths with bracket notation, path normalization to ./spec/... format, and runtime /dev/tty fallback for interactive confirmation when stdin is not a TTY

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Bash path parsing and normalization logic requires careful verification of edge cases
• TTY availability check and fallback behavior needs testing consideration across different shell environments
• String manipulation patterns in bash should be reviewed for robustness

Possibly related PRs

• Add CI debugging tools for efficient local failure reproduction #1973 — Directly modifies the same bin/ci-run-failed-specs script with identical input parsing and /dev/tty confirmation logic

Suggested labels

bug, review-needed

Suggested reviewers

• AbanoubGhadban
• alexeyr-ci2

Poem

🐰 A spec runner now reads from the tty's heart,
Parsing brackets and paths, playing each part,
When stdin's not there, it bravely persists,
Auto-confirming with a pragmatic twist! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main change: adding support for bare spec paths in the bin/ci-run-failed-specs script.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch justin808/fix-spec-runner

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review: Fix bin/ci-run-failed-specs to handle bare spec paths

Summary

This PR improves the bin/ci-run-failed-specs script to handle bare spec paths that users might copy directly from their terminal. Overall, the changes are well-implemented and solve a real usability issue.

Strengths

1. Solves a Real Problem: The enhancement addresses a common use case where users copy spec paths directly from their terminal output.

2. Good Regex Pattern: The pattern correctly handles bare paths, relative paths, and strips trailing % and whitespace.

3. Path Normalization: Consistent normalization to ./spec/ format is good for reliability.

4. Improved TTY Handling: The enhanced TTY detection prevents spurious errors in CI environments or when TTY is unavailable.

Potential Issues

1. Over-Broad Regex Pattern (Medium Priority)

Issue: The regex (.?/?spec/[^[:space:]%]+) might match unintended lines like comments or directory listings.

Suggestion: Consider making it more specific by requiring .rb extension or brackets.

2. Silent Auto-Confirmation (Low Priority)

Issue: Line 180 auto-confirms when TTY is unavailable, which could surprise users.

Suggestion: Document this behavior in the help text under Features section.

3. Edge Case: Paths Without Extensions (Low Priority)

The regex doesn't require .rb extension. Impact is minimal since RSpec will fail gracefully on invalid paths.

Testing Recommendations

Consider testing these additional cases:

• Mixed input (valid specs + comments mentioning spec/)
• Just comments mentioning spec/ paths
• Trailing whitespace and % characters

Best Practices Check

• Shell Safety: Good use of set -euo pipefail and array-based command building
• Error Handling: Proper TTY detection and fallback logic
• User Experience: Clear messaging about what will be run
• Documentation: Help text could mention auto-confirm behavior

Security Considerations

• No Command Injection: Uses array-based command building instead of eval
• Input Validation: Paths are validated through regex before use
• No Arbitrary Code Execution: Only runs rspec with validated spec paths

Recommendation

APPROVE with minor suggestions

The changes are solid and solve a real usability problem. The regex pattern could be more specific to avoid false positives, but even as-is, it's unlikely to cause issues in practice since RSpec will gracefully handle invalid paths.

The enhanced TTY handling is well-implemented and makes the script more robust in different execution environments.

Overall Assessment: Ready to merge after considering the regex specificity suggestion.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

bin/ci-run-failed-specs (2)

108-116: Consider tightening the regex pattern to avoid edge cases.

The pattern \.?/? matches dot and slash independently, which could incorrectly match .spec/ (missing slash) or /spec/ (missing dot prefix). While these cases are unlikely in practice, the regex could be more precise.

Consider this refactor:

-    elif [[ "$line" =~ (\.?/?spec/[^[:space:]%]+) ]]; then
-      spec="${BASH_REMATCH[1]}"
-      # Normalize to ./spec/ format
-      if [[ ! "$spec" =~ ^\. ]]; then
-        spec="./$spec"
-      fi
-      SPECS+=("$spec")
+    elif [[ "$line" =~ (\./)?spec/[^[:space:]%]+ ]]; then
+      # Always normalize to ./spec/ format
+      spec="./${BASH_REMATCH[0]#./}"
+      SPECS+=("$spec")

This explicitly matches the optional ./ prefix and unconditionally normalizes all paths.

---

169-183: Consider simplifying the TTY check using an if conditional.

The set +e / set -e pattern works correctly but can be simplified. Commands in if conditionals are allowed to fail without triggering set -e exit, making the toggle unnecessary.

Apply this diff to simplify:

-  # stdin is not a TTY (piped input), try /dev/tty
-  # Check if we can actually open /dev/tty by attempting to use it in a subshell
-  set +e  # Temporarily disable errexit for the check
-  (exec 0</dev/tty) 2>/dev/null
-  TTY_CHECK=$?
-  set -e  # Re-enable errexit
-
-  if [ $TTY_CHECK -eq 0 ]; then
-    # Successfully opened /dev/tty, use it for confirmation
+  # stdin is not a TTY (piped input), try /dev/tty
+  if (exec 0</dev/tty) 2>/dev/null; then
     read -p "Run these specs now? [Y/n] " -n 1 -r REPLY < /dev/tty
   else
-    # Cannot open /dev/tty, auto-confirm
     echo "Run these specs now? [Y/n] Y (auto-confirmed, TTY unavailable)"
     REPLY="Y"
   fi

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a977b3 and 4bca154.

📒 Files selected for processing (1)

• bin/ci-run-failed-specs (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: claude-review

[claude]

Code Review - PR #1989

Overview

This PR enhances the bin/ci-run-failed-specs script to handle multiple spec path input formats, improving developer experience when copying spec paths from various sources.

✅ Strengths

1. Excellent Problem-Solution Fit: The enhancement directly addresses a real usability pain point - users copying spec paths from terminal output that include trailing % characters or lack the ./ prefix.

2. Robust Pattern Matching: The regex (\.?/?spec/[^[:space:]%]+) elegantly handles all these variations:

   • spec/foo.rb[1:2:3]%
   • ./spec/foo.rb[1:2:3]
   • spec/foo.rb[1:2:3]

3. Path Normalization: Converting all spec paths to ./spec/ format ensures consistency and prevents duplicate entries after deduplication.

4. Improved TTY Detection: The TTY availability check is a significant improvement:

   • Tests if /dev/tty can actually be opened before attempting to use it
   • Gracefully auto-confirms when TTY is unavailable (e.g., in CI/automated environments)
   • Prevents spurious error messages

5. Backward Compatibility: The original rspec ./spec/... pattern matching is preserved, ensuring existing workflows continue to work.

💡 Code Quality Observations

Good practices:

• Proper use of set +e / set -e around the TTY check to prevent script failure
• Clear inline comments explaining the logic
• Safe array operations with proper quoting

🔍 Potential Considerations

1. Regex Specificity: The pattern (\.?/?spec/[^[:space:]%]+) will match any line containing "spec/" which could theoretically match unintended text. However, in the context of RSpec output, this is unlikely to cause issues and the broad matching is actually beneficial for user convenience.

2. Edge Case - Multiple specs per line: If someone pastes a line with multiple spec paths, only the first will be captured. However, this is an unlikely scenario given typical RSpec output format.

3. Performance: The regex matching is efficient and won't cause performance issues even with large failure outputs.

🧪 Testing Considerations

The PR description mentions testing with:

echo "spec/system/integration_spec.rb[1:1:6:1:2]%" | bin/ci-run-failed-specs

Additional test scenarios to consider (though not blocking):

• Mixed input with both formats
• Specs with special characters in filenames
• Very long spec paths
• Input with only whitespace or empty lines

🔒 Security

No security concerns. The script:

• Uses safe array operations (not eval)
• Properly quotes variables
• Uses set -euo pipefail for safe error handling

📝 Documentation

The help text and comments adequately explain the new functionality. The commit message and PR description are thorough and well-written.

🎯 Verdict

This is a high-quality enhancement that meaningfully improves developer experience. The code is clean, safe, and well-thought-out. The TTY detection improvement alone is valuable for automated environments.

No blocking issues identified. The implementation follows repository conventions (as per CLAUDE.md) and enhances an existing utility script without introducing risk.

Minor Suggestions (Optional)

1. Consider adding a comment on line 110 explaining why % is stripped (it's a shell output artifact).

2. The regex could be made slightly more specific if needed:

   elif [[ "$line" =~ ^[[:space:]]*(\.?/?spec/[^[:space:]%]+) ]]; then

   This anchors to line start (with optional whitespace), though the current version is probably fine for practical use.

---

Great work on this improvement! 🚀