-
Notifications
You must be signed in to change notification settings - Fork 0
fix: inline dependabot auto-merge; test pi + docker-sandbox executors #59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import type { Task, ExecutionEventCreateInput } from '@djimitflo/shared'; | ||
| import { ExecutionEventType, LogLevel } from '@djimitflo/shared'; | ||
| import type { TaskExecutor, ExecutionSession, ExecutorKind, ExecutorOptions } from '../execution/types'; | ||
| import { | ||
| DockerSandboxExecutor, | ||
| DEFAULT_SANDBOX_CONFIG, | ||
| type DockerSandboxConfig, | ||
| } from '../execution/executors/docker-sandbox-executor'; | ||
|
|
||
| function makeTask(overrides: Partial<Task> = {}): Task { | ||
| return { | ||
| id: 'test-task-id', | ||
| title: 'Test task', | ||
| description: 'echo hello world', | ||
| status: 'pending' as any, | ||
| priority: 'medium' as any, | ||
| risk_level: 'low' as any, | ||
| execution_mode: 'local' as any, | ||
| agent_id: null, | ||
| parent_task_id: null, | ||
| repository_id: null, | ||
| instruction_profile_id: null, | ||
| started_at: null, | ||
| completed_at: null, | ||
| failed_at: null, | ||
| execution_time_ms: null, | ||
| token_usage: null, | ||
| tags: [], | ||
| metadata: {}, | ||
| created_at: new Date().toISOString(), | ||
| updated_at: new Date().toISOString(), | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| const testConfig: DockerSandboxConfig = { | ||
| image: 'test-image:latest', | ||
| cpuLimit: '2.0', | ||
| memoryLimit: '256m', | ||
| networkMode: 'none', | ||
| timeoutMs: 60000, | ||
| readOnlyRoot: true, | ||
| bindMounts: [], | ||
| }; | ||
|
|
||
| /** Fast inner executor stub — MockExecutor sleeps between events, this one doesn't. */ | ||
| class StubExecutor implements TaskExecutor { | ||
| readonly kind: ExecutorKind = 'mock'; | ||
| lastEnvironment: Record<string, string> | undefined; | ||
|
|
||
| canExecute(task: Task): boolean { | ||
| return task.risk_level !== ('critical' as any); | ||
| } | ||
|
|
||
| async start(task: Task, options?: ExecutorOptions): Promise<ExecutionSession> { | ||
| this.lastEnvironment = options?.environment; | ||
| const events = (async function* (): AsyncIterable<ExecutionEventCreateInput> { | ||
| yield { | ||
| task_id: task.id, | ||
| event_type: ExecutionEventType.LOG, | ||
| message: 'inner event', | ||
| level: LogLevel.INFO, | ||
| metadata: {}, | ||
| }; | ||
| })(); | ||
| return { | ||
| id: 'inner-session', | ||
| taskId: task.id, | ||
| executorKind: this.kind, | ||
| status: 'running', | ||
| startedAt: new Date(), | ||
| events, | ||
| result: Promise.resolve({ status: 'completed', message: 'inner done' }), | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| describe('DockerSandboxExecutor.buildDockerArgs', () => { | ||
| it('applies resource limits, network isolation, and read-only root with tmpfs', () => { | ||
| const args = DockerSandboxExecutor.buildDockerArgs('sandbox-1', testConfig, 'node', ['script.js']); | ||
|
|
||
| expect(args.slice(0, 4)).toEqual(['run', '--rm', '--name', 'sandbox-1']); | ||
| expect(args).toContain('--cpus'); | ||
| expect(args[args.indexOf('--cpus') + 1]).toBe('2.0'); | ||
| expect(args).toContain('--memory'); | ||
| expect(args[args.indexOf('--memory') + 1]).toBe('256m'); | ||
| expect(args).toContain('--network'); | ||
| expect(args[args.indexOf('--network') + 1]).toBe('none'); | ||
| expect(args).toContain('--read-only'); | ||
| expect(args).toContain('--tmpfs'); | ||
| expect(args[args.indexOf('--tmpfs') + 1]).toBe('/tmp:size=64m'); | ||
|
Comment on lines
+85
to
+92
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assert that the flags exist in the expect(args).toContain('--cpus');
expect(args[args.indexOf('--cpus') + 1]).toBe('2.0');
expect(args).toContain('--memory');
expect(args[args.indexOf('--memory') + 1]).toBe('256m');
expect(args).toContain('--network');
expect(args[args.indexOf('--network') + 1]).toBe('none');
expect(args).toContain('--read-only');
expect(args).toContain('--tmpfs');
expect(args[args.indexOf('--tmpfs') + 1]).toBe('/tmp:size=64m'); |
||
| // image, then command and its args, close the invocation | ||
| expect(args.slice(-3)).toEqual(['test-image:latest', 'node', 'script.js']); | ||
| }); | ||
|
|
||
| it('omits --read-only when readOnlyRoot is disabled', () => { | ||
| const args = DockerSandboxExecutor.buildDockerArgs('sandbox-1', { ...testConfig, readOnlyRoot: false }, 'sh', []); | ||
| expect(args).not.toContain('--read-only'); | ||
| expect(args).not.toContain('--tmpfs'); | ||
| }); | ||
|
|
||
| it('mounts the working directory rw at /workspace and applies extra bind mounts', () => { | ||
| const config = { | ||
| ...testConfig, | ||
| bindMounts: [{ host: '/host/cache', container: '/cache', mode: 'ro' as const }], | ||
| }; | ||
| const args = DockerSandboxExecutor.buildDockerArgs('sandbox-1', config, 'sh', [], '/host/project'); | ||
|
|
||
| expect(args).toContain('/host/project:/workspace:rw'); | ||
| expect(args).toContain('-w'); | ||
| expect(args[args.indexOf('-w') + 1]).toBe('/workspace'); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| expect(args).toContain('/host/cache:/cache:ro'); | ||
| }); | ||
|
|
||
| it('forwards env vars but never the __-prefixed internal markers', () => { | ||
| const args = DockerSandboxExecutor.buildDockerArgs('sandbox-1', testConfig, 'sh', [], undefined, { | ||
| MY_VAR: 'value', | ||
| __DOCKER_SANDBOX_ENABLED: 'true', | ||
| }); | ||
|
|
||
| expect(args).toContain('MY_VAR=value'); | ||
| expect(args.join(' ')).not.toContain('__DOCKER_SANDBOX_ENABLED'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('DockerSandboxExecutor wrapper', () => { | ||
| // ponytail: dockerPath 'true' — cleanupContainer spawns a no-op instead of the docker CLI | ||
| const makeSandbox = (inner: TaskExecutor) => new DockerSandboxExecutor(inner, testConfig, 'true'); | ||
|
|
||
| it('mirrors the inner executor kind and delegates canExecute', () => { | ||
| const inner = new StubExecutor(); | ||
| const sandbox = makeSandbox(inner); | ||
|
|
||
| expect(sandbox.kind).toBe('mock'); | ||
| expect(sandbox.canExecute(makeTask())).toBe(true); | ||
| expect(sandbox.canExecute(makeTask({ risk_level: 'critical' as any }))).toBe(false); | ||
| }); | ||
|
|
||
| it('marks the inner environment as sandboxed and brackets events with sandbox lifecycle', async () => { | ||
| const inner = new StubExecutor(); | ||
| const sandbox = makeSandbox(inner); | ||
| const task = makeTask(); | ||
|
|
||
| const session = await sandbox.start(task); | ||
| expect(inner.lastEnvironment?.__DOCKER_SANDBOX_ENABLED).toBe('true'); | ||
|
|
||
| const events: ExecutionEventCreateInput[] = []; | ||
| for await (const event of session.events) events.push(event); | ||
|
|
||
| expect(events[0].message).toContain('Docker sandbox started'); | ||
| expect(events[0].metadata).toMatchObject({ sandbox: 'docker', image: 'test-image:latest', network: 'none' }); | ||
| expect(events[1].message).toBe('inner event'); | ||
| expect(events[events.length - 1].message).toContain('Docker sandbox cleaned up'); | ||
| }); | ||
|
|
||
| it('passes the inner result through', async () => { | ||
| const sandbox = makeSandbox(new StubExecutor()); | ||
| const session = await sandbox.start(makeTask()); | ||
| const result = await session.result; | ||
|
|
||
| expect(result.status).toBe('completed'); | ||
| expect(result.message).toBe('inner done'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('DEFAULT_SANDBOX_CONFIG', () => { | ||
| it('is default-deny on network and read-only on root', () => { | ||
| expect(DEFAULT_SANDBOX_CONFIG.networkMode).toBe('none'); | ||
| expect(DEFAULT_SANDBOX_CONFIG.readOnlyRoot).toBe(true); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,180 @@ | ||||||||
| import { afterEach, beforeEach, describe, expect, it } from 'vitest'; | ||||||||
| import type { Task } from '@djimitflo/shared'; | ||||||||
| import { ExecutionEventType, LogLevel } from '@djimitflo/shared'; | ||||||||
| import { PiExecutor } from '../execution/executors/pi-executor'; | ||||||||
| import { buildPiArgs, mapPiEvent } from '../execution/executors/pi-shared'; | ||||||||
|
|
||||||||
| function makeTask(overrides: Partial<Task> = {}): Task { | ||||||||
| return { | ||||||||
| id: 'test-task-id', | ||||||||
| title: 'Test task', | ||||||||
| description: 'echo hello world', | ||||||||
| status: 'pending' as any, | ||||||||
| priority: 'medium' as any, | ||||||||
| risk_level: 'low' as any, | ||||||||
| execution_mode: 'local' as any, | ||||||||
| agent_id: null, | ||||||||
| parent_task_id: null, | ||||||||
| repository_id: null, | ||||||||
| instruction_profile_id: null, | ||||||||
| started_at: null, | ||||||||
| completed_at: null, | ||||||||
| failed_at: null, | ||||||||
| execution_time_ms: null, | ||||||||
| token_usage: null, | ||||||||
| tags: [], | ||||||||
| metadata: {}, | ||||||||
| created_at: new Date().toISOString(), | ||||||||
| updated_at: new Date().toISOString(), | ||||||||
| ...overrides, | ||||||||
| }; | ||||||||
| } | ||||||||
|
|
||||||||
| const PI_ENV_KEYS = [ | ||||||||
| 'PI_NO_APPROVE', 'PI_NO_CONTEXT_FILES', 'PI_NO_EXTENSIONS', 'PI_NO_SKILLS', | ||||||||
| 'PI_OFFLINE', 'PI_PROVIDER', 'PI_MODEL', 'PI_THINKING', 'PI_TOOLS', 'PI_EXCLUDE_TOOLS', | ||||||||
| ]; | ||||||||
|
|
||||||||
| describe('buildPiArgs', () => { | ||||||||
| const previousEnv = { ...process.env }; | ||||||||
|
|
||||||||
| beforeEach(() => { | ||||||||
| for (const key of PI_ENV_KEYS) delete process.env[key]; | ||||||||
| }); | ||||||||
|
|
||||||||
| afterEach(() => { | ||||||||
| for (const key of Object.keys(process.env)) { | ||||||||
| if (!(key in previousEnv)) delete process.env[key]; | ||||||||
| } | ||||||||
| Object.assign(process.env, previousEnv); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('builds the headless json invocation with deterministic flags on by default', () => { | ||||||||
| const args = buildPiArgs(makeTask({ description: 'test prompt' })); | ||||||||
|
|
||||||||
| expect(args.slice(0, 4)).toEqual(['--mode', 'json', '-p', '--no-session']); | ||||||||
| // default-on hygiene flags (PI_NO_APPROVE / PI_NO_EXTENSIONS / PI_NO_SKILLS default to '1') | ||||||||
| expect(args).toContain('--no-approve'); | ||||||||
| expect(args).toContain('--no-extensions'); | ||||||||
| expect(args).toContain('--no-skills'); | ||||||||
| // default-off flags | ||||||||
| expect(args).not.toContain('--no-context-files'); | ||||||||
| expect(args).not.toContain('--offline'); | ||||||||
| // prompt is the trailing positional | ||||||||
| expect(args[args.length - 1]).toBe('test prompt'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('allows disabling the default-on flags via env', () => { | ||||||||
| process.env.PI_NO_APPROVE = '0'; | ||||||||
| process.env.PI_NO_EXTENSIONS = '0'; | ||||||||
| process.env.PI_NO_SKILLS = '0'; | ||||||||
| const args = buildPiArgs(makeTask()); | ||||||||
|
|
||||||||
| expect(args).not.toContain('--no-approve'); | ||||||||
| expect(args).not.toContain('--no-extensions'); | ||||||||
| expect(args).not.toContain('--no-skills'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('prefers options.model over PI_MODEL env', () => { | ||||||||
| process.env.PI_MODEL = 'env-model'; | ||||||||
| const optArgs = buildPiArgs(makeTask(), { model: 'option-model' }); | ||||||||
| expect(optArgs).toContain('--model'); | ||||||||
| expect(optArgs[optArgs.indexOf('--model') + 1]).toBe('option-model'); | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assert that the
Suggested change
|
||||||||
|
|
||||||||
| const envArgs = buildPiArgs(makeTask()); | ||||||||
| expect(envArgs).toContain('--model'); | ||||||||
| expect(envArgs[envArgs.indexOf('--model') + 1]).toBe('env-model'); | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assert that the
Suggested change
|
||||||||
|
|
||||||||
| delete process.env.PI_MODEL; | ||||||||
| expect(buildPiArgs(makeTask())).not.toContain('--model'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('passes provider, thinking, tools allowlist, and offline flag from env', () => { | ||||||||
| process.env.PI_PROVIDER = 'ollama'; | ||||||||
| process.env.PI_THINKING = 'high'; | ||||||||
| process.env.PI_TOOLS = 'read,ls'; | ||||||||
| process.env.PI_EXCLUDE_TOOLS = 'bash'; | ||||||||
| process.env.PI_OFFLINE = '1'; | ||||||||
| const args = buildPiArgs(makeTask()); | ||||||||
|
|
||||||||
| expect(args).toContain('--provider'); | ||||||||
| expect(args[args.indexOf('--provider') + 1]).toBe('ollama'); | ||||||||
| expect(args).toContain('--thinking'); | ||||||||
| expect(args[args.indexOf('--thinking') + 1]).toBe('high'); | ||||||||
| expect(args).toContain('--tools'); | ||||||||
| expect(args[args.indexOf('--tools') + 1]).toBe('read,ls'); | ||||||||
| expect(args).toContain('--exclude-tools'); | ||||||||
| expect(args[args.indexOf('--exclude-tools') + 1]).toBe('bash'); | ||||||||
|
Comment on lines
+101
to
+107
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assert that the flags exist in the expect(args).toContain('--provider');
expect(args[args.indexOf('--provider') + 1]).toBe('ollama');
expect(args).toContain('--thinking');
expect(args[args.indexOf('--thinking') + 1]).toBe('high');
expect(args).toContain('--tools');
expect(args[args.indexOf('--tools') + 1]).toBe('read,ls');
expect(args).toContain('--exclude-tools');
expect(args[args.indexOf('--exclude-tools') + 1]).toBe('bash'); |
||||||||
| expect(args).toContain('--offline'); | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
| describe('mapPiEvent', () => { | ||||||||
| const metrics = () => ({ tokenUsage: 0, toolCalls: 0, approvalsRequested: 0 }); | ||||||||
|
|
||||||||
| it('maps the lifecycle events to execution events', () => { | ||||||||
| const start = mapPiEvent('t1', { type: 'agent_start' }, metrics()); | ||||||||
| expect(start?.event_type).toBe(ExecutionEventType.TASK_STARTED); | ||||||||
|
|
||||||||
| const end = mapPiEvent('t1', { type: 'agent_end' }, metrics()); | ||||||||
| expect(end?.event_type).toBe(ExecutionEventType.TASK_COMPLETED); | ||||||||
|
|
||||||||
| const session = mapPiEvent('t1', { type: 'session', id: 's1', version: 3, cwd: '/w' }, metrics()); | ||||||||
| expect(session?.event_type).toBe(ExecutionEventType.LOG); | ||||||||
| expect(session?.metadata).toMatchObject({ pi_session_id: 's1', cwd: '/w' }); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('maps tool execution events and flags tool errors', () => { | ||||||||
| const call = mapPiEvent('t1', { type: 'tool_execution_start', toolName: 'ls', toolCallId: 'c1' }, metrics()); | ||||||||
| expect(call?.event_type).toBe(ExecutionEventType.TOOL_CALL); | ||||||||
| expect(call?.metadata).toMatchObject({ tool_name: 'ls' }); | ||||||||
|
|
||||||||
| const ok = mapPiEvent('t1', { type: 'tool_execution_end', toolName: 'ls', isError: false }, metrics()); | ||||||||
| expect(ok?.event_type).toBe(ExecutionEventType.TOOL_RESULT); | ||||||||
| expect(ok?.level).toBe(LogLevel.INFO); | ||||||||
|
|
||||||||
| const failed = mapPiEvent('t1', { type: 'tool_execution_end', toolName: 'bash', isError: true }, metrics()); | ||||||||
| expect(failed?.level).toBe(LogLevel.ERROR); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('returns null for events that should not reach the audit trail', () => { | ||||||||
| expect(mapPiEvent('t1', { type: 'message_update' }, metrics())).toBeNull(); | ||||||||
| expect(mapPiEvent('t1', { type: 'turn_start' }, metrics())).toBeNull(); | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
| describe('PiExecutor', () => { | ||||||||
| it('parses only valid typed JSON lines', () => { | ||||||||
| const executor = new PiExecutor('/usr/bin/pi'); | ||||||||
| expect((executor as any).parseJsonEvent('{"type":"agent_start"}')).toMatchObject({ type: 'agent_start' }); | ||||||||
| expect((executor as any).parseJsonEvent('{"no_type":true}')).toBeNull(); | ||||||||
| expect((executor as any).parseJsonEvent('not json at all')).toBeNull(); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('captures token usage from assistant message_end and maps toolResult messages', () => { | ||||||||
| const executor = new PiExecutor('/usr/bin/pi'); | ||||||||
| const metrics = { tokenUsage: 0, toolCalls: 0, approvalsRequested: 0 }; | ||||||||
|
|
||||||||
| const logEvent = (executor as any).mapJsonEventToExecutionEvent('t1', { | ||||||||
| type: 'message_end', | ||||||||
| message: { role: 'assistant', content: [{ type: 'text', text: 'done' }], usage: { totalTokens: 123 } }, | ||||||||
| }, metrics); | ||||||||
| expect(metrics.tokenUsage).toBe(123); | ||||||||
| expect(logEvent?.event_type).toBe(ExecutionEventType.LOG); | ||||||||
| expect(logEvent?.message).toBe('done'); | ||||||||
|
|
||||||||
| const toolResult = (executor as any).mapJsonEventToExecutionEvent('t1', { | ||||||||
| type: 'message_end', | ||||||||
| message: { role: 'toolResult', toolName: 'read', content: [{ type: 'text', text: 'file contents' }] }, | ||||||||
| }, metrics); | ||||||||
| expect(toolResult?.event_type).toBe(ExecutionEventType.TOOL_RESULT); | ||||||||
| expect(toolResult?.tool_output).toBe('file contents'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('classifies stderr and error-like lines in heuristic fallback', () => { | ||||||||
| const executor = new PiExecutor('/usr/bin/pi'); | ||||||||
| expect((executor as any).parseHeuristicLine('anything', 'stderr').event_type).toBe(ExecutionEventType.ERROR); | ||||||||
| expect((executor as any).parseHeuristicLine('operation failed', 'stdout').event_type).toBe(ExecutionEventType.ERROR); | ||||||||
| expect((executor as any).parseHeuristicLine('all good', 'stdout').event_type).toBe(ExecutionEventType.LOG); | ||||||||
| }); | ||||||||
| }); | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this workflow still runs on
pull_request, the newgh pr merge --autostep will execute on Dependabot PRs with a read-only token and fail instead of enabling auto-merge. The official dependabot/fetch-metadata README notes that write automation should usepull_request_targetorworkflow_run, and its auto-merge example usespull_request_targetwithcontents: writeandpull-requests: write.Useful? React with 👍 / 👎.