diff --git a/README.md b/README.md index 2b8b384..6e9531a 100644 --- a/README.md +++ b/README.md @@ -198,6 +198,8 @@ status inferred from output and no sibling awareness. | `note` | Append to the project's shared log | | `notes` | Read what other agents recorded | | `work` | Read what another session has changed — summary and patch | +| `analyse` | Have a separate agent review a session's work; returns an id | +| `analysis` | Collect that review, with what it cost | | `message` | Send to one sibling by name, or to all of them | | `inbox` | Collect messages sent to you | @@ -212,7 +214,24 @@ and submitting on its behalf would let one agent put instructions into another's prompt with nobody watching. The cost of pulling is that the recipient has to ask, so every other tool result carries an unread count. -The sidebar shows `⊙ n` for claims held and `✉ n` for messages waiting. +An agent can also read a sibling's work directly, and have it reviewed. `work` +returns the summary and patch of another session's worktree, which needs +nothing from that session — it is read while that agent is mid-turn and +interrupts nothing. `analyse` goes further and starts a **separate agent** to +review it. + +That review is asynchronous, because a real one outlasts the tool timeout of +whatever asked for it, and because a blocking call is invisible exactly while +you would want to watch it. The reviewer is handed the diff, given no tools, +and run in a mode that answers but cannot act. `analysis` collects the answer +along with what it cost. + +The sidebar shows `⊙ n` for claims held, `✉ n` for messages waiting, and +`⚗ n · $x.xx` for reviews running and what they have cost. That last one is +the only thing in Deck that spends money with nobody watching it: the review +has no pane, and the session that asked for it has moved on. The figure stays +after the last review finishes, so a total is not lost the moment it stops +moving. ## Status diff --git a/docs/architecture.md b/docs/architecture.md index eed0278..75c80ca 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -46,6 +46,7 @@ internal/agent PTY + vt emulator per session <- the subtle package render.go the cell walk status.go working / idle / exited, and how quiet env.go what a hosted agent must not inherit + headless.go one turn with no pane, and what it cost internal/coord cross-session coordination, exposed to agents over MCP: coord.go types + session lifecycle claims.go soft locks and who holds what @@ -54,7 +55,9 @@ internal/coord cross-session coordination, exposed to agents over MCP: status.go turn state reported by hooks mcp.go the listener and the hook endpoint rpc.go JSON-RPC envelopes and framing - tools.go the tool schemas and dispatch + tools.go the tool schemas; dispatch.go answers a call + work.go reading a sibling's changes + analyse.go starting a spawned review; jobs.go records it results.go tool results and the unread-mail hint internal/ui the Bubble Tea program, split by job: model.go the model and its builders @@ -73,6 +76,8 @@ internal/ui the Bubble Tea program, split by job: browser.go the directory explorer; dirlist.go lists it picker.go the list modal; picker_open.go opens it theme.go + palette.go / styles.go +internal/gittest throwaway git repositories for tests, in one place because + four packages had grown their own and they had drifted probe/ debug harness: renders frames without a human present ``` @@ -345,6 +350,47 @@ with anything else lying around in the tree. later, for the reason in (1): by the time anyone asks, the branch it came from has moved. +### A spawned review (`coord.Analyse`, `agent.RunClaude`) + +`analyse` starts a **separate agent** to review a sibling's work. Four +decisions shape it, and each was reached by measuring rather than by argument. + +**Asynchronous, because the timeout is not ours.** A synchronous tool call +blocks the *caller's* tool timeout, which belongs to the calling agent rather +than to Deck — `mcp.go` sets only a `ReadHeaderTimeout` on request headers, so +nothing here would cut a long review off. A review that outruns the caller +returns nothing and has already spent the money. `Analyse` hands back a handle +and `Analysis` collects it. + +**The reviewer is handed its evidence and given nothing else.** It receives the +diff in its prompt and no coordination config, because it has nothing to ask +anyone; wiring it to this server would be surface with no caller. It runs under +a permission mode that answers a question but refuses to act, so a review +cannot become an edit. Everything it needs must therefore be in the prompt, +which is what `TestTheQuestionReachesTheReviewer` pins. + +**A failed turn is a result, not an error.** `agent.ClaudeRun` carries +`Failure` as a field rather than returning a Go error, because a turn that ran +and refused still spent money and Go's convention tells a caller to discard the +value alongside an error. That would put the bill out of reach in the one case +where it is surprising. An `error` from `RunClaude` means the opposite: no +envelope came back, so there is no accounting to add and inventing a figure +would be worse than the gap. Do not "tidy" this into a plain error return. + +**Cost is per session, and never written to disk.** A per-run figure alone is +hard to read — the same short turn was measured at $0.012 and $0.237 depending +on whether its context was read from cache or written to it — so the running +total is what makes a pattern visible. It is dropped with the session, like +claims and the inbox, because a review belongs to the session that paid for it. + +Jobs are bounded like the inbox and the log. Dropping the oldest is safe: +`Spend` is a running total kept separately, so a discarded record costs the +reader an old answer and never the bill. + +`Close` cancels every run the coordinator started. Without that, quitting Deck +mid-review left an agent running and billing with no surface left to show it +on, which is the opposite of what the cost reporting exists for. + ### The store is global, not per-directory (`main.registerCwd`) One `state.json` holds every project and session, so all of them are reachable diff --git a/internal/agent/headless.go b/internal/agent/headless.go new file mode 100644 index 0000000..9a2d1db --- /dev/null +++ b/internal/agent/headless.go @@ -0,0 +1,132 @@ +package agent + +// A headless agent run: one turn, no pseudo-terminal, and a structured report +// of what it cost. Used for work Deck starts on an agent's behalf rather than +// on a person's, where nobody is watching a pane. + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "os/exec" + "strings" + "time" +) + +// Tokens is the usage a run reported, split the way billing splits it. +// +// Cache writes and reads are separated because they price differently and are +// the largest single influence on what a short run costs: the same one-word +// reply measured at $0.012 when its context was read from cache and $0.237 +// when the same context was written to it. +type Tokens struct { + Input int `json:"input_tokens"` + Output int `json:"output_tokens"` + CacheRead int `json:"cache_read_input_tokens"` + CacheWrite int `json:"cache_creation_input_tokens"` +} + +// ClaudeRun is what one headless turn reported. +// +// A turn that ran and failed is a result, not an error: it has a cost, and +// that cost is the one most worth noticing. So Failure is a field rather than +// a returned error. RunClaude returns an error only when there is no +// accounting at all — the process would not start, or produced nothing we can +// read — because Go's convention tells a caller to discard the value alongside +// an error, which would put the bill out of reach exactly when it is +// surprising. +type ClaudeRun struct { + Text string // the model's final answer, empty when it failed + Failure string // why the turn produced no answer; empty on success + CostUSD float64 // what the turn cost, whether or not it answered + Tokens Tokens // usage, split by kind + Took time.Duration // how long the turn took, as the CLI measured it +} + +// Failed reports whether the turn ran without producing an answer. +func (r ClaudeRun) Failed() bool { return r.Failure != "" } + +// resultEnvelope is the one event in the stream that carries the totals. The +// field names are claude's; another agent would need its own parser, which is +// why this function is named for the one it understands. +type resultEnvelope struct { + Type string `json:"type"` + Subtype string `json:"subtype"` + IsError bool `json:"is_error"` + Result string `json:"result"` + DurationMS int `json:"duration_ms"` + CostUSD float64 `json:"total_cost_usd"` + Usage Tokens `json:"usage"` +} + +// RunClaude runs one non-interactive turn in dir and reports what it cost, +// with the answer when there is one. +// +// A turn that ends in a refusal or an API error comes back as a ClaudeRun with +// Failure set and its cost intact, not as an error. See ClaudeRun. +// +// The prompt goes over stdin rather than as an argument: with stdin empty +// claude reports "input must be provided" and ignores a positional prompt. +// +// The environment is scrubbed exactly as an interactive session's is, so a +// spawned run cannot inherit credentials or the child-session marker from +// whatever started Deck. +func RunClaude(ctx context.Context, dir, prompt string, args ...string) (ClaudeRun, error) { + full := append([]string{"-p", "--output-format", "json"}, args...) + cmd := exec.CommandContext(ctx, "claude", full...) + cmd.Dir = dir + cmd.Env = ScrubbedEnv() + cmd.Stdin = strings.NewReader(prompt) + + out, err := cmd.Output() + if err != nil { + // The CLI reports its own diagnosis on stderr; a bare "exit status 1" + // tells the caller nothing about whether it was auth, a bad flag or a + // refusal. + var ee *exec.ExitError + if errors.As(err, &ee) && len(ee.Stderr) > 0 { + return ClaudeRun{}, fmt.Errorf("claude: %s", strings.TrimSpace(string(ee.Stderr))) + } + return ClaudeRun{}, fmt.Errorf("claude: %w", err) + } + return parseClaudeJSON(out) +} + +// parseClaudeJSON pulls the totals out of a --output-format json stream. +// +// The stream is an array of events, not a single object: an init event, the +// assistant turns, and one result event carrying the totals. Reading the last +// element would work today and break the moment anything is appended after +// it, so this selects by type. +func parseClaudeJSON(out []byte) (ClaudeRun, error) { + var events []resultEnvelope + if err := json.Unmarshal(out, &events); err != nil { + // A single object rather than an array is also valid JSON output; try + // it before giving up, so a change of shape degrades to one parse + // failure rather than to a wrong answer. + var one resultEnvelope + if json.Unmarshal(out, &one) != nil { + return ClaudeRun{}, fmt.Errorf("claude produced no readable result: %w", err) + } + events = []resultEnvelope{one} + } + for _, e := range events { + if e.Type != "result" { + continue + } + run := ClaudeRun{ + Text: e.Result, + CostUSD: e.CostUSD, + Tokens: e.Usage, + Took: time.Duration(e.DurationMS) * time.Millisecond, + } + if e.IsError { + run.Text = "" + run.Failure = strings.TrimSpace(e.Subtype + ": " + e.Result) + } + return run, nil + } + // No result event means no accounting: whatever it spent, we cannot say. + return ClaudeRun{}, fmt.Errorf("claude produced no result event") +} diff --git a/internal/agent/headless_test.go b/internal/agent/headless_test.go new file mode 100644 index 0000000..1fcf2da --- /dev/null +++ b/internal/agent/headless_test.go @@ -0,0 +1,96 @@ +package agent + +import ( + "strings" + "testing" + "time" +) + +// A captured stream, trimmed to the events that matter. The shape is the one +// `claude -p --output-format json` actually emits: an array, with the totals +// in a result event rather than in the last element by position. +const captured = `[ + {"type":"system","subtype":"init","session_id":"abc"}, + {"type":"assistant","message":{"role":"assistant"}}, + {"type":"result","subtype":"success","is_error":false,"result":"ZEPHYR_QUOTA_GUARD", + "duration_ms":1624,"num_turns":1,"total_cost_usd":0.23709, + "usage":{"input_tokens":2,"output_tokens":4, + "cache_read_input_tokens":0,"cache_creation_input_tokens":23698}}, + {"type":"trailing_event_added_later"} +]` + +// TestParseClaudeJSONSelectsByType is why the parser does not read the last +// element. A stream with anything appended after the result would otherwise +// report zero cost and no answer. +func TestParseClaudeJSONSelectsByType(t *testing.T) { + run, err := parseClaudeJSON([]byte(captured)) + if err != nil { + t.Fatal(err) + } + if run.Text != "ZEPHYR_QUOTA_GUARD" { + t.Errorf("text = %q", run.Text) + } + if run.CostUSD != 0.23709 { + t.Errorf("cost = %v, want 0.23709", run.CostUSD) + } + if run.Took != 1624*time.Millisecond { + t.Errorf("took = %v, want the duration the CLI reported", run.Took) + } +} + +// TestParseClaudeJSONSplitsCacheTokens covers the split that explains a bill. +// Reads and writes of the same context price differently, so collapsing them +// into one number would hide the largest influence on a short run's cost. +func TestParseClaudeJSONSplitsCacheTokens(t *testing.T) { + run, err := parseClaudeJSON([]byte(captured)) + if err != nil { + t.Fatal(err) + } + want := Tokens{Input: 2, Output: 4, CacheRead: 0, CacheWrite: 23698} + if run.Tokens != want { + t.Errorf("tokens = %+v, want %+v", run.Tokens, want) + } +} + +// TestFailedTurnKeepsItsCost is the property the whole shape exists for. A +// turn that ran and refused still spent money, and returning it as a Go error +// would tell every caller to discard the value — putting the bill out of reach +// in the one case where it is surprising. +func TestFailedTurnKeepsItsCost(t *testing.T) { + stream := `[{"type":"result","subtype":"error_max_turns","is_error":true, + "result":"ran out of turns","total_cost_usd":0.4, + "usage":{"input_tokens":7,"output_tokens":0}}]` + run, err := parseClaudeJSON([]byte(stream)) + if err != nil { + t.Fatalf("a turn that ran was reported as unusable: %v", err) + } + if !run.Failed() { + t.Error("a refused turn does not report itself as failed") + } + if run.CostUSD != 0.4 { + t.Errorf("cost = %v, want 0.4 — a failed run that spent was not counted", run.CostUSD) + } + if run.Tokens.Input != 7 { + t.Errorf("tokens = %+v, want the usage the envelope reported", run.Tokens) + } + if run.Text != "" { + t.Errorf("text = %q; a failed turn has no answer to give", run.Text) + } + for _, want := range []string{"error_max_turns", "ran out of turns"} { + if !strings.Contains(run.Failure, want) { + t.Errorf("failure %q does not mention %q", run.Failure, want) + } + } +} + +// TestParseClaudeJSONReportsAMissingResult covers a stream that ends without +// totals — a crash mid-run. Returning a zero-cost success would under-report +// the bill and hand the caller an empty answer as if it were real. +func TestParseClaudeJSONReportsAMissingResult(t *testing.T) { + if _, err := parseClaudeJSON([]byte(`[{"type":"system","subtype":"init"}]`)); err == nil { + t.Error("a stream with no result event parsed as a success") + } + if _, err := parseClaudeJSON([]byte(`not json at all`)); err == nil { + t.Error("unparseable output was accepted") + } +} diff --git a/internal/coord/analyse.go b/internal/coord/analyse.go new file mode 100644 index 0000000..1ef91b6 --- /dev/null +++ b/internal/coord/analyse.go @@ -0,0 +1,114 @@ +package coord + +// Starting a spawned analysis: what the reviewer is told, and how its run is +// bounded. The record it produces is in jobs.go. +// +// The caller does not wait. A review of a real diff outlasts the tool timeout +// of whatever asked for it, and that timeout belongs to the calling agent +// rather than to us, so a synchronous answer would be spent money nobody +// receives. Analyse returns a handle and Analysis collects it later. + +import ( + "context" + "fmt" + "strings" + "time" +) + +// jobTimeout bounds a spawned run. Long enough for a review of a substantial +// diff, short enough that a wedged process is not still billing an hour later. +const jobTimeout = 10 * time.Minute + +// reviewPrompt is what the spawned agent is given. It receives the diff in the +// prompt and no tools at all, so everything it needs to answer has to be here. +const reviewPrompt = `You are reviewing work done by another agent on this project. +You cannot run anything or read any file: the change is reproduced in full below. + +Session: %s — %s +Measured from: %s + +Files changed: +%s + +Patch: +%s + +%s` + +// defaultQuestion is used when the caller asks for a review without saying +// what it wants to know. +const defaultQuestion = "What is wrong, risky or incomplete in this change? " + + "Be specific and cite the lines you mean. If it looks sound, say so briefly." + +// Analyse spawns an agent to review a sibling's work and returns immediately +// with a handle. +// +// The reviewer is handed the diff and given no coordination tools: it has +// nothing to ask anyone, so wiring it to this server would be surface with no +// caller. It runs in plan mode, which answers a question but refuses to act, +// so a review cannot become an edit. +func (c *Coordinator) Analyse(sessionID, target, question string) (*Job, error) { + found, w, err := c.workOf(sessionID, target) + if err != nil { + return nil, err + } + if strings.TrimSpace(question) == "" { + question = defaultQuestion + } + prompt := fmt.Sprintf(reviewPrompt, + found.Name, found.Title, w.Base, w.Stat, w.Patch, question) + + job := &Job{ + ID: newJobID(), From: sessionID, Subject: found.Name, + State: JobRunning, Started: time.Now(), + } + c.mu.Lock() + c.jobs[job.ID] = job + c.trimJobs(sessionID) + dir := found.Dir + c.mu.Unlock() + + go c.runAnalysis(job, dir, prompt) + + // A copy, not the live record. The mutex guards the coordinator's own + // access to a job, not the caller's: handing back the pointer lets the + // caller read fields the spawned goroutine is still writing. + c.mu.Lock() + copied := *job + c.mu.Unlock() + return &copied, nil +} + +// runAnalysis performs the spawned turn and records what it cost. +// +// A turn that ran and refused is a result carrying its own cost, so the total +// counts it. An error here means the opposite: no envelope came back, so there +// is no accounting to add and the job records why rather than inventing a +// figure. +func (c *Coordinator) runAnalysis(job *Job, dir, prompt string) { + // Derived from the coordinator's life, so Close stops this run. The + // timeout then bounds a run within that lifetime rather than standing in + // for one. + ctx, cancel := context.WithTimeout(c.life, jobTimeout) + defer cancel() + + run, err := c.spawn(ctx, dir, prompt) + + c.mu.Lock() + defer c.mu.Unlock() + if err != nil { + // No envelope came back, so the CLI reported no duration either; our + // own measurement is all there is. + job.Elapsed = time.Since(job.Started) + job.State, job.Err = JobFailed, err.Error() + return + } + job.Elapsed = run.Took + job.Cost, job.Tokens = run.CostUSD, run.Tokens + c.spend[job.From] += run.CostUSD + if run.Failed() { + job.State, job.Err = JobFailed, run.Failure + return + } + job.State, job.Answer = JobDone, run.Text +} diff --git a/internal/coord/coord.go b/internal/coord/coord.go index a4f6905..2095bc6 100644 --- a/internal/coord/coord.go +++ b/internal/coord/coord.go @@ -12,7 +12,9 @@ package coord import ( + "context" "fmt" + "github.com/tripledownab/deck/internal/agent" "os" "sync" ) @@ -50,22 +52,65 @@ type Coordinator struct { // registered, and losing one to a race would leave a stale dot. status *statusBoard + // jobs are spawned analyses, and spend is what each session's have cost + // it. Both are dropped with the session, like claims and the inbox: a + // review belongs to the session that paid for it. + jobs map[string]*Job + spend map[string]float64 + + // spawn performs one headless turn. A field rather than a direct call + // because otherwise `go test` invokes the paid CLI on every run, and CI — + // where claude is not installed — reports green on a spawn that failed. + spawn Reviewer + + // life bounds everything the coordinator started. Close cancels it, so a + // spawned analysis cannot outlive the app that asked for it: without this + // quitting Deck mid-review left a claude process running, and billing, + // with no surface left to show it on. + life context.Context + endOfLife context.CancelFunc + notesDir string server *server } +// Reviewer performs one spawned analysis. Deck uses claude; a caller may +// substitute another, and the tests do so the suite neither spends money nor +// needs the CLI installed. +type Reviewer func(ctx context.Context, dir, prompt string) (agent.ClaudeRun, error) + +// Option configures a Coordinator at startup. +type Option func(*Coordinator) + +// WithReviewer replaces who performs a spawned analysis. +func WithReviewer(r Reviewer) Option { + return func(c *Coordinator) { c.spawn = r } +} + // Start brings up the coordinator and its MCP endpoint on localhost. -func Start(notesDir string) (*Coordinator, error) { +func Start(notesDir string, opts ...Option) (*Coordinator, error) { if err := os.MkdirAll(notesDir, 0o755); err != nil { return nil, fmt.Errorf("create notes dir: %w", err) } + life, endOfLife := context.WithCancel(context.Background()) c := &Coordinator{ + life: life, + endOfLife: endOfLife, sessions: map[string]Session{}, claims: map[string][]Claim{}, inbox: map[string][]Message{}, noteLines: map[string]int{}, - status: newStatusBoard(), - notesDir: notesDir, + jobs: map[string]*Job{}, + spend: map[string]float64{}, + spawn: func(ctx context.Context, dir, prompt string) (agent.ClaudeRun, error) { + return agent.RunClaude(ctx, dir, prompt, "--permission-mode", "plan") + }, + status: newStatusBoard(), + notesDir: notesDir, + } + // Options after the defaults, so a caller replaces rather than races them. + for _, opt := range opts { + opt(c) } srv, err := newServer(c) if err != nil { @@ -76,7 +121,15 @@ func Start(notesDir string) (*Coordinator, error) { } // Close stops the MCP endpoint. +// Close stops the endpoint and everything the coordinator started. +// +// Cancelling first: a spawned analysis holds no lock and needs none to stop, +// and closing the listener first would leave those runs alive for as long as +// the shutdown takes. func (c *Coordinator) Close() error { + if c.endOfLife != nil { + c.endOfLife() + } if c.server == nil { return nil } @@ -113,6 +166,12 @@ func (c *Coordinator) Unregister(id string) { delete(c.sessions, id) delete(c.claims, id) delete(c.inbox, id) + delete(c.spend, id) + for jid, j := range c.jobs { + if j.From == id { + delete(c.jobs, jid) + } + } c.status.clear(id) } diff --git a/internal/coord/coord_test.go b/internal/coord/coord_test.go index 2c4cd41..77ad803 100644 --- a/internal/coord/coord_test.go +++ b/internal/coord/coord_test.go @@ -219,7 +219,8 @@ func TestMCPHandshakeAndTools(t *testing.T) { list := rpc(t, c, "s1", "tools/list", nil) raw, _ := json.Marshal(list.Result) - for _, want := range []string{"sessions", "claim", "release", "note", "notes", "message", "inbox", "work"} { + for _, want := range []string{"sessions", "claim", "release", "note", "notes", + "message", "inbox", "work", "analyse", "analysis"} { if !strings.Contains(string(raw), `"`+want+`"`) { t.Errorf("tools/list is missing %q", want) } diff --git a/internal/coord/dispatch.go b/internal/coord/dispatch.go new file mode 100644 index 0000000..88827b7 --- /dev/null +++ b/internal/coord/dispatch.go @@ -0,0 +1,143 @@ +package coord + +// Answering a tool call: the arguments every tool can take, and what each one +// does. The list agents see is in tools.go. + +import ( + "encoding/json" + "fmt" + "time" +) + +type toolCall struct { + Name string `json:"name"` + Arguments struct { + Paths []string `json:"paths"` + Reason string `json:"reason"` + Text string `json:"text"` + To string `json:"to"` + Session string `json:"session"` + Question string `json:"question"` + ID string `json:"id"` + } `json:"arguments"` +} + +func (s *server) callTool(sessionID string, params json.RawMessage) map[string]any { + var p toolCall + if err := json.Unmarshal(params, &p); err != nil { + return errorResult("bad arguments: " + err.Error()) + } + + res := s.dispatch(sessionID, p) + + // Mail is pull-only, so a recipient would otherwise never learn it had + // any. Every result except the inbox's own carries the count, which makes + // any coordination call the moment a message surfaces. + if p.Name != "inbox" { + if n := s.c.Unread(sessionID); n > 0 { + res = withHint(res, fmt.Sprintf( + "\n\n(%d message(s) waiting from other agents — call the inbox tool.)", n)) + } + } + return res +} + +func (s *server) dispatch(sessionID string, p toolCall) map[string]any { + switch p.Name { + case "sessions": + siblings := s.c.Siblings(sessionID) + if len(siblings) == 0 { + return textResult("No other agents are working on this project.") + } + return jsonResult(map[string]any{"sessions": siblings}) + + case "claim": + granted, conflicts := s.c.Claim(sessionID, p.Arguments.Paths, p.Arguments.Reason) + out := map[string]any{"claimed": granted} + if len(conflicts) > 0 { + out["conflicts"] = conflicts + out["advice"] = "Another agent is already working on the conflicting paths. " + + "Pick different work, or leave a note explaining the overlap." + } + return jsonResult(out) + + case "work": + w, err := s.c.Work(sessionID, p.Arguments.Session) + if err != nil { + return errorResult(err.Error()) + } + return jsonResult(w) + + case "analyse": + job, err := s.c.Analyse(sessionID, p.Arguments.Session, p.Arguments.Question) + if err != nil { + return errorResult(err.Error()) + } + return jsonResult(map[string]any{ + "id": job.ID, + "session": job.Subject, + "state": job.State.String(), + "advice": "Reviewing takes a minute or two. Get on with something else " + + "and collect it with the analysis tool.", + }) + + case "analysis": + job, err := s.c.Analysis(sessionID, p.Arguments.ID) + if err != nil { + return errorResult(err.Error()) + } + out := map[string]any{"id": job.ID, "session": job.Subject, "state": job.State.String()} + switch job.State { + case JobRunning: + out["running_for"] = job.Elapsed.Round(time.Second).String() + case JobFailed: + out["error"] = job.Err + default: + out["answer"] = job.Answer + } + if job.State != JobRunning { + out["cost_usd"] = job.Cost + out["tokens"] = job.Tokens + out["took"] = job.Elapsed.Round(time.Second).String() + out["session_spend_usd"] = s.c.Spend(sessionID) + } + return jsonResult(out) + + case "release": + return jsonResult(map[string]any{"released": s.c.Release(sessionID, p.Arguments.Paths)}) + + case "note": + if err := s.c.AppendNote(sessionID, p.Arguments.Text); err != nil { + return errorResult(err.Error()) + } + return textResult("Noted.") + + case "notes": + notes, err := s.c.Notes(sessionID) + if err != nil { + return errorResult(err.Error()) + } + if len(notes) == 0 { + return textResult("No shared notes yet for this project.") + } + return jsonResult(map[string]any{"notes": notes}) + + case "message": + sent, err := s.c.Send(sessionID, p.Arguments.To, p.Arguments.Text) + if err != nil { + return errorResult(err.Error()) + } + if len(sent) == 0 { + return textResult("No other agents are on this project, so nobody received it.") + } + return jsonResult(map[string]any{"delivered_to": sent}) + + case "inbox": + msgs := s.c.Collect(sessionID) + if len(msgs) == 0 { + return textResult("No messages.") + } + return jsonResult(map[string]any{"messages": msgs}) + } + return errorResult("unknown tool: " + p.Name) +} diff --git a/internal/coord/jobs.go b/internal/coord/jobs.go new file mode 100644 index 0000000..ceee1d9 --- /dev/null +++ b/internal/coord/jobs.go @@ -0,0 +1,141 @@ +package coord + +// The record of a spawned analysis: what it is, what it cost, and reading it +// back. Starting one is in analyse.go. + +import ( + "fmt" + "sort" + "sync" + "time" + + "github.com/tripledownab/deck/internal/agent" +) + +// JobState is how far a spawned analysis has got. +type JobState int + +const ( + JobRunning JobState = iota + JobDone + JobFailed +) + +func (s JobState) String() string { + switch s { + case JobDone: + return "done" + case JobFailed: + return "failed" + default: + return "running" + } +} + +// maxJobs bounds a session's analyses, the way maxInbox bounds its mail and +// maxNotes bounds the log. Each record holds a full review, so a session that +// keeps asking would otherwise grow without limit. Dropping the oldest is +// safe: Spend is a running total kept separately, so a discarded record costs +// the reader an old answer, never the bill. +const maxJobs = 50 + +// Job is one spawned analysis and what it cost. +type Job struct { + ID string + From string // session id that asked + Subject string // session name analysed + State JobState + Started time.Time + + // Elapsed is how long the caller has waited while running, and what the + // turn itself took once finished. One field, because a reader wants "how + // long" in both cases and two clocks for one quantity invite a silent + // change of meaning. + Elapsed time.Duration + + Answer string + Cost float64 + Tokens agent.Tokens + Err string +} + +// Analysis returns a job the caller started. Jobs are private to the session +// that asked: a review is work someone paid for, and the answer is theirs. +func (c *Coordinator) Analysis(sessionID, jobID string) (*Job, error) { + c.mu.Lock() + defer c.mu.Unlock() + job, ok := c.jobs[jobID] + if !ok || job.From != sessionID { + return nil, fmt.Errorf("no analysis %q started by this session", jobID) + } + copied := *job + // Elapsed is written when a run finishes, so a job still in flight would + // otherwise report zero — the one case where the caller most wants to know + // how long it has been waiting. + if copied.State == JobRunning { + copied.Elapsed = time.Since(copied.Started) + } + return &copied, nil +} + +// Spend is what a session's spawned analyses have cost it so far. +// +// Per session rather than per project, and never written to disk. A per-run +// figure alone is hard to read — the same short turn measured at $0.012 and +// $0.237 depending on whether its context was read from cache or written to +// it — so the running total is what makes a pattern visible. +func (c *Coordinator) Spend(sessionID string) float64 { + c.mu.Lock() + defer c.mu.Unlock() + return c.spend[sessionID] +} + +// trimJobs drops a session's oldest analyses past maxJobs. Caller holds the +// lock. +func (c *Coordinator) trimJobs(sessionID string) { + mine := make([]*Job, 0, len(c.jobs)) + for _, j := range c.jobs { + if j.From == sessionID { + mine = append(mine, j) + } + } + if len(mine) <= maxJobs { + return + } + sort.Slice(mine, func(i, j int) bool { return mine[i].Started.Before(mine[j].Started) }) + for _, j := range mine[:len(mine)-maxJobs] { + delete(c.jobs, j.ID) + } +} + +// jobSeq numbers analyses. Sequential rather than random: an agent reads an id +// back to us, and a short one it can retype is worth more than an unguessable +// one for something scoped to a single session anyway. +var jobSeq struct { + sync.Mutex + n int +} + +func newJobID() string { + jobSeq.Lock() + defer jobSeq.Unlock() + jobSeq.n++ + return fmt.Sprintf("a%d", jobSeq.n) +} + +// Analyses is what a session's spawned reviews cost and how many are still +// running, for the sidebar badge. +// +// Two numbers rather than the records themselves: the sidebar has room for a +// glyph and a figure, and returning fifty full reviews so the caller can count +// them would hand a renderer the whole answer text on every frame. +func (c *Coordinator) Analyses(sessionID string) (running int, spent float64) { + c.mu.Lock() + defer c.mu.Unlock() + for _, j := range c.jobs { + if j.From == sessionID && j.State == JobRunning { + running++ + } + } + return running, c.spend[sessionID] +} diff --git a/internal/coord/jobs_test.go b/internal/coord/jobs_test.go new file mode 100644 index 0000000..d8b5b6d --- /dev/null +++ b/internal/coord/jobs_test.go @@ -0,0 +1,484 @@ +package coord + +import ( + "context" + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/tripledownab/deck/internal/agent" +) + +// analysable registers a reader and a sibling with real, uncommitted work, and +// substitutes the spawn so the suite neither spends money nor needs the CLI. +// +// run is what a spawned review will report. Without this the tests invoke the +// real claude: a probe measured one at $0.106, and in CI, where it is not +// installed, the spawn would fail while the tests still passed. +func analysable(t *testing.T, run agent.ClaudeRun, spawnErr error) *Coordinator { + t.Helper() + return analysableWatching(t, run, spawnErr, nil) +} + +// analysableWatching is analysable, recording the prompt each spawn is given. +// The prompt is the only place a caller's question becomes visible, so without +// this nothing proves the question reaches the reviewer at all. +func analysableWatching(t *testing.T, run agent.ClaudeRun, spawnErr error, seen *[]string) *Coordinator { + t.Helper() + dir, head := worktreeSession(t) + if err := os.WriteFile(filepath.Join(dir, "auth.go"), + []byte("package gateway\n\nfunc Auth() {}\n"), 0o644); err != nil { + t.Fatal(err) + } + var mu sync.Mutex + c := startWith(t, func(_ context.Context, _, prompt string) (agent.ClaudeRun, error) { + if seen != nil { + mu.Lock() + *seen = append(*seen, prompt) + mu.Unlock() + } + return run, spawnErr + }) + c.Register(Session{ID: "me", ProjectID: "p1", Name: "scheming-hawk-jhgk", Dir: t.TempDir()}) + c.Register(Session{ID: "them", ProjectID: "p1", Name: "wily-crane-bbbb", + Title: "split the auth middleware", Dir: dir, + Branch: "session/wily-crane-bbbb", Isolated: true, BaseRef: head}) + return c +} + +// TestAnalyseRefusesTheSameThingsWorkDoes is why both go through workOf. The +// scoping and the two refusals are stated once; if they drifted, a spawned run +// could read a session the work tool would not show. +func TestAnalyseRefusesTheSameThingsWorkDoes(t *testing.T) { + c := analysable(t, agent.ClaudeRun{}, nil) + c.Register(Session{ID: "shared", ProjectID: "p1", Name: "brisk-heron-cccc", + Dir: t.TempDir(), Isolated: false}) + c.Register(Session{ID: "far", ProjectID: "OTHER", Name: "quiet-lynx-dddd", + Dir: t.TempDir(), Isolated: true, BaseRef: "abc"}) + + for _, tc := range []struct{ target, want string }{ + {"brisk-heron-cccc", "project directory"}, + {"quiet-lynx-dddd", "no live session"}, + {"not-a-session", "no live session"}, + } { + _, err := c.Analyse("me", tc.target, "") + if err == nil { + t.Errorf("%s: spawned a run that work would have refused", tc.target) + continue + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("%s: error %q does not mention %q", tc.target, err, tc.want) + } + } +} + +// TestAnalyseReturnsBeforeItFinishes is the whole point of the handle. A +// synchronous answer would outlast the calling agent's tool timeout, which +// belongs to that agent rather than to us. +func TestAnalyseReturnsBeforeItFinishes(t *testing.T) { + c := analysable(t, agent.ClaudeRun{}, nil) + + start := time.Now() + job, err := c.Analyse("me", "wily-crane-bbbb", "") + if err != nil { + t.Fatal(err) + } + if took := time.Since(start); took > 2*time.Second { + t.Errorf("Analyse blocked for %v; it must hand back a handle", took) + } + if job.State != JobRunning { + t.Errorf("state = %v, want running", job.State) + } + if job.Subject != "wily-crane-bbbb" { + t.Errorf("subject = %q", job.Subject) + } +} + +// TestAnalysisIsPrivateToTheSessionThatPaid keeps one agent from collecting +// another's answer. A review is work someone spent money on. +func TestAnalysisIsPrivateToTheSessionThatPaid(t *testing.T) { + c := analysable(t, agent.ClaudeRun{}, nil) + c.Register(Session{ID: "other", ProjectID: "p1", Name: "brisk-heron-cccc", Dir: t.TempDir()}) + + job, err := c.Analyse("me", "wily-crane-bbbb", "") + if err != nil { + t.Fatal(err) + } + if _, err := c.Analysis("other", job.ID); err == nil { + t.Error("a different session collected an analysis it did not start") + } + if _, err := c.Analysis("me", job.ID); err != nil { + t.Errorf("the session that started it cannot collect it: %v", err) + } +} + +// TestRunningJobReportsHowLongItHasWaited covers the field that is only +// written on completion. A job still in flight would otherwise say 0s, which +// is the one moment the caller most wants the number. +func TestRunningJobReportsHowLongItHasWaited(t *testing.T) { + c := analysable(t, agent.ClaudeRun{}, nil) + job, err := c.Analyse("me", "wily-crane-bbbb", "") + if err != nil { + t.Fatal(err) + } + time.Sleep(1100 * time.Millisecond) + + got, err := c.Analysis("me", job.ID) + if err != nil { + t.Fatal(err) + } + if got.State == JobRunning && got.Elapsed < time.Second { + t.Errorf("a job running for over a second reports %v", got.Elapsed) + } +} + +// TestAnUnusableRunRecordsNoCost is the other half. An error from RunClaude +// means no envelope came back, so there is no figure to add — inventing one +// would be worse than the gap it fills. +func TestAnUnusableRunRecordsNoCost(t *testing.T) { + c := analysable(t, agent.ClaudeRun{}, errors.New("claude: command not found")) + + job, err := c.Analyse("me", "wily-crane-bbbb", "") + if err != nil { + t.Fatal(err) + } + got := waitFor(t, c, job.ID) + + if got.State != JobFailed { + t.Errorf("state = %v, want failed", got.State) + } + if !strings.Contains(got.Err, "command not found") { + t.Errorf("error = %q", got.Err) + } + if c.Spend("me") != 0 { + t.Errorf("spend = %v; a run that never happened was billed", c.Spend("me")) + } +} + +// TestJobsAreBounded matches how the inbox and the log are bounded. Each +// record holds a full review, so a session that keeps asking would otherwise +// grow without limit. +func TestJobsAreBounded(t *testing.T) { + c := analysable(t, agent.ClaudeRun{Text: "ok", CostUSD: 0.01}, nil) + + var first string + for i := range maxJobs + 5 { + job, err := c.Analyse("me", "wily-crane-bbbb", "") + if err != nil { + t.Fatal(err) + } + if i == 0 { + first = job.ID + } + } + c.mu.Lock() + kept := 0 + for _, j := range c.jobs { + if j.From == "me" { + kept++ + } + } + c.mu.Unlock() + if kept != maxJobs { + t.Errorf("kept %d analyses, want the bound of %d", kept, maxJobs) + } + if _, err := c.Analysis("me", first); err == nil { + t.Error("the oldest analysis survived the bound") + } +} + +// TestUnregisterDropsJobsAndSpend matches how claims and the inbox behave: a +// session's record goes with the session. +func TestUnregisterDropsJobsAndSpend(t *testing.T) { + c := analysable(t, agent.ClaudeRun{}, nil) + job, err := c.Analyse("me", "wily-crane-bbbb", "") + if err != nil { + t.Fatal(err) + } + c.Unregister("me") + + if _, err := c.Analysis("me", job.ID); err == nil { + t.Error("an unregistered session's analysis is still readable") + } + if got := c.Spend("me"); got != 0 { + t.Errorf("spend = %v after unregister, want 0", got) + } +} + +// TestSpendAccumulatesAcrossRuns covers the number the UI will show. A per-run +// figure alone is hard to read: the same short turn measured at $0.012 and +// $0.237 depending on whether its context was read from cache or written to +// it, so the running total is what makes a pattern visible. +func TestSpendAccumulatesAcrossRuns(t *testing.T) { + c := analysable(t, agent.ClaudeRun{Text: "looks fine", CostUSD: 0.05, + Tokens: agent.Tokens{Input: 10, Output: 20, CacheWrite: 23698}}, nil) + + for range 3 { + job, err := c.Analyse("me", "wily-crane-bbbb", "") + if err != nil { + t.Fatal(err) + } + waitFor(t, c, job.ID) + } + if got := c.Spend("me"); got < 0.149 || got > 0.151 { + t.Errorf("spend = %v, want three runs at 0.05", got) + } +} + +// TestAFailedRunStillCountsAgainstSpend keeps the total honest. A turn that +// refused after spending is still spending, and a total counting only +// successes would understate the bill in exactly the case worth noticing. +func TestAFailedRunStillCountsAgainstSpend(t *testing.T) { + // The shape RunClaude really returns for a refused turn: a result that + // carries its cost, with Failure set. The earlier version of this test + // paired a cost with a Go error, which no production path produces, so it + // asserted a property nothing implemented. + c := analysable(t, agent.ClaudeRun{CostUSD: 0.09, Failure: "error_max_turns: ran out of turns"}, nil) + + job, err := c.Analyse("me", "wily-crane-bbbb", "") + if err != nil { + t.Fatal(err) + } + got := waitFor(t, c, job.ID) + + if got.State != JobFailed { + t.Errorf("state = %v, want failed", got.State) + } + if !strings.Contains(got.Err, "ran out of turns") { + t.Errorf("error = %q", got.Err) + } + if c.Spend("me") != 0.09 { + t.Errorf("spend = %v; a failed run that cost money was not counted", c.Spend("me")) + } +} + +// TestFinishedJobCarriesItsUsage pins the split the bill is explained by. +func TestFinishedJobCarriesItsUsage(t *testing.T) { + want := agent.Tokens{Input: 10, Output: 20, CacheRead: 5, CacheWrite: 23698} + c := analysable(t, agent.ClaudeRun{Text: "ok", CostUSD: 0.05, Tokens: want}, nil) + + job, err := c.Analyse("me", "wily-crane-bbbb", "") + if err != nil { + t.Fatal(err) + } + got := waitFor(t, c, job.ID) + if got.Tokens != want { + t.Errorf("tokens = %+v, want %+v", got.Tokens, want) + } + if got.Answer != "ok" { + t.Errorf("answer = %q", got.Answer) + } +} + +// waitFor blocks until a job leaves the running state. +func waitFor(t *testing.T, c *Coordinator, id string) *Job { + t.Helper() + for range 100 { + job, err := c.Analysis("me", id) + if err != nil { + t.Fatal(err) + } + if job.State != JobRunning { + return job + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("job %s never finished", id) + return nil +} + +// toolPayload decodes what a tool returned. An MCP result carries the tool's +// JSON as text inside its content, so a test that string-matches the outer +// envelope is matching escaped JSON and breaks on any reformatting. +func toolPayload(t *testing.T, result any) map[string]any { + t.Helper() + var env struct { + Content []struct{ Text string } `json:"content"` + } + raw, _ := json.Marshal(result) + if err := json.Unmarshal(raw, &env); err != nil || len(env.Content) == 0 { + t.Fatalf("not a tool result: %s", raw) + } + var out map[string]any + if err := json.Unmarshal([]byte(env.Content[0].Text), &out); err != nil { + t.Fatalf("tool payload is not JSON: %s", env.Content[0].Text) + } + return out +} + +// TestAnalyseIsCallableOverMCP covers the wiring rather than the method, and +// exists because the same gap was found in the work tool and then repeated +// here: renaming either case name left the whole suite green while both tools +// appeared in tools/list and failed for a real agent. +// +// It drives the full round trip — start through tools/call, collect through +// tools/call — with the spawn stubbed, so it proves the dispatch and the +// argument names without costing anything. +func TestAnalyseIsCallableOverMCP(t *testing.T) { + c := analysable(t, agent.ClaudeRun{ + Text: "the retry budget is unbounded", CostUSD: 0.05, + Tokens: agent.Tokens{Input: 10, Output: 20}, Took: 3 * time.Second, + }, nil) + + started := rpc(t, c, "me", "tools/call", map[string]any{ + "name": "analyse", + "arguments": map[string]any{"session": "wily-crane-bbbb", "question": "anything risky?"}, + }) + if started.Error != nil { + t.Fatalf("analyse: %v", started.Error) + } + begun := toolPayload(t, started.Result) + if begun["session"] != "wily-crane-bbbb" { + t.Fatalf("analyse did not report the subject: %v", begun) + } + id, _ := begun["id"].(string) + if id == "" { + t.Fatalf("analyse returned no id: %v", begun) + } + + var done map[string]any + for range 100 { + got := rpc(t, c, "me", "tools/call", map[string]any{ + "name": "analysis", "arguments": map[string]any{"id": id}, + }) + if got.Error != nil { + t.Fatalf("analysis: %v", got.Error) + } + done = toolPayload(t, got.Result) + if done["state"] == "done" { + break + } + time.Sleep(20 * time.Millisecond) + } + + if done["state"] != "done" { + t.Fatalf("analysis never reported done: %v", done) + } + if done["answer"] != "the retry budget is unbounded" { + t.Errorf("answer = %v", done["answer"]) + } + if done["cost_usd"] != 0.05 { + t.Errorf("cost_usd = %v, want 0.05", done["cost_usd"]) + } + if done["session_spend_usd"] != 0.05 { + t.Errorf("session_spend_usd = %v, want the running total", done["session_spend_usd"]) + } + if done["took"] != "3s" { + t.Errorf("took = %v, want the duration the run reported", done["took"]) + } +} + +// TestTheQuestionReachesTheReviewer covers the argument the round-trip test +// cannot see. The question only becomes visible in the prompt, so renaming its +// JSON tag left every other assertion green while the reviewer silently got +// the general review instead of what was asked. +func TestTheQuestionReachesTheReviewer(t *testing.T) { + var prompts []string + c := analysableWatching(t, agent.ClaudeRun{Text: "ok"}, nil, &prompts) + + const asked = "does the retry budget have an upper bound?" + got := rpc(t, c, "me", "tools/call", map[string]any{ + "name": "analyse", + "arguments": map[string]any{"session": "wily-crane-bbbb", "question": asked}, + }) + if got.Error != nil { + t.Fatal(got.Error) + } + waitFor(t, c, toolPayload(t, got.Result)["id"].(string)) + + if len(prompts) != 1 { + t.Fatalf("spawned %d reviewers, want 1", len(prompts)) + } + if !strings.Contains(prompts[0], asked) { + t.Errorf("the question never reached the reviewer:\n%s", prompts[0]) + } + // The diff has to be there too: the reviewer is given no tools, so + // anything missing from the prompt is unavailable to it. + for _, want := range []string{"auth.go", "wily-crane-bbbb"} { + if !strings.Contains(prompts[0], want) { + t.Errorf("prompt is missing %q, which the reviewer cannot look up", want) + } + } +} + +// TestAGeneralReviewGetsTheDefaultQuestion covers the other branch: omitting +// the question must still ask something. +func TestAGeneralReviewGetsTheDefaultQuestion(t *testing.T) { + var prompts []string + c := analysableWatching(t, agent.ClaudeRun{Text: "ok"}, nil, &prompts) + + job, err := c.Analyse("me", "wily-crane-bbbb", " ") + if err != nil { + t.Fatal(err) + } + waitFor(t, c, job.ID) + + if len(prompts) != 1 || !strings.Contains(prompts[0], defaultQuestion) { + t.Errorf("a blank question did not fall back to the default:\n%v", prompts) + } +} + +// TestCloseStopsARunningAnalysis ties spawned work to the app that asked for +// it. Without it, quitting Deck mid-review left a claude process running and +// billing for as long as the job timeout allowed, with no surface left to +// show it on — the opposite of the visibility the cost reporting exists for. +func TestCloseStopsARunningAnalysis(t *testing.T) { + cancelled := make(chan bool, 1) + c := analysableWith(t, func(ctx context.Context, _, _ string) (agent.ClaudeRun, error) { + select { + case <-ctx.Done(): + cancelled <- true + case <-time.After(5 * time.Second): + cancelled <- false + } + return agent.ClaudeRun{}, ctx.Err() + }) + + if _, err := c.Analyse("me", "wily-crane-bbbb", ""); err != nil { + t.Fatal(err) + } + // Let the goroutine reach the spawn before closing, so this tests + // cancellation rather than a race to start. + time.Sleep(100 * time.Millisecond) + if err := c.Close(); err != nil { + t.Fatal(err) + } + + select { + case got := <-cancelled: + if !got { + t.Error("the spawned run survived Close; it will keep billing unattended") + } + case <-time.After(3 * time.Second): + t.Error("the spawned run never reported; Close did not reach it") + } +} + +// startWith is start with a substituted reviewer. +func startWith(t *testing.T, r Reviewer) *Coordinator { + t.Helper() + c, err := Start(t.TempDir(), WithReviewer(r)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = c.Close() }) + return c +} + +// analysableWith is analysable with a reviewer the caller controls, for tests +// about the run's lifetime rather than its result. +func analysableWith(t *testing.T, r Reviewer) *Coordinator { + t.Helper() + dir, head := worktreeSession(t) + c := startWith(t, r) + c.Register(Session{ID: "me", ProjectID: "p1", Name: "scheming-hawk-jhgk", Dir: t.TempDir()}) + c.Register(Session{ID: "them", ProjectID: "p1", Name: "wily-crane-bbbb", + Title: "split the auth middleware", Dir: dir, + Branch: "session/wily-crane-bbbb", Isolated: true, BaseRef: head}) + return c +} diff --git a/internal/coord/live_test.go b/internal/coord/live_test.go index 752dcf4..7d55f1c 100644 --- a/internal/coord/live_test.go +++ b/internal/coord/live_test.go @@ -276,3 +276,78 @@ func TestLiveClaudeReadsSiblingWork(t *testing.T) { t.Errorf("claude did not report the function only the patch could tell it.\n%s", got) } } + +// TestLiveClaudeSpawnedAnalysis drives the whole spawned path against the real +// CLI: Deck starts an agent nobody is watching, hands it a diff, and reports +// what it answered and what it cost. +// +// Every other test of this path substitutes the reviewer, which proves the +// registry and the accounting but nothing about whether claude accepts the +// argv we build, answers under plan mode, or reports usage we can read. This +// is the only test that spends money on purpose. +// +// The assertion is a token that exists nowhere except in the sibling's +// uncommitted diff, so an answer containing it cannot have come from the +// prompt's instructions or from a guess. +func TestLiveClaudeSpawnedAnalysis(t *testing.T) { + liveOnly(t) + c := start(t) + + target, head := worktreeSession(t) + const token = "ZEPHYR_RETRY_BUDGET" + if err := os.WriteFile(filepath.Join(target, "retry.go"), + []byte("package gateway\n\nfunc "+token+"() int { return 3 }\n"), 0o644); err != nil { + t.Fatal(err) + } + + c.Register(Session{ID: "asker", ProjectID: "p1", Name: "wily-crane-bbbb", + Title: "reviewing", Dir: t.TempDir()}) + c.Register(Session{ID: "worker", ProjectID: "p1", Name: "swift-otter-aaaa", + Title: "retry budget", Dir: target, Branch: "session/swift-otter-aaaa", + Isolated: true, BaseRef: head}) + + job, err := c.Analyse("asker", "swift-otter-aaaa", + "Reply with only the name of the function this change adds, nothing else.") + if err != nil { + t.Fatal(err) + } + if job.State != JobRunning { + t.Fatalf("Analyse did not return a running job: %v", job.State) + } + + var done *Job + for range 120 { + got, err := c.Analysis("asker", job.ID) + if err != nil { + t.Fatal(err) + } + if got.State != JobRunning { + done = got + break + } + time.Sleep(time.Second) + } + if done == nil { + t.Fatal("the analysis never finished within two minutes") + } + if done.State != JobDone { + t.Fatalf("analysis failed: %s", done.Err) + } + + t.Logf("answered %q in %v for $%.4f (%+v)", done.Answer, done.Elapsed, done.Cost, done.Tokens) + + if !strings.Contains(done.Answer, token) { + t.Errorf("the reviewer did not report what only the diff could tell it:\n%s", done.Answer) + } + // The accounting is the reason this feature reports anything at all: a run + // nobody watched has to say what it spent. + if done.Cost <= 0 { + t.Errorf("a real turn reported no cost") + } + if done.Tokens.Output <= 0 { + t.Errorf("a real turn reported no output tokens: %+v", done.Tokens) + } + if c.Spend("asker") != done.Cost { + t.Errorf("session spend %v does not match the one run's cost %v", c.Spend("asker"), done.Cost) + } +} diff --git a/internal/coord/tools.go b/internal/coord/tools.go index b4b8f0b..44ad242 100644 --- a/internal/coord/tools.go +++ b/internal/coord/tools.go @@ -1,11 +1,6 @@ package coord -// The tools agents see, and the dispatch that answers a call. - -import ( - "encoding/json" - "fmt" -) +// The tools agents see. Answering a call is in dispatch.go. func toolSchemas() []map[string]any { pathList := map[string]any{ @@ -90,6 +85,38 @@ func toolSchemas() []map[string]any { "required": []string{"session"}, }, }, + { + "name": "analyse", + "description": "Have a separate agent review another session's work and report back. " + + "Returns immediately with an id; collect the answer later with the analysis tool. " + + "The reviewer is given the diff and cannot run or change anything.", + "inputSchema": map[string]any{ + "type": "object", + "properties": map[string]any{ + "session": map[string]any{ + "type": "string", + "description": "Session name from the sessions tool.", + }, + "question": map[string]any{ + "type": "string", + "description": "What you want to know. Omit for a general review.", + }, + }, + "required": []string{"session"}, + }, + }, + { + "name": "analysis", + "description": "Collect an analysis you started. Reports whether it is still " + + "running, and when finished the answer along with what it cost.", + "inputSchema": map[string]any{ + "type": "object", + "properties": map[string]any{ + "id": map[string]any{"type": "string", "description": "The id analyse returned."}, + }, + "required": []string{"id"}, + }, + }, { "name": "inbox", "description": "Collect messages other agents sent you. Reading empties the box.", @@ -97,99 +124,3 @@ func toolSchemas() []map[string]any { }, } } - -type toolCall struct { - Name string `json:"name"` - Arguments struct { - Paths []string `json:"paths"` - Reason string `json:"reason"` - Text string `json:"text"` - To string `json:"to"` - Session string `json:"session"` - } `json:"arguments"` -} - -func (s *server) callTool(sessionID string, params json.RawMessage) map[string]any { - var p toolCall - if err := json.Unmarshal(params, &p); err != nil { - return errorResult("bad arguments: " + err.Error()) - } - - res := s.dispatch(sessionID, p) - - // Mail is pull-only, so a recipient would otherwise never learn it had - // any. Every result except the inbox's own carries the count, which makes - // any coordination call the moment a message surfaces. - if p.Name != "inbox" { - if n := s.c.Unread(sessionID); n > 0 { - res = withHint(res, fmt.Sprintf( - "\n\n(%d message(s) waiting from other agents — call the inbox tool.)", n)) - } - } - return res -} - -func (s *server) dispatch(sessionID string, p toolCall) map[string]any { - switch p.Name { - case "sessions": - siblings := s.c.Siblings(sessionID) - if len(siblings) == 0 { - return textResult("No other agents are working on this project.") - } - return jsonResult(map[string]any{"sessions": siblings}) - - case "claim": - granted, conflicts := s.c.Claim(sessionID, p.Arguments.Paths, p.Arguments.Reason) - out := map[string]any{"claimed": granted} - if len(conflicts) > 0 { - out["conflicts"] = conflicts - out["advice"] = "Another agent is already working on the conflicting paths. " + - "Pick different work, or leave a note explaining the overlap." - } - return jsonResult(out) - - case "work": - w, err := s.c.Work(sessionID, p.Arguments.Session) - if err != nil { - return errorResult(err.Error()) - } - return jsonResult(w) - - case "release": - return jsonResult(map[string]any{"released": s.c.Release(sessionID, p.Arguments.Paths)}) - - case "note": - if err := s.c.AppendNote(sessionID, p.Arguments.Text); err != nil { - return errorResult(err.Error()) - } - return textResult("Noted.") - - case "notes": - notes, err := s.c.Notes(sessionID) - if err != nil { - return errorResult(err.Error()) - } - if len(notes) == 0 { - return textResult("No shared notes yet for this project.") - } - return jsonResult(map[string]any{"notes": notes}) - - case "message": - sent, err := s.c.Send(sessionID, p.Arguments.To, p.Arguments.Text) - if err != nil { - return errorResult(err.Error()) - } - if len(sent) == 0 { - return textResult("No other agents are on this project, so nobody received it.") - } - return jsonResult(map[string]any{"delivered_to": sent}) - - case "inbox": - msgs := s.c.Collect(sessionID) - if len(msgs) == 0 { - return textResult("No messages.") - } - return jsonResult(map[string]any{"messages": msgs}) - } - return errorResult("unknown tool: " + p.Name) -} diff --git a/internal/coord/work.go b/internal/coord/work.go index 0c145f0..d3da70d 100644 --- a/internal/coord/work.go +++ b/internal/coord/work.go @@ -26,11 +26,33 @@ import ( // publishes and what message already accepts, so an agent has only one kind of // handle to learn. func (c *Coordinator) Work(sessionID, target string) (map[string]any, error) { + found, w, err := c.workOf(sessionID, target) + if err != nil { + return nil, err + } + out := map[string]any{ + "session": found.Name, + "title": found.Title, + "branch": found.Branch, + "base": w.Base, + "summary": w.Stat, + "patch": w.Patch, + } + if w.Truncated { + out["truncated"] = true + } + return out, nil +} + +// workOf resolves a sibling by name and reads its changes. Both the work tool +// and a spawned analysis go through it, so the scoping and the two refusals +// are stated once rather than in each caller. +func (c *Coordinator) workOf(sessionID, target string) (*Session, gitx.Work, error) { c.mu.Lock() me, ok := c.sessions[sessionID] if !ok { c.mu.Unlock() - return nil, fmt.Errorf("unknown session") + return nil, gitx.Work{}, fmt.Errorf("unknown session") } var found *Session for id, s := range c.sessions { @@ -47,30 +69,19 @@ func (c *Coordinator) Work(sessionID, target string) (map[string]any, error) { c.mu.Unlock() if found == nil { - return nil, fmt.Errorf("no live session named %q on this project", target) + return nil, gitx.Work{}, fmt.Errorf("no live session named %q on this project", target) } // Refused rather than answered approximately. A shared project directory // holds everyone's edits at once, so a diff of it would credit this // session with work it may not have done. if !found.Isolated { - return nil, fmt.Errorf("%s runs in the project directory, so its changes "+ + return nil, gitx.Work{}, fmt.Errorf("%s runs in the project directory, so its changes "+ "cannot be told apart from any other work there", found.Name) } w, err := gitx.Diff(found.Dir, found.BaseRef) if err != nil { - return nil, fmt.Errorf("read %s: %w", found.Name, err) - } - out := map[string]any{ - "session": found.Name, - "title": found.Title, - "branch": found.Branch, - "base": w.Base, - "summary": w.Stat, - "patch": w.Patch, + return nil, gitx.Work{}, fmt.Errorf("read %s: %w", found.Name, err) } - if w.Truncated { - out["truncated"] = true - } - return out, nil + return found, w, nil } diff --git a/internal/coord/work_test.go b/internal/coord/work_test.go index fb74986..6f30dd5 100644 --- a/internal/coord/work_test.go +++ b/internal/coord/work_test.go @@ -2,46 +2,18 @@ package coord import ( "encoding/json" + "github.com/tripledownab/deck/internal/gittest" "os" - "os/exec" "path/filepath" "strings" "testing" ) -// worktreeSession builds a repository with one commit, returns its path and -// HEAD, so a test can register a session that looks like an isolated one. +// worktreeSession is a repository with one commit, standing in for a session's +// isolated worktree. func worktreeSession(t *testing.T) (dir, head string) { t.Helper() - dir = t.TempDir() - if err := os.WriteFile(filepath.Join(dir, "router.go"), []byte("package gateway\n"), 0o644); err != nil { - t.Fatal(err) - } - for _, args := range [][]string{ - {"init", "-q", "-b", "main"}, - {"config", "user.name", "test"}, - {"config", "user.email", "test@example.test"}, - {"add", "."}, - {"commit", "-q", "-m", "initial"}, - } { - cmd := exec.Command("git", args...) - cmd.Dir = dir - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("git %s: %v: %s", args[0], err, out) - } - } - // Read separately rather than from the loop's last output. Taking it from - // the loop makes the sha depend on rev-parse staying last in the slice, - // and a command added after it would leave head empty — surfacing as - // "no base recorded" and reading like a bug in Work. - rev := exec.Command("git", "rev-parse", "HEAD") - rev.Dir = dir - out, err := rev.Output() - if err != nil { - t.Fatalf("rev-parse in %s: %v", dir, err) - } - head = strings.TrimSpace(string(out)) - return dir, head + return gittest.RepoWith(t, "router.go", "package gateway\n") } // TestWorkReadsASiblingWithoutItsHelp is the point of reading the worktree diff --git a/internal/gittest/gittest.go b/internal/gittest/gittest.go new file mode 100644 index 0000000..a6d2eab --- /dev/null +++ b/internal/gittest/gittest.go @@ -0,0 +1,68 @@ +// Package gittest builds throwaway git repositories for tests. +// +// It exists because four packages had grown their own copy of "make a +// repository with one commit", and they had already drifted: one resolved +// symlinks and the others did not, which is the difference that produced two +// registrations of a single directory elsewhere in this repository. +package gittest + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// Repo creates a git repository in a temporary directory and returns its path. +// +// The path is symlink-resolved. t.TempDir hands back a symlinked path on +// macOS, and git reports resolved ones, so a caller comparing the two without +// this sees a mismatch that has nothing to do with what it is testing. +// +// The identity is set per repository rather than read from the machine, so a +// test that inspects a commit cannot pick up whoever is running it. +func Repo(t *testing.T) string { + t.Helper() + dir, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("resolve temp dir: %v", err) + } + Run(t, dir, "init", "-q", "-b", "main") + Run(t, dir, "config", "user.name", "test") + Run(t, dir, "config", "user.email", "test@example.test") + return dir +} + +// RepoWith is Repo plus one committed file, returning the repository and the +// sha of that commit. +func RepoWith(t *testing.T, name, body string) (dir, head string) { + t.Helper() + dir = Repo(t) + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + Run(t, dir, "add", ".") + Run(t, dir, "commit", "-q", "-m", "initial") + return dir, headCommit(t, dir) +} + +// headCommit is the commit dir currently points at. Unexported, and not named +// head, because RepoWith's own return value is called that. +func headCommit(t *testing.T, dir string) string { + t.Helper() + return Run(t, dir, "rev-parse", "HEAD") +} + +// Run executes git in dir and returns its trimmed output, failing the test on +// error so a caller never has to decide whether a fixture step mattered. +func Run(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s in %s: %v\n%s", strings.Join(args, " "), dir, err, out) + } + return strings.TrimSpace(string(out)) +} diff --git a/internal/gittest/gittest_test.go b/internal/gittest/gittest_test.go new file mode 100644 index 0000000..1ff85a7 --- /dev/null +++ b/internal/gittest/gittest_test.go @@ -0,0 +1,32 @@ +package gittest + +import ( + "path/filepath" + "testing" +) + +// TestRepoReturnsAResolvedPath pins the one property that made this package +// worth extracting. Four callers had grown their own fixture and they had +// already drifted on exactly this: t.TempDir hands back a symlinked path on +// macOS, git reports the resolved one, and a caller comparing the two sees a +// mismatch that has nothing to do with what it is testing. The same difference +// registered one directory as two projects elsewhere in this repository. +// +// Nothing else fails if the resolution is dropped, so without this the guard +// is a comment rather than a rule. +func TestRepoReturnsAResolvedPath(t *testing.T) { + dir := Repo(t) + + resolved, err := filepath.EvalSymlinks(dir) + if err != nil { + t.Fatal(err) + } + if dir != resolved { + t.Errorf("Repo returned %q, which resolves to %q", dir, resolved) + } + + // And git agrees, which is the comparison a caller actually makes. + if got := Run(t, dir, "rev-parse", "--show-toplevel"); got != dir { + t.Errorf("git reports %q, Repo returned %q", got, dir) + } +} diff --git a/internal/gitx/diff_test.go b/internal/gitx/diff_test.go index 4256d6d..47ffc7d 100644 --- a/internal/gitx/diff_test.go +++ b/internal/gitx/diff_test.go @@ -1,43 +1,18 @@ package gitx import ( + "github.com/tripledownab/deck/internal/gittest" "os" "path/filepath" "strings" "testing" ) -// repoWithCommit builds on testRepo, adding one committed file, and returns -// the repository and the sha of that commit. -// -// It goes through testRepo rather than initialising its own repository so the -// symlink resolution there is not quietly lost: t.TempDir hands back a -// symlinked path on macOS, and a helper that skips EvalSymlinks is the -// difference that produced two registrations of one directory elsewhere. -func repoWithCommit(t *testing.T, file, body string) (string, string) { - t.Helper() - dir := testRepo(t) - if err := os.WriteFile(filepath.Join(dir, file), []byte(body), 0o644); err != nil { - t.Fatal(err) - } - if _, err := run(dir, "add", "."); err != nil { - t.Fatal(err) - } - if _, err := run(dir, "commit", "-q", "-m", "initial"); err != nil { - t.Fatal(err) - } - head, err := HeadCommit(dir) - if err != nil { - t.Fatal(err) - } - return dir, head -} - // TestDiffSeesUncommittedWork is the case that would mislead a reviewer worst. // A session that has edited but not committed has still done work, and being // told "no changes" would be wrong rather than merely incomplete. func TestDiffSeesUncommittedWork(t *testing.T) { - dir, base := repoWithCommit(t, "router.go", "package gateway\n") + dir, base := gittest.RepoWith(t, "router.go", "package gateway\n") if err := os.WriteFile(filepath.Join(dir, "router.go"), []byte("package gateway\n\nfunc Route() {}\n"), 0o644); err != nil { t.Fatal(err) @@ -59,7 +34,7 @@ func TestDiffSeesUncommittedWork(t *testing.T) { // A capped patch that also hid which files changed would leave the reader // unable to tell what they had not seen. func TestDiffSummaryIsNeverTruncated(t *testing.T) { - dir, base := repoWithCommit(t, "seed.txt", "seed\n") + dir, base := gittest.RepoWith(t, "seed.txt", "seed\n") // One file far past the budget, and a second small one after it // alphabetically, so a naive cap would lose the second entirely. @@ -95,7 +70,7 @@ func TestDiffSummaryIsNeverTruncated(t *testing.T) { // Comparing against the parent branch's tip would attribute everything that // landed there since the session started to this session as well. func TestDiffMeasuresFromTheMergeBase(t *testing.T) { - dir, base := repoWithCommit(t, "shared.txt", "one\n") + dir, base := gittest.RepoWith(t, "shared.txt", "one\n") // The session's own change, on a branch. run(dir, "checkout", "-q", "-b", "session/work") @@ -128,7 +103,7 @@ func TestDiffMeasuresFromTheMergeBase(t *testing.T) { // TestDiffRefusesWithoutABase covers sessions recorded before BaseRef existed. func TestDiffRefusesWithoutABase(t *testing.T) { - dir, _ := repoWithCommit(t, "a.txt", "a\n") + dir, _ := gittest.RepoWith(t, "a.txt", "a\n") if _, err := Diff(dir, ""); err == nil { t.Error("a worktree with no recorded base produced a diff anyway") } @@ -138,7 +113,7 @@ func TestDiffRefusesWithoutABase(t *testing.T) { // "git diff" ignores untracked files, so a session whose work is mostly new // files read as having done nothing at all. func TestDiffSeesNewFiles(t *testing.T) { - dir, base := repoWithCommit(t, "seed.txt", "seed\n") + dir, base := gittest.RepoWith(t, "seed.txt", "seed\n") if err := os.WriteFile(filepath.Join(dir, "handler.go"), []byte("package api\n\nfunc Handle() {}\n"), 0o644); err != nil { t.Fatal(err) @@ -160,7 +135,7 @@ func TestDiffSeesNewFiles(t *testing.T) { // in a throwaway index, so the session's own index and working tree must be // exactly as they were. func TestDiffLeavesTheWorktreeAlone(t *testing.T) { - dir, base := repoWithCommit(t, "seed.txt", "seed\n") + dir, base := gittest.RepoWith(t, "seed.txt", "seed\n") if err := os.WriteFile(filepath.Join(dir, "new.txt"), []byte("new\n"), 0o644); err != nil { t.Fatal(err) } @@ -187,7 +162,7 @@ func TestDiffLeavesTheWorktreeAlone(t *testing.T) { // TestDiffIgnoresIgnoredFiles keeps build output out of a review. func TestDiffIgnoresIgnoredFiles(t *testing.T) { - dir, base := repoWithCommit(t, ".gitignore", "build/\n") + dir, base := gittest.RepoWith(t, ".gitignore", "build/\n") if err := os.MkdirAll(filepath.Join(dir, "build"), 0o755); err != nil { t.Fatal(err) } @@ -218,7 +193,7 @@ func TestDiffIgnoresIgnoredFiles(t *testing.T) { // field identical to the one a success produces, so the caller could not tell a // real diff from a guess. func TestDiffReportsAnUnrelatedHistory(t *testing.T) { - dir, base := repoWithCommit(t, "seed.txt", "seed\n") + dir, base := gittest.RepoWith(t, "seed.txt", "seed\n") // An orphan branch shares no commit with base, so there is no merge base. if _, err := run(dir, "checkout", "-q", "--orphan", "elsewhere"); err != nil { diff --git a/internal/gitx/gitx_test.go b/internal/gitx/gitx_test.go index 6d28891..b2aa958 100644 --- a/internal/gitx/gitx_test.go +++ b/internal/gitx/gitx_test.go @@ -2,8 +2,8 @@ package gitx import ( "errors" + "github.com/tripledownab/deck/internal/gittest" "os" - "os/exec" "path/filepath" "strings" "testing" @@ -11,22 +11,8 @@ import ( func testRepo(t *testing.T) string { t.Helper() - dir, err := filepath.EvalSymlinks(t.TempDir()) - if err != nil { - t.Fatalf("resolve temp dir: %v", err) - } - for _, args := range [][]string{ - {"init", "-b", "main"}, - {"config", "user.email", "test@example.test"}, - {"config", "user.name", "Test"}, - {"commit", "--allow-empty", "-m", "root"}, - } { - cmd := exec.Command("git", args...) - cmd.Dir = dir - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) - } - } + dir := gittest.Repo(t) + gittest.Run(t, dir, "commit", "--allow-empty", "-m", "root") return dir } @@ -121,12 +107,9 @@ func TestRunErrorCarriesGitStderr(t *testing.T) { // own message is "fatal: invalid reference: HEAD", which is accurate and tells // a user nothing about what to do next. func TestAddWorktreeOnUnbornHead(t *testing.T) { - dir := t.TempDir() - cmd := exec.Command("git", "init", "-b", "main") - cmd.Dir = dir - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("git init: %v\n%s", err, out) - } + // gittest.Repo initialises without committing, which is the unborn HEAD + // this test is about. + dir := gittest.Repo(t) if HasCommits(dir) { t.Fatal("a repository with no commits reported HasCommits") diff --git a/internal/ui/actions_test.go b/internal/ui/actions_test.go index c1c40ad..865d8f6 100644 --- a/internal/ui/actions_test.go +++ b/internal/ui/actions_test.go @@ -1,8 +1,8 @@ package ui import ( + "github.com/tripledownab/deck/internal/gittest" "os" - "os/exec" "path/filepath" "testing" ) @@ -12,13 +12,7 @@ func repoAt(t *testing.T, dir string) { if err := os.MkdirAll(dir, 0o755); err != nil { t.Fatal(err) } - for _, args := range [][]string{{"init", "-b", "main"}} { - cmd := exec.Command("git", args...) - cmd.Dir = dir - if out, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("git %v: %v\n%s", args, err, out) - } - } + gittest.Run(t, dir, "init", "-b", "main") } // TestResolveProjectAcceptsACollector is the point of allowing non-repository diff --git a/internal/ui/sidebar.go b/internal/ui/sidebar.go index 2d4e4a7..c0e0d68 100644 --- a/internal/ui/sidebar.go +++ b/internal/ui/sidebar.go @@ -118,6 +118,22 @@ func (m Model) sessionCard(sess *store.Session, active bool, width, nth int) []s if n := m.coord.ClaimCount(sess.ID); n > 0 { label += s.Faint.Render(fmt.Sprintf(" ⊙ %d", n)) } + // A spawned review is the one thing here that costs money while + // nobody is watching it: it has no pane, and the session that asked + // for it has moved on. The badge is what makes it visible, and the + // figure stays after the last one finishes so a total is not lost the + // moment it stops moving. + // + // Coloured by the same rule as the two above. A run in flight is + // spending now, which is the accent's job; a settled total is a fact + // about the past, like a claim. + if n, spent := m.coord.Analyses(sess.ID); n > 0 || spent > 0 { + if n > 0 { + label += s.Accent.Render(fmt.Sprintf(" ⚗ %d · $%.2f", n, spent)) + } else { + label += s.Faint.Render(fmt.Sprintf(" ⚗ $%.2f", spent)) + } + } } // The status line is truncated like the other two. It is the one that grows diff --git a/internal/ui/status_test.go b/internal/ui/status_test.go index fa9bc70..45b8af0 100644 --- a/internal/ui/status_test.go +++ b/internal/ui/status_test.go @@ -7,8 +7,10 @@ import ( "github.com/charmbracelet/lipgloss" + "context" "github.com/tripledownab/deck/internal/agent" "github.com/tripledownab/deck/internal/coord" + "github.com/tripledownab/deck/internal/gittest" "github.com/tripledownab/deck/internal/store" ) @@ -175,3 +177,68 @@ func TestSidebarSurvivesALoadedStatusLine(t *testing.T) { } } } + +// TestSidebarShowsWhatAnalysesCost is the visibility a spawned review needs. +// It has no pane and the session that asked for it has moved on, so the +// sidebar is the only place a running one is visible and the only place its +// cost is reported at all. +// +// It drives the real path — Analyse, through a reviewer the test supplies — +// rather than seeding records, so the badge is asserted against what the +// coordinator actually produces. +func TestSidebarShowsWhatAnalysesCost(t *testing.T) { + release := make(chan struct{}) + c, err := coord.Start(t.TempDir(), coord.WithReviewer( + func(ctx context.Context, _, _ string) (agent.ClaudeRun, error) { + <-release + return agent.ClaudeRun{Text: "fine", CostUSD: 0.37}, nil + })) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = c.Close() }) + + repo, head := gittest.RepoWith(t, "router.go", "package gateway\n") + st := &store.State{} + p := st.AddProject(store.Project{Name: "api-gateway", Path: t.TempDir()}) + asker := st.AddSession(store.Session{ProjectID: p.ID, Name: "swift-otter-aaaa", + Title: "rate limiting", Dir: t.TempDir(), Agent: "bash"}) + worker := st.AddSession(store.Session{ProjectID: p.ID, Name: "wily-crane-bbbb", + Title: "auth", Dir: repo, Agent: "bash", Isolated: true, BaseRef: head}) + + m := New(st, "bash", nil).WithCoordinator(c) + m.width, m.height = 100, 30 + m.screen = screenSession + m.rebuildRows() + + // Nothing spawned: no badge, so an idle session is not decorated with a + // figure that means nothing. + if got := m.renderSidebar(34, 14); strings.Contains(got, "⚗") { + t.Fatalf("a session with no analyses shows the badge:\n%s", got) + } + + c.Register(coord.Session{ID: asker.ID, ProjectID: p.ID, Name: asker.Name, Dir: asker.Dir}) + c.Register(coord.Session{ID: worker.ID, ProjectID: p.ID, Name: worker.Name, + Dir: worker.Dir, Isolated: true, BaseRef: head}) + + if _, err := c.Analyse(asker.ID, worker.Name, ""); err != nil { + t.Fatal(err) + } + waitForBadge(t, m, "⚗ 1") + + close(release) + waitForBadge(t, m, "$0.37") +} + +// waitForBadge polls the rendered sidebar, because the analysis runs in its +// own goroutine and the UI reads whatever the coordinator has at frame time. +func waitForBadge(t *testing.T, m Model, want string) { + t.Helper() + for range 100 { + if got := m.renderSidebar(34, 14); strings.Contains(got, want) { + return + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("sidebar never showed %q:\n%s", want, m.renderSidebar(34, 14)) +} diff --git a/smoke_test.go b/smoke_test.go index f37e780..07d4247 100644 --- a/smoke_test.go +++ b/smoke_test.go @@ -12,6 +12,7 @@ import ( "github.com/charmbracelet/x/ansi" "github.com/creack/pty" + "github.com/tripledownab/deck/internal/gittest" "github.com/tripledownab/deck/internal/termquery" ) @@ -157,40 +158,19 @@ func buildBinary(t *testing.T) string { func initRepo(t *testing.T) string { t.Helper() - dir := t.TempDir() - // macOS temp dirs are symlinked through /private; git reports the resolved - // path, and the test compares against it. - resolved, err := filepath.EvalSymlinks(dir) - if err != nil { - t.Fatalf("resolve temp dir: %v", err) - } - steps := [][]string{ - {"init", "-b", "main"}, - {"config", "user.email", "smoke@example.test"}, - {"config", "user.name", "Smoke Test"}, - {"commit", "--allow-empty", "-m", "root"}, - } - for _, args := range steps { - cmd := exec.Command("git", args...) - cmd.Dir = resolved - if b, err := cmd.CombinedOutput(); err != nil { - t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, b) - } - } - return resolved + // gittest.Repo resolves the symlink macOS puts in a temp path, which this + // test needs because it compares against what git reports. + dir := gittest.Repo(t) + gittest.Run(t, dir, "commit", "--allow-empty", "-m", "root") + return dir } func findWorktreeBranch(t *testing.T, repo string) string { t.Helper() deadline := time.Now().Add(5 * time.Second) for time.Now().Before(deadline) { - cmd := exec.Command("git", "worktree", "list", "--porcelain") - cmd.Dir = repo - out, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("git worktree list: %v\n%s", err, out) - } - for _, line := range strings.Split(string(out), "\n") { + out := gittest.Run(t, repo, "worktree", "list", "--porcelain") + for _, line := range strings.Split(out, "\n") { if ref, ok := strings.CutPrefix(line, "branch refs/heads/"); ok && ref != "main" { return ref }