Add Magentic agent-workflow builder (implements #564) - #565
Add Magentic agent-workflow builder (implements #564)#565PratikDhanave (PratikDhanave) wants to merge 4 commits into
Conversation
a36b536 to
683aace
Compare
This comment has been minimized.
This comment has been minimized.
683aace to
47b181c
Compare
NewMagenticWorkflowBuilder(agents...) builds a manager-orchestrated multi-agent workflow. A dedicated orchestrator agent (WithManager) maintains a task ledger and, each round, evaluates a JSON progress ledger to decide whether the request is satisfied, whether progress has stalled, and which participant speaks next. It re-plans on stalls (WithMaximumStallCount) and stops after exhausting its reset budget (WithMaximumResetCount) or the round cap (WithMaximumRoundCount). Reuses the group-chat host and GroupChatManager machinery: the orchestrator drives SelectNextAgent and ShouldTerminate, with the ledger checkpoint- persisted as manager state. Unparseable orchestrator output and unknown next-speaker names degrade gracefully rather than failing the run.
47b181c to
a31d722
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Adds a new Magentic agent-workflow builder to the agentworkflow package, implementing an orchestrator-driven multi-agent pattern (task ledger + per-round progress ledger) as a thin wrapper over the existing group chat workflow machinery.
Changes:
- Introduces
NewMagenticWorkflowBuilderwith fluent configuration for orchestrator, round/stall/reset budgets, and output designations. - Implements a
magenticManagerthat builds a task ledger once, evaluates per-round progress JSON for routing/termination, and persists its state via checkpointing. - Adds unit tests covering builder validation, basic routing/termination, stall→reset-budget termination, unknown-speaker termination, and progress-ledger JSON parsing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| workflow/agentworkflow/magentic.go | New Magentic builder + manager implementation (routing, stall/reset logic, checkpointed state, JSON parsing). |
| workflow/agentworkflow/magentic_test.go | New tests validating Magentic builder behavior and progress-ledger parsing. |
| ledger, ok := parseMagenticProgressLedger(text) | ||
| if !ok { | ||
| // The orchestrator did not return parseable JSON. Treat this as an | ||
| // unproductive round so the stall/reset machinery can recover rather | ||
| // than failing the whole workflow. | ||
| return magenticProgressLedger{IsProgressBeingMade: false}, nil | ||
| } | ||
| return ledger, nil |
| func (m *magenticManager) onCheckpoint(ctx *workflow.Context) error { | ||
| return ctx.QueueStateUpdate(magenticManagerStateKey, "", m.state) | ||
| } | ||
|
|
||
| func (m *magenticManager) onCheckpointRestored(ctx *workflow.Context) error { | ||
| value, err := ctx.ReadState(magenticManagerStateKey, "") |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by Go API Consistency Review Agent · sonnet46 · 47.7 AIC · ⌖ 5.87 AIC · ⊞ 5.7K
| ) | ||
|
|
||
| const ( | ||
| defaultMagenticMaximumRounds = 30 |
There was a problem hiding this comment.
Parity issue: maxRounds and maxResets default to a fixed cap, but upstream leaves them unbounded by default
In .NET, WithMaxRounds and WithMaxResets both default to null (unlimited), meaning the workflow runs until maxStalls/natural termination. Python follows the same convention: max_round_count: int | None = None and max_reset_count: int | None = None.
This Go implementation defaults to defaultMagenticMaximumRounds = 30 and defaultMagenticMaximumResets = 2, which silently caps workflows that would otherwise complete naturally under upstream semantics.
References:
- .NET:
TaskLimits(MaxRoundCount: null, MaxResetCount: null)inMagenticTaskContext.cs/MagenticWorkflowBuilder.cs - Python:
max_round_count: int | None = None,max_reset_count: int | None = Nonein_magentic.py
Consider mirroring upstream by treating zero/unset as unlimited for these two limits (only maxStalls has a fixed default of 3 across all three SDKs).
| if next == nil { | ||
| // The orchestrator named an unknown participant; end gracefully rather | ||
| // than route to a non-participant (which the host would reject). | ||
| m.state.Done = true |
There was a problem hiding this comment.
Parity issue: Task ledger uses a single simplified prompt; upstream uses a structured two-phase fact-gathering + plan prompts
Both .NET (MagenticDefaultPrompts.TaskLedgerFactsPrompt + TaskLedgerPlanPrompt) and Python (ORCHESTRATOR_TASK_LEDGER_FACTS_PROMPT + plan prompt) split the task ledger into two phases:
- A detailed fact survey (known facts, facts to look up, facts to derive, educated guesses — structured under four headings)
- A separate plan prompt that asks for a numbered step-by-step plan for the team
The Go implementation collapses both into a single inline string asking for "known facts, the facts still to be discovered, and a short numbered plan." This produces structurally different ledger content compared to the upstream implementations, which may affect orchestration quality.
Reference:
- .NET:
MagenticDefaultPrompts.cs(TaskLedgerFactsPrompt / TaskLedgerPlanPrompt / TaskLedgerFullPrompt) - Python:
ORCHESTRATOR_TASK_LEDGER_FACTS_PROMPTin_magentic.py
Consider aligning the prompt structure, or at minimum adding a WithPromptOverrides escape hatch (see next comment) so callers can substitute the upstream-compatible prompts.
|
|
||
| // NewMagenticWorkflowBuilder creates a builder for a Magentic workflow over the | ||
| // given participant agents. Use [MagenticWorkflowBuilder.WithManager] to supply | ||
| // the orchestrator agent, which is required. |
There was a problem hiding this comment.
Parity gap: No WithPromptOverrides or WithResponseLanguage on the builder
Both upstream implementations expose prompt customization that Go does not:
- .NET:
MagenticWorkflowBuilder.WithPromptOverrides(MagenticPromptOverrides)lets callers override any of the six internal prompt templates (facts, plan, full ledger, update facts, update plan, progress ledger).MagenticDefaultPromptsexposes the defaults for reference.WithResponseLanguage(string)pins the manager's output language. - Python:
MagenticOrchestratoraccepts custom prompt callbacks and exposesrequire_plan_signofffor human-in-the-loop plan approval.
The Go builder embeds all prompts as hardcoded inline strings with no override path. This means:
- Callers cannot translate prompts for non-English tasks.
- Callers cannot customize the fact-survey or plan prompts without forking the package.
- There is no human-plan-review (HITL) hook.
At a minimum, consider adding a WithPromptOverrides option struct (even if only a subset of the six .NET overrides) to keep the public API extensible before it stabilises.
Reference: dotnet/src/Microsoft.Agents.AI.Workflows/MagenticPromptOverrides.cs and MagenticDefaultPrompts.cs.
|
PratikDhanave (@PratikDhanave) can you resolve the parity gaps? |
github-actions
Bot
added
area:workflow
Cross-repo parity review — PR #565 (Magentic builder)The core orchestration structure (task ledger → progress ledger loop → stall/reset/termination) is semantically aligned with the upstream .NET However, three parity gaps need to be resolved before merge: 1. Default limits are wrong (
|
There was a problem hiding this comment.
Generated by Go API Consistency Review Agent · sonnet46 · 53.7 AIC · ⌖ 5.63 AIC · ⊞ 6K
| defaultMagenticMaximumRounds = 30 | ||
| defaultMagenticMaximumStalls = 3 | ||
| defaultMagenticMaximumResets = 2 | ||
|
|
There was a problem hiding this comment.
Parity issue — default limits diverge from .NET
In the upstream .NET MagenticWorkflowBuilder, WithMaxRounds and WithMaxResets both default to null (unlimited), and only WithMaxStalls has a non-null default of 3. See TaskLimits:
internal record TaskLimits(
int MaxStallCount = TaskLimits.DefaultMaxStallCount, // 3
int? MaxRoundCount = null, // unlimited
int? MaxResetCount = null, // unlimited
...);This Go implementation hardcodes defaultMagenticMaximumRounds = 30 and defaultMagenticMaximumResets = 2, so a workflow built with no explicit options caps at 30 rounds and 2 resets in Go, while the same workflow with .NET defaults is uncapped. This is a silent behavioural divergence that will surprise users porting from .NET.
Suggested fix: use *int (pointer, nil = unlimited) for maxRounds and maxResets to match the upstream nullable semantics. maxStalls can keep its non-pointer default of 3.
| maxStalls int | ||
| maxResets int | ||
| outputs outputDesignations | ||
| err error |
There was a problem hiding this comment.
Parity issue — RequirePlanSignoff (HITL plan approval) is missing
The upstream .NET MagenticWorkflowBuilder includes a RequirePlanSignoff(bool) option (default true) that gates plan execution on human approval before the first speaker round and after every replan:
public MagenticWorkflowBuilder RequirePlanSignoff(bool requirePlanSignoff = true)This is part of the documented human-in-the-loop (HITL) support for Magentic. The Go builder has no equivalent, so Go Magentic workflows always run plans without a sign-off opportunity — the opposite of the .NET default.
This is a meaningful semantic gap: users familiar with the .NET SDK will expect plan approval to be on by default. Even if HITL wiring is not yet plumbed (as the PR description notes re: instruction injection), the option should at least be surfaced — or the divergence should be explicitly documented.
| // WithDescription sets the workflow description. | ||
| func (b *MagenticWorkflowBuilder) WithDescription(description string) *MagenticWorkflowBuilder { | ||
| if b == nil || b.err != nil { | ||
| return b |
There was a problem hiding this comment.
Parity gap — WithResponseLanguage and WithPromptOverrides are absent
The upstream .NET MagenticWorkflowBuilder exposes two experimental builder options that have no Go equivalent:
// Force the manager to write all internal messages in a specific language
[Experimental] MagenticWorkflowBuilder WithResponseLanguage(string? responseLanguage)
// Override built-in prompt templates (task ledger, progress ledger, final answer)
[Experimental] MagenticWorkflowBuilder WithPromptOverrides(MagenticPromptOverrides? promptOverrides)Both are marked [Experimental] in .NET and are optional, but their absence in Go means users cannot tune the manager's prompts or force a non-English response language — common needs in multilingual or custom deployments. If the design decision is to defer these, please note that explicitly in the PR and/or the builder's doc comment so it is clear this is intentional.
github-actions
Bot
added
failed-auto-risk
github-actions
Bot
added
pending-auto-risk
There was a problem hiding this comment.
Generated by Go API Consistency Review Agent · sonnet46 · 64 AIC · ⌖ 4.97 AIC · ⊞ 6K
| ) | ||
|
|
||
| const ( | ||
| defaultMagenticMaximumRounds = 30 |
There was a problem hiding this comment.
Parity issue: defaultMagenticMaximumRounds = 30 diverges from upstream defaults.
Both the .NET and Python implementations default MaxRoundCount/max_round_count to null/None (unlimited), not a fixed integer:
- .NET
MagenticWorkflowBuilder:private int? _maxRounds;(null = unlimited by default) - Python
MagenticBuilder:max_round_count: int | None = None(unlimited by default)
Go hardcodes defaultMagenticMaximumRounds = 30, which silently caps orchestrations at 30 rounds even when callers never call WithMaximumRoundCount. This is a behavioral divergence — callers porting from .NET or Python will be surprised that Go silently stops their workflow. Consider defaulting to unlimited (e.g., 0 treated as no-cap, or an unexported sentinel), and documenting any intentional Go-specific cap clearly.
| const ( | ||
| defaultMagenticMaximumRounds = 30 | ||
| defaultMagenticMaximumStalls = 3 | ||
| defaultMagenticMaximumResets = 2 |
There was a problem hiding this comment.
Parity issue: defaultMagenticMaximumResets = 2 diverges from upstream defaults.
Both .NET and Python default the reset count to unlimited:
- .NET:
private int? _maxResets;(null = unlimited) - Python:
max_reset_count: int | None = None(unlimited)
Go hardcodes 2, meaning after two re-plans the workflow stops regardless of whether the user requested a cap. Consider defaulting to unlimited and requiring callers to opt in to a cap via WithMaximumResetCount, matching upstream semantics.
| return b | ||
| } | ||
|
|
||
| // WithName sets the workflow name. |
There was a problem hiding this comment.
Parity gap: RequirePlanSignoff (human-in-the-loop plan approval) is absent.
Both upstream implementations expose a plan-review / human-in-the-loop gate on the initial plan and on re-plans after stalls:
- .NET:
MagenticWorkflowBuilder.RequirePlanSignoff(bool requirePlanSignoff = true)— on by default; the orchestrator emits aMagenticPlanReviewRequestand waits for human approval before proceeding. - Python:
MagenticOrchestrator(require_plan_signoff=False)— off by default, but the feature is present.
Go exposes no equivalent option, so callers cannot opt in to human plan approval. Importantly, the .NET default is true (approval required), which means Go and .NET have opposite default behaviors for the HITL gate. This is a meaningful divergence for any caller who expects to mirror the .NET experience without extra configuration.
This feature may warrant explicit deferral in the PR description (similar to how per-turn instruction injection is deferred), but the parity gap should be documented and tracked.
Implements the Magentic builder proposed in #564 — the last unimplemented agent-workflow builder per
docs/dotnet-go-sdk-feature-comparison.md("Handoff and Magentic builders are not yet implemented"). Handoff is #545; this is its sibling.Filed as a draft to anchor the design discussion in #564 — the four design questions there (orchestrator handle, ledger-prompt customization, stall/reset semantics, scope) are still open, and I would rather converge on the API with you before polishing. The core orchestration is implemented and tested.
API (matches the existing
New…WorkflowBuilderconventions)How it works
It is a thin wrapper over
NewGroupChatWorkflowBuilder. AmagenticManagersuppliesGroupChatManager.SelectNextAgentandShouldTerminate:{is_request_satisfied, is_progress_being_made, is_in_loop, next_speaker, instruction}, which decides routing and termination.OnCheckpoint/OnCheckpointRestored, matching the round-robin manager.next_speakerends the run rather than routing to a non-participant.Scope / open question (ties to #564 Q1/Q3)
This PR implements orchestrator-driven selection + termination + re-planning, which is the reviewable core. Per-turn instruction injection to the selected speaker is intentionally deferred: the group-chat host broadcasts only to non-speakers, so cleanly handing the orchestrator’s
instructionto the chosen agent needs either a small host hook or a manager-supplied preamble — a design choice I’d like your steer on (#564). Theinstructionfield is already parsed and part of the ledger.Tests
magentic_test.go(white-box, matching the package convention) covers: builder validation (missing manager, no agents), routing to the selected speaker then terminating onis_request_satisfied, stall→re-plan→reset-budget termination (bounded, not round-capped), unknown-speaker graceful termination, and progress-ledger JSON parsing (plain, fenced-with-prose, and non-JSON). Full package green;gofumpt/vetclean.