Repository navigation
Add a harness contract, so the engines are not tied to one agent - #33
Open
volen-silo wants to merge 2 commits into
Open
volen-silo wants to merge 2 commits into
volen-silo wants to merge 2 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>
Collaborator
Author
|
@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
Collaborator
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.