diff --git a/docs/agent/followups.md b/docs/agent/followups.md index cf91836..d77665c 100644 --- a/docs/agent/followups.md +++ b/docs/agent/followups.md @@ -14,13 +14,26 @@ > > **以 FU-4 的判据为准**:本地 tip **==** 或 **是任何一个**已合并 PR 的 `headRefOid` 的祖先 > (先 `git fetch origin pull/N/head` 把 head 抓到本地再算祖先)。这一条同时挡住上面两种错。 +> +> **⚠️ 再更正(实现时找到更好的)**:FU-4 的祖先算法是对的,但**实现用的不是它**。 +> GitHub 有 `GET /repos/{owner}/{repo}/commits/{sha}/pulls`,直接回答「哪个 PR 把这个 commit +> 引入了仓库」。它同样按 commit 判、两种错都没有,而且**每个分支只要一次 API 调用**、 +> 不用把所有 PR head 抓到本地 —— dry-run 因此保持零写入。四种形状实测通过: +> squash 后的 head ✓、分支中间的 commit ✓、CLOSED 未合并 → 无证据 ✓、从未开过 PR → 无证据 ✓。 +> 落地在 `safe-cleanup.sh` 的 `merged_pr_for()`。 - [x] FU-1 · B · src=PR#39 review [Low] · 2026-08-05 · git-guard merge-pr 拒绝 gh flag 用的是黑名单(--admin/--repo/-R 及其粘连形式)。黑名单追不上新 flag —— 以后 gh pr merge 若新增能绕过分支保护的 flag,这里不会自动知道。改成白名单(只放行 --squash/--merge/--rebase 等已知安全 flag)更耐久 · done=PR#47 -- [ ] FU-2 · B · src=2026-08-05 合并 #38/#39/#40/#41 后实测 · 2026-08-05 · safe-cleanup.sh 在 squash-merge 仓库里永远清不掉任何分支:本仓库 28 个本地分支,git branch --merged main 返回 0 个,因为 squash 后原 commit 不是 main 的祖先,而 safe-cleanup 只用 -d 永不 -D。这是继 #39(死代码)、#40(随机红灯)之后同一家族的第三个『守卫跑不起来』。正确改法:用 gh 核实『存在 headRefName==该分支且 state==MERGED 的 PR』作为已合并证据,再允许 -D;不能简单放开 -D -- [ ] FU-3 · C · src=PR#42 review [Low] · 2026-08-05 · check-version-sync.sh 只比对 plugin.json 与 SKILL.md 两处。今天 README 没有硬编码版本号(核过),所以没问题;但哪天 README 加上版本,这条守卫不会知道。在脚本里写一句把范围钉住:『目前只有这两处声明版本』 -- [ ] FU-4 · B · src=PR#42 review [Low] + 2026-08-05 清理 28 个分支的实测 · 2026-08-05 · 补充 FU-2 的实现要点(今天手工做过一遍,算法已验证):① git branch --merged 和 git cherry 在 squash 仓库里【全部失效】—— cherry 对 12 个分支全报『未在 main』,因为 squash 重写补丁、patch-id 永不匹配;② 可用判据是『本地 tip == 或 是 任何一个已合并 PR 的 headRefOid 的祖先』,要先 git fetch origin pull/N/head 把 head 抓到本地;③ 【不能只按分支名匹配 PR】—— work-pr18 / fix-pr18-round2 / worktree-agent-* 这三个分支名从没当过 PR head,但 tip 就是 PR#18/#23 的已合并 head,按名字匹配会漏掉;④ 反向风险(评审提的):分支名可复用,同名分支删掉重开后内容不同,旧 MERGED PR 仍在 —— 祖先检查恰好挡住这种情况(重开的 tip 不会是旧 head 的祖先),但若改成只按名字匹配就会误删 +- [x] FU-2 · B · src=2026-08-05 合并 #38/#39/#40/#41 后实测 · 2026-08-05 · safe-cleanup.sh 在 squash-merge 仓库里永远清不掉任何分支:本仓库 28 个本地分支,git branch --merged main 返回 0 个,因为 squash 后原 commit 不是 main 的祖先,而 safe-cleanup 只用 -d 永不 -D。这是继 #39(死代码)、#40(随机红灯)之后同一家族的第三个『守卫跑不起来』。正确改法:用 gh 核实『存在 headRefName==该分支且 state==MERGED 的 PR』作为已合并证据,再允许 -D;不能简单放开 -D · done=PR#45 +- [x] FU-3 · C · src=PR#42 review [Low] · 2026-08-05 · check-version-sync.sh 只比对 plugin.json 与 SKILL.md 两处。今天 README 没有硬编码版本号(核过),所以没问题;但哪天 README 加上版本,这条守卫不会知道。在脚本里写一句把范围钉住:『目前只有这两处声明版本』 · done=PR#43 +- [x] FU-4 · B · src=PR#42 review [Low] + 2026-08-05 清理 28 个分支的实测 · 2026-08-05 · 补充 FU-2 的实现要点(今天手工做过一遍,算法已验证):① git branch --merged 和 git cherry 在 squash 仓库里【全部失效】—— cherry 对 12 个分支全报『未在 main』,因为 squash 重写补丁、patch-id 永不匹配;② 可用判据是『本地 tip == 或 是 任何一个已合并 PR 的 headRefOid 的祖先』,要先 git fetch origin pull/N/head 把 head 抓到本地;③ 【不能只按分支名匹配 PR】—— work-pr18 / fix-pr18-round2 / worktree-agent-* 这三个分支名从没当过 PR head,但 tip 就是 PR#18/#23 的已合并 head,按名字匹配会漏掉;④ 反向风险(评审提的):分支名可复用,同名分支删掉重开后内容不同,旧 MERGED PR 仍在 —— 祖先检查恰好挡住这种情况(重开的 tip 不会是旧 head 的祖先),但若改成只按名字匹配就会误删 · done=PR#45 - [ ] FU-5 · B · src=2026-08-05 pilot 端到端测试(doctor+plan) · 2026-08-05 · pilot 的起跑门禁只认 docs_dir 下七个固定文件名,认不出等价(且更完整)的规划源。实测:Brood 的规划在 backlog/(4 个 milestone + 49 个带验收标准的 task + 2 个 ADR),check-docs.sh --strict 报 0/7、run 直接 fail-closed 拒跑;而 plan.md A.3 又明写『已有规划 → 不要重复造』—— 两条同时遵守不可能。本次用 docs/agent/ 做适配层(指向 backlog/ 的视图,不复制内容)绕过去了,但根治要给 check-docs.sh 加可配置规划源(如 .pilot.yml 声明 planning_source: backlog),否则每个用 backlog/issues/Jira 管规划的仓库都会被判未就绪 - [x] FU-6 · C · src=2026-08-05 pilot 端到端测试(doctor) · 2026-08-05 · doctor 第 4 步在单主干仓库里会误导:无 .pilot.yml 时默认 integration_branch=preview,doctor 发现它不存在就『提示先建』—— 但对单主干仓库正确答案是 integration_branch=main + 合并时用 --allow-trunk,不是去建一个 preview 分支。doctor 不知道这两件事是连着的。改法:检测到 preview 不存在但 default branch 存在时,提示单主干配置法并指向 --allow-trunk · done=PR#48 - [ ] FU-7 · B · src=2026-08-05 从 PR#45 拆出 PR#47 时实测 · 2026-08-05 · 『dist/ matches a fresh build』这条 CI 检查会随日期自己变红,与代码无关。实测:在 main(5549b8b)上不改任何代码只跑一次 preflight,dist/api/statistics.json 就变了 —— averageTaskAge 98→99,那是按天算的任务平均年龄。也就是说 main 放几天不动,下一个 PR 无论改什么都要顺带提交一次无关的 dist 变更,否则红灯。这是继 #39(死代码)、#40(随机红灯)、FU-2(守卫清不掉东西)之后同一家族的第四个『守卫自己不可靠』。改法二选一:① 比对时把时间派生字段(averageTaskAge、recentActivity 里的相对时间)剔除再 diff;② 导出时就不把这类字段写进 dist。⚠️ pilot 冻结期内不做,记账待解冻 - [x] FU-7 **撤回(误报)** · 2026-08-05 同日核实 · append-only 不删行,所以更正写在这里:**这条不成立,不要去修。** verify.yml 第 96–122 行**已经**在比对前把 `averageTaskAge` pin 到 `git show HEAD:` 里的 committed 值(正是 PR#40『归一化那一个字段』做的,注释里连 04:15 UTC 测到 98、十五分钟后 99 都记了),所以 CI 不会因它变红。我立 FU-7 时只看到本地 `git status` 有 diff 就下了结论,没读 CI 脚本 —— **本地 dist 会漂 ≠ CI 会红**,这两件事被我混成一件。真实结论:本地跑完 build 看到 statistics.json 变了,**不需要**提交,CI 会自己 pin 掉。教训比这条 bug 本身有用:报「守卫不可靠」之前先读那个守卫的实现,别只看症状 · done=PR#47(撤回) - [ ] FU-8 · B · src=PR#47 review R3(Codex)+R4 建议 · 2026-08-05 · **每个守卫必须校验自己的选择器,而不只是 flag** —— 这条纪律要写进 reference/。来源:PR#47 的白名单把 gh flag 管得很严(不认识就拒),却从没看过 merge-pr 的第一个位置参数,而 `gh pr merge` 接受 `[||]`,URL 选择器完全无视 `--repo`(gh 2.92.0 实测),于是所有闸门读本仓库、合并落到另一个仓库。已在 PR#47 修掉那一处实例(选择器必须是纯数字),但**纪律本身没落地**,下一个守卫照样可能只查 flag。这是本仓库同一家族的第五个:#39 死代码 / #40 随机红灯 / FU-2 清不掉东西 / FU-7(我自己的误报) / 本条 —— 共同形状是『读起来很严、但有一个输入它从来不检查』。落地时和 #45 一起收口,不要再往 PR#47 里加东西(它被拆出来就是因为 #45 装了四件事) +- [ ] FU-9 · B · src=PR#45 review R4 [Low] · 2026-08-05 · safe-cleanup 的 §3(--remote)只用 `git branch -r --merged` 找候选,而那个判据在 squash 仓库里按构造恒返回 0 行 —— 也就是说在【这个功能存在的理由所指的那种仓库形态里,第 3 节是死代码】,而 run.md 又把 --remote 写成合并后的远程清理手段。--squash-merged 没有延伸到远程。第五轮先只让它把话说清楚(为空时打印『没查,不是没有』,并指向手工清理 / GitHub auto-delete),没有实现远程的证据检查 —— 那要对每个远程分支再花一次 gh 调用,且远程路径没有 -d 兜底,风险高于本地,值得单独一个 PR 想清楚。这是同一家族的第六个『守卫在它最该起作用的场景里跑不起来』(#39 死代码 / #40 随机红灯 / FU-2 清不掉 / FU-7 我的误报 / FU-8 只查 flag 不查选择器 / 本条) +- [x] FU-2/FU-4 **收敛(2026-08-06)** · append-only 不删行,所以改法写在这里:**squash 清理最终只『列』不『删』**。PR#45 在这个能力上走了六轮评审,每轮都挖出实测复现的真缺陷(同名 tag 劫持→删掉未合并分支 / 恢复句柄打印另一个分支名(bash 3.2 的 local 语义) / 丢掉 git 自带的 worktree 占用拒绝 / TOCTOU / 拿本地判据在服务端删掉同事未合并的工作)。没有一条是评审吹毛求疵。**结论不是防得更严,而是:自动执行不可逆删除、判据又必须从服务端推断,所需的把握程度配不上它买到的东西 —— 它买到的只是不用敲 `git branch -D <名字>`。那六个缺陷全是「删」的属性,不是「列」的属性。** 所以脚本做难的那半(逐条给出合并证据),不可逆的那半留给人;远程分支交给 GitHub auto-delete-on-merge。FU-9(远程在 squash 仓库里是死代码)一并作废 —— 那一节现在也只列不删 · done=PR#45 +- [ ] FU-10 · C · src=PR#45 第八轮 [Low] · 2026-08-06 · **注释与文档漂移(收口时未清)**:safe-cleanup.sh 里若干注释仍在引用已删掉的东西 —— :150-151 / :247 / :317 提到 `update-ref` 的 expected-old-value(那个函数已删)、:501 附近说 §3「deletes on the SERVER」(已改成完全不处理)。另外 run.md:76 写「不加 --delete-branch,远程分支删除统一交给 safe-cleanup」,但 safe-cleanup 已不处理远程、git-guard 又硬拒 --delete-branch,于是**远程 head 分支的清理在文档流程里没有主人**,只剩 GitHub 的 auto-delete-on-merge,而 skill 既不检查该设置是否打开、也没在任何地方教人去开。改法:清一遍过时注释;在 doctor 里加一条只读检查「本仓库是否开了 auto-delete-on-merge」并在关闭时提示 +- [ ] FU-11 · C · src=PR#45 第八轮 [Low] · 2026-08-06 · §1b 现在是个纯报告,但计价没变:每个候选分支一次 gh API 调用(本仓库约 25 次 / 20.6 秒),而 status.md 让它在**每次** `pilot status` 都跑。改法二选一:① 按 tip sha 做本地缓存(tip 没动就不重查);② 在 status.md 里把 `--squash-merged` 改成显式 opt-in,默认不带 +- [ ] FU-12 · C · src=PR#45 第八轮 [Low] · 2026-08-06 · §1b 会把「某个活着的 worktree 正在 rebase/bisect 的分支」当成游离分支列出来并打印 `git branch -D `,而同一次运行里 §2 对那个 worktree 打的是 KEEP —— **同一份输出自相矛盾**。已验证无害(git 2.50 会拒绝 `cannot delete branch 'x' used by worktree`,分支完好,rebase --continue 正常),所以只是输出问题。顺带记一句:这也说明删掉 `ref_in_use_by_worktree` 是对的 —— `git branch -D` 自带那个拒绝,而 `update-ref -d` 没有 + diff --git a/plugins/pilot/skills/pilot/README.md b/plugins/pilot/skills/pilot/README.md index 737255e..a7b3404 100644 --- a/plugins/pilot/skills/pilot/README.md +++ b/plugins/pilot/skills/pilot/README.md @@ -16,7 +16,8 @@ pilot doctor # 自检本地就绪度(config/docs/分支/gh/hook)——** ## 为什么用它 把「有经验程序员的默认」固化成流程,且**危险动作确定性可控**: -- **安全清理**:只删「已合并进集成分支 + 干净」的分支/worktree;只 `git branch -d`(永不 `-D`);默认 dry-run,`--apply` 才动手;护住主干/集成/当前/protected/脏 worktree。逻辑全在 `scripts/safe-cleanup.sh`,不靠模型临场判断。 +- **安全清理**:**唯一会执行的删除是 `git branch -d`**(git 自己拒绝未合并的、以及被 worktree 占用的)。**永不 `-D`、永不删远程、永不 `git worktree remove`**——最后这条是**文件系统删除**,会连 `.gitignore` 掉的文件(真实案例:`.env`)一起抹掉,而 `git status --porcelain` 根本不显示它们,所以 worktree 只**列出来**并给出命令。远程分支**完全不处理**,用 GitHub 的 auto-delete-on-merge。逻辑全在 `scripts/safe-cleanup.sh`,不靠模型临场判断。 + **squash-merge 仓库**里 `git branch --merged` 通常返回 0(squash 重写补丁,原 commit 不是集成分支的祖先),于是上面那条 `-d` 在这类仓库里清不动任何东西。加 `--squash-merged`:它按 **commit** 向 GitHub 查「哪个已合并 PR 引入了这个提交」,把有证据的分支**列出来**(附 PR 号 + tip sha + 可粘贴的 `git branch -D` 命令)——**脚本自己不执行那一下**。难的那半(在 git 看不出来的仓库里逐条给出证据)自动做,不可逆的那半留给人。查不到证据、没装 gh、没登录 —— 一律不列,并明说「无法核实」。 - **PR 纪律**:绝不 `git add -A`;绝不直推/直合主干;一个 task = 一个分支 = 一个 worktree = 一个 PR;PR 前必自测 + 对抗式 review。 - **外部 review 回路(已生产验证)**:pilot **不自评 PR**——开好 PR 后只盯自己 PR 的状态(`scripts/pr-monitor.sh --pr --wait-for-verdict`,内置 3–5 分钟轮询与 **30 分钟硬上限**;只有评审 commit == 当前 head 才算裁决)。裁决由**外部评审服务**给出,契约见 `reference/review-contract.md`:排队 5–10 分钟、评审 5–10 分钟,通常 20 分钟内出 `APPROVED`/`CHANGES_REQUESTED`(超大 PR 例外)。推新 commit 自动触发再评审。**那个服务是什么、装在哪、覆盖哪些仓库,pilot 一概不知也不启动**——只依赖这份契约。 - **pilot 是入口,配套能力由它安排**:飞书 / Notion 等文档源是 pilot 的**配套 skill**——该不该装、装哪个、装到全局还是项目级、装完怎么验证,由 pilot 负责讲清楚和安排。**但「入口」是编排责任,不是运行时依赖**:pilot 自己不 import、不启动任何文档源,只探测能力是否存在,没有就说明缺什么、给出装法,然后降级继续干活。契约与实测过的安装命令见 `reference/doc-sources.md`。 diff --git a/plugins/pilot/skills/pilot/SKILL.md b/plugins/pilot/skills/pilot/SKILL.md index 3f4ec46..54701a6 100644 --- a/plugins/pilot/skills/pilot/SKILL.md +++ b/plugins/pilot/skills/pilot/SKILL.md @@ -46,7 +46,7 @@ pilot doctor # 自检本地就绪度(config/docs/分支/gh/hook)——** 1. **绝不 `git add -A` / `git add .`**。只 `git-guard.sh add <显式路径>`(裸 `git add -A` 会被 git-guard 拒绝)。理由:`-A` 会把未确认是否该跟踪的文件(密钥、`.env`、构建产物、临时文件)一起提交,是最危险的日常动作。提交前先 `git status` 看清,逐一列出要提交的路径。 2. **绝不直接 push 到主干(main/master),绝不直接合并自己的 PR 到主干**。push 走 `git-guard.sh push`、**开 PR 走 `git-guard.sh pr-create`**(先 `preflight.sh run` 让本仓库的检查真跑过)、合并走 `git-guard.sh merge-pr --integration `(推主干 / 合并 base≠集成分支都会被硬拒绝)。所有代码变更走:feature 分支 → PR → review → 合并到**集成分支**(默认 `preview`,见 `.pilot.yml`)。主干只由集成分支经受控流程进入。**单主干仓库**(没有集成分支,PR 直接开向 `main`)加 `--allow-trunk`——它**不是绕过**:仍要求该分支的 GitHub 保护规则要求审批、且这个 PR 已经 `APPROVED`,读不到保护规则就拒绝(fail-closed)。 3. **一个 task = 一个分支 = 一个(可选)worktree = 一个 PR**。不在一个分支里顺手做别的 task。 -4. **删除分支只用 `git branch -d`,永不 `-D`**;删除只针对「已合并 + 干净」的分支/worktree;一切经 `scripts/safe-cleanup.sh`,默认 dry-run。 +4. **绝不 `git branch -D`,绝不 `git push --delete`,绝不 `git worktree remove`**(最后这条是文件系统删除,会连 gitignore 掉的文件一起抹掉,而 `git status --porcelain` 看不见它们)。脚本唯一会执行的删除是 `git branch -d`——git 自己会拒绝未合并的、以及被 worktree 占用的,这是安全网。worktree 和远程分支只**列出来**给人。一切经 `scripts/safe-cleanup.sh`,默认 dry-run。**squash-merge 仓库**里 `git branch --merged` 通常为空,`--squash-merged` 会按 commit 向 GitHub 核实哪个已合并 PR 引入了它,**但只把结果列出来给人,脚本不执行 `-D`**——不可逆的那一下由人来敲(详见 `reference/git-safety.md`)。 5. **PR 之前必须自审 + 对抗 review**(怎么审见 `reference/pr-quality.md`,**审几轮由 `scripts/grade-change.sh` 机械定级,见 `reference/pre-pr-review.md`**——A/B 级 3 轮,不是作者自己说了算)。没过 review 的代码不进 PR,没 approve 的 PR 不合并。 6. **状态即文档**。每推进一步都更新 `docs/agent/tasks.md` 与 `docs/agent/progress.md`;宁可慢,不可让文档与仓库真实状态脱节。 7. **无人值守时不猜产品决策**。遇到影响产品方向/验收/架构的未知,把相关 task 标 `BLOCKED` 并记录待决问题,继续做不受影响的 task;绝不擅自替用户拍板。 diff --git a/plugins/pilot/skills/pilot/phases/run.md b/plugins/pilot/skills/pilot/phases/run.md index 7d8488a..d52a9d0 100644 --- a/plugins/pilot/skills/pilot/phases/run.md +++ b/plugins/pilot/skills/pilot/phases/run.md @@ -76,8 +76,8 @@ bash /scripts/check-docs.sh --docs-dir --strict `bash /scripts/git-guard.sh merge-pr --integration --squash` (git-guard 会先校验 PR base == `integration_branch`,base 是主干或其它分支会被拒绝;**不加 `--delete-branch`**——远程分支删除统一交给 §合并后的 safe-cleanup,受 `allow_remote_cleanup` 与 dirty-worktree 检查约束)。 合并后:把对应 Task 在 `tasks.md` 标 `DONE`、更新 `progress.md`,运行 - `bash /scripts/safe-cleanup.sh --integration [--protect ""] [--remote-name ] --apply` - 清掉本地已合并分支/worktree(**必须显式带上与 `.pilot.yml` 一致的 `--integration`/`--protect`/`--remote-name`,不要依赖脚本猜默认值**;要连带删远程,且 `allow_remote_cleanup: true` 时,再加 `--remote`)。**做完回到主循环顶端,继续下一项。** + `bash /scripts/safe-cleanup.sh --squash-merged --integration [--protect ""] [--remote-name ] --apply` + 清掉本地已合并分支/worktree(**必须显式带上与 `.pilot.yml` 一致的 `--integration`/`--protect`,不要依赖脚本猜默认值**)。脚本只会 `git branch -d`;squash 合并的分支和 worktree 它**只列出来**,把清单原样报给用户,不要代劳删。**远程分支脚本不处理**——让用户在仓库设置里开 GitHub 的 auto-delete-on-merge。**做完回到主循环顶端,继续下一项。** - **`decision=CHANGES_REQUESTED`** → 读全部 review 意见(`gh pr view --comments`),**先做中立 triage(见 `reference/review-triage.md`)**:装上本仓库业务上下文(CLAUDE.md / docs/agent / 领域文档),把每条意见分成 A 该修 / B 不重要 / C 缺业务上下文判错了 / D 过激 nitpick。外部评审是独立的、没有业务背景,你有——**既不盲改也不盲拒**。 - **A(+trivial 的 D)** → 在该 PR 分支修复 → 自测 → 自审 → `git-guard.sh add <显式路径>` + commit + `git-guard.sh push `(推新 commit 自动触发再评审)。 - **B(真问题但不阻塞)/ 非 trivial 的 D 里决定要做的** → **记进跟进账本,绝不丢**: diff --git a/plugins/pilot/skills/pilot/phases/status.md b/plugins/pilot/skills/pilot/phases/status.md index afde9a7..12fe1a5 100644 --- a/plugins/pilot/skills/pilot/phases/status.md +++ b/plugins/pilot/skills/pilot/phases/status.md @@ -22,10 +22,22 @@ bash /scripts/safe-cleanup.sh --integration [--protect ""] ``` (**dry-run**,只打印 would-delete / would-remove / KEEP)。把结果原样呈现。 + 这条命令**不带** `--squash-merged`,所以「Squash-merged local branches」一节会打印 + `(not checked — one gh API call per branch; pass --squash-merged to check)` —— + 那是**没查**,不是**没有**,别当成「干净」。 + - **本仓库用 squash 合并时必看**:`git branch --merged` 在 squash 仓库里**通常返回 0**(零提交的废弃分支仍会被它列出), + 上面那条命令的「Local merged branches」会永远是 `(none)`。脚本会在 + 「Squash-merged local branches」一节把候选列出来(每条附已合并的 PR 号), + 此时改用: + ``` + bash /scripts/safe-cleanup.sh --integration --squash-merged + ``` + 若该节打印 `(cannot verify …)`,说明 `gh` 没装/没登录 —— 那是**查不了**,不是**没有**, + 照实说,不要报成「没有可清理的」。 5. **征询清理**:把 dry-run 计划给用户,**问是否执行**。得到确认后才加 `--apply`: ``` - bash /scripts/safe-cleanup.sh --integration --apply + bash /scripts/safe-cleanup.sh --integration [--squash-merged] --apply ``` - 要连带删远程已合并分支:仅当 `.pilot.yml` 的 `allow_remote_cleanup: true`,且用户明确同意,才加 `--remote`。 - 无人值守模式(由 /loop 调用且用户已预先授权清理):可直接 `--apply`,但**永远不加 `--remote`** 除非配置显式开启。 @@ -34,6 +46,6 @@ ## 纪律 -- 清理的所有安全判断在 `safe-cleanup.sh` 里确定性执行(只删已合并+干净、只 `-d`、护住 main/集成/当前/protected/脏 worktree)。**不要在对话里手工 `git branch -d`** —— 走脚本,避免漏判。 -- 绝不 `-D`,绝不删未合并分支,绝不碰脏 worktree 或其远程分支。 +- 清理的所有安全判断在 `safe-cleanup.sh` 里确定性执行(只删已合并+干净、护住 main/集成/当前/protected/脏 worktree)。**不要在对话里手工 `git branch -d`/`-D`** —— 走脚本,避免漏判。 +- **脚本唯一会执行的删除是 `git branch -d`**(git 自己拒绝未合并的、以及被 worktree 占用的)。`-D`、`git push --delete`、`git worktree remove` **都不执行**——`--squash-merged` 拿到服务端证据后只把分支/worktree **列出来**(附 PR 号、tip sha、可粘贴的命令),删不删由人决定。把那份清单**原样**呈现给用户,不要代劳执行。远程分支脚本完全不处理,让用户去仓库设置里开 GitHub 的 auto-delete-on-merge。 - 汇报前若对某个数字存疑,重跑 `repo-scan.sh` 核对,不要凭记忆报数。 diff --git a/plugins/pilot/skills/pilot/reference/git-safety.md b/plugins/pilot/skills/pilot/reference/git-safety.md index 438fff8..5817a26 100644 --- a/plugins/pilot/skills/pilot/reference/git-safety.md +++ b/plugins/pilot/skills/pilot/reference/git-safety.md @@ -25,10 +25,36 @@ - 一个 task = 一个分支 = 一个(可选)worktree = 一个 PR。分支命名 `/-`,如 `feat/T1.3.2-admin-init`。 ## 删除(清理) -- **删本地分支只用 `git branch -d`,永不 `-D`**。`-d` 会拒绝删除未合并分支,是安全网;`-D` 强删会丢未合并工作。 +- **删本地分支只用 `git branch -d`**。`-d` 会拒绝删除未合并分支,是安全网;`-D` 强删会丢未合并工作。 + **脚本永远不执行 `-D`,不 `git push --delete`,也不 `git worktree remove`。** + 最后那条尤其容易被漏掉:它是**文件系统删除**,而判断它「干净」用的 `git status --porcelain` + **不列 gitignore 的文件**,`git worktree remove` 不带 `--force` 时也容忍「只有 ignored 文件」的 + 工作树 —— 两层各自放行,结果是一个装着 `.env` 的目录被整个删掉、不可恢复。所以 worktree 只列出来 + 并附上移除命令和「先查 ignored 文件」的提示。 +- **squash-merge 仓库的处理方式:列出来,不删。** 那里 `git branch --merged` **通常返回 0** —— squash 重写补丁, + 原 commit 不是集成分支的祖先。实测本仓库 28 个分支、`git branch --merged main` 返回 0, + 于是这个脚本在自己家里**什么都清理不了**。 + 所以 `safe-cleanup.sh --squash-merged` 引入第二种**服务端**证据:GitHub 的 + `/commits/{sha}/pulls` 告诉你「哪个 PR 把这个 commit 引入了仓库」,只有当它给出 + `merged_at != null` 的 PR 时,这个分支才会被**列进清单**(附 PR 号、tip sha、可粘贴的命令)。 + **按 commit 判、不按分支名判**,因为两个方向的错都真实发生过: + ① 漏报 —— `work-pr18` 这类分支名从没当过 PR head,但 tip 就是别的 PR 的已合并 head; + ② 错报 —— 分支名可复用,同名分支删掉重开后内容全不同,旧的 MERGED PR 仍然匹配名字。 + 没证据 / 没装 gh / 没登录 → **一律不列**,且明确报「无法核实」而不是「没有可清理的」。 + + **为什么只列不删。** 早先的版本是真删的(`-D` + `git push --delete`),六轮评审在这一个能力上 + 找出六个**实测复现**的缺陷:同名 tag 劫持证据、导致删掉一条**未合并**分支;恢复句柄打印**另一个** + 分支的名字(bash 3.2 会在外层作用域展开 `local a=.. b=${a..}` 的右值);丢掉了 git 自带的 + 「这个 ref 被 worktree 占用」拒绝;证据与删除之间的 TOCTOU 窗口;以及**拿本地分支当判据、 + 在服务端删掉同事没合并的工作**。没有一条是理论风险。 + 结论不是「防得更严」,而是:**自动执行不可逆删除、而判据又必须从服务端推断**,所需的把握程度 + 配不上它买到的东西——它买到的只是不用敲 `git branch -D <名字>`。那六个缺陷全都是「删」的属性, + 不是「列」的属性。所以脚本做难的那半(在 git 自己看不出来的仓库里逐条给出合并证据), + 不可逆的那半留给人。远程分支交给 GitHub 的 auto-delete-on-merge——它在**合并真正发生的那一侧** + 判定,不会被一个还没 push 的本地分支骗到。 - 只删「已合并进集成分支 + 干净」的分支/worktree。 - 一切经 `scripts/safe-cleanup.sh`,默认 dry-run,`--apply` 才执行。**不要在对话里手工逐个删**,避免漏判保护分支。 -- 删远程分支(`--remote`)需 `.pilot.yml` 里 `allow_remote_cleanup: true` + 用户明确同意;无人值守默认不删远程。 +- **远程分支:脚本完全不处理**(既不删也不列)。用 GitHub 的 auto-delete-on-merge——它在合并真正发生的那一侧判定,而从 clone 里判断要依赖可能过期的 remote-tracking ref,且 bare 仓库不留 reflog,是所有删除里唯一完全没有后悔药的。`.pilot.yml` 的 `allow_remote_cleanup` 因此不再有作用。 - 永不碰:当前分支、集成分支、主干、protected 前缀(release/hotfix…)、脏 worktree 及其远程分支。 ## 危险操作一律先确认 diff --git a/plugins/pilot/skills/pilot/reference/review-contract.md b/plugins/pilot/skills/pilot/reference/review-contract.md index 1de2d0f..e30565c 100644 --- a/plugins/pilot/skills/pilot/reference/review-contract.md +++ b/plugins/pilot/skills/pilot/reference/review-contract.md @@ -51,7 +51,7 @@ pilot **不裁决自己的 PR**。它依赖一个**外部评审服务**,并且 └─ CHANGES_REQUESTED → 中立 triage → 修 → 推 → 回到「等回执」 ``` -- **`APPROVED`** → `git-guard.sh merge-pr --integration --squash`,然后 `safe-cleanup.sh`。 +- **`APPROVED`** → `git-guard.sh merge-pr --integration --squash`,然后 `safe-cleanup.sh --squash-merged`。 - **`CHANGES_REQUESTED`** → 先按 [`review-triage.md`](review-triage.md) 做中立裁决(该改的改 / 判错的 回评论讲清业务理由 / 不阻塞的记进 [`followup-ledger.md`](followup-ledger.md)),修完推上去 **自动触发下一轮评审**,回到等回执。 diff --git a/plugins/pilot/skills/pilot/scripts/safe-cleanup.sh b/plugins/pilot/skills/pilot/scripts/safe-cleanup.sh index b5f91a8..8c8d384 100755 --- a/plugins/pilot/skills/pilot/scripts/safe-cleanup.sh +++ b/plugins/pilot/skills/pilot/scripts/safe-cleanup.sh @@ -1,36 +1,107 @@ #!/usr/bin/env bash -# safe-cleanup.sh — delete ONLY merged + clean branches/worktrees. Safe by design. +# safe-cleanup.sh — report what is safely cleanable; delete only what git itself will vouch for. # # Guarantees (do not weaken these): -# * Dry-run by default. Nothing is deleted unless --apply is passed. -# * Local branches: only `git branch -d` (git refuses to delete unmerged). NEVER `-D`. +# * The ONLY thing this script deletes is a local branch via `git branch -d`, under --apply. +# `-d` is git's own safety net: it refuses anything git cannot see as merged, so it cannot +# destroy unmerged work, and it refuses a branch a worktree is using. +# * NEVER `git branch -D`. NEVER `git push --delete`. NEVER `git worktree remove` — that last one +# is a FILESYSTEM delete and took gitignored files (a real `.env`) with it; see below. # * Never touch: the current branch, the integration branch, or any protected branch. -# * Worktrees: removed only if their working tree is CLEAN and their branch is merged. -# * Remote branches: only deleted with --remote (opt-in) AND only if merged + not protected. -# * A dirty worktree — and its remote branch — is always left completely alone. +# * Worktrees: REPORTED only, with the command to remove them. Never removed. +# * Remote branches: NOT HANDLED AT ALL — not deleted, not listed. Use GitHub's +# auto-delete-on-merge, which judges server-side where the merge happened. # -# Known limitation (intentional, conservative): squash-merged branches are not -# detected by `git branch --merged`, so they are SKIPPED, not deleted. Missing a -# cleanup is safe; a wrong deletion is not. Delete those by hand if you want them gone. +# Squash-merged branches (--squash-merged, opt-in): +# In a squash-merge repo `git branch --merged` returns NOTHING, ever — the squash rewrites the +# patch so the original commits are not ancestors of the integration branch. Measured here: +# 28 local branches, `git branch --merged main` → 0. So `git branch -d` alone can never clean +# this repo, and the section that uses it is honest but useless here. +# +# `--squash-merged` adds a SECOND source of merge evidence: GitHub's `/commits/{sha}/pulls` +# says which PR introduced a commit to the repository. A branch is reported only when that +# endpoint names a PR with `merged_at != null` for the branch's tip commit. +# +# Why keyed on the COMMIT and not the branch name — both directions of the name-based mistake +# are real and were hit here: +# * miss — `work-pr18` / `worktree-agent-*` were never any PR's head branch, yet their tips +# were the merged heads of #18 / #23. A name lookup calls them unmerged. +# * WRONG — branch names are reusable. Delete a branch, recreate it with unrelated work, and +# the old MERGED PR still matches the name. A name lookup would name it cleanable. +# The commit-keyed check has neither failure mode. Verified against all four shapes: +# squash-merged head ✓, mid-branch commit ✓, closed-but-unmerged → no evidence ✓, +# never-had-a-PR → no evidence ✓. +# +# Why listing, not deleting: +# Cleaning these needs `-D` (`-d` refuses — git cannot see the merge). An earlier version did +# exactly that, and six review rounds on that one capability found six real, reproduced defects: +# a same-named tag hijacking the evidence so an UNMERGED branch was deleted; a recovery handle +# printing a DIFFERENT branch's name (bash 3.2 expands the right-hand side of a multi-assignment +# `local` in the OUTER scope); the loss of git's own "that ref is checked out by a worktree" +# refusal; a TOCTOU window between evidence and delete; and a server-side delete judged by the +# operator's LOCAL branch, which removed a colleague's unmerged work from the remote. +# +# None of those was theoretical and none was the reviewer being picky — each was reproduced end +# to end. The conclusion was not "defend harder". Automating an IRREVERSIBLE delete, on evidence +# inferred from a server, demands a level of assurance that what it buys — not typing +# `git branch -D ` — does not justify. Every one of those six defects was a property of +# DELETING, not of listing. +# +# So: this script does the part that is genuinely hard (working out which branches are merged, +# with per-branch evidence, in a repo where git itself cannot tell you) and leaves the part that +# is trivial-but-unrecoverable to you. For remote branches, GitHub's auto-delete-on-merge already +# does it server-side, where the merge happened — strictly better than inferring it from a clone. +# +# That sweep had to be done TWICE. The first pass removed the ref writes (`-D`, `push --delete`) +# and declared victory — while `git worktree remove` was still there, newly wired to the same +# inferred evidence, deleting whole directories including gitignored files that `git status +# --porcelain` never shows. "No irreversible deletes" was written in this header while the +# filesystem delete four screens down was live. When auditing for destructive actions, ref writes +# are the ones you think of; the filesystem ones are the ones that actually lose data. # # Usage: -# safe-cleanup.sh [--integration ] [--protect "a,b,c"] [--apply] [--remote] [--remote-name origin] +# safe-cleanup.sh [--integration ] [--protect "a,b,c"] [--apply] [--squash-merged] +# [--remote] [--remote-name origin] +# ---8<--- end of header ---8<--- set -euo pipefail integration="" protect_csv="main,master,develop,preview,integration,release,hotfix" apply=0 +squash_merged=0 do_remote=0 remote_name="origin" +# Unknown arguments are REFUSED, not skipped. `*) shift ;;` silently dropped a typo like +# `--integraton main`, and `integration` then fell back to a guess from origin/HEAD — so +# `--apply` would delete against a baseline the caller never asked for. `--integration` is the +# one argument whose typo is unrecoverable, and run.md requires callers to pass it explicitly. +# (Same reasoning that turned git-guard's flag denylist into a refuse-unknown allowlist in this +# very commit; it applies here for identical reasons.) +# Counting arity is not enough: `--protect --apply` passed the arity test and swallowed --apply, +# so the script printed mode=DRY-RUN and exited 0 while the caller believed it had run. Same +# silent-swallow class B2 exists to close. +need_val() { + [ "$2" -ge 2 ] || { echo "safe-cleanup: '$1' requires a value" >&2; exit 2; } + case "$3" in + -*) echo "safe-cleanup: '$1' requires a value, got flag '$3'" >&2; exit 2 ;; + "") echo "safe-cleanup: '$1' requires a non-empty value" >&2; exit 2 ;; + esac +} while [ $# -gt 0 ]; do case "$1" in - --integration) integration="${2:-}"; shift 2 ;; - --protect) protect_csv="${protect_csv},${2:-}"; shift 2 ;; - --apply) apply=1; shift ;; - --remote) do_remote=1; shift ;; - --remote-name) remote_name="${2:-origin}"; shift 2 ;; - *) shift ;; + --integration) need_val "$1" $# "${2:-}"; integration="$2"; shift 2 ;; + --protect) need_val "$1" $# "${2:-}"; protect_csv="${protect_csv},$2"; shift 2 ;; + --remote-name) need_val "$1" $# "${2:-}"; remote_name="$2"; shift 2 ;; + --apply) apply=1; shift ;; + --squash-merged) squash_merged=1; shift ;; + --remote) do_remote=1; shift ;; + # `grep '^#'` keeps comment lines — but the sentinel IS a comment line, so it was printed + # verbatim at the end of every --help. Drop it explicitly. + -h|--help) sed -n '2,/^# ---8<--- end of header ---8<---$/p' "$0" | grep '^#' | grep -v '^# ---8<---'; exit 0 ;; + *) echo "safe-cleanup: unknown argument '$1'" >&2 + echo " usage: safe-cleanup.sh [--integration ] [--protect \"a,b\"] [--apply] [--squash-merged] [--remote] [--remote-name ]" >&2 + exit 2 ;; esac done @@ -52,22 +123,144 @@ if ! git show-ref --verify --quiet "refs/heads/$integration"; then echo "ERROR: integration branch '$integration' not found locally; refusing to guess. Pass --integration." >&2 exit 1 fi +# Use the FULL ref everywhere the integration branch is resolved. As a bare name it is subject to +# the same tag-beats-branch precedence as any other: with a tag also called `main`, `git branch -r +# --merged main` answered about the TAG's commit and listed `origin/unmerged-precious` as merged. +# Back when §3 still deleted, `--apply` destroyed that branch server-side; it only lists now, but a +# wrong list is still a wrong instruction to paste. The ambiguity warning git prints was being +# swallowed by `2>/dev/null`, so nothing surfaced. +integration_ref="refs/heads/$integration" is_protected() { local name="$1" [ "$name" = "$current_branch" ] && return 0 [ "$name" = "$integration" ] && return 0 - local IFS=',' - for p in $protect_csv; do - p="$(echo "$p" | xargs)" # trim whitespace - p="${p%/}" # tolerate trailing slash (e.g. "hotfix/") + # Split with `read -ra`, not word-splitting, and trim with parameter expansion, not `echo | xargs`. + # xargs parses SHELL QUOTING, so it mangled exactly the names that most need protecting: + # `a\b` came back as `ab`, and `keep'me` (a legal refname) killed xargs on an unterminated quote, + # yielding an empty string that the `continue` below silently dropped — the branch then fell + # through to the `-D` path. Measured: `--protect "keep'me"` printed `would delete keep'me`. + # `set -f` so a pattern like `rel*` is compared literally instead of being glob-expanded against + # the working directory. + local -a pats=(); local p + set -f + IFS=',' read -ra pats <<< "$protect_csv" + set +f + for p in "${pats[@]}"; do + p="${p#"${p%%[![:space:]]*}"}" # trim leading whitespace + p="${p%"${p##*[![:space:]]}"}" # trim trailing whitespace + p="${p%/}" # tolerate trailing slash (e.g. "hotfix/") [ -z "$p" ] && continue [ "$name" = "$p" ] && return 0 - case "$name" in "$p"/*) return 0 ;; esac # e.g. release/* protects release/1.2 + # Boundary set copied VERBATIM from git-guard.sh's is_protected. The two scripts ship in the + # same skill and must not disagree about what "protected" means: with the old `"$p"/*` a + # `release-1.2` that git-guard REFUSES to push to was still offered here as a deletion + # candidate. Harmless while this script only used `-d`; this PR added `-D`, which turns the + # divergence into data loss. `[-_/.0-9]` (not `*`) so `main` cannot swallow `mainline`. + case "$name" in "$p"[-_/.0-9]*) return 0 ;; esac done return 1 } +# ---- squash-merge evidence (read-only; no fetch, no ref writes, so dry-run stays read-only) ---- +# Resolved once, lazily: most repos never need it, and `gh` may not be installed at all. +gh_state="unknown" # unknown | ready | unavailable +gh_repo="" +gh_init() { + [ "$gh_state" = "unknown" ] || return 0 + gh_state="unavailable" + command -v gh >/dev/null 2>&1 || return 0 + gh auth status >/dev/null 2>&1 || return 0 + gh_repo="$(gh repo view --json nameWithOwner --jq .nameWithOwner 2>/dev/null || true)" + [ -n "$gh_repo" ] && gh_state="ready" + return 0 +} + +# Echo one of three things, and the difference matters: +# a MERGED PR whose BASE is $integration introduced this branch's tip commit +# ERR the lookup itself failed (rate limit, 5xx, missing scope) — verdict UNKNOWN +# (empty) the lookup succeeded and found no such PR — no evidence +# +# `.base.ref == $integration` is load-bearing, not decoration. Without it the test is merely +# "some merged PR touched this tip", which a STACKED PR satisfies: child merged into a parent +# feature branch, parent's own PR later closed unmerged — the child's tip then carries merge +# evidence forever while its work never reached the integration branch. Measured in a sandbox: +# a commit `git merge-base --is-ancestor … main` calls unreachable was still offered for +# deletion. Brood happens to have integration == default == main, which masks this entirely; +# pilot's documented default shape is `preview != main`, where it bites. +# +# NB: an `--is-ancestor $sha $integration` test is deliberately NOT added on top. In a +# squash-merge repo the tip is BY CONSTRUCTION never an ancestor of the integration branch — +# that is the whole reason this function exists. Requiring it would return the feature to the +# dead-code state it was written to fix. Base convergence is the correct narrowing; ancestry +# is not available here. +# +# Takes a resolved SHA, not a ref name. The caller resolves it once and keeps it, so the evidence +# and the sha that authorises the deletion are the SAME object by construction — there is no second +# resolution that could land on a different commit (or, before this round, on a same-named TAG). +merged_pr_for() { # merged_pr_for + local sha="$1" out rc + gh_init + [ "$gh_state" = "ready" ] || { printf 'ERR'; return 0; } + [ -n "$sha" ] || return 0 + # $integration goes in as DATA via $ENV, never interpolated into the jq program. Building the + # program by string substitution let a git-legal branch name rewrite the predicate: with + # `x"or(true)or"` the comparison becomes `.base.ref == "x"or(true)or""` — constantly true — and + # a merged PR whose base was some other branch then counted as evidence, ending in an + # irreversible `git branch -D`. That is the exact failure class the base check was added to + # close, re-entering through the check itself. + # `rc=0; … || rc=$?` instead of a bare `$?`: under `set -e` a failing command substitution only + # survives because bash suspends errexit inside `$( )` and `inherit_errexit` is off by default. + # Verified: convert either call site to a non-cmdsub form and the script exits 1 silently right + # after printing the section header, truncating the report with no error. This form keeps the + # real status AND survives `shopt -s inherit_errexit` or a future non-cmdsub caller. + rc=0 + out="$(PILOT_INTEG="$integration" gh api "repos/$gh_repo/commits/$sha/pulls" \ + --jq '[.[] | select(.merged_at != null and .base.ref == $ENV.PILOT_INTEG) | .number] | first // empty' 2>/dev/null)" || rc=$? + # A failed CALL is not the same as an empty ANSWER. Rate limiting is the most likely failure + # here precisely because this feature spends one API call per branch. + [ "$rc" -ne 0 ] && { printf 'ERR'; return 0; } + case "$out" in + '') return 0 ;; # 200 with no matching PR → genuinely no evidence + # ENUMERATED, not a range — the same convention PR#50 established for git-guard.sh. `[!0-9]` is + # a collation range whose membership depends on LC_COLLATE (under fa_IR/ar_SA the Eastern-Arabic + # digits fall inside it). Not exploitable here either — the value comes from jq's ASCII + # `.number` — but #50 made this a FILE-level convention precisely so no sibling script keeps a + # locale-dependent notion of "is this a number"; that skill ships both of these together. + *[!0123456789]*) printf 'ERR' ;; # 200 but unparseable → treat as unknown, never as "no" + *) printf '%s' "$out" ;; + esac +} + +# NO `-D` ANYWHERE. This script lists squash-merged branches; it never deletes them. +# +# It used to. Six review rounds on that one capability found six real, reproduced defects — a tag +# hijack that deleted an UNMERGED branch, a recovery handle naming a DIFFERENT branch (bash 3.2 +# expands the right-hand side of a multi-assignment `local` in the OUTER scope), a lost +# "branch is checked out by a worktree" refusal, a TOCTOU window, and a server-side delete judged +# by the operator's LOCAL branch. None was theoretical; each was reproduced end to end. +# +# The lesson was not "defend harder". It was that automating an IRREVERSIBLE delete, on evidence +# that has to be inferred from a server, needs a level of assurance this feature's value does not +# justify: what it buys is not typing `git branch -D `. So the delete is gone and the +# evidence-gathering stays — you get told exactly which branches are safe to remove and why, and +# you run one command. Every one of those six defects was a property of deleting, not of listing. +# +# Remote branches: GitHub's auto-delete-on-merge already does that server-side, where the merge +# actually happened, so it cannot be fooled by a local branch that has not been pushed. + +# Membership tests without a pipeline. `producer | grep -Fqx` looks harmless but `grep -q` exits at +# the first match, the producer takes SIGPIPE, and with `set -o pipefail` the whole pipeline returns +# 141 — a MATCH reported as a failure. Reproduced with a 5000-line producer (`MATCH-LOST rc=141`); +# at :349 that silently demoted a git-native merged worktree branch from the `-d` path to the `-D` +# path. Pure bash matching has no producer to kill. +list_has() { # list_has + case $'\n'"$1"$'\n' in + *$'\n'"$2"$'\n'*) return 0 ;; + esac + return 1 +} + mode="DRY-RUN (pass --apply to execute)" [ "$apply" = "1" ] && mode="APPLY" echo "== pilot safe-cleanup ==" @@ -81,18 +274,30 @@ while IFS= read -r line; do branch\ refs/heads/*) wt_branches="${wt_branches}${line#branch refs/heads/}"$'\n' ;; esac done < <(git worktree list --porcelain 2>/dev/null) -is_in_worktree() { printf '%s' "$wt_branches" | grep -Fqx "$1"; } +is_in_worktree() { list_has "$wt_branches" "$1"; } + +# Computed ONCE, from the full integration ref, and reused by every "is this already merged?" test +# below. Previously each test re-ran `git branch --merged | sed | grep -q` — three separate chances +# to hit the SIGPIPE/pipefail bug and the bare-name ambiguity, per branch. +# +# FULL refs, not `%(refname:short)`. Short-form output is DISAMBIGUATED: with a tag of the same +# name, `%(refname:short)` emits `heads/dup` rather than `dup`. That string then flowed into +# `git branch -d -- heads/dup` (→ "branch not found" → reported as `SKIP (not safely merged)`, i.e. +# a `-d`-safe branch declared unmerged) and into the §1b membership test (→ the branch was NOT +# recognised as already handled, so it came back round on the `-D` evidence path). Full refs are +# unambiguous by construction, and every consumer below strips the prefix for display/deletion. +git_merged_refs="$(git branch --format='%(refname)' --merged "$integration_ref" 2>/dev/null || true)" # ---- 1. Merged local branches ------------------------------------------------ echo "## Local merged branches" merged_list=() -while IFS= read -r b; do - b="$(echo "$b" | sed 's/^[* +] *//')" - [ -z "$b" ] && continue +while IFS= read -r mref; do + [ -z "$mref" ] && continue + b="${mref#refs/heads/}" if is_protected "$b"; then continue; fi if is_in_worktree "$b"; then continue; fi # leave worktree branches to section 2 merged_list+=("$b") -done < <(git branch --merged "$integration" 2>/dev/null) +done <<< "$git_merged_refs" if [ "${#merged_list[@]}" -eq 0 ]; then echo " (none)" @@ -108,6 +313,99 @@ else fi echo +# ---- 1b. Squash-merged local branches (evidence-based; opt-in) --------------- +# Separate section on purpose: these need `-D`, so they must never be confused with the +# `-d`-safe list above. A branch appears here ONLY with a merged-PR number attached. +echo "## Squash-merged local branches" +# Probe HERE, in the main shell. `merged_pr_for` is only ever called as `$( ... )`, which runs in +# a SUBSHELL — anything it assigns to gh_state is discarded on return. Relying on that left the +# "cannot verify" branch permanently unreachable, so a missing/unauthenticated gh printed the +# same "(none)" as a genuinely clean repo. "Could not check" must never look like "nothing found". +# COST GATE. ONE gh call per candidate, in both dry-run and apply. (Round 4 briefly made apply 2N +# by re-querying to close the TOCTOU window; `update-ref`'s expected-old-value does that atomically +# instead, so the number below is accurate again — rate limiting is this feature's most likely +# failure mode and doubling the calls made it twice as easy to hit.) A plain `pilot status` dry-run measured +# 25 calls / 20.6s on this repo, and status.md step 4 runs exactly that. Without --squash-merged +# spend ZERO calls and say so — silence would be indistinguishable from "nothing found", the very +# confusion B3 exists to prevent. +if [ "$squash_merged" != "1" ]; then + echo " (not checked — one gh API call per branch; pass --squash-merged to check)" + echo " NB: in a squash-merge repo the section above is USUALLY (none) — squashed work is" + echo " not an ancestor — so cleanable branches usually appear HERE, not there." +else +gh_init +sq_refs=() +sq_shas=() +sq_prs=() +sq_errors=0 +# Iterate over FULL refs. The previous round stripped them back to bare names with sed, which threw +# away the disambiguation `%(refname:short)` performs and handed every later step a name that git +# resolves tag-first — the Critical this round. +while IFS= read -r ref; do + [ -z "$ref" ] && continue + b="${ref#refs/heads/}" + # Say WHY a branch never appears. Silently skipping protected names made e.g. `integration-tests` + # permanently uncleanable with zero visible reason once the boundary set widened. + if is_protected "$b"; then + # Only surface PATTERN-protected names. The integration branch and the branch you are standing + # on are structurally protected and saying so every run is noise; what needed a visible reason + # is the third kind — e.g. `integration-tests` swept up by the widened boundary set, which + # otherwise vanishes from every section with nothing explaining why. + # Printed in APPLY too. Gating it on dry-run hid the reason in the exact invocation run.md:78 + # defines as the standard post-merge command (`--squash-merged … --apply`) — i.e. the fix for + # "say why a branch never appears" was invisible in the only call that matters. + if [ "$b" != "$integration" ] && [ "$b" != "$current_branch" ]; then + echo " KEEP (protected by name/pattern) $b" + fi + continue + fi + is_in_worktree "$b" && continue + # Anything git already calls merged was handled above. Compared as a FULL ref — see the note on + # git_merged_refs: the short form is disambiguated to `heads/x` under a same-named tag, so this + # test used to miss and the branch fell through to the `-D` evidence path. + list_has "$git_merged_refs" "$ref" && continue + # Resolve the tip ONCE, from the full ref, and carry it: this sha is what the evidence is about + # and what authorises the delete. + sha="$(git rev-parse --verify --quiet "$ref^{commit}" 2>/dev/null || true)" + [ -n "$sha" ] || continue + pr="$(merged_pr_for "$sha")" + # Count PER-BRANCH lookup failures separately. gh_state only records init-level failure; + # a 403/5xx on an individual branch used to land in the same "no evidence" bucket, so a + # rate-limited run printed a byte-identical "(none)" to a genuinely clean repo — and a + # partially failed run silently dropped the branches it could not check. status.md (added in + # this same PR) tells the model to say "could not verify, not nothing"; it needs this signal + # to be able to. + if [ "$pr" = "ERR" ]; then sq_errors=$((sq_errors + 1)); continue; fi + [ -z "$pr" ] && continue + sq_refs+=("$ref"); sq_shas+=("$sha"); sq_prs+=("$pr") +done < <(git branch --format='%(refname)' 2>/dev/null) + +if [ "${#sq_refs[@]}" -eq 0 ]; then + if [ "$gh_state" = "unavailable" ]; then + echo " (cannot verify — gh not installed/authenticated, or repo not resolvable; branches KEPT)" + elif [ "$sq_errors" -gt 0 ]; then + echo " ($sq_errors branch(es) could not be verified — KEPT. NOT the same as 'nothing to clean'.)" + else + echo " (none)" + fi +else + # REPORT ONLY — identical output with or without --apply, because this section never deletes. + # The sha is printed so the command below is copy-pasteable and so you can see exactly which tip + # the evidence was gathered for; %q-quoted because `feat/x$(id)` is a legal refname. + i=0 + while [ "$i" -lt "${#sq_refs[@]}" ]; do + ref="${sq_refs[$i]}"; sha="${sq_shas[$i]}"; pr="${sq_prs[$i]}" + printf ' %s (merged via PR #%s) tip=%s\n' "${ref#refs/heads/}" "$pr" "${sha:0:12}" + printf ' delete with: git branch -D %q\n' "${ref#refs/heads/}" + i=$((i + 1)) + done + echo " ── listed, NOT deleted. Read the PR numbers above, then run the commands you agree with." + # A partial result must never read as a complete one. + [ "$sq_errors" -gt 0 ] && echo " ($sq_errors more branch(es) could not be verified — KEPT; this list is INCOMPLETE)" +fi +fi +echo + # ---- 2. Worktrees: clean + merged only -------------------------------------- echo "## Worktrees (clean + merged only)" main_root="$(git rev-parse --show-toplevel)" @@ -133,21 +431,67 @@ handle_wt() { echo " KEEP (dirty, $dirty changes) $path [$short]" return fi - # merged check - if [ "$short" = "(detached)" ] || is_protected "$short" || ! git branch --merged "$integration" 2>/dev/null | sed 's/^[* +] *//' | grep -Fqx "$short"; then + # merged check — git-native first, then the same squash evidence section 1b uses. + # Using ONLY `git branch --merged` here would have left worktrees permanently uncleanable in a + # squash repo: that predicate returns 0 rows by construction, which is the entire premise of + # this PR. Missing it meant the fix stopped at loose branches while `SKILL.md`'s own doctrine + # ("一个 task = 一个分支 = 一个 worktree = 一个 PR") makes the worktree the dominant unit, and + # `phases/status.md` reports worktree cleanup on every `pilot status`. + local wt_pr="" via="" + if [ "$short" = "(detached)" ] || is_protected "$short"; then echo " KEEP (not a merged feature branch) $path [$short]" return fi - if [ "$apply" = "1" ]; then - if git worktree remove "$path" 2>/dev/null; then - echo " removed $path [$short]" - git branch -d -- "$short" 2>/dev/null && echo " + deleted branch $short" || true - else - echo " SKIP (remove failed) $path [$short]" - fi + local wt_sha="" + if list_has "$git_merged_refs" "${branch:-refs/heads/$short}"; then + via="git" + elif [ "$squash_merged" != "1" ]; then + # Same cost gate as section 1b: this ran BEFORE the flag check and was the bulk of the + # 25-call / 20.6s plain dry-run. + echo " KEEP (not merged per git; pass --squash-merged to also check for a squash merge) $path [$short]" + return else - echo " would remove $path [$short] (+ delete branch $short)" + # Resolve via the FULL ref, never the bare short name: with a tag of the same name, + # `git rev-parse decoy` picks the TAG, so evidence — and the recovery handle — would describe + # the wrong commit. `$branch` is already refs/heads/ here. + wt_sha="$(git rev-parse --verify --quiet "${branch:-refs/heads/$short}^{commit}" 2>/dev/null || true)" + if [ -z "$wt_sha" ]; then + echo " KEEP (cannot resolve tip — NOT 'unmerged') $path [$short]" + return + fi + wt_pr="$(merged_pr_for "$wt_sha")" + if [ "$wt_pr" = "ERR" ]; then + echo " KEEP (could not verify — lookup failed, NOT 'unmerged') $path [$short]" + return + fi + if [ -z "$wt_pr" ]; then + echo " KEEP (not a merged feature branch) $path [$short]" + return + fi + via="squash-PR#$wt_pr" fi + # REPORT ONLY — this section removes nothing, in either mode. + # + # `git worktree remove` is a FILESYSTEM delete, and the previous pass stopped at ref writes and + # never got to it. Two layers each let the same case through: `git status --porcelain` does not + # list gitignored files, and `git worktree remove` without --force tolerates a tree that has only + # ignored files in it. Measured: a worktree holding `.env` (SECRET=abc) plus `ignored-stuff/` was + # deleted outright and the files were unrecoverable — while the header two screens up promised + # "does not do irreversible deletes at all". + # + # It is also newly reachable: on this PR's base, §2 only fired on git-native merge evidence, which + # a squash repo produces for nothing. Wiring it to GitHub-inferred squash evidence made it fire on + # every worktree whose PR was merged — and by this skill's own doctrine (one task = one worktree) + # that is the dominant unit. And run.md/status.md both invoke this unattended, with --apply. + # + # The same rule the rest of this file now follows applies here: the hard part (working out which + # worktrees are merged, with evidence) is automated; the unrecoverable part is printed for a human. + # This also disposes of the mid-bisect case (`git bisect start` with HEAD still on a branch reports + # porcelain-clean, and removing the worktree takes BISECT_LOG/BISECT_START with it). + printf ' %s [%s] (%s)\n' "$path" "$short" "$via" + printf ' remove with: git worktree remove %q && git branch -d %q\n' "$path" "$short" + printf ' (check for ignored files first — `git worktree remove` deletes the directory:\n' + printf ' git -C %q status --porcelain --ignored=matching)\n' "$path" } while IFS= read -r line; do case "$line" in @@ -158,30 +502,31 @@ while IFS= read -r line; do done < <(git worktree list --porcelain 2>/dev/null; echo "") echo -# ---- 3. Remote merged branches (opt-in) ------------------------------------- +# ---- 3. Remote branches: NOT handled --------------------------------------------------------- +# This section used to delete on the server; then it was reduced to printing a pasteable +# `git push --delete`. Both were wrong, and the second was wrong in a way that is easy to miss: +# it went safe on the WRONG AXIS. `git fetch --prune` only ran under --apply, so the plain dry-run +# — the mode that looks harmless — built its list from stale remote-tracking refs and printed a +# delete command for a branch a colleague had since pushed to. Reproduced against a real bare +# remote: dry-run printed the command, --apply (which fetches) correctly printed nothing. Pasting +# the dry-run line removed the ref, and a bare repo keeps no reflog for it. +# +# The `|| true` on that fetch made --apply no cure either: with an unreachable remote the refresh +# failed silently and the section printed the same command with no staleness warning. +# +# Once the printed command IS the destructive act, freshness matters MORE, not less. Making that +# correct means refreshing outside --apply, checking the refresh succeeded, and re-verifying each +# candidate rather than just the integration ref — i.e. rebuilding the whole thing this PR just +# spent six rounds learning not to build. GitHub's auto-delete-on-merge already does this job on +# the side where the merge happens. So: not handled here, and said out loud rather than silently +# dropped. if [ "$do_remote" = "1" ]; then - echo "## Remote merged branches ($remote_name)" - # Only mutate on --apply: even `fetch --prune` rewrites remote-tracking refs, so dry-run never fetches. - if [ "$apply" = "1" ]; then git fetch --prune "$remote_name" >/dev/null 2>&1 || true; fi - any=0 - while IFS= read -r rb; do - rb="$(echo "$rb" | sed 's/^[* +] *//')" - case "$rb" in - "$remote_name/HEAD"*|"") continue ;; - "$remote_name/"*) : ;; # this remote only - *) continue ;; # skip other remotes entirely (e.g. upstream/*) — never delete cross-remote - esac - short="${rb#"$remote_name"/}" - if is_protected "$short"; then continue; fi - if is_in_worktree "$short"; then continue; fi # never delete the remote of a checked-out/dirty worktree branch - any=1 - if [ "$apply" = "1" ]; then - if git push "$remote_name" --delete -- "$short" >/dev/null 2>&1; then echo " deleted $remote_name/$short"; else echo " SKIP (delete failed) $remote_name/$short"; fi - else - echo " would delete $remote_name/$short" - fi - done < <(git branch -r --merged "$integration" 2>/dev/null) - [ "$any" = "0" ] && echo " (none)" + echo "## Remote branches ($remote_name)" + echo " (not handled — safe-cleanup does not delete or list remote branches)" + echo " Enable 'Automatically delete head branches' in the repo settings: GitHub deletes the" + echo " head branch when a PR merges, judged server-side where the merge actually happened." + echo " Doing it from a clone means judging by possibly-stale refs, and a bare remote keeps no" + echo " reflog — the one delete here with no undo at all." echo fi