[mirror] Upstream/pr1 on plan ready

[yashwant86]

Mirror of upstream open-multi-agent#181 (open PR live findings) for benchmark. Do not merge.

---

Summary by MergeMonkey

• Enhancements:
  • Adds onPlanReady callback to allow approval of task plans before execution begins.

[bot-mergemonkey]

⚡ Risk Assessment — NEEDS-TESTING · ~8 min review

Focus areas: onPlanReady callback invocation and error handling · Task list mutability and defensive copying · Early abort path consistency with metrics and token tracking

Assessment: Adds new callback hook to task execution flow with early abort path.

Walkthrough

When runTeam() decomposes a goal into tasks, the new onPlanReady callback is invoked with the task list. If the callback returns false, execution aborts and returns empty results. If true or undefined, execution proceeds normally.

Changes

Files	Summary
Plan Ready Callback Integration src/orchestrator/orchestrator.ts src/types.ts	Introduces onPlanReady callback to OrchestratorConfig, enabling plan approval before task execution. Callback receives decomposed task list and can abort execution if approval denied.

Sequence Diagram

sequenceDiagram
  participant Caller
  participant OpenMultiAgent
  participant Coordinator
  participant onPlanReady
  participant TaskQueue
  Caller->>OpenMultiAgent: runTeam(goal)
  OpenMultiAgent->>Coordinator: decompose goal
  Coordinator->>TaskQueue: create tasks
  OpenMultiAgent->>onPlanReady: invoke with task list
  alt Callback returns false
    onPlanReady-->>OpenMultiAgent: false
    OpenMultiAgent-->>Caller: {success: false, empty results}
  else Callback returns true or undefined
    onPlanReady-->>OpenMultiAgent: true
    OpenMultiAgent->>TaskQueue: executeQueue
    TaskQueue-->>OpenMultiAgent: execution complete
    OpenMultiAgent-->>Caller: {success: true, results}
  end

Loading

Dig Deeper With Commands

• /review <file-path> <function-optional>
• /chat <file-path> "<question>"
• /roast <file-path>

Runs only when explicitly triggered.

[bot-mergemonkey]

Plan-rejection path silently zeros out coordinator decomposition tokens

When onPlanReady returns false, runTeam returns hardcoded zero token usage and a fresh empty agentResults Map, but by that point the coordinator decomposition has already run and its tokens are sitting in cumulativeUsage (and its result in agentResults under 'coordinator:decompose'). Billing/observability will under-report real spend on every rejected plan.

Return the live cumulativeUsage and the existing agentResults Map (or call buildTeamRunResult with success:false) so the decomposition spend isn't lost.

[bot-mergemonkey]

onPlanReady call isn't wrapped in try/catch like its sibling onApproval

The existing onApproval callback in this file is invoked inside a try/catch that defaults to a safe approved=false on throw. onPlanReady is called bare — a thrown error from user code will propagate out of runTeam, leaving the already-spent coordinator tokens unaccounted and skipping any cleanup the caller expected. Worth matching the existing pattern.

Mirror the onApproval pattern: try/catch around the await, default approved=false on throw (or rethrow with context), so a buggy host callback can't tear down the run mid-flight.

[bot-mergemonkey]

Actionable Comments Posted: 2

🧹 Nitpick comments (1)

onPlanReady accepts mutable Task[] while sibling onApproval is readonly - src/types.ts (645, 653)

onApproval is typed `readonly Task[]` with an explicit "Do not mutate" doc note, but the new onPlanReady right next to it is plain `Task[]`. Same shape of callback, different mutability contract — easy for a host to accidentally rely on mutation here and then have it forbidden in the other hook.

Tighten to `readonly Task[]` and add the same "Do not mutate" note for parity with onApproval.

🧾 Coverage Summary

✔️ Covered (2 files)

- src/orchestrator/orchestrator.ts
- src/types.ts