Repository navigation
Python: Retry the Magentic progress ledger when it names an unknown agent - #9181
Lanre Shittu (Shizoqua) wants to merge 2 commits into
Conversation
|
/review |
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 45c45d921517
Model: gpt-5.6-sol
Overview
The change validates unsatisfied progress ledgers against configured participant names and reuses the existing bounded per-call parse retry mechanism. It preserves satisfied-ledger handling, empty-team validation, and configured round/reset limits, with a focused test for recovery on the second response. However, persistent invalid output now enters the orchestrator's reset/replan path, whose limits are unlimited by default, creating an unbounded paid-call loop.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 high) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/orchestrations/agent_framework_orchestrations/_magentic.py
| and isinstance(next_speaker, str) | ||
| and next_speaker not in agent_names | ||
| ): | ||
| raise ValueError(f"Unknown next speaker '{next_speaker}'. Valid names: {names_csv}.") |
There was a problem hiding this comment.
With the default max_reset_count and max_round_count both unlimited, a model that keeps returning an unknown speaker exhausts these retries and the resulting error is caught by the orchestrator as a reason to reset and replan. That repeats without advancing stall state, and the retries resend identical input, so one untrusted task can keep issuing paid model calls until cancellation or recursion failure. Please preserve the guard against premature final answers but make exhaustion terminate under a finite budget or use another bounded fallback instead of entering the reset loop.
There was a problem hiding this comment.
Thanks. Fixed in b1b5142. Unknown names are still retried, but once the retries run out the last ledger is returned, so the run ends through the existing invalid speaker path instead of resetting. Added a test for it.
| # brief backoff before next try | ||
| await asyncio.sleep(0.25 * attempts) | ||
|
|
||
| if unknown_speaker_ledger is not None: |
There was a problem hiding this comment.
Lanre Shittu (@Shizoqua) unknown_speaker_ledger survives later parse failures, so unknown -> malformed -> malformed returns the stale unknown-speaker ledger instead of raising the final parse failure. That masks the actual failure and routes exhaustion through the invalid-speaker final-answer path. The saved ledger should only win when the final attempt was an unknown-speaker result.
Motivation & Context
StandardMagenticManager.create_progress_ledgersays it avoids selecting a non-existent agent, but thenext_speakerin the ledger was never checked. When the model named a participant slightly wrong, such as "Coder" for "coder", the orchestrator treated it as invalid and went straight to the final answer, ending the run with the task unfinished.Description & Review Guide
next_speakeris not a participant now raises, so it is retried like a parse failure. The check is skipped when the request is already satisfied.Related Issue
Fixes #9178
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix), and a workflow keeps the label and title prefix in sync automatically.