fix: improve Windows shell quoting and test environment portability (…#89
Conversation
📝 WalkthroughWalkthroughRegex patterns embedded in generated ChangesRegex Quoting and Check Invocation Updates
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/becwright/cli.py (1)
628-645: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent quoting idiom between rule groups.
These commit-message signal
checkstrings use raw string literals (r'...') with a literal embedded", whereas the JS/Go/Rust starter rules above (Lines 509-528) use regular strings with escaped\"/\\. Both produce correct output, but mixing styles in the same file for the same kind of value is slightly harder to scan/verify at a glance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/becwright/cli.py` around lines 628 - 645, The commit-message signal definitions in _COMMIT_MSG_SIGNALS use a different quoting style than the adjacent rule groups, making the check values harder to scan consistently. Update the check strings in the conventional-commits and no-ai-attribution entries to match the existing idiom used in the other starter rules, and keep the symbol names _COMMIT_MSG_SIGNALS and the dict entries for those rules as the place to make the change.tests/test_cli_and_git.py (1)
25-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated check-command builder to a shared test helper.
The
sys.executable -c "..."command-construction snippet (path injection + import +sys.exit(main())) is copy-pasted nearly verbatim intest_cli_and_git.py,test_engine_integration.py,test_mcp.py,test_report_and_json.py, andtest_staged_content.py. Any future portability tweak (e.g., another Windows quoting fix) will need to be replicated in all five places.Consider moving this into a shared
conftest.pyfixture/helper (e.g.,_check_cmd(module)) that all test files import/use.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli_and_git.py` around lines 25 - 28, The check-command construction is duplicated across multiple test files, so any quoting or portability change must be updated in several places. Extract the shared sys.executable -c command builder into a common test helper in conftest.py (for example, a helper used by _rules_yaml), and update the affected tests to call that shared helper instead of rebuilding the import/path-injection/sys.exit(main()) string inline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_cli_and_git.py`:
- Around line 173-176: The inline comment near the advisory rule setup is stale
and refers to the old false command even though the check now runs
sys.executable with a Python exit(1) snippet. Update or remove the comment in
the test setup around the rules.yaml write in the relevant test so it accurately
describes the current advisory check mechanism and does not restate outdated
behavior.
---
Nitpick comments:
In `@src/becwright/cli.py`:
- Around line 628-645: The commit-message signal definitions in
_COMMIT_MSG_SIGNALS use a different quoting style than the adjacent rule groups,
making the check values harder to scan consistently. Update the check strings in
the conventional-commits and no-ai-attribution entries to match the existing
idiom used in the other starter rules, and keep the symbol names
_COMMIT_MSG_SIGNALS and the dict entries for those rules as the place to make
the change.
In `@tests/test_cli_and_git.py`:
- Around line 25-28: The check-command construction is duplicated across
multiple test files, so any quoting or portability change must be updated in
several places. Extract the shared sys.executable -c command builder into a
common test helper in conftest.py (for example, a helper used by _rules_yaml),
and update the affected tests to call that shared helper instead of rebuilding
the import/path-injection/sys.exit(main()) string inline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 205d77c2-96c7-44e7-b102-8273c0a997e9
📒 Files selected for processing (7)
src/becwright/cli.pytests/test_cli_and_git.pytests/test_engine_integration.pytests/test_init.pytests/test_mcp.pytests/test_report_and_json.pytests/test_staged_content.py
| # `false` always "fails"; an advisory rule must report but not block the commit. | ||
| (tmp_path / ".bec" / "rules.yaml").write_text( | ||
| "rules:\n - id: adv\n paths: ['**/*.py']\n check: 'false'\n severity: advisory\n", | ||
| f"rules:\n - id: adv\n paths: ['**/*.py']\n check: '\"{sys.executable}\" -c \"import sys; sys.exit(1)\"'\n severity: advisory\n", | ||
| encoding="utf-8") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale comment no longer matches the code.
The comment # \false` always "fails"...still refers to the old shellfalsecommand, but the check value was rewritten tosys.executable -c "import sys; sys.exit(1)"`. Update the comment to reflect the actual mechanism.
As per coding guidelines, "Use comments only for complex code; if the code is self-explanatory, do not comment it, and do not add comments that merely restate the obvious" — a comment must at least remain accurate.
📝 Proposed fix
- # `false` always "fails"; an advisory rule must report but not block the commit.
+ # The check exits 1 (always "fails"); an advisory rule must report but not block the commit.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # `false` always "fails"; an advisory rule must report but not block the commit. | |
| (tmp_path / ".bec" / "rules.yaml").write_text( | |
| "rules:\n - id: adv\n paths: ['**/*.py']\n check: 'false'\n severity: advisory\n", | |
| f"rules:\n - id: adv\n paths: ['**/*.py']\n check: '\"{sys.executable}\" -c \"import sys; sys.exit(1)\"'\n severity: advisory\n", | |
| encoding="utf-8") | |
| # The check exits 1 (always "fails"); an advisory rule must report but not block the commit. | |
| (tmp_path / ".bec" / "rules.yaml").write_text( | |
| f"rules:\n - id: adv\n paths: ['**/*.py']\n check: '\"{sys.executable}\" -c \"import sys; sys.exit(1)\"'\n severity: advisory\n", | |
| encoding="utf-8") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_cli_and_git.py` around lines 173 - 176, The inline comment near
the advisory rule setup is stale and refers to the old false command even though
the check now runs sys.executable with a Python exit(1) snippet. Update or
remove the comment in the test setup around the rules.yaml write in the relevant
test so it accurately describes the current advisory check mechanism and does
not restate outdated behavior.
Source: Coding guidelines
2 participants
Description
Key Changes
1. Windows Command Line Quoting Fixes
src/becwright/cli.py: Swapped single quotes (') with double quotes (") around--patternregex values inside the starter rules generated byinit. Windowscmd.exedoes not strip single quotes from commands, which was causing checks to silently pass (as quotes were compiled as literal characters of the regex pattern).tests/test_init.py: Updated starter rules test assertions to reflect double-quoted patterns.2. POSIX Inline Environment Portability in Tests
tests/test_engine_integration.py,tests/test_cli_and_git.py,tests/test_staged_content.py,tests/test_mcp.py,tests/test_report_and_json.py:PYTHONPATH="..." python -m ...) with cross-platform Python execution syntax:python -c "import sys; sys.path.insert(0, ...)".check: |-) to prevent path formatting conflicts (like Windows colonsC:/...and quotes) from breaking the generated YAML syntax.3. Portable Timeouts
tests/test_engine_integration.py: Swapped Unixsleep 5command used for timeout testing with a portable python-based sleep:python -c "import time; time.sleep(5)".4. Unix Command Fallbacks in Tests
tests/test_cli_and_git.py: Updated test rules using Unix-specifictrueandfalsecommands to run portable python exits instead (python -c "import sys; sys.exit(0/1)").Verification Results
Verified locally on a Windows platform. Running the test suite yields 100% green tests:
Summary by CodeRabbit