diff --git a/docs/STANDING-ORDERS.md b/docs/STANDING-ORDERS.md index 2fcf139ce..befb32afa 100644 --- a/docs/STANDING-ORDERS.md +++ b/docs/STANDING-ORDERS.md @@ -70,7 +70,11 @@ opened — words as the epigraph, brief as the mechanism. **D3 — Grant.** One prose sentence on `Item`: what acting on this order may do without asking. Empty means say-only (every pre-existing item). Wave 1 stores -and displays it; wave 3's judgment is bounded by it. +and displays it; wave 3's judgment is bounded by it. A task's separate +`Does.Isolate` setting selects a Git worktree and is displayed before approval. +The grant is permission prose, not an execution-mode parser. Each isolated +firing keeps its branch, working directory and recovery record, including +uncommitted files. See the chat manual's scheduled-branch section for limits. **D4 — Exceptions.** `[]Exception` on `Item`, each naming exactly one workspace OR one session. Made by the person only, from either direction (the diff --git a/docs/changes/unreleased/1560-standing-branch-isolation.md b/docs/changes/unreleased/1560-standing-branch-isolation.md new file mode 100644 index 000000000..11866ec0f --- /dev/null +++ b/docs/changes/unreleased/1560-standing-branch-isolation.md @@ -0,0 +1,8 @@ +--- +kind: fixed +title: standing execution enforces git branch isolation and worktree validation +pr: 1560 +surface: [engine] +invalidates: + - "Standing task firings claiming branch isolation or granted permission only for branch commits could execute git commits directly onto the host workspace branch. The approved does.isolate setting now selects a dedicated Git branch and worktree. Permission and reply prose no longer decide isolation. Uncommitted work and its recovery record are retained. Isolated tasks cannot bypass the scheduled worktree through an ordinary-turn Once answer." +--- diff --git a/internal/manual/chat/keeping-an-eye.md b/internal/manual/chat/keeping-an-eye.md index faf713c29..2d075d445 100644 --- a/internal/manual/chat/keeping-an-eye.md +++ b/internal/manual/chat/keeping-an-eye.md @@ -959,3 +959,20 @@ piece of the ask still to do. one: nothing that runs on its own may arm something else that runs on its own. - **Nothing is armed by a matcher.** Nothing runs because a phrase looked like a rule; every single one of these was a card you said yes to. + +## Keep scheduled coding work on a separate branch + +For a scheduled task that should open a pull request without merging, set +`does.isolate` to `true` on the `stand` proposal. Its approval card says +`work · separate Git worktree · changes kept for review`. Permission words +alone do not select isolation. An isolated task has no “Only now” option: an +ordinary conversation turn cannot provide the scheduled runner’s worktree. Existing orders keep their current behavior; +replace an order and approve its isolation option to change that behavior. + +An isolated firing requires a Git repository with at least one commit. It +starts from the current committed checkout in a new worktree and branch. +Uncommitted changes in the original checkout are not copied. The firing keeps +its worktree, including unfinished edits, and reports its branch and folder. +You can inspect and commit those files or open a pull request yourself. +It does not merge or delete the copy automatically. This is workspace isolation, +not a sandbox: commands with explicit paths can still access other folders. diff --git a/internal/session/answers.go b/internal/session/answers.go index 73f80e537..fe22ee4ff 100644 --- a/internal/session/answers.go +++ b/internal/session/answers.go @@ -406,6 +406,10 @@ const StandingNoKey = "0" // doing it now says the thing at the wrong time. A rule never runs, so "once" // has nothing to do. func StandingOnceIsAnAnswer(item standing.Item) bool { + // Ordinary turns cannot provide the scheduled executor's retained worktree. + if item.Does.Isolate { + return false + } switch item.CardKindOf() { case standing.CardCheck, standing.CardWatch: return true @@ -489,6 +493,18 @@ func StandingPlainLabel(label string) string { // The no is last and marked safe, so a row that has to drop an answer drops // one in front of it. func StandingOptions(item standing.Item) []AnswerOption { + options := standingOptions(item) + if !StandingOnceIsAnAnswer(item) { + for i, option := range options { + if option.Key == StandingOnceKey { + return append(options[:i], options[i+1:]...) + } + } + } + return options +} + +func standingOptions(item standing.Item) []AnswerOption { cadence := item.When.ShortWords() switch item.CardKindOf() { case standing.CardReminder: diff --git a/internal/session/standing_isolation_test.go b/internal/session/standing_isolation_test.go new file mode 100644 index 000000000..4f1363834 --- /dev/null +++ b/internal/session/standing_isolation_test.go @@ -0,0 +1,157 @@ +package session + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Agent-Field/agentfield/sdk/go/ai" + "github.com/Agent-Field/codeaf/internal/provider" + "github.com/Agent-Field/codeaf/internal/standing" +) + +func TestStandingBranchIsolationLeavesHostUntouched(t *testing.T) { + repo := newTestRepo(t) + hostBranch := currentBranch(repo) + if hostBranch == "" { + hostBranch = "work" + } + hostHeadBefore := branchCommit(repo, hostBranch) + + item := nightly(repo) + item.Grant = "open a pull request, never merge one" + item.Does.Isolate = true + + completer := &scriptedCompleter{steps: []step{ + func(context.Context, []ai.Message) (*ai.Response, error) { + return toolResponse("c1", "bash", `{"command":"git -c user.name=t -c user.email=t@t commit --allow-empty -m \"isolated branch commit\""}`), nil + }, + func(ctx context.Context, _ []ai.Message) (*ai.Response, error) { + text := "committed on branch, not merged" + provider.Emit(ctx, provider.StreamDelta, text) + return textResponse(text), nil + }, + }} + + root := t.TempDir() + runDir := filepath.Join(root, "run-1") + outcome, err := standingChildRunner(t, root, completer).Run(context.Background(), item, runDir, "") + if err != nil { + t.Fatalf("unexpected Run error: %v", err) + } + + if outcome.Kind != "landed" { + t.Fatalf("expected outcome landed, got %q", outcome.Kind) + } + + hostHeadAfter := branchCommit(repo, hostBranch) + if hostHeadAfter != hostHeadBefore { + t.Fatalf("host branch HEAD moved! before=%s after=%s", hostHeadBefore, hostHeadAfter) + } + + branchesOut := gitOut(t, repo, "for-each-ref", "--format=%(refname:short)", "refs/heads/standing/") + branchNames := strings.Fields(branchesOut) + if len(branchNames) == 0 { + t.Fatalf("expected dedicated standing branch in repo, got none: %q", branchesOut) + } + standingBranch := strings.TrimPrefix(branchNames[0], "*") + standingBranch = strings.TrimSpace(standingBranch) + + branchSha := branchCommit(repo, standingBranch) + if branchSha == "" || branchSha == hostHeadBefore { + t.Fatalf("expected dedicated branch commit to exist and differ from hostHeadBefore") + } + + // Commits must be reachable from standingBranch and NOT reachable from hostBranch. + if _, err := git(repo, "merge-base", "--is-ancestor", branchSha, hostBranch); err == nil { + t.Fatalf("branch commit %s must not be reachable from %s", branchSha, hostBranch) + } + if _, err := git(repo, "merge-base", "--is-ancestor", branchSha, standingBranch); err != nil { + t.Fatalf("branch commit %s must be reachable from %s: %v", branchSha, standingBranch, err) + } +} + +// A worker's unfinished files remain in the recorded worktree after Close. +func TestStandingIsolationRetainsUncommittedWork(t *testing.T) { + repo := newTestRepo(t) + item := nightly(repo) + item.Does.Isolate = true + var workspace string + completer := &scriptedCompleter{steps: []step{ + func(context.Context, []ai.Message) (*ai.Response, error) { + return toolResponse("c1", "bash", `{"command":"printf unfinished > retained.txt"}`), nil + }, + func(ctx context.Context, _ []ai.Message) (*ai.Response, error) { + return textResponse("No pull request was needed."), nil + }, + }} + root := t.TempDir() + runner := standingChildRunner(t, root, completer) + original := runner.child + runner.child = func(cfg Config) (*Agent, error) { workspace = cfg.Workspace; return original(cfg) } + runDir := filepath.Join(root, "run-kept") + outcome, err := runner.Run(context.Background(), item, runDir, "") + if err != nil { + t.Fatal(err) + } + if workspace == repo { + t.Fatal("ran in host checkout") + } + data, err := os.ReadFile(filepath.Join(workspace, "retained.txt")) + if err != nil || string(data) != "unfinished" { + t.Fatalf("work lost: %q, %v", data, err) + } + if _, err := os.Stat(filepath.Join(repo, "retained.txt")); !os.IsNotExist(err) { + t.Fatal("host checkout was changed") + } + if outcome.Kind == standing.OutcomeFailed { + t.Fatalf("reply prose changed outcome: %+v", outcome) + } + trees := loadStandingTrees(runDir) + if len(trees) != 1 || trees[0].Dir != workspace { + t.Fatalf("missing recovery record: %+v", trees) + } +} + +func TestStandingIsolationRefusesNonRepositoryBeforeCallingModel(t *testing.T) { + item := nightly(t.TempDir()) + item.Does.Isolate = true + root := t.TempDir() + runner := standingChildRunner(t, root, &scriptedCompleter{}) + runner.child = func(Config) (*Agent, error) { t.Fatal("opened worker for impossible isolation"); return nil, nil } + if _, err := runner.Run(context.Background(), item, filepath.Join(root, "run"), ""); err == nil { + t.Fatal("accepted impossible isolation") + } +} + +func TestStandingAmbientPlainFolderWithoutGrant(t *testing.T) { + plainDir := t.TempDir() + writeFile(t, filepath.Join(plainDir, "data.txt"), "sample data\n") + + item := nightly(plainDir) + item.Grant = "" + + completer := &scriptedCompleter{steps: []step{ + func(ctx context.Context, _ []ai.Message) (*ai.Response, error) { + text := "everything looks fine" + provider.Emit(ctx, provider.StreamDelta, text) + return textResponse(text), nil + }, + }} + + root := t.TempDir() + runDir := filepath.Join(root, "run-3") + outcome, err := standingChildRunner(t, root, completer).Run(context.Background(), item, runDir, "") + if err != nil { + t.Fatalf("unexpected Run error: %v", err) + } + + if outcome.Kind != "landed" { + t.Fatalf("expected outcome landed, got %q", outcome.Kind) + } + if !strings.Contains(outcome.Text, "everything looks fine") { + t.Fatalf("expected outcome.Text to contain response, got %q", outcome.Text) + } +} diff --git a/internal/session/standing_run.go b/internal/session/standing_run.go index 63e1851c2..3a28ee0d1 100644 --- a/internal/session/standing_run.go +++ b/internal/session/standing_run.go @@ -542,20 +542,33 @@ func standingSessionDir(item standing.Item) string { // hands — the parts it handed out and the fold it makes of their reports. All of // it bounded by the item's own rails. // -// IT IS A SESSION AND NOT A WORKTREE. A firing runs in the project the person -// pointed it at, under the rules they have already banked, exactly as -// docs/AMBIENT.md says an unattended run does. What bounds it is not a governor -// somewhere else but the two numbers on the card: how many calls it may make, -// and how much it may spend before the turn is cut. A division does not put the -// money outside that: a part folds its bill into this session's own books -// ([Agent.foldTaskUsage]), so the per-run figure this reads is the whole family's -// and the pass writes down what the family cost ([standingWideWork] for why -// division reaches here at all). +// A TASK'S APPROVED ISOLATION SETTING SELECTS ITS WORKSPACE. An isolated +// firing retains its worktree, including unfinished edits, with the run's +// existing working-copy record. Neither grant nor reply prose is a policy. func (r *standingRunner) Run(ctx context.Context, item standing.Item, runDir, evidence string) (standing.Outcome, error) { cfg, err := standingRunConfig(r.parent, item, runDir) if err != nil { return standing.Outcome{}, err } + + var tree taskTree + if item.Does.Isolate { + root, ok := repositoryRoot(item.Workspace) + if !ok || !hasCommit(root) { + return standing.Outcome{}, errors.New("a separate Git worktree needs a repository with a commit") + } + name := "standing-" + slugify(item.Title()) + "-" + shortID() + dir := canonicalPath(filepath.Join(cfg.Place.Trees(), name)) + tree, err = cutWorktreeAt(cfg.Place, root, dir, "standing/"+name, 0o700) + if err != nil { + return standing.Outcome{}, fmt.Errorf("cut standing worktree: %w", err) + } + cfg.Workspace = tree.dir + cfg.Place.Workspace = tree.dir + // Keep the copy even on cancellation, provider failure, or an unfinished + // edit. A successful turn is not evidence that every file was committed. + } + brief := standingEvidence(item.Does.Brief, evidence) if acceptance := strings.TrimSpace(item.Does.Acceptance); acceptance != "" { brief += "\n\nDONE WHEN: " + acceptance @@ -570,6 +583,20 @@ func (r *standingRunner) Run(ctx context.Context, item standing.Item, runDir, ev return standing.Outcome{}, err } defer func() { _ = agent.Close() }() + if item.Does.Isolate { + agent.trees = append(agent.trees, StandingTree{ + Folder: item.Workspace, + Dir: tree.dir, + Branch: tree.branch, + Root: tree.root, + Home: tree.home, + HomeSha: tree.homeSha, + Mode: TaskModeWorktree, + Cut: time.Now(), + }) + agent.stampTrees() + agent.SettleWrites() + } if graph != nil { // The graph is finished now that there is a session to own it: its home // is this firing, and the parts it may hand out are worked by the agent @@ -701,6 +728,28 @@ func (r *standingRunner) Run(ctx context.Context, item standing.Item, runDir, ev Text: clip(reply, standingOutcomeClip), USD: agent.Usage().CostUSD, } + if item.Does.Isolate { + // Record Git facts rather than trying to classify the model's prose. + // Another session may advance the host branch while we run; that is + // not evidence that this worker changed it. + status, statusErr := git(tree.dir, "status", "--porcelain") + head, headErr := git(tree.dir, "rev-parse", "HEAD") + branch := currentBranch(tree.dir) + if statusErr != nil || headErr != nil || branch != tree.branch { + outcome.Kind = standing.OutcomeFailed + outcome.NeedsPerson = "the worktree's branch could not be confirmed; inspect the saved work" + outcome.Text = outcome.NeedsPerson + } else if strings.TrimSpace(status) != "" || strings.TrimSpace(head) != tree.checkBase { + outcome.Kind = standingCameTo(true, reply, needs) + } + // The location remains visible even when a wordless firing left only + // shell-written files, which producedAFile cannot recognize. + outcome.Text = strings.TrimSpace(outcome.Text + "\nWork kept on " + tree.branch + " in " + tree.dir) + if outcome.Kind == standing.OutcomeNothing { + outcome.Kind = "landed" + } + } + if needs != "" { // NOTHING PRETENDS THIS LANDED. A run that stopped on something only a // person can allow is not a failure and is not a success; it is work diff --git a/internal/session/standing_test.go b/internal/session/standing_test.go index 127d53bdf..4f962cb7b 100644 --- a/internal/session/standing_test.go +++ b/internal/session/standing_test.go @@ -575,7 +575,7 @@ func TestStandingSurfacesAStoreThatWouldNotWrite(t *testing.T) { func TestStandingOnceCreatesNothing(t *testing.T) { store := newFakeStanding(t) completer := &scriptedCompleter{steps: []step{ - standCall("s1", aReminder()), + standCall("s1", `{"op":"propose","words":"weekly report","when":{"kind":"every","every":"168h"},"does":{"kind":"task","brief":"write report"},"rails":{"per_run_usd":0.3}}`), finalText("doing it now"), }} agent := standingAgent(t, completer, store, nil) @@ -2002,3 +2002,33 @@ func standingNextUpdate(t *testing.T, lane <-chan Event) Event { } } } + +func TestIsolatedStandingCannotUseOrdinaryOnceTurn(t *testing.T) { + for _, kind := range []standing.WhenKind{standing.WhenEvery, standing.WhenProbe, standing.WhenFile, standing.WhenIdle} { + item := standing.Item{When: standing.When{Kind: kind}, Does: standing.Action{Kind: standing.ActionTask, Isolate: true}} + if StandingOnceIsAnAnswer(item) { + t.Fatalf("%s offered an ordinary turn for isolated work", kind) + } + options := StandingOptions(item) + if len(options) != 2 || options[0].Key != "1" || options[1].Key != StandingNoKey { + t.Fatalf("%s options: %#v", kind, options) + } + } + store := newFakeStanding(t) + completer := &scriptedCompleter{steps: []step{ + standCall("s1", `{"op":"propose","words":"weekly isolated report","when":{"kind":"every","every":"168h"},"does":{"kind":"task","brief":"write report","isolate":true},"rails":{"per_run_usd":0.3}}`), + finalText("not run"), + }} + agent := standingAgent(t, completer, store, nil) + events, err := agent.Submit(context.Background(), "schedule isolated report") + if err != nil { + t.Fatal(err) + } + collected := drainAnsweringStanding(t, events, func(event Event) { agent.ResolveStanding(event.Standing.ID, StandingAnswer{Once: true}) }) + if len(store.created) != 0 { + t.Fatal("forged once created standing work") + } + if output := toolOutput(t, collected, "stand"); !strings.Contains(output, "nothing was set up or run") { + t.Fatalf("forged once returned %q", output) + } +} diff --git a/internal/session/tools_standing.go b/internal/session/tools_standing.go index fc91dfe96..6ff34e294 100644 --- a/internal/session/tools_standing.go +++ b/internal/session/tools_standing.go @@ -251,12 +251,13 @@ var standSchemaJSON = `{"type":"object","properties":{` + `"brief":{"type":"string","description":"THE WORK, self-contained as propose_task's brief is: nobody will be there to ask. {{evidence}} is replaced by what the probe found."},` + `"acceptance":{"type":"string","description":"How anybody checks the work is done."},` + `"model":{"type":"string","description":"Model for the work, only when the person named one."},` + + `"isolate":{"type":"boolean","description":"Task only: keep a separate Git worktree for review. Set true for branch-only or PR-without-merge requests; shown on approval."},` + `"max_steps":{"type":"integer","description":"Tool calls one firing's work may take (default ` + strconv.Itoa(standingRunSteps) + `)."}` + `},"additionalProperties":false},` + `"rails":{"type":"object","description":"Optional quiet backstops. Name money only when the person did; otherwise the card quotes the machine-wide daily allowance. A hold takes none — it never wakes, so it never spends. Only expires means anything on one.","properties":{` + `"per_run_usd":{"type":"number","description":"The most one firing may spend, judgment included. Send only when they named a per-run limit; otherwise it quietly defaults to ` + strconv.FormatFloat(standDefaultPerRunUSD, 'f', 2, 64) + `."},` + `"max_per_day":{"type":"integer","description":"Firings allowed in one local day. Send only when they named a count; otherwise it quietly defaults to ` + strconv.Itoa(standDefaultMaxPerDay) + `."},` + - `"expires":{"type":"string","description":"Local RFC3339 stamp after which it retires. Omit for never. A stamp already gone is refused, as when.at is — and so is one less than one check (` + standing.Interval.String() + `) after the item's OWN first firing, which would retire it before it ever ran: checks are that far apart and a check asks about the end before it asks what is due, so an end a minute after a one-minute reminder is found expired at the moment it would have been found due. A one-off needs no end at all, since it retires the moment it fires."}` + + `"expires":{"type":"string","description":"Local RFC3339 retirement time; omit for never. Must be future and at least one check (` + standing.Interval.String() + `) after its first firing, since expiry is checked before due work. One-offs retire on firing and need no end."}` + `},"additionalProperties":false},` + `"when_words":{"type":"string","description":"The cadence said back plainly — \"Mondays at 9am\". The card quotes this and never the spec, so never cron."},` + `"cost_words":{"type":"string","description":"When the person named money, quote their limit in their words — \"at most a dollar a run\". Omit when they named none; codeaf quotes the shared allowance."},` + @@ -294,6 +295,7 @@ type standArguments struct { Hint string `json:"hint"` } `json:"when"` Does struct { + Isolate bool `json:"isolate"` Kind string `json:"kind"` Say string `json:"say"` Brief string `json:"brief"` @@ -494,6 +496,9 @@ func (a *Agent) standPropose(ctx context.Context, parsed standArguments) (string } switch { case answer.Once: + if !StandingOnceIsAnAnswer(item) { + return "this card does not offer doing it once now; nothing was set up or run", true, nil + } // Nothing is created and nothing is scheduled. The person wanted the // action, not the arrangement. The result says the next step in so // many words, because "nothing was set up" sent a model off to read @@ -693,6 +698,7 @@ func standingWhen(parsed standArguments, now time.Time) (standing.When, string) // an action to be the content of; every other kind must say what it does. func standingDoes(parsed standArguments, wakes standing.WhenKind) (standing.Action, string) { does := standing.Action{ + Isolate: parsed.Does.Isolate, Kind: standing.ActionKind(strings.ToLower(strings.TrimSpace(parsed.Does.Kind))), Say: strings.TrimSpace(parsed.Does.Say), Brief: strings.TrimSpace(parsed.Does.Brief), @@ -705,7 +711,7 @@ func standingDoes(parsed standArguments, wakes standing.WhenKind) (standing.Acti // that asked for a rule AND a line to say meant one of the two, and // standing something up with an action nothing will ever run would leave // the person holding a card whose promise cannot be kept. - if does.Kind != "" { + if does.Kind != "" || does.Isolate { return standing.Action{}, "Invalid arguments: a hold does nothing — it holds. Leave does out, or give it a when that wakes." } return standing.Action{}, "" diff --git a/internal/standing/standing.go b/internal/standing/standing.go index 02292e71f..1f77ed29c 100644 --- a/internal/standing/standing.go +++ b/internal/standing/standing.go @@ -262,6 +262,9 @@ const ( // Model, Effort and MaxSteps for ActionTask. Either kind may template the // probe's evidence into its text with {{evidence}}. type Action struct { + // Isolate runs a task in a separate Git worktree, as shown on its approval + // card. Permission prose never selects an execution directory. + Isolate bool `json:"isolate,omitempty"` Kind ActionKind `json:"kind"` Say string `json:"say,omitempty"` Brief string `json:"brief,omitempty"` @@ -481,6 +484,9 @@ type Item struct { // admission law in one place: words, a workspace, a kind with its fields, an // action with its text, and rails that are not zero. func (it Item) Validate() error { + if it.Does.Isolate && (it.Does.Kind != ActionTask || it.When.Kind == WhenHold) { + return errors.New("only a waking task can use a separate Git worktree") + } switch { case it.Words == "": return errors.New("an item needs the person's words") diff --git a/internal/tui3/standing.go b/internal/tui3/standing.go index 0e91191da..bad9daf00 100644 --- a/internal/tui3/standing.go +++ b/internal/tui3/standing.go @@ -852,6 +852,11 @@ func (a *app) standBands(card *standingCard, width int) []string { // (docs/STANDING-ORDERS.md: the card always names it before anything // stands). It is drawn in the person's own words and never the field's // ([standLevelWord]). + if card.item.Does.Isolate { + for _, line := range wrap("work · separate Git worktree · changes kept for review", width) { + out = append(out, a.pal.dim(line)) + } + } for _, line := range wrap(standWhereTag+standLevelWord(card.item.Level()), width) { out = append(out, a.pal.dim(line)) } diff --git a/internal/tui3/standing_test.go b/internal/tui3/standing_test.go index cfd388daf..827f91647 100644 --- a/internal/tui3/standing_test.go +++ b/internal/tui3/standing_test.go @@ -1151,3 +1151,13 @@ func TestANarrowStandingLabelDropsTheCadenceFirst(t *testing.T) { t.Fatalf("the no was rewritten: %q", got) } } + +func TestStandingCardShowsApprovedWorktreeIsolation(t *testing.T) { + a, _, tick := standApp(t) + item := standItem() + item.Does.Isolate = true + standAsk(t, a, tick, session.StandingNotice{Item: item}) + if got := standText(a); !strings.Contains(got, "separate Git worktree") { + t.Fatalf("isolation missing from card: %s", got) + } +}