diff --git a/README.md b/README.md index 82a2ad3..3265b8f 100644 --- a/README.md +++ b/README.md @@ -473,8 +473,10 @@ always enforced. **No answer is a no.** A prompt waits two minutes, with the countdown in the status line. Then the call is denied and the task goes on without it, instead of sitting on the prompt until you come back. Only an explicit yes runs -anything. The risk model learns nothing from a prompt that ran out, because -nobody judged that call. +anything. The model is told that nobody answered, not that you said no. It goes +on without the call and says at the end that the call is still waiting on you. +The risk model learns nothing from a prompt that ran out, because nobody judged +that call. **Grant it for the session.** Press **`a`** on any prompt to stop asking about that whole *kind* of action (file changes, shell, git, sub-agent spawns, MCP) for diff --git a/cmd/agent/approval_timeout_test.go b/cmd/agent/approval_timeout_test.go index 613e004..8dc483c 100644 --- a/cmd/agent/approval_timeout_test.go +++ b/cmd/agent/approval_timeout_test.go @@ -11,6 +11,9 @@ import ( "github.com/charmbracelet/bubbles/spinner" "github.com/charmbracelet/bubbles/textarea" tea "github.com/charmbracelet/bubbletea" + + "github.com/ipsupport-llc/ipsupport-code/internal/config" + "github.com/ipsupport-llc/ipsupport-code/internal/tool" ) // askThroughTheUI starts an approval on the bridge and shows it on m, the way @@ -211,3 +214,92 @@ func TestAnUnansweredPlainPromptIsANo(t *testing.T) { t.Fatal("the line typed after the expired prompt never reached the REPL — the dead prompt took it") } } + +// silentApprover is a prompt that ran out: a no that nobody gave. +type silentApprover struct{} + +func (silentApprover) Approve(ctx context.Context, _, _ string) bool { + tool.NoAnswer(ctx) + return false +} + +// The model is told a prompt ran out, not that the user said no — through the +// TUI and the plain prompt alike — and a real no is still reported as one. +func TestTheModelIsToldNobodyAnswered(t *testing.T) { + m := &tuiModel{bridge: newBridge(), input: textarea.New(), state: stRunning, spin: spinner.New()} + ask := func() <-chan string { + got := make(chan string, 1) + go func() { _, gone := tool.Ask(context.Background(), m.bridge, "run", "make deploy"); got <- gone }() + m.Update(approvalMsg(<-m.bridge.approvals)) + return got + } + got := ask() + m.approveBy = time.Now().Add(-time.Second) + m.Update(spinner.TickMsg{}) + if gone := <-got; !strings.Contains(gone, "nobody answered") { + t.Errorf("TUI timeout told the model %q, want that nobody answered", gone) + } + got = ask() + m.handleKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'n'}}) + if gone := <-got; gone != "" { + t.Errorf("an explicit no came back as %q, want the plain refusal", gone) + } + // esc is someone there, not someone away — even for a prompt asked before + // the task, which esc denies by closing its own channel, not the abort. + got = ask() + m.bridge.arm() + m.state, m.cancel = stRunning, func() {} + m.handleKey(tea.KeyMsg{Type: tea.KeyEsc}) + if gone := <-got; gone != "" { + t.Errorf("a prompt esc dropped told the model %q, want no claim the user is away", gone) + } + m.cancel, m.taskCancelled = nil, false + + defer func(d time.Duration) { approvalTimeout = d }(approvalTimeout) + approvalTimeout = 50 * time.Millisecond + pr, pw := io.Pipe() + t.Cleanup(func() { pr.Close(); pw.Close() }) + s := &stdinApprover{stdin: newStdinOwner(bufio.NewReader(pr))} + if _, gone := tool.Ask(context.Background(), s, "run", "make deploy"); !strings.Contains(gone, "nobody answered") { + t.Errorf("plain timeout told the model %q, want that nobody answered", gone) + } + + // A closed stdin: nobody can answer at all. + closed := &stdinApprover{stdin: newStdinOwner(bufio.NewReader(strings.NewReader("")))} + if _, gone := tool.Ask(context.Background(), closed, "run", "make deploy"); !strings.Contains(gone, "nobody answered") { + t.Errorf("closed stdin told the model %q, want that nobody answered", gone) + } + // Nor is a refusal typed without its newline before stdin closed. + unterminated := &stdinApprover{stdin: newStdinOwner(bufio.NewReader(strings.NewReader("n")))} + if _, gone := tool.Ask(context.Background(), unterminated, "run", "make deploy"); gone != "" { + t.Errorf("a refusal before EOF told the model %q, want no claim nobody answered", gone) + } + // A cancelled task is not told anyone was away. + cctx, cancel := context.WithCancel(context.Background()) + cancel() + if _, gone := tool.Ask(cctx, closed, "run", "make deploy"); gone != "" { + t.Errorf("a cancelled task was told %q, want nothing", gone) + } +} + +// The calls asked about outside the tools say it too: the risk check's own stop, +// a sub-agent spawn and an external agent. +func TestEveryAskerSaysNobodyAnswered(t *testing.T) { + a, pol, _ := gateApp(t, "run.default=allow") + a.approver = silentApprover{} + ctx := scored(t, a, pol, flaggedCommand) + mustFlag(t, ctx) + if msg := a.riskGate(pol)(ctx, "run", "shell", map[string]any{"command": flaggedCommand}); !strings.Contains(msg, "nobody answered") || !strings.Contains(msg, "risk check had flagged it") { + t.Errorf("risk check: %q", msg) + } + + b := &app{cfg: config.Default(), workspace: t.TempDir(), approver: silentApprover{}} + plan := spawnPlan{profile: "p", llmCfg: config.LLM{Model: "m"}, subWorkspace: b.workspace, spawnDefault: "ask"} + if _, err := b.runSpawnPlan(context.Background(), plan, "a task", nil); err == nil || !strings.Contains(err.Error(), "nobody answered") { + t.Errorf("spawn: %v", err) + } + p := config.AgentProfile{Kind: "external", Command: "codex", Args: []string{"exec", "{task}"}} + if _, err := b.spawnExternalAgent(context.Background(), "codex", p, "a task", b.workspace, nil, nil); err == nil || !strings.Contains(err.Error(), "nobody answered") { + t.Errorf("external agent: %v", err) + } +} diff --git a/cmd/agent/bridge.go b/cmd/agent/bridge.go index bc7e43b..09c1809 100644 --- a/cmd/agent/bridge.go +++ b/cmd/agent/bridge.go @@ -7,6 +7,7 @@ import ( "time" "github.com/ipsupport-llc/ipsupport-code/internal/risk" + "github.com/ipsupport-llc/ipsupport-code/internal/tool" ) // uiBridge is the seam between the agent (running in a background goroutine) and @@ -39,6 +40,9 @@ type approvalReq struct { // expire is closed by the UI when nobody answered within approvalTimeout: // the call is denied, and — like esc — that is nobody's judgement of it. expire chan struct{} + // dismiss is closed when esc drops the prompt: denied too, but someone was + // there, so the tool is not told the user is away. + dismiss chan struct{} // aborted is the task's abort signal as of the ask. Once it is closed the // tool has already been told no, so the UI does not show the prompt: an // answer to it would change nothing, and its countdown would run for a @@ -95,9 +99,9 @@ func (b *uiBridge) ApproveAnswered(ctx context.Context, kind, detail string) (ok abort := b.abort b.mu.Unlock() - reply, expire := make(chan bool, 1), make(chan struct{}) + reply, expire, dismiss := make(chan bool, 1), make(chan struct{}), make(chan struct{}) select { - case b.approvals <- approvalReq{kind: kind, detail: detail, risk: riskNote(ctx), reply: reply, expire: expire, aborted: abort}: + case b.approvals <- approvalReq{kind: kind, detail: detail, risk: riskNote(ctx), reply: reply, expire: expire, dismiss: dismiss, aborted: abort}: case <-abort: return false, false case <-ctx.Done(): @@ -107,6 +111,9 @@ func (b *uiBridge) ApproveAnswered(ctx context.Context, kind, detail string) (ok case ok := <-reply: return ok, true case <-expire: + tool.NoAnswer(ctx) + return false, false + case <-dismiss: return false, false case <-abort: return false, false diff --git a/cmd/agent/external_agent.go b/cmd/agent/external_agent.go index f9174d6..9779f38 100644 --- a/cmd/agent/external_agent.go +++ b/cmd/agent/external_agent.go @@ -13,6 +13,7 @@ import ( "github.com/ipsupport-llc/ipsupport-code/internal/config" "github.com/ipsupport-llc/ipsupport-code/internal/procgroup" "github.com/ipsupport-llc/ipsupport-code/internal/textutil" + "github.com/ipsupport-llc/ipsupport-code/internal/tool" "github.com/ipsupport-llc/ipsupport-code/internal/trace" ) @@ -97,7 +98,10 @@ func (a *app) spawnExternalAgent(ctx context.Context, profile string, p config.A // with its own safety into one without (found by review: the prompt showed // "codex" and the task, never "exec --yolo"). launch := strings.TrimSpace(command + " " + strings.Join(p.Args, " ")) - approved := a.approveGated(a.withLaunchScore(ctx, root, launch), "external agent", head+"\n launch: "+launch+"\n task: "+task) + approved, gone := tool.AskWith(a.withLaunchScore(ctx, root, launch), "external agent", head+"\n launch: "+launch+"\n task: "+task, a.approveGated) + if gone != "" { + return "", errors.New(gone) + } if !approved { return "", fmt.Errorf("external agent denied by user") } diff --git a/cmd/agent/main.go b/cmd/agent/main.go index c6b2f11..8923585 100644 --- a/cmd/agent/main.go +++ b/cmd/agent/main.go @@ -1068,7 +1068,10 @@ func (a *app) runSpawnPlan(ctx context.Context, plan spawnPlan, task string, onL if plan.outsideJail { kind, where = "agent outside workspace", where+" (outside the workspace)" } - approved := a.approveGated(ctx, kind, fmt.Sprintf("%s · %s · %s\n task: %s", plan.profile, plan.llmCfg.Model, where, task)) + approved, gone := tool.AskWith(ctx, kind, fmt.Sprintf("%s · %s · %s\n task: %s", plan.profile, plan.llmCfg.Model, where, task), a.approveGated) + if gone != "" { + return "", errors.New(gone) + } if !approved { return "", fmt.Errorf("spawn denied by user") } @@ -2757,7 +2760,10 @@ func (a *app) mcpClient(ctx context.Context, servers map[string]mcp.Server, name // TestMCPClientLockNotHeldDuringApproval. attemptCtx (rather than ctx) // carries the wait so closeMCP can cut it short on shutdown instead of // leaving the eventual connection an orphan with nothing left to close it. - approved := a.approveGated(a.withLaunchScore(attemptCtx, "", launch), "mcp launch", detail) + approved, gone := tool.AskWith(a.withLaunchScore(attemptCtx, "", launch), "mcp launch", detail, a.approveGated) + if gone != "" { + return nil, errors.New(gone) + } if !approved { return nil, fmt.Errorf("mcp server %q launch denied by user", name) } @@ -2921,18 +2927,21 @@ func mcpCallDetail(server, tool string, args map[string]any) string { // mcpCall runs an MCP tool, asking approval first (it's external code that can do // anything). -func (a *app) mcpCall(ctx context.Context, server, tool string, args map[string]any) (string, error) { +func (a *app) mcpCall(ctx context.Context, server, name string, args map[string]any) (string, error) { c, err := a.mcpClient(ctx, a.cfg.McpServers, server) if err != nil { return "", err } - approved := a.approveGated(ctx, "mcp call", mcpCallDetail(server, tool, args)) + approved, gone := tool.AskWith(ctx, "mcp call", mcpCallDetail(server, name, args), a.approveGated) + if gone != "" { + return "", errors.New(gone) + } if !approved { return "", fmt.Errorf("mcp call denied by user") } cctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() - return c.Call(cctx, tool, args) + return c.Call(cctx, name, args) } // maybeDetectWindowSync detects the window inline for the REPL/one-shot paths; @@ -6407,12 +6416,19 @@ func (s *stdinApprover) ApproveAnswered(ctx context.Context, kind, detail string select { case res = <-result: case <-wait.Done(): - if ctx.Err() == nil { - fmt.Fprintf(os.Stderr, "\n no answer in %s — denied; the task goes on without it\n", approvalWait()) - } - return false, false + res.err = wait.Err() } + // No line: the deadline passed — the read gives up with it, so this is + // reached by either branch — or stdin is closed and nobody can answer at + // all. A cancelled task is neither, and is told nothing; nor is a last + // line typed without its newline ("n" then EOF), which is someone's answer. if res.err != nil { + if ctx.Err() == nil && strings.TrimSpace(res.line) == "" { + if wait.Err() != nil { + fmt.Fprintf(os.Stderr, "\n no answer in %s — denied; the task goes on without it\n", approvalWait()) + } + tool.NoAnswer(ctx) + } return false, false } switch strings.TrimSpace(strings.ToLower(res.line)) { diff --git a/cmd/agent/risk.go b/cmd/agent/risk.go index aa727ff..11b98a5 100644 --- a/cmd/agent/risk.go +++ b/cmd/agent/risk.go @@ -11,6 +11,7 @@ import ( "github.com/ipsupport-llc/ipsupport-code/internal/policy" "github.com/ipsupport-llc/ipsupport-code/internal/risk" "github.com/ipsupport-llc/ipsupport-code/internal/shellsplit" + "github.com/ipsupport-llc/ipsupport-code/internal/tool" ) // EnvRiskOff turns risk scoring off for a run, the check included. risk_check @@ -114,9 +115,9 @@ func (a *app) riskStops(ctx context.Context, kind string) bool { // and file (see policyVerdict). A call the policy asks about gets its prompt // from the tool, with the score on it, and one it refuses never runs. func (a *app) riskGate(pol *policy.Engine) func(ctx context.Context, tool, action string, params map[string]any) string { - return func(ctx context.Context, tool, action string, params map[string]any) string { - kind, detail := tool, "" - switch tool { + return func(ctx context.Context, name, action string, params map[string]any) string { + kind, detail := name, "" + switch name { case "run": detail, _ = params["command"].(string) case "file": @@ -125,12 +126,16 @@ func (a *app) riskGate(pol *policy.Engine) func(ctx context.Context, tool, actio default: return "" } - if !a.riskStops(ctx, kind) || policyVerdict(pol, tool, action, params) != risk.VerdictAllow { + if !a.riskStops(ctx, kind) || policyVerdict(pol, name, action, params) != risk.VerdictAllow { return "" } - if a.approveGated(ctx, kind, detail) { + ok, gone := tool.AskWith(ctx, kind, detail, a.approveGated) + if ok { return "" } + if gone != "" { + return gone + " (The risk check had flagged it: " + riskNote(ctx) + ".)" + } return fmt.Sprintf("%s %s denied by user — the risk check flagged it: %s", kind, detail, riskNote(ctx)) } } diff --git a/cmd/agent/tui.go b/cmd/agent/tui.go index 5d88fea..5a6ae5f 100644 --- a/cmd/agent/tui.go +++ b/cmd/agent/tui.go @@ -1218,8 +1218,8 @@ func (m *tuiModel) handleKey(k tea.KeyMsg) (tea.Model, tea.Cmd) { // next task's approvals are still read. Deny it outright: one asked // before this task began holds an earlier abort signal, which // Abort never closes. - if m.pending.expire != nil { - close(m.pending.expire) + if m.pending.dismiss != nil { + close(m.pending.dismiss) } m.pending = nil return m, m.waitApproval() diff --git a/internal/tool/ask_test.go b/internal/tool/ask_test.go new file mode 100644 index 0000000..db08d01 --- /dev/null +++ b/internal/tool/ask_test.go @@ -0,0 +1,67 @@ +package tool + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// silent is an Approver whose prompt ran out: it says no, and that nobody said it. +type silent struct{} + +func (silent) Approve(ctx context.Context, _, _ string) bool { NoAnswer(ctx); return false } + +// A prompt that ran out used to read "denied by user", so the model took the +// user to be against the call. Every tool that asks now says what happened: +// nobody answered, and the model should go on without it. +func TestAnUnansweredPromptIsNotReportedAsTheUsersNo(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "f.txt"), []byte("old\n"), 0o644); err != nil { + t.Fatal(err) + } + repo := initRepo(t) + if err := os.WriteFile(filepath.Join(repo, "a.txt"), []byte("a\n"), 0o644); err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + name string + tl Tool + action string + params map[string]any + }{ + {"run", runToolFor(t, dir, "ask", silent{}, nil), "shell", map[string]any{"command": "echo hi"}}, + {"write", fileToolFor(t, dir, "ask", silent{}), "write", map[string]any{"path": "x.txt", "content": "x"}}, + {"edit", fileToolFor(t, dir, "ask", silent{}), "edit", map[string]any{"path": "f.txt", "find": "old", "replace": "new"}}, + {"mkdir", fileToolFor(t, dir, "ask", silent{}), "mkdir", map[string]any{"path": "sub"}}, + {"git", gitToolFor(t, repo, silent{}), "add", map[string]any{"paths": "a.txt"}}, + } { + t.Run(tc.name, func(t *testing.T) { + r := tc.tl.Call(ctx, tc.action, tc.params) + if !r.IsError || !strings.Contains(r.Content, "nobody answered") || strings.Contains(r.Content, "denied by user") { + t.Errorf("result = %+v, want it to say nobody answered — not that the user denied it", r) + } + if strings.Contains(r.Content, "git git") { + t.Errorf("result = %q names the call twice", r.Content) + } + }) + } +} + +// NoAnswer marks only a no, and only under an Ask. +func TestNoAnswerMarksOnlyANo(t *testing.T) { + saidYes := approverFunc(func(_, _ string) bool { return true }) + ok, gone := AskWith(context.Background(), "run", "ls", func(ctx context.Context, kind, detail string) bool { + NoAnswer(ctx) + return saidYes.Approve(ctx, kind, detail) + }) + if !ok || gone != "" { + t.Errorf("Ask = (%v, %q), want a yes to stay a yes", ok, gone) + } + if ok, gone := Ask(context.Background(), no(), "run", "ls"); ok || gone != "" { + t.Errorf("Ask = (%v, %q), want a plain refusal with nothing to add", ok, gone) + } + NoAnswer(context.Background()) // outside an Ask: nothing to mark, and no panic +} diff --git a/internal/tool/file.go b/internal/tool/file.go index 711034c..7a2f951 100644 --- a/internal/tool/file.go +++ b/internal/tool/file.go @@ -391,7 +391,10 @@ func (f *fileTool) writeFile(ctx context.Context, action string, a Args, appendM case policy.Deny: return Err(action + " " + path + " denied by workspace policy") case policy.Ask: - if !f.ap.Approve(ctx, action, path) { + if ok, gone := Ask(ctx, f.ap, action, path); !ok { + if gone != "" { + return Err(gone) + } return Err(action + " " + path + " denied by user") } } @@ -529,7 +532,10 @@ func (f *fileTool) edit(ctx context.Context, a Args) Result { case policy.Deny: return Err("edit " + path + " denied by workspace policy") case policy.Ask: - if !f.ap.Approve(ctx, "edit", path) { + if ok, gone := Ask(ctx, f.ap, "edit", path); !ok { + if gone != "" { + return Err(gone) + } return Err("edit " + path + " denied by user") } } @@ -668,7 +674,10 @@ func (f *fileTool) mkdir(ctx context.Context, a Args) Result { case policy.Deny: return Err("mkdir " + path + " denied by workspace policy") case policy.Ask: - if !f.ap.Approve(ctx, "mkdir", path) { + if ok, gone := Ask(ctx, f.ap, "mkdir", path); !ok { + if gone != "" { + return Err(gone) + } return Err("mkdir " + path + " denied by user") } } diff --git a/internal/tool/git.go b/internal/tool/git.go index e201474..6a12f66 100644 --- a/internal/tool/git.go +++ b/internal/tool/git.go @@ -622,8 +622,13 @@ func (g *gitTool) run(ctx context.Context, action string, mutating bool, args .. // part of the approval text — what the user is asked to approve is the command // they would have typed, not our hardening. func (g *gitTool) runWith(ctx context.Context, action string, mutating bool, timeout time.Duration, extraEnv, gitFlags []string, note string, args ...string) Result { - if mutating && !g.ap.Approve(ctx, "git", "git "+strings.Join(args, " ")+note) { - return Err("git " + action + " denied by user") + if mutating { + if ok, gone := Ask(ctx, g.ap, "git", "git "+strings.Join(args, " ")+note); !ok { + if gone != "" { + return Err(gone) + } + return Err("git " + action + " denied by user") + } } dir, err := g.pol.Resolve(".") if err != nil { diff --git a/internal/tool/run.go b/internal/tool/run.go index 4ac9ca1..0d89e29 100644 --- a/internal/tool/run.go +++ b/internal/tool/run.go @@ -98,7 +98,10 @@ func (r *runTool) shell(ctx context.Context, a Args) Result { case policy.Deny: return Err("command denied by workspace policy: " + command) case policy.Ask: - if !r.ap.Approve(ctx, "run", command) { + if ok, gone := Ask(ctx, r.ap, "run", command); !ok { + if gone != "" { + return Err(gone) + } return Err("command denied by user: " + command) } } diff --git a/internal/tool/tool.go b/internal/tool/tool.go index 85456eb..ac2dc1c 100644 --- a/internal/tool/tool.go +++ b/internal/tool/tool.go @@ -9,6 +9,7 @@ import ( "fmt" "strconv" "strings" + "sync/atomic" ) // Result is a tool's outcome as the model sees it. Content is always model- @@ -58,6 +59,45 @@ type Approver interface { Approve(ctx context.Context, kind, detail string) bool } +type noAnswerKey struct{} + +// NoAnswer is how an Approver marks the no it is about to return as nobody's: +// the prompt ran out unanswered. A no under an Ask is otherwise reported as +// the user's. +func NoAnswer(ctx context.Context) { + if gone, ok := ctx.Value(noAnswerKey{}).(*atomic.Bool); ok { + gone.Store(true) + } +} + +// Ask is ap.Approve, plus — when the no was nobody's answer — what the call +// reports instead of "denied by user", or "" when someone did answer. +func Ask(ctx context.Context, ap Approver, kind, detail string) (ok bool, unanswered string) { + return AskWith(ctx, kind, detail, ap.Approve) +} + +// AskWith is Ask for an approval asked some other way. +// +// A refusal and a prompt that ran out read alike to the model as "denied by +// user", so it took the user to be against the call — dropping it, or working +// around a no nobody gave. What it needs instead is that the user is away: go on +// without it, and leave it for them. +func AskWith(ctx context.Context, kind, detail string, approve func(ctx context.Context, kind, detail string) bool) (ok bool, unanswered string) { + var gone atomic.Bool + if approve(context.WithValue(ctx, noAnswerKey{}, &gone), kind, detail) { + return true, "" + } + if !gone.Load() { + return false, "" + } + what, _, _ := strings.Cut(detail, "\n") + if !strings.HasPrefix(what, kind+" ") { // git's detail already opens "git …" + what = kind + " " + what + } + return false, what + " was not run: nobody answered the approval prompt in time — the user may be away. " + + "Don't ask for it again now; carry on with whatever doesn't need it, and when you finish, say it is still waiting on the user." +} + // Tool is one fat domain tool. type Tool interface { Name() string