fix(chat): make tool-call name recovery linear

[luvs01]

Summary

• Build one request-local call_id -> function name index while translating Chat Completions history instead of rescanning every previously emitted item for each assistant message.
• Preserve both earlier-message recovery and same-array recovery when replace-style clients resend a tool call without function.name.
• Add a deterministic 1,000-call regression that proves one matching Map.set per call rather than relying on machine-dependent timing.

The existing recovery path rebuilt the same map from the full translated transcript for every assistant message. Long tool-heavy histories therefore performed quadratic map population even though the required state is incremental and request-local.

Verification

• Base: b81314cd29b78fecb447df882dc4fc1a987434b9
• Head: 9118aef00e537f2abb09a3de3445f0cfd6b9bb18
• Bun 1.3.14: bun test --isolate --timeout 60000 tests/chat-completions-endpoint.test.ts — 75 passed, 241 assertions.
• Bun 1.4.0-canary.1: the same focused command — 75 passed, 241 assertions.
• bun run typecheck — passed under Bun 1.3.14 and Bun 1.4.0-canary.1.
• bun run privacy:scan — passed under both runtimes.
• git diff --check origin/dev...HEAD — passed.
• Stable patch ID 1940d3da5f8f38b8c4eb9d5cdbd6ddf6c669e5c4 is unchanged from the previously reviewed head; the 12 intervening upstream commits did not touch either changed file.
• Independent correctness and Codex Security diff reviews found no P0-P2 issue.

The full repository suite is not claimed green. Maintained exact-head cross-platform CI and fresh maintainer review remain required after this rebase.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed. No user-facing contract changes.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. This patch does not change an auth or credential boundary.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

• All CI tests are green on my local testing.

• I pushed my PR to the latest dev commit.

• I resolved all correct Codex and CodeRabbit findings.

• My PR is ready for review.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ec00ac30-a736-48e5-9e39-43689b2b534d

📥 Commits

Reviewing files that changed from the base of the PR and between 65eda6c and 941b8a2.

📒 Files selected for processing (2)

• src/chat/inbound.ts
• tests/chat-completions-endpoint.test.ts

Included review availability: Your plan includes up to 10 reviews per rolling hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

The inbound Chat Completions translation now shares tool-call names across messages. Tests cover cross-message recovery, duplicate call IDs, and map insertion behavior across 1,000 messages.

Changes

Tool-call name recovery

Layer / File(s)	Summary
Shared tool-call name state src/chat/inbound.ts	chatCompletionsToResponsesBody creates one call-ID/name map. toolCallsToItems uses the shared map during assistant tool-call translation.
Recovery and indexing validation tests/chat-completions-endpoint.test.ts	Tests cover recovery from earlier messages, repeated call IDs in one array, and one matching map insertion per call across 1,000 messages. Obsolete comments were removed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 941b8

This localized change makes tool-call name recovery linear and adds regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Possibly related PRs

• lidge-jun/opencodex#1531: Updates streamed tool-call name handling in another adapter.

Suggested reviewers: lidge-jun

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: making tool-call name recovery linear.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

✅ READY

• all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

• ✅ All CI tests are green on my local testing.
• ✅ I pushed my PR to the latest dev commit.
• ✅ I resolved all correct Codex and CodeRabbit findings.
• ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu @Wibias

[Wibias]

Full review: the actual linearization looks correct. I found no P0-P2 code defect in the PR patch. Keeping one request-local call_id -> function name map preserves both earlier-message and same-array recovery while removing the repeated transcript rescans.

Not merge-ready yet for integration reasons:

• This branch is now far behind current dev, and src/chat/inbound.ts has changed on dev since the PR's merge-base. The optimization is still needed on current dev, but the branch should be updated/rebased and revalidated against the current translator code.
• The current cross-platform run on this head has not established a final green result yet.
• The PR is still a draft.

Verdict: code direction is good; update to current dev, rerun CI, then this is ready for another merge-readiness check.

[abhisheksharma2411]

Nice find — rebuilding the whole call-id index on every assistant message is a genuine O(n^2), and on a long agent transcript that's exactly the shape that gets expensive quietly. Hoisting the Map out and letting it accumulate is the obvious right move once you see it.

I checked the bit that would worry me about this kind of change — whether the incremental map can ever see less than the rebuild did:

• toolCallsToItems is the only thing in inbound.ts that pushes a type: "function_call" item (one input.push at line 121), and it now registers the name on every push. So every entry the rebuild would have found is in the map.
• input is append-only inside chatCompletionsToResponsesBody — no splice/filter/reassign — so the persistent map can't drift from what a fresh scan would produce.

That makes it equivalent rather than just "probably fine", which is what I'd want before touching name recovery. Also glad the same-array case is still covered; the pre-existing knownNameByCallId.set(...) inside the loop already made that work, and the strengthened assertion (toEqual(["exec_command", "exec_command"]) instead of some(...)) is a real improvement — the old some would have passed on the named entry alone and told you nothing about the recovered one.

One thought on the linearity test, and it's a genuine trade-off rather than a complaint:

Patching Map.prototype.set pins the implementation rather than the property. It does distinguish the two versions cleanly — the old rebuild would run ~500k sets for count = 1000 against the new 1k — and the try/finally restore via the saved descriptor is careful. But a future refactor that stays linear while changing shape (a plain object, a Map that only sets when absent, indexing somewhere else) would fail this test without anything being wrong.

Not sure there's a better option that isn't flaky — wall-clock certainly isn't — so this may just be the right call. If you wanted to soften it slightly, asserting an upper bound (toBeLessThanOrEqual(count * 2)) rather than exact equality would keep the O(n^2) detection while leaving room for an honest refactor. Entirely your call; the strict version does document the intent more sharply.

Looks good to me 🙂