Repository navigation
openRepoShape: --doctor checks $OPENREPOSHAPE_REF before it clones the standard, so a ref checked_ref refuses (a.lock/b, a//b, one over 255 characters) makes no clone and, where the clone fails, is still refused in its own words - #967
Conversation
…s the standard, so a ref `checked_ref` refuses (`a.lock/b`, `a//b`, one over 255 characters) makes no clone and, where the clone fails, is still refused in its own words, and the test that names the order pins it with a `git` that logs what it is asked (#942) `shape_checkout` ran `git clone` and then `checked_ref "$REF"`, so a ref the rule refuses cost a whole clone first, and where the clone failed (offline, a mirror that is down) the run ended in "could not clone", never naming the ref. `checked_ref` reads only the value, and nothing between the two lines needs the clone, so the call now comes first, after `shape_clone_url` and before `workdir`: `[ "$REF_NAMED" -eq 0 ] || checked_ref "$REF"`. It stays in `shape_checkout`, not `refuse_bad_values`, whose check of the ref is its characters only on purpose (`--install` takes `bad..ref` with no opt-in, and `--doctor` beside a checkout never uses the ref). A refused ref now leaves no directory in `$TMPDIR` at all. The comment above the function says the order and why. `test_a_ref_the_doctor_would_check_out_is_validated` named this order in its docstring and did not pin it: it passed with the call above or below the clone, and each of its five cases made a real clone of this checkout. It runs with a `git` first on `$PATH` that logs what it is asked and fails every clone, over ten refs (its five, and `a.lock/b`, `a//b`, `a.`, `a/b/`, `topic.lock`): exit 2 in `ref_fault`'s words for the value, no clone and no checkout asked of git, nothing left in `$TMPDIR`. Two tests are new: a good ref (and no ref) still clones once and checks out once, and a bad ref with `OPENREPOSHAPE_REPO=/nonexistent/...` ends in the ref's refusal, not "could not clone", while a good ref there still meets the clone's. The five-ref test no longer clones: seconds, not 188. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Reviewer's Guide
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
(lane openRepoShape-2, 2026-10-18T18:39:41Z) Independent review of #967 (#942) at Reviewer rv967 (opus), who wrote none of the PR. Its report was stamped 2026-10-18T18:36:29Z. Lane 2 sends the SHOULDs and NITs below to its writer as one fix round, and asks the same reviewer to re-check the new head before the PR leaves DRAFT. SHOULD 1.
SHOULD 2.
NIT 1.
NIT 2.
Checked and true
Could not verify
Lane: openreposhape-2 (openRepoShape-2) |
…efused `$OPENREPOSHAPE_REF` is held to be refused above `workdir`, so it is named even where `$TMPDIR` is not there; the Windows job's `fetch-depth: 0` comment no longer says a test clones this checkout; two comments say the ref is refused before the clone (#942) Review round 1 on #967 (rv967's READY, two SHOULDs and two NITs). SHOULD 1. Nothing held `checked_ref` above `workdir`: swapped below it, all 457 tests of the file still passed, because the clone count stays 0 and the EXIT trap removes the empty folder. With `$TMPDIR` missing the swap, like the old code, ends in "could not make a temporary directory" and never names the ref. `doctor_from_stdin` takes the `$TMPDIR` it hands the run, and a new test runs `a.lock/b` and 256 `a` against one that does not exist: exit 2 in `ref_fault`'s words, git asked nothing, no "could not make a temporary directory", and the folder still not made; a good ref there still ends in that refusal. It fails on the swap and on the launcher at `a4de0aae`, and passes at the head. The helper looks for what is in `$TMPDIR` with `glob`, which a missing folder does not break. The first commit's message said a refused ref "leaves no directory in `$TMPDIR` at all", which was true before too: the trap removed the clone. What changed is that a refused ref makes none. SHOULD 2. The Windows job's comment said `fetch-depth: 0` was there because the launcher's suite clones this checkout itself, in the test this change makes run a fake `git`. None of its three files (the launcher's, `test_install_sh.py`, `test_install_sh_filter_repo.py`) clones it or reads its history now. The comment says so, as the ubuntu job has the setting, and the setting stays: a shallow Windows checkout is a change of its own and wants a Windows run behind it. NIT 1. The helper's docstring named a control-character refusal it does not handle, and none reaches `shape_checkout` (`refuse_bad_values` refuses it first). NIT 2. `ref_fault`'s comment says the doctor's ref is refused before its clone, not before `git checkout`. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
(lane openRepoShape-2, 2026-10-18T19:03:31Z) Delta re-check of #967 (#942) at Reviewer rv967 (opus), the same reviewer as issuecomment-6253648449. Its report was stamped 2026-10-18T18:59:46Z. The re-check covers one new commit on Lane 2's notes:
New findings MUST 1.
SHOULD 3. The PR body is stale in three places after the fix round.
Earlier findings: all fixed
Runs (one pytest process at a time, gh stub first on
Merges (
Lane: openreposhape-2 (openRepoShape-2) |
…`$TMPDIR` test expects the folder as bash spells it, so it passes under Git Bash as well as on Linux (#942) `test_a_ref_is_refused_before_a_directory_is_made_for_the_clone[a-good-ref]` expected "could not make a temporary directory under {missing}." with the native path, and the launcher prints `$TMPDIR` as bash spells it: under Git Bash `/d/a/.../not-there`, where the native one is `D:\a\...`. The `tests-git-bash` job failed that one row (1 failed, 337 passed, 410 skipped at d5d2152). The expected text goes through `bash_path`, which returns the path unchanged off Windows. No other expected text in the file's new tests holds a native path: the others name a ref, a URL, a fixed word or a `git` subcommand. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
(lane openRepoShape-2, 2026-10-18T20:25:33Z) Delta re-check of #967 (#942) at Reviewer rv967 (opus), the same reviewer as issuecomment-6253648449 and issuecomment-6253889845. Its report was stamped 2026-10-18T20:24:24Z. This round is one new commit on Findings
Run: Merge: Lane 2's note: #967 leaves DRAFT once its Lane: openreposhape-2 (openRepoShape-2) |
|
|
Sorry @brettheap, you've used your own review budget of 250,000 diff characters for the last 7 days. You can request another review in 12 hours and 5 minutes by commenting |



Lane: openreposhape-2 (openRepoShape-2)
Claim: #942 (comment)
Milestone v1.2.0. Brett Heap's v1.2.0 merge word, "merge v1.2.0 PRs once reviewed and green" (2026-10-18T14:45:28Z, relayed on #473 issuecomment-6251255596), lands it once an independent review is READY at the landing head and
testsand SonarCloud are green. It stays a DRAFT until then. Commits ona4de0aae, one squash at landing; nothing stacked.What changed
openRepoShape, inshape_checkoutand in one comment ofref_fault;tests/test_openreposhape_command.py; and one comment in.github/workflows/tests.yml.shape_checkoutrangit cloneand thenchecked_ref "$REF", so a ref the rule refuses cost a whole clone first, and where the clone failed (offline, a mirror that is down) the run ended in "could not clone" and never named the ref.checked_refreads only the value, and nothing between the two lines needs the clone (what lies between is the clone's own refusal text and theif;ref_faultandchecked_refnever look at$SHAPE_DIR). So the call moves up, aftershape_clone_urland beforeworkdir, and the later one goes:It stays in
shape_checkout, notrefuse_bad_values: the up-front check of the ref is its characters only, on purpose (--installtakesbad..refwith no opt-in,test_a_ref_set_without_the_opt_in_is_never_recorded), and--doctorbeside a checkout never uses the ref. A refused ref now makes no$TMPDIRfolder at all, so it is refused in its own words even where$TMPDIRis not there (the folder was made first, so before,workdir's "could not make a temporary directory" came before the ref was asked). The comment aboveshape_checkoutsays the order and why; the one inside it sayschecked_refruns "above, before the clone"; andref_fault's comment says "the doctor's ref is refused before its clone", where it said the doctor's checkout is refused beforegit checkout.The refusal's words are not touched. What a run does, measured with a
gitfirst onPATHthat logs its arguments and runs/usr/bin/git(an installed copy of the launcher with no checkout beside it,--doctor <an empty directory>,OPENREPOSHAPE_REPOa small local bare repository, a stubghthat exits 97,GH_TOKEN=bogus), before (a4de0aae) and after:Every refused row leaves
$TMPDIRempty before and after. Run time is the clone's: 140 to 350 ms before and 45 to 85 ms after for a refused ref against a one-commit repository, which is the clone's setup and nothing of its size..github/workflows/tests.yml, the comment above the Windows job's checkout (tests-git-bash): it saidfetch-depth: 0is there because the launcher's suite clones this checkout itself, in the test above, which now runs a fakegit. None of the job's three files (test_openreposhape_command.py,test_install_sh.py,test_install_sh_filter_repo.py) clones this checkout or reads its history. The comment now says: "fetch-depth: 0, as the ubuntu job has it: no test in these three files has cloned this checkout since #942. A shallow checkout here is a change of its own, and wants a Windows run behind it." The setting stays.Tests
All in
tests/test_openreposhape_command.py. The file went from 447 tests to 460: the five-ref test is ten, and three tests are new (four rows, one, and three). New helpers:doctor_from_stdin(runs--doctorfrom stdin with agitthat logs each call, leading-C <dir>dropped, fails every clone or makes the empty directory it is handed, and takes the$TMPDIRit hands the run),asked_of_gitandassert_the_ref_was_refused_before_any_clone.test_a_ref_the_doctor_would_check_out_is_validated(10 refs:-x,--upload-pack=touch /tmp/pwned, 256a,refs/heads/..,x.lock,a.lock/b,a//b,a.,a/b/,topic.lock)ref_fault's words for the value (the rule's words, or the length sentence without the value echoed), nocloneand nocheckoutasked of git, no "could not clone", nothing left in$TMPDIRREF_NAMEDtest inverted: 10 failedtest_a_ref_the_doctor_takes_still_clones_once_and_checks_out_once(no ref,main,v1.2.0,release/v1.2.0-rc_1)cloneofhttps://github.com/acme/x.git, then onecheckout --quiet <ref> --(none with no ref named), and the run reaches the doctor's own "has no shape-doctor.py"git checkoutline dropped: the three named rows failtest_a_ref_is_refused_where_the_clone_would_have_failedOPENREPOSHAPE_REPO=/nonexistent/openRepoShape.git,a.lock/bends in the ref's refusal with no clone asked, andv1.2.0still ends in "could not clone" after one clone and no checkoutREF_NAMEDinverted: each fails it (11 failed with the ten above)test_a_ref_is_refused_before_a_directory_is_made_for_the_clone(a.lock/b, 256a, andv1.2.0as the control)$TMPDIRa folder that is not there, a refused ref ends inref_fault's words (exit 2) with git asked nothing, no "could not make a temporary directory" and the folder still not made; a good ref still ends in that refusal, naming the folder as bash spells it (bash_path)checked_refmoved belowworkdir: 2 failed (the two refused rows); the check below the clone and the base launcher fail them too (13 failed on the file with the rows above)The ten-ref test no longer clones this checkout: it took 16.2 s for its five cases before (3 s each, a load of about 30; 188 s in the issue's run at a load above 100), and the four groups take 2.5 s for 18 tests.
Not changed
refuse_bad_valuesandref_character_fault: the up-front check stays characters only.checked_repoandref_character_faultare openRepoShape, setup.sh and install.sh: under theClocale a$OPENREPOSHAPE_REPOURL or path, or a ref, with a letter past ASCII is refused as "a control character", so--version,--installand--preflightrefuse what v1.1.0 ran #940's.read_recorded_ref: a recorded ref is held toref_faultas it is read, andshape_checkoutaskschecked_refof it again, first now, as it did after the clone; nothing it refuses changes.setup.shandinstall.sh, which check the ref before they clone or fetch (setup.sh, install.sh and the openRepoShape command: each keeps its own copy of the old ref rule (*..* | *.lock), so a ref git refuses part by part (a.lock/b,a/.b,a//b) clones the standard before it fails #874).fetch-depth: 0in.github/workflows/tests.yml: the setting stays; a shallow Windows checkout is a change of its own and wants a Windows run behind it.git grepofdocs/,README.md,AGENTS.mdandopenRepoShapefor "clones this checkout", "before any clone", "checks the ref", "clone comes first" and "clone first" finds no sentence that states the order.Closes #942
🤖 Generated with Claude Code
Summary by Sourcery
Validate doctor refs before preparing or cloning the standard so invalid values are rejected immediately and consistently.
Bug Fixes:
Enhancements:
CI:
Tests: