Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/server/src/agents/AcpAdapter.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { spawn as cpSpawn, type ChildProcess } from 'node:child_process';
import { Readable, Writable } from 'node:stream';
import { randomUUID } from 'node:crypto';
import { dirname, resolve, join } from 'node:path';

Check warning on line 4 in packages/server/src/agents/AcpAdapter.ts

View workflow job for this annotation

GitHub Actions / lint

'join' is defined but never used. Allowed unused vars must match /^_/u
import { fileURLToPath } from 'node:url';
import { existsSync } from 'node:fs';

Expand Down Expand Up @@ -233,7 +233,7 @@
session.onOutputChunk?.(update);
self.onAnySessionOutput?.(session.agentId, update);
if (self.onToolCall) {
let toolName = (update as any).title ?? (update as any).name ?? (update as any).toolName ?? 'unknown';

Check warning on line 236 in packages/server/src/agents/AcpAdapter.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type

Check warning on line 236 in packages/server/src/agents/AcpAdapter.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type

Check failure on line 236 in packages/server/src/agents/AcpAdapter.ts

View workflow job for this annotation

GitHub Actions / lint

'toolName' is never reassigned. Use 'const' instead
self.onToolCall(session.agentId, { toolName, status: 'completed' });
}
break;
Expand Down Expand Up @@ -293,7 +293,9 @@
// --- Terminal capabilities (Client provides these to the Agent) ---

async createTerminal(params: CreateTerminalRequest): Promise<CreateTerminalResponse> {
// Lead should not run arbitrary commands — only read operations
// Lead has no permission to run programs and no sub-agents, but it MAY
// read files for context. Allow only read-only commands; deny everything
// else (execution, writes, sub-agents). Delegate the rest to the Director.
if (session.role === 'lead') {
const cmd = params.command;
const cmdLower = cmd.toLowerCase();
Expand All @@ -306,9 +308,11 @@
const firstToken = cmdLower.trim().split(/\s+/)[0];
// Handle absolute paths like /usr/bin/cat → cat
const cmdBasename = firstToken.split('/').pop() ?? firstToken;
// Read-only commands only. Lead can read files for context, but cannot
// run programs, write files, or spawn sub-agents.
const allowedLeadCmds = ['cat', 'ls', 'find', 'grep', 'head', 'tail', 'wc', 'echo', 'flightdeck'];
if (!allowedLeadCmds.includes(cmdBasename)) {
throw new Error(`Role 'lead' cannot run '${params.command}'. Lead agents can only run read-only commands. Delegate implementation to worker agents.`);
throw new Error(`Role 'lead' cannot run '${params.command}'. Lead has no permission to run programs or spawn sub-agents — only read-only file commands are allowed. Delegate execution to the Director via flightdeck_send.`);
}
}
const termId = `term-${randomUUID().slice(0, 8)}`;
Expand Down
57 changes: 39 additions & 18 deletions packages/server/src/agents/CopilotSdkAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,41 @@
sawReasoningDelta?: boolean;
}

/**
* Native Copilot CLI tools that must be excluded for a given role.
*
* - Management roles (lead, director, scout) never get write/execute tools —
* they coordinate, they don't modify the workspace.
* - The Lead is additionally barred from running programs and from spawning
* sub-agents: it has no execution permission and no sub-agent capability.
* Anything requiring execution must be delegated to the Director.
*/
export function excludedNativeToolsForRole(role: string): string[] | undefined {
const isManagement = ['lead', 'director', 'scout'].includes(role);
if (!isManagement) return undefined;

const tools = [
// Write/execute — legacy tool names (Copilot CLI <1.0.40)
'bash', 'str_replace_editor', 'write_file', 'apply_patch',
'insert_edit_into_file', 'create_file', 'delete_file',
// Write/execute — new tool names (Copilot CLI >=1.0.40)
'write_bash', 'create', 'edit',
];

if (role === 'lead') {
// Lead has no run permission and no sub-agents. Exclude every shell/exec
// and sub-agent launcher tool name across CLI versions.
tools.push(
// Run / shell tools
'shell', 'exec', 'run_in_terminal', 'read_bash', 'stop_bash',
// Sub-agent / task delegation launchers
'task', 'subagent', 'run_subagent', 'launch_agent', 'spawn_agent',
);
}

return Array.from(new Set(tools));
}

export class CopilotSdkAdapter extends AgentAdapter {
readonly runtime: AgentRuntime = 'acp';
private client: CopilotClient | null = null;
Expand Down Expand Up @@ -1075,16 +1110,9 @@
// Format: fd-{agentId} — deterministic, maps 1:1 to the agent.
const sessionId = `fd-${aid}`;

// For management roles (lead, director, scout), exclude write/execute tools
// They can still read files for context, but cannot modify anything
const isManagement = ['lead', 'director', 'scout'].includes(opts.role);
const excludedNativeTools = isManagement ? [
// Legacy tool names (Copilot CLI <1.0.40)
'bash', 'str_replace_editor', 'write_file', 'apply_patch',
'insert_edit_into_file', 'create_file', 'delete_file',
// New tool names (Copilot CLI >=1.0.40)
'write_bash', 'create', 'edit',
] : undefined;
// For management roles (lead, director, scout), exclude write/execute tools.
// The Lead is further restricted: no run permission and no sub-agents.
const excludedNativeTools = excludedNativeToolsForRole(opts.role);

const sessionConfig: SessionConfig = {
sessionId,
Expand Down Expand Up @@ -1245,7 +1273,7 @@
// without calling it, every steer() leaks a handler that keeps firing
// on all future session events.
await new Promise<void>((resolve) => {
let timer: ReturnType<typeof setTimeout> | undefined;

Check failure on line 1276 in packages/server/src/agents/CopilotSdkAdapter.ts

View workflow job for this annotation

GitHub Actions / lint

'timer' is never reassigned. Use 'const' instead
const unsubscribe = agentSession.session.on((event: SessionEvent) => {
if (event.type === 'session.idle') {
if (timer) clearTimeout(timer);
Expand Down Expand Up @@ -1306,14 +1334,7 @@
const aid = (opts.agentId ?? makeAgentId(opts.role, Date.now().toString())) as AgentId;
const tools = this.buildTools(aid, opts.role as AgentRole, opts.projectName);

const isManagement = ['lead', 'director', 'scout'].includes(opts.role);
const excludedNativeTools = isManagement ? [
// Legacy tool names (Copilot CLI <1.0.40)
'bash', 'str_replace_editor', 'write_file', 'apply_patch',
'insert_edit_into_file', 'create_file', 'delete_file',
// New tool names (Copilot CLI >=1.0.40)
'write_bash', 'create', 'edit',
] : undefined;
const excludedNativeTools = excludedNativeToolsForRole(opts.role);

const session = await client.resumeSession(opts.previousSessionId, {
model: opts.model ?? this.defaultModel,
Expand Down
52 changes: 51 additions & 1 deletion packages/server/tests/agents/copilot-sdk-adapter.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { CopilotSdkAdapter } from '../../src/agents/CopilotSdkAdapter.js';
import { CopilotSdkAdapter, excludedNativeToolsForRole } from '../../src/agents/CopilotSdkAdapter.js';

const mockFetch = vi.fn();

Expand Down Expand Up @@ -29,6 +29,56 @@ describe('CopilotSdkAdapter', () => {
return getAllTools(role).map((t: any) => t.name);
}

// ─── Excluded Native Tools (run permission + sub-agents) ────

describe('excluded native tools by role', () => {
it('lead has no run permission (shell/exec tools excluded)', () => {
const excluded = excludedNativeToolsForRole('lead')!;
for (const t of ['bash', 'write_bash', 'shell', 'exec', 'run_in_terminal']) {
expect(excluded).toContain(t);
}
});

it('lead has no sub-agents (task/subagent launchers excluded)', () => {
const excluded = excludedNativeToolsForRole('lead')!;
for (const t of ['task', 'subagent', 'run_subagent', 'launch_agent', 'spawn_agent']) {
expect(excluded).toContain(t);
}
});

it('lead still excludes write/edit tools', () => {
const excluded = excludedNativeToolsForRole('lead')!;
expect(excluded).toContain('write_file');
expect(excluded).toContain('edit');
});

it('lead keeps read tools (can read files for context)', () => {
const excluded = excludedNativeToolsForRole('lead')!;
for (const t of ['view', 'read_file', 'grep', 'glob']) {
expect(excluded).not.toContain(t);
}
});

it('director excludes write/execute but is not stripped of sub-agents', () => {
const excluded = excludedNativeToolsForRole('director')!;
expect(excluded).toContain('bash');
expect(excluded).toContain('write_bash');
expect(excluded).not.toContain('task');
expect(excluded).not.toContain('subagent');
expect(excluded).not.toContain('shell');
});

it('non-management roles get no exclusions', () => {
expect(excludedNativeToolsForRole('worker')).toBeUndefined();
expect(excludedNativeToolsForRole('reviewer')).toBeUndefined();
});

it('returns a de-duplicated list', () => {
const excluded = excludedNativeToolsForRole('lead')!;
expect(excluded.length).toBe(new Set(excluded).size);
});
});

// ─── Role-Based Tool Gating ─────────────────────────────────

describe('role-based tool gating', () => {
Expand Down
7 changes: 4 additions & 3 deletions packages/server/tests/agents/lead-command-allowlist.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import { describe, it, expect } from 'vitest';

/**
* Tests for lead role command allowlist validation.
* Extracted logic mirrors AcpAdapter.ts createTerminal guard.
* The Lead has no run/sub-agent permission, but MAY read files for context.
* Mirrors AcpAdapter.ts createTerminal guard for role 'lead'.
*/

const allowedLeadCmds = ['cat', 'ls', 'find', 'grep', 'head', 'tail', 'wc', 'echo', 'flightdeck'];
Expand All @@ -22,7 +23,7 @@ function validateLeadCommand(command: string): { allowed: boolean; reason?: stri
}

describe('Lead role command allowlist', () => {
describe('allowed commands', () => {
describe('read-only commands are allowed (Lead can read files)', () => {
it.each([
'cat file.txt',
'ls -la',
Expand Down Expand Up @@ -60,7 +61,7 @@ describe('Lead role command allowlist', () => {
});
});

describe('disallowed commands', () => {
describe('non-read-only / execution commands - rejected', () => {
it.each([
'rm -rf /',
'chmod 777 file',
Expand Down
Loading