diff --git a/docs/sandbox-integration-package.md b/docs/sandbox-integration-package.md index 459adc4..712054e 100644 --- a/docs/sandbox-integration-package.md +++ b/docs/sandbox-integration-package.md @@ -69,10 +69,10 @@ class SandboxError extends Error { #### ensureSandboxExists() ```typescript -async function ensureSandboxExists(): Promise +async function ensureSandboxExists(requiredPaths: string[] = []): Promise ``` -Pre-flight check that the sandbox is running. Runs `sbx ls --quiet` and verifies `SANDBOX_NAME` exactly matches one output line. Throws `SandboxError` with `exitCode: null` if the sandbox is not found, with a message directing the user to run `./build/skillwalker sandbox create`. +Pre-flight check that the sandbox exists and mounts what the run needs. Runs `sbx ls --json`, finds the entry named `SANDBOX_NAME`, and checks that every path in `requiredPaths` is inside one of its workspaces (a trailing `:ro` on a listed workspace is ignored). `runEvals` and the SCIL and ACIL loops pass `[sandboxScriptsDir]`. A sandbox created before the scripts mount was added keeps its old workspaces, so this check fails it before any test runs instead of at the first `sbx exec`. Throws `SandboxError` with `exitCode: null` if the sandbox is not found, with a message directing the user to run `./build/skillwalker sandbox create`. **Consumers:** - `cli/src/commands/test-run.ts` -- before the per-eval test loop @@ -98,6 +98,8 @@ Primary execution function. Builds and spawns the command `sbx exec claude-skill - Both streams are fully captured regardless of the `debug` flag. - Does not throw on non-zero exit codes; the caller inspects `SandboxResult.exitCode`. +`sbx exec` exits 0 even when it cannot start the command, printing `OCI runtime exec failed: ...` instead (for example, when the script's host path is outside every sandbox workspace). `execInSandbox` throws `SandboxError` when any stdout or stderr line starts with that message, pointing the user at `sandbox update`. + **Consumer:** `claude-integration/src/run-claude.ts` imports `execInSandbox` as the execution primitive for all Claude invocations inside the sandbox. ### lifecycle.ts -- Lifecycle Management @@ -105,10 +107,12 @@ Primary execution function. Builds and spawns the command `sbx exec claude-skill #### createSandbox() ```typescript -async function createSandbox(repoRoot: string): Promise +async function createSandbox(repoRoot: string, extraWorkspaces: string[] = []): Promise ``` -Checks whether the sandbox already exists via an internal `sandboxExists()` helper (runs `sbx ls --quiet`). If found, prints a help message to stderr explaining how to recreate it, and returns early. Otherwise, spawns `sbx run --name claude-skills-skillwalker claude ` with inherited stdio for interactive OAuth login. Prints progress messages to stderr. +Checks whether the sandbox already exists via an internal `sandboxExists()` helper (runs `sbx ls --quiet`). If found, prints a help message to stderr explaining how to recreate it, and returns early. Otherwise, spawns `sbx run --name claude-skills-skillwalker claude [:ro ...]` with inherited stdio for interactive OAuth login. Prints progress messages to stderr. + +`extraWorkspaces` are mounted read-only after `repoRoot` (`:ro`); any already inside `repoRoot` are skipped. The CLI passes the directory holding `sandbox-run.sh` and `sandbox-extract.sh` (`sandboxScriptsDir` from `@testdouble/claude-integration`). `execInSandbox` runs those scripts by their host path, and the sandbox only sees host paths under a mounted workspace, so without this mount every test run fails whenever the target repo is not the skillwalker repo. **Consumer:** `cli/src/commands/sandbox/create.ts` @@ -125,14 +129,14 @@ Runs `sbx rm --force claude-skills-skillwalker`. Drains stdout and stderr in par #### updateSandbox() ```typescript -async function updateSandbox(repoRoot: string): Promise +async function updateSandbox(repoRoot: string, extraWorkspaces: string[] = []): Promise ``` Replaces the sandbox with one built from the latest Claude Code template. `sbx` has no pull command and reuses a cached template image, so this function: 1. Removes the sandbox with `removeSandbox()`, if it exists. 2. Lists templates with `sbx template ls` and removes each cached image whose repository is `docker/sandbox-templates` and whose tag starts with `claude-code`, using `sbx template rm `. -3. Calls `createSandbox(repoRoot)`, which makes `sbx run` fetch the current template. +3. Calls `createSandbox(repoRoot, extraWorkspaces)`, which makes `sbx run` fetch the current template. `sbx template ls` can list one image under several IDs, and removing the first ID removes them all. A later `rm` that reports `no template image` is therefore treated as already removed. Any other listing or removal failure throws `SandboxError`. @@ -207,7 +211,9 @@ flowchart TB | Scenario | Error Type | Behavior | |----------|------------|----------| | Sandbox not found by `ensureSandboxExists` | `SandboxError` (exitCode: `null`) | Thrown with message suggesting `./build/skillwalker sandbox create` | +| Required path not mounted, checked by `ensureSandboxExists` | `SandboxError` (exitCode: `null`) | Thrown naming the unmounted path, with a hint to run `skillwalker sandbox update` from the target repo | | `sbx rm` fails | `SandboxError` (exitCode: process code) | Thrown with stdout+stderr in message | +| `sbx exec` prints `OCI runtime exec failed` (exits 0) | `SandboxError` (exitCode: process code) | Thrown with the sbx output and a hint to run `skillwalker sandbox update` from the target repo | | Non-zero exit from `execInSandbox` | No error thrown | Returned in `SandboxResult.exitCode`; caller decides | | `proc.exitCode` is null in `execInSandbox` | No error thrown | Defaults to `1` in `SandboxResult` | diff --git a/docs/sandbox-integration.md b/docs/sandbox-integration.md index b3d07b9..0d6ad6c 100644 --- a/docs/sandbox-integration.md +++ b/docs/sandbox-integration.md @@ -98,7 +98,7 @@ export class SandboxError extends Error { #### ensureSandboxExists -Pre-flight check that the sandbox is running. Runs `sbx ls --quiet` and verifies `SANDBOX_NAME` exactly matches one output line. Throws `SandboxError` with `exitCode: null` if not found. +Pre-flight check that the sandbox exists and mounts what the run needs. Runs `sbx ls --json`, finds the entry named `SANDBOX_NAME`, and checks that every path in `requiredPaths` is inside one of its workspaces (a trailing `:ro` on a listed workspace is ignored). `runEvals` and the SCIL and ACIL loops pass `[sandboxScriptsDir]`. A sandbox created before the scripts mount was added keeps its old workspaces, so this check fails it before any test runs instead of at the first `sbx exec`. Throws `SandboxError` with `exitCode: null` if not found. Called by: - `commands/test-run.ts` — before the per-eval test loop @@ -124,6 +124,8 @@ export async function execInSandbox( - stderr is drained in parallel via `new Response(stream).text()`. When `debug` is `true` and stderr is non-empty, it is written to `process.stderr`. - Both streams are fully captured regardless of the `debug` flag. +`sbx exec` exits 0 even when it cannot start the command, printing `OCI runtime exec failed: ...` instead (for example, when the script's host path is outside every sandbox workspace). `execInSandbox` throws `SandboxError` when any stdout or stderr line starts with that message, pointing the user at `sandbox update`. + **Consumers and their claude args patterns:** | Consumer | Key Args | Scaffold | @@ -166,20 +168,22 @@ When a scaffold path is provided, it copies the scaffold into a fresh temp direc #### createSandbox ```typescript -export async function createSandbox(repoRoot: string): Promise +export async function createSandbox(repoRoot: string, extraWorkspaces: string[] = []): Promise ``` -Checks if the sandbox already exists via an internal `sandboxExists()` helper. If it does, prints a help message to stderr and returns. Otherwise, spawns `sbx run --name claude-skills-skillwalker claude ` with inherited stdio for interactive OAuth login. +Checks if the sandbox already exists via an internal `sandboxExists()` helper. If it does, prints a help message to stderr and returns. Otherwise, spawns `sbx run --name claude-skills-skillwalker claude [:ro ...]` with inherited stdio for interactive OAuth login. + +`extraWorkspaces` are mounted read-only after `repoRoot` (`:ro`); any already inside `repoRoot` are skipped. The CLI passes the directory holding `sandbox-run.sh` and `sandbox-extract.sh` (`sandboxScriptsDir` from `@testdouble/claude-integration`). `execInSandbox` runs those scripts by their host path, and the sandbox only sees host paths under a mounted workspace, so without this mount every test run fails whenever the target repo is not the skillwalker repo. Called by `commands/sandbox/create.ts`. #### updateSandbox ```typescript -export async function updateSandbox(repoRoot: string): Promise +export async function updateSandbox(repoRoot: string, extraWorkspaces: string[] = []): Promise ``` -Removes the sandbox if it exists, then removes every cached `docker/sandbox-templates` image tagged `claude-code*` (found with `sbx template ls`). Finally it calls `createSandbox`, so `sbx run` fetches the latest Claude Code template. `sbx` has no pull command, so deleting the cached image is the only way to get a newer one. An `rm` that reports `no template image` counts as already removed, because `sbx template ls` can list one image under several IDs. Any other listing or removal failure throws `SandboxError`. +Removes the sandbox if it exists, then removes every cached `docker/sandbox-templates` image tagged `claude-code*` (found with `sbx template ls`). Finally it calls `createSandbox` with the same arguments, so `sbx run` fetches the latest Claude Code template. `sbx` has no pull command, so deleting the cached image is the only way to get a newer one. An `rm` that reports `no template image` counts as already removed, because `sbx template ls` can list one image under several IDs. Any other listing or removal failure throws `SandboxError`. Called by `commands/sandbox/update.ts`, which catches `SandboxError` and re-throws as `SkillwalkerError`. @@ -223,7 +227,9 @@ See [Cross-Runtime Meta Property Resolution](coding-standards/cross-runtime-meta | Scenario | Error Type | Behavior | |----------|------------|----------| | Sandbox not found by `ensureSandboxExists` | `SandboxError` (exitCode: `null`) | Thrown with message suggesting `./build/skillwalker sandbox create` | +| Required path not mounted, checked by `ensureSandboxExists` | `SandboxError` (exitCode: `null`) | Thrown naming the unmounted path, with a hint to run `skillwalker sandbox update` from the target repo | | `sbx rm` fails | `SandboxError` (exitCode: process code) | Thrown with stdout+stderr in message | +| `sbx exec` prints `OCI runtime exec failed` (exits 0) | `SandboxError` (exitCode: process code) | Thrown with the sbx output and a hint to run `skillwalker sandbox update` from the target repo | | Non-zero exit code from `execInSandbox` | No error thrown | Returned in `SandboxResult.exitCode`; caller decides | | `execInSandbox` with `proc.exitCode` null | No error thrown | `exitCode` defaults to `1` in `SandboxResult` | @@ -232,7 +238,7 @@ See [Cross-Runtime Meta Property Resolution](coding-standards/cross-runtime-meta | Layer | Pattern | |-------|---------| | CLI commands (`clean.ts`) | Catches `SandboxError`, re-throws as `SkillwalkerError` | -| Pre-flight checks (`test-run.ts`, `loop.ts`) | No catch — `SandboxError` propagates and crashes the process | +| Pre-flight checks (`test-run.ts`, `loop.ts`) and test runners | No catch — `SandboxError` propagates to `cli/index.ts`, which prints `Error: ` and exits 1 | | Test runners (`prompt/`, `skill-call/`) | Checks `exitCode` on `SandboxResult`, increments failure counter | | LLM judge (`step-3b`) | Catches all errors, records `status: 'infrastructure-error'` in results | | SCIL step-5 | Catches errors per work-item, logs to stderr, continues | @@ -281,6 +287,10 @@ If `ensureSandboxExists` throws `SandboxError`, run: 1. `./build/skillwalker sandbox create` — creates the sandbox and completes OAuth 2. Verify with `sbx ls --quiet` — should list `claude-skills-skillwalker` +### Sandbox does not mount a required path + +If `ensureSandboxExists` reports that the sandbox does not mount a path, the sandbox predates that mount. From the target repo, run `./build/skillwalker sandbox update`, then verify with `sbx ls --json` that `claude-skills-skillwalker` lists both the target repo and the scripts directory. + ### Sandbox already exists during setup `createSandbox` returns early with a help message. To recreate: diff --git a/packages/claude-integration/index.ts b/packages/claude-integration/index.ts index 216dfdd..d8c1981 100644 --- a/packages/claude-integration/index.ts +++ b/packages/claude-integration/index.ts @@ -3,4 +3,5 @@ export type { OutputFile } from './src/extract-output-files.js' export { extractOutputFiles } from './src/extract-output-files.js' export { resolvePluginDirs } from './src/plugin-flags.js' export { runClaude } from './src/run-claude.js' +export { sandboxScriptsDir } from './src/sandbox-scripts.js' export type { ClaudeRunOptions, ClaudeRunResult } from './src/types.js' diff --git a/packages/claude-integration/src/extract-output-files.ts b/packages/claude-integration/src/extract-output-files.ts index 21e35a8..146e137 100644 --- a/packages/claude-integration/src/extract-output-files.ts +++ b/packages/claude-integration/src/extract-output-files.ts @@ -1,15 +1,13 @@ -import { resolveRelativePath } from '@testdouble/bun-helpers' import { execInSandbox } from '@testdouble/sandbox-integration' +import { sandboxExtractScript } from './sandbox-scripts.js' export interface OutputFile { path: string content: string } -const extractScript = resolveRelativePath(import.meta, '../sandbox-extract.sh', 'sandbox-extract.sh') - export async function extractOutputFiles(debug: boolean): Promise { - const { stdout } = await execInSandbox(extractScript, [], null, debug) + const { stdout } = await execInSandbox(sandboxExtractScript, [], null, debug) if (!stdout.trim()) return [] diff --git a/packages/claude-integration/src/run-claude.ts b/packages/claude-integration/src/run-claude.ts index e2826a4..e2afd37 100644 --- a/packages/claude-integration/src/run-claude.ts +++ b/packages/claude-integration/src/run-claude.ts @@ -1,9 +1,7 @@ -import { resolveRelativePath } from '@testdouble/bun-helpers' import { execInSandbox } from '@testdouble/sandbox-integration' +import { sandboxRunScript } from './sandbox-scripts.js' import type { ClaudeRunOptions, ClaudeRunResult } from './types.js' -const sandboxRunScript = resolveRelativePath(import.meta, '../sandbox-run.sh', 'sandbox-run.sh') - export async function runClaude(options: ClaudeRunOptions): Promise { const { model, prompt, pluginDirs = [], scaffold = null, debug = false } = options diff --git a/packages/claude-integration/src/sandbox-scripts.ts b/packages/claude-integration/src/sandbox-scripts.ts new file mode 100644 index 0000000..fa247d6 --- /dev/null +++ b/packages/claude-integration/src/sandbox-scripts.ts @@ -0,0 +1,12 @@ +import path from 'node:path' +import { resolveRelativePath } from '@testdouble/bun-helpers' + +export const sandboxRunScript = resolveRelativePath(import.meta, '../sandbox-run.sh', 'sandbox-run.sh') +export const sandboxExtractScript = resolveRelativePath(import.meta, '../sandbox-extract.sh', 'sandbox-extract.sh') + +/** + * Directory holding the scripts `sbx exec` runs by their host path. The sandbox + * only sees host paths under a mounted workspace, so this directory must be + * mounted alongside the target repo. + */ +export const sandboxScriptsDir = path.dirname(sandboxRunScript) diff --git a/packages/cli/index.ts b/packages/cli/index.ts index 4c5097f..304e7d8 100644 --- a/packages/cli/index.ts +++ b/packages/cli/index.ts @@ -1,4 +1,5 @@ #!/usr/bin/env bun +import { SandboxError } from '@testdouble/sandbox-integration' import { SkillwalkerError } from '@testdouble/skillwalker-execution' import yargs from 'yargs' import { hideBin } from 'yargs/helpers' @@ -14,10 +15,17 @@ try { .command(await import('./src/commands/acil.js')) .demandCommand(1) .strict() - .showHelpOnFail(true) + // Rethrow handler errors to the catch below. Without this, yargs prints + // help and the raw error for them and exits before the catch runs. + .fail((message, error, cli) => { + if (error) throw error + cli.showHelp() + process.stderr.write(`\n${message}\n`) + process.exit(1) + }) .parseAsync() } catch (err) { - if (err instanceof SkillwalkerError) { + if (err instanceof SkillwalkerError || err instanceof SandboxError) { process.stderr.write(`Error: ${err.message}\n`) process.exit(1) } diff --git a/packages/cli/src/command-registration.integration.test.ts b/packages/cli/src/command-registration.integration.test.ts index e6dffc8..4fc89a6 100644 --- a/packages/cli/src/command-registration.integration.test.ts +++ b/packages/cli/src/command-registration.integration.test.ts @@ -57,3 +57,13 @@ describe('sandbox sub-command registration', () => { expect(output).toContain('Unknown argument: bogus') }) }) + +describe('command handler errors', () => { + it('prints a domain error as a single Error line without help text', () => { + const { status, output } = runCli('test-eval', 'no-such-run-id') + expect(status).toBe(1) + expect(output).toMatch(/^Error: Test run directory not found:/m) + expect(output).not.toContain('Options:') + expect(output).not.toContain('RunNotFoundError') + }) +}) diff --git a/packages/cli/src/commands/sandbox/create.test.ts b/packages/cli/src/commands/sandbox/create.test.ts index e2b4c08..ea6447d 100644 --- a/packages/cli/src/commands/sandbox/create.test.ts +++ b/packages/cli/src/commands/sandbox/create.test.ts @@ -4,6 +4,10 @@ vi.mock('@testdouble/sandbox-integration', () => ({ createSandbox: vi.fn(), })) +vi.mock('@testdouble/claude-integration', () => ({ + sandboxScriptsDir: '/skillwalker/build', +})) + import { createSandbox } from '@testdouble/sandbox-integration' import { builder, command, describe as commandDescribe, handler } from './create.js' @@ -47,8 +51,8 @@ describe('sandbox create builder', () => { }) describe('sandbox create handler', () => { - it('calls createSandbox with the resolved repo-root', async () => { + it('calls createSandbox with the resolved repo-root and the sandbox scripts directory', async () => { await handler({ 'repo-root': '/repo/root' }) - expect(vi.mocked(createSandbox)).toHaveBeenCalledWith('/repo/root') + expect(vi.mocked(createSandbox)).toHaveBeenCalledWith('/repo/root', ['/skillwalker/build']) }) }) diff --git a/packages/cli/src/commands/sandbox/create.ts b/packages/cli/src/commands/sandbox/create.ts index 04533f2..da09283 100644 --- a/packages/cli/src/commands/sandbox/create.ts +++ b/packages/cli/src/commands/sandbox/create.ts @@ -1,3 +1,4 @@ +import { sandboxScriptsDir } from '@testdouble/claude-integration' import { createSandbox } from '@testdouble/sandbox-integration' import type { Argv } from 'yargs' @@ -13,5 +14,5 @@ export function builder(yargs: Argv): Argv { } export async function handler(argv: Record): Promise { - await createSandbox(argv['repo-root'] as string) + await createSandbox(argv['repo-root'] as string, [sandboxScriptsDir]) } diff --git a/packages/cli/src/commands/sandbox/update.test.ts b/packages/cli/src/commands/sandbox/update.test.ts index c24fdf2..46954ae 100644 --- a/packages/cli/src/commands/sandbox/update.test.ts +++ b/packages/cli/src/commands/sandbox/update.test.ts @@ -12,6 +12,10 @@ vi.mock('@testdouble/sandbox-integration', () => ({ }, })) +vi.mock('@testdouble/claude-integration', () => ({ + sandboxScriptsDir: '/skillwalker/build', +})) + import { SandboxError, updateSandbox } from '@testdouble/sandbox-integration' import { SkillwalkerError } from '@testdouble/skillwalker-execution' import { builder, command, describe as commandDescribe, handler } from './update.js' @@ -58,7 +62,7 @@ describe('sandbox update builder', () => { describe('sandbox update handler', () => { it('calls updateSandbox with the resolved repo-root', async () => { await handler({ 'repo-root': '/repo/root' }) - expect(vi.mocked(updateSandbox)).toHaveBeenCalledWith('/repo/root') + expect(vi.mocked(updateSandbox)).toHaveBeenCalledWith('/repo/root', ['/skillwalker/build']) }) it('throws SkillwalkerError when updateSandbox throws SandboxError', async () => { diff --git a/packages/cli/src/commands/sandbox/update.ts b/packages/cli/src/commands/sandbox/update.ts index e7c8509..05041a6 100644 --- a/packages/cli/src/commands/sandbox/update.ts +++ b/packages/cli/src/commands/sandbox/update.ts @@ -1,3 +1,4 @@ +import { sandboxScriptsDir } from '@testdouble/claude-integration' import { SandboxError, updateSandbox } from '@testdouble/sandbox-integration' import { SkillwalkerError } from '@testdouble/skillwalker-execution' import type { Argv } from 'yargs' @@ -15,7 +16,7 @@ export function builder(yargs: Argv): Argv { export async function handler(argv: Record): Promise { try { - await updateSandbox(argv['repo-root'] as string) + await updateSandbox(argv['repo-root'] as string, [sandboxScriptsDir]) } catch (error) { if (error instanceof SandboxError) { throw new SkillwalkerError(error.message) diff --git a/packages/execution/src/acil/loop.test.ts b/packages/execution/src/acil/loop.test.ts index c508388..2105798 100644 --- a/packages/execution/src/acil/loop.test.ts +++ b/packages/execution/src/acil/loop.test.ts @@ -47,6 +47,7 @@ vi.mock('node:readline/promises', () => ({ })) import { createInterface } from 'node:readline/promises' +import { sandboxScriptsDir } from '@testdouble/claude-integration' import { ensureSandboxExists } from '@testdouble/sandbox-integration' import { getPhase } from '@testdouble/skillwalker-data' import { generateRunId } from '../test-runners/steps/step-4-generate-run-id.js' @@ -147,6 +148,12 @@ beforeEach(() => { }) describe('runAcilLoop', () => { + it('requires the sandbox to mount the sandbox scripts directory', async () => { + await runAcilLoop(makeConfig({ maxIterations: 1, holdout: 0 })) + + expect(ensureSandboxExists).toHaveBeenCalledWith([sandboxScriptsDir]) + }) + it('exits after one iteration when train accuracy is 1.0 and holdout is 0', async () => { await runAcilLoop(makeConfig({ maxIterations: 5, holdout: 0 })) diff --git a/packages/execution/src/acil/loop.ts b/packages/execution/src/acil/loop.ts index e218a25..75a89b2 100644 --- a/packages/execution/src/acil/loop.ts +++ b/packages/execution/src/acil/loop.ts @@ -1,5 +1,6 @@ import path from 'node:path' import { createInterface } from 'node:readline/promises' +import { sandboxScriptsDir } from '@testdouble/claude-integration' import { ensureSandboxExists } from '@testdouble/sandbox-integration' import { getPhase } from '@testdouble/skillwalker-data' import { SkillwalkerError } from '../lib/errors.js' @@ -43,7 +44,7 @@ export async function runAcilLoop(config: AcilConfig): Promise { // Ensure sandbox exists process.stderr.write('Checking sandbox...\n') - await ensureSandboxExists() + await ensureSandboxExists([sandboxScriptsDir]) // Generate run ID and output directory const runId = generateRunId() diff --git a/packages/execution/src/evals/run-evals.test.ts b/packages/execution/src/evals/run-evals.test.ts new file mode 100644 index 0000000..d5e7204 --- /dev/null +++ b/packages/execution/src/evals/run-evals.test.ts @@ -0,0 +1,44 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('@testdouble/sandbox-integration', () => ({ + ensureSandboxExists: vi.fn(), +})) +vi.mock('../test-runners/steps/step-4-generate-run-id.js', () => ({ + generateRunId: vi.fn(), +})) +vi.mock('../test-runners/steps/step-7-init-totals.js', () => ({ + initTotals: vi.fn(), +})) +vi.mock('../test-runners/steps/step-9-print-totals.js', () => ({ + printTotals: vi.fn(), +})) + +import { sandboxScriptsDir } from '@testdouble/claude-integration' +import { ensureSandboxExists } from '@testdouble/sandbox-integration' +import { generateRunId } from '../test-runners/steps/step-4-generate-run-id.js' +import { initTotals } from '../test-runners/steps/step-7-init-totals.js' +import { runEvals } from './run-evals.js' + +beforeEach(() => { + vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + vi.mocked(ensureSandboxExists).mockResolvedValue(undefined) + vi.mocked(generateRunId).mockReturnValue('20260922T000000') + vi.mocked(initTotals).mockReturnValue({ + totalDurationMs: 0, + totalInputTokens: 0, + totalOutputTokens: 0, + failures: 0, + }) +}) + +afterEach(() => { + vi.restoreAllMocks() +}) + +describe('runEvals', () => { + it('requires the sandbox to mount the sandbox scripts directory', async () => { + await runEvals({ evals: [], debug: false, outputDir: '/out', testsDir: '/tests', repoRoot: '/repo' }) + + expect(ensureSandboxExists).toHaveBeenCalledWith([sandboxScriptsDir]) + }) +}) diff --git a/packages/execution/src/evals/run-evals.ts b/packages/execution/src/evals/run-evals.ts index eb1f9c5..f7098b9 100644 --- a/packages/execution/src/evals/run-evals.ts +++ b/packages/execution/src/evals/run-evals.ts @@ -1,3 +1,4 @@ +import { sandboxScriptsDir } from '@testdouble/claude-integration' import { ensureSandboxExists } from '@testdouble/sandbox-integration' import { resolvePaths } from '../test-runners/steps/step-1-resolve-paths.js' import { validateConfig } from '../test-runners/steps/step-2-validate-config.js' @@ -29,7 +30,7 @@ export async function runEvals(opts: RunEvalsOptions): Promise { const testRunId = generateRunId() process.stderr.write(`Run ID: ${testRunId}\n`) process.stderr.write('Checking sandbox...\n') - await ensureSandboxExists() + await ensureSandboxExists([sandboxScriptsDir]) let totals = initTotals() for (const evalName of opts.evals) { diff --git a/packages/execution/src/scil/loop.test.ts b/packages/execution/src/scil/loop.test.ts index b9e9c2c..4ace37a 100644 --- a/packages/execution/src/scil/loop.test.ts +++ b/packages/execution/src/scil/loop.test.ts @@ -47,6 +47,7 @@ vi.mock('node:readline/promises', () => ({ })) import { createInterface } from 'node:readline/promises' +import { sandboxScriptsDir } from '@testdouble/claude-integration' import { ensureSandboxExists } from '@testdouble/sandbox-integration' import { getPhase } from '@testdouble/skillwalker-data' import { generateRunId } from '../test-runners/steps/step-4-generate-run-id.js' @@ -150,6 +151,12 @@ beforeEach(() => { }) describe('runScilLoop', () => { + it('requires the sandbox to mount the sandbox scripts directory', async () => { + await runScilLoop(makeConfig({ maxIterations: 1, holdout: 0 })) + + expect(ensureSandboxExists).toHaveBeenCalledWith([sandboxScriptsDir]) + }) + // TP-003: early exit on perfect train accuracy (holdout=0) it('exits after one iteration when train accuracy is 1.0 and holdout is 0', async () => { await runScilLoop(makeConfig({ maxIterations: 5, holdout: 0 })) diff --git a/packages/execution/src/scil/loop.ts b/packages/execution/src/scil/loop.ts index ef5c0bf..4b99fc2 100644 --- a/packages/execution/src/scil/loop.ts +++ b/packages/execution/src/scil/loop.ts @@ -1,5 +1,6 @@ import path from 'node:path' import { createInterface } from 'node:readline/promises' +import { sandboxScriptsDir } from '@testdouble/claude-integration' import { ensureSandboxExists } from '@testdouble/sandbox-integration' import { getPhase } from '@testdouble/skillwalker-data' import { generateRunId } from '../test-runners/steps/step-4-generate-run-id.js' @@ -42,7 +43,7 @@ export async function runScilLoop(config: ScilConfig): Promise { // Ensure sandbox exists process.stderr.write('Checking sandbox...\n') - await ensureSandboxExists() + await ensureSandboxExists([sandboxScriptsDir]) // Generate run ID and output directory const runId = generateRunId() diff --git a/packages/sandbox-integration/src/lifecycle.test.ts b/packages/sandbox-integration/src/lifecycle.test.ts index 7664082..b0e4e6c 100644 --- a/packages/sandbox-integration/src/lifecycle.test.ts +++ b/packages/sandbox-integration/src/lifecycle.test.ts @@ -104,6 +104,56 @@ describe('createSandbox', () => { stderrSpy.mockRestore() }) + function stubNoSandboxThenRun() { + ;(globalThis as any).Bun.spawn + .mockReturnValueOnce({ + stdout: makeStream('other-sandbox\n'), + stderr: makeStream(''), + exited: Promise.resolve(), + exitCode: 0, + }) + .mockReturnValueOnce({ + exited: Promise.resolve(), + }) + } + + it('mounts extra workspaces read-only after the repo root', async () => { + stubNoSandboxThenRun() + vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + + const { createSandbox } = await import('./lifecycle.js') + await createSandbox('/repo/root', ['/skillwalker/build']) + + const runArgs = (globalThis as any).Bun.spawn.mock.calls[1][0] + expect(runArgs).toEqual([ + 'sbx', + 'run', + '--name', + 'claude-skills-skillwalker', + 'claude', + '/repo/root', + '/skillwalker/build:ro', + ]) + }) + + it('skips extra workspaces already inside the repo root', async () => { + stubNoSandboxThenRun() + vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + + const { createSandbox } = await import('./lifecycle.js') + await createSandbox('/repo/root', ['/repo/root/build', '/repo/root-sibling']) + + const runArgs = (globalThis as any).Bun.spawn.mock.calls[1][0] + expect(runArgs).toEqual([ + 'sbx', + 'run', + '--name', + 'claude-skills-skillwalker', + 'claude', + '/repo/root', + '/repo/root-sibling:ro', + ]) + }) }) describe('updateSandbox', () => { @@ -137,7 +187,7 @@ describe('updateSandbox', () => { const stderrSpy = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) const { updateSandbox } = await import('./lifecycle.js') - await updateSandbox('/repo/root') + await updateSandbox('/repo/root', ['/skillwalker/build']) expect(spawnedArgs()).toEqual([ ['sbx', 'ls', '--quiet'], @@ -146,7 +196,7 @@ describe('updateSandbox', () => { ['sbx', 'template', 'rm', '6f873d7e6093'], ['sbx', 'template', 'rm', '94670d5b2a24'], ['sbx', 'ls', '--quiet'], - ['sbx', 'run', '--name', 'claude-skills-skillwalker', 'claude', '/repo/root'], + ['sbx', 'run', '--name', 'claude-skills-skillwalker', 'claude', '/repo/root', '/skillwalker/build:ro'], ]) stderrSpy.mockRestore() diff --git a/packages/sandbox-integration/src/lifecycle.ts b/packages/sandbox-integration/src/lifecycle.ts index 319f780..ca96c6c 100644 --- a/packages/sandbox-integration/src/lifecycle.ts +++ b/packages/sandbox-integration/src/lifecycle.ts @@ -1,5 +1,5 @@ import { SandboxError } from './errors.js' -import { ensureSandboxExists, listSandboxNames, SANDBOX_NAME, spawnSbx } from './sandbox.js' +import { ensureSandboxExists, isWithin, listSandboxNames, SANDBOX_NAME, spawnSbx } from './sandbox.js' const CLAUDE_TEMPLATE_REPOSITORY = 'docker/sandbox-templates' const CLAUDE_TEMPLATE_TAG_PREFIX = 'claude-code' @@ -78,7 +78,18 @@ async function removeTemplateImage(imageId: string): Promise { ) } -export async function createSandbox(repoRoot: string): Promise { +/** + * `sbx run` arguments that mount `repoRoot` read-write and each extra workspace + * read-only. Extra workspaces already inside `repoRoot` are visible through it. + */ +function buildRunArgs(repoRoot: string, extraWorkspaces: string[]): string[] { + const extras = extraWorkspaces + .filter((workspace) => !isWithin(repoRoot, workspace)) + .map((workspace) => `${workspace}:ro`) + return ['run', '--name', SANDBOX_NAME, 'claude', repoRoot, ...extras] +} + +export async function createSandbox(repoRoot: string, extraWorkspaces: string[] = []): Promise { if (await sandboxExists()) { process.stderr.write(`Sandbox "${SANDBOX_NAME}" already exists. To recreate, run:\n`) process.stderr.write(` sbx rm --force ${SANDBOX_NAME}\n`) @@ -89,7 +100,7 @@ export async function createSandbox(repoRoot: string): Promise { process.stderr.write(`Creating sandbox "${SANDBOX_NAME}" with workspace ${repoRoot}...\n`) process.stderr.write(`Complete the OAuth login when Claude launches, then exit Claude.\n\n`) - const runProc = spawnSbx(['run', '--name', SANDBOX_NAME, 'claude', repoRoot], { + const runProc = spawnSbx(buildRunArgs(repoRoot, extraWorkspaces), { stdin: 'inherit', stdout: 'inherit', stderr: 'inherit', @@ -99,7 +110,7 @@ export async function createSandbox(repoRoot: string): Promise { process.stderr.write(`\nSandbox "${SANDBOX_NAME}" is ready. You can now run tests.\n`) } -export async function updateSandbox(repoRoot: string): Promise { +export async function updateSandbox(repoRoot: string, extraWorkspaces: string[] = []): Promise { if (await sandboxExists()) { process.stderr.write(`Removing sandbox "${SANDBOX_NAME}"...\n`) await removeSandbox() @@ -110,7 +121,7 @@ export async function updateSandbox(repoRoot: string): Promise { await removeTemplateImage(imageId) } - await createSandbox(repoRoot) + await createSandbox(repoRoot, extraWorkspaces) } export async function openShell(): Promise { diff --git a/packages/sandbox-integration/src/sandbox.test.ts b/packages/sandbox-integration/src/sandbox.test.ts index 70dfb37..7fa744b 100644 --- a/packages/sandbox-integration/src/sandbox.test.ts +++ b/packages/sandbox-integration/src/sandbox.test.ts @@ -21,31 +21,70 @@ afterEach(() => { vi.restoreAllMocks() }) +function makeSandboxList(sandboxes: { name: string; workspaces: string[] }[]): string { + return JSON.stringify({ sandboxes }) +} + +function mockSbxLs(stdout: string) { + ;(globalThis as any).Bun.spawn.mockReturnValue({ + stdout: makeStream(stdout), + stderr: makeStream(''), + exited: Promise.resolve(), + exitCode: 0, + }) +} + describe('ensureSandboxExists', () => { - it('resolves when sandbox is found in sbx ls output', async () => { - ;(globalThis as any).Bun.spawn.mockReturnValue({ - stdout: makeStream('claude-skills-skillwalker\n'), - stderr: makeStream(''), - exited: Promise.resolve(), - exitCode: 0, - }) + it('resolves when sandbox is found in sbx ls --json output', async () => { + mockSbxLs(makeSandboxList([{ name: 'claude-skills-skillwalker', workspaces: ['/repo'] }])) const { ensureSandboxExists } = await import('./sandbox.js') await expect(ensureSandboxExists()).resolves.toBeUndefined() + expect((globalThis as any).Bun.spawn.mock.calls[0][0]).toEqual(['sbx', 'ls', '--json']) }) it('throws SandboxError when sandbox is not found', async () => { - ;(globalThis as any).Bun.spawn.mockReturnValue({ - stdout: makeStream('some-other-sandbox\n'), - stderr: makeStream(''), - exited: Promise.resolve(), - exitCode: 0, - }) + mockSbxLs(makeSandboxList([{ name: 'some-other-sandbox', workspaces: ['/repo'] }])) const { ensureSandboxExists } = await import('./sandbox.js') await expect(ensureSandboxExists()).rejects.toThrow(SandboxError) }) + it('throws the not-found SandboxError when sbx lists no sandboxes', async () => { + mockSbxLs(JSON.stringify({ sandboxes: null })) + + const { ensureSandboxExists } = await import('./sandbox.js') + await expect(ensureSandboxExists()).rejects.toThrow(/not found.*sandbox create/) + }) + + it('resolves when every required path is under a mounted workspace', async () => { + mockSbxLs( + makeSandboxList([{ name: 'claude-skills-skillwalker', workspaces: ['/target/repo', '/skillwalker/build'] }]), + ) + + const { ensureSandboxExists } = await import('./sandbox.js') + await expect(ensureSandboxExists(['/target/repo/plugins', '/skillwalker/build'])).resolves.toBeUndefined() + }) + + it('treats a workspace listed with a :ro suffix as mounting its path', async () => { + mockSbxLs( + makeSandboxList([{ name: 'claude-skills-skillwalker', workspaces: ['/target/repo', '/skillwalker/build:ro'] }]), + ) + + const { ensureSandboxExists } = await import('./sandbox.js') + await expect(ensureSandboxExists(['/skillwalker/build'])).resolves.toBeUndefined() + }) + + it('throws SandboxError naming the path and sandbox update when a required path is not mounted', async () => { + mockSbxLs(makeSandboxList([{ name: 'claude-skills-skillwalker', workspaces: ['/target/repo'] }])) + + const { ensureSandboxExists } = await import('./sandbox.js') + const result = ensureSandboxExists(['/skillwalker/build']) + + await expect(result).rejects.toThrow(SandboxError) + await expect(result).rejects.toThrow(/does not mount \/skillwalker\/build.*sandbox update/s) + }) + it('throws SandboxError when sbx ls exits non-zero', async () => { ;(globalThis as any).Bun.spawn.mockReturnValue({ stdout: makeStream(''), @@ -138,4 +177,47 @@ describe('execInSandbox', () => { expect(result.exitCode).toBe(1) }) + + it('throws SandboxError when sbx reports the command could not be started, despite exit code 0', async () => { + ;(globalThis as any).Bun.spawn.mockReturnValue({ + stdout: makeStream( + 'OCI runtime exec failed: executable file `/host/build/sandbox-run.sh` not found: No such file or directory\n', + ), + stderr: makeStream('Sandbox claude-skills-skillwalker started successfully\nINFO: Starting Docker daemon\n'), + exited: Promise.resolve(), + exitCode: 0, + }) + + const { execInSandbox } = await import('./sandbox.js') + const result = execInSandbox('/host/build/sandbox-run.sh', [], null, false) + + await expect(result).rejects.toThrow(SandboxError) + await expect(result).rejects.toThrow(/sandbox update/) + }) + + it('throws SandboxError when the exec failure is reported on stderr after other lines', async () => { + ;(globalThis as any).Bun.spawn.mockReturnValue({ + stdout: makeStream(''), + stderr: makeStream('INFO: Starting Docker daemon\nOCI runtime exec failed: executable file not found\n'), + exited: Promise.resolve(), + exitCode: 0, + }) + + const { execInSandbox } = await import('./sandbox.js') + await expect(execInSandbox('/path/to/script', [], null, false)).rejects.toThrow(SandboxError) + }) + + it('does not throw when the message appears mid-line in command output', async () => { + ;(globalThis as any).Bun.spawn.mockReturnValue({ + stdout: makeStream('{"text":"OCI runtime exec failed is a docker error"}\n'), + stderr: makeStream(''), + exited: Promise.resolve(), + exitCode: 0, + }) + + const { execInSandbox } = await import('./sandbox.js') + const result = await execInSandbox('/path/to/script', [], null, false) + + expect(result.exitCode).toBe(0) + }) }) diff --git a/packages/sandbox-integration/src/sandbox.ts b/packages/sandbox-integration/src/sandbox.ts index 5ea2e24..13ad62b 100644 --- a/packages/sandbox-integration/src/sandbox.ts +++ b/packages/sandbox-integration/src/sandbox.ts @@ -1,8 +1,16 @@ +import path from 'node:path' import { SandboxError } from './errors.js' import type { SandboxResult } from './types.js' export const SANDBOX_NAME = 'claude-skills-skillwalker' +// `sbx exec` prints this and still exits 0 when the command cannot be started, +// such as when its host path is not under one of the sandbox's workspaces. +const EXEC_FAILED_MESSAGE = 'OCI runtime exec failed' + +// Workspaces mounted read-only may be listed with the same suffix `sbx run` takes. +const READ_ONLY_SUFFIX = /:ro$/ + export function spawnSbx(args: string[], options: Parameters[1]) { try { return Bun.spawn(['sbx', ...args], options) @@ -17,8 +25,8 @@ export function spawnSbx(args: string[], options: Parameters[1 } } -export async function listSandboxNames(): Promise { - const proc = spawnSbx(['ls', '--quiet'], { stdout: 'pipe', stderr: 'pipe' }) +async function runSbxLs(args: string[]): Promise { + const proc = spawnSbx(['ls', ...args], { stdout: 'pipe', stderr: 'pipe' }) const [stdout, stderr] = await Promise.all([ new Response(proc.stdout as ReadableStream).text(), new Response(proc.stderr as ReadableStream).text(), @@ -33,17 +41,50 @@ export async function listSandboxNames(): Promise { } return stdout +} + +export async function listSandboxNames(): Promise { + return (await runSbxLs(['--quiet'])) .split('\n') .map((line) => line.trim()) .filter((line) => line.length > 0) } -export async function ensureSandboxExists(): Promise { - const sandboxes = await listSandboxNames() +interface SandboxListing { + name: string + workspaces: string[] +} - if (!sandboxes.includes(SANDBOX_NAME)) { +async function listSandboxes(): Promise { + const { sandboxes } = JSON.parse(await runSbxLs(['--json'])) as { sandboxes?: SandboxListing[] | null } + return sandboxes ?? [] +} + +export function isWithin(parent: string, child: string): boolean { + const relative = path.relative(parent, child) + return !relative.startsWith('..') && !path.isAbsolute(relative) +} + +/** + * Throws unless the sandbox exists and mounts every path in `requiredPaths`. + * A sandbox created before a mount was added keeps its original workspaces + * until it is recreated. + */ +export async function ensureSandboxExists(requiredPaths: string[] = []): Promise { + const sandbox = (await listSandboxes()).find(({ name }) => name === SANDBOX_NAME) + + if (!sandbox) { throw new SandboxError(`Sandbox "${SANDBOX_NAME}" not found. Run './build/skillwalker sandbox create' first.`, null) } + + for (const requiredPath of requiredPaths) { + if (!sandbox.workspaces.some((workspace) => isWithin(workspace.replace(READ_ONLY_SUFFIX, ''), requiredPath))) { + throw new SandboxError( + `Sandbox "${SANDBOX_NAME}" does not mount ${requiredPath}.\nRun \`skillwalker sandbox update\` from the target repo to recreate it.`, + null, + ) + } + } } function isMissingExecutableError(error: unknown): boolean { @@ -51,6 +92,10 @@ function isMissingExecutableError(error: unknown): boolean { return (error as Error & { code?: string }).code === 'ENOENT' } +function reportsExecFailure(output: string): boolean { + return output.split('\n').some((line) => line.startsWith(EXEC_FAILED_MESSAGE)) +} + export async function execInSandbox( command: string, args: string[], @@ -77,5 +122,13 @@ export async function execInSandbox( if (debug && stderr) process.stderr.write(stderr) await proc.exited + + if (reportsExecFailure(stdout) || reportsExecFailure(stderr)) { + throw new SandboxError( + `Unable to run ${command} in sandbox "${SANDBOX_NAME}": ${`${stdout}${stderr}`.trim()}\nThe sandbox must mount the directory holding this file. Run \`skillwalker sandbox update\` from the target repo to recreate it with both mounts.`, + proc.exitCode, + ) + } + return { exitCode: proc.exitCode ?? 1, stdout, stderr } }