fix(wt): interim safety guards from #140 review follow-ups #141

Merged

gabogg merged 2 commits from fix/wt-safety-interim into master 2026-09-26 20:47:53 +00:00

Conversation 2 Commits 2 Files changed 3 +61 -14

gabogg commented 2026-09-26 16:35:43 +00:00

Owner

Copy link

Problem

The review follow-ups in #140 include two items that can cause harm today, plus one open design point that is causing friction:

• S2-3: wt rm .. -f / wt rm . -f pass the .worktrees/ prefix check and reach git worktree remove --force on the main checkout. Only git's own refusal stops it.
• C2-1: running wt rm/wt prune from inside the worktree being removed deletes the calling session's working directory.
• C2-3 (still open): test and doc runs leave data/hikcentral.db and logs/lifecycle.log in every worktree, so the data-loss guard keeps nearly every worktree and wt rm -f risks becoming routine.

Approach

This is an interim fix. It closes the two sharp edges and makes the guard's refusals informative, without changing what the guard protects.

• holds_cwd(): rm refuses, and prune skips with a message, when the caller's cwd is inside the target worktree.
• cmd_rm canonicalises the target with realpath and requires it to sit directly under .worktrees/.
• describe_local_files(): the "holds ignored local files" refusal now lists each file with its size, so a 4 KB empty DB is easy to tell from real data.
• Docs: workspaces.md and forgejo-cli.md say to run rm/prune from the main checkout, and give the interim rule for -f. The permanent policy stays open in #140 (C2-3).

Refs #140 (S2-3, C2-1; C2-3 interim only).

Verification

Exercised on local throwaway branch docs/int-probe:

Case	Result
rm with data/ DB + log	Refused, listing 4096 bytes data/hikcentral.db and 2 bytes logs/lifecycle.log
rm .. -f / rm . -f	Refused: not a worktree directly under …/.worktrees
rm -f from inside the worktree	Refused: is the current directory; run scripts/wt from the main checkout
prune from inside a prunable worktree	Kept, with the same message
prune from the main checkout	Removed the worktree and deleted its merged branch

Pre-commit (ruff, pytest) is green and scripts/check_docs.py is clean.

🤖 Generated with Claude Code

## Problem The review follow-ups in #140 include two items that can cause harm today, plus one open design point that is causing friction: - **S2-3:** `wt rm .. -f` / `wt rm . -f` pass the `.worktrees/` prefix check and reach `git worktree remove --force` on the main checkout. Only git's own refusal stops it. - **C2-1:** running `wt rm`/`wt prune` from inside the worktree being removed deletes the calling session's working directory. - **C2-3 (still open):** test and doc runs leave `data/hikcentral.db` and `logs/lifecycle.log` in every worktree, so the data-loss guard keeps nearly every worktree and `wt rm -f` risks becoming routine. ## Approach This is an interim fix. It closes the two sharp edges and makes the guard's refusals informative, without changing what the guard protects. - `holds_cwd()`: `rm` refuses, and `prune` skips with a message, when the caller's cwd is inside the target worktree. - `cmd_rm` canonicalises the target with `realpath` and requires it to sit directly under `.worktrees/`. - `describe_local_files()`: the "holds ignored local files" refusal now lists each file with its size, so a 4 KB empty DB is easy to tell from real data. - Docs: `workspaces.md` and `forgejo-cli.md` say to run `rm`/`prune` from the main checkout, and give the interim rule for `-f`. The permanent policy stays open in #140 (C2-3). Refs #140 (S2-3, C2-1; C2-3 interim only). ## Verification Exercised on local throwaway branch `docs/int-probe`: | Case | Result | |---|---| | `rm` with `data/` DB + log | Refused, listing `4096 bytes data/hikcentral.db` and `2 bytes logs/lifecycle.log` | | `rm .. -f` / `rm . -f` | Refused: `not a worktree directly under …/.worktrees` | | `rm -f` from inside the worktree | Refused: `is the current directory; run scripts/wt from the main checkout` | | `prune` from inside a prunable worktree | Kept, with the same message | | `prune` from the main checkout | Removed the worktree and deleted its merged branch | Pre-commit (ruff, pytest) is green and `scripts/check_docs.py` is clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-26 16:35:44 +00:00

fix(wt): interim safety guards from #140 review follow-ups ... a770766fa2

- rm/prune refuse to remove the worktree holding the caller's cwd, so a
  session pruning from inside its own worktree keeps its directory
  (#140 C2-1); docs say to run them from the main checkout.
- rm canonicalises the target with realpath and requires it to sit
  directly under .worktrees/, so `wt rm .. -f` / `wt rm . -f` can no
  longer reach the main checkout (#140 S2-3).
- The "holds ignored local files" refusal lists each file with its size,
  so test-run leftovers (4 KB empty DB, lifecycle log) are easy to tell
  from real data. The guard itself is unchanged; the permanent policy is
  still open in #140 C2-3.

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

gabogg commented 2026-09-26 20:41:48 +00:00

Author

Owner

Copy link

Code review: PR #141 (fix/wt-safety-interim @ a770766 vs master @ c096d72)

Standards sources: AGENTS.md, docs/standards/*, .claude/skills/writing-for-agents, and the Fowler smell baseline. Spec: the approved interim plan (sizes in the refusal with the guard unchanged; #140 S2-3; #140 C2-1). The C2-3 permanent policy stays in #140.

Standards

Verified: rm .. and rm . are refused, as is a subdirectory target and a cwd inside the target. holds_cwd handles sibling names (foo vs foo-bar) and symlinked paths correctly. No breaches of AGENTS.md or the code standards.

#	Pri	Finding
S-1	P2	cmd_prune runs the keep-guards before the merged/upstream-gone checks, so every live, unmerged worktree with a data/ DB prints kept …: check, then wt rm -f. That points agents at rm -f on other sessions' live work; it was observed on the #137/#138 worktrees. Move the guards after the candidate checks.
S-2	P2	describe_local_files receives C-quoted porcelain paths, so a top-level ignored run ñ.log was silently missing from the list the docs tell you to read before rm -f. Use --porcelain -z and read the entries null-delimited. An ignored directory also lists every file with no limit (judgement call).
S-3	P3	Possible Duplicated Code: prune runs holds_cwd/local_files and then calls remove, which checks again. Use one keep_reason for both.
S-4	P3	forgejo-cli.md "from the main checkout" repeats workspaces.md (single source of truth).
S-5	P3	workspaces.md "Run both from the main checkout" comes after three commands. Name rm and prune.
S-6	P3	workspaces.md "list those files with their sizes" repeats the script's own output, and "~4 KB" is an environment fact that will go stale.
S-7	P3	usage() doesn't mention the cwd refusal.

Spec

Verified in an isolated sandbox (bare origin + clone):

• Guard strength is unchanged: local_files is identical, and the new path check is strictly tighter than before.
• Sizes are listed in both rm and prune.
• rm .. -f, rm . -f and a .worktrees/evil -> main symlink are refused.
• The cwd guard holds in rm (including with -f, from subdirectories, and via symlinked paths) and in prune.
• The docs say "from the main checkout", and no out-of-scope #140 items slipped in.

#	Pri	Finding
C-1	P3	"list each file with its size": quoted non-ASCII names and ignored symlinks produce an empty list, even though the guard still refuses. (Same root as S-2.)
C-2	P3	"C2-3's permanent policy stays open": workspaces.md declares any lifecycle.log "safe to discard", which pre-decides part of option 1 in #140. Soften it to "usually", or drop the claim.

---

Summary. Standards: 7 findings, worst S-1 (prune points to rm -f on live unmerged worktrees). Spec: 2 findings, both P3; worst C-1 (incomplete size list for quoted names and symlinks). First pass, so all findings get fixed on this branch.

🤖 Generated with Claude Code

# Code review: PR #141 (`fix/wt-safety-interim` @ a770766 vs `master` @ c096d72) **Standards** sources: AGENTS.md, `docs/standards/*`, `.claude/skills/writing-for-agents`, and the Fowler smell baseline. **Spec**: the approved interim plan (sizes in the refusal with the guard unchanged; #140 S2-3; #140 C2-1). The C2-3 permanent policy stays in #140. ## Standards Verified: `rm ..` and `rm .` are refused, as is a subdirectory target and a cwd inside the target. `holds_cwd` handles sibling names (`foo` vs `foo-bar`) and symlinked paths correctly. No breaches of AGENTS.md or the code standards. | # | Pri | Finding | |---|---|---| | S-1 | **P2** | `cmd_prune` runs the keep-guards *before* the merged/upstream-gone checks, so every live, unmerged worktree with a `data/` DB prints `kept …: check, then wt rm -f`. That points agents at `rm -f` on other sessions' live work; it was observed on the #137/#138 worktrees. Move the guards after the candidate checks. | | S-2 | **P2** | `describe_local_files` receives C-quoted porcelain paths, so a top-level ignored `run ñ.log` was silently missing from the list the docs tell you to read before `rm -f`. Use `--porcelain -z` and read the entries null-delimited. An ignored directory also lists every file with no limit (judgement call). | | S-3 | P3 | Possible Duplicated Code: `prune` runs `holds_cwd`/`local_files` and then calls `remove`, which checks again. Use one `keep_reason` for both. | | S-4 | P3 | `forgejo-cli.md` "from the main checkout" repeats `workspaces.md` (single source of truth). | | S-5 | P3 | `workspaces.md` "Run both from the main checkout" comes after three commands. Name `rm` and `prune`. | | S-6 | P3 | `workspaces.md` "list those files with their sizes" repeats the script's own output, and "~4 KB" is an environment fact that will go stale. | | S-7 | P3 | `usage()` doesn't mention the cwd refusal. | ## Spec Verified in an isolated sandbox (bare origin + clone): - Guard strength is unchanged: `local_files` is identical, and the new path check is strictly tighter than before. - Sizes are listed in both `rm` and `prune`. - `rm .. -f`, `rm . -f` and a `.worktrees/evil -> main` symlink are refused. - The cwd guard holds in `rm` (including with `-f`, from subdirectories, and via symlinked paths) and in `prune`. - The docs say "from the main checkout", and no out-of-scope #140 items slipped in. | # | Pri | Finding | |---|---|---| | C-1 | P3 | *"list each file with its size"*: quoted non-ASCII names and ignored symlinks produce an empty list, even though the guard still refuses. (Same root as S-2.) | | C-2 | P3 | *"C2-3's permanent policy stays open"*: `workspaces.md` declares any `lifecycle.log` "safe to discard", which pre-decides part of option 1 in #140. Soften it to "usually", or drop the claim. | --- **Summary.** Standards: 7 findings, worst **S-1** (prune points to `rm -f` on live unmerged worktrees). Spec: 2 findings, both P3; worst **C-1** (incomplete size list for quoted names and symlinks). First pass, so all findings get fixed on this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request from a commit 2026-09-26 20:45:06 +00:00

fix(wt): address code-review findings for PR #141

gabogg added 1 commit 2026-09-26 20:45:07 +00:00

fix(wt): address code-review findings for PR #141 ... 7944c7b719

- prune reports kept worktrees only once they are confirmed merged
  candidates, so its "check, then wt rm -f" hint never points at live,
  unmerged work.
- local_files reads `status --porcelain -z` with core.quotePath=false, so
  non-ASCII and spaced names are listed; the listing covers ignored
  symlinks, caps each entry at 20 files, and falls back to the raw path.
- One keep_reason() serves rm and prune; usage() documents the cwd rule.
- workspaces.md names rm/prune, drops the restated output, and softens
  the interim leftovers guidance to "usually" pending #140 C2-3.

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

gabogg commented 2026-09-26 20:45:21 +00:00

Author

Owner

Copy link

Review fixes: 7944c7b

Every finding from the review is addressed; S-4 is deliberately kept, with the reason below.

#	Resolution
S-1 (P2)	Fixed. prune now runs the keep-guards only after a worktree is confirmed as a merged candidate (upstream gone + ancestor of master). Its messages read "kept …: merged, but …", and live unmerged worktrees are skipped silently. Verified: running prune alongside the live #137/#138 worktrees prints nothing about them.
S-2 (P2) / C-1	Fixed. local_files uses git -c core.quotePath=false status --porcelain -z --ignored and is read NUL-delimited. The listing covers regular files and symlinks (find \( -type f -o -type l \)), falls back to the raw path when find gives nothing, and caps each entry at 20 files with "... and N more under X". Verified with run ñ.log, a symlink in data/, and 23 files in logs/.
S-3	Fixed. A single keep_reason() is used by both rm and prune. holds_cwd stays separate, since -f must never bypass it.
S-4	Kept on purpose. #140 C2-1 explicitly asks that forgejo-cli.md §Merging say "from the main checkout". An agent follows that doc at the moment it runs prune, right after merging from inside its PR worktree, so the clause sits where the mistake happens. The Spec requirement outranks the judgement call.
S-5	Fixed. "Run rm and prune from the main checkout".
S-6	Fixed. Dropped the restated "list those files with their sizes". "~4 KB" became "a few KB", a claim that doesn't go stale with small schema changes.
S-7	Fixed. usage() now ends with: "rm and prune never remove the worktree holding your cwd: run them from the main checkout."
C-2	Fixed. The guidance now says leftovers are usually only an empty-schema DB and lifecycle.log, and says to treat anything else as real data until #140 settles the rule. It no longer declares logs safe.

Verification: all cases were run on local throwaway branches with the branch's own script:

• rm listing: quoted name, symlink, the 20-file cap.
• prune on a merged candidate: with files (kept), dirty (kept), clean (removed), and from inside it (kept, cwd).
• prune beside live unmerged worktrees: silent.

Pre-commit (ruff, pytest) is green and check_docs.py is clean.

🤖 Generated with Claude Code

## Review fixes: 7944c7b Every finding from the [review](https://git.gaboggamer.online/gabogg/hikcentral/pulls/141#issuecomment-2560) is addressed; S-4 is deliberately kept, with the reason below. | # | Resolution | |---|---| | S-1 (P2) | **Fixed.** `prune` now runs the keep-guards only after a worktree is confirmed as a merged candidate (upstream gone + ancestor of master). Its messages read "kept …: merged, but …", and live unmerged worktrees are skipped silently. Verified: running `prune` alongside the live #137/#138 worktrees prints nothing about them. | | S-2 (P2) / C-1 | **Fixed.** `local_files` uses `git -c core.quotePath=false status --porcelain -z --ignored` and is read NUL-delimited. The listing covers regular files *and* symlinks (`find \( -type f -o -type l \)`), falls back to the raw path when `find` gives nothing, and caps each entry at 20 files with "... and N more under X". Verified with `run ñ.log`, a symlink in `data/`, and 23 files in `logs/`. | | S-3 | **Fixed.** A single `keep_reason()` is used by both `rm` and `prune`. `holds_cwd` stays separate, since `-f` must never bypass it. | | S-4 | **Kept on purpose.** #140 C2-1 explicitly asks that `forgejo-cli.md` §Merging say "from the main checkout". An agent follows that doc at the moment it runs `prune`, right after merging from inside its PR worktree, so the clause sits where the mistake happens. The Spec requirement outranks the judgement call. | | S-5 | **Fixed.** "Run `rm` and `prune` from the main checkout". | | S-6 | **Fixed.** Dropped the restated "list those files with their sizes". "~4 KB" became "a few KB", a claim that doesn't go stale with small schema changes. | | S-7 | **Fixed.** `usage()` now ends with: "rm and prune never remove the worktree holding your cwd: run them from the main checkout." | | C-2 | **Fixed.** The guidance now says leftovers are *usually* only an empty-schema DB and `lifecycle.log`, and says to treat anything else as real data until #140 settles the rule. It no longer declares logs safe. | **Verification:** all cases were run on local throwaway branches with the branch's own script: - `rm` listing: quoted name, symlink, the 20-file cap. - `prune` on a merged candidate: with files (kept), dirty (kept), clean (removed), and from inside it (kept, cwd). - `prune` beside live unmerged worktrees: silent. Pre-commit (ruff, pytest) is green and `check_docs.py` is clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg merged commit 02f0971238 into master 2026-09-26 20:47:53 +00:00

gabogg deleted branch fix/wt-safety-interim 2026-09-26 20:47:53 +00:00

gabogg referenced this pull request from a commit 2026-09-26 20:47:55 +00:00

fix(wt): interim safety guards from #140 review follow-ups (#141)

gabogg referenced this pull request 2026-09-26 20:48:16 +00:00

chore(agents): scripts/wt and workflow-doc follow-ups from #139 review #140

gabogg referenced this pull request 2026-09-26 20:48:17 +00:00

chore(agents): scripts/wt and workflow-doc follow-ups from #139 review #140

gabogg referenced this pull request 2026-09-26 21:23:50 +00:00

chore(agents): one worktree per agent, and a Forgejo CLI instead of curl/Python snippets #118

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

blocked

Waits on the issues listed in its "Blocked by:" line before implementation

  

bug

Category: something is broken; behaviour differs from the spec or docs

  

enhancement

Category: new feature or improvement, including refactors, chores and docs

  

high-priority

Address before ordinary-priority work; independent of triage readiness and blockers

  

low-priority

Address after ordinary-priority work; independent of triage readiness and blockers

  

needs-info

Waiting on reporter for more information

  

needs-triage

Maintainer needs to evaluate this issue

  

ready-for-agent

Fully specified, ready for an AFK agent

  

ready-for-human

Requires human implementation

  

referenced

Previously closed issue reopened or referenced by an open issue; visible history marker

  

research

Needs investigation (internet or local resources) before it can be triaged down; used alongside a category and a triage role

  

wontfix

Will not be actioned

No labels

blocked

bug

enhancement

high-priority

low-priority

needs-info

needs-triage

ready-for-agent

ready-for-human

referenced

research

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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

gabogg/hikcentral!141

No description provided.