From 6d095ecf654d0c660f24fbc633d86ca4167f322d Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 15 Jun 2026 12:48:42 -0700 Subject: [PATCH] feat(orch): worker-authored result handoff via report_result tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Workers now choose what gets relayed to the manager instead of leaving it to a scrape of the last TUI pane. A new report_result MCP tool records an authoritative HandoffSummary — the worker's findings/outcome, optionally its diff vs base, and optionally inlined notes — which is preferred over the scraped LatestSummary everywhere a worker's result is surfaced (completion notification, manager context, supervisor snapshot). Why: a reviewer worker produced a full review (findings with file:line refs), but the manager received only a teaser. The handoff was scraped from the visible Codex TUI pane and truncated at 2000 BYTES; the pane's box-drawing separators (3 bytes each) meant only ~1050 characters survived, and head-anchored truncation kept the build-log preamble while dropping the findings that started at char 1188. The manager then wasted a follow-up worker re-deriving them. Also fixes latent over-permissioning: pr_followup/ci_followup workers shared the FULL manager MCP surface, so they could spawn_session, publish_pr, mark_objective_done, or cancel_session — none of which a worker should do. Tools are now split into three role-scoped surfaces. - New Forge.Diff (GitForge runs git diff vs resolved base, led by --stat; Fake is seedable) so report_result can attach the worker's changes. - Session.HandoffSummary column + idempotent migration for existing DBs. - Three MCP surfaces, each exposing exactly its tools: manager (/mcp/, full), follow-up (/fmcp/: update_pr/comment_pr/create_note/ask_user/report_result), coding worker (/wmcp/: report_result/create_note/ask_user). Tool defs factored into shared per-tool constructors. - Worker + follow-up preambles instruct calling report_result before the done marker. - Fallback scrape (LatestSummary) is now rune-safe and tail-anchored, so the conclusion survives even when a worker doesn't report. Tests: report_result summary/diff/notes/empty-diff/required-summary, relay preference order, tail truncation, GitForge.Diff, and a surface-scoping test asserting workers/follow-ups never see the manager-only tools. vet + gofmt + -race clean. --- cmd/orcha/main.go | 6 + internal/forge/forge.go | 16 ++ internal/forge/git.go | 32 ++++ internal/forge/git_test.go | 44 ++++++ internal/model/model.go | 25 ++-- internal/orch/context.go | 6 +- internal/orch/manager_mcp.go | 141 ++++++++++++------ internal/orch/sessions.go | 15 +- internal/orch/steering.go | 28 +++- internal/orch/supervisor.go | 4 +- internal/orch/title.go | 13 ++ internal/orch/worker_mcp.go | 194 ++++++++++++++++++++++++ internal/orch/worker_mcp_test.go | 245 +++++++++++++++++++++++++++++++ internal/store/schema.sql | 1 + internal/store/sessions.go | 16 +- internal/store/store.go | 5 +- 16 files changed, 707 insertions(+), 84 deletions(-) create mode 100644 internal/orch/worker_mcp.go create mode 100644 internal/orch/worker_mcp_test.go diff --git a/cmd/orcha/main.go b/cmd/orcha/main.go index 5735c87..16d4a77 100644 --- a/cmd/orcha/main.go +++ b/cmd/orcha/main.go @@ -182,6 +182,12 @@ func main() { // Manager tool surface (MCP). Manager sessions' Claude connects to // /mcp/ to drive the orchestrator. mux.Handle("/mcp/", http.StripPrefix("/mcp", o.ManagerMCPHandler())) + // Worker tool surface (MCP): the smaller report_result/create_note/ask_user + // subset. Coding workers connect to /wmcp/ to hand their result back. + mux.Handle("/wmcp/", http.StripPrefix("/wmcp", o.WorkerMCPHandler())) + // Follow-up tool surface (MCP): the PR-response tools (update_pr/comment_pr/…) + // plus report_result, but not the manager's spawn/publish/mark-done tools. + mux.Handle("/fmcp/", http.StripPrefix("/fmcp", o.FollowupMCPHandler())) // The dashboard SPA (built from ui/, embedded at compile time). mux.Handle("/", webui.Handler()) diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 054040c..2e8a8e8 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -68,6 +68,11 @@ type Forge interface { // HasDiff reports whether a workspace path has uncommitted/branch changes // worth publishing. HasDiff(ctx context.Context, workspacePath string) (bool, error) + // Diff returns the workspace's full change relative to its base (committed and + // uncommitted), led by a --stat summary so a truncated diff still shows which + // files changed. Used to attach a worker's changes to its result handoff. + // Empty (no error) when there is nothing to show or no base to compare against. + Diff(ctx context.Context, workspacePath string) (string, error) // PushBranch pushes the workspace branch to the repo. force must be // explicitly requested and is recorded with a reason by the caller. PushBranch(ctx context.Context, repo, workspacePath, branch string, force bool) (headSHA string, err error) @@ -132,6 +137,7 @@ type Fake struct { mu sync.Mutex repos map[string]bool diffs map[string]bool // workspacePath -> has diff + diffText map[string]string // workspacePath -> Diff() output prs map[string]*PRState // repo#number -> state openByBranch map[string]*PRState // repo\x00branch -> open PR (for FindOpenPR) nextNum int @@ -170,6 +176,7 @@ func NewFake() *Fake { return &Fake{ repos: map[string]bool{}, diffs: map[string]bool{}, + diffText: map[string]string{}, prs: map[string]*PRState{}, openByBranch: map[string]*PRState{}, issues: map[string]Issue{}, @@ -185,6 +192,9 @@ func (f *Fake) SetRepo(repo string, exists bool) { f.mu.Lock(); f.repos[repo] = // SetDiff marks whether a workspace path has a diff. func (f *Fake) SetDiff(path string, has bool) { f.mu.Lock(); f.diffs[path] = has; f.mu.Unlock() } +// SetDiffText seeds the patch text Diff returns for a workspace path. +func (f *Fake) SetDiffText(path, diff string) { f.mu.Lock(); f.diffText[path] = diff; f.mu.Unlock() } + // SetPRState seeds/overrides the host state for a PR. func (f *Fake) SetPRState(repo string, number int, st PRState) { f.mu.Lock() @@ -215,6 +225,12 @@ func (f *Fake) HasDiff(_ context.Context, workspacePath string) (bool, error) { return true, nil // default: has changes } +func (f *Fake) Diff(_ context.Context, workspacePath string) (string, error) { + f.mu.Lock() + defer f.mu.Unlock() + return f.diffText[workspacePath], nil +} + func (f *Fake) CommitAll(_ context.Context, workspacePath, message string) (bool, error) { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/git.go b/internal/forge/git.go index b0f2dbe..3f70b42 100644 --- a/internal/forge/git.go +++ b/internal/forge/git.go @@ -110,6 +110,38 @@ func (g *GitForge) HasDiff(ctx context.Context, workspacePath string) (bool, err return n > 0, nil } +// Diff returns the workspace's change relative to its base, led by a `--stat` +// summary followed by the full unified patch. Comparing the working tree to the +// base (a two-dot `git diff `) captures both committed and still-uncommitted +// changes, so it reflects everything the worker did. When no base can be resolved +// (e.g. a brand-new repo), it falls back to the uncommitted working-tree diff. +func (g *GitForge) Diff(ctx context.Context, workspacePath string) (string, error) { + base, err := g.resolveBase(ctx, workspacePath) + if err != nil { + base = "" + } + target := base + if target == "" { + target = "HEAD" // no base: show only what is not yet committed + } + stat, err := g.git(ctx, workspacePath, "diff", "--stat", target) + if err != nil { + return "", err + } + patch, err := g.git(ctx, workspacePath, "diff", target) + if err != nil { + return "", err + } + out := strings.TrimSpace(stat) + if p := strings.TrimSpace(patch); p != "" { + if out != "" { + out += "\n\n" + } + out += p + } + return out, nil +} + // resolveBase finds a ref to diff the branch against: the current branch's // upstream if set, else the remote's default branch (origin/HEAD). func (g *GitForge) resolveBase(ctx context.Context, dir string) (string, error) { diff --git a/internal/forge/git_test.go b/internal/forge/git_test.go index 36ab76b..de9897d 100644 --- a/internal/forge/git_test.go +++ b/internal/forge/git_test.go @@ -79,6 +79,50 @@ func TestGitForge_HasDiff(t *testing.T) { } } +func TestGitForge_Diff(t *testing.T) { + work, _ := setupRepo(t) + g := NewGit() + ctx := context.Background() + + // Clean checkout equal to origin/main -> empty diff. + if d, err := g.Diff(ctx, work); err != nil || d != "" { + t.Fatalf("clean checkout: diff=%q err=%v", d, err) + } + + // A committed change on a feature branch shows up vs the base, with the + // changed file named in the leading --stat (so a truncated diff still + // identifies it) and the added line in the patch body. + mustGit(t, work, "checkout", "-b", "feature") + if err := os.WriteFile(filepath.Join(work, "feature.txt"), []byte("hello world\n"), 0o644); err != nil { + t.Fatal(err) + } + mustGit(t, work, "add", ".") + mustGit(t, work, "commit", "-m", "add feature") + + d, err := g.Diff(ctx, work) + if err != nil { + t.Fatalf("diff: %v", err) + } + if !strings.Contains(d, "feature.txt") { + t.Fatalf("diff should name the changed file:\n%s", d) + } + if !strings.Contains(d, "+hello world") { + t.Fatalf("diff should include the added line:\n%s", d) + } + + // An uncommitted edit is also captured (working tree vs base). + if err := os.WriteFile(filepath.Join(work, "feature.txt"), []byte("hello world\nmore\n"), 0o644); err != nil { + t.Fatal(err) + } + d, err = g.Diff(ctx, work) + if err != nil { + t.Fatalf("diff after edit: %v", err) + } + if !strings.Contains(d, "+more") { + t.Fatalf("diff should include the uncommitted line:\n%s", d) + } +} + func TestGitForge_PushBranch(t *testing.T) { work, bare := setupRepo(t) g := NewGit() diff --git a/internal/model/model.go b/internal/model/model.go index 32c85d5..f8f705b 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -242,15 +242,22 @@ type Session struct { Goal string `json:"goal"` CurrentActivity string `json:"current_activity,omitempty"` LatestSummary string `json:"latest_summary,omitempty"` - TargetID string `json:"target_id,omitempty"` - WorkspaceID string `json:"workspace_id,omitempty"` - UsageProvider string `json:"usage_provider,omitempty"` - UsedTokens int64 `json:"used_tokens"` - CreatedAt time.Time `json:"created_at"` - StartedAt *time.Time `json:"started_at,omitempty"` - UpdatedAt time.Time `json:"updated_at"` - CompletedAt *time.Time `json:"completed_at,omitempty"` - Metadata JSONMap `json:"metadata,omitempty"` + // HandoffSummary is the worker-authored result relayed to the manager when the + // session finishes (set via the report_result tool). Unlike LatestSummary — + // which is scraped from the agent's last output and can capture a transitional + // line or noisy TUI pane — this is exactly what the worker chose to hand off + // (findings, a diff, references). Preferred over LatestSummary wherever a + // worker's result is relayed. + HandoffSummary string `json:"handoff_summary,omitempty"` + TargetID string `json:"target_id,omitempty"` + WorkspaceID string `json:"workspace_id,omitempty"` + UsageProvider string `json:"usage_provider,omitempty"` + UsedTokens int64 `json:"used_tokens"` + CreatedAt time.Time `json:"created_at"` + StartedAt *time.Time `json:"started_at,omitempty"` + UpdatedAt time.Time `json:"updated_at"` + CompletedAt *time.Time `json:"completed_at,omitempty"` + Metadata JSONMap `json:"metadata,omitempty"` } // Target is a machine where sessions can run. diff --git a/internal/orch/context.go b/internal/orch/context.go index 5510124..5c6a803 100644 --- a/internal/orch/context.go +++ b/internal/orch/context.go @@ -24,11 +24,7 @@ func (o *Orchestrator) compactContext(objectiveID string) string { if sessions, err := o.st.ListSessionsByObjective(objectiveID); err == nil && len(sessions) > 0 { b.WriteString("SESSION SUMMARIES:\n") for _, s := range sessions { - summary := s.LatestSummary - if summary == "" { - summary = s.CurrentActivity - } - fmt.Fprintf(&b, "- [%s/%s] %s: %s\n", s.Role, s.Status, s.Title, summary) + fmt.Fprintf(&b, "- [%s/%s] %s: %s\n", s.Role, s.Status, s.Title, relaySummaryLine(s)) } b.WriteString("\n") } diff --git a/internal/orch/manager_mcp.go b/internal/orch/manager_mcp.go index 7e3f0d3..74d2bda 100644 --- a/internal/orch/manager_mcp.go +++ b/internal/orch/manager_mcp.go @@ -16,100 +16,149 @@ import ( // "/mcp/", and tool calls are scoped to that session/objective. This // is what turns "the manager session runs" into "the manager actually manages": // its tool calls drive the orchestrator. +// +// The manager gets the full surface. Workers get strict subsets (see +// WorkerMCPHandler / FollowupMCPHandler) so a worker cannot, say, mark the +// objective done or spawn more workers — those are the manager's to decide. func (o *Orchestrator) ManagerMCPHandler() http.Handler { - s := mcp.NewServer("orcha", "0.1") + return mcpServer( + o.toolSpawnSession(), + o.toolAskUser(), + o.toolPublishPR(), + o.toolUpdatePR(), + o.toolCommentPR(), + o.toolAddressPRFeedback(), + o.toolCreateNote(), + o.toolMarkDone(), + o.toolCancelSession(), + ) +} - obj := func(props map[string]any, required ...string) map[string]any { - return map[string]any{"type": "object", "properties": props, "required": required} +// mcpServer builds an MCP HTTP handler exposing exactly the given tools. +func mcpServer(tools ...mcp.Tool) http.Handler { + s := mcp.NewServer("orcha", "0.1") + for _, t := range tools { + s.AddTool(t) } - str := map[string]any{"type": "string"} + return s.Handler() +} - s.AddTool(mcp.Tool{ +// mcpObj/mcpStr are tiny JSON-schema builders shared by the tool constructors. +func mcpObj(props map[string]any, required ...string) map[string]any { + return map[string]any{"type": "object", "properties": props, "required": required} +} + +var mcpStr = map[string]any{"type": "string"} + +func (o *Orchestrator) toolSpawnSession() mcp.Tool { + return mcp.Tool{ Name: "spawn_session", Description: "Spawn a scoped worker session under this objective. Returns the new session id. Use dependencies to make a session wait for others to succeed. To address feedback or push a fix to an existing PR, use address_pr_feedback instead — not spawn_session.", - InputSchema: obj(map[string]any{ + InputSchema: mcpObj(map[string]any{ "role": map[string]any{"type": "string", "enum": []string{"implementer", "reviewer", "validator", "researcher", "custom"}}, - "title": str, - "goal": str, + "title": mcpStr, + "goal": mcpStr, "agent_hint": map[string]any{"type": "string", "enum": []string{"claude", "codex"}}, - "dependencies": map[string]any{"type": "array", "items": str}, + "dependencies": map[string]any{"type": "array", "items": mcpStr}, "repo": map[string]any{"type": "string", "description": "override the objective's repo for this worker's checkout (owner/repo, the upstream)"}, "push_repo": map[string]any{"type": "string", "description": "fork to push branches to (owner/repo); omit to push to repo itself"}, "base_branch": map[string]any{"type": "string", "description": "base branch for the checkout (default main)"}, "target": map[string]any{"type": "string", "description": "pin this worker to a target machine (name or id), e.g. a remote SSH box"}, - "target_labels": map[string]any{"type": "array", "items": str, "description": "require a target with these labels"}, + "target_labels": map[string]any{"type": "array", "items": mcpStr, "description": "require a target with these labels"}, }, "role", "title", "goal"), Handler: o.mcpSpawnSession, - }) - s.AddTool(mcp.Tool{ + } +} + +func (o *Orchestrator) toolAskUser() mcp.Tool { + return mcp.Tool{ Name: "ask_user", Description: "Ask the user a question and block on their input. Use when requirements, credentials, setup, or direction are unclear.", - InputSchema: obj(map[string]any{"question": str, "context": str}, "question"), + InputSchema: mcpObj(map[string]any{"question": mcpStr, "context": mcpStr}, "question"), Handler: o.mcpAskUser, - }) - s.AddTool(mcp.Tool{ + } +} + +func (o *Orchestrator) toolPublishPR() mcp.Tool { + return mcp.Tool{ Name: "publish_pr", Description: "Publish a PR from a worker session's committed changes. The orchestrator verifies mechanical safety, pushes the branch, and opens the PR.", - InputSchema: obj(map[string]any{ - "session_id": str, - "title": str, - "body": str, - "commit_message": str, + InputSchema: mcpObj(map[string]any{ + "session_id": mcpStr, + "title": mcpStr, + "body": mcpStr, + "commit_message": mcpStr, }, "session_id", "title", "body"), Handler: o.mcpPublishPR, - }) - s.AddTool(mcp.Tool{ + } +} + +func (o *Orchestrator) toolUpdatePR() mcp.Tool { + return mcp.Tool{ Name: "update_pr", Description: "Push follow-up changes to an existing PR (branch-safe: never pushes to a merged PR). After a rebase (which rewrites history) set force=true, or the push is rejected as non-fast-forward.", - InputSchema: obj(map[string]any{ - "pr_id": str, - "session_id": str, - "title": str, - "body": str, + InputSchema: mcpObj(map[string]any{ + "pr_id": mcpStr, + "session_id": mcpStr, + "title": mcpStr, + "body": mcpStr, "commit_message": map[string]any{"type": "string", "description": "used only if you left changes uncommitted; prefer committing yourself with git"}, "force": map[string]any{"type": "boolean", "description": "force-push (--force-with-lease); required after a rebase or any history rewrite"}, "force_reason": map[string]any{"type": "string", "description": "why a force push is needed (e.g. 'rebased onto main to resolve conflicts')"}, }, "pr_id"), Handler: o.mcpUpdatePR, - }) - s.AddTool(mcp.Tool{ + } +} + +func (o *Orchestrator) toolCommentPR() mcp.Tool { + return mcp.Tool{ Name: "comment_pr", Description: "Leave a comment on a PR. pr_id accepts the Orcha pr_id or the GitHub PR number.", - InputSchema: obj(map[string]any{"pr_id": str, "body": str}, "pr_id", "body"), + InputSchema: mcpObj(map[string]any{"pr_id": mcpStr, "body": mcpStr}, "pr_id", "body"), Handler: o.mcpCommentPR, - }) - s.AddTool(mcp.Tool{ + } +} + +func (o *Orchestrator) toolAddressPRFeedback() mcp.Tool { + return mcp.Tool{ Name: "address_pr_feedback", Description: "Spawn a follow-up worker to address review feedback or push a fix to an existing PR. " + "Use this for any PR follow-up instead of spawn_session: the worker gets a checkout of the PR " + "branch and pushes its fix back to the same PR. pr_id accepts the Orcha pr_id or the GitHub PR number.", - InputSchema: obj(map[string]any{ - "pr_id": str, + InputSchema: mcpObj(map[string]any{ + "pr_id": mcpStr, "instructions": map[string]any{"type": "string", "description": "what the follow-up should do: the review comments to address or the fix to make"}, "role": map[string]any{"type": "string", "enum": []string{"pr_followup", "ci_followup"}, "description": "ci_followup for CI/check failures; pr_followup (default) for review feedback"}, }, "pr_id", "instructions"), Handler: o.mcpAddressPRFeedback, - }) - s.AddTool(mcp.Tool{ + } +} + +func (o *Orchestrator) toolCreateNote() mcp.Tool { + return mcp.Tool{ Name: "create_note", Description: "Record a note in the objective's shared memory (not stdout).", - InputSchema: obj(map[string]any{"title": str, "body": str}, "title", "body"), + InputSchema: mcpObj(map[string]any{"title": mcpStr, "body": mcpStr}, "title", "body"), Handler: o.mcpCreateNote, - }) - s.AddTool(mcp.Tool{ + } +} + +func (o *Orchestrator) toolMarkDone() mcp.Tool { + return mcp.Tool{ Name: "mark_objective_done", Description: "Mark the objective complete with a concise summary.", - InputSchema: obj(map[string]any{"summary": str}, "summary"), + InputSchema: mcpObj(map[string]any{"summary": mcpStr}, "summary"), Handler: o.mcpMarkDone, - }) - s.AddTool(mcp.Tool{ + } +} + +func (o *Orchestrator) toolCancelSession() mcp.Tool { + return mcp.Tool{ Name: "cancel_session", Description: "Cancel a session (and its children).", - InputSchema: obj(map[string]any{"session_id": str}, "session_id"), + InputSchema: mcpObj(map[string]any{"session_id": mcpStr}, "session_id"), Handler: o.mcpCancelSession, - }) - - return s.Handler() + } } // managerSession resolves the calling manager session from the request context. diff --git a/internal/orch/sessions.go b/internal/orch/sessions.go index 26a4222..44996d6 100644 --- a/internal/orch/sessions.go +++ b/internal/orch/sessions.go @@ -233,10 +233,12 @@ func (o *Orchestrator) consume(r *run, events <-chan agent.Event) { // parent manager is handed when this session finishes. Without this // LatestSummary is never written and the handoff says nothing useful. if ev.Kind == agent.EventText && ev.Source == model.MsgAgent && strings.TrimSpace(ev.Content) != "" { - summary := ev.Content - if len(summary) > 2000 { - summary = summary[:2000] - } + // Keep the TAIL, rune-safe: this is a fallback for when a worker did + // not call report_result, and the meaningful conclusion sits at the + // end of the message (or the bottom of a scraped TUI pane). Byte + // slicing here previously split multibyte box-drawing chars and, worse, + // kept the noisy build-log preamble while dropping the findings. + summary := tailRunes(ev.Content, 2000) _, _ = o.st.UpdateSessionRuntime(r.sessionID, func(s *model.Session) { s.LatestSummary = summary }) @@ -345,10 +347,7 @@ func (o *Orchestrator) notifyManagerOfChild(childID string, success bool) { if err != nil || mgr.Role != model.RoleManager || mgr.Status.IsTerminal() { return } - summary := child.LatestSummary - if summary == "" { - summary = child.CurrentActivity - } + summary := relaySummary(child) var msg string switch { case !success: diff --git a/internal/orch/steering.go b/internal/orch/steering.go index db43239..6be0952 100644 --- a/internal/orch/steering.go +++ b/internal/orch/steering.go @@ -135,10 +135,12 @@ func (o *Orchestrator) buildSpec(sess *model.Session, ws *model.Workspace, tgt * spec.Prompt = managerSystemPreamble + o.managerContext(sess) + "\n\n" + spec.Prompt } // PR/CI follow-up: the agent itself decides how to respond, using its tools - // (update_pr to push a fix, comment_pr to reply, ask_user, create_note). + // (update_pr to push a fix, comment_pr to reply, ask_user, create_note, + // report_result). It gets the follow-up surface — the PR-response tools, NOT + // the manager's spawn/publish/mark-done tools. case sess.Role == model.RolePRFollowup || sess.Role == model.RoleCIFollowup: if o.cfg.ManagerMCPBaseURL != "" { - spec.MCP = map[string]string{"orcha": o.mcpBaseFor(tgt) + "/mcp/" + sess.ID} + spec.MCP = map[string]string{"orcha": o.mcpBaseFor(tgt) + "/fmcp/" + sess.ID} spec.AllowedTools = []string{"mcp__orcha"} } spec.PermissionMode = o.cfg.WorkerPermissionMode // shell so it can commit @@ -146,8 +148,14 @@ func (o *Orchestrator) buildSpec(sess *model.Session, ws *model.Workspace, tgt * if spec.Prompt != "" { spec.Prompt = followupSystemPreamble + completionInstruction + "\n\n" + spec.Prompt } - // Other coding workers run one-shot in a checkout and do not publish. + // Other coding workers run one-shot in a checkout and do not publish. They get + // the small worker tool surface (report_result/create_note/ask_user) so they + // choose what to hand back to the manager instead of leaving it to a pane scrape. case isCodingWorker(sess.Role): + if o.cfg.ManagerMCPBaseURL != "" { + spec.MCP = map[string]string{"orcha": o.mcpBaseFor(tgt) + "/wmcp/" + sess.ID} + spec.AllowedTools = []string{"mcp__orcha"} + } spec.PermissionMode = o.cfg.WorkerPermissionMode spec.OneShot = true if spec.Prompt != "" { @@ -180,7 +188,10 @@ orcha MCP tools (named mcp__orcha*): - If you are blocked or need a decision: call ask_user. Always leave at least a comment so the reviewer knows the outcome. Commit with git, but do not "git push" or use the gh CLI directly and do not change the git -author/identity — push and comment through the tools.` +author/identity — push and comment through the tools. +When you are done, call report_result with what you did (the fix you pushed, the +reply you posted, or why no action was warranted) — that is what your manager +sees. Call it before printing the done marker.` // workerSystemPreamble orients a one-shot worker. const workerSystemPreamble = `You are a worker on an engineering team, running in an @@ -195,7 +206,14 @@ A long build or test run is expected and fine — let it finish; do not abandon just because it is slow. Only if a command is genuinely hung (no progress for a long time) in code unrelated to your change should you stop waiting on it, say so, and proceed. -Finish with a brief summary of what you changed.` +When you are done, call the report_result tool (one of your orcha MCP tools) with +the result your manager needs: the actual outcome — what you changed and why, or, +for a review, every finding with concrete file:line references — not a teaser like +"I'm preparing the review". This tool call IS your handoff; whatever you put there +is what the manager sees, so do not rely on your scrollback surviving. Set +include_diff: true to attach your diff. For a long write-up, create_note it and +pass its id in report_result's notes. Call report_result BEFORE printing the done +marker.` // managerSystemPreamble orients the manager agent toward the tool surface and // the operating rules from the spec. diff --git a/internal/orch/supervisor.go b/internal/orch/supervisor.go index 4c104fe..3bbccd9 100644 --- a/internal/orch/supervisor.go +++ b/internal/orch/supervisor.go @@ -32,7 +32,7 @@ const maxManagerSessions = 4 type supervisorAction struct { kind string objectiveID string - prompt string // objective prompt, for respawn + prompt string // objective prompt, for respawn agent model.AgentKind // manager agent to reuse, for respawn manager *model.Session // the live manager, for poke } @@ -217,7 +217,7 @@ func (o *Orchestrator) objectiveStateSnapshot(objectiveID string) string { continue } wrote = true - sum := firstNonEmpty(s.LatestSummary, s.CurrentActivity) + sum := relaySummaryLine(s) if sum == "" { sum = "(no summary reported)" } diff --git a/internal/orch/title.go b/internal/orch/title.go index c79896a..e367b88 100644 --- a/internal/orch/title.go +++ b/internal/orch/title.go @@ -50,6 +50,19 @@ func truncateRunes(s string, max int) string { return strings.TrimRight(string(r[:max-1]), " ") + "…" } +// tailRunes keeps at most the last max runes of s, prefixing an ellipsis when it +// had to cut. It counts runes (not bytes) so multibyte text — box-drawing chars +// in a scraped TUI pane, say — is never split mid-character. The TAIL is kept on +// purpose: an agent's conclusion (and the bottom of a settled pane) is at the +// end, so head-truncation would drop exactly the part that matters. +func tailRunes(s string, max int) string { + r := []rune(s) + if len(r) <= max { + return s + } + return "…" + strings.TrimLeft(string(r[len(r)-(max-1):]), " ") +} + // sanitizeTitle cleans an LLM-produced title into a single tidy line: it keeps // the first non-empty line (models sometimes add preamble or trailing notes), // strips surrounding quotes and whitespace, drops trailing sentence diff --git a/internal/orch/worker_mcp.go b/internal/orch/worker_mcp.go new file mode 100644 index 0000000..5503b6d --- /dev/null +++ b/internal/orch/worker_mcp.go @@ -0,0 +1,194 @@ +package orch + +import ( + "context" + "fmt" + "net/http" + "strings" + + "github.com/nathanwhit/orcha/internal/mcp" + "github.com/nathanwhit/orcha/internal/model" +) + +// Bounds on what a single report_result handoff carries. The handoff is stored on +// the session row and relayed verbatim to the manager, so it must be generous +// enough to hold real findings yet bounded so a runaway diff can't bloat the row +// or the manager's context. +const ( + maxHandoffText = 6000 // the worker's authored summary + maxHandoffNote = 1200 // each referenced note's inlined body + maxHandoffDiff = 8000 // the attached diff (stat + patch) +) + +// WorkerMCPHandler returns the MCP tool surface for one-shot coding workers +// (implementer/reviewer/validator/researcher/custom). It is deliberately a SMALL +// subset of the manager surface: a worker may report its result, record a +// shared-memory note, and ask the user — but it cannot spawn sessions, +// publish/merge PRs, or mark the objective done (those are the manager's to +// decide). Mount it under "/wmcp/"; each worker connects to "/wmcp/". +func (o *Orchestrator) WorkerMCPHandler() http.Handler { + return mcpServer( + o.toolReportResult(), + o.toolCreateNote(), + o.toolAskUser(), + ) +} + +// FollowupMCPHandler returns the MCP tool surface for PR/CI follow-up workers. +// A follow-up acts on an EXISTING PR, so it gets the PR-response tools on top of +// the base worker surface — but, like any worker, NOT spawn_session, publish_pr, +// mark_objective_done, or cancel_session (which previously leaked in because +// follow-ups shared the full manager surface). Mount it under "/fmcp/". +func (o *Orchestrator) FollowupMCPHandler() http.Handler { + return mcpServer( + o.toolUpdatePR(), + o.toolCommentPR(), + o.toolCreateNote(), + o.toolAskUser(), + o.toolReportResult(), + ) +} + +// toolReportResult builds the report_result tool. Workers (coding and follow-up) +// call it to author exactly what is relayed to the manager. +func (o *Orchestrator) toolReportResult() mcp.Tool { + return mcp.Tool{ + Name: "report_result", + Description: "Hand your result back to the manager. THIS is what the manager sees when you finish — " + + "put the conclusion that matters here (review findings with file:line refs, what you changed and why, " + + "or why you couldn't). Set include_diff to attach your diff. Call this once, when you are done, " + + "before printing the done marker.", + InputSchema: mcpObj(map[string]any{ + "summary": map[string]any{"type": "string", "description": "the result to relay to the manager (the full findings/outcome, not a teaser)"}, + "include_diff": map[string]any{"type": "boolean", "description": "attach your checkout's diff vs base (committed + uncommitted) so the manager can see the actual change"}, + "notes": map[string]any{"type": "array", "items": mcpStr, "description": "ids of notes you created with create_note to inline into the handoff (for long content kept in shared memory)"}, + }, "summary"), + Handler: o.mcpReportResult, + } +} + +// mcpReportResult records the worker-authored handoff that is relayed to the +// manager when the session finishes. The worker chooses exactly what to relay: +// its summary, optionally the checkout's diff, and optionally the bodies of notes +// it stashed in shared memory. Stored in HandoffSummary, which is preferred over +// the scraped LatestSummary everywhere a worker's result is surfaced. +func (o *Orchestrator) mcpReportResult(ctx context.Context, args map[string]any) (string, error) { + id := mcp.SessionFromContext(ctx) + if id == "" { + return "", fmt.Errorf("no session bound to request") + } + sess, err := o.st.GetSession(id) + if err != nil { + return "", err + } + summary := strings.TrimSpace(mcp.StringArg(args, "summary")) + if summary == "" { + return "", fmt.Errorf("summary is required") + } + + var b strings.Builder + b.WriteString(truncateRunes(summary, maxHandoffText)) + + // Inline the bodies of any referenced notes so the content actually reaches + // the manager (shared-memory notes are not otherwise folded into its context). + if noteIDs := mcp.StringsArg(args, "notes"); len(noteIDs) > 0 { + if notes := o.collectNotes(sess.ObjectiveID, noteIDs); notes != "" { + b.WriteString("\n\nReferenced notes:\n") + b.WriteString(notes) + } + } + + diffAttached := false + if mcp.BoolArg(args, "include_diff") { + if diff := o.workerDiff(ctx, sess); diff != "" { + b.WriteString("\n\n--- Changes (diff vs base) ---\n") + b.WriteString(truncateRunes(diff, maxHandoffDiff)) + diffAttached = true + } + } + + handoff := b.String() + if _, err := o.st.UpdateSessionRuntime(id, func(s *model.Session) { + s.HandoffSummary = handoff + }); err != nil { + return "", err + } + o.audit(sess.ObjectiveID, id, "worker_reported", "worker recorded its result handoff", + model.JSONMap{"include_diff": diffAttached}) + + msg := "result recorded; it will be relayed to your manager when you finish." + if mcp.BoolArg(args, "include_diff") && !diffAttached { + msg += " (no diff was attached — nothing to compare against the base)" + } + return msg, nil +} + +// workerDiff returns the session's checkout diff vs base, run on the worker's +// target (where the checkout lives). Empty on any failure — a missing diff must +// never block a worker from reporting. +func (o *Orchestrator) workerDiff(ctx context.Context, sess *model.Session) string { + if o.forge == nil || sess.WorkspaceID == "" { + return "" + } + ws, err := o.st.GetWorkspace(sess.WorkspaceID) + if err != nil || ws.Path == "" { + return "" + } + diff, err := o.forgeForWorkspace(ws).Diff(ctx, ws.Path) + if err != nil { + return "" + } + return strings.TrimSpace(diff) +} + +// relaySummary is what gets handed to the manager when a worker's result is +// surfaced in full (the completion notification). It prefers the worker-authored +// handoff (report_result), falling back to the scraped last output / activity for +// a worker that finished without reporting. +func relaySummary(s *model.Session) string { + switch { + case strings.TrimSpace(s.HandoffSummary) != "": + return s.HandoffSummary + case strings.TrimSpace(s.LatestSummary) != "": + return s.LatestSummary + default: + return s.CurrentActivity + } +} + +// relaySummaryLine is the compact one-line form of relaySummary, for context +// listings and status snapshots that show one worker per line. It keeps the first +// non-empty line so a multi-paragraph handoff doesn't blow out a one-line view. +func relaySummaryLine(s *model.Session) string { + full := relaySummary(s) + for _, ln := range strings.Split(full, "\n") { + if t := strings.TrimSpace(ln); t != "" { + return truncateRunes(t, 280) + } + } + return "" +} + +// collectNotes renders the referenced notes (matched by id within the objective) +// as a bounded, inlined list. Unknown ids are skipped. +func (o *Orchestrator) collectNotes(objectiveID string, ids []string) string { + if objectiveID == "" { + return "" + } + arts, err := o.st.ListArtifactsByObjective(objectiveID) + if err != nil { + return "" + } + want := make(map[string]bool, len(ids)) + for _, id := range ids { + want[id] = true + } + var b strings.Builder + for _, a := range arts { + if !want[a.ID] { + continue + } + fmt.Fprintf(&b, "- %s:\n%s\n", a.Title, truncateRunes(strings.TrimSpace(a.Summary), maxHandoffNote)) + } + return b.String() +} diff --git a/internal/orch/worker_mcp_test.go b/internal/orch/worker_mcp_test.go new file mode 100644 index 0000000..72df0ad --- /dev/null +++ b/internal/orch/worker_mcp_test.go @@ -0,0 +1,245 @@ +package orch + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "sort" + "strings" + "testing" + + "github.com/nathanwhit/orcha/internal/forge" + "github.com/nathanwhit/orcha/internal/mcp" + "github.com/nathanwhit/orcha/internal/model" + "github.com/nathanwhit/orcha/internal/store" +) + +// listTools returns the tool names an MCP handler exposes, via tools/list. +func listTools(t *testing.T, h http.Handler) []string { + t.Helper() + req := httptest.NewRequest("POST", "/sess1", bytes.NewBufferString(`{"jsonrpc":"2.0","id":1,"method":"tools/list"}`)) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + var out map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil { + t.Fatalf("decode tools/list: %v (%s)", err, rec.Body.String()) + } + res, _ := out["result"].(map[string]any) + raw, _ := res["tools"].([]any) + var names []string + for _, x := range raw { + if m, ok := x.(map[string]any); ok { + names = append(names, m["name"].(string)) + } + } + sort.Strings(names) + return names +} + +// The three MCP surfaces must expose exactly their intended tools — in +// particular a worker/follow-up must NOT see spawn_session, publish_pr, +// mark_objective_done, or cancel_session. +func TestMCPSurfaces_ToolScoping(t *testing.T) { + o, _ := newTestOrch(t) + + manager := listTools(t, o.ManagerMCPHandler()) + wantManager := []string{"address_pr_feedback", "ask_user", "cancel_session", "comment_pr", + "create_note", "mark_objective_done", "publish_pr", "spawn_session", "update_pr"} + if strings.Join(manager, ",") != strings.Join(wantManager, ",") { + t.Fatalf("manager surface = %v, want %v", manager, wantManager) + } + + worker := listTools(t, o.WorkerMCPHandler()) + wantWorker := []string{"ask_user", "create_note", "report_result"} + if strings.Join(worker, ",") != strings.Join(wantWorker, ",") { + t.Fatalf("worker surface = %v, want %v", worker, wantWorker) + } + + followup := listTools(t, o.FollowupMCPHandler()) + wantFollowup := []string{"ask_user", "comment_pr", "create_note", "report_result", "update_pr"} + if strings.Join(followup, ",") != strings.Join(wantFollowup, ",") { + t.Fatalf("followup surface = %v, want %v", followup, wantFollowup) + } + + // The escalation tools must never leak into a non-manager surface. + for _, banned := range []string{"spawn_session", "publish_pr", "mark_objective_done", "cancel_session"} { + for _, n := range append(append([]string{}, worker...), followup...) { + if n == banned { + t.Fatalf("%q must not be exposed to workers/follow-ups", banned) + } + } + } +} + +// reportCtx binds a session id to a context the way the MCP server does, so the +// report_result handler resolves the calling worker. +func reportCtx(sessionID string) context.Context { + return mcp.WithSession(context.Background(), sessionID) +} + +// seedWorker creates an objective + a worker session with a ready workspace. +func seedWorker(t *testing.T, o *Orchestrator, st *store.Store) (*model.Session, *model.Workspace) { + t.Helper() + obj, mgr, err := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + if err != nil { + t.Fatalf("create objective: %v", err) + } + w := &model.Session{ + ObjectiveID: obj.ID, ParentSessionID: mgr.ID, Role: model.RoleReviewer, + Agent: model.AgentCodex, Status: model.SessionRunning, Title: "Review", + } + if err := st.CreateSession(w); err != nil { + t.Fatalf("create worker: %v", err) + } + ws := &model.Workspace{ + ObjectiveID: obj.ID, SessionID: w.ID, Kind: model.WorkspaceIsolated, + Path: "/work/" + w.ID, Status: model.WorkspaceReady, + } + if err := st.CreateWorkspace(ws); err != nil { + t.Fatalf("create workspace: %v", err) + } + if _, err := st.UpdateSessionRuntime(w.ID, func(s *model.Session) { s.WorkspaceID = ws.ID }); err != nil { + t.Fatalf("bind workspace: %v", err) + } + w, _ = st.GetSession(w.ID) + return w, ws +} + +func TestReportResult_SetsHandoffSummary(t *testing.T) { + o, st := newTestOrch(t) + o.SetForge(forge.NewFake()) + w, _ := seedWorker(t, o, st) + + _, err := o.mcpReportResult(reportCtx(w.ID), map[string]any{ + "summary": "Found 1 medium issue: SSH metadata deletion at Targets.tsx:214.", + }) + if err != nil { + t.Fatalf("report_result: %v", err) + } + got, _ := st.GetSession(w.ID) + if !strings.Contains(got.HandoffSummary, "SSH metadata deletion at Targets.tsx:214") { + t.Fatalf("handoff missing findings: %q", got.HandoffSummary) + } +} + +func TestReportResult_AttachesDiff(t *testing.T) { + o, st := newTestOrch(t) + f := forge.NewFake() + o.SetForge(f) + w, ws := seedWorker(t, o, st) + f.SetDiffText(ws.Path, "diff --git a/x b/x\n+added line") + + if _, err := o.mcpReportResult(reportCtx(w.ID), map[string]any{ + "summary": "Implemented the change.", + "include_diff": true, + }); err != nil { + t.Fatalf("report_result: %v", err) + } + got, _ := st.GetSession(w.ID) + if !strings.Contains(got.HandoffSummary, "Implemented the change.") || + !strings.Contains(got.HandoffSummary, "+added line") { + t.Fatalf("handoff missing summary or diff: %q", got.HandoffSummary) + } +} + +func TestReportResult_IncludeDiffNoChanges(t *testing.T) { + o, st := newTestOrch(t) + o.SetForge(forge.NewFake()) // no diff text seeded -> empty diff + w, _ := seedWorker(t, o, st) + + msg, err := o.mcpReportResult(reportCtx(w.ID), map[string]any{ + "summary": "Read-only review; no changes.", + "include_diff": true, + }) + if err != nil { + t.Fatalf("report_result: %v", err) + } + if !strings.Contains(msg, "no diff was attached") { + t.Fatalf("expected a no-diff note, got %q", msg) + } + got, _ := st.GetSession(w.ID) + if strings.Contains(got.HandoffSummary, "diff vs base") { + t.Fatalf("should not have a diff section: %q", got.HandoffSummary) + } +} + +func TestReportResult_InlinesReferencedNotes(t *testing.T) { + o, st := newTestOrch(t) + o.SetForge(forge.NewFake()) + w, _ := seedWorker(t, o, st) + + note, err := o.CreateNote(w.ID, "Full review", "Three findings, the worst at api.go:563.") + if err != nil { + t.Fatalf("create note: %v", err) + } + if _, err := o.mcpReportResult(reportCtx(w.ID), map[string]any{ + "summary": "See the full review note.", + "notes": []any{note.ID}, + }); err != nil { + t.Fatalf("report_result: %v", err) + } + got, _ := st.GetSession(w.ID) + if !strings.Contains(got.HandoffSummary, "Full review") || + !strings.Contains(got.HandoffSummary, "api.go:563") { + t.Fatalf("handoff missing inlined note: %q", got.HandoffSummary) + } +} + +func TestReportResult_RequiresSummary(t *testing.T) { + o, st := newTestOrch(t) + o.SetForge(forge.NewFake()) + w, _ := seedWorker(t, o, st) + + if _, err := o.mcpReportResult(reportCtx(w.ID), map[string]any{"summary": " "}); err == nil { + t.Fatal("expected an error for an empty summary") + } +} + +func TestRelaySummary_PrefersHandoff(t *testing.T) { + s := &model.Session{ + HandoffSummary: "authoritative findings", + LatestSummary: "scraped pane noise", + CurrentActivity: "running build", + } + if got := relaySummary(s); got != "authoritative findings" { + t.Fatalf("relaySummary = %q, want the handoff", got) + } + // Falls back to the scrape when no handoff was recorded. + s.HandoffSummary = "" + if got := relaySummary(s); got != "scraped pane noise" { + t.Fatalf("relaySummary fallback = %q, want LatestSummary", got) + } + s.LatestSummary = "" + if got := relaySummary(s); got != "running build" { + t.Fatalf("relaySummary fallback = %q, want CurrentActivity", got) + } +} + +func TestRelaySummaryLine_FirstLineCapped(t *testing.T) { + s := &model.Session{HandoffSummary: "\n headline finding \n\nlong details below\n"} + if got := relaySummaryLine(s); got != "headline finding" { + t.Fatalf("relaySummaryLine = %q, want the first non-empty line", got) + } +} + +func TestTailRunes(t *testing.T) { + // Keeps the tail (the conclusion), rune-safe: a box-drawing-heavy string is + // never split mid-character, and the END is what survives. + s := strings.Repeat("─", 50) + "FINDINGS" + got := tailRunes(s, 10) + if !strings.HasSuffix(got, "FINDINGS") { + t.Fatalf("tailRunes dropped the tail: %q", got) + } + if !strings.HasPrefix(got, "…") { + t.Fatalf("tailRunes should mark truncation: %q", got) + } + if []rune(got)[1] == '�' { + t.Fatalf("tailRunes split a multibyte rune: %q", got) + } + // Short input is returned unchanged. + if got := tailRunes("hi", 10); got != "hi" { + t.Fatalf("tailRunes mangled short input: %q", got) + } +} diff --git a/internal/store/schema.sql b/internal/store/schema.sql index 306f1fd..7ad686a 100644 --- a/internal/store/schema.sql +++ b/internal/store/schema.sql @@ -27,6 +27,7 @@ CREATE TABLE IF NOT EXISTS sessions ( goal TEXT NOT NULL DEFAULT '', current_activity TEXT NOT NULL DEFAULT '', latest_summary TEXT NOT NULL DEFAULT '', + handoff_summary TEXT NOT NULL DEFAULT '', target_id TEXT, workspace_id TEXT, usage_provider TEXT, diff --git a/internal/store/sessions.go b/internal/store/sessions.go index bff2ff4..b99fe93 100644 --- a/internal/store/sessions.go +++ b/internal/store/sessions.go @@ -26,13 +26,13 @@ func (s *Store) CreateSession(sess *model.Session) error { } _, err := s.db.Exec( `INSERT INTO sessions(id, objective_id, parent_session_id, role, agent, mode, - status, title, goal, current_activity, latest_summary, target_id, - workspace_id, usage_provider, used_tokens, created_at, started_at, + status, title, goal, current_activity, latest_summary, handoff_summary, + target_id, workspace_id, usage_provider, used_tokens, created_at, started_at, updated_at, completed_at, metadata) - VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)`, + VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)`, sess.ID, nullStr(sess.ObjectiveID), nullStr(sess.ParentSessionID), string(sess.Role), string(sess.Agent), string(sess.Mode), string(sess.Status), - sess.Title, sess.Goal, sess.CurrentActivity, sess.LatestSummary, + sess.Title, sess.Goal, sess.CurrentActivity, sess.LatestSummary, sess.HandoffSummary, nullStr(sess.TargetID), nullStr(sess.WorkspaceID), nullStr(sess.UsageProvider), sess.UsedTokens, sess.CreatedAt, nullTime(sess.StartedAt), sess.UpdatedAt, nullTime(sess.CompletedAt), sess.Metadata, @@ -41,7 +41,7 @@ func (s *Store) CreateSession(sess *model.Session) error { } var sessionCols = `id, objective_id, parent_session_id, role, agent, mode, status, - title, goal, current_activity, latest_summary, target_id, workspace_id, + title, goal, current_activity, latest_summary, handoff_summary, target_id, workspace_id, usage_provider, used_tokens, created_at, started_at, updated_at, completed_at, metadata` func scanSession(r rowScanner) (*model.Session, error) { @@ -49,7 +49,7 @@ func scanSession(r rowScanner) (*model.Session, error) { var obj, parent, target, ws, provider sql.NullString var started, completed sql.NullTime err := r.Scan(&s.ID, &obj, &parent, &s.Role, &s.Agent, &s.Mode, &s.Status, - &s.Title, &s.Goal, &s.CurrentActivity, &s.LatestSummary, &target, &ws, + &s.Title, &s.Goal, &s.CurrentActivity, &s.LatestSummary, &s.HandoffSummary, &target, &ws, &provider, &s.UsedTokens, &s.CreatedAt, &started, &s.UpdatedAt, &completed, &s.Metadata) if errors.Is(err, sql.ErrNoRows) { return nil, ErrNotFound @@ -213,9 +213,9 @@ func (s *Store) UpdateSessionRuntime(id string, fn func(*model.Session)) (*model sess.UpdatedAt = now _, err = tx.Exec( `UPDATE sessions SET title = ?, goal = ?, current_activity = ?, - latest_summary = ?, target_id = ?, workspace_id = ?, + latest_summary = ?, handoff_summary = ?, target_id = ?, workspace_id = ?, usage_provider = ?, metadata = ?, updated_at = ? WHERE id = ?`, - sess.Title, sess.Goal, sess.CurrentActivity, sess.LatestSummary, + sess.Title, sess.Goal, sess.CurrentActivity, sess.LatestSummary, sess.HandoffSummary, nullStr(sess.TargetID), nullStr(sess.WorkspaceID), nullStr(sess.UsageProvider), sess.Metadata, now, id) if err != nil { diff --git a/internal/store/store.go b/internal/store/store.go index cbbeea1..d1ff1d4 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -91,7 +91,10 @@ func Open(path string, opts ...Option) (*Store, error) { // explicit, idempotent ALTER for older DBs. New DBs already have the column // from schema.sql and these calls become no-ops. func (s *Store) migrate() error { - return s.ensureColumn("sessions", "used_tokens", "INTEGER NOT NULL DEFAULT 0") + if err := s.ensureColumn("sessions", "used_tokens", "INTEGER NOT NULL DEFAULT 0"); err != nil { + return err + } + return s.ensureColumn("sessions", "handoff_summary", "TEXT NOT NULL DEFAULT ''") } // ensureColumn adds a column to a table if it is not already present. Existing