fix(agents): address all PR #2299 code review findings

Blocking fixes:
- B1: Return empty restrictions for unknown/custom agents instead of
  EXPLORATION_AGENT_DENYLIST, allowing custom agents full tool access
- B2: Use Object.create(null) consistently across all 5 agent-loading
  result objects to prevent prototype pollution
- B3: Add code comment documenting custom agent bash access trust model
- B4: Mock getOpenCodeConfigDir in opencode-config-agents-reader tests
  to prevent global config dir leakage

Non-blocking fixes:
- N1: Use resolveAgentDefinitionPaths with project boundary enforcement
  in opencode-config-agents-reader for path containment
- N2: Add session-scoped 30s TTL cache to resolveCallableAgents to
  avoid redundant SDK IPC calls per tool invocation
- N3: Extract shared parseToolsConfig into src/shared/parse-tools-config.ts
  replacing 4 duplicated local implementations
- N4: Add .min(1) to AgentDefinitionPathSchema rejecting empty paths
- N5: Add resolve-agent-definition-paths.test.ts covering tilde expansion,
  relative paths, boundary enforcement, and null containmentDir
- N6: Validate agent mode against allowed values instead of bare type
  assertion in opencode-config-agents-reader
This commit is contained in:
YeonGyu-Kim
2026-04-15 10:41:32 +09:00
parent 4c77045c47
commit e5d3fe96c4
14 changed files with 214 additions and 82 deletions
+1
View File
@@ -76,3 +76,4 @@ export { SessionCategoryRegistry } from "./session-category-registry"
export * from "./plugin-identity"
export * from "./log-legacy-plugin-startup-warning"
export * from "./task-system-enabled"
export * from "./parse-tools-config"
+25
View File
@@ -0,0 +1,25 @@
/**
* Parses a tools configuration value into a boolean record.
* Accepts comma-separated strings, string arrays, or unknown values from config files.
* Returns undefined when input is empty or invalid.
*/
export function parseToolsConfig(toolsValue: unknown): Record<string, boolean> | undefined {
if (!toolsValue) return undefined
let items: string[]
if (typeof toolsValue === "string") {
items = toolsValue.split(",").map((t) => t.trim()).filter(Boolean)
} else if (Array.isArray(toolsValue)) {
items = toolsValue.filter((t) => typeof t === "string" && t.trim().length > 0).map((t) => (t as string).trim())
} else {
return undefined
}
if (items.length === 0) return undefined
const result: Record<string, boolean> = {}
for (const tool of items) {
result[tool.toLowerCase()] = true
}
return result
}
@@ -0,0 +1,122 @@
import { describe, test, expect, beforeEach, afterEach } from "bun:test"
import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "fs"
import { join } from "path"
import { homedir } from "os"
import { tmpdir } from "os"
import { resolveAgentDefinitionPaths } from "./resolve-agent-definition-paths"
describe("resolveAgentDefinitionPaths", () => {
let tempDir: string
beforeEach(() => {
tempDir = mkdtempSync(join(tmpdir(), "resolve-agent-def-paths-"))
})
afterEach(() => {
rmSync(tempDir, { recursive: true, force: true })
})
describe("#given relative paths", () => {
test("#then they are resolved against baseDir", () => {
const result = resolveAgentDefinitionPaths(
["agents/my-agent.md"],
tempDir,
null,
)
expect(result).toEqual([join(tempDir, "agents/my-agent.md")])
})
})
describe("#given absolute paths", () => {
test("#then they are returned as-is", () => {
const absPath = join(tempDir, "absolute-agent.md")
const result = resolveAgentDefinitionPaths(
[absPath],
"/some/other/base",
null,
)
expect(result).toEqual([absPath])
})
})
describe("#given tilde-prefixed paths", () => {
test("#then ~ is expanded to homedir", () => {
const result = resolveAgentDefinitionPaths(
["~/agents/test.md"],
tempDir,
null,
)
expect(result).toEqual([join(homedir(), "agents/test.md")])
})
})
describe("#given containmentDir is set", () => {
test("#then paths outside the boundary are rejected", () => {
const projectDir = join(tempDir, "project")
mkdirSync(projectDir, { recursive: true })
const result = resolveAgentDefinitionPaths(
["/etc/passwd"],
projectDir,
projectDir,
)
expect(result).toEqual([])
})
test("#then paths inside the boundary are allowed", () => {
const projectDir = join(tempDir, "project")
const agentsDir = join(projectDir, "agents")
mkdirSync(agentsDir, { recursive: true })
writeFileSync(join(agentsDir, "a.md"), "test", "utf-8")
const result = resolveAgentDefinitionPaths(
["agents/a.md"],
projectDir,
projectDir,
)
expect(result).toEqual([join(projectDir, "agents/a.md")])
})
})
describe("#given containmentDir is null", () => {
test("#then no boundary check is applied", () => {
const result = resolveAgentDefinitionPaths(
["/some/outside/path/agent.md"],
tempDir,
null,
)
expect(result).toEqual(["/some/outside/path/agent.md"])
})
})
describe("#given an empty paths array", () => {
test("#then an empty array is returned", () => {
const result = resolveAgentDefinitionPaths([], tempDir, null)
expect(result).toEqual([])
})
})
describe("#given mixed valid and invalid paths", () => {
test("#then only valid paths within the boundary are returned", () => {
const projectDir = join(tempDir, "project")
mkdirSync(projectDir, { recursive: true })
const result = resolveAgentDefinitionPaths(
["./valid.md", "/outside/boundary.md"],
projectDir,
projectDir,
)
expect(result).toEqual([join(projectDir, "valid.md")])
})
})
})