mirror of
https://github.com/Gitlawb/openclaude.git
synced 2026-08-24 02:34:15 -05:00
fix(code-reviewer): require inline diff input and preserve read-only search in embedded-search builds (#2102)
* feat: add code reviewer agent * feat(agent): add code-reviewer built-in agent implementation and tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(code-reviewer): address review comments - Fix broken step numbering in system prompt (glob/grep as sub-bullets) - Remove unnecessary wrapper in getSystemPrompt - Drop unused beforeEach/afterEach lifecycle in tests - Remove redundant registration test (beforeAll already throws) - Fix CLAUDE_CONFIG_DIR leak in beforeAll (restore in finally) - Replace @ts-ignore with explicit ToolUseContext cast - Use placeholder in README agentRouting example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci: trigger rerun — pre-existing test failures on main * fix(code-reviewer): enforce read-only contract by disallowing Bash Add Bash to disallowedTools so the reviewer cannot run shell commands regardless of parent session's acceptEdits/bypassPermissions mode. resolveAgentTools() treated undefined tools as wildcard — Bash was available and could auto-approve mkdir/rm/mv in acceptEdits mode. Remove Bash guidance from system prompt; diff must now be supplied by the caller inline. Update test to assert Bash is in disallowedTools. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(code-reviewer): deny all shell tools (Bash and PowerShell) Use SHELL_TOOL_NAMES constant instead of just BASH_TOOL_NAME to ensure all shell-capable tools are denied from the code-reviewer agent. This prevents Windows sessions with PowerShell enabled from bypassing the read-only contract. - Import SHELL_TOOL_NAMES from shellToolUtils - Use spread operator to include both Bash and PowerShell in disallowedTools - Update test to verify PowerShell is denied Fixes the finding: [P2] Deny all shell tools for the reviewer agent * fix(code-reviewer): explicit read-only allow-list; drop unrelated artifacts Switch code-reviewer to an explicit `tools` allow-list (Read, Glob, Grep) instead of relying on wildcard access minus a deny-list. resolveAgentTools() resolves only the named tools, so write-capable mcp__* server tools (and any other mutation-capable tool) can never be handed to the read-only reviewer. Keep the mutation deny-list as defense-in-depth. Remove generated/scratch artifacts unrelated to the reviewer agent: AGENTS.md, ARCHITECTURE.md, the .openlore/ .gitignore rule, temp_reference/, and the .tmp/sdk-consumer-* scratch files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: restore temp_reference/ gitignore entry from main Entry was present on main from #1350 and was unintentionally removed during PR cleanup. Restoring it so temp_reference/ scratch directories remain untracked after merge. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(code-reviewer): require inline diff input and preserve read-only search in embedded-search builds Summary: - Update the `code-reviewer` built-in agent to require the caller to provide the diff or changed hunks inline in the prompt. - Preserve read-only search behavior in embedded-search builds by omitting `Glob`/`Grep` from the explicit tool allow-list when embedded search tools are enabled. - Keep a strict read-only policy by disallowing shell and mutation tools. Usage: - The `code-reviewer` agent is now explicitly guided to only review changes when the diff is provided inline. - In embedded-search builds, the agent only receives `Read` and cannot access `Glob`/`Grep`. - This prevents the agent from attempting shell-based diff discovery and enforces caller-provided diff input. Test plan: - `bun test src/tools/AgentTool/built-in/codeReviewerAgent.test.ts` — 12 pass, 0 fail - `bun run build` — success - `bun run smoke` — success - `bun run security:pr-scan` — success - `git diff --check` — success Credit: prior work from #1381/#1420. * fix(code-reviewer): clear cached agent definitions and markdown loader cache after test cleanup * fix(code-reviewer): address all P2/P3 review findings from jatmn - Always list [Read, Glob, Grep] in the tool allow-list; in embedded-search builds resolveAgentTools() silently drops unavailable Glob/Grep at runtime. The system prompt explicitly documents the narrowed search contract for embedded builds instead of silently degrading. - Update the code-reviewer invocation example in prompt.ts to include the diff inline, satisfying the reviewer's contract and preventing an avoidable extra turn. - Rewrite the test suite with the same isolation protocol as loadAgentsDir.test.ts: shared mutation lock, OPENCLAUDE_CONFIG_DIR, setClaudeConfigHomeDirForTesting, setAllowedSettingSources, and full env/cache restore in finally blocks. - Exercise both embedded-search branches: non-embedded tests verify Glob/Grep guidance, embedded tests verify the limited-search documentation and Read-only path. - Fix settings file path in README.md and docs/agent-routing.md from ~/.openclaude.json to ~/.openclaude/settings.json (the path the runtime actually loads). - Add blank line after fenced code block (MD031), add code-reviewer to the routable built-in agent list in both README and agent-routing docs. * fix(code-reviewer): restore prior setting sources in test cleanup, add credential security warning - Capture getAllowedSettingSources() before overwriting and restore it in the finally block instead of resetting to the default list, preventing state leakage to concurrent suites. - Add plaintext-credential security warning before the agentModels JSON example in README. - Update 'All settings-driven' to 'Configured via settings, agent frontmatter, and environment variables' for accuracy. * fix(code-reviewer): address remaining PR feedback (P1/P3) * docs: document feature gate for Explore and Plan agents * docs: document inline-diff requirement for code-reviewer agent * fix(code-reviewer): address P1/P2 review findings — teammate boundary, resume safety, lock-aware tests [P1] Reject built-in agent types from teammate spawn path to preserve read-only boundary. The teammate branch bypasses resolveAgentTools(), so built-ins like code-reviewer would receive Bash/Edit/Write tools. Guard added in AgentTool.tsx before spawnTeammate is called. [P1] Fail closed when resuming an unavailable agent type instead of silently falling back to GENERAL_PURPOSE_AGENT. A resumed code-reviewer must never gain edit-capable tools through a compatibility fallback. [P2] Snapshot environment and config state only after acquiring the shared mutation lock in codeReviewerAgent.test.ts. Moved from module- scope const to post-lock capture in beforeAll, with cleanup and lock release in afterAll's finally block. Regression tests added for all three findings. * fix(code-reviewer): guard lock release against failed acquisition Only call releaseSharedMutationLock() in afterAll when the lock was successfully acquired. Prevents releasing another suite's lock if acquireSharedMutationLock() throws on timeout. * fix(code-reviewer): remove trailing whitespace * fix(code-reviewer): reliably block built-in teammate spawns Reject built-in agent types from the teammate spawn path by looking them up in allAgents rather than activeAgents. This ensures the restriction remains intact even when built-in agents are disabled (e.g. via CLAUDE_AGENT_SDK_DISABLE_BUILTIN_AGENTS) and omitted from activeAgents. Regression test added. * fix(code-reviewer): preserve original agent identity when resuming a read-only reviewer Background-agent metadata persisted only agentType, so resuming selected whichever active definition currently had that name. A built-in code-reviewer could be started, followed by a project/SDK definition named code-reviewer taking precedence; resuming the original agent would then use the replacement's ordinary wildcard tool set and grant that reviewer transcript Bash/Edit/Write tools. Persist the agent definition source and verify it matches the resolved definition on resume, rejecting the resumption if the original was spoofed. * fix(code-reviewer): address remaining CodeRabbit feedback on test cleanup and metadata source * fix(code-reviewer): address P1/P2 issues for teammate spawns and resume safety * docs: make OpenLore prerequisite explicitly optional in AGENTS.md * docs: fix pinned OpenLore version in AGENTS.md * Fix review issues * Revert AGENTS.md changes * Restore AGENTS.md to match upstream/main * fix(agent): address maintainer feedback on teammate spawns and resume persistence * test(agent): add regression coverage for legacy source-less agent resume * fix(agent): propagation pass — batch fork regression, TeamCreate policy, trailing whitespace, verification gate docs * fix(batch): allow specific custom agent types while requiring subagent_type --------- Co-authored-by: Laurent FRANCOISE <lfrancoise@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
DINESH R
co-authored by
Laurent FRANCOISE
Claude Sonnet 4.6
Laurent FRANCOISE
Claude Sonnet 4.6
parent
787f2a9390
commit
645d596ea4
@@ -360,15 +360,18 @@ OpenClaude supports multiple providers, but behavior is not identical across all
|
||||
|
||||
For best results, use models with strong tool/function calling support.
|
||||
|
||||
|
||||
|
||||
## Agents
|
||||
|
||||
Route different agents to different models (cost optimization, splitting work
|
||||
by model strength), cap sub-agent tool steps with `maxSteps`, and tune GitHub
|
||||
Copilot sub-agent behavior. All settings-driven:
|
||||
Copilot sub-agent behavior. Configured via settings, agent frontmatter, and
|
||||
environment variables:
|
||||
|
||||
- per-agent provider/model overrides via `agentModels` + `agentRouting` in `~/.openclaude.json`
|
||||
- per-agent provider/model overrides via `agentModels` + `agentRouting` in `~/.openclaude/settings.json`
|
||||
- model-only routes that reuse your current provider's credentials
|
||||
- built-in agents (`Explore`, `Plan`, `verification`) routable by type name
|
||||
- built-in agents (`Explore` and `Plan` [feature-gated], `verification` [feature-gated: requires `VERIFICATION_AGENT` + `tengu_hive_evidence`], `code-reviewer` [requires diff inline]) routable by type name
|
||||
|
||||
See [Agent Routing and Step Limits](docs/agent-routing.md) for the full guide.
|
||||
|
||||
|
||||
@@ -29,7 +29,9 @@ OpenClaude can route different agents to different models through
|
||||
settings-based routing. This is useful for cost optimization or splitting work
|
||||
by model strength.
|
||||
|
||||
Add to `~/.openclaude.json`:
|
||||
Add to `~/.openclaude/settings.json`:
|
||||
|
||||
> **Note:** `api_key` values in `settings.json` are stored in plaintext. Keep this file private and do not commit it to version control.
|
||||
|
||||
```json
|
||||
{
|
||||
@@ -88,9 +90,9 @@ no credential duplication:
|
||||
```
|
||||
|
||||
**Built-in agents are routable by their type name.** Useful keys:
|
||||
`verification` (the read-only auditor that runs before completion), `Explore`,
|
||||
and `Plan`. For example, `"agentRouting": { "verification": "mini" }` runs the
|
||||
verifier on `gpt-5-mini` while your main session stays on its model. Absent
|
||||
`verification` (the read-only auditor that runs before completion; **feature-gated**: requires `VERIFICATION_AGENT` and `tengu_hive_evidence` flag), `Explore`
|
||||
and `Plan` (if feature-gated on), and `code-reviewer` (requires diff inline). For example, `"agentRouting": { "verification": "mini" }` runs the
|
||||
verifier on `gpt-5-mini` while your main session stays on its model, but only when the verification gate is active. Absent
|
||||
any entry, the verifier inherits the main-loop model.
|
||||
|
||||
## GitHub Copilot sub-agent optimization
|
||||
|
||||
@@ -1483,7 +1483,6 @@ test('OpenGateway MiMo replays real reasoning_content without adding empty fallb
|
||||
input: {
|
||||
description: 'Inspect code',
|
||||
prompt: 'Look at the relevant code',
|
||||
subagent_type: 'general-purpose',
|
||||
},
|
||||
},
|
||||
],
|
||||
@@ -1566,7 +1565,6 @@ test('Xiaomi MiMo replays real reasoning_content without adding empty fallback',
|
||||
input: {
|
||||
description: 'Inspect code',
|
||||
prompt: 'Look at the relevant code',
|
||||
subagent_type: 'general-purpose',
|
||||
},
|
||||
},
|
||||
],
|
||||
@@ -1643,7 +1641,6 @@ test('OpenGateway MiMo does not synthesize empty reasoning_content when missing'
|
||||
input: {
|
||||
description: 'Inspect code',
|
||||
prompt: 'Look at the relevant code',
|
||||
subagent_type: 'general-purpose',
|
||||
},
|
||||
},
|
||||
],
|
||||
|
||||
@@ -71,7 +71,7 @@ For each agent, the prompt must be fully self-contained. Include:
|
||||
${WORKER_INSTRUCTIONS}
|
||||
\`\`\`
|
||||
|
||||
Use \`subagent_type: "general-purpose"\` unless a more specific agent type fits.
|
||||
Use a specific custom agent type if one fits the work unit, or \`subagent_type: "general-purpose"\` for default workers. Do NOT omit \`subagent_type\` — batch requires fresh, isolated subprocesses, and omitting the type routes to a fork (which inherits the coordinator's context) when the fork gate is on, which breaks the self-contained worktree invariant.
|
||||
|
||||
## Phase 3: Track Progress
|
||||
|
||||
|
||||
@@ -77,7 +77,6 @@ test('normal subagent prompt metadata uses routed effective model', async () =>
|
||||
{
|
||||
description: 'Inspect implementation',
|
||||
prompt: 'Find the bug',
|
||||
subagent_type: 'general-purpose',
|
||||
},
|
||||
createToolUseContext('parent-model', [createAgentDefinition()]),
|
||||
mock(async () => ({ behavior: 'allow' })) as never,
|
||||
@@ -124,7 +123,6 @@ test('agent invocation entering plan mode during MCP wait stays synchronous', as
|
||||
{
|
||||
description: 'Inspect implementation',
|
||||
prompt: 'Find the bug',
|
||||
subagent_type: 'general-purpose',
|
||||
},
|
||||
createToolUseContext('parent-model', [agent], 'default', 'plan'),
|
||||
mock(async () => ({ behavior: 'allow' })) as never,
|
||||
@@ -193,7 +191,6 @@ test('sync agents forward long-running tool progress to the parent tool call', a
|
||||
{
|
||||
description: 'Inspect implementation',
|
||||
prompt: 'Find the bug',
|
||||
subagent_type: 'general-purpose',
|
||||
},
|
||||
createToolUseContext('parent-model', [createAgentDefinition()]),
|
||||
mock(async () => ({ behavior: 'allow' })) as never,
|
||||
@@ -236,7 +233,6 @@ test('a throwing parent progress consumer does not change the subagent outcome',
|
||||
{
|
||||
description: 'Inspect implementation',
|
||||
prompt: 'Find the bug',
|
||||
subagent_type: 'general-purpose',
|
||||
},
|
||||
createToolUseContext('parent-model', [createAgentDefinition()]),
|
||||
mock(async () => ({ behavior: 'allow' })) as never,
|
||||
|
||||
@@ -127,6 +127,7 @@ async function importAgentToolWithSpawnMock(): Promise<{
|
||||
function makeToolUseContext(options: {
|
||||
mainLoopModel?: string
|
||||
activeAgents?: AgentDefinition[]
|
||||
allAgents?: AgentDefinition[]
|
||||
} = {}): ToolUseContext {
|
||||
const appState = {
|
||||
toolPermissionContext: { mode: 'default' },
|
||||
@@ -146,7 +147,7 @@ function makeToolUseContext(options: {
|
||||
isNonInteractiveSession: false,
|
||||
agentDefinitions: {
|
||||
activeAgents: options.activeAgents ?? [],
|
||||
allAgents: options.activeAgents ?? [],
|
||||
allAgents: options.allAgents ?? options.activeAgents ?? [],
|
||||
},
|
||||
},
|
||||
abortController: new AbortController(),
|
||||
@@ -191,6 +192,7 @@ function callTeammateAgentTool(
|
||||
contextOptions: {
|
||||
mainLoopModel?: string
|
||||
activeAgents?: AgentDefinition[]
|
||||
allAgents?: AgentDefinition[]
|
||||
} = {},
|
||||
): ReturnType<typeof AgentTool.call> {
|
||||
return AgentTool.call(
|
||||
@@ -272,7 +274,7 @@ test('passes routed agentModels keys to teammate spawns by subagent type', async
|
||||
},
|
||||
},
|
||||
agentRouting: {
|
||||
'general-purpose': 'deepseek-grunt',
|
||||
'custom-helper': 'deepseek-grunt',
|
||||
},
|
||||
} as unknown as SettingsJson
|
||||
allowedModelsForTest = new Set(['deepseek-grunt'])
|
||||
@@ -280,8 +282,8 @@ test('passes routed agentModels keys to teammate spawns by subagent type', async
|
||||
|
||||
await callTeammateAgentTool(
|
||||
AgentTool,
|
||||
{ subagent_type: 'general-purpose' },
|
||||
{ activeAgents: [createAgentDefinition('general-purpose')] },
|
||||
{ subagent_type: 'custom-helper' },
|
||||
{ activeAgents: [createAgentDefinition('custom-helper')] },
|
||||
)
|
||||
|
||||
expect(getSpawnConfig(spawnTeammate).model).toBe('deepseek-grunt')
|
||||
@@ -300,7 +302,7 @@ test('applies a model-only route to a teammate spawn without a cross-provider ov
|
||||
mini: { model: 'gpt-5-mini' },
|
||||
},
|
||||
agentRouting: {
|
||||
verification: 'mini',
|
||||
'custom-researcher': 'mini',
|
||||
},
|
||||
} as unknown as SettingsJson
|
||||
allowedModelsForTest = new Set(['gpt-5-mini'])
|
||||
@@ -308,8 +310,8 @@ test('applies a model-only route to a teammate spawn without a cross-provider ov
|
||||
|
||||
await callTeammateAgentTool(
|
||||
AgentTool,
|
||||
{ subagent_type: 'verification' },
|
||||
{ activeAgents: [createAgentDefinition('verification')] },
|
||||
{ subagent_type: 'custom-researcher' },
|
||||
{ activeAgents: [createAgentDefinition('custom-researcher')] },
|
||||
)
|
||||
|
||||
expect(getSpawnConfig(spawnTeammate).model).toBe('gpt-5-mini')
|
||||
@@ -383,9 +385,9 @@ test('does not let non-configured explicit teammate models fall through to defau
|
||||
AgentTool,
|
||||
{
|
||||
model: 'custom-provider-model',
|
||||
subagent_type: 'general-purpose',
|
||||
subagent_type: 'custom-helper',
|
||||
},
|
||||
{ activeAgents: [createAgentDefinition('general-purpose')] },
|
||||
{ activeAgents: [createAgentDefinition('custom-helper')] },
|
||||
)
|
||||
|
||||
expect(getSpawnConfig(spawnTeammate).model).toBe('custom-provider-model')
|
||||
@@ -401,7 +403,7 @@ test('rejects disallowed routed provider models before spawning a teammate', asy
|
||||
},
|
||||
},
|
||||
agentRouting: {
|
||||
'general-purpose': 'deepseek-grunt',
|
||||
'custom-helper': 'deepseek-grunt',
|
||||
},
|
||||
} as unknown as SettingsJson
|
||||
const { AgentTool, spawnTeammate } = await importAgentToolWithSpawnMock()
|
||||
@@ -409,11 +411,96 @@ test('rejects disallowed routed provider models before spawning a teammate', asy
|
||||
await expect(
|
||||
callTeammateAgentTool(
|
||||
AgentTool,
|
||||
{ subagent_type: 'general-purpose' },
|
||||
{ activeAgents: [createAgentDefinition('general-purpose')] },
|
||||
{ subagent_type: 'custom-helper' },
|
||||
{ activeAgents: [createAgentDefinition('custom-helper')] },
|
||||
),
|
||||
).rejects.toThrow(
|
||||
"Model 'deepseek-grunt' is not available. Your organization restricts model selection.",
|
||||
)
|
||||
expect(spawnTeammate).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
test('rejects built-in agents from being spawned as teammates', async () => {
|
||||
const { AgentTool, spawnTeammate } = await importAgentToolWithSpawnMock()
|
||||
|
||||
const builtinAgent = {
|
||||
agentType: 'code-reviewer',
|
||||
source: 'built-in',
|
||||
getSystemPrompt: () => 'review code',
|
||||
} as unknown as AgentDefinition
|
||||
|
||||
await expect(
|
||||
callTeammateAgentTool(
|
||||
AgentTool,
|
||||
{ subagent_type: 'code-reviewer' },
|
||||
{ activeAgents: [builtinAgent] },
|
||||
),
|
||||
).rejects.toThrow(
|
||||
"Built-in agent type 'code-reviewer' cannot be spawned as a teammate. Please omit name and team_name to use it as a standard subagent.",
|
||||
)
|
||||
expect(spawnTeammate).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
test('rejects built-in agents from being spawned as teammates even when built-ins are disabled', async () => {
|
||||
const { AgentTool, spawnTeammate } = await importAgentToolWithSpawnMock()
|
||||
|
||||
const prevEnv = process.env.CLAUDE_AGENT_SDK_DISABLE_BUILTIN_AGENTS
|
||||
process.env.CLAUDE_AGENT_SDK_DISABLE_BUILTIN_AGENTS = '1'
|
||||
const { setIsInteractive, getIsNonInteractiveSession } = await import('../../bootstrap/state.js')
|
||||
const { getAgentDefinitionsWithOverrides, clearAgentDefinitionsCache } = await import('./loadAgentsDir.js')
|
||||
|
||||
const prevInteractive = !getIsNonInteractiveSession()
|
||||
setIsInteractive(false)
|
||||
clearAgentDefinitionsCache()
|
||||
|
||||
try {
|
||||
const definitions = await getAgentDefinitionsWithOverrides()
|
||||
|
||||
await expect(
|
||||
callTeammateAgentTool(
|
||||
AgentTool,
|
||||
{ subagent_type: 'code-reviewer' },
|
||||
{ activeAgents: definitions.activeAgents, allAgents: definitions.allAgents },
|
||||
),
|
||||
).rejects.toThrow(
|
||||
"Built-in agent type 'code-reviewer' cannot be spawned as a teammate. Please omit name and team_name to use it as a standard subagent.",
|
||||
)
|
||||
|
||||
await expect(
|
||||
callTeammateAgentTool(
|
||||
AgentTool,
|
||||
{ subagent_type: 'claude-code-guide' },
|
||||
{ activeAgents: definitions.activeAgents, allAgents: definitions.allAgents },
|
||||
),
|
||||
).rejects.toThrow(
|
||||
"Built-in agent type 'claude-code-guide' cannot be spawned as a teammate. Please omit name and team_name to use it as a standard subagent.",
|
||||
)
|
||||
expect(spawnTeammate).not.toHaveBeenCalled()
|
||||
} finally {
|
||||
setIsInteractive(prevInteractive)
|
||||
if (prevEnv !== undefined) {
|
||||
process.env.CLAUDE_AGENT_SDK_DISABLE_BUILTIN_AGENTS = prevEnv
|
||||
} else {
|
||||
delete process.env.CLAUDE_AGENT_SDK_DISABLE_BUILTIN_AGENTS
|
||||
}
|
||||
clearAgentDefinitionsCache()
|
||||
}
|
||||
|
||||
})
|
||||
|
||||
test('allows a custom agent to be spawned as a teammate even if it shadows a built-in name', async () => {
|
||||
const { AgentTool, spawnTeammate } = await importAgentToolWithSpawnMock()
|
||||
|
||||
const customShadowAgent = {
|
||||
agentType: 'code-reviewer',
|
||||
source: 'projectSettings', // It shadows the name but has a different source
|
||||
} as unknown as AgentDefinition
|
||||
|
||||
await callTeammateAgentTool(
|
||||
AgentTool,
|
||||
{ subagent_type: 'code-reviewer' },
|
||||
{ activeAgents: [customShadowAgent] },
|
||||
)
|
||||
|
||||
expect(spawnTeammate).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
@@ -16,6 +16,7 @@ import { clearDumpState } from '../../services/api/dumpPrompts.js';
|
||||
import { resolveAgentRunModelRouting, resolveOutOfProcessTeammateProvider, resolveOutOfProcessTeammateModelOnly } from '../../services/api/agentRouting.js';
|
||||
import { completeAgentTask as completeAsyncAgent, createActivityDescriptionResolver, createProgressTracker, enqueueAgentNotification, failAgentTask as failAsyncAgent, getProgressUpdate, getTokenCountFromTracker, isLocalAgentTask, killAsyncAgent, registerAgentForeground, registerAsyncAgent, unregisterAgentForeground, updateAgentProgress as updateAsyncAgentProgress, updateProgressFromMessage } from '../../tasks/LocalAgentTask/LocalAgentTask.js';
|
||||
import { assembleToolPool } from '../../tools.js';
|
||||
import { isBuiltInAgentType } from './builtInAgents.js';
|
||||
import { asAgentId } from '../../types/ids.js';
|
||||
import { runWithAgentContext } from '../../utils/agentContext.js';
|
||||
import { isAgentSwarmsEnabled } from '../../utils/agentSwarmsEnabled.js';
|
||||
@@ -374,6 +375,17 @@ export const AgentTool = buildTool({
|
||||
if (teamName && name) {
|
||||
// Set agent definition color for grouped UI display before spawning
|
||||
const agentDef = subagent_type ? toolUseContext.options.agentDefinitions.activeAgents.find(a => a.agentType === subagent_type) : undefined;
|
||||
|
||||
if (subagent_type) {
|
||||
if (agentDef) {
|
||||
if (agentDef.source === 'built-in') {
|
||||
throw new Error(`Built-in agent type '${subagent_type}' cannot be spawned as a teammate. Please omit name and team_name to use it as a standard subagent.`);
|
||||
}
|
||||
} else if (isBuiltInAgentType(subagent_type)) {
|
||||
throw new Error(`Built-in agent type '${subagent_type}' cannot be spawned as a teammate. Please omit name and team_name to use it as a standard subagent.`);
|
||||
}
|
||||
}
|
||||
|
||||
if (agentDef?.color) {
|
||||
setAgentColor(subagent_type!, agentDef.color);
|
||||
}
|
||||
@@ -849,6 +861,7 @@ export const AgentTool = buildTool({
|
||||
// present so resume can still land in the target repository.
|
||||
void writeAgentMetadata(asAgentId(earlyAgentId), {
|
||||
agentType: selectedAgent.agentType,
|
||||
source: selectedAgent.source,
|
||||
...(cwd && { cwd }),
|
||||
...(description && { description }),
|
||||
}).catch(_err => logForDebugging(`Failed to clear worktree metadata: ${_err}`));
|
||||
|
||||
@@ -0,0 +1,262 @@
|
||||
import {
|
||||
afterAll,
|
||||
afterEach,
|
||||
beforeAll,
|
||||
beforeEach,
|
||||
describe,
|
||||
expect,
|
||||
test,
|
||||
} from 'bun:test'
|
||||
import { mkdtemp, rm } from 'fs/promises'
|
||||
import { tmpdir } from 'os'
|
||||
import { join } from 'path'
|
||||
import {
|
||||
getAllowedSettingSources,
|
||||
setAllowedSettingSources,
|
||||
} from 'src/bootstrap/state.js'
|
||||
import {
|
||||
acquireSharedMutationLock,
|
||||
releaseSharedMutationLock,
|
||||
} from 'src/test/sharedMutationLock.js'
|
||||
import {
|
||||
getClaudeConfigHomeDir,
|
||||
getClaudeConfigHomeDirOverrideForTesting,
|
||||
setClaudeConfigHomeDirForTesting,
|
||||
} from 'src/utils/envUtils.js'
|
||||
import { loadMarkdownFilesForSubdir } from 'src/utils/markdownConfigLoader.js'
|
||||
import { SETTING_SOURCES } from 'src/utils/settings/constants.js'
|
||||
import { resetSettingsCache } from 'src/utils/settings/settingsCache.js'
|
||||
import {
|
||||
clearAgentDefinitionsCache,
|
||||
getAgentDefinitionsWithOverrides,
|
||||
} from '../loadAgentsDir.js'
|
||||
import type { BuiltInAgentDefinition } from '../loadAgentsDir.js'
|
||||
import type { ToolUseContext } from 'src/Tool.js'
|
||||
|
||||
// ── Shared env snapshot ────────────────────────────────────────
|
||||
|
||||
let originalEnv: Record<string, string | undefined> = {}
|
||||
|
||||
function restoreEnv(key: string): void {
|
||||
if (!originalEnv || !(key in originalEnv)) return
|
||||
const val = originalEnv[key]
|
||||
if (val === undefined) delete process.env[key]
|
||||
else process.env[key] = val
|
||||
}
|
||||
|
||||
function restoreAllEnv(): void {
|
||||
if (!originalEnv) return
|
||||
for (const key of Object.keys(originalEnv)) {
|
||||
restoreEnv(key)
|
||||
}
|
||||
}
|
||||
|
||||
describe('code-reviewer built-in agent', () => {
|
||||
let agent: BuiltInAgentDefinition
|
||||
let dir: string
|
||||
let lockAcquired = false
|
||||
let previousOverride: ReturnType<typeof getClaudeConfigHomeDirOverrideForTesting>
|
||||
let previousSettingSources: ReturnType<typeof getAllowedSettingSources>
|
||||
|
||||
beforeAll(async () => {
|
||||
await acquireSharedMutationLock('codeReviewerAgent.test.ts')
|
||||
lockAcquired = true
|
||||
|
||||
originalEnv = {
|
||||
HOME: process.env.HOME,
|
||||
OPENCLAUDE_CONFIG_DIR: process.env.OPENCLAUDE_CONFIG_DIR,
|
||||
CLAUDE_CONFIG_DIR: process.env.CLAUDE_CONFIG_DIR,
|
||||
EMBEDDED_SEARCH_TOOLS: process.env.EMBEDDED_SEARCH_TOOLS,
|
||||
CLAUDE_CODE_ENTRYPOINT: process.env.CLAUDE_CODE_ENTRYPOINT,
|
||||
CLAUDE_CODE_USE_NATIVE_FILE_SEARCH: process.env.CLAUDE_CODE_USE_NATIVE_FILE_SEARCH,
|
||||
}
|
||||
previousOverride = getClaudeConfigHomeDirOverrideForTesting()
|
||||
previousSettingSources = getAllowedSettingSources()
|
||||
|
||||
dir = await mkdtemp(join(tmpdir(), 'openclaude-reviewer-test-'))
|
||||
const configDir = join(dir, '.openclaude')
|
||||
|
||||
setClaudeConfigHomeDirForTesting(configDir)
|
||||
process.env.HOME = dir
|
||||
process.env.OPENCLAUDE_CONFIG_DIR = configDir
|
||||
process.env.CLAUDE_CONFIG_DIR = configDir
|
||||
process.env.CLAUDE_CODE_USE_NATIVE_FILE_SEARCH = '1'
|
||||
setAllowedSettingSources([...SETTING_SOURCES])
|
||||
getClaudeConfigHomeDir.cache?.clear?.()
|
||||
resetSettingsCache()
|
||||
clearAgentDefinitionsCache()
|
||||
loadMarkdownFilesForSubdir.cache.clear?.()
|
||||
|
||||
const { activeAgents } = await getAgentDefinitionsWithOverrides(dir)
|
||||
const found = activeAgents.find((a) => a.agentType === 'code-reviewer')
|
||||
if (!found || found.source !== 'built-in') {
|
||||
throw new Error('code-reviewer agent not found in built-in agents')
|
||||
}
|
||||
agent = found as BuiltInAgentDefinition
|
||||
})
|
||||
|
||||
afterAll(async () => {
|
||||
try {
|
||||
restoreAllEnv()
|
||||
setClaudeConfigHomeDirForTesting(previousOverride)
|
||||
getClaudeConfigHomeDir.cache?.clear?.()
|
||||
setAllowedSettingSources(previousSettingSources)
|
||||
resetSettingsCache()
|
||||
clearAgentDefinitionsCache()
|
||||
loadMarkdownFilesForSubdir.cache.clear?.()
|
||||
if (dir) {
|
||||
await rm(dir, { recursive: true, force: true })
|
||||
}
|
||||
} finally {
|
||||
if (lockAcquired) {
|
||||
releaseSharedMutationLock()
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
// ── Definition ────────────────────────────────────────────────
|
||||
|
||||
test('source is built-in', () => {
|
||||
expect(agent.source).toBe('built-in')
|
||||
})
|
||||
|
||||
test('model is inherit (allows agentRouting override)', () => {
|
||||
expect(agent.model).toBe('inherit')
|
||||
})
|
||||
|
||||
test('omitClaudeMd is true', () => {
|
||||
expect(agent.omitClaudeMd).toBe(true)
|
||||
})
|
||||
|
||||
test('whenToUse is non-empty', () => {
|
||||
expect(agent.whenToUse.length).toBeGreaterThan(0)
|
||||
})
|
||||
|
||||
test('whenToUse requires the caller to provide the diff inline', () => {
|
||||
expect(agent.whenToUse).toContain('diff')
|
||||
expect(agent.whenToUse).toContain('inline')
|
||||
})
|
||||
|
||||
test('disallows mutation tools', () => {
|
||||
const disallowed = agent.disallowedTools ?? []
|
||||
expect(disallowed).toContain('Agent')
|
||||
expect(disallowed).toContain('Bash')
|
||||
expect(disallowed).toContain('PowerShell')
|
||||
expect(disallowed).toContain('Edit')
|
||||
expect(disallowed).toContain('Write')
|
||||
expect(disallowed).toContain('NotebookEdit')
|
||||
expect(disallowed).toContain('ExitPlanMode')
|
||||
})
|
||||
|
||||
// ── System prompt — non-embedded ─────────────────────────────
|
||||
|
||||
describe('system prompt (non-embedded search)', () => {
|
||||
let prompt: string
|
||||
|
||||
beforeEach(() => {
|
||||
// Ensure embedded search is OFF for this branch
|
||||
delete process.env.EMBEDDED_SEARCH_TOOLS
|
||||
prompt = agent.getSystemPrompt({
|
||||
toolUseContext: {} as Pick<ToolUseContext, 'options'>,
|
||||
})
|
||||
})
|
||||
|
||||
test('returns non-empty string', () => {
|
||||
expect(typeof prompt).toBe('string')
|
||||
expect(prompt.length).toBeGreaterThan(0)
|
||||
})
|
||||
|
||||
test('lists Read, Glob, and Grep in tools', () => {
|
||||
const tools = agent.tools ?? []
|
||||
expect(tools).toEqual(['Read', 'Glob', 'Grep'])
|
||||
})
|
||||
|
||||
test('covers all review dimensions', () => {
|
||||
expect(prompt).toContain('Correctness')
|
||||
expect(prompt).toContain('Security')
|
||||
expect(prompt).toContain('Performance')
|
||||
expect(prompt).toContain('Maintainability')
|
||||
expect(prompt).toContain('Design')
|
||||
})
|
||||
|
||||
test('defines severity levels', () => {
|
||||
expect(prompt).toContain('CRITICAL')
|
||||
expect(prompt).toContain('HIGH')
|
||||
expect(prompt).toContain('MEDIUM')
|
||||
expect(prompt).toContain('LOW')
|
||||
})
|
||||
|
||||
test('enforces read-only constraint', () => {
|
||||
expect(prompt).toContain('READ-ONLY')
|
||||
expect(prompt).toContain('Do NOT attempt to run shell commands')
|
||||
})
|
||||
|
||||
test('includes verdict in output format', () => {
|
||||
expect(prompt).toContain('Verdict')
|
||||
})
|
||||
|
||||
test('references Glob and Grep search tools', () => {
|
||||
expect(prompt).toContain('Glob')
|
||||
expect(prompt).toContain('Grep')
|
||||
})
|
||||
|
||||
test('requires diff to be provided inline', () => {
|
||||
expect(prompt).toContain('diff MUST be provided inline')
|
||||
})
|
||||
})
|
||||
|
||||
// ── System prompt — embedded search ──────────────────────────
|
||||
|
||||
describe('system prompt (embedded search)', () => {
|
||||
let prompt: string
|
||||
|
||||
beforeEach(() => {
|
||||
// Simulate the embedded-search build variant
|
||||
process.env.EMBEDDED_SEARCH_TOOLS = '1'
|
||||
// Ensure we're not in an SDK entrypoint that disables embedded tools
|
||||
delete process.env.CLAUDE_CODE_ENTRYPOINT
|
||||
prompt = agent.getSystemPrompt({
|
||||
toolUseContext: {} as Pick<ToolUseContext, 'options'>,
|
||||
})
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
restoreEnv('EMBEDDED_SEARCH_TOOLS')
|
||||
restoreEnv('CLAUDE_CODE_ENTRYPOINT')
|
||||
})
|
||||
|
||||
test('returns non-empty string', () => {
|
||||
expect(typeof prompt).toBe('string')
|
||||
expect(prompt.length).toBeGreaterThan(0)
|
||||
})
|
||||
|
||||
test('lists only Read in tools since Glob/Grep are absent', () => {
|
||||
const tools = agent.tools ?? []
|
||||
expect(tools).toEqual(['Read'])
|
||||
})
|
||||
|
||||
test('documents limited search capability', () => {
|
||||
// In embedded builds, the prompt must acknowledge that Glob/Grep
|
||||
// are unavailable and shell access is denied.
|
||||
expect(prompt).toContain('unavailable')
|
||||
expect(prompt).toContain('Read')
|
||||
})
|
||||
|
||||
test('still covers all review dimensions', () => {
|
||||
expect(prompt).toContain('Correctness')
|
||||
expect(prompt).toContain('Security')
|
||||
expect(prompt).toContain('Performance')
|
||||
expect(prompt).toContain('Maintainability')
|
||||
expect(prompt).toContain('Design')
|
||||
})
|
||||
|
||||
test('still enforces read-only constraint', () => {
|
||||
expect(prompt).toContain('READ-ONLY')
|
||||
expect(prompt).toContain('Do NOT attempt to run shell commands')
|
||||
})
|
||||
|
||||
test('requires diff to be provided inline', () => {
|
||||
expect(prompt).toContain('diff MUST be provided inline')
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,101 @@
|
||||
import { EXIT_PLAN_MODE_TOOL_NAME } from 'src/tools/ExitPlanModeTool/constants.js'
|
||||
import { FILE_EDIT_TOOL_NAME } from 'src/tools/FileEditTool/constants.js'
|
||||
import { FILE_READ_TOOL_NAME } from 'src/tools/FileReadTool/prompt.js'
|
||||
import { FILE_WRITE_TOOL_NAME } from 'src/tools/FileWriteTool/prompt.js'
|
||||
import { GLOB_TOOL_NAME } from 'src/tools/GlobTool/prompt.js'
|
||||
import { GREP_TOOL_NAME } from 'src/tools/GrepTool/prompt.js'
|
||||
import { NOTEBOOK_EDIT_TOOL_NAME } from 'src/tools/NotebookEditTool/constants.js'
|
||||
import { hasEmbeddedSearchTools } from 'src/utils/embeddedTools.js'
|
||||
import { SHELL_TOOL_NAMES } from 'src/utils/shell/shellToolUtils.js'
|
||||
import { AGENT_TOOL_NAME } from '../constants.js'
|
||||
import type { BuiltInAgentDefinition } from '../loadAgentsDir.js'
|
||||
|
||||
function getCodeReviewerSystemPrompt(): string {
|
||||
const embedded = hasEmbeddedSearchTools()
|
||||
const searchGuidance = embedded
|
||||
? `- Use ${FILE_READ_TOOL_NAME} to read specific files named in the diff for surrounding context
|
||||
- Note: In this build, file search tools (Glob/Grep) are unavailable and shell access is denied. You can only read files explicitly named in the diff or referenced in the code you read. If you need to find callers or dependents beyond the diff, ask the caller to supply the relevant file paths or additional context.`
|
||||
: `- Use ${GLOB_TOOL_NAME} for file pattern matching to find callers and dependents
|
||||
- Use ${GREP_TOOL_NAME} for searching file contents to trace references`
|
||||
|
||||
return `You are an independent code reviewer for OpenClaude. Your role is to provide critical, balanced review of code changes.
|
||||
|
||||
=== CRITICAL: READ-ONLY MODE - NO FILE MODIFICATIONS ===
|
||||
You are STRICTLY PROHIBITED from creating, modifying, or deleting any files.
|
||||
You do NOT have access to file editing tools or a shell — attempting to edit files or run shell commands will fail.
|
||||
|
||||
## Review Dimensions
|
||||
|
||||
Evaluate changes across all dimensions with equal weight:
|
||||
|
||||
1. **Correctness** — Logic errors, off-by-one, null/undefined handling, race conditions, incorrect assumptions
|
||||
2. **Security** — Injection, auth bypass, insecure defaults, sensitive data exposure, input validation
|
||||
3. **Performance** — Unnecessary work in hot paths, memory leaks, O(n²) where O(n) suffices, missing caching
|
||||
4. **Maintainability** — Dead code, duplicated logic, unclear naming, missing edge case handling
|
||||
5. **Design** — API consistency, abstraction leaks, coupling, adherence to existing patterns in the codebase
|
||||
|
||||
## Process
|
||||
|
||||
1. The diff MUST be provided inline in the prompt. If it is not, respond with a single message asking the caller to supply the diff or changed hunks — do NOT attempt to discover changes yourself.
|
||||
2. For each changed file, read surrounding context with ${FILE_READ_TOOL_NAME} to understand intent
|
||||
${searchGuidance}
|
||||
3. Do NOT attempt to run shell commands such as \`git diff\` yourself.
|
||||
4. Check callers/dependents if the change modifies a public interface
|
||||
|
||||
## Output Format
|
||||
|
||||
Structure your findings as:
|
||||
|
||||
### Summary
|
||||
One paragraph: what changed and overall assessment (approve / approve with suggestions / request changes).
|
||||
|
||||
### Findings
|
||||
|
||||
For each finding:
|
||||
- **[CRITICAL|HIGH|MEDIUM|LOW]** \`path/to/file.ts:line\` — Problem description. Suggested fix (if applicable).
|
||||
|
||||
If no findings at a given severity, omit that level.
|
||||
|
||||
### Verdict
|
||||
One of: ✓ Approve | ~ Approve with suggestions | ✗ Request changes
|
||||
|
||||
Be direct and specific. Skip praise. Focus on what could break, be exploited, or cause future pain.`
|
||||
}
|
||||
|
||||
// Explicit read-only allow-list. resolveAgentTools() resolves ONLY the tools
|
||||
// named here, so write-capable tools (Bash/PowerShell, Edit/Write/Notebook,
|
||||
// Agent) and any user-configured write-capable mcp__* server tools can never
|
||||
// be handed to this agent — an omitted `tools` list would wildcard them in.
|
||||
//
|
||||
// In embedded-search builds, Glob and Grep are absent from the tool registry,
|
||||
// so we omit them from the allow-list here to prevent advertising unrecognized
|
||||
// capabilities to callers or in the `/agents` UI.
|
||||
function getCodeReviewerTools(): string[] {
|
||||
return hasEmbeddedSearchTools()
|
||||
? [FILE_READ_TOOL_NAME]
|
||||
: [FILE_READ_TOOL_NAME, GLOB_TOOL_NAME, GREP_TOOL_NAME]
|
||||
}
|
||||
|
||||
export const CODE_REVIEWER_AGENT: BuiltInAgentDefinition = {
|
||||
agentType: 'code-reviewer',
|
||||
whenToUse:
|
||||
'Independent code reviewer for changes, diffs, and pull requests. Provides balanced critique across correctness, security, performance, maintainability, and design. Use after completing a coding task or when asked to review specific changes. The caller must provide the diff or changed hunks inline in the prompt because this agent cannot run shell commands. Invoke with subagent_type: "code-reviewer".',
|
||||
get tools() {
|
||||
return getCodeReviewerTools()
|
||||
},
|
||||
// Defense-in-depth: also deny mutation tools by name so the read-only
|
||||
// contract holds even if the allow-list above is later widened.
|
||||
disallowedTools: [
|
||||
AGENT_TOOL_NAME,
|
||||
...SHELL_TOOL_NAMES,
|
||||
EXIT_PLAN_MODE_TOOL_NAME,
|
||||
FILE_EDIT_TOOL_NAME,
|
||||
FILE_WRITE_TOOL_NAME,
|
||||
NOTEBOOK_EDIT_TOOL_NAME,
|
||||
],
|
||||
source: 'built-in',
|
||||
baseDir: 'built-in',
|
||||
model: 'inherit',
|
||||
omitClaudeMd: true,
|
||||
getSystemPrompt: getCodeReviewerSystemPrompt,
|
||||
}
|
||||
@@ -3,6 +3,7 @@ import { getIsNonInteractiveSession } from '../../bootstrap/state.js'
|
||||
import { getFeatureValue_CACHED_MAY_BE_STALE } from '../../services/analytics/growthbook.js'
|
||||
import { isEnvTruthy } from '../../utils/envUtils.js'
|
||||
import { CLAUDE_CODE_GUIDE_AGENT } from './built-in/claudeCodeGuideAgent.js'
|
||||
import { CODE_REVIEWER_AGENT } from './built-in/codeReviewerAgent.js'
|
||||
import { EXPLORE_AGENT } from './built-in/exploreAgent.js'
|
||||
import { GENERAL_PURPOSE_AGENT } from './built-in/generalPurposeAgent.js'
|
||||
import { PLAN_AGENT } from './built-in/planAgent.js'
|
||||
@@ -45,6 +46,7 @@ export function getBuiltInAgents(): AgentDefinition[] {
|
||||
const agents: AgentDefinition[] = [
|
||||
GENERAL_PURPOSE_AGENT,
|
||||
STATUSLINE_SETUP_AGENT,
|
||||
CODE_REVIEWER_AGENT,
|
||||
]
|
||||
|
||||
if (areExplorePlanAgentsEnabled()) {
|
||||
@@ -70,3 +72,17 @@ export function getBuiltInAgents(): AgentDefinition[] {
|
||||
|
||||
return agents
|
||||
}
|
||||
|
||||
const BUILT_IN_AGENT_TYPES = new Set([
|
||||
GENERAL_PURPOSE_AGENT.agentType,
|
||||
STATUSLINE_SETUP_AGENT.agentType,
|
||||
CODE_REVIEWER_AGENT.agentType,
|
||||
EXPLORE_AGENT.agentType,
|
||||
PLAN_AGENT.agentType,
|
||||
CLAUDE_CODE_GUIDE_AGENT.agentType,
|
||||
VERIFICATION_AGENT.agentType
|
||||
])
|
||||
|
||||
export function isBuiltInAgentType(agentType: string): boolean {
|
||||
return BUILT_IN_AGENT_TYPES.has(agentType)
|
||||
}
|
||||
|
||||
@@ -142,15 +142,15 @@ assistant: Still waiting on the audit \u2014 that's one of the things it's check
|
||||
|
||||
<example>
|
||||
user: "Can you get a second opinion on whether this migration is safe?"
|
||||
assistant: <thinking>I'll ask the code-reviewer agent — it won't see my analysis, so it can give an independent read.</thinking>
|
||||
assistant: <thinking>I'll ask the code-reviewer agent — it won't see my analysis, so it can give an independent read. The code-reviewer requires the diff inline, so I need to include the changed hunks.</thinking>
|
||||
<commentary>
|
||||
A subagent_type is specified, so the agent starts fresh. It needs full context in the prompt. The briefing explains what to assess and why.
|
||||
A subagent_type is specified, so the agent starts fresh. It needs full context in the prompt. The code-reviewer contract requires the caller to provide the diff or changed hunks inline — the reviewer cannot run git diff itself.
|
||||
Note: do NOT add a name parameter here — code-reviewer is a built-in and will be rejected if spawned as a teammate. Omit name/team_name so it runs as a standard subagent.
|
||||
</commentary>
|
||||
${AGENT_TOOL_NAME}({
|
||||
name: "migration-review",
|
||||
description: "Independent migration review",
|
||||
subagent_type: "code-reviewer",
|
||||
prompt: "Review migration 0042_user_schema.sql for safety. Context: we're adding a NOT NULL column to a 50M-row table. Existing rows get a backfill default. I want a second opinion on whether the backfill approach is safe under concurrent writes — I've checked locking behavior but want independent verification. Report: is this safe, and if not, what specifically breaks?"
|
||||
prompt: "Review migration 0042_user_schema.sql for safety. Context: we're adding a NOT NULL column to a 50M-row table with a backfill default.\n\nHere is the diff:\n\`\`\`sql\n--- a/migrations/0042_user_schema.sql\n+++ b/migrations/0042_user_schema.sql\n@@ -0,0 +1,5 @@\n+ALTER TABLE users ADD COLUMN org_id INTEGER NOT NULL DEFAULT 0;\n+UPDATE users SET org_id = (SELECT id FROM orgs WHERE orgs.legacy_id = users.legacy_org_id);\n+ALTER TABLE users ALTER COLUMN org_id DROP DEFAULT;\n\`\`\`\n\nI want a second opinion on whether the backfill approach is safe under concurrent writes — I've checked locking behavior but want independent verification. Report: is this safe, and if not, what specifically breaks?"
|
||||
})
|
||||
</example>
|
||||
`
|
||||
|
||||
@@ -0,0 +1,176 @@
|
||||
import { beforeEach, expect, mock, test } from 'bun:test'
|
||||
import type { ToolUseContext } from '../../Tool.js'
|
||||
import type { AgentDefinition } from './loadAgentsDir.js'
|
||||
import { resumeAgentBackground } from './resumeAgent.js'
|
||||
|
||||
let mockTranscript: any = {
|
||||
messages: [],
|
||||
contentReplacements: [],
|
||||
}
|
||||
|
||||
let mockMetadata: any = {
|
||||
agentType: 'code-reviewer',
|
||||
source: 'built-in',
|
||||
}
|
||||
|
||||
mock.module('../../utils/sessionStorage.js', () => ({
|
||||
getAgentTranscript: async () => mockTranscript,
|
||||
readAgentMetadata: async () => mockMetadata,
|
||||
writeAgentMetadata: async () => {},
|
||||
}))
|
||||
|
||||
mock.module('../../tasks/LocalAgentTask/LocalAgentTask.js', () => ({
|
||||
registerAsyncAgent: () => ({
|
||||
agentId: 'test-agent',
|
||||
abortController: new AbortController(),
|
||||
}),
|
||||
}))
|
||||
|
||||
mock.module('./agentToolUtils.js', () => ({
|
||||
runAsyncAgentLifecycle: async () => {},
|
||||
}))
|
||||
|
||||
beforeEach(() => {
|
||||
mockTranscript = {
|
||||
messages: [],
|
||||
contentReplacements: [],
|
||||
}
|
||||
mockMetadata = {
|
||||
agentType: 'code-reviewer',
|
||||
source: 'built-in',
|
||||
}
|
||||
})
|
||||
|
||||
function makeToolUseContext(activeAgents: AgentDefinition[]): ToolUseContext {
|
||||
const appState = {
|
||||
toolPermissionContext: {
|
||||
mode: 'default',
|
||||
additionalWorkingDirectories: new Map(),
|
||||
alwaysDenyRules: {},
|
||||
},
|
||||
mcp: { tools: [], clients: [] },
|
||||
}
|
||||
|
||||
return {
|
||||
options: {
|
||||
agentDefinitions: { activeAgents, allAgents: activeAgents },
|
||||
tools: [],
|
||||
mainLoopModel: 'test-model',
|
||||
mcpClients: [],
|
||||
},
|
||||
getAppState: () => appState,
|
||||
setAppState: () => {},
|
||||
contentReplacementState: { replacements: new Map() },
|
||||
} as unknown as ToolUseContext
|
||||
}
|
||||
|
||||
test('fails closed when resuming an unavailable agent instead of falling back', async () => {
|
||||
const context = makeToolUseContext([]) // Empty active agents list, so code-reviewer is unavailable
|
||||
|
||||
await expect(
|
||||
resumeAgentBackground({
|
||||
agentId: 'test-agent',
|
||||
prompt: 'continue',
|
||||
toolUseContext: context,
|
||||
canUseTool: async () => ({ behavior: 'allow' } as any),
|
||||
}),
|
||||
).rejects.toThrow(
|
||||
"Cannot resume agent: type 'code-reviewer' is unavailable or disabled in the current session."
|
||||
)
|
||||
})
|
||||
|
||||
test('successfully resumes when agent is available', async () => {
|
||||
const codeReviewer = {
|
||||
agentType: 'code-reviewer',
|
||||
source: 'built-in',
|
||||
getSystemPrompt: () => 'review code',
|
||||
} as unknown as AgentDefinition
|
||||
|
||||
const context = makeToolUseContext([codeReviewer])
|
||||
|
||||
const result = await resumeAgentBackground({
|
||||
agentId: 'test-agent',
|
||||
prompt: 'continue',
|
||||
toolUseContext: context,
|
||||
canUseTool: async () => ({ behavior: 'allow' } as any),
|
||||
})
|
||||
|
||||
expect(result.agentId).toBe('test-agent')
|
||||
})
|
||||
|
||||
test('rejects resume when agent definition source does not match metadata', async () => {
|
||||
mockMetadata = {
|
||||
agentType: 'code-reviewer',
|
||||
source: 'built-in', // Originally launched as a built-in
|
||||
}
|
||||
|
||||
// A custom agent was added to the project that shadows the built-in name
|
||||
const customReviewer = {
|
||||
agentType: 'code-reviewer',
|
||||
source: 'projectSettings', // Different source
|
||||
getSystemPrompt: () => 'review code differently',
|
||||
} as unknown as AgentDefinition
|
||||
|
||||
const context = makeToolUseContext([customReviewer])
|
||||
|
||||
await expect(
|
||||
resumeAgentBackground({
|
||||
agentId: 'test-agent',
|
||||
prompt: 'continue',
|
||||
toolUseContext: context,
|
||||
canUseTool: async () => ({ behavior: 'allow' } as any),
|
||||
}),
|
||||
).rejects.toThrow(
|
||||
"Cannot resume agent: identity mismatch. Expected source 'built-in', found 'projectSettings' for type 'code-reviewer'."
|
||||
)
|
||||
})
|
||||
|
||||
test('rejects resume when legacy metadata lacks a source', async () => {
|
||||
mockMetadata = {
|
||||
agentType: 'code-reviewer',
|
||||
// Legacy metadata lacks a source field
|
||||
}
|
||||
|
||||
const codeReviewer = {
|
||||
agentType: 'code-reviewer',
|
||||
source: 'built-in',
|
||||
getSystemPrompt: () => 'review code',
|
||||
} as unknown as AgentDefinition
|
||||
|
||||
const context = makeToolUseContext([codeReviewer])
|
||||
|
||||
await expect(
|
||||
resumeAgentBackground({
|
||||
agentId: 'test-agent',
|
||||
prompt: 'continue',
|
||||
toolUseContext: context,
|
||||
canUseTool: async () => ({ behavior: 'allow' } as any),
|
||||
}),
|
||||
).rejects.toThrow(
|
||||
"Cannot resume agent: identity mismatch. Expected source 'undefined', found 'built-in' for type 'code-reviewer'."
|
||||
)
|
||||
})
|
||||
|
||||
test('successfully resumes when legacy metadata lacks a source and agent is not built-in', async () => {
|
||||
mockMetadata = {
|
||||
agentType: 'custom-agent',
|
||||
// Legacy metadata lacks a source field
|
||||
}
|
||||
|
||||
const customAgent = {
|
||||
agentType: 'custom-agent',
|
||||
source: 'projectSettings', // Non-built-in source
|
||||
getSystemPrompt: () => 'do custom work',
|
||||
} as unknown as AgentDefinition
|
||||
|
||||
const context = makeToolUseContext([customAgent])
|
||||
|
||||
const result = await resumeAgentBackground({
|
||||
agentId: 'test-agent',
|
||||
prompt: 'continue',
|
||||
toolUseContext: context,
|
||||
canUseTool: async () => ({ behavior: 'allow' } as any),
|
||||
})
|
||||
|
||||
expect(result.agentId).toBe('test-agent')
|
||||
})
|
||||
@@ -121,7 +121,14 @@ export async function resumeAgentBackground({
|
||||
const found = toolUseContext.options.agentDefinitions.activeAgents.find(
|
||||
a => a.agentType === meta.agentType,
|
||||
)
|
||||
selectedAgent = found ?? GENERAL_PURPOSE_AGENT
|
||||
if (!found) {
|
||||
throw new Error(`Cannot resume agent: type '${meta.agentType}' is unavailable or disabled in the current session.`)
|
||||
}
|
||||
const isLegacyMatch = meta.source === undefined && found.source !== 'built-in'
|
||||
if (meta.source !== found.source && !isLegacyMatch) {
|
||||
throw new Error(`Cannot resume agent: identity mismatch. Expected source '${meta.source}', found '${found.source}' for type '${meta.agentType}'.`)
|
||||
}
|
||||
selectedAgent = found
|
||||
} else {
|
||||
selectedAgent = GENERAL_PURPOSE_AGENT
|
||||
}
|
||||
|
||||
@@ -0,0 +1,99 @@
|
||||
import { afterEach, beforeEach, expect, mock, test } from 'bun:test'
|
||||
import type { AgentDefinition } from './loadAgentsDir.js'
|
||||
import type { ToolUseContext } from '../../Tool.js'
|
||||
|
||||
// Track how many times each storage function is called
|
||||
let metadataWriteCount = 0
|
||||
let transcriptWriteCount = 0
|
||||
|
||||
beforeEach(() => {
|
||||
metadataWriteCount = 0
|
||||
transcriptWriteCount = 0
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
mock.restore()
|
||||
})
|
||||
|
||||
async function importRunAgentWithMocks() {
|
||||
const sessionStorageMock = {
|
||||
writeAgentMetadata: async () => {
|
||||
metadataWriteCount++
|
||||
throw new Error('Simulated disk error during metadata write')
|
||||
},
|
||||
recordSidechainTranscript: async () => {
|
||||
transcriptWriteCount++
|
||||
},
|
||||
getAgentMetadataPath: () => '/mock/path',
|
||||
}
|
||||
|
||||
const queryMock = {
|
||||
query: async function* () {
|
||||
yield {
|
||||
type: 'assistant',
|
||||
uuid: 'msg-1',
|
||||
message: { role: 'assistant', content: 'test' },
|
||||
}
|
||||
yield {
|
||||
reason: 'done',
|
||||
}
|
||||
},
|
||||
}
|
||||
|
||||
mock.module('../../utils/sessionStorage.js', () => sessionStorageMock)
|
||||
mock.module('../../query.js', () => queryMock)
|
||||
|
||||
const module = await import(`./runAgent.js?persistence=${Date.now()}-${Math.random()}`)
|
||||
return module.runAgent
|
||||
}
|
||||
|
||||
test('skips transcript write if identity metadata fails to persist', async () => {
|
||||
const runAgent = await importRunAgentWithMocks()
|
||||
|
||||
const agentDefinition: AgentDefinition = {
|
||||
agentType: 'code-reviewer',
|
||||
source: 'built-in',
|
||||
} as AgentDefinition
|
||||
|
||||
const toolUseContext = {
|
||||
options: {
|
||||
mainLoopModel: 'test-model',
|
||||
agentDefinitions: { activeAgents: [agentDefinition] },
|
||||
tools: [],
|
||||
},
|
||||
getAppState: () => ({
|
||||
toolPermissionContext: { mode: 'default', additionalWorkingDirectories: new Map() },
|
||||
}),
|
||||
setAppState: () => {},
|
||||
} as unknown as ToolUseContext
|
||||
|
||||
const runParams = {
|
||||
agentId: 'test-agent' as any,
|
||||
agentDefinition,
|
||||
prompt: 'review this',
|
||||
promptMessages: [{ role: 'user', content: 'review this' }],
|
||||
toolUseContext,
|
||||
override: {
|
||||
abortController: new AbortController(),
|
||||
},
|
||||
querySource: 'subagent',
|
||||
canUseTool: async () => ({ behavior: 'allow' } as any),
|
||||
availableTools: [],
|
||||
allowedTools: undefined,
|
||||
} as any
|
||||
|
||||
const iterator = runAgent(runParams)
|
||||
let chunkCount = 0
|
||||
for await (const _ of iterator) {
|
||||
chunkCount++
|
||||
}
|
||||
|
||||
// The generator should still produce output despite the storage error
|
||||
expect(chunkCount).toBeGreaterThan(0)
|
||||
|
||||
// We should have attempted to write metadata and failed
|
||||
expect(metadataWriteCount).toBe(1)
|
||||
|
||||
// The transcript writes should have been skipped to prevent an orphaned transcript
|
||||
expect(transcriptWriteCount).toBe(0)
|
||||
})
|
||||
@@ -778,21 +778,36 @@ export async function* runAgent({
|
||||
})
|
||||
}
|
||||
|
||||
// Record initial messages before the query loop starts, plus the agentType
|
||||
// so resume can route correctly when subagent_type is omitted. Both writes
|
||||
// are fire-and-forget — persistence failure shouldn't block the agent.
|
||||
void recordSidechainTranscript(initialMessages, agentId).catch(_err =>
|
||||
logForDebugging(`Failed to record sidechain transcript: ${_err}`),
|
||||
)
|
||||
void writeAgentMetadata(agentId, {
|
||||
agentType: agentDefinition.agentType,
|
||||
...(worktreePath && { worktreePath }),
|
||||
// Keep explicit cwd even when a worktree exists so resume can fall back
|
||||
// to the child repo if the worktree is later removed.
|
||||
...(cwd && { cwd }),
|
||||
...(description && { description }),
|
||||
}).catch(_err => logForDebugging(`Failed to write agent metadata: ${_err}`))
|
||||
// Record agentType and identity metadata so resume can route correctly.
|
||||
// This must be awaited before writing the initial transcript so that any
|
||||
// resume attempt reading the transcript is guaranteed to find the metadata.
|
||||
let metadataWritten = false
|
||||
try {
|
||||
await writeAgentMetadata(agentId, {
|
||||
agentType: agentDefinition.agentType,
|
||||
source: agentDefinition.source,
|
||||
...(worktreePath && { worktreePath }),
|
||||
// Keep explicit cwd even when a worktree exists so resume can fall back
|
||||
// to the child repo if the worktree is later removed.
|
||||
...(cwd && { cwd }),
|
||||
...(description && { description }),
|
||||
})
|
||||
metadataWritten = true
|
||||
} catch (_err) {
|
||||
logForDebugging(`Failed to write agent metadata: ${_err}`)
|
||||
}
|
||||
|
||||
// Record initial messages before the query loop starts.
|
||||
// Fire-and-forget — persistence failure shouldn't block the agent.
|
||||
// Only write the transcript if identity metadata was successfully persisted,
|
||||
// ensuring we never leave a transcript that would resume without its restricted identity.
|
||||
if (metadataWritten) {
|
||||
void recordSidechainTranscript(initialMessages, agentId).catch(_err =>
|
||||
logForDebugging(`Failed to record sidechain transcript: ${_err}`),
|
||||
)
|
||||
} else {
|
||||
logForDebugging('Skipping initial transcript write because identity metadata persistence failed')
|
||||
}
|
||||
// Track the last recorded message UUID for parent chain continuity
|
||||
let lastRecordedUuid: UUID | null = initialMessages.at(-1)?.uuid ?? null
|
||||
|
||||
@@ -859,13 +874,16 @@ export async function* runAgent({
|
||||
|
||||
if (isRecordableMessage(message)) {
|
||||
// Record only the new message with correct parent (O(1) per message)
|
||||
await recordSidechainTranscript(
|
||||
[message],
|
||||
agentId,
|
||||
lastRecordedUuid,
|
||||
).catch(err =>
|
||||
logForDebugging(`Failed to record sidechain transcript: ${err}`),
|
||||
)
|
||||
// Only write if identity metadata was successfully persisted.
|
||||
if (metadataWritten) {
|
||||
await recordSidechainTranscript(
|
||||
[message],
|
||||
agentId,
|
||||
lastRecordedUuid,
|
||||
).catch(err =>
|
||||
logForDebugging(`Failed to record sidechain transcript: ${err}`),
|
||||
)
|
||||
}
|
||||
if (message.type !== 'progress') {
|
||||
lastRecordedUuid = message.uuid
|
||||
}
|
||||
|
||||
@@ -13,13 +13,11 @@ When in doubt about whether a task warrants a team, prefer spawning a team.
|
||||
|
||||
## Choosing Agent Types for Teammates
|
||||
|
||||
When spawning teammates via the Agent tool, choose the \`subagent_type\` based on what tools the agent needs for its task. Each agent type has a different set of available tools — match the agent to the work:
|
||||
When spawning teammates via the Agent tool (with \`team_name\` and \`name\`), follow these rules:
|
||||
|
||||
- **Read-only agents** (e.g., Explore, Plan) cannot edit or write files. Only assign them research, search, or planning tasks. Never assign them implementation work.
|
||||
- **Full-capability agents** (e.g., general-purpose) have access to all tools including file editing, writing, and bash. Use these for tasks that require making changes.
|
||||
- **Custom agents** defined in \`.openclaude/agents/\` may have their own tool restrictions. Check their descriptions to understand what they can and cannot do.
|
||||
|
||||
Always review the agent type descriptions and their available tools listed in the Agent tool prompt before selecting a \`subagent_type\` for a teammate.
|
||||
- **Omit \`subagent_type\`** to spawn a default full-capability teammate. This is the standard choice for tasks that require making changes — editing files, running bash, writing code.
|
||||
- **Set a custom \`subagent_type\`** only when you have a custom agent defined in \`.openclaude/agents/\` that fits the task. Check the agent's description and tool restrictions before selecting it.
|
||||
- **Do NOT use built-in types** (e.g., \`Explore\`, \`Plan\`, \`code-reviewer\`, \`general-purpose\`) as \`subagent_type\` on a teammate spawn. Built-in types are rejected with an error on the teammate path. To use Explore, Plan, or code-reviewer, call the Agent tool without \`name\` and \`team_name\` so it runs as a standard subagent, not a teammate.
|
||||
|
||||
Create a new team to coordinate multiple agents working on a project. Teams have a 1:1 correspondence with task lists (Team = TaskList).
|
||||
|
||||
|
||||
@@ -753,7 +753,7 @@ describe('plan mode mechanical read-only policy', () => {
|
||||
})
|
||||
|
||||
test.each([
|
||||
{ description: 'General', prompt: 'Work', subagent_type: 'general-purpose' },
|
||||
{ description: 'General', prompt: 'Work' },
|
||||
{ description: 'Fork', prompt: 'Work' },
|
||||
{ description: 'Team', prompt: 'Work', subagent_type: 'Explore', name: 'worker' },
|
||||
{ description: 'Team', prompt: 'Work', subagent_type: 'Plan', team_name: 'team' },
|
||||
|
||||
@@ -598,6 +598,9 @@ export type AgentMetadata = {
|
||||
* resumed agent's notification can show the original description instead
|
||||
* of a placeholder. Optional — older metadata files lack this field. */
|
||||
description?: string
|
||||
/** Source of the agent definition (e.g. 'built-in', 'projectSettings').
|
||||
* Used on resume to verify the resolved definition matches the original. */
|
||||
source?: string
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -615,7 +618,7 @@ export async function writeAgentMetadata(
|
||||
): Promise<void> {
|
||||
const path = getAgentMetadataPath(agentId)
|
||||
await mkdir(dirname(path), { recursive: true })
|
||||
await writeFile(path, JSON.stringify(metadata))
|
||||
await replaceFileAtomic(path, JSON.stringify(metadata))
|
||||
}
|
||||
|
||||
export async function readAgentMetadata(
|
||||
|
||||
Reference in New Issue
Block a user