Rebuild request payload on cross-protocol failover retries

[Copilot]

Current in-request failover correctly reselects adapters, but retry recursion reused a request body already converted for the previous provider. That is safe for same-protocol retries and unsafe when fallback switches protocol (e.g., Gemini ↔ OpenAI/Claude).

• What changed

  • Added retry-time request reconstruction from the immutable original client payload instead of reusing previously converted provider payload.
  • Applied this in both retry branches:
    • handleStreamRequest (before recursive retry call)
    • handleUnaryRequest (before recursive retry call)
  • Kept existing stream integrity semantics: protocol-switch retry remains gated by anyDataSent (no restart after downstream bytes are emitted).

• Request reconstruction logic

  • Added rebuildRequestBodyForProtocolRetry(...) in src/utils/common.js.
  • Behavior:
    • starts from originalRequestBody
    • preserves internal runtime fields prefixed with _
    • converts only when retry provider protocol differs
    • reapplies provider-specific shaping (_applySystemPromptFromFile, custom model parameters)

• Retry context propagation

  • Extended retry context to carry originalRequestBody, enabling deterministic rebuild on recursive retries without re-reading request streams.

if (getProtocolPrefix(toProvider) !== getProtocolPrefix(retryToProvider)) {
  retryRequestBody = await rebuildRequestBodyForProtocolRetry(
    retryContext?.originalRequestBody,
    requestBody,
    fromProvider,
    retryToProvider,
    CONFIG
  );
}

[llamapreview]

AI Code Review by LlamaPReview

🎯 TL;DR & Recommendation

Recommendation: Approve with suggestions.
This PR adds request reconstruction for cross-protocol failover retries, ensuring correct provider conversion. However, the implementation introduces maintainability concerns and a speculative risk regarding the origin of originalRequestBody.

📄 Documentation Diagram

This diagram documents the refactored cross-protocol failover retry flow.

sequenceDiagram
    participant Client
    participant Handler
    participant RebuildFn as rebuildRequestBodyForProtocolRetry
    Client->>Handler: Original request
    Handler->>Handler: Process & convert for initial provider
    Note over Handler: PR #35;18: retry context carries originalRequestBody
    Handler->>Handler: Retry triggered
    alt Protocol change detected
        Handler->>RebuildFn: rebuildRequestBodyForProtocolRetry(...)
        RebuildFn->>RebuildFn: Clone & convert to new provider
        RebuildFn-->>Handler: Rebuilt request body
    else Same protocol
        Handler->>Handler: Reuse current body
    end
    Handler->>Client: Response

Loading

🌟 Strengths

• Changes address the cross-protocol payload corruption issue, critical for reliability.
• Unit tests pass, indicating basic correctness.

Priority	File	Category	Impact Summary	Anchors
P2	src/utils/common.js	Bug	Potential double conversion on cross-protocol retries
P2	src/utils/common.js	Maintainability	Deep clone pattern may drop non-serializable fields
P2	src/utils/common.js	Maintainability	Duplicated retry rebuild logic increases maintenance debt

🔍 Notable Themes

• Maintainability concerns: duplication of retry rebuild logic and use of JSON.stringify deep clone pattern.

---

💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.

[llamapreview]

P2 | Confidence: High

Using JSON.parse(JSON.stringify(…)) for deep cloning will silently strip undefined values, Symbol keys, functions, and convert Date objects to strings. While incoming request bodies are likely plain JSON‑compatible objects, this pattern introduces a latent risk if future enhancements add non‑serializable runtime fields (e.g., a function placeholder, undefined flags). Prefer a structured clone helper or a shallow copy that explicitly preserves required properties. The risk is low but grows as the codebase evolves.

Suggested change

	let rebuiltRequestBody = JSON.parse(JSON.stringify(originalRequestBody || {}));
	// Option 1: Use structuredClone if Node.js >= 17 (check runtime)
	let rebuiltRequestBody = structuredClone(originalRequestBody || {});
	// Option 2: Manual copy of expected keys (more explicit)
	// let rebuiltRequestBody = { ...originalRequestBody };

[llamapreview]

P2 | Confidence: High

The same protocol‑check and rebuild logic is duplicated in both handleStreamRequest and handleUnaryRequest. This duplicates maintenance burden and increases the chance of divergence. Consider extracting a small helper (e.g., maybeRebuildForRetry) to encapsulate the condition and call. While acceptable for a small PR, it adds technical debt.

Code Suggestion:

async function maybeRebuildForRetry(requestBody, toProvider, retryToProvider, fromProvider, retryContext, CONFIG) {
  if (getProtocolPrefix(toProvider) !== getProtocolPrefix(retryToProvider)) {
    return await rebuildRequestBodyForProtocolRetry(
      retryContext?.originalRequestBody,
      requestBody,
      fromProvider,
      retryToProvider,
      CONFIG
    );
  }
  return requestBody;
}

[llamapreview]

P2 | Confidence: Medium

Speculative: The variable originalRequestBody is used to seed the retry context, but its origin is not shown in the diff. The PR description states it should be the “immutable original client payload”. However, the surrounding code creates the retry context right after processedRequestBody is computed (which already has provider-specific transformations applied). If originalRequestBody is actually the already-converted processedRequestBody, then when rebuildRequestBodyForProtocolRetry deep‑clones it and later calls convertData from the original source provider to a new target provider, the result will be a double‑conversion – first to the initial provider’s format, then to the new provider’s format. This would produce a malformed request. Without the variable definition (outside the visible diff), the correctness cannot be confirmed. Unit tests passing reduces the likelihood, but they may not cover cross‑protocol failovers.