fix(audit-git-checkouts): submodule gates should keep what origin lacks and remove what it holds #81

Merged
jercik merged 5 commits from fix/audit-submodule-followups into main 2026-09-25 18:58:46 +00:00
Owner

Fixes the five submodule bugs deferred on #78, and applies the wording and test items deferred on #78, #79, and #80.

It also closes five paths by which removal deleted a submodule's unique content without inspecting it: ignored files inside the submodule, a Git directory left by git submodule deinit, the submodule's own linked worktrees, a tag on a blob or tree, and files under a Gitlink path with no checkout. Each now keeps the worktree as a decision.

--force now follows Git's own refusal (a populated Gitlink or a modules/ directory) instead of the presence of .gitmodules. Upstream tags inside a submodule no longer block removal; only a tag that origin lacks, or names for another object, does.

🤖 Generated with Claude Code

Fixes the five submodule bugs deferred on #78, and applies the wording and test items deferred on #78, #79, and #80. It also closes five paths by which removal deleted a submodule's unique content without inspecting it: ignored files inside the submodule, a Git directory left by `git submodule deinit`, the submodule's own linked worktrees, a tag on a blob or tree, and files under a Gitlink path with no checkout. Each now keeps the worktree as a decision. `--force` now follows Git's own refusal (a populated Gitlink or a `modules/` directory) instead of the presence of `.gitmodules`. Upstream tags inside a submodule no longer block removal; only a tag that origin lacks, or names for another object, does. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(audit-git-checkouts): submodule gates should remove what origin holds and keep what it lacks
All checks were successful
commit-msg / commitlint (pull_request) Successful in 22s
Node tests / node:test (pull_request) Successful in 1m16s
Review / Review (pull_request_target) Successful in 6m23s
5a76becc36
- Count a submodule tag as local work only when origin lacks it or names
  another object, so upstream tags no longer block removal forever.
- Force removal when Git would refuse it (a populated Gitlink or a
  modules/ directory), not when .gitmodules exists, and keep a worktree
  whose modules/ holds a submodule Git directory without a checkout,
  which a forced removal would delete unseen.
- Fail the submodule walk when the superproject index cannot be read.
- Carry the update walk's error into strandedSubmodulesError and the
  failure row; scope that row and the stranded-submodule decision to
  checkouts on the default branch, the only ones the driver updates.
- Apply the wording and test items deferred on #78, #79, and #80.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Review 01M3CM491QHBEFY37DG68PZJCJ — head 5a8ae47b63ed2dafd3a08bc8efb6ed25cd34a1b8

Review — j4k-oss/agent-skills @ 19897a019b

Scope: diff against base tree 08872ffc9b56
Status: dispatched — coverage complete (3/3 slots terminal)
Facts: current review-wide projection

Computed under:

{
  "abandonment": "abandonment-v1",
  "anchor_recipe": 1,
  "batch_policy": "batch-v1",
  "coverage": "coverage-v3",
  "dispatch_policy": "dispatch-v2",
  "grounder_version": 1,
  "grounding_read_rule": "grounding-read-v1",
  "promotion_policy": "promotion-v1",
  "report": "report-v3",
  "tally": "tally-v1",
  "triage_settle": "triage-settle-v2"
}

Findings (2)

low — (not checked out) resolution uses <modules> and <path> placeholders the entry doesn't supply, and gives no way to map the listed name to a checkout path

  • claim: 01M3CM8KYQHXHR3WX1YYHYJTA5
  • anchor: skills/audit-git-checkouts/references/removal-gates.md (snippet)
Then, on their decision, restore the checkout with `git submodule update --init -- <path>` while a Gitlink remains, or delete the directory, and rerun.
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
  • disposition: none

What I examined: the new judgment/submodule-local-work row of the "Resolve a kept worktree" table and the three-form list above it in skills/audit-git-checkouts/references/removal-gates.md; the driver code that writes these entries (list_unchecked_submodule_git_dirs in scripts/audit-checkouts.sh, which appends " (not checked out)").

What the text says: the list defines a <name> (not checked out) entry as the Git directory's path relative to $(git -C <worktree> rev-parse --path-format=absolute --git-path modules), "which is the submodule's name, not its checkout path". A nested one is listed "by its worktree-relative path instead, such as dep/.git/modules/inner". The resolution cell then uses <modules>/<name> in the inspection commands and ends with git submodule update --init -- <path>.

What goes wrong: <modules> is never defined as a name; the reader has to work out that it means the output of the command in the bullet above. <path> is a checkout path, but the entry gives only the name, and the text itself warns that the two differ. Nothing tells the agent how to get from one to the other, for example through submodule.<name>.path in .gitmodules. For the nested form, neither <modules>/<name> nor <path> applies directly: the Git directory is under dep/, and git submodule update has to run inside dep with dep's own path for inner. An agent following the cell literally is likely to pass the name as the pathspec (which fails, or matches a different submodule) or to guess. The writing skill asks for one term per concept and for project-local terms to be defined on first use, and says to spend detail where a plausible mistake would derail the task.

Proposed correction: in the <name> (not checked out) bullet, name the directory once, e.g. "… under <modules>, the output of git -C <worktree> rev-parse --path-format=absolute --git-path modules". In the resolution, state the mapping: "restore the checkout with git submodule update --init -- <path>, where <path> is git config -f .gitmodules submodule.<name>.path, while a Gitlink remains; for a nested entry, run it inside the enclosing submodule." This keeps the existing safety ordering (inspect first, then restore or delete) and removes the guesswork.

Evidence: static reading of the reference and the driver. I didn't run the resolution commands.

low — --help says submodule Git directories are deleted only once every commit is held by a remote-tracking ref, but the gate also accepts commits that only an origin tag holds

  • claim: 01M3CMGDAWQGRCQQGWEHE4R0MG
  • anchor: skills/audit-git-checkouts/scripts/audit-checkouts.sh (snippet)
  echo "worktree deletes its submodules' Git directories, local branches included, once every"
  echo "commit there is held by a remote-tracking ref."
  • lens: general-bug · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
  • disposition: none

What I examined: the new usage() text in audit-checkouts.sh, the removal-mode branch of submodule_holds_local_work, and the new driver test "a submodule tag counts as local work only when origin lacks it or names another object" in audit-checkouts.test.mjs.

What the help says: "Removing a worktree deletes its submodules' Git directories, local branches included, once every commit there is held by a remote-tracking ref."

What the code does: submodule_holds_local_work (removal mode) checks tags separately. For each local tag it first runs grep -F -x -q -- "$tag_object $tag_ref" <<<"$remote_tags" && continue, where remote_tags comes from git ls-remote --tags origin. For a lightweight tag it also accepts a match on origin's peeled ^{} line. Only when neither matches does it run rev-list -n 1 "$tag_object" --not --remotes. So a submodule tag on a commit that no remote-tracking ref holds passes the gate whenever origin advertises the same tag. The worktree is then removed with --force, and that commit is deleted with the submodule Git directory. The new test depends on this. tagUpstreamReleaseOffBranch creates v0.9 on a commit whose branch was deleted upstream, and the test ends with assert.equal(remove().removal.outcome, "removed"). removal-gates.md describes the exemption correctly ("Such a tag does not count when origin has the same tag on the same object…"). Earlier in the same help text, the Refuses list also qualifies it ("a tag on such a commit that origin lacks"). Only this closing sentence states the stricter remote-tracking-ref guarantee.

What goes wrong: an operator deciding from --help whether removal is safe is told that every deleted commit is still on a remote-tracking ref. In fact some commits survive only as an upstream tag, which the server can move or delete, and no local remote-tracking ref backs them. The docs contradict each other and understate what removal relies on.

Evidence: static reading plus the in-tree test above. I could not run the driver tests because jq is not installed in this sandbox. Fix: reword the sentence, e.g. "…once a remote-tracking ref holds every branch and HEAD commit there, and origin has every tag on a commit that none holds."

Other claims

  • grounding-pending (0)
  • ungrounded (0)
  • rejected (2)
    • 01M3CM81NTXYWX1GTSFFCSKDD4 low — Default-mode "What changes" cell ties submodule moves to the fast-forward and no longer says Gitlink changes are left unstaged
    • 01M3CM8MB3E5S2SCKBHJK8EFNJ low — Resolution for judgment/submodule-local-work doesn't cover (files without a checkout) entries: those files can't be pushed or "discarded in the submodule"
  • duplicate-of (0)
  • unadjudicated (0)

Coverage

Coverage pass: 01M3CM494T6QVACD7TVNYQXJCM
Accounting: complete
Slot health: healthy

lens part arm unit status runs loss
general-bug whole default claims-emitted 1 no
writing-quality whole default claims-emitted 1 no
test-trimming whole default no-claims 1 no
<!-- review:summary --> **Review** `01M3CM491QHBEFY37DG68PZJCJ` — head `5a8ae47b63ed2dafd3a08bc8efb6ed25cd34a1b8` # Review — j4k-oss/agent-skills @ 19897a019b18 Scope: diff against base tree `08872ffc9b56` Status: dispatched — coverage complete (3/3 slots terminal) Facts: current review-wide projection Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v3", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v3", "tally": "tally-v1", "triage_settle": "triage-settle-v2" } ``` ## Findings (2) ### low — `(not checked out)` resolution uses `<modules>` and `<path>` placeholders the entry doesn't supply, and gives no way to map the listed name to a checkout path - claim: `01M3CM8KYQHXHR3WX1YYHYJTA5` - anchor: `skills/audit-git-checkouts/references/removal-gates.md` (snippet) ``` Then, on their decision, restore the checkout with `git submodule update --init -- <path>` while a Gitlink remains, or delete the directory, and rerun. ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: the new `judgment/submodule-local-work` row of the "Resolve a kept worktree" table and the three-form list above it in skills/audit-git-checkouts/references/removal-gates.md; the driver code that writes these entries (`list_unchecked_submodule_git_dirs` in scripts/audit-checkouts.sh, which appends " (not checked out)"). > > What the text says: the list defines a `<name> (not checked out)` entry as the Git directory's path relative to `$(git -C <worktree> rev-parse --path-format=absolute --git-path modules)`, "which is the submodule's name, not its checkout path". A nested one is listed "by its worktree-relative path instead, such as `dep/.git/modules/inner`". The resolution cell then uses `<modules>/<name>` in the inspection commands and ends with `git submodule update --init -- <path>`. > > What goes wrong: `<modules>` is never defined as a name; the reader has to work out that it means the output of the command in the bullet above. `<path>` is a checkout path, but the entry gives only the name, and the text itself warns that the two differ. Nothing tells the agent how to get from one to the other, for example through `submodule.<name>.path` in `.gitmodules`. For the nested form, neither `<modules>/<name>` nor `<path>` applies directly: the Git directory is under `dep/`, and `git submodule update` has to run inside `dep` with dep's own path for `inner`. An agent following the cell literally is likely to pass the name as the pathspec (which fails, or matches a different submodule) or to guess. The writing skill asks for one term per concept and for project-local terms to be defined on first use, and says to spend detail where a plausible mistake would derail the task. > > Proposed correction: in the `<name> (not checked out)` bullet, name the directory once, e.g. "… under `<modules>`, the output of `git -C <worktree> rev-parse --path-format=absolute --git-path modules`". In the resolution, state the mapping: "restore the checkout with `git submodule update --init -- <path>`, where `<path>` is `git config -f .gitmodules submodule.<name>.path`, while a Gitlink remains; for a nested entry, run it inside the enclosing submodule." This keeps the existing safety ordering (inspect first, then restore or delete) and removes the guesswork. > > Evidence: static reading of the reference and the driver. I didn't run the resolution commands. ### low — --help says submodule Git directories are deleted only once every commit is held by a remote-tracking ref, but the gate also accepts commits that only an origin tag holds - claim: `01M3CMGDAWQGRCQQGWEHE4R0MG` - anchor: `skills/audit-git-checkouts/scripts/audit-checkouts.sh` (snippet) ``` echo "worktree deletes its submodules' Git directories, local branches included, once every" echo "commit there is held by a remote-tracking ref." ``` - lens: general-bug · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - disposition: none > What I examined: the new `usage()` text in audit-checkouts.sh, the removal-mode branch of `submodule_holds_local_work`, and the new driver test "a submodule tag counts as local work only when origin lacks it or names another object" in audit-checkouts.test.mjs. > > What the help says: "Removing a worktree deletes its submodules' Git directories, local branches included, once every commit there is held by a remote-tracking ref." > > What the code does: `submodule_holds_local_work` (removal mode) checks tags separately. For each local tag it first runs `grep -F -x -q -- "$tag_object $tag_ref" <<<"$remote_tags" && continue`, where `remote_tags` comes from `git ls-remote --tags origin`. For a lightweight tag it also accepts a match on origin's peeled `^{}` line. Only when neither matches does it run `rev-list -n 1 "$tag_object" --not --remotes`. So a submodule tag on a commit that no remote-tracking ref holds passes the gate whenever origin advertises the same tag. The worktree is then removed with `--force`, and that commit is deleted with the submodule Git directory. The new test depends on this. `tagUpstreamReleaseOffBranch` creates `v0.9` on a commit whose branch was deleted upstream, and the test ends with `assert.equal(remove().removal.outcome, "removed")`. removal-gates.md describes the exemption correctly ("Such a tag does not count when origin has the same tag on the same object…"). Earlier in the same help text, the Refuses list also qualifies it ("a tag on such a commit that origin lacks"). Only this closing sentence states the stricter remote-tracking-ref guarantee. > > What goes wrong: an operator deciding from `--help` whether removal is safe is told that every deleted commit is still on a remote-tracking ref. In fact some commits survive only as an upstream tag, which the server can move or delete, and no local remote-tracking ref backs them. The docs contradict each other and understate what removal relies on. > > Evidence: static reading plus the in-tree test above. I could not run the driver tests because `jq` is not installed in this sandbox. Fix: reword the sentence, e.g. "…once a remote-tracking ref holds every branch and HEAD commit there, and origin has every tag on a commit that none holds." ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (2) - `01M3CM81NTXYWX1GTSFFCSKDD4` low — Default-mode "What changes" cell ties submodule moves to the fast-forward and no longer says Gitlink changes are left unstaged - `01M3CM8MB3E5S2SCKBHJK8EFNJ` low — Resolution for `judgment/submodule-local-work` doesn't cover `(files without a checkout)` entries: those files can't be pushed or "discarded in the submodule" - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M3CM494T6QVACD7TVNYQXJCM Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | claims-emitted | 1 | no | | writing-quality | whole | default | claims-emitted | 1 | no | | test-trimming | whole | default | no-claims | 1 | no |
@ -5,3 +5,3 @@
## What the driver already updates
The driver fast-forwards a default-branch checkout only when its comparison is fresh, it has no local commits, and it is strictly behind `origin/<default>`. A dirty checkout qualifies only when every changed path is deferred guidance (`AGENTS.md`, `.agents/**`, at any depth), a first-party `.gitmodules` edit, or a first-party Gitlink change on a submodule with a `branch` or `tag` selector, and upstream did not touch those paths. The merge runs `--ff-only` with `merge.autostash=false`, because autostash would round-trip the tree through a stash and silently unstage staged guidance. After a fast-forward it initializes committed submodules and checks them out at the recorded Gitlinks. When any populated submodule, at any depth, found from the Gitlinks rather than `.gitmodules`, is checked out at a commit that its superproject does not record and no ref holds, the driver skips the fast-forward and every selector move for that checkout, and the report lists it under "Needs your decision" with the submodule's path.
The driver fast-forwards a default-branch checkout only when its comparison is fresh, it has no local commits, and it is strictly behind `origin/<default>`. A dirty checkout qualifies only when every changed path is deferred guidance (`AGENTS.md`, `.agents/**`, at any depth), a first-party `.gitmodules` edit, or a first-party Gitlink change on a submodule with a `branch` or `tag` selector, and upstream did not touch those paths. The merge runs `--ff-only` with `merge.autostash=false`, because autostash would round-trip the tree through a stash and silently unstage staged guidance. After a fast-forward it initializes committed submodules and checks them out at the recorded Gitlinks. The driver skips the fast-forward and every selector move for a checkout holding a populated submodule, at any depth, that is checked out at a commit its superproject does not record and no ref in the submodule holds; the report lists the checkout under "Needs your decision" with the submodule's path. Submodules are found from the Gitlinks in HEAD and the index, so neither a removed `.gitmodules` nor an `ignore` setting hides one. When that submodule check itself fails, the driver changes nothing in the checkout, and its failure row says the submodule check failed and names the submodule it could not read: make that submodule readable, then rerun.

medium — checkout-updates.md promises the failure row always names an unreadable submodule, but several failure paths show a Git error or the superproject index instead
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new sentence in references/checkout-updates.md (anchored), the walk in scripts/audit-checkouts.sh (list_gitlink_paths, list_submodules_with_local_work, submodule_holds_local_work), how the driver captures its stderr (list_submodules_with_local_work "$worktree_path" update 2>"$stranded_error_path"), and how the renderer builds the row in render-audit-report.ts: submodule check failed; checkout not updated${detail ? : ${detail} : ""} where detail = firstLine(worktree.strandedSubmodulesError) and firstLine returns only the first line of the text.

What the code does: only one failure path writes a message that names the submodule first. That path is rev-parse --verify --quiet HEAD failing, which prints cannot read submodule <path>. The other paths put something else on the first line:

  • list_gitlink_paths fails when git ls-files --stage cannot read the superproject's index (|| return 1). The first line is then Git's own stderr, such as an index-corruption message, and it names no submodule. The new test a failed superproject index read fails the submodule walk exercises exactly this path. The same happens when the recursive call hits a nested submodule's index.
  • In update mode, git for-each-ref --contains can fail inside submodule_holds_local_work. Git's stderr then goes into the error file before the driver writes cannot inspect submodule <path>, so firstLine shows Git's message.

What goes wrong: an agent reading this reference expects the row to name a submodule and is told to "make that submodule readable". When the superproject index is the unreadable part, no submodule is named and the instruction points at the wrong repair. The writing skill asks for verifiable claims; this one is true for only one failure path.

Proposed correction: "When that submodule check itself fails, the driver changes nothing in the checkout, and its failure row says the submodule check failed, followed by the first line of the error. That line names the submodule when one could not be read and otherwise quotes Git. strandedSubmodulesError in the JSON record holds the full text. Repair what it names, then rerun." This keeps the safety fact (nothing changes) and the rerun step, and stops promising a submodule name.

Evidence status: static trace of the code plus the test named above. I did not run the driver to capture the rendered first line for each path.

claim 01M3C1GV4Z8640Q8HW35Y6SF16 of review 01M3C1D15AZRH3JXFPN27W5W94

<!-- review:claim:01M3C1GV4Z8640Q8HW35Y6SF16 --> **medium** — checkout-updates.md promises the failure row always names an unreadable submodule, but several failure paths show a Git error or the superproject index instead lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new sentence in `references/checkout-updates.md` (anchored), the walk in `scripts/audit-checkouts.sh` (`list_gitlink_paths`, `list_submodules_with_local_work`, `submodule_holds_local_work`), how the driver captures its stderr (`list_submodules_with_local_work "$worktree_path" update 2>"$stranded_error_path"`), and how the renderer builds the row in `render-audit-report.ts`: `submodule check failed; checkout not updated${detail ? `: ${detail}` : ""}` where `detail = firstLine(worktree.strandedSubmodulesError)` and `firstLine` returns only the first line of the text. > > What the code does: only one failure path writes a message that names the submodule first. That path is `rev-parse --verify --quiet HEAD` failing, which prints `cannot read submodule <path>`. The other paths put something else on the first line: > - `list_gitlink_paths` fails when `git ls-files --stage` cannot read the superproject's index (`|| return 1`). The first line is then Git's own stderr, such as an index-corruption message, and it names no submodule. The new test `a failed superproject index read fails the submodule walk` exercises exactly this path. The same happens when the recursive call hits a nested submodule's index. > - In update mode, `git for-each-ref --contains` can fail inside `submodule_holds_local_work`. Git's stderr then goes into the error file before the driver writes `cannot inspect submodule <path>`, so `firstLine` shows Git's message. > > What goes wrong: an agent reading this reference expects the row to name a submodule and is told to "make that submodule readable". When the superproject index is the unreadable part, no submodule is named and the instruction points at the wrong repair. The writing skill asks for verifiable claims; this one is true for only one failure path. > > Proposed correction: "When that submodule check itself fails, the driver changes nothing in the checkout, and its failure row says the submodule check failed, followed by the first line of the error. That line names the submodule when one could not be read and otherwise quotes Git. `strandedSubmodulesError` in the JSON record holds the full text. Repair what it names, then rerun." This keeps the safety fact (nothing changes) and the rerun step, and stops promising a submodule name. > > Evidence status: static trace of the code plus the test named above. I did not run the driver to capture the rendered first line for each path. claim `01M3C1GV4Z8640Q8HW35Y6SF16` of review `01M3C1D15AZRH3JXFPN27W5W94`
Author
Owner

Fixed in dc5750e. references/checkout-updates.md now says the row quotes the first line of the error, which names the submodule when one could not be read and otherwise quotes Git, and points at strandedSubmodulesError for the full text.

<!-- gh-feedback:reply-to:88476 --> Fixed in dc5750e. `references/checkout-updates.md` now says the row quotes the first line of the error, which names the submodule when one could not be read and otherwise quotes Git, and points at `strandedSubmodulesError` for the full text.
jercik marked this conversation as resolved
@ -19,3 +19,3 @@
A worktree lock is an owner pin that expires seven days after its `locked` file's mtime. `git worktree lock` refuses an already locked worktree, so the mtime dates from the original lock; to renew a pin, unlock and lock again. A future-dated lock counts as young. The driver unlocks an expired lock only after every other gate passes, immediately before removal. It reads the lock once, at the gate: a pin renewed during the containment proof and ignored scan that follow is unlocked anyway. If unlock fails, the outcome is `operational/removal-failed`. If removal then fails, the worktree stays unlocked and the next run evaluates it from scratch. Expiry deliberately overrides pins agents leave behind.
Worktrees containing `.gitmodules` are removed with `--force`, because Git otherwise refuses any worktree with submodules; the repeated status check replaces the check `--force` disables. Removal also deletes each submodule's Git directory, local branches and stash included, so right before the status check the driver walks every populated submodule at any depth. A submodule keeps the worktree (`judgment/submodule-local-work`, paths in `removal.error`) when it has uncommitted files, a stash, a branch or tag commit no remote-tracking ref holds, or a HEAD no remote-tracking ref holds. A Gitlink names a commit without keeping it, so a recorded HEAD needs a remote-tracking ref too. Ignored files inside submodules are not scanned.
Git refuses to remove a worktree with a populated Gitlink or a submodule Git directory under the worktree's own `modules/`, with or without `.gitmodules`, so the driver removes such a worktree with `--force`; the repeated status check replaces the check `--force` disables. Removal also deletes each submodule's Git directory, local branches and stash included, so immediately before the second status check the driver walks every populated submodule at any depth. A submodule keeps the worktree (`judgment/submodule-local-work`, paths in `removal.error`) when it has uncommitted files, a stash, a HEAD or branch commit no remote-tracking ref holds, or a tag on such a commit that origin lacks or names for another object. A Gitlink names a commit without holding it, so a recorded HEAD needs a remote-tracking ref too. A submodule Git directory with no checkout, such as one `git submodule deinit` leaves, cannot be inspected, so it keeps the worktree too, listed as `<name> (not checked out)`. Ignored files inside submodules are not scanned.

low — removal-gates.md's submodule tag clause ("a tag on such a commit that origin lacks or names for another object") is hard to parse and can be read too narrowly
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the paragraph in references/removal-gates.md that explains when a submodule keeps a worktree (judgment/submodule-local-work). I compared it with the tag loop in submodule_holds_local_work in scripts/audit-checkouts.sh. That loop treats a local tag as work when its objectname\trefname line is missing from git ls-remote --tags origin and rev-list -n 1 <tag> --not --remotes is non-empty.

What the text says: "when it has uncommitted files, a stash, a HEAD or branch commit no remote-tracking ref holds, or a tag on such a commit that origin lacks or names for another object."

What goes wrong: "such a commit" refers back to "a HEAD or branch commit no remote-tracking ref holds". A literal reader can take it to mean the tag rule applies only to tags on the HEAD or on a branch commit. The code applies it to any tag whose commit no remote-tracking ref holds, whatever else points there. "names for another object" is a garden-path phrase: the reader has to work out that it means "origin has a tag of the same name pointing at a different object". This paragraph is what an agent reads before explaining to the owner why a merged worktree was kept, so the owner can end up with a wrong account of which tags matter. The writing skill asks for claims that can be checked and for conditions attached to the thing they govern.

Proposed correction: "...a stash, a HEAD or branch commit no remote-tracking ref holds, or a tag whose commit no remote-tracking ref holds, unless origin has the same tag on the same object." This keeps both tag conditions, drops the back-reference, and matches the code.

Evidence status: static comparison of the prose with the script. No runtime claim.

claim 01M3C1J0848CBQ4B86RAC5K4CZ of review 01M3C1D15AZRH3JXFPN27W5W94

<!-- review:claim:01M3C1J0848CBQ4B86RAC5K4CZ --> **low** — removal-gates.md's submodule tag clause ("a tag on such a commit that origin lacks or names for another object") is hard to parse and can be read too narrowly lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the paragraph in `references/removal-gates.md` that explains when a submodule keeps a worktree (`judgment/submodule-local-work`). I compared it with the tag loop in `submodule_holds_local_work` in `scripts/audit-checkouts.sh`. That loop treats a local tag as work when its `objectname\trefname` line is missing from `git ls-remote --tags origin` and `rev-list -n 1 <tag> --not --remotes` is non-empty. > > What the text says: "when it has uncommitted files, a stash, a HEAD or branch commit no remote-tracking ref holds, or a tag on such a commit that origin lacks or names for another object." > > What goes wrong: "such a commit" refers back to "a HEAD or branch commit no remote-tracking ref holds". A literal reader can take it to mean the tag rule applies only to tags on the HEAD or on a branch commit. The code applies it to any tag whose commit no remote-tracking ref holds, whatever else points there. "names for another object" is a garden-path phrase: the reader has to work out that it means "origin has a tag of the same name pointing at a different object". This paragraph is what an agent reads before explaining to the owner why a merged worktree was kept, so the owner can end up with a wrong account of which tags matter. The writing skill asks for claims that can be checked and for conditions attached to the thing they govern. > > Proposed correction: "...a stash, a HEAD or branch commit no remote-tracking ref holds, or a tag whose commit no remote-tracking ref holds, unless origin has the same tag on the same object." This keeps both tag conditions, drops the back-reference, and matches the code. > > Evidence status: static comparison of the prose with the script. No runtime claim. claim `01M3C1J0848CBQ4B86RAC5K4CZ` of review `01M3C1D15AZRH3JXFPN27W5W94`
Author
Owner

Fixed in dc5750e with the proposed wording, extended for the tag cases this round added (a tag that names no commit; a lightweight tag matching origin's tag on its commit).

<!-- gh-feedback:reply-to:88478 --> Fixed in dc5750e with the proposed wording, extended for the tag cases this round added (a tag that names no commit; a lightweight tag matching origin's tag on its commit).
jercik marked this conversation as resolved
@ -46,3 +46,3 @@
| `judgment/sparse-checkout` | Keep the cone's `skip-worktree` bits: clearing them turns absent files into apparent deletions. Confirm the cone with `git sparse-checkout list`, verify no file outside it is materialized, then remove manually once every other gate holds. |
| `judgment/precious-ignored-files` | Leave `preciousPaths` in place until the owner disposes of them or authorizes their deletion, then rerun. |
| `judgment/submodule-local-work` | The listed submodules hold work only this worktree has. Show the owner what each holds; rerun once it is pushed or they authorize discarding it. |
| `judgment/submodule-local-work` | The listed submodules hold work only this worktree has. Show the owner what each holds; rerun once the work is pushed. If the owner authorizes discarding it instead, discard it in the submodule first, then rerun. For a `(not checked out)` entry, run `git submodule update --init` in the worktree first, so the next run can inspect it. |

medium — (not checked out) remedy fails for a submodule removed with git rm, so the worktree stays kept on every run
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new removal gate in scripts/audit-checkouts.sh (list_populated_submodule_git_dirs, list_submodule_git_dirs_without_checkout, and the block in the removal gate that writes <name> (not checked out) and returns judgment/submodule-local-work), plus the new remediation row for that outcome in references/removal-gates.md.

Mechanism: list_submodule_git_dirs_without_checkout flags every Git directory under the worktree's modules/ (it finds each HEAD file that sits next to an objects/ directory) unless a populated submodule uses it. The populated set comes from list_gitlink_paths, which reads Gitlinks from HEAD and the index. The only remedy the reference gives is to run git submodule update --init in the worktree and rerun. That only helps when a Gitlink still exists, as after git submodule deinit. It does nothing when the branch removed the submodule, and git rm <submodule> is the standard way to do that. git rm deletes the working directory and the Gitlink but deliberately keeps $GIT_DIR/modules/<name>.

Reproduction (observed; Git 2.47.3, script sourced in bash): create a superproject with submodule dependency and add a linked worktree on feature. Run git submodule update --init in the worktree, then git rm dependency and commit. ls $(git rev-parse --path-format=absolute --git-path modules) still prints dependency. Running git submodule update --init in the worktree afterwards prints nothing and populates nothing. list_populated_submodule_git_dirs <wt> then returns an empty list, and list_submodule_git_dirs_without_checkout <modules> "" prints dependency. So the gate reports dependency (not checked out) again.

Impact: a merged feature branch that removed a submodule is never removed by the driver. The agent-facing runbook sends the agent through a remedy that cannot clear the gate, so a rerun produces the same kept row. The reference gives no other route; its only other advice is to push or discard work "in the submodule", and there is no checkout to do that in.

Fix: in the remediation row, cover a Git directory whose submodule has no Gitlink in HEAD or the index. For example, inspect it with git --git-dir <modules>/<name> (branches, stash, and commits no remote-tracking ref holds), and remove the directory only on the owner's authorization. Alternatively, have the driver inspect such an orphaned Git directory directly with --git-dir instead of refusing it outright.

Not verified: I did not run the full driver end to end, because jq is not installed in this sandbox. The gate functions and the Git behaviour were run directly.

claim 01M3C1MZJYWWWCG1R79GZRSW8J of review 01M3C1D15AZRH3JXFPN27W5W94

<!-- review:claim:01M3C1MZJYWWWCG1R79GZRSW8J --> **medium** — `(not checked out)` remedy fails for a submodule removed with `git rm`, so the worktree stays kept on every run lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new removal gate in `scripts/audit-checkouts.sh` (`list_populated_submodule_git_dirs`, `list_submodule_git_dirs_without_checkout`, and the block in the removal gate that writes `<name> (not checked out)` and returns `judgment/submodule-local-work`), plus the new remediation row for that outcome in `references/removal-gates.md`. > > Mechanism: `list_submodule_git_dirs_without_checkout` flags every Git directory under the worktree's `modules/` (it finds each `HEAD` file that sits next to an `objects/` directory) unless a populated submodule uses it. The populated set comes from `list_gitlink_paths`, which reads Gitlinks from HEAD and the index. The only remedy the reference gives is to run `git submodule update --init` in the worktree and rerun. That only helps when a Gitlink still exists, as after `git submodule deinit`. It does nothing when the branch removed the submodule, and `git rm <submodule>` is the standard way to do that. `git rm` deletes the working directory and the Gitlink but deliberately keeps `$GIT_DIR/modules/<name>`. > > Reproduction (observed; Git 2.47.3, script sourced in bash): create a superproject with submodule `dependency` and add a linked worktree on `feature`. Run `git submodule update --init` in the worktree, then `git rm dependency` and commit. `ls $(git rev-parse --path-format=absolute --git-path modules)` still prints `dependency`. Running `git submodule update --init` in the worktree afterwards prints nothing and populates nothing. `list_populated_submodule_git_dirs <wt>` then returns an empty list, and `list_submodule_git_dirs_without_checkout <modules> ""` prints `dependency`. So the gate reports `dependency (not checked out)` again. > > Impact: a merged feature branch that removed a submodule is never removed by the driver. The agent-facing runbook sends the agent through a remedy that cannot clear the gate, so a rerun produces the same kept row. The reference gives no other route; its only other advice is to push or discard work "in the submodule", and there is no checkout to do that in. > > Fix: in the remediation row, cover a Git directory whose submodule has no Gitlink in HEAD or the index. For example, inspect it with `git --git-dir <modules>/<name>` (branches, stash, and commits no remote-tracking ref holds), and remove the directory only on the owner's authorization. Alternatively, have the driver inspect such an orphaned Git directory directly with `--git-dir` instead of refusing it outright. > > Not verified: I did not run the full driver end to end, because `jq` is not installed in this sandbox. The gate functions and the Git behaviour were run directly. claim `01M3C1MZJYWWWCG1R79GZRSW8J` of review `01M3C1D15AZRH3JXFPN27W5W94`
Author
Owner

Fixed in dc5750e. Reproduced: after git rm -f the worktree's modules/<name> stays and git submodule update --init repopulates nothing. The gate stays conservative; the judgment/submodule-local-work row in references/removal-gates.md now covers that case: show the owner what the Git directory holds (git --git-dir <dir> --work-tree <dir> log --oneline --branches --tags --not --remotes, and stash list; plain --git-dir fails because the directory's core.worktree names the deleted checkout), delete it only on their authorization, then rerun.

<!-- gh-feedback:reply-to:88477 --> Fixed in dc5750e. Reproduced: after `git rm -f` the worktree's `modules/<name>` stays and `git submodule update --init` repopulates nothing. The gate stays conservative; the `judgment/submodule-local-work` row in `references/removal-gates.md` now covers that case: show the owner what the Git directory holds (`git --git-dir <dir> --work-tree <dir> log --oneline --branches --tags --not --remotes`, and `stash list`; plain `--git-dir` fails because the directory's `core.worktree` names the deleted checkout), delete it only on their authorization, then rerun.
jercik marked this conversation as resolved
Lines 505-506
@ -491,3 +503,4 @@
# remote-tracking ref, and the submodule must have no uncommitted
# files, no stash, and no branch or tag commit that no
# files, no stash, no branch commit that no remote-tracking ref
# holds, and no tag that origin lacks whose commit no
# remote-tracking ref holds.

low — Header of list_submodules_with_local_work states the removal tag rule without the "origin names another object" case that the code and docs enforce
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the header comment of list_submodules_with_local_work in scripts/audit-checkouts.sh, the tag loop in submodule_holds_local_work, the comment above that loop, references/removal-gates.md, and the new test a submodule tag counts as local work only when origin lacks it or names another object.

What the code does: for each local tag, it compares the whole line %(objectname)\t%(refname) against git ls-remote --tags origin using grep -F -x. A tag is skipped only when origin has the same name pointing at the same object. Otherwise, if rev-list -n 1 <tag> --not --remotes is non-empty, the tag counts as work. The comment above the loop says this correctly: "only a tag origin lacks, or names another object for, is local work." The test confirms it. A local annotated v0.9 that origin also has, but as a different object, keeps the worktree. Resetting it to the upstream object lets removal proceed.

What goes wrong: the header comment, which is the function's contract for the removal mode, says only "no tag that origin lacks whose commit no remote-tracking ref holds". A maintainer reading the header would conclude that any tag name origin also has is safe, which is exactly the case the new test guards against. The rule now appears twice in the same file and the two copies disagree. The writing skill's "One Idea, One Place" section warns about this kind of duplication.

Proposed correction: make the header complete and let the inner comment keep only the reason. For example: "... no branch commit that no remote-tracking ref holds, and no tag whose commit no remote-tracking ref holds unless origin has that tag on the same object." Then trim the inner comment to its rationale: "A clone fetches every upstream tag, including ones on commits no remote branch holds; such a tag survives at origin." This keeps both the contract and the reason, with one statement of the rule.

Evidence status: static reading of the code and the named test; I did not run the tests.

claim 01M3C1HZT9GR87D52JE69SHKTW of review 01M3C1D15AZRH3JXFPN27W5W94

<!-- review:claim:01M3C1HZT9GR87D52JE69SHKTW --> **low** — Header of list_submodules_with_local_work states the removal tag rule without the "origin names another object" case that the code and docs enforce lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the header comment of `list_submodules_with_local_work` in `scripts/audit-checkouts.sh`, the tag loop in `submodule_holds_local_work`, the comment above that loop, `references/removal-gates.md`, and the new test `a submodule tag counts as local work only when origin lacks it or names another object`. > > What the code does: for each local tag, it compares the whole line `%(objectname)\t%(refname)` against `git ls-remote --tags origin` using `grep -F -x`. A tag is skipped only when origin has the same name pointing at the same object. Otherwise, if `rev-list -n 1 <tag> --not --remotes` is non-empty, the tag counts as work. The comment above the loop says this correctly: "only a tag origin lacks, or names another object for, is local work." The test confirms it. A local annotated `v0.9` that origin also has, but as a different object, keeps the worktree. Resetting it to the upstream object lets removal proceed. > > What goes wrong: the header comment, which is the function's contract for the `removal` mode, says only "no tag that origin lacks whose commit no remote-tracking ref holds". A maintainer reading the header would conclude that any tag name origin also has is safe, which is exactly the case the new test guards against. The rule now appears twice in the same file and the two copies disagree. The writing skill's "One Idea, One Place" section warns about this kind of duplication. > > Proposed correction: make the header complete and let the inner comment keep only the reason. For example: "... no branch commit that no remote-tracking ref holds, and no tag whose commit no remote-tracking ref holds unless origin has that tag on the same object." Then trim the inner comment to its rationale: "A clone fetches every upstream tag, including ones on commits no remote branch holds; such a tag survives at origin." This keeps both the contract and the reason, with one statement of the rule. > > Evidence status: static reading of the code and the named test; I did not run the tests. claim `01M3C1HZT9GR87D52JE69SHKTW` of review `01M3C1D15AZRH3JXFPN27W5W94`
Author
Owner

Fixed in dc5750e. The header now states the full rule, and the inner comment keeps only the reason.

<!-- gh-feedback:reply-to:88479 --> Fixed in dc5750e. The header now states the full rule, and the inner comment keeps only the reason.
jercik marked this conversation as resolved
@ -1553,6 +1644,7 @@ function createActivityFixture(context, headActionDate = "2001-09-09T01:46:40Z")
branch: { current: "feature", isDetached: false },
workingTree: { isClean: false, files: { staged: [], unstaged: ["sample.txt"], untracked: [] } },
}));
// A shim ending in a semicolon yields `;;` and a bash parse error, not a test failure.

low — Shim comment in createActivityFixture says a trailing ; causes "not a test failure", but the bash parse error does fail the test, and the rule is no longer stated
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: createActivityFixture in scripts/audit-checkouts.test.mjs. The anchored comment sits above const classify = (days = "14", defaultBranch = "main", shim = "") => {. Inside classify, the command is built as const annotationCommand = `source "$1"; ${shim ? `${shim}; ` : ""}annotate_activity_review ...`; and run with execFileSync("/bin/bash", ["-c", annotationCommand, ...]). The diff replaced the old comment, // Pass shimwithout a trailing;; the template adds one., with the anchored one and moved it above the function.

What goes wrong: a shim that ends in ; produces ;;, which bash rejects as a syntax error. execFileSync then throws on the non-zero exit, and the test fails. So "not a test failure" is false. The author probably meant "not an assertion failure that explains the cause", but the comment does not say that. The comment also drops the instruction a test author needs: omit the trailing ;. The reader now has to work that out from a description of the symptom. The writing skill says to lead with the action and prefer positive directives, and says claims should be verifiable. This comment gets the observable behavior wrong and leaves the action implicit.

Proposed correction: "// Pass shim without a trailing ;: the template appends one, and ;; makes bash fail to parse the command before any assertion runs." This keeps the useful fact (the failure is a confusing parse error) and states the rule first.

Evidence status: static reading of the template string and Node's documented execFileSync behavior, which throws on a non-zero exit. I did not run the test with a trailing-semicolon shim.

claim 01M3C1HBWP6ZA0NK30ET1WR7YQ of review 01M3C1D15AZRH3JXFPN27W5W94

<!-- review:claim:01M3C1HBWP6ZA0NK30ET1WR7YQ --> **low** — Shim comment in createActivityFixture says a trailing `;` causes "not a test failure", but the bash parse error does fail the test, and the rule is no longer stated lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: `createActivityFixture` in `scripts/audit-checkouts.test.mjs`. The anchored comment sits above `const classify = (days = "14", defaultBranch = "main", shim = "") => {`. Inside `classify`, the command is built as ``const annotationCommand = `source "$1"; ${shim ? `${shim}; ` : ""}annotate_activity_review ...`;`` and run with `execFileSync("/bin/bash", ["-c", annotationCommand, ...])`. The diff replaced the old comment, `// Pass `shim` without a trailing `;`; the template adds one.`, with the anchored one and moved it above the function. > > What goes wrong: a shim that ends in `;` produces `;;`, which bash rejects as a syntax error. `execFileSync` then throws on the non-zero exit, and the test fails. So "not a test failure" is false. The author probably meant "not an assertion failure that explains the cause", but the comment does not say that. The comment also drops the instruction a test author needs: omit the trailing `;`. The reader now has to work that out from a description of the symptom. The writing skill says to lead with the action and prefer positive directives, and says claims should be verifiable. This comment gets the observable behavior wrong and leaves the action implicit. > > Proposed correction: "// Pass `shim` without a trailing `;`: the template appends one, and `;;` makes bash fail to parse the command before any assertion runs." This keeps the useful fact (the failure is a confusing parse error) and states the rule first. > > Evidence status: static reading of the template string and Node's documented `execFileSync` behavior, which throws on a non-zero exit. I did not run the test with a trailing-semicolon shim. claim `01M3C1HBWP6ZA0NK30ET1WR7YQ` of review `01M3C1D15AZRH3JXFPN27W5W94`
Author
Owner

Fixed in dc5750e with the proposed comment: pass shim without a trailing ;, since ;; makes bash fail to parse before any assertion runs.

<!-- gh-feedback:reply-to:88480 --> Fixed in dc5750e with the proposed comment: pass `shim` without a trailing `;`, since `;;` makes bash fail to parse before any assertion runs.
jercik marked this conversation as resolved
fix(audit-git-checkouts): submodule removal gate should see every place unique work can hide
Some checks failed
commit-msg / commitlint (pull_request) Successful in 19s
Node tests / node:test (pull_request) Successful in 1m41s
Review / Review (pull_request_target) Failing after 4m51s
dc5750ea75
- Keep a worktree whose submodule has a linked worktree of its own, a tag
  on a blob or tree that origin lacks, or files under a Gitlink path with
  no checkout; each was deleted unseen.
- Treat a lightweight tag on the commit origin's tag peels to as held, and
  a submodule without an origin as unconfirmed rather than a failed check.
- Say how to clear a submodule Git directory that no Gitlink uses, and
  reword the failure-row, tag-rule, and shim comments the review flagged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ -19,3 +19,3 @@
A worktree lock is an owner pin that expires seven days after its `locked` file's mtime. `git worktree lock` refuses an already locked worktree, so the mtime dates from the original lock; to renew a pin, unlock and lock again. A future-dated lock counts as young. The driver unlocks an expired lock only after every other gate passes, immediately before removal. It reads the lock once, at the gate: a pin renewed during the containment proof and ignored scan that follow is unlocked anyway. If unlock fails, the outcome is `operational/removal-failed`. If removal then fails, the worktree stays unlocked and the next run evaluates it from scratch. Expiry deliberately overrides pins agents leave behind.
Worktrees containing `.gitmodules` are removed with `--force`, because Git otherwise refuses any worktree with submodules; the repeated status check replaces the check `--force` disables. Removal also deletes each submodule's Git directory, local branches and stash included, so right before the status check the driver walks every populated submodule at any depth. A submodule keeps the worktree (`judgment/submodule-local-work`, paths in `removal.error`) when it has uncommitted files, a stash, a branch or tag commit no remote-tracking ref holds, or a HEAD no remote-tracking ref holds. A Gitlink names a commit without keeping it, so a recorded HEAD needs a remote-tracking ref too. Ignored files inside submodules are not scanned.
Git refuses to remove a worktree with a populated Gitlink or a submodule Git directory under the worktree's own `modules/`, with or without `.gitmodules`, so the driver removes such a worktree with `--force`; the repeated status check replaces the check `--force` disables. Removal also deletes each submodule's Git directory, local branches and stash included, so immediately before the second status check the driver walks every populated submodule at any depth. A submodule keeps the worktree (`judgment/submodule-local-work`, paths in `removal.error`) when it has uncommitted files, a stash, a linked worktree of its own, a HEAD or branch commit no remote-tracking ref holds, or a tag that names no commit or whose commit no remote-tracking ref holds, unless origin has the same tag on the same object (a lightweight tag also matches origin's tag on its commit). A Gitlink names a commit without holding it, so a recorded HEAD needs a remote-tracking ref too. Files under a Gitlink path with no checkout keep the worktree as `<path> (files without a checkout)`, because status never lists them. A submodule Git directory with no checkout, such as one `git submodule deinit` or `git rm` leaves under `$(git -C <worktree> rev-parse --path-format=absolute --git-path modules)`, is not inspected, so it keeps the worktree too, listed as `<name> (not checked out)`. Ignored files inside submodules are not scanned.

low — The (not checked out) guidance never ties the reported <name> to the <dir> its commands act on
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the submodule paragraph in references/removal-gates.md, the judgment/submodule-local-work resolution row in the same file, and list_submodule_git_dirs_without_checkout in scripts/audit-checkouts.sh, which prints ${candidate#"$modules_path"/}, meaning the Git directory's path relative to the worktree's modules/ directory.

What the text says: the paragraph says such a directory is "listed as <name> (not checked out)" and gives the modules location as $(git -C <worktree> rev-parse --path-format=absolute --git-path modules). The resolution row then runs commands against <dir> (git --git-dir <dir> --work-tree <dir> log …) and branches on whether "the submodule still has a Gitlink". It never says that <dir> is <modules>/<name>. It also never says that <name> is the submodule's name, not its path, so finding its Gitlink means looking up submodule.<name>.path. For nested submodules it can be a multi-segment path such as a/modules/b.

Why it matters: the agent must connect three placeholders across two sections to reach the one directory it is supposed to show the owner and possibly delete. Mixing up name and path, or treating <dir> as the empty checkout path, points the inspection or the deletion at the wrong place. The writing standard asks for one term per concept, with project-local terms defined on first use, and asks that a term's definition stay next to its rule.

Proposed correction: in the paragraph, say "listed as <name> (not checked out), where <name> is the directory's path relative to that modules directory (the submodule's name, not its checkout path)". In the table row, write <modules>/<name> in place of <dir>, or say "where <dir> is <modules>/<name>". Nothing else in the row needs to change.

Evidence basis: static reading of the script's printf and the two passages.

claim 01M3C301QE717D7BGXPB4QHAAQ of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3C301QE717D7BGXPB4QHAAQ --> **low** — The `(not checked out)` guidance never ties the reported `<name>` to the `<dir>` its commands act on lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the submodule paragraph in `references/removal-gates.md`, the `judgment/submodule-local-work` resolution row in the same file, and `list_submodule_git_dirs_without_checkout` in `scripts/audit-checkouts.sh`, which prints `${candidate#"$modules_path"/}`, meaning the Git directory's path relative to the worktree's `modules/` directory. > > What the text says: the paragraph says such a directory is "listed as `<name> (not checked out)`" and gives the modules location as `$(git -C <worktree> rev-parse --path-format=absolute --git-path modules)`. The resolution row then runs commands against `<dir>` (`git --git-dir <dir> --work-tree <dir> log …`) and branches on whether "the submodule still has a Gitlink". It never says that `<dir>` is `<modules>/<name>`. It also never says that `<name>` is the submodule's name, not its path, so finding its Gitlink means looking up `submodule.<name>.path`. For nested submodules it can be a multi-segment path such as `a/modules/b`. > > Why it matters: the agent must connect three placeholders across two sections to reach the one directory it is supposed to show the owner and possibly delete. Mixing up name and path, or treating `<dir>` as the empty checkout path, points the inspection or the deletion at the wrong place. The writing standard asks for one term per concept, with project-local terms defined on first use, and asks that a term's definition stay next to its rule. > > Proposed correction: in the paragraph, say "listed as `<name> (not checked out)`, where `<name>` is the directory's path relative to that `modules` directory (the submodule's name, not its checkout path)". In the table row, write `<modules>/<name>` in place of `<dir>`, or say "where `<dir>` is `<modules>/<name>`". Nothing else in the row needs to change. > > Evidence basis: static reading of the script's printf and the two passages. claim `01M3C301QE717D7BGXPB4QHAAQ` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`
Author
Owner

Fixed in f1da8d6. The paragraph defines <name> as the directory's path relative to modules (the submodule's name, not its checkout path), and the row acts on <modules>/<name>.

<!-- gh-feedback:reply-to:88700 --> Fixed in f1da8d6. The paragraph defines `<name>` as the directory's path relative to `modules` (the submodule's name, not its checkout path), and the row acts on `<modules>/<name>`.
jercik marked this conversation as resolved
@ -46,3 +46,3 @@
| `judgment/sparse-checkout` | Keep the cone's `skip-worktree` bits: clearing them turns absent files into apparent deletions. Confirm the cone with `git sparse-checkout list`, verify no file outside it is materialized, then remove manually once every other gate holds. |
| `judgment/precious-ignored-files` | Leave `preciousPaths` in place until the owner disposes of them or authorizes their deletion, then rerun. |
| `judgment/submodule-local-work` | The listed submodules hold work only this worktree has. Show the owner what each holds; rerun once it is pushed or they authorize discarding it. |
| `judgment/submodule-local-work` | The listed submodules hold work only this worktree has. Show the owner what each holds; rerun once the work is pushed. If the owner authorizes discarding it instead, discard it in the submodule first, then rerun. For a `(not checked out)` entry whose submodule still has a Gitlink, as after `git submodule deinit`, run `git submodule update --init` in the worktree so the next run can inspect it. When no Gitlink is left, as after `git rm`, show the owner what the Git directory holds (`git --git-dir <dir> --work-tree <dir> log --oneline --branches --tags --not --remotes` and `… stash list`; `--work-tree` overrides the deleted checkout its config names), delete the directory only on their authorization, then rerun. |

medium — The (not checked out) resolution hides or moves a detached HEAD before the owner sees it, so the prescribed cleanup can lose the work the gate protects
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the judgment/submodule-local-work row of the "Resolve a kept worktree" table in references/removal-gates.md. I read it against the paragraph above it that defines the (not checked out) entry, and against submodule_holds_local_work / list_submodule_git_dirs_without_checkout in scripts/audit-checkouts.sh.

What the text says: a (not checked out) entry is a submodule Git directory the driver "is not inspected". It exists so an agent can review work that removal would otherwise delete. For the git rm case, the row tells the agent to show the owner git --git-dir <dir> --work-tree <dir> log --oneline --branches --tags --not --remotes plus stash list, and to delete the directory once the owner authorizes it. For the git submodule deinit case, it says to "run git submodule update --init in the worktree so the next run can inspect it".

What goes wrong: both paths lose a detached HEAD. With explicit revisions, git log does not add HEAD, so a Git directory whose HEAD is a detached, local-only commit shows nothing unique. The owner then approves deletion with the only unique commit hidden. The driver's own walk treats that HEAD as work: "HEAD must be held by a remote-tracking ref". In the deinit path, submodule update --init checks the recorded commit out in the leftover Git directory before anything inspects it. That detaches HEAD away from a local-only commit, so the next run sees HEAD on the recorded commit and can pass the gate. It also initializes every other submodule in the worktree. So the resolution this row prescribes can discard exactly the work the gate kept the worktree for.

Proposed correction: inspect before re-populating, and include HEAD. For example: "Show the owner what the Git directory holds, for either cause: git --git-dir <dir> --work-tree <dir> log --oneline HEAD --branches --tags --not --remotes and … stash list. Then, on their decision, either restore the checkout (git submodule update --init -- <path> while a Gitlink remains) or delete the directory, and rerun." This keeps both cases and the --work-tree note.

Evidence basis: static reasoning from git's documented behavior (explicit revisions suppress the implicit HEAD; submodule update checks out the recorded commit). I did not run a reproduction. I have not confirmed how submodule update --init treats a pre-existing absorbed Git directory in every Git version.

claim 01M3C2ZN0Y2DAMM8YJHGS8XTPC of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3C2ZN0Y2DAMM8YJHGS8XTPC --> **medium** — The `(not checked out)` resolution hides or moves a detached HEAD before the owner sees it, so the prescribed cleanup can lose the work the gate protects lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `judgment/submodule-local-work` row of the "Resolve a kept worktree" table in `references/removal-gates.md`. I read it against the paragraph above it that defines the `(not checked out)` entry, and against `submodule_holds_local_work` / `list_submodule_git_dirs_without_checkout` in `scripts/audit-checkouts.sh`. > > What the text says: a `(not checked out)` entry is a submodule Git directory the driver "is not inspected". It exists so an agent can review work that removal would otherwise delete. For the `git rm` case, the row tells the agent to show the owner `git --git-dir <dir> --work-tree <dir> log --oneline --branches --tags --not --remotes` plus `stash list`, and to delete the directory once the owner authorizes it. For the `git submodule deinit` case, it says to "run `git submodule update --init` in the worktree so the next run can inspect it". > > What goes wrong: both paths lose a detached HEAD. With explicit revisions, `git log` does not add HEAD, so a Git directory whose HEAD is a detached, local-only commit shows nothing unique. The owner then approves deletion with the only unique commit hidden. The driver's own walk treats that HEAD as work: "HEAD must be held by a remote-tracking ref". In the deinit path, `submodule update --init` checks the recorded commit out in the leftover Git directory before anything inspects it. That detaches HEAD away from a local-only commit, so the next run sees HEAD on the recorded commit and can pass the gate. It also initializes every other submodule in the worktree. So the resolution this row prescribes can discard exactly the work the gate kept the worktree for. > > Proposed correction: inspect before re-populating, and include HEAD. For example: "Show the owner what the Git directory holds, for either cause: `git --git-dir <dir> --work-tree <dir> log --oneline HEAD --branches --tags --not --remotes` and `… stash list`. Then, on their decision, either restore the checkout (`git submodule update --init -- <path>` while a Gitlink remains) or delete the directory, and rerun." This keeps both cases and the `--work-tree` note. > > Evidence basis: static reasoning from git's documented behavior (explicit revisions suppress the implicit HEAD; `submodule update` checks out the recorded commit). I did not run a reproduction. I have not confirmed how `submodule update --init` treats a pre-existing absorbed Git directory in every Git version. claim `01M3C2ZN0Y2DAMM8YJHGS8XTPC` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`
Author
Owner

Fixed in f1da8d6. Confirmed log with explicit revisions omits HEAD. The (not checked out) row now inspects <modules>/<name> first with log --oneline HEAD --branches --tags --not --remotes and stash list, and only then, on the owner's decision, restores that one path with git submodule update --init -- <path> or deletes the directory.

<!-- gh-feedback:reply-to:88698 --> Fixed in f1da8d6. Confirmed `log` with explicit revisions omits HEAD. The `(not checked out)` row now inspects `<modules>/<name>` first with `log --oneline HEAD --branches --tags --not --remotes` and `stash list`, and only then, on the owner's decision, restores that one path with `git submodule update --init -- <path>` or deletes the directory.
jercik marked this conversation as resolved
@ -31,0 +29,4 @@
echo " a fast-forward or selector move while a submodule, at any depth, sits on a commit"
echo " that its superproject does not record and no ref in the submodule holds;"
echo " replacing a local selector tag unless the fetched tag or another ref holds its commit;"
echo " removing a worktree while a submodule holds a commit, stash, or edit that origin lacks,"

medium — --help and the removal comment promise origin holds submodule work, but the gate accepts any remote-tracking ref
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new "Refuses, per repository:" block in usage() in scripts/audit-checkouts.sh, the comment above list_submodules_with_local_work, the gate code in submodule_holds_local_work, and the matching prose in references/removal-gates.md.

What the text says: the help text says removal is refused while a submodule "holds a commit, stash, or edit that origin lacks". The function comment says "removing a linked worktree deletes its submodules' Git directories, so only what origin has survives", then in the next sentence requires only that "HEAD must be held by a remote-tracking ref".

What the code checks: git -C "$submodule_path" rev-list -n 1 --branches --not --remotes, plus the matching HEAD and tag checks. --remotes covers every refs/remotes/*, meaning cached refs from any remote, not origin's live state. Only the tag fallback actually queries origin (ls-remote --tags origin). removal-gates.md describes this accurately ("no remote-tracking ref holds"). The help text and comment use "origin" for two different things.

Why it matters: an agent that reads --help (SKILL.md tells it to run --help before first use) will assume the driver removes a worktree only when origin has every submodule commit. In fact, a submodule commit held only by refs/remotes/upstream/*, or by a stale remote-tracking ref for a branch origin has since deleted, passes the gate, and removal deletes the submodule's Git directory. The writing standard asks for one term per concept and verifiable claims. This wording overstates the safety guarantee.

Proposed correction: help text: "removing a worktree while a submodule holds a stash, an edit, or a commit no remote-tracking ref holds, …". Comment: "…so only commits that remote-tracking refs hold, or tags origin has, survive; a recorded Gitlink…". Both keep the meaning and match removal-gates.md.

Evidence basis: static reading of the rev-list arguments. I did not run a reproduction with a second remote.

claim 01M3C2Z5Z5PX6K1VFMXRFZR2N3 of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3C2Z5Z5PX6K1VFMXRFZR2N3 --> **medium** — `--help` and the removal comment promise origin holds submodule work, but the gate accepts any remote-tracking ref lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new "Refuses, per repository:" block in `usage()` in `scripts/audit-checkouts.sh`, the comment above `list_submodules_with_local_work`, the gate code in `submodule_holds_local_work`, and the matching prose in `references/removal-gates.md`. > > What the text says: the help text says removal is refused while a submodule "holds a commit, stash, or edit that origin lacks". The function comment says "removing a linked worktree deletes its submodules' Git directories, so only what origin has survives", then in the next sentence requires only that "HEAD must be held by a remote-tracking ref". > > What the code checks: `git -C "$submodule_path" rev-list -n 1 --branches --not --remotes`, plus the matching HEAD and tag checks. `--remotes` covers every `refs/remotes/*`, meaning cached refs from any remote, not origin's live state. Only the tag fallback actually queries origin (`ls-remote --tags origin`). `removal-gates.md` describes this accurately ("no remote-tracking ref holds"). The help text and comment use "origin" for two different things. > > Why it matters: an agent that reads `--help` (SKILL.md tells it to run `--help` before first use) will assume the driver removes a worktree only when origin has every submodule commit. In fact, a submodule commit held only by `refs/remotes/upstream/*`, or by a stale remote-tracking ref for a branch origin has since deleted, passes the gate, and removal deletes the submodule's Git directory. The writing standard asks for one term per concept and verifiable claims. This wording overstates the safety guarantee. > > Proposed correction: help text: "removing a worktree while a submodule holds a stash, an edit, or a commit no remote-tracking ref holds, …". Comment: "…so only commits that remote-tracking refs hold, or tags origin has, survive; a recorded Gitlink…". Both keep the meaning and match removal-gates.md. > > Evidence basis: static reading of the rev-list arguments. I did not run a reproduction with a second remote. claim `01M3C2Z5Z5PX6K1VFMXRFZR2N3` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`
Author
Owner

Fixed in f1da8d6. --help and the walker's comment now say a commit no remote-tracking ref holds, or a tag origin lacks, matching the checks.

<!-- gh-feedback:reply-to:88699 --> Fixed in f1da8d6. `--help` and the walker's comment now say a commit no remote-tracking ref holds, or a tag origin lacks, matching the checks.
jercik marked this conversation as resolved
Lines 509-510
@ -508,3 +525,5 @@
continue
fi
# A submodule the walk cannot read may hold anything; fail rather than report it empty.
if ! head=$(git -C "$submodule_path" rev-parse --verify --quiet HEAD); then
printf 'cannot read submodule %s\n' "$prefix$gitlink_path" >&2

low — A failed submodule walk puts Git's fatal: line first, so the report row never names the unreadable submodule, contrary to checkout-updates.md
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: list_submodules_with_local_work in audit-checkouts.sh, the new stranded_error_path capture in the per-worktree update flow (list_submodules_with_local_work "$worktree_path" update 2>"$stranded_error_path"), stepFailure in render-audit-report.ts (it renders firstLine(worktree.strandedSubmodulesError)), and references/checkout-updates.md.

What the docs promise: checkout-updates.md now says the failure row shows "the first line of the error. That line names the submodule when one could not be read and otherwise quotes Git". The new driver test is titled "a failed submodule walk names the submodule it could not read".

What the code does: --quiet on rev-parse --verify only suppresses the "needed a single revision" message. It does not suppress a fatal repository-setup error, and stderr is not redirected. So when a submodule cannot be read, Git's own fatal: line reaches the error file before the script's cannot read submodule <path> line. The renderer keeps only the first line, which drops the submodule name.

Observed reproduction (git 2.47.3): I created a superproject with a submodule dependency and set its core.worktree to a missing directory, which is the same setup the new test uses. Then I ran bash -c 'source audit-checkouts.sh; list_submodules_with_local_work /tmp/r/sup update' 2>err. It exited 1, and err contained, in this order:

fatal: Invalid path '/tmp/r/missing': No such file or directory
cannot read submodule dependency

So the report row is submodule check failed; checkout not updated: fatal: Invalid path '/tmp/r/missing': .... It names the missing worktree path, not the submodule. The linked-worktree removal row (removal check failed (gate-check-failed): …) has the same problem, because the removal walk writes to gate_error_path in the same order.

Why the tests pass anyway: the driver tests use assert.match(result.strandedSubmodulesError, /cannot read submodule dependency/) and assert.match(unreadable.removal.error, /cannot read submodule dependency/), which search the whole text. The renderer test feeds in a made-up single-line error, "cannot read submodule dep\n". None of them check the first line that actually gets rendered.

Fix: send this rev-parse call's stderr to /dev/null, or print the cannot read submodule line before Git's output, so the first line names the submodule as documented. Also make the tests assert on the first line. I could not run the full driver tests here because jq is not installed; the ordering above comes from calling the function directly.

claim 01M3C30G287WV92PRMJP0EZ9FR of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3C30G287WV92PRMJP0EZ9FR --> **low** — A failed submodule walk puts Git's `fatal:` line first, so the report row never names the unreadable submodule, contrary to checkout-updates.md lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: `list_submodules_with_local_work` in audit-checkouts.sh, the new `stranded_error_path` capture in the per-worktree update flow (`list_submodules_with_local_work "$worktree_path" update 2>"$stranded_error_path"`), `stepFailure` in render-audit-report.ts (it renders `firstLine(worktree.strandedSubmodulesError)`), and references/checkout-updates.md. > > What the docs promise: checkout-updates.md now says the failure row shows "the first line of the error. That line names the submodule when one could not be read and otherwise quotes Git". The new driver test is titled "a failed submodule walk names the submodule it could not read". > > What the code does: `--quiet` on `rev-parse --verify` only suppresses the "needed a single revision" message. It does not suppress a fatal repository-setup error, and stderr is not redirected. So when a submodule cannot be read, Git's own `fatal:` line reaches the error file before the script's `cannot read submodule <path>` line. The renderer keeps only the first line, which drops the submodule name. > > Observed reproduction (git 2.47.3): I created a superproject with a submodule `dependency` and set its `core.worktree` to a missing directory, which is the same setup the new test uses. Then I ran `bash -c 'source audit-checkouts.sh; list_submodules_with_local_work /tmp/r/sup update' 2>err`. It exited 1, and `err` contained, in this order: > ``` > fatal: Invalid path '/tmp/r/missing': No such file or directory > cannot read submodule dependency > ``` > So the report row is `submodule check failed; checkout not updated: fatal: Invalid path '/tmp/r/missing': ...`. It names the missing worktree path, not the submodule. The linked-worktree removal row (`removal check failed (gate-check-failed): …`) has the same problem, because the removal walk writes to `gate_error_path` in the same order. > > Why the tests pass anyway: the driver tests use `assert.match(result.strandedSubmodulesError, /cannot read submodule dependency/)` and `assert.match(unreadable.removal.error, /cannot read submodule dependency/)`, which search the whole text. The renderer test feeds in a made-up single-line error, `"cannot read submodule dep\n"`. None of them check the first line that actually gets rendered. > > Fix: send this `rev-parse` call's stderr to /dev/null, or print the `cannot read submodule` line before Git's output, so the first line names the submodule as documented. Also make the tests assert on the first line. I could not run the full driver tests here because `jq` is not installed; the ordering above comes from calling the function directly. claim `01M3C30G287WV92PRMJP0EZ9FR` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`
Author
Owner

Fixed in f1da8d6. Reproduced: Git's fatal: line came first. The walk now prints cannot read submodule <path>: <Git's message> (and cannot inspect submodule <path>: …) as one line, and the driver tests assert on the first line.

<!-- gh-feedback:reply-to:88701 --> Fixed in f1da8d6. Reproduced: Git's `fatal:` line came first. The walk now prints `cannot read submodule <path>: <Git's message>` (and `cannot inspect submodule <path>: …`) as one line, and the driver tests assert on the first line.
jercik marked this conversation as resolved
Lines 1480-1483
@ -1424,0 +1483,7 @@
git(dependencyPath, "tag", "--no-sign", "--force", "-a", "v0.9", "-m", "local v0.9", "v0.9^{commit}");
assert.equal(remove().removal.outcome, "judgment/submodule-local-work");
git(dependencyPath, "update-ref", "refs/tags/v0.9", upstreamTag);
const blob = execFileSync("git", ["-C", dependencyPath, "hash-object", "-w", "--stdin"], { input: "private note\n", encoding: "utf8" }).trim();
git(dependencyPath, "tag", "--no-sign", "note", blob);
assert.equal(remove().removal.outcome, "judgment/submodule-local-work");

low — Blob-tag step in the submodule tag test cannot catch loss of the non-commit-tag guard, because v0.9 already forces the origin comparison
lens test-trimming · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new test "a submodule tag counts as local work only when origin lacks it or names another object" in audit-checkouts.test.mjs, its helper tagUpstreamReleaseOffBranch, and submodule_holds_local_work in audit-checkouts.sh.

What the implementation does: this new guard decides whether the tag scan can stop early:

rev-list skips tags on blobs and trees, so those always reach the origin comparison.

local_tags=$(git ... for-each-ref --format='...%(objecttype)...' refs/tags) || return 2
output=$(git -C "$submodule_path" rev-list -n 1 --tags --not --remotes) || return 2
if [ -z "$output" ] && ! awk -F '\t' 'NF && $4 != "commit" {found = 1} END {exit !found}' <<<"$local_tags"; then
return 1
fi

The awk clause is there so a tag on a blob or tree, which rev-list ignores, does not make the function return early with "no work".

What the test does: the blob step (anchored) creates note on a blob while v0.9 is still present. tagUpstreamReleaseOffBranch puts v0.9 on a release commit, then deletes that branch upstream, so no remote-tracking ref holds the commit. As a result rev-list --tags --not --remotes already returns output because of v0.9. The early exit is skipped whether or not the awk clause exists. The loop then compares against origin and flags note by itself. So this assertion only covers the loop, which the earlier annotated-v0.9 step already reaches. It never covers the guard its comment describes.

Executed check: I rebuilt the fixture state with plain git: an upstream repo, a clone, an annotated v0.9 on a deleted release branch fetched with --tags, and a lightweight note tag on a blob. I then sourced the script and called submodule_holds_local_work <dep> <HEAD> "" removal. For the mutation, I replaced the guard with if [ -z "$output" ]; then (awk clause removed).

  • State from the test (v0.9 + note): original rc=0, mutant rc=0. The assertion stays green.
  • With v0.9 deleted (note only): original rc=0 (work), mutant rc=1 (no work, so the worktree would be removed and the blob lost).
    I could not run the full test file because jq is not installed in this sandbox. The result above comes from calling the function directly on an equivalent repo.

What goes wrong: someone could remove or break the non-commit-tag guard, a data-loss protection for a forced worktree removal, and this test would still pass. No other changed test covers a blob tag in removal mode. The other blob tag, around line 1321, is in the tracking-update test.

Correction: keep the step, but make it the only tag that can reach the origin comparison. Either delete v0.9 before creating note (git tag -d v0.9, then tag the blob and assert judgment/submodule-local-work), or move the blob case into its own fixture without tagUpstreamReleaseOffBranch. Then the final tag -d note → "removed" step keeps its meaning.

claim 01M3C2YN0C0SA5RJ9VDKEMWXRX of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3C2YN0C0SA5RJ9VDKEMWXRX --> **low** — Blob-tag step in the submodule tag test cannot catch loss of the non-commit-tag guard, because v0.9 already forces the origin comparison lens `test-trimming` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new test "a submodule tag counts as local work only when origin lacks it or names another object" in audit-checkouts.test.mjs, its helper tagUpstreamReleaseOffBranch, and submodule_holds_local_work in audit-checkouts.sh. > > What the implementation does: this new guard decides whether the tag scan can stop early: > > # rev-list skips tags on blobs and trees, so those always reach the origin comparison. > local_tags=$(git ... for-each-ref --format='...%(objecttype)...' refs/tags) || return 2 > output=$(git -C "$submodule_path" rev-list -n 1 --tags --not --remotes) || return 2 > if [ -z "$output" ] && ! awk -F '\t' 'NF && $4 != "commit" {found = 1} END {exit !found}' <<<"$local_tags"; then > return 1 > fi > > The awk clause is there so a tag on a blob or tree, which rev-list ignores, does not make the function return early with "no work". > > What the test does: the blob step (anchored) creates `note` on a blob while v0.9 is still present. tagUpstreamReleaseOffBranch puts v0.9 on a `release` commit, then deletes that branch upstream, so no remote-tracking ref holds the commit. As a result `rev-list --tags --not --remotes` already returns output because of v0.9. The early exit is skipped whether or not the awk clause exists. The loop then compares against origin and flags `note` by itself. So this assertion only covers the loop, which the earlier annotated-v0.9 step already reaches. It never covers the guard its comment describes. > > Executed check: I rebuilt the fixture state with plain git: an upstream repo, a clone, an annotated v0.9 on a deleted `release` branch fetched with `--tags`, and a lightweight `note` tag on a blob. I then sourced the script and called `submodule_holds_local_work <dep> <HEAD> "" removal`. For the mutation, I replaced the guard with `if [ -z "$output" ]; then` (awk clause removed). > - State from the test (v0.9 + note): original rc=0, mutant rc=0. The assertion stays green. > - With v0.9 deleted (note only): original rc=0 (work), mutant rc=1 (no work, so the worktree would be removed and the blob lost). > I could not run the full test file because jq is not installed in this sandbox. The result above comes from calling the function directly on an equivalent repo. > > What goes wrong: someone could remove or break the non-commit-tag guard, a data-loss protection for a forced worktree removal, and this test would still pass. No other changed test covers a blob tag in removal mode. The other blob tag, around line 1321, is in the tracking-update test. > > Correction: keep the step, but make it the only tag that can reach the origin comparison. Either delete v0.9 before creating `note` (`git tag -d v0.9`, then tag the blob and assert judgment/submodule-local-work), or move the blob case into its own fixture without tagUpstreamReleaseOffBranch. Then the final `tag -d note` → "removed" step keeps its meaning. claim `01M3C2YN0C0SA5RJ9VDKEMWXRX` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`
Author
Owner

Fixed in f1da8d6. The blob-tag step now runs with v0.9 deleted, so only the blob tag can reach the origin comparison; removing the non-commit guard now fails the test.

<!-- gh-feedback:reply-to:88702 --> Fixed in f1da8d6. The blob-tag step now runs with `v0.9` deleted, so only the blob tag can reach the origin comparison; removing the non-commit guard now fails the test.
jercik marked this conversation as resolved
@ -201,3 +207,3 @@
if (comparison.ahead > 0) return `${count(comparison.ahead, "local commit")} not pushed${dirtyNote}`;
if (dirty > 0 && comparison.behind > 0) {
return `behind; uncommitted changes (${count(dirty, "file")}) block the fast-forward. A dirty checkout fast-forwards only when every changed path is AGENTS.md or under .agents/, at any depth, or first-party submodule metadata, and upstream changed none of those paths.`;
return `behind; uncommitted changes (${count(dirty, "file")}) block the fast-forward (see references/checkout-updates.md)`;

low — User-facing report cell points to references/checkout-updates.md, a skill-internal path that the report's reader cannot resolve
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: defaultNotCurrentReason in scripts/render-audit-report.ts, the render-audit-report.test.ts assertion that pins this string, and the SKILL.md sections "Run the driver" and "Report, then work the decisions".

What the text says: the "Needs your decision" cell for a behind, dirty default checkout now reads "behind; uncommitted changes (4 files) block the fast-forward (see references/checkout-updates.md)". Before this change, the cell said which dirty paths still allow a fast-forward.

What goes wrong: report.md is written to a mktemp -d directory and handed to the user ("Reply with … the path to report.md"). SKILL.md also tells the agent to "Use the report's plain wording" because internal codes "mean nothing to the user". references/checkout-updates.md is a path relative to the installed skill directory. From the report's location it resolves to nothing, and a user reading the report has no skill directory to open. For the agent the pointer adds nothing: SKILL.md "Acting on a decision" already sends "A default checkout that was not fast-forwarded" to that reference. So the cell lost the one fact that helps the reader (what would make the checkout eligible) and gained an internal link. That goes against the standard's "one idea, one place" and against writing user-facing text in the reader's terms.

Proposed correction: drop the pointer and either stop at "block the fast-forward", or keep a short, plain reason, such as "…block the fast-forward; only AGENTS.md, .agents/, and first-party submodule selector changes are allowed". Update the test assertion to match.

Evidence basis: static reading. The report destination comes from the SKILL.md run block.

claim 01M3C30H25YYX0RHFA1116NG7R of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3C30H25YYX0RHFA1116NG7R --> **low** — User-facing report cell points to `references/checkout-updates.md`, a skill-internal path that the report's reader cannot resolve lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: `defaultNotCurrentReason` in `scripts/render-audit-report.ts`, the `render-audit-report.test.ts` assertion that pins this string, and the SKILL.md sections "Run the driver" and "Report, then work the decisions". > > What the text says: the "Needs your decision" cell for a behind, dirty default checkout now reads "behind; uncommitted changes (4 files) block the fast-forward (see references/checkout-updates.md)". Before this change, the cell said which dirty paths still allow a fast-forward. > > What goes wrong: `report.md` is written to a `mktemp -d` directory and handed to the user ("Reply with … the path to `report.md`"). SKILL.md also tells the agent to "Use the report's plain wording" because internal codes "mean nothing to the user". `references/checkout-updates.md` is a path relative to the installed skill directory. From the report's location it resolves to nothing, and a user reading the report has no skill directory to open. For the agent the pointer adds nothing: SKILL.md "Acting on a decision" already sends "A default checkout that was not fast-forwarded" to that reference. So the cell lost the one fact that helps the reader (what would make the checkout eligible) and gained an internal link. That goes against the standard's "one idea, one place" and against writing user-facing text in the reader's terms. > > Proposed correction: drop the pointer and either stop at "block the fast-forward", or keep a short, plain reason, such as "…block the fast-forward; only AGENTS.md, .agents/, and first-party submodule selector changes are allowed". Update the test assertion to match. > > Evidence basis: static reading. The report destination comes from the SKILL.md run block. claim `01M3C30H25YYX0RHFA1116NG7R` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`
Author
Owner

Fixed in f1da8d6. The cell ends at "block the fast-forward"; SKILL.md already routes that decision to the reference.

<!-- gh-feedback:reply-to:88703 --> Fixed in f1da8d6. The cell ends at "block the fast-forward"; SKILL.md already routes that decision to the reference.
jercik marked this conversation as resolved
@ -335,2 +341,2 @@
if ((isMain || onDefault) && stranded.length > 0) {
const why = `submodule on a commit no ref holds: ${listPaths(stranded)}; not updated`;
if (onDefault && stranded.length > 0) {
const why = `submodules on a commit no ref holds: ${listPaths(stranded)}; not updated`;

low — "Needs your decision" row always says "submodules", even when it lists one path
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the "Needs your decision" row built in the main render loop of scripts/render-audit-report.ts, the count(n, singular, plural) helper in the same file, and the test in render-audit-report.test.ts that asserts | app | main | +0/-0 | submodules on a commit no ref holds: mid/inner; not updated |.

What the text says: this change swapped the singular "submodule on a commit no ref holds" for a fixed plural. The test fixture shows the result: a single path (mid/inner) gets "submodules on a commit". "on a commit" also suggests that all the listed submodules share one commit, when each sits on its own commit.

Why it matters: the cell goes straight to the user (SKILL.md: "Use the report's plain wording"). A reader seeing "submodules" next to one path will wonder whether the list was cut short. listPaths already truncates with "(+N more)", so the plural looks like a hint that there are more. The file already has count() for exactly this kind of agreement.

Proposed correction: `${stranded.length === 1 ? "submodule" : "submodules"} on a commit no ref holds: …`. Or use a form that agrees with any count, e.g. `submodule work no ref holds: ${listPaths(stranded)}; not updated`. Update the test string to match.

Evidence basis: the test fixture's own expected output.

claim 01M3C30HGPK6WN4VMZW092SAYE of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3C30HGPK6WN4VMZW092SAYE --> **low** — "Needs your decision" row always says "submodules", even when it lists one path lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the "Needs your decision" row built in the main render loop of `scripts/render-audit-report.ts`, the `count(n, singular, plural)` helper in the same file, and the test in `render-audit-report.test.ts` that asserts `| app | main | +0/-0 | submodules on a commit no ref holds: mid/inner; not updated |`. > > What the text says: this change swapped the singular "submodule on a commit no ref holds" for a fixed plural. The test fixture shows the result: a single path (`mid/inner`) gets "submodules on a commit". "on a commit" also suggests that all the listed submodules share one commit, when each sits on its own commit. > > Why it matters: the cell goes straight to the user (SKILL.md: "Use the report's plain wording"). A reader seeing "submodules" next to one path will wonder whether the list was cut short. `listPaths` already truncates with "(+N more)", so the plural looks like a hint that there are more. The file already has `count()` for exactly this kind of agreement. > > Proposed correction: `` `${stranded.length === 1 ? "submodule" : "submodules"} on a commit no ref holds: …` ``. Or use a form that agrees with any count, e.g. `` `submodule work no ref holds: ${listPaths(stranded)}; not updated` ``. Update the test string to match. > > Evidence basis: the test fixture's own expected output. claim `01M3C30HGPK6WN4VMZW092SAYE` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`
Author
Owner

Fixed in f1da8d6. One path reads "submodule on a commit no ref holds", several read "submodules on commits no ref holds"; the render test covers both.

<!-- gh-feedback:reply-to:88704 --> Fixed in f1da8d6. One path reads "submodule on a commit no ref holds", several read "submodules on commits no ref holds"; the render test covers both.
jercik marked this conversation as resolved
@ -46,3 +46,3 @@
| `judgment/sparse-checkout` | Keep the cone's `skip-worktree` bits: clearing them turns absent files into apparent deletions. Confirm the cone with `git sparse-checkout list`, verify no file outside it is materialized, then remove manually once every other gate holds. |
| `judgment/precious-ignored-files` | Leave `preciousPaths` in place until the owner disposes of them or authorizes their deletion, then rerun. |
| `judgment/submodule-local-work` | The listed submodules hold work only this worktree has. Show the owner what each holds; rerun once it is pushed or they authorize discarding it. |
| `judgment/submodule-local-work` | The listed submodules hold work only this worktree has. Show the owner what each holds; rerun once the work is pushed. If the owner authorizes discarding it instead, discard it in the submodule first, then rerun. For a `(not checked out)` entry whose submodule still has a Gitlink, as after `git submodule deinit`, run `git submodule update --init` in the worktree so the next run can inspect it. When no Gitlink is left, as after `git rm`, show the owner what the Git directory holds (`git --git-dir <dir> --work-tree <dir> log --oneline --branches --tags --not --remotes` and `… stash list`; `--work-tree` overrides the deleted checkout its config names), delete the directory only on their authorization, then rerun. |

medium — Removal guidance asserts unique submodule work where the driver has not inspected it
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

I read the removal-gates reference, list_submodule_git_dirs_without_checkout, the removal gate, and the report renderer. The new gate emits judgment/submodule-local-work for any submodule Git directory without a checkout, even when its refs contain no unique work. The same reference explicitly says that such a directory is not inspected. Yet this resolution row says the listed submodules hold work only this worktree has; the report renderer likewise describes the outcome as work that exists only here. That turns an uncertainty into a fact for the owner and can send them to push or discard work that has not been established. Say the listed paths may contain work that removal would lose, then give the inspect-and-decide steps already present. This preserves the reason for blocking removal while stating the actual evidence. A clean deinitialized submodule that still leaves its Git directory would establish the false-positive wording in a report; the unconditional directory check establishes the possibility statically.

claim 01M3CJ43ZDW3V7V9B1W24Y8JES of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3CJ43ZDW3V7V9B1W24Y8JES --> **medium** — Removal guidance asserts unique submodule work where the driver has not inspected it lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > I read the removal-gates reference, list_submodule_git_dirs_without_checkout, the removal gate, and the report renderer. The new gate emits judgment/submodule-local-work for any submodule Git directory without a checkout, even when its refs contain no unique work. The same reference explicitly says that such a directory is not inspected. Yet this resolution row says the listed submodules hold work only this worktree has; the report renderer likewise describes the outcome as work that exists only here. That turns an uncertainty into a fact for the owner and can send them to push or discard work that has not been established. Say the listed paths may contain work that removal would lose, then give the inspect-and-decide steps already present. This preserves the reason for blocking removal while stating the actual evidence. A clean deinitialized submodule that still leaves its Git directory would establish the false-positive wording in a report; the unconditional directory check establishes the possibility statically. claim `01M3CJ43ZDW3V7V9B1W24Y8JES` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`

medium — Reinitialization instruction updates every submodule in the worktree
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

I read the new resolution for a (not checked out) submodule directory and the checkout-updates guidance. When a Gitlink remains after deinit, the resolution tells the agent to run git submodule update --init with no path. Git treats that as an update of all registered submodules in the worktree, so resolving one blocked path can check out unrelated submodules at recorded commits. This conflicts with the skill guidance that submodules may need to stay where they are, and can move a separate local checkout before its work has been examined. Name the reported Gitlink in the command, using git submodule update --init -- , and inspect its state before rerunning the audit. This keeps the intended ability to inspect the blocked submodule while limiting the action to it. The effect follows from the pathless Git command; I did not run it against a multi-submodule fixture.

claim 01M3CJ9FW2QGN1ES7CNTGMRJW7 of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3CJ9FW2QGN1ES7CNTGMRJW7 --> **medium** — Reinitialization instruction updates every submodule in the worktree lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > I read the new resolution for a (not checked out) submodule directory and the checkout-updates guidance. When a Gitlink remains after deinit, the resolution tells the agent to run git submodule update --init with no path. Git treats that as an update of all registered submodules in the worktree, so resolving one blocked path can check out unrelated submodules at recorded commits. This conflicts with the skill guidance that submodules may need to stay where they are, and can move a separate local checkout before its work has been examined. Name the reported Gitlink in the command, using git submodule update --init -- <path>, and inspect its state before rerunning the audit. This keeps the intended ability to inspect the blocked submodule while limiting the action to it. The effect follows from the pathless Git command; I did not run it against a multi-submodule fixture. claim `01M3CJ9FW2QGN1ES7CNTGMRJW7` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`
Author
Owner

Fixed in f1da8d6. The report reads "submodules may hold work that exists only here", and the reference row says the listed submodules hold such work or content the driver could not inspect.

<!-- gh-feedback:reply-to:89290 --> Fixed in f1da8d6. The report reads "submodules may hold work that exists only here", and the reference row says the listed submodules hold such work or content the driver could not inspect.
Author
Owner

Fixed in f1da8d6. The row now restores one path, git submodule update --init -- <path>, and only after inspecting it.

<!-- gh-feedback:reply-to:89291 --> Fixed in f1da8d6. The row now restores one path, `git submodule update --init -- <path>`, and only after inspecting it.
jercik marked this conversation as resolved
@ -31,1 +31,4 @@
echo " replacing a local selector tag unless the fetched tag or another ref holds its commit;"
echo " removing a worktree while a submodule holds a commit, stash, or edit that origin lacks,"
echo " or while files or a submodule Git directory in it have no checkout to inspect."
echo "Branch refs and stashes are never deleted."

medium — Help promises branch refs survive although submodule refs can be removed
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

I read usage, the removal gate, and the removal-gates reference. Usage tells the operator that branch refs and stashes are never deleted. The same driver can force-remove a linked worktree with an initialized submodule after its branch commits are found on a remote-tracking ref; the reference states that removal deletes the submodule Git directory, including its local branches. A local submodule branch at a remotely held commit therefore can disappear even though the help promises all branch refs survive. Qualify the promise as applying to superproject refs, and say that submodule local refs can be deleted with a removed worktree. This preserves the useful promise for the audited repository while making the deletion scope clear. I established the mismatch from the code and reference; I did not execute a removal scenario.

claim 01M3CJ67MC4VNGTTDF0E34WPDE of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3CJ67MC4VNGTTDF0E34WPDE --> **medium** — Help promises branch refs survive although submodule refs can be removed lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > I read usage, the removal gate, and the removal-gates reference. Usage tells the operator that branch refs and stashes are never deleted. The same driver can force-remove a linked worktree with an initialized submodule after its branch commits are found on a remote-tracking ref; the reference states that removal deletes the submodule Git directory, including its local branches. A local submodule branch at a remotely held commit therefore can disappear even though the help promises all branch refs survive. Qualify the promise as applying to superproject refs, and say that submodule local refs can be deleted with a removed worktree. This preserves the useful promise for the audited repository while making the deletion scope clear. I established the mismatch from the code and reference; I did not execute a removal scenario. claim `01M3CJ67MC4VNGTTDF0E34WPDE` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`
Author
Owner

Fixed in f1da8d6. --help now limits the promise to the audited repositories' branch refs and stashes, and says a removed worktree takes its submodules' Git directories, local branches included, once every commit there is held by a remote-tracking ref.

<!-- gh-feedback:reply-to:89292 --> Fixed in f1da8d6. `--help` now limits the promise to the audited repositories' branch refs and stashes, and says a removed worktree takes its submodules' Git directories, local branches included, once every commit there is held by a remote-tracking ref.
jercik marked this conversation as resolved
Lines 1174-1177
@ -1099,5 +1193,6 @@
# is-it-clean safeguard inside worktree remove. Never -f -f: that also
# bypasses Git's unclean refusal.
# Git refuses a populated Gitlink or a modules/ directory, not .gitmodules.
worktree_remove_args=(worktree remove)
if [ -e "$worktree_path/.gitmodules" ]; then
if [ -n "$populated_git_dirs" ] || [ -d "$modules_path" ]; then
worktree_remove_args+=(--force)

high — Forced removal deletes ignored files inside a Gitlink checkout
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

I examined decide_removal_outcome, list_submodules_with_local_work, inspect-ignored-content.sh, and references/removal-gates.md. The new force condition makes a populated Gitlink eligible for deletion even when the worktree has no .gitmodules. The live submodule check runs git status without --ignored, and the top-level ignored scan uses git status in the superproject, which does not enumerate ignored content inside a Gitlink. I reproduced a linked worktree containing a plain cloned Gitlink with no .gitmodules and a locally written .env ignored by that clone: top-level status, top-level ignored listing, submodule status, and list_submodules_with_local_work removal were all empty; git worktree remove --force then deleted the .env file. This is a normal default-removal path and silently loses local data. Scan each populated submodule for ignored content and keep the worktree when it is not known regenerable or duplicated before applying --force. The fixture directly confirmed the deletion; the full driver could not be run because jq is unavailable here.

claim 01M3CJAVKF798P7SV0A4EHP3BA of review 01M3C2TJ98PWK7BAJMKTCHQMQ1

<!-- review:claim:01M3CJAVKF798P7SV0A4EHP3BA --> **high** — Forced removal deletes ignored files inside a Gitlink checkout lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > I examined decide_removal_outcome, list_submodules_with_local_work, inspect-ignored-content.sh, and references/removal-gates.md. The new force condition makes a populated Gitlink eligible for deletion even when the worktree has no .gitmodules. The live submodule check runs git status without --ignored, and the top-level ignored scan uses git status in the superproject, which does not enumerate ignored content inside a Gitlink. I reproduced a linked worktree containing a plain cloned Gitlink with no .gitmodules and a locally written .env ignored by that clone: top-level status, top-level ignored listing, submodule status, and list_submodules_with_local_work removal were all empty; git worktree remove --force then deleted the .env file. This is a normal default-removal path and silently loses local data. Scan each populated submodule for ignored content and keep the worktree when it is not known regenerable or duplicated before applying --force. The fixture directly confirmed the deletion; the full driver could not be run because jq is unavailable here. claim `01M3CJAVKF798P7SV0A4EHP3BA` of review `01M3C2TJ98PWK7BAJMKTCHQMQ1`
Author
Owner

Fixed in f1da8d6. Reproduced with a plain-clone Gitlink holding an ignored .env. The removal gate now scans ignored files in each populated submodule, at any depth, with the same rules as the worktree's own scan, against the primary checkout's copy of that submodule; a precious one keeps the worktree as judgment/precious-ignored-files (for example dependency/.env). New test: "ignored files inside a submodule are scanned like the worktree's own".

<!-- gh-feedback:reply-to:89289 --> Fixed in f1da8d6. Reproduced with a plain-clone Gitlink holding an ignored `.env`. The removal gate now scans ignored files in each populated submodule, at any depth, with the same rules as the worktree's own scan, against the primary checkout's copy of that submodule; a precious one keeps the worktree as `judgment/precious-ignored-files` (for example `dependency/.env`). New test: "ignored files inside a submodule are scanned like the worktree's own".
jercik marked this conversation as resolved
fix(audit-git-checkouts): forced removal should not delete ignored files inside a submodule
All checks were successful
commit-msg / commitlint (pull_request) Successful in 24s
Node tests / node:test (pull_request) Successful in 1m44s
Review / Review (pull_request_target) Successful in 6m9s
f1da8d6848
- Scan ignored files in each populated submodule against the primary
  checkout's copy of it, and keep the worktree when any is precious.
- Put the submodule's name ahead of Git's message in a failed walk's
  error, so the report's first line names it.
- Inspect a leftover submodule Git directory, HEAD included, before
  restoring it, and restore only that path.
- Word the removal rule, help text, and report cells for what the gate
  actually checks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ -5,3 +5,3 @@
## What the driver already updates
The driver fast-forwards a default-branch checkout only when its comparison is fresh, it has no local commits, and it is strictly behind `origin/<default>`. A dirty checkout qualifies only when every changed path is deferred guidance (`AGENTS.md`, `.agents/**`, at any depth), a first-party `.gitmodules` edit, or a first-party Gitlink change on a submodule with a `branch` or `tag` selector, and upstream did not touch those paths. The merge runs `--ff-only` with `merge.autostash=false`, because autostash would round-trip the tree through a stash and silently unstage staged guidance. After a fast-forward it initializes committed submodules and checks them out at the recorded Gitlinks. When any populated submodule, at any depth, found from the Gitlinks rather than `.gitmodules`, is checked out at a commit that its superproject does not record and no ref holds, the driver skips the fast-forward and every selector move for that checkout, and the report lists it under "Needs your decision" with the submodule's path.
The driver fast-forwards a default-branch checkout only when its comparison is fresh, it has no local commits, and it is strictly behind `origin/<default>`. A dirty checkout qualifies only when every changed path is deferred guidance (`AGENTS.md`, `.agents/**`, at any depth), a first-party `.gitmodules` edit, or a first-party Gitlink change on a submodule with a `branch` or `tag` selector, and upstream did not touch those paths. The merge runs `--ff-only` with `merge.autostash=false`, because autostash would round-trip the tree through a stash and silently unstage staged guidance. After a fast-forward it initializes committed submodules and checks them out at the recorded Gitlinks. The driver skips the fast-forward and every selector move for a checkout holding a populated submodule, at any depth, that is checked out at a commit its superproject does not record and no ref in the submodule holds; the report lists the checkout under "Needs your decision" with the submodule's path. Submodules are found from the Gitlinks in HEAD and the index, so neither a removed `.gitmodules` nor an `ignore` setting hides one. When that submodule check itself fails, the driver changes nothing in the checkout, and its failure row says the submodule check failed, followed by the first line of the error. That line names the submodule when one could not be read and otherwise quotes Git; `strandedSubmodulesError` in the JSON record holds the full text. Repair what it names, then rerun.

low — checkout-updates.md narrates the wording of the submodule-check failure row that the agent already sees in the report
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new sentences at the end of "What the driver already updates" in references/checkout-updates.md, the stepFailure function in render-audit-report.ts that produces the row (submodule check failed; checkout not updated: <first line>), and the error text in list_submodules_with_local_work / list_gitlink_paths in audit-checkouts.sh.

What the passage does: it says what the failure row reads ("the submodule check failed, followed by the first line of the error") and what that line contains ("names the submodule when one could not be read and otherwise quotes Git"). The agent reaches this reference after seeing that exact row in report.md, so these two clauses restate text already in front of it. The second clause is also incomplete: the driver emits both cannot read submodule <path>: … and cannot inspect submodule <path>: …, and both name the submodule; only the superproject index read (list_gitlink_paths) yields bare Git text.

What carries weight: the driver changed nothing in the checkout; strandedSubmodulesError holds the full text; repair and rerun.

Skill guidance: the no-op test ("would the models this content serves already behave correctly without this line? Delete it if yes") and "Prefer a cheap authoritative lookup to copying a fact." This reference loads for every not-fast-forwarded checkout and selector problem, so extra words cost on each read.

Proposed correction: "When that submodule check itself fails, the driver changes nothing in the checkout; the failure row shows the error's first line, and strandedSubmodulesError in the JSON record holds the full text. Repair what it names, then rerun." This removes the restated row wording and the partly inaccurate description of the line, and keeps the no-change guarantee, the JSON field, and the remedy.

claim 01M3CK2F90945GX05Z9MBEAA3F of review 01M3CJXYC691YPN10BHMK7PR00

<!-- review:claim:01M3CK2F90945GX05Z9MBEAA3F --> **low** — checkout-updates.md narrates the wording of the submodule-check failure row that the agent already sees in the report lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new sentences at the end of "What the driver already updates" in `references/checkout-updates.md`, the `stepFailure` function in `render-audit-report.ts` that produces the row (`submodule check failed; checkout not updated: <first line>`), and the error text in `list_submodules_with_local_work` / `list_gitlink_paths` in `audit-checkouts.sh`. > > What the passage does: it says what the failure row reads ("the submodule check failed, followed by the first line of the error") and what that line contains ("names the submodule when one could not be read and otherwise quotes Git"). The agent reaches this reference after seeing that exact row in `report.md`, so these two clauses restate text already in front of it. The second clause is also incomplete: the driver emits both `cannot read submodule <path>: …` and `cannot inspect submodule <path>: …`, and both name the submodule; only the superproject index read (`list_gitlink_paths`) yields bare Git text. > > What carries weight: the driver changed nothing in the checkout; `strandedSubmodulesError` holds the full text; repair and rerun. > > Skill guidance: the no-op test ("would the models this content serves already behave correctly without this line? Delete it if yes") and "Prefer a cheap authoritative lookup to copying a fact." This reference loads for every not-fast-forwarded checkout and selector problem, so extra words cost on each read. > > Proposed correction: "When that submodule check itself fails, the driver changes nothing in the checkout; the failure row shows the error's first line, and `strandedSubmodulesError` in the JSON record holds the full text. Repair what it names, then rerun." This removes the restated row wording and the partly inaccurate description of the line, and keeps the no-change guarantee, the JSON field, and the remedy. claim `01M3CK2F90945GX05Z9MBEAA3F` of review `01M3CJXYC691YPN10BHMK7PR00`
Author
Owner

Fixed in 88273cd with the proposed wording.

<!-- gh-feedback:reply-to:89316 --> Fixed in 88273cd with the proposed wording.
jercik marked this conversation as resolved
@ -19,3 +19,3 @@
A worktree lock is an owner pin that expires seven days after its `locked` file's mtime. `git worktree lock` refuses an already locked worktree, so the mtime dates from the original lock; to renew a pin, unlock and lock again. A future-dated lock counts as young. The driver unlocks an expired lock only after every other gate passes, immediately before removal. It reads the lock once, at the gate: a pin renewed during the containment proof and ignored scan that follow is unlocked anyway. If unlock fails, the outcome is `operational/removal-failed`. If removal then fails, the worktree stays unlocked and the next run evaluates it from scratch. Expiry deliberately overrides pins agents leave behind.
Worktrees containing `.gitmodules` are removed with `--force`, because Git otherwise refuses any worktree with submodules; the repeated status check replaces the check `--force` disables. Removal also deletes each submodule's Git directory, local branches and stash included, so right before the status check the driver walks every populated submodule at any depth. A submodule keeps the worktree (`judgment/submodule-local-work`, paths in `removal.error`) when it has uncommitted files, a stash, a branch or tag commit no remote-tracking ref holds, or a HEAD no remote-tracking ref holds. A Gitlink names a commit without keeping it, so a recorded HEAD needs a remote-tracking ref too. Ignored files inside submodules are not scanned.
Git refuses to remove a worktree with a populated Gitlink or a submodule Git directory under the worktree's own `modules/`, with or without `.gitmodules`, so the driver removes such a worktree with `--force`; the repeated status check replaces the check `--force` disables. Removal also deletes each submodule's Git directory, local branches and stash included, so immediately before the second status check the driver walks every populated submodule at any depth. A submodule keeps the worktree (`judgment/submodule-local-work`, paths in `removal.error`) when it has uncommitted files, a stash, a linked worktree of its own, a HEAD or branch commit no remote-tracking ref holds, or a tag that names no commit or whose commit no remote-tracking ref holds, unless origin has the same tag on the same object (a lightweight tag also matches origin's tag on its commit). A Gitlink names a commit without holding it, so a recorded HEAD needs a remote-tracking ref too. Files under a Gitlink path with no checkout keep the worktree as `<path> (files without a checkout)`, because status never lists them. A submodule Git directory with no checkout, such as one `git submodule deinit` or `git rm` leaves under `$(git -C <worktree> rev-parse --path-format=absolute --git-path modules)`, is not inspected, so it keeps the worktree too, listed as `<name> (not checked out)`: `<name>` is the directory's path relative to that `modules` directory, which is the submodule's name, not its checkout path. Ignored files inside each populated submodule are scanned like the worktree's own, against the primary checkout's copy of that submodule, and precious ones keep the worktree as `judgment/precious-ignored-files` with paths relative to the worktree.

low — Submodule removal-gate paragraph packs five blockers, two outcomes, and three removal.error entry forms into one run of prose
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the rewritten submodule paragraph under "What removal requires" in references/removal-gates.md, the gate sequence in remove_worktree (audit-checkouts.sh), and the judgment/submodule-local-work row of the "Resolve a kept worktree" table in the same file.

What the passage does: one paragraph of roughly 260 words now covers (1) why Git forces --force, (2) the per-submodule local-work conditions, including a tag exception, (3) files under a Gitlink path with no checkout, listed as <path> (files without a checkout), (4) submodule Git directories under modules/ with no checkout, listed as <name> (not checked out), where <name> is a submodule name, not a path, and (5) ignored files inside submodules, which produce a different outcome, judgment/precious-ignored-files. The anchored sentence ends a long or list with "unless origin has the same tag on the same object". The code applies that exception only to tags (the ls-remote loop in submodule_holds_local_work), but in running prose a literal reader can take unless to cover the whole list.

Why it matters: an agent handling a kept worktree reads this reference to match each removal.error line to its cause and remedy. The three entry forms and their different meanings (path vs. submodule name, inspected vs. uninspectable) are the decisive facts, and they are buried mid-paragraph. Skill guidance: "Group by concept: keep a term's definition, rule, and caveat together", "bullets for flat choices", and "attach conditions to the action they govern."

Proposed correction: keep the --force rationale as its own short paragraph, then list what keeps the worktree as bullets, one per removal.error form:

  • <path>: the submodule has uncommitted files, a stash, a linked worktree of its own, a HEAD or branch commit no remote-tracking ref holds, or a tag naming no commit or a commit no remote-tracking ref holds. A tag that origin has on the same object (or, for a lightweight tag, on its commit) does not count.
  • <path> (files without a checkout): files under a Gitlink path that has no checkout; status never lists them.
  • <name> (not checked out): a submodule Git directory under the worktree's modules path that no checkout uses; <name> is the submodule name, not its path.
    Then one sentence on precious ignored files inside submodules producing judgment/precious-ignored-files. This keeps every existing fact and puts the tag exception on the tag item.

This is a readability finding. I did not observe an agent misreading the passage.

claim 01M3CK25TEKJN30QYDF0Z4SGXT of review 01M3CJXYC691YPN10BHMK7PR00

<!-- review:claim:01M3CK25TEKJN30QYDF0Z4SGXT --> **low** — Submodule removal-gate paragraph packs five blockers, two outcomes, and three `removal.error` entry forms into one run of prose lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the rewritten submodule paragraph under "What removal requires" in `references/removal-gates.md`, the gate sequence in `remove_worktree` (`audit-checkouts.sh`), and the `judgment/submodule-local-work` row of the "Resolve a kept worktree" table in the same file. > > What the passage does: one paragraph of roughly 260 words now covers (1) why Git forces `--force`, (2) the per-submodule local-work conditions, including a tag exception, (3) files under a Gitlink path with no checkout, listed as `<path> (files without a checkout)`, (4) submodule Git directories under `modules/` with no checkout, listed as `<name> (not checked out)`, where `<name>` is a submodule name, not a path, and (5) ignored files inside submodules, which produce a different outcome, `judgment/precious-ignored-files`. The anchored sentence ends a long `or` list with "unless origin has the same tag on the same object". The code applies that exception only to tags (the `ls-remote` loop in `submodule_holds_local_work`), but in running prose a literal reader can take `unless` to cover the whole list. > > Why it matters: an agent handling a kept worktree reads this reference to match each `removal.error` line to its cause and remedy. The three entry forms and their different meanings (path vs. submodule name, inspected vs. uninspectable) are the decisive facts, and they are buried mid-paragraph. Skill guidance: "Group by concept: keep a term's definition, rule, and caveat together", "bullets for flat choices", and "attach conditions to the action they govern." > > Proposed correction: keep the `--force` rationale as its own short paragraph, then list what keeps the worktree as bullets, one per `removal.error` form: > - `<path>`: the submodule has uncommitted files, a stash, a linked worktree of its own, a HEAD or branch commit no remote-tracking ref holds, or a tag naming no commit or a commit no remote-tracking ref holds. A tag that origin has on the same object (or, for a lightweight tag, on its commit) does not count. > - `<path> (files without a checkout)`: files under a Gitlink path that has no checkout; status never lists them. > - `<name> (not checked out)`: a submodule Git directory under the worktree's `modules` path that no checkout uses; `<name>` is the submodule name, not its path. > Then one sentence on precious ignored files inside submodules producing `judgment/precious-ignored-files`. This keeps every existing fact and puts the tag exception on the tag item. > > This is a readability finding. I did not observe an agent misreading the passage. claim `01M3CK25TEKJN30QYDF0Z4SGXT` of review `01M3CJXYC691YPN10BHMK7PR00`
Author
Owner

Fixed in 88273cd. The --force rationale is its own paragraph, the blockers are one bullet per removal.error form with the tag exception on the tag item, and ignored files in submodules get their own sentence.

<!-- gh-feedback:reply-to:89317 --> Fixed in 88273cd. The `--force` rationale is its own paragraph, the blockers are one bullet per `removal.error` form with the tag exception on the tag item, and ignored files in submodules get their own sentence.
jercik marked this conversation as resolved
Lines 32-34
@ -32,0 +29,6 @@
echo " a fast-forward or selector move while a submodule, at any depth, sits on a commit"
echo " that its superproject does not record and no ref in the submodule holds;"
echo " replacing a local selector tag unless the fetched tag or another ref holds its commit;"
echo " removing a worktree while a submodule holds a stash, an edit, a commit no remote-tracking"
echo " ref holds, or a tag origin lacks, or while files or a submodule Git directory in it"
echo " have no checkout to inspect."

medium — --help says removal refuses any submodule tag origin lacks, but a local tag on a pushed commit is deleted without refusal
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new Refuses, per repository: block in usage() of audit-checkouts.sh, the removal gate it summarizes (submodule_holds_local_work in removal mode, and remove_worktree's gate sequence), and the matching prose in references/removal-gates.md. SKILL.md tells the agent to run scripts/audit-checkouts.sh --help before first use, so this help text is agent-facing reference.

What the help says: removal is refused while a submodule holds "a stash, an edit, a commit no remote-tracking ref holds, or a tag origin lacks".

What the code does: submodule_holds_local_work first runs rev-list -n 1 --tags --not --remotes; when that is empty and no tag points at a non-commit, it return 1 (no work) before any ls-remote comparison. So a local lightweight or annotated tag that origin lacks, sitting on a commit some remote-tracking ref holds, does not block removal, and the tag ref is deleted with the submodule's Git directory. removal-gates.md states the rule correctly: a tag blocks when it "names no commit or whose commit no remote-tracking ref holds, unless origin has the same tag on the same object". The help's own final sentence ("once every commit there is held by a remote-tracking ref") also contradicts "a tag origin lacks". The list is also incomplete against the gates the change added: a submodule's own linked worktree (worktree list --porcelain count > 1) and precious ignored files inside a submodule (list_submodule_precious_ignored_paths → judgment/precious-ignored-files) both refuse removal but are not named.

Why it matters: an agent that reads the help (per SKILL.md) and then tells the user their unpushed submodule tag is protected is wrong; the tag is gone after removal. Skill guidance: "Make claims verifiable" and "use one term for one concept" — the help and the reference now state two different tag rules.

Proposed correction: "removing a worktree while a submodule holds a stash, an edit, a linked worktree, precious ignored files, a commit no remote-tracking ref holds, or a tag on such a commit that origin lacks, or while files or a submodule Git directory in it have no checkout to inspect." This keeps the compact summary while matching the gate and the reference.

Proof gap: I traced the shell statically; I did not run a reproduction with a pushed-commit tag.

claim 01M3CK1S2686KAF9CGEEVDCTN3 of review 01M3CJXYC691YPN10BHMK7PR00

<!-- review:claim:01M3CK1S2686KAF9CGEEVDCTN3 --> **medium** — `--help` says removal refuses any submodule tag origin lacks, but a local tag on a pushed commit is deleted without refusal lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new `Refuses, per repository:` block in `usage()` of `audit-checkouts.sh`, the removal gate it summarizes (`submodule_holds_local_work` in removal mode, and `remove_worktree`'s gate sequence), and the matching prose in `references/removal-gates.md`. SKILL.md tells the agent to run `scripts/audit-checkouts.sh --help` before first use, so this help text is agent-facing reference. > > What the help says: removal is refused while a submodule holds "a stash, an edit, a commit no remote-tracking ref holds, or a tag origin lacks". > > What the code does: `submodule_holds_local_work` first runs `rev-list -n 1 --tags --not --remotes`; when that is empty and no tag points at a non-commit, it `return 1` (no work) before any `ls-remote` comparison. So a local lightweight or annotated tag that origin lacks, sitting on a commit some remote-tracking ref holds, does not block removal, and the tag ref is deleted with the submodule's Git directory. removal-gates.md states the rule correctly: a tag blocks when it "names no commit or whose commit no remote-tracking ref holds, unless origin has the same tag on the same object". The help's own final sentence ("once every commit there is held by a remote-tracking ref") also contradicts "a tag origin lacks". The list is also incomplete against the gates the change added: a submodule's own linked worktree (`worktree list --porcelain` count > 1) and precious ignored files inside a submodule (`list_submodule_precious_ignored_paths` → `judgment/precious-ignored-files`) both refuse removal but are not named. > > Why it matters: an agent that reads the help (per SKILL.md) and then tells the user their unpushed submodule tag is protected is wrong; the tag is gone after removal. Skill guidance: "Make claims verifiable" and "use one term for one concept" — the help and the reference now state two different tag rules. > > Proposed correction: "removing a worktree while a submodule holds a stash, an edit, a linked worktree, precious ignored files, a commit no remote-tracking ref holds, or a tag on such a commit that origin lacks, or while files or a submodule Git directory in it have no checkout to inspect." This keeps the compact summary while matching the gate and the reference. > > Proof gap: I traced the shell statically; I did not run a reproduction with a pushed-commit tag. claim `01M3CK1S2686KAF9CGEEVDCTN3` of review `01M3CJXYC691YPN10BHMK7PR00`
Author
Owner

Fixed in 88273cd with the proposed wording: --help now names a linked worktree, precious ignored files, and a tag on a commit no remote-tracking ref holds that origin lacks.

<!-- gh-feedback:reply-to:89315 --> Fixed in 88273cd with the proposed wording: `--help` now names a linked worktree, precious ignored files, and a tag on a commit no remote-tracking ref holds that origin lacks.
jercik marked this conversation as resolved
@ -552,0 +617,4 @@
if [ -n "$primary_path" ] && [ -e "$primary_path/$gitlink_path/.git" ]; then
submodule_primary="$primary_path/$gitlink_path"
fi
if ! list_precious_ignored_paths "$submodule_path" "$submodule_primary" "$scratch.status" "$scratch.error" >"$scratch.precious"; then

low — The submodule ignored-file scan matches AUDIT_CHECKOUTS_REGENERABLE_IGNORED against submodule-relative paths, not the documented worktree-relative ones
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new list_submodule_precious_ignored_paths in scripts/audit-checkouts.sh, list_precious_ignored_paths / ignored_content_classify_root / is_regenerable_ignored_path in scripts/inspect-ignored-content.sh, the --help Environment text, and references/removal-gates.md.

Mechanism: the new function calls list_precious_ignored_paths with each submodule's own path as the scan root. That function takes entries from git status --ignored run inside the submodule, so each entry is relative to the submodule. ignored_content_classify_root then passes that entry straight to is_regenerable_ignored_path, which matches it against the user's AUDIT_CHECKOUTS_REGENERABLE_IGNORED globs. The prefix is added only when paths are printed ($prefix$gitlink_path/$precious_path), after classification.

What the docs say: removal-gates.md says the globs are "matched against paths relative to the worktree root ... so .tool-cache/:*/.tool-cache/ covers the root and nested directories while .tool-cache matches neither". --help says "relative to the worktree". So .tool-cache/ alone should exempt only <worktree>/.tool-cache/.

Observed reproduction (git 2.47, script sourced): I used a linked worktree wt3 with submodule dep, excluded .tool-cache/ in the submodule, and wrote dep/.tool-cache/data. Running list_submodule_precious_ignored_paths /tmp/exp/wt3 /tmp/exp/sup scr/s gave:

  • no env: prints dep/.tool-cache/ (precious, blocks removal)
  • AUDIT_CHECKOUTS_REGENERABLE_IGNORED=.tool-cache/: prints nothing, so the file is treated as regenerable and worktree remove --force would delete it
  • AUDIT_CHECKOUTS_REGENERABLE_IGNORED=dep/.tool-cache/, the documented worktree-relative path: still prints dep/.tool-cache/

Impact: a root-only exemption silently widens to the root of every submodule, so the removal deletes ignored files the docs say still block it. An exemption written with the documented worktree-relative path into a submodule never matches. Built-in root-anchored names (next-env.d.ts, .ansible/vault_pass.txt) are re-anchored the same way. Fix: classify $prefix$gitlink_path/<entry> instead, or document the per-submodule anchoring.

claim 01M3CK8MTB5JR5H0N24X0JEH1A of review 01M3CJXYC691YPN10BHMK7PR00

<!-- review:claim:01M3CK8MTB5JR5H0N24X0JEH1A --> **low** — The submodule ignored-file scan matches AUDIT_CHECKOUTS_REGENERABLE_IGNORED against submodule-relative paths, not the documented worktree-relative ones lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new `list_submodule_precious_ignored_paths` in scripts/audit-checkouts.sh, `list_precious_ignored_paths` / `ignored_content_classify_root` / `is_regenerable_ignored_path` in scripts/inspect-ignored-content.sh, the --help Environment text, and references/removal-gates.md. > > Mechanism: the new function calls `list_precious_ignored_paths` with each submodule's own path as the scan root. That function takes entries from `git status --ignored` run inside the submodule, so each entry is relative to the submodule. `ignored_content_classify_root` then passes that entry straight to `is_regenerable_ignored_path`, which matches it against the user's `AUDIT_CHECKOUTS_REGENERABLE_IGNORED` globs. The prefix is added only when paths are printed (`$prefix$gitlink_path/$precious_path`), after classification. > > What the docs say: removal-gates.md says the globs are "matched against paths relative to the worktree root ... so `.tool-cache/:*/.tool-cache/` covers the root and nested directories while `.tool-cache` matches neither". --help says "relative to the worktree". So `.tool-cache/` alone should exempt only `<worktree>/.tool-cache/`. > > Observed reproduction (git 2.47, script sourced): I used a linked worktree `wt3` with submodule `dep`, excluded `.tool-cache/` in the submodule, and wrote `dep/.tool-cache/data`. Running `list_submodule_precious_ignored_paths /tmp/exp/wt3 /tmp/exp/sup scr/s` gave: > - no env: prints `dep/.tool-cache/` (precious, blocks removal) > - `AUDIT_CHECKOUTS_REGENERABLE_IGNORED=.tool-cache/`: prints nothing, so the file is treated as regenerable and `worktree remove --force` would delete it > - `AUDIT_CHECKOUTS_REGENERABLE_IGNORED=dep/.tool-cache/`, the documented worktree-relative path: still prints `dep/.tool-cache/` > > Impact: a root-only exemption silently widens to the root of every submodule, so the removal deletes ignored files the docs say still block it. An exemption written with the documented worktree-relative path into a submodule never matches. Built-in root-anchored names (`next-env.d.ts`, `.ansible/vault_pass.txt`) are re-anchored the same way. Fix: classify `$prefix$gitlink_path/<entry>` instead, or document the per-submodule anchoring. claim `01M3CK8MTB5JR5H0N24X0JEH1A` of review `01M3CJXYC691YPN10BHMK7PR00`
Author
Owner

Fixed in 88273cd. Reproduced: .tool-cache/ exempted dependency/.tool-cache/. The submodule scan now classifies each path with the submodule's path as a prefix, so built-in and AUDIT_CHECKOUTS_REGENERABLE_IGNORED globs see worktree-relative paths; the test covers both the root-only glob (still precious) and dependency/.tool-cache/ (exempt).

<!-- gh-feedback:reply-to:89318 --> Fixed in 88273cd. Reproduced: `.tool-cache/` exempted `dependency/.tool-cache/`. The submodule scan now classifies each path with the submodule's path as a prefix, so built-in and `AUDIT_CHECKOUTS_REGENERABLE_IGNORED` globs see worktree-relative paths; the test covers both the root-only glob (still precious) and `dependency/.tool-cache/` (exempt).
jercik marked this conversation as resolved
fix(audit-git-checkouts): submodule ignored-file exemptions should match worktree-relative paths
All checks were successful
commit-msg / commitlint (pull_request) Successful in 20s
Node tests / node:test (pull_request) Successful in 1m46s
Review / Review (pull_request_target) Successful in 7m40s
88273cdb55
- Classify a submodule's ignored paths as the worktree sees them, so a
  root-only AUDIT_CHECKOUTS_REGENERABLE_IGNORED glob no longer exempts
  the same name at every submodule root.
- List the submodule removal blockers per removal.error form, and name
  every blocker in --help.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ -22,0 +26,4 @@
- `<path> (files without a checkout)`: files under a Gitlink path that has no checkout, which status never lists.
- `<name> (not checked out)`: a submodule Git directory that no checkout uses, such as one `git submodule deinit` or `git rm` leaves under `$(git -C <worktree> rev-parse --path-format=absolute --git-path modules)`. The driver does not inspect it. `<name>` is its path relative to that `modules` directory, which is the submodule's name, not its checkout path.
Ignored files inside each populated submodule are scanned like the worktree's own, against the primary checkout's copy of that submodule, with paths and `AUDIT_CHECKOUTS_REGENERABLE_IGNORED` globs relative to the worktree; precious ones keep the worktree as `judgment/precious-ignored-files`.

low — Submodule ignored-file scanning is described as "like the worktree's own", but it runs only in the removal gate and never appears in the diagnostic ignoredScan
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new paragraph in references/removal-gates.md and the "Ignored files" section that follows it. In scripts/audit-checkouts.sh I read annotate_ignored_scan, which builds the diagnostic ignoredScan by calling only write_ignored_scan on the worktree, and the removal gate, which calls list_submodule_precious_ignored_paths and then rewrites removal-ignored.json's preciousPaths with the submodule paths. list_submodule_precious_ignored_paths has exactly one call site, in the removal gate.

What the reference says: "Ignored files inside each populated submodule are scanned like the worktree's own". The next section says "Each in-root linked worktree carries a diagnostic ignoredScan, and removal records its own fresh removal.ignoredScan". SKILL.md adds that a cached or --no-remove run "still shows the locks and primary-only ignored files it observed". Taken together, a reader expects submodule files such as dependency/.env to appear in the diagnostic scan too. They do not. They appear only in removal.ignoredScan.preciousPaths, and only when the removal gate reaches that step, after the submodule local-work walk has passed.

Why it matters: on a --no-remove run, or when an earlier gate keeps the worktree, an agent reading an empty diagnostic preciousPaths can tell the user a merged worktree holds no primary-only ignored files. A submodule's .env would then be deleted on a later manual or driver removal without the user having been told about it. SKILL.md's rule that a --no-remove run proves no gate limits the damage, so this is low.

Suggested correction: state the scope and where the result lands, for example: "The removal gate also scans ignored files inside each populated submodule like the worktree's own… Precious ones join removal.ignoredScan.preciousPaths as worktree-relative paths and keep the worktree as judgment/precious-ignored-files. The diagnostic ignoredScan covers the worktree's own files only." This keeps the existing comparison and glob rules and removes the false impression that diagnostic output covers submodules.

Proof: static tracing of the call sites named above. I did not run a --no-remove audit.

claim 01M3CKNMSKPYRCBY73KKT1ZJM4 of review 01M3CKGAA518KBQ3S8DYGAZZKF

<!-- review:claim:01M3CKNMSKPYRCBY73KKT1ZJM4 --> **low** — Submodule ignored-file scanning is described as "like the worktree's own", but it runs only in the removal gate and never appears in the diagnostic `ignoredScan` lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new paragraph in `references/removal-gates.md` and the "Ignored files" section that follows it. In `scripts/audit-checkouts.sh` I read `annotate_ignored_scan`, which builds the diagnostic `ignoredScan` by calling only `write_ignored_scan` on the worktree, and the removal gate, which calls `list_submodule_precious_ignored_paths` and then rewrites `removal-ignored.json`'s `preciousPaths` with the submodule paths. `list_submodule_precious_ignored_paths` has exactly one call site, in the removal gate. > > What the reference says: "Ignored files inside each populated submodule are scanned like the worktree's own". The next section says "Each in-root linked worktree carries a diagnostic `ignoredScan`, and removal records its own fresh `removal.ignoredScan`". SKILL.md adds that a cached or `--no-remove` run "still shows the locks and primary-only ignored files it observed". Taken together, a reader expects submodule files such as `dependency/.env` to appear in the diagnostic scan too. They do not. They appear only in `removal.ignoredScan.preciousPaths`, and only when the removal gate reaches that step, after the submodule local-work walk has passed. > > Why it matters: on a `--no-remove` run, or when an earlier gate keeps the worktree, an agent reading an empty diagnostic `preciousPaths` can tell the user a merged worktree holds no primary-only ignored files. A submodule's `.env` would then be deleted on a later manual or driver removal without the user having been told about it. SKILL.md's rule that a `--no-remove` run proves no gate limits the damage, so this is low. > > Suggested correction: state the scope and where the result lands, for example: "The removal gate also scans ignored files inside each populated submodule like the worktree's own… Precious ones join `removal.ignoredScan.preciousPaths` as worktree-relative paths and keep the worktree as `judgment/precious-ignored-files`. The diagnostic `ignoredScan` covers the worktree's own files only." This keeps the existing comparison and glob rules and removes the false impression that diagnostic output covers submodules. > > Proof: static tracing of the call sites named above. I did not run a `--no-remove` audit. claim `01M3CKNMSKPYRCBY73KKT1ZJM4` of review `01M3CKGAA518KBQ3S8DYGAZZKF`
jercik marked this conversation as resolved
Lines 36-37
@ -32,0 +34,5 @@
echo " origin lacks, or while files or a submodule Git directory in it have no checkout to"
echo " inspect."
echo "The audited repositories' branch refs and stashes are never deleted. Removing a"
echo "worktree deletes its submodules' Git directories, local branches included, once every"
echo "commit there is held by a remote-tracking ref."

low — --help says submodule Git directories are deleted only once every commit is held by a remote-tracking ref, but a commit held only by an origin tag also qualifies
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the usage() text in audit-checkouts.sh, submodule_holds_local_work in the same file, the test "a submodule tag counts as local work only when origin lacks it or names another object" in audit-checkouts.test.mjs, and the <path> bullet in references/removal-gates.md.

What the help says: the closing sentence reads "Removing a worktree deletes its submodules' Git directories, local branches included, once every commit there is held by a remote-tracking ref." A few lines earlier, the same help text's "Refuses" list says removal is refused only for "a tag on such a commit that origin lacks". In submodule_holds_local_work, a local tag passes when git ls-remote --tags origin lists the same tag on the same object, or, for a lightweight tag, the peeled ^{} entry. The code does not check whether a remote-tracking ref holds that tag's commit. The test tags v0.9 upstream on a commit whose branch is then deleted, fetches the tag into the submodule, and asserts removed. So the tool removes a submodule Git directory that holds a commit no remote-tracking ref holds.

Why it matters: --help is the driver's contract, and SKILL.md tells the agent to read it before first use. An agent that tells the user "every deleted commit was on a remote-tracking ref" is overstating the guarantee. The two sentences in the same help block also disagree with each other.

Suggested correction: state the real condition, for example: "Removing a worktree deletes its submodules' Git directories, local branches and tags included, once origin holds every branch commit and tag there." Or end the sentence at "…Git directories, local branches included" and let the Refuses list carry the conditions. Either version keeps the useful warning that removal deletes local branches and drops the false guarantee.

Proof: static reading of the code and the test's assertion. I did not run the suite.

claim 01M3CKMB3NGD4D3W36QRR7MD57 of review 01M3CKGAA518KBQ3S8DYGAZZKF

<!-- review:claim:01M3CKMB3NGD4D3W36QRR7MD57 --> **low** — `--help` says submodule Git directories are deleted only once every commit is held by a remote-tracking ref, but a commit held only by an origin tag also qualifies lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the `usage()` text in `audit-checkouts.sh`, `submodule_holds_local_work` in the same file, the test "a submodule tag counts as local work only when origin lacks it or names another object" in `audit-checkouts.test.mjs`, and the `<path>` bullet in `references/removal-gates.md`. > > What the help says: the closing sentence reads "Removing a worktree deletes its submodules' Git directories, local branches included, once every commit there is held by a remote-tracking ref." A few lines earlier, the same help text's "Refuses" list says removal is refused only for "a tag on such a commit that origin lacks". In `submodule_holds_local_work`, a local tag passes when `git ls-remote --tags origin` lists the same tag on the same object, or, for a lightweight tag, the peeled `^{}` entry. The code does not check whether a remote-tracking ref holds that tag's commit. The test tags `v0.9` upstream on a commit whose branch is then deleted, fetches the tag into the submodule, and asserts `removed`. So the tool removes a submodule Git directory that holds a commit no remote-tracking ref holds. > > Why it matters: `--help` is the driver's contract, and SKILL.md tells the agent to read it before first use. An agent that tells the user "every deleted commit was on a remote-tracking ref" is overstating the guarantee. The two sentences in the same help block also disagree with each other. > > Suggested correction: state the real condition, for example: "Removing a worktree deletes its submodules' Git directories, local branches and tags included, once origin holds every branch commit and tag there." Or end the sentence at "…Git directories, local branches included" and let the Refuses list carry the conditions. Either version keeps the useful warning that removal deletes local branches and drops the false guarantee. > > Proof: static reading of the code and the test's assertion. I did not run the suite. claim `01M3CKMB3NGD4D3W36QRR7MD57` of review `01M3CKGAA518KBQ3S8DYGAZZKF`
jercik marked this conversation as resolved
Lines 506-512
@ -493,1 +505,9 @@
# remote-tracking ref holds.
# directories, so only commits remote-tracking refs hold, and tags
# origin has, survive; a recorded Gitlink names a commit without
# holding it. HEAD must be held by a remote-tracking ref, and the
# submodule must have no uncommitted
# files, no stash, no linked worktree, no branch commit that no
# remote-tracking ref holds, and no tag on a non-commit or on a
# commit no remote-tracking ref holds unless origin has that tag
# on the same object. A Gitlink path without a checkout must be
# empty.

low — list_submodules_with_local_work header comment misses the lightweight-tag exception, contradicts its own "populated submodule" scope, and is broken mid-sentence
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the header comment above list_submodules_with_local_work and the functions it describes, list_submodules_with_local_work and submodule_holds_local_work, in scripts/audit-checkouts.sh.

What the comment says, and three problems with it:

  1. The first line says the function "Prints, one per line, each populated submodule at any depth that holds work". In removal mode it now also prints <path> (files without a checkout) for a Gitlink path that has no .git, which by definition is not a populated submodule. The last sentence of the anchored block ("A Gitlink path without a checkout must be empty.") hints at this, but it contradicts the opening line and does not mention the different output form.
  2. The tag rule says a tag is work "unless origin has that tag on the same object". The code has a second exemption: [ "$tag_type" = commit ] && grep -F -x -q -- "$tag_object $tag_ref^{}" <<<"$remote_tags" && continue. That exempts a lightweight local tag whose commit matches origin's annotated tag of the same name after peeling, and the test "a lightweight submodule tag on the commit origin's tag peels to is not local work" covers it. references/removal-gates.md documents this case; this comment does not.
  3. The line break after "no uncommitted" leaves a short line in the middle of the sentence, left over from an edit.

Why it matters: this comment is the one place that states the whole removal-mode contract in the script. A maintainer who trusts it will think lightweight tags need an exact object match, and will miss that the function's output can include annotated lines that are not submodule paths. Downstream code does rely on that output form: the removal.error lines rendered in the report.

Suggested correction: open with "Prints, one per line, each submodule path at any depth that holds work a submodule move or a worktree removal would lose; in removal mode a Gitlink path with files but no checkout prints as <path> (files without a checkout)." End the tag clause with "unless origin has that tag on the same object or, for a lightweight tag, a tag of that name peeling to its commit." Reflow the paragraph. This keeps every condition the comment already lists and makes it match the code.

Proof: static comparison of the comment with the code lines quoted above.

claim 01M3CKMPYNTF6MYKABB37GYB9Y of review 01M3CKGAA518KBQ3S8DYGAZZKF

<!-- review:claim:01M3CKMPYNTF6MYKABB37GYB9Y --> **low** — `list_submodules_with_local_work` header comment misses the lightweight-tag exception, contradicts its own "populated submodule" scope, and is broken mid-sentence lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the header comment above `list_submodules_with_local_work` and the functions it describes, `list_submodules_with_local_work` and `submodule_holds_local_work`, in `scripts/audit-checkouts.sh`. > > What the comment says, and three problems with it: > 1. The first line says the function "Prints, one per line, each populated submodule at any depth that holds work". In removal mode it now also prints `<path> (files without a checkout)` for a Gitlink path that has no `.git`, which by definition is not a populated submodule. The last sentence of the anchored block ("A Gitlink path without a checkout must be empty.") hints at this, but it contradicts the opening line and does not mention the different output form. > 2. The tag rule says a tag is work "unless origin has that tag on the same object". The code has a second exemption: `[ "$tag_type" = commit ] && grep -F -x -q -- "$tag_object $tag_ref^{}" <<<"$remote_tags" && continue`. That exempts a lightweight local tag whose commit matches origin's annotated tag of the same name after peeling, and the test "a lightweight submodule tag on the commit origin's tag peels to is not local work" covers it. `references/removal-gates.md` documents this case; this comment does not. > 3. The line break after "no uncommitted" leaves a short line in the middle of the sentence, left over from an edit. > > Why it matters: this comment is the one place that states the whole removal-mode contract in the script. A maintainer who trusts it will think lightweight tags need an exact object match, and will miss that the function's output can include annotated lines that are not submodule paths. Downstream code does rely on that output form: the `removal.error` lines rendered in the report. > > Suggested correction: open with "Prints, one per line, each submodule path at any depth that holds work a submodule move or a worktree removal would lose; in removal mode a Gitlink path with files but no checkout prints as `<path> (files without a checkout)`." End the tag clause with "unless origin has that tag on the same object or, for a lightweight tag, a tag of that name peeling to its commit." Reflow the paragraph. This keeps every condition the comment already lists and makes it match the code. > > Proof: static comparison of the comment with the code lines quoted above. claim `01M3CKMPYNTF6MYKABB37GYB9Y` of review `01M3CKGAA518KBQ3S8DYGAZZKF`
jercik marked this conversation as resolved
Lines 1127-1128
@ -1065,0 +1192,5 @@
echo operational/gate-check-failed
return
fi
if [ -d "$modules_path" ]; then
if ! unchecked_git_dirs=$(list_submodule_git_dirs_without_checkout "$modules_path" "$populated_git_dirs" 2>>"$gate_error_path"); then

medium — Removal gate only looks for checkout-less submodule Git dirs under the worktree's own modules/, missing ones inside a non-absorbed submodule's .git, which --force then deletes
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: decide_removal_outcome in audit-checkouts.sh, plus the new helpers list_submodule_git_dirs_without_checkout, list_populated_submodule_git_dirs, and list_submodules_with_local_work (removal mode).

Mechanism: the gate for "a submodule Git directory without a checkout" runs only on modules_path=$(git -C "$worktree_path" rev-parse --path-format=absolute --git-path modules), which is the superproject worktree's own .git/worktrees/<id>/modules, and only when that directory exists. list_submodules_with_local_work recurses only into Gitlinks whose <path>/.git exists, and for an unpopulated Gitlink it only checks whether the directory has files. So when a populated submodule is not absorbed (its .git is a real directory inside the checkout, as with the git add <nested-clone> layout the new gitmodules: false fixture sets up), any deinit'ed nested submodule Git dir at <sub>/.git/modules/<name> is never looked at. populated_git_dirs is non-empty, so removal runs with --force and deletes that Git dir, including any local branches.

Reproduction (observed, git 2.47.3, sourcing the script): superproject worktree wt with Gitlink dep filled by a plain git clone of a repo that has submodule inner; git -C dep submodule update --init; in dep/inner, git checkout -b wip && git commit --allow-empty -m wip; then git -C dep submodule deinit --force inner. After that, dep/.git/modules/inner still holds branch wip (git --git-dir dep/.git/modules/inner log wip shows it). The worktree's --git-path modules directory does not exist, so the [ -d "$modules_path" ] block is skipped. list_submodules_with_local_work wt removal printed nothing and returned 0, and list_populated_submodule_git_dirs wt printed /tmp/exp2/wt/dep/.git, which forces removal. I could not run the full driver because jq is missing in this sandbox, so the final git worktree remove --force step is inferred from the code rather than seen.

Impact: this contradicts the documented gate in references/removal-gates.md ("No submodule, at any depth, holding work ... and no submodule Git directory without a checkout") and the --help text ("or while files or a submodule Git directory in it have no checkout to inspect"). In this layout, unpushed commits in a nested submodule are deleted silently. Fix: for each populated submodule, also run list_submodule_git_dirs_without_checkout on its own --git-path modules directory, or walk the modules dir of every Git dir that list_populated_submodule_git_dirs returns. This layout is uncommon, so I rated it medium.

claim 01M3CKTQ0NCBZ4S0CZ69KDDDEA of review 01M3CKGAA518KBQ3S8DYGAZZKF

<!-- review:claim:01M3CKTQ0NCBZ4S0CZ69KDDDEA --> **medium** — Removal gate only looks for checkout-less submodule Git dirs under the worktree's own modules/, missing ones inside a non-absorbed submodule's .git, which --force then deletes lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: `decide_removal_outcome` in audit-checkouts.sh, plus the new helpers `list_submodule_git_dirs_without_checkout`, `list_populated_submodule_git_dirs`, and `list_submodules_with_local_work` (removal mode). > > Mechanism: the gate for "a submodule Git directory without a checkout" runs only on `modules_path=$(git -C "$worktree_path" rev-parse --path-format=absolute --git-path modules)`, which is the superproject worktree's own `.git/worktrees/<id>/modules`, and only when that directory exists. `list_submodules_with_local_work` recurses only into Gitlinks whose `<path>/.git` exists, and for an unpopulated Gitlink it only checks whether the directory has files. So when a populated submodule is *not* absorbed (its `.git` is a real directory inside the checkout, as with the `git add <nested-clone>` layout the new `gitmodules: false` fixture sets up), any deinit'ed nested submodule Git dir at `<sub>/.git/modules/<name>` is never looked at. `populated_git_dirs` is non-empty, so removal runs with `--force` and deletes that Git dir, including any local branches. > > Reproduction (observed, git 2.47.3, sourcing the script): superproject worktree `wt` with Gitlink `dep` filled by a plain `git clone` of a repo that has submodule `inner`; `git -C dep submodule update --init`; in `dep/inner`, `git checkout -b wip && git commit --allow-empty -m wip`; then `git -C dep submodule deinit --force inner`. After that, `dep/.git/modules/inner` still holds branch `wip` (`git --git-dir dep/.git/modules/inner log wip` shows it). The worktree's `--git-path modules` directory does not exist, so the `[ -d "$modules_path" ]` block is skipped. `list_submodules_with_local_work wt removal` printed nothing and returned 0, and `list_populated_submodule_git_dirs wt` printed `/tmp/exp2/wt/dep/.git`, which forces removal. I could not run the full driver because jq is missing in this sandbox, so the final `git worktree remove --force` step is inferred from the code rather than seen. > > Impact: this contradicts the documented gate in references/removal-gates.md ("No submodule, at any depth, holding work ... and no submodule Git directory without a checkout") and the `--help` text ("or while files or a submodule Git directory in it have no checkout to inspect"). In this layout, unpushed commits in a nested submodule are deleted silently. Fix: for each populated submodule, also run `list_submodule_git_dirs_without_checkout` on its own `--git-path modules` directory, or walk the modules dir of every Git dir that `list_populated_submodule_git_dirs` returns. This layout is uncommon, so I rated it medium. claim `01M3CKTQ0NCBZ4S0CZ69KDDDEA` of review `01M3CKGAA518KBQ3S8DYGAZZKF`
Author
Owner

Fixed in 5a8ae47. Reproduced end to end through maybe_remove_worktree: the worktree was removed and the nested wip branch with it. The gate now also checks the modules directory inside every populated submodule's Git directory that lies outside the worktree's own modules/, and lists such an entry by its worktree-relative path (dependency/.git/modules/inner (not checked out)). New test: "a nested submodule Git directory without a checkout keeps the worktree when its parent is not absorbed".

<!-- gh-feedback:reply-to:89341 --> Fixed in 5a8ae47. Reproduced end to end through `maybe_remove_worktree`: the worktree was removed and the nested `wip` branch with it. The gate now also checks the `modules` directory inside every populated submodule's Git directory that lies outside the worktree's own `modules/`, and lists such an entry by its worktree-relative path (`dependency/.git/modules/inner (not checked out)`). New test: "a nested submodule Git directory without a checkout keeps the worktree when its parent is not absorbed".
jercik marked this conversation as resolved
fix(audit-git-checkouts): removal should keep a nested submodule Git directory without a checkout
All checks were successful
commit-msg / commitlint (pull_request) Successful in 18s
Node tests / node:test (pull_request) Successful in 1m21s
Review / Review (pull_request_target) Successful in 8m10s
5a8ae47b63
A submodule whose Git directory sits in its checkout keeps its own
submodules' Git directories under that .git/modules, which the gate did
not look in, so --force deleted a deinitialized nested submodule's
unpushed branches.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
Owner

Replying to review summary comment #88475

Round-4 outcomes for review 01M3CKGAA518KBQ3S8DYGAZZKF (head 88273cd). From round 4 only clear, severe bugs get a push; 89341 (data loss) is fixed in 5a8ae47. The other three are real but change only wording, so they are acknowledged and deferred to a follow-up PR:

  • 89342: in references/removal-gates.md, say the removal gate scans ignored files inside submodules, that precious ones join removal.ignoredScan.preciousPaths as worktree-relative paths, and that the diagnostic ignoredScan covers the worktree's own files only.
  • 89343: in scripts/audit-checkouts.sh usage(), end the closing sentence at "…deletes its submodules' Git directories, local branches and tags included" and let the Refuses list carry the conditions.
  • 89344: in scripts/audit-checkouts.sh, open the list_submodules_with_local_work header with "each submodule path at any depth … in removal mode a Gitlink path with files but no checkout prints as <path> (files without a checkout)", add the lightweight-tag exception to its tag clause, and reflow the paragraph.
> Replying to review summary comment #88475 Round-4 outcomes for review `01M3CKGAA518KBQ3S8DYGAZZKF` (head 88273cd). From round 4 only clear, severe bugs get a push; 89341 (data loss) is fixed in 5a8ae47. The other three are real but change only wording, so they are acknowledged and deferred to a follow-up PR: - **89342:** in `references/removal-gates.md`, say the removal gate scans ignored files inside submodules, that precious ones join `removal.ignoredScan.preciousPaths` as worktree-relative paths, and that the diagnostic `ignoredScan` covers the worktree's own files only. - **89343:** in `scripts/audit-checkouts.sh` `usage()`, end the closing sentence at "…deletes its submodules' Git directories, local branches and tags included" and let the Refuses list carry the conditions. - **89344:** in `scripts/audit-checkouts.sh`, open the `list_submodules_with_local_work` header with "each submodule path at any depth … in removal mode a Gitlink path with files but no checkout prints as `<path> (files without a checkout)`", add the lightweight-tag exception to its tag clause, and reflow the paragraph.
@ -46,3 +54,3 @@
| `judgment/sparse-checkout` | Keep the cone's `skip-worktree` bits: clearing them turns absent files into apparent deletions. Confirm the cone with `git sparse-checkout list`, verify no file outside it is materialized, then remove manually once every other gate holds. |
| `judgment/precious-ignored-files` | Leave `preciousPaths` in place until the owner disposes of them or authorizes their deletion, then rerun. |
| `judgment/submodule-local-work` | The listed submodules hold work only this worktree has. Show the owner what each holds; rerun once it is pushed or they authorize discarding it. |
| `judgment/submodule-local-work` | The listed submodules hold work only this worktree has, or content the driver could not inspect. Show the owner what each holds; rerun once the work is pushed. If the owner authorizes discarding it instead, discard it in the submodule first, then rerun. For a `(not checked out)` entry, first show the owner what `<modules>/<name>` (or the listed worktree-relative path) holds, before anything checks it out: `git --git-dir <modules>/<name> --work-tree <modules>/<name> log --oneline HEAD --branches --tags --not --remotes` and `… stash list` (`--work-tree` overrides the deleted checkout its config names). Then, on their decision, restore the checkout with `git submodule update --init -- <path>` while a Gitlink remains, or delete the directory, and rerun. |

low — (not checked out) resolution uses <modules> and <path> placeholders the entry doesn't supply, and gives no way to map the listed name to a checkout path
lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new judgment/submodule-local-work row of the "Resolve a kept worktree" table and the three-form list above it in skills/audit-git-checkouts/references/removal-gates.md; the driver code that writes these entries (list_unchecked_submodule_git_dirs in scripts/audit-checkouts.sh, which appends " (not checked out)").

What the text says: the list defines a <name> (not checked out) entry as the Git directory's path relative to $(git -C <worktree> rev-parse --path-format=absolute --git-path modules), "which is the submodule's name, not its checkout path". A nested one is listed "by its worktree-relative path instead, such as dep/.git/modules/inner". The resolution cell then uses <modules>/<name> in the inspection commands and ends with git submodule update --init -- <path>.

What goes wrong: <modules> is never defined as a name; the reader has to work out that it means the output of the command in the bullet above. <path> is a checkout path, but the entry gives only the name, and the text itself warns that the two differ. Nothing tells the agent how to get from one to the other, for example through submodule.<name>.path in .gitmodules. For the nested form, neither <modules>/<name> nor <path> applies directly: the Git directory is under dep/, and git submodule update has to run inside dep with dep's own path for inner. An agent following the cell literally is likely to pass the name as the pathspec (which fails, or matches a different submodule) or to guess. The writing skill asks for one term per concept and for project-local terms to be defined on first use, and says to spend detail where a plausible mistake would derail the task.

Proposed correction: in the <name> (not checked out) bullet, name the directory once, e.g. "… under <modules>, the output of git -C <worktree> rev-parse --path-format=absolute --git-path modules". In the resolution, state the mapping: "restore the checkout with git submodule update --init -- <path>, where <path> is git config -f .gitmodules submodule.<name>.path, while a Gitlink remains; for a nested entry, run it inside the enclosing submodule." This keeps the existing safety ordering (inspect first, then restore or delete) and removes the guesswork.

Evidence: static reading of the reference and the driver. I didn't run the resolution commands.

claim 01M3CM8KYQHXHR3WX1YYHYJTA5 of review 01M3CM491QHBEFY37DG68PZJCJ

<!-- review:claim:01M3CM8KYQHXHR3WX1YYHYJTA5 --> **low** — `(not checked out)` resolution uses `<modules>` and `<path>` placeholders the entry doesn't supply, and gives no way to map the listed name to a checkout path lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new `judgment/submodule-local-work` row of the "Resolve a kept worktree" table and the three-form list above it in skills/audit-git-checkouts/references/removal-gates.md; the driver code that writes these entries (`list_unchecked_submodule_git_dirs` in scripts/audit-checkouts.sh, which appends " (not checked out)"). > > What the text says: the list defines a `<name> (not checked out)` entry as the Git directory's path relative to `$(git -C <worktree> rev-parse --path-format=absolute --git-path modules)`, "which is the submodule's name, not its checkout path". A nested one is listed "by its worktree-relative path instead, such as `dep/.git/modules/inner`". The resolution cell then uses `<modules>/<name>` in the inspection commands and ends with `git submodule update --init -- <path>`. > > What goes wrong: `<modules>` is never defined as a name; the reader has to work out that it means the output of the command in the bullet above. `<path>` is a checkout path, but the entry gives only the name, and the text itself warns that the two differ. Nothing tells the agent how to get from one to the other, for example through `submodule.<name>.path` in `.gitmodules`. For the nested form, neither `<modules>/<name>` nor `<path>` applies directly: the Git directory is under `dep/`, and `git submodule update` has to run inside `dep` with dep's own path for `inner`. An agent following the cell literally is likely to pass the name as the pathspec (which fails, or matches a different submodule) or to guess. The writing skill asks for one term per concept and for project-local terms to be defined on first use, and says to spend detail where a plausible mistake would derail the task. > > Proposed correction: in the `<name> (not checked out)` bullet, name the directory once, e.g. "… under `<modules>`, the output of `git -C <worktree> rev-parse --path-format=absolute --git-path modules`". In the resolution, state the mapping: "restore the checkout with `git submodule update --init -- <path>`, where `<path>` is `git config -f .gitmodules submodule.<name>.path`, while a Gitlink remains; for a nested entry, run it inside the enclosing submodule." This keeps the existing safety ordering (inspect first, then restore or delete) and removes the guesswork. > > Evidence: static reading of the reference and the driver. I didn't run the resolution commands. claim `01M3CM8KYQHXHR3WX1YYHYJTA5` of review `01M3CM491QHBEFY37DG68PZJCJ`
jercik marked this conversation as resolved
Lines 36-37
@ -32,0 +34,5 @@
echo " origin lacks, or while files or a submodule Git directory in it have no checkout to"
echo " inspect."
echo "The audited repositories' branch refs and stashes are never deleted. Removing a"
echo "worktree deletes its submodules' Git directories, local branches included, once every"
echo "commit there is held by a remote-tracking ref."

low — --help says submodule Git directories are deleted only once every commit is held by a remote-tracking ref, but the gate also accepts commits that only an origin tag holds
lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain

What I examined: the new usage() text in audit-checkouts.sh, the removal-mode branch of submodule_holds_local_work, and the new driver test "a submodule tag counts as local work only when origin lacks it or names another object" in audit-checkouts.test.mjs.

What the help says: "Removing a worktree deletes its submodules' Git directories, local branches included, once every commit there is held by a remote-tracking ref."

What the code does: submodule_holds_local_work (removal mode) checks tags separately. For each local tag it first runs grep -F -x -q -- "$tag_object $tag_ref" <<<"$remote_tags" && continue, where remote_tags comes from git ls-remote --tags origin. For a lightweight tag it also accepts a match on origin's peeled ^{} line. Only when neither matches does it run rev-list -n 1 "$tag_object" --not --remotes. So a submodule tag on a commit that no remote-tracking ref holds passes the gate whenever origin advertises the same tag. The worktree is then removed with --force, and that commit is deleted with the submodule Git directory. The new test depends on this. tagUpstreamReleaseOffBranch creates v0.9 on a commit whose branch was deleted upstream, and the test ends with assert.equal(remove().removal.outcome, "removed"). removal-gates.md describes the exemption correctly ("Such a tag does not count when origin has the same tag on the same object…"). Earlier in the same help text, the Refuses list also qualifies it ("a tag on such a commit that origin lacks"). Only this closing sentence states the stricter remote-tracking-ref guarantee.

What goes wrong: an operator deciding from --help whether removal is safe is told that every deleted commit is still on a remote-tracking ref. In fact some commits survive only as an upstream tag, which the server can move or delete, and no local remote-tracking ref backs them. The docs contradict each other and understate what removal relies on.

Evidence: static reading plus the in-tree test above. I could not run the driver tests because jq is not installed in this sandbox. Fix: reword the sentence, e.g. "…once a remote-tracking ref holds every branch and HEAD commit there, and origin has every tag on a commit that none holds."

claim 01M3CMGDAWQGRCQQGWEHE4R0MG of review 01M3CM491QHBEFY37DG68PZJCJ

<!-- review:claim:01M3CMGDAWQGRCQQGWEHE4R0MG --> **low** — --help says submodule Git directories are deleted only once every commit is held by a remote-tracking ref, but the gate also accepts commits that only an origin tag holds lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain > What I examined: the new `usage()` text in audit-checkouts.sh, the removal-mode branch of `submodule_holds_local_work`, and the new driver test "a submodule tag counts as local work only when origin lacks it or names another object" in audit-checkouts.test.mjs. > > What the help says: "Removing a worktree deletes its submodules' Git directories, local branches included, once every commit there is held by a remote-tracking ref." > > What the code does: `submodule_holds_local_work` (removal mode) checks tags separately. For each local tag it first runs `grep -F -x -q -- "$tag_object $tag_ref" <<<"$remote_tags" && continue`, where `remote_tags` comes from `git ls-remote --tags origin`. For a lightweight tag it also accepts a match on origin's peeled `^{}` line. Only when neither matches does it run `rev-list -n 1 "$tag_object" --not --remotes`. So a submodule tag on a commit that no remote-tracking ref holds passes the gate whenever origin advertises the same tag. The worktree is then removed with `--force`, and that commit is deleted with the submodule Git directory. The new test depends on this. `tagUpstreamReleaseOffBranch` creates `v0.9` on a commit whose branch was deleted upstream, and the test ends with `assert.equal(remove().removal.outcome, "removed")`. removal-gates.md describes the exemption correctly ("Such a tag does not count when origin has the same tag on the same object…"). Earlier in the same help text, the Refuses list also qualifies it ("a tag on such a commit that origin lacks"). Only this closing sentence states the stricter remote-tracking-ref guarantee. > > What goes wrong: an operator deciding from `--help` whether removal is safe is told that every deleted commit is still on a remote-tracking ref. In fact some commits survive only as an upstream tag, which the server can move or delete, and no local remote-tracking ref backs them. The docs contradict each other and understate what removal relies on. > > Evidence: static reading plus the in-tree test above. I could not run the driver tests because `jq` is not installed in this sandbox. Fix: reword the sentence, e.g. "…once a remote-tracking ref holds every branch and HEAD commit there, and origin has every tag on a commit that none holds." claim `01M3CMGDAWQGRCQQGWEHE4R0MG` of review `01M3CM491QHBEFY37DG68PZJCJ`
jercik marked this conversation as resolved
Author
Owner

Replying to review summary comment #88475

Round-5 outcomes for review 01M3CM491QHBEFY37DG68PZJCJ (head 5a8ae47). From round 5 only clear, severe bugs get a push. Both findings are real but change only wording, so nothing is pushed; they are acknowledged and deferred to a follow-up PR:

  • 89353: in references/removal-gates.md, name <modules> once in the (not checked out) bullet as the output of git -C <worktree> rev-parse --path-format=absolute --git-path modules, and in the resolution row say <path> is git config -f .gitmodules submodule.<name>.path, run inside the enclosing submodule for a nested entry.
  • 89354 (same as 89343): in scripts/audit-checkouts.sh usage(), end the closing sentence at "…deletes its submodules' Git directories, local branches and tags included", so it no longer claims every deleted commit is on a remote-tracking ref; the Refuses list already carries the tag condition.
> Replying to review summary comment #88475 Round-5 outcomes for review `01M3CM491QHBEFY37DG68PZJCJ` (head 5a8ae47). From round 5 only clear, severe bugs get a push. Both findings are real but change only wording, so nothing is pushed; they are acknowledged and deferred to a follow-up PR: - **89353:** in `references/removal-gates.md`, name `<modules>` once in the `(not checked out)` bullet as the output of `git -C <worktree> rev-parse --path-format=absolute --git-path modules`, and in the resolution row say `<path>` is `git config -f .gitmodules submodule.<name>.path`, run inside the enclosing submodule for a nested entry. - **89354 (same as 89343):** in `scripts/audit-checkouts.sh` `usage()`, end the closing sentence at "…deletes its submodules' Git directories, local branches and tags included", so it no longer claims every deleted commit is on a remote-tracking ref; the Refuses list already carries the tag condition.
jercik merged commit 070525d402 into main 2026-09-25 18:58:46 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
j4k-oss/agent-skills!81
No description provided.