From f59741872cb666173da0db8705638ec3136ff03d Mon Sep 17 00:00:00 2001 From: screenleon Date: Thu, 2 Jul 2026 12:10:31 +0900 Subject: [PATCH 01/10] docs(CC-014): broaden scope to repo-wide worktree tooling Was scoped only to --parallel PR gate reviewer isolation. Requirement now covers a general worktree create/list/clean utility for parallel multi-ticket development, with gate reviewer isolation as one downstream use case rather than the whole scope. --- BACKLOG.md | 15 +++++++++------ MILESTONES.md | 6 +++--- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index dda82ab..bd96e0d 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -13,7 +13,7 @@ CC-001/CC-002 were consumed by PR #24 fix bundle inline, with no standalone entr | CC-004 | 🟢 someday | test-pr-gate.sh docstring 格式統一 | ops | 2026-05-12 | pr:#38 | P3 | — | | CC-011 | 🟢 someday | sync-memory.sh + install 選項：symlink memory 到雲端資料夾實現跨裝置共用 | ux/memory | 2026-05-14 | — | — | — | | CC-012 | 🟢 someday | SessionStart hook：session 啟動時 pull 最新 memory（git/rsync）確保跨裝置同步 | ux/memory | 2026-05-14 | — | — | — | -| CC-014 | 🔵 active | `using-git-worktrees` skill：parallel PR gate 隔離開發環境。v0.8.0 Phase 4 | arch | 2026-05-14 | — | — | — | +| CC-014 | 🔵 active | repo 通用 worktree 平行開發工具：建立/清理 worktree + using-git-worktrees skill。v0.8.0 Phase 4 | arch | 2026-05-14 | — | — | — | | CC-015 | ⏸ deferred | `systematic-debugging` skill：結構化偵錯工作流 | ux | 2026-05-14 | — | — | — | | CC-018 | 🟢 someday | Codex quota 自動追蹤 + rate-limit 路徑統一（吸收 CC-269）：寫到 `~/.local/share/pm-dispatch/state/rate-limits.json`；解析 API response headers；token-usage.sh 加 Codex pool 顯示 | ux/token | 2026-05-14 | — | P3 | — | | CC-023 | ⏸ deferred | `coupling-reviewer`：PR gate 加入語言感知耦合分析（dependency-cruiser/gocyclo/coca） | ops/gate | 2026-05-14 | — | — | — | @@ -216,12 +216,15 @@ _Terminal_ (CC-378: swept OUT to `BACKLOG-ARCHIVE.md` by `scripts/archive-closed **Note**: 依賴 CC-011；建議與 CC-011 合入同一 PR（Phase 1 + Phase 2 同步落地，CC-012 無獨立實作意義）。 **Status note (CC-050 audit 2026-05-18)**: Downgraded from ⏸ deferred to 🟢 someday — depends on CC-011; no active plan. Re-evaluate together with CC-011. -## CC-014 — `using-git-worktrees` skill +## CC-014 — repo 通用 worktree 平行開發工具 -**Status note (v0.8.0 planning 2026-07-01)**: Re-activated (was downgraded to ⏸ deferred by the CC-050 audit 2026-05-18 for lacking an open branch) — assigned to v0.8.0 Phase 4. -**Problem**: `--parallel` PR gate 各 reviewer 在同一 working tree 執行，reviewer 寫入可能互相干擾。 -**Why**: git worktree 讓每個 subagent 在獨立環境工作，避免狀態污染，也直接補強 CC-003 的解法方向。 -**Requirement**: `commands/using-git-worktrees.md` skill，指導平行開發中使用 git worktree；評估 `--parallel` gate 是否可為每個 reviewer 建立獨立 worktree。 +**Status note (v0.8.0 planning 2026-07-02)**: Re-activated (was downgraded to ⏸ deferred by the CC-050 audit 2026-05-18 for lacking an open branch) — assigned to v0.8.0 Phase 4. Scope broadened 2026-07-02: 不再侷限於 pr-gate reviewer 隔離，改為 repo 層級通用 worktree 工具，讓任一 ticket/分支都能快速建立、切換、清理獨立 worktree 以支援多票並行開發。 +**Problem**: 目前開發者（與 `--parallel` PR gate 各 reviewer）都在同一 working tree 上工作，跨票並行開發時彼此的未 commit 變更、build 產物會互相干擾；沒有標準化的方式建立/清理獨立 worktree。 +**Why**: git worktree 讓每個工作串流（人或 subagent）在獨立環境工作，避免狀態污染；同時直接補強 CC-003 的解法方向，也可延伸解掉 `--parallel` gate 的 reviewer 隔離問題。 +**Requirement**: +1. `scripts/worktree-*.sh`（或等效 `pmctl` 子指令）：為指定 ticket/分支建立、列出、清理 worktree，統一命名慣例與清理時機（避免孤兒 worktree 殘留）。 +2. `commands/using-git-worktrees.md` skill：指導開發者（人或 dispatch executor）如何用這些工具做功能分支平行開發。 +3. 評估 `--parallel` PR gate 是否可改用同一套工具為每個 reviewer 建立獨立 worktree（原票聚焦點，現列為本票子項而非全部範圍）。 ## CC-015 — `systematic-debugging` skill diff --git a/MILESTONES.md b/MILESTONES.md index 29b6ba6..c7ddfc5 100644 --- a/MILESTONES.md +++ b/MILESTONES.md @@ -51,13 +51,13 @@ |----|------|------| | CC-381 | install host-PM-aware — 縮小為 read-only host-profile-detection / doctor 擴充切片：讓 `doctor.sh`/`pmctl doctor` 能回答「目前 host 是 claude/codex/opencode？哪些能力有 wiring？哪些只能透過 pmctl 手動使用？」不動 installer write path。前置票 CC-372/374/375/380 已全數 done，本 Phase 目標是把 CC-381 從設計陳述推進為有明確 Requirement 的實作票 | 🔵 active | -### Phase 4 — CC-014 `using-git-worktrees` skill（P3；低風險並行；與 Phase 1-3 檔案面不重疊） +### Phase 4 — CC-014 repo 通用 worktree 平行開發工具（P3；低風險並行；與 Phase 1-3 檔案面不重疊） | 票 | 摘要 | 狀態 | |----|------|------| -| CC-014 | `using-git-worktrees` skill：為平行開發（`--parallel` PR gate 各 reviewer 目前共用同一 working tree，寫入可能互相干擾）補一份指導 skill；評估 `--parallel` gate 是否可為每個 reviewer 建立獨立 git worktree | 🔵 active | +| CC-014 | repo 通用 worktree 建立/列出/清理工具 + `using-git-worktrees` skill，支援多票並行開發；並評估 `--parallel` PR gate 是否可沿用同一套工具為每個 reviewer 建立獨立 worktree | 🔵 active | -> 由 CC-050 稽核降級的 ⏸ deferred（無開放分支）重新啟用；規劃時尚未有實作分支，範圍與時程由後續 `/pre-impl` 或直接 dispatch 時再收斂。 +> 由 CC-050 稽核降級的 ⏸ deferred（無開放分支）重新啟用；2026-07-02 範圍由「pr-gate reviewer 隔離」擴大為「repo 通用 worktree 工具」；規劃時尚未有實作分支，範圍與時程由後續 `/pre-impl` 或直接 dispatch 時再收斂。 ### 待後續 / 明確排除 From 61d1a456403cf6ac74ee433ed2f7b1beefef7ca6 Mon Sep 17 00:00:00 2001 From: screenleon Date: Thu, 2 Jul 2026 13:34:30 +0900 Subject: [PATCH 02/10] feat(CC-014): pmctl worktree create/list/remove/gc + using-git-worktrees skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a repo-wide git worktree registry so multiple branches/tickets can be worked on in parallel without interfering with each other's uncommitted changes. Wraps `git worktree add/remove/prune` with an out-of-repo JSONL manifest (state store, keyed by main-repo identity). Identity seam: manifest partitioning uses a new _sw_main_repo_root / _sw_worktree_project_key pair in state-paths.sh, resolved via `git rev-parse --git-common-dir` instead of `--show-toplevel` — this makes `pmctl worktree list/gc` return the same result whether invoked from the primary checkout or from inside a linked worktree it created. Verified manually before building the rest of the feature on top of it. Existing _sw_project_key/dispatch/gate partitioning is untouched. `--parallel` PR gate reviewer isolation is intentionally out of scope here (tracked as a separate follow-up) — this PR only adds the general-purpose worktree utility and its skill doc. --- .github/workflows/lint.yml | 8 + cli/pmctl | 26 +- commands/using-git-worktrees.md | 60 +++++ scripts/lib/pmctl-worktree.sh | 382 ++++++++++++++++++++++++++++ scripts/lib/state-paths.sh | 75 ++++++ scripts/test-commands.sh | 16 ++ scripts/test-pmctl-worktree.sh | 425 ++++++++++++++++++++++++++++++++ 7 files changed, 991 insertions(+), 1 deletion(-) create mode 100644 commands/using-git-worktrees.md create mode 100644 scripts/lib/pmctl-worktree.sh create mode 100755 scripts/test-pmctl-worktree.sh diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index cf9119b..1b6217a 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -193,6 +193,7 @@ jobs: test-pmctl-safe.sh test-pmctl-validate.sh test-pmctl-decision.sh + test-pmctl-worktree.sh test-doctor: runs-on: ubuntu-latest @@ -419,6 +420,13 @@ jobs: - name: pmctl validate brief regression suite run: bash scripts/test-pmctl-validate.sh + test-pmctl-worktree: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: pmctl worktree regression suite + run: bash scripts/test-pmctl-worktree.sh + lint-backlog: runs-on: ubuntu-latest steps: diff --git a/cli/pmctl b/cli/pmctl index 40899ee..80a9f91 100755 --- a/cli/pmctl +++ b/cli/pmctl @@ -16,7 +16,7 @@ done REPO_ROOT="$(cd "$(dirname "$_pmctl_self")/.." && pwd)" unset _pmctl_self _pmctl_dir -for _lib in detached-launch pmctl-policy pmctl-fs pmctl-adapter pmctl-backlog pmctl-guard executor-router pmctl-dispatch pmctl-trace pmctl-task pmctl-decision gate-result-verify pmctl-gate pmctl-safe pmctl-validate pmctl-context pmctl-memory pmctl-artifacts pmctl-pre-release; do +for _lib in detached-launch pmctl-policy pmctl-fs pmctl-adapter pmctl-backlog pmctl-guard executor-router pmctl-dispatch pmctl-trace pmctl-task pmctl-decision gate-result-verify pmctl-gate pmctl-safe pmctl-validate pmctl-context pmctl-memory pmctl-artifacts pmctl-pre-release pmctl-worktree; do # shellcheck source=/dev/null [[ -r "$REPO_ROOT/scripts/lib/$_lib.sh" ]] && . "$REPO_ROOT/scripts/lib/$_lib.sh" done @@ -99,6 +99,30 @@ case "$cmd/$sub" in fi pmctl_artifacts_migrate "$REPO_ROOT" "" "$@" ;; + worktree/create) + if ! declare -F pmctl_worktree_create >/dev/null; then + pmctl_die "worktree create unavailable" + fi + pmctl_worktree_create "$REPO_ROOT" "" "$@" + ;; + worktree/list) + if ! declare -F pmctl_worktree_list >/dev/null; then + pmctl_die "worktree list unavailable" + fi + pmctl_worktree_list "$REPO_ROOT" "" "$@" + ;; + worktree/remove) + if ! declare -F pmctl_worktree_remove >/dev/null; then + pmctl_die "worktree remove unavailable" + fi + pmctl_worktree_remove "$REPO_ROOT" "" "$@" + ;; + worktree/gc) + if ! declare -F pmctl_worktree_gc >/dev/null; then + pmctl_die "worktree gc unavailable" + fi + pmctl_worktree_gc "$REPO_ROOT" "" "$@" + ;; trace/tail) if ! declare -F pmctl_trace_tail >/dev/null; then pmctl_die "trace tail unavailable" diff --git a/commands/using-git-worktrees.md b/commands/using-git-worktrees.md new file mode 100644 index 0000000..d30bcf5 --- /dev/null +++ b/commands/using-git-worktrees.md @@ -0,0 +1,60 @@ +--- +description: Use a dedicated git worktree per ticket/branch for parallel development — isolates uncommitted changes and build artifacts so multiple branches can be worked on at once without stepping on each other. +argument-hint: "[branch-name]" +--- + +Set up an isolated `git worktree` for parallel development using `pmctl worktree`, instead of switching branches in place or cloning the repo again. + +## What + +`pmctl worktree` wraps `git worktree` with a small out-of-repo registry (manifest) so linked worktrees are easy to list and clean up later, without hand-tracking which directory belongs to which branch. + +**Prerequisite: this requires git.** `pmctl worktree` is a thin wrapper over `git worktree add/remove/prune` — there is no non-git fallback. The target directory must already be a git repository; `pmctl worktree create` fails immediately (git itself rejects the operation) if it is not. + +Worktrees are stored out-of-repo, under the state store (`~/.local/share/pm-dispatch/state/projects//worktrees/checkouts/`), not inside the repo — so they never show up in `git status`, don't need a `.gitignore` entry, and survive `git clean`. The registry (manifest) resolves to the **same partition** whether `pmctl worktree` is invoked from the primary checkout or from inside one of the linked worktrees it created, so `pmctl worktree list` always shows the full picture regardless of which checkout you run it from. + +## When to use + +- You're mid-way through one ticket and need to start a second one without stashing/committing WIP just to switch branches. +- You want to run a long build/test in one branch's worktree while continuing to edit another branch. +- You need a disposable checkout of a branch (e.g. to inspect an old PR) without disturbing your primary working tree. + +## Example + +```sh +pmctl worktree create feat/my-feature +pmctl worktree list +pmctl worktree remove feat/my-feature +``` + +## Usage + +``` +pmctl worktree create [--from ] [--name ] [--cd ] +pmctl worktree list [--cd ] [--json] +pmctl worktree remove [--force] [--cd ] +pmctl worktree gc [--dry-run] [--merged] [--max-age-days D] [--cd ] +``` + +- `create ` — attaches an existing local branch, or creates a new one off the current `HEAD` if it doesn't exist yet. Pass `--from ` to create the new branch off a specific base instead of `HEAD`. Prints the new worktree's absolute path on success — capture it if you need to `cd` into it. +- `create --name ` — override the manifest slug (defaults to the branch name with `/` replaced by `-`). Two worktrees cannot share a slug; `create` fails rather than silently overwriting an existing one. +- `list` — table of registered worktrees (`SLUG`, `BRANCH`, `PATH`). Add `--json` for a machine-readable array (each entry: `slug`, `branch`, `path`, `created_ts`). +- `remove ` — matches by slug or by branch name. Fails if the worktree has uncommitted changes; pass `--force` to discard them and remove anyway. **`--force` is destructive** — it discards uncommitted work in that worktree with no recovery path, so confirm you don't need those changes before passing it. +- `gc` — reconciles the manifest against actual git/filesystem state: drops entries whose directory was removed manually (e.g. `rm -rf`) or that git no longer tracks. Add `--merged` to also remove worktrees whose branch is fully merged, or `--max-age-days N` to remove entries older than N days. Always run with `--dry-run` first to see what would be removed before running for real. + +All four subcommands accept `--cd ` to target a repo other than the current directory (same convention as `pmctl artifacts`/`pmctl dispatch`). + +## Typical workflow + +1. `pmctl worktree create feat/my-feature` — creates the worktree and prints its path. +2. `cd ` and work there like a normal checkout: `git add`, `git commit`, push, open a PR — all git operations behave exactly as in a regular checkout, because a linked worktree shares the same object store and refs as the primary checkout. +3. When the ticket's PR is merged (or abandoned), `pmctl worktree remove feat/my-feature` from either the primary checkout or from inside the worktree itself. +4. Periodically run `pmctl worktree gc --dry-run --merged` to spot worktrees left behind for already-merged branches, then `pmctl worktree gc --merged` to clean them up. + +## Cleanup and orphan recovery + +If a worktree directory is deleted directly (`rm -rf` instead of `pmctl worktree remove`), git and the manifest both still reference it. Run `pmctl worktree gc` — it detects the missing path, removes the stale manifest entry, and runs `git worktree prune` so `git worktree list` stays in sync too. `gc` never force-deletes a directory that still exists and looks live; it only prunes entries that are already gone, already merged (`--merged`), or past the age threshold (`--max-age-days`). + +## Out of scope + +This tool does not touch the `--parallel` PR gate's reviewer-isolation logic (`scripts/pr-gate.sh`) — that is a separate integration tracked independently. `pmctl worktree` is a general-purpose utility for any parallel branch work, not specific to the gate. diff --git a/scripts/lib/pmctl-worktree.sh b/scripts/lib/pmctl-worktree.sh new file mode 100644 index 0000000..a4ed080 --- /dev/null +++ b/scripts/lib/pmctl-worktree.sh @@ -0,0 +1,382 @@ +#!/usr/bin/env bash +# pmctl worktree create/list/remove/gc -- repo-wide git worktree registry for +# parallel multi-ticket development. Manifest is stored out-of-repo in the +# state store, keyed by the MAIN repo identity (sw_project_worktree_dir / +# state-paths.sh:_sw_worktree_project_key) so a linked worktree and its +# primary checkout resolve to the SAME partition regardless of which one the +# command is invoked from. + +pmctl_worktree_usage() { + printf 'usage: pmctl worktree create [--from ] [--name ] [--cd ]\n' >&2 + printf ' pmctl worktree list [--cd ] [--json]\n' >&2 + printf ' pmctl worktree remove [--force] [--cd ]\n' >&2 + printf ' pmctl worktree gc [--dry-run] [--merged] [--max-age-days D] [--cd ]\n' >&2 +} + +pmctl_worktree_ensure_state_paths() { + local repo_root="${1:-}" + if [[ "$(type -t sw_project_worktree_dir 2>/dev/null)" != function ]]; then + local _sp_lib="${repo_root:-}/scripts/lib/state-paths.sh" + if [[ -r "$_sp_lib" ]]; then + # shellcheck disable=SC1090,SC1091 # dynamic repo-root path. + . "$_sp_lib" 2>/dev/null || true + fi + fi + if [[ "$(type -t sw_project_worktree_dir 2>/dev/null)" != function ]]; then + printf 'pmctl worktree: state-paths.sh unavailable; cannot resolve worktree registry dir\n' >&2 + return 2 + fi +} + +pmctl_worktree_ensure_writer() { + local repo_root="${1:-}" + if [[ "$(type -t _sw_compact_json_line 2>/dev/null)" != function || "$(type -t serialize_with_lock 2>/dev/null)" != function ]]; then + local _sw_lib="${repo_root:-}/scripts/lib/state-writer.sh" + if [[ -r "$_sw_lib" ]]; then + # shellcheck disable=SC1090,SC1091 # dynamic repo-root path. + . "$_sw_lib" 2>/dev/null || true + fi + fi + if [[ "$(type -t _sw_compact_json_line 2>/dev/null)" != function || "$(type -t serialize_with_lock 2>/dev/null)" != function ]]; then + printf 'pmctl worktree: state-writer.sh unavailable; cannot write worktree manifest\n' >&2 + return 2 + fi +} + +# _pmctl_worktree_slugify +# Normalize a branch name into a filesystem-safe, single-path-segment slug: +# `/` -> `-`, then strip everything outside [A-Za-z0-9._-]. Rejects a branch +# that slugifies to empty or that still contains a path-escape sequence. +_pmctl_worktree_slugify() { + local branch="${1:-}" slug + slug="${branch//\//-}" + slug="$(printf '%s' "$slug" | tr -c 'A-Za-z0-9._-' '-')" + if [[ -z "$slug" || "$slug" == *..* || "$slug" == .* ]]; then + printf 'pmctl worktree: branch name does not produce a safe slug: %s\n' "$branch" >&2 + return 1 + fi + printf '%s\n' "$slug" +} + +pmctl_worktree_manifest_path() { + local reg_dir + reg_dir="$(sw_project_worktree_dir)" || return 1 + printf '%s/manifest.jsonl\n' "$reg_dir" +} + +_pmctl_worktree_manifest_append_inner() { + local json_line="$1" compact manifest="$2" + compact="$(_sw_compact_json_line "$json_line")" || return $? + printf '%s\n' "$compact" >> "$manifest" +} + +pmctl_worktree_manifest_append() { + local json_line="$1" reg_dir manifest rc=0 + reg_dir="$(sw_project_worktree_dir)" || return 1 + mkdir -p "$reg_dir" 2>/dev/null || { printf 'pmctl worktree: mkdir failed: %s\n' "$reg_dir" >&2; return 1; } + chmod 0700 "$reg_dir" 2>/dev/null || true + manifest="$reg_dir/manifest.jsonl" + serialize_with_lock "$reg_dir/manifest" _pmctl_worktree_manifest_append_inner "$json_line" "$manifest" || rc=$? + return "$rc" +} + +# _pmctl_worktree_manifest_rewrite_inner +# Atomically replace manifest.jsonl with tmp_content_file's contents, run +# under the same lock as append so remove/gc never race a concurrent create. +_pmctl_worktree_manifest_rewrite_inner() { + local manifest="$1" tmp_content="$2" + mv -f "$tmp_content" "$manifest" +} + +pmctl_worktree_manifest_rewrite() { + local new_content="$1" reg_dir manifest tmp rc=0 + reg_dir="$(sw_project_worktree_dir)" || return 1 + manifest="$reg_dir/manifest.jsonl" + tmp="$(mktemp "$reg_dir/.manifest.XXXXXX")" || return 1 + printf '%s' "$new_content" > "$tmp" + serialize_with_lock "$reg_dir/manifest" _pmctl_worktree_manifest_rewrite_inner "$manifest" "$tmp" || rc=$? + [[ -f "$tmp" ]] && rm -f "$tmp" + return "$rc" +} + +pmctl_worktree_manifest_read() { + local manifest + manifest="$(pmctl_worktree_manifest_path)" || return 1 + [[ -f "$manifest" ]] && cat "$manifest" + return 0 +} + +pmctl_worktree_create() { + local repo_root="${1:-}" work_dir="${2:-}" branch="" base="" name="" args=() + shift 2 || true + [[ -n "$work_dir" ]] || work_dir="$repo_root" + args=("$@") + local i=0 rest=() + while [[ $i -lt ${#args[@]} ]]; do + case "${args[$i]}" in + --from) + base="${args[$((i+1))]:-}" + [[ -n "$base" ]] || { printf 'pmctl worktree create: --from requires a branch\n' >&2; return 2; } + i=$((i+2)) + ;; + --name) + name="${args[$((i+1))]:-}" + [[ -n "$name" ]] || { printf 'pmctl worktree create: --name requires a slug\n' >&2; return 2; } + i=$((i+2)) + ;; + --cd) + work_dir="${args[$((i+1))]:-}" + i=$((i+2)) + ;; + -h|--help) + pmctl_worktree_usage + return 0 + ;; + *) + rest+=("${args[$i]}") + i=$((i+1)) + ;; + esac + done + branch="${rest[0]:-}" + if [[ -z "$branch" ]]; then + printf 'pmctl worktree create: is required\n' >&2 + pmctl_worktree_usage + return 2 + fi + [[ -n "$work_dir" ]] || work_dir="$repo_root" + pmctl_worktree_ensure_state_paths "$repo_root" || return $? + pmctl_worktree_ensure_writer "$repo_root" || return $? + + local slug + slug="$(_pmctl_worktree_slugify "${name:-$branch}")" || return 1 + + local reg_dir wt_path + reg_dir="$(cd "$work_dir" 2>/dev/null && sw_project_worktree_dir)" || { + printf 'pmctl worktree create: cannot resolve worktree registry dir from %s\n' "$work_dir" >&2 + return 1 + } + wt_path="$reg_dir/checkouts/$slug" + + if [[ -e "$wt_path" ]]; then + printf 'pmctl worktree create: a worktree already exists at %s (slug %s in use)\n' "$wt_path" "$slug" >&2 + return 1 + fi + if (cd "$work_dir" && git worktree list --porcelain 2>/dev/null | grep -q "^worktree $wt_path\$"); then + printf 'pmctl worktree create: git already tracks a worktree at %s\n' "$wt_path" >&2 + return 1 + fi + + mkdir -p "$reg_dir/checkouts" 2>/dev/null || true + + local git_args=(worktree add) + if [[ -n "$base" ]]; then + git_args+=(-b "$branch" "$wt_path" "$base") + elif (cd "$work_dir" && git show-ref --verify --quiet "refs/heads/$branch" 2>/dev/null); then + git_args+=("$wt_path" "$branch") + else + git_args+=(-b "$branch" "$wt_path") + fi + + if ! (cd "$work_dir" && git "${git_args[@]}"); then + printf 'pmctl worktree create: git worktree add failed\n' >&2 + return 1 + fi + + local created_ts json_line + created_ts="$(date -Is 2>/dev/null || date)" + json_line="$(printf '{"slug":%s,"branch":%s,"path":%s,"created_ts":%s}' \ + "$(jq -Rn --arg v "$slug" '$v')" \ + "$(jq -Rn --arg v "$branch" '$v')" \ + "$(jq -Rn --arg v "$wt_path" '$v')" \ + "$(jq -Rn --arg v "$created_ts" '$v')")" + (cd "$work_dir" && pmctl_worktree_manifest_append "$json_line") || { + printf 'pmctl worktree create: worktree created but manifest write failed -- run '\''pmctl worktree gc'\'' to reconcile\n' >&2 + } + printf '%s\n' "$wt_path" +} + +pmctl_worktree_list() { + local repo_root="${1:-}" work_dir json_out=0 args=() + shift || true + work_dir="${1:-$repo_root}" + shift || true + args=("$@") + local i=0 + while [[ $i -lt ${#args[@]} ]]; do + case "${args[$i]}" in + --cd) work_dir="${args[$((i+1))]:-}"; i=$((i+2)) ;; + --json) json_out=1; i=$((i+1)) ;; + -h|--help) pmctl_worktree_usage; return 0 ;; + *) i=$((i+1)) ;; + esac + done + [[ -n "$work_dir" ]] || work_dir="$repo_root" + pmctl_worktree_ensure_state_paths "$repo_root" || return $? + + local manifest_content + manifest_content="$(cd "$work_dir" 2>/dev/null && pmctl_worktree_manifest_read)" || true + + if [[ "$json_out" -eq 1 ]]; then + if [[ -z "$manifest_content" ]]; then + printf '[]\n' + else + printf '%s\n' "$manifest_content" | jq -s -c . + fi + return 0 + fi + + if [[ -z "$manifest_content" ]]; then + printf 'No registered worktrees.\n' + return 0 + fi + printf '%-30s %-30s %s\n' SLUG BRANCH PATH + printf '%s\n' "$manifest_content" | while IFS= read -r line; do + [[ -n "$line" ]] || continue + jq -r '[.slug, .branch, .path] | @tsv' <<<"$line" | while IFS=$'\t' read -r slug branch path; do + printf '%-30s %-30s %s\n' "$slug" "$branch" "$path" + done + done +} + +pmctl_worktree_remove() { + local repo_root="${1:-}" work_dir target force=0 args=() + shift || true + work_dir="${1:-$repo_root}" + shift || true + args=("$@") + local i=0 rest=() + while [[ $i -lt ${#args[@]} ]]; do + case "${args[$i]}" in + --force) force=1; i=$((i+1)) ;; + --cd) work_dir="${args[$((i+1))]:-}"; i=$((i+2)) ;; + -h|--help) pmctl_worktree_usage; return 0 ;; + *) rest+=("${args[$i]}"); i=$((i+1)) ;; + esac + done + target="${rest[0]:-}" + if [[ -z "$target" ]]; then + printf 'pmctl worktree remove: is required\n' >&2 + pmctl_worktree_usage + return 2 + fi + [[ -n "$work_dir" ]] || work_dir="$repo_root" + pmctl_worktree_ensure_state_paths "$repo_root" || return $? + pmctl_worktree_ensure_writer "$repo_root" || return $? + + local manifest_content match_line match_path + manifest_content="$(cd "$work_dir" 2>/dev/null && pmctl_worktree_manifest_read)" || true + match_line="$(printf '%s\n' "$manifest_content" | jq -c --arg t "$target" 'select(.slug == $t or .branch == $t)' | head -1)" + if [[ -z "$match_line" ]]; then + printf 'pmctl worktree remove: no registered worktree matches %s\n' "$target" >&2 + return 1 + fi + match_path="$(jq -r '.path' <<<"$match_line")" + + local git_rm_args=(worktree remove) + [[ "$force" -eq 1 ]] && git_rm_args+=(--force) + git_rm_args+=("$match_path") + if [[ -d "$match_path" ]]; then + if ! (cd "$work_dir" && git "${git_rm_args[@]}"); then + printf 'pmctl worktree remove: git worktree remove failed for %s (dirty? pass --force to override)\n' "$match_path" >&2 + return 1 + fi + fi + (cd "$work_dir" && git worktree prune) 2>/dev/null || true + + local remaining + remaining="$(printf '%s\n' "$manifest_content" | jq -c --arg t "$target" 'select(.slug != $t and .branch != $t)')" + (cd "$work_dir" && pmctl_worktree_manifest_rewrite "$remaining") || { + printf 'pmctl worktree remove: worktree removed but manifest cleanup failed -- run '\''pmctl worktree gc'\'' to reconcile\n' >&2 + } + printf 'removed %s (%s)\n' "$target" "$match_path" +} + +pmctl_worktree_gc() { + local repo_root="${1:-}" work_dir dry_run=0 merged_only=0 max_age_days=0 args=() + shift || true + work_dir="${1:-$repo_root}" + shift || true + args=("$@") + local i=0 + while [[ $i -lt ${#args[@]} ]]; do + case "${args[$i]}" in + --dry-run) dry_run=1; i=$((i+1)) ;; + --merged) merged_only=1; i=$((i+1)) ;; + --max-age-days) + max_age_days="${args[$((i+1))]:-0}" + if ! [[ "$max_age_days" =~ ^[0-9]+$ ]]; then + printf 'pmctl worktree gc: --max-age-days requires an integer >= 0\n' >&2 + return 2 + fi + i=$((i+2)) + ;; + --cd) work_dir="${args[$((i+1))]:-}"; i=$((i+2)) ;; + -h|--help) pmctl_worktree_usage; return 0 ;; + *) i=$((i+1)) ;; + esac + done + [[ -n "$work_dir" ]] || work_dir="$repo_root" + pmctl_worktree_ensure_state_paths "$repo_root" || return $? + pmctl_worktree_ensure_writer "$repo_root" || return $? + + local manifest_content now_epoch max_age_seconds + manifest_content="$(cd "$work_dir" 2>/dev/null && pmctl_worktree_manifest_read)" || true + [[ -n "$manifest_content" ]] || { printf 'gc: no registered worktrees\n'; return 0; } + now_epoch="$(date +%s 2>/dev/null || echo 0)" + max_age_seconds=$(( max_age_days * 86400 )) + + local kept_lines="" removed_count=0 + while IFS= read -r line; do + [[ -n "$line" ]] || continue + local slug branch path created_ts age_seconds should_remove=0 reason="" + slug="$(jq -r '.slug' <<<"$line")" + branch="$(jq -r '.branch' <<<"$line")" + path="$(jq -r '.path' <<<"$line")" + created_ts="$(jq -r '.created_ts' <<<"$line")" + + if [[ ! -d "$path" ]]; then + should_remove=1; reason="path missing (orphaned manifest entry)" + elif ! (cd "$work_dir" && git worktree list --porcelain 2>/dev/null | grep -q "^worktree $path\$"); then + should_remove=1; reason="git no longer tracks this worktree" + elif [[ "$merged_only" -eq 1 ]] && (cd "$work_dir" && git branch --merged 2>/dev/null | grep -qE "^[*+ ]+$branch\$"); then + should_remove=1; reason="branch merged" + elif [[ "$max_age_days" -gt 0 ]]; then + local created_epoch + created_epoch="$(date -d "$created_ts" +%s 2>/dev/null || date -jf '%Y-%m-%dT%H:%M:%S' "${created_ts%%[+-]*}" +%s 2>/dev/null || echo "$now_epoch")" + age_seconds=$(( now_epoch - created_epoch )) + if [[ "$age_seconds" -ge "$max_age_seconds" ]]; then + should_remove=1; reason="older than $max_age_days day(s)" + fi + fi + + if [[ "$should_remove" -eq 1 ]]; then + removed_count=$((removed_count+1)) + if [[ "$dry_run" -eq 1 ]]; then + printf 'would remove %s (%s): %s\n' "$slug" "$path" "$reason" + else + printf 'removing %s (%s): %s\n' "$slug" "$path" "$reason" + if [[ -d "$path" ]]; then + (cd "$work_dir" && git worktree remove --force "$path") 2>/dev/null || true + fi + fi + else + kept_lines="${kept_lines}${line}"$'\n' + fi + done <<<"$manifest_content" + + (cd "$work_dir" && git worktree prune) 2>/dev/null || true + + if [[ "$dry_run" -eq 0 ]]; then + (cd "$work_dir" && pmctl_worktree_manifest_rewrite "$kept_lines") || { + printf 'pmctl worktree gc: manifest rewrite failed after removal\n' >&2 + return 1 + } + fi + + if [[ "$dry_run" -eq 1 ]]; then + printf 'gc: dry-run, would remove %d worktree(s)\n' "$removed_count" + else + printf 'gc: removed %d worktree(s)\n' "$removed_count" + fi +} diff --git a/scripts/lib/state-paths.sh b/scripts/lib/state-paths.sh index dc25b36..adc9deb 100644 --- a/scripts/lib/state-paths.sh +++ b/scripts/lib/state-paths.sh @@ -95,6 +95,81 @@ _sw_project_dir() { return 0 } +# _sw_main_repo_root [repo_root] +# Resolve the PRIMARY (non-worktree) checkout root, so callers get the same +# identity whether invoked from the main checkout or from inside a linked +# `git worktree`. `git rev-parse --show-toplevel` (used by _sw_project_key) +# returns the linked worktree's OWN path when run inside one, which would +# split one project's state across partitions depending on caller cwd. +# `--git-common-dir` instead always resolves to the primary worktree's `.git`: +# an absolute path when run from inside a linked worktree, or the relative +# string ".git" when run from the primary worktree itself (git only prints +# it relative to cwd there, since git-common-dir == git-dir in that case). +_sw_main_repo_root() { + { + local repo_root="${1:-}" common_dir + if [[ -n "$repo_root" ]]; then + common_dir="$(git -C "$repo_root" rev-parse --git-common-dir 2>/dev/null)" + else + common_dir="$(git rev-parse --git-common-dir 2>/dev/null)" + fi + [[ -n "$common_dir" ]] || return 1 + if [[ "$common_dir" == /* ]]; then + dirname "$common_dir" + elif [[ -n "$repo_root" ]]; then + git -C "$repo_root" rev-parse --show-toplevel 2>/dev/null + else + git rev-parse --show-toplevel 2>/dev/null + fi + } 2>/dev/null || true + return 0 +} + +# _sw_worktree_project_key +# Same hashing scheme as _sw_project_key, but keyed off _sw_main_repo_root +# instead of --show-toplevel, so a linked worktree and its primary checkout +# resolve to the SAME partition. Used only by sw_project_worktree_dir -- +# existing _sw_project_key/_sw_project_dir/sw_project_run_dir callers +# (dispatch, gate, artifacts) are intentionally left untouched by this +# helper; see docs/architecture/v0.4.0-state-first-foundation.md A5 for the +# broader identity-reconciliation gap this does NOT attempt to fix. +_sw_worktree_project_key() { + { + local main_root project_key + if [[ -n "${_SW_REPO_ROOT:-}" ]]; then + main_root="$(_sw_main_repo_root "${_SW_REPO_ROOT}")" + else + main_root="$(_sw_main_repo_root)" + fi + if [[ -z "$main_root" ]]; then + printf 'global\n' + return 0 + fi + main_root="$(_portable_canonical_path "$main_root")" + if ! project_key="$(printf '%s\n' "$main_root" | _portable_sha1 2>/dev/null)"; then + _sw_log_error "_sw_worktree_project_key: failed to hash main repo root; falling back to global: $main_root" + project_key="" + fi + if [[ -n "$project_key" ]]; then + printf '%s\n' "$project_key" + else + printf 'global\n' + fi + } 2>/dev/null || true + return 0 +} + +# sw_project_worktree_dir +# Print the absolute worktree-registry directory for the current project's +# MAIN repo partition (stable whether invoked from the main checkout or from +# inside a linked worktree pmctl created): +# /projects//worktrees +# Pure computation -- does NOT create the directory; pmctl-worktree.sh owns +# mkdir + manifest writes. +sw_project_worktree_dir() { + printf '%s/projects/%s/worktrees\n' "$(_sw_store_root)" "$(_sw_worktree_project_key)" +} + # sw_project_run_dir # Print the absolute run-artifact directory for the current project partition: # /projects//runs/ diff --git a/scripts/test-commands.sh b/scripts/test-commands.sh index d30570d..d6434d8 100755 --- a/scripts/test-commands.sh +++ b/scripts/test-commands.sh @@ -352,6 +352,22 @@ should_run "pre-release: Layer 2 section is informational only" && assert_file_c should_run "pre-release: documents Layer 3 blind spots" && assert_file_contains "pre-release: documents Layer 3 blind spots" "$PRE_RELEASE" "Layer 3" && pass "pre-release: documents Layer 3 blind spots" should_run "pre-release: documents exit codes" && assert_file_contains "pre-release: documents exit codes" "$PRE_RELEASE" "Exit codes" && pass "pre-release: documents exit codes" +# ── using-git-worktrees.md contract ───────────────────────────────────────── + +USING_GIT_WORKTREES="$COMMANDS_DIR/using-git-worktrees.md" + +assert_frontmatter "using-git-worktrees: frontmatter valid" "$USING_GIT_WORKTREES" +should_run "using-git-worktrees: states git is a hard prerequisite" && assert_file_contains "using-git-worktrees: states git is a hard prerequisite" "$USING_GIT_WORKTREES" "this requires git" && pass "using-git-worktrees: states git is a hard prerequisite" +should_run "using-git-worktrees: documents create subcommand" && assert_file_contains "using-git-worktrees: documents create subcommand" "$USING_GIT_WORKTREES" "pmctl worktree create" && pass "using-git-worktrees: documents create subcommand" +should_run "using-git-worktrees: documents list subcommand" && assert_file_contains "using-git-worktrees: documents list subcommand" "$USING_GIT_WORKTREES" "pmctl worktree list" && pass "using-git-worktrees: documents list subcommand" +should_run "using-git-worktrees: documents remove subcommand" && assert_file_contains "using-git-worktrees: documents remove subcommand" "$USING_GIT_WORKTREES" "pmctl worktree remove" && pass "using-git-worktrees: documents remove subcommand" +should_run "using-git-worktrees: documents gc subcommand" && assert_file_contains "using-git-worktrees: documents gc subcommand" "$USING_GIT_WORKTREES" "pmctl worktree gc" && pass "using-git-worktrees: documents gc subcommand" +should_run "using-git-worktrees: documents --force is destructive" && assert_file_contains "using-git-worktrees: documents --force is destructive" "$USING_GIT_WORKTREES" "destructive" && pass "using-git-worktrees: documents --force is destructive" +should_run "using-git-worktrees: documents cross-worktree identity guarantee" && assert_file_contains "using-git-worktrees: documents cross-worktree identity guarantee" "$USING_GIT_WORKTREES" "same partition" && pass "using-git-worktrees: documents cross-worktree identity guarantee" +should_run "using-git-worktrees: documents orphan recovery via gc" && assert_file_contains "using-git-worktrees: documents orphan recovery via gc" "$USING_GIT_WORKTREES" "git worktree prune" && pass "using-git-worktrees: documents orphan recovery via gc" +should_run "using-git-worktrees: excludes --parallel gate reviewer isolation from scope" && assert_file_contains "using-git-worktrees: excludes --parallel gate reviewer isolation from scope" "$USING_GIT_WORKTREES" "does not touch the \`--parallel\` PR gate" && pass "using-git-worktrees: excludes --parallel gate reviewer isolation from scope" +should_run "using-git-worktrees: no CC ticket references" && assert_not_contains "using-git-worktrees: no CC ticket references" "$USING_GIT_WORKTREES" "CC-" + # ── summary ────────────────────────────────────────────────────────────────── th_summary diff --git a/scripts/test-pmctl-worktree.sh b/scripts/test-pmctl-worktree.sh new file mode 100755 index 0000000..970d358 --- /dev/null +++ b/scripts/test-pmctl-worktree.sh @@ -0,0 +1,425 @@ +#!/usr/bin/env bash +# Regression tests for `pmctl worktree create/list/remove/gc`. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +PMCTL="$REPO_ROOT/cli/pmctl" + +# shellcheck source=scripts/lib/test-harness.sh +. "$SCRIPT_DIR/lib/test-harness.sh" +th_init "$@" + +make_work_repo() { + local path="$1" + mkdir -p "$path" + git init -q "$path" + git -C "$path" config user.email test@example.com + git -C "$path" config user.name test + printf 'seed\n' > "$path/seed.txt" + git -C "$path" add seed.txt + git -C "$path" commit -q -m seed +} + +wt_list_json() { + local store="$1" work="$2" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree list --json --cd "$work" +} + +case_create_requires_branch() { + local name="worktree create: missing exits 2 with usage" + should_run "$name" || return 0 + local store work out err status=0 + store="$tmp_root/state-create-noarg" + work="$tmp_root/work-create-noarg" + make_work_repo "$work" + out="$tmp_root/c1.out"; err="$tmp_root/c1.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create --cd "$work" > "$out" 2> "$err" || status=$? + if [[ "$status" -eq 2 && "$(<"$err")" == *" is required"* && "$(<"$err")" == *"usage:"* ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + +case_create_new_branch() { + local name="worktree create: new branch creates worktree + manifest entry" + should_run "$name" || return 0 + local store work out err status=0 wt_path + store="$tmp_root/state-create-new" + work="$tmp_root/work-create-new" + make_work_repo "$work" + out="$tmp_root/c2.out"; err="$tmp_root/c2.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/x --cd "$work" > "$out" 2> "$err" || status=$? + wt_path="$(tail -1 "$out")" + if [[ "$status" -eq 0 && -d "$wt_path" ]] \ + && [[ "$(wt_list_json "$store" "$work" | jq -r '.[0].branch')" == "feat/x" ]]; then + pass "$name" + else + fail "$name" "status=$status out=$(<"$out") err=$(<"$err")" + fi +} + +case_create_from_base() { + local name="worktree create: --from creates a new branch off the given base" + should_run "$name" || return 0 + local store work out err status=0 wt_path base_sha branch_sha + store="$tmp_root/state-create-from" + work="$tmp_root/work-create-from" + make_work_repo "$work" + git -C "$work" branch base-branch + out="$tmp_root/c3.out"; err="$tmp_root/c3.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/from-base --from base-branch --cd "$work" > "$out" 2> "$err" || status=$? + wt_path="$(tail -1 "$out")" + base_sha="$(git -C "$work" rev-parse base-branch)" + branch_sha="$(git -C "$wt_path" rev-parse HEAD 2>/dev/null || true)" + if [[ "$status" -eq 0 && "$branch_sha" == "$base_sha" ]]; then + pass "$name" + else + fail "$name" "status=$status base_sha=$base_sha branch_sha=$branch_sha err=$(<"$err")" + fi +} + +case_create_existing_branch_no_new_ref() { + local name="worktree create: existing branch attaches without creating a duplicate ref" + should_run "$name" || return 0 + local store work out err status=0 + store="$tmp_root/state-create-existing" + work="$tmp_root/work-create-existing" + make_work_repo "$work" + git -C "$work" branch existing-branch + out="$tmp_root/c4.out"; err="$tmp_root/c4.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create existing-branch --cd "$work" > "$out" 2> "$err" || status=$? + if [[ "$status" -eq 0 && "$(<"$err")" != *"already exists"* ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + +case_create_name_override_slug() { + local name="worktree create: --name overrides the manifest slug" + should_run "$name" || return 0 + local store work out err status=0 + store="$tmp_root/state-create-name" + work="$tmp_root/work-create-name" + make_work_repo "$work" + out="$tmp_root/c5.out"; err="$tmp_root/c5.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/named --name custom-slug --cd "$work" > "$out" 2> "$err" || status=$? + if [[ "$status" -eq 0 && "$(wt_list_json "$store" "$work" | jq -r '.[0].slug')" == "custom-slug" ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + +case_create_duplicate_slug_rejected() { + local name="worktree create: duplicate slug is rejected, no duplicate manifest entry" + should_run "$name" || return 0 + local store work err1 err2 status1=0 status2=0 + store="$tmp_root/state-create-dup" + work="$tmp_root/work-create-dup" + make_work_repo "$work" + err1="$tmp_root/c6a.err"; err2="$tmp_root/c6b.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/dup --cd "$work" > /dev/null 2> "$err1" || status1=$? + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/dup --cd "$work" > /dev/null 2> "$err2" || status2=$? + if [[ "$status1" -eq 0 && "$status2" -ne 0 && "$(<"$err2")" == *"already exists"* \ + && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status1=$status1 status2=$status2 err2=$(<"$err2")" + fi +} + +case_create_unsafe_slug_rejected() { + local name="worktree create: a branch slug of '..' is rejected before touching git" + should_run "$name" || return 0 + local store work err status=0 + store="$tmp_root/state-create-unsafe" + work="$tmp_root/work-create-unsafe" + make_work_repo "$work" + err="$tmp_root/c7.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create '..' --cd "$work" > /dev/null 2> "$err" || status=$? + if [[ "$status" -ne 0 && "$(<"$err")" == *"safe slug"* ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + +case_create_help() { + local name="worktree create: -h prints usage and exits 0" + should_run "$name" || return 0 + local store work out status=0 + store="$tmp_root/state-create-help" + work="$tmp_root/work-create-help" + make_work_repo "$work" + out="$tmp_root/c8.out" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create -h --cd "$work" > /dev/null 2> "$out" || status=$? + if [[ "$status" -eq 0 && "$(<"$out")" == *"usage:"* ]]; then + pass "$name" + else + fail "$name" "status=$status out=$(<"$out")" + fi +} + +case_list_empty() { + local name="worktree list: empty registry prints no-worktrees message" + should_run "$name" || return 0 + local store work out status=0 + store="$tmp_root/state-list-empty" + work="$tmp_root/work-list-empty" + make_work_repo "$work" + out="$tmp_root/l1.out" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree list --cd "$work" > "$out" 2>&1 || status=$? + if [[ "$status" -eq 0 && "$(<"$out")" == *"No registered worktrees."* ]]; then + pass "$name" + else + fail "$name" "status=$status out=$(<"$out")" + fi +} + +case_list_json_valid() { + local name="worktree list: --json prints a valid JSON array" + should_run "$name" || return 0 + local store work status=0 + store="$tmp_root/state-list-json" + work="$tmp_root/work-list-json" + make_work_repo "$work" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/j --cd "$work" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && "$(wt_list_json "$store" "$work" | jq 'type')" == '"array"' \ + && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status=$status" + fi +} + +case_list_text_table() { + local name="worktree list: text mode prints a SLUG/BRANCH/PATH table" + should_run "$name" || return 0 + local store work out status=0 + store="$tmp_root/state-list-text" + work="$tmp_root/work-list-text" + make_work_repo "$work" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/t --cd "$work" > /dev/null 2>&1 + out="$tmp_root/l3.out" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree list --cd "$work" > "$out" 2>&1 || status=$? + if [[ "$status" -eq 0 && "$(sed -n '1p' "$out")" == *SLUG*BRANCH*PATH* && "$(sed -n '2p' "$out")" == *"feat/t"* ]]; then + pass "$name" + else + fail "$name" "status=$status out=$(<"$out")" + fi +} + +case_list_cross_worktree_identity() { + local name="worktree list: invoked from inside a linked worktree sees the same manifest" + should_run "$name" || return 0 + local store work wt_path status=0 + store="$tmp_root/state-list-identity" + work="$tmp_root/work-list-identity" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/identity --cd "$work" 2>/dev/null | tail -1)" + local inside_json + inside_json="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree list --json --cd "$wt_path" 2>/dev/null)" || status=$? + if [[ "$status" -eq 0 && "$(jq -r '.[0].branch' <<<"$inside_json")" == "feat/identity" ]]; then + pass "$name" + else + fail "$name" "status=$status inside_json=$inside_json" + fi +} + +case_remove_requires_target() { + local name="worktree remove: missing exits 2 with usage" + should_run "$name" || return 0 + local store work err status=0 + store="$tmp_root/state-remove-noarg" + work="$tmp_root/work-remove-noarg" + make_work_repo "$work" + err="$tmp_root/r1.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree remove --cd "$work" > /dev/null 2> "$err" || status=$? + if [[ "$status" -eq 2 && "$(<"$err")" == *" is required"* ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + +case_remove_unknown_target() { + local name="worktree remove: unknown target exits 1" + should_run "$name" || return 0 + local store work err status=0 + store="$tmp_root/state-remove-unknown" + work="$tmp_root/work-remove-unknown" + make_work_repo "$work" + err="$tmp_root/r2.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree remove nope --cd "$work" > /dev/null 2> "$err" || status=$? + if [[ "$status" -ne 0 && "$(<"$err")" == *"no registered worktree matches"* ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + +case_remove_success() { + local name="worktree remove: removes git worktree and manifest entry" + should_run "$name" || return 0 + local store work wt_path status=0 + store="$tmp_root/state-remove-ok" + work="$tmp_root/work-remove-ok" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/rm --cd "$work" 2>/dev/null | tail -1)" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree remove feat/rm --cd "$work" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && ! -d "$wt_path" && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 0 ]]; then + pass "$name" + else + fail "$name" "status=$status wt_path=$wt_path exists=$([[ -d "$wt_path" ]] && echo yes || echo no)" + fi +} + +case_remove_dirty_requires_force() { + local name="worktree remove: dirty worktree fails without --force, succeeds with it" + should_run "$name" || return 0 + local store work wt_path status1=0 status2=0 existed_after_first=0 existed_after_second=0 + store="$tmp_root/state-remove-dirty" + work="$tmp_root/work-remove-dirty" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/dirty --cd "$work" 2>/dev/null | tail -1)" + printf 'dirty\n' > "$wt_path/dirty.txt" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree remove feat/dirty --cd "$work" > /dev/null 2>&1 || status1=$? + [[ -d "$wt_path" ]] && existed_after_first=1 + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree remove feat/dirty --force --cd "$work" > /dev/null 2>&1 || status2=$? + [[ -d "$wt_path" ]] && existed_after_second=1 + if [[ "$status1" -ne 0 && "$existed_after_first" -eq 1 && "$status2" -eq 0 && "$existed_after_second" -eq 0 ]]; then + pass "$name" + else + fail "$name" "status1=$status1 existed_after_first=$existed_after_first status2=$status2 existed_after_second=$existed_after_second" + fi +} + +case_gc_no_worktrees() { + local name="worktree gc: empty registry is a no-op" + should_run "$name" || return 0 + local store work out status=0 + store="$tmp_root/state-gc-empty" + work="$tmp_root/work-gc-empty" + make_work_repo "$work" + out="$tmp_root/g1.out" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --cd "$work" > "$out" 2>&1 || status=$? + if [[ "$status" -eq 0 && "$(<"$out")" == *"no registered worktrees"* ]]; then + pass "$name" + else + fail "$name" "status=$status out=$(<"$out")" + fi +} + +case_gc_dry_run_no_mutation() { + local name="worktree gc: --dry-run reports but does not mutate the manifest" + should_run "$name" || return 0 + local store work wt_path out status=0 + store="$tmp_root/state-gc-dry" + work="$tmp_root/work-gc-dry" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/gcdry --cd "$work" 2>/dev/null | tail -1)" + rm -rf "$wt_path" + out="$tmp_root/g2.out" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --dry-run --cd "$work" > "$out" 2>&1 || status=$? + if [[ "$status" -eq 0 && "$(<"$out")" == *"would remove"* \ + && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status=$status out=$(<"$out")" + fi +} + +case_gc_removes_orphaned_manifest_entry() { + local name="worktree gc: removes a manifest entry whose path was manually deleted" + should_run "$name" || return 0 + local store work wt_path status=0 + store="$tmp_root/state-gc-orphan" + work="$tmp_root/work-gc-orphan" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/orphan --cd "$work" 2>/dev/null | tail -1)" + rm -rf "$wt_path" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --cd "$work" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 0 ]]; then + pass "$name" + else + fail "$name" "status=$status" + fi +} + +case_gc_merged_flag() { + local name="worktree gc: --merged removes worktrees whose branch is fully merged" + should_run "$name" || return 0 + local store work wt_path status=0 + store="$tmp_root/state-gc-merged" + work="$tmp_root/work-gc-merged" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/merged --cd "$work" 2>/dev/null | tail -1)" + # feat/merged has no new commits, so it is already fully merged into the base. + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --merged --cd "$work" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 0 ]]; then + pass "$name" + else + fail "$name" "status=$status" + fi +} + +case_gc_max_age_days_filters() { + local name="worktree gc: --max-age-days only removes entries older than the threshold" + should_run "$name" || return 0 + local store work wt_path status=0 + store="$tmp_root/state-gc-age" + work="$tmp_root/work-gc-age" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/fresh --cd "$work" 2>/dev/null | tail -1)" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --max-age-days 30 --cd "$work" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && -d "$wt_path" && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status=$status" + fi +} + +case_gc_prunes_git_state() { + local name="worktree gc: leaves git's own worktree list in sync (no stray entries)" + should_run "$name" || return 0 + local store work wt_path status=0 + store="$tmp_root/state-gc-prune" + work="$tmp_root/work-gc-prune" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/prune --cd "$work" 2>/dev/null | tail -1)" + rm -rf "$wt_path" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --cd "$work" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && "$(git -C "$work" worktree list --porcelain | grep -c '^worktree ')" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status=$status" + fi +} + +case_create_requires_branch +case_create_new_branch +case_create_from_base +case_create_existing_branch_no_new_ref +case_create_name_override_slug +case_create_duplicate_slug_rejected +case_create_unsafe_slug_rejected +case_create_help +case_list_empty +case_list_json_valid +case_list_text_table +case_list_cross_worktree_identity +case_remove_requires_target +case_remove_unknown_target +case_remove_success +case_remove_dirty_requires_force +case_gc_no_worktrees +case_gc_dry_run_no_mutation +case_gc_removes_orphaned_manifest_entry +case_gc_merged_flag +case_gc_max_age_days_filters +case_gc_prunes_git_state + +th_summary From 5fb4e3f9a6c4c6e319ff058c14aebcf65a14716e Mon Sep 17 00:00:00 2001 From: screenleon Date: Thu, 2 Jul 2026 13:58:11 +0900 Subject: [PATCH 03/10] =?UTF-8?q?fix(CC-014):=20gate=20findings=20?= =?UTF-8?q?=E2=80=94=20dirty-worktree=20data=20loss=20+=20shellcheck=20+?= =?UTF-8?q?=20self-cwd=20bug?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses NO-GO findings from pr-gate (qa-tester/risk-reviewer block, critic block-soft): - gc --merged evaluated "merged" against the invoking worktree's own HEAD, so running it from inside a linked worktree treated that worktree's branch as trivially "merged into itself" and removed it. Now evaluated against the primary checkout's HEAD via a new _pmctl_worktree_main_root helper, regardless of which worktree --cd points at. - gc unconditionally force-removed merged/aged worktrees, discarding uncommitted changes with no confirmation (remove requires --force for the same case). gc now attempts a plain removal first and skips + reports dirty matches unless the caller passes the new `gc --force` flag. Orphaned/untracked entries (nothing live to lose) are unaffected. - Manifest reg_dir is now resolved once up front via `sw_project_worktree_dir ` (an explicit-arg variant added to state-paths.sh) instead of re-deriving it through `cd "$work_dir"` after a destructive operation may have deleted that very directory. - scripts/test-pmctl-worktree.sh: inline shellcheck disable comments (SC1091/SC2154) matching the repo's existing test-pmctl-*.sh convention, raw `shellcheck` is clean without relying solely on lint.yml ignore_names. - Added regression coverage for the self-removal and dirty-merged-skip cases, plus behavior/Steps docstrings on every case per QA convention. --- commands/using-git-worktrees.md | 6 +- scripts/lib/pmctl-worktree.sh | 134 ++++++++++++++++++++++---------- scripts/lib/state-paths.sh | 20 +++-- scripts/test-pmctl-worktree.sh | 109 ++++++++++++++++++++++++++ 4 files changed, 220 insertions(+), 49 deletions(-) diff --git a/commands/using-git-worktrees.md b/commands/using-git-worktrees.md index d30bcf5..59e2a1f 100644 --- a/commands/using-git-worktrees.md +++ b/commands/using-git-worktrees.md @@ -33,14 +33,14 @@ pmctl worktree remove feat/my-feature pmctl worktree create [--from ] [--name ] [--cd ] pmctl worktree list [--cd ] [--json] pmctl worktree remove [--force] [--cd ] -pmctl worktree gc [--dry-run] [--merged] [--max-age-days D] [--cd ] +pmctl worktree gc [--dry-run] [--merged] [--max-age-days D] [--force] [--cd ] ``` - `create ` — attaches an existing local branch, or creates a new one off the current `HEAD` if it doesn't exist yet. Pass `--from ` to create the new branch off a specific base instead of `HEAD`. Prints the new worktree's absolute path on success — capture it if you need to `cd` into it. - `create --name ` — override the manifest slug (defaults to the branch name with `/` replaced by `-`). Two worktrees cannot share a slug; `create` fails rather than silently overwriting an existing one. - `list` — table of registered worktrees (`SLUG`, `BRANCH`, `PATH`). Add `--json` for a machine-readable array (each entry: `slug`, `branch`, `path`, `created_ts`). - `remove ` — matches by slug or by branch name. Fails if the worktree has uncommitted changes; pass `--force` to discard them and remove anyway. **`--force` is destructive** — it discards uncommitted work in that worktree with no recovery path, so confirm you don't need those changes before passing it. -- `gc` — reconciles the manifest against actual git/filesystem state: drops entries whose directory was removed manually (e.g. `rm -rf`) or that git no longer tracks. Add `--merged` to also remove worktrees whose branch is fully merged, or `--max-age-days N` to remove entries older than N days. Always run with `--dry-run` first to see what would be removed before running for real. +- `gc` — reconciles the manifest against actual git/filesystem state: drops entries whose directory was removed manually (e.g. `rm -rf`) or that git no longer tracks (these are never destructive — the worktree is already gone or already untracked, so `gc` only cleans up the leftover manifest entry). Add `--merged` to also remove worktrees whose branch is fully merged, or `--max-age-days N` to remove entries older than N days — by default `gc` will *not* remove a merged/aged worktree that still has uncommitted changes (same dirty-worktree protection as plain `remove` without `--force`); it prints a `skipping ... has uncommitted changes` line and keeps the manifest entry instead. Pass `gc --force` to override that protection and force-remove merged/aged worktrees even when dirty — treat it with the same caution as `remove --force`. `--merged` is evaluated against the primary checkout's branch, not whichever worktree you happen to run `gc` from, so running `gc --merged` from inside a linked worktree never mistakes "merged into itself" for "safe to delete". Always run with `--dry-run` first to see what would be removed before running for real. All four subcommands accept `--cd ` to target a repo other than the current directory (same convention as `pmctl artifacts`/`pmctl dispatch`). @@ -53,7 +53,7 @@ All four subcommands accept `--cd ` to target a repo other than the cu ## Cleanup and orphan recovery -If a worktree directory is deleted directly (`rm -rf` instead of `pmctl worktree remove`), git and the manifest both still reference it. Run `pmctl worktree gc` — it detects the missing path, removes the stale manifest entry, and runs `git worktree prune` so `git worktree list` stays in sync too. `gc` never force-deletes a directory that still exists and looks live; it only prunes entries that are already gone, already merged (`--merged`), or past the age threshold (`--max-age-days`). +If a worktree directory is deleted directly (`rm -rf` instead of `pmctl worktree remove`), git and the manifest both still reference it. Run `pmctl worktree gc` — it detects the missing path, removes the stale manifest entry, and runs `git worktree prune` so `git worktree list` stays in sync too. By default `gc` never discards a directory that still exists and has uncommitted changes: for `--merged`/`--max-age-days` matches it attempts a plain (non-forced) removal and skips + reports any that turn out dirty, exactly like `remove` without `--force`. Only entries that are already gone or already untracked by git are removed unconditionally, since there is nothing live left to lose. Pass `gc --force` if you want merged/aged dirty worktrees discarded anyway. ## Out of scope diff --git a/scripts/lib/pmctl-worktree.sh b/scripts/lib/pmctl-worktree.sh index a4ed080..c5bf921 100644 --- a/scripts/lib/pmctl-worktree.sh +++ b/scripts/lib/pmctl-worktree.sh @@ -5,12 +5,22 @@ # state-paths.sh:_sw_worktree_project_key) so a linked worktree and its # primary checkout resolve to the SAME partition regardless of which one the # command is invoked from. +# +# Every subcommand resolves reg_dir ONCE, up front, via +# `sw_project_worktree_dir "$work_dir"` (a pure computation that takes an +# explicit repo_root -- no cd required). All manifest reads/writes take that +# resolved reg_dir as an explicit argument instead of re-deriving it via a +# `cd "$work_dir"` subshell each time. This matters because gc/remove can +# delete the very directory `$work_dir` points at (a linked worktree +# removing itself) -- re-deriving reg_dir afterward via cd would fail once +# that directory is gone, but the already-resolved absolute reg_dir (which +# lives in the state store, not the repo) keeps working. pmctl_worktree_usage() { printf 'usage: pmctl worktree create [--from ] [--name ] [--cd ]\n' >&2 printf ' pmctl worktree list [--cd ] [--json]\n' >&2 printf ' pmctl worktree remove [--force] [--cd ]\n' >&2 - printf ' pmctl worktree gc [--dry-run] [--merged] [--max-age-days D] [--cd ]\n' >&2 + printf ' pmctl worktree gc [--dry-run] [--merged] [--max-age-days D] [--force] [--cd ]\n' >&2 } pmctl_worktree_ensure_state_paths() { @@ -58,10 +68,19 @@ _pmctl_worktree_slugify() { printf '%s\n' "$slug" } -pmctl_worktree_manifest_path() { - local reg_dir - reg_dir="$(sw_project_worktree_dir)" || return 1 - printf '%s/manifest.jsonl\n' "$reg_dir" +# _pmctl_worktree_main_root +# Resolve the PRIMARY (non-worktree) checkout root for work_dir, mirroring +# state-paths.sh:_sw_main_repo_root. Used to evaluate branch --merged status +# against a STABLE reference instead of the caller's own possibly-linked- +# worktree HEAD (see pmctl_worktree_gc for why that distinction matters). +_pmctl_worktree_main_root() { + local work_dir="$1" common_dir + common_dir="$(git -C "$work_dir" rev-parse --git-common-dir 2>/dev/null)" || return 1 + if [[ "$common_dir" == /* ]]; then + dirname "$common_dir" + else + git -C "$work_dir" rev-parse --show-toplevel 2>/dev/null + fi } _pmctl_worktree_manifest_append_inner() { @@ -70,9 +89,9 @@ _pmctl_worktree_manifest_append_inner() { printf '%s\n' "$compact" >> "$manifest" } +# pmctl_worktree_manifest_append pmctl_worktree_manifest_append() { - local json_line="$1" reg_dir manifest rc=0 - reg_dir="$(sw_project_worktree_dir)" || return 1 + local reg_dir="$1" json_line="$2" manifest rc=0 mkdir -p "$reg_dir" 2>/dev/null || { printf 'pmctl worktree: mkdir failed: %s\n' "$reg_dir" >&2; return 1; } chmod 0700 "$reg_dir" 2>/dev/null || true manifest="$reg_dir/manifest.jsonl" @@ -88,10 +107,11 @@ _pmctl_worktree_manifest_rewrite_inner() { mv -f "$tmp_content" "$manifest" } +# pmctl_worktree_manifest_rewrite pmctl_worktree_manifest_rewrite() { - local new_content="$1" reg_dir manifest tmp rc=0 - reg_dir="$(sw_project_worktree_dir)" || return 1 + local reg_dir="$1" new_content="$2" manifest tmp rc=0 manifest="$reg_dir/manifest.jsonl" + mkdir -p "$reg_dir" 2>/dev/null || { printf 'pmctl worktree: mkdir failed: %s\n' "$reg_dir" >&2; return 1; } tmp="$(mktemp "$reg_dir/.manifest.XXXXXX")" || return 1 printf '%s' "$new_content" > "$tmp" serialize_with_lock "$reg_dir/manifest" _pmctl_worktree_manifest_rewrite_inner "$manifest" "$tmp" || rc=$? @@ -99,9 +119,9 @@ pmctl_worktree_manifest_rewrite() { return "$rc" } +# pmctl_worktree_manifest_read pmctl_worktree_manifest_read() { - local manifest - manifest="$(pmctl_worktree_manifest_path)" || return 1 + local reg_dir="$1" manifest="$1/manifest.jsonl" [[ -f "$manifest" ]] && cat "$manifest" return 0 } @@ -152,7 +172,7 @@ pmctl_worktree_create() { slug="$(_pmctl_worktree_slugify "${name:-$branch}")" || return 1 local reg_dir wt_path - reg_dir="$(cd "$work_dir" 2>/dev/null && sw_project_worktree_dir)" || { + reg_dir="$(sw_project_worktree_dir "$work_dir")" || { printf 'pmctl worktree create: cannot resolve worktree registry dir from %s\n' "$work_dir" >&2 return 1 } @@ -162,7 +182,7 @@ pmctl_worktree_create() { printf 'pmctl worktree create: a worktree already exists at %s (slug %s in use)\n' "$wt_path" "$slug" >&2 return 1 fi - if (cd "$work_dir" && git worktree list --porcelain 2>/dev/null | grep -q "^worktree $wt_path\$"); then + if git -C "$work_dir" worktree list --porcelain 2>/dev/null | grep -q "^worktree $wt_path\$"; then printf 'pmctl worktree create: git already tracks a worktree at %s\n' "$wt_path" >&2 return 1 fi @@ -172,13 +192,13 @@ pmctl_worktree_create() { local git_args=(worktree add) if [[ -n "$base" ]]; then git_args+=(-b "$branch" "$wt_path" "$base") - elif (cd "$work_dir" && git show-ref --verify --quiet "refs/heads/$branch" 2>/dev/null); then + elif git -C "$work_dir" show-ref --verify --quiet "refs/heads/$branch" 2>/dev/null; then git_args+=("$wt_path" "$branch") else git_args+=(-b "$branch" "$wt_path") fi - if ! (cd "$work_dir" && git "${git_args[@]}"); then + if ! git -C "$work_dir" "${git_args[@]}"; then printf 'pmctl worktree create: git worktree add failed\n' >&2 return 1 fi @@ -190,7 +210,7 @@ pmctl_worktree_create() { "$(jq -Rn --arg v "$branch" '$v')" \ "$(jq -Rn --arg v "$wt_path" '$v')" \ "$(jq -Rn --arg v "$created_ts" '$v')")" - (cd "$work_dir" && pmctl_worktree_manifest_append "$json_line") || { + pmctl_worktree_manifest_append "$reg_dir" "$json_line" || { printf 'pmctl worktree create: worktree created but manifest write failed -- run '\''pmctl worktree gc'\'' to reconcile\n' >&2 } printf '%s\n' "$wt_path" @@ -214,8 +234,9 @@ pmctl_worktree_list() { [[ -n "$work_dir" ]] || work_dir="$repo_root" pmctl_worktree_ensure_state_paths "$repo_root" || return $? - local manifest_content - manifest_content="$(cd "$work_dir" 2>/dev/null && pmctl_worktree_manifest_read)" || true + local reg_dir manifest_content + reg_dir="$(sw_project_worktree_dir "$work_dir")" || true + manifest_content="$(pmctl_worktree_manifest_read "$reg_dir")" if [[ "$json_out" -eq 1 ]]; then if [[ -z "$manifest_content" ]]; then @@ -264,8 +285,12 @@ pmctl_worktree_remove() { pmctl_worktree_ensure_state_paths "$repo_root" || return $? pmctl_worktree_ensure_writer "$repo_root" || return $? - local manifest_content match_line match_path - manifest_content="$(cd "$work_dir" 2>/dev/null && pmctl_worktree_manifest_read)" || true + local reg_dir manifest_content match_line match_path + reg_dir="$(sw_project_worktree_dir "$work_dir")" || { + printf 'pmctl worktree remove: cannot resolve worktree registry dir from %s\n' "$work_dir" >&2 + return 1 + } + manifest_content="$(pmctl_worktree_manifest_read "$reg_dir")" match_line="$(printf '%s\n' "$manifest_content" | jq -c --arg t "$target" 'select(.slug == $t or .branch == $t)' | head -1)" if [[ -z "$match_line" ]]; then printf 'pmctl worktree remove: no registered worktree matches %s\n' "$target" >&2 @@ -277,23 +302,23 @@ pmctl_worktree_remove() { [[ "$force" -eq 1 ]] && git_rm_args+=(--force) git_rm_args+=("$match_path") if [[ -d "$match_path" ]]; then - if ! (cd "$work_dir" && git "${git_rm_args[@]}"); then + if ! git -C "$work_dir" "${git_rm_args[@]}"; then printf 'pmctl worktree remove: git worktree remove failed for %s (dirty? pass --force to override)\n' "$match_path" >&2 return 1 fi fi - (cd "$work_dir" && git worktree prune) 2>/dev/null || true + git -C "$work_dir" worktree prune 2>/dev/null || true local remaining remaining="$(printf '%s\n' "$manifest_content" | jq -c --arg t "$target" 'select(.slug != $t and .branch != $t)')" - (cd "$work_dir" && pmctl_worktree_manifest_rewrite "$remaining") || { + pmctl_worktree_manifest_rewrite "$reg_dir" "$remaining" || { printf 'pmctl worktree remove: worktree removed but manifest cleanup failed -- run '\''pmctl worktree gc'\'' to reconcile\n' >&2 } printf 'removed %s (%s)\n' "$target" "$match_path" } pmctl_worktree_gc() { - local repo_root="${1:-}" work_dir dry_run=0 merged_only=0 max_age_days=0 args=() + local repo_root="${1:-}" work_dir dry_run=0 merged_only=0 max_age_days=0 force=0 args=() shift || true work_dir="${1:-$repo_root}" shift || true @@ -303,6 +328,7 @@ pmctl_worktree_gc() { case "${args[$i]}" in --dry-run) dry_run=1; i=$((i+1)) ;; --merged) merged_only=1; i=$((i+1)) ;; + --force) force=1; i=$((i+1)) ;; --max-age-days) max_age_days="${args[$((i+1))]:-0}" if ! [[ "$max_age_days" =~ ^[0-9]+$ ]]; then @@ -320,16 +346,25 @@ pmctl_worktree_gc() { pmctl_worktree_ensure_state_paths "$repo_root" || return $? pmctl_worktree_ensure_writer "$repo_root" || return $? - local manifest_content now_epoch max_age_seconds - manifest_content="$(cd "$work_dir" 2>/dev/null && pmctl_worktree_manifest_read)" || true + local reg_dir manifest_content now_epoch max_age_seconds main_root + reg_dir="$(sw_project_worktree_dir "$work_dir")" || { + printf 'pmctl worktree gc: cannot resolve worktree registry dir from %s\n' "$work_dir" >&2 + return 1 + } + manifest_content="$(pmctl_worktree_manifest_read "$reg_dir")" [[ -n "$manifest_content" ]] || { printf 'gc: no registered worktrees\n'; return 0; } now_epoch="$(date +%s 2>/dev/null || echo 0)" max_age_seconds=$(( max_age_days * 86400 )) + # --merged must be evaluated against the PRIMARY checkout's HEAD, not + # work_dir's own HEAD: a branch is trivially "merged into itself", so if + # work_dir is itself a linked worktree, `git branch --merged` run there + # would flag the worktree's own checked-out branch as removable. + main_root="$(_pmctl_worktree_main_root "$work_dir")" || main_root="$work_dir" - local kept_lines="" removed_count=0 + local kept_lines="" removed_count=0 skipped_dirty=0 while IFS= read -r line; do [[ -n "$line" ]] || continue - local slug branch path created_ts age_seconds should_remove=0 reason="" + local slug branch path created_ts age_seconds should_remove=0 reason="" destructive=0 slug="$(jq -r '.slug' <<<"$line")" branch="$(jq -r '.branch' <<<"$line")" path="$(jq -r '.path' <<<"$line")" @@ -337,38 +372,55 @@ pmctl_worktree_gc() { if [[ ! -d "$path" ]]; then should_remove=1; reason="path missing (orphaned manifest entry)" - elif ! (cd "$work_dir" && git worktree list --porcelain 2>/dev/null | grep -q "^worktree $path\$"); then + elif ! git -C "$work_dir" worktree list --porcelain 2>/dev/null | grep -q "^worktree $path\$"; then should_remove=1; reason="git no longer tracks this worktree" - elif [[ "$merged_only" -eq 1 ]] && (cd "$work_dir" && git branch --merged 2>/dev/null | grep -qE "^[*+ ]+$branch\$"); then - should_remove=1; reason="branch merged" + elif [[ "$merged_only" -eq 1 ]] && git -C "$main_root" branch --merged 2>/dev/null | grep -qE "^[*+ ]+$branch\$"; then + should_remove=1; destructive=1; reason="branch merged" elif [[ "$max_age_days" -gt 0 ]]; then local created_epoch created_epoch="$(date -d "$created_ts" +%s 2>/dev/null || date -jf '%Y-%m-%dT%H:%M:%S' "${created_ts%%[+-]*}" +%s 2>/dev/null || echo "$now_epoch")" age_seconds=$(( now_epoch - created_epoch )) if [[ "$age_seconds" -ge "$max_age_seconds" ]]; then - should_remove=1; reason="older than $max_age_days day(s)" + should_remove=1; destructive=1; reason="older than $max_age_days day(s)" fi fi if [[ "$should_remove" -eq 1 ]]; then - removed_count=$((removed_count+1)) if [[ "$dry_run" -eq 1 ]]; then + removed_count=$((removed_count+1)) printf 'would remove %s (%s): %s\n' "$slug" "$path" "$reason" - else - printf 'removing %s (%s): %s\n' "$slug" "$path" "$reason" - if [[ -d "$path" ]]; then - (cd "$work_dir" && git worktree remove --force "$path") 2>/dev/null || true + continue + fi + if [[ "$destructive" -eq 1 && -d "$path" ]]; then + # merged/age-based reasons touch a LIVE worktree that may have + # uncommitted changes -- default to git's own dirty-worktree + # refusal (same as `remove` without --force) unless the caller + # explicitly opted into `gc --force`. + local rm_args=(worktree remove) + [[ "$force" -eq 1 ]] && rm_args+=(--force) + rm_args+=("$path") + if ! git -C "$work_dir" "${rm_args[@]}" 2>/dev/null; then + printf 'skipping %s (%s): has uncommitted changes -- pass gc --force to remove anyway\n' "$slug" "$path" + skipped_dirty=$((skipped_dirty+1)) + kept_lines="${kept_lines}${line}"$'\n' + continue fi + elif [[ -d "$path" ]]; then + # orphaned / no-longer-tracked reasons: nothing live to lose, safe + # to force since git already doesn't consider this a real worktree. + git -C "$work_dir" worktree remove --force "$path" 2>/dev/null || true fi + removed_count=$((removed_count+1)) + printf 'removed %s (%s): %s\n' "$slug" "$path" "$reason" else kept_lines="${kept_lines}${line}"$'\n' fi done <<<"$manifest_content" - (cd "$work_dir" && git worktree prune) 2>/dev/null || true + git -C "$work_dir" worktree prune 2>/dev/null || true if [[ "$dry_run" -eq 0 ]]; then - (cd "$work_dir" && pmctl_worktree_manifest_rewrite "$kept_lines") || { + pmctl_worktree_manifest_rewrite "$reg_dir" "$kept_lines" || { printf 'pmctl worktree gc: manifest rewrite failed after removal\n' >&2 return 1 } @@ -377,6 +429,8 @@ pmctl_worktree_gc() { if [[ "$dry_run" -eq 1 ]]; then printf 'gc: dry-run, would remove %d worktree(s)\n' "$removed_count" else - printf 'gc: removed %d worktree(s)\n' "$removed_count" + printf 'gc: removed %d worktree(s)' "$removed_count" + [[ "$skipped_dirty" -gt 0 ]] && printf ', skipped %d dirty worktree(s)' "$skipped_dirty" + printf '\n' fi } diff --git a/scripts/lib/state-paths.sh b/scripts/lib/state-paths.sh index adc9deb..f976597 100644 --- a/scripts/lib/state-paths.sh +++ b/scripts/lib/state-paths.sh @@ -159,15 +159,23 @@ _sw_worktree_project_key() { return 0 } -# sw_project_worktree_dir -# Print the absolute worktree-registry directory for the current project's -# MAIN repo partition (stable whether invoked from the main checkout or from -# inside a linked worktree pmctl created): +# sw_project_worktree_dir [repo_root] +# Print the absolute worktree-registry directory for the given (or ambient +# cwd) repo's MAIN partition (stable whether invoked from the main checkout +# or from inside a linked worktree pmctl created): # /projects//worktrees # Pure computation -- does NOT create the directory; pmctl-worktree.sh owns -# mkdir + manifest writes. +# mkdir + manifest writes. Accepting an explicit repo_root lets callers +# resolve the registry dir WITHOUT cd'ing into it first, so the resolution +# doesn't depend on that directory continuing to exist for the rest of the +# call (e.g. gc deleting the very worktree it was invoked from via --cd). sw_project_worktree_dir() { - printf '%s/projects/%s/worktrees\n' "$(_sw_store_root)" "$(_sw_worktree_project_key)" + local repo_root="${1:-}" + if [[ -n "$repo_root" ]]; then + printf '%s/projects/%s/worktrees\n' "$(_sw_store_root)" "$(_SW_REPO_ROOT="$repo_root" _sw_worktree_project_key)" + else + printf '%s/projects/%s/worktrees\n' "$(_sw_store_root)" "$(_sw_worktree_project_key)" + fi } # sw_project_run_dir diff --git a/scripts/test-pmctl-worktree.sh b/scripts/test-pmctl-worktree.sh index 970d358..0e7a73a 100755 --- a/scripts/test-pmctl-worktree.sh +++ b/scripts/test-pmctl-worktree.sh @@ -1,5 +1,6 @@ #!/usr/bin/env bash # Regression tests for `pmctl worktree create/list/remove/gc`. +# shellcheck disable=SC2154 # tmp_root supplied by sourced test-harness set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -7,6 +8,7 @@ REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" PMCTL="$REPO_ROOT/cli/pmctl" # shellcheck source=scripts/lib/test-harness.sh +# shellcheck disable=SC1091 . "$SCRIPT_DIR/lib/test-harness.sh" th_init "$@" @@ -27,6 +29,8 @@ wt_list_json() { } case_create_requires_branch() { + # behavior: pmctl worktree create with no arg exits 2 and prints usage + # Steps: run create with only --cd; assert exit 2, stderr has " is required" and "usage:" local name="worktree create: missing exits 2 with usage" should_run "$name" || return 0 local store work out err status=0 @@ -43,6 +47,8 @@ case_create_requires_branch() { } case_create_new_branch() { + # behavior: create with a branch name that doesn't exist yet creates the branch + worktree + manifest entry + # Steps: run create feat/x; assert exit 0, printed path is a directory, manifest has branch feat/x local name="worktree create: new branch creates worktree + manifest entry" should_run "$name" || return 0 local store work out err status=0 wt_path @@ -61,6 +67,8 @@ case_create_new_branch() { } case_create_from_base() { + # behavior: create --from creates the new branch off the given base commit, not HEAD + # Steps: create a base-branch pointer; create feat/from-base --from base-branch; assert its HEAD sha == base sha local name="worktree create: --from creates a new branch off the given base" should_run "$name" || return 0 local store work out err status=0 wt_path base_sha branch_sha @@ -81,6 +89,8 @@ case_create_from_base() { } case_create_existing_branch_no_new_ref() { + # behavior: create on a branch that already exists attaches to it instead of erroring or creating a duplicate ref + # Steps: create a local branch; run create ; assert exit 0 and no "already exists" error local name="worktree create: existing branch attaches without creating a duplicate ref" should_run "$name" || return 0 local store work out err status=0 @@ -98,6 +108,8 @@ case_create_existing_branch_no_new_ref() { } case_create_name_override_slug() { + # behavior: create --name overrides the default branch-derived manifest slug + # Steps: run create feat/named --name custom-slug; assert the manifest's slug field is custom-slug local name="worktree create: --name overrides the manifest slug" should_run "$name" || return 0 local store work out err status=0 @@ -114,6 +126,8 @@ case_create_name_override_slug() { } case_create_duplicate_slug_rejected() { + # behavior: create with a slug that is already registered fails and does not add a second manifest entry + # Steps: create feat/dup twice; assert first succeeds, second fails with "already exists", manifest has exactly 1 entry local name="worktree create: duplicate slug is rejected, no duplicate manifest entry" should_run "$name" || return 0 local store work err1 err2 status1=0 status2=0 @@ -132,6 +146,8 @@ case_create_duplicate_slug_rejected() { } case_create_unsafe_slug_rejected() { + # behavior: a branch name that slugifies to an unsafe segment (e.g. "..") is rejected before touching git + # Steps: run create '..'; assert non-zero exit and stderr mentions "safe slug" local name="worktree create: a branch slug of '..' is rejected before touching git" should_run "$name" || return 0 local store work err status=0 @@ -148,6 +164,8 @@ case_create_unsafe_slug_rejected() { } case_create_help() { + # behavior: create -h prints usage and exits 0 without creating anything + # Steps: run create -h; assert exit 0 and stderr contains "usage:" local name="worktree create: -h prints usage and exits 0" should_run "$name" || return 0 local store work out status=0 @@ -164,6 +182,8 @@ case_create_help() { } case_list_empty() { + # behavior: list on an empty registry prints a human-readable "no worktrees" message + # Steps: run list on a fresh repo with no registered worktrees; assert output contains "No registered worktrees." local name="worktree list: empty registry prints no-worktrees message" should_run "$name" || return 0 local store work out status=0 @@ -180,6 +200,8 @@ case_list_empty() { } case_list_json_valid() { + # behavior: list --json prints a valid JSON array with one element per registered worktree + # Steps: create one worktree; run list --json; assert output type is array with length 1 local name="worktree list: --json prints a valid JSON array" should_run "$name" || return 0 local store work status=0 @@ -196,6 +218,8 @@ case_list_json_valid() { } case_list_text_table() { + # behavior: list (no --json) prints a human-readable SLUG/BRANCH/PATH table + # Steps: create one worktree; run list; assert header row has SLUG/BRANCH/PATH and a data row has the branch local name="worktree list: text mode prints a SLUG/BRANCH/PATH table" should_run "$name" || return 0 local store work out status=0 @@ -213,6 +237,10 @@ case_list_text_table() { } case_list_cross_worktree_identity() { + # behavior: list invoked with --cd pointing INSIDE a linked worktree resolves the same manifest partition + # as the primary checkout (the identity seam this whole feature depends on) + # Steps: create a worktree from the primary checkout; run list --json --cd ; + # assert it sees the same entry it was just registered under local name="worktree list: invoked from inside a linked worktree sees the same manifest" should_run "$name" || return 0 local store work wt_path status=0 @@ -230,6 +258,8 @@ case_list_cross_worktree_identity() { } case_remove_requires_target() { + # behavior: remove with no arg exits 2 and prints usage + # Steps: run remove with only --cd; assert exit 2 and stderr has " is required" local name="worktree remove: missing exits 2 with usage" should_run "$name" || return 0 local store work err status=0 @@ -246,6 +276,8 @@ case_remove_requires_target() { } case_remove_unknown_target() { + # behavior: remove with a target that matches no manifest entry exits non-zero with a clear error + # Steps: run remove nope on an empty registry; assert non-zero exit and stderr mentions no match found local name="worktree remove: unknown target exits 1" should_run "$name" || return 0 local store work err status=0 @@ -262,6 +294,8 @@ case_remove_unknown_target() { } case_remove_success() { + # behavior: remove on a clean worktree deletes the git worktree directory and its manifest entry + # Steps: create feat/rm; remove feat/rm; assert exit 0, directory gone, manifest empty local name="worktree remove: removes git worktree and manifest entry" should_run "$name" || return 0 local store work wt_path status=0 @@ -278,6 +312,9 @@ case_remove_success() { } case_remove_dirty_requires_force() { + # behavior: remove on a worktree with uncommitted changes fails without --force and succeeds with it + # Steps: create feat/dirty, add an untracked file; remove without --force (assert fails, dir survives); + # remove --force (assert succeeds, dir gone) local name="worktree remove: dirty worktree fails without --force, succeeds with it" should_run "$name" || return 0 local store work wt_path status1=0 status2=0 existed_after_first=0 existed_after_second=0 @@ -298,6 +335,8 @@ case_remove_dirty_requires_force() { } case_gc_no_worktrees() { + # behavior: gc on an empty registry is a no-op that reports so and exits 0 + # Steps: run gc on a repo with no registered worktrees; assert exit 0 and output mentions no registered worktrees local name="worktree gc: empty registry is a no-op" should_run "$name" || return 0 local store work out status=0 @@ -314,6 +353,9 @@ case_gc_no_worktrees() { } case_gc_dry_run_no_mutation() { + # behavior: gc --dry-run reports what it would remove but leaves the manifest untouched + # Steps: create a worktree, delete its directory manually (orphan it); run gc --dry-run; + # assert output says "would remove" and the manifest still has the entry local name="worktree gc: --dry-run reports but does not mutate the manifest" should_run "$name" || return 0 local store work wt_path out status=0 @@ -333,6 +375,8 @@ case_gc_dry_run_no_mutation() { } case_gc_removes_orphaned_manifest_entry() { + # behavior: gc removes a manifest entry whose directory was deleted outside of pmctl (e.g. rm -rf) + # Steps: create a worktree, delete its directory manually; run gc; assert manifest is empty afterward local name="worktree gc: removes a manifest entry whose path was manually deleted" should_run "$name" || return 0 local store work wt_path status=0 @@ -350,6 +394,9 @@ case_gc_removes_orphaned_manifest_entry() { } case_gc_merged_flag() { + # behavior: gc --merged removes a clean worktree whose branch is fully merged into the primary checkout's HEAD + # Steps: create feat/merged with no new commits (trivially merged); run gc --merged from the primary checkout; + # assert manifest is empty afterward local name="worktree gc: --merged removes worktrees whose branch is fully merged" should_run "$name" || return 0 local store work wt_path status=0 @@ -366,7 +413,64 @@ case_gc_merged_flag() { fi } +case_gc_merged_does_not_self_remove_when_invoked_from_inside() { + # behavior: gc --merged evaluates "merged" against the PRIMARY checkout's HEAD, not the invoking worktree's + # own HEAD -- so it must not treat an unmerged branch as removable just because gc was run + # from inside that very worktree (a branch is trivially "merged into itself") + # Steps: create feat/self, commit something on it that master does NOT have (genuinely unmerged); + # run gc --merged --cd ; assert the worktree and its manifest entry survive + local name="worktree gc: --merged run from inside the linked worktree does not remove itself" + should_run "$name" || return 0 + local store work wt_path status=0 + store="$tmp_root/state-gc-self" + work="$tmp_root/work-gc-self" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/self --cd "$work" 2>/dev/null | tail -1)" + git -C "$wt_path" config user.email t@e.com + git -C "$wt_path" config user.name t + printf 'unmerged\n' > "$wt_path/unmerged.txt" + git -C "$wt_path" add unmerged.txt + git -C "$wt_path" commit -q -m unmerged + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --merged --cd "$wt_path" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && -d "$wt_path" && "$(wt_list_json "$store" "$wt_path" | jq 'length')" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status=$status wt_path_exists=$([[ -d "$wt_path" ]] && echo yes || echo no)" + fi +} + +case_gc_merged_skips_dirty_without_force() { + # behavior: gc --merged must not silently discard uncommitted changes -- a merged-but-dirty worktree is + # skipped (kept in the manifest, reported) unless the caller explicitly passes gc --force + # Steps: create feat/dirty-merged (trivially merged), add an untracked file; run gc --merged (no --force): + # assert it is skipped with an "uncommitted changes" message and the directory/manifest entry survive; + # run gc --merged --force: assert it is now removed + local name="worktree gc: --merged skips a dirty worktree without --force, removes it with --force" + should_run "$name" || return 0 + local store work wt_path out status1=0 status2=0 existed_after_skip=0 existed_after_force=0 + store="$tmp_root/state-gc-dirty-merged" + work="$tmp_root/work-gc-dirty-merged" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/dirty-merged --cd "$work" 2>/dev/null | tail -1)" + printf 'dirty\n' > "$wt_path/dirty.txt" + out="$tmp_root/gcd1.out" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --merged --cd "$work" > "$out" 2>&1 || status1=$? + [[ -d "$wt_path" ]] && existed_after_skip=1 + local kept_after_skip + kept_after_skip="$(wt_list_json "$store" "$work" | jq 'length')" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --merged --force --cd "$work" > /dev/null 2>&1 || status2=$? + [[ -d "$wt_path" ]] && existed_after_force=1 + if [[ "$status1" -eq 0 && "$existed_after_skip" -eq 1 && "$kept_after_skip" -eq 1 && "$(<"$out")" == *"uncommitted changes"* \ + && "$status2" -eq 0 && "$existed_after_force" -eq 0 && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 0 ]]; then + pass "$name" + else + fail "$name" "status1=$status1 existed_after_skip=$existed_after_skip kept_after_skip=$kept_after_skip status2=$status2 existed_after_force=$existed_after_force out=$(<"$out")" + fi +} + case_gc_max_age_days_filters() { + # behavior: gc --max-age-days N only removes entries older than N days; a freshly created entry is kept + # Steps: create feat/fresh; run gc --max-age-days 30 immediately afterward; assert directory and manifest entry survive local name="worktree gc: --max-age-days only removes entries older than the threshold" should_run "$name" || return 0 local store work wt_path status=0 @@ -383,6 +487,9 @@ case_gc_max_age_days_filters() { } case_gc_prunes_git_state() { + # behavior: gc also runs `git worktree prune` so git's own bookkeeping stays in sync with the manifest + # Steps: create a worktree, delete its directory manually, run gc; assert `git worktree list` shows only + # the primary checkout afterward (no stray registered-but-gone entries) local name="worktree gc: leaves git's own worktree list in sync (no stray entries)" should_run "$name" || return 0 local store work wt_path status=0 @@ -419,6 +526,8 @@ case_gc_no_worktrees case_gc_dry_run_no_mutation case_gc_removes_orphaned_manifest_entry case_gc_merged_flag +case_gc_merged_does_not_self_remove_when_invoked_from_inside +case_gc_merged_skips_dirty_without_force case_gc_max_age_days_filters case_gc_prunes_git_state From 560471f79d72ff317239578fcc91b805b68de865 Mon Sep 17 00:00:00 2001 From: screenleon Date: Thu, 2 Jul 2026 14:05:14 +0900 Subject: [PATCH 04/10] fix(CC-014): fail loud on manifest-write failure, add gc age-removal coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses second-round pr-gate findings (qa-tester block, critic/risk- reviewer medium): - create no longer silently succeeds when the manifest append fails after git worktree add already ran. gc cannot discover a worktree that never got a manifest entry, so this is not gc-recoverable — return nonzero and point at manual `git worktree remove` recovery instead of a misleading "run gc to reconcile" message. - Added direct coverage for gc --max-age-days actually removing an aged entry (previous test only proved a fresh entry survives) and for --max-age-days rejecting a non-integer value. --- scripts/lib/pmctl-worktree.sh | 12 ++++++-- scripts/test-pmctl-worktree.sh | 56 ++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/scripts/lib/pmctl-worktree.sh b/scripts/lib/pmctl-worktree.sh index c5bf921..748dd59 100644 --- a/scripts/lib/pmctl-worktree.sh +++ b/scripts/lib/pmctl-worktree.sh @@ -210,9 +210,15 @@ pmctl_worktree_create() { "$(jq -Rn --arg v "$branch" '$v')" \ "$(jq -Rn --arg v "$wt_path" '$v')" \ "$(jq -Rn --arg v "$created_ts" '$v')")" - pmctl_worktree_manifest_append "$reg_dir" "$json_line" || { - printf 'pmctl worktree create: worktree created but manifest write failed -- run '\''pmctl worktree gc'\'' to reconcile\n' >&2 - } + if ! pmctl_worktree_manifest_append "$reg_dir" "$json_line"; then + # gc reconciles EXISTING manifest entries against git/filesystem state -- + # it cannot discover a worktree that never got a manifest entry in the + # first place, so this is not gc-recoverable. Surface it as a failure + # (not a warning) and point at the manual recovery path instead. + printf 'pmctl worktree create: worktree created at %s but manifest registration failed -- it will not appear in '\''pmctl worktree list/gc'\''. Run '\''git worktree remove %s'\'' to discard it, or retry '\''pmctl worktree create'\'' after fixing the manifest write error above.\n' "$wt_path" "$wt_path" >&2 + printf '%s\n' "$wt_path" + return 1 + fi printf '%s\n' "$wt_path" } diff --git a/scripts/test-pmctl-worktree.sh b/scripts/test-pmctl-worktree.sh index 0e7a73a..97987ea 100755 --- a/scripts/test-pmctl-worktree.sh +++ b/scripts/test-pmctl-worktree.sh @@ -28,6 +28,17 @@ wt_list_json() { PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree list --json --cd "$work" } +# reg_dir_for +# Resolve the worktree-registry directory for a test partition, so a case +# can directly rewrite manifest.jsonl's created_ts to simulate an aged +# entry without waiting. +reg_dir_for() { + local store="$1" work="$2" + PM_DISPATCH_STATE_ROOT="$store" bash -c \ + '. "$1/scripts/lib/state-paths.sh" && sw_project_worktree_dir "$2"' \ + _ "$REPO_ROOT" "$work" +} + case_create_requires_branch() { # behavior: pmctl worktree create with no arg exits 2 and prints usage # Steps: run create with only --cd; assert exit 2, stderr has " is required" and "usage:" @@ -486,6 +497,49 @@ case_gc_max_age_days_filters() { fi } +case_gc_max_age_days_removes_aged_entry() { + # behavior: gc --max-age-days N actually removes a worktree whose manifest created_ts is older than N days + # (case_gc_max_age_days_filters above only proves a FRESH entry survives -- this proves the + # destructive removal side of the same flag actually fires) + # Steps: create feat/aged, then directly rewrite its manifest created_ts to 60 days ago; run + # gc --max-age-days 30; assert the directory is gone and the manifest entry is removed + local name="worktree gc: --max-age-days removes an entry older than the threshold" + should_run "$name" || return 0 + local store work wt_path reg_dir manifest old_ts status=0 + store="$tmp_root/state-gc-age-aged" + work="$tmp_root/work-gc-age-aged" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/aged --cd "$work" 2>/dev/null | tail -1)" + reg_dir="$(reg_dir_for "$store" "$work")" + manifest="$reg_dir/manifest.jsonl" + old_ts="$(date -d '60 days ago' -Is 2>/dev/null || date -v-60d -Is 2>/dev/null)" + jq -c --arg ts "$old_ts" '.created_ts = $ts' "$manifest" > "$manifest.new" && mv "$manifest.new" "$manifest" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --max-age-days 30 --cd "$work" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && ! -d "$wt_path" && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 0 ]]; then + pass "$name" + else + fail "$name" "status=$status wt_path_exists=$([[ -d "$wt_path" ]] && echo yes || echo no)" + fi +} + +case_gc_max_age_days_rejects_non_integer() { + # behavior: gc --max-age-days with a non-integer value is rejected with exit 2 and a documented error + # Steps: run gc --max-age-days nope; assert exit 2 and stderr mentions the requirement + local name="worktree gc: --max-age-days rejects a non-integer value" + should_run "$name" || return 0 + local store work err status=0 + store="$tmp_root/state-gc-age-badarg" + work="$tmp_root/work-gc-age-badarg" + make_work_repo "$work" + err="$tmp_root/gcbad.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --max-age-days nope --cd "$work" > /dev/null 2> "$err" || status=$? + if [[ "$status" -eq 2 && "$(<"$err")" == *"--max-age-days requires an integer"* ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + case_gc_prunes_git_state() { # behavior: gc also runs `git worktree prune` so git's own bookkeeping stays in sync with the manifest # Steps: create a worktree, delete its directory manually, run gc; assert `git worktree list` shows only @@ -529,6 +583,8 @@ case_gc_merged_flag case_gc_merged_does_not_self_remove_when_invoked_from_inside case_gc_merged_skips_dirty_without_force case_gc_max_age_days_filters +case_gc_max_age_days_removes_aged_entry +case_gc_max_age_days_rejects_non_integer case_gc_prunes_git_state th_summary From 6e547274b38841982693cfeb8567c61f38f99979 Mon Sep 17 00:00:00 2001 From: screenleon Date: Thu, 2 Jul 2026 14:20:01 +0900 Subject: [PATCH 05/10] fix(CC-014): route writes through unsafe-root policy, exact branch matching, stale-slug guard Addresses third-round pr-gate findings (security-reviewer block, hard gate; architecture-reviewer block-soft; qa-tester block; corroborated independently by critic and risk-reviewer): - pmctl_worktree_ensure_root_safe() routes every worktree-registry write (manifest mkdir, mktemp/rewrite, and the git checkout create/remove/gc perform under it) through the same _sw_ensure_store_root_safe check every other state-store writer already goes through, so a symlinked or non-owned PM_DISPATCH_STATE_ROOT is rejected instead of silently accepted. - gc --merged now does exact string matching against `git branch --merged` output (stripping git's fixed 2-char marker column) instead of interpolating the branch name into a `grep -E` pattern -- a branch containing regex metacharacters (e.g. "a.b") could previously false-positive match an unrelated already-merged branch ("axb") and get wrongly removed. - create now also checks the manifest itself for an existing entry with the target slug, not just live filesystem/git state -- a checkout deleted and `git worktree prune`d outside pmctl (bypassing remove/gc) left a stale manifest row that the old checks couldn't see, letting the same slug get registered twice. - Added direct regression coverage for all three: symlinked state-root rejection, stale-manifest duplicate-slug recreation, and a regex- metacharacter branch name that must not false-positive match. --- scripts/lib/pmctl-worktree.sh | 48 ++++++++++++++++++-- scripts/test-pmctl-worktree.sh | 80 ++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 3 deletions(-) diff --git a/scripts/lib/pmctl-worktree.sh b/scripts/lib/pmctl-worktree.sh index 748dd59..1915586 100644 --- a/scripts/lib/pmctl-worktree.sh +++ b/scripts/lib/pmctl-worktree.sh @@ -40,19 +40,33 @@ pmctl_worktree_ensure_state_paths() { pmctl_worktree_ensure_writer() { local repo_root="${1:-}" - if [[ "$(type -t _sw_compact_json_line 2>/dev/null)" != function || "$(type -t serialize_with_lock 2>/dev/null)" != function ]]; then + if [[ "$(type -t _sw_compact_json_line 2>/dev/null)" != function || "$(type -t serialize_with_lock 2>/dev/null)" != function \ + || "$(type -t _sw_ensure_store_root_safe 2>/dev/null)" != function ]]; then local _sw_lib="${repo_root:-}/scripts/lib/state-writer.sh" if [[ -r "$_sw_lib" ]]; then # shellcheck disable=SC1090,SC1091 # dynamic repo-root path. . "$_sw_lib" 2>/dev/null || true fi fi - if [[ "$(type -t _sw_compact_json_line 2>/dev/null)" != function || "$(type -t serialize_with_lock 2>/dev/null)" != function ]]; then + if [[ "$(type -t _sw_compact_json_line 2>/dev/null)" != function || "$(type -t serialize_with_lock 2>/dev/null)" != function \ + || "$(type -t _sw_ensure_store_root_safe 2>/dev/null)" != function ]]; then printf 'pmctl worktree: state-writer.sh unavailable; cannot write worktree manifest\n' >&2 return 2 fi } +# pmctl_worktree_ensure_root_safe +# Route every worktree-registry write (manifest mkdir/mktemp, and the git +# checkout create/remove/gc perform under it) through the SAME unsafe-root +# check ("state_store_init" / "_sw_ensure_store_root_safe") that guards every +# other state-store writer -- a symlinked or non-owned PM_DISPATCH_STATE_ROOT +# leaf must be rejected here too, not just for runs/events/decisions. Must be +# called (after pmctl_worktree_ensure_writer) before ANY mkdir/mktemp/git-add +# under a resolved reg_dir. +pmctl_worktree_ensure_root_safe() { + _sw_ensure_store_root_safe "$(_sw_store_root)" +} + # _pmctl_worktree_slugify # Normalize a branch name into a filesystem-safe, single-path-segment slug: # `/` -> `-`, then strip everything outside [A-Za-z0-9._-]. Rejects a branch @@ -83,6 +97,22 @@ _pmctl_worktree_main_root() { fi } +# _pmctl_worktree_branch_is_merged +# Exact-match membership test against `git branch --merged` output. `git +# branch [--merged]` always prints a fixed 2-character marker column +# ("* ", "+ ", " ") followed by the ref name, so stripping exactly 2 +# characters and comparing for equality is both correct and immune to +# branch names containing regex metacharacters (a prior version matched +# via `grep -E`, so a branch named e.g. "a.b" could false-positive match +# an unrelated merged branch "axb"). +_pmctl_worktree_branch_is_merged() { + local main_root="$1" branch="$2" line + while IFS= read -r line; do + [[ "${line:2}" == "$branch" ]] && return 0 + done < <(git -C "$main_root" branch --merged 2>/dev/null) + return 1 +} + _pmctl_worktree_manifest_append_inner() { local json_line="$1" compact manifest="$2" compact="$(_sw_compact_json_line "$json_line")" || return $? @@ -167,6 +197,7 @@ pmctl_worktree_create() { [[ -n "$work_dir" ]] || work_dir="$repo_root" pmctl_worktree_ensure_state_paths "$repo_root" || return $? pmctl_worktree_ensure_writer "$repo_root" || return $? + pmctl_worktree_ensure_root_safe || return 1 local slug slug="$(_pmctl_worktree_slugify "${name:-$branch}")" || return 1 @@ -186,6 +217,15 @@ pmctl_worktree_create() { printf 'pmctl worktree create: git already tracks a worktree at %s\n' "$wt_path" >&2 return 1 fi + # A manifest entry can outlive its checkout (manual `rm -rf` + `git + # worktree prune` without going through `pmctl worktree remove`/`gc` + # first) -- the two checks above only see live filesystem/git state, so + # they would miss that stale registration and let this same slug get + # appended a second time. Check the manifest itself too. + if [[ -n "$(pmctl_worktree_manifest_read "$reg_dir" | jq -c --arg s "$slug" 'select(.slug == $s)' | head -1)" ]]; then + printf 'pmctl worktree create: slug %s is already registered in the manifest (stale entry? run '\''pmctl worktree gc'\'' first)\n' "$slug" >&2 + return 1 + fi mkdir -p "$reg_dir/checkouts" 2>/dev/null || true @@ -290,6 +330,7 @@ pmctl_worktree_remove() { [[ -n "$work_dir" ]] || work_dir="$repo_root" pmctl_worktree_ensure_state_paths "$repo_root" || return $? pmctl_worktree_ensure_writer "$repo_root" || return $? + pmctl_worktree_ensure_root_safe || return 1 local reg_dir manifest_content match_line match_path reg_dir="$(sw_project_worktree_dir "$work_dir")" || { @@ -351,6 +392,7 @@ pmctl_worktree_gc() { [[ -n "$work_dir" ]] || work_dir="$repo_root" pmctl_worktree_ensure_state_paths "$repo_root" || return $? pmctl_worktree_ensure_writer "$repo_root" || return $? + pmctl_worktree_ensure_root_safe || return 1 local reg_dir manifest_content now_epoch max_age_seconds main_root reg_dir="$(sw_project_worktree_dir "$work_dir")" || { @@ -380,7 +422,7 @@ pmctl_worktree_gc() { should_remove=1; reason="path missing (orphaned manifest entry)" elif ! git -C "$work_dir" worktree list --porcelain 2>/dev/null | grep -q "^worktree $path\$"; then should_remove=1; reason="git no longer tracks this worktree" - elif [[ "$merged_only" -eq 1 ]] && git -C "$main_root" branch --merged 2>/dev/null | grep -qE "^[*+ ]+$branch\$"; then + elif [[ "$merged_only" -eq 1 ]] && _pmctl_worktree_branch_is_merged "$main_root" "$branch"; then should_remove=1; destructive=1; reason="branch merged" elif [[ "$max_age_days" -gt 0 ]]; then local created_epoch diff --git a/scripts/test-pmctl-worktree.sh b/scripts/test-pmctl-worktree.sh index 97987ea..e46a8c8 100755 --- a/scripts/test-pmctl-worktree.sh +++ b/scripts/test-pmctl-worktree.sh @@ -156,6 +156,55 @@ case_create_duplicate_slug_rejected() { fi } +case_create_stale_manifest_duplicate_slug_rejected() { + # behavior: create rejects a slug that is still registered in the manifest even when the checkout was + # deleted and pruned OUTSIDE pmctl (rm -rf + git worktree prune, bypassing `remove`/`gc`) -- + # the live-path and git-worktree-list checks alone would miss this and append a duplicate row + # Steps: create feat/stale, then rm -rf its directory and `git worktree prune` directly (not via pmctl); + # run create feat/stale again; assert it is rejected and the manifest still has exactly 1 entry + local name="worktree create: stale manifest entry blocks recreating the same slug" + should_run "$name" || return 0 + local store work wt_path err status=0 + store="$tmp_root/state-create-stale" + work="$tmp_root/work-create-stale" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/stale --cd "$work" 2>/dev/null | tail -1)" + rm -rf "$wt_path" + git -C "$work" worktree prune + err="$tmp_root/c9.err" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/stale --cd "$work" > /dev/null 2> "$err" || status=$? + if [[ "$status" -ne 0 && "$(<"$err")" == *"already registered"* \ + && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err") count=$(wt_list_json "$store" "$work" | jq 'length')" + fi +} + +case_create_rejects_unsafe_symlinked_state_root() { + # behavior: worktree writes (manifest mkdir, checkout creation) are rejected when PM_DISPATCH_STATE_ROOT + # itself is a symlink, mirroring the same unsafe-root policy other state-store writers enforce + # Steps: point PM_DISPATCH_STATE_ROOT at a symlink to a real directory; run create; assert nonzero exit + # and stderr mentions the unsafe-root rejection, and nothing was created under the symlink target + local name="worktree create: rejects a symlinked PM_DISPATCH_STATE_ROOT" + should_run "$name" || return 0 + local work real_target link err status=0 + work="$tmp_root/work-create-unsafe-root" + make_work_repo "$work" + real_target="$tmp_root/unsafe-root-real" + mkdir -p "$real_target" + link="$tmp_root/unsafe-root-link" + ln -s "$real_target" "$link" + err="$tmp_root/c10.err" + PM_DISPATCH_STATE_ROOT="$link" "$PMCTL" worktree create feat/unsafe-root --cd "$work" > /dev/null 2> "$err" || status=$? + if [[ "$status" -ne 0 && "$(<"$err")" == *"unsafe state root rejected"* \ + && -z "$(find "$real_target" -mindepth 1 2>/dev/null)" ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + case_create_unsafe_slug_rejected() { # behavior: a branch name that slugifies to an unsafe segment (e.g. "..") is rejected before touching git # Steps: run create '..'; assert non-zero exit and stderr mentions "safe slug" @@ -479,6 +528,34 @@ case_gc_merged_skips_dirty_without_force() { fi } +case_gc_merged_exact_match_ignores_regex_metachar_collision() { + # behavior: gc --merged uses EXACT branch-name matching, so a branch containing regex metacharacters + # (e.g. a literal dot) must not false-positive match an unrelated already-merged branch whose + # name happens to satisfy the metachar as a wildcard (a prior version used `grep -E` and a + # branch "a.b" would incorrectly match a merged branch literally named "axb") + # Steps: create an unrelated already-merged plain branch "axb"; create worktree branch "a.b" and give it + # a commit master does not have (genuinely unmerged); run gc --merged; assert "a.b" survives + local name="worktree gc: --merged exact-matches branch names, ignoring regex-metachar collisions" + should_run "$name" || return 0 + local store work wt_path status=0 + store="$tmp_root/state-gc-regex" + work="$tmp_root/work-gc-regex" + make_work_repo "$work" + git -C "$work" branch axb + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create 'a.b' --cd "$work" 2>/dev/null | tail -1)" + git -C "$wt_path" config user.email t@e.com + git -C "$wt_path" config user.name t + printf 'unmerged\n' > "$wt_path/unmerged.txt" + git -C "$wt_path" add unmerged.txt + git -C "$wt_path" commit -q -m unmerged + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --merged --cd "$work" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && -d "$wt_path" && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status=$status wt_path_exists=$([[ -d "$wt_path" ]] && echo yes || echo no)" + fi +} + case_gc_max_age_days_filters() { # behavior: gc --max-age-days N only removes entries older than N days; a freshly created entry is kept # Steps: create feat/fresh; run gc --max-age-days 30 immediately afterward; assert directory and manifest entry survive @@ -566,6 +643,8 @@ case_create_from_base case_create_existing_branch_no_new_ref case_create_name_override_slug case_create_duplicate_slug_rejected +case_create_stale_manifest_duplicate_slug_rejected +case_create_rejects_unsafe_symlinked_state_root case_create_unsafe_slug_rejected case_create_help case_list_empty @@ -582,6 +661,7 @@ case_gc_removes_orphaned_manifest_entry case_gc_merged_flag case_gc_merged_does_not_self_remove_when_invoked_from_inside case_gc_merged_skips_dirty_without_force +case_gc_merged_exact_match_ignores_regex_metachar_collision case_gc_max_age_days_filters case_gc_max_age_days_removes_aged_entry case_gc_max_age_days_rejects_non_integer From 27c654721050d32bec83cfbf8ab6a753ab4b1b32 Mon Sep 17 00:00:00 2001 From: screenleon Date: Thu, 2 Jul 2026 14:36:44 +0900 Subject: [PATCH 06/10] fix(CC-014): reject missing --cd operand instead of silently defaulting Addresses fourth-round pr-gate finding (risk-reviewer block; corroborated by critic block-soft and qa-tester block): --cd on create/list/remove/gc accepted a missing value and fell back to the invoking pmctl's own repo (work_dir="${args[i+1]:-}" -> empty -> "${work_dir:-$repo_root}"), so a malformed `pmctl worktree remove --cd` (or gc) with the value omitted could silently operate on the pm-dispatch checkout instead of failing -- risky for subcommands that delete worktrees. --cd now requires a non-empty operand at all four call sites, matching the existing --from/--name validation style, and exits 2 before touching git or the manifest. Added a missing-operand regression test per subcommand. --- scripts/lib/pmctl-worktree.sh | 19 ++++++++-- scripts/test-pmctl-worktree.sh | 69 ++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/scripts/lib/pmctl-worktree.sh b/scripts/lib/pmctl-worktree.sh index 1915586..6214c4c 100644 --- a/scripts/lib/pmctl-worktree.sh +++ b/scripts/lib/pmctl-worktree.sh @@ -176,6 +176,7 @@ pmctl_worktree_create() { ;; --cd) work_dir="${args[$((i+1))]:-}" + [[ -n "$work_dir" ]] || { printf 'pmctl worktree create: --cd requires a directory\n' >&2; return 2; } i=$((i+2)) ;; -h|--help) @@ -271,7 +272,11 @@ pmctl_worktree_list() { local i=0 while [[ $i -lt ${#args[@]} ]]; do case "${args[$i]}" in - --cd) work_dir="${args[$((i+1))]:-}"; i=$((i+2)) ;; + --cd) + work_dir="${args[$((i+1))]:-}" + [[ -n "$work_dir" ]] || { printf 'pmctl worktree list: --cd requires a directory\n' >&2; return 2; } + i=$((i+2)) + ;; --json) json_out=1; i=$((i+1)) ;; -h|--help) pmctl_worktree_usage; return 0 ;; *) i=$((i+1)) ;; @@ -316,7 +321,11 @@ pmctl_worktree_remove() { while [[ $i -lt ${#args[@]} ]]; do case "${args[$i]}" in --force) force=1; i=$((i+1)) ;; - --cd) work_dir="${args[$((i+1))]:-}"; i=$((i+2)) ;; + --cd) + work_dir="${args[$((i+1))]:-}" + [[ -n "$work_dir" ]] || { printf 'pmctl worktree remove: --cd requires a directory\n' >&2; return 2; } + i=$((i+2)) + ;; -h|--help) pmctl_worktree_usage; return 0 ;; *) rest+=("${args[$i]}"); i=$((i+1)) ;; esac @@ -384,7 +393,11 @@ pmctl_worktree_gc() { fi i=$((i+2)) ;; - --cd) work_dir="${args[$((i+1))]:-}"; i=$((i+2)) ;; + --cd) + work_dir="${args[$((i+1))]:-}" + [[ -n "$work_dir" ]] || { printf 'pmctl worktree gc: --cd requires a directory\n' >&2; return 2; } + i=$((i+2)) + ;; -h|--help) pmctl_worktree_usage; return 0 ;; *) i=$((i+1)) ;; esac diff --git a/scripts/test-pmctl-worktree.sh b/scripts/test-pmctl-worktree.sh index e46a8c8..8a348d3 100755 --- a/scripts/test-pmctl-worktree.sh +++ b/scripts/test-pmctl-worktree.sh @@ -241,6 +241,71 @@ case_create_help() { fi } +case_create_missing_cd_value() { + # behavior: create --cd with no following operand exits 2 instead of silently falling back to the + # invoking pmctl's own repo (a malformed --cd must never resolve to a default target -- + # that default could be the wrong repo for a command that creates a checkout) + # Steps: run create feat/x --cd (no value after --cd, nothing else follows); assert exit 2 and + # stderr says --cd requires a directory + local name="worktree create: missing --cd operand exits 2 instead of defaulting to another repo" + should_run "$name" || return 0 + local err status=0 + err="$tmp_root/cdmiss1.err" + PM_DISPATCH_STATE_ROOT="$tmp_root/state-cd-missing-guard" "$PMCTL" worktree create feat/x --cd > /dev/null 2> "$err" || status=$? + if [[ "$status" -eq 2 && "$(<"$err")" == *"--cd requires a directory"* ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + +case_list_missing_cd_value() { + # behavior: list --cd with no following operand exits 2 instead of silently defaulting + # Steps: run list --cd (no value); assert exit 2 and stderr says --cd requires a directory + local name="worktree list: missing --cd operand exits 2 instead of defaulting to another repo" + should_run "$name" || return 0 + local err status=0 + err="$tmp_root/cdmiss2.err" + PM_DISPATCH_STATE_ROOT="$tmp_root/state-cd-missing-guard" "$PMCTL" worktree list --cd > /dev/null 2> "$err" || status=$? + if [[ "$status" -eq 2 && "$(<"$err")" == *"--cd requires a directory"* ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + +case_remove_missing_cd_value() { + # behavior: remove --cd with no following operand exits 2 BEFORE any destructive git/manifest + # operation, instead of silently defaulting to another repo's worktree registry + # Steps: run remove sometarget --cd (no value); assert exit 2 and stderr says --cd requires a directory + local name="worktree remove: missing --cd operand exits 2 instead of defaulting to another repo" + should_run "$name" || return 0 + local err status=0 + err="$tmp_root/cdmiss3.err" + PM_DISPATCH_STATE_ROOT="$tmp_root/state-cd-missing-guard" "$PMCTL" worktree remove sometarget --cd > /dev/null 2> "$err" || status=$? + if [[ "$status" -eq 2 && "$(<"$err")" == *"--cd requires a directory"* ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + +case_gc_missing_cd_value() { + # behavior: gc --cd with no following operand exits 2 BEFORE any destructive git/manifest + # operation, instead of silently defaulting to another repo's worktree registry + # Steps: run gc --cd (no value); assert exit 2 and stderr says --cd requires a directory + local name="worktree gc: missing --cd operand exits 2 instead of defaulting to another repo" + should_run "$name" || return 0 + local err status=0 + err="$tmp_root/cdmiss4.err" + PM_DISPATCH_STATE_ROOT="$tmp_root/state-cd-missing-guard" "$PMCTL" worktree gc --cd > /dev/null 2> "$err" || status=$? + if [[ "$status" -eq 2 && "$(<"$err")" == *"--cd requires a directory"* ]]; then + pass "$name" + else + fail "$name" "status=$status err=$(<"$err")" + fi +} + case_list_empty() { # behavior: list on an empty registry prints a human-readable "no worktrees" message # Steps: run list on a fresh repo with no registered worktrees; assert output contains "No registered worktrees." @@ -647,15 +712,19 @@ case_create_stale_manifest_duplicate_slug_rejected case_create_rejects_unsafe_symlinked_state_root case_create_unsafe_slug_rejected case_create_help +case_create_missing_cd_value case_list_empty +case_list_missing_cd_value case_list_json_valid case_list_text_table case_list_cross_worktree_identity case_remove_requires_target +case_remove_missing_cd_value case_remove_unknown_target case_remove_success case_remove_dirty_requires_force case_gc_no_worktrees +case_gc_missing_cd_value case_gc_dry_run_no_mutation case_gc_removes_orphaned_manifest_entry case_gc_merged_flag From fe82ba92c16801af124a2e022174138cf8b8712b Mon Sep 17 00:00:00 2001 From: screenleon Date: Thu, 2 Jul 2026 14:47:22 +0900 Subject: [PATCH 07/10] fix(CC-014): correct BSD/macOS date fallback timestamp parsing in gc --max-age-days Addresses fifth-round pr-gate finding (qa-tester block; critic advise). The `date -jf` fallback used for gc --max-age-days on platforms without GNU `date -d` stripped the timezone suffix with `${created_ts%%[+-]*}` (greedy, longest-suffix match) instead of `${created_ts%[+-]*}` (shortest match). For an ISO timestamp like "2026-07-02T13:27:18+09:00" the greedy form strips from the FIRST "-" in the string (inside the date portion itself), leaving only "2026" -- so the fallback path silently computed a wrong epoch and aged worktrees were never recognized as aged on that path. Added a regression test that installs a fake `date` on PATH which rejects `-d` (forcing the fallback branch to actually run) and re-derives the epoch through the real system date for `-jf` calls -- this proves both that the fallback branch executes and that it parses the timestamp correctly, on Linux CI where the primary `date -d` path would otherwise always be taken. Verified the test fails against the pre-fix `%%` form before restoring the fix. --- scripts/lib/pmctl-worktree.sh | 8 ++++- scripts/test-pmctl-worktree.sh | 53 ++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/scripts/lib/pmctl-worktree.sh b/scripts/lib/pmctl-worktree.sh index 6214c4c..f92ae0c 100644 --- a/scripts/lib/pmctl-worktree.sh +++ b/scripts/lib/pmctl-worktree.sh @@ -439,7 +439,13 @@ pmctl_worktree_gc() { should_remove=1; destructive=1; reason="branch merged" elif [[ "$max_age_days" -gt 0 ]]; then local created_epoch - created_epoch="$(date -d "$created_ts" +%s 2>/dev/null || date -jf '%Y-%m-%dT%H:%M:%S' "${created_ts%%[+-]*}" +%s 2>/dev/null || echo "$now_epoch")" + # Strip only the trailing timezone offset (the LAST +/- in the string, + # via the shortest-suffix-match `%` form) before handing to BSD/macOS + # `date -jf`, which has no offset syntax. Using the greedy `%%` form + # here was a bug: it strips from the FIRST `-` in the string, which + # for an ISO timestamp is inside the date portion itself + # ("2026-07-02T..." -> greedy strip left only "2026"). + created_epoch="$(date -d "$created_ts" +%s 2>/dev/null || date -jf '%Y-%m-%dT%H:%M:%S' "${created_ts%[+-]*}" +%s 2>/dev/null || echo "$now_epoch")" age_seconds=$(( now_epoch - created_epoch )) if [[ "$age_seconds" -ge "$max_age_seconds" ]]; then should_remove=1; destructive=1; reason="older than $max_age_days day(s)" diff --git a/scripts/test-pmctl-worktree.sh b/scripts/test-pmctl-worktree.sh index 8a348d3..7b70b03 100755 --- a/scripts/test-pmctl-worktree.sh +++ b/scripts/test-pmctl-worktree.sh @@ -664,6 +664,58 @@ case_gc_max_age_days_removes_aged_entry() { fi } +case_gc_max_age_days_bsd_fallback_parses_correctly() { + # behavior: when `date -d` is unavailable (the BSD/macOS case), gc --max-age-days falls back to + # `date -jf '%Y-%m-%dT%H:%M:%S' "${created_ts%[+-]*}"` -- this must strip ONLY the trailing + # timezone offset, not truncate the whole ISO timestamp down to just the year (a prior `%%` + # greedy-strip bug did exactly that, since the date portion itself contains "-") + # Steps: install a fake `date` on PATH that rejects `-d` (forcing the fallback branch) and re-derives + # the epoch for `-jf` calls via the real system date, so this test proves BOTH that the fallback + # branch actually executes AND that it computes the correct (not year-only) epoch; assert an + # aged entry is removed and a fresh entry survives under the SAME fallback-only `date` + local name="worktree gc: --max-age-days BSD-fallback path strips only the timezone, not the whole date" + should_run "$name" || return 0 + local store work wt_path_aged wt_path_fresh reg_dir manifest old_ts fake_bin status=0 + + fake_bin="$tmp_root/fake-bsd-date-bin" + mkdir -p "$fake_bin" + cat > "$fake_bin/date" <<'EOF' +#!/usr/bin/env bash +# Fake BSD-style `date`: rejects -d (forcing callers onto the -jf fallback +# path), and implements -jf by re-deriving the epoch through the real +# system date binary -- so this stub proves the -jf branch actually ran +# with a correctly-stripped timestamp, not just that SOME epoch came out. +if [[ "$1" == "-d" ]]; then + exit 1 +fi +if [[ "$1" == "-jf" ]]; then + shift 2 + exec /usr/bin/date -d "$1" "${@:2}" +fi +exec /usr/bin/date "$@" +EOF + chmod +x "$fake_bin/date" + + store="$tmp_root/state-gc-age-bsd" + work="$tmp_root/work-gc-age-bsd" + make_work_repo "$work" + wt_path_aged="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/bsd-aged --cd "$work" 2>/dev/null | tail -1)" + wt_path_fresh="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/bsd-fresh --cd "$work" 2>/dev/null | tail -1)" + reg_dir="$(reg_dir_for "$store" "$work")" + manifest="$reg_dir/manifest.jsonl" + old_ts="$(date -d '60 days ago' -Is)" + jq -c --arg ts "$old_ts" --arg slug feat-bsd-aged 'if .slug == $slug then .created_ts = $ts else . end' "$manifest" \ + > "$manifest.new" && mv "$manifest.new" "$manifest" + + PATH="$fake_bin:$PATH" PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --max-age-days 30 --cd "$work" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && ! -d "$wt_path_aged" && -d "$wt_path_fresh" \ + && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status=$status aged_exists=$([[ -d "$wt_path_aged" ]] && echo yes || echo no) fresh_exists=$([[ -d "$wt_path_fresh" ]] && echo yes || echo no)" + fi +} + case_gc_max_age_days_rejects_non_integer() { # behavior: gc --max-age-days with a non-integer value is rejected with exit 2 and a documented error # Steps: run gc --max-age-days nope; assert exit 2 and stderr mentions the requirement @@ -733,6 +785,7 @@ case_gc_merged_skips_dirty_without_force case_gc_merged_exact_match_ignores_regex_metachar_collision case_gc_max_age_days_filters case_gc_max_age_days_removes_aged_entry +case_gc_max_age_days_bsd_fallback_parses_correctly case_gc_max_age_days_rejects_non_integer case_gc_prunes_git_state From eef2eb5d9a3e5e3fa499c054a4848c6fd38ee677 Mon Sep 17 00:00:00 2001 From: screenleon Date: Thu, 2 Jul 2026 15:01:29 +0900 Subject: [PATCH 08/10] fix(CC-014): atomic manifest read-modify-write, closing the remove/gc/create race Addresses sixth-round pr-gate findings (risk-reviewer block; critic and architecture-reviewer block-soft; qa-tester block on missing concurrency coverage) -- all four independently converged on the same root cause. remove/gc read the manifest unlocked to DECIDE what to remove, then wrote that decision's full-content snapshot back under a lock that only protected the final `mv`. A `create` that appended between the read and the write was silently overwritten out of existence: the checkout stayed on disk but `pmctl worktree list/gc` could no longer see it. Replaced the "read snapshot -> compute full replacement -> lock only the write" pattern (pmctl_worktree_manifest_rewrite, now removed) with pmctl_worktree_manifest_remove_slugs(reg_dir, slug...): a single locked critical section that re-reads the manifest, filters out only the given slugs, and writes back -- all inside one lock acquisition. remove commits one slug; gc accumulates removed_slugs during its (still unlocked) decision pass and commits them all in one call at the end. Either way, any entry a concurrent create appended after the decision-time read is never in the removal set, so it survives the commit regardless of when it landed relative to that read. remove also now fails (nonzero exit) if manifest cleanup fails after the git worktree is gone, instead of reporting success with a stale registry (critic/risk-reviewer medium finding, folded in with the same commit since it's the same code path). Added a deterministic regression test that calls the manifest primitives directly to force the exact interleaving (decision read -> concurrent append -> removal commit) without relying on OS-level thread timing, and asserts the concurrently-appended entry survives. --- commands/using-git-worktrees.md | 2 +- scripts/lib/pmctl-worktree.sh | 76 ++++++++++++++++++++------------ scripts/test-commands.sh | 1 + scripts/test-pmctl-worktree.sh | 77 +++++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+), 29 deletions(-) diff --git a/commands/using-git-worktrees.md b/commands/using-git-worktrees.md index 59e2a1f..437cd0f 100644 --- a/commands/using-git-worktrees.md +++ b/commands/using-git-worktrees.md @@ -11,7 +11,7 @@ Set up an isolated `git worktree` for parallel development using `pmctl worktree **Prerequisite: this requires git.** `pmctl worktree` is a thin wrapper over `git worktree add/remove/prune` — there is no non-git fallback. The target directory must already be a git repository; `pmctl worktree create` fails immediately (git itself rejects the operation) if it is not. -Worktrees are stored out-of-repo, under the state store (`~/.local/share/pm-dispatch/state/projects//worktrees/checkouts/`), not inside the repo — so they never show up in `git status`, don't need a `.gitignore` entry, and survive `git clean`. The registry (manifest) resolves to the **same partition** whether `pmctl worktree` is invoked from the primary checkout or from inside one of the linked worktrees it created, so `pmctl worktree list` always shows the full picture regardless of which checkout you run it from. +Worktrees are stored out-of-repo, under the state store (`~/.local/share/pm-dispatch/state/projects//worktrees/checkouts/`), not inside the repo — so they never show up in `git status`, don't need a `.gitignore` entry, and survive `git clean`. The registry (manifest) resolves to the **same partition** whether `pmctl worktree` is invoked from the primary checkout or from inside one of the linked worktrees it created, so `pmctl worktree list` always shows the full picture regardless of which checkout you run it from. Manifest writes (`create` appending, `remove`/`gc` removing entries) are serialized under a single lock and always commit against the manifest's current on-disk state, not a snapshot taken earlier — running `pmctl worktree create` concurrently with `remove`/`gc` from another shell or process is safe. ## When to use diff --git a/scripts/lib/pmctl-worktree.sh b/scripts/lib/pmctl-worktree.sh index f92ae0c..697cb34 100644 --- a/scripts/lib/pmctl-worktree.sh +++ b/scripts/lib/pmctl-worktree.sh @@ -129,24 +129,41 @@ pmctl_worktree_manifest_append() { return "$rc" } -# _pmctl_worktree_manifest_rewrite_inner -# Atomically replace manifest.jsonl with tmp_content_file's contents, run -# under the same lock as append so remove/gc never race a concurrent create. -_pmctl_worktree_manifest_rewrite_inner() { - local manifest="$1" tmp_content="$2" - mv -f "$tmp_content" "$manifest" +# _pmctl_worktree_manifest_remove_slugs_inner +# Runs under the SAME lock as append: re-reads the CURRENT manifest content +# (not a snapshot taken before acquiring the lock) and writes back only the +# entries whose slug is NOT in the removal set. Read + filter + write all +# happen inside one locked critical section, so a `create` that appends +# between an earlier (unlocked) decision-making read and this commit is +# never silently dropped -- its new slug simply isn't in the removal set, +# so it survives regardless of when it was appended relative to that +# earlier read. +_pmctl_worktree_manifest_remove_slugs_inner() { + local reg_dir="$1" manifest="$1/manifest.jsonl" tmp current new_content slugs_json + shift + current="" + [[ -f "$manifest" ]] && current="$(cat "$manifest")" + [[ -n "$current" ]] || return 0 + slugs_json="$(printf '%s\n' "$@" | jq -R . | jq -s -c .)" + new_content="$(printf '%s\n' "$current" | jq -c --argjson slugs "$slugs_json" \ + 'select(.slug as $s | ($slugs | index($s)) == null)')" + tmp="$(mktemp "$reg_dir/.manifest.XXXXXX")" || return 1 + printf '%s' "$new_content" > "$tmp" + mv -f "$tmp" "$manifest" } -# pmctl_worktree_manifest_rewrite -pmctl_worktree_manifest_rewrite() { - local reg_dir="$1" new_content="$2" manifest tmp rc=0 - manifest="$reg_dir/manifest.jsonl" +# pmctl_worktree_manifest_remove_slugs +# Atomic (locked read-modify-write) removal of one or more slugs from the +# manifest. This is the ONLY way remove/gc should mutate the manifest -- +# never pass a pre-filtered full-content blob to be written blind, since +# that would silently discard any entry a concurrent `create` appended +# after the caller's own (unlocked) read. +pmctl_worktree_manifest_remove_slugs() { + local reg_dir="$1" + shift + [[ $# -gt 0 ]] || return 0 mkdir -p "$reg_dir" 2>/dev/null || { printf 'pmctl worktree: mkdir failed: %s\n' "$reg_dir" >&2; return 1; } - tmp="$(mktemp "$reg_dir/.manifest.XXXXXX")" || return 1 - printf '%s' "$new_content" > "$tmp" - serialize_with_lock "$reg_dir/manifest" _pmctl_worktree_manifest_rewrite_inner "$manifest" "$tmp" || rc=$? - [[ -f "$tmp" ]] && rm -f "$tmp" - return "$rc" + serialize_with_lock "$reg_dir/manifest" _pmctl_worktree_manifest_remove_slugs_inner "$reg_dir" "$@" } # pmctl_worktree_manifest_read @@ -341,7 +358,7 @@ pmctl_worktree_remove() { pmctl_worktree_ensure_writer "$repo_root" || return $? pmctl_worktree_ensure_root_safe || return 1 - local reg_dir manifest_content match_line match_path + local reg_dir manifest_content match_line match_path match_slug reg_dir="$(sw_project_worktree_dir "$work_dir")" || { printf 'pmctl worktree remove: cannot resolve worktree registry dir from %s\n' "$work_dir" >&2 return 1 @@ -353,6 +370,7 @@ pmctl_worktree_remove() { return 1 fi match_path="$(jq -r '.path' <<<"$match_line")" + match_slug="$(jq -r '.slug' <<<"$match_line")" local git_rm_args=(worktree remove) [[ "$force" -eq 1 ]] && git_rm_args+=(--force) @@ -365,11 +383,10 @@ pmctl_worktree_remove() { fi git -C "$work_dir" worktree prune 2>/dev/null || true - local remaining - remaining="$(printf '%s\n' "$manifest_content" | jq -c --arg t "$target" 'select(.slug != $t and .branch != $t)')" - pmctl_worktree_manifest_rewrite "$reg_dir" "$remaining" || { - printf 'pmctl worktree remove: worktree removed but manifest cleanup failed -- run '\''pmctl worktree gc'\'' to reconcile\n' >&2 - } + if ! pmctl_worktree_manifest_remove_slugs "$reg_dir" "$match_slug"; then + printf 'pmctl worktree remove: worktree removed but manifest cleanup failed for slug %s -- run '\''pmctl worktree gc'\'' to reconcile\n' "$match_slug" >&2 + return 1 + fi printf 'removed %s (%s)\n' "$target" "$match_path" } @@ -422,7 +439,7 @@ pmctl_worktree_gc() { # would flag the worktree's own checked-out branch as removable. main_root="$(_pmctl_worktree_main_root "$work_dir")" || main_root="$work_dir" - local kept_lines="" removed_count=0 skipped_dirty=0 + local removed_slugs=() removed_count=0 skipped_dirty=0 while IFS= read -r line; do [[ -n "$line" ]] || continue local slug branch path created_ts age_seconds should_remove=0 reason="" destructive=0 @@ -469,7 +486,6 @@ pmctl_worktree_gc() { if ! git -C "$work_dir" "${rm_args[@]}" 2>/dev/null; then printf 'skipping %s (%s): has uncommitted changes -- pass gc --force to remove anyway\n' "$slug" "$path" skipped_dirty=$((skipped_dirty+1)) - kept_lines="${kept_lines}${line}"$'\n' continue fi elif [[ -d "$path" ]]; then @@ -478,17 +494,21 @@ pmctl_worktree_gc() { git -C "$work_dir" worktree remove --force "$path" 2>/dev/null || true fi removed_count=$((removed_count+1)) + removed_slugs+=("$slug") printf 'removed %s (%s): %s\n' "$slug" "$path" "$reason" - else - kept_lines="${kept_lines}${line}"$'\n' fi done <<<"$manifest_content" git -C "$work_dir" worktree prune 2>/dev/null || true - if [[ "$dry_run" -eq 0 ]]; then - pmctl_worktree_manifest_rewrite "$reg_dir" "$kept_lines" || { - printf 'pmctl worktree gc: manifest rewrite failed after removal\n' >&2 + if [[ "$dry_run" -eq 0 && "${#removed_slugs[@]}" -gt 0 ]]; then + # Commits the removal atomically against the CURRENT manifest (see + # pmctl_worktree_manifest_remove_slugs) -- any worktree a concurrent + # `create` registered after the read at the top of this function is + # NOT in removed_slugs, so it survives this write even though our + # should_remove decisions above were made from a stale snapshot. + pmctl_worktree_manifest_remove_slugs "$reg_dir" "${removed_slugs[@]}" || { + printf 'pmctl worktree gc: manifest cleanup failed after removal -- some removed worktrees may still be listed; re-run '\''pmctl worktree gc'\'' to reconcile\n' >&2 return 1 } fi diff --git a/scripts/test-commands.sh b/scripts/test-commands.sh index d6434d8..d84dd8d 100755 --- a/scripts/test-commands.sh +++ b/scripts/test-commands.sh @@ -364,6 +364,7 @@ should_run "using-git-worktrees: documents remove subcommand" && assert_file_con should_run "using-git-worktrees: documents gc subcommand" && assert_file_contains "using-git-worktrees: documents gc subcommand" "$USING_GIT_WORKTREES" "pmctl worktree gc" && pass "using-git-worktrees: documents gc subcommand" should_run "using-git-worktrees: documents --force is destructive" && assert_file_contains "using-git-worktrees: documents --force is destructive" "$USING_GIT_WORKTREES" "destructive" && pass "using-git-worktrees: documents --force is destructive" should_run "using-git-worktrees: documents cross-worktree identity guarantee" && assert_file_contains "using-git-worktrees: documents cross-worktree identity guarantee" "$USING_GIT_WORKTREES" "same partition" && pass "using-git-worktrees: documents cross-worktree identity guarantee" +should_run "using-git-worktrees: documents concurrent manifest write safety" && assert_file_contains "using-git-worktrees: documents concurrent manifest write safety" "$USING_GIT_WORKTREES" "serialized under a single lock" && pass "using-git-worktrees: documents concurrent manifest write safety" should_run "using-git-worktrees: documents orphan recovery via gc" && assert_file_contains "using-git-worktrees: documents orphan recovery via gc" "$USING_GIT_WORKTREES" "git worktree prune" && pass "using-git-worktrees: documents orphan recovery via gc" should_run "using-git-worktrees: excludes --parallel gate reviewer isolation from scope" && assert_file_contains "using-git-worktrees: excludes --parallel gate reviewer isolation from scope" "$USING_GIT_WORKTREES" "does not touch the \`--parallel\` PR gate" && pass "using-git-worktrees: excludes --parallel gate reviewer isolation from scope" should_run "using-git-worktrees: no CC ticket references" && assert_not_contains "using-git-worktrees: no CC ticket references" "$USING_GIT_WORKTREES" "CC-" diff --git a/scripts/test-pmctl-worktree.sh b/scripts/test-pmctl-worktree.sh index 7b70b03..9a4511d 100755 --- a/scripts/test-pmctl-worktree.sh +++ b/scripts/test-pmctl-worktree.sh @@ -39,6 +39,36 @@ reg_dir_for() { _ "$REPO_ROOT" "$work" } +# manifest_append_raw +# Calls pmctl_worktree_manifest_append directly (bypassing `pmctl worktree +# create`'s git-checkout step) so a test can simulate a concurrent create's +# manifest write landing at a precise point in another operation's sequence. +manifest_append_raw() { + local reg_dir="$1" json_line="$2" + bash -c ' + repo_root="$1"; reg_dir="$2"; json_line="$3" + . "$repo_root/scripts/lib/portable.sh" + . "$repo_root/scripts/lib/state-writer.sh" + . "$repo_root/scripts/lib/pmctl-worktree.sh" + pmctl_worktree_manifest_append "$reg_dir" "$json_line" + ' _ "$REPO_ROOT" "$reg_dir" "$json_line" +} + +# manifest_remove_slugs_raw +# Calls pmctl_worktree_manifest_remove_slugs directly -- the same locked +# read-modify-write primitive `remove`/`gc` commit through. +manifest_remove_slugs_raw() { + local reg_dir="$1" + shift + bash -c ' + repo_root="$1"; reg_dir="$2"; shift 2 + . "$repo_root/scripts/lib/portable.sh" + . "$repo_root/scripts/lib/state-writer.sh" + . "$repo_root/scripts/lib/pmctl-worktree.sh" + pmctl_worktree_manifest_remove_slugs "$reg_dir" "$@" + ' _ "$REPO_ROOT" "$reg_dir" "$@" +} + case_create_requires_branch() { # behavior: pmctl worktree create with no arg exits 2 and prints usage # Steps: run create with only --cd; assert exit 2, stderr has " is required" and "usage:" @@ -459,6 +489,52 @@ case_remove_dirty_requires_force() { fi } +case_manifest_remove_slugs_survives_concurrent_append() { + # behavior: pmctl_worktree_manifest_remove_slugs (the primitive remove/gc commit their removal + # through) reads the manifest FRESH inside its lock, so an entry appended AFTER an + # earlier decision-time read but BEFORE the removal commits is never silently dropped -- + # this is the exact race pattern remove/gc go through: read manifest (unlocked, to decide + # what to remove) -> [a concurrent create can land here] -> commit removal (locked) + # Steps: register A and B; take a manifest snapshot (simulating remove/gc's decision-time read); + # THEN append C directly (simulating a concurrent `create` landing in the race window between + # that read and the commit below); THEN commit removal of A via the same primitive remove/gc + # use; assert the final manifest has B and C but NOT A -- proving C survived even though it + # was appended after the snapshot the removal decision was based on + local name="worktree manifest: concurrent create appended during a remove/gc race window survives" + should_run "$name" || return 0 + local store work reg_dir status=0 + store="$tmp_root/state-manifest-race" + work="$tmp_root/work-manifest-race" + make_work_repo "$work" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/race-a --cd "$work" > /dev/null 2>&1 + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/race-b --cd "$work" > /dev/null 2>&1 + reg_dir="$(reg_dir_for "$store" "$work")" + + # Simulate remove/gc's unlocked decision-time read (its content is irrelevant here -- what matters + # is that this read happens BEFORE the concurrent append below, mirroring the real race window). + cat "$reg_dir/manifest.jsonl" > /dev/null + + # Concurrent create landing in the race window: appends C AFTER the decision read above but + # BEFORE the removal of A commits below. + local race_json + race_json="$(jq -cn --arg v race-c '{"slug":$v,"branch":"feat/race-c","path":"/tmp/race-c","created_ts":"2026-01-01T00:00:00+00:00"}')" + manifest_append_raw "$reg_dir" "$race_json" + + # Commit removal of A via the exact same primitive remove/gc use. + manifest_remove_slugs_raw "$reg_dir" feat-race-a > /dev/null 2>&1 || status=$? + + local final_json has_a has_b has_c + final_json="$(wt_list_json "$store" "$work")" + has_a="$(jq '[.[] | select(.slug=="feat-race-a")] | length' <<<"$final_json")" + has_b="$(jq '[.[] | select(.slug=="feat-race-b")] | length' <<<"$final_json")" + has_c="$(jq '[.[] | select(.slug=="race-c")] | length' <<<"$final_json")" + if [[ "$status" -eq 0 && "$has_a" -eq 0 && "$has_b" -eq 1 && "$has_c" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status=$status has_a=$has_a has_b=$has_b has_c=$has_c final=$final_json" + fi +} + case_gc_no_worktrees() { # behavior: gc on an empty registry is a no-op that reports so and exits 0 # Steps: run gc on a repo with no registered worktrees; assert exit 0 and output mentions no registered worktrees @@ -775,6 +851,7 @@ case_remove_missing_cd_value case_remove_unknown_target case_remove_success case_remove_dirty_requires_force +case_manifest_remove_slugs_survives_concurrent_append case_gc_no_worktrees case_gc_missing_cd_value case_gc_dry_run_no_mutation From 0adecf1fbd84b642bdbc9d2d7cc7b36d5a0c146d Mon Sep 17 00:00:00 2001 From: screenleon Date: Thu, 2 Jul 2026 15:11:55 +0900 Subject: [PATCH 09/10] fix(CC-014): fixed-string path matching for git-worktree tracked-path checks Addresses seventh-round pr-gate finding (security-reviewer block, hard gate, reproduced with a `[`-containing state root; risk-reviewer block; qa-tester block; critic block-soft) -- the same regex-vs-data bug class already fixed once for branch names in gc --merged, missed here because it also existed independently in two other call sites that interpolate a filesystem path (not a branch name) into `grep -q "^worktree $path\$"`. PM_DISPATCH_STATE_ROOT is arbitrary external data, not something this tool controls the syntax of -- a state root containing `[` or other regex metacharacters corrupted the pattern, so `gc`'s "is this path still tracked by git" check could false-negative a live, dirty worktree as "git no longer tracks this" and force-remove it with no --force confirmation, discarding uncommitted changes. The same interpolation existed in `create`'s duplicate-path check (non-destructive there, but same bug class). Both call sites now use `grep -Fxq` (fixed-string, whole-line match) against `git worktree list --porcelain` output instead of a regex. Added a regression test that reproduces the exact scenario security- reviewer used (PM_DISPATCH_STATE_ROOT containing `[`, a dirty live worktree, plain `gc`) and asserts the worktree survives. Verified the test fails against the pre-fix regex form before restoring the fix. --- scripts/lib/pmctl-worktree.sh | 13 +++++++++++-- scripts/test-pmctl-worktree.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/scripts/lib/pmctl-worktree.sh b/scripts/lib/pmctl-worktree.sh index 697cb34..d0d95f0 100644 --- a/scripts/lib/pmctl-worktree.sh +++ b/scripts/lib/pmctl-worktree.sh @@ -231,7 +231,11 @@ pmctl_worktree_create() { printf 'pmctl worktree create: a worktree already exists at %s (slug %s in use)\n' "$wt_path" "$slug" >&2 return 1 fi - if git -C "$work_dir" worktree list --porcelain 2>/dev/null | grep -q "^worktree $wt_path\$"; then + # `-Fxq` (fixed-string, whole-line match) -- NOT `grep -q "^worktree $path\$"`. + # A path is arbitrary data (PM_DISPATCH_STATE_ROOT can contain regex + # metacharacters like `[`), so treating it as a regex pattern can silently + # misclassify a tracked path as untracked or vice versa. + if git -C "$work_dir" worktree list --porcelain 2>/dev/null | grep -Fxq "worktree $wt_path"; then printf 'pmctl worktree create: git already tracks a worktree at %s\n' "$wt_path" >&2 return 1 fi @@ -450,7 +454,12 @@ pmctl_worktree_gc() { if [[ ! -d "$path" ]]; then should_remove=1; reason="path missing (orphaned manifest entry)" - elif ! git -C "$work_dir" worktree list --porcelain 2>/dev/null | grep -q "^worktree $path\$"; then + elif ! git -C "$work_dir" worktree list --porcelain 2>/dev/null | grep -Fxq "worktree $path"; then + # `-Fxq`, not a regex `grep -q "^worktree $path\$"`: $path is data (a + # symlink-free but otherwise arbitrary filesystem path derived from + # PM_DISPATCH_STATE_ROOT), and this decision gates a `git worktree + # remove --force` a few lines below -- a false "no longer tracked" + # here force-deletes a still-live, possibly dirty worktree. should_remove=1; reason="git no longer tracks this worktree" elif [[ "$merged_only" -eq 1 ]] && _pmctl_worktree_branch_is_merged "$main_root" "$branch"; then should_remove=1; destructive=1; reason="branch merged" diff --git a/scripts/test-pmctl-worktree.sh b/scripts/test-pmctl-worktree.sh index 9a4511d..10c114b 100755 --- a/scripts/test-pmctl-worktree.sh +++ b/scripts/test-pmctl-worktree.sh @@ -830,6 +830,34 @@ case_gc_prunes_git_state() { fi } +case_gc_path_with_regex_metachar_not_misclassified() { + # behavior: gc's "is this path still tracked by git" check must use fixed-string/exact matching, not + # regex -- the checkout path is built from PM_DISPATCH_STATE_ROOT, which is arbitrary data + # (not something the tool controls the syntax of), so a state root containing a regex + # metacharacter like `[` must not make `grep` misparse the pattern and false-negative a + # still-tracked, still-dirty LIVE worktree as "git no longer tracks this" -- which would + # then force-remove it and discard uncommitted changes with no --force confirmation + # Steps: use a PM_DISPATCH_STATE_ROOT containing `[`; create a worktree (its path inherits the `[`); + # make it dirty; run plain `gc` (no flags); assert the worktree and its uncommitted file + # survive and the manifest still has the entry -- proving it was correctly recognized as + # still tracked, not force-removed as a false orphan + local name="worktree gc: a checkout path containing a regex metacharacter is not misclassified as untracked" + should_run "$name" || return 0 + local store work wt_path status=0 + store="$tmp_root/state[meta" + work="$tmp_root/work-gc-metachar" + make_work_repo "$work" + wt_path="$(PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree create feat/metachar --cd "$work" 2>/dev/null | tail -1)" + printf 'dirty\n' > "$wt_path/dirty.txt" + PM_DISPATCH_STATE_ROOT="$store" "$PMCTL" worktree gc --cd "$work" > /dev/null 2>&1 || status=$? + if [[ "$status" -eq 0 && -d "$wt_path" && -f "$wt_path/dirty.txt" \ + && "$(wt_list_json "$store" "$work" | jq 'length')" -eq 1 ]]; then + pass "$name" + else + fail "$name" "status=$status wt_path_exists=$([[ -d "$wt_path" ]] && echo yes || echo no)" + fi +} + case_create_requires_branch case_create_new_branch case_create_from_base @@ -865,5 +893,6 @@ case_gc_max_age_days_removes_aged_entry case_gc_max_age_days_bsd_fallback_parses_correctly case_gc_max_age_days_rejects_non_integer case_gc_prunes_git_state +case_gc_path_with_regex_metachar_not_misclassified th_summary From 7d0fdbbfa0b4d128c53dd5a7aba1d7e51b42ff51 Mon Sep 17 00:00:00 2001 From: screenleon Date: Thu, 2 Jul 2026 15:26:42 +0900 Subject: [PATCH 10/10] docs(CC-014): close ticket in BACKLOG.md and MILESTONES.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Marks CC-014 terminal (✅ closed 2026-07-02 / ✅ done) with pr:#358 and an Outcome summary, per pr-gate GO on this branch. --- BACKLOG.md | 6 ++++-- MILESTONES.md | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/BACKLOG.md b/BACKLOG.md index bd96e0d..7a79ed9 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -13,7 +13,7 @@ CC-001/CC-002 were consumed by PR #24 fix bundle inline, with no standalone entr | CC-004 | 🟢 someday | test-pr-gate.sh docstring 格式統一 | ops | 2026-05-12 | pr:#38 | P3 | — | | CC-011 | 🟢 someday | sync-memory.sh + install 選項：symlink memory 到雲端資料夾實現跨裝置共用 | ux/memory | 2026-05-14 | — | — | — | | CC-012 | 🟢 someday | SessionStart hook：session 啟動時 pull 最新 memory（git/rsync）確保跨裝置同步 | ux/memory | 2026-05-14 | — | — | — | -| CC-014 | 🔵 active | repo 通用 worktree 平行開發工具：建立/清理 worktree + using-git-worktrees skill。v0.8.0 Phase 4 | arch | 2026-05-14 | — | — | — | +| CC-014 | ✅ closed 2026-07-02 | repo 通用 worktree 平行開發工具：建立/清理 worktree + using-git-worktrees skill。v0.8.0 Phase 4 | arch | 2026-05-14 | pr:#358 | — | — | | CC-015 | ⏸ deferred | `systematic-debugging` skill：結構化偵錯工作流 | ux | 2026-05-14 | — | — | — | | CC-018 | 🟢 someday | Codex quota 自動追蹤 + rate-limit 路徑統一（吸收 CC-269）：寫到 `~/.local/share/pm-dispatch/state/rate-limits.json`；解析 API response headers；token-usage.sh 加 Codex pool 顯示 | ux/token | 2026-05-14 | — | P3 | — | | CC-023 | ⏸ deferred | `coupling-reviewer`：PR gate 加入語言感知耦合分析（dependency-cruiser/gocyclo/coca） | ops/gate | 2026-05-14 | — | — | — | @@ -216,7 +216,7 @@ _Terminal_ (CC-378: swept OUT to `BACKLOG-ARCHIVE.md` by `scripts/archive-closed **Note**: 依賴 CC-011；建議與 CC-011 合入同一 PR（Phase 1 + Phase 2 同步落地，CC-012 無獨立實作意義）。 **Status note (CC-050 audit 2026-05-18)**: Downgraded from ⏸ deferred to 🟢 someday — depends on CC-011; no active plan. Re-evaluate together with CC-011. -## CC-014 — repo 通用 worktree 平行開發工具 +## CC-014 — repo 通用 worktree 平行開發工具 ✅ 2026-07-02 **Status note (v0.8.0 planning 2026-07-02)**: Re-activated (was downgraded to ⏸ deferred by the CC-050 audit 2026-05-18 for lacking an open branch) — assigned to v0.8.0 Phase 4. Scope broadened 2026-07-02: 不再侷限於 pr-gate reviewer 隔離，改為 repo 層級通用 worktree 工具，讓任一 ticket/分支都能快速建立、切換、清理獨立 worktree 以支援多票並行開發。 **Problem**: 目前開發者（與 `--parallel` PR gate 各 reviewer）都在同一 working tree 上工作，跨票並行開發時彼此的未 commit 變更、build 產物會互相干擾；沒有標準化的方式建立/清理獨立 worktree。 @@ -225,6 +225,8 @@ _Terminal_ (CC-378: swept OUT to `BACKLOG-ARCHIVE.md` by `scripts/archive-closed 1. `scripts/worktree-*.sh`（或等效 `pmctl` 子指令）：為指定 ticket/分支建立、列出、清理 worktree，統一命名慣例與清理時機（避免孤兒 worktree 殘留）。 2. `commands/using-git-worktrees.md` skill：指導開發者（人或 dispatch executor）如何用這些工具做功能分支平行開發。 3. 評估 `--parallel` PR gate 是否可改用同一套工具為每個 reviewer 建立獨立 worktree（原票聚焦點，現列為本票子項而非全部範圍）。 +**Outcome**: `pmctl worktree create/list/remove/gc` 落地，manifest 存於 state store（`sw_project_worktree_dir`，跨主 repo/linked worktree 同一 partition）；`commands/using-git-worktrees.md` skill 文件；36 個 focused test。`--parallel` gate reviewer 隔離（原需求 3）留待未來 follow-up ticket，未併入本次範圍。 +**See**: pr:#358 ## CC-015 — `systematic-debugging` skill diff --git a/MILESTONES.md b/MILESTONES.md index c7ddfc5..cfb04a8 100644 --- a/MILESTONES.md +++ b/MILESTONES.md @@ -55,7 +55,7 @@ | 票 | 摘要 | 狀態 | |----|------|------| -| CC-014 | repo 通用 worktree 建立/列出/清理工具 + `using-git-worktrees` skill，支援多票並行開發；並評估 `--parallel` PR gate 是否可沿用同一套工具為每個 reviewer 建立獨立 worktree | 🔵 active | +| CC-014 | repo 通用 worktree 建立/列出/清理工具 + `using-git-worktrees` skill，支援多票並行開發；`--parallel` PR gate reviewer 隔離整合留待未來 follow-up ticket，未併入本次範圍 | ✅ done pr:#358 | > 由 CC-050 稽核降級的 ⏸ deferred（無開放分支）重新啟用；2026-07-02 範圍由「pr-gate reviewer 隔離」擴大為「repo 通用 worktree 工具」；規劃時尚未有實作分支，範圍與時程由後續 `/pre-impl` 或直接 dispatch 時再收斂。