Repository navigation
park, resume and status find a family folder named for its repository or its display name, and resume names the holder's repository from family.yaml where it is on disk (#236) - #240
Conversation
… or its display name, and resume reads the holder's repository from family.yaml where it is on disk (#236) is_family_folder took <dir> as a family folder only when family.yaml's name: equalled the folder. openRepoShape v1.1.0 names the folder, and the holder in it, after the holder's REPOSITORY, the last part of repository: (opensoft/openRepoShape#629), and make siblings stops before cloning when the folder is not that name (opensoft/openRepoShape#543). A family whose display name: differs from its repository name had no layout that worked for both. The folder now matches when it equals name: (estates laid out before v1.1.0) or the last part of repository: (v1.1.0). The shared resolver stays byte for byte the same in all three commands. resume's holder repository comes from the org the record is filed under and the family folder; the record never carries the repository itself. Where a holder is already on disk, its family.yaml repository: is read instead. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
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 |
Reviewer's GuideUpdates park, resume, and status to recognize family folders named either for their display name or the repository’s final component, while resume uses the holder’s on-disk manifest when available and retains a fresh-machine fallback; extensive fixtures and regression tests cover both layouts, strict refusals, precedence, cloning, and repository reporting. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
brettheap
left a comment
There was a problem hiding this comment.
Independent review of #240 at c824e4e: LAND
No finding blocks the merge. Finding 1 is cheap and worth a follow-up (or one more commit if a fix round is already open); the rest are notes. Linux CI was still pending when I looked, so the verdict stands on a green tests and tests-no-submodule.
Findings
1. should-fix (non-blocking): family_repository_on_disk keys on the record's folder, not on the estate resume resolved. The "fallback only when no holder is on disk" claim is not exact.
resume:1245 calls family_repository_on_disk "$MANIFEST_DIRNAME", and MANIFEST_DIRNAME is the first segment of the record's root:. After a person follows make siblings' own mv QuillWorks/QuillWorks Quill/Quill, the record is stale until the next park (root: QuillWorks/Alpha). The holder IS on disk, resolved by estate_by_name "$NAME" at resume:1158, but the lookup misses it and the fallback prints the repository that does not exist. That is the exact misprint the PR sets out to remove.
Evidence. The record is the one park files for this family (workspaces/<org>/quill.yaml, family: quill, root: QuillWorks/Alpha) and Quill/Quill is on disk:
$ resume Quill --dry-run
records a family estate, root repository testorg/QuillWorks
present ~/projects/Quill (family)
With the holder at QuillWorks/QuillWorks and resume quill, the PR's lookup works (root repository testorg/Quill).
The value is print-only today. The function can return 0 only when estate_by_name has already set ESTATE_PRESENT=1 (resume:1158, resume:1256), so it never reaches the clone at resume:1324. The PR body is honest about that.
Fix: read the holder that was resolved, not one looked up by the record's name. Delete the three-line block at resume:1245-1247 and the say " records ..." at resume:1248. Put this after the ESTATE_PRESENT lookups (after resume:1259, before the (3) banner). The lookups print nothing, so the output order is unchanged:
if [ -n "$MANIFEST_FILE" ]; then
if [ "$MANIFEST_KIND" = "family" ] && [ "$ESTATE_PRESENT" -eq 1 ] && [ "$ESTATE_KIND" = "family" ] &&
yaml_scalar "$ESTATE_ROOT/family.yaml" repository && [ -n "$YAML_VALUE" ]; then
ROOT_REPOSITORY="$YAML_VALUE"
fi
say " records a $MANIFEST_KIND estate, root repository $ROOT_REPOSITORY"
fifamily_repository_on_disk is then unused and goes. I tried this in a scratch copy: the stale-record case prints testorg/Quill, and the other six probe layouts print what they printed before.
Related and not a regression: with the same stale record, resume quill (the id, which is how park files the record) says absent cloning testorg/QuillWorks -> ~/projects/QuillWorks/QuillWorks although the family is on disk at Quill/Quill. Main does the same for any folder/record mismatch. Fixing it needs the holder found by family.yaml's id: against the record's family:, which is a different change. File it, do not fold it in here.
2. note: the bash reader and v1.1.0's reader disagree on two value shapes.
I ran both on the same text. openRepoShape's parse_yaml + repo_basename against is_family_folder (park:268-279, resume:251-262, status:389-400):
family.yaml line |
v1.1.0 | is_family_folder |
|---|---|---|
repository: testorg/Quill # the holder |
basename Quill |
rejected (value keeps the comment: Quill # the holder) |
repository: "testorg/Quill" # the holder |
basename Quill |
rejected |
repository: null in a folder named null |
no repository, falls back to name: |
accepted (null is read as text) |
Everything else I tried agreed: quotes, single quotes, CRLF, a bare Quill with no slash, https://github.com/testorg/Quill, git@github.com:testorg/Quill, an empty value, "", ~, a trailing slash, testorg/Quill.git, testorg/quill (case), either key order. kind: and name: already have the comment gap on main, family.py never writes a comment, and a folder named null is not a real case. So this does not hold the PR. File it against the shared trim_unquote, because a %%[[:space:]]#* strip in is_family_folder alone would leave the quotes on the quoted-with-comment form.
3. note (test gaps, none of them hides a defect today).
- No negative test for a nested
repository:. The refusal shapes usemembers: [].yaml_scalaronly matches column 0, so a member row (repository: testorg/Quill) and the template's ownshape:block (repository: opensoft/openRepoShape) are never read as the holder's. I ran 4 variants of that and all are rejected, but nothing pins it. A fourth shape in both parametrized lists (tests/test_park_resume_commands.py:729,tests/test_status_command.py:587) would: no top-levelrepository:, plus a member row whoserepository:ends in the folder name. named_apartfiles two records,quill.yamlandquillworks.yaml(tests/test_park_resume_commands.py:617). Park writes ONE record per family, named for the id, soquillworks.yamlis a file it never writes forid: quill.test_resume_names_the_repository_the_holders_family_yaml_givesreaches the on-disk lookup through it. The realistic spelling,resume quillagainstquill.yaml, works (verified), but the stale-record case in finding 1 has no test.- The status "wins over a standalone root" test (
tests/test_status_command.py:604) never checks the exit code. Addassert result.returncode != 2(status exits 1 here, on findings). - Every new case resolves by
<Name>. The cwd walk-up (estate_walk_up) is untested for a repository-named family. I checked it by hand: fromQuill/Quill/scripts,Quill/QuillandQuill,park --dry-runanswers the holder ofQuill; fromQuill/Alphait answers the member (nearest wins), as intended.
4. note: the two layouts of one family can both be found, and nothing de-duplicates them.
Quill/Quill (repository rule) and QuillWorks/QuillWorks (name rule) are different directories, so each is found by its own name and neither shadows the other. park --all and bare status list two rows and walk each holder once (checked: family Quill, family QuillWorks, === park Quill ===, === park QuillWorks ===, no holder repeated). Before the PR only QuillWorks was visible.
Both parks write under the same record key (the family id). From AGENTS.md ("a park anywhere else rewrites the whole block") I expect the later park to replace the first one's member blocks. I did not run that. A person ends up with both layouts by resuming a record parked from the other layout on a machine that has the legacy one. This is inherent to a folder-name-keyed estate, so nothing to change here. Say it in AGENTS.md or in a follow-up issue.
5. nit: stale comment in the shared block. park:203, resume:186 and status:324 say yaml_scalar is "narrowed to the keys these two commands read (kind, name, family, tracking_branch)". repository now joins them. The list was already short of id and commit. The block is byte-identical, so the edit goes in all three.
What I checked and found no defect in
- Item 1. Both arms, either key order, the
kindguard, and the "another family's repository" case all hold. A file fororg/Otheris refused in folderQuill. The repository arm needs the LAST part only (an earlier part is refused). A missing key is refused. Nested keys are not read (see finding 3).${YAML_VALUE##*/}equals openRepoShape'srepo_basename(rsplit("/", 1)[-1]) on every input I tried. No caller readsYAML_VALUEafteris_family_folder. - Item 2. The resolver block has the same md5 in
park,resumeandstatus(6a697c90df00).test_the_three_commands_carry_the_same_resolver_byte_for_byteandtests/test_repo_hygiene.py(189 passed) hold at the head. - Item 3.
family_repository_on_diskruns afteris_safe_folder_name "$MANIFEST_DIRNAME"(resume:1240beforeresume:1245). The fallback$MANIFEST_ORG/$MANIFEST_DIRNAMEis used when no holder sits at the recorded folder or itsfamily.yamlhas norepository:. Finding 1 covers the exception. In the legacy layout,resume QuillWorksat v1.1.0 ends with oneFINDING: make siblings exited 2plus the holder's ownmvadvice. That is accurate and not a regression. - Item 4. A family folder still beats a standalone root in both naming modes (park and status verified in the repository mode; the name mode is the existing test).
- Item 5.
/bin/bashhere is 5.2.21, so there is no 3.2 to run. I scanned the added lines for,,,^^,mapfile,readarray,declare -A,local -n,awk -v,[[,<<<and found none. The new code is${x##*/},local,if a && [ ],forover a non-empty array.bash -nandshellcheck -s bashare clean on the three scripts. - Item 6. Red and green reproduced. At the mounted
190133d, against main'spark,resumeandstatus: 7 failed, 13 passed (the 12 guards plus the byte-identity test), and the 7 are the ones the PR names. They fail for the stated reasons (presentline missing,the assembly rootinstead ofthe family holder,testorg/QuillWorksprinted). At7f84ca4(submodule moved to it in a scratch copy): the same, 7 failed against main and 20 passed against the head. At the head, at190133d, the wholetests/test_park_resume_commands.pyandtests/test_status_command.pymodules: 355 passed (149 + 206, matching the PR body). The PR's mutation claims reproduce: first part ofrepository:fails 9, a missing key accepted fails 3,resumenot readingfamily.yamlfails 1 (the on-disk test). - Item 7. No host path, bench name or lane path in the diff, the commit messages or the PR body. The hygiene module passes.
- Item 8. The body's counts and test names are accurate.
Fixes #236is the only closing keyword. The title carries(#236)only. The openRepoShape references are qualified (opensoft/openRepoShape#629,#543).#233and#235are this repository's. - Item 9. No textual conflict with #238 (
fefeaca).git merge-tree --write-treeof the two heads is clean, in either order (it is symmetric). #238 only insertsleg_branch_ofafterlegs_of()(base lines:parkafter 488,resumeafter 471,statusafter 609, 48 lines each). #240 touches, at base lines, the header comment (park133,resume116,status254) andis_family_folder(park254-267,resume237-250,status375-388), and inresumealonefamily_repository_on_disk(after 628) plus the comment andrecordscall (1164-1206). The hunks are about 200 lines apart in the shared block and over 500 apart inresume's tail. The merged tree has one resolver-block md5 across the three files, and the 20 new tests and the byte-identity test pass on it. I did not run #238's own tests on the merge.
What I ran. Everything under a private HOME, TMPDIR, --basetemp and pycache prefix, every LANES_* and TMUX* variable unset, under the shared pytest lock, with /usr/bin/python3 -m pytest. Scoped runs of tests/test_park_resume_commands.py and tests/test_status_command.py, the full two modules once, tests/test_repo_hygiene.py, and an untracked scratch file of resume probes in my own clone (never committed). No file outside the review's work directory was written.
🤖 Generated with Claude Code
…e record's folder points at; status exit code, a nested repository: negative case, and yaml_scalar's key list (#236) Applies the independent review 5561239588 of #240. family_repository_on_disk looked the holder up by the record's folder, so a record gone stale after make siblings' own mv (root: QuillWorks/Alpha, the holder now at Quill/Quill) missed a holder that is on disk and printed testorg/QuillWorks, which is no repository. resume now reads family.yaml of the estate it resolved (), after both ESTATE_PRESENT lookups, and says its 'records' line there; the record-derived repository stays as the fallback. The helper is gone. Tests: the stale-record case (and the same record against the holder still in the display name's folder, typed as the id), a member's own repository: that is never the holder's (park, resume, status), and the status 'wins over a standalone root' test now checks the exit is not 2. The yaml_scalar header comment lists repository, byte for byte in all three. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
brettheap
left a comment
There was a problem hiding this comment.
Delta re-check of #240 at 85a49d9: LAND
No findings. CI's tests and tests-no-submodule were still pending when I looked, so this stands on them going green.
Should-fix 1: fixed. resume now reads $ESTATE_ROOT/family.yaml after the ESTATE_PRESENT lookups, and family_repository_on_disk is gone.
- The
recordsline moved below the (silent) lookups, so the order of printed lines is unchanged. Thedieon an unsafe folder name still precedes it. - Stale record (
quill.yaml,root: QuillWorks/Alpha) with the holder atQuill/Quill:resume Quill --dry-runnow printsroot repository testorg/Quill(it printedtestorg/QuillWorksatc824e4e). - My other six layouts print what they printed before, in every
records,present,absentandwould cloneline: holder in the display-name folder (resume quillprintstestorg/Quill;resume QuillWorkshas no record line), the repository-named folder with a current record (testorg/Quill), the stale-recordresume quill(unchanged, see below), and the two fresh-machine dry runs. - The record fallback still prints correctly with no holder on disk and the workspace already cloned:
Quillrecord givesroot repository testorg/Quillandcloning testorg/Quill -> .../Quill/Quill;QuillWorksrecord givestestorg/QuillWorksandcloning ... QuillWorks/QuillWorks. resume quillagainst a stale record still planscloning testorg/QuillWorks. That is the case #243 item 3 names, and main does the same.
The new tests test what they claim.
- Stale record, red then green: the
the-holder-was-moved-to-the-repositorys-namecase fails againstc824e4e'sresume(assert 'testorg/QuillWorks' not in ...) and passes at85a49d9. The second case,the-holder-is-still-in-the-display-names-folder, passes either way and is a guard, as its docstring says. Therecords ... testorg/Quillassertion is a prefix oftestorg/QuillWorks, so thenot inline is the one that bites. Together they are right. - Nested
repository:: I mutatedyaml_scalarin all three scripts to left-trim lines, so an indented memberrepository:reads as the holder's. Exactly the three newonly-a-members-repository-ends-in-the-foldercases fail (park, resume, status), and the other nineneither_namecases pass. - Status "wins" test:
assert result.returncode != 2is in. Its comment says status exits 1 there (findings). - Run at the head: 26 scoped tests (the new ones, the earlier 20,
fresh_machine,resume_names) passed, andtests/test_repo_hygiene.pypassed (189).
Nit 5: fixed. The yaml_scalar header comment lists repository in park, resume and status. The resolver block has the same md5 in all three (76cb3ec30a3d), and the byte-identity test is in the passing set. bash -n and shellcheck -s bash are clean, the added lines hold no bash 4+ construct, and no host path is added.
The merge of f5c3bcd. It changed lane-start, skills/handoff/SKILL.md, tests/test_lane_helpers.sh and tests/test_skill_shell_blocks.py against its first parent, and nothing in park, resume, status, tests/test_park_resume_commands.py or tests/test_status_command.py. git diff b80d1f8 85a49d9 is the writer's commit alone: three comment/script files, two test files, 97 insertions and 48 deletions.
#243 is accurate. Its item 1 is my note 2 (same table), item 2 is note 4 with the "not run" hedge kept, item 3 is the resume quill stale-record clone plan, and item 4 is the rest of note 3 (walk-up and the second named_apart record). The nested-key and status exit-code gaps and nit 5 were taken here, so they are rightly absent. Every line number it cites matches 85a49d9, including repo_shape.py:1039 and :2399 at 190133d.
🤖 Generated with Claude Code
… this PR on today's main No conflict: of the three commits since 2cb14a9, #238 and #240 touch only park, resume, status and their tests, and #213 touches lanes-edit.sh (managed_seam_refuse, managed_local_hint, managed_local_remedy, lane_trees_list and the managed-projection and lane-trees verbs), docs/README-lanes.md and tests/test_lane_helpers.sh (its managed-seam cases) only away from this PR's functions and hunks, and all three shared files merged as exactly main's version plus this PR's own change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>



The defect
park,resumeandstatusfind a family estate withis_family_folder, in the shared estate resolver they carry byte for byte. A directory was a family folder only when<dir>/<dir>/family.yamlhadkind: family-manifestAND its displayname:equalled the folder's name.openRepoShape v1.1.0 names that folder, and the holder inside it, after the holder's REPOSITORY: the last part of
repository:(opensoft/openRepoShape#629). Itsmake siblingsstops before cloning when the folder is not that name (opensoft/openRepoShape#543). So a family whose displayname:differs from its repository name (family.py init --family Quill --name QuillWorkswritesname: "QuillWorks"andrepository: testorg/Quill) had no layout that worked for both:QuillWorks/QuillWorks: the three commands found it, but the holder's ownmake siblingsrefused to clone.Quill/Quill: the holder worked, butpark Quill,status Quillandresume Quillall answeredno estate named 'Quill'.The design
is_family_folderaccepts<dir>when<dir>/<dir>/family.yamlhaskind: family-manifestAND the folder name equals either itsname:(the old rule, kept for estates laid out before v1.1.0) or the last part of itsrepository:(v1.1.0's rule). It uses the scripts' ownyaml_scalarreader and strips the repository with${YAML_VALUE##*/}. The same edit is made in all three scripts, sotest_the_three_commands_carry_the_same_resolver_byte_for_bytestill holds. The comments that said the folder must equalname:now say what is true.resume(it clones a family holder that is not on disk).What the record carries for the holder's repository
Nothing.
resume's manifest, as the extension writes it (and as this suite'sfamily_manifest()hand-writes it), hasfamily:(the lowercase id),written_by:, and per memberid, its OWNrepository:,shape,root: <family folder>/<member folder>and the parked features. The holder's repository is not a key anywhere in it; the org is the directory it is filed under (workspaces/<org>/).So
resumederivesROOT_REPOSITORY="$MANIFEST_ORG/$MANIFEST_DIRNAME"from that org and the family folder inroot:, and that derivation is kept as the fallback. It is exact for a v1.1.0 layout, where the folder IS the repository's last part, and it is all a fresh machine has to go on (nofamily.yamlexists to read before the holder is cloned).Where a holder is already on disk, its own
family.yamlknows better, and that matters for an estate laid out before v1.1.0 (folder = display name, which can be another word than the repository).resumereads$ESTATE_ROOT/family.yaml, the holder this run RESOLVED (not one looked up by the record's folder, which goes stale when a person followsmake siblings' ownmv QuillWorks/QuillWorks Quill/Quill), once bothESTATE_PRESENTlookups have run, and names that repository on itsrecords ... root repositoryline instead oftestorg/QuillWorks, a repository that does not exist. Therecordsline is said there, after those silent lookups, so the order of the lines is unchanged. (The clone itself happens only when no holder is on disk, so on a fresh machine the record's own derivation is what clones.)Tests
New fixtures:
FAMILY_TWO_NAMES_YAML(aname:that differs from the repository),probe_family_two_names, and a session fixturenamed_apart(aQuill.githolder, made from the suite's own holder with onlyfamily.yamlrewritten, plus a workspace carrying a record for each folder).FAMILY_YAMLand every existing fixture build exactly what they built before. The tests sit beside the existing family-folder and fresh-machine tests intests/test_park_resume_commands.pyandtests/test_status_command.py.test_a_family_folder_is_found_by_its_repository_name_or_its_display_name(park, resume ×Quill/Quill,QuillWorks/QuillWorks)test_a_family_folder_that_is_neither_name_still_refuses(park, resume × 4 shapes)repository:, with norepository:key at all, or with only a MEMBER's own indentedrepository:ending in the folder name, is refused as beforetest_a_family_folder_named_for_its_repository_wins_over_a_standalone_root(park)name:differstest_resume_clones_the_holder_under_its_repositorys_name_when_the_display_name_differstestorg/Quill, clones toQuill/Quill, the pinned v1.1.0make siblingsplaces both members, exit 0test_resume_names_the_repository_the_holders_family_yaml_givesresumereportstestorg/Quill, nottestorg/QuillWorkstest_resume_names_the_repository_of_the_holder_it_resolved_not_the_records_folder(2 cases)quill.yaml,family: quill,root: QuillWorks/Alpha) with the holder moved toQuill/Quill(resume Quill), and the same record with the holder still inQuillWorks/QuillWorks(resume quill): both saytestorg/Quill..._is_read_by_its_repository_name_or_its_display_name(2),..._is_still_not_an_estate(4),..._wins_over_a_standalone_root(1, and its exit is checked not to be 2)statusRed and green, 24 new cases (19 in the first round, 5 in the review round: the 2 stale-record cases and 3 nested-
repository:cases). With the three scripts as they are on main, run at both pins (7f84ca4before the first merge,190133dafter): the first round's 19 give 7 fail, 12 pass.Quill/Quillcases of the three commands (park, resume, status), the standalone-root test for park and for status,test_resume_clones_the_holder_under_its_repositorys_name_...(REFUSED:was cloned, but ... is not an estate) andtest_resume_names_the_repository_the_holders_family_yaml_gives(printstestorg/QuillWorks).QuillWorks/QuillWorksfound, the three refusal shapes): they hold today's behaviour so the fix cannot widen it. They are made to fail by a wrong fix: mutating the rule to the FIRST part ofrepository:fails 9, accepting a missingrepository:key fails 3, stoppingresumereadingfamily.yamlfails exactly the on-disk test, and reverting onlyis_family_folderfails 6.190133d. Against the PR's previous headc824e4e's scripts, the moved-holder stale-record case fails (testorg/QuillWorks) and the other 15 of the 16 round-two selections pass; against main's scripts the stale-record cases and theQuill/Quillstandalone-root cases fail (4 fail, 12 pass). The nested-repository:cases are guards: an indentation-blindyaml_scalar(nested keys read as top-level) fails all 3.190133d(the v1.1.0 pin mounted by Pin openRepoShape at v1.1.0, 190133d on its main: six of the eight mounted files moved (bootstrap places legs only where no commit is lost; the family Makefile refuses a holder without siblings.py); no test expectation moved #233).Modules, with the fix, at the
190133dpin, merged with mainf5c3bcd, everyLANES_*andTMUX*unset:tests/test_park_resume_commands.py153 passed,tests/test_status_command.py207 passed,tests/test_repo_hygiene.py189 passed: 549 passed. The resolver byte-identity test is in the first, and the resolver block has one md5 across the three scripts.Not in this PR
Quill/Quill/family.yamlexists but names another family): the refusal wording is unchanged here, by design of this slice.nullvalue shapes against v1.1.0'sparse_yaml, both layouts of one family listed as two rows,resume quill(the id) against a stale record, the cwd walk-up test for a repository-named family, andnamed_apart's unrealistic second record.resume'sroot_legs_okandstatus'sread_legcall; this PR touches neither.Fixes #236
🤖 Generated with Claude Code