fix(deploy): validate an archive .mcp.json before it is persisted, not after (#2006)

src/backend/services/agent_service/deploy.py

@@ -83,6 +83,50 @@ def _validate_archive_mcp_config(mcp_file: Path, version_name: str,
         )
 
 
+def _remove_partial_deploy(dest_created: Path | None) -> None:
+    """Remove a deployed-templates directory a failed deploy left behind (#2006).
+
+    Second layer under the moved `.mcp.json` gate: that gate now runs before
+    the copy, but every OTHER failure between `copytree` and `create_agent_fn`
+    (`_prepopulate_workspace_from_template`'s 500, a docker outage) leaves the
+    same addressable residue, because `/data/deployed-templates` is a member of
+    `_LOCAL_TEMPLATE_ROOTS` and the directory name is exactly the
+    `local:<version_name>` id the caller can pass to `POST /api/agents`.
+
+    Deliberately NOT a blanket cleanup in `finally`: the caller clears its
+    handle before creation starts, so a directory a container may already
+    reference is never removed. Never raises — this runs on an error path and
+    must not replace the real failure with a cleanup failure.
+
+    The containment check is repeated HERE rather than inherited from the
+    caller's #950 guard. `rmtree` is a destructive sink whose path descends
+    from a caller-supplied name, and "the caller already validated it" is a
+    property that survives exactly until someone adds a second call site. It is
+    also the barrier CodeQL recognizes (normalize → prefix-check → use the
+    normalized value), which is why the flagged alert was fixed rather than
+    dismissed: on a `rmtree`, "probably confined" is not the standard.
+    """
+    if dest_created is None:
+        return
+
+    base = os.path.normpath(DEPLOYED_TEMPLATES_DIR_IN_BACKEND)
+    target = os.path.normpath(str(dest_created))
+    if not target.startswith(base + os.sep) or target == base:
+        logger.error(
+            "refusing to remove a partial deploy outside the deployed-templates "
+            "directory: %s", target,
+        )
+        return
+
+    try:
+        shutil.rmtree(target)
+        logger.info("Removed partial deploy directory: %s", target)
+    except Exception as e:  # noqa: BLE001 — the original error is what matters
+        logger.warning(
+            "could not remove partial deploy directory %s: %s", dest_created, e
+        )
+
+
 def _prepopulate_workspace_from_template(version_name: str, template_dir: Path) -> None:
     """Pre-populate `agent-{version_name}-workspace` with the template files (#950).
 
@@ -359,6 +403,12 @@ async def deploy_local_agent_logic(
         DeployLocalResponse with deployment details
     """
     temp_dir = None
+    # #2006: the deployed-templates dir this call created, while it is still
+    # this call's to remove. Cleared just before `create_agent_fn` — once
+    # creation starts, the directory may be referenced by a container mount
+    # spec, so removing it on a late failure would break a half-created agent
+    # rather than clean up after one.
+    dest_created = None
 
     try:
         # 1. Validate archive size
@@ -441,6 +491,26 @@ async def deploy_local_agent_logic(
 
         base_name = sanitize_agent_name(base_name)
 
+        # 6a. Structure-validate an archive-supplied `.mcp.json` (ent#213) —
+        # on the EXTRACTED copy, BEFORE anything is persisted or stopped
+        # (#2006). The guard used to run 32 lines after `copytree`, so a 400
+        # aborted the deploy while leaving the rejected config on disk under
+        # `/data/deployed-templates/<version_name>/` — a live member of
+        # `_LOCAL_TEMPLATE_ROOTS`, therefore reachable by a subsequent
+        # `POST /api/agents {"template": "local:<version_name>"}`. A guard that
+        # runs after the persist is not a gate.
+        #
+        # `temp_dir` is the only thing written before this point and the
+        # `finally` already removes it, so a rejection now leaves nothing
+        # behind. Running here also precedes the quota check and the
+        # stop-previous-version step, so a deploy that will be refused no
+        # longer stops a running agent on its way to the refusal.
+        archive_mcp_file = extract_root / ".mcp.json"
+        if archive_mcp_file.exists():
+            _validate_archive_mcp_config(
+                archive_mcp_file, base_name, getattr(current_user, "email", None)
+            )
+
         # 6b. Agent quota enforcement: per-role limits (QUOTA-001)
         # Skip for redeploys of existing agents owned by this user
         existing_versions = get_agents_by_prefix(base_name)
@@ -532,6 +602,7 @@ async def deploy_local_agent_logic(
             shutil.rmtree(dest_path)
 
         shutil.copytree(extract_root, dest_path)
+        dest_created = dest_path
         logger.info(f"Copied agent template to: {dest_path}")
 
         # 10. Create agent
@@ -565,10 +636,11 @@ async def deploy_local_agent_logic(
 
         mcp_file = dest_path / ".mcp.json"
         if mcp_file.exists():
-            # ent#213: structure-validate an archive-supplied `.mcp.json` before
-            # it is pre-populated into the workspace, matching the inject path.
-            _validate_archive_mcp_config(mcp_file, version_name,
-                                         getattr(current_user, "email", None))
+            # ent#213's validation moved to step 6a (#2006) — it now runs on
+            # `extract_root` before this copy exists. This is a copy of the
+            # bytes that already passed, so re-validating here would only be
+            # able to fail on something written between the two points, and
+            # nothing writes `.mcp.json` in that window. Bookkeeping only.
             credentials_imported[".mcp.json"] = "from_archive"
 
         # Write credentials from request to template directory
@@ -608,6 +680,10 @@ async def deploy_local_agent_logic(
         # run anyway since no /template bind is set up — see crud.py).
         _prepopulate_workspace_from_template(version_name, dest_path)
 
+        # Hand the directory over: from here it can be referenced by the
+        # container's mount spec, so it is no longer ours to delete (#2006).
+        dest_created = None
+
         agent_status = await create_agent_fn(
             agent_config,
             current_user,
@@ -636,8 +712,10 @@ async def deploy_local_agent_logic(
         )
 
     except HTTPException:
+        _remove_partial_deploy(dest_created)
         raise
     except Exception as e:
+        _remove_partial_deploy(dest_created)
         raise HTTPException(
             status_code=500,
             detail=f"Failed to deploy local agent: {str(e)}"

tests/registry.json

@@ -1635,6 +1635,18 @@
         "regression"
       ],
       "description": "Claude Code's internal [ede_diagnostic] header must never be surfaced as the error cause (#1849): stream_parser filters diagnostic entries from result.errors at BOTH the max_turns and execution_error branches, joins the remaining real errors, keeps the token-stripped diagnostic payload as labelled context, and no longer raises on a malformed errors shape (which previously landed as HTTP 200 success). Acceptance test drives a marker-first errors[] through _finalize_headless_result to routers/sessions.py::_is_resume_not_found."
+    },
+    {
+      "file": "unit/test_2006_deploy_residue.py",
+      "feature": "#2006",
+      "added": "2026-08-05",
+      "categories": [
+        "backend",
+        "unit",
+        "security",
+        "deploy"
+      ],
+      "description": "deploy-local validated an archive-supplied .mcp.json 32 lines AFTER shutil.copytree(extract_root, dest_path), and the finally block removed only temp_dir - so a config the ent#213 validator refused stayed on disk under /data/deployed-templates/<version_name>/, a live member of crud._LOCAL_TEMPLATE_ROOTS, addressable by the same caller via POST /api/agents {template: local:<version_name>}. A guard that runs after the persist is not a gate. Fix: the guard moved to step 6a, running on extract_root before the copy AND before the quota check and the stop-previous-version step (a doomed deploy no longer stops a running agent on its way to a 400), plus _remove_partial_deploy as a second layer for failures between copytree and create_agent_fn (_prepopulate_workspace_from_template's 500) - deliberately handed over before creation starts, since a directory a container may already reference must not be removed on a late failure. ent#213's suite tests _validate_archive_mcp_config in isolation ('the full deploy flow needs Docker'), which is exactly why the ordering was never covered; these drive the REAL deploy_local_agent_logic and assert on the filesystem, needing no Docker because the rejection path now reaches nothing external. Covers: the issue's reproduction, local:<name> resolving to nothing, the ordering pinned twice (behaviourally by exploding every post-guard step, and statically by source order - the behavioural residue assertion alone cannot distinguish the two candidate fixes once cleanup exists), the valid path reaching creation with the template in place, an archive with no .mcp.json, and both sides of the cleanup boundary."
     }
   ]
 }