Repository navigation
Rebuild the eval engine on inspect_ai and retire the legacy runner - #19
Conversation
The default sandbox has no network, which I had been treating as a reason those skills cannot be containerised. It is not: it is a default, and `sandbox:` in machine.yml exists so a skill can say otherwise. That key has been unit-tested and never used for real, so this is its first outing. `local-ai-app-integration` installs a runtime and fetches models. Its compose file is inspect's own default minus the line that removes the network, with the image fully qualified because podman will not guess a registry. Note this cannot merge ahead of amd/skillscope#19: releases before it reject `sandbox:` as an unknown key outright.
rominf
changed the title
johnl-amd
left a comment
johnl-amd
left a comment
There was a problem hiding this comment.
Thanks for this. The work is careful, and the description is honest about its own weak points. That made the review easier.
I installed the branch and ran the routing leg with a stub claude on the PATH. The stub records each call and then exits, so no tokens were used. The fixture is one skill with five cases. Three defects follow. All three are from runs, not from reading.
1. Routing ignores the engine, but the report claims a sandbox
cmd_routing tests for inspect only. claude-cli and claude-code go to the legacy path and run the CLI on the host. But _finish_routing sends each engine that is not legacy to _sandbox_meta, which returns the default provider name.
Same command, three engines, same fixture:
--engine |
calls to the stub CLI | report meta |
|---|---|---|
legacy |
11 | sandbox: host, sandbox_isolated: false |
claude-cli |
11 | sandbox: docker, sandbox_isolated: true |
claude-code |
11 | sandbox: docker, sandbox_isolated: true |
The three runs did the same work on the host. The last two reports say the opposite. No container starts, because the inspect code never runs. provider() examines nothing. It returns the default string.
This is the one claim the PR asks a reader to trust, so I think it must be correct before merge. Either refuse these two engines in cmd_routing, or report the engine that ran.
2. The preflight tests the wrong credential, and asks for an unused package
- With
--engine legacy, the preflight calls the CLI. The stub log shows-p --output-format json. The run then stops witherror: claude API not reachable. - With
--engine claude-cli, the stub log is empty. The preflight tests the inspect model provider instead. The run does not use that provider. - With no inspect extra installed, the same command stops with
error: --engine claude-cli needs the inspect extra. That run needs no inspect code at all.
So the routing leg asks for a package it does not use, tests a credential it does not use, and skips the credential it does use.
3. The new preflight can hang
engine/models.check_reachable sets no timeout and does not use deadline.cap. In inspect-ai 0.3.266 the default of GenerateConfig.max_retries is None, so a connection error causes retries without a limit. I pointed the base URL at a closed port. The call was still running after four minutes, and I stopped it.
The legacy probe prevents this on purpose. claude_env() sets CLAUDE_CODE_MAX_RETRIES=0, and the docstring gives the reason. check_api_reachable also has a 60 second limit. Please give the new probe both.
Repro for all three: put a script named claude on the PATH that logs its arguments, reads stdin, and exits 1. Then run skillscope routing with --skip-preflight --model mockllm/model for each engine, and compare meta. I can attach the fixture.
Questions about the environment
The container is a clean machine. A developer machine is not clean. Your own results show both effects. local-ai-use gets 7 of 7 on the host and 4 of 7 in the container, so the container has less than the host. tracelens gets 0 of 7 on the host and 7 of 7 in the container, because the host had a bad installation. The agent found that installation and stopped to ask a question, which is possibly correct behavior.
- When the host and the container disagree, which result is true? Which one controls the gate?
- A skill for real hardware needs the hardware. Is one compose file for each skill the plan? The PR adds the
sandbox:key but includes no example, and the two GPU skills still give no result with any engine. An example with a device and a network would show that the path works. - The
inspectagent has five tools. The real harness has more. If a skill refers to a tool that is not present, the case fails for the wrong reason. Is--engine inspectintended to gate, or to diagnose? - Routing with
--engine inspectoffers each skill as a tool definition and takes one turn. The real harness finds a skill by a different method, and you say the numbers are not interchangeable. Can routing keep the legacy engine until aclaude-coderouting leg exists? - The Windows result moved from 21 of 21 to 15 of 21 with no change. If the variation between runs is that large, most of the agreement in the Linux table proves little.
tools/benchmark_engines.pyhas a--noisemode. Please use it for the main table. That one change would make the rest of the evidence much stronger.
On keeping both engines
My first opinion was that two engines is too much code. I no longer think that. The two engines do different jobs. The host engine measures the machine as it is, with its dirt, which is the machine of the user. The container engine measures the skill alone, with nothing else present. Neither one replaces the other.
The difference between the two results is itself a signal. The bad installation in tracelens became visible only because two engines disagreed. One engine cannot find that class of problem.
What I did not test
The code paths above only. I did not test the sandbox with real hardware, the LLM judge, the GPU skills, or Windows.
The candidate engine has run against one skill in a container on a laptop, and against fixtures in hosted CI. It has never run here: these runners, this catalogue, this gateway. Routing first, because it starts no sandbox and so cannot be blocked by whatever runtime the runner has -- and because one model call per case makes it the cheap half. Scoped to its own branch and reporting rather than gating. It is not part of this repository's CI and deleting the branch removes it.
The default sandbox has no network, which I had been treating as a reason those skills cannot be containerised. It is not: it is a default, and `sandbox:` in machine.yml exists so a skill can say otherwise. That key has been unit-tested and never used for real, so this is its first outing. `local-ai-app-integration` installs a runtime and fetches models. Its compose file is inspect's own default minus the line that removes the network, with the image fully qualified because podman will not guess a registry. Note this cannot merge ahead of amd/skillscope#19: releases before it reject `sandbox:` as an unknown key outright.
|
Thank you for this — the three defects were all real, and the repro made them quick to confirm. The branch has moved a long way since you read it at The three defects1. Routing ignored the engine and the report claimed a sandbox. Fixed by refusing rather than relabelling: 2. The preflight tested the wrong credential and asked for an unused package. Fixed. The routing leg no longer requires the inspect extra it does not use, and each engine's preflight probes the credential that engine actually spends. 3. The new preflight could hang. Fixed: the probe is bounded by Your questions about the environmentQuestion 3 turned out to be the important one, and more literal than either of us expected. You asked what happens when a skill refers to a tool that is not present. The same thing happens with a file: So the container held a fraction of each skill while the host legs That is exactly your "fails for the wrong reason", and I would not have gone looking without the question. Question 5 — noise. Agreed, and adopted. Every routing table now carries a second Question 1 — when host and container disagree, which is true? I do not think this has one answer, and your own framing below is the one I have come round to. What I can now say is narrower: of Question 4 — can routing keep Question 2 — one compose file per skill? On keeping both enginesI think you are right, and the evidence since has strengthened your argument rather than mine. The two legs disagree for at least three distinct reasons — a clean filesystem, a prompt whose premises are absent, and plain first-call variance — and only the first is the container doing its job. A single engine cannot tell those apart; two engines disagreeing is what made each of them findable. Two further correctionsWhile re-measuring I found two bugs of my own that had made earlier versions of these tables wrong, and both are worth stating because they invalidate numbers you may have read:
One thing I have deliberately not changed, because it affects the baseline everything is compared against: both host legs stage |
rominf
changed the title
|
@johnl-amd — ready for another look when you have time. Your review is pinned at All three defects are fixed — refusal rather than relabelling on the engine/sandbox mismatch, per-engine preflight on the credential each engine actually spends, and the model probe now bounded by Your question 3 was the most valuable thing in the review, and more literal than either of us meant it. You asked what happens when a skill refers to a tool that is not present. The same thing happens with a file: Your question 5 is adopted as the standard. Every table now carries a second The biggest change since you looked: Two of my own measurement bugs invalidated numbers you may have read. The sandboxed legs never passed On keeping both engines — you were right and I have stopped arguing. The evidence since strengthened your case rather than mine. The two legs disagree for at least three distinct reasons, and only one is the container doing its job. The behavioral run makes it concrete: the container wins one case ( Two things I would particularly like your eyes on, because both are judgement calls I did not want to make alone:
Still outstanding, and not hidden: Windows has never run in CI. |
|
Two things landed after I asked for re-review, and both change what I told you, so flagging them rather than letting you read a stale summary. The hook loss is repaired, not accepted. I said Windows has run, three times. Your question 5 is the reason it's three and not one:
Four of five identical, times within seconds. The fifth at 600s, 478s, 1032s with nothing changed between them — a single run would have reported any one of three answers. The old text flagged Windows variance as uncharacterised on one 21/21 and one 15/21; it's now one case rather than a warning about the platform. Worth noting One thing I won't overstate: hook |
|
Reporting back on the Instinct run I said was in flight, because it half-failed and I said I'd say so. The control passed. The hook skill failed, and not because of the hook. The traceback contains no skillscope or hooks frames; it ends inside The Instinct pool holds no API key — it trades a GitHub OIDC token for a short-lived Anthropic one. Timeline: token minted 14:01:41, That is a real constraint on this pool, not a flake, and it is mine — the credential model I wrote for that job mints once at job start. It did not surface earlier because the pool had only ever graded one short skill. So the honest state of the hook work: exonerated, not validated. Two things came out of it worth having:
|
|
Closing the loop on the two things I left open above. The hook is validated now, not just exonerated. The 401s were mine and are fixed. The pool's federated token is minted once at job start and the job now re-mints before each skill. No authentication errors in the run. One correction to my own number: I told you the token lasted about twelve and a half minutes. It is 9m 58s. My figure was the gap between minting and the first observed 401, which lags the actual expiry — the exchange response carried This only fixes jobs whose skills are each shorter than the lifetime. A single case that runs longer still needs a refresh mid-run, which is a change to how the model client gets its credential rather than a loop tweak. Not done, and not hidden. The remaining failure is a finding about the skill's sandbox, and it is your question 3 again. The one unmet check:
That is the first time this has been visible, because until now the case never got far enough to say anything. It is a real gap in the |
The legacy engine shells out to the claude CLI with permissions bypassed on the runner, and hand-rolls workspace staging, LLM judging and routing termination around it. This adds a second engine that hands that work to inspect_ai, selected with `--engine inspect` and off by default until a benchmark shows what it changes. Its agent is harness-independent rather than the claude CLI, so a skill is graded on whether its instructions work rather than on one harness's reading of them. That also removes what blocked Windows: with no CLI in the sandbox there is no agent bridge, and the bridge is Linux-only. Both engines produce the same outcome objects, so `summarize`, `render_markdown` and the report writers are untouched. Three pieces are not thin wrappers. The tools are ours because inspect's assume a POSIX guest, which the Windows legs do not have. The judge sees artifacts rather than the agent's prose, attaches images so "did it draw a cat" is answerable, and reports whether a requirement is satisfied so a "must not" is never scored by negating a verdict. Routing is a single model call with no agent loop: the decision is a tool call, visible in the first turn, so there is no turn to pay for and kill. machine.yml gains a `sandbox` key naming a compose file, so a skill that must reach the network to pull a model can say so.
Neither engine reported its own cost. The legacy engine discards the `total_cost_usd` the CLI returns with every result, and inspect keeps usage in its own log where skillscope's report never looks. That was fine with one engine and nothing to compare it against. Both now record into `usage`, which the report meta carries, and `tools/benchmark_engines.py` runs the same dataset through both and reports per-case agreement plus what each run spent. Agreement is compared case by case rather than in aggregate: two runs can post identical accuracy while individual cases flip in both directions and cancel out. `--noise` runs the legacy engine twice so flips can be read against how much it already disagrees with itself. The spend columns are not equally trustworthy and the report says so rather than tabulating them as though they were. Legacy token counts are a floor -- assistant events omit the system prompt and cached input, and a routing case is killed before its totals arrive -- so cost is its reliable figure. inspect has a cost only when the provider supplies pricing, which a gateway does not, so wall time is often the only column comparable on both sides.
The inspect engine grades a skill with a harness-independent agent, which tests whether the skill's instructions work rather than how one product reads them. That choice raises an obvious question, so `--engine claude-code` answers it: run the real harness in the sandbox and see whether it agrees. It slots into the existing flag rather than adding a command, so the benchmark tool can diff its report against either other engine with no new machinery. Reporting only -- harness runs are nondeterministic and the harness is not what we grade, so a divergence is a question about the skill rather than a build failure. Linux only, because inspect_swe shells `bash -c` to find the CLI and the proxy it installs in the guest is a Linux binary. Also fixes how the inspect engines count model calls. They recorded one per sample while the legacy engine records one per assistant response: identical for routing, where a case is a single turn, but a large undercount for behavioral, where the agent loops. The benchmark puts the two columns side by side and names that one comparable, so it had to mean the same thing on both sides. Counted from assistant messages now.
The two legacy commands read the same `claude` stream but recorded from different events: routing counted assistant replies, behavioral counted only the result event, which is one per case. A behavioral run of three cases reported three model calls while costing $0.72 -- far more than three small calls -- and the benchmark puts that column beside the other engine's and calls it comparable. Both now go through one recorder in `usage`. The same three cases report fourteen calls against the new engine's twelve, which is the like-for- like reading: two agents looping to similar depth.
`--engine` changes what drives the agent while leaving the dataset, the CLI and the reports identical, so it belongs in usage rather than being discoverable only from --help. Says what each engine needs, why the inspect one is cheaper, why the claude-code leg never gates, and where the sandbox does and does not exist.
…config Which provider to use is a property of the runner -- docker, podman on a host that has that instead, local where there is no container -- and whoever runs the job knows it while a skill does not. What the sandbox has to provide is the opposite: the skill declares a compose file when it needs network egress or a device. `SKILLSCOPE_SANDBOX` already selected the provider but returned it bare, discarding any compose file the skill had declared. A skill that needs the network would then run without it and fail for a reason nothing in the report explains. The two are now resolved independently. podman works with no registration: `inspect-podman` publishes an `inspect_ai` entry point and inspect resolves the bare name, so installing the extra is the whole setup.
A behavioral report showed the same numbers whether the agent had been contained or had worked directly in the harness's filesystem with permissions bypassed, which invites the reader to assume the former. Both engines now record what held the run, and the markdown says it outright -- the legacy engine reports `host`, and the Windows legs, which have no sandbox available at all, no longer look like the Linux ones. Routing under the inspect engine reports `none` rather than a provider: it offers the skill tool and never calls it, so nothing is executed and there is nothing to isolate. That is not the same claim as unprotected. Also stops the gateway-versus-federation guard firing on runs that reach no provider. `--model mockllm/model` is the wiring check that costs nothing, and refusing it because the shell holds both Anthropic variables broke it on exactly the machines most likely to have an OAuth token.
A graded run on the inspect engine starts a sandbox and installs a skill before it first reaches a provider, so a bad key surfaced as a task that failed after all that work -- a 401 buried in a sample error, eighteen seconds and a container in. One tiny call up front turns that into a message on the first line. The legacy engine already had this; the inspect one skipped it on the grounds that the CLI-based probe tested the wrong thing, which was true and left nothing in its place. mockllm reaches no provider, so it is skipped rather than charged for a round trip that proves nothing.
`list_paths` returned an empty list when the listing command failed, so a sandbox that could not be listed was indistinguishable from one the agent had left empty. That is not a cosmetic difference: `files_exist` fails, and the judge builds its evidence from the same listing, so every judged expectation fails too with "no files were produced". Nine checks blame the skill for what the harness did. It now raises. The scorer reports that it could not list the sandbox, and the judge declines to rule rather than being handed an empty workspace as fact -- a judge told "no files" will confidently conclude the agent did nothing.
A container sandbox starts at `/`. So `write_file("router.json")` landed
at `/router.json`, `find . -type f` walked the whole image and died on
`/proc`, and the judge -- had the listing survived -- would have read
twenty arbitrary system files as the case's artifacts. An agent left to
guess reasonably tried `/app`, then `~`, and scattered its output.
Everything a case does now happens in `/workspace`: fixtures are seeded
there, the file tools resolve against it, shell commands run in it, and
it is what gets listed. The agent is told so, because where its output
lands should not be something the dataset has to predict.
`local` keeps the harness's own working directory -- it already has a
sensible one, and creating `/workspace` on someone's machine would not be.
inspect_swe solves the same problem the same way; its agent cwd falls
back to the home directory when the sandbox default is `/`.
Two reasons a case could not pass in a container. The judge read artifacts by the path `list_paths` reported, which is relative to the case's working directory, while `read_file` resolves against the sandbox's own -- `/`. Every artifact came back unreadable and the judge concluded, reasonably, that nothing had been produced. It now resolves them the same way the tools do. And it never saw the agent's final message, so an expectation about what the agent *told* the user -- "output the curl commands they need" -- was unanswerable however well the agent had done. The legacy judge was given both halves and this one was not. They stay labelled apart, because the distinction is what stops an agent writing "I won't call the cloud API" from settling an expectation that it avoided doing so: actions are judged from tool calls and artifacts, and only what the agent told the user is judged from its message.
The judge's transcript was cut at the end, so the agent's most recent tool calls were the first to go -- and those are usually the ones an expectation is about. An agent writes a file, validates it, and reports the result; losing the validation makes the run look like it claimed something it never did, which is precisely how the last failing check read: "only the agent's unsupported claim". Tool calls are now kept whole, since each is short and the list of them is the record of what happened. Only results are capped, individually, because they are what grows without bound. If the whole still overflows, the middle goes rather than the end.
This repository is public and the fixture named a real internal CI service account alongside the gateway's header, which together describe how model access is fronted. The test is about parsing `Key: value` lines; the values were never the point.
Every bug in this branch was found by running against a real container, and none of them would have been caught by the unit suite. `mockllm` reaches no provider, so the machinery around the model can be driven for free, with no key, on a pull request from a fork -- on Ubuntu, where Docker is the default sandbox, and on Windows, where there is none and the cross-platform tools are the only thing that works. The assertions are about the harness rather than the skill, because the mock satisfies nothing. It checks that no case errored, that the sandbox that ran is the one the platform should have chosen, and that no check failed because the sandbox could not be listed -- which otherwise reports identically to an agent that did nothing. The load-bearing one is the seeded fixture. The file exists because the case staged it, not because the agent acted, so `files_exist` passing proves the whole path: staged into the working directory, listed there, and matched against what the case asked for. That is exactly the chain that was broken in a container an hour ago.
Two failures on the first run of the new job, both worth having. The sandbox-selection tests asserted `docker` on any host, which is only true on a POSIX one -- on the Windows runners the answer is `local`, because inspect's sandbox layer assumes a POSIX guest. They pin the platform now and assert both answers, which is what they should have said in the first place. The engine job pointed at its fixture with `working-directory`, but a `--skills-dir` glob that was passed is resolved against the repo root, and `find_root` takes that from the nearest `.git`. Running from a subdirectory of a checkout still globbed the checkout, so the fixture was never found. `SKILLSCOPE_REPO` is the documented way to say which repo is under test, and is what the action already uses. The local reproduction missed this because the throwaway fixture was not inside a git repository, so the root happened to be the fixture.
CI ran the unit suite on a plain `pip install .` and every graded test failed with `ModuleNotFoundError: inspect_ai`. Not a test problem: with `legacy` gone, both remaining engines run under inspect_ai, so the default engine needs a dependency the package called optional. The extra existed because `legacy` was standard library only and a repo that had not migrated installed nothing new. That property died with the engine. Keeping the extra would ship a tool whose two graded commands both fail out of the box. `skillscope[inspect]` stays as an empty extra so downstream pins and workflows that name it keep resolving. `verify` and `podman` are still real extras: only the sandboxed leg needs inspect-swe, and only a podman host needs the provider. The install hint changes with it. It used to say `pip install 'skillscope[inspect]'`, which now points at an empty extra and would fix nothing -- reaching that error means a broken install, so it says to reinstall. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
`render_markdown` calls it and the legacy surgery deleted it. Every unit test passed, because nothing called `behavior.render_markdown` at all. It surfaced on a self-hosted MI300X: two cases graded 4/4, the agent time paid for, and then `NameError: _isolation_note` while writing the report. `continue-on-error` on that step turned the crash into a green job, so the run reported success having produced no report. The line it renders is the one whose absence reads as isolation -- a report that omits "cases ran unsandboxed" says nothing and implies the opposite -- which is the same class of mistake this PR already had to withdraw a claim over. So: render both reports in tests, sandboxed, unsandboxed, and with nothing said about containment. It is the cheapest check available and it would have caught this before the hardware time. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Retiring the legacy engine took the behavioral leg away from
serving-llms-on-instinct, the one skill in the catalogue that ships a
hook. It uses `setup` to clear stale vLLM containers and `teardown` to
remove them, on a shared MI300X runner -- so the loss landed on scarce
hardware, and the pool went from grading two skills to one.
It did not need to. inspect already has both shapes: `Task.setup` takes
a solver that runs ahead of the agent, and `Task.cleanup` takes a
per-state callable invoked inside a `finally` under a shielded cancel
scope -- so teardown still runs when the agent raises, which is the
property a container-removing hook exists for.
The old refusal fired on the file existing. It now fires on the entry
points a hook actually defines, and only for the two with no equivalent
here: `check` is handed the retired engine's `Run` object where these
engines have inspect's `TaskState`, and `setup_session` returns template
variables substituted into prompts that are built before any hook runs.
A `setup` that returns them is refused at runtime, because dropping them
would leave `{placeholder}` in the prompt the agent is graded on.
Checked before narrowing rather than assumed: both federated source
repos have no hooks at all, neither federated skill carries one, and the
single hook in the catalogue defines only setup and teardown, returns
nothing from either, and its dataset uses no placeholders. Nothing that
exists relies on what is still refused.
The docs advertised all four to skill authors, so they now say which two
are gone and what to do instead -- including the `setup_session` example
further down, which demonstrated an entry point that no longer runs.
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The exchange response carries `expires_in` and this discarded it. A graded run on an MI300X then spent twenty minutes and failed with `401 OAuth access token has expired`, forty-eight times, and nothing in the job had ever said the token had a deadline. Measured afterwards from the transcript: minted 14:01:41, first 401 at 14:14:20. About twelve and a half minutes -- a number that was in the response all along, and the only thing needed to recognise the failure on sight rather than by reading an eval log. It does not fix the expiry. A job that grades for longer than the lifetime still needs a fresh token part-way through; this is what makes that knowable before the run rather than after it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Two breakages, both reaching every downstream repo, both found by running the default engine the way a product repo would. The reusable workflow names no engine, so it takes whatever skillscope's default is -- and that default changed when legacy was retired. A fresh `pip install skillscope && skillscope routing` refused: `claude-code-no-sandbox` demanded SKILLSCOPE_SANDBOX=local while the default provider is docker. The two defaults contradicted each other. The host leg now settles on local when nobody asked, and still refuses an explicit container request, because overriding that would answer a different question than the one put. It exports the choice rather than just returning it, so the report says `local` -- naming `docker` for a run on the host filesystem is the failure this engine exists to avoid. And a runner authenticating by workload identity federation lost its routing leg. `can_isolate_config` tested for ANTHROPIC_API_KEY alone, where the question is whether auth lives in the environment and so survives a redirect to a throwaway config dir. A federated token does. Federation is what the reusable workflow offers downstream through `federation_rule_id`, holding no key at all on purpose -- it is how the Instinct pool authenticates. Every existing test set SKILLSCOPE_SANDBOX, which is why neither showed up until the engine was run the way a caller would run it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
9c980f1 to
573b934
Compare
`evals/evals.json` pairs each prompt with `skill_should_trigger` -- the routing answer -- and with the `expected_behavior` a behavioral case is graded against. The host leg copied the skill directory wholesale, so that file sat inside the room the agent was being asked to choose from. The sandboxed leg never had it, which is the part that matters for this branch: the two legs whose agreement the legacy retirement rests on were choosing from different rooms, and only one of them held the answers. No agent was ever observed opening it -- every case in a 67-case run was checked, and none touched it. That is a belief about behaviour, not a property of the room, which is why this is a removal *and* a detector. A case that opens a dataset file now reports `answer_key_read` in the column a reader is already looking at, because the routes by which a copy could reach an agent are exactly what is not known in advance: a fixture directory, a checkout a skill asked for, a path its own instructions name. Nothing depended on the file being there. Workspace fixtures under `evals/files` are read from the skill directory on disk and seeded into the workspace, which is a different path -- checked before removing, and a test now pins that the skill's own content still stages. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
|
Design looks good, and the three defects from last time are properly fixed. Eight things look like leftovers from the legacy removal:
FYI, four capabilities go away with
Also worth knowing:
Defects 1 to 6 and the room check are what I'd want before approving. 7 and 8 are cheap but not blockers. |
A review of this branch found eight defects, all of them things that stopped working when `legacy` went and nothing noticed, because no test and no CI job asserts any of them. Every one was reproduced before being fixed. Four were reports that described a run other than the one that happened. `model_calls` counted each assistant reply twice on the default engine -- once off the CLI's stream, once off the message that stream became -- so the host leg showed double the sandboxed leg's figure for identical work, in the two columns of the benchmark whose whole purpose is to be comparable. `near_limit` was structurally zero there: the caps went into `meta` only for `claude-code`, and `near_a_limit` reads a missing cap as no cap, so a case that stopped *on* its budget counted as one that chose to stop. `case_timeout` reported the argument rather than the clipped value a case was actually held to. And `--jobs` was printed in the header and passed to nothing; it is inspect's `max_samples` now, which is what the flag always claimed to be. `--max-budget-usd` reached nothing at all on `claude-code`. `inspect_swe` builds its command line from a closed set of arguments and takes no passthrough, so the value was accepted, carried as far as the solver, and dropped -- while `--help` said the CLI enforced it. inspect's own per-sample `cost_limit` is the equivalent and the task carries it now. It binds only where inspect can price the model, which it cannot do for any model it currently ships, so that is reported rather than assumed: `max_budget_can_bind` says which of the two legs actually holds a budget. Swapping a silent drop for a cap that silently cannot fire would have been the same bug one layer down. `--keep-logs` was declared, passed by both workflows, and read by nothing, so a job that asked to keep its transcripts uploaded whatever `.skillscope/logs` happened to hold -- and stayed green, which is why it survived. It chooses inspect's log directory now. One bad sample failed a whole behavioral batch. inspect marks the log `error` when any sample raises, and the mapper returned one failure per case on that condition, discarding every case already graded and paid for: one container that would not start, and a skill's whole report read as a total failure. Routing never did this, for the same reason, and the two now agree -- inspect is also told not to abandon the task, so the later cases still run. The hook contract changed in all three arguments without the signature changing. `workspace` became `None` off-container and the *string* `/workspace` on it, where the retired engine passed a real directory and asserted it was not None; `case` became a metadata dict holding neither `id` nor `prompt`, where the retired engine passed the `Case` object. The documented example calls `sources.resolve(skill, workspace)`, whose parameter is annotated `Path` -- it raised `TypeError` on both engines. `case` is the object again and `workspace` is a real per-case host directory, removed afterwards. One meaning did have to change and the docs now say so: the retired engine's workspace *was* the agent's room, and this one is scratch space for the hook, because the agent's room may be inside a container. The room check is back. `Outcome.visible_skills` and `extra_skills` were read in three places and assigned in none, so a skill that failed to install scored as `missed_trigger` -- which reads as a description that did not attract its prompt -- and a stray `~/.claude/skills` entry joining every case went unmentioned. The host leg reads both off the CLI's session-init event again. `claude-code` cannot: it drives the CLI through `inspect_swe`, which surfaces no such event. So the report says the room was not verified, rather than letting two warnings that cannot fire read as a clean room. Smaller leftovers: `benchmark_engines.py` defaulted `--baseline` to `legacy` after `legacy` was removed, which made both examples in its own docstring fail; `RoutingConfig`, `_result_error` and the retired engine's `Agent`/`claude()` driver were dead; the README still claimed standard library only with 85 packages in the tree, and two docs named `ANTHROPIC_API_KEY` where the code takes `ANTHROPIC_AUTH_TOKEN` too. `--timeout` expiring still kills the process with `os._exit`, which is the point of a watchdog but also skips the shielded scope inspect runs `Task.cleanup` in -- so the run most likely to have left a container behind is the one that gets no teardown. Cleanups can register for a bounded last attempt now, and the process says what it could not finish. Verified against a stub `claude` on a two-skill room: both room-check warnings fire, `model_calls` reads 15 for 15 single-reply cases rather than 30, the caps appear in `meta` on the host leg, and `--keep-logs` writes the transcript where it was told to. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
`sandbox_cost_limit` explains why the sandboxed leg cannot hand the CLI `--max-budget-usd` and uses inspect's `cost_limit` instead. The reason is a gap in someone else's API, so it now names the issue tracking it -- meridianlabs-ai/inspect_swe#178 -- rather than describing a constraint with no way to find out whether it still holds. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
|
All eight are fixed, plus the room check. Head is 1. 2. 3. 4. Hook signature. Worse than it looked: the retired engine passed the 5. One bad sample failing the batch. 6. 7. 8. Dead code. The room check. Back on the host leg, read off the Also fixed from your "worth knowing" list: the Not fixed, and not hidden. The Verification. One run, stub
Both room-check warnings fired. 42 new tests, 474 total, green on Linux and Windows. |
Two CLI-level controls the retired engine passed, neither of which survived the move intact. `--max-budget-usd` still binds on the host leg, which hands it to `claude` exactly as legacy did. It cannot on `claude-code`: `inspect_swe` builds its command line from a closed set and takes no passthrough, so that leg uses inspect's per-sample `cost_limit`, and inspect computes a cost only when its registry knows the rate. It ships none -- 796 entries across ten provider files, not one with a price -- so the cap was reported as unenforceable and was. `SKILLSCOPE_MODEL_PRICING` is the way out, and it is opt-in deliberately. skillscope ships no price list: rates change without notice and differ by contract, and a stale table would hold a run to a number nobody agreed while the report said the budget was enforced. That is worse than no cap, and the same shape as the defects this engine has already been corrected for. The operator supplies a file (or the JSON itself), and `meta.max_budget_pricing` names the source beside `max_budget_can_bind`, so a cap backed by someone's own figures is distinguishable from one that cannot fire. A malformed table fails the preflight rather than one sample into a graded run. A file rather than a URL on purpose. Fetching rates would mean holding some catalogue's credentials and reaching the network inside a preflight, and would tie this tool to one deployment's API. Whoever knows where their rates live can produce the file in one command. `--no-session-persistence` was simply lost. Legacy passed it because a routing run is dozens of throwaway sessions; the host leg stopped, and `no_sandbox` went on naming it in a docstring as something `extra_flags` carries while nothing supplied it. Both flags now come from one `--help` probe instead of two, and the report keys off the budget flag itself rather than the list being non-empty -- otherwise a run that passed no budget would have claimed an enforced one the moment the session flag joined the list. Verified against a real rate card: 55 models registered, `cost_limit_binds` flips, and the host leg's stub shows `--no-session-persistence` on all 15 cases with `--max-budget-usd` still present -- and absent from `meta` when the budget is disabled. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
johnl-amd
left a comment
johnl-amd
left a comment
There was a problem hiding this comment.
Seven of the nine are properly fixed, and I checked those by running them rather than reading. budget_for being the same function on both the reporting and the enforcement side is the right shape, those two physically can't drift now. And the room check note that says "could not look" instead of going quiet is exactly what I was after.
Four things need another pass. The first one is the serious one.
1. --engine claude-code crashes on default arguments
--max-budget-usd defaults to 0.75 and sandbox_cost_limit hands that to the task regardless (engine/routing.py:1045), on the reasoning that a limit which can't fire is inert. It isn't inert, inspect refuses the whole run:
PrerequisiteError: cost_limit requires cost data for all models.
Missing cost data for: anthropic/claude-opus-5.
That's before any sample starts, no API call, and nothing in cli.main catches it. So the default engine on default flags won't start unless someone sets SKILLSCOPE_MODEL_PRICING first.
I'd guess it slipped through because your verification had a rate card registered, so the configured path works and the bare one is the broken one. CI never runs a graded leg, so it stays green. Looks like a one line fix, return None when cost_limit_binds is False, which also makes the behaviour match what max_budget_can_bind already reports. "inert" appears in pricing.py:18, engine/routing.py:1045, docs/usage.md:74 and your comment above, so all four want the same edit.
2. The deadline cleanup isn't wired to anything, and the bound isn't a bound
deadline.on_expire() has no callers outside the tests. Nothing registers container teardown, so a run that hits the wall clock still tears down nothing. The original finding stands.
Separately, the 10s budget only gets checked between callbacks, never inside one. A 120s cleanup against a 10s budget had to be killed at 45s and the second callback never ran. A cleanup that shells out to remove a container is exactly the blocking case, so this turns a bounded command into an unbounded one, which is worse than the os._exit it replaced.
3. pricing.py accepts rates that quietly kill the cap
input=NaN -> registered, can_bind=True
input=-5.0 -> registered, can_bind=True
input=true -> registered, can_bind=True
input=Infinity -> registered, can_bind=True
float() is happy with all of those and json.loads takes bare NaN and Infinity. nan >= limit is always False so the cap never fires, while the report says it binds. That's the exact failure the module docstring names as its reason for existing. A finiteness and sign check in _rates closes it.
4. Dead code sweep is about half done
agent.py is 412 lines, roughly 223 unreachable. Run, Check, _grade_with_llm and _list_workspace_files have no callers outside the file, the one hit in judge.py:12 is prose inside a docstring. The module docstring also still demos the function you removed:
ImportError: cannot import name 'claude' from 'skillscope.agent'
Same defect as number 7 last round, in the file you were editing at the time. ruff check --select F401 also finds 12 unused imports. Full linter is fair to defer, but that one rule on changed files would have caught both.
Tests don't hold these fixes down
Reverted a couple in a scratch copy and reran:
| reverted | suite |
|---|---|
the _record_room(...) call site, exact original bug |
490 passed |
max_samples=jobs back to None |
490 passed |
Nothing drives cost_limit through a real eval(), which is why 1 got through. The getsource guards catch the line being deleted, not the behaviour going away.
Four from review, and the first is mine from last round.
**An unpriced `cost_limit` is not inert.** I passed one to the task on the
reasoning that a limit which cannot fire is harmless. inspect validates it
before the first sample and refuses the whole eval:
PrerequisiteError: cost_limit requires cost data for all models.
So `--engine claude-code` would not start on its default arguments unless
`SKILLSCOPE_MODEL_PRICING` was already set. It survived because every run
I made of it had a rate card registered -- I verified the configured path
and never the bare one -- and CI grades nothing. The limit is now passed
exactly when `cost_limit_binds` says it can bind, which is the same
question `meta.max_budget_can_bind` already answered, so the behaviour and
the report cannot drift. The "inert" claim is corrected everywhere it was
written.
**The deadline cleanup now happens earlier, where it can work.** The
registry of last-resort callbacks was wired to nothing, and its budget was
checked only between callbacks, so one slow cleanup ran as long as it
liked -- 6s against a 2s budget, measured. A bound that does not bind is
worse than the hard exit it was softening.
The real fix is a stage before the wall: `GRACEFUL_RESERVE_S` early, the
watchdog raises KeyboardInterrupt in the main thread, inspect cancels its
samples and unwinds *through* `Task.cleanup` rather than around it, and
`cli.main` reports the overrun instead of a traceback. That is the same
reserve `behavioral.TIMEOUT_RESERVE_S` already holds back, for the same
reason. The hard stage stays as the backstop, its callbacks now joined with
the remaining budget, and hook scratch directories actually register one.
**A rate can no longer disable the cap it configures.** `float()` is not a
check: it takes `True`, and `json.loads` takes bare `NaN` and `Infinity`.
`input: NaN` registered and reported `can_bind: true` while every
comparison against it was false -- precisely the failure the module exists
to prevent, arriving through the module. Non-finite, negative and boolean
rates are refused by name.
**The rest of the dead code.** `Run`, `Check`, `_grade_with_llm` and
`_list_workspace_files` had no callers; the module docstring still demoed
`claude()`, removed last round, so anyone following it got an ImportError.
`ruff --select F401` found twelve unused imports and now runs in CI as its
own job -- one rule rather than a linter, because that one rule would have
caught both halves of this and reformats nothing.
**And the tests now hold these down.** The review showed two fixes could be
reverted with the suite still green: `getsource` guards catch a line being
deleted, not the behaviour going away. The cost limit is driven through a
real `eval()` (which is what would have caught the first defect), the room
recorder and `max_samples` are asserted by running them, and the cleanup
bound is timed rather than assumed. Reverting any of the five fixes now
fails the suite; `mockllm/model` keeps all of it credential-free.
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
|
All four fixed at 1. The cost limit crash — mine, and your diagnosis of how it got through is exact. Reproduced before fixing:
One correction to the scope, not in my favour but worth being precise: it's 2. The deadline — you're right that the original finding stood, and the bound wasn't one. Measured your case: a 6s callback against a 2s budget took the full 6s. Checking the clock between callbacks is not a bound, and the blocking case is exactly the one that overruns. Rather than make the last-resort registry better, I moved the fix earlier, which is the only place it can actually work. The hard stage stays as the backstop, its callbacks now each joined with the remaining budget, and 3. Rates that kill the cap. All four refused by name — non-finite, negative, and boolean ( 4. Dead code. Worth noting what I didn't remove: The tests. This was the real finding. Both your reverts passed because
The cost limit is driven through a real |
All four findings are fixed at 9967cb5, which is past the c3bbbda this review was submitted against. Dismissing so the stale CHANGES_REQUESTED does not sit on a head it no longer describes — not to settle anything. Re-requesting review; details in the reply above, including the revert table showing each fix now fails the suite when undone.
|
Three of the four are properly fixed, and I checked those by running them rather than reading. I reproduced every row of your revert table independently, and each one fails exactly as you said. The cost limit now stays withheld when it cannot bind, and Then the dead code sweep broke the default engine. 1. Every case errors on
I drove the real solver with a stub
One line, confirmed: from .. import usage # beside the existing legacy_agent import
...
usage.record_stream_event(event) # line 502, drop the legacy_agent prefixThe part worth knowing is that the suite gives 487 passes with and without that fix. F401 cannot see 2. The graceful stage does not reach a run that is actually stuck
You always pass The second half is the reporting. When the stop does land,
3. Two of the fixes have no regression test at the wiring point Both of these leave 487 green:
Both want a test that calls the thing and asserts the effect. One Same reason 2 is invisible, by the way: 1 is blocking. 2 and 3 are what I would want before approving, 3 especially since this is the second round where the guard turned out to be textual. Everything else I had is small and can wait. |
…t kept Three from review, and the first is mine from last round. **Every case on `claude-code-no-sandbox` errored.** The dead-code sweep dropped `usage` from `agent.py`'s imports, and the host leg's solver was reaching it through there: `legacy_agent.usage.record_stream_event` raised AttributeError on the first event of every case, on both graded commands. F401 cannot see an attribute reached through another module, and nothing in the suite ran that solver -- the sandbox job uses a stub solver and every other test called a piece of it directly -- so all 487 tests passed. It imports `usage` itself now. A scan of every module-attribute access in the package finds no other dangling reference. The test that would have caught it now exists: `engine.routing.run` on the default engine, end to end, against a stub `claude` on PATH, asserting the case is graded, the room is recorded and the tokens are counted. **The graceful stop now reaches a run that is actually stuck, and keeps what it finished.** `_thread.interrupt_main()` sets a flag; it does not wake a main thread parked in `epoll_wait`. Through a real `eval()` it happened to land within a second here, because something in inspect wakes the loop -- but on a bare loop it waited out the full sleep, and the stop should not depend on an inspect internal. It is a real SIGINT to the main thread now. Windows has no `pthread_kill` and `os.kill` there terminates, so it keeps `interrupt_main`. When the stop lands, inspect runs `Task.cleanup`, writes the log as `cancelled` with the finished samples in it, and returns `[]`. So a run that overran dropped every case it had graded, and the gate -- which only checked `expired()`, still false 120s before the wall -- blamed "every case errored". `run_eval` now reads that log back for all three `eval()` call sites, including when the interrupt escapes `eval()` as KeyboardInterrupt or, on 3.10, CancelledError. Only this call's log: the default directory is shared, and routing's task is always `routing`, so a name or a time window would take a parity run's log beside it for this one. Each call tags its eval with a token and reads back what carries it. A stop that left no log fails the cases rather than dropping them, no new skill starts after one, and the gate reports the overrun. **The guards that were textual are behavioural.** `build_task(...)` is built and its `cost_limit` read; the room recorder is reached by running the solver; the stop is delivered for real, in a subprocess, to a bare parked loop and to an `eval()` with five finished cases and one hung. Eighteen reverts, each checked in a scratch copy, and each fails the suite. The one line nothing holds down is the listing that skips files older than the call: it only saves reading their headers, and the token rejects them anyway. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Every readback failed on Windows with FileNotFoundError on a path that began `\Users\...`: the drive letter was gone. `list_eval_logs` names a local log `file://C:/...`, one slash short of a file URI, so `C:` parses as the host and `read_eval_log` drops it. The names never reach the filesystem on POSIX in a way that shows this, which is why every local run passed and both Windows unit jobs failed on the five tests that read a log back. `run_eval` lists the directory itself and hands inspect `Path.as_uri()`, which is `file:///C:/...` and round-trips. The log directory is always local, so nothing is lost by not going through inspect's filesystem layer. The existing readback tests are the regression test; they are what failed on Windows. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
|
All three are fixed as of 1. The default engine — mine, and your diagnosis is exact. I reproduced it with your stub-
2. The graceful stage. You were right on both halves. I'd like to correct one detail of the first. Delivery. On a bare Windows has no Reporting. Confirmed. inspect writes the stopped run's log as
Your 3. The textual guards. You were right. This is the second round of it, so this time they are gone rather than patched:
That's sixteen reverts. The suite passes locally on 3.10, 3.12 and 3.13, and in CI on all four ubuntu/windows legs. |
johnl-amd
left a comment
johnl-amd
left a comment
There was a problem hiding this comment.
Re-reviewed at ff33db4f5. All three from last round are fixed, and I checked each by running it rather than reading.
1. The usage reach-through. Fixed with the one line import. I drove the real solver with a stub claude on PATH: case graded, room recorded, tokens counted, no AttributeError. The gap behind it is closed too. TestTheDefaultEngineRunsACase runs engine.routing.run end to end on the default engine, and putting legacy_agent.usage back fails 3 of its 4 tests with the exact original error.
2. Stop delivery, and what a stopped run reports. signal.pthread_kill aimed at the main thread is a better fix than the os.kill I suggested, since it cannot land on the watchdog thread instead. Measured against a bare parked loop: delivered at 1.0s, where reverting to interrupt_main alone waits out the full sleep, 5.0s of a 5s sleep. The reporting half works as well. run_eval reads the cancelled log back by call token, and a run stopped with five of six samples finished returns all five. _fail_if_expired reports the overrun now instead of blaming the cases, and on_expire finally has a real caller.
3. The guards are behavioural. This is the part I most wanted, and it holds. I reran the experiment that embarrassed both of us last round, breaking each fix semantically with the source text left in place rather than deleting the line:
| break applied | suite |
|---|---|
cost_limit= to None at the call site |
FAILED, None != 0.75 |
_record_room(...) made unreachable with if False: |
FAILED, [] != ['alpha'] |
_interrupt_main body made a no op |
FAILED, "the loop was never interrupted" |
505 green unmodified, same as CI.
Approving. Three things I would not hold it for.
The last commit is right about Windows. I confirmed file://C:/... parses the drive letter as netloc and drops it, while file:///C:/... round trips. _log_files is narrower than list_eval_logs in three ways though, none of them reachable from current call sites. It does not recurse, where list_eval_logs does by default, which only matters if inspect ever shards or checkpoints. It filters on suffix alone where list_eval_logs also matches the timestamp filename pattern, so a stray .json landing in the log dir during an interrupted run raises an uncaught ValueError out of the readback rather than being skipped. And "local only" in the docstring is true by convention rather than enforced, since --keep-logs takes any string and a remote dir would silently give back an empty set. The second one is worth a line of hardening if you touch that file again.
The textual guard pattern got fixed at the spots I named rather than swept. Around seven other sites are still source text with no behavioural backstop: routing_approver's tally and its delegation to routing_decision, the host leg having no approver, _probe's retry and timeout and memoize settings, the hook wiring into build_task, and fail_on_error=False. Most predate this branch, and the effort one you already call out as a deliberate tradeoff against a live sandboxed run. Not this PR's debt, just noting where the same failure mode still lives.
Daniel's two threads are still open and unanswered, the upstream signal on the Windows driver and the fresh catalogue parity run. Those are his to clear and my approval does not speak for them.
Summary
Rebuilds the eval engine on
inspect_aiand retires the hand-rolled one. Two engines remain, selectable with--engine; both drive the realclaudeCLI, and what differs is where it runs.claude-code-no-sandboxis the default and the replacement forlegacy. The two were measured per case against the run's own noise floor —legacyrun twice in one session, disagreeing with itself on 2 of 67 — and agreed on every routing case outside that floor, across two runs, and on 12 of 13 behavioral cases.inspect_aiis now a required dependency rather than an extra. It was optional whilelegacyexisted, because that engine was standard library only. With it gone, both engines need inspect, so an extra would ship a tool whose graded commands fail on a plain install.skillscope[inspect]still resolves, as an empty extra, so downstream pins keep working.Two engines remain. Both drive the agent a skill is actually written for, so what differs is where it runs — not what it is, and not how cases are written or graded:
--engineclaude-codeinspect_sweclaude-code-no-sandboxinspect_aiWhy. An earlier revision of this section argued that the old engine hand-rolled machinery a framework already provides — subprocess stream pumping, process-group termination. That is not true of the engine that replaced it:
no_sandbox.pycontainscreate_subprocess_exec, areadlineloop,os.killpgandtaskkill, becauseinspect_ai.util.subprocessdoes not stream and the routing leg has to stop at the decision. The host leg hand-rolls the same machinery, plus a framework.What the move actually buys, stated so it can be argued with:
.evaltranscripts. The decisive one, and this branch is the evidence: the double-counted bridge turns, the dropped--effort, the installer droppingreference.md, and 152 post-decision tool calls were all found by reading transcripts. A report records what the harness claimed; a transcript records what the agent did, and when the harness is the broken thing the report inherits the bug. Loud failures need no transcript — those came from CI logs.Parity is the headline result, and parity means no better. The honest case for this work is not that it scores higher — it is a sandbox, transcripts, and two engines whose disagreements find bugs one engine cannot.
A note on the isolation claim, and how it resolved. An earlier revision argued that isolation was the substantive gain, on the evidence that one skill scored 0/7 under the host-driven engines and 7/7 under a sandboxed one. That evidence did not support the claim, and the claim was withdrawn: the trial runner's sandbox probe had been falling through to
localsince 2026-09-15, so exactly one Linux run was ever genuinely containerized, and everyclaude-codenumber at the time was produced on the host filesystem by a leg reporting itself as sandboxed.Both halves are now settled, and neither is what the original claim said.
The runner was fixed — rootless podman keeps a pause process the runner's session does not outlive, and
podman system migrateis the remedy — so the numbers below come from real containers.And the 0/7 → 7/7 split had a cause, found later:
inspect_ai's skill installer copiesSKILL.mdplusscripts/,references/andassets/, and this catalogue keepsreference.md,examples.md,templates/andagents/at the top level. The container held a fraction of each skill. With that fixed the same skill scores 7/7 sandboxed against the host's 6/7 — the split is real, an order of magnitude smaller, and points the other way from where it started.What this PR claims is narrower and checkable: the engines are the same agent in different places, the reports say which, and the guards refuse the configurations that silently lied about it.
--engine claude-codenow refuses a sandbox that shares the host's filesystem, which is what surfaced all of the above.Non-obvious decisions
The dataset contract is unchanged.
evals.jsonis untouched, so no skill's cases need rewriting. The CLI surface is not untouched:--engine legacyis rejected, and the default moved toclaude-code-no-sandbox. A product repo migrates by changing nothing, which is the risk rather than the convenience — see Risk.machine.ymlgains an optionalsandbox:key naming a compose file beside it. Absent means the default container with no network — right for a skill that only reads and writes files. Name one to opt into egress or to bind a device in.examples/skill-with-a-device/evals/is the worked pair: amachine.ymlasking for GPU runners, and the compose file that binds the devices in and grants egress. The two are not substitutes —labels:decides which machine the job lands on,sandbox:decides whether the container on it can see the hardware that machine has.Routing runs on both engines, and both stop at the decision. It previously had one leg, and the other two were refused rather than allowed to fall through and report a container that never started. Routing is where a contaminated room matters most — a stray user-level skill changes every case's decision rather than one case's grade — so
claude-codeis the leg whose guest has no~/.claudeto contribute.claude-code-no-sandboxcannot read the CLI's session-init event, so it cannot notice or report that contamination, and therefore refuses a routing leg withoutANTHROPIC_API_KEY, which is what lets it redirect the config dir. Each leg reaches the same stopping rule from a different side:claude-code's calls cross inspect's bridge, so the one revealing the decision is declined before it runs;claude-code-no-sandboxreads the CLI'sstream-jsonas it arrives and kills the process group at the decision, which is what the retired engine always did. An earlier revision of this PR said the host leg could not do this because its output was buffered until exit. That was a property of how it drove the subprocess, not of the CLI, and it cost real money: 152 of that leg's 220 tool calls in one 67-case room happened after the routing question had already been answered. Each report records which caps actually applied rather than which were passed.There is no harness-independent engine. An earlier revision of this PR added one — a framework-native agent, no
claudeCLI involved. It is gone. It carried a smaller tool set than the real harness, so a skill referring to a tool it did not have failed for a reason belonging to the agent rather than the skill, and nothing in the report distinguished the two. It never scored above the sandboxed real-CLI leg on any skill, and its routing numbers measured a different activation mechanism than the harness uses, so they were never interchangeable with the rest.Windows runs unsandboxed, by construction.
inspect_sweinstalls a Linux proxy binary in the guest and cannot run on a Windows guest at all, soclaude-code-no-sandboxdrives the CLI from the host instead. That is what the retired engine already did on Windows, so it is not a regression — but it is not an improvement either, and reports now statesandbox_isolatedrather than leaving a reader to assume. Worth stating plainly: requirement 1 (same harness on Windows and Linux) was already met by the engine this replaces, which ran everywhere. This work does not close a Windows gap; it puts the only Windows-capable engine in the default position, which is why its three-repetition measurement below matters more than it would have before.inspect_sweis an optional extra, not load-bearing.claude-code-no-sandboxreaches the same real agent throughinspect_aialone, so only theclaude-codecross-check depends on it.The package still gets bigger. 7,261 insertions against 722 deletions, 2,463 of the insertions tests. Retiring
legacyaccounts for the deletions and turned out to be surgery rather than a file removal: the inspect engines import the shared vocabulary from those modules —classify,Outcome,detect_activation,claude_env, the stream walker — sorouting.pyis the grader every recorded result was measured against, not a leg.agent.pycontributes nothing to the removal at all;engine/scorers.pyandengine/behavioral.pyboth import from it.What this costs
evals/hooks.pyran only onlegacy, and retiring it took the behavioral leg away from the one skill in the catalogue that ships a hook — on scarce GPU hardware. That is now fixed rather than accepted.setupandteardownrun again: inspect'sTask.setupandTask.cleanupare the same two shapes, andcleanupruns inside afinallyunder a shielded cancel scope, so teardown still happens when the agent raises.What remains refused is narrower and now fires on the entry points a hook defines rather than on the file existing.
checkwas handed the retired engine'sRunobject where these engines have inspect'sTaskState;setup_sessionreturns template variables substituted into prompts that are built before any hook runs. Checked before narrowing: both federated source repos ship no hooks, neither federated skill carries one, and the single hook in the catalogue defines onlysetup/teardownand returns nothing.docs/authoring-evals.mdadvertised all four, and now says which two are gone.Non-goals
checkandsetup_sessionhooks, for the reasons above — they need the retired engine'sRunobject and a template-variable pass that happens before any hook runs. Nothing in the catalogue or either federated source repo uses them.A mid-run credential refresh. The Instinct pool's federated token lives 9m 58s; the trial now re-mints between skills, which covers a job whose skills each fit inside that, and does nothing for a single case that runs longer.
Rolling out to product repos. That needs a merged, tagged release; the consumer path is validated against this branch but no repo has been pointed at it.
Test plan
Unit tests run in CI on Linux and Windows, alongside a key-free pass over the sandbox layer itself — a case seeds a file and expects that same file, with a stub solver standing in for the agent, so what passes or fails is whether the container started, the fixture staged, the guest listed and the listing matched. No model is called, so it runs on a pull request from a fork, which is where a cross-platform sandbox bug shows up.
The evidence below comes from running the whole catalogue of a downstream skills repository on self-hosted runners, through an enterprise Anthropic-compatible gateway, at one commit.
Per leg, against the run's own noise floor. Whole room, one commit, one session, no degraded cases. The second
legacypass is the floor: how much one engine disagrees with itself.legacylegacyclaude-code-no-sandboxclaude-codelegacyvs itselfclaude-code-no-sandboxis at parity withlegacy: across two runs its only differing cases were oneslegacyalso disagreed with itself on. That is the result the legacy retirement depends on, so it is stated per case rather than as an aggregate — an earlier revision of this table quoted aggregate accuracies of 0.892 against 0.894, which agreed while the cases underneath them did not.claude-code's remaining gap is 3 net against a floor of 2. Two of the five are prompts naming artifacts — a stopped session, three kernel variants — that exist on no clean machine; the other three were decided on the agent's first tool call, with no environmental input at all.Two measurement bugs found while producing this table, both of which had made earlier versions of it wrong:
--effort, while the other two did.inspect_swe's default leaves the model's own reasoning effort in place, so the comparison was between a leg thinking as hard as it was told and one thinking as hard as it liked. Fixing it changed no scores, but the earlier numbers were not comparable.inspect_ai's skill installer copiesSKILL.mdplusscripts/,references/andassets/, and drops everything else. This catalogue keepsreference.md,examples.md,templates/andagents/at the top level and has noreferences/directory anywhere — so the container held a fraction of each skill while the host legs copied all of it. In the clearest case the agent opened the right skill'sSKILL.md, followed it toreference.md, found nothing, and stopped: scored as a missed trigger. Staging the dropped files movedclaude-codefrom 55/67 to 58/67 and left the host legs untouched.Cost is no longer the lopsided figure an earlier revision reported. That 3.9× was measured while the host leg ran the skill's whole job after routing; with the early stop it makes zero post-decision tool calls, and its total fell from 266 to 83 on the same room.
Both legs reported zero provider-degraded cases, which is what makes the comparison readable at all — a gateway outage during either pass would otherwise have shown up as an engine difference.
Behavioral, checks passed per skill (Linux). Re-measured on one commit after both measurement fixes, with
claude-codegenuinely containerized. All six Linux skills, 13 behavioral cases, both engines.claude-code-no-sandbox(host)claude-code(container)Three cases differ, and each differs for a different reason — which is the argument for keeping both engines rather than picking one:
analyze-simple-hip-kernel— container 3/4. The judge: "The agent never ran Magpie analyze (Magpie and hipcc were absent); it only wrote a plan document." The toolchain genuinely is not in the container. A clean machine measures what the skill needs; a developer machine already has it.gemm-01-repeatability— host 6/7, container 7/7. The container is the one that passes. On the host the judge found "no tool calls, final message, or analysis.md content" after 682s; the container finished in 407s with the artifact present.generate-cat-image— container 6/7, failingunexpected_behavior: "the agent installed TTS/STT components (kokoro, whispercpp) unrelated to the image task." That is a finding about the skill, not the engine, and only the container surfaced it.An earlier version of this table reported
tracelens-analysis-orchestratoras 0/7 on the host against 7/7 in the container, andlocal-ai-useas timed out. Both were artifacts: theclaude-codecolumn had been produced on the host filesystem rather than in a container, and inspect's skill installer was dropping most of each skill's supporting files. With those fixed the two engines are within one case of each other.Windows, measured three times.
claude-code-no-sandboxis the only engine that reaches a Windows runner —inspect_sweinstalls a Linux proxy binary in the guest — and it is the default, so this is requirement 1 and it had never run in CI until now. Same commit, same machine, three consecutive repetitions:keyword-routerllm-as-routerpii-regex-routergenerate-cat-imagegemm-01-repeatabilityFour of five are stable — identical scores, times within seconds of each other. The fifth is not, and a single run would have reported any one of three different answers: 600s, 478s, 1032s, with nothing changed between them. An earlier revision of this table flagged Windows variance as uncharacterised on the strength of one 21/21 and one 15/21; it is now localised to one case rather than left as a warning about the platform. Note that
gemm-01-repeatabilityis also the case the container wins on Linux (7/7 against the host's 6/7), so this looks like a property of that case on a host leg rather than of Windows.Known gaps
hyperloom-workload-optimizerscores 2/2 cases, 8/8 checks in a podman container on an MI300X — the first graded result that pool has produced. Four earlier runs timed out at 300 minutes each, which read as a hardware or engine problem and was neither: the trial's Instinct job had been built on the wrong credential model. That pool sits off the gateway's network and authenticates by trading a GitHub OIDC token for a short-lived Anthropic one; the job inherited a workflow-levelANTHROPIC_BASE_URLit had no route to, carried a key it must not use, asked for runner labels the pool does not advertise, and declared noid-token: write— so it could not have requested a token even had the rest been right. Fixed in the trial workflow, not in this package.serving-llms-on-instinctshipsevals/hooks.py; retiringlegacytook its behavioral leg away, and hook support restored it. It now runs 114s with a real agent inside its compose sandbox on an MI300X —setupbefore,teardownafter — scoring 4/5, where the first attempt was 0/0 checks in 0.0s.sandbox:feature, not in the engines. The judge: "The agent never launched or reused any container; no Docker runtime existed and it stopped." The skill exists to serve a model in a container; itsevals/compose.yamlgrants the card and the network but not a Docker runtime. It is the only non-example user of thesandbox:key this PR adds, and this is the first run that got far enough to say so. The agent detecting the blocker and stopping is arguably correct behaviour being scored as a miss.401 OAuth access token has expired. The job now re-mints before each skill.expires_inwas in the exchange response and being discarded; skillscope now prints it, which is the only reason that number is known rather than inferred. A single case running longer than the lifetime still needs a mid-run refresh — not done.evals/evals.jsonpairs each prompt withskill_should_trigger— the routing answer — and with theexpected_behaviora behavioral case is graded against. The host leg copied the skill directory wholesale, so that file sat inside the room the agent was being asked to choose from; the sandboxed leg never had it, so the two legs whose agreement the legacy retirement rests on were choosing from different rooms. No agent was ever observed opening it — every case in a 67-case run was checked — but that is a belief about behaviour rather than a property of the room. It is now excluded from both legs, and a case that opens a dataset file reportsanswer_key_readin the column a reader is already looking at, because the routes by which a copy could reach an agent are what is not known in advance. Nothing depended on it: workspace fixtures underevals/filesare read from the skill directory on disk and seeded into the workspace, a different path.inspect_swecannot run there at all.Risk
Medium, and carried by everyone rather than by opt-in. An earlier revision of this section said the default engine, the CLI surface and the optional dependencies were unchanged. None of those is true any more:
legacyis removed, so the default engine isclaude-code-no-sandbox,--engine legacyis rejected by argparse, andinspect_aiis a required dependency. The dataset format is genuinely unchanged.Nothing here is opt-in. The reusable workflow and the composite action name no engine and no sandbox — I checked both — so every consumer takes whatever the defaults are. That is how validating against a real consumer turned up two defaults that contradicted each other, and neither appeared in 429 unit tests because every one of them set
SKILLSCOPE_SANDBOXexplicitly:pip install skillscope && skillscope routingrefused, because the default engine requiredSKILLSCOPE_SANDBOX=localwhile the default provider wasdockerANTHROPIC_API_KEYspecifically and federation holds no key by designBoth are fixed. The consumer path is now exercised end-to-end through
bootstrap/launch.py— the entry pointaction.ymlinvokes — against a product repo's vendored skill with nothing set.Of the four consumers I can find, three pin a tag (
v0.1.3) and one,AMD-AGI/TraceLens, tracksmainthroughreusable.yml. Its graded legs are dispatch-only, so ordinary runs executestructuralalone; the pin is raised on their PR rather than assumed. There is no authoritative list of consumers — these were found by code search across two orgs.