Conversation
`scripts/pr_review.py` resolves the owner a write may stay within from the script's own directory, which is deliberate: the helper is hub-hosted rather than carried, so it is reached from a hub checkout while the repository being answered is named on the command line, and the working directory says nothing about who owns either. Its refusals called that directory "this checkout", which an operator reads as the working directory. A copy of the script outside any checkout therefore produced a refusal the operator could disprove on the spot with `git remote get-url origin` in the tree they were standing in, with nothing on screen explaining the disagreement. Message only, per the issue. The anchoring, the refusal, and the refusal writing nothing are unchanged. ## What changed - Both refusals name the directory that was actually probed, and describe the probe rather than claiming which repository encloses that directory. - The unreadable-owner refusal names a remedy, scoped to the cause it actually fixes rather than stated as the diagnosis. - `HERE` is a module constant, the single reader for the probe and for the messages that name it, matching the shape six sibling scripts already use. - `reply`'s exit-64 gloss and `origin_owner`'s docstring each said less than the code does, and `scripts/README.md` carried the same "this checkout" claim. All three now agree with the messages. Before, from a copy outside any checkout: ``` status=OUT_OF_SCOPE nothing was written: this checkout has no readable `origin`, so the owner a write would stay within cannot be established, and an unverified scope is not a scope ``` After, against the same reproduction: ``` status=OUT_OF_SCOPE nothing was written: this script got no owner from `git remote get-url origin` against the directory it sits in, /tmp/scratch, so the owner a write would stay within cannot be established, and an unverified scope is not a scope. That directory is this script's own rather than the working directory, so a readable `origin` in the working directory says nothing about this, and the remedy for a copy of this script sitting outside a checkout is to run the copy that lives inside a hub checkout instead of this one ``` ## Verification Reproduced the issue's own scenario against the fix, a copy of the script outside any checkout and a cross-owner target from a fleet checkout, and read both messages back. 322 cases in the module and 1403 under discovery, plus `prose_lint`, `repo_gate --check eol`, `spec/validate.py`, `ruff format --check` and `ruff check`, all clean. The new probe case was proved by mutation rather than by passing: against a copy whose probe reads `Path.cwd()` instead of `HERE`, `test_the_probe_reads_the_directory_the_refusal_names` fails. Without that, the assertion passed on a path coincidence whenever the working directory happened to equal the anchor. Five local review rounds ran over this diff. Each message clause was checked against every state that reaches its branch, meaning a missing `git`, a timeout, no remote named `origin`, a directory under no repository, an `origin` URL the owner regex cannot parse, and an inherited `GIT_DIR`. Three earlier wordings were rejected for failing one of those: one asserted the probe had executed when a missing `git` means it never did, one named a remedy that read as an instruction to change directory and so reproduced the same refusal, and one claimed a repository encloses the probed directory, which an inherited `GIT_DIR` falsifies. ## Raised separately rather than folded in Two real defects surfaced during review that are code changes rather than wording, so they are filed instead of carried here: - #1561, the probe inherits `GIT_DIR`, so an inherited one redirects it to a different repository. The messages here describe the probe rather than the directory's repository, which keeps them true under that leak, but the leak itself is untouched. - #1562, `wait`'s Copilot review request is a mutation that never passes through `in_scope`, so the in-process owner check does not cover it. Addresses #1557. The closing reference goes on the promotion pull request, since a `develop`-targeted one never registers.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 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 |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are limited to message/anchor consistency plus targeted tests and documentation updates, with no behavioral scope expansion beyond improved diagnostics.
Pull request overview
Promotes develop to main with message-only improvements to scripts/pr_review.py scope refusals so they explicitly name the directory that was actually probed (the script's own directory), reducing operator confusion when running a copied script outside a checkout.
Changes:
- Introduce a module-level
HEREanchor for the script directory and reuse it in both thegit -C ... remote get-url originprobe and the refusal messages. - Expand
in_scope()refusal strings (and exit-64 help text) to describe the probe and name the probed directory, plus a concrete remedy for the "copied outside a checkout" case. - Add targeted tests asserting the probe and refusal messages stay aligned, and update
scripts/README.mdto match the refined exit-64 semantics.
File summaries
| File | Description |
|---|---|
| scripts/pr_review.py | Centralizes the scope anchor as HERE and updates exit-64 / refusal messages to name the probed directory and remedy. |
| scripts/tests/test_pr_review.py | Adds regression tests ensuring the probe directory and refusal wording cannot drift apart. |
| scripts/README.md | Updates documentation for exit 64 to reflect the two refusal states and the script-directory anchoring. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Promotes one commit from
developtomain.1c7e1f1Name the Directory the Scope Refusal Probed, Not the Checkout (Name the Directory the Scope Refusal Probed, Not the Checkout #1563)scripts/pr_review.pyreads the owner a write may stay within from the script's own directory rather than the working directory, deliberately, since the helper is hub-hosted and reached from a hub checkout while the repository being answered is named on the command line. Its out-of-scope refusals called that directory "this checkout", which an operator reads as the working directory and can disprove on the spot withgit remote get-url originin the tree they are standing in. Both refusals now describe the probe and name the directory it ran against, andreply's exit-64 gloss,origin_owner's docstring and the matchingscripts/README.mdsentence agree with them.Message only. The anchoring, the refusal, and the refusal writing nothing are unchanged.
Copilot reviewed the feature pull request at its merged head, 3 of 3 changed files, no findings and none suppressed.
Closes #1557