test only(agents): inherit statefulness into member agents' tool workers - #455
test only(agents): inherit statefulness into member agents' tool workers#455ling-senpeng13 wants to merge 1 commit into
Conversation
|
Follow-up that resolves open question 1 in the description ("is
def _has_stateful_tools(agent: Any) -> bool:
"""Return True if the agent is stateful or any @tool has stateful=True."""
if getattr(agent, "stateful", False):
return True
...
for sub in getattr(agent, "agents", []):
if _has_stateful_tools(sub): # <-- recurses into members
return True
return FalseThat predicate is what decides whether a domain exists at all: run_id = uuid.uuid4().hex if _has_stateful_tools(agent) else None # runtime.py:2491, 3550, 3952and So the two halves were using different definitions of the same word:
A stateful swarm therefore always minted a domain and tagged its members' tool tasks, while the members' tool workers were registered without one. Hence That makes this a consistency fix rather than a semantic change: it makes worker registration agree with the definition the SDK already uses one function away, not with a guess about intent. Two things follow:
Worth considering as a separate cleanup (not in this PR): all three sites should probably call one shared helper rather than each re-deriving it. Three independent derivations of one concept is how they drifted apart in the first place. Still genuinely open, and the reason I'd keep this draft for a moment: open question 2 — the serializer change alters what's sent to the server, and I've only verified |
Why neither CI caught thisWorth recording, because the fix is small but the reason it went unnoticed is structural. This repo's env:
CONDUCTOR_SERVER_VERSION: "3.32.0-rc.8" # pinned server release — bump deliberately
Meanwhile conductor-oss/conductor builds the server from
Neither side is misreporting. But between the two, the bug had nowhere to surface: the repo that owns the code tests against a server too old to show it, and the repo with a current server had stopped running the test. The part I'd raise separatelyThis repo's e2e never runs against a current server. rc.8 is the pin; the current line is well past it. That's a standing blind spot for exactly this class of defect — server/SDK contract drift — and it will hide the next one the same way. Two options, neither in scope for this PR:
The second is the shape most projects settle on: pinned for reproducible PR signal, latest on a schedule for early warning. Happy to open an issue for it if that's useful. Verification note for this PRBecause of the above, I validated against a server built from conductor-oss |
A stateful swarm/team shares one session, and the server domain-routes the tasks of everything inside it — taskToDomain maps the member agents' tool tasks to the session domain. But _register_workers read `agent.stateful` on the immediate agent only. When a tool hangs off a swarm MEMBER (the members are not themselves stateful), its worker registered with domain=None while its tasks were queued in the domain queue. Nothing polled them: pollCount=0, startTime=0, the enclosing FORK's JOIN never satisfied, and the workflow ran to TIMED_OUT. Propagate statefulness downward so worker registration matches how the server routes. ConfigSerializer._serialize_agent had the same gap — it dropped agent_stateful when recursing into sub-agents, so member tools were never marked stateful in the config sent to the server — and is fixed the same way to keep the two sides in agreement. Also fixes the e2e helper that hid this: _find_tasks_by_type matched only taskDefName, but compiler-generated system tasks carry their identity in referenceTaskName — handoff_check is emitted as INLINE, so its taskDefName is literally "INLINE". The assertion failed even though 20 handoff_check tasks existed and had COMPLETED. Verified against a conductor-oss server built from main: stock SDK FAILED — workflow RUNNING, 304s runtime fix only FAILED — workflow COMPLETED, handoff_check assert runtime + test fix PASSED — 118s + serializer fix PASSED — whole suite14 green, 6/6 full mainline e2e 136 passed, 1 failed, 7 skipped, 1 xfailed The one remaining failure is test_get_after_delete_returns_none, an unrelated scheduler 404-vs-None contract mismatch. test_non_stateful_no_domain is the guard against over-propagation and still passes, so statefulness is not leaking into agents that never declared it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
393787c to
9765b6d
Compare
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
rc4 is the release that lands the changes several deferred items were waiting on, so they all come due together: * env names. rc4's conftest migrated to CONDUCTOR_SERVER_URL / CONDUCTOR_AGENT_LLM_MODEL, so the rename TODO is resolved and the vars are live rather than inert. Noted in-file that they must go back if the pin is ever moved down to rc2 or earlier, which read only AGENTSPAN_*. * agentspan CLI, removed. rc4 dropped it entirely — no CredentialsCLI, no CLI_PATH, and test_suite16_cli_skills.py is gone — so provisioning it buys nothing. Recorded that it WAS load-bearing under rc2, where the credential suites reached their read-only skip by invoking it and removing it turned clean skips into FileNotFoundError failures, so it is not restored on stale reasoning. * the three Suite16 cli-skills known failures, removed. Their file no longer exists, and a key matching nothing now raises a KnownFailuresWarning, so leaving them would add noise every run. History kept in _HOWTO. * floor 137 -> 135. This is a coverage LOSS, not a fix: rc4 deleted test_suite16_cli_skills.py, and while three of its tests were xfail-ed here, two were genuinely passing (load_serve_and_run_by_name, run_ephemeral_executes_script_worker). Real coverage of the ephemeral/by-name CLI skill paths went away with the file, invisibly in pass/fail terms. Spelled out at the constant so the -2 is not read as routine. Verified locally against a server built from this branch: 135 passed, 7 skipped, 3 xfailed, 0 failed; floor gate green; plugin reports both remaining entries matching exactly one test with no MATCHED NOTHING. suite14's entry stays — its fixes are in conductor-oss/python-sdk#455, still unmerged, so rc4 does not carry them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bug
A stateful swarm/team shares one session, and the server domain-routes the tasks of everything inside it —
taskToDomainmaps the member agents' tool tasks to the session domain. ButAgentRuntime._register_workersreadagent.statefulon the immediate agent only.When a tool hangs off a swarm member — and members aren't themselves stateful — its worker registered with
domain=Nonewhile its tasks were queued in the domain queue. Nothing was polling there:The enclosing
FORK'sJOINnever satisfied and the workflow ran toTIMED_OUT.The fix
Propagate statefulness downward so worker registration matches how the server routes:
_register_workerstakesinherited_statefuland passes it to both recursion sites (sub-agents, and nestedagent_toolagents).ConfigSerializer._serialize_agenthad the same gap — it droppedagent_statefulwhen recursing into sub-agents, so member tools were never markedstatefulin the config sent to the server. Fixed the same way, so both sides agree. This one wasn't strictly required to make the test pass (the server derives routing from the swarm), but leaving the two halves disagreeing is how this bug happened in the first place.Also: an e2e helper that was hiding it
_find_tasks_by_typematched onlytaskDefName. Compiler-generated system tasks carry their identity inreferenceTaskName—handoff_checkis emitted as anINLINEtask, so itstaskDefNameis literally"INLINE":assert handoff_taskstherefore failed with 20handoff_checktasks present and COMPLETED. Now matches either field.Verification
Against a conductor-oss server built from
main, isolating each change:RUNNING, 304sCOMPLETED, failshandoff_checkassertsuite14green, 6/6The middle row is the useful one: it isolates the two bugs and shows the runtime fix alone is enough to get the workflow completing.
The one remaining failure is
test_get_after_delete_returns_none— an unrelated scheduler 404-vs-Nonecontract mismatch, present before this change.test_non_stateful_no_domainis the guard against over-propagation, and it still passes — statefulness isn't leaking into agents that never declared it. That's why I ran the whole file rather than the one test.Draft, because two things want a second opinion
statefulintended to be inherited? I've assumed yes — a member of a stateful composite shares its session, so its tools are stateful in every sense that matters to routing. If the intended semantics are "only the declaring agent", then the fix belongs on the server side instead (don't domain-route tools that aren't marked stateful), and this PR is wrong.Notes
*_transfer_to_*task defs; those register fine and the handoff itself succeeds. Corrected in conductor-oss/conductor@63c0715b7.conftest's unconditionalflaky(reruns=2).