[simonferquel-clanker]

🤖 Barry (automated agent) — this comment was posted by an AI agent, not by a human developer

Symptom

With strict: true (set by ConvertParametersToSchema when isStrictCompatible holds), the OpenAI Responses API rejects some tool schemas with Invalid schema for function '<name>'. Please ensure it is a valid JSON Schema. (invalid_function_parameters, no path). Reproduced on gpt-4o-mini, gpt-4.1-mini and gpt-5-mini. The same schema passes without strict.

Cause

makeAllRequired makes optional properties nullable via "type": ["array", "null"]. Strict mode rejects that form when the array property sits in an object reached through a $defs entry that itself contains a $ref (or an anyOf of $refs). Scalars (["string","null"]) are accepted. The Notion MCP server's query-sessions tool triggers it (sessionNestedFilter → sessionStatusFilter.status.in).

Minimal repro:

{"type":"object","additionalProperties":false,"required":["f"],
 "properties":{"f":{"$ref":"#/$defs/N"}},
 "$defs":{"N":{"$ref":"#/$defs/S"},
  "S":{"type":"object","additionalProperties":false,"required":["p"],
   "properties":{"p":{"type":["array","null"],"items":{"type":"string"}}}}}}

Fix

Nullable array and object properties become {"anyOf": [<schema>, {"type": "null"}]} — the form already used for $ref properties. This covers newly-nullable properties and ones already declared as an ["array","null"] pair (either order; jsonschema-go emits ["null","array"]). Scalars are unchanged. The anyOf form is emitted for every provider that goes through ConvertParametersToSchema (the Mistral cassette shows it on the non-strict Chat Completions path too); anyOf-with-null is valid non-strict JSON Schema, so no behaviour regression is expected there. fixSchemaArrayItems also checks anyOf variants so arrays without items keep their default. With the new form all three models return 200 under strict: true.

Wrapping relocates the property's schema to <path>/anyOf/0, so local $refs pointing at or into a wrapped property are rewritten accordingly; strict mode is disabled if any local $ref of the normalized schema no longer resolves.

Known limit: containers that are not properties (e.g. array items) are untouched.

Deliberate scope

• Rewriting already-declared ["array","null"] pairs is required, not incidental: in the repro p is already required, so handling only newly-optional properties would not fix it.
• The fixSchemaArrayItems change is a direct consequence: it only inspected prop["type"], so without it optional arrays without items would silently lose their default once wrapped in anyOf.
• The three e2e cassette edits only change the schema shape of excludePatterns in recorded request bodies (matched exactly): "type":["null","array"] becomes anyOf:[{...,"type":"array"},{"type":"null"}].

Tests

• New table-driven TestConvertParametersToSchema_NullableContainers (repro, both type orders, optional array/object, scalar, $ref); asserts strict-compatibility and no nullable container type pairs remain.
• Updated TestMakeAllRequired_TypeArrayWithObject, TestSchemaForOpenai, and request bodies in three e2e cassettes (OpenAI, OpenAI hide-tool-calls, Mistral).
• go test ./pkg/model/provider/openai/ and golangci-lint on that package pass locally; the sandbox could not download all modules for ./e2e and ./pkg/model/provider, so CI covers those.

[aheritier]

Substantively reviewed all six changed files against the PR's stated scope, including nullable-container normalization, scalar handling, missing-items fallback, and cassette updates. See inline comments.

CI: verified green for this head, including gate, lint, Linux/Windows tests, cross-compilation, image build, and CodeQL. Linux logs confirm the OpenAI provider, provider integration, and e2e packages passed. No local PR tests were run.

Issue context: needs-decision. No explicit GitHub/Jira references or linked closing issues were found, and there are no PR comments or review threads supplying additional acceptance criteria. Review therefore used the PR description; external acceptance criteria remain unverified.

Merge readiness: not merge-ready. The PR is non-draft and conflict-free, but GitHub reports BLOCKED / REVIEW_REQUIRED.

[aheritier]

[blocking] Preserve local JSON-pointer targets when wrapping containers (confidence: high).

Here is the failing case:

{"type":"object","required":["copy"],"properties":{"cfg":{"type":"object","required":["values"],"properties":{"values":{"type":"array","items":{"type":"string"}}}},"copy":{"$ref":"#/properties/cfg/properties/values"}}}

cfg is optional, so this assignment moves its original schema under #/properties/cfg/anyOf/0. The unchanged copy.$ref consequently points to a nonexistent path. A source-backed JSON-pointer reproduction resolves the reference before this change and fails at properties after wrapping; the previous type-pair transformation kept that path intact. Since isStrictCompatible runs before normalization, this also returns strict=true despite producing an unresolved reference, breaking an otherwise valid tool schema.

Preserve existing pointer targets or rewrite affected local references when relocating schemas, and add a pipeline regression test covering references into a wrapped container's descendants.

[simonferquel-clanker]

🤖 Barry (automated agent) — this comment was posted by an AI agent, not by a human developer

Good catch, fixed in 7e161a6. After wrapping, local $refs are rewritten to follow the relocated schema (#/properties/cfg/properties/values becomes #/properties/cfg/anyOf/0/properties/values; a ref to the wrapped property itself becomes .../anyOf/0 so it keeps the non-null schema it saw before). ConvertParametersToSchema also reports strict=false if any local $ref of the normalized schema fails to resolve. Added pipeline tests for your exact case, a ref to the wrapped node, a $defs ref (untouched), an unresolvable ref, and a resolve-every-local-ref assertion on the nullable-container table. Re-review requested.

[simonferquel-clanker]

🤖 Barry (automated agent) — this comment was posted by an AI agent, not by a human developer

@aheritier @dgageot re-review requested: 7e161a6 rewrites local $refs that point into wrapped properties (see the reply on the inline thread).

[aheritier]

I think your fixed 7e161a6 was not merged on master @simonferquel-clanker