Skip to content

refactor(adapters/langgraph): align ManagerWorkers code and tests with codebase style - #32

Merged
spichen merged 1 commit into
feat/managerworkers-adapterfrom
review/pr-211-managerworkers-cleanup
Aug 2, 2026
Merged

refactor(adapters/langgraph): align ManagerWorkers code and tests with codebase style#32
spichen merged 1 commit into
feat/managerworkers-adapterfrom
review/pr-211-managerworkers-cleanup

Conversation

@spichen

@spichen spichen commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Cleanup pass over oracle#211, plus one bug fix its own test exposed.

Bug fix: ManagerWorkers as a flow step could not produce its declared output

test_managerworkers_runs_as_a_flow_step_with_data_edge_inputs fails on the PR head: the ManagerWorkers parent graph runs over MessagesState, so the manager subgraph's structured_response never propagates outward, and extract_outputs_from_invoke_result finds nothing — any manager with declared outputs raised ValueError: Expected node ... to have a value for property .... (The fallback the test relies on lives in the unmerged feat/agent-single-string-output branch.)

AgentNodeExecutor._format_agent_result now answers a ManagerWorkers node's single string output with the manager's final message content, and raises NotImplementedError for other output shapes.

Test cleanup (797 → 421 lines)

  • Removed the box-drawing section dividers and module docstring; no other test file uses them.
  • Module-level pyagentspec imports and a shared _agent() helper instead of ~10 lines of Agent(...) boilerplate repeated per test; langchain/langgraph imports stay in-function, matching test_swarm.py / test_agentnode.py.
  • pytestmark = pytest.mark.usefixtures(...) instead of an empty-bodied autouse fixture.
  • Dropped micro-tests of trivial private helpers (_messages_of, _surface_to_parent_command, tool name/description, echo-worker wrapper) — all covered by the end-to-end delegation tests — and merged duplicates (test_normalize_identifier duplicated test_safe_node_name; the roster and routing tests are consolidated).
  • The astream_events-unwrapped test asserts on compiled.__dict__ instead of patched-function __name__s.

Source cleanup

  • Dropped the @lru_cache on the delegation-tool factory, replaced the __slots__ class _WorkerSubgraphNode with closures, removed the _tc_get dict-or-object shim (langchain tool calls are always dicts), and inlined _surface_to_parent_command into the delegation tool.
  • Trimmed decision-justifying comments to codebase density while keeping the load-bearing ones (no-goto invariant, one-Send-per-delegation, ambient-config namespacing).
  • black formatting fix for one over-long line in _langgraphconverter.py.

Net: −452 lines against the PR head. Verified offline: 17/17 ManagerWorkers tests pass, plus the langgraph flows suite, swarm, tracing/serialization/validation core suites.

…h codebase style

Trim reviewer-directed comments to codebase density; drop the lru_cache on the
delegation-tool factory, the _WorkerSubgraphNode class (now closures) and the
dict-or-object tool-call shim. Rewrite the tests to repo conventions: module-level
pyagentspec imports, a shared _agent() helper, pytestmark instead of an empty
autouse fixture, no section dividers, and behavior-level coverage instead of
micro-tests of private helpers.

Also fix a real bug the flow-step test exposed: the ManagerWorkers parent graph
runs over MessagesState, so a structured_response can never reach the node
executor and any declared output raised ValueError. A ManagerWorkers flow step
now answers its single string output with the manager's final message, and
rejects other output shapes with NotImplementedError.
@spichen
spichen merged commit 7230275 into feat/managerworkers-adapter Aug 2, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant