fix: drop the unreachable root rule, correct the always-false suggestion
Copilot review round 2.
- Remove `non-object-root` entirely. It could never fire: the SDK types
`inputSchema` with `type: literal("object")`, so a tool with any other
root fails `ListToolsResultSchema` and `salvageListItems` drops it
before any client sees it. Verified against a live server advertising
`inputSchema: {type: "array"}` — `listAllTools()` returns []. A rule
that cannot fire is worse than none, because the docs then claim a
check the tool does not perform; the condition is already reported
through the malformed-items surface. Pinned by an integration test so
a change in that behavior is a signal rather than a silent gap.
- Fix the bare-`false` suggestion. `properties: {a: false}` forbids the
property; deleting the entry *permits* it with any value under the
default `additionalProperties`, so the advice inverted the schema it
replaced. Now recommends `{"not": {}}` and says why deletion differs.
- Exempt `not`-parented schemas from `untyped-schema`, so the `{"not":
{}}` above does not trip this module's own rule — under negation an
always-accepting subschema means always-reject, making "accepts any
value" the exact opposite of what it does. Scoped to that position,
not inherited by its subtree.
- Say plainly, in the `true` and untyped suggestions, that they narrow
the schema deliberately rather than implying an equivalent rewrite.
`true` has no portable object-form equivalent — `{}` is the very shape
this lint flags.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
c389565783
commit
cac97c142c
2 changed files with 136 additions and 78 deletions
|
|
@ -131,14 +131,48 @@ describe("lintToolSchemas — boolean-schema", () => {
|
|||
expect(findings[0]!.suggestion).toContain("additionalProperties");
|
||||
});
|
||||
|
||||
it("flags a bare false, with a different suggestion", () => {
|
||||
it("suggests the always-false object form for a bare false", () => {
|
||||
// `properties: {a: false}` forbids the property. Deleting the entry would
|
||||
// *permit* it with any value under the default `additionalProperties`, so
|
||||
// the suggestion has to preserve the constraint rather than drop it.
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { data: false } },
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["boolean-schema"]);
|
||||
expect(findings[0]!.suggestion).toContain("Remove the property");
|
||||
expect(findings[0]!.suggestion).toContain('{"not": {}}');
|
||||
expect(findings[0]!.suggestion).toContain("different contract");
|
||||
expect(findings[0]!.suggestion).not.toMatch(/^Remove the property/);
|
||||
});
|
||||
|
||||
it('does not flag the `{"not": {}}` form it recommends', () => {
|
||||
// The suggestion above would be self-defeating if this module reported its
|
||||
// own recommended replacement.
|
||||
expect(
|
||||
lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { data: { not: {} } } },
|
||||
}),
|
||||
),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it("still flags an unconstrained schema elsewhere under a not branch", () => {
|
||||
// The exemption is scoped to the `not` position itself, not inherited by
|
||||
// everything beneath it.
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { not: { properties: { b: {} } } } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["untyped-schema"]);
|
||||
expect(paths(findings)).toEqual([
|
||||
"inputSchema.properties.a.not.properties.b",
|
||||
]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
|
|
@ -274,9 +308,7 @@ describe("lintToolSchemas — type-union", () => {
|
|||
expect(findings[0]!.suggestion).toContain("anyOf");
|
||||
});
|
||||
|
||||
it("does not raise non-object-root for an array-typed root", () => {
|
||||
// The root rule reads a *string* `type`; an array root is the union rule's
|
||||
// business, and reporting both would name one defect twice.
|
||||
it("raises only the union rule for an array-typed root", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({ inputSchema: { type: ["object", "null"] } }),
|
||||
);
|
||||
|
|
@ -354,38 +386,27 @@ describe("lintToolSchemas — untyped-schema", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("lintToolSchemas — non-object-root", () => {
|
||||
it("flags a non-object inputSchema root", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "array", items: { type: "string" } },
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["non-object-root"]);
|
||||
expect(findings[0]!.severity).toBe("error");
|
||||
expect(findings[0]!.issue).toContain("inputSchema");
|
||||
});
|
||||
|
||||
it("does NOT flag a non-object outputSchema root", () => {
|
||||
// The SDK's `Tool` schema types `inputSchema` with `type: literal("object")`
|
||||
// but `outputSchema` as a plain loose object with no `type` constraint, so
|
||||
// a string-typed output is conforming. Flagging it would report an error
|
||||
// MCP does not require and would fail `--strict` on a valid server.
|
||||
expect(lintToolSchemas(tool({ outputSchema: { type: "string" } }))).toEqual(
|
||||
[],
|
||||
);
|
||||
});
|
||||
|
||||
it("does not flag a nested non-object schema", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
describe("lintToolSchemas — no root-type rule", () => {
|
||||
// There is deliberately no "inputSchema must be an object" rule: the SDK
|
||||
// types `inputSchema` with `type: literal("object")`, so such a tool fails
|
||||
// `ListToolsResultSchema` and `salvageListItems` drops it before any client
|
||||
// sees it. `raw-tool-schemas.test.ts` pins that live behaviour; these cases
|
||||
// pin that the module stays quiet rather than reporting a check nothing can
|
||||
// reach.
|
||||
it.each([
|
||||
["a non-object inputSchema root", { inputSchema: { type: "array" } }],
|
||||
["a non-object outputSchema root", { outputSchema: { type: "string" } }],
|
||||
[
|
||||
"a nested non-object schema",
|
||||
{
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { type: "array", items: { type: "string" } } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(findings).toEqual([]);
|
||||
},
|
||||
],
|
||||
])("reports nothing for %s", (_label, overrides) => {
|
||||
expect(lintToolSchemas(tool(overrides))).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -110,6 +110,24 @@ describe("rawToolSchemas override (#1005)", () => {
|
|||
]);
|
||||
});
|
||||
|
||||
it("drops a tool whose inputSchema root is not an object, before any client sees it", async () => {
|
||||
// This is why `schemaLint` has no "inputSchema must be an object" rule.
|
||||
// The SDK types `inputSchema` with `type: literal("object")`, so such a
|
||||
// tool fails `ListToolsResultSchema` and `salvageListItems` removes it —
|
||||
// no client can pass it to the lint, and a rule for it could never fire.
|
||||
// If this ever starts returning the tool, that rule becomes worth adding
|
||||
// back and this test is the signal.
|
||||
const started = await start({
|
||||
rawToolSchemas: {
|
||||
echo: { inputSchema: { type: "array", items: { type: "string" } } },
|
||||
},
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
expect(tools.map((t) => t.name)).toEqual(["get_weather"]);
|
||||
});
|
||||
|
||||
it("leaves an unnamed tool's real schema alone", async () => {
|
||||
const started = await start({
|
||||
rawToolSchemas: { echo: { outputSchema: { type: "object" } } },
|
||||
|
|
|
|||
Loading…
Reference in a new issue