From a9cb7661ba04b78232dd5f4ba1085b5409dcf591 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 22 Aug 2026 18:45:42 +0900 Subject: [PATCH] fix(tools): repair integral floats in native u64 tool fields Codex advertises multi_agent_v1__wait_agent's timeout_ms as a JSON Schema number, but its Rust runtime deserializes the field as u64. Grok serializes the integer through a float, so a wait of 120000 arrives as 120000.0 and Codex rejects the call before the tool runs, with an invalid-type error naming a floating point value where u64 was expected. The #1611 repair already existed but declined here, because it only fires on a declared integer. The schema lookup was never the problem: the error text comes from Codex's own deserializer, which only sees the call after the bridge emitted it. Treat a known native u64 field as integer-declared when the schema declares a numeric type, so the existing re-stringify path emits 120000. A fractional value is a real disagreement and still fails upstream. The allowlist is one field wide on purpose. It names only what has a captured u64 rejection, because the repair is unambiguous only for a field that cannot hold a fraction; a generic name like start, priority, or port would silently rewrite a third-party tool's legitimate fractional value. Cursor's sibling yield_time_ms is also declared number and is deliberately not included: it gets its own change when it gets its own reproduction. Array items are judged by their own schema rather than inheriting the key, so an array named like the allowlist is not rewritten. Closes #2316 --- .../011_wp1_execution_record.md | 103 +++++++++++++++++ src/lib/tool-argument-integers.ts | 40 ++++++- tests/tool-argument-integers.test.ts | 104 ++++++++++++++++++ 3 files changed, 244 insertions(+), 3 deletions(-) create mode 100644 devlog/_plan/260822_backlog_disposition_program/011_wp1_execution_record.md diff --git a/devlog/_plan/260822_backlog_disposition_program/011_wp1_execution_record.md b/devlog/_plan/260822_backlog_disposition_program/011_wp1_execution_record.md new file mode 100644 index 0000000000..66800c48e9 --- /dev/null +++ b/devlog/_plan/260822_backlog_disposition_program/011_wp1_execution_record.md @@ -0,0 +1,103 @@ +# 011 — WP1 execution record: four merged, one held + +Work-phase 1 closed with **four of five** PRs on `origin/dev` and one held on +reproduced test evidence. + +## Merged + +| PR | Author | Merge commit | Review verdict | +|----|--------|--------------|----------------| +| #2309 kiro parallel permission | `Ingwannu` | `b96af222b` | PASS | +| #2339 google signature order | `luvs01` | `f26c7b5d2` | PASS | +| #2335 tool-choice linear time | `luvs01` | `25324f839` | GO-WITH-FIXES (blockers=0) | +| #2313 reasoning replay scoping | `olddonkey` | `f96d9efd2` | PASS | +| (wp0 roadmap, PR #2369) | — | `5921c20df` | docs-only | + +Each was reviewed by an independent read-only lane on `openrouter/stealth-ox-alpha` +at high reasoning effort. Two lanes produced **falsifiable** regression evidence rather +than diff-reading: + +- **#2339**: reverting only the `src/adapters/google.ts` hunk makes exactly one test + fail — `streaming signatures only attach to function calls that follow them in the + same frame`, expected `[undefined, SIGNATURE]`, received `[SIGNATURE, ...]`. +- **#2313**: five separate mutations of the fix each turn the suite red, including + dropping the serving identity from the memo key (the exact cross-conversation + leakage shape), which fails 2 tests. A baseline diff proves all 11 new integration + tests are genuinely new coverage, none tautological. +- **#2335**: the perf test was adversarially falsified before being trusted — the old + path produces ~65k proxy catalog reads at n=256 versus 512 for the new one, so the + `size * 2` bound genuinely discriminates O(n²) from O(n). + +## Held: #2359 + +The review lane returned **FAIL**, and the main agent reproduced it independently on +the merged tree: + +``` +$ bun test tests/provider-live-models.test.ts +(fail) opencode-free live discovery exposes big-pickle plus -free ids + [ "big-pickle", - "deepseek-v4-flash-free", "hy3-free", ... ] + at tests/provider-live-models.test.ts:163 + 7 pass, 1 fail +``` + +A live probe of `GET https://opencode.ai/zen/v1/models` shows +`deepseek-v4-flash-free` is **still advertised**, so excluding it hides a model the +gateway is currently serving. This is the same error class the author already +self-corrected once inside this PR: `d587a4b4` added `opencode-go/grok-4.6` to the +exclusion set and `e5c83067` retracted it after finding grok-4.6 live. + +Evidence posted to the PR (comment `5379495549`) with the failing assertion, the live +probe, the author's own precedent, and two non-blocking follow-ups. The two +`opencode-go` exclusions in the same PR are correct and land as soon as the Zen entry +is resolved. + +## Correction to `001`: `dev` IS protected + +`001` recorded "dev protection = 404 not protected" from +`GET /repos/.../branches/dev/protection`. That endpoint reports only **classic branch +protection**. The push was rejected: + +``` +remote: - Changes must be made through a pull request. + ! [remote rejected] dev -> dev (push declined due to repository rule violations) +``` + +`GET /repos/lidge-jun/opencodex/rulesets` shows four **active rulesets**, and +`GET /repos/.../rules/branches/dev` shows `deletion`, `non_fast_forward`, and +`pull_request` rules from ruleset `20763889` ("Protect dev"). Every later work-phase +lands through a pull request with admin merge — which is also better practice, since it +closes each PR and credits its author. The repository's security configuration was not +weakened to force a direct push. + +## Incident: a reset dropped an unpushed commit + +While preparing the merge I ran `git reset --hard origin/dev` on the shared checkout, +which discarded the unpushed wp0 devlog commit `eb3c97476`. Detected immediately with +`git merge-base --is-ancestor` (returned LOST), confirmed the object still existed via +`git cat-file -t`, and restored all 12 documents by cherry-pick (`d2f0aab86`, finally +`d374bb893` on the PR branch). No work was lost. + +The lesson is recorded rather than quietly fixed: a `--hard` reset on a shared checkout +carrying unpushed work is exactly the destructive-command class that deserves a +reachability check *before* it runs, not after. + +## Verification + +Local, focused only — full suites are not run on this machine: + +``` +bun x tsc --noEmit TSC=0 +bun test <9 suites covering every changed file> + 339 pass / 0 fail / 1578 expect() +``` + +Suites: `kiro-adapter`, `google-signature-history-roundtrip`, +`tool-choice-performance`, `types-barrel-identity`, `reasoning-replay-identity`, +`request-log-conversation`, `responses-opaque-blob-recovery`, +`provider-live-models`, `codex-catalog`. + +The full suite runs on remote host `lidge` against the merged head `5921c20df` as a +trailing job, observed rather than blocking; its result and the exact-head CI check are +recorded at the end of the program, not per phase. + diff --git a/src/lib/tool-argument-integers.ts b/src/lib/tool-argument-integers.ts index 19457c535f..6724fa002c 100644 --- a/src/lib/tool-argument-integers.ts +++ b/src/lib/tool-argument-integers.ts @@ -59,6 +59,24 @@ function declaresString(schema: SchemaNode): boolean { return Array.isArray(type) && type.includes("string"); } +// Issue #2316: Codex advertises `multi_agent_v1__wait_agent`'s `timeout_ms` as a JSON +// Schema `number`, but its Rust runtime deserializes the field as `u64` and rejects +// `120000.0` with "invalid type: floating point `120000.0`, expected u64" before the +// tool ever runs. The schema lookup is not the problem — the error text comes from +// Codex's own deserializer, so the call reached it — the problem is that `number` +// alone never authorizes the integral-float repair below. +// +// This allowlist is deliberately one field wide. It names only what has a live +// reproduction against a real `u64`, because the repair is only unambiguous for a +// field that cannot legitimately hold a fraction. Generic names (`start`, `end`, +// `priority`, `port`) would silently rewrite a third-party tool's fractional value, +// so a new entry needs its own evidence, not a plausible-sounding name. +// +// Cursor's sibling `yield_time_ms` (src/adapters/cursor/tool-definitions.ts) is also +// declared `number`; it is NOT included here because no rejection has been captured +// against it. It gets its own change when it gets its own reproduction. +const U64_NUMBER_FIELDS = new Set(["timeout_ms"]); + /** True when the node accepts a JSON number (`integer` or `number`), so a numeric * value is already schema-valid and must not be rewritten into a string. */ function declaresNumeric(schema: SchemaNode): boolean { @@ -118,7 +136,13 @@ interface CoerceResult { changed: boolean; } -function coerceValue(value: unknown, schema: SchemaNode | undefined, root: SchemaNode, depth: number): CoerceResult { +function coerceValue( + value: unknown, + schema: SchemaNode | undefined, + root: SchemaNode, + depth: number, + propertyName?: string, +): CoerceResult { // A hostile or deeply nested schema must not blow the stack. if (depth > 64) return { value, changed: false }; const resolved = schema ? resolveRef(schema, root, new Set()) : undefined; @@ -126,7 +150,15 @@ function coerceValue(value: unknown, schema: SchemaNode | undefined, root: Schem if (typeof value === "number") { if (!resolved) return { value, changed: false }; const branches = compositionBranches(resolved); - const integerDeclared = declaresInteger(resolved) || branches.some(declaresInteger); + // #2316: a known Codex-native u64 field counts as integer-declared even when the + // advertised schema says `number`, but only when the field really is numeric — + // an allowlisted name over a string-typed field is a disagreement, not a repair. + const nativeU64Declared = propertyName !== undefined + && U64_NUMBER_FIELDS.has(propertyName) + && (declaresNumeric(resolved) || branches.some(declaresNumeric)); + const integerDeclared = declaresInteger(resolved) + || branches.some(declaresInteger) + || nativeU64Declared; if (!integerDeclared && safelyIntegral(value)) { // Issue #1938: a bare integer in a string-only field has exactly one faithful // string reading. A field that also accepts a numeric type keeps the number. @@ -148,6 +180,8 @@ function coerceValue(value: unknown, schema: SchemaNode | undefined, root: Schem const itemSchema = resolved ? asSchema(resolved.items) : undefined; let changed = false; const next = value.map(entry => { + // Array items have no property name of their own; passing the array's key would + // let `timeout_ms: [1.5]` inherit the allowlist. Items are judged by schema only. const result = coerceValue(entry, itemSchema, root, depth + 1); if (result.changed) changed = true; return result.value; @@ -164,7 +198,7 @@ function coerceValue(value: unknown, schema: SchemaNode | undefined, root: Schem const next: Record = {}; for (const [key, entry] of Object.entries(object)) { const childSchema = asSchema(properties?.[key]) ?? additional; - const result = coerceValue(entry, childSchema, root, depth + 1); + const result = coerceValue(entry, childSchema, root, depth + 1, key); if (result.changed) changed = true; next[key] = result.value; } diff --git a/tests/tool-argument-integers.test.ts b/tests/tool-argument-integers.test.ts index 196cd74428..b09c782dc1 100644 --- a/tests/tool-argument-integers.test.ts +++ b/tests/tool-argument-integers.test.ts @@ -259,3 +259,107 @@ describe("bare-integer-for-string tool argument repair (#1938)", () => { }); }); + +/** + * The multi_agent wait shape from the #2316 report. Codex advertises `timeout_ms` as a + * JSON Schema `number` while its Rust runtime deserializes it as `u64`, so an integral + * float that is perfectly valid JSON is rejected before the tool runs. + */ +const MULTI_AGENT_WAIT_SCHEMA = { + type: "object", + properties: { + targets: { type: "array", items: { type: "string" } }, + timeout_ms: { type: "number" }, + temperature: { type: "number" }, + }, +}; + +describe("native u64 fields advertised as number (#2316)", () => { + test("repairs the exact wait_agent call from the report", () => { + expect(coerceIntegerToolArguments('{"targets":["a"],"timeout_ms":120000.0}', MULTI_AGENT_WAIT_SCHEMA)) + .toBe('{"targets":["a"],"timeout_ms":120000}'); + expect(coerceIntegerToolArguments('{"timeout_ms":60000.0}', MULTI_AGENT_WAIT_SCHEMA)) + .toBe('{"timeout_ms":60000}'); + }); + + test("a fractional timeout is a real disagreement and still fails upstream", () => { + const raw = '{"timeout_ms":1.5}'; + expect(coerceIntegerToolArguments(raw, MULTI_AGENT_WAIT_SCHEMA)).toBe(raw); + }); + + test("an ordinary number field beside it is still never touched", () => { + const raw = '{"temperature":1.0}'; + expect(coerceIntegerToolArguments(raw, MULTI_AGENT_WAIT_SCHEMA)).toBe(raw); + }); + + test("an already-integral payload keeps its original bytes", () => { + const clean = '{"targets":["a"],"timeout_ms":120000}'; + expect(coerceIntegerToolArguments(clean, MULTI_AGENT_WAIT_SCHEMA)).toBe(clean); + }); + + test("the allowlist reaches a nested object, not just the top level", () => { + const nested = { + type: "object", + properties: { opts: { type: "object", properties: { timeout_ms: { type: "number" } } } }, + }; + expect(coerceIntegerToolArguments('{"opts":{"timeout_ms":120000.0}}', nested)) + .toBe('{"opts":{"timeout_ms":120000}}'); + }); + + test("an array named like the allowlist does not inherit it", () => { + // Array items have no property name of their own, so the element is judged by its + // own schema. A fractional element stays fractional rather than being rewritten. + const arrayed = { + type: "object", + properties: { timeout_ms: { type: "array", items: { type: "number" } } }, + }; + const raw = '{"timeout_ms":[1.5]}'; + expect(coerceIntegerToolArguments(raw, arrayed)).toBe(raw); + }); + + test("an allowlisted name over a string field is a disagreement, not a repair", () => { + // `number` is what authorizes the repair. A string-typed timeout_ms carrying a bare + // integer falls to the #1938 rule instead, which stringifies it. + const stringy = { type: "object", properties: { timeout_ms: { type: "string" } } }; + expect(coerceIntegerToolArguments('{"timeout_ms":120000}', stringy)) + .toBe('{"timeout_ms":"120000"}'); + }); + + test("a field NOT on the allowlist keeps its float even when integral", () => { + // Deliberate scope proof: only names with a captured u64 rejection are repaired. + const others = { + type: "object", + properties: { yield_time_ms: { type: "number" }, priority: { type: "number" } }, + }; + const raw = '{"yield_time_ms":60000.0,"priority":2.0}'; + expect(coerceIntegerToolArguments(raw, others)).toBe(raw); + }); + + test("the namespaced wait_agent call is repaired through the real bridge", async () => { + const schemas = new Map>([ + ["multi_agent_v1__wait_agent", MULTI_AGENT_WAIT_SCHEMA], + ]); + const frames = await collectSse(bridgeToResponsesSSE( + replay([ + { type: "tool_call_start", id: "call_1", name: "multi_agent_v1__wait_agent" }, + { type: "tool_call_delta", arguments: '{"timeout_ms":120000.0}' }, + { type: "tool_call_end", id: "call_1" }, + { type: "done" }, + ]), + "grok-4.6", undefined, undefined, undefined, undefined, 2_000, { toolParameterSchemas: schemas }, + )); + const done = frames.find(f => f.event === "response.function_call_arguments.done"); + expect(done?.data.arguments).toBe('{"timeout_ms":120000}'); + + // The non-streaming path is the same contract; Codex parses the completed item. + const body = buildResponseJSON([ + { type: "tool_call_start", id: "call_1", name: "multi_agent_v1__wait_agent" }, + { type: "tool_call_delta", arguments: '{"timeout_ms":120000.0}' }, + { type: "tool_call_end", id: "call_1" }, + { type: "done" }, + ], "grok-4.6", { toolParameterSchemas: schemas }) as Record; + const call = (body.output as Record[]).find(i => i.type === "function_call"); + expect(call?.arguments).toBe('{"timeout_ms":120000}'); + }); +}); +