feat(tool-approval): add approval plugin and playgrounds#21
feat(tool-approval): add approval plugin and playgrounds#21exoticknight wants to merge 1 commit into
Conversation
…a agents - Implemented the core functionality for tool approval, allowing agents to manage tool calls based on defined policies. - Created README documentation detailing installation, usage, and API. - Added package.json for plugin metadata and dependencies. - Developed the main plugin logic in src/index.ts, including approval decision-making and state management. - Included tests to validate approval behavior and policy enforcement. - Updated pnpm-lock.yaml to include new dependencies for examples and testing.
There was a problem hiding this comment.
Code Review
This pull request introduces a new tool approval plugin and associated examples for the Apeira agent framework. Key changes include updates to the core plugin system to support preToolCall and postToolCall hooks, the addition of private state management for plugins, and the implementation of the @apeira/plugin-tool-approval package. Feedback focuses on improving the robustness of the stableStringify utility to handle undefined values and locale-independent sorting, addressing a potential memory leak in the session version tracking map, and mitigating XSS vulnerabilities in the UI playground caused by unsanitized innerHTML usage.
| const stableStringify = (value: unknown): string => { | ||
| if (value == null || typeof value !== 'object') | ||
| return JSON.stringify(value) | ||
|
|
||
| if (Array.isArray(value)) | ||
| return `[${value.map(item => stableStringify(item)).join(',')}]` | ||
|
|
||
| const serializedEntries = Object.entries(value as Record<string, unknown>) | ||
| .sort(([a], [b]) => a.localeCompare(b)) | ||
| .map(([key, item]) => { | ||
| const serialized = stableStringify(item) | ||
| return `${JSON.stringify(key)}:${serialized}` | ||
| }) | ||
| .join(',') | ||
|
|
||
| return `{${serializedEntries}}` | ||
| } |
There was a problem hiding this comment.
The stableStringify implementation has a few issues:
- It doesn't handle
undefinedvalues correctly within arrays or objects. String interpolation ofundefinedresults in the string"undefined", andjoin(',')on an array containingundefinedresults in invalid JSON-like strings (e.g.,[1,,2]). localeCompareis locale-dependent, which might lead to different keys for the same input across different environments.- It doesn't filter out
undefinedproperties in objects, whichJSON.stringifynormally does.
Using a more robust implementation that matches JSON.stringify behavior for undefined and uses a stable sort order is recommended.
const stableStringify = (value: unknown): string => {
if (typeof value !== 'object' || value === null) {
return JSON.stringify(value) ?? 'null'
}
if (Array.isArray(value)) {
return `[${value.map(item => (item === undefined ? 'null' : stableStringify(item))).join(',')}]`
}
const keys = Object.keys(value).sort()
const entries: string[] = []
for (const key of keys) {
const val = (value as Record<string, unknown>)[key]
if (val !== undefined) {
entries.push(`${JSON.stringify(key)}:${stableStringify(val)}`)
}
}
return `{${entries.join(',')}}`
}| const clearedSessionVersions = new Map<string, number>() | ||
| const turnAllows = new Map<string, Set<string>>() | ||
|
|
||
| const applyPendingConversationClear = (privateState: PluginPrivateStateApi | undefined, sessionId: string) => { | ||
| if (clearConversationHistoryVersion === 0) | ||
| return | ||
|
|
||
| if (clearedSessionVersions.get(sessionId) === clearConversationHistoryVersion) | ||
| return | ||
|
|
||
| setPrivateState(privateState, createInitialState()) | ||
| clearedSessionVersions.set(sessionId, clearConversationHistoryVersion) | ||
| } |
There was a problem hiding this comment.
The clearedSessionVersions map grows indefinitely as new sessions are processed, leading to a memory leak in long-running processes (like a server).
A better approach is to store the clearedVersion within the session's private state itself. This allows each session to lazily clear its own history when it encounters a higher global clearConversationHistoryVersion, without the plugin needing to track every session ID globally in a map that never gets cleaned up.
| const renderedMessages = state.messages.map(message => ` | ||
| <article class="message ${message.kind}"> | ||
| <span class="label">${message.kind}</span> | ||
| ${message.text} | ||
| </article> | ||
| `).join('') || '<p class="empty-chat">Send a message to start the fake agent turn.</p>' |
There was a problem hiding this comment.
Using innerHTML to render messages and other dynamic content without sanitization introduces a Cross-Site Scripting (XSS) vulnerability. If the agent output or user input contains malicious HTML/scripts, they will be executed in the browser.
Even for a playground, it's best practice to escape dynamic content or use textContent where possible to prevent script injection.
architecture
Core now owns the generic tool-call control pipeline. It does not know about approval policy semantics. It only asks plugins whether a tool call should continue or be blocked, then reports the final tool-call status through postToolCall.
The approval plugin is implemented as a normal plugin on top of that pipeline. It owns approval modes, policy decisions, approval key generation, remembered allow scopes, and decision events. Conversation-level history is stored in plugin private state instead of normal context, so regular setContext() updates cannot forge trusted approvals.
Tool metadata is intentionally kept outside existing tool plugins by using approval hint wrappers. Existing plugins do not need to know whether approval is enabled; the agent/plugin user can wrap tools or plugins with approval hints when they want richer approval UI and policy matching.
feat
preToolCall/postToolCallpipeline around resolved tools, includingcontinue,block,success,error, andblockedstatuses.@apeira/plugin-tool-approvalwithoff,allow,ask, anddenymodes, runtime policy updates, approval keys, andonce/turn/conversationscopes.chore
verification
node_modules\.bin\tsc.CMD --noEmit --pretty falsenode_modules\.bin\vitest.CMD run packages\core\test\index.test.ts packages\plugin-tool-approval\test\index.test.tsnode_modules\.bin\eslint.CMD --flag unstable_native_nodejs_ts_config .