feat: multi-instance OpenClaw orchestration#15
Conversation
Add InstanceRegistry to support orchestrating multiple OpenClaw gateways from a single MCP server. Fully backward-compatible — existing single-instance deployments work without any configuration change. New features: - OPENCLAW_INSTANCES env var for named instance configuration - Optional `instance` parameter on all gateway-facing tools - New `openclaw_instances` tool to list available instances - Instance-scoped async tasks with instanceId tracking - Instance filter on openclaw_task_list Architecture: - InstanceRegistry class wraps Map<name, OpenClawClient> - Single-instance mode auto-creates "default" registry entry from existing env vars - Tokens stay in env vars, never flow through MCP protocol - 13 new tests for registry, 128 total passing
There was a problem hiding this comment.
CodeForge Review
Verdict: request_changes | Score: 7/10
Solid multi-instance feature with clean architecture — InstanceRegistry is well-designed, backward compat is preserved, and tokens are kept out of list responses. Two issues need addressing before merge: the OPENCLAW_INSTANCES JSON is cast without runtime field validation (allowing undefined fields to silently slip through the name regex as the string 'undefined'), and multi-instance configs don't inherit the CLI --timeout fallback, creating a silent behavioral gap. Test coverage for the new tool handlers is also missing.
Reviewed by CodeForge
| if (!Array.isArray(parsed) || parsed.length === 0) { | ||
| throw new Error('OPENCLAW_INSTANCES must be a non-empty JSON array'); | ||
| } | ||
| instances = parsed as InstanceConfig[]; |
There was a problem hiding this comment.
[MAJOR] OPENCLAW_INSTANCES is parsed then immediately cast with as InstanceConfig[] — no runtime validation of required fields. If an element is missing name, config.name is undefined at runtime; INSTANCE_NAME_RE.test(undefined) coerces it to the string 'undefined', which matches the regex (^[a-zA-Z0-9]…), so the invalid config passes silently and an instance named 'undefined' is registered. Likewise a missing url reaches new OpenClawClient(undefined, …) and fails later with a confusing error.
Suggestion: Validate each element after parsing: check
typeof config.name === 'string'andtypeof config.url === 'string'(and non-empty) before passing to InstanceRegistry. A minimal guard:if (!config || typeof config.name !== 'string' || !config.name || typeof config.url !== 'string' || !config.url) throw new Error('Each instance must have a string name and url').
| } | ||
| } else { | ||
| // Backward-compatible: single instance from existing env vars / CLI args | ||
| instances = [ |
There was a problem hiding this comment.
[MINOR] When OPENCLAW_INSTANCES is used, per-instance configs that omit timeout receive undefined, meaning the OpenClawClient falls back to whatever its internal default is — not the CLI --timeout / OPENCLAW_TIMEOUT_MS value. The single-instance path always sets timeout: argv.timeout. This silent inconsistency will surprise operators who set a custom timeout globally.
Suggestion: Apply the global timeout as a fallback for each instance:
timeout: config.timeout ?? argv.timeoutwhen building the InstanceConfig array, or document clearly that per-instance timeout must be set explicitly when OPENCLAW_INSTANCES is used.
| const client = this.get(name); | ||
| if (!client) { | ||
| const available = this.listNames().join(', '); | ||
| throw new Error(`Unknown instance "${name}". Available: ${available}`); |
There was a problem hiding this comment.
[MINOR] The error thrown by resolve() for an unknown instance name includes the full list of configured instance names: Available: ${available}. In a multi-tenant or production deployment this leaks internal topology to any caller who can trigger the error (e.g., by passing an arbitrary instance parameter to a tool).
Suggestion: Either omit the 'Available' hint entirely, or restrict it to single-instance situations where there is no meaningful information to protect:
throw new Error('Unknown instance "' + name + '"').
| let processorRunning = false; | ||
| let processorClient: OpenClawClient | null = null; | ||
| let processorRegistry: InstanceRegistry | null = null; | ||
|
|
There was a problem hiding this comment.
[MINOR] processTask calls registry.resolve(task.instanceId) where task.instanceId is string | undefined. If the instance that was targeted when the task was created is later removed (e.g., config reload), this throws and leaves the task in 'running' state permanently because the catch block in processTask only catches errors from client.chat.
Suggestion: Wrap the
registry.resolvecall in its own try/catch insideprocessTaskand fail the task explicitly:taskManager.updateStatus(task.id, 'failed', 'Instance no longer available')before returning.
| @@ -0,0 +1,102 @@ | |||
| import { describe, it, expect } from 'vitest'; | |||
There was a problem hiding this comment.
[MINOR] New test file covers InstanceRegistry well, but there are no tests for the updated tool handlers: handleOpenclawChat with an instance param (valid and invalid), handleOpenclawStatus with an instance param, handleOpenclawChatAsync instance resolution and instanceId propagation to the task, handleOpenclawTaskList instance filter, and handleOpenclawInstances. The tool-handler paths are the primary integration surface.
Suggestion: Add tests in the existing
src/__tests__/mcp/tools/directory (or equivalent) covering: (1) valid instance name routed to correct client, (2) unknown instance name returns errorResponse, (3) task created with correct instanceId, (4) openclaw_instances returns list without token field.
There was a problem hiding this comment.
CodeForge Review
Verdict: request_changes | Score: 7/10
Clean, well-structured multi-instance feature with good registry abstraction, backward compatibility, and solid unit tests for the registry. However, there are two input validation gaps: OPENCLAW_INSTANCES items are cast without structural validation (missing fields cause silent coercion bugs), and instance URLs are not validated for scheme, creating a potential SSRF surface.
Reviewed by CodeForge
| throw new Error('OPENCLAW_INSTANCES must be a non-empty JSON array'); | ||
| } | ||
| instances = parsed as InstanceConfig[]; | ||
| } catch (error) { |
There was a problem hiding this comment.
[MAJOR] Parsed OPENCLAW_INSTANCES items are cast directly to InstanceConfig[] without structural validation. If an item is missing 'name', INSTANCE_NAME_RE.test(undefined) coerces undefined to the string "undefined" (which passes the regex), silently registering a strangely-named instance. If 'url' is missing or not a string, the error only surfaces at request time with a confusing message.
Suggestion: Before passing to InstanceRegistry, validate each parsed item: check typeof item.name === 'string' and typeof item.url === 'string'. Example:
if (typeof item.name !== 'string' || typeof item.url !== 'string') { throw new Error('Each instance must have string name and url') }
| } | ||
|
|
||
| const client = new OpenClawClient(config.url, config.token, config.timeout); | ||
| this.instances.set(config.name, { config, client }); |
There was a problem hiding this comment.
[MAJOR] No URL scheme validation before constructing OpenClawClient. An attacker (or misconfiguration) supplying a non-HTTP scheme such as file://, gopher://, or an internal metadata IP (169.254.169.254) could be used for SSRF. Only the instance name is validated, not the URL.
Suggestion: Add URL scheme validation in the constructor loop:
const parsed = new URL(config.url); if (!['http:', 'https:'].includes(parsed.protocol)) { throw new Error('Instance URL must use http or https') }
| } | ||
|
|
||
| return { | ||
| openclawUrl: argv['openclaw-url'] as string, |
There was a problem hiding this comment.
[MINOR] When OPENCLAW_INSTANCES is set, the --openclaw-url and --gateway-token CLI args are silently ignored. A user who sets both will get no feedback that the explicit flags are being overridden.
Suggestion: Log a warning:
if (instancesEnv && (process.env.OPENCLAW_GATEWAY_URL || process.env.OPENCLAW_GATEWAY_TOKEN)) { log('WARN: OPENCLAW_INSTANCES is set; --openclaw-url and --gateway-token are ignored') }
| @@ -0,0 +1,102 @@ | |||
| import { describe, it, expect } from 'vitest'; | |||
There was a problem hiding this comment.
[MINOR] No tests for the CLI parsing of OPENCLAW_INSTANCES (missing fields, non-array JSON, invalid item types). The registry itself is well-tested, but the parsing layer in cli.ts has no coverage.
Suggestion: Add tests in a cli.test.ts that set process.env.OPENCLAW_INSTANCES to invalid JSON, a non-array, items missing 'name'/'url', etc., and assert the expected thrown errors.
| } | ||
|
|
||
| /** | ||
| * Get the default client. |
There was a problem hiding this comment.
[SUGGESTION] getDefault() uses a non-null assertion (!). Safe by construction since defaultName is always set from the validated configs array, but the pattern is fragile if defaultName could ever go stale (e.g. if a future refactor allows removing instances).
Suggestion: Replace with a guarded lookup:
const entry = this.instances.get(this.defaultName); if (!entry) throw new Error('Default instance not found'); return entry.client;
There was a problem hiding this comment.
CodeForge Review
Verdict: comment | Score: 7/10
Solid multi-instance implementation with good security practices (URL scheme validation, token-safe list(), backward compatibility). The registry design is clean and the SSRF guard via URL scheme check is appreciated. Main concerns are: validateId reuse for instance name validation creates a mismatch with INSTANCE_NAME_RE constraints, test coverage gaps for the new tool handler paths, and a subtle inconsistency in how the global gateway token is (not) applied as a fallback for multi-instance mode.
Reviewed by CodeForge
| sessionId = sidResult.value; | ||
| } | ||
|
|
||
| let instanceName: string | undefined; |
There was a problem hiding this comment.
[MINOR] validateId is used to validate instance names, but its constraints (max 256 chars, no control chars) are much more permissive than the INSTANCE_NAME_RE in InstanceRegistry (/^[a-zA-Z0-9][a-zA-Z0-9_-]{0,63}$/). An instance name like 'my instance' or 'foo@bar' passes validateId but fails at registry.resolve() with a generic 'Unknown instance' error rather than a descriptive format error. The same pattern is repeated in status.ts, tasks.ts.
Suggestion: Add a dedicated validateInstanceName() function in validation.ts that uses INSTANCE_NAME_RE (or import the regex from registry.ts) and returns a clear format error message. This aligns tool-layer validation with registry-layer constraints and gives callers actionable feedback.
| if (error instanceof SyntaxError) { | ||
| throw new Error(`OPENCLAW_INSTANCES contains invalid JSON: ${error.message}`); | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
[MINOR] When OPENCLAW_INSTANCES is set, the global OPENCLAW_GATEWAY_TOKEN / --gateway-token value is applied as a timeout fallback but NOT as a token fallback for instances that omit their own token. Only timeout gets a fallback: timeout: cfg.timeout ?? argv.timeout. A user migrating from single-instance to OPENCLAW_INSTANCES would need to re-specify their token inside the JSON array, which is non-obvious.
Suggestion: Apply the global gateway token as a fallback:
token: cfg.token ?? (argv['gateway-token'] as string | undefined). Add a note in docs/env-var docs about this behavior either way.
| @@ -0,0 +1,123 @@ | |||
| import { describe, it, expect } from 'vitest'; | |||
There was a problem hiding this comment.
[MINOR] InstanceRegistry is well tested, but there are no tests for the tool handlers that now do instance routing (handleOpenclawChat, handleOpenclawStatus, handleOpenclawChatAsync, handleOpenclawTaskList with instance filter). The happy path (instance resolves) and error path (unknown instance name, validateId failure) are untested.
Suggestion: Add tests for the tool handlers covering: (1) omitted instance falls back to default, (2) valid named instance routes to correct client, (3) unknown instance name returns errorResponse, (4) instance field failing validateId returns errorResponse.
| * Number of registered instances. | ||
| */ | ||
| get size(): number { | ||
| return this.instances.size; |
There was a problem hiding this comment.
[SUGGESTION] The isSingleInstance getter is defined but does not appear to be used anywhere in the diff or referenced in any tool/handler. If it has no planned use, it is dead code.
Suggestion: Remove isSingleInstance if it is not referenced elsewhere in the codebase. If it is intentionally kept as a public API, add a test case and document its intended use case.
| @@ -296,7 +343,7 @@ export async function handleOpenclawTaskList( | |||
| } | |||
There was a problem hiding this comment.
[SUGGESTION] openclaw_task_cancel does not accept an instance parameter in its inputSchema, unlike openclaw_chat, openclaw_status, and openclaw_chat_async. While cancellation is purely task-ID-based and does not require routing to a specific gateway, the inconsistency may confuse callers who expect the same interface across all task-related tools.
Suggestion: Either add an instance parameter (even if ignored) for API surface consistency, or add a brief comment in the tool description noting that cancellation is instance-agnostic.
Issues addressed in follow-up commits
Summary
InstanceRegistryto support orchestrating multiple OpenClaw gateways from a single MCP serveropenclaw_instancestool for discovering available instancesinstanceparameter on all gateway-facing tools (chat, status, chat_async)Configuration
Changed files
src/openclaw/registry.tssrc/openclaw/types.tsInstanceConfiginterfacesrc/mcp/tools/instances.tsopenclaw_instancestoolsrc/cli.tsOPENCLAW_INSTANCESenv varsrc/index.tssrc/server/tools-registration.tssrc/mcp/tools/chat.tsinstanceparamsrc/mcp/tools/status.tsinstanceparamsrc/mcp/tools/tasks.tssrc/mcp/tasks/manager.tsinstanceIdon TaskTest plan
npm run build)