-
Notifications
You must be signed in to change notification settings - Fork 10
feat(agent): consume MCP tool registry (Operations profile), RBAC-filtered #893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,8 +9,9 @@ | |
| * | ||
| * The component intentionally avoids `handleApplication`: it has nothing | ||
| * worker-thread-shaped to do. Operator-only tools (FS, schedule, fetch) are | ||
| * inline; registry-backed tools (#615/#617/#618) will fold in via toolset.ts | ||
| * once those land. | ||
| * inline; RBAC-filtered tools from the unified MCP registry (#615/#781, | ||
| * Operations profile #617) fold in via `registryTools.ts` for the agent's | ||
| * configured user. | ||
| */ | ||
|
|
||
| import { dirname, isAbsolute, resolve as resolvePath } from 'node:path'; | ||
|
|
@@ -21,6 +22,9 @@ import { composeToolset } from './toolset.ts'; | |
| import { buildOperations } from './operations.ts'; | ||
| import { runAgent, _resetInFlightForTests } from './loop.ts'; | ||
| import { appendMessage, getSession } from './session.ts'; | ||
| import { ensureOperationsToolsRegistered, composeRegistryTools } from './registryTools.ts'; | ||
| import { server } from '../server/Server.ts'; | ||
| import type { AuthedUser } from '../components/mcp/toolRegistry.ts'; | ||
| import type { AgentConfig, AgentScopes, AgentTool } from './types.ts'; | ||
|
|
||
| const log = harperLogger.loggerWithTag('agent'); | ||
|
|
@@ -64,10 +68,22 @@ export async function startOnMainThread(opts: StartOpts): Promise<void> { | |
| const models = new Models(); | ||
| const abortControllers = new Map<string, AbortController>(); | ||
| let liveConfig: AgentConfig = config; | ||
|
|
||
| // Populate the MCP registry's Operations profile (idempotent) and resolve the agent's RBAC | ||
| // identity, then pull the RBAC-filtered registry tools for that user. Computed once at startup: | ||
| // the agent acts as a single configured user, and the Operations profile is registered once on | ||
| // the main thread. (Application-profile per-Resource tools are a follow-up — they populate from | ||
| // the worker-thread Resources registry.) | ||
| ensureOperationsToolsRegistered(); | ||
| const agentUser = await resolveAgentUser(liveConfig.user); | ||
| const registryTools = composeRegistryTools(agentUser, `agent:registry:${liveConfig.user}`); | ||
|
|
||
| let composed = composeToolset({ | ||
| allowDestructive: liveConfig.allowDestructive, | ||
| onFollowup: handleFollowup, | ||
| registryTools, | ||
| }); | ||
| log.info?.(`Agent: composed ${registryTools.length} RBAC-filtered registry tool(s) for user '${liveConfig.user}'`); | ||
|
|
||
| // Only warn when the operator explicitly configured `maxCostUsd`. Logging on the default | ||
| // every boot would flood the log without telling anyone anything actionable. | ||
|
|
@@ -140,6 +156,7 @@ export async function startOnMainThread(opts: StartOpts): Promise<void> { | |
| composed = composeToolset({ | ||
| allowDestructive: liveConfig.allowDestructive, | ||
| onFollowup: handleFollowup, | ||
| registryTools, | ||
| }); | ||
| } | ||
| // NOTE: an already in-flight run captured its toolset (and autoApprove) at start, so flipping | ||
|
|
@@ -161,6 +178,30 @@ export async function startOnMainThread(opts: StartOpts): Promise<void> { | |
| log.info?.(`Agent component initialized with ${composed.tools.length} tools`); | ||
| } | ||
|
|
||
| /** | ||
| * Resolve the agent's configured user to its full permission object so registry tools are | ||
| * RBAC-filtered correctly. Falls back to a super_user identity (the documented `hdb_agent` | ||
| * default) if the user can't be loaded yet — e.g. the system user hasn't been created, or the | ||
| * security layer isn't initialized in a given context. A restricted role that fails to load would | ||
| * over-grant under this fallback, so the failure is logged at warn. | ||
| */ | ||
| async function resolveAgentUser(username: string): Promise<AuthedUser> { | ||
| try { | ||
| // `server.getUser(username)` resolves the stored user incl. role/permissions. Typed loosely | ||
| // here because Server.ts declares extra auth params this lookup-only call doesn't need. | ||
| const getUser = (server as any).getUser as ((u: string) => Promise<AuthedUser>) | undefined; | ||
| if (typeof getUser === 'function') { | ||
| const user = await getUser(username); | ||
| if (user?.role?.permission) return user; | ||
| } | ||
| } catch (err) { | ||
| log.warn?.( | ||
| `Agent: could not resolve user '${username}' (${(err as Error)?.message ?? err}); using super_user fallback` | ||
| ); | ||
| } | ||
| return { username, role: { permission: { super_user: true } } }; | ||
| } | ||
|
Comment on lines
+188
to
+203
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current fallback logic in We should only fall back to async function resolveAgentUser(username: string): Promise<AuthedUser> {
try {
// server.getUser(username) resolves the stored user incl. role/permissions. Typed loosely
// here because Server.ts declares extra auth params this lookup-only call doesn't need.
const getUser = (server as any).getUser as ((u: string) => Promise<AuthedUser>) | undefined;
if (typeof getUser === 'function') {
const user = await getUser(username);
if (user) return user;
}
} catch (err) {
log.warn?.(
"Agent: could not resolve user '" + username + "' (" + ((err as Error)?.message ?? err) + ")"
);
}
if (username === 'hdb_agent') {
return { username, role: { permission: { super_user: true } } };
}
throw new Error("Agent: failed to resolve non-default user '" + username + "' and refused super_user fallback");
} |
||
|
|
||
| function resolveScopes( | ||
| config: AgentConfig, | ||
| getConfigPath: (param: string) => string | undefined, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| /** | ||
| * Bridges the unified MCP tool registry (#615/#781, Operations profile #617) | ||
| * into the built-in agent's toolset. | ||
| * | ||
| * The agent consumes the SAME registry the MCP server exposes, RBAC-filtered | ||
| * for the agent's configured user — so an operator who points `agent.user` at | ||
| * a restricted role automatically gets a narrowed tool surface, with no | ||
| * per-tool wiring here. Operator-only tools (FS, schedule, fetch) stay inline | ||
| * in `toolset.ts`; they are intentionally NOT in the registry. | ||
| * | ||
| * Scope (v1): the **Operations** profile only. Those tools are registered on | ||
| * the main thread (where the agent runs) by `registerOperationsTools()`, which | ||
| * is idempotent — so the agent populates them itself rather than depending on | ||
| * the operator having enabled the `mcp:` HTTP surface. The **Application** | ||
| * profile (#618, per-Resource tools) is populated from the worker-thread | ||
| * Resources registry and is a follow-up. | ||
| */ | ||
|
|
||
| import harperLogger from '../utility/logging/harper_logger.ts'; | ||
| import { getTool, listTools, type AuthedUser, type ToolResult } from '../components/mcp/toolRegistry.ts'; | ||
| import { registerOperationsTools } from '../components/mcp/tools/operations.ts'; | ||
| import type { AgentTool, AgentToolContext } from './types.ts'; | ||
|
|
||
| const log = harperLogger.loggerWithTag('agent'); | ||
| const REGISTRY_PAGE_SIZE = 200; | ||
|
|
||
| /** | ||
| * Ensure the Operations-profile tools exist in the registry. Idempotent | ||
| * (`addTool` is `Map.set`-backed), so it's safe whether or not the MCP | ||
| * component already registered them. Failures are logged, not thrown — a | ||
| * registry hiccup must not stop the agent from starting with its inline tools. | ||
| */ | ||
| export function ensureOperationsToolsRegistered(): void { | ||
| try { | ||
| registerOperationsTools(); | ||
| } catch (err) { | ||
| log.warn?.(`Agent: failed to populate operations tools in MCP registry: ${(err as Error)?.message ?? err}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Build {@link AgentTool}s from the registry's Operations profile, filtered to | ||
| * what `agentUser` may invoke. Pagination is drained fully — the agent wants | ||
| * every visible tool, not a page. | ||
| */ | ||
| export function composeRegistryTools(agentUser: AuthedUser, sessionId: string): AgentTool[] { | ||
| const tools: AgentTool[] = []; | ||
| let cursor: string | undefined; | ||
| do { | ||
| const page = listTools({ user: agentUser, profile: 'operations', sessionId, cursor, limit: REGISTRY_PAGE_SIZE }); | ||
| for (const descriptor of page.tools) { | ||
| tools.push( | ||
| adaptRegistryTool( | ||
| descriptor.name, | ||
| descriptor.description, | ||
| descriptor.inputSchema, | ||
| agentUser, | ||
| descriptor.annotations?.destructiveHint === true | ||
| ) | ||
| ); | ||
| } | ||
| cursor = page.nextCursor; | ||
| } while (cursor); | ||
| return tools; | ||
| } | ||
|
|
||
| function adaptRegistryTool( | ||
| name: string, | ||
| description: string, | ||
| inputSchema: object, | ||
| agentUser: AuthedUser, | ||
| destructive: boolean | ||
| ): AgentTool { | ||
| return { | ||
| def: { name, description, parameters: inputSchema }, | ||
| destructive, | ||
| handler: async (args: object, ctx: AgentToolContext) => { | ||
| const tool = getTool(name); | ||
| if (!tool) throw new Error(`registry tool '${name}' no longer registered`); | ||
| const result: ToolResult = await tool.handler(args ?? {}, { | ||
| user: agentUser, | ||
| profile: 'operations', | ||
| sessionId: ctx.sessionId, | ||
| }); | ||
| if (result.isError) { | ||
| // Surface the registry tool's error text to the loop, which records it as a structured | ||
| // failure observation (the model can then adjust) rather than aborting the run. | ||
| const text = result.content?.find((c) => c.type === 'text')?.text ?? `tool '${name}' failed`; | ||
| throw new Error(text); | ||
| } | ||
| // Prefer the structured payload; fall back to concatenated text content. | ||
| if (result.structuredContent !== undefined) return result.structuredContent; | ||
| return result.content?.map((c) => c.text ?? '').join('') ?? ''; | ||
| }, | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| 'use strict'; | ||
|
|
||
| const assert = require('node:assert/strict'); | ||
| const { addTool, _resetRegistryForTest } = require('#src/components/mcp/toolRegistry'); | ||
| const { composeRegistryTools } = require('#src/agent/registryTools'); | ||
| const { composeToolset } = require('#src/agent/toolset'); | ||
|
|
||
| const superUser = { username: 'hdb_agent', role: { permission: { super_user: true } } }; | ||
| const limitedUser = { username: 'ro', role: { permission: { operations: ['describe_all'] } } }; | ||
|
|
||
| function fakeOpTool(name, overrides = {}) { | ||
| return { | ||
| name, | ||
| description: `op ${name}`, | ||
| inputSchema: { type: 'object' }, | ||
| profile: 'operations', | ||
| visibleTo: () => true, | ||
| handler: async () => ({ content: [{ type: 'text', text: 'ok' }], structuredContent: { ran: name } }), | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| describe('agent/registryTools', () => { | ||
| beforeEach(() => _resetRegistryForTest()); | ||
| afterEach(() => _resetRegistryForTest()); | ||
|
|
||
| it('adapts visible operations-profile tools to AgentTools', async () => { | ||
| addTool(fakeOpTool('describe_all')); | ||
| addTool(fakeOpTool('search')); | ||
| const tools = composeRegistryTools(superUser, 'sess'); | ||
| const names = tools.map((t) => t.def.name).sort(); | ||
| assert.deepEqual(names, ['describe_all', 'search']); | ||
| // parameters carries the registry inputSchema | ||
| assert.deepEqual(tools[0].def.parameters, { type: 'object' }); | ||
| }); | ||
|
|
||
| it('respects RBAC visibleTo — excludes tools the user cannot invoke', async () => { | ||
| addTool(fakeOpTool('describe_all', { visibleTo: () => true })); | ||
| addTool(fakeOpTool('drop_table', { visibleTo: (u) => u.role?.permission?.super_user === true })); | ||
| const visible = composeRegistryTools(limitedUser, 'sess').map((t) => t.def.name); | ||
| assert.deepEqual(visible, ['describe_all']); | ||
| const all = composeRegistryTools(superUser, 'sess') | ||
| .map((t) => t.def.name) | ||
| .sort(); | ||
| assert.deepEqual(all, ['describe_all', 'drop_table']); | ||
| }); | ||
|
|
||
| it('maps destructiveHint annotation to the AgentTool destructive flag', async () => { | ||
| addTool(fakeOpTool('restart', { annotations: { destructiveHint: true } })); | ||
| addTool(fakeOpTool('describe_all', { annotations: { readOnlyHint: true } })); | ||
| const tools = composeRegistryTools(superUser, 'sess'); | ||
| const restart = tools.find((t) => t.def.name === 'restart'); | ||
| const describe = tools.find((t) => t.def.name === 'describe_all'); | ||
| assert.equal(restart.destructive, true); | ||
| assert.notEqual(describe.destructive, true); | ||
| }); | ||
|
|
||
| it('adapter handler returns structuredContent on success', async () => { | ||
| addTool(fakeOpTool('search')); | ||
| const [tool] = composeRegistryTools(superUser, 'sess'); | ||
| const result = await tool.handler({ q: 1 }, { sessionId: 'sess', scopes: {} }); | ||
| assert.deepEqual(result, { ran: 'search' }); | ||
| }); | ||
|
|
||
| it('adapter handler throws the error text when the registry tool returns isError', async () => { | ||
| addTool( | ||
| fakeOpTool('drop_table', { | ||
| handler: async () => ({ isError: true, content: [{ type: 'text', text: 'permission denied' }] }), | ||
| }) | ||
| ); | ||
| const [tool] = composeRegistryTools(superUser, 'sess'); | ||
| await assert.rejects(tool.handler({}, { sessionId: 'sess', scopes: {} }), /permission denied/); | ||
| }); | ||
|
|
||
| it('composeToolset drops a registry tool that collides with an operator-only tool name', () => { | ||
| // `read_file` is an operator-only FS tool; a registry tool of the same name must not shadow it. | ||
| const fakeRegistryReadFile = { | ||
| def: { name: 'read_file', description: 'registry shadow', parameters: { type: 'object' } }, | ||
| handler: async () => ({ shadow: true }), | ||
| }; | ||
| const { tools } = composeToolset({ | ||
| onFollowup: () => {}, | ||
| registryTools: [fakeRegistryReadFile], | ||
| }); | ||
| const readFileTools = tools.filter((t) => t.def.name === 'read_file'); | ||
| assert.equal(readFileTools.length, 1); | ||
| assert.equal( | ||
| readFileTools[0].def.description, | ||
| 'Read a UTF-8 text file within componentsRoot, logDir, or configDir.' | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change
consttoletso thatregistryToolscan be reassigned when the configured user is dynamically updated viasetConfig.