Say the Archived-Exemption Region Makes No gh Call of Its Own - #1854
Merged
Merged
Conversation
The test module's docstring said the region under test runs before gh is ever invoked, but configure.sh resolves a default repository with gh repo view before it when no repository argument is given. What lets the test run without a stub is that the region itself calls no gh, so the docstring now says that. Raised by Copilot on the promotion PR #1850. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
The module docstring said no stub is needed, but the end-to-end case stubs gh and the carriage-return case stubs jq. Only the cases running the lifted region need no gh stub, so the sentence now says that. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Owner
Author
|
@coderabbitai review |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: ptr727/ProjectTemplate/.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 |
Rate Limit Exceeded
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The updated docstring still reads ambiguously (comma splice and unclear antecedent), which undermines the PR’s goal of making the behavior explanation unambiguous.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR updates the module docstring in scripts/tests/test_configure_archived.py to correct an inaccurate statement about when gh is invoked, clarifying that the lifted region itself does not call gh and explaining why the unit tests do not need a gh stub in the cases that execute that region.
Changes:
- Reword the module docstring to avoid claiming the lifted region runs before any
ghinvocation. - Clarify the scope of the “no
ghstub needed” statement to the cases that run the lifted region.
| File | Description |
|---|---|
| scripts/tests/test_configure_archived.py | Docstring wording update to accurately describe gh usage and test harness assumptions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The docstring joined two claims with a comma splice and referred to the region with an ambiguous "before it". It now states that the region makes no gh call, then that the script resolves the repository before this region runs. Raised by Copilot on #1854. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
The previous wording credited configure.sh's own repository resolution, which never runs in the lifted-region cases, and "the cases running it" could bind to the script, whose end-to-end case does stub gh. The docstring now names the real reason, the harness assigning every variable the region reads, and says "the lifted region" outright. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
"The harness assigns every variable it reads" bound "it" to the harness, which reads nothing. The sentence now says "the region reads". Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is a docstring correction that matches the described test harness behavior and introduces no functional or behavioral risk.
Review effort: Lite
Findings: None
Resolved since last review (1)
This was referenced Sep 26, 2026
ptr727
added a commit
that referenced
this pull request
Sep 26, 2026
#1858) Fixes the second finding Copilot raised on the promotion PR #1850, against #1846's test. `EndToEndCase`'s docstring said the script "exits before its first `gh` call", which reads as a property of `configure.sh` in general. That's false in two cases: - With no repository argument, `configure.sh` runs `gh repo view` to resolve a default before the archived check. - For an active entry, the script goes on to its `gh api` calls. The case passes `owner/Fixture` against an archived entry, so the docstring now names both conditions. #1854 fixed the module docstring's version of this claim, but my sweep for other statements of it searched a few phrasings and missed this one. A second sweep, across every mention of `gh` ordering or the archived check, found no others. Two local strict review passes are recorded: the first raised the missing archived condition, and the second found nothing. The module's tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
ptr727
added a commit
that referenced
this pull request
Sep 26, 2026
…and Overnight Fixes to Main (#1850) Promotes `develop` to `main`. ## Carried - #1846: exits `repo-config/configure.sh` early, writing nothing, for a repository whose registry `status` is `archived`. - #1848: fails the validator's C# unit test step when the run wrote no non-empty Cobertura report, clearing `./coverage` first. It declines #1134's `ref` input with evidence, since a bare checkout already validates `github.sha`, and D1.2 now says so. #1134's third gap moved to #1800. - #1851: asserts that the two planted registration defects are themselves reported. - #1854, #1858: correct #1846's test docstrings, which claimed the script makes no `gh` call before the archived check in cases where it does. Raised by Copilot on this pull request. - #1856: passes `--repo` on documented handoff commands and guards handoff reads against a full page, per #1847. - #1859, #1863: fall back to the default search path when `PATH` is unset in #1848's and #1846's test harnesses. Raised by Copilot on this pull request. - #1861: scopes `VerifyReferenceAotCompatibility` to an AOT publish in `dotnet-codestyle`, per #1857. - #1867: establishes the `PATH` order `tool_shadow_path` names, per #1644. - #1870: makes the installer's dirty-checkout tests independent of the real checkout's state, per #1641. - #1873: pins and decodes git's quoting in `repo_gate.py`'s `ls-files` read, per #1580 and #1872. - #1878: folds typographic punctuation in `pr_review.py reply --match`, per #1299. - #1883: routes `configure.sh`'s `ruleset_id()` through `jqr`, per #1253. - #1885: states the pin comment as the release tag and defines `$/`, per #1805. - #1888: distinguishes `./` from `$/` resolution in the pin rule's prose, per #1886. - #1893: drops the issue reference from `repo-config/README.md`'s archived-exemption note, which Copilot flagged on six rounds of this pull request. - #1895: describes IL3058 in `dotnet-codestyle` as a referenced assembly lacking `IsAotCompatible` metadata set to `true`, and drops the unversioned package examples. Raised by CodeRabbit on this pull request. Callers that pin a hub release get the new C# check on their next pin bump. A test project that runs `dotnet test --coverage` without writing a report now fails its step rather than passing silently. Closes #1134 Closes #1847 Closes #1857 Closes #1644 Closes #1641 Closes #1580 Closes #1872 Closes #1299 Closes #1253 Closes #1805 Closes #1886 🤖 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** * Backlog counts and rankings now consistently exclude handoff issues, including those also marked blocked. * Repository configuration commands now exit without writing when a repository is archived. * Review-thread matching handles typographic punctuation, and no-match responses report the unresolved-thread count. * Tool setup handles PATH entries more precisely, and repository checks report unusual file paths without crashing. * **Reliability** * Validation now fails when C# or Python tests produce no coverage report. * Agent setup can use an explicit dirty-checkout override. * Workflow and repository guidance clarifies reference resolution, release-tag pinning, and AOT configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Fixes a finding Copilot raised on the promotion PR #1850, against #1846's test.
scripts/tests/test_configure_archived.py's module docstring said the region under test "runs beforeghis ever invoked". That's false when no repository argument is given:repo-config/configure.shthen resolves the default repository withgh repo viewbefore the region runs. The test runs without a stub for a different reason: the region itself calls onlyjq,sed, andecho, and the harness setsrepoitself. The docstring now says that, and it limits the no-stub claim to the cases that run the lifted region. Elsewhere, the end-to-end case stubsghas a tripwire, and the carriage-return case stubsjq.A search of the tree found no other statement of the old claim. The docstring is the only change, and the module's tests pass. Two local strict review passes are recorded: the first raised the stub-scope point, which the second commit fixes, and the second found nothing.
🤖 Generated with Claude Code