Repository navigation
Add a harness contract, so the engines are not tied to one agent - #33
volen-silo wants to merge 3 commits into
Conversation
The host engine assumed one agent. Its command line, environment, skills directory and "is it installed" check were all written for the `claude` CLI inside the solver, so adding another agent meant copying the solver. Add `skillscope.harness`: a small `Harness` contract (binary, skills_dir, command, env, normalize, require), a registry, and `ClaudeCode`, which is the command line and environment the engine already used, moved out unchanged. `normalize` translates a harness's own events into the stream-json subset the engines already read, so stop rules, budgets, scorers and the judge are untouched and written once for every harness. `no_sandbox.harness_solver(harness, ...)` is the old solver, generalised. `claude_code_no_sandbox` keeps its signature and calls it with Claude Code, so every caller behaves as before. `_stream_until` gains an optional `normalize` so the stop rule sees canonical events only, and `install_skill` takes the harness's skills directory. Nothing user-visible changes: no new flag, and the 505 existing tests pass unmodified, including the one that drives the real solver against a stub `claude`. Choosing a harness from the command line, and moving the preflight, credential check, spend flags, CI model pin and judge behind the contract, are left for the change that adds a second harness; docs/harnesses.md lists them. New tests: the registry; a golden check that the Claude Code command and environment equal what the engine built inline; a contract test that runs once per registered harness against recorded output in tests/fixtures/harness and fails when a harness ships none; and a harness whose events look nothing like Claude Code's, driven both through `_stream_until` and through the solver under inspect. Each was checked by breaking the code it covers and seeing it fail. Signed-off-by: Eugene Volen <eugene.volen@amd.com>
`Harness.normalize` translates each event on its own. That is not enough for an agent whose closing event does not repeat the answer: GitHub Copilot CLI's final `result` record carries only an exit code and usage, and the answer is the last assistant message, printed several events earlier. A translator for such a harness has to remember it. Add `Harness.normalizer()`, which returns a translator for one run and may keep state. The default returns the stateless `normalize`, so Claude Code is unchanged. The engine asks for a fresh one per case, so nothing remembered in one case reaches the next, and the state lives in the returned callable rather than on the harness instance, which every concurrent case shares. The test harness now behaves like that: its closing event carries no answer. New tests check that each translator has its own memory, that two runs translated at once do not share it (for every registered harness, from the recorded fixtures), and that a case's answer does not leak into the next one when the solver runs two cases in a row under inspect. Breaking the code two ways -- sharing one translator across cases, and bypassing it on the non-streaming path -- makes those tests fail. Signed-off-by: Eugene Volen <eugene.volen@amd.com>
|
@danielholanda this is PR 1 of the plan in #4: the harness contract and the Claude Code adapter, with no CLI or report change. Could you review it when you have time? #34 (Copilot CLI as the second harness) is a draft stacked on it. |
danielholanda
left a comment
There was a problem hiding this comment.
I can confirm I'm aligned in terms of direction of extending the harnesses supported by skillscope. Just please make sure that folks that are using our reusable workflow only un claude code tests by default, as increasing the default test matrix would generate a huge amount of additional compute that is not feasible for many repos at this point.
@rominf / @johnl-amd would love if you folks could review this PR
There was a problem hiding this comment.
Thanks for this. The direction is right, and the contract is very close to what we need: command, env, skills_dir, prompt_on_stdin, normalize / normalizer() into the stream-json subset, a harness package with no inspect_ai import, and a contract test that runs on recorded fixtures for every registered harness. I'd like to keep all of that.
I'm requesting changes for one reason. As written, Harness only works on the host, and the plan on #4 adds sandbox legs for other agents through inspect_swe. That would make every agent exist twice in this repo: once as a Harness for the host, and once as an inspect_swe agent with its own command line, environment, skill staging and event handling. Claude Code already exists twice (claude_code_no_sandbox and inspect_swe.claude_code()), and that is where the sandbox-only workarounds come from: Inspect's cost_limit instead of --max-budget-usd, the room check that can't run because the init event is lost, and the approver-based stop. I'd rather the harness define the agent once for both places, and the sandbox only decide where the process starts.
The changes needed in this PR are small: four host assumptions in the record, plus one paragraph in the docs. Everything else below is direction for follow-ups, so we agree on it before #34 and a Codex adapter are built on top.
Requested changes
ClaudeCode.command()puts a host path in the argv. It starts withshutil.which(self.binary), which is an absolute path on the runner and wrong inside a container. Emitself.binaryand let the thing that starts the process resolve it (inline comment).ClaudeCode.env()copies the whole host environment.agent.claude_env()starts fromdict(os.environ), soPATH,HOMEand every API key on the runner would go into a container.env()should return only what the agent needs (CLAUDE_CODE_MAX_RETRIES,CLAUDE_CONFIG_DIR). The host runner can then put it on top ofos.environ, so host behaviour doesn't change (inline comment).Harness.require()checks the runner'sPATH. That's right on the host and meaningless for a container image. Either document it as the host check, or let the caller say where it's checking (inline comment).RunSpec.config_diris a hostPath. It should be a path where the agent runs, as astr, likeworkspacealready is (inline comment).docs/harnesses.md: say that the harness is used by both legs. Something like: "A harness defines the agent once. On the host the engine startscommand(spec)as a subprocess. A sandbox leg starts the same command inside the container rather than delegating to aninspect_sweagent." Please also add the sandbox leg to "What is not yet behind the contract".
The golden test (Claude's command and environment equal what the engine built inline) will need adjusting for 1 and 2. Comparing env() plus os.environ against the old inline environment should keep its intent.
Why not inspect_swe for the sandbox leg
inspect_swe.codex_cli() and friends are inspect_ai.agent.Agents, and they work well for what they're for. They don't fit the measurement skillscope makes:
- The command line is closed. Unknown flags raise
TypeError, so--max-budget-usdcan't reach the CLI (see meridianlabs-ai/inspect_swe#178, closed without a passthrough). - The raw stream stays inside the agent. It goes to an internal consumer and comes out as Inspect messages after the run. That's why the init event never reaches us, and why the early stop on that leg depends on an approver, which may drop the tool call that triggered it.
- It rewrites
$HOME/.claude/settings.json._seed_claude_configdoes this and ignores theenv=you pass. That's whyverify.require()refuses to run under thelocalsandbox. - It would be a second definition of each agent. Its own flags, binary version, skill directory and event parsing, which can drift from the host harness silently.
None of that is needed. Everything inspect_swe uses to run a CLI in a container is public in inspect_ai. sandbox().exec_remote(cmd, ExecRemoteStreamingOptions(...)) streams stdout from the container and can kill() the process; inspect_swe.claude_code() uses exactly that call. sandbox_agent_bridge (in inspect_ai.agent) provides the model proxy.
Where this is today
flowchart TB
CMD["skillscope routing / behavioral"]
CMD --> E1["--engine claude-code-no-sandbox"]
CMD --> E2["--engine claude-code"]
E1 --> H["Harness (this PR)<br/>command, env, skills_dir, normalize, require"]
H --> HC["ClaudeCode"]
H -.-> HP["Copilot (#34)"]
HC --> R1["harness_solver / _stream_until<br/>asyncio subprocess on the host<br/>streaming, early stop, room check"]
HP -.-> R1
R1 --> P1["claude on the host<br/>own credentials, --max-budget-usd"]
E2 --> SWE["inspect_swe.claude_code()<br/>its own argv, env, skill install, event consumer"]
SWE --> BR["sandbox_agent_bridge<br/>Inspect's Linux proxy binary in the container"]
SWE --> P2["claude in the container"]
SWE -.-> SWE2["planned: inspect_swe.codex_cli()<br/>a second Codex definition"]
W["only on this leg:<br/>cost_limit instead of --max-budget-usd,<br/>no init event, approver stop"] -.- SWE
Proposed: one record, two transports, optional proxy
flowchart TB
CMD["skillscope routing / behavioral<br/>--harness claude-code | copilot | codex<br/>--engine host | sandbox (later: --proxy)"]
CMD --> REC["Harness, one per CLI<br/>command, env, skills_dir, normalizer<br/>(later: proxy_env(port))"]
REC --> RUN["one runner<br/>stage skill, read events, stop rule,<br/>room check, detect_activation"]
RUN -->|host| HOST["host transport<br/>asyncio subprocess, instant kill<br/>Windows, macOS, Linux"]
RUN -->|sandbox| CONT["container transport<br/>sandbox().exec_remote(stream=True)<br/>polls every 5 s or more, Linux guest"]
HOST -->|"default"| H1["CLI's own credentials"]
HOST -->|"--proxy"| HP["host proxy, in our process"]
CONT -->|"default"| CP["sandbox_agent_bridge"]
CONT -->|"no proxy"| C1["key passed into the container"]
HP --> BR["Inspect bridge: generate_anthropic, generate_responses, ...<br/>eval model, ModelEvents in the log,<br/>message_limit and cost_limit bind"]
CP --> BR
INS["inspect_ai: Task, sandbox providers,<br/>.eval logs, scorers, judge"] --- RUN
We keep almost all of inspect_ai: tasks, samples, sandbox providers (Docker, Compose, local), exec_remote, .eval logs, time_limit, concurrency, scorers, the judge, and the bridge. inspect_swe would be dropped only after the container transport matches --engine claude-code on Claude, and the three workarounds above would go with it. Until then --engine claude-code stays as it is.
The proxy on Windows
The container proxy is only Linux because Inspect ships it as a prebuilt Linux binary. Its source, src/inspect_sandbox_tools/_agent_bridge/proxy.py in inspect_ai (MIT), is about 2,400 lines of standard-library asyncio, binds 127.0.0.1, and serves /v1/messages, /v1/responses and /v1/chat/completions. model_proxy_server(port, call_bridge_model_service_async=...) already accepts a replacement for the function that talks to Inspect, and Inspect's own tests use that hook. On the host, the replacement can call the bridge functions in inspect_ai.agent._bridge.sandbox.service directly:
sequenceDiagram
autonumber
participant R as skillscope runner
participant P as host proxy (proxy.py on 127.0.0.1)
participant B as Inspect bridge (generate_anthropic)
participant M as eval model (Inspect model API)
participant C as claude CLI (host subprocess)
R->>B: SandboxAgentBridge(state, model)
R->>P: model_proxy_server(port, call)
R->>C: start with ANTHROPIC_BASE_URL=127.0.0.1:port
loop each model turn
C->>P: POST /v1/messages
P->>B: call("generate_anthropic", json_data, headers)
B->>M: generate(), logged as a ModelEvent
M-->>B: ModelOutput
B-->>P: Anthropic JSON reply
P-->>C: streamed reply
C-->>R: stream-json event on stdout
R->>R: normalize, check stop rule
end
alt skill activated, or limit hit
R->>C: kill the process group
end
R->>P: shut down
For comparison, this is what sandbox_agent_bridge does in a container:
sequenceDiagram
autonumber
participant R as skillscope runner (host)
participant S as Inspect sandbox service (host)
participant F as request/response files<br/>/var/tmp/sandbox-services
participant P as proxy binary (container)
participant C as claude CLI (container)
R->>S: sandbox_agent_bridge(state)
S->>P: exec_remote(model_proxy)
R->>C: exec_remote(claude, ANTHROPIC_BASE_URL=localhost:13131)
loop each model turn
C->>P: POST /v1/messages
P->>F: write request file
S->>F: poll (about every 2 s), read request
S->>S: generate_anthropic, eval model
S->>F: write response file
P->>F: poll (every 0.1 s), read response
P-->>C: streamed reply
C-->>R: stdout via exec_remote (polled every 5 s or more)
end
Doing this on the host has costs:
- It uses private Inspect APIs,
SandboxAgentBridgeand thegenerate_*functions, and needsproxy.pyvendored. We'd pininspect_aiand add a round-trip test. - Our wrapper has to end the sample on limits and refusals, the way the bridge's monitor task does.
- The proxy is an open loopback port while a case runs, so it should check a per-run token.
- Each concurrent case needs its own port.
Long term, I'd rather propose a public host_agent_bridge() upstream. None of this is needed for this PR. proxy_env(port) can be added when the proxy lands.
Suggested order
- This PR, with the five changes above.
- A container transport for
harness_solverusingexec_remote, run against--engine claude-codeon Claude and compared case by case. - #34 (Copilot) lands host-only, because the container transport from step 2 doesn't exist yet. Once it does, Copilot gets a sandbox leg the same way as Claude: the CLI installed in the image and
GITHUB_TOKENpassed in. No Copilot-specific sandbox code. - Codex as a
Harness, not asinspect_swe.codex_cli(). - The optional proxy:
sandbox_agent_bridgein the container, then the host proxy.
Two things I haven't verified and would check first: whether Copilot CLI accepts a custom model endpoint (if not, it runs without the proxy), and whether claude --max-budget-usd still prices correctly when it's sending to the proxy.
The default stays Claude Code only, so the reusable workflow's compute doesn't grow, per @danielholanda's note.
|
|
||
| def command(self, spec: RunSpec) -> list[str]: | ||
| cmd = [ | ||
| shutil.which(self.binary), "-p", |
There was a problem hiding this comment.
This resolves the binary on the runner and bakes an absolute host path into the argv, so the same command can't be started inside a container. Could command() emit self.binary and let the caller resolve it? The host runner would keep shutil.which (and the not-found error), and a sandbox runner would leave it to the guest's PATH.
| return cmd | ||
|
|
||
| def env(self, spec: RunSpec) -> dict[str, str]: | ||
| env = agent.claude_env() |
There was a problem hiding this comment.
agent.claude_env() starts from dict(os.environ), so the harness's environment is the whole runner environment: PATH, HOME, and every key on the machine. That's fine on the host, but a sandbox leg would carry all of it into the container. Suggest env() returns only what Claude Code needs (CLAUDE_CODE_MAX_RETRIES=0 and CLAUDE_CONFIG_DIR), and the host runner does {**os.environ, **harness.env(spec)}. Host behaviour stays the same, and the golden test can compare that merge with what the engine used to build inline.
| # stdin is closed either way, because some CLIs wait on it. | ||
| prompt_on_stdin: bool = True | ||
|
|
||
| def require(self) -> None: |
There was a problem hiding this comment.
This checks the runner's PATH, which is the right question on the host and the wrong one for a container image. Either name it as the host check (docstring: "on the host"), or let the caller say where to look, so a sandbox leg can check the image instead of failing on a runner that doesn't have the CLI installed.
| effort: str | None = None | ||
| # A throwaway config directory, so the runner's own settings and user-level | ||
| # skills stay out of the session. `None` when the caller does not isolate. | ||
| config_dir: Path | None = None |
There was a problem hiding this comment.
workspace is a str naming a path wherever the agent runs, but config_dir is a host Path. Could it be a str with the same meaning, so a sandbox leg can pass a directory inside the container?
|
|
||
| A **harness** is the agent that is handed a prompt and a workspace and does the | ||
| work: Claude Code, GitHub Copilot CLI, Codex, and so on. It is a different | ||
| question from where the agent runs. `--engine` says *where* (on the host, or in |
There was a problem hiding this comment.
Agreed on the split between where and what. Could this page also say that the harness defines the agent for both answers to where? Something like: "On the host the engine starts command(spec) as a subprocess; a sandbox leg starts the same command inside the container rather than delegating to an inspect_swe agent." Otherwise each new harness invites a second, inspect_swe-based definition for the sandbox. Details and diagrams in the review summary.
A harness should define an agent once, for the host and for a container alike, so that a sandbox leg can start the same command instead of delegating to a second definition of the same agent. Four things in the contract assumed the host. `command()` named an absolute path found on the runner. It now emits the executable's name, and `host_command()` resolves it on this machine, so the same command can be started anywhere. `env()` copied the whole runner environment, so a container would have inherited PATH, HOME and every key on the machine. It now returns only what the harness sets, and `host_env()` puts that on top of `os.environ`, which leaves host behaviour as it was: the runner's own CLAUDE_CODE_MAX_RETRIES still overrides the default, and the isolation setting still wins over the runner's own CLAUDE_CONFIG_DIR. `RunSpec.config_dir` was a host Path and is now a string naming a path wherever the agent runs, as `workspace` already was. `require()` is documented as the host check. docs/harnesses.md now says that a harness defines the agent once for both legs, and lists the sandbox leg under what is not yet behind the contract. New tests run for every registered harness: the command names its executable and does not locate it, the environment carries only what the harness sets, and on the host that goes on top of the runner's. The Claude Code golden checks compare the host command and environment with what the engine built inline. The engine hands the harness strings, not host Paths. Each of the five changes was broken in turn and caught. Signed-off-by: Eugene Volen <eugene.volen@amd.com>
|
Thanks, this is a clear review and I agree with all five requested changes. They are in 96d63b9:
The contract tests now run for every registered harness: the command names its executable and does not locate it, On the direction: I agree with one definition per agent and with the order you propose. In particular I withdraw the idea in my RFC of a sandbox leg through #34 stays host-only until a container transport exists. I have rebased it onto this branch and it passes: the Copilot adapter needed the same two changes (a bare executable, and settings-only One thing to keep in mind for the container transport. Besides On your two open questions for Copilot: its own help mentions a bring-your-own-key provider configuration ( And per @danielholanda's note: the default stays Claude Code only, and nothing in #33 or #34 changes the reusable workflow's matrix. |
rominf
left a comment
There was a problem hiding this comment.
Thanks, all five changes are in 96d63b9, and they're done the way I'd hoped:
command()namesself.binary.host_command()resolves it withshutil.whichonly for a host run, and keeps the bare name if nothing is found, sorequire()still gives the error. The note about npm's.cmdshims on Windows is a good reason to keepshutil.whichthere.env()is settings-only:claude_settings()isCLAUDE_CODE_MAX_RETRIESplusCLAUDE_CONFIG_DIR.host_env()puts it on top ofos.environ. The golden test pins the old inline behaviour, including the runner's ownCLAUDE_CODE_MAX_RETRIESwinning and the isolation directory beating the runner'sCLAUDE_CONFIG_DIR.claude_env()is still used by the reachability probe, which also runs on the host, so that's fine.require()is documented as the host check.RunSpec.config_diris astr, andharness_solverconverts at the boundary. The new end-to-end test checks that the harness receives strings.docs/harnesses.mdnow says a harness defines the agent once for both legs, and lists the sandbox leg under what is not yet behind the contract.
The contract tests running for every registered harness are the right guard: the command names its executable, env() carries no ambient variables (including SYSTEMROOT), and the host merge keeps the harness's values. CI is green on Ubuntu and Windows, on Python 3.10 and 3.12.
One thing to note, not a change request: CLAUDE_CODE_MAX_RETRIES is the one runner variable that env() still carries into a container, because claude_settings() reads the override from os.environ. I think that's what we want, since it lets a runner raise retries on either leg, and the docstring says so.
Thanks also for withdrawing the inspect_swe sandbox leg and for rebasing #34. Your point about Copilot's other hooks (preflight, version, complete for the judge, and room, which runs copilot skill list --json) is important for the container transport. Those should go through the same transport as command, so a sandboxed Copilot run asks the image rather than the runner. I'll keep that in mind when we design it. The BYOK variables (COPILOT_PROVIDERS_CONFIG, COPILOT_PROVIDER_*) look like the way to put Copilot behind the proxy later.
Approving.
Why
Part of #4 and the harness interface asked for in #28; the design is in
this comment.
The host engine assumes one agent. Its command line, environment, skills
directory and "is it installed" check are all written for the
claudeCLI insidethe solver, so supporting another agent means copying the solver. This puts that
behind a small contract, with Claude Code as the first implementation, so a second
harness is an adapter and a set of tests rather than a change to the engines.
What changes
skillscope/harness/, with noinspect_aiimport so it is testable in theplain unit job: a
Harnessbase class (binary, skills directory,command,env,require,normalize), a registry, andClaudeCode, which is thecommand line and environment the engine already used, moved out unchanged.
no_sandbox.harness_solver(harness, ...)is the existing solver,generalised.
claude_code_no_sandboxkeeps its signature and calls it withClaude Code, so every caller behaves as before.
normalizetranslates a harness's own events into the stream-json subsetthe engines already read, so stop rules, budgets, scorers and the judge are
untouched and written once for every harness. The default is the identity,
which is all Claude Code needs.
normalizer()(second commit) returns a translator for one run that may keepstate. It exists because some CLIs do not repeat the answer in their closing
event (GitHub Copilot CLI's
resultrecord carries only an exit code and usage;the answer is the last assistant message, printed earlier). The engine asks for
a fresh one per case, so nothing remembered in one case reaches the next.
install_skilltakes the harness's skills directory, and_stream_untiltakesan optional translator. Both default to today's behaviour.
docs/harnesses.mdexplains the contract, the canonical event form, how to adda harness, and what is not yet behind the contract.
What does not change
No new flag, no CLI change, no report change. The 505 existing tests pass
unmodified, including the one that drives the real solver against a stub
claude.Verification
ruff --select F401is clean; a built wheel contains the new package.environment equal what the engine built inline, a contract test that runs
once per registered harness against recorded output in
tests/fixtures/harness/and fails when a harness ships none, and a harnesswhose events look nothing like Claude Code's, driven both through
_stream_untiland through the solver under inspect.event translation, ignoring the harness's skills directory, changing the
Claude command, always writing the prompt to stdin, sharing one translator
across cases, bypassing the translator on the non-streaming path.
stream-json shape) and says so in
expected.json. The Copilot adapter in thefollow-up PR ships real recordings.
I have only run the suite on Windows. One existing test,
TestTheHostLegActuallyKillsTheProcess.test_the_work_after_the_decision_never_happens,is flaky there and fails on a clean checkout of
maintoo (3 of 8 runs): it givesa child a 1.5 s sleep and asserts the kill took under 1.4 s. Unrelated to this
change, and not touched here.
Not in this PR
Choosing a harness from the command line, and moving the preflight, credential
check, spend flags, CI model pin and judge behind the contract, come with the
second harness (GitHub Copilot CLI), which follows as a separate PR on top of
this one.
docs/harnesses.mdlists them.