From 5b23fbc994575c45121995a6b39e8a5d248091b4 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 15 Jun 2026 12:59:43 -0700 Subject: [PATCH] fix(orch): make PR follow-ups land their fixes on the PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A manager-spawned pr_followup (#17) hit 'store: not found' on update_pr and its fix commit was stranded — three layered root causes, all fixed here. RC1 (id mismatch): update_pr/comment_pr required the internal Orcha pr_id but agents naturally pass the GitHub PR number they see everywhere. Add resolvePR: accept either, resolving a bare number within the caller's objective, and on miss return an error listing the objective's PRs and ids instead of a bare 'not found'. RC2 (no workspace): a follow-up's PR-branch checkout + pr_id were only wired up in the automatic ProcessFeedback path. A manager spawning pr_followup/ci_followup via spawn_session got none of it, so the worker ran in a scratch dir and its commit went to a stranded branch. Extract spawnPRFollowup as the single creation path; add an address_pr_feedback manager tool; remove the follow-up roles from spawn_session and reject them there so the wiring can't be bypassed. RC3 (silent wrong push): UpdatePR fell back to the orchestrator's own cwd when no workspace resolved, pushing the wrong branch (or nothing) silently. Resolve the checkout from the PR's own branch workspace and refuse to push when none exists. Updates manager steering/guidance to point at address_pr_feedback. Adds tests covering id resolution, the spawn_session guard, address_pr_feedback wiring, and the no-checkout refusal. --- internal/orch/feedback.go | 39 ++++++---- internal/orch/manager.go | 6 +- internal/orch/manager_mcp.go | 117 +++++++++++++++++++++++++++-- internal/orch/orch_test.go | 138 +++++++++++++++++++++++++++++++++++ internal/orch/pr.go | 63 +++++++++++++--- internal/orch/steering.go | 11 +-- 6 files changed, 336 insertions(+), 38 deletions(-) diff --git a/internal/orch/feedback.go b/internal/orch/feedback.go index 1c39a40..126f969 100644 --- a/internal/orch/feedback.go +++ b/internal/orch/feedback.go @@ -78,6 +78,28 @@ func (o *Orchestrator) ProcessFeedback(ctx context.Context, prID string) ([]*mod checkLogs = append(checkLogs, string(f.Kind)+": "+f.Body) } + sess, err := o.spawnPRFollowup(ctx, pr, role, strings.Join(checkLogs, "\n")) + if err != nil { + return nil, err + } + for _, f := range pending { + _ = o.st.MarkFeedbackHandled(f.ID, sess.ID) + } + o.audit(pr.ObjectiveID, sess.ID, "followup_spawned", + fmt.Sprintf("spawned %s for PR #%d", role, pr.Number), model.JSONMap{"pr_id": prID}) + return []*model.Session{sess}, nil +} + +// spawnPRFollowup creates a follow-up session bound to a PR and returns it. It is +// the single path for creating PR/CI follow-ups: it provisions the PR-branch +// workspace (the canonical checkout for the PR's branch, which update_pr pushes +// from), bakes the PR's internal id and branch into the goal, and records the +// pr_id in session metadata. Both the automatic feedback path (ProcessFeedback) +// and the manager's address_pr_feedback tool go through here, so a follow-up can +// never be created without the checkout it must push its fix back from. feedback +// is the body the agent acts on (review comments, check logs, or the manager's +// instructions). +func (o *Orchestrator) spawnPRFollowup(ctx context.Context, pr *model.PullRequest, role model.SessionRole, feedback string) (*model.Session, error) { // PR-branch workspace for the follow-up. Pick a schedulable target. target, err := o.SelectTarget(TargetRequest{}) if err != nil { @@ -102,7 +124,7 @@ func (o *Orchestrator) ProcessFeedback(ctx context.Context, prID string) ([]*mod BaseSHA: pr.HeadSHA, BranchName: pr.Branch, Status: model.WorkspacePreparing, - Metadata: prWorkspaceMeta(pr.Repo, prID, cloneURL, pushRepo), + Metadata: prWorkspaceMeta(pr.Repo, pr.ID, cloneURL, pushRepo), } if err := o.st.CreateWorkspace(ws); err != nil { return nil, err @@ -124,9 +146,9 @@ func (o *Orchestrator) ProcessFeedback(ctx context.Context, prID string) ([]*mod goal := fmt.Sprintf("Address feedback on PR #%d (%q) in repo %s.\n"+ "Use orcha pr_id=%q for your tool calls (update_pr / comment_pr take pr_id).\n"+ "This checkout is the PR branch %q with origin set.\n\nFeedback:\n%s\n\nPrior PR summary: %s", - pr.Number, pr.Title, pr.Repo, prID, pr.Branch, strings.Join(checkLogs, "\n"), pr.Summary) + pr.Number, pr.Title, pr.Repo, pr.ID, pr.Branch, feedback, pr.Summary) - sess, err := o.CreateSession(SpawnSpec{ + return o.CreateSession(SpawnSpec{ ObjectiveID: pr.ObjectiveID, Role: role, Agent: o.defaultAgent(), @@ -134,17 +156,8 @@ func (o *Orchestrator) ProcessFeedback(ctx context.Context, prID string) ([]*mod Title: fmt.Sprintf("Follow-up: PR #%d", pr.Number), Goal: goal, WorkspaceID: ws.ID, - Metadata: model.JSONMap{"pr_id": prID}, + Metadata: model.JSONMap{"pr_id": pr.ID}, }) - if err != nil { - return nil, err - } - for _, f := range pending { - _ = o.st.MarkFeedbackHandled(f.ID, sess.ID) - } - o.audit(pr.ObjectiveID, sess.ID, "followup_spawned", - fmt.Sprintf("spawned %s for PR #%d", role, pr.Number), model.JSONMap{"pr_id": prID}) - return []*model.Session{sess}, nil } // orchaBotMarker tags orcha's own PR comments so the monitor doesn't react to diff --git a/internal/orch/manager.go b/internal/orch/manager.go index 19bbc6e..852e463 100644 --- a/internal/orch/manager.go +++ b/internal/orch/manager.go @@ -10,7 +10,7 @@ import ( ) // The manager tool surface. These methods back the manager agent's tools -// (spawn_session, ask_user, publish_pr, update_pr, comment_pr, create_note, +// (spawn_session, ask_user, publish_pr, update_pr, comment_pr, address_pr_feedback, create_note, // mark_objective_done, cancel_session). Each acquires the objective_manager // lock so only one manager mutation per objective happens at a time. @@ -176,8 +176,8 @@ func (o *Orchestrator) MarkObjectiveDone(managerSessionID, summary string) error if len(open) > 0 { return fmt.Errorf("objective is not done: %d open PR(s) %v are not merged yet. "+ "It completes when its PRs merge — you are steered automatically then. If a PR "+ - "needs review replies, CI fixes, or conflict resolution, push fixes with update_pr "+ - "or spawn a follow-up; do NOT mark the objective done now", len(open), open) + "needs review replies, CI fixes, or conflict resolution, call address_pr_feedback "+ + "to push fixes; do NOT mark the objective done now", len(open), open) } } if err := o.withManagerLock(mgr.ObjectiveID, managerSessionID, func() error { diff --git a/internal/orch/manager_mcp.go b/internal/orch/manager_mcp.go index 6825d45..7e3f0d3 100644 --- a/internal/orch/manager_mcp.go +++ b/internal/orch/manager_mcp.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "net/http" + "strconv" + "strings" "github.com/nathanwhit/orcha/internal/mcp" "github.com/nathanwhit/orcha/internal/model" @@ -24,9 +26,9 @@ func (o *Orchestrator) ManagerMCPHandler() http.Handler { s.AddTool(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.", + 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{ - "role": map[string]any{"type": "string", "enum": []string{"implementer", "reviewer", "validator", "researcher", "pr_followup", "ci_followup", "custom"}}, + "role": map[string]any{"type": "string", "enum": []string{"implementer", "reviewer", "validator", "researcher", "custom"}}, "title": str, "goal": str, "agent_hint": map[string]any{"type": "string", "enum": []string{"claude", "codex"}}, @@ -72,10 +74,22 @@ func (o *Orchestrator) ManagerMCPHandler() http.Handler { }) s.AddTool(mcp.Tool{ Name: "comment_pr", - Description: "Leave a comment on a 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"), Handler: o.mcpCommentPR, }) + s.AddTool(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, + "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{ Name: "create_note", Description: "Record a note in the objective's shared memory (not stdout).", @@ -112,6 +126,14 @@ func (o *Orchestrator) mcpSpawnSession(ctx context.Context, args map[string]any) if err != nil { return "", err } + role := model.SessionRole(mcp.StringArg(args, "role")) + // PR/CI follow-ups need a PR-branch checkout that spawn_session cannot + // provision; routing them through address_pr_feedback is the only way they get + // one (without it the follow-up commits to a stranded branch and update_pr has + // nothing to push). Block them here so the enum can't be worked around. + if role == model.RolePRFollowup || role == model.RoleCIFollowup { + return "", fmt.Errorf("use address_pr_feedback (not spawn_session) for PR follow-ups: it provisions the PR-branch checkout the follow-up must push its fix from") + } agentHint := model.AgentKind(mcp.StringArg(args, "agent_hint")) meta := model.JSONMap{} if repo := mcp.StringArg(args, "repo"); repo != "" { @@ -137,7 +159,7 @@ func (o *Orchestrator) mcpSpawnSession(ctx context.Context, args map[string]any) meta = nil } sess, err := o.SpawnSession(mgr.ID, SpawnSpec{ - Role: model.SessionRole(mcp.StringArg(args, "role")), + Role: role, Title: mcp.StringArg(args, "title"), Goal: mcp.StringArg(args, "goal"), Agent: agentHint, @@ -175,6 +197,10 @@ func (o *Orchestrator) mcpPublishPR(ctx context.Context, args map[string]any) (s } func (o *Orchestrator) mcpUpdatePR(ctx context.Context, args map[string]any) (string, error) { + pr, err := o.resolvePR(ctx, mcp.StringArg(args, "pr_id")) + if err != nil { + return "", err + } // Default the pushing session to the caller (e.g. a follow-up pushing its own // checkout), so the agent need not know its own session/workspace id. sessionID := mcp.StringArg(args, "session_id") @@ -192,7 +218,7 @@ func (o *Orchestrator) mcpUpdatePR(ctx context.Context, args map[string]any) (st // so an agent that force-pushes after a rebase isn't blocked on wording. reason = "force update after history rewrite (e.g. rebase to resolve conflicts)" } - pr, err := o.UpdatePR(ctx, mcp.StringArg(args, "pr_id"), UpdateSpec{ + updated, err := o.UpdatePR(ctx, pr.ID, UpdateSpec{ SessionID: sessionID, WorkspaceID: workspaceID, Title: mcp.StringArg(args, "title"), @@ -204,16 +230,93 @@ func (o *Orchestrator) mcpUpdatePR(ctx context.Context, args map[string]any) (st if err != nil { return "", err } - return fmt.Sprintf("updated PR #%d (head %s)", pr.Number, pr.HeadSHA), nil + return fmt.Sprintf("updated PR #%d (head %s)", updated.Number, updated.HeadSHA), nil } func (o *Orchestrator) mcpCommentPR(ctx context.Context, args map[string]any) (string, error) { - if err := o.CommentPR(ctx, mcp.StringArg(args, "pr_id"), mcp.StringArg(args, "body")); err != nil { + pr, err := o.resolvePR(ctx, mcp.StringArg(args, "pr_id")) + if err != nil { + return "", err + } + if err := o.CommentPR(ctx, pr.ID, mcp.StringArg(args, "body")); err != nil { return "", err } return "comment posted.", nil } +func (o *Orchestrator) mcpAddressPRFeedback(ctx context.Context, args map[string]any) (string, error) { + pr, err := o.resolvePR(ctx, mcp.StringArg(args, "pr_id")) + if err != nil { + return "", err + } + role := model.RolePRFollowup + if mcp.StringArg(args, "role") == string(model.RoleCIFollowup) { + role = model.RoleCIFollowup + } + sess, err := o.spawnPRFollowup(ctx, pr, role, mcp.StringArg(args, "instructions")) + if err != nil { + return "", err + } + return fmt.Sprintf("spawned %s session %s for PR #%d on a checkout of branch %q; it will push its fix back to the PR.", + role, sess.ID, pr.Number, pr.Branch), nil +} + +// resolvePR resolves a pr_id tool argument to a PR. It accepts either the +// internal Orcha PR id (a UUID) or the GitHub PR number (e.g. "17"): the number +// is the identifier agents see on GitHub and in PR comments, so accepting it +// avoids the confusing "store: not found" an agent hits when it passes the number +// it knows. A bare number is resolved within the caller's objective. +func (o *Orchestrator) resolvePR(ctx context.Context, idArg string) (*model.PullRequest, error) { + idArg = strings.TrimSpace(idArg) + if idArg == "" { + return nil, fmt.Errorf("pr_id is required") + } + if pr, err := o.st.GetPR(idArg); err == nil { + return pr, nil + } + objID := o.callerObjective(ctx) + if n, convErr := strconv.Atoi(idArg); convErr == nil && n > 0 && objID != "" { + prs, _ := o.st.ListPRsByObjective(objID) + var match *model.PullRequest + for _, pr := range prs { + if pr.Number != n { + continue + } + // Prefer an open/draft PR if a number somehow appears more than once. + if match == nil || pr.Status == model.PROpen || pr.Status == model.PRDraft { + match = pr + } + } + if match != nil { + return match, nil + } + } + // Help the agent recover by listing the objective's PRs and their ids. + if objID != "" { + if prs, _ := o.st.ListPRsByObjective(objID); len(prs) > 0 { + var b strings.Builder + for _, pr := range prs { + fmt.Fprintf(&b, "\n #%d -> pr_id %s (%s)", pr.Number, pr.ID, pr.Status) + } + return nil, fmt.Errorf("no PR found for pr_id %q; this objective's PRs:%s", idArg, b.String()) + } + } + return nil, fmt.Errorf("no PR found for pr_id %q (pass the Orcha pr_id or the GitHub PR number)", idArg) +} + +// callerObjective returns the objective of the session bound to the MCP request, +// or "" if none is bound. +func (o *Orchestrator) callerObjective(ctx context.Context) string { + id := mcp.SessionFromContext(ctx) + if id == "" { + return "" + } + if s, err := o.st.GetSession(id); err == nil { + return s.ObjectiveID + } + return "" +} + func (o *Orchestrator) mcpCreateNote(ctx context.Context, args map[string]any) (string, error) { mgr, err := o.managerSession(ctx) if err != nil { diff --git a/internal/orch/orch_test.go b/internal/orch/orch_test.go index bed0f21..c33d6f3 100644 --- a/internal/orch/orch_test.go +++ b/internal/orch/orch_test.go @@ -1749,3 +1749,141 @@ func TestRecordUsage_IncrementsSessionAndProviderBucket(t *testing.T) { t.Fatalf("session used=%d after no-op calls, want 150", got) } } + +// ---- PR follow-up wiring (manager-spawned follow-ups & id resolution) ---- + +// update_pr/comment_pr must accept the GitHub PR number, not just the internal +// Orcha pr_id — agents naturally pass the number they see on GitHub. Regression +// for "store: not found" when a follow-up passed pr_id="17". +func TestUpdatePR_AcceptsGitHubNumber(t *testing.T) { + o, st := newTestOrch(t) + o.RegisterProvider(agent.NewFake(model.AgentClaude, true, nil)) + f := forge.NewFake() + o.SetForge(f) + obj, _, _ := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + + ws := &model.Workspace{ObjectiveID: obj.ID, Kind: model.WorkspacePRBranch, ProjectPath: "octo/repo", + VCS: model.VCSGit, BranchName: "orcha/impl-x", Path: "/tmp/pr-7", Status: model.WorkspaceReady} + _ = st.CreateWorkspace(ws) + fu := &model.Session{ObjectiveID: obj.ID, Role: model.RolePRFollowup, Agent: model.AgentClaude, + Status: model.SessionRunning, WorkspaceID: ws.ID} + _ = st.CreateSession(fu) + pr := &model.PullRequest{ObjectiveID: obj.ID, Repo: "octo/repo", Number: 7, Branch: "orcha/impl-x", + BaseBranch: "main", Status: model.PROpen} + _ = st.CreatePR(pr) + + // Pass the GitHub number, not the UUID. + ctx := mcp.WithSession(context.Background(), fu.ID) + if _, err := o.mcpUpdatePR(ctx, map[string]any{"pr_id": "7"}); err != nil { + t.Fatalf("update_pr by GitHub number: %v", err) + } + if len(f.Pushes) == 0 { + t.Fatal("update_pr by GitHub number should have pushed the branch") + } +} + +// A bare unknown number returns a helpful error listing the objective's PRs. +func TestResolvePR_UnknownNumberListsPRs(t *testing.T) { + o, st := newTestOrch(t) + o.SetForge(forge.NewFake()) + obj, _, _ := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + mgr := &model.Session{ObjectiveID: obj.ID, Role: model.RoleManager, Status: model.SessionRunning} + _ = st.CreateSession(mgr) + pr := &model.PullRequest{ObjectiveID: obj.ID, Repo: "octo/repo", Number: 7, Branch: "b", BaseBranch: "main", Status: model.PROpen} + _ = st.CreatePR(pr) + + ctx := mcp.WithSession(context.Background(), mgr.ID) + _, err := o.resolvePR(ctx, "999") + if err == nil { + t.Fatal("unknown number should error") + } + if !strings.Contains(err.Error(), "#7") || !strings.Contains(err.Error(), pr.ID) { + t.Fatalf("error should list the objective's PRs, got: %v", err) + } +} + +// spawn_session must refuse PR/CI follow-up roles: it can't provision the +// PR-branch checkout those roles need, so they must go through +// address_pr_feedback. This is the root-cause guard for the stranded-commit bug. +func TestSpawnSession_RejectsFollowupRoles(t *testing.T) { + o, st := newTestOrch(t) + obj, _, _ := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + mgr := &model.Session{ObjectiveID: obj.ID, Role: model.RoleManager, Status: model.SessionRunning} + _ = st.CreateSession(mgr) + + ctx := mcp.WithSession(context.Background(), mgr.ID) + for _, role := range []string{"pr_followup", "ci_followup"} { + _, err := o.mcpSpawnSession(ctx, map[string]any{"role": role, "title": "t", "goal": "g"}) + if err == nil { + t.Fatalf("spawn_session must reject role %q", role) + } + if !strings.Contains(err.Error(), "address_pr_feedback") { + t.Fatalf("rejection should point at address_pr_feedback, got: %v", err) + } + } +} + +// address_pr_feedback provisions the PR-branch workspace and attaches the +// follow-up to the PR, accepting the GitHub number for pr_id. +func TestAddressPRFeedback_ProvisionsPRBranchWorkspace(t *testing.T) { + o, st := newTestOrch(t) + addTarget(t, st, "local", model.TargetLocal, 4) + o.RegisterProvider(agent.NewFake(model.AgentClaude, true, nil)) + o.SetForge(forge.NewFake()) + obj, _, _ := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + mgr := &model.Session{ObjectiveID: obj.ID, Role: model.RoleManager, Status: model.SessionRunning} + _ = st.CreateSession(mgr) + pr := &model.PullRequest{ObjectiveID: obj.ID, Repo: "octo/repo", Number: 5, Branch: "feature", + BaseBranch: "main", HeadSHA: "sha1", Status: model.PROpen} + _ = st.CreatePR(pr) + + ctx := mcp.WithSession(context.Background(), mgr.ID) + if _, err := o.mcpAddressPRFeedback(ctx, map[string]any{"pr_id": "5", "instructions": "preserve unchanged fields"}); err != nil { + t.Fatalf("address_pr_feedback: %v", err) + } + + sessions, _ := st.ListSessionsByObjective(obj.ID) + var fu *model.Session + for _, s := range sessions { + if s.Role == model.RolePRFollowup { + fu = s + } + } + if fu == nil { + t.Fatal("address_pr_feedback should spawn a pr_followup session") + } + if fu.Metadata["pr_id"] != pr.ID { + t.Fatalf("follow-up must be attached to the PR, got pr_id=%v", fu.Metadata["pr_id"]) + } + if !strings.Contains(fu.Goal, "preserve unchanged fields") || !strings.Contains(fu.Goal, pr.ID) { + t.Fatalf("goal should carry the instructions and the pr_id, got: %q", fu.Goal) + } + ws, _ := st.GetWorkspace(fu.WorkspaceID) + if ws == nil || ws.Kind != model.WorkspacePRBranch || ws.BranchName != pr.Branch { + t.Fatalf("follow-up should use a PR-branch checkout, got %+v", ws) + } +} + +// UpdatePR must refuse to push when no checkout holds the PR branch, rather than +// running git in the orchestrator's own cwd. This is what turned a manager-spawned +// follow-up's stranded commit into a silent no-op. +func TestUpdatePR_NoCheckoutRefuses(t *testing.T) { + o, st := newTestOrch(t) + f := forge.NewFake() + o.SetForge(f) + obj, _, _ := o.CreateObjective(NewObjectiveSpec{Title: "x", Prompt: "p"}) + pr := &model.PullRequest{ObjectiveID: obj.ID, Repo: "octo/repo", Number: 4, Branch: "b", + BaseBranch: "main", Status: model.PROpen} + _ = st.CreatePR(pr) + + _, err := o.UpdatePR(context.Background(), pr.ID, UpdateSpec{SessionID: "x"}) + if err == nil { + t.Fatal("update_pr with no PR-branch checkout must error") + } + if !strings.Contains(err.Error(), "no PR-branch checkout") { + t.Fatalf("error should explain the missing checkout, got: %v", err) + } + if len(f.Pushes) != 0 { + t.Fatalf("no push should have happened, got %d", len(f.Pushes)) + } +} diff --git a/internal/orch/pr.go b/internal/orch/pr.go index a40ed52..5e45d1a 100644 --- a/internal/orch/pr.go +++ b/internal/orch/pr.go @@ -37,6 +37,44 @@ func (o *Orchestrator) forgeForWorkspace(ws *model.Workspace) forge.Forge { return o.forgeFor(tgt) } +// prBranchWorkspace resolves the checkout a PR follow-up should push from. It +// prefers an explicitly supplied workspace (the follow-up session's own checkout) +// when that workspace actually tracks the PR's branch, and otherwise finds the +// ready PR-branch workspace recorded for this PR. It returns nil when neither +// resolves, so UpdatePR can refuse to push instead of running git in the +// orchestrator's own cwd against the wrong (or a missing) branch. +func (o *Orchestrator) prBranchWorkspace(workspaceID string, pr *model.PullRequest) *model.Workspace { + isForPR := func(ws *model.Workspace) bool { + if ws == nil { + return false + } + if id, _ := ws.Metadata["pr_id"].(string); id == pr.ID { + return true + } + // A PR-branch checkout tracks the PR's branch even without pr_id metadata. + return ws.Kind == model.WorkspacePRBranch && ws.BranchName != "" && ws.BranchName == pr.Branch + } + if workspaceID != "" { + if ws, err := o.st.GetWorkspace(workspaceID); err == nil && isForPR(ws) { + return ws + } + } + wss, err := o.st.ListWorkspacesByObjective(pr.ObjectiveID) + if err != nil { + return nil + } + var best *model.Workspace + for _, ws := range wss { + if ws.Kind != model.WorkspacePRBranch || ws.Status != model.WorkspaceReady || !isForPR(ws) { + continue + } + if best == nil || ws.CreatedAt.After(best.CreatedAt) { + best = ws + } + } + return best +} + // PublishSpec carries the agent-supplied PR content. type PublishSpec struct { Title string @@ -195,12 +233,11 @@ func (o *Orchestrator) UpdatePR(ctx context.Context, prID string, spec UpdateSpe if err != nil { return nil, err } - // Run git/gh on the machine holding the PR-branch checkout (the follow-up - // workspace's target), not the orchestrator host. - var fws *model.Workspace - if spec.WorkspaceID != "" { - fws, _ = o.st.GetWorkspace(spec.WorkspaceID) - } + // Resolve the checkout that actually holds the PR branch and run git/gh on its + // target, not the orchestrator host. fws may be nil here — the state refresh + // and the merged/closed/force checks below don't need a checkout, so we defer + // the "no checkout" failure until just before the push. + fws := o.prBranchWorkspace(spec.WorkspaceID, pr) f := o.forgeForWorkspace(fws) // Refresh host state before deciding anything. @@ -235,6 +272,15 @@ func (o *Orchestrator) UpdatePR(ctx context.Context, prID string, spec UpdateSpe return pr, errors.New("orch: force push requires an explicit reason") } + // A push needs a checkout of the PR branch. Refuse rather than fall back to + // the orchestrator host's cwd (which would push the wrong branch, or nothing, + // silently). This is the case a manager-spawned follow-up with no PR-branch + // workspace used to hit — its fix would commit to a stranded local branch and + // update_pr would appear to do nothing. + if fws == nil { + return pr, fmt.Errorf("%w: no PR-branch checkout for PR #%d — spawn the follow-up with address_pr_feedback so its fix lands on the PR branch", ErrUnsafePublish, pr.Number) + } + // One updater per PR branch, keyed by PR id. lockKey := prBranchLockKey(prID) if err := o.st.AcquireLock(lockKey, model.LockPRBranch, spec.SessionID, "follow-up update"); err != nil { @@ -245,10 +291,7 @@ func (o *Orchestrator) UpdatePR(ctx context.Context, prID string, spec UpdateSpe } defer o.st.ReleaseLock(lockKey, spec.SessionID) - wsPath := "" - if fws != nil { - wsPath = fws.Path - } + wsPath := fws.Path // Fallback: commit any edits the agent left uncommitted (normally the agent // commits its own work with its own message, so this is a no-op). if wsPath != "" { diff --git a/internal/orch/steering.go b/internal/orch/steering.go index 8d29f8e..db43239 100644 --- a/internal/orch/steering.go +++ b/internal/orch/steering.go @@ -202,17 +202,18 @@ Finish with a brief summary of what you changed.` const managerSystemPreamble = `You are the MANAGER of an engineering team working toward an objective. You coordinate via your orcha MCP tools (named mcp__orcha*): spawn_session to delegate scoped work, ask_user when direction/credentials are unclear, publish_pr to ship -coherent slices, comment_pr/update_pr for follow-ups, create_note for shared -memory, and mark_objective_done when finished. Prefer several clean PR-sized +coherent slices, address_pr_feedback to push a fix or reply to an existing PR, +create_note for shared memory, and mark_objective_done when finished. Prefer several clean PR-sized slices over one giant PR. Keep working after publishing intermediate PRs unless truly blocked. A published PR is NOT a finished objective — the work lands when the PR MERGES. After publishing, do NOT call mark_objective_done while any PR is still open; it will be refused. Wait — you are steered automatically when a PR merges, gets review comments, fails CI, or has merge conflicts. To resolve conflicts or -address feedback, push fixes with update_pr or spawn a follow-up; do not merge -PRs yourself. Call mark_objective_done only once every PR has merged (or there -was never a PR to open). +address feedback, call address_pr_feedback (NOT spawn_session) — it gives the +follow-up a checkout of the PR branch so its fix is pushed back to the same PR; +do not merge PRs yourself. Call mark_objective_done only once every PR has merged +(or there was never a PR to open). When the objective names a repo, you are running in a fresh checkout of it: explore the code first and scope workers' goals precisely, with verified file references. Do NOT code, commit, or push yourself — workers do the coding in