diff --git a/.agents/skills/e2e/SKILL.md b/.agents/skills/e2e/SKILL.md index 126018c590..fa4bca2d9c 100644 --- a/.agents/skills/e2e/SKILL.md +++ b/.agents/skills/e2e/SKILL.md @@ -157,7 +157,10 @@ path. Always inspect `error-context.md`; mobile/terminal flakes often show state that the stack trace alone hides, such as duplicate active terminals or a terminal stuck on "Starting terminal...". -When a PR-specific E2E shard fails, first identify the failed spec(s). If failures are in unrelated existing specs and no changed code plausibly affects that surface, record the failure as unrelated in the PR fixup summary instead of changing unrelated tests. +When a PR E2E shard fails, investigate every spec, including those outside the +diff; never dismiss, rerun, or merely record a failure as unrelated. Reproduce +the exact test, then preserve shard ordering and CI pressure. Fix every valid +defect with retries disabled, or evidence a concrete external blocker. **CRITICAL: E2E tests run against the production Vite build served by the Go backend**, not dev mode. After any frontend code change, you **must** rebuild before running tests (`pnpm e2e:run` does this for you): diff --git a/.agents/skills/pr-fixup/references/ci-troubleshooting.md b/.agents/skills/pr-fixup/references/ci-troubleshooting.md index 0ba6fa6236..dd2ef7a551 100644 --- a/.agents/skills/pr-fixup/references/ci-troubleshooting.md +++ b/.agents/skills/pr-fixup/references/ci-troubleshooting.md @@ -167,10 +167,11 @@ go test -race ./path/to/package -run '^TestName$' -count=20 go test -race ./path/to/package -count=3 ``` -If a failed-job rerun reports a different, unrelated package or test, validate -that second failure the same way and allow one additional failed-job rerun -instead of changing unrelated PR code. Stop and fix code when the same failure -reproduces locally or repeats in CI. +If a failed-job rerun reports a different package or test, validate that failure +with the same rigor even when it is outside the PR diff. Do not dismiss or rerun +past a valid race, leak, or product defect because it appears unrelated; fix it +and exercise the affected package without retry masking. Only an evidenced +external dependency or infrastructure fault is exempt from code remediation. ## E2E Failures @@ -191,14 +192,12 @@ scripts/run-quiet e2e -- bash -c 'cd apps && pnpm --filter @kandev/web e2e -- te scripts/run-quiet e2e -- bash -c 'cd apps && pnpm --filter @kandev/web e2e -- tests/path/to/failing.spec.ts -g "exact failing test title"' ``` -If the failed E2E spec is unrelated to the PR diff, the exact failing test -passes locally via `pnpm e2e:run`, and there are no unresolved review threads -or other failed checks, rerun the failed GitHub job once and poll until terminal: - -```bash -gh run rerun --failed -scripts/pr-state --summary -``` +An isolated pass does not invalidate a CI failure, and a spec outside the PR +diff remains part of fixup scope. Re-run it with retries disabled under CI +resource limits and preserve the failed shard's ordering. If that exposes a +race, stale state, or interaction leak, fix it and stress the smallest +reproducing sequence before running the full affected shard. Never use a +failed-job rerun as a substitute for explaining and fixing a valid test failure. When a UI copy rename is intentional, search E2E specs for old visible text before debugging deeper. Prefer updating assertions to the new label while diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go b/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go index 68d2dd4a69..6b2a7a4ffd 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go @@ -475,11 +475,12 @@ func (m *Manager) createExecution(ctx context.Context, taskID string, info *Work if info == nil { return nil, fmt.Errorf("workspace info is required") } - if err := reconcileWorkspaceSources(ctx, info.WorkspacePath, info.WorkspaceFolders); err != nil { + owner := ownedDirectoryLinkOwner(taskID, info.TaskDirName) + if err := reconcileWorkspaceSources(ctx, info.WorkspacePath, info.WorkspaceFolders, owner); err != nil { return nil, err } if info.ExecutorType == string(models.ExecutorTypeLocal) || info.ExecutorType == "local_pc" { - if err := reconcileWorkspaceRepositories(info.WorkspacePath, info.WorkspaceRepositories, m.logger); err != nil { + if err := reconcileWorkspaceRepositories(info.WorkspacePath, info.WorkspaceRepositories, m.logger, owner); err != nil { return nil, err } } diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go index 76ba8012a7..1434d16b84 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go @@ -1074,11 +1074,12 @@ func (m *Manager) launchInternal(ctx context.Context, req *LaunchRequest) (*Agen // 4. Resolve workspace path (non-worktree executors use this directly) workspacePath, mainRepoGitDir, worktreeID, worktreeBranch := m.launchResolveWorkspacePath(ctx, req) - if err := reconcileWorkspaceSources(ctx, workspacePath, req.WorkspaceFolders); err != nil { + owner := ownedDirectoryLinkOwner(req.TaskID, req.TaskDirName) + if err := reconcileWorkspaceSources(ctx, workspacePath, req.WorkspaceFolders, owner); err != nil { return nil, err } if req.ExecutorType == string(models.ExecutorTypeLocal) || req.ExecutorType == legacyExecutorTypeLocalPC { - if err := reconcileWorkspaceRepositories(workspacePath, workspaceRepositorySpecsFromLaunch(req), m.logger); err != nil { + if err := reconcileWorkspaceRepositories(workspacePath, workspaceRepositorySpecsFromLaunch(req), m.logger, owner); err != nil { return nil, err } } diff --git a/apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile.go b/apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile.go index 7d2405fb7f..0a22bd10b0 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile.go +++ b/apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile.go @@ -12,9 +12,13 @@ import ( "github.com/kandev/kandev/internal/worktree" ) +func ownedDirectoryLinkOwner(taskID, taskDirName string) worktree.OwnedDirectoryLinkOwner { + return worktree.OwnedDirectoryLinkOwner{TaskID: taskID, TaskDirName: taskDirName} +} + // reconcileWorkspaceSources recreates Kandev-owned links from durable source // specs before a host launch or workspace-only resume. -func reconcileWorkspaceSources(_ context.Context, root string, folders []WorkspaceFolderSpec) error { +func reconcileWorkspaceSources(_ context.Context, root string, folders []WorkspaceFolderSpec, owner worktree.OwnedDirectoryLinkOwner) error { if len(folders) == 0 { return nil } @@ -29,7 +33,7 @@ func reconcileWorkspaceSources(_ context.Context, root string, folders []Workspa if err != nil || !info.IsDir() { return fmt.Errorf("workspace folder %q target is missing: %s", folder.Name, folder.LocalPath) } - if _, _, err := worktree.EnsureOwnedDirectoryLink(root, folder.Name, folder.LocalPath); err != nil { + if _, err := worktree.EnsureOwnedDirectoryLink(root, folder.Name, folder.LocalPath, owner); err != nil { return fmt.Errorf("link workspace folder %q: %w", folder.Name, err) } } @@ -45,7 +49,7 @@ func reconcileWorkspaceSources(_ context.Context, root string, folders []Workspa // by index, because a host-materialized multi-repo local task roots the // workspace at ~/.kandev/tasks/ — there repositories[0] is a real // sibling and must keep its link. -func reconcileWorkspaceRepositories(root string, repositories []WorkspaceRepositorySpec, log *logger.Logger) error { +func reconcileWorkspaceRepositories(root string, repositories []WorkspaceRepositorySpec, log *logger.Logger, owner worktree.OwnedDirectoryLinkOwner) error { if len(repositories) == 0 { return nil } @@ -64,7 +68,7 @@ func reconcileWorkspaceRepositories(root string, repositories []WorkspaceReposit if err != nil || !info.IsDir() { return fmt.Errorf("workspace repository %q target is missing: %s", repository.RepoName, repository.RepositoryPath) } - if _, _, err := worktree.EnsureOwnedDirectoryLink(root, repository.RepoName, repository.RepositoryPath); err != nil { + if _, err := worktree.EnsureOwnedDirectoryLink(root, repository.RepoName, repository.RepositoryPath, owner); err != nil { return fmt.Errorf("link workspace repository %q: %w", repository.RepoName, err) } } diff --git a/apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile_test.go b/apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile_test.go index f6c03d2740..22a0aa82d6 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile_test.go @@ -16,7 +16,7 @@ import ( ) func TestReconcileWorkspaceSources_RejectsMissingFolderTarget(t *testing.T) { - err := reconcileWorkspaceSources(context.Background(), t.TempDir(), []WorkspaceFolderSpec{{Name: "missing", LocalPath: "/definitely/not/a/kandev-folder"}}) + err := reconcileWorkspaceSources(context.Background(), t.TempDir(), []WorkspaceFolderSpec{{Name: "missing", LocalPath: "/definitely/not/a/kandev-folder"}}, testWorkspaceLinkOwner()) if err == nil { t.Fatal("missing durable folder target was accepted") } @@ -33,10 +33,14 @@ func canonicalTempDir(t *testing.T) string { return d } +func testWorkspaceLinkOwner() worktree.OwnedDirectoryLinkOwner { + return worktree.OwnedDirectoryLinkOwner{TaskID: "task-1", TaskDirName: "task-1"} +} + func TestReconcileWorkspaceRepositories_RecreatesMissingOwnedLink(t *testing.T) { root, source := canonicalTempDir(t), t.TempDir() writeMarker(t, source) - if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: source}}, nil); err != nil { + if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: source}}, nil, testWorkspaceLinkOwner()); err != nil { t.Fatalf("reconcileWorkspaceRepositories: %v", err) } // Read through the link instead of comparing os.Readlink output: Go @@ -48,7 +52,7 @@ func TestReconcileWorkspaceRepositories_RecreatesMissingOwnedLink(t *testing.T) if err := os.Remove(filepath.Join(root, "api")); err != nil { t.Fatal(err) } - if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: source}}, nil); err != nil { + if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: source}}, nil, testWorkspaceLinkOwner()); err != nil { t.Fatalf("reconcile after reset: %v", err) } } @@ -71,7 +75,7 @@ func TestReconcileWorkspaceRepositories_SkipsRepositoryThatIsWorkspaceRoot(t *te root := t.TempDir() marker := writeMarker(t, root) - if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: root}}, nil); err != nil { + if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: root}}, nil, testWorkspaceLinkOwner()); err != nil { t.Fatalf("reconcileWorkspaceRepositories: %v", err) } if _, err := os.Lstat(filepath.Join(root, "api")); !os.IsNotExist(err) { @@ -99,7 +103,7 @@ func TestReconcileWorkspaceRepositories_PreservesAndReportsPreExistingSelfLink(t t.Fatal(err) } - if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: root}}, log); err != nil { + if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: root}}, log, testWorkspaceLinkOwner()); err != nil { t.Fatalf("reconcileWorkspaceRepositories: %v", err) } if _, err := os.Lstat(filepath.Join(root, "api")); err != nil { @@ -128,7 +132,7 @@ func TestReconcileWorkspaceRepositories_DoesNotWarnWithoutSelfLink(t *testing.T) t.Fatal(err) } - if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: root}}, log); err != nil { + if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: root}}, log, testWorkspaceLinkOwner()); err != nil { t.Fatalf("reconcileWorkspaceRepositories: %v", err) } if got := logs.FilterMessage(selfReferentialEntryWarning).Len(); got != 0 { @@ -144,7 +148,7 @@ func TestReconcileWorkspaceRepositories_LinksSiblingWhenPrimaryIsWorkspaceRoot(t err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{ {RepoName: "api", RepositoryPath: root}, {RepoName: "libs", RepositoryPath: sibling}, - }, nil) + }, nil, testWorkspaceLinkOwner()) if err != nil { t.Fatalf("reconcileWorkspaceRepositories: %v", err) } @@ -172,7 +176,7 @@ func TestReconcileWorkspaceRepositories_LocalLaunchRequestPlantsNoSelfLink(t *te // launchResolveWorkspacePath returns req.RepositoryPath when WorkspacePath // is empty and the executor is not worktree-backed. - if err := reconcileWorkspaceRepositories(repo, workspaceRepositorySpecsFromLaunch(req), nil); err != nil { + if err := reconcileWorkspaceRepositories(repo, workspaceRepositorySpecsFromLaunch(req), nil, testWorkspaceLinkOwner()); err != nil { t.Fatalf("reconcileWorkspaceRepositories: %v", err) } if _, err := os.Lstat(filepath.Join(repo, req.RepoName)); !os.IsNotExist(err) { @@ -193,7 +197,7 @@ func TestReconcileWorkspaceRepositories_RejectsFileAsRepositoryPath(t *testing.T t.Fatal(err) } - err := reconcileWorkspaceRepositories(file, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: file}}, nil) + err := reconcileWorkspaceRepositories(file, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: file}}, nil, testWorkspaceLinkOwner()) if err == nil { t.Fatal("a regular file was accepted as both workspace root and repository") } @@ -232,7 +236,7 @@ func TestIsWorkspaceEntryName(t *testing.T) { func TestReconcileWorkspaceRepositories_RejectsTraversalRepoName(t *testing.T) { root, source := t.TempDir(), t.TempDir() for _, name := range []string{".", "..", "/", string(filepath.Separator)} { - if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: name, RepositoryPath: source}}, nil); err == nil { + if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: name, RepositoryPath: source}}, nil, testWorkspaceLinkOwner()); err == nil { t.Fatalf("RepoName %q was accepted", name) } } @@ -246,7 +250,7 @@ func TestReconcileWorkspaceRepositories_LinksPrimaryWhenRootIsTaskDirectory(t *t primary := t.TempDir() writeMarker(t, primary) - if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: primary}}, nil); err != nil { + if err := reconcileWorkspaceRepositories(root, []WorkspaceRepositorySpec{{RepoName: "api", RepositoryPath: primary}}, nil, testWorkspaceLinkOwner()); err != nil { t.Fatalf("reconcileWorkspaceRepositories: %v", err) } if got, err := os.ReadFile(filepath.Join(root, "api", "live.txt")); err != nil || string(got) != "one" { diff --git a/apps/backend/internal/backendapp/workspace_source_materializer.go b/apps/backend/internal/backendapp/workspace_source_materializer.go index b147a6c14c..32cf64d946 100644 --- a/apps/backend/internal/backendapp/workspace_source_materializer.go +++ b/apps/backend/internal/backendapp/workspace_source_materializer.go @@ -70,6 +70,16 @@ type hostWorkspaceMaterialization struct { knownWorktree map[string]bool priorRoots []string postRoots []string + linkUndo []ownedDirectoryLinkUndo +} + +type ownedDirectoryLinkUndo struct { + Path string + PriorTarget string +} + +func materializedLinkOwner(taskID, taskDirName string) worktree.OwnedDirectoryLinkOwner { + return worktree.OwnedDirectoryLinkOwner{TaskID: taskID, TaskDirName: taskDirName} } func newWorkspaceSourceMaterializer(repo workspaceSourceMaterializerRepo, mgr *worktree.Manager, lc *lifecycle.Manager, log *logger.Logger) *workspaceSourceMaterializer { @@ -119,18 +129,17 @@ func (m *workspaceSourceMaterializer) materializeHostWorkspaceSources(ctx contex if err != nil { return nil, err } - created := make([]string, 0, len(batch.Sources)) adoptedSessions := make([]*models.TaskSession, 0, len(state.sessions)) defer func() { if err == nil { return } - if rollbackErr := m.rollbackHostWorkspaceMaterialization(ctx, taskID, state, materialization, adoptedSessions, created); rollbackErr != nil { + if rollbackErr := m.rollbackHostWorkspaceMaterialization(ctx, taskID, state, materialization, adoptedSessions); rollbackErr != nil { err = fmt.Errorf("%w; restore adopted session workspaces: %v", err, rollbackErr) } }() branchMaterializations, materialized, materializeErr := m.materializeHostRuntime(ctx, taskID, materialization.root, state, batch) - created = append(created, materialized...) + materialization.linkUndo = append(materialization.linkUndo, materialized...) if materializeErr != nil { return nil, materializeErr } @@ -171,6 +180,7 @@ func (m *workspaceSourceMaterializer) prepareHostWorkspaceMaterialization(ctx co return &hostWorkspaceMaterialization{ root: root, oldPath: state.environment.WorkspacePath, rootExisted: pathExists(root), knownWorktree: worktreeIDs(worktrees), priorRoots: priorRoots, postRoots: postRoots, + linkUndo: make([]ownedDirectoryLinkUndo, 0, len(batch.Sources)), }, nil } @@ -184,28 +194,29 @@ func worktreeIDs(worktrees []*worktree.Worktree) map[string]bool { return ids } -func (m *workspaceSourceMaterializer) materializeHostRuntime(ctx context.Context, taskID, root string, state *workspaceSourceMaterializationState, batch *models.WorkspaceSourceBatch) ([]*branchMaterialization, []string, error) { +func (m *workspaceSourceMaterializer) materializeHostRuntime(ctx context.Context, taskID, root string, state *workspaceSourceMaterializationState, batch *models.WorkspaceSourceBatch) ([]*branchMaterialization, []ownedDirectoryLinkUndo, error) { + owner := materializedLinkOwner(taskID, state.environment.TaskDirName) if state.environment.ExecutorType == string(models.ExecutorTypeWorktree) { - created, materializations, err := m.materializeWorktreeSources(ctx, taskID, root, batch, state.folders) + created, materializations, err := m.materializeWorktreeSources(ctx, taskID, root, batch, state.folders, owner) return materializations, created, err } entries, err := localWorkspaceEntries(state.repositories, state.folders, state.entities, batch) if err != nil { return nil, nil, err } - created, err := materializeDirectoryLinks(root, entries, "workspace source") + created, err := materializeDirectoryLinks(root, entries, "workspace source", owner) return nil, created, err } -func materializeDirectoryLinks(root string, entries map[string]string, description string) ([]string, error) { - created := make([]string, 0, len(entries)) +func materializeDirectoryLinks(root string, entries map[string]string, description string, owner worktree.OwnedDirectoryLinkOwner) ([]ownedDirectoryLinkUndo, error) { + created := make([]ownedDirectoryLinkUndo, 0, len(entries)) for name, target := range entries { - entry, wasCreated, err := worktree.EnsureOwnedDirectoryLink(root, name, target) + result, err := worktree.EnsureOwnedDirectoryLink(root, name, target, owner) if err != nil { return created, fmt.Errorf("link %s %q: %w", description, name, err) } - if wasCreated { - created = append(created, entry) + if result.Created { + created = append(created, ownedDirectoryLinkUndo{Path: result.Path, PriorTarget: result.PriorTarget}) } } return created, nil @@ -226,10 +237,10 @@ func (m *workspaceSourceMaterializer) adoptSessionWorkspaces(ctx context.Context return ids, adopted, nil } -func (m *workspaceSourceMaterializer) rollbackHostWorkspaceMaterialization(ctx context.Context, taskID string, state *workspaceSourceMaterializationState, materialization *hostWorkspaceMaterialization, adopted []*models.TaskSession, created []string) error { +func (m *workspaceSourceMaterializer) rollbackHostWorkspaceMaterialization(ctx context.Context, taskID string, state *workspaceSourceMaterializationState, materialization *hostWorkspaceMaterialization, adopted []*models.TaskSession) error { rollbackErr := m.restoreSessionWorkspaces(ctx, adopted, materialization.oldPath, materialization.priorRoots) - for index := len(created) - 1; index >= 0; index-- { - _ = os.Remove(created[index]) + if err := rollbackOwnedDirectoryLinks(materialization.linkUndo); err != nil { + rollbackErr = errors.Join(rollbackErr, err) } m.cleanupNewWorktrees(ctx, taskID, materialization.knownWorktree) if !materialization.rootExisted { @@ -243,6 +254,29 @@ func (m *workspaceSourceMaterializer) rollbackHostWorkspaceMaterialization(ctx c return rollbackErr } +func rollbackOwnedDirectoryLinks(undo []ownedDirectoryLinkUndo) error { + var rollbackErr error + for index := len(undo) - 1; index >= 0; index-- { + if err := rollbackOwnedDirectoryLink(undo[index]); err != nil { + rollbackErr = errors.Join(rollbackErr, err) + } + } + return rollbackErr +} + +func rollbackOwnedDirectoryLink(undo ownedDirectoryLinkUndo) error { + if undo.PriorTarget == "" { + if err := worktree.RemoveOwnedDirectoryLink(filepath.Dir(undo.Path), filepath.Base(undo.Path)); err != nil { + return fmt.Errorf("remove materialized link %q: %w", undo.Path, err) + } + return nil + } + if err := worktree.RestoreOwnedDirectoryLink(filepath.Dir(undo.Path), filepath.Base(undo.Path), undo.PriorTarget); err != nil { + return fmt.Errorf("restore repointed link %q: %w", undo.Path, err) + } + return nil +} + func (m *workspaceSourceMaterializer) cleanupNewWorktrees(ctx context.Context, taskID string, known map[string]bool) { worktrees, err := m.worktreeMgr.GetAllByTaskID(context.WithoutCancel(ctx), taskID) if err != nil { @@ -544,7 +578,7 @@ func isHostWorkspaceExecutor(executorType string) bool { return executorType == string(models.ExecutorTypeLocal) || executorType == legacyLocalPCExecutor || executorType == string(models.ExecutorTypeWorktree) } -func (m *workspaceSourceMaterializer) materializeWorktreeSources(ctx context.Context, taskID, root string, batch *models.WorkspaceSourceBatch, folders []*models.TaskWorkspaceFolder) ([]string, []*branchMaterialization, error) { +func (m *workspaceSourceMaterializer) materializeWorktreeSources(ctx context.Context, taskID, root string, batch *models.WorkspaceSourceBatch, folders []*models.TaskWorkspaceFolder, owner worktree.OwnedDirectoryLinkOwner) ([]ownedDirectoryLinkUndo, []*branchMaterialization, error) { materializations := make([]*branchMaterialization, 0, len(batch.Sources)) for _, source := range batch.Sources { if source.Repository != nil { @@ -564,14 +598,14 @@ func (m *workspaceSourceMaterializer) materializeWorktreeSources(ctx context.Con if err != nil { return nil, nil, err } - created := make([]string, 0, len(entries)) + created := make([]ownedDirectoryLinkUndo, 0, len(entries)) for name, target := range entries { - entry, wasCreated, err := worktree.EnsureOwnedDirectoryLink(root, name, target) + result, err := worktree.EnsureOwnedDirectoryLink(root, name, target, owner) if err != nil { return created, nil, fmt.Errorf("link worktree folder %q: %w", name, err) } - if wasCreated { - created = append(created, entry) + if result.Created { + created = append(created, ownedDirectoryLinkUndo{Path: result.Path, PriorTarget: result.PriorTarget}) } } return created, materializations, nil diff --git a/apps/backend/internal/backendapp/workspace_source_materializer_test.go b/apps/backend/internal/backendapp/workspace_source_materializer_test.go index c7125cf75b..82329b0f04 100644 --- a/apps/backend/internal/backendapp/workspace_source_materializer_test.go +++ b/apps/backend/internal/backendapp/workspace_source_materializer_test.go @@ -12,6 +12,7 @@ import ( "github.com/kandev/kandev/internal/agent/runtime/lifecycle" "github.com/kandev/kandev/internal/task/models" sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" + "github.com/kandev/kandev/internal/worktree" ) type remoteWorkspaceMaterializerStub struct { @@ -478,6 +479,53 @@ func TestWorkspaceSourceMaterializer_RollsBackLinkAndPathWhenAdoptionFails(t *te } } +func TestRollbackOwnedDirectoryLinkPreservesReplacementFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "notes") + const contents = "user replacement" + if err := os.WriteFile(path, []byte(contents), 0o644); err != nil { + t.Fatal(err) + } + + err := rollbackOwnedDirectoryLink(ownedDirectoryLinkUndo{Path: path}) + if err == nil { + t.Fatal("rollbackOwnedDirectoryLink removed a replacement file") + } + if got, readErr := os.ReadFile(path); readErr != nil || string(got) != contents { + t.Fatalf("replacement file = %q, %v; want it preserved", got, readErr) + } +} + +func TestWorkspaceSourceMaterializer_RestoresRepointedLinkWhenAdoptionFails(t *testing.T) { + ctx := context.Background() + repo := newMaterializerRepo(t) + tasksBase := filepath.Join(canonicalTempDir(t), "tasks") + root := filepath.Join(tasksBase, "task-1") + mgr := newMaterializerWorktreeMgr(t, root) + original := filepath.Join(canonicalTempDir(t), "original-notes") + replacement := filepath.Join(canonicalTempDir(t), "replacement-notes") + for path, content := range map[string]string{original: "before", replacement: "after"} { + if err := os.MkdirAll(path, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(path, "note.txt"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + seedWorkspaceSourceTask(t, repo, original) + if _, err := worktree.CreateOwnedDirectoryLink(root, "notes", original); err != nil { + t.Fatalf("seed owned directory link: %v", err) + } + + materializer := &workspaceSourceMaterializer{repo: repo, worktreeMgr: mgr, rescanner: &workspaceSourceRescanStub{err: os.ErrPermission}, logger: newTestLogger()} + batch := &models.WorkspaceSourceBatch{TaskID: "task-1", Sources: []models.WorkspaceSource{{Folder: &models.TaskWorkspaceFolder{DisplayName: "notes", LocalPath: replacement}}}} + if _, err := materializer.MaterializeWorkspaceSources(ctx, "task-1", batch); err == nil { + t.Fatal("MaterializeWorkspaceSources succeeded despite failed adoption") + } + if got, err := os.ReadFile(filepath.Join(root, "notes", "note.txt")); err != nil || string(got) != "before" { + t.Fatalf("repointed link after rollback = %q, %v; want original target restored", got, err) + } +} + func TestWorkspaceSourceMaterializer_RebindFailureRestoresEarlierSessionsInReverseOrder(t *testing.T) { ctx := context.Background() repo := newMaterializerRepo(t) diff --git a/apps/backend/internal/orchestrator/executor/executor_execute.go b/apps/backend/internal/orchestrator/executor/executor_execute.go index 8fb9b0c864..18cba5a1ba 100644 --- a/apps/backend/internal/orchestrator/executor/executor_execute.go +++ b/apps/backend/internal/orchestrator/executor/executor_execute.go @@ -1477,7 +1477,7 @@ func (e *Executor) applyRepositoryConfig(req *LaunchAgentRequest, task *v1.Task, } // Task directory mode: place worktree inside per-task directory if req.UseWorktree && repoInfo.Repository != nil && repoInfo.Repository.Name != "" { - req.TaskDirName = worktree.SemanticWorktreeName(task.Title, worktree.SmallSuffix(3)) + req.TaskDirName = worktree.SemanticWorktreeName(task.Title, worktree.TaskDirSuffix(task.ID)) } if repoInfo.Repository != nil && repoInfo.Repository.SetupScript != "" { if metadata == nil { diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index 2ca2e19506..c4ea42a25c 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -1285,7 +1285,7 @@ func resolveResumeTaskDirName(existingEnv *models.TaskEnvironment, task *v1.Task if existingEnv != nil && existingEnv.TaskDirName != "" { return existingEnv.TaskDirName } - return worktree.SemanticWorktreeName(task.Title, worktree.SmallSuffix(3)) + return worktree.SemanticWorktreeName(task.Title, worktree.TaskDirSuffix(task.ID)) } // persistResumeState updates the session row for a resume launch. For an agent diff --git a/apps/backend/internal/worktree/config.go b/apps/backend/internal/worktree/config.go index 5f7415d62b..d4c74c5f3d 100644 --- a/apps/backend/internal/worktree/config.go +++ b/apps/backend/internal/worktree/config.go @@ -2,6 +2,7 @@ package worktree import ( "crypto/rand" + "crypto/sha256" "fmt" "os" "path/filepath" @@ -378,6 +379,33 @@ func SmallSuffix(maxLen int) string { return string(buf) } +// taskDirSuffixLen is the number of characters in a task-directory suffix. Eight +// characters over the 36-symbol branchSuffixAlphabet give roughly 36^8 (~2.8e12) +// values, so cross-task collisions are negligible while the on-disk name stays +// short. The projection is collision-resistant, not injective. +const taskDirSuffixLen = 8 + +// TaskDirSuffix returns a deterministic, filesystem-safe suffix derived from a +// task ID. Unlike SmallSuffix (a random per-call value used for branch slugs), +// it is stable for a given ID and collision-resistant across distinct IDs, so +// two tasks whose titles sanitize to the same slug are overwhelmingly unlikely +// to resolve to the same ~/.kandev/tasks root, and a resume that must recompute +// the name reproduces the launch value. The suffix is a fixed-width projection +// of sha256(taskID), so it cannot guarantee uniqueness; a residual owned-root +// contention is caught downstream by the ownership marker, which fails closed on +// a TaskID/TaskDirName conflict. Returns the empty string for an empty ID. +func TaskDirSuffix(taskID string) string { + if taskID == "" { + return "" + } + sum := sha256.Sum256([]byte(taskID)) + buf := make([]byte, taskDirSuffixLen) + for i := range buf { + buf[i] = branchSuffixAlphabet[int(sum[i])%len(branchSuffixAlphabet)] + } + return string(buf) +} + // SanitizeRepoDirName converts a repository display name into a single, // filesystem-safe path segment. Path separators and other unsafe characters // are replaced with hyphens, runs of hyphens are collapsed, and surrounding diff --git a/apps/backend/internal/worktree/config_test.go b/apps/backend/internal/worktree/config_test.go index d6580efe27..fb6b600a2c 100644 --- a/apps/backend/internal/worktree/config_test.go +++ b/apps/backend/internal/worktree/config_test.go @@ -2,6 +2,48 @@ package worktree import "testing" +func TestTaskDirSuffix(t *testing.T) { + const id = "61ccfd2c-1121-4226-99ab-8d9a60a57e6e" + + got := TaskDirSuffix(id) + if got == "" { + t.Fatal("TaskDirSuffix returned empty string") + } + if len(got) != taskDirSuffixLen { + t.Fatalf("TaskDirSuffix(%q) length = %d, want %d", id, len(got), taskDirSuffixLen) + } + for _, r := range got { + if !isASCIIAlphaNum(r) { + t.Errorf("TaskDirSuffix(%q) = %q, contains non-alphanumeric %q", id, got, r) + } + if r >= 'A' && r <= 'Z' { + t.Errorf("TaskDirSuffix(%q) = %q, contains uppercase %q", id, got, r) + } + } + + if again := TaskDirSuffix(id); again != got { + t.Errorf("TaskDirSuffix is not stable: first %q, second %q", got, again) + } + + other := TaskDirSuffix("a2ac3b48-0000-0000-0000-000000000000") + if other == got { + t.Errorf("TaskDirSuffix collided for two different IDs: both %q", got) + } + + if TaskDirSuffix("") != "" { + t.Errorf("TaskDirSuffix(\"\") = %q, want empty", TaskDirSuffix("")) + } +} + +func TestSemanticWorktreeNameTaskUnique(t *testing.T) { + const title = "We need to improve alerting" + a := SemanticWorktreeName(title, TaskDirSuffix("61ccfd2c-1121-4226-99ab-8d9a60a57e6e")) + b := SemanticWorktreeName(title, TaskDirSuffix("a2ac3b48-1111-2222-3333-444455556666")) + if a == b { + t.Errorf("two tasks with the same title produced the same task-root name %q", a) + } +} + func TestSanitizeRepoDirName(t *testing.T) { cases := []struct { in, want string diff --git a/apps/backend/internal/worktree/directory_link.go b/apps/backend/internal/worktree/directory_link.go index 09a1607a8a..ee8ef73fb0 100644 --- a/apps/backend/internal/worktree/directory_link.go +++ b/apps/backend/internal/worktree/directory_link.go @@ -1,12 +1,18 @@ package worktree import ( + "context" + "errors" "fmt" "os" "path/filepath" "strings" + + storageworkspaces "github.com/kandev/kandev/internal/system/storage/workspaces" ) +const errWorkspaceOwnershipMarkerConflict = "workspace ownership marker conflicts with requested task root" + // CreateOwnedDirectoryLink creates a live directory reference below root. // root is created only through real (non-symlink) ancestors, and name must be // a single path segment. The caller owns root; this function never alters the @@ -26,7 +32,16 @@ func CreateOwnedDirectoryLink(root, name, target string) (string, error) { if err != nil { return "", err } - _, err = os.Lstat(link) + release, err := acquireWorktreeTargetPath(context.Background(), link) + if err != nil { + return "", fmt.Errorf("acquire owned link lock: %w", err) + } + defer release() + return createOwnedDirectoryLinkLocked(root, name, link, canonicalTarget) +} + +func createOwnedDirectoryLinkLocked(root, name, link, canonicalTarget string) (string, error) { + _, err := os.Lstat(link) if err == nil { return "", fmt.Errorf("owned link entry already exists: %s", name) } @@ -42,6 +57,61 @@ func CreateOwnedDirectoryLink(root, name, target string) (string, error) { return link, nil } +// RestoreOwnedDirectoryLink updates or recreates a live directory reference +// below root so it points at target, preserving the current link if the +// replacement cannot be completed safely. +func RestoreOwnedDirectoryLink(root, name, target string) error { + if !isOwnedDirectoryLinkPath(root, name) { + return fmt.Errorf("invalid owned link path") + } + if err := mkdirOwned(root); err != nil { + return err + } + link, err := ownedDirectoryLinkPath(root, name) + if err != nil { + return err + } + release, err := acquireWorktreeTargetPath(context.Background(), link) + if err != nil { + return fmt.Errorf("acquire owned link lock: %w", err) + } + defer release() + return restoreOwnedDirectoryLinkLocked(root, name, link, target) +} + +// RemoveOwnedDirectoryLink removes root/name only while it is still the +// directory link inspected under the per-link lock. A missing entry is already +// rolled back and is therefore treated as success. +func RemoveOwnedDirectoryLink(root, name string) error { + if !isOwnedDirectoryLinkPath(root, name) { + return fmt.Errorf("invalid owned link path") + } + link, err := ownedDirectoryLinkPath(root, name) + if err != nil { + return err + } + release, err := acquireWorktreeTargetPath(context.Background(), link) + if err != nil { + return fmt.Errorf("acquire owned link lock: %w", err) + } + defer release() + + info, err := os.Lstat(link) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return fmt.Errorf("inspect owned link entry: %w", err) + } + if !isPlatformDirectoryLink(info, link) { + return fmt.Errorf("owned link entry is not a directory link: %s", name) + } + if err := removeInspectedDirectoryLink(link, info); err != nil { + return fmt.Errorf("remove owned link: %w", err) + } + return nil +} + func isOwnedDirectoryLinkPath(root, name string) bool { return root != "" && filepath.IsAbs(root) && name != "" && filepath.Base(name) == name && name != "." && name != ".." } @@ -78,9 +148,24 @@ func verifyCreatedOwnedDirectoryLink(root, link string) error { return nil } -// EnsureOwnedDirectoryLink returns an existing matching live link or creates -// it. A non-link/collision, or a link to another directory, fails closed and is -// never replaced. +// OwnedDirectoryLinkOwner identifies the task that is allowed to repoint a +// Kandev-owned directory link under a shared task root. +type OwnedDirectoryLinkOwner struct { + TaskID string + TaskDirName string +} + +// OwnedDirectoryLinkResult describes the durable effect of EnsureOwnedDirectoryLink. +type OwnedDirectoryLinkResult struct { + Path string + Created bool + PriorTarget string +} + +// EnsureOwnedDirectoryLink returns an existing matching live link, repoints an +// owned link whose target drifted, or creates the link. A non-link/collision +// still fails closed and is never removed; only a Kandev-owned directory link +// (a pointer, not content) is replaced when its target no longer matches. // // The existing link is matched by filesystem identity, not by resolved path. // filepath.EvalSymlinks does not traverse a Windows junction — it returns the @@ -89,31 +174,163 @@ func verifyCreatedOwnedDirectoryLink(root, link string) error { // workspace source links failed. os.Stat follows a junction and a Unix symlink // alike, and os.SameFile compares volume and file index, which is also immune // to 8.3 short paths and path case. -func EnsureOwnedDirectoryLink(root, name, target string) (string, bool, error) { - link := filepath.Join(root, name) +// +// Repoint-on-mismatch is deliberately scoped here to the Kandev-owned task root: +// the entry lives below a root built through real ancestors, and the entry is a +// directory link, so removing and recreating it is safe. This is distinct from +// IsSelfReferentialDirectoryLink, which stays report-only because it concerns +// entries inside a user's own repository. +// +// The full inspect, create, and repoint flow is serialized per owned-link path +// with the shared target-path lock used by other Kandev writers. Unix still +// stages a sibling temp link and renames it into place; Windows still removes +// and recreates, but only while that exclusive slot is held. +func EnsureOwnedDirectoryLink(root, name, target string, owner OwnedDirectoryLinkOwner) (OwnedDirectoryLinkResult, error) { + link, err := ownedDirectoryLinkPath(root, name) + if err != nil { + return OwnedDirectoryLinkResult{}, err + } + release, err := acquireWorktreeTargetPath(context.Background(), link) + if err != nil { + return OwnedDirectoryLinkResult{}, fmt.Errorf("acquire owned link lock: %w", err) + } + defer release() + canonicalTarget, err := canonicalDirectoryLinkTarget(target) + if err != nil { + return OwnedDirectoryLinkResult{}, err + } info, err := os.Lstat(link) if err == nil { if !isPlatformDirectoryLink(info, link) { - return "", false, fmt.Errorf("owned link entry already exists: %s", name) + return OwnedDirectoryLinkResult{}, fmt.Errorf("owned link entry already exists: %s", name) } actual, err := os.Stat(link) if err != nil { - return "", false, fmt.Errorf("resolve owned link: %w", err) + return OwnedDirectoryLinkResult{}, fmt.Errorf("resolve owned link: %w", err) } - expected, err := os.Stat(target) + expected, err := os.Stat(canonicalTarget) if err != nil { - return "", false, fmt.Errorf("inspect link target: %w", err) + return OwnedDirectoryLinkResult{}, fmt.Errorf("inspect link target: %w", err) } - if !os.SameFile(actual, expected) { - return "", false, fmt.Errorf("owned link target mismatch: %s", name) + if os.SameFile(actual, expected) { + return OwnedDirectoryLinkResult{Path: link}, nil } - return link, false, nil + priorTarget, err := platformDirectoryLinkTarget(link) + if err != nil { + return OwnedDirectoryLinkResult{}, fmt.Errorf("read owned link target: %w", err) + } + if err := ensureOwnedLinkRepointAllowed(root, owner); err != nil { + return OwnedDirectoryLinkResult{}, err + } + if err := repointOwnedDirectoryLink(root, name, link, info, priorTarget, canonicalTarget); err != nil { + return OwnedDirectoryLinkResult{}, err + } + return OwnedDirectoryLinkResult{Path: link, Created: true, PriorTarget: priorTarget}, nil } if !os.IsNotExist(err) { - return "", false, fmt.Errorf("inspect owned link entry: %w", err) + return OwnedDirectoryLinkResult{}, fmt.Errorf("inspect owned link entry: %w", err) } - created, err := CreateOwnedDirectoryLink(root, name, target) - return created, err == nil, err + if err := mkdirOwned(root); err != nil { + return OwnedDirectoryLinkResult{}, err + } + created, err := createOwnedDirectoryLinkLocked(root, name, link, canonicalTarget) + if err != nil { + return OwnedDirectoryLinkResult{}, err + } + return OwnedDirectoryLinkResult{Path: created, Created: true}, nil +} + +func repointOwnedDirectoryLink(root, name, link string, inspected os.FileInfo, priorTarget, target string) error { + if err := replacePlatformDirectoryLink(link, inspected, target, priorTarget); err != nil { + return err + } + if err := verifyCreatedOwnedDirectoryLink(root, link); err != nil { + if restoreErr := restoreOwnedDirectoryLinkLocked(root, name, link, priorTarget); restoreErr != nil { + return errors.Join(err, restoreErr) + } + return err + } + return nil +} + +func restoreOwnedDirectoryLinkLocked(root, name, link, target string) error { + if target == "" { + return nil + } + canonicalTarget, err := canonicalDirectoryLinkTarget(target) + if err != nil { + return fmt.Errorf("restore prior directory link: %w", err) + } + info, err := os.Lstat(link) + if os.IsNotExist(err) { + if _, err := createOwnedDirectoryLinkLocked(root, name, link, canonicalTarget); err != nil { + return fmt.Errorf("restore prior directory link: %w", err) + } + return nil + } + if err != nil { + return fmt.Errorf("restore prior directory link: inspect owned link entry: %w", err) + } + if !isPlatformDirectoryLink(info, link) { + return fmt.Errorf("restore prior directory link: owned link entry already exists: %s", name) + } + actual, err := os.Stat(link) + if err != nil { + return fmt.Errorf("restore prior directory link: resolve owned link: %w", err) + } + expected, err := os.Stat(canonicalTarget) + if err != nil { + return fmt.Errorf("restore prior directory link: inspect link target: %w", err) + } + if os.SameFile(actual, expected) { + return nil + } + currentTarget, err := platformDirectoryLinkTarget(link) + if err != nil { + return fmt.Errorf("restore prior directory link: read owned link target: %w", err) + } + if err := replacePlatformDirectoryLink(link, info, canonicalTarget, currentTarget); err != nil { + return fmt.Errorf("restore prior directory link: %w", err) + } + if err := verifyCreatedOwnedDirectoryLink(root, link); err != nil { + return fmt.Errorf("restore prior directory link: %w", err) + } + return nil +} + +func ensureOwnedLinkRepointAllowed(root string, owner OwnedDirectoryLinkOwner) error { + marker, found, err := storageworkspaces.ReadOwnershipMarker(root) + if err != nil { + return fmt.Errorf("inspect workspace ownership marker: %w", err) + } + if !found { + return nil + } + if marker.TaskID != owner.TaskID || marker.TaskDirName != owner.TaskDirName { + return errors.New(errWorkspaceOwnershipMarkerConflict) + } + return nil +} + +func revalidateInspectedDirectoryLink(link string, inspected os.FileInfo) error { + current, err := os.Lstat(link) + if err != nil { + return fmt.Errorf("re-inspect owned link: %w", err) + } + if !isPlatformDirectoryLink(current, link) || !os.SameFile(current, inspected) { + return fmt.Errorf("owned link entry changed during repoint: %s", filepath.Base(link)) + } + return nil +} + +func renameInspectedDirectoryLink(tempLink, link string, inspected os.FileInfo) error { + if err := revalidateInspectedDirectoryLink(link, inspected); err != nil { + return err + } + if err := os.Rename(tempLink, link); err != nil { + return fmt.Errorf("repoint owned link: %w", err) + } + return nil } // IsSelfReferentialDirectoryLink reports whether root/name is a platform diff --git a/apps/backend/internal/worktree/directory_link_test.go b/apps/backend/internal/worktree/directory_link_test.go index 136c3c64cd..0166655e95 100644 --- a/apps/backend/internal/worktree/directory_link_test.go +++ b/apps/backend/internal/worktree/directory_link_test.go @@ -3,7 +3,10 @@ package worktree import ( "os" "path/filepath" + "strings" "testing" + + storageworkspaces "github.com/kandev/kandev/internal/system/storage/workspaces" ) // canonicalTempDir resolves symlinks in t.TempDir() so tests hand production @@ -79,6 +82,19 @@ func seedOwnedDirectoryLink(t *testing.T, root, name, target string) { } } +func testOwnedDirectoryLinkOwner() OwnedDirectoryLinkOwner { + return OwnedDirectoryLinkOwner{TaskID: "task-1", TaskDirName: "task-1"} +} + +func writeOwnershipMarker(t *testing.T, root string, owner OwnedDirectoryLinkOwner) { + t.Helper() + if err := storageworkspaces.WriteOwnershipMarker(root, storageworkspaces.OwnershipMarker{ + TaskID: owner.TaskID, TaskDirName: owner.TaskDirName, LayoutVersion: storageworkspaces.LayoutVersionSemantic, + }); err != nil { + t.Fatalf("WriteOwnershipMarker: %v", err) + } +} + // The predicate only reports. Every case below additionally asserts that the // inspected entry is still on disk afterwards, since it may be a link the user // or the repository keeps on purpose. @@ -146,34 +162,249 @@ func TestEnsureOwnedDirectoryLinkIsIdempotent(t *testing.T) { if err := os.WriteFile(filepath.Join(target, "live.txt"), []byte("one"), 0o644); err != nil { t.Fatal(err) } - first, created, err := EnsureOwnedDirectoryLink(root, "api", target) - if err != nil || !created { - t.Fatalf("first EnsureOwnedDirectoryLink: created=%v err=%v", created, err) + first, err := EnsureOwnedDirectoryLink(root, "api", target, testOwnedDirectoryLinkOwner()) + if err != nil || !first.Created { + t.Fatalf("first EnsureOwnedDirectoryLink: created=%v err=%v", first.Created, err) } - second, created, err := EnsureOwnedDirectoryLink(root, "api", target) + second, err := EnsureOwnedDirectoryLink(root, "api", target, testOwnedDirectoryLinkOwner()) if err != nil { t.Fatalf("second EnsureOwnedDirectoryLink: %v; an unchanged link must be reused", err) } - if created { + if second.Created { t.Fatal("second EnsureOwnedDirectoryLink reported creation") } - if second != first { - t.Fatalf("second link = %q, want %q", second, first) + if second.Path != first.Path { + t.Fatalf("second link = %q, want %q", second.Path, first.Path) } - if got, err := os.ReadFile(filepath.Join(second, "live.txt")); err != nil || string(got) != "one" { + if got, err := os.ReadFile(filepath.Join(second.Path, "live.txt")); err != nil || string(got) != "one" { t.Fatalf("read through reused link = %q, %v", got, err) } } -func TestEnsureOwnedDirectoryLinkRejectsDifferentTarget(t *testing.T) { +// A Kandev-owned task root is Kandev's to reconcile: an owned directory link is +// a pointer, not content, so a stale target left by an earlier launch must be +// repointed rather than wedging every launch and resume with a target mismatch. +func TestEnsureOwnedDirectoryLinkRepointsOwnedLinkOnMismatch(t *testing.T) { root := filepath.Join(canonicalTempDir(t), "tasks", "task-1") target, other := t.TempDir(), t.TempDir() - if _, _, err := EnsureOwnedDirectoryLink(root, "api", target); err != nil { - t.Fatalf("EnsureOwnedDirectoryLink: %v", err) + if err := os.WriteFile(filepath.Join(other, "live.txt"), []byte("two"), 0o644); err != nil { + t.Fatal(err) } + seedOwnedDirectoryLink(t, root, "api", target) - if _, _, err := EnsureOwnedDirectoryLink(root, "api", other); err == nil { - t.Fatal("EnsureOwnedDirectoryLink accepted a link pointing elsewhere") + result, err := EnsureOwnedDirectoryLink(root, "api", other, testOwnedDirectoryLinkOwner()) + if err != nil { + t.Fatalf("EnsureOwnedDirectoryLink repoint: %v", err) + } + if !result.Created { + t.Fatal("EnsureOwnedDirectoryLink did not report a recreate on mismatch") + } + linkInfo, err := os.Stat(result.Path) + if err != nil { + t.Fatalf("stat repointed link: %v", err) + } + otherInfo, err := os.Stat(other) + if err != nil { + t.Fatalf("stat new target: %v", err) + } + if !os.SameFile(linkInfo, otherInfo) { + t.Fatal("link was not repointed to the new target") + } + if got, err := os.ReadFile(filepath.Join(result.Path, "live.txt")); err != nil || string(got) != "two" { + t.Fatalf("read through repointed link = %q, %v", got, err) + } +} + +func TestEnsureOwnedDirectoryLinkRepointsOwnedLinkWithMatchingMarker(t *testing.T) { + root := filepath.Join(canonicalTempDir(t), "tasks", "task-1") + current, other := t.TempDir(), t.TempDir() + if err := os.WriteFile(filepath.Join(other, "live.txt"), []byte("two"), 0o644); err != nil { + t.Fatal(err) + } + seedOwnedDirectoryLink(t, root, "api", current) + writeOwnershipMarker(t, root, testOwnedDirectoryLinkOwner()) + + result, err := EnsureOwnedDirectoryLink(root, "api", other, testOwnedDirectoryLinkOwner()) + if err != nil { + t.Fatalf("EnsureOwnedDirectoryLink with matching marker: %v", err) + } + if !result.Created || result.PriorTarget == "" { + t.Fatalf("result = %+v, want Created with a prior target", result) + } + if got, err := os.ReadFile(filepath.Join(result.Path, "live.txt")); err != nil || string(got) != "two" { + t.Fatalf("read through matching-marker repoint = %q, %v", got, err) + } +} + +func TestEnsureOwnedDirectoryLinkRejectsMarkerConflictOnMismatch(t *testing.T) { + root := filepath.Join(canonicalTempDir(t), "tasks", "task-1") + current, other := t.TempDir(), t.TempDir() + if err := os.WriteFile(filepath.Join(current, "live.txt"), []byte("one"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(other, "live.txt"), []byte("two"), 0o644); err != nil { + t.Fatal(err) + } + seedOwnedDirectoryLink(t, root, "api", current) + writeOwnershipMarker(t, root, testOwnedDirectoryLinkOwner()) + + _, err := EnsureOwnedDirectoryLink(root, "api", other, OwnedDirectoryLinkOwner{TaskID: "task-2", TaskDirName: "task-2"}) + if err == nil || !strings.Contains(err.Error(), errWorkspaceOwnershipMarkerConflict) { + t.Fatalf("EnsureOwnedDirectoryLink error = %v, want marker conflict", err) + } + if got, err := os.ReadFile(filepath.Join(root, "api", "live.txt")); err != nil || string(got) != "one" { + t.Fatalf("marker-conflict repoint disturbed original link = %q, %v", got, err) + } +} + +func TestEnsureOwnedDirectoryLinkRejectsTraversalBeforeRepoint(t *testing.T) { + root := filepath.Join(canonicalTempDir(t), "tasks", "task-1") + current, other := t.TempDir(), t.TempDir() + if err := os.WriteFile(filepath.Join(current, "live.txt"), []byte("one"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(other, "live.txt"), []byte("two"), 0o644); err != nil { + t.Fatal(err) + } + seedOwnedDirectoryLink(t, root, "api", current) + + if _, err := EnsureOwnedDirectoryLink(root, "../api", other, testOwnedDirectoryLinkOwner()); err == nil { + t.Fatal("EnsureOwnedDirectoryLink accepted a traversal entry name") + } + if got, err := os.ReadFile(filepath.Join(root, "api", "live.txt")); err != nil || string(got) != "one" { + t.Fatalf("traversal attempt disturbed original link = %q, %v", got, err) + } +} + +func TestRemoveOwnedDirectoryLinkRemovesDirectoryLink(t *testing.T) { + root := filepath.Join(canonicalTempDir(t), "tasks", "task-1") + target := t.TempDir() + seedOwnedDirectoryLink(t, root, "notes", target) + + if err := RemoveOwnedDirectoryLink(root, "notes"); err != nil { + t.Fatalf("RemoveOwnedDirectoryLink: %v", err) + } + if _, err := os.Lstat(filepath.Join(root, "notes")); !os.IsNotExist(err) { + t.Fatalf("removed directory link still exists: %v", err) + } +} + +func TestRemoveOwnedDirectoryLinkPreservesNonLinkEntry(t *testing.T) { + root := filepath.Join(canonicalTempDir(t), "tasks", "task-1") + path := filepath.Join(root, "notes") + const contents = "user content" + if err := os.MkdirAll(root, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(contents), 0o644); err != nil { + t.Fatal(err) + } + + if err := RemoveOwnedDirectoryLink(root, "notes"); err == nil { + t.Fatal("RemoveOwnedDirectoryLink removed a non-link entry") + } + if got, err := os.ReadFile(path); err != nil || string(got) != contents { + t.Fatalf("non-link entry = %q, %v; want it preserved", got, err) + } +} + +func TestEnsureOwnedDirectoryLinkKeepsOriginalLinkWhenReplacementTargetIsInvalid(t *testing.T) { + root := filepath.Join(canonicalTempDir(t), "tasks", "task-1") + current := t.TempDir() + if err := os.WriteFile(filepath.Join(current, "live.txt"), []byte("one"), 0o644); err != nil { + t.Fatal(err) + } + seedOwnedDirectoryLink(t, root, "api", current) + + if _, err := EnsureOwnedDirectoryLink(root, "api", filepath.Join(t.TempDir(), "missing"), testOwnedDirectoryLinkOwner()); err == nil { + t.Fatal("EnsureOwnedDirectoryLink accepted a missing replacement target") + } + if got, err := os.ReadFile(filepath.Join(root, "api", "live.txt")); err != nil || string(got) != "one" { + t.Fatalf("failed replacement disturbed original link = %q, %v", got, err) + } +} + +func TestRestoreOwnedDirectoryLinkKeepsCurrentLinkWhenReplacementTargetIsInvalid(t *testing.T) { + root := filepath.Join(canonicalTempDir(t), "tasks", "task-1") + current := t.TempDir() + if err := os.WriteFile(filepath.Join(current, "live.txt"), []byte("one"), 0o644); err != nil { + t.Fatal(err) + } + seedOwnedDirectoryLink(t, root, "api", current) + + if err := RestoreOwnedDirectoryLink(root, "api", filepath.Join(t.TempDir(), "missing")); err == nil { + t.Fatal("RestoreOwnedDirectoryLink accepted a missing replacement target") + } + if got, err := os.ReadFile(filepath.Join(root, "api", "live.txt")); err != nil || string(got) != "one" { + t.Fatalf("failed restore disturbed current link = %q, %v", got, err) + } +} + +func TestRenameInspectedDirectoryLinkRejectsChangedEntry(t *testing.T) { + root := filepath.Join(canonicalTempDir(t), "tasks", "task-1") + current, other := t.TempDir(), t.TempDir() + if err := os.WriteFile(filepath.Join(current, "live.txt"), []byte("one"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(other, "live.txt"), []byte("two"), 0o644); err != nil { + t.Fatal(err) + } + seedOwnedDirectoryLink(t, root, "api", current) + inspected, err := os.Lstat(filepath.Join(root, "api")) + if err != nil { + t.Fatalf("Lstat(api): %v", err) + } + tempLink := filepath.Join(root, "api.tmp") + if err := createPlatformDirectoryLink(other, tempLink); err != nil { + t.Fatalf("create temp replacement link: %v", err) + } + if err := os.Remove(filepath.Join(root, "api")); err != nil { + t.Fatalf("remove original link: %v", err) + } + if err := os.Mkdir(filepath.Join(root, "api"), 0o755); err != nil { + t.Fatalf("create replacement directory: %v", err) + } + if err := os.WriteFile(filepath.Join(root, "api", "keep.txt"), []byte("keep"), 0o644); err != nil { + t.Fatal(err) + } + + if err := renameInspectedDirectoryLink(tempLink, filepath.Join(root, "api"), inspected); err == nil { + t.Fatal("renameInspectedDirectoryLink accepted a changed entry") + } + if got, err := os.ReadFile(filepath.Join(root, "api", "keep.txt")); err != nil || string(got) != "keep" { + t.Fatalf("changed entry was disturbed = %q, %v", got, err) + } +} + +// A non-link entry (a real file or directory a reconcile did not create) is not +// Kandev's pointer to replace: it stays fail-closed and is never removed. +func TestEnsureOwnedDirectoryLinkRejectsNonLinkEntry(t *testing.T) { + root := filepath.Join(canonicalTempDir(t), "tasks", "task-1") + if err := os.MkdirAll(filepath.Join(root, "api"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "api", "keep.txt"), []byte("keep"), 0o644); err != nil { + t.Fatal(err) + } + + if _, err := EnsureOwnedDirectoryLink(root, "api", t.TempDir(), testOwnedDirectoryLinkOwner()); err == nil { + t.Fatal("EnsureOwnedDirectoryLink overwrote a non-link entry") + } + if got, err := os.ReadFile(filepath.Join(root, "api", "keep.txt")); err != nil || string(got) != "keep" { + t.Fatalf("non-link entry was disturbed = %q, %v", got, err) + } + + // A regular file occupying the entry name is likewise not Kandev's pointer to + // replace: it stays fail-closed and its bytes must survive untouched. + file := filepath.Join(root, "web") + if err := os.WriteFile(file, []byte("keep"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := EnsureOwnedDirectoryLink(root, "web", t.TempDir(), testOwnedDirectoryLinkOwner()); err == nil { + t.Fatal("EnsureOwnedDirectoryLink overwrote a non-link file") + } + if got, err := os.ReadFile(file); err != nil || string(got) != "keep" { + t.Fatalf("non-link file was disturbed = %q, %v", got, err) } } diff --git a/apps/backend/internal/worktree/directory_link_unix.go b/apps/backend/internal/worktree/directory_link_unix.go index bcb2b547ba..a1726f7d09 100644 --- a/apps/backend/internal/worktree/directory_link_unix.go +++ b/apps/backend/internal/worktree/directory_link_unix.go @@ -2,10 +2,38 @@ package worktree -import "os" +import ( + "fmt" + "os" + "path/filepath" +) func createPlatformDirectoryLink(target, link string) error { return os.Symlink(target, link) } func isPlatformDirectoryLink(info os.FileInfo, _ string) bool { return info.Mode()&os.ModeSymlink != 0 } +func platformDirectoryLinkTarget(path string) (string, error) { return os.Readlink(path) } +func replacePlatformDirectoryLink(link string, inspected os.FileInfo, target, _ string) error { + tmp, err := os.CreateTemp(filepath.Dir(link), filepath.Base(link)+".tmp-*") + if err != nil { + return fmt.Errorf("stage directory link replacement: %w", err) + } + tmpPath := tmp.Name() + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("stage directory link replacement: %w", err) + } + if err := os.Remove(tmpPath); err != nil { + return fmt.Errorf("stage directory link replacement: %w", err) + } + if err := createPlatformDirectoryLink(target, tmpPath); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("create directory link: %w", err) + } + if err := renameInspectedDirectoryLink(tmpPath, link, inspected); err != nil { + _ = os.Remove(tmpPath) + return err + } + return nil +} func requirePlatformDirectoryLink(path string) error { info, err := os.Lstat(path) if err != nil { @@ -16,3 +44,13 @@ func requirePlatformDirectoryLink(path string) error { } return nil } + +func removeInspectedDirectoryLink(link string, inspected os.FileInfo) error { + if err := revalidateInspectedDirectoryLink(link, inspected); err != nil { + return err + } + if err := os.Remove(link); err != nil { + return fmt.Errorf("remove owned link: %w", err) + } + return nil +} diff --git a/apps/backend/internal/worktree/directory_link_windows.go b/apps/backend/internal/worktree/directory_link_windows.go index 1ad91e96c1..08462d6654 100644 --- a/apps/backend/internal/worktree/directory_link_windows.go +++ b/apps/backend/internal/worktree/directory_link_windows.go @@ -4,6 +4,7 @@ package worktree import ( "encoding/binary" + "errors" "fmt" "golang.org/x/sys/windows" "os" @@ -61,6 +62,30 @@ func isPlatformDirectoryLink(info os.FileInfo, path string) bool { } return hasMountPoint(path) } +func platformDirectoryLinkTarget(path string) (string, error) { return os.Readlink(path) } +func removeInspectedDirectoryLink(link string, inspected os.FileInfo) error { + if err := revalidateInspectedDirectoryLink(link, inspected); err != nil { + return err + } + if err := os.Remove(link); err != nil { + return fmt.Errorf("repoint owned link: %w", err) + } + return nil +} +func replacePlatformDirectoryLink(link string, inspected os.FileInfo, target, priorTarget string) error { + if err := removeInspectedDirectoryLink(link, inspected); err != nil { + return err + } + if err := createPlatformDirectoryLink(target, link); err != nil { + if priorTarget != "" { + if restoreErr := createPlatformDirectoryLink(priorTarget, link); restoreErr != nil { + return errors.Join(fmt.Errorf("create directory link: %w", err), fmt.Errorf("restore prior directory link: %w", restoreErr)) + } + } + return fmt.Errorf("create directory link: %w", err) + } + return nil +} func requirePlatformDirectoryLink(path string) error { info, err := os.Lstat(path) if err != nil { diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index 5e1db28334..42c465a9b7 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -1782,7 +1782,7 @@ export class ApiClient { } async setPrimarySession(sessionId: string): Promise { - await this.request("POST", `/api/v1/task-sessions/${sessionId}/set-primary`); + await this.wsRequest("session.set_primary", { session_id: sessionId }); } async deleteSession(sessionId: string): Promise { diff --git a/apps/web/e2e/helpers/session.ts b/apps/web/e2e/helpers/session.ts index 51de46c398..e17b5276f0 100644 --- a/apps/web/e2e/helpers/session.ts +++ b/apps/web/e2e/helpers/session.ts @@ -110,5 +110,6 @@ export async function seedIdleSession( if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id"); const session = await openTaskSession(testPage, task.id); await session.waitForChatIdle({ timeout: 30_000 }); + await session.composerReady(); return session; } diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index b416d778da..43e3a11475 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -102,11 +102,47 @@ export class SessionPage { return this.page.locator("[data-testid='session-chat']:visible").first(); } + /** + * Wait for the session chat panel to be visible. + * + * When multiple session tabs are open, multiple session-chat panels exist in + * the DOM but only the active one is visible. Use :visible to avoid matching + * a hidden background panel (which would cause the wait to time out). + * + * Under CI shard load the freshly-navigated task page can be slow to hydrate: + * the SSR boot payload + React mount + WS connect sequence races, and a single + * hard `waitFor` occasionally exceeds its budget before the chat panel mounts. + * Reloading re-drives SSR hydration and reliably recovers, so instead of one + * fixed wait we poll with a bounded reload-and-retry loop (same recovery shape + * as `waitForChatIdle`). The fast path stays instant when the chat is already + * visible. + */ async waitForLoad(timeout = 15_000) { - // When multiple session tabs are open, multiple session-chat panels exist in - // the DOM but only the active one is visible. Use :visible to avoid matching - // a hidden background panel (which would cause the wait to time out). - await this.activeChat().waitFor({ state: "visible", timeout }); + const chat = this.activeChat(); + // Fast path: already foregrounded (common case, no reload cost). + if (await chat.isVisible()) return; + + const attemptTimeout = Math.min(timeout, Math.max(5_000, Math.floor(timeout / 2))); + const start = Date.now(); + let lastReloadAt = start; + + while (Date.now() - start < timeout) { + const remaining = timeout - (Date.now() - start); + const now = Date.now(); + // Re-drive SSR hydration once per attemptTimeout slice while budget remains + // for the reloaded page to settle. + if (now - lastReloadAt >= attemptTimeout && remaining > attemptTimeout) { + lastReloadAt = now; + await this.page.reload(); + } + await chat + .waitFor({ state: "visible", timeout: Math.min(attemptTimeout, remaining) }) + .catch(() => undefined); + if (await chat.isVisible()) return; + } + + // Final bounded check: still throws on a genuinely stuck page. + await chat.waitFor({ state: "visible", timeout: attemptTimeout }); } /** @@ -148,17 +184,22 @@ export class SessionPage { * This is the same race the office agent-run-live spec rides out with * `expect.poll`-based re-seeding. */ - async waitForChatIdle(opts: { timeout?: number; attemptTimeout?: number } = {}) { + async waitForChatIdle( + opts: { timeout?: number; attemptTimeout?: number; requireEditable?: boolean } = {}, + ) { const softTotalTimeout = opts.timeout ?? 45_000; const attemptTimeout = opts.attemptTimeout ?? Math.min(15_000, Math.max(5_000, Math.floor(softTotalTimeout / 3))); const pollSlice = 1_500; const idle = this.anyIdleInput(); + const editor = this.activeChat().locator(".tiptap.ProseMirror:visible").first(); + const isReady = async () => + (await idle.isVisible()) && (!opts.requireEditable || (await editor.isEditable())); const start = Date.now(); let lastReloadAt = start; while (Date.now() - start < softTotalTimeout) { - if (await idle.isVisible()) return; + if (await isReady()) return; const resumeButton = this.recoveryResumeButton(); if (await resumeButton.isVisible()) { @@ -191,6 +232,10 @@ export class SessionPage { // Final bounded check: still throws on a genuinely stuck session, but gives // the last hydration attempt a full attemptTimeout slice to land. + if (opts.requireEditable) { + await expect.poll(isReady, { timeout: attemptTimeout }).toBe(true); + return; + } await idle.waitFor({ state: "visible", timeout: attemptTimeout }); } @@ -343,20 +388,20 @@ export class SessionPage { /** Chat input placeholder when agent is idle (default mode). */ idleInput(): Locator { - return this.page.locator('[data-placeholder="Continue working on the task..."]'); + return this.activeChat().locator('[data-placeholder="Continue working on the task..."]'); } /** Chat input placeholder when agent is idle in any current mode. */ anyIdleInput(): Locator { - return this.page + return this.activeChat() .locator('[data-placeholder="Continue working on the task..."]') - .or(this.page.locator('[data-placeholder="Continue working on the plan..."]')) - .or(this.page.locator('[data-placeholder="Continue working on the file..."]')); + .or(this.activeChat().locator('[data-placeholder="Continue working on the plan..."]')) + .or(this.activeChat().locator('[data-placeholder="Continue working on the file..."]')); } /** Chat input placeholder when agent is idle (plan mode). */ planModeInput(): Locator { - return this.page.locator('[data-placeholder="Continue working on the plan..."]'); + return this.activeChat().locator('[data-placeholder="Continue working on the plan..."]'); } /** @@ -965,8 +1010,7 @@ export class SessionPage { * TipTap maps "Mod" to Meta on macOS and Control on Linux/Windows. */ async sendMessage(text: string) { - const editor = this.activeChat().locator('.tiptap.ProseMirror[contenteditable="true"]').first(); - await expect(editor).toBeEditable(); + const editor = await this.composerReady(); await editor.click(); await editor.fill(text); const modifier = process.platform === "darwin" ? "Meta" : "Control"; @@ -978,11 +1022,70 @@ export class SessionPage { * don't submit on Ctrl/Cmd+Enter, so mobile specs use this instead. */ async sendMessageViaButton(text: string) { - const editor = this.activeChat().locator('.tiptap.ProseMirror[contenteditable="true"]').first(); - await expect(editor).toBeEditable(); + const editor = await this.composerReady(); await editor.click(); await editor.fill(text); - await this.page.getByTestId("submit-message-button").click(); + const isTouch = await this.page.evaluate(() => window.matchMedia("(pointer: coarse)").matches); + if (isTouch) { + await this.tapSubmitWhenReady(); + return; + } + await this.clickSubmitWhenReady(); + } + + /** The composer's send/submit button (scoped to the active chat panel). */ + submitButton(): Locator { + return this.activeChat().getByTestId("submit-message-button"); + } + + /** + * Tap the submit button only once it is actually enabled. + * + * The button renders a spinner and is `disabled` while the composer is in a + * transient not-ready state (`isSending`/`isStarting`/`isMoving`) — most + * commonly the brief STARTING lifecycle an auto-started session passes through + * right after it first goes idle. Acting on the button during that window is a + * no-op tap that silently drops the message, so we gate on `toBeEnabled` + * (waiting for the `disabled` attribute to clear — a condition, not a longer + * fixed delay) before tapping. Mirrors `clickSubmitWhenReady` for desktop. + */ + async tapSubmitWhenReady() { + const submit = this.submitButton(); + await expect(submit).toBeEnabled(); + await submit.tap(); + } + + /** Desktop analog of `tapSubmitWhenReady` (uses click instead of tap). */ + async clickSubmitWhenReady() { + const submit = this.submitButton(); + await expect(submit).toBeEnabled(); + await submit.click(); + } + + /** + * Resolve the active chat's ProseMirror composer and wait until it is + * actually editable before returning it. + * + * TipTap uses `immediatelyRender: false`, so `EditorContent` mounts the + * `.tiptap.ProseMirror` node only after the editor instance is created in a + * post-mount effect; until then the contenteditable host is absent or still + * `contenteditable="false"`. Callers reach here after `waitForLoad` / + * `waitForChatIdle` have already driven hydration, so the default + * `toBeEditable` wait is the correct condition to synchronize on. + */ + async composerReady(): Promise { + const editor = this.activeChat().locator('.tiptap.ProseMirror[contenteditable="true"]').first(); + try { + await expect(editor).toBeEditable(); + } catch { + // `waitForChatIdle` intentionally treats the visible idle placeholder as + // sufficient for terminal workflow states, where the composer may stay + // disabled. Sending requires the stronger editable condition; re-drive + // that condition when startup hydration exposed a stale idle placeholder. + await this.waitForChatIdle({ timeout: 30_000, requireEditable: true }); + await expect(editor).toBeEditable(); + } + return editor; } /** diff --git a/apps/web/e2e/tests/chat/chat-input-font-size-helpers.ts b/apps/web/e2e/tests/chat/chat-input-font-size-helpers.ts index 606ebbcfbb..35564559b2 100644 --- a/apps/web/e2e/tests/chat/chat-input-font-size-helpers.ts +++ b/apps/web/e2e/tests/chat/chat-input-font-size-helpers.ts @@ -23,6 +23,7 @@ export async function openTaskChat(page: Page, taskId: string): Promise await session.waitForLoad(); await session.waitForChatIdle({ timeout: 30_000 }); - const editor = session.activeChat().locator(".tiptap.ProseMirror:visible"); + const editor = await session.composerReady(); await editor.tap(); await editor.fill("/detached-background 20s"); await testPage.getByTestId("submit-message-button").tap(); diff --git a/apps/web/e2e/tests/chat/mobile-entity-reference-composer.spec.ts b/apps/web/e2e/tests/chat/mobile-entity-reference-composer.spec.ts index 7ef8e8dbe2..aaa12cc480 100644 --- a/apps/web/e2e/tests/chat/mobile-entity-reference-composer.spec.ts +++ b/apps/web/e2e/tests/chat/mobile-entity-reference-composer.spec.ts @@ -1,4 +1,4 @@ -import { type Locator, type Page } from "@playwright/test"; +import { type Page } from "@playwright/test"; import { test, expect, type SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; import { SessionPage } from "../../pages/session-page"; @@ -61,10 +61,6 @@ async function openTaskChat(page: Page, taskId: string): Promise { return session; } -function visibleEditor(scope: Locator | Page): Locator { - return scope.locator(".tiptap.ProseMirror:visible").first(); -} - async function expectPersistedReference( apiClient: ApiClient, sessionId: string, @@ -152,7 +148,7 @@ test.describe("Mobile entity reference composer", () => { }); }); - const editor = visibleEditor(session.activeChat()); + const editor = await session.composerReady(); await editor.tap(); await editor.fill(""); await editor.pressSequentially("#Mobile Reference"); diff --git a/apps/web/e2e/tests/chat/mobile-slash-command-composer.spec.ts b/apps/web/e2e/tests/chat/mobile-slash-command-composer.spec.ts index d6f342a36c..b80f5579c9 100644 --- a/apps/web/e2e/tests/chat/mobile-slash-command-composer.spec.ts +++ b/apps/web/e2e/tests/chat/mobile-slash-command-composer.spec.ts @@ -43,10 +43,14 @@ test.describe("Mobile slash command composer", () => { if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id"); const session = await openTaskChat(testPage, task.id); - await seedAvailableCommands(testPage, task.session_id, [SLOW_COMMAND]); - // Multiple TipTap instances can be mounted in mobile layouts; scope to the first visible one. - const editor = testPage.locator(".tiptap.ProseMirror:visible").first(); + // Resolve the active editor through the same startup-aware gate used by + // send helpers; the visible idle placeholder can briefly precede TipTap's + // editable host while the mobile session finishes starting. + const editor = await session.composerReady(); + // composerReady may reload to reconcile stale startup state. Seed the + // client-side command list only after that recovery so it is not discarded. + await seedAvailableCommands(testPage, task.session_id, [SLOW_COMMAND]); await editor.tap(); await editor.fill(""); await editor.pressSequentially("/s"); @@ -62,7 +66,11 @@ test.describe("Mobile slash command composer", () => { await editor.pressSequentially("1s"); await expect(editor).toHaveText(/slow\s+1s/, { timeout: 5_000 }); - await testPage.getByTestId("submit-message-button").tap(); + // The submit button shows a spinner and is `disabled` while the auto-started + // session finishes its brief STARTING transition. Tapping it then is a no-op + // that drops the message, so wait for it to be enabled before tapping. + await expect(session.submitButton()).toBeEnabled(); + await session.submitButton().tap(); await expect(chatList.getByText("/slow 1s", { exact: false })).toBeVisible({ timeout: 10_000, }); diff --git a/apps/web/e2e/tests/chat/share-markdown-flow.ts b/apps/web/e2e/tests/chat/share-markdown-flow.ts index d2e8e2c275..d6b69386d1 100644 --- a/apps/web/e2e/tests/chat/share-markdown-flow.ts +++ b/apps/web/e2e/tests/chat/share-markdown-flow.ts @@ -24,6 +24,7 @@ export async function verifyShareMarkdownPreview( const session = new SessionPage(testPage); await session.waitForLoad(); await session.waitForChatIdle({ timeout: 30_000 }); + await session.composerReady(); await testPage.getByTestId("share-task-button").click(); const dialog = testPage.getByRole("dialog", { name: "Share this task" }); diff --git a/apps/web/e2e/tests/git/git-changes-panel.spec.ts b/apps/web/e2e/tests/git/git-changes-panel.spec.ts index 3604d16e9e..a235808b9f 100644 --- a/apps/web/e2e/tests/git/git-changes-panel.spec.ts +++ b/apps/web/e2e/tests/git/git-changes-panel.spec.ts @@ -520,12 +520,18 @@ test.describe("Git Changes Panel", () => { // Wait for both commits to appear. The commits-section header renders as // soon as the section mounts (no commits required), so asserting on the // text alone races the WS git-status push that carries the actual commit - // list. Gate on the row count first — that's the signal that the FE - // received both commits — and only then check text content. + // list. Gate on the two *named* commit rows we created — that's the signal + // that the FE received them. Don't assert an exact total count: the + // auto-started agent can land its own commit in the same worktree, so the + // list may legitimately hold a third row. await expect(testPage.getByTestId("commits-section")).toBeVisible({ timeout: 15_000 }); await session.expandCommitsSection(); const commitsList = testPage.getByTestId("commits-list"); - await expect(commitsList.locator('[data-testid^="commit-row-"]')).toHaveCount(2, { + const commitRows = commitsList.locator('[data-testid^="commit-row-"]'); + await expect(commitRows.filter({ hasText: "First commit" })).toHaveCount(1, { + timeout: 15_000, + }); + await expect(commitRows.filter({ hasText: "Second commit" })).toHaveCount(1, { timeout: 15_000, }); await expect(session.changes.getByText("First commit")).toBeVisible({ timeout: 5_000 }); @@ -1026,7 +1032,19 @@ test.describe("Git Changes Panel", () => { git.exec(`git reset --soft ${baseSha}`); git.commit("Squashed commit"); - // Wait for the UI to update - old commits should disappear + // The squash happens on the filesystem while the Changes tab is already + // open, so the FE only reflects it once the backend's git-status watcher + // fires a WS push. Under CI shard load that push can lag past the wait + // budget. Reload to force a fresh git-status load — deterministic instead of + // racing the watcher (same approach as the persistent-commits test above). + await testPage.reload(); + await session.waitForLoad(); + await session.clickTab("Changes"); + await expect(session.changes).toBeVisible({ timeout: 10_000 }); + await expect(testPage.getByTestId("commits-section")).toBeVisible({ timeout: 15_000 }); + await session.expandCommitsSection(); + + // Old commits should be gone after the squash. await expect(session.changes.getByText("First commit to squash")).not.toBeVisible({ timeout: 15_000, }); diff --git a/apps/web/e2e/tests/kanban/preview-session-tabs.spec.ts b/apps/web/e2e/tests/kanban/preview-session-tabs.spec.ts index d740ff81a5..b94baa6b93 100644 --- a/apps/web/e2e/tests/kanban/preview-session-tabs.spec.ts +++ b/apps/web/e2e/tests/kanban/preview-session-tabs.spec.ts @@ -1,6 +1,6 @@ import { test, expect } from "../../fixtures/test-base"; import { KanbanPage } from "../../pages/kanban-page"; -import { SessionPage } from "../../pages/session-page"; +import { waitForSessionDone } from "../../helpers/session"; const DONE_STATES = ["COMPLETED", "WAITING_FOR_INPUT"]; @@ -50,42 +50,43 @@ test.describe("Preview session tabs", () => { const { sessions: afterFirst } = await apiClient.listTaskSessions(task.id); const primaryId = afterFirst[0].id; - // 3. Navigate to the full task view and launch a second session via the new-session dialog. - // This mirrors the approach in preview-primary-session.spec.ts since there is no - // dedicated API helper to start a second session on an existing task. - const kanban = new KanbanPage(testPage); - await kanban.goto(); + // 3. Launch a second session through the same WS API path the UI uses. + // This spec is about preview tabs, not dialog mechanics, so it avoids the + // separate new-session-dialog UI surface which has its own dedicated tests. + const launched = await apiClient.launchSession( + { + task_id: task.id, + agent_profile_id: seedData.agentProfileId, + executor_profile_id: seedData.worktreeExecutorProfileId, + workflow_step_id: seedData.startStepId, + prompt: 'e2e:message("secondary-session-response")', + }, + 60_000, + ); - const card = kanban.taskCardByTitle("Preview Tabs Task"); - await expect(card).toBeVisible({ timeout: 10_000 }); - await card.click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); + // 4. Wait for the launched second session to finish. + await waitForSessionDone( + apiClient, + task.id, + launched.session_id, + "Waiting for second session to finish", + 60_000, + ); - const session = new SessionPage(testPage); - await session.waitForLoad(); - await expect(session.chat.getByText("simple mock response", { exact: false })).toBeVisible({ - timeout: 15_000, - }); - - await session.addPanelButton().click(); - await testPage.getByTestId("new-session-button").click(); - const dialog = testPage.getByRole("dialog"); - await expect(dialog).toBeVisible({ timeout: 5_000 }); - // Dialog prompts use the script command form; the agent echoes the argument. - await dialog.locator("textarea").fill('e2e:message("secondary-session-response")'); - await dialog.getByRole("button").filter({ hasText: /Start/ }).click(); - await expect(dialog).not.toBeVisible({ timeout: 10_000 }); - - // 4. Wait for the second session to finish (two sessions in a done state). + // Keep the original preview semantics under test: the first session should + // remain the task's primary/default tab even after another session exists. + // The direct WS launch path can promote the new session, so restore the + // original primary explicitly before opening the kanban preview. + await apiClient.setPrimarySession(primaryId); await expect .poll( async () => { - const { sessions } = await apiClient.listTaskSessions(task.id); - return sessions.filter((s) => DONE_STATES.includes(s.state)).length; + const taskData = await apiClient.getTask(task.id); + return taskData.primary_session_id ?? null; }, - { timeout: 60_000, message: "Waiting for second session to finish" }, + { timeout: 15_000, message: "Waiting for primary session to be restored" }, ) - .toBe(2); + .toBe(primaryId); const { sessions: afterSecond } = await apiClient.listTaskSessions(task.id); const secondaryId = afterSecond.find((s) => s.id !== primaryId)?.id; @@ -95,7 +96,9 @@ test.describe("Preview session tabs", () => { // new-session dialog does not steal the primary flag (verified by // preview-primary-session.spec.ts). - // 5. Enable preview-on-click and return to the kanban board. + const kanban = new KanbanPage(testPage); + + // 5. Enable preview-on-click and open the kanban board. await apiClient.saveUserSettings({ enable_preview_on_click: true }); await kanban.goto(); diff --git a/apps/web/e2e/tests/task/mobile-plan-toolbar-implement.spec.ts b/apps/web/e2e/tests/task/mobile-plan-toolbar-implement.spec.ts index 1f41daa4df..e2a4be5b97 100644 --- a/apps/web/e2e/tests/task/mobile-plan-toolbar-implement.spec.ts +++ b/apps/web/e2e/tests/task/mobile-plan-toolbar-implement.spec.ts @@ -24,6 +24,7 @@ async function seedMobileTaskWithPlan(testPage: Page, apiClient: ApiClient, seed await session.waitForLoad(); await expect.poll(() => apiClient.getTaskPlan(task.id), { timeout: 30_000 }).not.toBeNull(); await session.waitForChatIdle({ timeout: 45_000 }); + await session.composerReady(); await openMobilePlanPanel(testPage, session); return { taskId: task.id, sessionId: task.session_id!, session }; } @@ -35,8 +36,6 @@ async function openMobilePlanPanel(testPage: Page, session: SessionPage) { } test.describe("mobile: Plan toolbar implement", () => { - test.describe.configure({ retries: 1 }); - test("marks the plan implemented and remains disabled after refresh", async ({ testPage, apiClient, @@ -51,6 +50,7 @@ test.describe("mobile: Plan toolbar implement", () => { const toolbarButton = testPage.getByTestId("plan-toolbar-implement-button"); await expect(toolbarButton).toBeVisible({ timeout: 10_000 }); + await expect(toolbarButton).toBeEnabled(); await expect(toolbarButton).toBeInViewport(); const toolbarSpacing = await testPage diff --git a/apps/web/e2e/tests/task/plan-toolbar-implement.spec.ts b/apps/web/e2e/tests/task/plan-toolbar-implement.spec.ts index 311ea51b37..1882ad704b 100644 --- a/apps/web/e2e/tests/task/plan-toolbar-implement.spec.ts +++ b/apps/web/e2e/tests/task/plan-toolbar-implement.spec.ts @@ -28,6 +28,7 @@ async function seedTaskWithPlan( await session.waitForLoad(); await expect.poll(() => apiClient.getTaskPlan(task.id), { timeout: 30_000 }).not.toBeNull(); await session.waitForChatIdle({ timeout: 45_000 }); + await session.composerReady(); await openPlanPanel(session); return { taskId: task.id, sessionId: task.session_id!, session }; } @@ -39,8 +40,6 @@ async function openPlanPanel(session: SessionPage) { } test.describe("Plan toolbar implement", () => { - test.describe.configure({ retries: 1 }); - test("saves unsaved plan text, starts implementation, and stays disabled after refresh", async ({ testPage, apiClient, @@ -56,6 +55,7 @@ test.describe("Plan toolbar implement", () => { const toolbarButton = testPage.getByTestId("plan-toolbar-implement-button"); await expect(toolbarButton).toBeVisible({ timeout: 10_000 }); + await expect(toolbarButton).toBeEnabled(); const toolbarSpacing = await testPage .getByTestId("plan-toolbar-implement-control") diff --git a/apps/web/e2e/tests/task/repository-selector-scroll.spec.ts b/apps/web/e2e/tests/task/repository-selector-scroll.spec.ts index cf3788a2b2..3abf574048 100644 --- a/apps/web/e2e/tests/task/repository-selector-scroll.spec.ts +++ b/apps/web/e2e/tests/task/repository-selector-scroll.spec.ts @@ -21,6 +21,26 @@ async function expectWheelScrollsListInsideDialog(testPage: Page) { .poll(() => list.evaluate((el) => Boolean(el.closest('[data-testid="create-task-dialog"]')))) .toBe(true); + await expect + .poll( + () => + list.evaluate((el) => ({ + clientHeight: el.clientHeight, + scrollHeight: el.scrollHeight, + })), + { timeout: 5_000 }, + ) + .toEqual( + expect.objectContaining({ + clientHeight: expect.any(Number), + scrollHeight: expect.any(Number), + }), + ); + + await expect + .poll(() => list.evaluate((el) => el.scrollHeight > el.clientHeight), { timeout: 5_000 }) + .toBe(true); + const { clientHeight, scrollHeight } = await list.evaluate((el) => ({ clientHeight: el.clientHeight, scrollHeight: el.scrollHeight, diff --git a/apps/web/e2e/tests/task/subtask.spec.ts b/apps/web/e2e/tests/task/subtask.spec.ts index 2d267098f6..78dfb42d2b 100644 --- a/apps/web/e2e/tests/task/subtask.spec.ts +++ b/apps/web/e2e/tests/task/subtask.spec.ts @@ -1,6 +1,7 @@ import { execSync } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; +import type { Locator } from "@playwright/test"; import { test, expect } from "../../fixtures/test-base"; import { makeGitEnv } from "../../helpers/git-helper"; import { useRegularMode } from "../../helpers/regular-mode"; @@ -27,6 +28,17 @@ type SessionInfo = { state: string; }; +async function selectRepositoryFromChip(repoChip: Locator, option: Locator, name: string) { + await expect(async () => { + if (!(await option.isVisible())) { + await repoChip.click(); + } + await expect(option).toBeVisible({ timeout: 3_000 }); + await option.click({ timeout: 3_000 }); + await expect(repoChip).toContainText(name, { timeout: 3_000 }); + }).toPass({ timeout: 15_000 }); +} + test.describe("Subtask basics", () => { test("subtask badge visible on kanban card", async ({ testPage, apiClient, seedData }) => { // Create parent task via API @@ -320,12 +332,15 @@ test.describe("MCP subtask creation", () => { // the combobox option role. const repoChip = testPage.getByTestId("repo-chip-trigger").first(); await expect(repoChip).toBeVisible({ timeout: 5_000 }); - await repoChip.click(); // The combobox option label includes a truncated-path badge alongside the - // repo name, so match by name substring rather than exact. - await testPage.getByRole("option", { name: new RegExp(otherRepoName) }).click(); - // After selection, the chip should now show repo B's name. - await expect(repoChip).toContainText(otherRepoName); + // repo name, so match by name substring rather than exact. Retry opening + // and selecting as one transaction because WS-driven dialog re-renders can + // detach the option between those two actions. + await selectRepositoryFromChip( + repoChip, + testPage.getByRole("option", { name: new RegExp(otherRepoName) }), + otherRepoName, + ); // 5. Submit. Use a unique title so the listTasks poll can find this row. const subtaskTitle = `UI Override Subtask ${Date.now()}`; diff --git a/apps/web/e2e/tests/task/task-create-prompt-autocomplete.spec.ts b/apps/web/e2e/tests/task/task-create-prompt-autocomplete.spec.ts index f7feb4bd35..91ab46f3fc 100644 --- a/apps/web/e2e/tests/task/task-create-prompt-autocomplete.spec.ts +++ b/apps/web/e2e/tests/task/task-create-prompt-autocomplete.spec.ts @@ -81,7 +81,14 @@ test.describe("Task creation: custom prompt autocomplete", () => { await textarea.click(); await textarea.pressSequentially("@e2e-bu"); + // Wait for the menu AND its option to actually populate before pressing + // Enter. The autocomplete popup can open a beat before its options hydrate; + // pressing Enter against an empty/half-open menu lets the key fall through + // to the form submit, which closes the dialog and fails the assertions + // below. Gating on the option (not just the title) is the condition that + // makes the selection deterministic. await expect(testPage.getByText(MENU_TITLE)).toBeVisible(); + await expect(testPage.getByRole("option", { name: new RegExp(PROMPT_NAME) })).toBeVisible(); await textarea.press("Enter"); // Dialog must still be open — Enter selected the menu item, not the form submit. diff --git a/apps/web/src/locales/zh-cn/chat.json b/apps/web/src/locales/zh-cn/chat.json index 423aa779f8..cdd3a17fdb 100644 --- a/apps/web/src/locales/zh-cn/chat.json +++ b/apps/web/src/locales/zh-cn/chat.json @@ -1,4 +1,17 @@ { + "attachmentDeliveryFile": "文件", + "attachmentDeliveryMode": "附件传递方式", + "attachmentDeliveryPathDescription": "上传到工作区,以便代理读取或编辑该文件。", + "attachmentDeliveryPrompt": "提示词", + "attachmentDeliveryPromptDescription": "作为提示词上下文发送以供视觉理解。代理不会获得文件路径。", + "attachmentFallbackName": "附件", + "attachmentMessageAlt": "附件 {{index}}", + "attachmentPreviewAlt": "预览", + "attachmentPreviewFullAlt": "全尺寸预览", + "attachmentUploadFailed": "附件上传失败", + "attachmentUploadPending": "正在上传附件…", + "attachmentUploadPendingSubmit": "请先完成附件上传再发送。", + "retryAttachmentUpload": "重试附件上传", "collapseMessage": "折叠消息", "collapseQueuedMessages": "折叠排队中的消息", "clearAll": "全部清除", diff --git a/docs/plans/owned-link-target-mismatch-repair/plan.md b/docs/plans/owned-link-target-mismatch-repair/plan.md new file mode 100644 index 0000000000..6c00c44945 --- /dev/null +++ b/docs/plans/owned-link-target-mismatch-repair/plan.md @@ -0,0 +1,203 @@ +--- +spec: docs/specs/tasks/attach-workspace-sources.md +created: 2026-08-04 +status: completed +--- + +# Implementation Plan: Owned Link Target Mismatch Repair + +## Overview + +A task's on-disk root under `~/.kandev/tasks/` is named from a title slug plus a **random** +3-character suffix (`SemanticWorktreeName(task.Title, SmallSuffix(3))`) and never includes task +identity. Two distinct tasks whose titles sanitize to the same 20-char slug can therefore resolve to +the same task root and contend over its sibling entries. When one task's Kandev-owned directory-link +entry (e.g. `gsp-aieng-kloud8-helm-dedicated`) already points at a different directory than another +task's durable spec target, `worktree.EnsureOwnedDirectoryLink` compares filesystem identity with +`os.SameFile` and returns `owned link target mismatch`, failing closed with no repair — so the task's +first launch **and every subsequent resume** fail identically (observed on task `61ccfd2c`). + +This fix has two parts, in dependency order: + +1. **Collision resistance (root cause):** derive the task-root suffix deterministically from the task + ID so distinct tasks normally resolve to different task roots, while the ownership marker guards + the residual collision case; the persisted name remains reproducible across launch/resume without + relying on a stored random value. +2. **Self-heal (defense in depth):** inside a Kandev-owned task root, `EnsureOwnedDirectoryLink` + repoints an existing Kandev-owned *directory-link* entry on target mismatch instead of failing + closed forever. A non-link entry still fails closed. This is scoped to the owned task root and does + **not** change the self-referential-entry behavior inside a user's own repository. + +## Confirmed root cause + +- Task directory name: `worktree.SemanticWorktreeName(task.Title, worktree.SmallSuffix(3))` + (`apps/backend/internal/worktree/config.go:452`, suffix `apps/backend/internal/worktree/config.go:364`). + Set at launch in `apps/backend/internal/orchestrator/executor/executor_execute.go:1465` and on + resume in `resolveResumeTaskDirName` (`apps/backend/internal/orchestrator/executor/executor_resume.go:1284`). + The task ID is never incorporated; there is no cross-task uniqueness guarantee. +- The fail-closed mismatch is in `EnsureOwnedDirectoryLink` + (`apps/backend/internal/worktree/directory_link.go:107-109`), reached from + `reconcileWorkspaceRepositories` / `reconcileWorkspaceSources` + (`apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile.go:32,67`), which run on + every local launch and workspace resume (`apps/backend/internal/agent/runtime/lifecycle/manager_launch.go:1076-1083`). +- No code path removes or repoints a Kandev-owned link on mismatch, so the failure is permanent. + +--- + +## Backend + +### Area 1 — Deterministic, task-unique task-root name (Task 01) + +- Add a deterministic suffix derived from the task ID in + `apps/backend/internal/worktree/config.go`: a new exported helper (e.g. + `TaskDirSuffix(taskID string) string`) returning a short lowercase hash (same + `branchSuffixAlphabet` alphabet, 6-8 chars) of the task ID. It must be pure and stable for a given + ID. Keep `SmallSuffix` for existing branch-slug callers. +- Update the two task-root call sites to pass a task-ID-derived suffix instead of `SmallSuffix(3)`: + - `executor_execute.go:1465` → `worktree.SemanticWorktreeName(task.Title, worktree.TaskDirSuffix(task.ID))` + - `resolveResumeTaskDirName` fallback (`executor_resume.go:1288`) → same, so the fallback recomputes + the identical name the initial launch would have used (no drift when the env row was never stamped). +- Do not change `SemanticWorktreeName`'s signature; only what the callers pass as `suffix`. +- Persisted `task_dir_name` reuse on resume is unchanged; the deterministic suffix simply makes the + fallback reproducible and collision-resistant across tasks. + +### Area 2 — Owned-link self-heal on mismatch (Task 02) + +- In `apps/backend/internal/worktree/directory_link.go`, change `EnsureOwnedDirectoryLink` so that when + the existing entry **is** a platform directory link (`isPlatformDirectoryLink`) but `os.SameFile` + reports a different target, it removes the link and recreates it via `CreateOwnedDirectoryLink` + (returning `created=true`), rather than returning `owned link target mismatch`. A non-link entry + still returns the existing `owned link entry already exists` error (never deleted/overwritten). +- Removal is safe here: the entry lives under a Kandev-owned task root (built by `mkdirOwned` through + real, non-symlink ancestors), and a directory link is a pointer, not content. This is distinct from + `IsSelfReferentialDirectoryLink` / `warnSelfReferentialEntry`, which stay report-only because they + concern entries inside the **user's own** repository — leave that path unchanged. +- Keep the doc comment on `EnsureOwnedDirectoryLink` accurate to the new repoint-on-mismatch behavior. + +--- + +## Tests + +- **Deterministic suffix is stable and collision-resistant** — `apps/backend/internal/worktree/config_test.go`: + table test that `TaskDirSuffix(id)` is non-empty, uses only the safe alphabet, is identical across + repeated calls for the same ID, and differs for the covered different IDs. (Task 01) +- **Same-title tasks normally get different roots** — `config_test.go`: two different task IDs with an + identical title produce different `SemanticWorktreeName(...)` results for the covered IDs. (Task 01) +- **Regression: self-heal repoints an owned link on mismatch** — `directory_link_test.go`: seed an + owned link to target A, call `EnsureOwnedDirectoryLink(root, name, B)`, assert no error, + `created=true`, and that the entry now resolves to B. This test must **fail before** the Task 02 + change and pass after. (Task 02) +- **Non-link entry still fails closed** — `directory_link_test.go`: a real directory/file at + `root/name` still returns an error and is not deleted. (Task 02) +- **Update existing behavior test** — `TestEnsureOwnedDirectoryLinkRejectsDifferentTarget` + (`directory_link_test.go:169`) currently asserts fail-closed on a different target; it must be + updated to assert the new repoint-on-mismatch behavior for a Kandev-owned link. (Task 02) + +--- + +## Verification Results + +- Task 01 (`config.go` + executor call sites): `go test ./internal/worktree/... ./internal/orchestrator/executor/...` → all `ok`. New `TestTaskDirSuffix` and `TestSemanticWorktreeNameTaskUnique` failed red (build: `undefined: TaskDirSuffix`) before the helper, pass after. +- Task 02 (`directory_link.go`): `go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/...` → all `ok`. `TestEnsureOwnedDirectoryLinkRepointsOwnedLinkOnMismatch` failed red (`owned link target mismatch: api`) before, passes after; `TestEnsureOwnedDirectoryLinkRejectsNonLinkEntry` passes. +- Changed-file lint: `golangci-lint run ./internal/worktree/... ./internal/orchestrator/executor/... --new-from-rev=` → `0 issues`. +- Task 03 (`ownership-aware repoint`): `go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/... ./internal/backendapp/...` → all `ok`. Added marker-absent, marker-match, and marker-conflict coverage around `EnsureOwnedDirectoryLink`, and threaded the new owner/result contract through lifecycle reconcile and host materialization. +- Task 04 (`rollback-faithful repoint`): `go test ./internal/backendapp/... ./internal/worktree/...` → all `ok`. `TestWorkspaceSourceMaterializer_RestoresRepointedLinkWhenAdoptionFails` proves rollback restores the prior target instead of deleting the pre-existing owned entry. +- Task 05 (`safe atomic replacement`): `go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/... ./internal/backendapp/...` → all `ok`. New traversal-before-repoint, invalid-target-keeps-original-link, and changed-entry rename-guard tests pass. Changed-file lint with base `d6b95500eb3662ecd7fd41f67a5ae4c0ddbfb376`: `golangci-lint run ./internal/worktree/... ./internal/agent/runtime/lifecycle/... ./internal/backendapp/... --new-from-rev= --timeout=5m` → `0 issues`. + +--- + +## PR #2253 review remediation + +An automated review of the landed fix (head `5c2873d76`, before the review-fixup commit) raised four +blockers. Findings 1-3 are genuine gaps the two landed tasks did not fully close; Finding 4 was +already corrected by the fixup and needs no further code change. Tasks 03-05 address them. + +**Finding 1 — legacy-colliding roots reassigned; repoint does no identity check (Task 03).** Resume +reuses the persisted legacy `TaskDirName` +(`apps/backend/internal/orchestrator/executor/executor_resume.go:1285-1286`), so two roots created +under the old random suffix still collide. `EnsureOwnedDirectoryLink` +(`apps/backend/internal/worktree/directory_link.go:104-133`) receives no task identity and never reads +the ownership marker before repointing, so on a shared legacy root the task that reconciles last can +redirect another task's live entry. The marker +(`WriteOwnershipMarker`/`existingMarkerMatches`/`ReadOwnershipMarker`, +`apps/backend/internal/system/storage/workspaces/marker.go:17-62,79-91,120-122`) already fails closed +on a `TaskID`/`TaskDirName` conflict, but it is written only in `prepareTaskWorktreePath` +(`apps/backend/internal/worktree/manager_lifecycle.go:324-329`) and for scratch workspaces +(`manager_launch.go:418-421`) — **not** on the plain local reconcile path — so the repoint helper must +verify it itself. Reconcile runs at `manager_launch.go:1077-1083` and `manager_execution.go:471-477`, +calling `reconcileWorkspaceSources`/`reconcileWorkspaceRepositories` +(`apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile.go:32,67`), both of which +already hold the identity to thread through. + +**Finding 2 — repointing breaks workspace-source rollback (Task 04, most serious).** +`materializeDirectoryLinks` (`apps/backend/internal/backendapp/workspace_source_materializer.go:200-212`) +and `materializeWorktreeSources` (`:569-574`) append an entry to the `created` slice whenever +`wasCreated==true`. Task 02 now returns `created=true` for a **repointed** pre-existing link, and +`rollbackHostWorkspaceMaterialization` (`:229-244`) `os.Remove`s every `created` entry (`:231-233`) on +a later failure (rescan/persist/adopt at `:134,:139,:145`). So a failed submission **deletes** a link +that existed before the attachment instead of restoring its prior target — violating the spec's +atomicity guarantee (`docs/specs/tasks/attach-workspace-sources.md:40-43`,`:51`,`:62`). The two +reconcile call sites ignore the return values, so the rollback bug is confined to the two +`materialize*` sites. + +**Finding 3 — remove-and-recreate not atomic; containment validated after the destructive remove +(Task 05).** In the repoint branch, `removeInspectedDirectoryLink` (`directory_link.go:122`) runs +**before** `CreateOwnedDirectoryLink`'s containment check (`isOwnedDirectoryLinkPath`, +`directory_link.go:15`, reached at `:125`), so a traversal/out-of-root name is rejected only after the +remove. A create failure after the remove leaves the entry missing with no restore. There is no atomic +temp+rename wrapper for directory links (see the file pattern at +`apps/backend/internal/agent/usage/fileutil.go:11-33`); Unix `os.Symlink` is a single atomic syscall, +but a Windows junction cannot be atomically renamed over an existing target +(`directory_link_unix.go`, `directory_link_windows.go`). + +**Finding 4 — suffix uniqueness (reviewed, no code change).** The review read the pre-fixup comments. +`config.go:382-407` already states the 8-char base-36 projection of `sha256(taskID)` is +collision-resistant (~36^8 ≈ 2.8e12), not injective, and that residual owned-root contention is caught +by the fail-closed ownership marker; `config_test.go` already asserts the exact suffix length. The +suffix stays intentionally probabilistic; Task 03 is what makes that marker backstop effective on the +reconcile path. No change required. + +### Single-signature-change decision + +Tasks 03-05 all touch `EnsureOwnedDirectoryLink`. Rather than three overlapping signature edits, Task +03 changes it **once**: it takes an ownership descriptor (`TaskID`, `TaskDirName`) and returns a result +struct (`{Path string; Created bool; PriorTarget string}`). Task 04 consumes `PriorTarget` for +rollback-faithful undo; Task 05 adds the containment-before-remove ordering and the safe/atomic +replacement inside that same function. The four call sites are updated in Task 03; the two reconcile +sites keep ignoring the extra result fields. + +### Tests (Tasks 03-05) + +- **Marker-mismatch repoint fails closed** — `directory_link_test.go`: seed an owned link under a root + whose `.kandev-workspace.json` names task A; `EnsureOwnedDirectoryLink` for task B fails closed with + a marker-conflict error and leaves the entry unchanged. Marker absent and marker-matches-this-task + both allow the repoint. (Task 03) +- **Rollback restores a repointed link's prior target** — `workspace_source_materializer` test: repoint + a pre-existing entry (A→B), force a later rescan/persist failure, assert the entry is restored to A + (not deleted); a genuinely created entry is still deleted. (Task 04) +- **Containment before remove + recreate-failure restore** — `directory_link_test.go`: a traversal / + out-of-root name is rejected before any removal; a forced recreate failure leaves the original link + intact; the existing swap-race guard still holds. (Task 05) + +## Implementation Waves And Parallel Candidates + +Tasks 01-02 landed. Tasks 03-05 all touch `EnsureOwnedDirectoryLink` and its callers and must run +sequentially in dependency order (03 defines the shared signature; 04 and 05 build on it). The default +is sequential execution in the primary conversation; waves do not authorize subagents. + +```text +Wave 1: +- [x] [task-01-task-unique-root-name](task-01-task-unique-root-name.md) + +Wave 2: +- [x] [task-02-owned-link-self-heal](task-02-owned-link-self-heal.md) + +Wave 3: +- [x] [task-03-ownership-aware-repoint](task-03-ownership-aware-repoint.md) + +Wave 4: +- [x] [task-04-rollback-faithful-repoint](task-04-rollback-faithful-repoint.md) + +Wave 5: +- [x] [task-05-safe-atomic-link-replacement](task-05-safe-atomic-link-replacement.md) +``` diff --git a/docs/plans/owned-link-target-mismatch-repair/task-01-task-unique-root-name.md b/docs/plans/owned-link-target-mismatch-repair/task-01-task-unique-root-name.md new file mode 100644 index 0000000000..fa4959f0b5 --- /dev/null +++ b/docs/plans/owned-link-target-mismatch-repair/task-01-task-unique-root-name.md @@ -0,0 +1,85 @@ +--- +id: "01-task-unique-root-name" +title: "Task-unique task-root directory name" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/tasks/attach-workspace-sources.md" +parallelism: sequential +--- + +# Task 01: Task-unique task-root directory name + +Make the `~/.kandev/tasks/` root name derive from the task ID so distinct tasks — even with +identical titles — normally resolve to different task roots, while the ownership marker rejects any +residual collision, and so the resume fallback recomputes the same name the initial launch would have +produced. + +## Acceptance + +- A new pure helper in `apps/backend/internal/worktree/config.go` produces a short, filesystem-safe, + lowercase, collision-resistant suffix from a task ID that is stable across repeated calls for the + same ID and normally differs for different IDs. +- The two task-root call sites build the task directory name from the task ID rather than + `SmallSuffix(3)`, so two tasks with the same title normally produce different task-root names and a + residual collision is blocked by the ownership marker. +- `SmallSuffix` and its existing branch-slug callers are unchanged; `SemanticWorktreeName`'s signature + is unchanged. + +## Verification + +```bash +cd apps/backend +go test ./internal/worktree/... ./internal/orchestrator/executor/... +golangci-lint run ./internal/worktree/... ./internal/orchestrator/executor/... --timeout=5m +``` + +## Files likely touched + +- `apps/backend/internal/worktree/config.go` (add `TaskDirSuffix(taskID string) string`) +- `apps/backend/internal/worktree/config_test.go` (suffix determinism + same-title-different-root tests) +- `apps/backend/internal/orchestrator/executor/executor_execute.go` (~line 1465) +- `apps/backend/internal/orchestrator/executor/executor_resume.go` (`resolveResumeTaskDirName`, ~line 1288) + +## Dependencies + +None. + +## Parallelism + +`sequential`. + +## Inputs + +- Spec: Persistence guarantees paragraph on per-task task-root uniqueness, and the same-title-tasks + scenario. +- Plan: Backend Area 1. +- Existing `SemanticWorktreeName` (`config.go:452`) and `SmallSuffix` (`config.go:364`). +- Root cause: task ID is never incorporated; random 3-char suffix has no cross-task uniqueness. + +## Output contract + +Summary, files changed, tests run, blockers, risks, and task/plan status updates in the same +conversation. Reconcile **Files likely touched** with the actual diff before marking done. + +## Results + +- Added `TaskDirSuffix(taskID string) string` to `apps/backend/internal/worktree/config.go`: a + deterministic, collision-resistant 8-char lowercase suffix over `branchSuffixAlphabet`, derived + from `sha256(taskID)`. Empty ID returns empty. `SmallSuffix` and `SemanticWorktreeName`'s signature + are unchanged. +- Updated both task-root call sites to pass `worktree.TaskDirSuffix(task.ID)` instead of + `worktree.SmallSuffix(3)`: `executor_execute.go:1465` and `resolveResumeTaskDirName` + (`executor_resume.go:1288`). +- Added `TestTaskDirSuffix` (stable, lowercase-alphanumeric, representative different IDs, empty-ID) + and `TestSemanticWorktreeNameTaskUnique` (same title + different task IDs → different roots for the + covered IDs) to + `config_test.go`. Both failed red before the helper existed (build error: `undefined: TaskDirSuffix`) + and pass after. +- Commands: + - `go test ./internal/worktree/ -run 'TestTaskDirSuffix|TestSemanticWorktreeNameTaskUnique'` → build + error red before helper; `ok` after. + - `go test ./internal/worktree/... ./internal/orchestrator/executor/...` → all `ok`. +- External side-effect boundaries: None. Pure helper plus two in-process call-site edits; no + filesystem, network, or DB writes introduced. diff --git a/docs/plans/owned-link-target-mismatch-repair/task-02-owned-link-self-heal.md b/docs/plans/owned-link-target-mismatch-repair/task-02-owned-link-self-heal.md new file mode 100644 index 0000000000..688d05cda7 --- /dev/null +++ b/docs/plans/owned-link-target-mismatch-repair/task-02-owned-link-self-heal.md @@ -0,0 +1,108 @@ +--- +id: "02-owned-link-self-heal" +title: "Self-heal owned directory-link on target mismatch" +status: done +wave: 2 +depends_on: ["01-task-unique-root-name"] +plan: "plan.md" +spec: "../../specs/tasks/attach-workspace-sources.md" +parallelism: sequential +--- + +# Task 02: Self-heal owned directory-link on target mismatch + +Inside a Kandev-owned task root, an existing Kandev-owned directory-link whose target differs from the +current durable spec must be repointed, not treated as a permanent failure. This is defense in depth +so a task that already wedged on a stale entry self-heals on the next launch/resume. + +## Acceptance + +- `EnsureOwnedDirectoryLink` (`apps/backend/internal/worktree/directory_link.go`) repoints an existing + entry when it **is** a platform directory link but `os.SameFile(actual, expected)` is false: it + removes the link and recreates it to the current target, returning `created=true` with no error. +- A non-link entry (real file/directory) at the same path still returns an error and is neither + deleted nor overwritten. +- The self-referential-entry path (`IsSelfReferentialDirectoryLink` / `warnSelfReferentialEntry`) is + unchanged — those concern entries inside a user's own repository and stay report-only. +- The `EnsureOwnedDirectoryLink` doc comment reflects the new repoint-on-mismatch behavior. + +## Verification + +```bash +cd apps/backend +go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/... +golangci-lint run ./internal/worktree/... --timeout=5m +``` + +## Files likely touched + +- `apps/backend/internal/worktree/directory_link.go` (`EnsureOwnedDirectoryLink` mismatch branch) +- `apps/backend/internal/worktree/directory_link_test.go`: + - new: seed owned link to A, `Ensure(...B)` returns `created=true`, entry now resolves to B + (must fail before this change, pass after) + - new: non-link entry still fails closed and is not removed + - update: `TestEnsureOwnedDirectoryLinkRejectsDifferentTarget` (line 169) to assert the new + repoint behavior for a Kandev-owned link + +## Dependencies + +Task 01 (root-cause uniqueness) lands first; this task is defense in depth. + +## Parallelism + +`sequential`. + +## Inputs + +- Spec: Failure-modes row for a stale owned entry, and the owned-link self-heal + non-link scenarios. +- Plan: Backend Area 2. +- Existing `EnsureOwnedDirectoryLink` (`directory_link.go:92-117`), `CreateOwnedDirectoryLink`, + `isPlatformDirectoryLink`, and the report-only `IsSelfReferentialDirectoryLink` contract. + +## Output contract + +Summary, files changed, tests run (including the red-before/green-after regression), blockers, risks, +and task/plan status updates in the same conversation. Reconcile **Files likely touched** with the +actual diff before marking done. + +## Results + +- `EnsureOwnedDirectoryLink` (`apps/backend/internal/worktree/directory_link.go`) now repoints an + existing entry when it **is** a platform directory link but `os.SameFile(actual, expected)` is + false: it `os.Remove`s the link and recreates it via `CreateOwnedDirectoryLink`, returning + `created=true`. A non-link entry still returns `owned link entry already exists` and is never + removed. The doc comment was rewritten to describe repoint-on-mismatch and to scope it to the + Kandev-owned task root, explicitly contrasting with the report-only self-referential path. +- The self-referential path (`IsSelfReferentialDirectoryLink` / callers) is unchanged. +- Tests in `apps/backend/internal/worktree/directory_link_test.go`: + - Replaced `TestEnsureOwnedDirectoryLinkRejectsDifferentTarget` with + `TestEnsureOwnedDirectoryLinkRepointsOwnedLinkOnMismatch` (seed owned link → A, `Ensure(..., B)` + returns `created=true`, entry now `SameFile` as B and reads B's content). Red before the change + (`owned link target mismatch: api`), green after. + - Added `TestEnsureOwnedDirectoryLinkRejectsNonLinkEntry` (real dir at `root/name` still errors and + its file is untouched). Passed before and after (path unchanged). +- Commands: + - `go test ./internal/worktree/ -run 'TestEnsureOwnedDirectoryLinkRepointsOwnedLinkOnMismatch|TestEnsureOwnedDirectoryLinkRejectsNonLinkEntry' -v` + → repoint red before, both `PASS` after. + - `go test ./internal/worktree/ -run 'TestEnsureOwnedDirectoryLink|TestCreateOwnedDirectoryLink|TestIsSelfReferential' -v` + → all `PASS`. + - `go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/...` → all `ok`. + - `golangci-lint run ./internal/worktree/... ./internal/orchestrator/executor/... --new-from-rev=` + → `0 issues`. +- External side-effect boundaries: the repoint removes and recreates a directory link (a pointer, not + content) strictly under a Kandev-owned task root; no user-repository entries are touched. + +### PR #2253 fixup (automated-review remediation) + +- Serialized both `CreateOwnedDirectoryLink` and `EnsureOwnedDirectoryLink` with the shared + `acquireWorktreeTargetPath` lock keyed by the owned-link path, so every Kandev writer for the same + entry now shares one exclusive inspect/create/repoint critical section. +- Under that lock, Unix uses a sibling temp link plus rename-over replacement; Windows still removes and + recreates the link, but no competing Kandev writer can now swap a new entry into the same path during + the repair window. +- Extended `TestEnsureOwnedDirectoryLinkRejectsNonLinkEntry` with a regular-file collision case: a file + at the entry name still fails closed and its bytes survive untouched. +- Follow-on work in Task 03 then threaded task identity into `EnsureOwnedDirectoryLink`, so repoints now + also fail closed when the workspace ownership marker belongs to a different task root. +- Re-ran `go test ./internal/worktree/... ./internal/orchestrator/executor/... ./internal/agent/runtime/lifecycle/...` + → all `ok`; changed-file `golangci-lint --new-from-rev=` → `0 issues`. diff --git a/docs/plans/owned-link-target-mismatch-repair/task-03-ownership-aware-repoint.md b/docs/plans/owned-link-target-mismatch-repair/task-03-ownership-aware-repoint.md new file mode 100644 index 0000000000..c0395005a8 --- /dev/null +++ b/docs/plans/owned-link-target-mismatch-repair/task-03-ownership-aware-repoint.md @@ -0,0 +1,113 @@ +--- +id: "03-ownership-aware-repoint" +title: "Ownership-aware owned-link repoint" +status: done +wave: 3 +depends_on: ["02-owned-link-self-heal"] +plan: "plan.md" +spec: "../../specs/tasks/attach-workspace-sources.md" +parallelism: sequential +--- + +# Task 03: Ownership-aware owned-link repoint + +Task 02's repoint receives no task identity and never reads the ownership marker before removing and +recreating a Kandev-owned link, so on a shared legacy task root the task that reconciles last can +repoint another task's live workspace entry to its own repository. Thread the current task identity +into the reconcile and materialize paths and only allow a repoint when the owned root's +`.kandev-workspace.json` marker names **this** task; otherwise fail closed and leave the other task's +entry intact. + +This is the **single-signature-change** task. Design the new `EnsureOwnedDirectoryLink` contract once +here — an ownership descriptor input plus a result struct — and have Task 04 and Task 05 build on it. + +## Acceptance + +- `EnsureOwnedDirectoryLink` (`apps/backend/internal/worktree/directory_link.go:92-125`) takes an + ownership descriptor (`TaskID`, `TaskDirName`) and returns a result struct + (`{Path string; Created bool; PriorTarget string}`) instead of the current `(entry, created)` pair. +- A repoint proceeds only when `ReadOwnershipMarker(root)` + (`apps/backend/internal/system/storage/workspaces/marker.go:120-122`) is **absent**, or is present + and its `TaskID` **and** `TaskDirName` match the descriptor. A present marker naming a different + task fails closed with a marker-conflict error and neither removes nor repoints the entry. This + mirrors `existingMarkerMatches` fail-closed-on-mismatch, allow-on-absent semantics + (`marker.go:79-91`). +- `reconcileWorkspaceSources` / `reconcileWorkspaceRepositories` + (`apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile.go:16,40`) thread the + identity they already hold (`workspacePath` = root; the launch/resume req carries `TaskID`, + `WorkspaceID`, `TaskDirName`) into the `EnsureOwnedDirectoryLink` calls at + `workspace_sources_reconcile.go:32,67`. +- The two `materialize*` call sites (`materializeDirectoryLinks` + `apps/backend/internal/backendapp/workspace_source_materializer.go:200-212` and + `materializeWorktreeSources` `:569-574`) pass the identity already available to + `materializeHostRuntime` (it receives `taskID`). +- All four call sites compile against the new signature; the two reconcile sites continue to ignore + the `Created`/`PriorTarget` result fields (Task 04 consumes them). + +## Verification + +```bash +cd apps/backend +go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/... ./internal/backendapp/... +golangci-lint run ./internal/worktree/... ./internal/agent/runtime/lifecycle/... ./internal/backendapp/... --timeout=5m +``` + +## Files likely touched + +- `apps/backend/internal/worktree/directory_link.go` (ownership descriptor param + result struct; + read/verify marker before the repoint branch) +- `apps/backend/internal/worktree/directory_link_test.go`: + - new: marker absent → repoint allowed (`created=true`) + - new: marker present and names this task → repoint allowed + - new: marker present and names a **different** task → fails closed with a marker-conflict error, + entry unchanged (must fail before this change if the guard is missing) +- `apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile.go` (thread identity into + both calls at `:32,:67`) +- `apps/backend/internal/agent/runtime/lifecycle/manager_launch.go` (~`:1077-1083`) and + `manager_execution.go` (~`:471-477`) if the reconcile signatures must widen to carry identity +- `apps/backend/internal/backendapp/workspace_source_materializer.go` (pass identity at the two + `materialize*` call sites) + +## Dependencies + +Task 02 (owned-link self-heal) lands first; this task makes that repoint ownership-aware. + +## Parallelism + +`sequential`. It defines the shared `EnsureOwnedDirectoryLink` signature that Tasks 04 and 05 consume. + +## Inputs + +- Spec: the failure-mode row and scenario for an ownership-marker-mismatch repoint that fails closed + and leaves the other task's entry intact. +- Plan: PR #2253 review remediation — Finding 1 and the single-signature-change decision. +- `WriteOwnershipMarker` / `existingMarkerMatches` / `ReadOwnershipMarker` + (`apps/backend/internal/system/storage/workspaces/marker.go:17-62,79-91,120-122`) and + `OwnershipMarker` (`.../workspaces/types.go:23-28`). Marker is written only in + `prepareTaskWorktreePath` (`apps/backend/internal/worktree/manager_lifecycle.go:324-329`) and for + scratch workspaces (`manager_launch.go:418-421`); a plain local-executor reconcile root may lack + one, which is why absent → allowed. + +## Output contract + +Summary, files changed, tests run (including the marker-mismatch fail-closed regression), blockers, +risks, and task/plan status updates in the same conversation. Reconcile **Files likely touched** with +the actual diff before marking done. + +## Results + +- Added `OwnedDirectoryLinkOwner` and `OwnedDirectoryLinkResult` to `apps/backend/internal/worktree/directory_link.go`, and changed `EnsureOwnedDirectoryLink` to take the owner descriptor and return the result struct in one signature update. +- The repoint branch now reads `.kandev-workspace.json` via `ReadOwnershipMarker(root)` and allows repoint only when the marker is absent or matches `TaskID` plus `TaskDirName`; a mismatched marker fails closed with `workspace ownership marker conflicts with requested task root` and leaves the existing entry untouched. +- Threaded the new contract through all four callers: + - `apps/backend/internal/agent/runtime/lifecycle/workspace_sources_reconcile.go` + - `apps/backend/internal/agent/runtime/lifecycle/manager_launch.go` + - `apps/backend/internal/agent/runtime/lifecycle/manager_execution.go` + - `apps/backend/internal/backendapp/workspace_source_materializer.go` +- Tests: + - `TestEnsureOwnedDirectoryLinkRepointsOwnedLinkOnMismatch` now covers the marker-absent allow path. + - Added `TestEnsureOwnedDirectoryLinkRepointsOwnedLinkWithMatchingMarker`. + - Added `TestEnsureOwnedDirectoryLinkRejectsMarkerConflictOnMismatch`. + - Updated lifecycle reconcile tests to compile against the new owner-aware signatures. +- Commands: + - `cd apps/backend && go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/... ./internal/backendapp/...` → all `ok`. +- External side-effect boundary: marker reads plus owned-link repoint/create operations strictly under the Kandev-owned task root; no user-owned repository entries are deleted or overwritten. diff --git a/docs/plans/owned-link-target-mismatch-repair/task-04-rollback-faithful-repoint.md b/docs/plans/owned-link-target-mismatch-repair/task-04-rollback-faithful-repoint.md new file mode 100644 index 0000000000..fce4b29702 --- /dev/null +++ b/docs/plans/owned-link-target-mismatch-repair/task-04-rollback-faithful-repoint.md @@ -0,0 +1,90 @@ +--- +id: "04-rollback-faithful-repoint" +title: "Rollback-faithful owned-link repoint" +status: done +wave: 4 +depends_on: ["03-ownership-aware-repoint"] +plan: "plan.md" +spec: "../../specs/tasks/attach-workspace-sources.md" +parallelism: sequential +--- + +# Task 04: Rollback-faithful owned-link repoint + +Since Task 02, `EnsureOwnedDirectoryLink` returns `created=true` for a **repointed** pre-existing link, +not only a brand-new one. `materializeDirectoryLinks` and `materializeWorktreeSources` append such +entries to the `created []string` slice, and `rollbackHostWorkspaceMaterialization` does +`os.Remove(created[index])` on each. So a later failed multi-source attachment **deletes** a link that +existed before this attachment instead of restoring its prior target — breaking the spec's atomicity +guarantee. Make the materialization undo contract distinguish a repoint from a create and carry the +prior target so rollback restores it. + +## Acceptance + +- A per-entry undo record type (e.g. `{Path string; PriorTarget string}`) replaces the bare + `created []string`: `PriorTarget == ""` ⇒ genuinely created-new ⇒ delete on rollback; non-empty ⇒ + repointed ⇒ restore the prior target on rollback. +- `materializeDirectoryLinks` + (`apps/backend/internal/backendapp/workspace_source_materializer.go:200-212`, record at `:207-208`) + and `materializeWorktreeSources` (`:569-574`) populate the undo record from the Task 03 result + struct (`Created`, `PriorTarget`). +- `rollbackHostWorkspaceMaterialization` (`:229-244`, current `os.Remove` at `:231-233`) restores each + repointed entry to its `PriorTarget` (remove + recreate to the prior target) and deletes only + genuinely created-new entries. +- The `hostWorkspaceMaterialization` struct (`:66-73`, constructed at `:171-174`) carries the undo + records in place of `created []string`. +- The two reconcile call sites (`workspace_sources_reconcile.go:32,67`) still ignore the result and + are not part of the rollback slice — this bug is confined to the two `materialize*` sites. + +## Verification + +```bash +cd apps/backend +go test ./internal/backendapp/... ./internal/worktree/... +golangci-lint run ./internal/backendapp/... --timeout=5m +``` + +## Files likely touched + +- `apps/backend/internal/backendapp/workspace_source_materializer.go` (undo-record type; + `hostWorkspaceMaterialization` field; `materializeDirectoryLinks`, `materializeWorktreeSources`, + `rollbackHostWorkspaceMaterialization`) +- `apps/backend/internal/backendapp/workspace_source_materializer_test.go`: + - new/updated: repoint a pre-existing link, force a later failure at one of the rollback trigger + points (`:134`/`:139`/`:145`; deferred rollback `:124-131`), assert the **original** target is + restored (not deleted) and a genuinely created-new entry is still deleted + +## Dependencies + +Task 03 (defines the `EnsureOwnedDirectoryLink` result struct with `PriorTarget`). + +## Parallelism + +`sequential`. Consumes the Task 03 signature. + +## Inputs + +- Spec: strengthened atomicity behavior (`docs/specs/tasks/attach-workspace-sources.md:40-43`) and the + failure-mode row for restoring a repointed pre-existing link on a failed submission. Cancel/intact + guarantees at `:51` and `:62`. +- Plan: PR #2253 review remediation — Finding 2 (confirmed, most serious). +- Trigger points: rescan/persist/adopt at `workspace_source_materializer.go:134,139,145`; deferred + rollback at `:124-131`. + +## Output contract + +Summary, files changed, tests run (including the restore-not-delete regression), blockers, risks, and +task/plan status updates in the same conversation. Reconcile **Files likely touched** with the actual +diff before marking done. + +## Results + +- Replaced the bare `created []string` rollback tracking with `ownedDirectoryLinkUndo{Path, PriorTarget}` records carried on `hostWorkspaceMaterialization.linkUndo`. +- `materializeDirectoryLinks`, `materializeWorktreeSources`, and `materializeHostRuntime` now preserve `PriorTarget` from the Task 03 result struct, so rollback can distinguish a brand-new link from a repointed pre-existing one. +- `rollbackHostWorkspaceMaterialization` now restores repointed entries to their prior target and deletes only genuinely new links, processing the undo records in reverse order. +- Added `TestWorkspaceSourceMaterializer_RestoresRepointedLinkWhenAdoptionFails`, which seeds a pre-existing owned link, forces adoption failure, and proves the original target is restored rather than deleted. +- PR #2253 fixup: `rollbackOwnedDirectoryLink` now delegates pre-existing entries to `worktree.RestoreOwnedDirectoryLink`, so rollback uses the same platform-safe replacement path as owned-link repair instead of doing a raw remove-then-create. +- Added `TestRestoreOwnedDirectoryLinkKeepsCurrentLinkWhenReplacementTargetIsInvalid`, which proves a failed restore request leaves the current owned link intact. +- Commands: + - `cd apps/backend && go test ./internal/backendapp/... ./internal/worktree/...` → all `ok`. +- External side-effect boundary: rollback removes or recreates Kandev-owned directory links only under the task root, and restores a prior target only for entries this materialization repointed. diff --git a/docs/plans/owned-link-target-mismatch-repair/task-05-safe-atomic-link-replacement.md b/docs/plans/owned-link-target-mismatch-repair/task-05-safe-atomic-link-replacement.md new file mode 100644 index 0000000000..fb0bf05761 --- /dev/null +++ b/docs/plans/owned-link-target-mismatch-repair/task-05-safe-atomic-link-replacement.md @@ -0,0 +1,96 @@ +--- +id: "05-safe-atomic-link-replacement" +title: "Safe, atomic owned-link replacement" +status: done +wave: 5 +depends_on: ["04-rollback-faithful-repoint"] +plan: "plan.md" +spec: "../../specs/tasks/attach-workspace-sources.md" +parallelism: sequential +--- + +# Task 05: Safe, atomic owned-link replacement + +The Task 02 repoint runs the destructive `removeInspectedDirectoryLink` (`directory_link.go:122`) +**before** `CreateOwnedDirectoryLink` validates root/name containment (`directory_link.go:125`, check +at `:15`). So a traversal or out-of-root name is only rejected after the remove, and a +create-fails-after-remove leaves the entry missing with no restore of the prior target. Validate +containment before any removal and make the replacement safe against create-fails-after-remove. + +## Acceptance + +- Containment is validated with `isOwnedDirectoryLinkPath` / `ownedDirectoryLinkPath` + (`directory_link.go:45-67`) **before** `removeInspectedDirectoryLink` runs. A traversal name + (e.g. `../x`) is rejected before any removal. +- **Unix**: the new link is created at a temp sibling name and `os.Rename`d over the target — a single + atomic swap that also closes the check→remove→create window. The temp+rename helper lives in + `directory_link_unix.go` beside the existing `os.Symlink` path. +- **Windows**: because a junction cannot be atomically renamed over an existing target, keep + remove-then-create but restore the prior link (best-effort recreate to `PriorTarget` from the Task + 03/04 result) if the create fails, so the entry is never left missing when the prior target is + known. This lives in `directory_link_windows.go` beside `createPlatformDirectoryLink` + (`os.Mkdir` + `CreateFile` + `DeviceIoControl FSCTL_SET_REPARSE_POINT`). +- `CreateOwnedDirectoryLink`'s canonicalize / `mkdirOwned` / `verifyCreatedOwnedDirectoryLink` path + is unchanged for the genuine create-new case. + +## Verification + +```bash +cd apps/backend +go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/... +golangci-lint run ./internal/worktree/... --timeout=5m +``` + +## Files likely touched + +- `apps/backend/internal/worktree/directory_link.go` (reorder containment ahead of the destructive + remove; call into the platform atomic-replace helper) +- `apps/backend/internal/worktree/directory_link_unix.go` (temp-sibling + `os.Rename` helper) +- `apps/backend/internal/worktree/directory_link_windows.go` (remove-then-create with restore-on- + failure to the prior target) +- `apps/backend/internal/worktree/directory_link_test.go`: + - traversal name (`../x`) rejected before any removal + - recreate-failure leaves the **original** link intact (restored) + - the deterministic swap-race (already partly covered by `removeInspectedDirectoryLink`) still fails + closed + +## Dependencies + +Task 04 (rollback contract); the Windows restore path reuses the `PriorTarget` from the shared result +struct introduced in Task 03. + +## Parallelism + +`sequential`. Consumes the Task 03 signature and the Task 04 undo semantics. + +## Inputs + +- Spec: the safe-replacement failure-mode row and scenario — never deletes a non-owned/changed entry + and never leaves the entry missing on a failed recreate. +- Plan: PR #2253 review remediation — Finding 3. +- Platform helpers `isPlatformDirectoryLink`, `createPlatformDirectoryLink`, + `requirePlatformDirectoryLink` in `directory_link_unix.go` / `directory_link_windows.go`; the atomic + temp+rename pattern reference at `apps/backend/internal/agent/usage/fileutil.go:11-33`. + +## Output contract + +Summary, files changed, tests run (traversal-before-remove, recreate-failure-restores, swap-race), +blockers, risks, and task/plan status updates in the same conversation. Reconcile **Files likely +touched** with the actual diff before marking done. + +## Results + +- `EnsureOwnedDirectoryLink` now resolves the owned entry path through `ownedDirectoryLinkPath(root, name)` before any filesystem mutation, so traversal and out-of-root names fail before a repoint can touch an existing entry. +- The repoint path now canonicalizes the replacement target before acting, then delegates to platform-specific replacement helpers: + - Unix: `replacePlatformDirectoryLink` stages a sibling temp link and renames it over the target path. + - Windows: `replacePlatformDirectoryLink` keeps remove-then-create, but restores the prior target if create fails after removal. +- Added the shared `revalidateInspectedDirectoryLink` / `renameInspectedDirectoryLink` guards so the changed-entry race still fails closed before a rename-over replacement. +- Tests: + - Added `TestEnsureOwnedDirectoryLinkRejectsTraversalBeforeRepoint`. + - Added `TestEnsureOwnedDirectoryLinkKeepsOriginalLinkWhenReplacementTargetIsInvalid`. + - Added `TestRenameInspectedDirectoryLinkRejectsChangedEntry`. +- Commands: + - `cd apps/backend && gofmt -w internal/worktree/directory_link.go internal/worktree/directory_link_unix.go internal/worktree/directory_link_windows.go internal/worktree/directory_link_test.go` + - `cd apps/backend && go test ./internal/worktree/... ./internal/agent/runtime/lifecycle/... ./internal/backendapp/...` → all `ok`. + - `cd apps/backend && base=$(git merge-base HEAD origin/main) && golangci-lint run ./internal/worktree/... ./internal/agent/runtime/lifecycle/... ./internal/backendapp/... --new-from-rev="$base" --timeout=5m` → `0 issues`. +- External side-effect boundary: on Unix the repoint stages a temp sibling link and renames it over the owned entry; on Windows it may remove and recreate the owned entry but restores the prior target on failure. In both cases the mutation stays confined to Kandev-owned task-root entries. diff --git a/docs/specs/tasks/attach-workspace-sources.md b/docs/specs/tasks/attach-workspace-sources.md index 1cfd9d3214..75d580e860 100644 --- a/docs/specs/tasks/attach-workspace-sources.md +++ b/docs/specs/tasks/attach-workspace-sources.md @@ -38,7 +38,9 @@ manually moving files into the task workspace. - A source file rename may stay within its canonical workspace/source root; a cross-root move or rename is rejected before either source is mutated. - A multi-source submission is atomic from the user's perspective: either every source is attached - and materialized in the current task environment, or none of the new attachments remain. + and materialized in the current task environment, or none of the new attachments remain. When an + attachment repointed a pre-existing Kandev-owned entry, a failed submission restores that entry to + the target it had before the attachment rather than deleting it. - Repository attachment works for every executor that can run the task. Arbitrary folders are available only to Local and Worktree tasks, where the selected host paths remain live. Container and remote pickers do not offer the folder source kind, and the backend rejects a forged folder @@ -192,6 +194,10 @@ persisted in source URLs or copied into agent-visible metadata. | A requested file move or rename crosses canonical source roots | The request is rejected before either source is mutated. | | A persisted local folder later disappears | The current live environment keeps its existing materialization; a new/reset environment surfaces the missing source and does not silently omit it. | | The client disconnects during materialization | Rollback runs on a detached bounded context and the eventual task event reflects durable state. | +| A Kandev-owned task-root entry already points at a different directory than the current durable spec (e.g. a stale entry left by an earlier launch) | Because the task root is Kandev-owned and the entry is a directory link, reconciliation repoints it to the current spec target and the launch/resume proceeds; it does not fail closed forever. | +| A Kandev-owned task-root entry's ownership marker names a different task than the one reconciling | Reconciliation does not repoint the entry; it fails closed with a marker-conflict error and leaves the other task's entry pointing at its existing target. | +| A multi-source attachment that repointed a pre-existing owned entry later fails during materialization | Rollback restores each repointed entry to the target it had before the attachment and deletes only entries this attachment created; no pre-existing entry is left deleted. | +| A safe replacement would overwrite a non-owned or out-of-root entry, or its recreate fails after removal | A non-owned or changed entry, or a traversal path, is rejected before any removal, and a failed recreate restores the entry to its prior target rather than leaving it missing. | ## Persistence guarantees @@ -201,6 +207,14 @@ checkouts from durable repository attachments; they never persist folder attachm conversations and source records survive an environment restart even when runtime materialization must be retried. +Each task that materializes a Kandev-owned task root under the tasks base directory derives its +task-root directory name from task identity. The name is collision-resistant, not injective: two +distinct tasks — including two tasks whose titles sanitize to the same slug, or a local task and a +worktree task sharing a title — are overwhelmingly unlikely to resolve to the same task root, and any +residual collision is caught by the fail-closed ownership marker, which verifies task identity before +any Kandev-owned entry under a shared root is repointed. The task-root name is computed once and +persisted; every relaunch and resume of that task reuses the persisted name. + ## Scenarios - **GIVEN** a repository-backed task with the Files tree loaded, **WHEN** the user opens the @@ -278,6 +292,29 @@ must be retried. horizontal document overflow and returns focus to the workspace-actions control. - **GIVEN** an agent calls `add_workspace_sources_kandev` for its current idle task, **WHEN** all inputs materialize, **THEN** the UI receives the same task and session updates as the human flow. +- **GIVEN** two distinct tasks whose titles sanitize to the same task-root slug, **WHEN** each task + materializes a Kandev-owned task root, **THEN** their collision-resistant suffixes normally produce + different task-root directory names; if a residual suffix collision occurs, the ownership marker + rejects cross-task repointing with a marker-conflict error rather than redirecting the other task's + entries. +- **GIVEN** a local task whose persisted task root already contains a Kandev-owned directory-link + entry for a repository that points at a different directory than the current durable spec target, + **WHEN** the task launches or resumes, **THEN** reconciliation repoints the owned entry to the + current spec target and the launch/resume proceeds instead of failing with an owned-link target + mismatch on every attempt. +- **GIVEN** a Kandev-owned task-root entry that is not a directory link (a real file or directory a + reconcile did not create), **WHEN** the task launches or resumes, **THEN** reconciliation does not + delete or overwrite it and the launch surfaces an error identifying the conflicting entry. +- **GIVEN** two persisted legacy tasks whose environments share one Kandev-owned task root, **WHEN** + the second task launches or resumes and its ownership marker does not match the root's marker, + **THEN** reconciliation fails closed with a marker-conflict error and does not redirect the first + task's live entry. +- **GIVEN** a multi-source attachment that repointed a pre-existing Kandev-owned entry, **WHEN** a + later source in the same submission fails to materialize, **THEN** rollback restores the repointed + entry to its prior target and no pre-existing entry is left deleted. +- **GIVEN** a Kandev-owned entry whose recreate could fail after the old link is removed, **WHEN** + reconciliation replaces it, **THEN** a traversal or out-of-root name is rejected before any removal + and a failed recreate leaves the original entry intact rather than missing. ## Out of scope @@ -300,4 +337,5 @@ must be retried. See [Attach Workspace Sources plan](../../plans/attach-workspace-sources/plan.md) and the [live add-branch compatibility repair plan](../../plans/restore-live-add-branch/plan.md), plus the -[multi-repository chat file-link repair plan](../../plans/multi-repo-chat-file-links/plan.md). +[multi-repository chat file-link repair plan](../../plans/multi-repo-chat-file-links/plan.md) and the +[owned link target mismatch repair plan](../../plans/owned-link-target-mismatch-repair/plan.md).