[brettheap]

Fixes #235

The defect

resume's root_legs_ok is F3 of the review on #83: it refuses a root whose leg sits off the branch the root's own make bootstrap would move it onto. It took that branch from the ROOT (tracking_branch_of "$root") for every leg. But templates/assembly-root/scripts/bootstrap.py places a leg on str(leg.get("branch") or branch): the branch: the leg's own row in project.yaml declares, and the root's tracking_branch only when the row declares none. So for a leg with its own branch: the guard reasoned about the wrong branch, in both directions:

• it let a move through: a leg on main whose row says branch: develop was moved onto develop by bootstrap and resume exited 0;
• it refused a leg that was where it belongs: a leg on its own develop was refused as "not its tracking branch 'main'".

status had the same blind spot: it passed the root's branch to read_leg.

The gap is older than v1.1.0 (it happens with the bootstrap at 7f84ca4 too), and opensoft/openRepoShape#603 → opensoft/openRepoShape#615 widens it: a declared branch BEHIND the pin is now fast-forwarded and checked out, from that branch, from a HEAD detached at the pin, or from another branch.

The design

• leg_branch_of <root> <leg path>, new, in the shared estate resolver (so all three copies stay byte-identical; park carries it unused, like legs_of). It reads a leg row's own branch: the way legs_of reads the same rows (role: before path:, the assembly row skipped), except that branch: may sit on either side of path:, and a row that does not open with - role: clears what the row before it left. It prints nothing when the row declares none. No branch is: an empty value, null/Null/NULL/~, once a trailing # comment is cut off. A leg is found by its path SPELLED EITHER WAY: a leading ./ and a trailing / come off both sides before they are compared, because status holds .gitmodules' path and resume the manifest's own text (the review's finding 2). The loader is not matched to the letter for a quoted "null"/'~' or false/0; no real branch is named so, and the comment says so.

• root_legs_ok holds each leg to leg_branch_of, else the root's tracking_branch, which is bootstrap's or. The refusal names the leg, the branch it is on, the branch it should be on and where that comes from:

  • leg spec is on 'main', not its own branch 'develop' (the branch: of its row in project.yaml);
  • leg spec is on '001-my-feature', not its tracking branch 'main' (the root's tracking_branch);

  Detached at the pin is still not a refusal, and the dirty and every other check are untouched. A leg with no branch: is refused exactly as before; only the parenthetical is new.

• The comment above root_legs_ok says what bootstrap does with a leg's own branch:, including bootstrap.py: a leg on another branch whose tracking branch is behind the pin is fast-forwarded and checked out as one at the pin already is, and the family holder's AGENTS-shape.md and bump-leg.py say bootstrap places a leg where that loses no commit (#603) openRepoShape#615's fast-forward and checkout of a branch behind the pin, and the move back to the pin.

• status: estate_repos gives each leg's row its own branch: (else the root's), so everything read under that row (the pin against origin/<branch>, the leg's fork check, the fetch line, the "at its pin, which is origin/'s tip" line) is about the branch the leg belongs on, and read_leg's finding names it: on branch main, not develop, the branch: this leg's own row declares in project.yaml (...). The finding for a leg with no branch: of its own is byte-identical to before.

• Text that said the old rule is brought along: resume --help, AGENTS.md rule 1, and the comments above status's estate_repos and in park's report_repos.

Tests

seed_leg_member (resume's tests) and seed_root_with_leg (status's) take optional arguments (leg_branch, and for the first code_leg and spec_path); the defaults build the same estates as before. The new tests sit beside the F3 tests and beside read_leg's tests. Each estate is built with the suite's own helpers and the REAL bootstrap.py mounted at upstream/openRepoShape.

test	what it holds
test_a_leg_on_its_own_declared_branch_is_not_refused	(a) a leg declaring branch: develop, on develop at the pin: exit 0, no refusal, the verb runs, HEAD unchanged
test_a_leg_off_its_own_declared_branch_refuses_the_root_and_is_not_moved	(b) the same leg on main with a local develop at the pin: refused by name, exit 1, HEAD unchanged, verb not run; then the root's real bootstrap, run by hand, moves the leg onto develop, so the refusal is not theatre
test_a_leg_with_no_branch_of_its_own_is_held_to_the_roots_and_says_so	(c) a leg with no branch: on a feature branch: refused as before, naming the root's tracking_branch
test_a_leg_off_its_own_declared_branch_is_a_finding_that_names_it, test_a_leg_on_its_own_declared_branch_is_not_a_finding	(d) status in both directions
test_leg_branch_of_reads_a_rows_own_branch_the_way_bootstrap_does (22 cases)	the parsing, run against the real trim_unquote and leg_branch_of cut out of resume: branch: before and after path:, double and single quotes, a trailing comment (also after quotes), an empty value, only a comment, null/Null/~, a row with none beside one with one, the assembly row, paths sharing a prefix, ./spec, spec/, a ./spec asked for, and a row that does not open with - role:
test_each_leg_of_one_root_is_held_to_its_own_branch	one root, a spec leg declaring develop and a code leg declaring nothing: neither is refused where it belongs; the code leg on develop is refused by name and the spec leg is left alone
test_a_leg_spelled_dot_slash_or_with_a_slash_is_one_leg_to_status_and_resume (2 spellings)	the independent review's case: path: "./spec" or spec/, branch: develop, leg on main: status reports it, resume refuses it

Red/green. Every LANES_*/TMUX* variable unset, private HOME, TMPDIR, basetemp and pycache, under the pytest lock, mount at 190133d (v1.1.0):

• The first 5 tests (a to d): 5 failed on the old park/resume/status, at both 7f84ca4 and 190133d; 5 passed on the branch. At v1.1.0 the (b) failure is resume exiting 0 over [spec] spec: on develop at 98154768e7a2 (branch tip == pin), the leg moved.
• The review's mutants, applied identically to all three copies, each fail the new tests:

mutant	failing new tests
drop the trailing-comment cut	3 (a-trailing-comment-is-cut, ...-after-quotes-is-cut, only-a-comment-is-no-branch)
drop the null/~ case	3 (null, Null, tilde)
branch reset when path: is read (branch before path)	1 (before-path)
drop the ./ and / normalisation	5 (3 table cases and both spellings of the status/resume test)
drop the "row that does not open with a role" clear	1
leg_branch_of always answers nothing (the old behaviour)	19

With the fix every one of these passes.

Modules, this branch merged with f5c3bcd, submodule at 190133d: tests/test_park_resume_commands.py (164 tests), tests/test_status_command.py (202) and tests/test_repo_hygiene.py (189, which holds the three-way byte-identity of the shared resolver and the host-path rule): 555 passed. The full suite was not run locally; CI's Linux jobs are the suite of record.

Not in this change

park's report_repos has the same assumption when it counts unpushed commits on a leg's tracking branch, so work on a leg's declared branch is never reported. The independent review of this PR found it (review 5561239715) and it is filed as #242. The comment there now says so.

🤖 Generated with Claude Code

[sourcery-ai]

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 12 hours by commenting @sourcery-ai review. Upgrade to get a review now.

[sourcery-ai]

Reviewer's Guide

The PR fixes branch selection for resume and status by matching each leg to the branch bootstrap will use—its row-level branch when declared, otherwise the root's tracking_branch—through a shared resolver, clearer diagnostics, and regression tests covering both declared and fallback cases.

File-Level Changes

Change	Details	Files
Resolve each leg's effective bootstrap branch from its own project manifest row, falling back to the root tracking branch.	Add a shared leg-branch resolver that parses row-level branch values and preserves byte identity across scripts. Update resume's guard and diagnostics to validate each leg against its declared branch or the root fallback. Update status to read and report each leg against that same effective branch. Leave park's resolver copy aligned while intentionally not changing its reporting behavior.	park resume status
Add regression coverage for declared leg branches while preserving existing fallback behavior.	Extend test fixtures to create legs with optional declared branches and matching remote refs. Verify resume accepts correctly placed legs, refuses and does not move misplaced legs, and retains existing no-branch behavior. Verify status uses the declared branch for findings, pin comparisons, and clean output. Retain repository hygiene coverage for shared resolver identity and add real-bootstrap premise checks.	tests/test_park_resume_commands.py tests/test_status_command.py

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#235	Have resume compare each leg against the branch bootstrap would use: the leg row’s own branch: when declared, otherwise the root’s tracking_branch, and refuse before bootstrap can move the leg.	✅
#235	Make status use the same per-leg branch choice for its reads and findings, so a leg on its declared branch is not incorrectly reported as off-branch and findings name the expected branch.	✅
#235	Keep the shared resolver copies consistent and update the guard’s explanation and tests to cover declared branches and the fallback to the root branch.	✅

Possibly linked issues

• resume's leg guard checks each leg against the root's tracking_branch, not the leg's own branch:, so it lets bootstrap move a leg (exit 0) and refuses a leg on its own branch #235: The PR fixes the issue by checking each leg against its declared branch (or the root tracking branch when absent) in resume and status.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[brettheap]

Independent review of #238 at fefeaca: LAND after fixes

The rule is the right one and the behaviour is correct for every manifest the scaffold writes. I found no blocking defect. Finding 1 should be fixed before this lands. The rest can go in the same push or be filed.

Run at fefeaca, submodule at 190133d (v1.1.0), with every LANES_* and TMUX* variable unset and a private HOME, TMPDIR, basetemp and pycache, under the pytest lock:

• The 5 new tests pass on the head and all 5 fail on f595e44's park/resume/status. On the old scripts (b) exits 0 over [spec] spec: on develop at <sha> (branch tip == pin), and (d1) reports 0 findings. (c) fails on the old scripts only through the new parenthetical, as the PR says.
• tests/test_repo_hygiene.py passes (189). The three resolver blocks have the same sha256.
• shellcheck is clean on all three files.
• No bash 3.2 was available. I ran bash 5.2 and read the new code for 3.2: no ${x,,}, mapfile, declare -A, local -n or multi-line awk -v. The # inside ${branch%%[[:space:]]#*} is not at a word start, so 3.2's $() parsing is not affected.
• A full run of the two touched modules was cut at about 84% by my own timeout, with no failure shown. I did not complete it. CI's tests and tests-no-submodule were still pending when I looked, and CI is the record.

Findings

1. should-fix: nothing tests leg_branch_of's parsing, which the PR body and code comment claim.

All 5 tests use branch: develop on the line after path: spec. The comment and PR body claim four more things: branch: on either side of path:, an empty value, null/~, and a trailing # comment cut off. None of them is exercised.

I proved it with a mutant, applied identically to all three copies so byte-identity still holds. It dropped the comment cut, dropped the null/~ case, and reset branch when path: is read. That makes branch: develop # c return develop # c, branch: null return null, and branch-before-path return nothing. Result: the 5 new tests, test_repo_hygiene.py (189) and the 72 existing leg and refusal tests in test_park_resume_commands.py all still pass.

There is also no mixed case, one leg with its own branch: and one without. It works. I ran root_legs_ok over a two-leg root with tracking_branch: trunk and a branch: develop spec leg: both legs where they belong passed, and a code leg on develop was refused as "not its tracking branch 'trunk'". It is simply unguarded.

Fix: add a table-driven test that sources the shared block (sed -n '/BEGIN shared estate resolver/,/END shared estate resolver/p' resume) into bash and asserts leg_branch_of over:

row	expected
branch: develop after path:	develop
branch: develop before path:	develop
branch: "develop"	develop
branch: develop # own	develop
empty branch:	nothing
branch: null, branch: ~	nothing
branch: # none	nothing
another row's branch:, and the assembly row's	nothing
two rows whose paths share a prefix (spec, spec-extra)	the exact row only

Then add one two-leg resume case.

2. should-fix: status and resume look a leg up under different path spellings.

status finds the leg's row by its .gitmodules path (declared_legs, then leg_branch_of "$root" "$leg"). resume uses the path: text from project.yaml (legs_of). The two are compared by exact string. validate-manifest accepts path: "./spec" and spec/ (only absolute paths, .. and . are refused), and status's own comment above physical_path says "./spec against spec is the same leg and two strings".

Repro: branch: develop, path: "./spec", .gitmodules path spec, leg on main.

• status: leg main <leg> then at its pin, which is origin/main's tip as of the last fetch; clean. This is the blind spot the PR sets out to close.
• resume (root_legs_ok): REFUSED R: leg ./spec is on 'main', not its own branch 'develop' (...), rc=1.

Fix, in the shared block so all three copies move together: normalise both sides in leg_branch_of before comparing (p="${path#./}"; p="${p%/}", the same for want). Or document the limitation next to leg_branch_of.

3. should-fix: the operator-facing text still states the old rule, and two comments now say something untrue.

• resume --help (resume:99-102): "a root one of whose LEGS is off its tracking branch".
• AGENTS.md:79-82, driving rule 1: "would walk the leg back onto its tracking branch".
• status:703-704 still says estate_repos is "THE SAME ENUMERATION park's not-carried report makes". It no longer is: the branch column for a leg now differs.
• park:584-585 still says "A LEG'S branch is the ROOT's tracking_branch: — that is the one value scripts/bootstrap.py places every leg on".

Fix: reword the first two to "off the branch bootstrap places it on (its row's branch:, else the root's tracking_branch)". Reword the two comments, with a pointer to leg_branch_of. The PR body says park's report_repos is "left alone" but names no issue. File one and link it. It is a data-loss-adjacent report: for a leg on its own branch, park counts unpushed commits against the root's branch, not the leg's.

4. note: for a few scalars the shell and the loader disagree, so the comment's claim is not exactly true.

The comment says "what the loader would call no branch is none here too". I checked repo_shape.parse_yaml and bootstrap.py's str(leg.get("branch") or branch):

YAML	loader gives	shell gives
"null", '~'	the branch null, ~	none
false, 0	falsy, so the root's branch	the branch false, 0
true	True	true

No real branch is named that, so this is cosmetic. Soften the comment to "null, Null, NULL, ~ unquoted, empty, or a comment".

5. note: a row that does not open with - role: does not reset branch.

This is the same limitation legs_of has for path:, but branch: is new. Probe: - role: spec / path: spec, then - path: other / branch: leak, with want=spec, returns leak.

The scaffold template always opens a row with - role:, so this is not reachable from a generated manifest. Hardening, if wanted: add a ' - '*) arm that clears role, path and branch before the more specific arms. That is one line in the shared block, and it changes nothing for role:-first rows.

Checked and not findings

Each is what I looked for in the checklist.

• resume's refusal. It comes before pull_ff_only and make bootstrap (refresh_root). (b) asserts exit 1, the exact refusal line, HEAD name and sha unchanged, and that the verb did not run. Then it runs the real bootstrap.py by hand to show it would have moved the leg. Detached at the pin still passes. The detached-off-pin, dirty and root-branch checks are unchanged in the diff.
• status consumers. The row header, the fetch line (fetch_estate_first, fetch_remote), read_fork, ask_github_fork, read_tracking, read_branches, no_origin_branch_finding and the "at its pin ... origin/'s tip" line all take the row's branch, so they are right for a leg with its own branch:. For a leg without one, or a non-leg .gitmodules entry, leg_branch_of returns nothing and the column is the root's, as before. holder, root and missing rows are untouched.
• A pin ahead of origin/<own branch>. With a declared branch develop behind the pin, status now says pin <sha> is not on origin/develop: 1 commit(s) unpushed in the leg. That is correct for v1.1.0's fast-forward, which leaves local develop ahead of origin/develop.
• Hygiene and body. The diff has no host-absolute path. The one ~/ is the existing workspace.yaml fixture line. The body has "Fixes #235" only, and "Bug Fixes:" is Sourcery's heading with no issue number after it. The openRepoShape references are opensoft/openRepoShape#603 and #615.
• The v1.1.0 bootstrap behaviour the new resume comment describes. I read fast_forward_to_pin and move_back_to_pin in the mounted bootstrap.py, and the comment matches them.

🤖 Generated with Claude Code

[brettheap]

Applied the independent review 5561239715 at 9be1a44: findings 1-3 (a 22-case table and a two-leg case for leg_branch_of's parsing, each failing the review's mutants; ./ and trailing / normalised in the shared block, with the review's status/resume case as a test; resume --help, AGENTS.md and the two comments reworded), and finding 5's row clear; finding 4's comment softened; follow-up #242.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[brettheap]

Delta re-check of #238 at 9be1a44: LAND

Findings 1, 2, 3 and 5 of review 5561239715 are fixed, and finding 4's comment is softened. I found nothing new. Landing still needs CI's tests and tests-no-submodule on 9be1a44 green; both were pending when I looked.

Finding 1, parsing tests.

• shared_function reads resume as it is at test time, so it cannot hold stale code. It uses str.index, so a renamed or missing function fails the test with a ValueError instead of passing silently.
• It cuts at the first \n}\n. Both trim_unquote and leg_branch_of end at column 0, and there is no inner column-0 brace.
• The other two copies are covered by the existing byte-identity test.
• I re-applied my three mutants identically to park, resume and status. Each now fails:
  • comment cut dropped: 3 failures (a-trailing-comment-is-cut, a-trailing-comment-after-quotes-is-cut, only-a-comment-is-no-branch)
  • null/Null/~ case dropped: 3 failures
  • branch reset at the path: line: 1 failure (before-path)
• I added three more mutants. Dropping the ./ and trailing / normalisation gives 5 failures. Dropping the new row-clear arm gives 1 failure. Making leg_branch_of ignore which path was asked gives 5 failures, including test_each_leg_of_one_root_is_held_to_its_own_branch.
• test_each_leg_of_one_root_is_held_to_its_own_branch fails on f5c3bcd's scripts. It passes with spec on develop and code on main. It then refuses code on develop by name, leaves spec alone, and leaves both HEADs and branches unchanged.

Finding 2, ./spec and spec/.

• The new test_a_leg_spelled_dot_slash_or_with_a_slash_is_one_leg_to_status_and_resume is my probe as a test. status prints leg develop <leg> and the "on branch main, not develop" finding. resume refuses with exit 1, and HEAD is unchanged. It fails on the pre-PR scripts.
• The normalisation strips one leading ./ and one trailing / from both sides.
• Probes show it cannot join two different legs:
  • ../spec, sub/spec, .//spec, spec// and ./sub/../spec do not equal spec.
  • ../spec still matches itself.
• A path of . is unchanged. The assembly row never emits, and resume never asks for ..
• The shared block has the same sha256 in all three scripts, and tests/test_repo_hygiene.py passes.

Finding 3, stale text. resume --help, AGENTS.md rule 1, the status comment above estate_repos and the park comment in report_repos now describe the real rule and point at #242. A grep for the old wording finds no remaining copy.

Merge 7ae0f46 (main f5c3bcd).

• It touches only lane-start, skills/handoff/SKILL.md, tests/test_lane_helpers.sh and tests/test_skill_shell_blocks.py.
• git diff f5c3bcd 7ae0f46 is byte-for-byte the previous PR diff (git diff f595e44 fefeaca; sha256 398a799b… for both once index lines are dropped). So it adds nothing to park, resume, status or the test files.
• The writer's own commit touches only AGENTS.md, park, resume, status and tests/test_park_resume_commands.py.

#242 is accurate. Against f5c3bcd's park: report_repos is at lines 526-548, the comment at 536-537, unpushed_on_tracking at 559 and not_parked_report at 579. The unpushed line prints the branch. The suggested fix works with the new normalisation. The test it asks for would fail without the change.

Runs at 9be1a44, submodule at 190133d, with every LANES_* and TMUX* variable unset, private dirs, under the pytest lock:

• The 30 new and earlier branch tests pass.
• tests/test_repo_hygiene.py passes (189).
• tests/test_park_resume_commands.py passes in full (164 tests).
• tests/test_status_command.py passes in full (202 tests). This completes the full-module run I cut short last time.
• shellcheck is clean on all three scripts.
• No bash 3.2 was available. The new constructs (${want#./}, ${want%/} and one more case arm) are plain parameter expansion and glob patterns, as the old code uses.

🤖 Generated with Claude Code