From 182c48f2969f4715350f8a7b5cf18cc98f251f44 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 25 Aug 2026 03:41:17 -0400 Subject: [PATCH] fix: stop claiming a bare-true replacement narrows the contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 8 (its one suppressed finding; no new inline comments). The `boolean-schema` suggestion for a bare `true` said the replacement "narrows the schema deliberately". That holds in an ordinary property position and inverts under a negating parent: `{"not": true}` rejects every value, so swapping in `{"type": "object", …}` leaves the parent rejecting only objects and every non-object then passes — the enclosing contract is widened. Under `if` it redirects which branch applies rather than tightening anything. - Parent-aware advice under `not`: suggest `{}`, which is the exact object form of `true` and is already exempt from `untyped-schema` in that position, with the message saying why a concrete type would be wrong there. - Everywhere else, and in the `untyped-schema` suggestion too, describe the replacement as a deliberate CHANGE of contract rather than a narrowing. A suggestion has no business claiming an effect it cannot guarantee across the positions it can print from. Tests pin the `not`-specific advice and assert no bare-`true` suggestion in any of the three positions contains "narrow". The CLI README's sample report is re-captured from a real run. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/cli/README.md | 4 +- clients/web/src/test/core/schemaLint.test.ts | 39 ++++++++++++++++++++ core/json/schemaLint.ts | 36 +++++++++++++++--- 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index 45b7d0f9..918be93f 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -173,7 +173,7 @@ handler adds one more stderr line after this, the Error: tool "get_temp" Path: outputSchema.properties.data Issue: Bare `true` used where a schema object is expected. - Suggestion: Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That narrows the schema deliberately; `true` accepts any JSON value at all. + Suggestion: Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That is a deliberate change of contract, not an equivalent rewrite: `true` accepts any JSON value at all. Warning: tool "echo" Path: inputSchema.properties.show_ids @@ -183,7 +183,7 @@ Warning: tool "echo" Warning: tool "echo" Path: inputSchema.properties.opts Issue: Schema carries no validation keyword at all, so it accepts any value — the object-literal spelling of a bare `true`. - Suggestion: Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That narrows the schema deliberately, which is the point: as written it constrains nothing. + Suggestion: Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That is a deliberate change of contract, which is the point: as written it constrains nothing. Warning: tool "add" Path: inputSchema.properties.a diff --git a/clients/web/src/test/core/schemaLint.test.ts b/clients/web/src/test/core/schemaLint.test.ts index b96ac634..cb2db9d2 100644 --- a/clients/web/src/test/core/schemaLint.test.ts +++ b/clients/web/src/test/core/schemaLint.test.ts @@ -132,6 +132,45 @@ describe("lintToolSchemas — boolean-schema", () => { expect(findings[0]!.suggestion).toContain("additionalProperties"); }); + it("suggests `{}` for a bare true under not, where a typed schema would widen", () => { + // `{"not": true}` rejects every value. Swapping in `{"type": "object", …}` + // leaves the parent rejecting only objects, so every non-object would then + // pass — the enclosing contract is widened, not narrowed. `{}` is the + // exact replacement, and is exempt from `untyped-schema` in that position. + const findings = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { a: { not: true } } }, + }), + ); + expect(rules(findings)).toEqual(["boolean-schema"]); + expect(findings[0]!.suggestion).toContain("exact replacement"); + expect(findings[0]!.suggestion).not.toContain('"additionalProperties"'); + }); + + it("never claims a bare-true replacement narrows the contract", () => { + // It narrows in an ordinary property position and does not under `not` or + // `if`, so the wording must not assert an effect it cannot guarantee. + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: true, b: { not: true }, c: { if: true } }, + }, + }), + ); + // Four: a bare `true` in each of the three positions, plus `c` itself + // being unconstrained — a lone `if` asserts nothing without a `then`. + expect(rules(findings)).toEqual([ + "boolean-schema", + "boolean-schema", + "untyped-schema", + "boolean-schema", + ]); + for (const finding of findings) { + expect(finding.suggestion).not.toMatch(/narrow/i); + } + }); + 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 diff --git a/core/json/schemaLint.ts b/core/json/schemaLint.ts index b054683a..e6d82c31 100644 --- a/core/json/schemaLint.ts +++ b/core/json/schemaLint.ts @@ -395,6 +395,31 @@ function forEachSubschema( } } +/** + * What to replace a bare `true` with — which depends on the keyword above it. + * + * `{}` and `true` are the same schema, so `{}` is always the *exact* rewrite. + * It is normally not what to advise, because `{}` is the very shape + * `untyped-schema` flags — hence the usual advice to declare a real type. + * + * Under `not` that reverses. `{"not": true}` rejects every value; swapping in + * `{"type": "object", …}` leaves the parent rejecting only objects, so every + * non-object now passes — the enclosing contract is **widened**, not narrowed. + * `{}` is exempt from `untyped-schema` in that position for the same reason, + * so it is both exact and safe to recommend there. + * + * Everywhere else the replacement is described as a deliberate *change* rather + * than a narrowing: in an ordinary property position it does narrow, but under + * a conditional (`if`) it redirects which branch applies, and a suggestion has + * no business claiming an effect it cannot guarantee. + */ +function booleanTrueSuggestion(keyword: string | undefined): string { + if (keyword === "not") { + return "Use `{}`, the object form of the always-true schema — under `not` that is an exact replacement. (Declaring a concrete type here would instead let every value of another type through.)"; + } + return 'Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That is a deliberate change of contract, not an equivalent rewrite: `true` accepts any JSON value at all.'; +} + /** * Visit one schema position. `keyword` is the parent keyword this node sits * under (undefined at the root), which is what decides whether a bare boolean @@ -418,11 +443,7 @@ function walk( path, `Bare \`${String(node)}\` used where a schema object is expected.`, node - ? // `true` accepts any JSON value, and no portable object form says - // exactly that — `{}` is the same shape this lint flags as untyped. - // So the advice is to narrow, and it says so rather than implying an - // equivalent rewrite. - 'Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That narrows the schema deliberately; `true` accepts any JSON value at all.' + ? booleanTrueSuggestion(keyword) : // `false` in a property position means the property is FORBIDDEN. // Deleting the entry is not the same thing: with `additionalProperties` // absent (default `true`) the property becomes allowed with any value, @@ -505,7 +526,10 @@ function lintNode( "warning", path, "Schema carries no validation keyword at all, so it accepts any value — the object-literal spelling of a bare `true`.", - 'Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That narrows the schema deliberately, which is the point: as written it constrains nothing.', + // "changes", not "narrows": in an ordinary property position it does + // narrow, but under a conditional (`if`) a typed replacement redirects + // which branch applies rather than tightening anything. + 'Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That is a deliberate change of contract, which is the point: as written it constrains nothing.', ); } }