fix(agents): thread adopt_existing_workspace through create_agent_internal facade - #1769
Conversation
…ernal facade (#1768) The deploy-local path (#950) passes adopt_existing_workspace=True through create_agent_fn, but the router facade create_agent_internal never accepted the kwarg (added to the service layer in #1667), so every POST /api/agents/deploy-local and MCP deploy_local_agent call died with "TypeError: unexpected keyword argument 'adopt_existing_workspace'" -> 500. Add the kwarg (default False) to the facade and forward it to the service-layer call. All other callers are unaffected. Validated live against the dev stack: the 7 failing tests from the issue plus the rest of test_deploy_local.py (16/16) now pass; red baseline reproduced the exact TypeError before the fix. Fixes #1768 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Resolve by running |
PR Validation Report —
|
| Category | Status | Notes |
|---|---|---|
| Commit Messages | ✅ | Single commit 443141db, conventional fix(agents):, states what + why |
| Base Branch | ✅ | Targets dev |
| PR Size | ✅ | 1 file, +4/−2 — minimal |
| Status Auto-Promotion | ✅ | Fixes #1768 same-repo closing keyword → auto-promotes to status-in-dev on merge |
| Roadmap | ✅ | Issue resolves in abilityai/trinity (both trackers probed); correct priority + type labels |
| Requirements | ➖ | Bug fix restoring documented behavior — no scope change |
| Architecture | ➖ | No endpoint, schema, integration or component change |
| Feature Flows | ➖ | Behavior restored, not changed |
| Security Check | ✅ | 4.1–4.6 clean — no keys, emails, public IPs, .env, hardcoded secrets, credential files |
| Infrastructure | ➖ | No compose / Dockerfile / nginx change |
| Build Packaging (4.8) | ➖ | No new top-level src/backend/*.py |
| Config Packaging (4.9) | ➖ | No new os.getenv() var |
| Test Adequacy | No named regression test in the diff — see Warnings | |
| Code Quality | ✅ | Focused, no scope drift, matches existing facade pattern |
| Requirements Trace | ✅ | Maps 1:1 to issue AC #1; ACs #2–#5 evidenced by the live-stack run in the PR body |
End-to-end trace (verified against origin/dev, not just the diff)
The kwarg chain resolves correctly in both directions:
- Origin —
services/agent_service/deploy.py:611-621callscreate_agent_fn(..., adopt_existing_workspace=True). - Injection point —
routers/agents.py:467passescreate_agent_fn=create_agent_internal— the facade, confirming the deploy path never reachescrud.pydirectly. - Break (pre-fix) — facade signature at
routers/agents.py:103had noadopt_existing_workspaceand no**kwargs→TypeError→ 500. Matches the stack in bug: deploy-local returns 500 — create_agent_internal facade drops adopt_existing_workspace kwarg #1768. - Consumer —
crud.py:1729-1736accepts the kwarg; consumed atcrud.py:1790by_guard_leftover_workspace_volume(...)(crud.py:279-312), which skips the bug: agent creation silently reuses a pre-existing Docker volume of the same name (get-then-create, no adopt/refuse decision) #1667 409-refusal when adopting. The new plumbing does reach the real consumer. - MCP surface —
src/mcp-server/src/tools/agents.ts:706,779POSTs to/api/agents/deploy-local, riding the same repaired path. No MCP change needed (Invariant feat: SMARTS trading pipeline with Telegram notifications and Miro visualization #13 — no contract change).
All other facade callers verified unaffected: routers/agents.py:436 (3 positionals + skip_name_sanitization=), services/system_service.py:849 via _default_create_agent_fn (all-keyword), and cornelius_agent_service.py:181 (imports crud.create_agent_internal directly, bypassing the facade). A trailing defaulted kwarg is inert for all three.
Facade/service parity is now complete — the facade covers every crud.create_agent_internal param except ws_manager, which it deliberately injects. This was the last drifted kwarg.
Bug attribution independently confirmed (not taken from the PR body): git log -S"adopt_existing_workspace" pins introduction to f55eb6cb (2026‑07‑17, PR #1666), whose --name-only touches crud.py and deploy.py but not routers/agents.py. The later #1484 decomposition (f6b81ace, PR #1691) never touched the facade either. So deploy-local has 500'd on every call for ~8 days.
Architectural hot-spot check (#1484): the diff touches only the router facade — not crud.create_agent_internal or any fenced phase-helper. No re-monolithing, no helper grew, no if docker_client: fencing disturbed.
Issues found
Critical (block merge) — none.
Warnings (review required)
- The 7 failing tests named in bug: deploy-local returns 500 — create_agent_internal facade drops adopt_existing_workspace kwarg #1768 live in
tests/test_deploy_local.py/tests/test_agent_quota.py— root-level live-stack integration tests. CI runspytest unit/ -m "not slow"only, so none of them run in CI. - The bug: agent creation silently reuses a pre-existing Docker volume of the same name (get-then-create, no adopt/refuse decision) #1667 unit coverage that does run (
test_1484_create_agent_characterization.py:699,732,test_1664_renamed_agent_volume_safety.py:689) callscrud.create_agent_internaldirectly — never the router facade, precisely the seam that broke. - Net: fix(volumes): rename-safe volume identity — sweep, purge, create and recovery (#1664, #1665, #1667) #1666 shipped fully green while breaking a P0 path, and after this PR that escape route is still open.
Cheap, permanent guard (~10 lines, matching 7 existing parity-test precedents — test_1560_agent_redis_key_parity.py, test_credential_paths_parity.py, test_model_context_parity.py, test_agent_cleanup_parity.py, …):
# tests/unit/test_1768_create_agent_facade_parity.py
import inspect
def test_facade_accepts_every_service_kwarg():
from routers.agents import create_agent_internal as facade
from services.agent_service.crud import create_agent_internal as service
svc = set(inspect.signature(service).parameters) - {"ws_manager"} # facade injects this
assert svc <= set(inspect.signature(facade).parameters)Judgment: warning, not a blocker. The fix is correct by inspection (full chain traced above), the default is inert for every other caller, and the author documented a red baseline plus 16/16 test_deploy_local.py green on a live stack. Holding a P0 restore of a fully-dead deploy path hostage to a test file is the wrong trade — but the guard should land as an immediate follow-up.
Suggestions (optional)
docs/memory/feature-flows/local-agent-deploy.mdstep 11 (line 229) never documented the adopt contract, and the Revision History has no bug: agent creation silently reuses a pre-existing Docker volume of the same name (get-then-create, no adopt/refuse decision) #1667 entry. Pre-existing gap from fix(volumes): rename-safe volume identity — sweep, purge, create and recovery (#1664, #1665, #1667) #1666 — but that kwarg is now the load-bearing part of the deploy↔facade contract and its absence just cost a P0.- Adjacent facade divergence (same class, non-breaking): the facade annotates
request: Requestwhilecruddeclaresrequest: Optional[Request] = None, andsystem_seed_service.py:272already flows aNonethrough it on every fresh install (ent#124). Harmless at runtime; worth aligning whenever this function is next touched. The parity test above would not catch it (names only).
Recommendation
✅ APPROVE
Correct, genuinely minimal fix for a P0 regression that has had POST /api/agents/deploy-local and the MCP deploy_local_agent tool returning 500 on every call since 2026‑07‑17. Kwarg threaded end-to-end, default inert for all three other callers (each individually verified), #1484 phase-helper structure untouched, all security greps clean, packaging checks N/A, CI green across 18 checks.
Follow-up requested: file the ~10-line facade/service signature-parity test as a type-bug / complexity-low issue referencing #1768 — the drift class that produced this P0 is otherwise still CI-invisible.
🤖 Validated with Claude Code via /validate-pr
AndriiPasternak31
left a comment
There was a problem hiding this comment.
Approved via /validate-pr — see the validation report above. Correct minimal P0 fix; kwarg traced end-to-end (deploy.py → facade → _guard_leftover_workspace_volume), all three other facade callers verified inert, #1484 phase-helper structure untouched, CI green across 18 checks. One non-blocking follow-up requested: the facade/service signature-parity unit test.
Fixes #1768
Summary
POST /api/agents/deploy-local(and the MCPdeploy_local_agenttool) returned 500 on every call:deploy.pypassesadopt_existing_workspace=Truethroughcreate_agent_fn, but the router facadecreate_agent_internalinrouters/agents.pynever accepted that kwarg — #1667 added it only to the service-layercrud.pyfunction. The call died with:Change
One facade signature fix in
src/backend/routers/agents.py: acceptadopt_existing_workspace: bool = Falseand forward it to the service-layer_create_agent_internal(...)call. DefaultFalsepreserves every other caller's behavior (all callers checked — leading positionals + keywords only, so a trailing default kwarg is inert for them).Validation (live dev stack)
TypeErrorabove returned as a 500 by the running backend.test_deploy_local.py—TestDeployLocalSuccessful(×4),TestDeployLocalResponse::test_response_structure,TestDeployLocalVersioning::test_redeploy_creates_version— plustest_agent_quota.py::TestAgentQuotaRedeploy::test_redeploy_existing_agent_bypasses_quota. Real agent containers created/torn down (7 passed in 150s).test_deploy_local.py: 16/16 passed (98s) — auth, validation, security, limits classes included.🤖 Generated with Claude Code