feat(agent): consume MCP tool registry (Operations profile), RBAC-filtered (#626)

[kriszyp]

Summary

Wires the unified MCP tool registry's Operations profile (#617) into the built-in Harper agent (#626), fulfilling the tool-composition design: the agent consumes the same registry the MCP server exposes, RBAC-filtered for its configured user. This re-does the intent of the earlier #893 against current main (that branch was ~465 commits stale and used the pre-merge registry API).

Scope is deliberately Operations-profile only — the built-in agent runs on the main thread adjacent to the operations API server. The Application profile (#618, per-Resource tools, worker-thread-shaped) is out of scope by design.

What it does

• agent/registryTools.ts (new)
  • ensureOperationsToolsRegistered() — populates the Operations profile on the main thread. Idempotent (addTool is Map.set-backed), so the agent gets these tools whether or not the operator enabled the mcp: HTTP surface.
  • composeRegistryTools(listingUser, resolveIdentity) — snapshots the Operations profile, filters by visibleTo(listingUser), and adapts each ToolDef → AgentTool: inputSchema → parameters, annotations.destructiveHint → the loop's approval gate, and a handler wrapping the registry handler (ToolResult.structuredContent/text as the observation; isError → throw, so the loop records a recoverable failure rather than aborting the run).
• agent/agent.ts — resolves the agent identity and threads registry tools into composeToolset.
• agent/toolset.ts — merges operator-only + registry tools; operator-only tools win on a name collision (a private tool is never shadowed).
• unitTests/agent/registryTools.test.js — 12 tests: RBAC filtering, shape adaptation, destructive gating, result unwrap/throw, per-call re-resolution, fail-closed propagation, and the collision/allowDestructive merge rules. No server boot, no LLM credits.

The six agent operations (agent_prompt, get_agent_session, list_agent_sessions, cancel_agent_run, approve_agent_action, set_agent_config) already landed with the scaffold (#839); this PR adds the tool surface they drive.

Security model

visibleTo controls listing only (which tools the LLM is shown) — it is not a boundary. Real enforcement runs per-call in the operation handler via hdb_user set to the agent's identity, gated by Harper's existing verifyPerms in the operations runtime.

Cross-model review (Codex + Gemini + Harper-domain)

Two authorization defects were caught and fixed at root in the second commit:

• Blocker — fail-open super_user. The identity fallback previously fabricated { super_user: true } whenever agent.user couldn't be resolved, silently escalating a misconfigured/transient restricted service account to admin. Now the super_user fallback is bounded to the default hdb_agent bootstrap user (whose provisioning Built-in Harper Agent Component #626 defers); an explicitly-configured user that won't resolve fails closed (agent runs with only operator-only tools).
• Significant — stale cached identity. The agent user was resolved once at startup and closed over in every handler, so a role revocation wasn't honored until restart. Identity is now resolved per call (resolveIdentity thunk), mirroring how the MCP HTTP path re-auths per request. The startup snapshot is used only for visibleTo listing.
• Adjudicated out: Gemini's "setConfig ignores agent.user changes" — set_agent_config doesn't accept user, so it can't change at runtime.

Known follow-ups (not in this PR)

• Provision the hdb_agent system user at startup so the default agent resolves to a real user instead of relying on the bootstrap fallback. Built-in Harper Agent Component #626 explicitly defers this; the fail-closed policy above is the interim guard.
• agent.maxCostUsd remains advertised-but-not-enforced (depends on Add agent-loop orchestration / toolMode: 'auto' to scope.models #612 telemetry).

Verification

• npm run build clean.
• Full agent unit suite (unitTests/agent/**): 51 passing.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request integrates the unified MCP tool registry's Operations profile into the built-in agent, allowing RBAC-filtered tools to be composed and executed based on the agent's configured user identity. Feedback on these changes highlights a bug where updating the agent's user configuration does not re-compose the tool listing snapshot, potentially leaving the LLM with stale tools. Additionally, suggestions were made to add defensive checks when handling tool execution results and parsing content to prevent potential runtime errors.

[gemini-code-assist]

When liveConfig.user is updated via setConfig, the registryTools listing snapshot is not re-composed. This results in a stale tool listing for the LLM (which remains filtered by visibleTo for the old user), even though the enforcement identity is correctly resolved fresh per call.

Since setConfig is synchronous and composeRegistryToolsForListing is asynchronous, you can trigger the async re-composition and update the toolset once it resolves:

	function setConfig(patch: Partial<AgentConfig>): AgentConfig {
		const previousAllowDestructive = liveConfig.allowDestructive;
		const previousUser = liveConfig.user;
		liveConfig = { ...liveConfig, ...patch };

		const userChanged = liveConfig.user !== previousUser;
		const destructiveChanged = liveConfig.allowDestructive !== previousAllowDestructive;

		if (userChanged) {
			composeRegistryToolsForListing(agentIdentity)
				.then((tools) => {
					registryTools = tools;
					composed = composeToolset({
						allowDestructive: liveConfig.allowDestructive,
						onFollowup: handleFollowup,
						registryTools,
					});
				})
				.catch((err) => {
					log.error?.(`Failed to update registry tools for new user: ${err instanceof Error ? err.message : String(err)}`);
				});
		} else if (destructiveChanged) {
			composed = composeToolset({
				allowDestructive: liveConfig.allowDestructive,
				onFollowup: handleFollowup,
				registryTools,
			});
		}
		return liveConfig;
	}

[gemini-code-assist]

To prevent potential TypeErrors if a tool handler returns an empty or invalid result, add a defensive check to ensure result is defined before accessing its properties.

			const result = await def.handler(args, context);
			if (!result) {
				throw new Error('Operation returned an empty or invalid result');
			}
			// The loop records a tool observation from the return value and turns a thrown error
			// into a structured '{ ok: false, error }' note for the next model turn. Mapping
			// 'isError' -> throw keeps a Harper operation failure a recoverable observation rather
			// than aborting the whole run.
			if (result.isError) throw new Error(errorText(result));
			return result.structuredContent ?? textOf(result);

References

1. When accessing properties of a result from a method that might return nullish, guard against nullish values to prevent runtime errors.

[gemini-code-assist]

If result.content contains any null or undefined elements, accessing c.type will throw a TypeError. Add a defensive check to ensure c is defined before accessing its properties.

Suggested change

	function textOf(result: ToolResult): string {
	return (result.content ?? [])
	.filter((c) => c.type === 'text' && typeof c.text === 'string')
	.map((c) => c.text)
	.join('\n');
	}
	function textOf(result: ToolResult): string {
	return (result.content ?? [])
	.filter((c) => c && c.type === 'text' && typeof c.text === 'string')
	.map((c) => c.text)
	.join('\n');
	}

[claude]

Reviewed; no blockers found.

[dawsontoth]

Should this be token aware too? And if so, having a third parameter with an unknown type seems rather dangerous.

[dawsontoth]

From the other PR #1546

[claude]

Suggestion (non-blocking): swap to plain assert — AGENTS.md explicitly says not to use node:assert/strict ("strict mode's deep-equality and coercion rules cause more friction and surprising failures than they prevent; plain assert is the house style").

Suggested change

	const assert = require('node:assert/strict');
	const assert = require('node:assert');

[claude]

Suggestion (non-blocking): AGENTS.md prescribes plain assert over node:assert/strict — strict mode's deep-equality coercion is called out as causing friction, and all new test files in this repo use require('node:assert').

Suggested change

	const assert = require('node:assert/strict');
	const assert = require('node:assert');