fix(xai): normalize Responses root tool schemas, and close out the merge round (#2690) #2727
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| # wp4 — L2 lane outcome | ||
|
|
||
| #2663 landed on `dev` as `cebe005db` (PR #2724), squashed into `cb9bb9b76`. | ||
| The PR is CLOSED with a comment naming the sha. | ||
|
|
||
| ## Squash fidelity was proven, not assumed | ||
|
|
||
| ``` | ||
| git diff 58f5a294e 71e182ae6 -> pr.diff (1087 lines) | ||
| git diff cb9bb9b76~1 cb9bb9b76 -> squash.diff (1087 lines) IDENTICAL | ||
| git diff --stat 71e182ae6 cb9bb9b76 -- <12 files> -> empty | ||
| git diff --stat 58f5a294e 64c6d642b -- <12 files> -> empty | ||
| ``` | ||
|
Comment on lines
+8
to
+13
Contributor
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add language tags to every evidence fence.
🧰 Tools🪛 markdownlint-cli2 (0.23.2)[warning] 8-8: Fenced code blocks should have a language specified (MD040, fenced-code-language) 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
|
|
||
| The last line is the one that mattered and would have been easy to skip: `dev` | ||
| moved 96 commits between the PR's merge base and the squash base, but touched none | ||
| of these 12 files. That is WHY a whole-take squash was safe here. Had any of those | ||
| files moved, the same procedure would have silently produced a different result. | ||
|
Comment on lines
+15
to
+18
Contributor
There was a problem hiding this comment. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
git diff --name-only 58f5a294e 71e182ae6 | sort -u > /tmp/pr-files
git log --format= --name-only 58f5a294e..64c6d642b | sed '/^$/d' | sort -u > /tmp/base-touched
comm -12 /tmp/pr-files /tmp/base-touchedRepository: lidge-jun/opencodex Length of output: 351 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- applicable repository conventions ---'
find /tmp/coderabbit-repo-knowledge/lidge-jun-opencodex-7afea732 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- target file ---'
cat -n devlog/_plan/260827_bug_pr_merge_round/031_wp4_l2_outcome.md
printf '%s\n' '--- available revisions ---'
git rev-parse --verify 58f5a294e^{commit} 2>&1 || true
git rev-parse --verify 71e182ae6^{commit} 2>&1 || true
git rev-parse --verify 64c6d642b^{commit} 2>&1 || true
git show-ref | head -40Repository: lidge-jun/opencodex Length of output: 6960 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- referenced commit objects ---'
for rev in cb9bb9b76 cebe005db 58f5a294e 64c6d642b 71e182ae6; do
printf '%s: ' "$rev"
git rev-parse --verify "$rev^{commit}" 2>&1 || true
done
printf '%s\n' '--- relevant commit metadata ---'
for rev in 58f5a294e 64c6d642b cebe005db cb9bb9b76; do
git show -s --format='%H%n%P%n%ad%n%s' --date=iso-strict "$rev" 2>&1 || true
done
printf '%s\n' '--- commit counts and changed paths ---'
git rev-list --count 58f5a294e..64c6d642b
git diff --name-only 58f5a294e 64c6d642b | sort -u
git diff --name-only 58f5a294e cebe005db | sort -u
git diff --name-only 58f5a294e cb9bb9b76 | sort -u
printf '%s\n' '--- nearby history ---'
git log --oneline --decorate --ancestry-path 58f5a294e..64c6d642b | head -120Repository: lidge-jun/opencodex Length of output: 36462 🏁 Script executed: #!/bin/bash
set -euo pipefail
pr_files=$(mktemp)
history_files=$(mktemp)
trap 'rm -f "$pr_files" "$history_files"' EXIT
git diff --name-only cb9bb9b7634640f18568207322d386a059f6c9ac^ cb9bb9b7634640f18568207322d386a059f6c9ac | sort -u > "$pr_files"
git log --format= --name-only 58f5a294e618ee5d9bcabf7f7a349ddbbb6a8ad5..64c6d642bd795ef5328e51292bc72300847189fe -- $(cat "$pr_files") |
sed '/^$/d' | sort -u > "$history_files"
printf '%s\n' '--- squash commit files ---'
cat "$pr_files"
printf '%s\n' '--- historical intersection: 58f5a294e..64c6d642b ---'
comm -12 "$pr_files" "$history_files"
printf '%s\n' '--- counts ---'
printf 'all commits: '
git rev-list --count 58f5a294e618ee5d9bcabf7f7a349ddbbb6a8ad5..64c6d642bd795ef5328e51292bc72300847189fe
printf 'first-parent commits: '
git rev-list --count --first-parent 58f5a294e618ee5d9bcabf7f7a349ddbbb6a8ad5..64c6d642bd795ef5328e51292bc72300847189fe
printf 'non-merge commits: '
git rev-list --count --no-merges 58f5a294e618ee5d9bcabf7f7a349ddbbb6a8ad5..64c6d642bd795ef5328e51292bc72300847189fe
printf '%s\n' '--- endpoint path diff for the squash files ---'
git diff --stat 58f5a294e618ee5d9bcabf7f7a349ddbbb6a8ad5 64c6d642bd795ef5328e51292bc72300847189fe -- $(cat "$pr_files")Repository: lidge-jun/opencodex Length of output: 783 Record the historical non-touch check and use a valid revision. At 🤖 Prompt for AI Agents |
||
|
|
||
| ## The guard question, answered properly | ||
|
|
||
| The reviewer's main line of attack was the right one: `rememberPassthroughResponseChecked` | ||
| now runs the undeclared-tool guard on a RESTORED response | ||
| (`src/server/responses/core.ts:3286-3299`), so restoration could in principle launder | ||
| an undeclared name into a declared one. | ||
|
|
||
| It cannot, and the reason is structural rather than incidental: restoration is strictly | ||
| NARROWER than the guard's own normalization. Both call `normalizeDeclaredToolName` | ||
| (`src/types/tools.ts:47`), and `routedCustomToolTargetName` | ||
| (`src/responses/custom-tool-compat.ts:70`) additionally requires the normalized target | ||
| to be in the routed set. Any name restoration rewrites was already declared-equivalent | ||
| to the guard. | ||
|
|
||
| Measured, guard verdict before vs after restoration: | ||
|
|
||
| | item | before | after | resulting name | | ||
| |---|---|---|---| | ||
| | `other_tool` | refused | refused | `other_tool` | | ||
| | `rm` | refused | refused | `rm` | | ||
| | `apply_patch` (helper) | accepted | accepted | `exec` | | ||
| | `namespace:"evil"` + `apply_patch` | refused | refused | `apply_patch` | | ||
| | `apply_patch_evil` | refused | refused | `apply_patch_evil` | | ||
|
|
||
| No row flips from refused to accepted. | ||
|
|
||
| ## Helper matching is exact membership, and that was tested adversarially | ||
|
|
||
| 13 hostile variants probed — `apply_patch_evil`, `evil__apply_patch`, `Apply_Patch`, | ||
| `apply_patch ` (trailing space), `apply-patch`, `tools.apply_patch`, `exec_commandX` | ||
| — all returned `target = undefined`. A substring predicate would have captured three | ||
| of them. Injection is closed the same way: `compileCodeModeHelperInput` serializes | ||
| every value with `JSON.stringify`, and hostile payloads (quote-escape, backtick/\${}, | ||
| U+2028/2029, patch-body breakout) each compiled to exactly one `await tools.` call | ||
| with zero escapes out of the string literal. | ||
|
|
||
| ## Test changes were repointed, not weakened | ||
|
|
||
| Every removed `apply_patch` refusal assertion is replaced by an equivalent | ||
| `other_tool` refusal — still-undeclared, so refusal coverage is preserved while | ||
| `apply_patch` becomes a recognized helper. The suite also gains bridged-turn | ||
| continuation, streamed bridging, and escape-resistance coverage. | ||
|
|
||
| Incidental find: `tests/bridge-legacy-shell-normalization.test.ts` fixes a latent bug | ||
| in its own helper (`delta:` -> `arguments:`). The old field was never read, so those | ||
| assertions were partly vacuous before. | ||
|
|
||
| ## Evidence | ||
|
|
||
| - `bun run test` full suite: exit 0 | ||
| - reviewer's independent run over 7 relevant files: 189 pass / 0 fail | ||
| - `bun x tsc --noEmit`: clean on the merged tree | ||
| - PR #2724 CI: 23/23 green after one rerun of the `update-stop-first` launcher flake | ||
| (`git diff --name-only` vs dev shows NO_OVERLAP with any update/launcher file) | ||
| - VERDICT: PASS | ||
|
|
||
| ## Incident: `core.bare` flipped mid-lane | ||
|
|
||
| After the merge, `git status` in the main checkout began failing with "this operation | ||
| must be run in a work tree" and `git worktree list` reported the repository as | ||
| `(bare)`. `git config --local --get core.bare` returned `true` while every tracked | ||
| file and `.git/index` were intact. Restored with | ||
| `git config --local core.bare false`; no data was lost and no commit was affected. | ||
| Cause not established — worth watching if it recurs while many worktrees are live. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| # wp5 — L4 lane outcome | ||
|
|
||
| ## #2694 — NOOP, closed | ||
|
|
||
| The landed #2663 (`cebe005db`) does provider-agnostically what #2694 hand-built for | ||
| one provider. Measured on current dev: | ||
|
|
||
| ``` | ||
| exec_command -> exec | ||
| shell_command -> exec | ||
| apply_patch -> exec | ||
| compiled: const result = await tools.exec_command({"cmd":"pwd"}); | ||
| text(result); | ||
| ``` | ||
|
|
||
| That is the same wrapper `codeModeExecCommandInput` produced. Closed with the five | ||
| `tsc` errors and the nonexistent `sensenova` provider id spelled out, so the author | ||
| knows why it could not have landed as written. | ||
|
|
||
| ## #2690 — landed whole, NOT reimplemented | ||
|
|
||
| 003 reclassified this L3 -> L4 on the reviewer's finding that the fix imports the | ||
| module the refactor creates, so "take the fix, leave the refactor" was incoherent. | ||
| The plan then said: rebase and land whole, or reimplement against the existing helper. | ||
|
|
||
| The author (olddonkey) rebased it themselves onto post-#2684 dev, twice, ending at | ||
| `669efd568`. So the reimplement never became necessary — the branch merges clean and | ||
| the merged tree passes: | ||
|
|
||
| ``` | ||
| git merge-tree origin/dev pr2690-fresh -> exit 0 (no conflict) | ||
| bun x tsc --noEmit (merged tree) -> clean | ||
| bun test xai-tool-schema, xai-transport, | ||
| openai-responses-passthrough, | ||
| azure-model-router-tool-schema -> 165 pass / 0 fail | ||
| ``` | ||
|
|
||
| The Azure suite is the one that matters there: it is #2684's, in the region #2690 | ||
| conflicted with. Both now coexist. Merged as `3d986ef9c`. | ||
|
|
||
| Lesson worth keeping: ordering #2684 first (005, correction 2) was what made this | ||
| cheap. Had #2690 landed first, #2684 would have been the one needing a rewrite. | ||
|
|
||
| ## #2693 — BLOCKED on an upstream fact, left open | ||
|
|
||
| Test-only diff whose test fails on its own branch; | ||
| `skip_thought_signature_validator` exists nowhere in `src/`. The blocking question is | ||
| posted on the PR: does Gemini 3 on Antigravity actually honor that sentinel as a | ||
| functionCall `thoughtSignature`? Dev deliberately refuses to forward non-genuine | ||
| signatures, so implementing the fallback without that fact risks trading a clean 400 | ||
| for a silently degraded turn. Left OPEN rather than closed — it is a real question, | ||
| not a rejected patch. | ||
|
|
||
| ## #2638 and #2497 — NEEDS_HUMAN, security boundary | ||
|
|
||
| Both are hygiene-blocked for the same reason, and it is the correct reason. | ||
|
|
||
| ``` | ||
| unsponsored_surface — This changes an authentication, workflow, release-automation, | ||
| or dependency surface. MAINTAINERS.md requires security review for these. | ||
| Paths: src/codex/auth-context.ts | ||
| ``` | ||
|
Comment on lines
+58
to
+62
Contributor
There was a problem hiding this comment. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win Separate the security-surface evidence by PR. The Also applies to: 87-91 🧰 Tools🪛 markdownlint-cli2 (0.23.2)[warning] 58-58: Fenced code blocks should have a language specified (MD040, fenced-code-language) 🤖 Prompt for AI Agents |
||
|
|
||
| ### #2638 is in better shape than its label suggests | ||
|
|
||
| Its auth surface is 14 lines in `src/codex/auth-context.ts`: hoisting | ||
| `nativeMainSelectionOnly` to a const, and widening ONE condition from | ||
| `nativeMainTrafficBlocked` to `nativeMainReadsForbidden` so a turn drain reports the | ||
| temporary fence instead of a permanent model-entitlement denial. No other auth, | ||
| credential, OAuth, token, workflow or release path is touched | ||
| (`git diff --name-only` over auth-ish patterns returns only that file and its test). | ||
|
|
||
| Verified at the merged tree: | ||
|
|
||
| ``` | ||
| git merge (dev + pr2638) -> MERGE_OK | ||
| bun x tsc --noEmit -> clean | ||
| codex-auth-context, codex-routing, subagent-fallback-handle-responses, | ||
| core-lab-boundary -> 267 pass / 0 fail | ||
| src/server/index.ts, lifecycle.ts, router.ts -> NOT TOUCHED | ||
| ``` | ||
|
|
||
| That last line matters: `AGENTS.md` warns that an `await` added to the synchronous | ||
| activation chain silently reroutes subagents to a different model than the operator | ||
| configured. This PR does not touch those files, and `core-lab-boundary` passes. | ||
|
|
||
| ### #2497 is the heavier one | ||
|
|
||
| 2622/76 across 20 files, CONFLICTING on five including `src/server/responses/core.ts`, | ||
| and it touches `src/oauth/chatgpt.ts` (+88/-12) and `src/codex/main-account.ts` | ||
| (+551/-12) — OAuth refresh and credential storage. | ||
|
|
||
| ### Why this round stops here | ||
|
|
||
| `MAINTAINERS.md` requires explicit security review for these surfaces, and the gate | ||
| asks for a maintainer to apply `maintainer-sponsored` after reviewing. An agent | ||
| applying that label to clear its own PR would defeat the check entirely — the label | ||
| IS the human judgment. So both are reported with evidence and left for the maintainer. | ||
|
|
||
| This is `NEEDS_HUMAN`, not `BLOCKED`: nothing external is missing, a person's | ||
| decision is. | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the PR reference as paragraph text.
Line 3 starts with
#2663without a space.markdownlint-cli2reports MD018. Prefix the reference withPRor escape#so the record renders consistently and remains lint-clean.🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 3-3: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Source: Linters/SAST tools