fix(responses): clear stripped service tier from logs - #1368
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Responses adapter now clears unsupported service-tier metadata from request logging. A DeepSeek integration test verifies removal from the outbound request and logging context. ChangesService-tier capability handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
Please rebase this PR onto the latest |
a41938a to
a8751d5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/service-tier-capability.test.ts`:
- Around line 150-152: Update the test around the outbound request assertions to
first require that bodies[0] is defined, then check that its body does not
contain service_tier. Remove the (bodies[0] ?? {}) fallback so the test cannot
pass without handleResponses capturing a fetch request; preserve the existing
logCtx.requestedServiceTier and requestedSpeedLabel assertions.
🪄 Autofix
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a96af793-9a15-4467-ad70-e3a6f832e248
📒 Files selected for processing (2)
src/server/responses/core.tstests/service-tier-capability.test.ts
|
|
|
|
|
An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
a8751d5 to
eb5fdfb
Compare
Rate Limit Exceeded
|
Rate Limit Exceeded
|
eb5fdfb to
aead7d6
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== HEAD and base ancestry =='
git rev-parse HEAD
git merge-base --is-ancestor e8ce2b93dd3ef8074d96dee407704c5dc06e2699 HEAD && echo 'required dev commit is an ancestor of HEAD'
git merge-base HEAD e8ce2b93dd3ef8074d96dee407704c5dc06e2699
echo '== Changed files relative to required dev commit =='
git diff --name-status e8ce2b93dd3ef8074d96dee407704c5dc06e2699...HEAD
echo '== Relevant file outlines =='
ast-grep outline src/server/responses/core.ts --items all --match 'service|tier|request|log' || true
ast-grep outline tests/service-tier-capability.test.ts --items all --match 'service|tier|DeepSeek|request|log' || true
echo '== PR diff =='
git diff --unified=80 e8ce2b93dd3ef8074d96dee407704c5dc06e2699...HEAD -- \
src/server/responses/core.ts \
tests/service-tier-capability.test.ts
echo '== Service-tier and request-log references =='
rg -n -C 5 --glob '*.ts' \
'(requestedServiceTier|requestedSpeed|service_tier|serviceTier|applyServiceTier|supportsServiceTier)' \
src/server/responses/core.ts tests/service-tier-capability.test.tsLength of output: 57200 🏁 Script executed: #!/bin/bash
set -euo pipefail
completed="eb5fdfbea39194a9d64a5e0a4eafa1b0e12bdac8"
head="HEAD"
echo '== SHA existence and relationship =='
git rev-parse "$completed"
git rev-parse "$head"
if git merge-base --is-ancestor "$completed" "$head"; then
echo 'completed gate SHA is an ancestor of current HEAD'
else
echo 'completed gate SHA is NOT an ancestor of current HEAD'
fi
if git merge-base --is-ancestor "$head" "$completed"; then
echo 'current HEAD is an ancestor of completed gate SHA'
else
echo 'current HEAD is NOT an ancestor of completed gate SHA'
fi
echo '== Commits after completed gate SHA =='
git log --oneline --decorate "$completed..$head"
echo '== Files changed after completed gate SHA =='
git diff --name-status "$completed..$head"
echo '== Current PR metadata =='
gh pr view 1368 --json number,url,headRefName,headRefOid,baseRefName,baseRefOid,isDraft,mergeable,reviewDecision,statusCheckRollupLength of output: 3680
The gate completed at GitHub also reports that PR Do not mark this PR ready from the completed gate result. Run the exact-head checks for You are interacting with an AI system. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/responses/core.ts (1)
1714-1731: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCover Claude main-auth stripping on noncanonical forward routes.
stripClaudeMainAuthForNoncanonicalForwardis only set fromsrc/server/claude-messages.ts:735and tested intests/service-tier-capability.test.ts, whose cases do not exercise the branch. Add a flat Bun test that injects Claude-originauthorization/chatgpt-account-id, routes through a noncanonicalopenai-responsesforward provider withauthMode: "forward", and assertsselectedForwardHeadersno longer contains those internal credentials and Codex runtime fields are removed. Also include a canonical forward route case that keeps its normal authentication path unchanged in the same regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/responses/core.ts` around lines 1714 - 1731, Add flat Bun regression coverage for the response flow that sets stripClaudeMainAuthForNoncanonicalForward, using Claude-origin authorization and chatgpt-account-id headers with a noncanonical openai-responses forward provider in authMode "forward"; assert selectedForwardHeaders removes both credentials and route/adapter Codex runtime fields are cleared. Include a canonical forward-provider case in the same test that preserves the existing authentication path.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/server/responses/core.ts`:
- Around line 1714-1731: Add flat Bun regression coverage for the response flow
that sets stripClaudeMainAuthForNoncanonicalForward, using Claude-origin
authorization and chatgpt-account-id headers with a noncanonical
openai-responses forward provider in authMode "forward"; assert
selectedForwardHeaders removes both credentials and route/adapter Codex runtime
fields are cleared. Include a canonical forward-provider case in the same test
that preserves the existing authentication path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9865b677-6b1f-4bc2-bff6-4d1ac10246fc
📒 Files selected for processing (2)
src/server/responses/core.tstests/service-tier-capability.test.ts
aead7d6 to
c444365
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
required_dev="e8ce2b93dd3ef8074d96dee407704c5dc06e2699"
gate_head="c444365786b0656ea7bfb30dddad99a39ad22bd9"
echo '== Local head and ancestry =='
current_head="$(git rev-parse HEAD)"
printf 'current HEAD: %s\n' "$current_head"
printf 'gate head: %s\n' "$gate_head"
if git merge-base --is-ancestor "$gate_head" HEAD; then
echo 'gate head is an ancestor of current HEAD'
else
echo 'gate head is NOT an ancestor of current HEAD'
fi
if git merge-base --is-ancestor "$required_dev" HEAD; then
echo 'required dev commit is an ancestor of current HEAD'
else
echo 'required dev commit is NOT an ancestor of current HEAD'
fi
printf 'merge-base(required dev, HEAD): %s\n' "$(git merge-base "$required_dev" HEAD)"
echo '== Commits and files after gate head =='
git log --oneline "${gate_head}..HEAD" || true
git diff --name-status "${gate_head}..HEAD" || true
echo '== GitHub PR state =='
gh pr view 1368 --json number,url,headRefOid,baseRefOid,baseRefName,isDraft,mergeable,reviewDecision,labels,statusCheckRollupLength of output: 10067
The required The PR is not a draft. The PR targets Do not treat the PR as fully ready yet. GitHub still reports these jobs as
The You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
pr=1368
required_dev='e8ce2b93dd3ef8074d96dee407704c5dc06e2699'
gate_sha='c444365786b0656ea7bfb30dddad99a39ad22bd9'
echo '== GitHub PR metadata =='
gh pr view "$pr" --json \
number,url,isDraft,headRefName,headRefOid,baseRefName,baseRefOid,mergeable,reviewDecision,labels,statusCheckRollup \
--jq '{
number, url, isDraft, headRefName, headRefOid, baseRefName, baseRefOid,
mergeable, reviewDecision,
labels: [.labels[].name],
checks: [.statusCheckRollup[] | {
name, status, conclusion, workflowName,
headSha: (.detailsUrl // "")
}]
}'
echo
echo '== Local commit relationships =='
head="$(git rev-parse HEAD)"
printf 'local HEAD: %s\n' "$head"
printf 'gate SHA: %s\n' "$gate_sha"
printf 'required dev SHA: %s\n' "$required_dev"
for pair in "gate_to_head:$gate_sha:$head" "dev_to_head:$required_dev:$head"; do
IFS=: read -r name ancestor descendant <<<"$pair"
if git merge-base --is-ancestor "$ancestor" "$descendant"; then
printf '%s: ancestor\n' "$name"
else
printf '%s: not-ancestor\n' "$name"
fi
done
echo
echo '== Commits after gate SHA, if comparable =='
if git merge-base --is-ancestor "$gate_sha" "$head"; then
git log --oneline "$gate_sha..$head"
elif git merge-base --is-ancestor "$head" "$gate_sha"; then
git log --oneline "$head..$gate_sha"
else
echo 'Gate SHA and local HEAD have diverged.'
fiLength of output: 7536
The required However, the exact-head CI checks are still running. These jobs are
Do not mark the PR as fully ready yet. Wait for these exact-head checks to complete successfully. You are interacting with an AI system. |
|
Thanks @luvs01 — merged. 🙌 This is a useful correctness fix: when a provider strips unsupported |
2 participants
Summary
requestedServiceTierand its speed label after the final provider capability gate strips an unsupportedservice_tier.The request log captured caller tier metadata before final route normalization. DeepSeek and Volcengine correctly removed the unsupported wire field later, but the earlier log metadata still described the request as Fast/priority.
Verification
bun test tests/service-tier-capability.test.ts— 13 passed.bun run typecheck— passed.bun run privacy:scan— passed.git diff --check— passed.dev.Checklist
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.
Summary by CodeRabbit