feat: support additional schemas via additionalSchemas parameter#166
Conversation
Extension servers (e.g., mcpServerInternal) can now register additional schemas at startup without modifying the OSS package. Call registerSchema() before registerSchemaTool() so the new schemas appear in harness_schema. SCHEMAS and VALID_SCHEMAS are now mutable (string-keyed) to support this. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
…xtension schema test
There was a problem hiding this comment.
Stale comment
Found 3 important issues in this diff:
src/tools/index.ts:20-31,src/resources/index.ts:10-13
This only threadsadditionalSchemasthrough helper signatures. The shipped server still callsregisterAllTools(server, registry, client, config)andregisterAllResources(server, registry, client, config)insrc/index.ts:90-91, and there is no remaining runtimeregisterSchema()path on the branch. So extension schemas are still unreachable outside direct unit tests.
src/tools/harness-schema.ts:127-140,src/tools/harness-schema.ts:97-124,tests/tools/harness-schema-tool.test.ts:25-30
The new contract accepts arbitrary extra JSON objects, but summary/path mode still assumes the generated Harness layout underdefinitions[resourceType][resourceType]. A plain JSON Schema like the one blessed by the new test will silently producefields: []/ no sections instead of failing loudly or being normalized, which misleads agents building payloads.
src/tools/harness-schema.ts:131-140,src/resources/harness-schema.ts:16-25
Duplicate names only log a warning and then overwrite the built-in schema. That lets callers replace canonical contracts likepipelineortriggerin bothharness_schemaandschema:///..., violating the repo's fail-loud requirement and risking invalid Harness payload guidance.Sent by Cursor Automation: Sunil On Demand Architecture Review
| registerDescribeTool(server, registry); | ||
| registerStatusTool(server, registry, client, config); | ||
| registerSchemaTool(server); | ||
| registerSchemaTool(server, additionalSchemas); |
There was a problem hiding this comment.
additionalSchemas gets threaded through here, but the actual server constructor still calls registerAllTools(server, registry, client, config) / registerAllResources(...) without a fifth argument in src/index.ts. With no registerSchema() or other startup hook left on the branch, this new path is still test-only: extension schemas cannot actually reach the shipped MCP server.
| } | ||
| } | ||
| } | ||
| const allSchemas: Record<string, Record<string, any>> = additionalSchemas |
There was a problem hiding this comment.
This accepts arbitrary additionalSchemas, but the rest of the tool still assumes the generated Harness layout under definitions[resourceType][resourceType] when building the summary/path responses. For a plain JSON Schema like the one added in tests/tools/harness-schema-tool.test.ts, harness_schema(resource_type='DashboardContract') will silently report fields: [] and no sections instead of failing loudly or normalizing the schema. Please validate or normalize extension schemas up front.
| } | ||
| } | ||
| } | ||
| const allSchemas: Record<string, Record<string, any>> = additionalSchemas |
There was a problem hiding this comment.
This merge silently allows an extension schema to shadow a built-in name after only logging a warning. That means callers can replace canonical contracts like pipeline or trigger in both schema:///... and harness_schema(...), which is the opposite of the repo's fail-loud guidance. Duplicate keys should throw instead of overriding built-ins.
There was a problem hiding this comment.
Stale comment
Found 3 important issues against the architecture standards:
src/tools/index.ts,src/resources/index.ts,src/index.ts
The newadditionalSchemashook is only threaded through helper signatures. The shipped server still builds its runtime increateHarnessServer()and calls the helper registrars without any extension map, and there is no remainingregisterSchema()API on this branch. As written, the advertised runtime-registration path is still unreachable in the actual server entrypoint.
src/tools/harness-schema.ts,tests/tools/harness-schema-tool.test.ts
The tool now accepts arbitrary extension schema objects, but summary/path mode still assumes the generated Harness layout underdefinitions[resourceType][resourceType]. A plain JSON Schema like the one blessed by the new registration test will register successfully yet return an empty summary/no sections instead of a clear error or normalized output. That violates the repo's fail-loud guidance and can mislead agents building payloads.
src/tools/harness-schema.ts,src/resources/harness-schema.ts
Built-in name collisions only emit a warning and then let the extension map overwrite canonical schemas. That means callers can replace contracts likepipelineortriggerin bothharness_schemaandschema:///..., which goes against the repo's fail-loud architecture standards and risks surfacing invalid guidance.Open questions / assumptions:
- I'm assuming the intended consumer is either the runtime built in
src/index.tsor another supported embedder path, not a private deep-import-only integration.- I'm also assuming extension schemas may be plain JSON Schemas, as the new API shape and tests imply, not only pre-wrapped
definitions[...]objects.Verification:
pnpm test(1191passed)pnpm typecheckSent by Cursor Automation: Sunil On Demand Architecture Review
|
|
||
|
|
||
| export function registerAllTools(server: McpServer, registry: Registry, client: HarnessClient, config: Config): void { | ||
| export function registerAllTools(server: McpServer, registry: Registry, client: HarnessClient, config: Config, additionalSchemas?: Record<string, Record<string, any>>): void { |
There was a problem hiding this comment.
Threading additionalSchemas through this helper isn't enough on its own: the shipped server still calls registerAllTools(server, registry, client, config) / registerAllResources(server, registry, client, config) from createHarnessServer() in src/index.ts, and there is no remaining registerSchema() API on this branch. Unless an exported server-construction path can actually supply this map, the advertised runtime-registration feature stays unreachable outside direct unit tests.
| } | ||
| } | ||
| } | ||
| const allSchemas: Record<string, Record<string, any>> = additionalSchemas |
There was a problem hiding this comment.
additionalSchemas is typed as arbitrary JSON objects, but the rest of this tool still assumes the generated Harness layout under definitions[resourceType][resourceType] via getSummary() / navigateToPath(). A plain JSON Schema like the one accepted in the new test will register successfully here, then return fields: [] / no sections instead of failing loudly or being normalized, which can mislead agents using harness_schema to build payloads.
| if (additionalSchemas) { | ||
| for (const key of Object.keys(additionalSchemas)) { | ||
| if (key in SCHEMAS) { | ||
| log.warn(`additionalSchemas key '${key}' overrides a built-in schema`); |
There was a problem hiding this comment.
I'd prefer to reject built-in name collisions here instead of warning and overwriting. Letting an extension replace canonical contracts like pipeline or trigger means harness_schema can surface invalid guidance for core Harness resources. The same fail-loud guard should apply in registerHarnessSchemaResource() too.
…o root-level properties - registerSchemaTool/registerHarnessSchemaResource now throw instead of warn when additionalSchemas contains a key that matches a built-in schema name - getSummary falls back to root-level properties for plain JSON Schemas that don't use the Harness definitions[type][type] layout - Tests cover collision-throws, Harness-layout summary, and plain JSON Schema summary Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Stale comment
Found 2 issues:
Critical: the new runtime schema-registration path is not reachable from the shipped/public server API.
registerAllTools()andregisterAllResources()now acceptadditionalSchemas, but the real bootstrap insrc/index.tsstill calls them with four arguments and there is no publicregisterSchema()/constructor hook anywhere in the package. As written, the PR changes helper signatures without giving stdio/HTTP consumers any supported way to register runtime schemas.Important: the new tests only exercise helper functions, not the real entrypoint.
tests/tools/harness-schema-tool.test.tsandtests/resources/harness-schema.test.tscallregisterSchemaTool()/registerHarnessSchemaResource()directly, so they would still pass even whencreateHarnessServer()forgets to thread the new parameter, which is the current behavior. Please add coverage that drives the packaged bootstrap or another real server startup path.Sent by Cursor Automation: Sunil On Demand Architecture Review
|
|
||
|
|
||
| export function registerAllTools(server: McpServer, registry: Registry, client: HarnessClient, config: Config): void { | ||
| export function registerAllTools(server: McpServer, registry: Registry, client: HarnessClient, config: Config, additionalSchemas?: Record<string, Record<string, any>>): void { |
There was a problem hiding this comment.
Critical: this helper signature change does not make runtime schema registration reachable from the shipped server.
createHarnessServer() in src/index.ts still calls registerAllTools(server, registry, client, config) and registerAllResources(server, registry, client, config) with no fifth argument, and the package does not expose a public registerSchema() or constructor hook anywhere else. That means stdio/HTTP consumers still have no supported way to inject additionalSchemas, so the new path here is dead code from the public API's perspective.
| return JSON.parse(result.content[0]!.text); | ||
| } | ||
|
|
||
| describe("registerSchemaTool additionalSchemas", () => { |
There was a problem hiding this comment.
Important: these new cases only prove that the internal helper works when called directly. They do not exercise createHarnessServer() or any packaged stdio/HTTP startup path, so they miss the broken wiring above.
Please add at least one test or smoke check that goes through the real bootstrap and verifies the extra schema is visible from the actual exported server surface, not just from registerSchemaTool() / registerHarnessSchemaResource() in isolation.
There was a problem hiding this comment.
Stale comment
Found 2 issues that keep this from meeting the runtime schema-registration architecture goal.
- Critical: the new
additionalSchemasplumbing is still helper-only. The shipped bootstrap insrc/index.tsnever passes the extra schemas intoregisterAllTools()/registerAllResources(), and there is no exported public server factory to accept them. That means the default server path users actually run cannot surface the new dashboard/internal schemas.- Important: the new tests only exercise helper registration, not the real bootstrap path, so they would stay green even when runtime registration is unreachable in production.
Open question: if the intended contract is that internal consumers deep-import
build/tools/index.js/build/resources/index.js, can we make that explicit and stable? Right now the package surface still looks CLI-first rather than library-first.Local verification:
pnpm test tests/tools/harness-schema-tool.test.ts tests/resources/harness-schema.test.tspnpm typecheckpnpm buildSent by Cursor Automation: Sunil On Demand Architecture Review
| @@ -28,5 +28,5 @@ export function registerAllTools(server: McpServer, registry: Registry, client: | |||
| registerSearchTool(server, registry, client); | |||
| registerDescribeTool(server, registry); | |||
| registerStatusTool(server, registry, client, config); | |||
| registerSchemaTool(server); | |||
| registerSchemaTool(server, additionalSchemas); | |||
There was a problem hiding this comment.
Critical: this only exposes additionalSchemas at the helper layer. The real bootstrap in src/index.ts still calls registerAllTools(server, registry, client, config) and registerAllResources(server, registry, client, config) with four arguments, and createHarnessServer() is private to that file. So the default server that ships in this package never sees these extra schemas, which means the PR's runtime-registration use case is still unreachable unless downstream code reconstructs the server around deep imports. To meet the architecture goal, we need a top-level/public hook that the real startup path consumes.
| describe("registerSchemaTool additionalSchemas", () => { | ||
| it("accepts additionalSchemas without throwing", () => { | ||
| const server = makeMcpServer(); | ||
| const extra = { | ||
| DashboardContract: { type: "object", properties: { id: { type: "string" } } }, | ||
| }; | ||
| expect(() => registerSchemaTool(server, extra)).not.toThrow(); |
There was a problem hiding this comment.
These tests prove registerSchemaTool() can merge extra schemas, but they never exercise the shipped bootstrap path. Because src/index.ts does not pass additionalSchemas into registerAllTools() / registerAllResources(), the suite still stays green even when runtime registration is dead in the real server. Sunil's verification standard here should include at least one integration/startup test around the actual entrypoint (or an exported public constructor) so this wiring can't regress silently.
…cription and group
There was a problem hiding this comment.
1 finding.
Critical: the advertised runtime schema registration hook is still not reachable from the shipped server surface.
Affected files: src/data/schemas/index.ts, src/tools/index.ts, src/resources/index.ts, tests/tools/harness-schema-tool.test.ts, tests/resources/harness-schema.test.ts.
Why it matters: the PR summary/use case says extension servers should be able to call registerSchema() at startup and have those schemas show up through the production harness_schema tool and schema:///... resource. This branch never adds that exported hook, and the real bootstrap in src/index.ts still invokes registerAllTools(server, registry, client, config) / registerAllResources(server, registry, client, config) with no schema-registration input. The only new behavior is an optional additionalSchemas argument on internal helper functions, which makes tool/resource consistency caller-managed instead of coming from a single explicit contract.
Likely runtime consequence: the published binary/package still exposes only the built-in schemas, so the DashboardContract / WidgetContract startup registration flow described in the PR cannot work in production. The new tests stay green because they only instantiate stub MCP servers and call the helpers directly instead of exercising the real entrypoint/startup path.
Sent by Cursor Automation: Sunil On Demand Architecture Review
|
|
||
|
|
||
| export function registerAllTools(server: McpServer, registry: Registry, client: HarnessClient, config: Config): void { | ||
| export function registerAllTools(server: McpServer, registry: Registry, client: HarnessClient, config: Config, additionalSchemas?: Record<string, SchemaEntry>): void { |
There was a problem hiding this comment.
This still doesn't make the new schema path reachable from the shipped server. src/index.ts is unchanged and still calls registerAllTools(server, registry, client, config) / registerAllResources(...) with no fifth argument, and this branch also never adds the advertised registerSchema() export in src/data/schemas/index.ts. So the only way to hit additionalSchemas is from internal helper callers that manually keep both tool and resource registration in sync, which means the production binary/package still cannot register DashboardContract-style schemas at startup.
There was a problem hiding this comment.
This is expected for now.


Summary
additionalSchemasparameter toregisterSchemaTool()andregisterHarnessSchemaResource(), allowing extension servers to inject extra JSON schemas at registration timeSchemaEntrytype ({ schema, description, group }) as the extension API surface — typed for future use, no migration requiredgetSummaryfalls back to root-levelpropertiesfor plain JSON Schemas, in addition to the existing Harness nested-definition layoutisValidSchemaNamenow accepts an optionalvalidNamesparameter so callers with a merged schema set can validate without re-reading global stateMotivation
The OSS package is used as a dependency by extension servers that need to surface additional JSON schemas to agents (e.g. domain-specific resource contracts). The previous global-mutation approach (
registerSchema()) had ordering hazards and no collision protection. Parameter injection is explicit, testable, and isolated per registration.Test plan
registerSchemaTool(server, { MyExt: entry({...}) })registers without throwing"conflicts with a built-in schema name"definitions[type][type])properties)pipeline, etc.) still work when noadditionalSchemaspassedisValidSchemaNamecorrectly validates extension names when merged set is passed🤖 Generated with Claude Code