Skip to content

Migrate claude --print to Agent SDK - #1088

Merged
jwbron merged 9 commits into
mainfrom
egg/migrate-agent-sdk
Mar 14, 2026
Merged

Migrate claude --print to Agent SDK#1088
jwbron merged 9 commits into
mainfrom
egg/migrate-agent-sdk

Conversation

@james-in-a-box

Copy link
Copy Markdown
Contributor

Summary

All headless Claude Code sessions used claude --print with subprocess spawning across 15+ independent sites that constructed identical command lists. This PR consolidates all invocations into a shared egg_agent package.

Changes:

  • Created shared/egg_agent/ package with build_agent_command() for container commands and run_agent_async()/run_agent() wrapping claude_agent_sdk.query() for in-process execution
  • Replaced 13 duplicate command-building blocks in orchestrator/routes/pipelines.py, coordinator.py, and concurrent_executor.py with single build_agent_command() calls
  • Gutted sandbox/llm/claude/runner.py to a thin wrapper delegating to the SDK client
  • Migrated egg-health-inspect from subprocess.run(["claude", ...]) to egg_agent.client.run_agent()
  • Removed dead code: stream-json parsing, _read_lines_unbuffered, version checking, allowed_tools config field

Not changed: Interactive mode (os.execvpe), shell alias, shared/egg_contracts/orchestrator.py:AgentResult (different class).

Issue: #1080

Test plan:

  • All 56 related tests pass (18 new + 38 existing)
  • EGG100 linter: zero violations
  • build_agent_command() output verified to match original command format exactly
  • Verify python3 -m egg_agent --model sonnet --max-turns 1 "Say hello" works in sandbox container

Authored-by: egg

Create shared/egg_agent package wrapping claude_agent_sdk.query() for
in-process use and build_agent_command() for container command construction.
Replace 13 duplicate command-building sites in the orchestrator with a
single function call, gut the sandbox runner to delegate to the SDK client,
and migrate egg-health-inspect from subprocess to the SDK.

Issue: #1080
@james-in-a-box

This comment has been minimized.

@james-in-a-box

james-in-a-box Bot commented Mar 14, 2026

Copy link
Copy Markdown
Contributor Author
Autofix tracking
{"Lint/Python": 3, "Test/Unit Tests": 1, "Lint/Custom Checks": 1}

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

Conflict Resolution Summary

Resolved merge conflicts with main (specifically commit 476c39d from PR #1085: "Enforce spawn dependency ordering and fix coordinator bugs"):

File Category Resolution
orchestrator/routes/coordinator.py (imports) Additive Included both: from egg_agent import build_agent_command (PR) and get_role_definition added to egg_contracts.agent_roles import (main)
orchestrator/routes/coordinator.py (spawn logic) Complementary Kept consensus protocol reminder from main (appends stay-alive instructions to agent_prompt), then used build_agent_command(agent_prompt) from PR instead of inline ["claude", ...] command list
orchestrator/concurrent_executor.py Auto-merged No conflicts — git resolved automatically
orchestrator/routes/pipelines.py Auto-merged No conflicts — git resolved automatically

Resolution rationale for the spawn logic conflict: Main added a new feature (consensus protocol reminder appended to agent_prompt) while the PR refactored command building into build_agent_command(). These changes are complementary — the prompt modification happens before the command is built, so both are preserved by keeping the prompt modification and then passing the enriched prompt to build_agent_command().

Verification:

  • Ruff lint + format: all passed
  • 70/70 coordinator route tests passed
  • 1932/1932 orchestrator tests passed (5 pre-existing failures from missing starlette module, unrelated)

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No agent-mode design concerns.

The consolidation into egg_agent follows the "shared infrastructure" good pattern from the design guide. The orchestrator continues to use claude --print via container commands (build_agent_command), preserving the security boundary. The SDK client (claude_agent_sdk.query()) is only used inside sandbox containers — it's the programmatic equivalent of claude --print, not a direct API call. Model aliases used throughout.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Migrate claude --print to Agent SDK

Clean, well-scoped refactoring that eliminates 13 duplicate command-building blocks and replaces subprocess-based Claude invocation with the Agent SDK. Net -91 lines is good. No blocking issues found.

Issues

1. runner.py wrapper silently drops new AgentResult fieldssandbox/llm/claude/runner.py:60-68

llm.result.AgentResult is now a re-export of egg_agent.result.AgentResult — they are the same class. But the wrapper reconstructs a new instance copying only the original 6 fields, silently discarding cost_usd, num_turns, duration_ms, and session_id:

# Current — drops 4 fields
return AgentResult(
    success=sdk_result.success,
    stdout=sdk_result.stdout,
    stderr=sdk_result.stderr,
    returncode=sdk_result.returncode,
    error=sdk_result.error,
    metadata=sdk_result.metadata,
)

Since both types are the same class, just return the SDK result directly:

return sdk_result

Or if you want to keep the explicit mapping for documentation purposes, include the new fields too.

2. Dead stderr_parts variableshared/egg_agent/client.py:84

stderr_parts: list[str] = [] is initialized but never appended to. It's referenced in the TimeoutError handler and success path as "".join(stderr_parts), which always evaluates to "". Remove the variable and use "" directly.

3. Docstring claims python3 -m egg_agent but code builds claude commandshared/egg_agent/command.py:1-6,21-23

Module docstring says:

"run python3 -m egg_agent (or the claude CLI as a fallback)"

Function docstring says:

"Inside the container this is executed as python3 -m egg_agent ..."

The actual code builds a claude CLI command. The docstrings describe behavior that doesn't exist yet. Fix to match reality or add a note that this is planned future behavior.

4. Missing noqa: E402 on mid-file importorchestrator/routes/pipelines.py:145

The from egg_agent import build_agent_command import is placed after the Blueprint(...) definition at line 142, making it a module-level-import-not-at-top. The adjacent import at line 146 already has # noqa: E402. If the linter is configured to check this, the new import needs the same annotation.

Observations (no action required)

  • The allowed_tools removal from ClaudeConfig is safe — confirmed unused anywhere.
  • All private functions removed from runner.py (_read_lines_unbuffered, _classify_error, etc.) are confirmed to have no external consumers.
  • The max_turns values at all call sites match the old inline commands exactly (200 default, 100 for checker, 50 for reviewer).
  • egg-health-inspect migration from subprocess to run_agent() is clean. The retry logic correctly handles the new error surface.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No agent-mode design concerns with the new changes.

The delta since my last review is a single commit addressing code review feedback (dropped fields fix, dead code removal, docstring corrections, lint suppression). None of these affect agent-mode design — the architecture reviewed and approved previously is unchanged.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: Feedback Addressing Commit

All four issues from the previous review are properly fixed:

1. runner.py dropped AgentResult fields — Fixed. Returns sdk_result directly instead of reconstructing with only 6 fields. Since llm.result.AgentResult is a re-export of egg_agent.result.AgentResult (same class), this is correct and now preserves cost_usd, num_turns, duration_ms, and session_id.

2. Dead stderr_parts variable — Fixed. Variable removed, "".join(stderr_parts) replaced with "" in both the TimeoutError handler and the success return path.

3. Inaccurate docstrings in command.py — Fixed. Module and function docstrings now accurately describe "claude CLI" instead of claiming python3 -m egg_agent.

4. Missing noqa: E402 — Fixed. pipelines.py:145 now has the annotation, consistent with the adjacent import.

No new issues introduced. The changes are mechanical and well-scoped.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor Author
egg is addressing review feedback...

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg feedback addressed. View run logs

18 previous review(s) hidden.

@jwbron
jwbron merged commit f1e3d47 into main Mar 14, 2026
30 checks passed
jwbron added a commit that referenced this pull request Mar 14, 2026
The egg_agent shared module was added in #1088 but the orchestrator
Dockerfile was not updated to copy it into the container image, causing
an ImportError crash loop on startup.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jwbron added a commit that referenced this pull request Mar 14, 2026
…#1100)

* Fix orchestrator crash: add missing egg_agent module to Dockerfile

The egg_agent shared module was added in #1088 but the orchestrator
Dockerfile was not updated to copy it into the container image, causing
an ImportError crash loop on startup.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Update Dockerfile comment to include egg_agent in shared modules list

* Remove spawn dependency enforcement to allow concurrent agents

The dependency blocking added in #1085 prevented agents from spawning
until their declared dependencies completed. This is wrong — agents
should run concurrently and communicate via the messaging system.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix stale container/agent status after pipeline cancellation

When a pipeline is cancelled, cleanup_pipeline() removes Docker containers
but the pipeline state store still shows them as "running". This adds
_mark_pipeline_records_cancelled() which syncs the persisted state after
container cleanup — marking containers as REMOVED, agents as FAILED, and
coordinator spawn records as cancelled.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix cancel state sync: reload pipeline before marking records

The previous approach mutated the stale pipeline object returned by
update_pipeline(), which could overwrite coordinator state changes made
between the status update and container cleanup. Now reloads the latest
state from the store after containers are killed, ensuring we mark the
most up-to-date records and don't clobber concurrent writes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Address review feedback: fix error message, rename function, clean up docs

* Fix spawned agents missing EGG_ORCHESTRATOR_URL env var

container_spawner.py set EGG_PIPELINE_ID but never set
EGG_ORCHESTRATOR_URL, causing the sandbox entrypoint to bail out
with "Orchestrator mode enabled but missing URL or pipeline_id"
(exit 124). The other two spawn paths (sandbox_template.py and
routes/pipelines.py) both set it correctly — this was the only
one missing it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Increase cancel_task MCP endpoint timeout from 30s to 120s

Cancellation involves stopping containers which regularly exceeds the
default 30s request timeout, causing spurious "timed out" errors.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix test to expect timeout=120 in cancel_task

* Align orchestrator URL construction across spawn paths

Use ORCHESTRATOR_EXTERNAL_IP (static IP) instead of
ORCHESTRATOR_CONTAINER_NAME (hostname) in container_spawner.py for
public mode, matching the pattern in routes/pipelines.py.

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
jwbron added a commit that referenced this pull request Mar 14, 2026
* Fix orchestrator crash: add missing egg_agent module to Dockerfile

The egg_agent shared module was added in #1088 but the orchestrator
Dockerfile was not updated to copy it into the container image, causing
an ImportError crash loop on startup.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Update Dockerfile comment to include egg_agent in shared modules list

* Remove spawn dependency enforcement to allow concurrent agents

The dependency blocking added in #1085 prevented agents from spawning
until their declared dependencies completed. This is wrong — agents
should run concurrently and communicate via the messaging system.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix stale container/agent status after pipeline cancellation

When a pipeline is cancelled, cleanup_pipeline() removes Docker containers
but the pipeline state store still shows them as "running". This adds
_mark_pipeline_records_cancelled() which syncs the persisted state after
container cleanup — marking containers as REMOVED, agents as FAILED, and
coordinator spawn records as cancelled.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix cancel state sync: reload pipeline before marking records

The previous approach mutated the stale pipeline object returned by
update_pipeline(), which could overwrite coordinator state changes made
between the status update and container cleanup. Now reloads the latest
state from the store after containers are killed, ensuring we mark the
most up-to-date records and don't clobber concurrent writes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Replace implicit consensus READY with agent restart

The consensus wrapper was blindly auto-signaling READY when an agent
exited cleanly without participating in consensus. This masked the
real problem: the agent didn't follow the consensus protocol.

Instead of faking consensus, the wrapper now restarts Claude with a
recovery prompt that explains what happened and instructs the agent
to assess state and explicitly signal READY or continue working.
Restarts are capped at 2 (configurable). After exhausting restarts,
the wrapper enters a passive wait loop but does NOT auto-signal READY.

Also removes the orchestrator-level implicit READY registration in
the concurrent phase poll loop (pipelines.py) — agents must explicitly
participate in consensus.

Issue: #1081

* Fix checks: apply automated formatting fixes

* Exit with failure after max restarts instead of passive wait

After exhausting restart attempts, the wrapper now exits with code 1
instead of entering a passive wait loop. There's nothing to wait for —
no process inside the container can make progress. The non-zero exit
triggers the orchestrator's agent failure path (HITL decision).

* Update docs for restart-based consensus wrapper

Update concurrent-execution.md to reflect the new behavior: the wrapper
restarts the agent with a recovery prompt instead of auto-signaling
READY. Remove references to EGG_CONSENSUS_WRAPPER_TIMEOUT (no longer
used) and implicit READY (removed from orchestrator).

* Address review feedback on consensus restart PR

* Fix consensus wrapper to use pipeline status endpoint

All consensus checks were querying egg-orch message status, which returns
message bus statistics — not consensus data. Changed to egg-orch pipeline
status with the correct nested path (data.concurrent.consensus) matching
the real API response.

Also addressed:
- Added MAX_READY_POLL_CYCLES (default 10) as a separate constant for the
  READY polling loop instead of reusing MAX_RESTARTS (default 2)
- Extracted claude mock creation to shared _make_mock_claude() helper
- Updated all test mocks to use pipeline status and the correct response
  structure

* Add behavioral test for READY polling path

---------

Co-authored-by: James Wiesebron <jameswiesebron@khanacademy.org>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Co-authored-by: egg <egg@localhost>
Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
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