session: a worker may not replace the git metadata of the folder it was given (#1351)

cmd/codeaf/do_git_metadata_test.go

@@ -0,0 +1,205 @@
+package main
+
+// #1351: A WORKER MAY NOT REPLACE THE GIT METADATA OF THE FOLDER IT WAS GIVEN.
+//
+// On staging 17ae56d34 a `codeaf do --dir <repo>` worker ran, in order:
+//
+//	rm -rf <repo>/.git
+//	cd <parent> && git init
+//	git commit
+//
+// The repository's history was gone and the parent folder had become a
+// repository of its own. The folder guard (internal/session's taskoutside.go)
+// already refused the `git init` and `git commit` aimed at the parent, because
+// the parent is outside the worker's ground. It allowed the first command,
+// because <repo>/.git is INSIDE the ground. That one command is the whole of the
+// data loss: once the history is deleted there is nothing the later two need to
+// do to finish it.
+//
+// So these tests drive the real headless door with a scripted model that sends
+// exactly those three commands, in a repository nested inside a parent that is
+// NOT scratch (a run's --dir is never under the temp directory, and the scratch
+// exemption would otherwise let the parent through), and assert what the issue
+// asks for: the repository's .git survives, the parent never becomes a
+// repository, and every attempt is refused with the boundary's own words.
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/Agent-Field/agentfield/sdk/go/ai"
+	"github.com/Agent-Field/codeaf/internal/session"
+)
+
+// nestedRunRepo is a committed repository nested one level inside a parent
+// folder, both standing OUTSIDE the machine's scratch directories, the way a
+// person's `--dir` stands.
+//
+// It is placed under the module root rather than t.TempDir because the folder
+// guard treats the temp directory as scratch a task may write to (it is where
+// a shell command keeps what it reads back), and a parent there would be let
+// through by that exemption rather than tested by the boundary.
+func nestedRunRepo(t *testing.T) (parent, repo string) {
+	t.Helper()
+	root, err := os.MkdirTemp(".", "issue1351-")
+	if err != nil {
+		t.Fatal(err)
+	}
+	root, err = filepath.Abs(root)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if root, err = filepath.EvalSymlinks(root); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.RemoveAll(root) })
+	parent = filepath.Join(root, "parent")
+	repo = filepath.Join(parent, "repo")
+	if err := os.MkdirAll(repo, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	beltGit(t, repo, "init")
+	beltGit(t, repo, "checkout", "-b", "work")
+	if err := os.WriteFile(filepath.Join(repo, "README.md"), []byte("the project\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	beltGit(t, repo, "add", "-A")
+	beltGit(t, repo, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-m", "first")
+	return parent, repo
+}
+
+// recordedReplacementSeat is the scripted worker from the issue: it tries the
+// three recorded commands, one per turn, and then finishes its task so the run
+// ends rather than walling. Every tool result it is handed back is kept so the
+// test can read the boundary's own words.
+type recordedReplacementSeat struct {
+	commands []string
+	seen     int
+	results  []string
+}
+
+func (s *recordedReplacementSeat) CompleteWithMessages(_ context.Context, messages []ai.Message, _ ...ai.Option) (*ai.Response, error) {
+	if s.seen > 0 {
+		s.results = append(s.results, lastToolResult(messages))
+	}
+	if doc := beltDocument(messages); strings.Contains(doc, "## Who checks this work") {
+		id := briefTaskID(doc)
+		return beltToolReply("plandb done " + id + " --agent " + id + " --result 'holds: the acceptance is met'"), nil
+	}
+	if s.seen < len(s.commands) {
+		command := s.commands[s.seen]
+		s.seen++
+		return beltToolReply(command), nil
+	}
+	s.seen++
+	return beltToolReply(beltFinish(beltAnswer)), nil
+}
+
+// lastToolResult is the text of the newest tool message: what the belt said
+// back about the command the worker just sent.
+func lastToolResult(messages []ai.Message) string {
+	for index := len(messages) - 1; index >= 0; index-- {
+		if messages[index].Role != "tool" {
+			continue
+		}
+		var b strings.Builder
+		for _, part := range messages[index].Content {
+			b.WriteString(part.Text)
+		}
+		return b.String()
+	}
+	return ""
+}
+
+func TestDoOnTheRunEngineRefusesReplacingTheFoldersGitMetadata(t *testing.T) {
+	beltRunEnv(t)
+	t.Setenv("CODEAF_PLANDB_BIN", beltPlandbDoor(t))
+	parent, repo := nestedRunRepo(t)
+	head := doGitIn(t, repo, "rev-parse", "HEAD")
+
+	// THE DELETE IS SPELLED RELATIVE, AS A WORKER STANDING IN ITS FOLDER SPELLS
+	// IT. The absolute spelling (`rm -rf <repo>/.git`) is already stopped, but
+	// only by accident: the critical-command floor (internal/approval) prompts on
+	// every absolute recursive delete, so it matches `/*` rather than knowing what
+	// .git is. `rm -rf .git` matches nothing there and nothing in the folder guard,
+	// because .git is inside the worker's own ground, which is the gap this
+	// issue is about.
+	seat := &recordedReplacementSeat{commands: []string{
+		"rm -rf .git",
+		"cd " + parent + " && git init",
+		"cd " + parent + " && git -c user.name=t -c user.email=t@t commit --allow-empty -m 'Add add(a,b) and one pytest test'",
+	}}
+	var stdout, stderr strings.Builder
+	_ = doErrand(doRequest{
+		task: "create add.py and its test", workspace: repo, asJSON: true,
+		timeout: 60 * time.Second, slots: bound(1), stdout: &stdout, stderr: &stderr,
+		newBeltCompleter: func(string) session.Completer { return seat },
+	})
+
+	if info, err := os.Stat(filepath.Join(repo, ".git")); err != nil || !info.IsDir() {
+		t.Fatalf("the repository's .git is gone after the run (err %v): the worker replaced the folder's history", err)
+	}
+	if now := doGitIn(t, repo, "rev-parse", "HEAD"); now != head {
+		t.Fatalf("the repository's history moved %s -> %s", head, now)
+	}
+	if _, err := os.Stat(filepath.Join(parent, ".git")); err == nil {
+		t.Fatal("the parent folder became a repository: git metadata was created outside the folder the run was given")
+	}
+	if len(seat.results) < 3 {
+		t.Fatalf("the worker saw %d answers to its three commands, want three\nstdout:\n%s\nstderr:\n%s",
+			len(seat.results), stdout.String(), stderr.String())
+	}
+	for index, said := range seat.results[:3] {
+		if !strings.Contains(said, "is outside your copy") {
+			t.Errorf("command %d (%q) was answered %q, want the boundary's refusal", index+1, seat.commands[index], said)
+		}
+	}
+}
+
+// AND THE WORK THE FOLDER WAS GIVEN FOR STILL RUNS. A guard that kept .git
+// safe by refusing everything would protect nothing anybody wanted done: the
+// same door, in the same nested repository, still writes add.py and its test
+// inside the given folder, and the parent is left exactly as it was.
+func TestDoOnTheRunEngineStillWritesInsideTheFolderItWasGiven(t *testing.T) {
+	beltRunEnv(t)
+	t.Setenv("CODEAF_PLANDB_BIN", beltPlandbDoor(t))
+	parent, repo := nestedRunRepo(t)
+	before, err := os.ReadDir(parent)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	seat := &recordedReplacementSeat{commands: []string{
+		"printf 'def add(a, b):\\n    return a + b\\n' > add.py",
+		"printf 'from add import add\\n\\ndef test_add():\\n    assert add(1, 2) == 3\\n' > test_add.py",
+	}}
+	var stdout, stderr strings.Builder
+	if err := doErrand(doRequest{
+		task: "create add.py and its test", workspace: repo, asJSON: true,
+		timeout: 60 * time.Second, slots: bound(1), stdout: &stdout, stderr: &stderr,
+		newBeltCompleter: func(string) session.Completer { return seat },
+	}); err != nil {
+		t.Fatalf("an ordinary run in its own folder left with %v\nstdout:\n%s\nstderr:\n%s", err, stdout.String(), stderr.String())
+	}
+	for _, name := range []string{"add.py", "test_add.py"} {
+		if _, err := os.Stat(filepath.Join(repo, name)); err != nil {
+			t.Fatalf("%s was not written inside the given folder: %v", name, err)
+		}
+	}
+	for _, said := range seat.results {
+		if strings.Contains(said, "is outside your copy") {
+			t.Fatalf("an ordinary write in the given folder was refused: %q", said)
+		}
+	}
+	after, err := os.ReadDir(parent)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(after) != len(before) {
+		t.Fatalf("the parent folder changed: %d entries before, %d after", len(before), len(after))
+	}
+}

docs/changes/unreleased/2097-worker-keeps-folder-git-metadata.md

@@ -0,0 +1,11 @@
+---
+kind: fixed
+title: a task or codeaf do worker can no longer delete the git history of the folder it was given
+pr: 2097
+surface: [engine, docs]
+invalidates:
+  - "`codeaf do --dir <repo>` and every task kept their worker inside the folder they were given, but `.git` sits inside that folder, so the folder guard read `rm -rf .git` as ordinary work there and let it run. A worker that ran it deleted the repository's history; the `git init` and `git commit` in the parent that followed were already refused, but by then nothing was left to save. `--dir` alone did not protect a repository's history. The absolute spelling `rm -rf <repo>/.git` was stopped only by the critical-command floor's `rm -rf /*` pattern, which asks about every absolute recursive delete and knows nothing about `.git`. Now any command that removes, moves, overwrites or rewrites the given folder's own `.git`, or removes the folder itself, is refused with the boundary's `is outside your copy` sentence, through a symlink as well. Git's own commands, reading `.git`, and every other write in the folder are unchanged, and a folder that is not a repository is unaffected."
+---
+
+Found once on staging 17ae56d34 and not reproduced in three reruns, so this
+closes the boundary rather than a path that always fires.

internal/manual/chat/choosing-a-folder.md

@@ -572,6 +572,17 @@ change it there". That is remembered for that one folder, and from then on write
 it change the folder itself immediately, exactly like the folder you are standing in.
 Nothing guesses this; it is only ever set because you said it.
 
+## Can work in a folder delete its .git? No, the history of the folder a task is given is kept
+
+Whichever way a task stands in a folder you chose, in its own copy or in the folder
+directly, it may not remove, move, overwrite or rewrite that folder's `.git`. That holds
+for `codeaf do --dir` too: the folder you hand it is the one whose history is kept. A task
+that tries `rm -rf .git`, `mv .git …`, `echo … > .git/HEAD` or `rm -rf .` reads the
+same refusal every write outside its folder gets, naming the `.git` and the word
+`is outside your copy`, and carries on with its work. Ordinary git work in the folder, such
+as `git commit` or `git checkout`, is untouched, and so is reading `.git`. permissions has
+the exact wording; how-tasks-run has the full list.
+
 ## Where did my changes go — the folder still looks unchanged
 
 If the model edited files in a folder you chose with `/folder` and that folder still looks

internal/manual/chat/how-tasks-run.md

@@ -882,6 +882,33 @@ The wording it reads names **both the path and the directory it may write in**:
 **The machine's scratch is not yours.** `/tmp`, the temp directory and `/dev/null` are
 written freely — that is where a command line puts what it is about to read back.
 
+**A task may not replace its own copy's git history either.** The table above is about
+writing somewhere else; this is the one thing inside the task's own copy that is refused.
+A command that removes, moves, overwrites or rewrites the copy's `.git` is turned back with
+the same sentence, naming that `.git`:
+
+| Refused inside its own copy | What it looks like |
+| --- | --- |
+| deleting the history | `rm -rf .git`, `rm -rf <copy>/.git`, `rm .git/HEAD`, `rm -rf .git/objects` |
+| moving or replacing it | `mv .git ../kept`, `cp -r other/.git .git`, `rsync --delete … .git/` |
+| rewriting it in place | `sed -i … .git/HEAD`, `echo … > .git/HEAD` |
+| removing the copy itself | `rm -rf .`, `rm -rf <copy>` |
+
+> replacing /Users/you/code/project/.git is outside your copy — this task works in
+> /Users/you/code/project; read anywhere, write only there. …
+
+Everything a task does with git stays allowed: `git commit`, `git checkout`, `git reset`
+and every other git command write inside `.git` on the task's behalf and are answered by
+the git rules as before. So are reading it (`cat .git/HEAD`, `git log`), editing a
+`.gitignore`, and every ordinary write in the copy. A folder that is not a repository has no
+history to protect, so nothing changes there. This holds for `codeaf do --dir` as much as
+for a chat task: the folder you hand it is the copy.
+
+It exists because a `codeaf do --dir <repo>` worker once ran `rm -rf <repo>/.git`, then
+`git init` and `git commit` in the folder above. The last two were already refused, since
+the folder above is outside the copy. But `.git` sits inside the copy, so deleting it passed
+as ordinary work and the repository's history was gone.
+
 **What it cannot see.** It reads the paths a command *names*. `cd elsewhere && python
 fix.py`, where the script writes what it likes, is not caught, and neither is a path built
 out of a shell variable. What it does close is every shape that says its target out loud.

internal/manual/chat/permissions.md

@@ -800,6 +800,23 @@ tool**. `gmail_send:allow` in `tools.approval` runs it silently. So does a
 capability set to `yes`. So does a remembered "always" answer to this approval
 question. Nothing else lifts it.
 
+## Can a task delete my repository's .git or its history? No, it is refused in its own copy
+
+The dangerous-command floor above asks before an absolute recursive delete, but it would not
+notice `rm -rf .git`, written relative to the folder the task stands in. So a task, and a
+`codeaf do --dir` worker, is separately refused any command that removes, moves, overwrites
+or rewrites the `.git` of the folder it was given: `rm -rf .git`, `mv .git …`,
+`cp -r … .git`, `echo … > .git/HEAD`, or removing the folder itself with `rm -rf .`. The
+refusal is the same one every write outside the task's folder gets:
+
+> replacing /Users/you/code/project/.git is outside your copy — this task works in
+> /Users/you/code/project; read anywhere, write only there. …
+
+This is not a question and no setting lifts it, because a task has nobody to ask. Ordinary
+git work is untouched: `git commit`, `git checkout` and the rest write inside `.git` on the
+task's behalf and run as before, and reading `.git` is reading. A conversation you are
+sitting in is not held by it. The full list is on how-tasks-run.
+
 ## Neither floor can be swallowed by "stop asking me"
 
 Before any session memo is consulted, codeaf asks whether the call is one of the

internal/manual/chat_test.go

@@ -3479,6 +3479,55 @@ func TestTheChatManualAnswersTheQuestionsPeopleAsk(t *testing.T) {
 	}
 }
 
+// #1351: a worker given a repository deleted its `.git` and the folder guard let
+// it, because `.git` is inside the folder it was given. The guard now refuses
+// replacing the folder's own git metadata, and the questions a person asks
+// after seeing that, or before trusting `--dir`, must reach the sections that
+// say so: in their words, with the refusal quoted and the shapes named.
+func TestTheGitMetadataQuestionsReachTheSectionsThatRefuseIt(t *testing.T) {
+	asked := []struct {
+		question string
+		page     string
+		title    string
+		says     []string
+	}{
+		{
+			question: "can a task delete my repository's .git",
+			page:     "permissions",
+			title:    "Can a task delete my repository's .git or its history",
+			says:     []string{"is outside your copy", "rm -rf .git", "codeaf do --dir", "git commit"},
+		},
+		{
+			question: "can work in a folder delete its .git history",
+			page:     "choosing-a-folder",
+			title:    "Can work in a folder delete its .git",
+			says:     []string{"is outside your copy", "codeaf do --dir", "rm -rf ."},
+		},
+	}
+	for _, ask := range asked {
+		reached := false
+		for _, section := range Chat().Search(ask.question, DefaultResults) {
+			if section.Page != ask.page || !strings.Contains(section.Title, ask.title) {
+				continue
+			}
+			reached = true
+			for _, needle := range ask.says {
+				if !strings.Contains(section.Body, needle) {
+					t.Errorf("%q reached %q but its body omits %q", ask.question, section.Title, needle)
+				}
+			}
+		}
+		if !reached {
+			found := Chat().Search(ask.question, DefaultResults)
+			where := make([]string, 0, len(found))
+			for _, section := range found {
+				where = append(where, section.Page+" · "+section.Title)
+			}
+			t.Errorf("%q does not reach %s · %q; it reached %v", ask.question, ask.page, ask.title, where)
+		}
+	}
+}
+
 // #1469: home listed waiting standing orders without their due time, and a person
 // asking when one goes off there had to be sent to the standing place for it.
 // Standing orders are automations now (automations.md), and home's `automations`