mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 05:01:28 -10:00
fix(plan): create scratchpad records exclusively and correct the invariant
Record used os.WriteFile, so a symlink or hard link planted at a not-yet-recorded task's path redirected the unsandboxed executor's write. It now creates each file with O_EXCL and refuses anything already at the destination. The package comment claimed no task could write to the pad; write-capable tasks can, so it now states what is and is not guaranteed. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
2 files changed
+102
-6
No files matched your search
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
Reference in new issue
Block a user