fix(codemode): decode JSON-string args for object- and array-typed inputs - #32
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
4 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/codemode/src/tool-schema.ts">
<violation number="1" location="packages/codemode/src/tool-schema.ts:294">
P2: When a local `$ref` targets another local `$ref`, `dereference` stops at the intermediate reference and encoded object or array arguments remain strings. Resolve local references transitively with a cycle/depth guard.</violation>
<violation number="2" location="packages/codemode/src/tool-schema.ts:297">
P2: When an object or array alternative is expressed with `anyOf`, `oneOf`, or `allOf`, `coerceJsonStrings` never detects it because it only inspects `resolved.type`. Traverse composition branches and decode a parsed value when an object or array branch matches.</violation>
<violation number="3" location="packages/codemode/src/tool-schema.ts:330">
P2: When a schema allows both objects and arrays via `type: ["object", "array"]`, an encoded array stays a string because the object branch returns before the array branch. Select the branch matching the parsed value, or attempt both declared types.</violation>
<violation number="4" location="packages/codemode/src/tool-schema.ts:330">
P2: The new decode path only fires when the resolved schema declares `type` directly on an object or array. Schemas using composition (`allOf`/`anyOf`/`oneOf` — including the `Schema.Struct({})` case that this file already special-cases at line 140) or chained `$ref`s (a definition that itself only `$ref`s another) silently fall through to no-op, so a JSON-string arg is not decoded and the MCP rejection this PR targets persists for those common real-world schemas. `dereference` is also single-hop and ignores non-`#/$defs|definitions` ref scopes. The existing render path (`hasUnresolvedRef`, `renderSchema`) already recurses through `anyOf`/`oneOf`/`allOf` and follows refs, so this decode path understands a smaller subset of the schema. It is safe (pass-through, no false decode), but it under-delivers on the stated objective for composed/aliased schemas. Consider following refs to their terminal schema and unwrapping `allOf`/`anyOf`/`oneOf` (with a depth bound) before checking the declared type.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (schema.$ref === undefined) return schema | ||
| const segment = schema.$ref.match(/^#\/(?:\$defs|definitions)\/([^/]+)$/)?.[1] | ||
| const name = segment === undefined ? undefined : JsonPointer.unescapeToken(segment) | ||
| return name === undefined ? undefined : definitions[name] |
There was a problem hiding this comment.
P2: When a local $ref targets another local $ref, dereference stops at the intermediate reference and encoded object or array arguments remain strings. Resolve local references transitively with a cycle/depth guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-schema.ts, line 294:
<comment>When a local `$ref` targets another local `$ref`, `dereference` stops at the intermediate reference and encoded object or array arguments remain strings. Resolve local references transitively with a cycle/depth guard.</comment>
<file context>
@@ -283,12 +283,102 @@ export const outputTypeScript = <R>(definition: Definition<R>, pretty = false):
+ if (schema.$ref === undefined) return schema
+ const segment = schema.$ref.match(/^#\/(?:\$defs|definitions)\/([^/]+)$/)?.[1]
+ const name = segment === undefined ? undefined : JsonPointer.unescapeToken(segment)
+ return name === undefined ? undefined : definitions[name]
+}
+
</file context>
| return name === undefined ? undefined : definitions[name] | ||
| } | ||
|
|
||
| const declaresType = (schema: JsonSchema, type: "object" | "array"): boolean => |
There was a problem hiding this comment.
P2: When an object or array alternative is expressed with anyOf, oneOf, or allOf, coerceJsonStrings never detects it because it only inspects resolved.type. Traverse composition branches and decode a parsed value when an object or array branch matches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-schema.ts, line 297:
<comment>When an object or array alternative is expressed with `anyOf`, `oneOf`, or `allOf`, `coerceJsonStrings` never detects it because it only inspects `resolved.type`. Traverse composition branches and decode a parsed value when an object or array branch matches.</comment>
<file context>
@@ -283,12 +283,102 @@ export const outputTypeScript = <R>(definition: Definition<R>, pretty = false):
+ return name === undefined ? undefined : definitions[name]
+}
+
+const declaresType = (schema: JsonSchema, type: "object" | "array"): boolean =>
+ schema.type === type || (Array.isArray(schema.type) && schema.type.includes(type))
+
</file context>
| const resolved = dereference(schema, definitions) | ||
| if (resolved === undefined) return value | ||
|
|
||
| if (declaresType(resolved, "object")) { |
There was a problem hiding this comment.
P2: When a schema allows both objects and arrays via type: ["object", "array"], an encoded array stays a string because the object branch returns before the array branch. Select the branch matching the parsed value, or attempt both declared types.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-schema.ts, line 330:
<comment>When a schema allows both objects and arrays via `type: ["object", "array"]`, an encoded array stays a string because the object branch returns before the array branch. Select the branch matching the parsed value, or attempt both declared types.</comment>
<file context>
@@ -283,12 +283,102 @@ export const outputTypeScript = <R>(definition: Definition<R>, pretty = false):
+ const resolved = dereference(schema, definitions)
+ if (resolved === undefined) return value
+
+ if (declaresType(resolved, "object")) {
+ const decoded = typeof value === "string" ? (parseAs(value, "object") ?? value) : value
+ if (!isPlainObject(decoded)) return decoded
</file context>
| const resolved = dereference(schema, definitions) | ||
| if (resolved === undefined) return value | ||
|
|
||
| if (declaresType(resolved, "object")) { |
There was a problem hiding this comment.
P2: The new decode path only fires when the resolved schema declares type directly on an object or array. Schemas using composition (allOf/anyOf/oneOf — including the Schema.Struct({}) case that this file already special-cases at line 140) or chained $refs (a definition that itself only $refs another) silently fall through to no-op, so a JSON-string arg is not decoded and the MCP rejection this PR targets persists for those common real-world schemas. dereference is also single-hop and ignores non-#/$defs|definitions ref scopes. The existing render path (hasUnresolvedRef, renderSchema) already recurses through anyOf/oneOf/allOf and follows refs, so this decode path understands a smaller subset of the schema. It is safe (pass-through, no false decode), but it under-delivers on the stated objective for composed/aliased schemas. Consider following refs to their terminal schema and unwrapping allOf/anyOf/oneOf (with a depth bound) before checking the declared type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-schema.ts, line 330:
<comment>The new decode path only fires when the resolved schema declares `type` directly on an object or array. Schemas using composition (`allOf`/`anyOf`/`oneOf` — including the `Schema.Struct({})` case that this file already special-cases at line 140) or chained `$ref`s (a definition that itself only `$ref`s another) silently fall through to no-op, so a JSON-string arg is not decoded and the MCP rejection this PR targets persists for those common real-world schemas. `dereference` is also single-hop and ignores non-`#/$defs|definitions` ref scopes. The existing render path (`hasUnresolvedRef`, `renderSchema`) already recurses through `anyOf`/`oneOf`/`allOf` and follows refs, so this decode path understands a smaller subset of the schema. It is safe (pass-through, no false decode), but it under-delivers on the stated objective for composed/aliased schemas. Consider following refs to their terminal schema and unwrapping `allOf`/`anyOf`/`oneOf` (with a depth bound) before checking the declared type.</comment>
<file context>
@@ -283,12 +283,102 @@ export const outputTypeScript = <R>(definition: Definition<R>, pretty = false):
+ const resolved = dereference(schema, definitions)
+ if (resolved === undefined) return value
+
+ if (declaresType(resolved, "object")) {
+ const decoded = typeof value === "string" ? (parseAs(value, "object") ?? value) : value
+ if (!isPlainObject(decoded)) return decoded
</file context>
9e8fe41 to
87531e2
Compare
There was a problem hiding this comment.
1 existing issue remains and 2 new issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/codemode/src/tool-schema.ts">
<violation number="1" location="packages/codemode/src/tool-schema.ts:319">
P1: When an object or array field contains another declared object or array field, `decodeInput` stops after the first level. Recursively apply each parsed child value's schema so nested MCP arguments are not still rejected as strings.</violation>
<violation number="2" location="packages/codemode/src/tool-schema.ts:319">
P3: Every JSON-Schema-described tool call with declared properties now rebuilds the whole input object via `Object.entries`/`Object.fromEntries`, even when no property needs decoding. The args were already copied by `copyIn`/`copyOut` in tool-runtime.ts before `decodeInput` runs, so this is a second full copy per call — pure overhead for large inputs. Only rebuild the object when at least one property actually decodes (e.g. track whether any `decodeJsonString` call changed its value).</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
| if (properties === undefined || typeof decoded !== "object" || decoded === null || Array.isArray(decoded)) | ||
| return decoded | ||
| return Object.fromEntries( | ||
| Object.entries(decoded).map(([name, entry]) => [name, decodeJsonString(entry, properties[name])]), |
There was a problem hiding this comment.
P1: When an object or array field contains another declared object or array field, decodeInput stops after the first level. Recursively apply each parsed child value's schema so nested MCP arguments are not still rejected as strings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-schema.ts, line 319:
<comment>When an object or array field contains another declared object or array field, `decodeInput` stops after the first level. Recursively apply each parsed child value's schema so nested MCP arguments are not still rejected as strings.</comment>
<file context>
@@ -283,12 +283,42 @@ export const outputTypeScript = <R>(definition: Definition<R>, pretty = false):
+ if (properties === undefined || typeof decoded !== "object" || decoded === null || Array.isArray(decoded))
+ return decoded
+ return Object.fromEntries(
+ Object.entries(decoded).map(([name, entry]) => [name, decodeJsonString(entry, properties[name])]),
+ )
+}
</file context>
| if (properties === undefined || typeof decoded !== "object" || decoded === null || Array.isArray(decoded)) | ||
| return decoded | ||
| return Object.fromEntries( | ||
| Object.entries(decoded).map(([name, entry]) => [name, decodeJsonString(entry, properties[name])]), |
There was a problem hiding this comment.
P3: Every JSON-Schema-described tool call with declared properties now rebuilds the whole input object via Object.entries/Object.fromEntries, even when no property needs decoding. The args were already copied by copyIn/copyOut in tool-runtime.ts before decodeInput runs, so this is a second full copy per call — pure overhead for large inputs. Only rebuild the object when at least one property actually decodes (e.g. track whether any decodeJsonString call changed its value).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-schema.ts, line 319:
<comment>Every JSON-Schema-described tool call with declared properties now rebuilds the whole input object via `Object.entries`/`Object.fromEntries`, even when no property needs decoding. The args were already copied by `copyIn`/`copyOut` in tool-runtime.ts before `decodeInput` runs, so this is a second full copy per call — pure overhead for large inputs. Only rebuild the object when at least one property actually decodes (e.g. track whether any `decodeJsonString` call changed its value).</comment>
<file context>
@@ -283,12 +283,42 @@ export const outputTypeScript = <R>(definition: Definition<R>, pretty = false):
+ if (properties === undefined || typeof decoded !== "object" || decoded === null || Array.isArray(decoded))
+ return decoded
+ return Object.fromEntries(
+ Object.entries(decoded).map(([name, entry]) => [name, decodeJsonString(entry, properties[name])]),
+ )
+}
</file context>
87531e2 to
f0b0937
Compare
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/codemode/src/tool-schema.ts">
<violation number="1" location="packages/codemode/src/tool-schema.ts:293">
P1: When a JSON-Schema object/array argument is passed as a string, `JSON.parse` runs after the runtime validates the original string. Revalidate the parsed value through the same boundary sanitizer, or decode before `copyIn`, so depth and blocked-member checks still apply.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const decodeJsonString = (value: unknown, schema: JsonSchema | undefined): unknown => { | ||
| if (typeof value !== "string" || (schema?.type !== "object" && schema?.type !== "array")) return value | ||
| try { | ||
| const parsed: unknown = JSON.parse(value) |
There was a problem hiding this comment.
P1: When a JSON-Schema object/array argument is passed as a string, JSON.parse runs after the runtime validates the original string. Revalidate the parsed value through the same boundary sanitizer, or decode before copyIn, so depth and blocked-member checks still apply.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-schema.ts, line 293:
<comment>When a JSON-Schema object/array argument is passed as a string, `JSON.parse` runs after the runtime validates the original string. Revalidate the parsed value through the same boundary sanitizer, or decode before `copyIn`, so depth and blocked-member checks still apply.</comment>
<file context>
@@ -283,12 +283,35 @@ export const outputTypeScript = <R>(definition: Definition<R>, pretty = false):
+const decodeJsonString = (value: unknown, schema: JsonSchema | undefined): unknown => {
+ if (typeof value !== "string" || (schema?.type !== "object" && schema?.type !== "array")) return value
+ try {
+ const parsed: unknown = JSON.parse(value)
+ if (schema.type === "array" ? Array.isArray(parsed) : isRecord(parsed)) return parsed
+ } catch {
</file context>
…object or array Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f0b0937 to
c50e705
Compare
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/codemode/src/tool-schema.ts">
<violation number="1" location="packages/codemode/src/tool-schema.ts:290">
P2: When a JSON Schema uses a legal union such as `type: ["object", "null"]`, this guard skips decoding, so stringified object or array arguments still reach `run` as strings. Accept `object` and `array` members of an array-valued `type` before parsing.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const decodeJsonString = (value: unknown, schema: JsonSchema | undefined): unknown => { | ||
| if (typeof value !== "string" || (schema?.type !== "object" && schema?.type !== "array")) return value | ||
| try { | ||
| const parsed: unknown = JSON.parse(value) | ||
| if (schema.type === "array" ? Array.isArray(parsed) : isRecord(parsed)) return parsed | ||
| } catch { | ||
| // not JSON: keep the original string | ||
| } | ||
| return value | ||
| } |
There was a problem hiding this comment.
P2: When a JSON Schema uses a legal union such as type: ["object", "null"], this guard skips decoding, so stringified object or array arguments still reach run as strings. Accept object and array members of an array-valued type before parsing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/codemode/src/tool-schema.ts, line 290:
<comment>When a JSON Schema uses a legal union such as `type: ["object", "null"]`, this guard skips decoding, so stringified object or array arguments still reach `run` as strings. Accept `object` and `array` members of an array-valued `type` before parsing.</comment>
<file context>
@@ -283,12 +283,34 @@ export const outputTypeScript = <R>(definition: Definition<R>, pretty = false):
+ typeof value === "object" && value !== null && !Array.isArray(value)
+
+/** Parses a JSON string standing in for a declared object/array (e.g. `args: "{}"`); every other value is untouched. */
+const decodeJsonString = (value: unknown, schema: JsonSchema | undefined): unknown => {
+ if (typeof value !== "string" || (schema?.type !== "object" && schema?.type !== "array")) return value
+ try {
</file context>
| const decodeJsonString = (value: unknown, schema: JsonSchema | undefined): unknown => { | |
| if (typeof value !== "string" || (schema?.type !== "object" && schema?.type !== "array")) return value | |
| try { | |
| const parsed: unknown = JSON.parse(value) | |
| if (schema.type === "array" ? Array.isArray(parsed) : isRecord(parsed)) return parsed | |
| } catch { | |
| // not JSON: keep the original string | |
| } | |
| return value | |
| } | |
| const decodeJsonString = (value: unknown, schema: JsonSchema | undefined): unknown => { | |
| const types: ReadonlyArray<string> = | |
| schema === undefined | |
| ? [] | |
| : Array.isArray(schema.type) | |
| ? schema.type | |
| : schema.type === undefined | |
| ? [] | |
| : [schema.type] | |
| if (typeof value !== "string" || (!types.includes("object") && !types.includes("array"))) return value | |
| try { | |
| const parsed: unknown = JSON.parse(value) | |
| if (types.includes("array") && Array.isArray(parsed)) return parsed | |
| if (types.includes("object") && isRecord(parsed)) return parsed | |
| } catch { | |
| // not JSON: keep the original string | |
| } | |
| return value | |
| } |
…hen the server rejects a string for a structure The schema-driven decode from #32 only fires when the declared property type is object or array. next-devtools-mcp advertises nextjs_call's args as type string while validating it as an object, so every args: "{}" call still fails with Invalid argument 'args' ... expected object, received string. decodeRejectedInput recognises that rejection text (zod issue and message forms) and re-parses every string property that encodes an object or array; invokeChildTool retries client.callTool once with the result, so the before hook and the permission ask run once and no non-MCP tool is affected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…hen the server rejects a string for a structure (#38) The schema-driven decode from #32 only fires when the declared property type is object or array. next-devtools-mcp advertises nextjs_call's args as type string while validating it as an object, so every args: "{}" call still fails with Invalid argument 'args' ... expected object, received string. decodeRejectedInput recognises that rejection text (zod issue and message forms) and re-parses every string property that encodes an object or array; invokeChildTool retries client.callTool once with the result, so the before hook and the permission ask run once and no non-MCP tool is affected. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
When the model hands a tool call a piece of input as JSON text instead of a real object (e.g.
args: "{}"), the call now works instead of failing. Previously the string passed through CodeMode untouched, the MCP server rejected it, and the whole turn was lost to a mistake with exactly one possible meaning.Context
Found while investigating the
executetool's ~41% error rate on Replo's/agent-toolsdashboard. Over 30 days of external production traffic this one mistake accounts for 274 failed tool calls — 34% of allexecutefailures, the largest single error class. The model writes:argsis declared{ type: "object" }, arrives as the string"{}", and the server rejects it (Expected object, received string). The split is perfectly clean:argsomitted → 1,016 calls / 3 errors;argsas an object → 482 calls / 0 errors;argsas a JSON string → 0 successes / 274 errors. Every observed failure is flat — a top-level property holding stringified JSON.What changed
decodeInput's JSON-Schema path (previously a pure pass-through) now decodes a JSON-encoded string in exactly two places: the input value itself, and each top-level property, when the schema declaresobjectorarraythere and the parse actually yields that type. Everything else — including a string the schema genuinely asks for, or a string that doesn't parse — is returned untouched.This is deliberately a decode, not a validation pass, and deliberately shallow: every failure in the data is a flat property, so there is no recursion, no
$refresolution, and no depth bookkeeping. If a nested variant ever shows up in the dashboard, deepening the walk is a small mechanical change to the same helper.Testing Done
bun testinpackages/codemode(264 pass),bun run typecheck,oxlintclean (same 8 pre-existing warnings asdev, none in new code).One test added covering all behaviors in a single call: object- and array-typed properties given JSON strings are decoded, a string-typed property whose value parses as JSON is left alone, a non-JSON string under an object-typed property passes through, and a whole input handed as a JSON string is decoded. The existing "JSON Schema is render-only" test passes unchanged — the guard that this didn't become a validation layer.