Commit b6423cff4312f4919b2374aca072b1b9d99197bd
1 parent
8d980dec
U-013 fix: auto-commit main-checkout drift too, not just the worktree's own
Attempts 8 and 9 both hit "local changes would be overwritten by merge" again, despite the earlier _check_prd_consistency worktree-isolation fix. Root cause is deeper: git worktrees deliberately SHARE project identity with their main checkout (same .git, just another checkout) -- a real `claude -p` call's memory:project write doesn't reliably respect the cwd it was invoked with, and can land directly on the main checkout regardless of which specific call triggered it. Worktree-wrapping one call (as done for _check_prd_consistency) only moves around which call happens to trigger this; it doesn't fix the underlying issue. ensure_worktree_committed() now also runs against repo_path itself (not just the worktree) right before every merge_worktree() call, in both work_node_run.run_work_node() and e2e_pipeline._check_prd_consistency(). Same already-tested mechanism, now applied generically regardless of which real call caused the drift or why. Fixing this surfaced a second real bug in the fix itself: running `git add -A` against repo_path while a node's own .graphlemon-worktrees/<id>/ checkout is still present on disk picks it up as a gitlink-like entry, which then shows as "deleted" the instant remove_worktree() runs right after. Fixed by excluding .graphlemon-worktrees/ from both the dirty-check and the add via pathspec (not relying on the caller repo's own .gitignore), plus added it to graphlemon's own .gitignore for correctness generally. New regression test: test_drift_on_main_checkout_is_auto_committed_before_merge reproduces a run_fn writing directly to repo_path instead of its worktree, and confirms a second node's own merge isn't affected by it. 562 tests green (441 src + 121 supervisor). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Showing
7 changed files
with
91 additions
and
10 deletions
.gitignore
| ... | ... | @@ -6,3 +6,10 @@ __pycache__/ |
| 6 | 6 | # source content. Agent memory (.claude/agent-memory/) IS tracked per |
| 7 | 7 | # architecture doc §8 decision 1 ("Memory...必须通过 Git 提交留痕"). |
| 8 | 8 | .claude/workflows/ |
| 9 | + | |
| 10 | +# Coder Work Node isolation checkouts (work_node_run.create_worktree). Live | |
| 11 | +# INSIDE the main repo's own working tree while a node is running -- must | |
| 12 | +# never be picked up by `git add -A` on repo_path itself (2026-08-03: doing | |
| 13 | +# so once made a still-present worktree checkout look like a tracked | |
| 14 | +# gitlink, which then showed as "deleted" the moment remove_worktree() ran). | |
| 15 | +.graphlemon-worktrees/ | ... | ... |
docs/development-plan.md
| ... | ... | @@ -136,6 +136,7 @@ |
| 136 | 136 | - **精度提升(2026-07-31,实现后补充)**:03-C §3.6 给出了 `USAGE_LIMIT_REACHED` 的精确探测正则 `/hit your (session|weekly|Opus) limit/i`,比 U-010/U-012 当时用的短语近似匹配更准——U-013 按这条正则实现,且真正写回 `work_node_execution.quota_wait_started_at`/`quota_limit_type`(U-010/U-012 阶段这两张表字段还没有真实归属)。`c_work_node_run` 是内部接口(03-C 原话"不对外暴露 HTTP,供 c_work_graph_execute 调用"),退避重试从 U-012 的整图级别下沉到这里的单节点级别 |
| 137 | 137 | - 覆盖:01-M `work_node_execution`;03-C `c_work_node_run`;REQ-03(boundary) |
| 138 | 138 | - **真实 bug(2026-08-02/03,U-042 第一次真实自举压测时发现)**:`RealGitWorktreeIsolationTest` 从 U-013 建成那天起,`run_fn` 一直是测试自己写的、写完文件后主动 `git add` + `git commit` 的假实现——这个盲区从没被真实 Coder 会话踩过,因为 PH1~PH4 所有真实调用都没启用 `needs_worktree=True`(development-plan.md U-031 的实现补充里已经记过这条),直到 U-035 让 self-target 场景无条件强制 worktree,U-042 第一次真实自举跑起来后才第一次真正踩中。真实现象:Coder 在 worktree 里确实编辑了文件,自己也验证过(`git status`/`git diff --stat` 都在它自己的 `test_report` 里回显了),但从未 `git commit`——Coder 人设里没有任何一处要求它提交自己的改动。`merge_worktree()` 只会 `git merge` 已提交的分支,worktree 分支上如果压根没有新提交,merge 就是个真正的空操作("Already up to date",不产生新 commit),紧接着 `remove_worktree()` 把这个还带着未提交改动的目录直接删掉——一次真实、已验证过的 Coder 改动就这样被无声吞掉,`implement_code()` 算出来的 `code_diff` 自然是空的。修复:新增 `work_node_run.ensure_worktree_committed()`,在 `merge_worktree()` 之前检查 worktree 是否有未提交改动(`git status --porcelain`),有则自动 `git add -A` + 提交一条 `graphlemon: auto-commit uncommitted worktree changes for <node_exec_id>`,worktree 本来就干净(比如纯排查、没有真的改代码)时什么也不做。新增两个回归测试:`test_uncommitted_worktree_changes_are_auto_committed_before_merge`(复现"改了但没提交"场景,断言改动最终真的落到主分支)、`test_clean_worktree_with_no_changes_is_still_a_valid_merge`(确认"确实没什么好改"这种合法结果不会被误伤)。这是"判断用 Agent,机制用代码"原则的又一次体现——不指望靠给人设加一条"记得 commit"的提示词来保证正确性,而是在系统层面兜底,人设漂移/遗忘不应该导致真实工作成果被无声丢弃。 |
| 139 | + - **真实 bug 的第二层(2026-08-03,U-042 第八/九次真实压测发现,比上面这条更深一层)**:`ensure_worktree_committed()` 只兜住了 worktree 自己的未提交改动,但 git worktree 的设计本身是**和主仓库共享同一个项目身份**(同一个 `.git`,只是另一份检出)——一次真实压测发现,`claude -p` 的 `memory: project` 写入并不总是遵守调用时传入的 `cwd`,而是可能直接落在**主仓库自己的检出目录**上,和当时具体是哪个节点、哪次调用完全无关(不是 `_check_prd_consistency` 一个函数的问题,把它单独包一层 worktree 只是挪动了"这次踩中的是哪次调用",没有从根上解决)。后果:主仓库工作区多出一处未提交的 memory 文件改动,下一个节点自己的、原本完全正常的 worktree merge 会因为"主分支有本地改动会被覆盖"直接报错退出(`git merge` exit 2)。修复:`ensure_worktree_committed()` 从只处理 worktree 自身,扩展成同时也对 `repo_path`(主仓库检出目录)本身跑一遍同样的"有脏改动就自动提交"逻辑,在每次 `merge_worktree()` 之前调用两次——不管是哪次真实调用造成的漂移、原因是什么,统一兜底,而不是针对某一个特定调用点单独打补丁。修复过程中顺带发现并解决了这条兜底自己引入的一个新问题:对主仓库跑 `git add -A` 时,如果某个节点的 worktree 检出目录(`.graphlemon-worktrees/<node_exec_id>/`)当时还没被 `remove_worktree()` 清理,会被当成一个类似 git link 的条目一并加进去,紧接着目录被删掉后 `git status` 就会显示成一条诡异的"D"(删除)——两处修复:`.graphlemon-worktrees/` 加进 `.gitignore`(这是长期正确性),`ensure_worktree_committed()` 自己的 `git add -A`/`git status --porcelain` 也显式排除这个路径(`-- . ':!.graphlemon-worktrees'`),不依赖调用方仓库一定配置了对的 `.gitignore`。新增回归测试 `test_drift_on_main_checkout_is_auto_committed_before_merge`,模拟"改动落在主仓库而不是 worktree"这个具体场景,并验证接下来一个节点的合并不受影响。 | |
| 139 | 140 | |
| 140 | 141 | ### PH1-S3 端到端验收(收口) |
| 141 | 142 | ... | ... |
docs/sdd/unit_matrix.json
| ... | ... | @@ -25,7 +25,7 @@ |
| 25 | 25 | {"id": "U-010", "phase": "PH1", "sub_phase": "PH1-S1", "name": "c_org_node_trigger 完整实现(三段前向状态迁移,见 development-plan.md 2026-07-31 范围说明)", "classification": "Value Path", "depends_on": ["U-005", "U-009"], "coverage": {"model": [], "view": [], "controller": ["c_org_node_trigger"], "process": [], "requirement": []}, "done": "三个 Org Agent 分别可被触发(真实 claude --agent 调用),task.status 按 queued→planning_brd_prd_sdd→coding→validating 正确流转;重试/路由/blocked_pending_human 不在本 Unit 范围"}, |
| 26 | 26 | {"id": "U-011", "phase": "PH1", "sub_phase": "PH1-S2", "name": "c_work_graph_plan 实现(只规划不执行,见 development-plan.md 2026-07-31/2026-08-03 订正)", "classification": "Value Path", "depends_on": ["U-004", "U-010"], "coverage": {"model": ["artifact", "work_graph_run"], "view": [], "controller": ["c_work_graph_plan"], "process": [], "requirement": ["REQ-02"]}, "done": "graph_manifest 通过结构校验(brd/prd/sdd/benchmark 四节点+依赖链),work_graph_run(status=planning) 落地;REQ-02 happy 用例整体随 U-012/U-013 累计满足。c_artifact_write(03-C §3.7)目前没有任何 Unit 认领,是已知缺口。2026-08-03:U-042 第六次真实自举压测发现真实 LLM 输出可能完全不给 depends_on/edges(比此前已知的\"字段名不对\"更彻底),_normalize_graph_manifest 现在对 brd/prd/sdd/benchmark 四个规范 artifact_type 齐全时强制按已知的固定线性链补 depends_on,不再完全依赖 LLM 自己给对"}, |
| 27 | 27 | {"id": "U-012", "phase": "PH1", "sub_phase": "PH1-S2", "name": "c_work_graph_execute 完整实现(见 development-plan.md 2026-07-31 范围说明)", "classification": "Value Path", "depends_on": ["U-006", "U-011"], "coverage": {"model": [], "view": [], "controller": ["c_work_graph_execute"], "process": ["p_work_graph_execution", "p_usage_limit_wait"], "requirement": ["REQ-07", "REQ-14"]}, "done": "REQ-07 boundary/failure 通过(happy 需 PH2 REQ-04/05/06 就绪后累计满足);REQ-14 happy 通过。退避重试是整图级别,非逐节点;CLASSIFIER_BLOCKED 用单次调用 permission_denials≥3 近似判定"}, |
| 28 | - {"id": "U-013", "phase": "PH1", "sub_phase": "PH1-S2", "name": "c_work_node_run + git worktree 隔离(见 development-plan.md 2026-07-31 订正、2026-08-02/03 真实 bug 修复)", "classification": "Value Path", "depends_on": ["U-012"], "coverage": {"model": ["work_node_execution"], "view": [], "controller": ["c_work_node_run"], "process": [], "requirement": ["REQ-03"]}, "done": "REQ-03 boundary 通过(真实 git worktree 隔离+merge,无冲突);happy 需 c_artifact_write 落地后累计满足。2026-08-02/03:U-042 第一次真实自举压测发现 Coder 编辑 worktree 后从不 commit、merge_worktree 空操作、真实改动被 remove_worktree 静默吞掉的真实 bug,新增 ensure_worktree_committed() 兜底自动提交,已修复并补测试"}, | |
| 28 | + {"id": "U-013", "phase": "PH1", "sub_phase": "PH1-S2", "name": "c_work_node_run + git worktree 隔离(见 development-plan.md 2026-07-31 订正、2026-08-02/03 真实 bug 修复)", "classification": "Value Path", "depends_on": ["U-012"], "coverage": {"model": ["work_node_execution"], "view": [], "controller": ["c_work_node_run"], "process": [], "requirement": ["REQ-03"]}, "done": "REQ-03 boundary 通过(真实 git worktree 隔离+merge,无冲突);happy 需 c_artifact_write 落地后累计满足。2026-08-02/03:U-042 第一次真实自举压测发现 Coder 编辑 worktree 后从不 commit、merge_worktree 空操作、真实改动被 remove_worktree 静默吞掉的真实 bug,新增 ensure_worktree_committed() 兜底自动提交;第八/九次压测又发现更深一层——worktree 和主仓库共享项目身份,memory:project 写入可能直接落在主仓库检出目录而非 worktree,导致下一个节点 merge 失败,已扩展 ensure_worktree_committed() 同时兜底主仓库自身、并把 .graphlemon-worktrees/ 加入 .gitignore,全部补了真实回归测试"}, | |
| 29 | 29 | {"id": "U-014", "phase": "PH1", "sub_phase": "PH1-S3", "name": "贪吃蛇场景端到端验收(含补建 c_artifact_write,见 development-plan.md 2026-07-31 实现补充)", "classification": "Value Path", "depends_on": ["U-011", "U-012", "U-013"], "coverage": {"model": ["artifact"], "view": [], "controller": ["c_artifact_write"], "process": [], "requirement": []}, "done": "架构文档 §11 验收标准第 1-3 条全部达成;plan_documents(Planner 四件套+血缘)真实验证过,Coder/Validator 全流程真实跑通待用户确认后执行"}, |
| 30 | 30 | |
| 31 | 31 | {"id": "U-015", "phase": "PH2", "sub_phase": "PH2-S1", "name": "p_validator_root_cause_trace 实现", "classification": "Risk Gate", "depends_on": ["U-013"], "coverage": {"model": ["bug_record"], "view": [], "controller": ["c_bug_record_create"], "process": ["p_validator_root_cause_trace"], "requirement": ["REQ-04"]}, "done": "REQ-04 happy/boundary/failure 通过"}, | ... | ... |
src/e2e_pipeline.py
| ... | ... | @@ -187,6 +187,11 @@ def _check_prd_consistency(target_folder_path, prd_content, timeout=180): |
| 187 | 187 | ) |
| 188 | 188 | if worktree_path: |
| 189 | 189 | work_node_run.ensure_worktree_committed(worktree_path, node_exec_id) |
| 190 | + # also catch drift on the MAIN checkout (worktrees share project | |
| 191 | + # identity, so a real `memory: project` write can land there | |
| 192 | + # regardless of `cwd` -- see work_node_run.ensure_worktree_ | |
| 193 | + # committed's own docstring) before the merge below. | |
| 194 | + work_node_run.ensure_worktree_committed(target_folder_path, node_exec_id + "-main-checkout-drift") | |
| 190 | 195 | work_node_run.merge_worktree(target_folder_path, node_exec_id) |
| 191 | 196 | return _coerce_bool(result["consistent"]), result.get("issues", []) |
| 192 | 197 | finally: | ... | ... |
src/test_e2e_pipeline.py
| ... | ... | @@ -235,7 +235,14 @@ class PrdConsistencySelfTargetWorktreeTest(unittest.TestCase): |
| 235 | 235 | # WORKTREE path as cwd, never the main checkout, when self-targeting |
| 236 | 236 | self.assertEqual(mock_write.call_args[0][0], "/repo/.graphlemon-worktrees/x") |
| 237 | 237 | self.assertEqual(mock_execute.call_args[0][0], "/repo/.graphlemon-worktrees/x") |
| 238 | - mock_ensure.assert_called_once() | |
| 238 | + # called twice: once for the worktree itself, once for the main | |
| 239 | + # checkout (worktrees share project identity, so a real memory | |
| 240 | + # write can land on the main checkout regardless of cwd -- see | |
| 241 | + # work_node_run.ensure_worktree_committed's own docstring) | |
| 242 | + self.assertEqual(mock_ensure.call_count, 2) | |
| 243 | + ensure_paths = [call.args[0] for call in mock_ensure.call_args_list] | |
| 244 | + self.assertIn("/repo/.graphlemon-worktrees/x", ensure_paths) | |
| 245 | + self.assertIn("/repo", ensure_paths) | |
| 239 | 246 | mock_merge.assert_called_once() |
| 240 | 247 | mock_remove.assert_called_once() |
| 241 | 248 | ... | ... |
src/test_work_node_run.py
| ... | ... | @@ -425,6 +425,40 @@ class RealGitWorktreeIsolationTest(WorkNodeRunTestBase): |
| 425 | 425 | ).stdout |
| 426 | 426 | self.assertEqual(status.strip(), "") |
| 427 | 427 | |
| 428 | + def test_drift_on_main_checkout_is_auto_committed_before_merge(self): | |
| 429 | + # 2026-08-03 regression: git worktrees deliberately share project | |
| 430 | + # identity with their main checkout -- a real self-hosting run | |
| 431 | + # showed `claude`'s own memory writes can land on the MAIN | |
| 432 | + # checkout (repo_path) instead of the worktree, regardless of | |
| 433 | + # cwd. If left uncommitted, the NEXT node's own worktree merge | |
| 434 | + # fails with "local changes would be overwritten by merge". This | |
| 435 | + # run_fn simulates that: it writes a file directly into repo_path | |
| 436 | + # (not cwd, i.e. not the worktree) to reproduce main-checkout | |
| 437 | + # drift caused by something outside the node's own isolated work. | |
| 438 | + def run_fn(cwd, model=None): | |
| 439 | + with open(os.path.join(self.tmp.name, "main-checkout-drift.txt"), "w") as f: | |
| 440 | + f.write("landed on the main checkout, not the worktree") | |
| 441 | + return {"value": "done"} | |
| 442 | + | |
| 443 | + work_node_run.run_work_node( | |
| 444 | + self.conn, "node-1", {"goal": "x"}, _IN_SCHEMA, _OUT_SCHEMA, | |
| 445 | + run_fn=run_fn, needs_worktree=True, repo_path=self.tmp.name, | |
| 446 | + ) | |
| 447 | + | |
| 448 | + status = subprocess.run( | |
| 449 | + ["git", "status", "--porcelain"], cwd=self.tmp.name, check=True, capture_output=True, text=True | |
| 450 | + ).stdout | |
| 451 | + self.assertEqual(status.strip(), "", "main checkout drift must be auto-committed, not left dirty") | |
| 452 | + self.assertTrue(os.path.isfile(os.path.join(self.tmp.name, "main-checkout-drift.txt"))) | |
| 453 | + | |
| 454 | + # a SECOND node's own worktree merge must not fail because of it | |
| 455 | + work_node_run.run_work_node( | |
| 456 | + self.conn, "node-2", {"goal": "x"}, _IN_SCHEMA, _OUT_SCHEMA, | |
| 457 | + run_fn=self._write_file_run_fn("module_b.txt", "module b content"), | |
| 458 | + needs_worktree=True, repo_path=self.tmp.name, | |
| 459 | + ) | |
| 460 | + self.assertTrue(os.path.isfile(os.path.join(self.tmp.name, "module_b.txt"))) | |
| 461 | + | |
| 428 | 462 | |
| 429 | 463 | class BreakpointRecoveryTest(WorkNodeRunTestBase): |
| 430 | 464 | """U-021 / REQ-06: run_work_node called again for a node_exec_id that was | ... | ... |
src/work_node_run.py
| ... | ... | @@ -117,7 +117,7 @@ def create_worktree(repo_path, node_exec_id): |
| 117 | 117 | return worktree_path |
| 118 | 118 | |
| 119 | 119 | |
| 120 | -def ensure_worktree_committed(worktree_path, node_exec_id): | |
| 120 | +def ensure_worktree_committed(path, node_exec_id): | |
| 121 | 121 | """A Coder Work Node is never actually required to `git commit` its own |
| 122 | 122 | edits before finishing -- nothing in its persona instructs it to, and |
| 123 | 123 | every prior test's `run_fn` happened to commit its own changes, so this |
| ... | ... | @@ -126,19 +126,40 @@ def ensure_worktree_committed(worktree_path, node_exec_id): |
| 126 | 126 | merge_worktree() found nothing to merge (a no-op "Already up to date" |
| 127 | 127 | merge) -- remove_worktree() then silently discarded the dirty working |
| 128 | 128 | tree along with the real work. Auto-commits any uncommitted state left |
| 129 | - in the worktree before it gets merged, so "the Coder said it edited a | |
| 129 | + at `path` before a merge is attempted, so "the Coder said it edited a | |
| 130 | 130 | file" and "the change actually lands" can't diverge silently. No-op |
| 131 | - when the worktree is already clean (a node that legitimately made no | |
| 132 | - changes, e.g. a pure investigation).""" | |
| 131 | + when `path` is already clean (a node that legitimately made no | |
| 132 | + changes, e.g. a pure investigation). | |
| 133 | + | |
| 134 | + **Also called on `repo_path` itself, not just the worktree (2026-08-03, | |
| 135 | + real self-hosting run)**: git worktrees deliberately SHARE project | |
| 136 | + identity with their main checkout -- a real run showed that `claude | |
| 137 | + -p`'s own `memory: project` writes land on the main checkout's | |
| 138 | + `.claude/agent-memory/` regardless of which worktree `cwd` the call | |
| 139 | + actually ran in (worktree-wrapping one specific call, as first tried | |
| 140 | + for `_check_prd_consistency`, only moves around which call happens to | |
| 141 | + trigger this, it doesn't prevent it). Calling this same, already-tested | |
| 142 | + committer on `repo_path` right before every `merge_worktree()` closes | |
| 143 | + that gap generically, regardless of which real call caused the drift or | |
| 144 | + why -- `path` being a worktree or the main checkout doesn't change what | |
| 145 | + "auto-commit any dirty state here" means.""" | |
| 146 | + # `.graphlemon-worktrees/` (create_worktree's own checkout location) | |
| 147 | + # lives INSIDE repo_path while a node is running -- excluded from both | |
| 148 | + # the dirty-check and the add, not just relying on the caller repo | |
| 149 | + # having a matching .gitignore (2026-08-03: without this, `git add -A` | |
| 150 | + # on repo_path picked up the still-present worktree checkout itself as | |
| 151 | + # a tracked gitlink-like entry, which then showed as "deleted" the | |
| 152 | + # moment remove_worktree() ran right after). | |
| 153 | + exclude_worktrees = ["--", ".", ":!.graphlemon-worktrees"] | |
| 133 | 154 | status = subprocess.run( |
| 134 | - ["git", "status", "--porcelain"], cwd=worktree_path, check=True, capture_output=True, text=True, | |
| 155 | + ["git", "status", "--porcelain"] + exclude_worktrees, cwd=path, check=True, capture_output=True, text=True, | |
| 135 | 156 | ).stdout |
| 136 | 157 | if not status.strip(): |
| 137 | 158 | return |
| 138 | - subprocess.run(["git", "add", "-A"], cwd=worktree_path, check=True, capture_output=True) | |
| 159 | + subprocess.run(["git", "add", "-A"] + exclude_worktrees, cwd=path, check=True, capture_output=True) | |
| 139 | 160 | subprocess.run( |
| 140 | - _prefix_git(["commit", "-m", "graphlemon: auto-commit uncommitted worktree changes for " + node_exec_id]), | |
| 141 | - cwd=worktree_path, check=True, capture_output=True, text=True, | |
| 161 | + _prefix_git(["commit", "-m", "graphlemon: auto-commit uncommitted changes for " + node_exec_id]), | |
| 162 | + cwd=path, check=True, capture_output=True, text=True, | |
| 142 | 163 | ) |
| 143 | 164 | |
| 144 | 165 | |
| ... | ... | @@ -342,6 +363,12 @@ def run_work_node( |
| 342 | 363 | conn.commit() |
| 343 | 364 | if worktree_path: |
| 344 | 365 | ensure_worktree_committed(worktree_path, node_exec_id) |
| 366 | + # also catch drift on the MAIN checkout itself (e.g. a | |
| 367 | + # real `memory: project` write that landed there instead | |
| 368 | + # of the worktree, see this function's own docstring) -- | |
| 369 | + # otherwise the merge below can fail with "local changes | |
| 370 | + # would be overwritten by merge". | |
| 371 | + ensure_worktree_committed(repo_path, node_exec_id + "-main-checkout-drift") | |
| 345 | 372 | merge_worktree(repo_path, node_exec_id) |
| 346 | 373 | remove_worktree(repo_path, worktree_path) # 03-C §3.6: "执行完成后清理 worktree" |
| 347 | 374 | return output_payload | ... | ... |