diff --git a/internal/specialist/plan_scratchpad.go b/internal/specialist/plan_scratchpad.go index 684ade63..c2dee062 100644 --- a/internal/specialist/plan_scratchpad.go +++ b/internal/specialist/plan_scratchpad.go @@ -1,6 +1,7 @@ package specialist import ( + "errors" "fmt" "os" "path/filepath" @@ -28,11 +29,23 @@ import ( // keys on precisely `!planReadOnlyTools[name]`, so adding a write tool would // silently make every scratchpad-using plan demand a git worktree. // -// So NO TASK WRITES HERE. The EXECUTOR does, single-threaded, as each result -// comes back — and dependents read with the read_file they already hold. Write -// contention is not locked against; it cannot arise, because there is exactly -// one writer. The excerpt stops being lossy and becomes a summary with the whole -// thing one tool call behind it. +// So tasks are not ASKED to write here. The EXECUTOR records each result, +// single-threaded, as it comes back — and dependents read with the read_file +// they already hold. The excerpt stops being lossy and becomes a summary with +// the whole thing one tool call behind it. +// +// WHAT IS AND IS NOT GUARANTEED. The directory sits under the OS temp dir, which +// is a default sandbox write root, so a task granted a write tool CAN write here: +// it can list the directory, and edit or delete a record after the executor +// wrote it. Nothing binds a record to the task that produced it, so this is not +// an integrity boundary — the dependent's prompt still carries the real excerpt +// inline, and a task holding write_file over the shared worktree already holds a +// larger target. A read-only plan's tasks cannot write here at all. +// +// What IS guaranteed is that the executor — the unsandboxed parent — never +// writes through something a task planted: Record creates each file with +// O_EXCL, so a file, symlink or hard link already at a task's destination makes +// the record fail instead of redirecting the write. // // DISPOSABLE, and deliberately not memory: memory is durable, user-facing and // believed by later sessions. This is one plan's working notes and it is deleted @@ -93,9 +106,20 @@ func (pad *Scratchpad) Record(taskID, output string) (string, error) { return "", nil } path := filepath.Join(pad.root, taskID+".md") - if err := os.WriteFile(path, []byte(output), 0o600); err != nil { + // EXCLUSIVE CREATE. Each task is recorded exactly once, so anything already + // at the destination was put there by someone else — and following it would + // let a planted symlink or hard link aim the parent's write at any file it + // can reach. O_EXCL refuses every such case, a dangling symlink included. + file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil { return "", fmt.Errorf("record task %q output: %w", taskID, err) } + _, writeErr := file.WriteString(output) + closeErr := file.Close() + if writeErr != nil || closeErr != nil { + _ = os.Remove(path) + return "", fmt.Errorf("record task %q output: %w", taskID, errors.Join(writeErr, closeErr)) + } return path, nil } diff --git a/internal/specialist/plan_scratchpad_test.go b/internal/specialist/plan_scratchpad_test.go index 28beffe4..9dc6c1ed 100644 --- a/internal/specialist/plan_scratchpad_test.go +++ b/internal/specialist/plan_scratchpad_test.go @@ -290,3 +290,75 @@ func TestACancelledTasksPartialOutputIsStillRecorded(t *testing.T) { t.Fatalf("recorded outside the scratchpad: %s", path) } } + +// RECORD NEVER WRITES THROUGH SOMETHING ALREADY AT THE DESTINATION. The pad is +// listable and, for a write-capable task, writable; a link planted under a +// not-yet-recorded task's id would otherwise aim the unsandboxed parent's write +// at a file of the planter's choosing. +func TestRecordRefusesAnythingAlreadyAtTheDestination(t *testing.T) { + pad, err := NewScratchpad("p") + if err != nil { + t.Fatal(err) + } + defer pad.Release() + + victimDir := t.TempDir() + const original = "victim settings" + newVictim := func(name string) string { + path := filepath.Join(victimDir, name) + if err := os.WriteFile(path, []byte(original), 0o600); err != nil { + t.Fatal(err) + } + return path + } + assertUntouched := func(path string) { + t.Helper() + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != original { + t.Fatalf("Record wrote through a planted link: %s now holds %q", path, got) + } + } + refuses := func(id string) { + t.Helper() + path, err := pad.Record(id, "model-chosen content") + if err == nil || path != "" { + t.Fatalf("Record(%q) = %q, %v; it must refuse an occupied destination", id, path, err) + } + } + + symlinkVictim := newVictim("symlinked.json") + if err := os.Symlink(symlinkVictim, filepath.Join(pad.Root(), "via_symlink.md")); err != nil { + t.Logf("symlinks unsupported here, skipping that case: %v", err) + } else { + refuses("via_symlink") + assertUntouched(symlinkVictim) + } + + // A dangling symlink must not be created-through either. + dangling := filepath.Join(victimDir, "not-yet.json") + if err := os.Symlink(dangling, filepath.Join(pad.Root(), "via_dangling.md")); err == nil { + refuses("via_dangling") + if _, err := os.Lstat(dangling); !os.IsNotExist(err) { + t.Fatalf("Record created the dangling symlink's target: %v", err) + } + } + + hardVictim := newVictim("hardlinked.json") + if err := os.Link(hardVictim, filepath.Join(pad.Root(), "via_hardlink.md")); err != nil { + t.Logf("hard links unsupported here, skipping that case: %v", err) + } else { + refuses("via_hardlink") + assertUntouched(hardVictim) + } + + // And a plain file already there is not overwritten. + planted := filepath.Join(pad.Root(), "via_file.md") + if err := os.WriteFile(planted, []byte(original), 0o600); err != nil { + t.Fatal(err) + } + refuses("via_file") + assertUntouched(planted) +}