Repository navigation
Conversation
…ocal_review (#2521) ## Summary Clears the `test_local_review` cluster of #2488, 15 of its Windows failures, two of them real defects in `scripts/local_review.py` rather than in the tests. - **A closed pipe on Windows.** A write to a pipe whose reader has exited raises `OSError` with `errno` `EINVAL` there, not `BrokenPipeError`. `emit` and `main` caught only the subclass, so the interpreter's shutdown flush exited 120, outside the 0/1/2 contract. A new `closed_reader` recognizes both forms, and `main`'s backstop now catches `OSError`, sending any other one to the same boundary report as before through a small `crashed` helper. - **A leaked object directory on Windows.** `worktree_states` removed its throwaway object directory with `ignore_errors=True`. Windows refuses to unlink git's read-only loose objects, so the directory, holding the staged content of every untracked file, outlived each read inside the git directory. The removal now clears the read-only bit and retries through an `onexc` handler. - **Tests.** The `coderabbit` stand-in gets a `.cmd` wrapper on Windows, since Windows runs no shebang and `shutil.which` there finds only a `PATHEXT` name. The dangling-ref case makes git's read-only loose object writable before unlinking it. The piped case runs under the suite's own interpreter rather than a bare `python3`. Part of #2488. The remaining failures there sit in `test_spec_validate`, `test_host_gate`, `test_docker_lint`, and `test_release_guards`, and are left to a separate pull request. ## Verification - On the Windows host from Git Bash, `python3 -m unittest discover -s tests -p test_local_review.py` went from 7 failures and 8 errors to `OK (skipped=6)`, every skip an existing host-capability one. - The two `ReceiptCase` piped tests and `test_staging_leaves_no_throwaway_paths_behind` still fail against the unchanged `local_review.py`, so the script change rather than the test change is what fixes them. - The full suite on this host now reports 7 failures and 2 errors, all in the four modules named above. - `ruff format --check`, `ruff check`, and `mypy` pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Local reviews now handle closed output streams more reliably and report unexpected I/O errors instead of silently ignoring them. * Temporary review files are cleaned up more reliably on Windows, including when files are read-only. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…2524) ## Summary `ArchivedRepositoryCase.test_a_trailing_carriage_return_from_jq_is_stripped` stubbed `jq` to print `archived` followed by a carriage return, while its registry fixture also said `archived`. Any real `jq` on PATH gave the same verdict, so the test passed whether or not the stub ran. The fixture now says `cataloged`, so only the stub can stop the script at the archived exemption. Part of the Windows lane. The promotion pull request closes #2513. ## Verification - With the change, the module passes on the Windows host from Git Bash and, run by the local review pass, under Linux with a real `jq`. - With the stub's `PATH` line removed, the test fails on both, since a real `jq` now answers `cataloged` and the script falls through to `REACHED_NEXT`. - With `configure.sh`'s carriage-return strip removed, the test fails under Linux, which is what CI runs. Under Git Bash the strip makes no difference to the outcome, since Git Bash's command substitution already drops a trailing carriage return before the newline, so a Windows run cannot speak for the strip either way. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…est Assertions (#2523) ## Summary Clears the rest of #2488, the 9 Windows failures left once #2521 cleared the `test_local_review` cluster. With both changes, `python3 -m unittest discover -s tests` passes on the Windows host from Git Bash. - **`spec/validate.py` writes UTF-8.** On Windows a piped stdout uses the ANSI code page, so the validator crashed reporting a registry name that code page cannot encode, a zero-width space for one, and wrote others, such as `ß`, in a form the suite's UTF-8 decode rejected. It now reconfigures both streams to UTF-8 at the top of `main`, through the same `utf8_console` helper `scripts/pr_review.py` already carries. On Linux the streams were already UTF-8, so CI output is unchanged. - **`test_release_guards` runs its guard script from a file.** The handoff read this failure as jq 1.8.2 rejecting `\S`, and that diagnosis is wrong: winget's jq 1.8.2 compiles the filter and gives every expected verdict when it receives it intact. The script went through `bash -c`, and Git Bash's own parse of the Windows command line collapses each doubled backslash inside a quoted argument, so the filter arrived holding an invalid escape and every value was refused. The test now writes the script to a temporary file and runs that, and `host_capability.bash_path`'s docstring records the trap for the next test. - **`test_host_gate`** asserted POSIX forms where Windows prints a backslash path, the PowerShell call operator, and `-Repo`. Each assertion now takes the host's form, and on Linux each expected string is still byte-identical to the literal it replaces. - **`test_docker_lint`** expected a forward-slash mount source from a native Windows `Path`. The expectation takes the separator from the host, and the doubled-quote escaping it pins is unchanged. Part of #2488, which the promotion pull request closes. ## Verification - On the Windows host from Git Bash, `test_spec_validate`, `test_host_gate`, `test_docker_lint`, and `test_release_guards` each pass, and the python-versions guard case runs rather than skips. - The full suite on this branch failed only in `test_local_review`, which #2521 fixes and which has since merged into `develop`. - `ruff format --check`, `ruff check`, and `mypy` pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2528 +/- ##
==========================================
- Coverage 58.50% 58.40% -0.10%
==========================================
Files 16 16
Lines 8066 8090 +24
==========================================
+ Hits 4719 4725 +6
- Misses 3347 3365 +18
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.
🟡 Changes recommended
Re-raise unexpected non-pipe output errors in scripts/local_review.py.
1 open finding
What changed in this PR
Promotes Windows compatibility fixes and regression tests so the full unittest suite passes under Git Bash.
Changes:
- Handles Windows pipe errors and read-only cleanup.
- Forces validator output to UTF-8.
- Strengthens platform-specific shell, path, and fixture tests.
Review found that scripts/local_review.py can silently discard unexpected output errors; non-pipe errors should be re-raised.
| File | Description |
|---|---|
tests/test_spec_validate.py |
Adds UTF-8 regression coverage. |
tests/test_release_guards.py |
Runs guard scripts from temporary files. |
tests/test_local_review.py |
Updates Windows-compatible test helpers. |
tests/test_host_gate.py |
Uses platform-specific command forms. |
tests/test_docker_lint.py |
Uses host-specific path separators. |
tests/test_configure_archived.py |
Ensures the jq stub is exercised. |
tests/host_capability.py |
Documents Git Bash behavior. |
spec/validate.py |
Configures validator output as UTF-8. |
scripts/local_review.py |
Handles Windows pipe errors and read-only cleanup. |
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
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
Promotes three changes from
develop, which together makepython3 -m unittest discover -s testspass on a Windows host from Git Bash.scripts/local_review.py, and the tests that exercise them.OSErrorwithEINVALon Windows rather thanBrokenPipeError, sostatus | head -0exited 120 outside the 0/1/2 contract.closed_readernow recognizes both forms.worktree_statesleft its throwaway object directory behind on Windows, holding the staged content of every untracked file, becausermtree(ignore_errors=True)could not unlink git's read-only loose objects. Anonexchandler now clears the read-only bit and retries.spec/validate.pywrites its output as UTF-8 on every host, and a cp1252 regression case pins that on Linux CI too. Four Windows-shaped test assertions take the host's path, quoting, and flag forms.test_release_guardsruns its guard script from a file, since Git Bash's parse of a Windows command line halves each doubled backslash inside a quoted-cargument, andhost_capability.bash_pathrecords that.cataloged, so the test fails when its stub is not reached.Closes #2488.
Closes #2513.
Verification
python3 -m unittest discover -s testsatdevelop's head ran 2245 tests and reportedOK, where the round started from 15 failures and 10 errors.🤖 Generated with Claude Code