feat(agents): add the Claude Code CLI agent harness - #88
Conversation
Runs the claude binary headless (-p --output-format stream-json --verbose --dangerously-skip-permissions) and folds the wrapped SDK event stream into the canonical trajectory. Selectable as BENCH_AGENT_TYPE=claude, with a claude-code alias. Capabilities ride Claude Code's native cwd channels: rules to CLAUDE.md, skills to <cwd>/.claude/skills, and MCP bindings to a --mcp-config document pinned with --strict-mcp-config. Auth is env-driven through the shared provider contract (Anthropic key, or keyless Vertex / Bedrock), and CLAUDE_CONFIG_DIR is redirected per run so concurrent evals never race on the CLI's global state. Token usage maps onto the shared TOKEN_BUCKETS: Anthropic's input_tokens is already the uncached prompt, cache reads and writes stay separate buckets, and reasoning stays unreported because thinking is billed inside output_tokens. Aliases now canonicalize before the manifest is written, so an arm selected as claude-code aggregates with claude instead of splitting the dashboard setup. Signed-off-by: Eugene Ng <ngeugene@google.com>
…ation Review-panel findings against the new harness, each verified against the real ``claude`` binary rather than the docs: * Extended thinking is billed inside ``output_tokens`` and reported again under ``output_tokens_details.thinking_tokens``. Split it back out into ``reasoning`` so ``output`` honours the canonical contract while ``total`` stays equal to the provider's own accounting. * The per-turn usage accumulator summed ``output_tokens``, which on an assistant envelope is the ``message_start`` placeholder — a summed 3 against a terminal 3069. Drop it, so a timed-out run leaves the bucket unreported instead of persisting an invented number. * ``str.splitlines`` split on unescaped U+0085, which the CLI emits raw inside JSON strings. That shredded the event, lost its tool call, and injected decode errors that flip the run to unvalidated. Frame on ``\n``. * Tool results were retained verbatim, and the trajectory is re-serialized into the judge prompts; clip to a head+tail slice with the middle marked. * ``--strict-mcp-config`` was conditional on the harness writing its own config, so a stray ``.mcp.json`` in the workspace silently granted MCP tools to the baseline arm. Always pass it. * Shield the prompt behind ``--``; the option parser otherwise reads a leading ``-`` as a flag and aborts the run. * Clip the child stderr that reaches the persisted ``errors`` list, matching the cap the sibling ``metadata`` field already applies. Signed-off-by: Eugene Ng <ngeugene@google.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eugeneng04 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
cncf-cla: yes
|
Hi @eugeneng04. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
needs-ok-to-test
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a Claude Code CLI harness with stream-JSON parsing, provider and MCP configuration, isolated workspaces, failure recovery, canonical registry resolution, documentation updates, and unit tests. ChangesClaude Code integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeCodeAgent
participant Workspace
participant ClaudeCLI
participant StreamParser
ClaudeCodeAgent->>Workspace: materialize rules, skills, and MCP configuration
ClaudeCodeAgent->>ClaudeCLI: execute headless stream-JSON command
ClaudeCLI-->>ClaudeCodeAgent: return stdout, stderr, and exit status
ClaudeCodeAgent->>StreamParser: parse captured stdout
StreamParser-->>ClaudeCodeAgent: return text, trajectory, usage, and errors
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/unit/agents/test_agents_cli_claude_code.py (1)
49-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the fixture parameters and the remaining helper return types.
Test functions in this file annotate their return type but not their fixture parameters.
monkeypatchandtmp_pathare unannotated at Lines 742, 753, 823, 842, 856, 867, 888, 901, 912, 934, 945, 964, 984, 997, 1022, 1041, 1071, 1092, and 1107. The helpers_assistantand_userat Lines 49 and 53 have no return annotation.♻️ Proposed annotations
-def _assistant(*blocks: dict) -> dict: +def _assistant(*blocks: dict) -> dict: return {"type": "assistant", "message": {"content": list(blocks)}} -def _user(*blocks: dict) -> dict: +def _user(*blocks: dict) -> dict: return {"type": "user", "message": {"content": list(blocks)}}Import the fixture types and apply them to every test that takes a fixture:
import pytest def test_build_env_keyless_vertex_sets_switch_and_maps_project_region( monkeypatch: pytest.MonkeyPatch, ) -> None: ... def test_execute_materializes_skills_into_workspace( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: ...As per path instructions for
tests/**/*.py: "Ensure test functions have proper type annotations and clean structure."Also applies to: 742-742
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/agents/test_agents_cli_claude_code.py` around lines 49 - 54, Annotate the `_assistant` and `_user` helper return types, and add the appropriate pytest fixture imports. Update every listed test function accepting `monkeypatch` or `tmp_path` to annotate those parameters as `pytest.MonkeyPatch` and `Path`, respectively, while preserving their existing `-> None` return annotations.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devops_bench/agents/cli/claude_code/agent.py`:
- Around line 35-41: Update the module docstring’s reference to ensure it points
to an existing `docs/appendix/known_issues.md`, or add that document at the
referenced location with the keyless-Vertex `--parallel` guidance.
- Around line 273-279: Update the MCP configuration write flow in the block
assigning mcp_path to explicitly set the resulting mcp-config.json file
permissions to owner-only (0o600) immediately after write_text, ensuring
credentials in serialized server arguments are not group- or world-readable.
- Around line 138-153: Ensure the Claude Code binary used by the agent is
version 2.1.221 or newer before constructing or executing the argv in the
agent’s command flow, particularly when mcp_config_path enables --mcp-config.
Validate the AGENT_TARGET binary version and fail with a clear actionable error
if it is older, rather than allowing MCP-enabled runs to proceed.
In `@tests/unit/agents/test_agents_cli_claude_code.py`:
- Around line 753-757: Update test_build_env_vertex_region_defaults_to_global to
remove CLOUD_ML_REGION from the environment with monkeypatch.delenv before
calling _build_env, alongside the existing GCP_VERTEX_LOCATION cleanup, so the
test reliably exercises the "global" fallback.
In `@tests/unit/evalharness/test_registry_resolution.py`:
- Around line 95-105: Update both alias tests to call harness.resolve_agent(...)
before accessing AGENTS.get(...) for the canonical key. Ensure the resolved
agent is obtained first, then retrieve the canonical class from AGENTS and keep
the existing isinstance assertions.
---
Nitpick comments:
In `@tests/unit/agents/test_agents_cli_claude_code.py`:
- Around line 49-54: Annotate the `_assistant` and `_user` helper return types,
and add the appropriate pytest fixture imports. Update every listed test
function accepting `monkeypatch` or `tmp_path` to annotate those parameters as
`pytest.MonkeyPatch` and `Path`, respectively, while preserving their existing
`-> None` return annotations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: be2b5e28-2d92-4724-b7b9-f3ec58e8be7e
📒 Files selected for processing (8)
devops_bench/agents/cli/claude_code/__init__.pydevops_bench/agents/cli/claude_code/agent.pydevops_bench/agents/cli/claude_code/parsing.pydevops_bench/evalharness/default.pydocs/components/model_providers.mddocs/how-to/add-a-model-provider.mdtests/unit/agents/test_agents_cli_claude_code.pytests/unit/evalharness/test_registry_resolution.py
* Drop the module docstring's pointer to ``docs/appendix/known_issues.md``,
which does not exist in this tree — the surrounding text already explains
the per-run config-dir isolation.
* Restrict ``mcp-config.json`` to owner-only. A binding's argv can carry a
server credential and the file lands in the run workspace the harness
later collects, so the umask default (0o644 on most machines) is too open.
* Clear ``CLOUD_ML_REGION`` as well as ``GCP_VERTEX_LOCATION`` in the
default-region test; it is the second link in the same fallback chain, so
an ambient value on a developer machine or CI runner shadowed the default.
* Resolve the agent before reading ``AGENTS`` in both alias tests. A builtin
self-registers on the lazy import ``resolve_agent`` performs, so reading
the registry first passed only when an earlier test happened to import the
module — both tests failed when run alone.
* Annotate the ``monkeypatch`` / ``tmp_path`` fixture parameters, matching
the convention the rest of the suite already follows.
Not applied: gating the run on Claude Code v2.1.221 for ``--mcp-config``.
The CLI reference attaches that floor to the wait-for-pending-servers
behaviour ("The wait requires Claude Code v2.1.221 or later"), not to the
flag, so a version probe would add a subprocess call per run for nothing.
Signed-off-by: Eugene Ng <ngeugene@google.com>
|
Thanks — addressed in 8499755. Four of the five actionable items applied, one skipped with reasoning below. Applied
Not applied Gating the run on Claude Code v2.1.221 for
|
janetkuo
added
ok-to-test
…sion ``--mcp-config`` under ``-p`` only waits for still-pending MCP servers to connect before the first turn from Claude Code v2.1.221 on. An older binary accepts the flag and starts the turn regardless, so an MCP-augmented arm can run with none of its tools attached and still be scored as augmented -- the same silent contamination ``--strict-mcp-config`` prevents from the other direction, and worse than a loud failure for a benchmark. Probe ``claude --version`` and refuse the run below the floor. The probe is gated on a bound run, so a baseline arm pays nothing. An inconclusive probe (non-zero exit, unparseable output, spawn failure) proceeds: ``config.target`` may be a wrapper script with its own ``--version`` surface, and blocking on a probe that merely failed to parse would cost more than the risk it guards. Reverses the one item skipped in 8499755. The CLI reference does attach the floor to the wait rather than to the flag ("The wait requires Claude Code v2.1.221 or later"), but losing the wait is itself a correctness bug here, not a cosmetic one. Signed-off-by: Eugene Ng <ngeugene@google.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/agents/test_agents_cli_claude_code.py (2)
830-846: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd type annotations to the
fake_runtest doubles.Each nested
fake_runfunction has untypedargvandkwargsparameters. Add parameter and return annotations, or replace them with a typed reusable test helper. As per coding guidelines, "**/*.py: All Python code must include type hints."Also applies to: 849-860, 863-870, 874-892, 895-905, 908-916, 919-938, 941-949, 952-961, 971-990, 993-1003, 1006-1034, 1043-1063, 1066-1080, 1092-1112, 1114-1130, 1133-1157, 1165-1185, 1188-1200, 1203-1219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/agents/test_agents_cli_claude_code.py` around lines 830 - 846, Annotate every nested fake_run test double in the affected Claude Code agent tests, including those in test_execute_returns_typed_result_with_trajectory and the listed neighboring tests. Add appropriate types for argv, kwargs, and the returned subprocess-like result, or replace the repeated doubles with a typed reusable helper while preserving each test’s existing behavior.Source: Coding guidelines
1188-1200: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlways isolate
CLAUDE_CONFIG_DIRfor the child process.This test asserts that the subprocess inherits
/operator/claudewhen the parent environment setsCLAUDE_CONFIG_DIR. That permits concurrent evaluations to share mutable Claude configuration and makes results depend on operator state.Generate and pass a unique per-run
CLAUDE_CONFIG_DIRinextra_enveven when the parent has one. Do not modify the parent environment. Update this test to assert that the child value differs from/operator/claude.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/agents/test_agents_cli_claude_code.py` around lines 1188 - 1200, Update ClaudeCodeAgent.run and its child-process environment construction to always generate and pass a unique per-run CLAUDE_CONFIG_DIR through extra_env, overriding any parent-exported value without mutating os.environ. Revise test_execute_respects_operator_config_dir to assert the captured child value is present and differs from /operator/claude.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit/agents/test_agents_cli_claude_code.py`:
- Around line 830-846: Annotate every nested fake_run test double in the
affected Claude Code agent tests, including those in
test_execute_returns_typed_result_with_trajectory and the listed neighboring
tests. Add appropriate types for argv, kwargs, and the returned subprocess-like
result, or replace the repeated doubles with a typed reusable helper while
preserving each test’s existing behavior.
- Around line 1188-1200: Update ClaudeCodeAgent.run and its child-process
environment construction to always generate and pass a unique per-run
CLAUDE_CONFIG_DIR through extra_env, overriding any parent-exported value
without mutating os.environ. Revise test_execute_respects_operator_config_dir to
assert the captured child value is present and differs from /operator/claude.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a8ea4fd7-30be-43a9-b50b-b84bbfc68a36
📒 Files selected for processing (3)
devops_bench/agents/cli/claude_code/agent.pytests/unit/agents/test_agents_cli_claude_code.pytests/unit/evalharness/test_registry_resolution.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/evalharness/test_registry_resolution.py
- devops_bench/agents/cli/claude_code/agent.py
An ambient CLAUDE_CONFIG_DIR was honoured unconditionally as the operator's escape hatch for reusing a cached OAuth login. Under ``--parallel`` that is wrong: several benchmark processes share one host, and each would inherit the same mutable Claude config dir -- the collision class ``core.run_env`` exists to prevent for KUBECONFIG / CLOUDSDK_CONFIG / TF_DATA_DIR. The operator has already declared concurrency, so isolation outranks the login cache; the override warns rather than failing. The hatch is kept for serial runs. Always isolating would leave an OAuth-authenticated operator unable to run the bench at all without an API key, since credentials live in the config dir the fresh temp dir replaces. Also annotate the nested ``fake_run`` test doubles per AGENTS.md typing. Signed-off-by: Eugene Ng <ngeugene@google.com>
|
Both outside-diff comments addressed in 253e0ef. Type annotations on the Worth flagging for the repo rather than this PR: the convention across
Always generating a per-run dir would leave an OAuth-authenticated operator unable to run the bench at all without an API key: credentials live in the config dir that the fresh temp dir replaces. That's the entire reason the escape hatch exists, so removing it trades one breakage for another. What actually makes the hatch dangerous is concurrency, and this bench's concurrency is explicit and opt-in — several benchmark processes on one host under
The parent environment is never modified in either path; the per-run value goes through Verified in both modes: serial yields
|
2 participants
Adds a
claudeCLI agent harness that drives theclaudebinary in headlessmode and folds its event stream into the canonical trajectory and token
buckets. Ported from gke-labs/devops-bench#199 and rewired onto the shared
token schema that landed here since.
What it does
Runs
claude -p --output-format stream-json --verboseand parses the wrappedSDK event stream on stdout — no session-file reads off disk. Capabilities go
through Claude Code's native cwd channels, written into the per-run working
directory:
CLAUDE.mdin the cwd<cwd>/.claude/skills/<name>/SKILL.md<cwd>/.claude/mcp-config.jsonvia--mcp-configAuth is env-driven through the shared provider contract:
config.api_keyontothe provider's key env var(s), or keyless Vertex / Bedrock via ADC / AWS
credentials.
CLAUDE_CONFIG_DIRis redirected to a fresh per-run temp dir sothe CLI's mutable global state never races across concurrent evals.
Token buckets
The upstream PR carried a harness-local bucket tuple pending the shared schema.
This version maps the Anthropic
usageblock straight ontoTOKEN_BUCKETS,with cache reads and cache writes kept in separate buckets (writes bill at a
premium):
input_tokensinputcache_read_input_tokenscachedcache_creation_input_tokenscache_writeoutput_tokens_details.thinking_tokensreasoningoutput_tokensless the aboveoutputExtended thinking is billed inside
output_tokensand counted again underoutput_tokens_details, so it is subtracted back out —outputexcludesreasoningper the contract, andtotalstill equals the provider's ownaccounting. An unreported bucket stays
Nonerather than a fabricated0. Across-layer test asserts the emitted keys survive
normalize_tokensonto thedashboard row.
Alias canonicalization
claude-codejoinsgemini-clias a friendly alias. Both the registry lookupand the manifest write now go through one
_canonical_agent_typehelper, so anarm selected by alias aggregates under the same
harness/setup_idas thecanonical key instead of splitting into a second dashboard setup.
Review fixes
A review panel over the port turned up seven issues, each reproduced against
the real
claudebinary (2.1.228) before fixing, and fixed in the secondcommit:
output. The port assumed Anthropic reportsno separate thinking count. A live run returns
"output_tokens":3069,"output_tokens_details":{"thinking_tokens":2778}, andthe thinking count is included in
output_tokens.output~1000x too low. Per-turnusage.output_tokenson an assistant envelope is themessage_startplaceholder — a summed 3 against a terminal 3069. It only fires on the
timed-out path, so every timed-out run recorded a plausible-looking wrong
number. The bucket is now left unreported; the prompt-side fields do
accumulate faithfully and are kept.
str.splitlinesshredded events containing U+0085. The CLI escapes U+2028but leaves NEL raw inside JSON strings, which real command output carries when
a log is mis-decoded as latin-1. On a reproduced stream that cost 2 of 3 tool
calls and injected 6 bogus errors — enough to flip
validatedtoFalseanddrop the run off the leaderboard. Framing now keys on
\nalone.Read/Bash result (measured ~20 KB per call, one Read at 58 KB), and the whole
trace is
json.dumps'd into two LLM judge prompts with nothing truncating inbetween. Clipped to a head+tail slice with the middle marked elided.
--strict-mcp-configskipped on the baseline arm. It was only passedalongside
--mcp-config. Verified: without it a stray.mcp.jsonin theworkspace loads; with it,
mcp_serversis empty. That silently granted MCPtools to the un-augmented arm, contaminating the comparison the bench exists
to measure. Now unconditional.
--before the prompt. A prompt whose first token starts with-wasread as a flag and aborted the run with rc=1 and no output.
metadata["stderr"]wasclipped at 2000 chars while the same stderr went unbounded into
errors,which is what reaches disk. Both paths now share one cap.
Also verified and deliberately left alone:
CLOUD_ML_REGIONis the correctVertex region variable for this CLI (
ANTHROPIC_VERTEX_LOCATIONdoes notappear in the binary at all).
Testing
ruff check/ruff formatclean..mcp.json: clean run, one matched tool call, stray server suppressed, andtotalequal to the sum of the reported buckets.Summary by CodeRabbit
New Features
claude-codealias to the canonical Claude harness.Documentation
Tests