diff --git a/cmd/codeaf/do_git_metadata_test.go b/cmd/codeaf/do_git_metadata_test.go new file mode 100644 index 0000000000..8f444667be --- /dev/null +++ b/cmd/codeaf/do_git_metadata_test.go @@ -0,0 +1,267 @@ +package main + +// #1351: A WORKER MAY NOT REPLACE THE GIT METADATA OF THE FOLDER IT WAS GIVEN. +// +// On staging 17ae56d34 a `codeaf do --dir ` worker ran, in order: +// +// rm -rf /.git +// cd && 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 /.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" + "encoding/json" + "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 /.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 + runErr := 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 runErr == nil { + t.Fatalf("a run whose every attempted action was refused exited successfully\nstdout:\n%s\nstderr:\n%s", stdout.String(), stderr.String()) + } + var status exitStatus + if !asExitStatus(runErr, &status) || status != exitIncomplete { + t.Fatalf("the refused-only run exited with %v, want incomplete (%d)", runErr, exitIncomplete) + } + // THE RECEIPT IS THE SERIALIZED ENVELOPE, not the private outcome: its `stop` + // is a JSON field, while headlessOutcome.stop is unexported and never + // survives a round trip, so the incomplete verdict must be read from the + // envelope the run actually printed. + var receipt struct { + Stop stopReason `json:"stop"` + OK bool `json:"ok"` + Settled bool `json:"settled"` + Error string `json:"error"` + } + if err := json.Unmarshal([]byte(stdout.String()), &receipt); err != nil { + t.Fatalf("stdout is not one JSON object: %v\n%s", err, stdout.String()) + } + if receipt.Stop != stopIncomplete || receipt.OK || !receipt.Settled { + t.Fatalf("the refused-only receipt is not a settled incomplete outcome: %+v", receipt) + } + // The incomplete ending carries a reason in its own words. Which reason + // depends on where the worker gave up -- the boundary's refusal when it + // finished after the refusals, the same-step ending when it kept retrying + // them -- and both are a run that ran, was refused, and did not finish. The + // boundary's own words are proved against the answers the worker was handed + // below, which the seat records whichever ending the run reached. + if strings.TrimSpace(receipt.Error) == "" { + t.Fatalf("the incomplete receipt omits its reason: %+v", receipt) + } + + 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) + } + } +} + +// A REFUSAL IS RECOVERABLE. If the worker reads the door's answer, performs real +// work inside its folder, and only then finishes, the run still exits zero. The +// false-success rule is about a claimed completion over nothing but refused +// actions, never about poisoning a task for having met one refusal. +func TestDoOnTheRunEngineCanRecoverAfterARefusedMetadataChange(t *testing.T) { + beltRunEnv(t) + t.Setenv("CODEAF_PLANDB_BIN", beltPlandbDoor(t)) + _, repo := nestedRunRepo(t) + + seat := &recordedReplacementSeat{commands: []string{ + "rm -rf .git", + "printf 'def add(a, b):\\n return a + b\\n' > add.py", + }} + var stdout, stderr strings.Builder + if err := doErrand(doRequest{ + task: "create add.py", 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("a worker that recovered after the refusal left with %v\nstdout:\n%s\nstderr:\n%s", err, stdout.String(), stderr.String()) + } + if _, err := os.Stat(filepath.Join(repo, "add.py")); err != nil { + t.Fatalf("the recovery write did not run: %v", err) + } + if _, err := os.Stat(filepath.Join(repo, ".git")); err != nil { + t.Fatalf("the refused metadata still disappeared: %v", err) + } +} + +// 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)) + } +} diff --git a/docs/changes/unreleased/2097-worker-keeps-folder-git-metadata.md b/docs/changes/unreleased/2097-worker-keeps-folder-git-metadata.md new file mode 100644 index 0000000000..eaa928d692 --- /dev/null +++ b/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 ` 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`, and direct `write` or `edit` calls aimed under `.git`, as ordinary work there and let them run. A worker that ran the delete lost 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 /.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 or direct file tool 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, including paths reached through a symlink. Git's own commands, reading `.git`, ordinary writes in the folder, and copying or moving an ordinary file into the repository root are unchanged; a folder that is not a repository is unaffected. A headless run whose every attempted action was refused can no longer turn that into success by claiming done: it exits incomplete with the refusal in its receipt, while real recovery work after a refusal can still finish." +--- + +Found once on staging 17ae56d34 and not reproduced in three reruns, so this +closes the boundary rather than a path that always fires. diff --git a/internal/manual/chat/choosing-a-folder.md b/internal/manual/chat/choosing-a-folder.md index 25186042f2..72db15f185 100644 --- a/internal/manual/chat/choosing-a-folder.md +++ b/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 diff --git a/internal/manual/chat/how-tasks-run.md b/internal/manual/chat/how-tasks-run.md index 85db2510f6..ea9b2e13e2 100644 --- a/internal/manual/chat/how-tasks-run.md +++ b/internal/manual/chat/how-tasks-run.md @@ -882,6 +882,36 @@ 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 /.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 ` | + +> 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, including copying or moving a file into +`.`. 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. If every attempted action was refused and the worker then claims `done`, `codeaf do` +exits incomplete (status 2) and its JSON receipt includes the refusal. A worker that reads a +refusal and then performs real work may still finish normally. + +It exists because a `codeaf do --dir ` worker once ran `rm -rf /.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. diff --git a/internal/manual/chat/permissions.md b/internal/manual/chat/permissions.md index 8d7b4bf6a4..7c99b90204 100644 --- a/internal/manual/chat/permissions.md +++ b/internal/manual/chat/permissions.md @@ -800,6 +800,26 @@ 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. If every +action the worker attempts is refused and it then claims `done`, a headless run exits +**incomplete** (status 2) and its JSON receipt carries the refusal; a refusal followed by +real work can still finish normally. 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 diff --git a/internal/manual/chat_test.go b/internal/manual/chat_test.go index 5038d1ad67..88121754fd 100644 --- a/internal/manual/chat_test.go +++ b/internal/manual/chat_test.go @@ -3525,6 +3525,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", "exits\n**incomplete** (status 2)"}, + }, + { + 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) + } + } +} + // #1454: task.parallel is on the Tasks tab as `tasks at once`, not under // Spending. This checks the retrieved section's answer rather than only its // page, so restoring the former category cannot leave the retrieval gate green. diff --git a/internal/run/bashworker.go b/internal/run/bashworker.go index 816b870e8d..c975a2890a 100644 --- a/internal/run/bashworker.go +++ b/internal/run/bashworker.go @@ -221,6 +221,7 @@ func (w *BashWorker) Run(ctx context.Context, task plandb.Task) (rep Report, run // running total is the one account that moves per paid call, so it is read // as each event comes by and told to the run whenever it has risen. banked := 0.0 + refused, executed, lastRefusal := attemptedWork(past) // sameStep is the law below, held for the whole turn: the identity of the // last finished step, how many identical ones have come back in a row, and // the moment the store was last read. The words a worker says between its @@ -451,6 +452,14 @@ func (w *BashWorker) Run(ctx context.Context, task plandb.Task) (rep Report, run // not end the task. _ = appendTrajectory(storeDir, task.ID, Step{Kind: trajectoryNotesKind, Notes: had}) } + if event.HarnessMade { + if event.Refused { + refused = true + lastRefusal = event.Output + } + } else if !onlyPlanFinish(stepCommand(event)) { + executed = true + } // THE STORE'S OWN ENDING IS DETECTED AFTER THE COMMAND RUNS. A // `plandb done`, or a `plandb wait`, that the worker itself just // ran is the end of the loop: the shim's verb is already in the @@ -491,6 +500,16 @@ func (w *BashWorker) Run(ctx context.Context, task plandb.Task) (rep Report, run } switch { case ending.kind == endingDone: + if refused && !executed { + reason := "the worker could not finish: every attempted action was refused" + if lastRefusal = strings.TrimSpace(lastRefusal); lastRefusal != "" { + reason += " · " + lastRefusal + } + if err := appendTrajectory(storeDir, task.ID, Step{Kind: trajectoryEndKind, ExitsRecorded: true, Steps: steps, Reason: reason}); err != nil { + return Report{Steps: steps}, err + } + return Report{Steps: steps}, refusedOnlyCompletionError{reason: reason} + } if err := appendTrajectory(storeDir, task.ID, Step{Kind: trajectoryEndKind, ExitsRecorded: true, Steps: steps, Result: ending.result, Reason: "finished in the store"}); err != nil { return Report{Steps: steps}, err } @@ -836,6 +855,41 @@ type storeEnding struct { reason string } +// attemptedWork reads whether a task has met a door refusal, whether anything +// other than a bare finish really ran, and the newest refusal's words. It is +// one function for a resumed trajectory and the live worker's same booleans: +// their truth must not change when a worker is relaunched. +func attemptedWork(steps []Step) (refused, executed bool, lastRefusal string) { + for _, step := range steps { + if step.Refused { + refused = true + lastRefusal = step.Observation + } + if !step.NotRun && !onlyPlanFinish(step.Command) { + executed = true + } + } + return refused, executed, lastRefusal +} + +// refusedOnlyCompletionError is a worker claiming completion after every +// action it attempted was refused and nothing else ran. It is typed so the +// supervisor can keep that false completion from winning merely because +// `plandb done` reached the store before the worker returned (#1351). Other +// errors after a stored completion keep the review behavior they already have. +type refusedOnlyCompletionError struct{ reason string } + +func (e refusedOnlyCompletionError) Error() string { return e.reason } + +// onlyPlanFinish says a command's only act is the store's `plandb done`. A bare +// finish is valid for read-only or already-complete work when no door refused +// anything, but it is not executed recovery after a refusal. Any compound shell +// command is conservatively a real action: this parser does not claim to prove +// the other segment had no effect. +func onlyPlanFinish(command string) bool { + return session.BashCommandIsOnly(command, "plandb", "done") +} + // storeEnding reads the task's row and answers the ending it carries, if any. // A task with no row has no ending; a parked task ends as a wait; a done task // ends with its own result. diff --git a/internal/run/refused_finish_internal_test.go b/internal/run/refused_finish_internal_test.go new file mode 100644 index 0000000000..6e569e91df --- /dev/null +++ b/internal/run/refused_finish_internal_test.go @@ -0,0 +1,65 @@ +package run + +import "testing" + +func TestOnlyARefusedOnlyFinishIsIncomplete(t *testing.T) { + cases := []struct { + name string + past []Step + refused bool + executed bool + }{ + { + name: "a refusal with nothing else run", + past: []Step{{Command: "rm -rf .git", NotRun: true, Refused: true, Observation: "is outside your copy"}}, + refused: true, + }, + { + name: "a refusal followed by real recovery work", + past: []Step{ + {Command: "rm -rf .git", NotRun: true, Refused: true}, + {Command: "printf x > add.py", ExitCode: new(int)}, + }, + refused: true, executed: true, + }, + { + name: "a direct read-only completion", + past: []Step{{Command: "plandb done root --agent root --result ok", ExitCode: new(int)}}, + }, + { + name: "a compound finish carries another action", + past: []Step{{Command: "printf x > add.py && plandb done root --agent root --result ok", ExitCode: new(int)}}, + executed: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + refused, executed, _ := attemptedWork(tc.past) + if refused != tc.refused || executed != tc.executed { + t.Fatalf("attemptedWork = refused %v, executed %v; want %v, %v", refused, executed, tc.refused, tc.executed) + } + }) + } +} + +func TestOnlyPlanFinishRecognizesNoOtherShellAction(t *testing.T) { + for _, command := range []string{ + "plandb done root --agent root --result ok", + "plandb done root --agent root --result 'words with spaces'", + "plandb done root --agent root --result 'research R&D; no change'", + } { + if !onlyPlanFinish(command) { + t.Errorf("%q was not recognized as only a finish", command) + } + } + for _, command := range []string{ + "printf x > add.py && plandb done root --agent root --result ok", + "plandb done root --agent root --result ok; echo x", + "echo plandb done root", + "plandb wait root --agent root", + } { + if onlyPlanFinish(command) { + t.Errorf("%q was read as only a finish", command) + } + } +} diff --git a/internal/run/run.go b/internal/run/run.go index 179f4d1e9f..f91cab74ba 100644 --- a/internal/run/run.go +++ b/internal/run/run.go @@ -893,7 +893,17 @@ func (s *Supervisor) absorb(ret workerReturn) { // stops the run from ever completing, not a row. if ret.err != nil { root := s.store.Task(ret.task.ID) - if root.Status == plandb.StatusDone { + var refusedOnly refusedOnlyCompletionError + if errors.As(ret.err, &refusedOnly) { + // A WORKER CANNOT TURN REFUSED-ONLY WORK INTO DONE BY WRITING + // `plandb done` before it returns. The store mutation arrives first, + // but the trajectory fact arrives with the return and outranks that + // false completion: the run's outcome is incomplete, carries the + // boundary's refusal, and no review is seated for work that did not + // run (#1351). A successful recovery never returns this typed error. + s.rootFailed = true + s.rootFailure = refusedOnly.Error() + } else if root.Status == plandb.StatusDone { // FINISHED WORK GETS A REVIEW ROUND BEFORE THE RUN MAY ANSWER // DONE. The store's ending stands even when its worker later // returns an error, including the result the review must read. diff --git a/internal/session/taskoutside.go b/internal/session/taskoutside.go index e977318ce9..720cb3aae3 100644 --- a/internal/session/taskoutside.go +++ b/internal/session/taskoutside.go @@ -126,7 +126,18 @@ func (g taskGroundGuard) PreAction(_ context.Context, _ *episode, _ *eventHub, c return call, toolResult{}, true } path, shown, ok := g.agent.mutatingPath(call) - if !ok || g.agent.taskMayWrite(path) { + if !ok { + return call, toolResult{}, true + } + // `write` AND `edit` NAME THEIR TARGET WITHOUT A SHELL. The bash road + // below protects the ground's `.git`; letting these hands take the + // direct road around it would leave the same history writable through + // another tool. Ask on the real spelling too, so a symlink to `.git` is + // not the spelling that gets through (#1351). + if metadata := groundMetadataDir(ground); metadata != "" && protectedGroundMetadata(metadata, path, true) { + return call, toolResult{text: g.agent.taskOutsideRefusal("replacing " + metadata), isError: true}, false + } + if g.agent.taskMayWrite(path) { return call, toolResult{}, true } return call, toolResult{text: g.agent.taskOutsideRefusal(shown), isError: true}, false @@ -474,13 +485,8 @@ func outsideScopeAim(ground, command string, allowed func(string) bool) string { cwd := ground for _, segment := range taskSegments(command) { words, assignments := taskSegmentHead(stripRedirections(segment)) - for index, word := range segment { - if index+1 < len(segment) && fileWriteRedirection(word, segment[index+1]) { - target := resolvePath(cwd, segment[index+1]) - if !allowed(target) { - return "writing to " + target - } - } + if aimed := redirectionAimedOutside(ground, cwd, segment, allowed); aimed != "" { + return aimed } if len(words) == 0 { continue @@ -497,6 +503,9 @@ func outsideScopeAim(ground, command string, allowed func(string) bool) string { } return "git " + verb + " in " + dir default: + if aimed := replacedGroundMetadata(ground, cwd, program, rest); aimed != "" { + return aimed + } if aimed := writeAimedOutsideScope(cwd, program, rest, allowed); aimed != "" { return aimed } @@ -505,6 +514,345 @@ func outsideScopeAim(ground, command string, allowed func(string) bool) string { return "" } +// redirectionAimedOutside answers the first file a segment's redirection would +// write that the task may not write, and "" when every one of them is the task's. +// +// A REDIRECTION INTO THE GROUND'S OWN `.git` IS THE METADATA LAW'S, NOT THE PATH +// LAW'S: `echo ref: x > .git/HEAD` is inside the ground and would pass a check +// that only asked where it was aimed, so it is answered first, in the same +// sentence [replacedGroundMetadata] gives the hands that name `.git`. +func redirectionAimedOutside(ground, cwd string, segment []string, allowed func(string) bool) string { + for index, word := range segment { + if index+1 >= len(segment) || !fileWriteRedirection(word, segment[index+1]) { + continue + } + target := resolvePath(cwd, segment[index+1]) + if metadata := groundMetadataDir(ground); metadata != "" && protectedGroundMetadata(metadata, target, true) { + return "replacing " + metadata + } + if !allowed(target) { + return "writing to " + target + } + } + return "" +} + +// replacedGroundMetadata answers the ground's own `.git` when a hand would remove, +// move, overwrite or rewrite it, and "" when it would not (#1351). +// +// THE PATH LAW CANNOT SEE THIS ON ITS OWN, and that is the whole defect. It +// asks whether a path is inside the folder the task may write in, and a +// repository's `.git` always is, because it sits inside the tree it belongs to. +// So a worker that ran `rm -rf .git` on the folder it was given passed every +// check, and the history was gone, which is the data loss; the `git init` and +// `git commit` in the parent that followed were already refused, but by then +// they had nothing left to save. +// +// IT IS ASKED OF THE SAME HANDS THE PATH LAW READS ([writesEveryOperand], +// [writesItsLastOperand], an in-place sed or perl, and a redirection, which +// [outsideScopeAim] asks beside it), so it closes exactly the shapes that name +// `.git` and claims nothing more: a script that deletes it by itself is the +// blind spot this file already says it has. Git's own verbs are not hands here: +// `git commit` and `git checkout` write inside `.git` on the task's behalf, and +// that is the work, so they are answered by the git guard as before. +// +// AN OPERAND IS THE METADATA WHEN IT IS `.git` OR UNDER IT, OR WHEN IT HOLDS +// IT. Removing the ground itself (`rm -rf .`, `rm -rf `) takes `.git` +// with it, so it is refused by the same sentence; a file elsewhere in the +// ground is untouched by this and stays the task's to change. +func replacedGroundMetadata(ground, cwd, program string, rest []string) string { + metadata := groundMetadataDir(ground) + if metadata == "" { + return "" + } + operands := commandOperands(rest) + switch { + case program == "sed" || program == "perl": + if !inPlaceEdit(rest) { + return "" + } + return protectedMetadataOperand(metadata, cwd, operands, false, true) + case program == "rm" || program == "rmdir" || program == "unlink": + // These hands remove the named entry. An ancestor takes `.git` with + // it, but a final symlink is only unlinked; a path below a directory + // symlink still follows its parent into the real metadata. + return protectedMetadataOperand(metadata, cwd, operands, true, false) + case program == "mv": + return movedGroundMetadata(metadata, cwd, operands) + case writesItsLastOperand[program]: + return copiedGroundMetadata(metadata, cwd, program, rest, operands) + case writesEveryOperand[program]: + // mkdir/chmod/touch on `.` are ordinary upkeep, not replacement of + // everything the directory contains. Only a protected metadata path + // they name directly is held. + return protectedMetadataOperand(metadata, cwd, operands, false, true) + default: + return "" + } +} + +// protectedMetadataOperand answers the first operand that names protected git +// metadata. When holding is true, an ancestor that would take `.git` with it +// counts too. Shell globs are expanded against cwd before the comparison: the +// shell turns `.g?t` and `.[g]it` into `.git`, so the guard must read the same +// target rather than the unexpanded spelling (#2097 review). +func protectedMetadataOperand(metadata, cwd string, operands []string, holding, followFinal bool) string { + for _, operand := range operands { + for _, target := range expandedOperand(cwd, operand) { + if protectedGroundMetadata(metadata, target, followFinal) || + holding && holdsGroundMetadata(metadata, target) { + return "replacing " + metadata + } + } + } + return "" +} + +// expandedOperand is the path or paths the shell hands one filesystem command. +// An unmatched pattern stays itself, exactly as an ordinary shell leaves it. +func expandedOperand(cwd, operand string) []string { + target := resolvePath(cwd, operand) + if !strings.ContainsAny(operand, "*?[") { + return []string{target} + } + matches, err := filepath.Glob(target) + if err != nil || len(matches) == 0 { + return []string{target} + } + return matches +} + +// movedGroundMetadata reads mv's two sides differently. Every operand but the +// last is a source and removing a source directory removes any `.git` under it; +// the last is a destination, where `mv old.py .` merely adds a file to the +// repository and must stay allowed. A source actually named `.git` and moved +// into the repository root is the one destination-directory form that installs +// metadata and is refused too. +func movedGroundMetadata(metadata, cwd string, operands []string) string { + if len(operands) == 0 { + return "" + } + last := len(operands) - 1 + for index, operand := range operands { + targets := expandedOperand(cwd, operand) + if index < last { + // Moving a source removes it, but moving a symlink removes only the + // link. A path below a directory symlink still resolves through it. + for _, target := range targets { + if protectedGroundMetadata(metadata, target, false) || holdsGroundMetadata(metadata, target) { + return "replacing " + metadata + } + } + continue + } + for _, target := range targets { + if last == 0 && (protectedGroundMetadata(metadata, target, false) || holdsGroundMetadata(metadata, target)) { + return "replacing " + metadata + } + if last > 0 && protectedGroundMetadata(metadata, target, true) { + return "replacing " + metadata + } + } + } + if last > 0 && destinationInstallsMetadata(metadata, cwd, operands[:last], operands[last]) { + return "replacing " + metadata + } + return "" +} + +// copiedGroundMetadata reads cp, ln, install and rsync as sources followed by +// one destination. Writing TO `.git` is always refused. Writing an ordinary file +// into `.` is not. A source itself named `.git` would install metadata at a +// directory destination, and rsync --delete aimed at an ancestor can remove the +// existing metadata, so those two forms are refused by name. +func copiedGroundMetadata(metadata, cwd, program string, rest, operands []string) string { + destination, sources, ok := copyDestination(rest, operands) + if !ok { + return "" + } + target := resolvePath(cwd, destination) + if protectedGroundMetadata(metadata, target, true) || + program == "rsync" && rsyncDeletes(rest) && holdsGroundMetadata(metadata, target) { + return "replacing " + metadata + } + if destinationInstallsMetadata(metadata, cwd, sources, destination) { + return "replacing " + metadata + } + return "" +} + +// copyDestination returns cp/ln/install/rsync's destination and sources. GNU +// cp and install may put the destination before the sources with `-t DIR` or +// `--target-directory=DIR`; commandOperands deliberately drops flags, so that +// value must be read from the original words before the ordinary final-operand +// rule (#2097 review). +func copyDestination(rest, operands []string) (destination string, sources []string, ok bool) { + for index := 0; index < len(rest); index++ { + word := rest[index] + switch { + case word == "-t" || word == "--target-directory": + if index+1 >= len(rest) { + return "", nil, false + } + destination = rest[index+1] + sources = removeOperand(operands, destination) + return destination, sources, true + case strings.HasPrefix(word, "--target-directory="): + destination = strings.TrimPrefix(word, "--target-directory=") + sources = removeOperand(operands, destination) + return destination, sources, destination != "" + } + } + if len(operands) == 0 { + return "", nil, false + } + last := len(operands) - 1 + return operands[last], operands[:last], true +} + +func removeOperand(operands []string, value string) []string { + out := make([]string, 0, len(operands)) + removed := false + for _, operand := range operands { + if !removed && operand == value { + removed = true + continue + } + out = append(out, operand) + } + return out +} + +func destinationInstallsMetadata(metadata, cwd string, sources []string, destination string) bool { + dir := resolvePath(cwd, destination) + info, err := os.Stat(dir) + if err != nil || !info.IsDir() { + return false + } + for _, source := range sources { + name := filepath.Base(filepath.Clean(strings.TrimRight(source, `/\\`))) + if name == ".git" && namesGroundMetadata(metadata, filepath.Join(dir, name), false, true) { + return true + } + } + return false +} + +func rsyncDeletes(rest []string) bool { + for _, word := range rest { + if strings.HasPrefix(word, "--delete") { + return true + } + } + return false +} + +// protectedGroundMetadata answers whether target is the `.git` entry itself or +// repository-defining/history data under it. Routine transient and local +// customization paths stay writable: a worker may remove `index.lock`, update +// `info/exclude`, and install hooks without being mistaken for replacing the +// repository's identity or history (#2097 review). +func protectedGroundMetadata(metadata, target string, followFinal bool) bool { + paths := [3]string{target, resolveParentSymlinks(target), ""} + if followFinal { + paths[2] = resolveSymlinks(target) + } + for _, path := range paths { + if path == "" { + continue + } + relative, err := filepath.Rel(metadata, path) + if err != nil || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + continue + } + relative = filepath.ToSlash(relative) + if relative == "." || relative == "HEAD" || relative == "config" || relative == "packed-refs" || + withinMetadataTree(relative, "objects") || withinMetadataTree(relative, "refs") || + withinMetadataTree(relative, "worktrees") || withinMetadataTree(relative, "modules") { + return true + } + } + return false +} + +// withinMetadataTree answers whether relative (always forward-slashed) is a +// protected `.git` subtree: the directory `name` itself, removed whole, or +// anything beneath it. Removing `.git/objects` takes every object with it just +// as removing `.git/objects/pack` does, so the directory's own name counts. +func withinMetadataTree(relative, name string) bool { + return relative == name || strings.HasPrefix(relative, name+"/") +} + +// holdsGroundMetadata says deleting or moving target itself would also remove +// metadata because target is `.git` or an ancestor of it. Descendants do not +// answer true: removing `.git/index.lock` does not remove `.git`. +func holdsGroundMetadata(metadata, target string) bool { + for _, path := range [2]string{target, resolveParentSymlinks(target)} { + if withinDir(path, metadata) { + return true + } + } + return false +} + +// namesGroundMetadata answers whether a path a command names is the ground's +// `.git` or something under it, and, when holding counts, a directory that holds +// it (removing the ground itself takes its history too). +// +// IT IS ASKED ON BOTH SPELLINGS, the one written and the real one. A symlink to +// the ground, or to `.git` itself, is the same history under another name, and +// [insideGround] resolves symlinks for exactly this reason; a law that compared +// only the written path would be one more spelling the model learns to use. +func namesGroundMetadata(metadata, target string, holding, followFinal bool) bool { + real := resolveSymlinks(metadata) + paths := [3]string{target, resolveParentSymlinks(target), ""} + if followFinal { + paths[2] = resolveSymlinks(target) + } + for _, path := range paths { + if path == "" { + continue + } + for _, dir := range [2]string{metadata, real} { + if withinDir(dir, path) || (holding && withinDir(path, dir)) { + return true + } + } + } + return false +} + +// resolveParentSymlinks resolves every directory before the final name but +// leaves that final entry itself alone. Removing or moving a symlink acts on the +// link; writing through it follows it. Both still follow a linked parent, so +// `linked-ground/.git` and `metadata-link/HEAD` reach the protected metadata. +func resolveParentSymlinks(path string) string { + parent, name := filepath.Dir(path), filepath.Base(path) + if resolved, err := filepath.EvalSymlinks(parent); err == nil { + return filepath.Join(resolved, name) + } + return path +} + +// groundMetadataDir is the `.git` that makes the ground a repository, spelled the +// way [resolvePath] spells a command's operands so the two compare. It is "" for +// a ground that is not one, which leaves a plain folder's work untouched. +// +// A LINKED WORKTREE'S `.git` IS A FILE AND IS STILL ITS METADATA: it is the +// pointer that ties the copy to its repository, and removing it is the same +// loss of history from the copy's side. +func groundMetadataDir(ground string) string { + ground = strings.TrimSpace(ground) + if ground == "" { + return "" + } + metadata := filepath.Join(filepath.Clean(ground), ".git") + if _, err := os.Lstat(metadata); err != nil { + return "" + } + return metadata +} + // insideGround is the one comparison the whole law rests on. // // THE MACHINE'S SCRATCH IS NOT SOMEBODY'S WORK. /tmp, the process's own temp @@ -938,6 +1286,28 @@ func commandVerb(words []string) (string, []string) { // is written, so a path built out of a variable is a path this scan cannot place — // stated here because it is the shape of the next escalation, and because a // reader of this file should not have to discover it by testing. +// BashCommandIsOnly reports whether command is one shell segment whose command +// words begin with the exact wanted words. It is the exported reading the run +// worker needs for its `plandb done` completion gate: use the same parser that +// guards bash rather than a second delimiter heuristic that mistakes `&` or `;` +// inside a quoted result for another command. +func BashCommandIsOnly(command string, wanted ...string) bool { + segments := taskSegments(command) + if len(segments) != 1 { + return false + } + words, _ := taskSegmentHead(stripRedirections(segments[0])) + if len(words) < len(wanted) { + return false + } + for index, word := range wanted { + if words[index] != word { + return false + } + } + return true +} + func taskSegments(command string) [][]string { var segments [][]string var words []string @@ -1041,7 +1411,7 @@ func taskSegmentHead(segment []string) ([]string, map[string]string) { continue } switch word { - case "sudo", "env", "time", "nohup", "exec", "command", "then", "else", "do", "!": + case "sudo", "doas", "env", "time", "nohup", "exec", "command", "then", "else", "do", "!": continue } return segment[index:], assignments diff --git a/internal/session/taskoutside_test.go b/internal/session/taskoutside_test.go index 708c2dd930..cb27f24817 100644 --- a/internal/session/taskoutside_test.go +++ b/internal/session/taskoutside_test.go @@ -11,6 +11,7 @@ package session import ( "context" + "fmt" "os" "path/filepath" "strings" @@ -229,6 +230,48 @@ func TestAWorkerIsRefusedOnTheSeamForWritingOutsideItsGround(t *testing.T) { t.Fatal("the shell refusal was booked against the model") } + // AND THE NON-SHELL HANDS DO NOT GO ROUND THE METADATA LAW. `write` and + // `edit` name their path directly, so they are refused before they can + // rewrite the ground's HEAD or config, including through a symlink. + if err := os.MkdirAll(filepath.Join(ground, ".git"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(ground, ".git", "HEAD"), []byte("original\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(ground, "payload"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(ground, "payload", "HEAD"), []byte("broken\n"), 0o644); err != nil { + t.Fatal(err) + } + metadataCalls := []ai.ToolCall{ + withdrawnCall("metadata-write", "write", `{"path":".git/HEAD","content":"ref: refs/heads/replaced"}`), + withdrawnCall("metadata-edit", "edit", `{"path":".git/config","oldText":"a","newText":"b"}`), + } + if err := os.Symlink(filepath.Join(ground, ".git"), filepath.Join(ground, "metadata-link")); err == nil { + metadataCalls = append(metadataCalls, + withdrawnCall("metadata-link-write", "write", `{"path":"metadata-link/HEAD","content":"ref: refs/heads/replaced"}`), + withdrawnCall("metadata-link-edit", "edit", `{"path":"metadata-link/config","oldText":"a","newText":"b"}`)) + } + for index, call := range metadataCalls { + result := worker.executeTool(context.Background(), episode, nil, call, "") + if !result.isError || !result.harness || !strings.Contains(result.text, outsideYourCopy) { + t.Errorf("metadata call %d answered %+v, want the harness boundary refusal", index+1, result) + } + } + + for index, command := range []string{"rm -rf .g?t", "rm -rf .[g]it", "cp -t .git payload/HEAD", "install --target-directory=.git payload/HEAD"} { + result := worker.executeTool(context.Background(), episode, nil, + withdrawnCall(fmt.Sprintf("metadata-bash-%d", index), "bash", `{"command":`+quoteJSON(command)+`}`), "") + if !result.isError || !result.harness || !strings.Contains(result.text, outsideYourCopy) { + t.Errorf("metadata command %q answered %+v, want the harness boundary refusal", command, result) + } + if got, err := os.ReadFile(filepath.Join(ground, ".git", "HEAD")); err != nil || string(got) != "original\n" { + t.Errorf("metadata command %q changed HEAD to %q (err %v)", command, got, err) + } + } + // AND THE CONVERSATION IS UNTOUCHED. A person in their own checkout writes // wherever they like. chat, _ := newTestAgent(t, &scriptedCompleter{}, nil) @@ -239,3 +282,181 @@ func TestAWorkerIsRefusedOnTheSeamForWritingOutsideItsGround(t *testing.T) { t.Fatal("the guard refused a person's own session; it answers for work inside a task and nothing else") } } + +// #1351: THE GROUND'S OWN GIT METADATA IS NOT THE GROUND'S TO REPLACE. +// +// A worker given a repository ran `rm -rf /.git`, then `git init` and +// `git commit` in the parent. The parent half was already refused because the +// parent is outside the ground. The deletion was not, because a repository's +// `.git` sits inside the tree it belongs to, so the path law read it as ordinary +// work in the folder the worker was given. Every spelling of that act is refused +// here with the boundary's own words, and the ground keeps its history. +func TestATaskMayNotReplaceItsGroundsGitMetadata(t *testing.T) { + ground := groundWithMetadata(t) + parent := filepath.Dir(ground) + var allowed []string + refused := []string{ + // The recorded command, and the spelling a worker standing in the + // folder uses. + "rm -rf " + filepath.Join(ground, ".git"), + "rm -rf .git", + "doas rm -rf .git", + "rm -rf .g?t", + "rm -rf .[g]it", + "rm -r .git", + "rm -fr ./.git", + "rm -rf .git/", + "cd " + ground + " && rm -rf .git", + // Taking the history apart a piece at a time is the same loss. + "rm -rf .git/objects", + "rm .git/HEAD", + // Moving it out of the way, or overwriting it with another one. + "mv .git ../kept-history", + "cp -r /tmp/other/.git .git", + "cp -t .git /tmp/payload/HEAD", + "cp --target-directory=.git /tmp/payload/HEAD", + "install -t .git /tmp/payload/HEAD", + "install --target-directory=.git /tmp/payload/HEAD", + "rsync -a --delete /tmp/empty/ .git/", + // Rewriting it in place. + "sed -i 's/main/x/' .git/HEAD", + "echo 'ref: refs/heads/x' > .git/HEAD", + // Removing the ground itself takes its history with it. + "rm -rf .", + "rm -rf " + ground, + // And the parent half the issue recorded stays refused. + "cd " + parent + " && git init", + "cd .. && git init", + "git -C " + parent + " init", + // Walking down into the tree and back out is still the ground's .git. + "cd sub && rm -rf ../.git", + } + if err := os.MkdirAll(filepath.Join(ground, "sub"), 0o755); err != nil { + t.Fatal(err) + } + // A SYMLINK IS THE SAME HISTORY UNDER ANOTHER NAME. A link to the ground, or + // to its .git, must not be the spelling that gets through. + linkedGround := filepath.Join(ground, "linked-ground") + linkedMetadata := filepath.Join(ground, "linked-metadata") + if err := os.Symlink(ground, linkedGround); err == nil { + refused = append(refused, + "rm -rf "+filepath.Join(linkedGround, ".git"), + "cd "+linkedGround+" && rm -rf .git") + allowed = append(allowed, + "rm "+linkedGround, + "mv "+linkedGround+" "+linkedGround+"-moved") + } + if err := os.Symlink(filepath.Join(ground, ".git"), linkedMetadata); err == nil { + refused = append(refused, + "rm -rf "+filepath.Join(linkedMetadata, "objects")) + allowed = append(allowed, + "rm "+linkedMetadata, + "unlink "+linkedMetadata, + "mv "+linkedMetadata+" "+linkedMetadata+"-moved") + } + for _, command := range refused { + why := refusedOutsideGround(ground, command) + if why == "" { + t.Errorf("%q was allowed, want the ground's git metadata protected", command) + continue + } + if !strings.Contains(why, outsideYourCopy) { + t.Errorf("%q was refused with %q, want the boundary's own words %q", command, why, outsideYourCopy) + } + } + for _, command := range allowed { + if why := refusedOutsideGround(ground, command); why != "" { + t.Errorf("%q was refused with %q, but it removes or moves only a symlink", command, why) + } + } +} + +// AND THE WORK THE GROUND IS FOR STILL RUNS. Protecting .git by refusing the +// folder would protect nothing anybody wanted done, so ordinary writes, git's +// own verbs (which write inside .git on the task's behalf), a .gitignore beside +// it, and reading .git all pass exactly as before. +func TestATaskStillWorksInAGroundWhoseMetadataIsProtected(t *testing.T) { + ground := groundWithMetadata(t) + allowed := []string{ + "printf 'def add(a, b):\n return a + b\n' > add.py", + "mkdir -p tests && touch tests/__init__.py", + "rm -rf build dist", + "mv old.py new.py", + "cp template.py add.py", + "sed -i 's/a/b/' add.py", + "echo node_modules/ >> .gitignore", + "rm .gitignore", + "git add -A", + "git -c user.name=t -c user.email=t@t commit -m 'add add'", + "git checkout -b fix/add", + "git reset --hard HEAD", + "cat .git/HEAD", + "ls .git", + "git log --oneline", + // A destination directory that happens to contain `.git` is not itself + // metadata. These common commands add or move ordinary work into the + // repository root and must not be confused with removing the root. + "cp /tmp/a .", + "mv old.py .", + "install /tmp/tool .", + "rsync -a src/ .", + "mkdir -p .", + "touch .", + "chmod 755 .", + "chown user .", + // Ordinary upkeep under .git does not replace repository identity or + // history. A worker may clear a stale lock, write local excludes, or + // install a hook. + "rm -f .git/index.lock", + "printf 'scratch/\\n' >> .git/info/exclude", + "cp /tmp/pre-commit .git/hooks/pre-commit", + "install /tmp/pre-commit .git/hooks/pre-commit", + // Names that only begin with .git are not the history. + "rm -rf .github", + "rm .gitmodules", + "rm -rf .git.bak", + // And a nested .git below the ground belongs to nothing this law guards. + "rm -rf vendor/lib/.git", + } + for _, command := range allowed { + if why := refusedOutsideGround(ground, command); why != "" { + t.Errorf("%q was refused with %q, want ordinary work in the ground allowed", command, why) + } + } +} + +// A GROUND THAT IS NOT A REPOSITORY HAS NO HISTORY TO PROTECT, so a plain +// folder's work, including a file a person happens to name `.git`-free, is left +// exactly as the path law always left it. +func TestAPlainFolderGroundIsUnchangedByTheMetadataLaw(t *testing.T) { + ground := t.TempDir() + for _, command := range []string{"rm -rf build", "rm -rf .", "printf x > notes.md"} { + if why := refusedOutsideGround(ground, command); why != "" { + t.Errorf("%q in a plain folder was refused with %q, want it allowed", command, why) + } + } +} + +// groundWithMetadata is a repository-shaped ground standing OUTSIDE the +// machine's scratch, the way a person's `--dir` stands. The scratch exemption in +// [insideGround] would otherwise let its parent through, which would hide the +// parent half of the law this file checks. +func groundWithMetadata(t *testing.T) string { + t.Helper() + root, err := os.MkdirTemp(".", "ground-metadata-") + if err != nil { + t.Fatal(err) + } + if root, err = filepath.Abs(root); err != nil { + t.Fatal(err) + } + if root, err = filepath.EvalSymlinks(root); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.RemoveAll(root) }) + ground := filepath.Join(root, "parent", "repo") + if err := os.MkdirAll(filepath.Join(ground, ".git"), 0o755); err != nil { + t.Fatal(err) + } + return ground +}