feat(agents): ApiAgent on run_tool_loop; skills⊥MCP; no env-smuggling (Phase 3 PR2)

[pradeepvrd]

Scope (Phase 3 PR2)

Reworks open PR #9 — the devops_bench/agents/api/ package — onto the Wave 1 foundations:

• the AgentHarness template / AgentConfig / AgentResult landed by Wave 1 PR1 (feat(agents): AgentHarness base + CLI agents (Phase 3 PR1) #13)
• the shared models/loop.run_tool_loop primitive landed by Wave 1 PR (feat(models): run_tool_loop shared turn-loop primitive (Phase 1, D1) #11)

Agents and chaos are now true siblings: both depend on models, neither on the other. No agent code reaches around the typed seam any more.

Conventions adhered to

Convention (docs/refactor/CONVENTIONS.md)	How this PR honors it
§3 AgentResult / ToolCall shapes (dataclass)	_execute returns an AgentResult; trajectory is a list of ToolCall.to_dict() entries
§5 neutral message contract	Tool calls and results flow through run_tool_loop unchanged
§6 run_tool_loop (caller-formats-tools)	Agent calls client.format_tools(...) before invoking the loop; the loop sees pre-formatted descriptors
§7 env rules — no env-smuggling	No BENCH_USE_MCP read; no os.environ; capability/MCP on-off comes from AgentConfig (target gates MCP, skills_paths gates skills); get_model(provider, model) called with EXPLICIT args
§8 light __init__ / lazy imports	import devops_bench.agents.api pulls no mcp/deepeval/SDK (asserted in two import-hygiene tests)
§9 test bar	ruff clean; 464 pytest GREEN (421 baseline + 43 new); Google-style docstrings throughout

Key implementation points

• ApiAgent (agents/api/agent.py): subclasses AgentHarness, overrides _execute. Builds the LLM client via get_model(config.provider, config.model) with EXPLICIT args, opens an MCP session when config.target is set, discovers local skills when config.skills_paths is set, format tools, then drives run_tool_loop. AgentResult.output = LoopResult.final_text, latency / tokens / tools_used propagate through unchanged (semantically byte-identical to the OLD api loop — same usage_metadata/usage extraction, same per-generate_content latency accounting that run_tool_loop already preserves).
• fold_trajectory: collapses LoopResult.contents into canonical ToolCall entries — each assistant tool_calls[*] is paired with its matching {role:tool, tool_call_id, ...} result by id. Unmatched calls survive as status="called" (no silent drop); dispatcher Error: ... strings surface as status="error".
• Skills ⊥ MCP: agents/api/skills.py is a pure stdlib discovery module (no MCP coupling, no heavy imports). The dispatch closure routes skill names to a local file read; MCP-only tool names hit the MCP session.
• Dispatch error handling: each tool call is wrapped in its own try/except inside _build_dispatch. A tool failure lands on AgentResult.errors and the trajectory (status="error"), but the loop keeps going — one failing tool never aborts the benchmark. (run_tool_loop itself propagates dispatch errors by design — the agent's wrapper is what makes that benchmark-safe.)
• No legacy surface: ApiAgent.run(prompt) now takes only (self, prompt) — context / system_instruction are gone. The _SKILLS_DIR = "third_party/gke-mcp/skills" constant is deleted. BENCH_USE_MCP is never read inside the agent (an AST walk in tests enforces this).
• AgentConfig gains two interim fields: skills_paths (PR3 migrates → SkillBinding.paths) and max_turns (env: AGENT_SKILLS_PATHS, AGENT_MAX_TURNS).

Tests added (tests/unit/agents/api/)

• test_agents_api_agent.py — registration under "api", fold_trajectory (pairing, error status, unmatched call, text-only-turn skip, defensive args/id handling), extract_tokens (usage_metadata + fallback to usage, missing usage → {}), ApiAgent._execute end-to-end (MCP-off path, explicit provider/model wiring, full trajectory folding with MCP on, dispatch error → errors + trajectory.error, missing-MCP graceful path, skills-without-MCP, empty MCP server → errored result, env ignored), AST walk asserting zero BENCH_USE_MCP / os.environ / env-helper imports, public surface signature check, sync _execute invariant.
• test_agents_api_skills.py — frontmatter parsing, normalization, multi-path walk, empty/missing path handling, plus a subprocess-isolated import-hygiene check on skills.py.
• test_agents_api_mcp.py — extract_tool_text joining/fallbacks, empty server_path rejected with ValueError, subprocess-isolated import-hygiene on mcp.py (the SDK does not load until __aenter__).
• test_agents_api_import_hygiene.py — subprocess-isolated assertion that both import devops_bench.agents.api and import devops_bench.agents.api.agent pull no mcp/deepeval/SDK at module top.

Existing tests/unit/agents/test_agents_config.py is extended to cover the two new fields.

Reviewer call-outs

1. fold_trajectory is the load-bearing extraction. Verify the assistant↔tool pairing by tool_call_id matches what the model adapters actually produce. The dispatcher emits "Error: ..." strings as the tool result text, and the folder detects that prefix to mark the entry status="error" — this is the single coupling between dispatcher and folder.
2. AgentResult.errors semantics — dispatch failures are recorded both on errors and as a status="error" trajectory entry. The model still sees the error text via Error: ... as the tool result, so it can react on the next turn. Confirm this matches what metrics/pipeline.py expects.
3. MCP gate = config.target (no BENCH_USE_MCP env) — the harness PR (Phase 5) will own the env→config translation. Until then, anyone driving ApiAgent directly must set config.target to enable MCP.
4. Skills gate = config.skills_paths — fully independent of MCP. The PR3 capabilities work migrates this to SkillBinding.paths.
5. max_turns lives on AgentConfig now; the agent never reads AGENT_MAX_TURNS directly. Confirm the orchestrator threads it through.
6. Static AST check for env-smuggling — the test walks agents/api/*.py for os.environ / get_env / BENCH_USE_MCP accessor patterns. Worth confirming the false-positive avoidance (it allows the names in docstrings but rejects them on any line that mentions environ / getenv / get_env).

Stack

This PR is stacked on refactor/integration (Wave 1 merged) and is the agents-side input to Wave 2. The chaos refactor (rework #7) runs in parallel and consumes the same run_tool_loop primitive.