[BUG] HierarchicalSwarm silently returns no output when given a caller-supplied director - #1741
Conversation
|
Hello there, thank you for opening an PR ! 🙏🏻 The team was notified and they will get back to you asap. |
|
Follow-up: this affects three shipped examples, not just the docs. I had originally justified this against the
All three hardcode On On this branch:
There is a detail here worth calling out, because it explains how this survived unnoticed and it is arguably worse than the empty return in the original report. That example uses self.conversation.add(role="Director", content=function_call)So loop 2 returns the director's prose — and the director, being a capable model asked to plan a project, produces a long, structured, confident-looking deliverable. Roughly 8KB of phase breakdowns, sprint tables, risk matrices and success metrics. It reads exactly like a successful hierarchical swarm run. It is one agent talking to itself, with all three specialists idle. A user eyeballing that output has no reason to suspect anything is wrong, which is likely why this was never reported: the failure mode is not an empty result, it is a plausible result produced by the wrong number of agents. Same three-line reproduction as in the description, if it is easier to check directly: sed 's/gpt-5.4/claude-haiku-4-5/g' \
examples/multi_agent/hierarchical_swarm/example_hierarchical.py > /tmp/ex.py
python /tmp/ex.py | grep -o "'role': '[^']*'" | sort -u |
There was a problem hiding this comment.
Pyre found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
e6376ec to
9bcd35c
Compare
|
On the Pyre summary ("more than 20 potential problems"): those are file-scoped, not diff-scoped. Pyre reports every alert in a file this PR touches, and d = Agent(agent_name="MyDirector", model_name="gpt-4o", max_loops=1)
print(d.tools_list_dictionary) # [] — no SwarmSpec schema
s = HierarchicalSwarm(name="t", agents=[...], director=d)That is the whole bug: a caller-supplied director arrives with no Also on this branch now: the test file is dropped and it is rebased on today's master, so the diff is 38 lines in one file. Worth noting the red checks here are not this PR's — see #1812, which fixes the two workflow defects that make them red for everything (pytest collecting |
|
Rebased onto master (9d8f6ef) and trimmed the comments to the load-bearing lines. While re-verifying I found something the PR should state plainly rather than leave for CI to surface. Six existing tests in They construct real result = swarm.run("Develop a comprehensive strategy …")
assert result is not Nonewhich passes on a run that produced no agent output at all — the exact behaviour #1740 reports. With this change the same situation raises: So the six failures are the fix working, on tests whose only assertion was satisfied by the silent failure. Master's own baseline for this file is Two ways to land it, your call:
I did not change them here, because rewriting six tests to keep a suite green is the kind of edit that should be a deliberate decision rather than a quiet part of a bug fix. Say which you prefer and I will push it. |
1e94a04 to
c50b838
Compare
…pec schema][bugf][hierarchical-swarm][raise instead of returning an empty history when every loop fails]
HierarchicalSwarm silently produced no output whenever the caller passed
their own director agent -- the pattern shown in the project's own docs:
director = Agent(agent_name="Director", model_name=..., max_loops=1)
swarm = HierarchicalSwarm(director=director, agents=workers)
swarm.run(task) # returned only "--- Loop 1/1 completed ---"
Two independent defects combined to hide it.
1. reliability_checks() only attached the SwarmSpec schema when it built the
director itself. A caller-supplied director was stored verbatim, so it
answered in prose and parse_orders could never read a plan out of it.
Attaching the schema is not sufficient on its own. Agent bakes its tool
schemas into the LiteLLM instance during __init__, so the schema has to be
followed by an llm_handling() rebuild -- the same step Agent performs when
it registers planning tools after construction. The director prompt is also
required: with the schema but no prompt the model emits a tool call with
invented field names ("agent" instead of "agent_name") and
HierarchicalOrder rejects the order. It is added to short_memory because
Agent has already copied system_prompt into its conversation by then.
2. Both step() and run() logged the resulting exception and carried on. step()
fell off the end of its except block returning None -- contradicting its own
docstring, which documents that it raises -- and run() returned the
conversation, so the caller saw a successful run that happened to contain no
agent work. step() now re-raises, and run() raises only when every loop
failed, preserving partial results when some loops succeed.
Verified against claude-haiku-4-5: a caller-supplied director now runs its
workers, where before the run returned 61 characters of loop marker.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c50b838 to
774fbce
Compare
|
Rebased onto Flagging something before you merge this, because it will look like a regression and I do not think it is one. Six tests in Not a stale base — I rebased first and re-ran; the delta is this diff. They fail because of the The part worth your attention is what that means about the tests. Here is # Verify result structure
assert result is not None
# HierarchicalSwarm returns a SwarmSpec or conversation history, just ensure it's not NoneThat is the whole assertion. On So the six failures are the fix doing its job on tests that were only green by accident. I have deliberately not touched them:
Your call on what happens to them. If you want, I will send a follow-up that gives them a stubbed director so they assert something real and run offline — that is a test-quality change and reviewable on its own terms. Worth noting the whole |
|
Follow-up to the above — this PR and #1886 add the same mechanism twice, so they should not both merge as-is. Both add the identical What is unique to each:
Suggested order, your call: merge #1886 first as the general "stop swallowing the error" change, then I rebase this one onto it and drop the now-redundant The six failing tests I described above are common to both PRs and have the same cause. |
|
Six tests in All six build real On master that exception is swallowed, assert result is not None— holds. It passes in 1.25s, which is not four LLM round-trips. So the test currently passes against a run that produced nothing at all. That is this PR's bug, demonstrated by the suite itself: a total director outage reported as a successful run. Re-raising makes the six stop lying, which is why they now fail. I have not touched them, because Say which you prefer and I will do it here. Same six fail identically on #1741, which changes the same failure path. For the record, the change itself is clean: |
…arm-silent-failure # Conflicts: # swarms/structs/hiearchical_swarm.py
|
Brought up to date by merging What changed in scope. This PR previously did two things: backfilled the What's left is the fix this PR is actually about, and it's now a single hunk. The bug. Confirmed on current master: The fix backfills only what's missing, and only when the caller didn't supply it: the schema, a rebuilt Verification on this branch:
#1886 and this PR now touch different parts of the file and can land in either order. |
…arm-silent-failure # Conflicts: # swarms/structs/hiearchical_swarm.py
A
HierarchicalSwarmbuilt with your own director agent runs to completion, returns successfully, and produces no agent work at all. This is the pattern the project documents:Before this patch that returns 61 characters:
No exception, no failed status. The workers never ran.
Minimal reproduction
Root cause
Two independent defects combine, which is why this stayed hidden.
1. A caller-supplied director never receives the
SwarmSpecschema.reliability_checks()attaches the schema only on the path where it constructs the director itself:A director passed in by the caller is stored verbatim during
__init__, so it replies in prose andparse_ordersraisesJSONDecodeError: Expecting value: line 1 column 1.Two non-obvious details make the naive fix insufficient:
Agentbakes its tool schemas into the LiteLLM instance during__init__(agent.py:677). Settingtools_list_dictionaryafter construction is a no-op unless the LLM is rebuilt. The patch callsllm_handling(), which is exactly whatAgentitself does when it registers planning tools post-construction (agent.py:2058,agent.py:2295).The schema alone still fails. With the schema but no director prompt, the model emits a tool call with invented field names and pydantic rejects it:
So
director_system_promptis added too. It goes intoshort_memoryrather thansystem_prompt, becauseshort_memory_init()(agent.py:1010) has already copiedsystem_promptinto the conversation by that point. Appending to the attribute afterwards would be another silent no-op.2. Both
step()andrun()swallow the resulting exception.step()logged and then fell off the end of itsexceptblock, returningNone, contradicting its own docstring which documentsRaises: Exception: If step execution fails.The inner handler inrun()then logged again and continued the loop, andrun()returned the conversation. The caller got a successful return containing no work.Fixing only one of these changes nothing: with just the schema fix a genuine failure is still silent, and with just the raise fix the documented pattern still cannot work.
Changes
reliability_checks(): attach theSwarmSpecschema to a caller-supplied director, rebuild its LLM, and seed the director prompt intoshort_memory. All three are guarded withgetattr, so duck-typed stand-ins withoutllm/short_memory(as used in the existing tests) are unaffected.step(): re-raise after logging, matching its documented contract.run(): count failed loops and raiseRuntimeErroronly when every loop failed. Partial success is preserved, so if some loops succeed their output is still returned.Tests
Two tests added to
tests/structs/test_hierarchical_swarm.py, both offline, reusing the existingStubAgentandmake_recovery_swarmhelpers:test_custom_director_receives_swarmspec_schema— a caller-supplied director ends up with aplan/ordersschema.test_run_raises_when_every_loop_fails— a run where no loop succeeds raises instead of returning an empty history.Both were confirmed to fail against unpatched
hiearchical_swarm.pyand to pass with the patch. The 7 pre-existing offline tests in that file still pass.Verified end to end against
claude-haiku-4-5: case B above now returns roles[Writer, Checker, Director, System]and ~5000 characters.Note, not fixed here
run_director()setsself.director.tools_list_dictionary = Nonewhenplanning_enabled=Truewithout rebuilding the LLM, so the tools stay live in the already-built LiteLLM instance. Same class of latent bug, left alone to keep this diff scoped.🤖 Generated with Claude Code
Update — test file dropped. This is a small change to one function, so it ships without a test per the repo's preference for keeping diffs to the fix itself. The verification described above was run directly (reproduction before, same reproduction after); nothing about the fix or the evidence changed, only the absence of a committed test file.