Fix #2070: restart_agent works for roles with underscores

[jwbron]

Summary

• Fixes orchestrator: restart_agent crashes for any role with underscore (task_planner, reviewer_plan, reviewer_refine, etc.) #2070. `restart_agent_job` constructed the k8s Job name from `agent_role.value` directly, so any role with an underscore (8 of 14 roles, including every reviewer) crashed the call. The constructed name was also missing the `egg-sandbox-` JOB_PREFIX.
• Extracts `_build_k8s_job_names()` so spawn and restart agree on the `(unprefixed, prefixed)` pair. Restart now calls `delete_job` with the prefixed k8s name and `delete_session_by_container` with the unprefixed name the gateway session was registered under at spawn time.
• Adds a parametrized regression test that exercises every `AgentRole` whose value contains an underscore (`task_planner`, `risk_analyst`, `reviewer_code`, `reviewer_contract`, `reviewer_agent_design`, `reviewer_refine`, `reviewer_plan`, `conflict_resolver`).

Why two identifiers

`KubernetesClient.create_container` prepends `JOB_PREFIX` (`egg-sandbox-`) when creating the Job, but the gateway session is registered with the unprefixed name. Routing both calls through `remove_agent_job` (which uses one identifier for k8s and the gateway) only works when those happen to coincide — which they don't. Spawn already inlined the right pattern; restart now does too.

Test plan

• `pytest orchestrator/tests/test_kubernetes_spawner.py` — 73 passed (incl. 8 new parametrized cases)
• `pytest orchestrator/tests/test_decisions_routes.py orchestrator/tests/test_restart_mcp_tools.py orchestrator/tests/test_pipelines_api.py` — 104 passed
• `ruff check` clean on changed files
• Manual: trigger `restart_agent` on a `task_planner` (or any underscore role) in a live pipeline and confirm the Job is recreated

🤖 Generated with Claude Code

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 1}

[egg-reviewer]

Review

The core fix is correct and well-targeted. _build_k8s_job_names() is the right factoring — both spawn and restart now derive their (unprefixed, prefixed) pair from a single helper, and the underscore→hyphen conversion is applied consistently.

I traced the data flow end-to-end:

• spawn_agent_job registers the gateway session with container_id=job_name (hyphenated, unprefixed) at kubernetes_spawner.py:595, and passes the same job_name to KubernetesClient.create_container, which then prepends JOB_PREFIX internally to land the actual k8s Job at egg-sandbox-{job_name}.
• The new restart_agent_job now calls delete_job(actual_k8s_job_name, ...) (prefixed) at kubernetes_spawner.py:1056 and delete_session_by_container(job_name) (unprefixed) at 1068. Both match what spawn registered. ✓
• The parametrized regression test exercises every underscore role and asserts the prefixed/unprefixed split correctly. Good coverage.

Three non-blocking observations:

1. test_restart_handles_stop_failure_gracefully is now a no-op assertion — orchestrator/tests/test_restart_agent.py:195-209

mock_docker_client.remove_container.side_effect = ContainerOperationError("timeout")

The new code path no longer calls remove_container — it calls delete_job directly. Setting remove_container.side_effect no longer exercises any failure path, so this test passes trivially regardless of the code's behavior. Either delete the test or rewrite it to set delete_job.side_effect = JobOperationError("timeout") so it actually verifies the new exception handling at kubernetes_spawner.py:1061.

2. Log message at kubernetes_spawner.py:1062 is misleading for real errors

except (PodNotFoundError, JobOperationError) as e:
    logger.info(
        "No existing Job found during restart (already removed)",

PodNotFoundError genuinely means "already removed," but JobOperationError is raised by KubernetesClient.delete_job for any non-404 failure (auth issues, control-plane unavailability, validation errors — see kubernetes_client.py:705-709). Logging that at info level with "already removed" hides a real failure. Suggest splitting:

except PodNotFoundError:
    logger.debug("No existing Job found during restart", job_name=actual_k8s_job_name)
except JobOperationError as e:
    logger.warning("Failed to delete existing Job during restart, continuing",
                   job_name=actual_k8s_job_name, error=str(e))

That way a real k8s API failure is visible in logs instead of being silently swallowed.

3. Dead branch in _build_k8s_job_names

if job_name.startswith(KubernetesClient.JOB_PREFIX):
    return job_name, job_name

JOB_NAME_FORMAT = "egg-agent-{pipeline_id}-{role}" can never produce a string starting with "egg-sandbox-", so this branch is unreachable. Either drop it or hoist _build_k8s_job_names to also accept pre-formatted names (which the spawn-time cleanup at kubernetes_spawner.py:404 could then route through). Not blocking — just defensive code that doesn't defend against anything.

---

Pre-existing issues (out of scope, flagging for awareness)

• The 63-char k8s name limit triggers truncation-with-hash inside KubernetesClient.create_container (kubernetes_client.py:242-250), but neither the spawn-time cleanup at kubernetes_spawner.py:404 nor the new restart cleanup at 1056 replicate that truncation. For very long pipeline-id + role combinations the delete_job calls would 404 against the un-truncated name and orphan the truncated Job. Not introduced by this PR, but _build_k8s_job_names would be the natural place to centralize the full mapping if you want to fix it.
• agent_worktree_id = f"{pipeline_id}-{agent_role.value}" at kubernetes_spawner.py:444 keeps underscores. Worktree paths / gateway worktree-container-ids are still underscore-bearing. Unrelated to this PR but worth a follow-up audit if you've found other id-mismatch bugs in this area.

— Authored by egg

[egg-reviewer]

Re-review

All three follow-ups from the prior review are properly addressed in 6285115. I re-verified each one against the current source rather than just trusting the commit message.

1. Split exception handlers — kubernetes_spawner.py:1059-1069 ✓

except PodNotFoundError:
    logger.debug("No existing Job found during restart (already removed)", ...)
except JobOperationError as e:
    logger.warning("Failed to delete existing Job during restart, continuing",
                   job_name=actual_k8s_job_name, error=str(e))

PodNotFoundError (subclass of KubernetesClientError) is the genuine 404-from-k8s case and now logs at debug. JobOperationError (the catch-all for delete_job's non-404 failures, see kubernetes_client.py:705-709) now logs at warning with the underlying error preserved. Real k8s API failures are no longer hidden behind an "already removed" message.

2. Dead branch removed — kubernetes_spawner.py:258-262 ✓

job_name = cls.JOB_NAME_FORMAT.format(pipeline_id=pipeline_id, role=...)
return job_name, f"{KubernetesClient.JOB_PREFIX}{job_name}"

JOB_NAME_FORMAT = "egg-agent-{...}" and JOB_PREFIX = "egg-sandbox-" are disjoint, so the prior if job_name.startswith(JOB_PREFIX): return job_name, job_name branch was unreachable. Both call sites (spawn_agent_job at line 398 and restart_agent_job at line 1037) only ever pass (pipeline_id, agent_role), so removing it is safe.

3. Test now exercises the failure path — test_restart_agent.py:196-216 ✓

mock_docker_client.delete_job.side_effect = [JobOperationError("api timeout"), None]
...
assert mock_docker_client.delete_job.call_args_list[0].args[0] == (
    "egg-sandbox-egg-agent-issue-100-coder"
)

Two-element side_effect: first call (the new restart-side delete_job at kubernetes_spawner.py:1054) raises JobOperationError, second call (spawn_agent_job's own pre-spawn cleanup at kubernetes_spawner.py:402) returns None. The assertion confirms the failing call was made against the prefixed k8s name (egg-sandbox-egg-agent-issue-100-coder), which is exactly the underscore-role bug regressed in #2070. The test now actually verifies the new exception handler instead of being a no-op.

I ran pytest orchestrator/tests/test_restart_agent.py and all 41 cases pass.

Nit (non-blocking)

test_restart_agent.py:200-201 says "line ~404" for the in-spawn cleanup delete_job — the actual call is at line 402 (the logger.info is at 403-406). Off by 2; not worth a respin.

The pre-existing observations from my earlier review (k8s 63-char truncation in kubernetes_client.py:242-250 not mirrored in cleanup paths; agent_worktree_id keeping underscores at kubernetes_spawner.py:442) remain out of scope for this PR.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.