Repository navigation
duplicate-holder and lane-end --retire read a --resume path whole and act on every verified child, not the first (#76, #86) - #205
Merged
Conversation
… act on every verified child, not the first (#76, #86) #76: `ps -o args=` joins a process's argv with spaces and quotes nothing, and all four `--resume` reads (both matches in `duplicate_holder_pids`, both pre-kill rechecks in `lane-end`) cut the value at the first space. A duplicate resumed from a directory with a space in it was never found, and the recheck would have withheld its TERM. The value now runs to the first `.jsonl` that ends an argument, so a directory with a space is read whole while Copilot round 5's `--resume /other.jsonl --label /<id>.jsonl` still reads `/other.jsonl`; a value that would span a later `--option` is refused. One rule, `dhp_resume_value` in lanes-edit.sh and its word-for-word copy `dh_resume_value` in lane-end, with its known limits written beside it. #86: the child scan stopped at the first child that verified and the record had one child slot, so `lane-end --retire` TERMed the parent and one child, said RETIRED, and left a second child holding the transcript. The slot is now a space-separated list of every verified child; lane-end matches its target against each, rechecks and TERMs each, settles each, and refuses as partly retired if any one is still alive. Its dry-run line and its refusal listing name every child too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideThis PR fixes duplicate-holder detection and retirement for spaced --resume paths, and closes a safety gap where only the first verified child was acted on. It centralizes the path-shape rule across discovery and pre-kill validation, records every matching child, and makes lane-end signal, settle, report, and success determination depend on the entire verified process group. Sequence diagram for duplicate-holder discovery and complete retirementsequenceDiagram
participant LE as lanes-edit.sh
participant PS as Process table
participant LaneEnd as lane-end
participant Parent as Duplicate parent
participant ChildA as Verified child A
participant ChildB as Verified child B
LE->>PS: pgrep -f duplicate candidates
LE->>PS: ps -o pid,ppid,args per candidate
LE->>LE: dhp_resume_value(args)
LE->>PS: pgrep -P parent
loop Every child
LE->>PS: ps -o args= child
LE->>LE: dhp_resume_value(child args)
end
LE-->>LaneEnd: parent + child A + child B record
LaneEnd->>LaneEnd: dh_resume_value(parent and children)
LaneEnd->>Parent: kill -TERM
LaneEnd->>ChildA: kill -TERM
LaneEnd->>ChildB: kill -TERM
LaneEnd->>Parent: dh_settle_gone
LaneEnd->>ChildA: dh_settle_gone
LaneEnd->>ChildB: dh_settle_gone
alt Parent and every child gone
LaneEnd-->>LaneEnd: RETIRED
else Any process remains
LaneEnd-->>LaneEnd: Refuse as partly retired
end
Flow diagram for whole --resume matching and child handlingflowchart TD
A[Read flattened process argv] --> B[Find first --resume value]
B --> C{Find .jsonl ending an argument?}
C -- No --> D[No match]
C -- Yes --> E{Value contains later -- option?}
E -- Yes --> D
E -- No --> F{Resume path matches transcript UUID?}
F -- No --> D
F -- Yes --> G[Verify parent or child]
G --> H[Append every verified child to record]
H --> I[lane-end rechecks each recorded PID]
I --> J[TERM parent and every verified child]
J --> K[Settle and report each PID]
K --> L{All PIDs gone?}
L -- Yes --> M[Print RETIRED]
L -- No --> N[Exit 1: partly retired]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was referenced Oct 13, 2026
…86, so CI tests this PR on today's main Main moved 15 commits past this branch's base f87fd5a, through the suite's machinery and pin (.github/workflows/tests.yml, contracts/openreposhape-pin.yaml, tests/conftest.py, tests/test_repo_hygiene.py, upstream/openRepoShape), after this PR's last green run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… starting with / and holding no ' /', in both copies of the read Brett Heap's ruling of 2026-10-18 on PR #205, verbatim "Add the condition first (Recommended)". The independent review of d4cf2ee found that reading the --resume value whole made five argv shapes that main ignores MATCH: a bare session id followed by a prompt naming the transcript (two forms), an empty argument before the path, a <id>.jsonl.bak followed by a word naming the transcript, and a shell -c string that resumes $ID and then tails the file. That is the dangerous direction, because lane-end's pre-kill recheck uses the same rule. The condition refuses all five and keeps every real match on the reviewer's 19-case table, spaced home and project directories included; a value with no space in it reads as before. The same four lines go into lanes-edit.sh's dhp_resume_value and lane-end's dh_resume_value, which stay word for word the same. The comment above dhp_resume_value says what the condition refuses and what it still cannot tell apart, and the manual's duplicate-holder paragraph states the rule. Two rows join the SPAN case of tests/test_lane_helpers.sh: a bare session id followed by a prompt, and a <id>.jsonl.bak followed by a word naming the transcript. With 8b16668's code they fail (duplicate-holder names both, exit 0); here they pass, and the SP case's spaced path still matches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…86, so CI tests this PR on today's main Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… row, test that lane-end's copy of the read stays identical to lanes-edit.sh's, and say exactly which shapes the rule still leaves open The independent delta review of de0809c gave LAND with these non-blocking points, all done here. A third SPAN row, 900205 (`--resume <wrong id> summarise nonexistent/projects/<id>.jsonl`), holds no ' /', so only the start-with-/ line of the condition refuses it. Rows 900203 and 900204 are both refused by the ' /' half alone, so deleting lanes-edit.sh's start-with-/ line still gave 75 passed. lane-end's dh_resume_value had no test: removing it alone still gave 75 passed. A case in the repoM-1 block now extracts dhp_resume_value from lanes-edit.sh and dh_resume_value from lane-end by awk, maps the dhp_/dh_ prefix, takes lane-end's indent off, checks that each extraction is a whole function and compares the two with diff. Three comments now say what the rule does: - lanes-edit.sh's known limits no longer call the open shape "a RELATIVE path": it is a later word that does not begin with `/`, which also covers `~/`, `$HOME/`, `file:///`, `path=/` and `(/`. - lane-end no longer implies that every <uuid>.jsonl.bak form is refused: only one whose later path is reached through ' /' is. - lanes-edit.sh's "a transcript path does neither" now excepts a directory whose name ends in a space before its `/`, which the known limits cover. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…86, so CI tests this PR on today's main One conflict, in docs/README-lanes.md at the end of the duplicate-holder paragraph: #221 (now on main) added the clause "and a register that cannot be read is never 'no duplicate': the row's ids are the whole search, so it exits 1 and names the read (#67)" in front of the lane-end --retire sentence, and this PR rewrote that sentence for #86 (TERM every child, not only the first; report each pid; say RETIRED only once all are gone). Both stay: #221's clause first, then this PR's sentences, which read on from it unchanged. Nothing else conflicted; lanes-edit.sh and tests/test_lane_helpers.sh merged cleanly. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
brettheap
marked this pull request as ready for review
October 18, 2026 19:50
|
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 and 23 hours by commenting |
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
added a commit
that referenced
this pull request
Oct 18, 2026
…otes, and the suite stops counting its bash files (#54 part b, #58) (#239) 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 39136612565) was green on head 7c4df04 with main 586dea2 in its tree. Main 7748375 merged with head 7c4df04 is tree 8c13769, which queue run 39154634006 (workflow_dispatch of tests on 1d149e3) tested whole and green. Independent review (lane 5, Opus, of bb3c7dc): LAND; the later commits 5f1c329 and 7c4df04 change comments only. Closes #54 Closes #58 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.



What was wrong
#76: a
--resumepath with a space in it was cut at the space.ps -o args=joins a process's argv with spaces and quotes nothing. All four--resumereads cut the value at the first space with${v%% *}: both matches inlanes-edit.sh'sduplicate_holder_pids, and both pre-kill rechecks inlane-end. Here is whatpsactually prints on Linux, for an argv with a spaced path:The old cut took
/home/janefrom that line. As a result:duplicate-holderanswered 8 over a duplicate that was really there.lane-end's recheck still cuts the value. The mixed run below shows it printingSTILL ALIVE, TERM withheld: argv no longer matchedand withholding the TERM.#86: only the first verified child was named.
duplicate_holder_pids' child scan didbreakon the first child that verified, and the output record had one child slot. With two children both resuming the transcript,lane-end --retireTERMed the parent and the first child and printed RETIRED. The second child kept running and holding the transcript. Measured on main f87fd5a with the new fixture:What changed
The
--resumevalue is now read whole (#76). One rule replaces the cut, asdhp_resume_valueinlanes-edit.shand a word-for-word copy,dh_resume_value, inlane-end:--resumeto the first.jsonlthat ends an argument, meaning one followed by a space or by the end of the line.--falls inside the value, the value is empty: that word is a later option, not part of a directory name./and must not contain/. Otherwise the value is empty. This is Brett Heap's ruling of 2026-10-18, in its own section below.The coordinator's sketch matched
*"/$uuid.jsonl"anywhere after--resume. That form would have reopened PR #61's Copilot round 5 (--resume /other.jsonl --label /<id>.jsonlmatching), so I tightened it as above. Both earlier guarantees still hold:/other.jsonl;<id>.jsonl.bakandother-<id>.jsonlstill don't match.I chose one portable
psreading over the issue's other option,/proc/<pid>/cmdlineon Linux with a fallback elsewhere. That way there is a single rule on every platform, and it stays testable under the suite's fakeps. Its known limits are written in the comment abovedhp_resume_value:.jsonlor--, or ends in a space before its/, is missed. That is the safe direction.--resumevalue that starts with/but doesn't end in.jsonl, followed by a later word that names<id>.jsonland does not begin with/, as in--resume /p/<id>.jsonl.bak note q/<id>.jsonl. Main ignores that shape, and this rule still matches it. See "A shape the condition does not close" below.Every verified child is now named and acted on (#86).
pgrep -P's order. The fallback rule (a sole unreadable child) is unchanged.lane-endcompares the target with each pid in the list by equality, so--retire=<two pids>matches nothing.dh_settle_goneand gets its own report line.lane-endrefuses as partly retired, exit 1.--dry-runline and the refusal listing name every child.The record format change is safe across version skew. If a new
lanes-edit.shruns beside an oldlane-end, the old one fails closed: it rechecks the whole list as one pid, withholds the TERM and refuses, exit 1. It never kills the wrong process and never prints a false RETIRED (shown in the mixed run).git grep -n "duplicate-holder\|duplicate_holder_pids": the only reader of the record islane-end, at the target match, the own-session fence, the dry-run line, the TERM loop and the refusal listing. All five now handle the list.Brett Heap's ruling of 2026-10-18: a spaced value must be one absolute path
The independent review of d4cf2ee (verdict LAND) found a problem with reading the value whole. Five argv shapes that main correctly ignores now MATCHED. That is the dangerous direction, because
lane-end's recheck before TERM uses the same rule:--fork-session --resume <other id> read /p/<id>.jsonl and summariseclaude -p --fork-session --resume <id> summarise /p/<id>.jsonl--resume /p/<id>.jsonl(two spaces)--resume /p/<id>.jsonl.bak note /q/<id>.jsonl-cstring holds--fork-session --resume $ID ; tail …/<id>.jsonlLane 5 asked Brett Heap to confirm a tightened
--resumerule. On 2026-10-18 at 14:0xZ he chose, verbatim, "Add the condition first (Recommended)". The option he chose read:What changed for it (de0809c). Four lines go after the
--check, the same in both copies. Here islanes-edit.sh's:lane-end'sdh_resume_valuehas the same four lines with its own prefix. I extracted both bodies from de0809c and diffed them: they are identical apart from thedh_/dhp_prefix andlane-end's indent. The comment abovedhp_resume_valuenow says what the condition refuses and what the read still can't tell apart.lane-end's comment and the manual's duplicate-holder paragraph now state the rule. The syntax is bash 3.2-safe:caseonly.The two rows join the SPAN case in
tests/test_lane_helpers.sh. It is discovery only, so the pids are fixed:900203, shape 1:--resume <wrong id> read /nonexistent/projects/<id>.jsonl and summarise900204, shape 4:--resume /nonexistent/projects/<id>.jsonl.bak note /nonexistent/other-projects/<id>.jsonlEach row has its own
hasnt, and the SPAN case's existing exit-8 row covers both. The spaced real path that must still match is the SP case (/nonexistent/projects with a space/<id>.jsonl --output-format stream-json). It runs through bothduplicate-holderandlane-end --retire, and it still passes.Proof, isolated. This uses the same kind of slice as the original proof below: the file's setup, its
E=line, the repoM-1 duplicate-holder block (from the issue #39 header to the clause (c) header), and the footer. It ran underenv -ufor everyLANES_*seam this session carries plusTMUXandTMUX_PANE, andenv | grep '^LANES_\|^TMUX'printed nothing inside that env before the slice ran.The reviewer's 19-case table, run against both copies extracted from de0809c:
Compared with the rule before the condition, only five rows change, each from MATCH to empty:
LIMIT bare id+prompt,LIMIT .bak+positional,LIMIT empty arg,LIMIT -p promptandshell wrapper. Every real match is kept:plain,spaced+opts(a home directory with a space),double space dir,--resume-fileandglob chars.A shape the condition does not close. One argv still matches here although main ignores it:
--resume /p/<id>.jsonl.bak note q/<id>.jsonl. More generally, it is any value that starts with/and doesn't end in.jsonl, followed by a later word that names<id>.jsonland does not begin with/. That also covers a word opening~/,$HOME/,file:///,path=/or(/, none of which reaches the path through/. The reviewer's sketch matches it too. It is not one of the review's five shapes or in the 19-case table, and the ruling did not cover it, so I left it as ruled. The comment abovedhp_resume_valuenames it as the one shape the read can still mistake.Merged main. 7c4006e merges main 87a5e9d, which brings in #238, #240 and #213, so CI tests this PR on today's main. 4e3f110 merges main 586dea2 the same way, and it touches none of this PR's files.
git diff origin/main...HEAD --name-onlystill lists only this PR's four files.After the delta review
The independent delta review of de0809c gave LAND, with the points below. 4e3f110 merges main 586dea2, and one commit on top of it, 0cf7417, does all of them.
900205, pins the start-with-/half of the condition.--resume <wrong id> summarise nonexistent/projects/<id>.jsonlholds no/, so only that line refuses it; rows900203and900204are both refused by the/half alone, so deleting the line used to leave 75 passed. With it deleted from both copies, the duplicate-holder and lane-end --retire read a --resume path whole and act on every verified child, not the first (#76, #86) #205 slice gives 76 passed, 2 failed (the exit-8 row and the900205hasnt); at 0cf7417, 78 passed, 0 failed.lane-end'sdh_resume_valueandlanes-edit.sh'sdhp_resume_valuestay identical. It sits right after the SPAN rows in the repoM-1 block: it extracts each body from its file with awk, mapsdhp_todh_, takeslane-end's indent off and compares them withdiff. Removinglane-end's start-with-/line alone used to leave 75 passed. With one copy's condition line changed (/*)to/*|~*), in either file), the slice gives 77 passed, 1 failed (this case); at 0cf7417, 78 passed, 0 failed./", not "a RELATIVE path"; not every<uuid>.jsonl.bakform is refused, only one whose later path is reached through/; and a directory name that ends in a space before its/is the exception to "a transcript path does neither".Functions and regions edited
Line numbers are as of de0809c. The merge of main (4e3f110) leaves them alone. The later commit (0cf7417) adds comment lines to
dhp_resume_value's comment andlane-end's dispatch, and a block to the repoM-1 test block, so they have moved by a few lines.lanes-edit.sh: header comment aboveduplicate_holder_pids, one line (6734: "child pid" becomes "child pids"; session_ids_of_lane / session_ids_local_of_lane don't distinguish "no row" from "register unreadable", so every caller that treats an empty id set as a fact is fail-open on that read #67's line below it is untouched)lanes-edit.sh:duplicate_holder_pids(the parent's--resumeread, 6881-6886; the child scan, 6926-6977)lanes-edit.sh:dhp_resume_value(new, with its comment, 7008-7083, right afterduplicate_holder_pids)lane-end: usage header for--retire's duplicate holder (73-78)lane-end: the--retiredispatch's duplicate-holder branch, top level: record read, own-session fence, dry-run, new inlinedh_resume_value, pre-kill rechecks, TERMs, settle and report (1107-1365)lane-end: the--retirerefusal listing of duplicate rows (1583-1588)docs/README-lanes.md: the duplicate-holder paragraph (1742-1760)tests/test_lane_helpers.sh:cleanup()(100-109); the repoM-1 duplicate-holder block (AMBIG fixture, 5260-5289; new fixtures, 5393-5547, after the RACE case)I stayed out of
lane-end1401-1449 (#73),lane-end~210 and 1973 onward (lane 3), and every file of #191, #195, #196 and #199. Open PR #202 shares files with this one, but its hunks are at other line ranges. (#201 and #203 have merged, at 2026-10-18T01:16Z and 03:03Z.)Tests
In
tests/test_lane_helpers.sh, repoM-1 block:900104, listed bypgrep -Pafter the first. Both are named, the slot is exactly900102 900104, and the wrong-transcript sibling is still never named./nonexistent/projects with a space/<id>.jsonl --output-format stream-json.duplicate-holdernames both, and--retireretires both with exit 0. Both are verified gone withkill -0.--labelshape, and a bare session id followed by--append-system-prompt-file …/<id>.jsonl. Both giveduplicate-holderexit 8. These pass before and after, which is the point: they guard that the fix does not reopen round 5. Two more rows come from Brett Heap's ruling (section above): a bare session id followed by a prompt that names the transcript (900203), and a<id>.jsonl.bakfollowed by a word that names it (900204). Both fail at 8b16668 and pass at de0809c. A third row,900205, and a test that the two copies stay identical came after the delta review (the "After the delta review" section above).--dry-runnames both.--retireTERMs all three and exits 0, and all three are verified gone.trap "" TERM; exec sleep) and is named as the target itself. The result is exit 1, "only partly retired", no RETIRED, and that child is still alive. It is then killed and verified gone.Every real fixture pid is in
cleanup(), and it is cleared only once verified gone.Original proof for #76 and #86, isolated (the full suite was not run locally, per the brief). I built a slice of
tests/test_lane_helpers.sh: its setup through line 981, the whole repoM-1 duplicate-holder block (5135-5487) and its footer, withSRC_DIRpointed at a given tree. Seams were unset (env -u LANES_EDIT -u LANES_WORKSPACE_ROOT -u LANES_REPO -u LANES_LANE …).The rule itself, on 15 cases (spaced, double-spaced, trailing options, round 4/5 shapes,
--resume-file, empty), run against both copies: they agree on every case. I also extracted both function bodies and diffed them: identical apart from the variable prefix. Results were identical under local bash 5.2 and underbash:3.2(3.2.57).Other checks:
bash -npasses onlanes-edit.sh,lane-endandtests/test_lane_helpers.shunder bash 5.2 and 3.2.57.python3 -m pytest tests/test_repo_hygiene.py: 152 passed, including the host-absolute-path and backtick-in-description guards.bash -npasses onlanes-edit.sh,lane-endandtests/test_lane_helpers.shunder bash 5.2.21 and 3.2.57. I appliedhost_absolute_path_hitsfromtests/test_repo_hygiene.pyto the lines the condition commit adds and to the four whole files, and it finds nothing. I did not rerun pytest locally; CI'stestsjob is the suite of record.Closes #76
Closes #86
🤖 Generated with Claude Code
Summary by Sourcery
Fix duplicate-holder detection and retirement so spaced transcript paths are matched safely and all verified child processes are handled.
Bug Fixes:
--resumepaths during duplicate-holder discovery and retirement rechecks.Enhancements:
Documentation:
Tests: