diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9f7a4fa6a7..8924b6f858 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -318,6 +318,32 @@ jobs: "TestHeldPathsCoverOnlyTheSameFileAcrossCaseSpellings", "TestLegacyWaitDoesNotConsumeExecutionCapacity"] - $tests | length) == 0 ' "$RUNNER_TEMP/isolation-owners.jsonl" + jq -se ' + . as $events | + {"reasonix/internal/platform/worktree": [ + "TestApplyPathsRejectsRedirectedAncestors", + "TestApplyPathsRejectsRedirectedAncestors/outside_delete", + "TestApplyPathsRejectsRedirectedAncestors/inside_delete", + "TestApplyPathsRejectsRedirectedAncestors/dangling_delete", + "TestApplyPathsRejectsRedirectedAncestors/outside_write", + "TestApplyPathsRejectsRedirectedAncestors/inside_write", + "TestApplyPathsRejectsRedirectedAncestors/regular_parent", + "TestApplyPathsRejectsRedirectedAncestors/unchanged", + "TestApplyPathsRejectsRedirectedAncestors/missing_creation", + "TestWriteChangesRejectsRedirectedDeletion", + "TestWriteChangesUnlinksLeafDirectoryLink", + "TestWholeWorkspaceApplyPreservesFileToDirectoryTransition"], + "reasonix/internal/runtime/isolation": [ + "TestApplyToolRetainsResultAfterAncestorReplacement"]} | + to_entries | all(.[]; . as $package | + ([ $events[] | select(.Package == $package.key and .Test == null and + (.Action == "pass" or .Action == "fail" or .Action == "skip")) ] | + length == 1 and .[0].Action == "pass") and + all(.value[]; . as $test | + [ $events[] | select(.Package == $package.key and .Test == $test and + (.Action == "pass" or .Action == "fail" or .Action == "skip")) ] | + length == 1 and .[0].Action == "pass")) + ' "$RUNNER_TEMP/isolation-owners.jsonl" - name: upload Windows isolation test inventory if: always() && env.RUN_STEPS == 'true' && runner.os == 'Windows' && github.event_name == 'pull_request' diff --git a/docs/WORKTREE_APPLY_ANCESTORS.md b/docs/WORKTREE_APPLY_ANCESTORS.md new file mode 100644 index 0000000000..fcb4ffd0d7 --- /dev/null +++ b/docs/WORKTREE_APPLY_ANCESTORS.md @@ -0,0 +1,95 @@ +--- +owner: @SivanCola +backup: @esengine +status: active +reviewed: 2026-10-28 +--- + +# Worktree apply ancestors / 工作树应用的父目录边界 + +## Behavior / 行为 + +An isolated candidate names Git directory entries. If a source ancestor becomes +a directory link while that candidate runs, matching leaf bytes do not establish +that the candidate still addresses the original path. + +Applying now refuses that +ancestor with the existing path-conflict result before reading leaf content or +writing a sibling change. Links inside the repository are refused too, including +links from a selected project folder to its surrounding repository. + +隔离候选使用 Git 目录项路径。候选运行期间,如果源文件的父目录被换成目录链接, +即使最终文件内容相同,也不能认定仍是原路径。应用现在会在读取最终文件内容或写入 +其他变更前,以已有的路径冲突结果拒绝该父目录。仓库内部链接也会被拒绝,包括从所选 +项目目录指向仓库其他目录的链接。 + +Deletion ancestors are checked in the shared worktree publisher, including +ordinary isolation and best-of callers. Path-by-path apply checks every changed +path's ancestors before hashing leaves. + +Deletion uses an open `os.Root` for the source repository, so a parent +link cannot redirect that filesystem operation outside the opened root. A link +at the final deleted entry is removed as an entry; its target is not removed. + +Whole-workspace application may delete a file before creating a directory at +the same name. Its deletion preflight preserves that conversion; Git owns the +subsequent restoration. Path-by-path application's existing refusal of +unsupported tree entries is unchanged. + +共同 worktree 发布层检查删除路径的父目录,覆盖普通隔离和 best-of 调用方。逐路径应用 +还在计算文件摘要前检查所有变更路径的父目录。 + +删除通过源仓库的已打开 `os.Root` 执行,父目录链接不能将该删除 +操作重定向到根目录外。被删除的最终目录项本身若是链接,只删除链接,不删除其目标。 + +整工作区应用可能先删除一个文件,再创建同名目录。删除前置检查保留这一转换,后续 +恢复由 Git 负责。逐路径应用原有的“不支持的 tree 条目”拒绝规则不变。 + +The existing apply tool reports the conflict and retains the original result ID +and candidate. After restoring the directory, reopening the Store can apply that +same ID. New directories, ordinary deletions, and unrelated workspace edits keep +their existing behavior. + +No configuration, tool schema, prompt prefix, journal, +or persisted format changes; existing results use the checks when applied by the +new reader. Older binaries retain their previous behavior. + +已有应用工具报告冲突并保留原 result ID 和候选。恢复目录后,重新打开 Store 可使用 +同一 ID 应用。新建目录、正常删除及工作区其他修改保留原行为。不修改配置、工具 +schema、提示词前缀、journal 或持久化格式;新版本应用旧结果时同样执行检查。 +旧二进制仍保留原行为。 + +## Evidence and limits / 证据与限制 + +Regression fixtures create real Git candidates, then replace source ancestors +with external, internal, dangling, or non-directory entries. They check both +deletions and writes, sibling preservation, and Store reconstruction followed by +same-ID retry. + +Windows fixtures use directory junctions; Unix fixtures use +symbolic links. The existing Windows isolation inventory requires every named +regression to pass, with no missing, duplicate, failed, or skipped terminal. + +回归测试创建真实 Git 候选,再将源父目录换成外部链接、内部链接、悬空链接或普通文件, +覆盖删除、写入、其他文件保持不变,以及重建 Store 后用原 ID 重试。Windows 使用目录 +junction,Unix 使用符号链接。已有 Windows 隔离清单要求每个指定测试通过,拒绝缺失、 +重复、失败或跳过的终态。 + +These are ancestor-type preconditions and confined deletion, not an atomic +multi-file transaction or an inode compare-and-swap. A directory replaced by +another real directory containing identical files is not distinguished. + +Checks cannot fence arbitrary writers between inspection and Git restoration. File +restoration still uses Git, and errors after publication begins may leave partial +changes. + +Physical process settlement, remote effects, mount changes, complete +native publisher ownership, and crash-safe rollback remain separate work. + +本修补提供父目录类型前置条件及受根目录约束的删除,不是多文件原子事务或 inode CAS。 +真实目录被换成包含相同文件的另一真实目录时,不会区分。 + +检查无法阻止任意外部写入者 +在检查与 Git 恢复之间修改路径。文件恢复仍由 Git 执行,开始发布后的错误可能留下部分 +变更。进程物理终结、远程效果、挂载点变化、完整原生发布所有权与崩溃安全回滚仍是 +独立的未完成工作。 diff --git a/internal/platform/worktree/apply_ancestor_test.go b/internal/platform/worktree/apply_ancestor_test.go new file mode 100644 index 0000000000..60f8e53441 --- /dev/null +++ b/internal/platform/worktree/apply_ancestor_test.go @@ -0,0 +1,162 @@ +package worktree + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "runtime" + "testing" + + "reasonix/internal/base/testenv" +) + +func TestApplyPathsRejectsRedirectedAncestors(t *testing.T) { + for _, kind := range []string{"outside_delete", "inside_delete", "dangling_delete", "outside_write", "inside_write", "regular_parent", "unchanged", "missing_creation"} { + t.Run(kind, func(t *testing.T) { + repo := initRepo(t) + writeRepoFile(t, repo, "app/nested/victim.txt", "original\n") + writeRepoFile(t, repo, "app/first.txt", "sibling\n") + gitIn(t, repo, "add", "-A") + gitIn(t, repo, "commit", "-q", "-m", "add project") + snap, err := TakeSnapshot(t.Context(), opened(t, filepath.Join(repo, "app"))) + if err != nil { + t.Fatal(err) + } + c, err := CreateCandidate(t.Context(), snap, filepath.Join(testenv.TempDir(t), "candidates")) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = RemoveCandidate(context.Background(), snap, c) }) + if err := os.Remove(filepath.Join(c.WorkspaceRoot, "first.txt")); err != nil { + t.Fatal(err) + } + writeTarget := kind == "outside_write" || kind == "inside_write" + if writeTarget { + writeRepoFile(t, c.WorkspaceRoot, "nested/victim.txt", "candidate\n") + } else if err := os.Remove(filepath.Join(c.WorkspaceRoot, "nested/victim.txt")); err != nil { + t.Fatal(err) + } + if kind == "missing_creation" { + writeRepoFile(t, c.WorkspaceRoot, "new/deep/created.txt", "created\n") + } + tree, changes, err := CandidateTree(t.Context(), snap, c) + if err != nil { + t.Fatal(err) + } + if len(changes) < 2 { + t.Fatalf("missing candidate changes: %+v", changes) + } + outside := testenv.TempDir(t) + if kind == "inside_delete" || kind == "inside_write" { + outside = filepath.Join(repo, "elsewhere") + } + writeRepoFile(t, outside, "victim.txt", "original\n") + parent := filepath.Join(repo, "app", "nested") + control := kind == "unchanged" || kind == "missing_creation" + if !control { + if err := os.Rename(parent, filepath.Join(repo, "saved")); err != nil { + t.Fatal(err) + } + if kind == "regular_parent" { + if err := os.WriteFile(parent, []byte("not a directory\n"), 0o644); err != nil { + t.Fatal(err) + } + } else { + target := outside + if kind == "dangling_delete" { + target = filepath.Join(outside, "missing") + } + linkApplyDirectory(t, target, parent) + } + } + err = ApplyPaths(t.Context(), snap, tree, changes) + if got := readRepoFile(t, outside, "victim.txt"); got != "original\n" { + t.Fatalf("outside content changed: %q", got) + } + if control { + if err != nil { + t.Fatal(err) + } + for _, path := range []string{"app/first.txt", "app/nested/victim.txt"} { + if _, err := os.Lstat(filepath.Join(repo, path)); !os.IsNotExist(err) { + t.Fatalf("deletion %s: %v", path, err) + } + } + if kind == "missing_creation" && readRepoFile(t, repo, "app/new/deep/created.txt") != "created\n" { + t.Fatal("missing parent creation failed") + } + return + } + var conflict *ConflictError + if !errors.As(err, &conflict) || len(conflict.Paths) != 1 || conflict.Paths[0] != "app/nested/victim.txt" { + t.Fatalf("ancestor conflict = %v", err) + } + if got := readRepoFile(t, repo, "app/first.txt"); got != "sibling\n" { + t.Fatalf("refusal wrote sibling: %q", got) + } + }) + } +} + +func TestWriteChangesRejectsRedirectedDeletion(t *testing.T) { + repo := initRepo(t) + outside := testenv.TempDir(t) + writeRepoFile(t, outside, "victim.txt", "original\n") + linkApplyDirectory(t, outside, filepath.Join(repo, "nested")) + err := writeChanges(t.Context(), opened(t, repo), "HEAD", []Change{{Status: "D", Path: "nested/victim.txt"}}) + if err == nil { + t.Fatal("publication accepted a redirected deletion") + } + if got := readRepoFile(t, outside, "victim.txt"); got != "original\n" { + t.Fatalf("outside file changed: %q", got) + } +} + +func TestWriteChangesUnlinksLeafDirectoryLink(t *testing.T) { + repo := initRepo(t) + outside := testenv.TempDir(t) + writeRepoFile(t, outside, "victim.txt", "original\n") + link := filepath.Join(repo, "link") + linkApplyDirectory(t, outside, link) + if err := writeChanges(t.Context(), opened(t, repo), "HEAD", []Change{{Status: "D", Path: "link"}}); err != nil { + t.Fatal(err) + } + if _, err := os.Lstat(link); !os.IsNotExist(err) { + t.Fatalf("leaf link was not removed: %v", err) + } + if got := readRepoFile(t, outside, "victim.txt"); got != "original\n" { + t.Fatalf("leaf deletion followed the link: %q", got) + } +} + +func TestWholeWorkspaceApplyPreservesFileToDirectoryTransition(t *testing.T) { + repo, snap, c, finish := isolatedCandidate(t) + if err := os.Remove(filepath.Join(c.WorkspaceRoot, "a.txt")); err != nil { + t.Fatal(err) + } + writeRepoFile(t, c.WorkspaceRoot, "a.txt/child.txt", "converted\n") + tree, changes := finish() + if err := Apply(t.Context(), snap, tree, changes); err != nil { + t.Fatal(err) + } + if got := readRepoFile(t, repo, "a.txt/child.txt"); got != "converted\n" { + t.Fatalf("conversion = %q", got) + } + if got := readRepoFile(t, repo, "b.txt"); got != "two\n" { + t.Fatalf("sibling = %q", got) + } +} + +func linkApplyDirectory(t *testing.T, target, path string) { + t.Helper() + if runtime.GOOS == "windows" { + out, err := exec.Command("cmd", "/c", "mklink", "/J", path, target).CombinedOutput() + if err != nil { + t.Fatalf("create directory junction: %v %s", err, out) + } + } else if err := os.Symlink(target, path); err != nil { + t.Fatal(err) + } +} diff --git a/internal/platform/worktree/apply_ancestors.go b/internal/platform/worktree/apply_ancestors.go new file mode 100644 index 0000000000..21be3ac790 --- /dev/null +++ b/internal/platform/worktree/apply_ancestors.go @@ -0,0 +1,50 @@ +package worktree + +import ( + "fmt" + "os" + "path/filepath" + "slices" +) + +// Git trees name directory entries, not paths reached through directory links. +// Check every ancestor before hashing leaves or publishing any sibling change. +func checkApplyAncestors(root *os.Root, changes []Change) error { + checked := make(map[string]bool) + var conflicts []string + for _, ch := range changes { + if !filepath.IsLocal(filepath.FromSlash(ch.Path)) { + return fmt.Errorf("%w: %s", ErrOutsideWorkspace, ch.Path) + } + var parents []string + for p := filepath.Dir(filepath.FromSlash(ch.Path)); p != "."; p = filepath.Dir(p) { + if p == filepath.Dir(p) { + return fmt.Errorf("%w: %s", ErrOutsideWorkspace, ch.Path) + } + parents = append(parents, p) + } + slices.Reverse(parents) + for _, parent := range parents { + blocked, known := checked[parent] + if !known { + info, err := root.Lstat(parent) + if os.IsNotExist(err) { + break + } + if err != nil { + return fmt.Errorf("inspect apply ancestor %s: %w", parent, err) + } + blocked = !info.IsDir() + checked[parent] = blocked + } + if blocked { + conflicts = append(conflicts, ch.Path) + break + } + } + } + if len(conflicts) > 0 { + return &ConflictError{Paths: conflicts} + } + return nil +} diff --git a/internal/platform/worktree/apply_paths.go b/internal/platform/worktree/apply_paths.go index b2cf332853..dbcf004ebb 100644 --- a/internal/platform/worktree/apply_paths.go +++ b/internal/platform/worktree/apply_paths.go @@ -83,6 +83,14 @@ func ApplyPaths(ctx context.Context, snap Snapshot, tree string, changes []Chang if err != nil { return err } + root, err := os.OpenRoot(source.WorkTree) + if err != nil { + return err + } + defer root.Close() + if err := checkApplyAncestors(root, changes); err != nil { + return err + } live, err := liveBlobs(ctx, source, paths, trust) if err != nil { return err @@ -104,7 +112,7 @@ func ApplyPaths(ctx context.Context, snap Snapshot, tree string, changes []Chang if len(conflicts) > 0 { return &ConflictError{Paths: conflicts} } - return writeChanges(ctx, source, tree, pending) + return writeChangesInRoot(ctx, source, root, tree, pending) } // ChangePaths is where each change lands in the source workspace, refusing any @@ -267,16 +275,33 @@ func hashText(ctx context.Context, repo gitcmd.Repo, text string) (string, error // writeChanges writes tree's version of each change into repo's files. func writeChanges(ctx context.Context, repo gitcmd.Repo, tree string, changes []Change) error { - repoRoot := repo.WorkTree + root, err := os.OpenRoot(repo.WorkTree) + if err != nil { + return err + } + defer root.Close() + return writeChangesInRoot(ctx, repo, root, tree, changes) +} + +func writeChangesInRoot(ctx context.Context, repo gitcmd.Repo, root *os.Root, tree string, changes []Change) error { + var deletions []Change var restore []string for _, ch := range changes { if ch.Status == "D" { - if err := os.Remove(filepath.Join(repoRoot, filepath.FromSlash(ch.Path))); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("apply deletion of %s: %w", ch.Path, err) - } - continue + deletions = append(deletions, ch) + } else { + restore = append(restore, ch.Path) + } + } + // Restored paths may need directories created by these deletions. Git owns + // their checkout; deletion must not follow a redirected source ancestor. + if err := checkApplyAncestors(root, deletions); err != nil { + return err + } + for _, ch := range deletions { + if err := root.Remove(filepath.FromSlash(ch.Path)); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("apply deletion of %s: %w", ch.Path, err) } - restore = append(restore, ch.Path) } if len(restore) == 0 { return nil diff --git a/internal/runtime/isolation/apply_ancestor_test.go b/internal/runtime/isolation/apply_ancestor_test.go new file mode 100644 index 0000000000..4c7b7c4785 --- /dev/null +++ b/internal/runtime/isolation/apply_ancestor_test.go @@ -0,0 +1,84 @@ +package isolation + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "runtime" + "slices" + "testing" + + "reasonix/internal/base/testenv" + "reasonix/internal/contract/tool" +) + +func TestApplyToolRetainsResultAfterAncestorReplacement(t *testing.T) { + repo := gitRepo(t) + write(t, repo, "nested/victim.txt", "original\n") + managed := testenv.TempDir(t) + s := NewStore(managed, repo, opened(t, repo)) + report, err := s.Execute(t.Context(), func(_ context.Context, run Run) (Outcome, error) { + write(t, run.WorkspaceRoot, "a.txt", "candidate\n") + return Outcome{}, os.Remove(filepath.Join(run.WorkspaceRoot, "nested/victim.txt")) + }, repo, "delete a nested file", "") + if err != nil { + t.Fatal(err) + } + id := idOf(t, report) + e, err := s.lookup(id) + if err != nil { + t.Fatal(err) + } + outside := testenv.TempDir(t) + write(t, outside, "victim.txt", "original\n") + parent, saved := filepath.Join(repo, "nested"), filepath.Join(repo, "saved") + if err := os.Rename(parent, saved); err != nil { + t.Fatal(err) + } + if runtime.GOOS == "windows" { + out, err := exec.Command("cmd", "/c", "mklink", "/J", parent, outside).CombinedOutput() + if err != nil { + t.Fatalf("create directory junction: %v %s", err, out) + } + } else if err := os.Symlink(outside, parent); err != nil { + t.Fatal(err) + } + resumed := NewStore(managed, repo, opened(t, repo)) + _, err = NewApplyTool(resumed).Execute(t.Context(), args(id)) + if got := read(t, outside, "victim.txt"); got != "original\n" { + t.Fatalf("external file changed: %q", got) + } + var refusal tool.Refusal + if !errors.As(err, &refusal) || refusal.Code != CodeApplyConflict { + t.Fatalf("apply did not report the ancestor conflict: %v", err) + } + if !slices.Contains(resumed.Pending(), id) { + t.Fatal("result lost its original pending ID") + } + if _, err := os.Stat(e.Cand.WorktreeRoot); err != nil { + t.Fatalf("candidate lost after refusal: %v", err) + } + if got := read(t, repo, "a.txt"); got != "one\n" { + t.Fatalf("refusal wrote sibling: %q", got) + } + if err := os.Remove(parent); err != nil { + t.Fatal(err) + } + if err := os.Rename(saved, parent); err != nil { + t.Fatal(err) + } + if _, err := NewApplyTool(NewStore(managed, repo, opened(t, repo))).Execute(t.Context(), args(id)); err != nil { + t.Fatalf("same-ID retry: %v", err) + } + if read(t, repo, "a.txt") != "candidate\n" { + t.Fatal("retry did not apply sibling") + } + if _, err := os.Stat(filepath.Join(repo, "nested/victim.txt")); !os.IsNotExist(err) { + t.Fatalf("retry did not delete intended path: %v", err) + } + if got := read(t, outside, "victim.txt"); got != "original\n" { + t.Fatalf("retry changed external file: %q", got) + } +} diff --git a/release-notes/studio/unreleased/14025-worktree-apply-ancestors.md b/release-notes/studio/unreleased/14025-worktree-apply-ancestors.md new file mode 100644 index 0000000000..e3ab051d05 --- /dev/null +++ b/release-notes/studio/unreleased/14025-worktree-apply-ancestors.md @@ -0,0 +1,11 @@ +## 修复 + +- 应用隔离结果时,拒绝被换成目录链接或普通文件的父目录,避免重定向删除;保留原结果 ID,恢复目录后可重试。 (#14025) +- 保留正常删除、新目录创建、最终链接删除和整工作区文件转目录行为。 (#14025) + + + +## Fixed + +- Refuse changed directory-link or file ancestors during isolated application; prevent redirected deletion and retain the original result ID for retry after restoring the directory. (#14025) +- Preserve ordinary deletion, new-directory creation, leaf-link deletion, and whole-workspace file-to-directory conversion. (#14025)