Browse Source

fix: make council agents read-only

Berserk Agent 1 month ago
parent
commit
d952c96aa1

+ 6 - 1
src/agents/council.ts

@@ -1,6 +1,7 @@
 import { READONLY_FILE_OPERATIONS_RULES } from '../config';
 import { shortModelLabel } from '../utils/session';
 import { type AgentDefinition, resolvePrompt } from './orchestrator';
+import { createReadOnlyAgentPermission } from './permissions';
 
 // NOTE: Councillor system prompts live in the councillor agent factory.
 // The format functions below only structure the USER message content — the
@@ -9,7 +10,7 @@ import { type AgentDefinition, resolvePrompt } from './orchestrator';
 const COUNCIL_AGENT_PROMPT = `You are the Council agent — a multi-LLM \
 orchestration system that runs consensus across multiple models.
 
-**Tool**: You have access to the \`council_session\` tool.
+**Tool**: You have access to the \`council_session\` tool. You also have read-only codebase inspection tools. You do not have write, edit, shell, or subagent-delegation tools.
 
 **When to use**:
 - When invoked by a user with a request
@@ -87,6 +88,10 @@ export function createCouncilAgent(
     config: {
       temperature: 0.1,
       prompt,
+      permission: {
+        ...createReadOnlyAgentPermission(),
+        council_session: 'allow',
+      },
     },
   };
 

+ 13 - 2
src/agents/councillor.test.ts

@@ -94,9 +94,20 @@ describe('councillor permissions', () => {
     expect(permission.ast_grep_search).toBe('allow');
   });
 
-  test('has exactly 9 permission entries', () => {
+  test('denies mutating and delegation tools explicitly', () => {
     const agent = createCouncillorAgent('test-model');
     const permission = agent.config.permission as Record<string, string>;
-    expect(Object.keys(permission)).toHaveLength(9);
+    expect(permission.bash).toBe('deny');
+    expect(permission.edit).toBe('deny');
+    expect(permission.write).toBe('deny');
+    expect(permission.apply_patch).toBe('deny');
+    expect(permission.ast_grep_replace).toBe('deny');
+    expect(permission.task).toBe('deny');
+  });
+
+  test('has exactly 15 permission entries', () => {
+    const agent = createCouncillorAgent('test-model');
+    const permission = agent.config.permission as Record<string, string>;
+    expect(Object.keys(permission)).toHaveLength(15);
   });
 });

+ 3 - 12
src/agents/councillor.ts

@@ -1,5 +1,6 @@
 import { NO_SHELL_READONLY_FILE_OPERATIONS_RULES } from '../config';
 import { type AgentDefinition, resolvePrompt } from './orchestrator';
+import { createReadOnlyAgentPermission } from './permissions';
 
 /**
  * Councillor agent — a read-only advisor in the multi-LLM council.
@@ -69,18 +70,8 @@ export function createCouncillorAgent(
       model,
       temperature: 0.2,
       prompt,
-      // Mirror OpenCode's explore agent: deny all, then allow read-only tools
-      permission: {
-        '*': 'deny',
-        question: 'deny',
-        read: 'allow',
-        glob: 'allow',
-        grep: 'allow',
-        lsp: 'allow',
-        list: 'allow',
-        codesearch: 'allow',
-        ast_grep_search: 'allow',
-      },
+      // Strict read-only allowlist: deny all, then allow inspection tools only.
+      permission: createReadOnlyAgentPermission(),
     },
   };
 }

+ 34 - 5
src/agents/index.test.ts

@@ -312,14 +312,43 @@ describe('tool permissions', () => {
     expect((councillor?.config.permission as any).council_session).toBe('deny');
   });
 
-  test('subagents are denied access to cancel_task', () => {
+  test('council agent is read-only except council_session', () => {
     const agents = createAgents({
       council: councilConfig(),
     });
-    for (const name of ['oracle', 'explorer', 'fixer', 'council']) {
-      const agent = agents.find((a) => a.name === name);
-      expect((agent?.config.permission as any).cancel_task).toBe('deny');
-    }
+    const council = agents.find((a) => a.name === 'council');
+    const permission = council?.config.permission as Record<string, string>;
+    expect(permission['*']).toBe('deny');
+    expect(permission.read).toBe('allow');
+    expect(permission.glob).toBe('allow');
+    expect(permission.grep).toBe('allow');
+    expect(permission.ast_grep_search).toBe('allow');
+    expect(permission.council_session).toBe('allow');
+    expect(permission.bash).toBe('deny');
+    expect(permission.edit).toBe('deny');
+    expect(permission.write).toBe('deny');
+    expect(permission.apply_patch).toBe('deny');
+    expect(permission.ast_grep_replace).toBe('deny');
+    expect(permission.task).toBe('deny');
+  });
+
+  test('councillor remains read-only after default permissions are applied', () => {
+    const agents = createAgents({
+      council: councilConfig(),
+    });
+    const councillor = agents.find((a) => a.name === 'councillor');
+    const permission = councillor?.config.permission as Record<string, string>;
+    expect(permission['*']).toBe('deny');
+    expect(permission.read).toBe('allow');
+    expect(permission.glob).toBe('allow');
+    expect(permission.grep).toBe('allow');
+    expect(permission.council_session).toBe('deny');
+    expect(permission.bash).toBe('deny');
+    expect(permission.edit).toBe('deny');
+    expect(permission.write).toBe('deny');
+    expect(permission.apply_patch).toBe('deny');
+    expect(permission.ast_grep_replace).toBe('deny');
+    expect(permission.task).toBe('deny');
   });
 });
 

+ 31 - 0
src/agents/permissions.ts

@@ -0,0 +1,31 @@
+type AgentPermission = Record<
+  string,
+  'allow' | 'ask' | 'deny' | Record<string, 'allow' | 'ask' | 'deny'>
+>;
+
+/**
+ * Strict read-only tool permissions for advisory agents.
+ *
+ * Start with wildcard deny so newly-added tools are unavailable by default,
+ * then allow only inspection/search tools. Explicitly deny known mutating and
+ * delegation tools to make the read-only boundary obvious in generated config.
+ */
+export function createReadOnlyAgentPermission(): AgentPermission {
+  return {
+    '*': 'deny',
+    bash: 'deny',
+    edit: 'deny',
+    write: 'deny',
+    apply_patch: 'deny',
+    ast_grep_replace: 'deny',
+    task: 'deny',
+    question: 'deny',
+    read: 'allow',
+    glob: 'allow',
+    grep: 'allow',
+    lsp: 'allow',
+    list: 'allow',
+    codesearch: 'allow',
+    ast_grep_search: 'allow',
+  } as AgentPermission;
+}

+ 32 - 0
src/council/council-manager.test.ts

@@ -577,6 +577,38 @@ describe('CouncilManager', () => {
       expect(councillorCall).toBeDefined();
     });
 
+    test('disables mutating and delegation tools in councillor prompt body', async () => {
+      const ctx = createMockContext({
+        sessionMessagesResult: {
+          data: [
+            {
+              info: { role: 'assistant' },
+              parts: [{ type: 'text', text: 'Response' }],
+            },
+          ],
+        },
+      });
+      const config = createTestCouncilConfig();
+      const manager = new CouncilManager(ctx, config, undefined);
+
+      await manager.runCouncil('test prompt', undefined, 'parent-id');
+
+      const promptCalls = ctx.client.session.prompt.mock.calls as Array<
+        [{ body?: { agent?: string; tools?: Record<string, boolean> } }]
+      >;
+      const councillorCall = promptCalls.find(
+        (c) => c[0].body?.agent === 'councillor',
+      );
+      expect(councillorCall?.[0].body?.tools).toEqual({
+        task: false,
+        edit: false,
+        write: false,
+        apply_patch: false,
+        ast_grep_replace: false,
+        bash: false,
+      });
+    });
+
     test('creates session with model label in title', async () => {
       const ctx = createMockContext({
         sessionMessagesResult: {

+ 10 - 1
src/council/council-manager.ts

@@ -268,10 +268,19 @@ export class CouncilManager {
         await new Promise((r) => setTimeout(r, TMUX_SPAWN_DELAY_MS));
       }
 
+      // Councillors are advisory only: disable delegation and known mutating
+      // tools even if host defaults would otherwise expose them.
       const body: PromptBody = {
         agent: options.agent,
         model: modelRef,
-        tools: { task: false },
+        tools: {
+          task: false,
+          edit: false,
+          write: false,
+          apply_patch: false,
+          ast_grep_replace: false,
+          bash: false,
+        },
         parts: [{ type: 'text', text: options.promptText }],
       };