Repository navigation
The hygiene check's end-of-options rule splits a line only outside quotes, and the suite stops counting its bash files (#54 part b, #58) - #239
Merged
Conversation
…otes, and the suite stops counting its bash files (#54 part b, #58) `_end_of_options_after_an_operand` cut each line with a bare `re.split(r"[|;&()]|\$\(|`", line)` before tokenizing, so a quoted pattern holding a separator came apart at it. `lanes` ships `grep -v 'not fetching; reading origin/' -- "$local_err_file"`, the shape BSD's getopt reads `--` in as a filename, and the rule called it clean because the `;` inside the pattern cut the call before its `--`. A quoted message holding `; grep x -- y` was the same cut the other way: a false finding in a string nothing runs. `_shell_tokens` now tokenizes the whole line first and hands each separator back as a token of its own, only where the shell reads it as syntax: never inside single quotes, and inside double quotes only `$(` and a backtick, whose nested command quotes afresh. `_commands_on` cuts that token stream into commands. A line that ends inside a quoted run may be the last line of a multi-line quote, so it is read again from inside each kind of quote; that keeps the second `awk` at the tail of a multi-line program in `lanes-edit.sh`, which a single reading would have swallowed. On the bash files the rule reads (ALL_BASH, claude-current and claude-restart-check) the findings are unchanged but one: `lanes`, at the line above, which #209 gives its `-e`. So this lands after #209; until then `test_no_shipped_bash_ends_its_options_after_an_operand[lanes]` is red here, and it is red because the rule now sees the defect it exists to catch. Fourteen new cases, modelled on the bench rule's red and green rows, each failing against main's copy of the function and passing on this one. Three comments still counted the bash files ("All three files here are bash", "these four bash files", "these three files are bash"); they now say it without a number. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideThe PR fixes the hygiene check’s command parsing by tokenizing shell syntax with quote and nested-command awareness before splitting commands, preventing both missed BSD-incompatible calls and false findings inside quoted text. It adds focused regression tests, handles quote-spanning lines, and removes stale hard-coded Bash-file counts from comments and docstrings. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
… tests this PR on today's main #209 gave lanes:520 its -e (now lanes:526), the shape this PR's corrected end-of-options check catches; on this main the check finds nothing in lanes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…l places and which commands share canon-lane, and the all-or-none test drops its count of nine (#58) The counts #209's review found beside #58's own sites, each measured against the code on 2cb14a9 and false there: - docs/README-lanes.md:76 said "thirteen files"; `INSTALLABLES` in `openRepoTools` holds seventeen (sixteen commands and `repos.tsv`). It now says "every command and the alias table". - docs/README-lanes.md:4 said "the four commands it describes"; the four it names are the ones that came from brett-wip with their history (bd27fc3, 2026-09-13), and the manual now describes many more. It now says "the commands it then described". - tests/test_openrepotools_command.py:395 said "All NINE files" over a loop of `INSTALLED`, seventeen names. It now says "Every `INSTALLED` file". - `test_install_places_all_nine_or_none` withholds `park` and asserts that nothing is placed; nine was never its subject. It is now `test_install_places_all_or_none`, and the comment at :145 that names it follows. Three more of the same kind in the manual, false on the same commit: - :289 and :301, "the four commands" and "the one all four commands share", leaned on :4's count; `canon-lane` is called by lane, lane-start, lane-end, lane-handoff, lane-rename and lane-worktrees besides lanes-edit.sh itself, and `lanes --worktrees <lane>` reaches it through lanes-edit.sh. They now say "the commands here" and "the one the commands share rather than each keeping its own". - :2792 said "not a thirteenth word"; seventeen files are placed. It now says "not one more word". - :4127 said "before any of the twenty-seven artifacts is placed, it walks all thirteen targets"; ARTIFACTS is thirty-one and the bin planner (`openRepoTools:2449`) walks all seventeen `INSTALLABLES`. It now says "before any of its artifacts is placed, it walks every target in the bin directory". Prose and a test's name only; no assertion moved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… tests this PR on today's main Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ay it without a number (#58) The review of #239 found three more sentences of the kind #58 names, which the earlier commits here did not reach. All three are comments or test prose; no behaviour changes. - lane-end, the Amendment 15 block: 'the ONE resolver all four commands share' was never true of four. lane, lane-start, lane-end, lane-handoff, lane-rename and lane-worktrees all call canon-lane, so it now says what docs/README-lanes.md already says, that the commands share it rather than each keeping its own, and the clause (h) reason after it no longer counts implementations either. - tests/test_lane_helpers.sh, the alias-read refusal's comment: 'the four commands that read canon-lane' is now 'the commands that read canon-lane'. - tests/test_repo_hygiene.py, the docstring of the case-in-a-command- substitution rule: 'all twelve files already pass bash -n in CI, and the macOS job is the only place the defect exists' counted a list that holds 14 and named a job that Brett Heap took out of CI on 2026-10-15 (#227). It now says every file in ALL_BASH passes bash -n under the Linux bash CI runs, and bash 3.2 is the only place the defect exists. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…nting the commands (#58) 5f1c329 reworded lane-end's copy of the clause that gives the reason for one canon-lane resolver, so it holds no number. The same clause was written out three more times, and each still counted: 'four implementations of which row is this name's is how the four would come to disagree about it'. Six commands call canon-lane today, so the four copies now said two different things. - lane, the Amendment 15 block above its canon-lane call: 'four implementations of *which row is this name's* is how the four come to disagree about it' is now 'an implementation of *which row is this name's* in each command is how they come to disagree about it'. - lane-start, the ONE RESOLVER, SHARED comment: the same clause, in its own tense and quoting, now 'an implementation of "which row is this name's" in each command is how they would come to disagree about it'. - lanes-edit.sh, the comment above the canon-lane subcommand: the same clause, and 'gives all four the SAME resolver' just before it, which counted the commands in the same sentence, is now 'gives them all the SAME resolver'. Comments only; each file keeps its own markup, indentation and surrounding sentence, and the lines are re-wrapped only where the shorter words moved them. No test quotes the clause. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
brettheap
added a commit
that referenced
this pull request
Oct 18, 2026
… act on every verified child, not the first (#76, #86) (#205) Landed on Brett Heap's word "land #205 #222 #223 #239 when green" (2026-10-18), by lane openRepoTools-5. Required Linux CI (guard-launch-mode, tests, tests-no-submodule; run 39148145583) was green on head 4c7e847 with main babd9fe in its tree. Main a059c1a merged with head 4c7e847 is tree 55e51f6, which queue run 39154616302 (workflow_dispatch of tests on cb9da52) tested whole and green. Independent review (lane 5, Opus, of d4cf2ee) and delta review of de0809c with its polish 0cf7417 confirmed: LAND; the merges of main add nothing to the PR's own lines. The --resume condition is Brett Heap's ruling of 2026-10-18. Closes #76 Closes #86 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap
added a commit
that referenced
this pull request
Oct 18, 2026
…ct is read off the lane's live window from any window, and the guard's cure for a lane named before Rule 4 says what it cannot fill in (#73) (#222) Landed on Brett Heap's word "land #205 #222 #223 #239 when green" (2026-10-18), by lane openRepoTools-5. Required Linux CI (guard-launch-mode, tests, tests-no-submodule; run 39146171277) was green on head 0b72a5c with main babd9fe in its tree. Main a656627 merged with head 0b72a5c is tree 7aee6bf, which queue run 39154621848 (workflow_dispatch of tests on 0dbafc9) tested whole and green. Independent review (lane 5, Opus, of 2946a41): LAND; the merges of main add nothing to the PR's own lines. Closes #73 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap
added a commit
that referenced
this pull request
Oct 18, 2026
… being bound, so the offer's yes no longer records a RESUMED nobody is in (#70) (#223) Landed on Brett Heap's word "land #205 #222 #223 #239 when green" (2026-10-18), by lane openRepoTools-5. Required Linux CI (guard-launch-mode, tests, tests-no-submodule; run 39096012830) was green on head e40c067 with main 2cb14a9 in its tree. Main 0f53e40 merged with head e40c067 is tree d2da196, which queue run 39154627955 (workflow_dispatch of tests on 2109b12) tested whole and green. Independent review (lane 5, Opus, of a4acad3) and delta review of e40c067: LAND; the merges of main add nothing to the PR's own lines. Closes #70 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap
marked this pull request as ready for review
October 18, 2026 19:53
|
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 2 days by commenting |
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.



Closes #54 (its part b, the checker; #209 did its part a). Closes #58 (its last count sites; #209 did the rest).
Landing order: after #209, which landed on main as 2cb14a9 (2026-10-18T04:01Z). Before it landed, this branch was red on exactly one case,
test_no_shipped_bash_ends_its_options_after_an_operand[lanes]: the corrected rule caughtlanes:520'sgrep -v 'not fetching; reading origin/' -- "$local_err_file", the BSD-incompatible shape #54 reported, which #209 gave its-e(part a). With main merged in (9332add),tests/test_repo_hygiene.pyis green (203 passed) with no exception added, and the required Linux CI passed at bb3c7dc.With #209 on main, this PR completes #54 and #58, so it closes both.
What was wrong
_end_of_options_after_an_operandcut each line with a barere.split(r"[|;&()]|\$\(|", line)before tokenizing. That cut does not see quotes. A quoted pattern holding;,&,|,(,)` or a backtick came apart at that character:[]forlanes, although line 520 has the shape the rule exists to catch. The;inside'not fetching; reading origin/'cuts the call before its--, so neither half holds both the operand and the--. restart:247 and lanes:311 carry the same BSD-incompatible grep -v 'pattern' -- file shape PR #53 fixes at skills/restart/SKILL.md:58, and the hygiene checker's own text-split can't see it #54's body shows the BSD failure withPOSIXLY_CORRECT=1 grep -v 'not fetching; reading origin/' -- /tmp/x→grep: --: No such file or directory.; grep x -- ybecame a new "command" after the;and was flagged, although nothing runs it.What changed
_shell_tokensnow tokenizes the whole line first. It hands each command separator back as a token of its own, but only where the shell reads it as syntax:$(and a backtick are, because they open a nested command whose quotes start afresh. The matching)is tracked, so the outer double quote resumes in the right place:out="$(sed -n 'p' -- "$f")"still yields the innersedcall._commands_oncuts that token stream into commands at the separator tokens, as restart:247 and lanes:311 carry the same BSD-incompatible grep -v 'pattern' -- file shape PR #53 fixes at skills/restart/SKILL.md:58, and the hygiene checker's own text-split can't see it #54 asked._end_of_options_after_an_operand's per-command logic is unchanged; only its loop header now reads_commands_on(line).Quotes that span lines. The rule still reads one line at a time. A line that ends inside a quoted run could be the last line of a multi-line quote. Read from outside every quote, its closing quote would open one instead and swallow the rest of the line. For example, the tail of a multi-line awk program in
lanes-edit.sh(}' | LC_ALL=C sort | awk -v s="$US" '{ … }')") would lose its secondawkcall. So a line that ends inside a quote is read again from inside a single-quoted run and from inside a double-quoted one, and every reading's commands are checked. A line that closes every quote it opens is read once, as before.The prose still says "four" about inventories that hold six, ten and eleven — six sites, all predating #45 #58. Three comments still counted the bash files. They now say it without a number:
README.md). "The files here are all bash" would be false, becauselanes-indexandlane-worktreesarepython3.conftest.py, "these three files are bash" → "there is no Windows twin to run instead for any shipped bash file".These are comment and docstring changes only.
Functions and regions edited
tests/test_repo_hygiene.py: test_no_document_offers_a_windows_powershell_twin(docstring only)tests/test_repo_hygiene.py: test_the_root_carries_the_line_ending_rule(docstring only)tests/test_repo_hygiene.py: _COMMAND_SEPARATORS(new constant, after_OPTIONS_WITH_ARGUMENTS)tests/test_repo_hygiene.py: _shell_tokenstests/test_repo_hygiene.py: _commands_on(new)tests/test_repo_hygiene.py: _end_of_options_after_an_operand(loop header only)tests/test_repo_hygiene.py: LATE_END_OF_OPTIONS_REFUSED,LATE_END_OF_OPTIONS_ACCEPTED,test_the_end_of_options_rule_sees_a_call_whose_quotes_hold_a_separator,test_the_end_of_options_rule_finds_no_call_inside_a_quoted_string(new, directly aftertest_no_shipped_bash_ends_its_options_after_an_operand)tests/conftest.py: the comment block aboveWINDOWS_SKIP(one line)lane-end: the comment in the Amendment 15 block above thecanon-lanecall (comment only)tests/test_lane_helpers.sh: the comment above the alias-read refusal'scanon-lanecase (comment only)tests/test_repo_hygiene.py: test_no_shipped_bash_opens_a_case_inside_a_command_substitution(docstring only)lane: the comment in the Amendment 15 block above itscanon-lanecall (comment only)lane-start: the "ONE RESOLVER, SHARED" comment in its Amendment 15 block (comment only)lanes-edit.sh: the comment above thecanon-lane)subcommand (comment only)Behaviour on today's files, measured
I extracted the function from main's copy and from this branch's copy (via
ast, no conftest import) and compared them over every file the rule reads: the 14ALL_BASHfiles, plusclaude-currentandclaude-restart-check, whichtests/test_claude_current.pyholds to the same rule.Findings: unchanged in 15 of 16 files. The one change is a new finding at
lanes:520, described above.Calls examined: in four lines, the old cut examined a call that the new reading does not:
lanes-edit.sh:2163is anote "…| grep -c '^+' …"message.tests/test_lane_helpers.sh:11732/11735/11739have asedinside a single-quotedbash -c '…'body.The old cut saw these only in pieces, split at the
(of their pattern. The new reading treats single-quoted text as text, which is what restart:247 and lanes:311 carry the same BSD-incompatible grep -v 'pattern' -- file shape PR #53 fixes at skills/restart/SKILL.md:58, and the hygiene checker's own text-split can't see it #54 asks for. Three lines now show a bare--that the old cut hid:openRepoTools:1661andlane:371(both the correct-e … --shape, no finding) andlanes:520. No line lost a--.Not changed here (pre-existing): the rule reads
git grepasgrep, sogit grep -n 'x' REV -- pathwould be flagged. That happens with or without a separator in the quote, and no file the rule reads has the shape today.Tests
There are 14 new parametrized cases, modelled on the bench-root rule's red and green
(why, text)rows:&in a sed replacement,()in a double-quoted pattern, a;in an awk program, a backtick in a single-quoted pattern, a nested$(…)inside double quotes, and the closing line of a multi-line awk program followed by a second call.;,&&,|,()or backticks, and a correct-ecall whose pattern holds; sed … --.Fail before: I appended the new block, byte-identical, to
git show origin/main:tests/test_repo_hygiene.pyin a scratch tree beside main'sconftest.py, and ran it withOPENREPOTOOLS_CODE_ROOTpointed at this worktree and everyLANES_*/TMUX*variable unset:Pass after, in this worktree with the same environment:
python3 -m py_compileis clean on both changed files. I did not run the full suite locally; CI'stestsjob is the suite of record.The remaining #58-shaped counts (bb3c7dc, after #209 landed)
#209's independent review found counts of #58's kind in files #209 edits, which #58 did not list. Each true count was measured from the code (
INSTALLABLESatopenRepoTools:1066holds 17: 16 commands plusrepos.tsv), and each sentence now says it without a number where a number would drift again:docs/README-lanes.md:76"thirteen files" → "every command and the alias table";docs/README-lanes.md:4"the four commands it describes" → "the commands it then described" (the four that came from brett-wip with their history);docs/README-lanes.md:289and:301, which leaned on :4's "four" and were false anyway (canon-laneis called bylane,lane-start,lane-end,lane-handoff,lane-renameandlane-worktreesbesideslanes-edit.sh) → "the commands here" and "the one the commands share rather than each keeping its own";docs/README-lanes.md:2792"not a thirteenth word" → "not one more word";docs/README-lanes.md:4127"twenty-seven artifacts" and "all thirteen targets" (31 and 17 now) → "any of its artifacts" and "every target in the bin directory";tests/test_openrepotools_command.py:395"All NINE files" → "EveryINSTALLEDfile", and the testtest_install_places_all_nine_or_none→test_install_places_all_or_none(it withholds onlypark, so nine was never its subject).Proof, with every
LANES_*andTMUX*variable unset: the two touched install tests pass;tests/test_repo_hygiene.pyplustests/test_skill_shell_blocks.py(which reads the manual) give 209 passed; and of the 10,748 quoted strings in the six test files that readdocs/README-lanes.md, none changed whether it matches.Three more, which the independent review at bb3c7dc found, are now fixed too (5f1c329). They are the same kind and #58 does not list them, but this PR closes #58. Comments and test prose only; no behaviour changes:
lane-end, the Amendment 15 block: "the ONE resolver all four commands share" → "the ONE resolver the commands share rather than each keeping its own", the wordingdocs/README-lanes.md:301has. The clause (h) reason after it, "four implementations of … the four would come to disagree", now reads "an implementation of … in each command … they would come to disagree", so the sentence holds no number.tests/test_lane_helpers.sh, the comment above the alias-read refusal: "the four commands that readcanon-lane" → "the commands that readcanon-lane".tests/test_repo_hygiene.py, the docstring oftest_no_shipped_bash_opens_a_case_inside_a_command_substitution: "all twelve files already passbash -nin CI, and the macOS job is the only place the defect exists" → "every file inALL_BASHalready passesbash -nunder the Linux bash CI runs, and bash 3.2 is the only place the defect exists".ALL_BASHholds 14, and the macOS job the old sentence named was taken out of CI on 2026-10-15 (ci: run the test suite on Linux only #227).Proof for 5f1c329, with every
LANES_*andTMUX*variable unset:bash -n lane-end tests/test_lane_helpers.shandpy_compile tests/test_repo_hygiene.pyare clean;tests/test_repo_hygiene.pygives 203 passed;command grep -n 'all four commands\|all twelve files'over the three files prints nothing.git merge-tree --write-tree --name-onlyof origin/main plus this commit against the heads of #202, #204, #205, #206, #208, #218, #220, #221 and #223 merges clean for each (#213 has landed on main).The same clause, written out in three more places, is fixed too (7c4df04). Rewording
lane-end's copy left the other three still counting, and the four copies would then have said two different things. Comments only; each file keeps its own markup, indentation and tense, and the lines are re-wrapped only where the shorter words moved them:lane(the Amendment 15 block above itscanon-lanecall): "four implementations of which row is this name's is how the four come to disagree about it" → "an implementation of which row is this name's in each command is how they come to disagree about it".lane-start(the "ONE RESOLVER, SHARED" comment): "four implementations of "which row is this name's" is how the four would come to disagree about it" → "an implementation of "which row is this name's" in each command is how they would come to disagree about it".lanes-edit.sh(the comment above thecanon-lane)subcommand): the same clause, and "the amendment gives all four the SAME resolver" just before it, which counted the commands in the same sentence → "gives them all the SAME resolver" and "An implementation of "which row is this name's" in each command is how they would come to disagree about it".Proof for 7c4df04, with every
LANES_*andTMUX*variable unset:bash -n lane lane-start lanes-edit.shis clean;command grep -in 'four implementations\|the four would\|the four come'overlane,lane-start,lanes-edit.shandlane-endprints nothing;tests/test_repo_hygiene.pygives 203 passed. No test quotes the clause.git merge-tree --write-tree --name-onlyof origin/main (586dea2) plus this commit against the heads of every open PR that toucheslane,lane-startorlanes-edit.sh(#204, #205, #206, #208, #210, #214, #218, #219, #220, #221, #223, #230, #246) merges clean for each. #222 conflicts with origin/main intests/test_lane_helpers.shlines 101-107 on its own, with or without this commit.Remaining, not in scope here
die-message extraction in this same file, stays with PR #50 round 5 (Copilot 5203628254): commit_push's recovery block leaves $LANES_REPO unquoted, git cherry unbounded for multi-commit checkouts, and git_timeout_die still push-shaped for a pull timeout #68, whose other items wait for lane 4's lanes-edit.sh: a write with nothing staged still publishes the local commits origin lacks, so the RECOVERY's re-run ofcommitpushes the edit it stranded, and the conflict RECOVERY warns thatreset --harddrops a peer's commits #214.specs/004-migrate-to-triad/installer-2026-10-08.md:125still cites the old test name; it is a dated record, accurate when written, and off-limits here.🤖 Generated with Claude Code
Summary by Sourcery
Correct shell command boundary handling in the hygiene checker and remove stale shipped-file counts from documentation and test prose.
Bug Fixes:
Enhancements:
Documentation:
Tests: