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}'); + }); +}); +