Optimize cosmos gateway header handling

[xinlian12]

No description provided.

[github-actions]

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• releaseassets.githubusercontent.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "releaseassets.githubusercontent.com"

See Network Configuration for more information.

Generated by sdkReviewAgent for issue #28

[github-actions]

🟡 Recommendation · Correctness: getHeader() optimization silently breaks case-insensitive lookup

When keysAlreadyLowerCased=true, the lookup key is used as-is (no toLowerCase). But stored keys are lowercase because HTTP/2 Netty frames provide them in lowercase, while callers of value()/values() may still pass mixed-case names like "Content-Type". Those lookups will silently return null, despite the header being present.

Example:

// keysAlreadyLowerCased=true (H2 path)
headers.set("content-type", "application/json");  // stored as "content-type"
headers.value("Content-Type");                     // → null (bug: key="Content-Type" ≠ "content-type")

The optimization is sound for set() (callers provide lowercase keys from the H2 frame), but not for lookups — callers have no reason to know they must pass lowercase names. The current call path in RxGatewayStoreModel doesn't trigger this (it only calls toMap()/toLowerCaseMap()), so it's latent rather than a current regression, but it breaks the class's implicit case-insensitive contract.

Fix: Always normalize the lookup key, regardless of the flag:

private HttpHeader getHeader(String headerName) {
    return headers.get(headerName.toLowerCase(Locale.ROOT));
}

The keysAlreadyLowerCased flag is only appropriate as a write-path optimization in set().

---

_{⚠️ AI-generated review — may be incorrect. Agree? → resolve the conversation. Disagree? → reply with your reasoning.}

[github-actions]

🟡 Recommendation · Correctness: withRemappedStatusCode now unconditionally inserts REQUEST_CHARGE

The previous implementation only updated REQUEST_CHARGE if it already existed among responseHeaderNames (it iterated and updated in-place). The new code always inserts the key:

headers.put(requestChargeKey, String.valueOf(newRequestCharge));

If a StoreResponse was constructed without a REQUEST_CHARGE header (e.g. a synthetic or fault-injected response), the original withRemappedStatusCode returned a response still lacking the header. Now it returns a response with REQUEST_CHARGE = 0.0 + additionalRequestCharge.

In production Cosmos DB responses this header is always present, so this is unlikely to cause a visible regression. However, it's a silent behavioral change that could affect tests that deliberately construct responses without REQUEST_CHARGE. Consider mirroring the original guard:

if (headers.containsKey(requestChargeKey) || additionalRequestCharge != 0) {
    headers.put(requestChargeKey, String.valueOf(newRequestCharge));
}

---

_{⚠️ AI-generated review — may be incorrect. Agree? → resolve the conversation. Disagree? → reply with your reasoning.}

[github-actions]

🟢 Suggestion · Code Quality: The toMap() vs toLowerCaseMap() conditional provides no performance benefit here

The comment says "avoid redundant work" by calling toMap() for H2. But toLowerCaseMap() already uses the pre-lowercased map key (entry.getKey()) — it does not call toLowerCase() on each key at this step:

// toLowerCaseMap() — no toLowerCase() called on keys here
for (Map.Entry(String, HttpHeader) entry : headers.entrySet()) {
    result.put(entry.getKey(), entry.getValue().value());  // key already lowercase
}

The actual savings from keysAlreadyLowerCased=true happen earlier, in HttpHeaders.set() during header population in ReactorNettyClient.headers(). The conditional on line 236 is correct for correctness (toMap() preserves original case for H1, which would be wrong), but the performance framing in the comment is misleading.

Consider updating the comment to accurately reflect where the optimization occurs, so future readers don't draw incorrect conclusions about toLowerCaseMap().

---

_{⚠️ AI-generated review — may be incorrect. Agree? → resolve the conversation. Disagree? → reply with your reasoning.}

[github-actions]

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• releaseassets.githubusercontent.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "releaseassets.githubusercontent.com"

See Network Configuration for more information.

Generated by sdkReviewAgent for issue #28

[github-actions]

🟡 Recommendation · Code Quality: getResponseHeaders() exposes the live mutable internal map

responseHeaders is a final HashMap, but final only prevents reassignment — the map's contents remain mutable. Returning the live reference means any caller can silently modify StoreResponse internal state:

storeResponse.getResponseHeaders().put("x-ms-foo", "injected"); // modifies StoreResponse internals

All current production callers are well-behaved (they either take a defensive copy or read-only), but this is a latent footgun. Hardening with Collections.unmodifiableMap is zero-cost at runtime for read paths and doesn't break anything — setHeaderValue() mutates this.responseHeaders directly, not through the returned reference:

public Map(String, String) getResponseHeaders() {
    return Collections.unmodifiableMap(responseHeaders);
}

---

_{⚠️ AI-generated review — may be incorrect. Agree? → resolve the conversation. Disagree? → reply with your reasoning.}

[github-actions]

🟡 Recommendation · Test Coverage: withRemappedStatusCode has no unit test

This method was substantially refactored (parallel arrays → Map, new keysAlreadyLowerCased=true constructor path, containsKey guard for REQUEST_CHARGE), but StoreResponseTest covers none of it. Consider adding tests for:

1. Status code is updated — returned response has newStatusCode, not the original
2. REQUEST_CHARGE is accumulated — if header is present, additionalRequestCharge is added
3. REQUEST_CHARGE absent — if header was never set, no charge header is introduced (guards against regression of the prior unconditional-insert bug)
4. Headers are copied, not aliased — mutating the original StoreResponse headers after calling withRemappedStatusCode does not affect the returned response

Example skeleton:

`@Test`(groups = { "unit" })
public void withRemappedStatusCode_updatesChargeAndStatus() {
    HashMap(String, String) headers = new HashMap<>();
    headers.put(HttpConstants.HttpHeaders.REQUEST_CHARGE.toLowerCase(Locale.ROOT), "5.0");
    StoreResponse original = new StoreResponse(null, 200, headers, null, 0);

    StoreResponse remapped = original.withRemappedStatusCode(201, 2.0);

    assertThat(remapped.getStatus()).isEqualTo(201);
    assertThat(Double.parseDouble(remapped.getHeaderValue(HttpConstants.HttpHeaders.REQUEST_CHARGE))).isEqualTo(7.0);
    // original is unmodified
    assertThat(original.getStatus()).isEqualTo(200);
}

---

_{⚠️ AI-generated review — may be incorrect. Agree? → resolve the conversation. Disagree? → reply with your reasoning.}

[github-actions]

Warning

⚠️ Firewall blocked 2 domains

The following domains were blocked by the firewall during workflow execution:

• centralus2.in.applicationinsights.azure.com
• dc.services.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "centralus2.in.applicationinsights.azure.com"
    - "dc.services.visualstudio.com"

See Network Configuration for more information.

Note

🔒 Integrity filtering filtered 1 item

Integrity filtering activated and filtered the following item during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.

• pr:#28 (pull_request_read: Resource 'pr:#28' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)

Generated by sdkReviewAgent for issue #28

[github-actions]

🔴 Blocking · Correctness: HttpUtils.unescape() always throws UnsupportedOperationException on the ImmutableMap returned by asMap()

response.getHeaders().asMap(activityId) returns a com.azure.cosmos.implementation.guava25.collect.ImmutableMap. This PR now passes that directly to HttpUtils.unescape():

// ThinClientStoreModel.java line 145
HttpUtils.unescape(response.getHeaders().asMap(request.getActivityId()))

HttpUtils.unescape(Map) calls headers.computeIfPresent(...), and the vendored ImmutableMap overrides computeIfPresent to always throw unconditionally:

// guava25/collect/ImmutableMap.java:583-586
`@Deprecated`
`@Override`
public final V computeIfPresent(K key, BiFunction<...> remappingFunction) {
    throw new UnsupportedOperationException();   // always, no key check
}

So every call through ThinClientStoreModel.unwrapToStoreResponse that successfully decodes an RntbdResponse will throw UnsupportedOperationException before producing a StoreResponse. ThinClient mode is broken for all responses.

Before this PR, the old code wrapped in HttpHeaders first and then called unescape on the mutable map from toLowerCaseMap():

// Old (safe)
new HttpHeaders(response.getHeaders().asMap(request.getActivityId()))
// ... parent called: HttpUtils.unescape(headers.toLowerCaseMap())  ← new mutable HashMap

Fix: Wrap in a mutable HashMap before calling unescape:

HttpUtils.unescape(new HashMap<>(response.getHeaders().asMap(request.getActivityId()))),

This preserves the original semantics and avoids the UnsupportedOperationException.

---

_{⚠️ AI-generated review — may be incorrect. Agree? → resolve the conversation. Disagree? → reply with your reasoning.}

[github-actions]

Warning

⚠️ Firewall blocked 2 domains

The following domains were blocked by the firewall during workflow execution:

• centralus2.in.applicationinsights.azure.com
• dc.services.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "centralus2.in.applicationinsights.azure.com"
    - "dc.services.visualstudio.com"

See Network Configuration for more information.

Note

🔒 Integrity filtering filtered 2 items

Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.

• pr:#28 (pull_request_read: Resource 'pr:#28' has lower integrity than agent requires. Agent would need to drop integrity tags [approved:all unapproved:all] to trust this resource.)
• pr:#28 (pull_request_read: Resource 'pr:#28' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)

Generated by sdkReviewAgent for issue #28

[github-actions]

🔴 Blocking · Correctness: Mixed-case RNTBD header keys bypass lowercase normalization

new HashMap<>(...) copies keys verbatim from the ImmutableMap returned by RntbdResponseHeaders.asMap(). That map is built using WFConstants.BackendHeaders constants, which include mixed-case values:

// WFConstants.BackendHeaders
public static final String GLOBAL_COMMITTED_LSN = "x-ms-global-Committed-lsn";  // uppercase 'C'

The resulting HashMap therefore contains "x-ms-global-Committed-lsn" as a key. It is then passed to super.unwrapToStoreResponse() → StoreResponse.withLowerCasedHeaders(…, keysAlreadyLowerCased=true), which stores the key as-is, without normalisation.

Subsequent lookups call getHeaderValue(WFConstants.BackendHeaders.GLOBAL_COMMITTED_LSN), which normalises the argument to "x-ms-global-committed-lsn" (all-lowercase) via attribute.toLowerCase(Locale.ROOT). That key is absent from the map, so the lookup returns null and globalCommittedLSN is always −1 in ThinClient mode — silently breaking quorum-read consistency semantics.

Root cause: before this PR, new HttpHeaders(immutableMap) called set() for every entry, which performed name.toLowerCase(Locale.ROOT). The new new HashMap<>(...) copy preserves the original casing.

Fix — normalise during the copy:

Map(String, String) rawHeaders = response.getHeaders().asMap(request.getActivityId());
Map(String, String) loweredHeaders = new HashMap<>(rawHeaders.size());
rawHeaders.forEach((k, v) -> loweredHeaders.put(k.toLowerCase(Locale.ROOT), v));
StoreResponse storeResponse = super.unwrapToStoreResponse(
    endpoint, request, response.getStatus().code(),
    HttpUtils.unescape(loweredHeaders), payloadBuf);

---

_{⚠️ AI-generated review — may be incorrect. Agree? → resolve the conversation. Disagree? → reply with your reasoning.}