fix(sandbox): install skills natively per harness (AI-1034) - #240
Open
seanoliver wants to merge 3 commits into
Open
fix(sandbox): install skills natively per harness (AI-1034)#240seanoliver wants to merge 3 commits into
seanoliver wants to merge 3 commits into
Conversation
Skills were installed with a flagless `skills add`, which — finding no agent CLI installed yet — falls back to every one of the 71 agents the CLI knows. That scattered ~53 stray roots across the workspace (`.adal`, `.factory`, and non-dotted `data/` and `skills/` among them), and the workspace is exported into run artifacts and scored. It is also order-dependent: had an agent CLI been installed first, the fallback would have quietly stopped producing `.claude/skills` altogether. `skills add` now names the three CLI harnesses explicitly, which installs into exactly the two project scopes they discover natively: - `.claude/skills/` — Claude Code - `.agents/skills/` — Codex and OpenCode This is the actual fix for Codex, which does not read `.claude/skills` at all and therefore saw no skills in any eval. All three are installed unconditionally: the ids collapse to two directories, an unused copy costs a few kilobytes, and no agent id has to be threaded through `createAgentEnvironment` for correctness. Argument order matters — `--agent` is variadic, so the source directory must precede it and `--skill` terminates the list. `--copy` stays: symlink mode skips agents whose top-level directory does not already exist. The post-install check now verifies every agent scope, so a skill missing from one of them fails loudly instead of leaving that harness silently skill-less. The session's harness id is threaded through to the two prompt-addendum builders (`buildSkillsPrompt`, `buildToolSurfaceAddendum`, the latter lifted out of `startSession` so it is testable), but neither branches on it yet: every harness still gets the same injected text as before. What each one should actually be told is a separate change. Refs AI-1034, #164
Review fixes on the explicit-agent install: - The no-`--agent` fallback does reach both `.claude/skills` and `.agents/skills`, so it was never the reason skills failed to arrive. It is a pollution and determinism problem: ~52 stray roots in the scored workspace, and detection that depends on the environment. The order-dependence the comment claimed does not reproduce on 1.5.11. - `LocalStackSessionArgs.agent` said the addendum builders tailor their text to it. They take it and ignore it here. - Quote the staging dir in the install command. - The docker test checks a sample of the fallback's stray roots, not the whole set. Say so, and include the non-dotted ones. - README described the injected-listing path as though it were the native one.
The README claimed the harness does not describe installed skills. It still does, for every harness, and this change's own unit test pins that.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
This was referenced Aug 25, 2026
seanoliver
marked this pull request as ready for review
August 25, 2026 23:38
mattrossman
reviewed
Aug 28, 2026
| * Detection depends on the surrounding environment, so naming the agents is | ||
| * what makes the install predictable. | ||
| */ | ||
| export const SKILLS_INSTALL_AGENTS = [ |
Collaborator
There was a problem hiding this comment.
When someone tries to add a new harness elsewhere in the repo, can we make this fail loudly if they forget to update it, instead of silently running evals w/o skills?
An exhaustive Record<AgentHarnessId, string | null> could do it, since the runner relies on the same id. SKILLS_INSTALL_AGENTS and SKILLS_INSTALL_DIRS could then be derived from it.
E.g.
const SKILLS_PATH_BY_AGENT: Record<AgentHarnessId, string | null> = {
'ai-sdk': null,
'claude-code': '.claude/skills',
codex: '.agents/skills',
opencode: '.agents/skills',
};
export const SKILLS_INSTALL_AGENTS: AgentHarnessId[] = [];
export const SKILLS_INSTALL_DIRS: string[] = [];
for (const id of agentHarnessIdSchema.options) {
const path = SKILLS_PATH_BY_AGENT[id];
if (path === null) continue;
SKILLS_INSTALL_AGENTS.push(id);
if (!SKILLS_INSTALL_DIRS.includes(path)) SKILLS_INSTALL_DIRS.push(path);
}
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.
What's inside
Bottom of a 2-PR stack under #241, which removes the system prompt we write for the CLI agents.
Claude Tag wrote the first pass at this (previously #180, now closed).
Problem
Installing a skill means copying its folder somewhere an agent will look, and each agent reads a different directory.
skills addtakes a flag naming which agents to install for, but we weren't not passing it..aider-desk,.factory,.kilocode,.windsurf,.zencoder, and non-dotteddata/andskills/among them. Each holds a copy of the same two Supabase skills..agents/skillsfolder, which required us to include an addendum to the Codex prompt to get it to look in the Claude skills folder (.claude/skills).Changes
Install
Name the three agents we run in this repo:
That writes two directories,
.claude/skillsand.agents/skills, instead of 52.--copyis required so that.claude/skillsgets created in an empty eval workspace. Without it only.agents/skillsshows up.After installing,
installSkillsconfirms every skill is actually present in both directories (.claude/skillsand.agents/skills). It used to check.claude/skillsonly.Plumbing for the next PR
The prompt builders now take the agent as an argument and ignore it. This is used by #241 to skip the prompt text for the CLI agents.
How to review
Confirm the new install works:
If interested, run both install strategies side by side in a temp folder to see the difference:
You should see 53 entries (old) vs. 3 (new).
Follow up tasks
skills-lock.jsonfrom the workspace after installing. The CLI drops it in the root and it ends up in the exported workspace, same as before this change.Ref AI-1034