fix(execution-router): validate adapter shape and pass manifest defaultConfig#91
Conversation
…ltConfig
Closes the adapter-discovery hardening item from the session-start
audit. The discovery loader called factory({}) with empty config and
never validated the returned object — adapters that expected config
keys silently failed, and adapters that returned malformed objects
surfaced as NoAdapterError under load instead of failing at load time.
Two improvements:
1. Manifest gains optional defaultConfig (Record<string, unknown>).
Plugins can now declare bootstrap settings (api URL, channel id,
etc) that the loader passes to the factory. Falls back to {} when
absent, preserving current behavior. defaultConfig must be a plain
object — strings, arrays, numbers, and booleans are stripped.
2. After construction, the loader validates the returned object has the
four required IronClawAdapter methods (buildPlan, execute, rollback,
healthCheck) via isAdapterShape(). Plugins returning malformed
objects now log "factory result is not a valid adapter" at load
time instead of bubbling up as a NoAdapterError under load. Also
wraps factory() in try/catch so a thrown constructor doesn't kill
discovery for unrelated plugins.
Tests: +8 (29 → 37 in adapter-manifest, 67 → 75 in execution-router).
Coverage: defaultConfig parse + drop-on-non-object, isAdapterShape
required-method enumeration via REQUIRED_ADAPTER_METHODS, null/
undefined/primitive rejection.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Hardens adapter discovery by passing manifest-provided default configuration into adapter factories and validating that discovered adapters implement the expected runtime contract, so malformed plugins fail fast during load rather than under execution load.
Changes:
- Add optional
defaultConfigto adapter manifests and parse/validate it during manifest validation. - Wrap adapter factory construction in
try/catchand validate the returned adapter shape before registering. - Add/extend unit tests for
defaultConfigparsing and adapter shape validation helpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/execution-router/src/adapter-manifest.ts | Adds defaultConfig to manifest type/validation and introduces REQUIRED_ADAPTER_METHODS + isAdapterShape helper. |
| packages/execution-router/src/adapter-discovery.ts | Passes manifest.defaultConfig into adapter factories, catches factory errors, and rejects malformed adapters at discovery time. |
| packages/execution-router/src/tests/adapter-manifest.test.ts | Adds tests for defaultConfig parsing behavior and isAdapterShape validation logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe('isAdapterShape', () => { | ||
| function validShape(): Record<string, unknown> { | ||
| return { | ||
| buildPlan: () => ({}), | ||
| execute: () => ({}), | ||
| rollback: () => ({}), | ||
| healthCheck: () => ({ healthy: true, latencyMs: 0 }), | ||
| }; |
There was a problem hiding this comment.
validShape() does not include getStatus, even though IronClawAdapter requires it. Once the required-method list is corrected to include getStatus, this helper and the related shape tests should be updated to include and validate getStatus as well.
| export const REQUIRED_ADAPTER_METHODS: readonly string[] = [ | ||
| 'buildPlan', | ||
| 'execute', | ||
| 'rollback', | ||
| 'healthCheck', | ||
| ] as const; | ||
|
|
||
| export function isAdapterShape(value: unknown): boolean { | ||
| if (!value || typeof value !== 'object') return false; | ||
| const obj = value as Record<string, unknown>; | ||
| return REQUIRED_ADAPTER_METHODS.every((m) => typeof obj[m] === 'function'); | ||
| } |
There was a problem hiding this comment.
REQUIRED_ADAPTER_METHODS / isAdapterShape omit getStatus, but IronClawAdapter requires it (see packages/ironclaw-adapter/src/ironclaw-adapter.ts:21-46). As written, plugins missing getStatus will be accepted during discovery and then fail at runtime when the router calls adapter.getStatus. Add getStatus to the required-method list (and update any related docs/tests).
| // Validate the adapter implements the contract before registering. | ||
| // A plugin that returns a malformed object would otherwise surface as a | ||
| // NoAdapterError under load — fail fast at load time instead. | ||
| if (!isAdapterShape(adapter)) { | ||
| console.warn(`[adapter-discovery] ${dirName}: factory result is not a valid adapter (missing one of: ${REQUIRED_ADAPTER_METHODS.join(', ')})`); | ||
| continue; |
There was a problem hiding this comment.
The runtime validation here relies on isAdapterShape / REQUIRED_ADAPTER_METHODS, which currently don’t include getStatus even though it’s required by IronClawAdapter. That means discovery can still register adapters that will later crash on status checks. Ensure getStatus is part of the required shape.
| it('parses defaultConfig when present and a plain object', () => { | ||
| const input = makeValidManifestInput(); | ||
| input['defaultConfig'] = { apiUrl: 'https://x.example.com', channel: 'main' }; | ||
| const result = validateManifest(input); | ||
| expect(result.valid).toBe(true); | ||
| if (result.valid) { | ||
| expect(result.manifest.defaultConfig).toEqual({ apiUrl: 'https://x.example.com', channel: 'main' }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
These defaultConfig assertions are placed under the riskModifier clamping describe block, which makes the test structure misleading. Consider moving the defaultConfig tests into their own describe('defaultConfig', ...) section to keep failures easier to interpret.
Captures the second half of the audit cleanup work that landed after #87: - #88 config/core test coverage (was 0/missing helpers) - #89 connectors gmail + calendar pure-logic tests (8 → 43) - #90 llm-client URL validation hardening (zone IDs, trailing dots, CGNAT) - #91 adapter-discovery factory shape check + manifest defaultConfig - #92 preference-archaeologist coverage (action keys, multi-group, expiry) - #93 worker SignalDeduper extraction + 11 tests Pure docs. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Closes the adapter-discovery hardening item from the session-start audit. The discovery loader called `factory({})` with empty config and never validated the returned object — adapters that expected config keys silently failed, and adapters that returned malformed objects surfaced as `NoAdapterError` under load instead of failing fast.
1. Manifest gains optional `defaultConfig`. Plugins declare their bootstrap settings (api URL, channel id, etc.) and the loader passes them to the factory. Falls back to `{}` when absent — current plugins keep working. Strings/arrays/numbers/booleans for `defaultConfig` are dropped (must be a plain object).
2. `isAdapterShape` shape check after construction. The loader now validates the returned object implements the four required `IronClawAdapter` methods (`buildPlan`, `execute`, `rollback`, `healthCheck`) before registering. Plugins returning malformed objects now log "factory result is not a valid adapter" at load time instead of bubbling up as `NoAdapterError` under load. Also wraps `factory()` in try/catch so a throwing constructor doesn't kill discovery for unrelated plugins.
Test plan
🤖 Generated with Claude Code