fix: preserve Claude Go affinity across combo selection and failover - #4050
fix: preserve Claude Go affinity across combo selection and failover#4050david-wang-0 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughClaude Messages now derives Go session affinity separately from replay headers. Responses handling passes this identity to final Go transport resolution. Tests cover identity precedence, combo strategies, failover, and non-Go target isolation. Provider documentation describes the fallback behavior. ChangesClaude Go session affinity
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The affinity behavior appears well targeted, but the new regression suite cannot run because of a duplicate declaration. Fix the test parse failure and rerun the checks before merging. Sequence Diagram(s)sequenceDiagram
participant ClaudeMessages
participant Responses
participant OpenCodeGoTransport
ClaudeMessages->>ClaudeMessages: Derive session lane
ClaudeMessages->>Responses: Pass claudeGoAffinity
Responses->>OpenCodeGoTransport: Resolve final Go transport
OpenCodeGoTransport-->>Responses: Apply session affinity
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 unsupported.)
✨ 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. |
⏳ DRAFT
What to do
Review readiness checklist
1/4 boxes ticked. This PR stays in draft until every box above is ticked. |
Carry validated Claude affinity privately through combo replay and consume it only at the final canonical Go transport. Preserve explicit identity and operator precedence without leaking Go-only headers to other destinations. Addresses the late review on lidge-jun#3961. Adds deterministic random and failover regressions across both Go wires. Co-authored-by: GPT-6 Astra <noreply@openai.com>
리뷰 · 우선순위 72 / 80이 PR은 #3961 늦은 리뷰 후속입니다. Claude 패치 요지는 affinity를 대역 외 옵션으로 옮기는 것입니다. Claude 쪽에서 쓸 수 있는 레인( 테스트 라인 src/server/claude-messages.ts · claudeGoSessionLane - 명시 레인 → 클라이언트 Go 헤더 → metadata 합성 순서가 문서·테스트와 맞습니다. shared system 키는 metadataGoLane에 안 들어갑니다. 라인 src/server/responses/core.ts · resolveOpenCodeGoTransport 호출 - 경로 draft + rebase - tip이 #4049까지 전진했습니다. undraft 전에 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
3e6a831 to
ee9a86c
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Rebased and revalidated at ee9a86c on dev 7dc7dc9. The updated description records the new bounded test results and exact reproduction pins; cfff026 landed afterward and is not included in those local results, so the latest-dev checkbox remains unchecked. The empty claudeGoAffinity object is intentional negative information, not a missing fallback value. Claude snapshots Go eligibility before the existing native preflight may synthesize a session_id header. When metadata is ineligible and no usable explicit identity exists, an empty snapshot must suppress fallback to that later synthesized header. Testing sessionLane != null instead would re-enable that fallback. The regression matrix covers invalid metadata and shared-system-only identity with an openai-responses preliminary target followed by Go on both wires. Calls that omit claudeGoAffinity entirely retain the existing header-based behavior. A full CodeRabbit review has been requested for this pushed head. The PR remains draft pending remaining readiness work. |
|
@coderabbitai Your completed summary reports no actionable comments, but its merge-risk paragraph says the regression suite has a duplicate declaration and cannot parse. Please identify the exact identifier and line, or correct that paragraph. I verified that the published test blob and the executed local blob are identical: 4171ef00a85a0a115d551ce551681dc37056ec32 at head ee9a86c. A fresh run of Please also identify the functions behind the advisory docstring-coverage warning; the report gives only an aggregate percentage, so it does not currently identify a concrete missing contract to assess. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== HEAD =='
git rev-parse HEAD
git status --short
echo '== Test file outline =='
ast-grep outline tests/providers/opencode-go-session-header.test.ts --items all || true
echo '== Import and top-level declarations with line numbers =='
rg -n -C 2 --type ts \
'^\s*import\b|^\s*(const|let|var|function|class|interface|type|enum)\s+' \
tests/providers/opencode-go-session-header.test.ts || true
echo '== Repeated const/let/function/class/type/interface/enum names in that file =='
python3 - <<'PY'
from collections import defaultdict
import re
p = "tests/providers/opencode-go-session-header.test.ts"
decl = re.compile(r'^\s*(?:export\s+)?(?:async\s+)?(?:const|let|var|function|class|interface|type|enum)\s+([A-Za-z_$][\w$]*)\b')
seen = defaultdict(list)
with open(p, encoding="utf-8") as f:
for n, line in enumerate(f, 1):
m = decl.match(line)
if m:
seen[m.group(1)].append(n)
for name, lines in sorted(seen.items()):
if len(lines) > 1:
print(f"{name}: {', '.join(map(str, lines))}")
PY
echo '== Documentation coverage references in repository configuration and scripts =='
rg -n -i -C 3 \
'(docstring|documentation)[ _-]?(coverage|cover)|doc-coverage|typedoc|api[ _-]?extractor' \
. \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' -g '!coverage/**' \
|| true
echo '== Package scripts relevant to docs/coverage =='
if [ -f package.json ]; then
rg -n -C 2 '"[^"]*(doc|coverage)[^"]*"\s*:' package.json || true
fiLength of output: 24525 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Changed test range =='
sed -n '115,225p' tests/providers/opencode-go-session-header.test.ts | nl -ba -v 115
echo '== PR diff for the test file =='
git diff "$(git merge-base HEAD origin/dev)" HEAD -- tests/providers/opencode-go-session-header.test.ts || true
echo '== TypeScript configuration candidates =='
fd -HI '^tsconfig.*\.json$|^biome\.jsonc?$|^eslint\.config\.' . -E node_modules -E dist -E build \
| while IFS= read -r f; do
echo "--- $f"
sed -n '1,240p' "$f"
done
echo '== PR `#4050` comments and reviews containing parse or docstring terms =='
gh api --paginate repos/lidge-jun/opencodex/issues/4050/comments \
--jq '.[] | select((.body | test("duplicate|parse|docstring|coverage"; "i"))) | {user: .user.login, created_at, body}' \
|| true
gh api --paginate repos/lidge-jun/opencodex/pulls/4050/comments \
--jq '.[] | select((.body | test("duplicate|parse|docstring|coverage"; "i"))) | {user: .user.login, path, line, original_line, body}' \
|| true
gh api repos/lidge-jun/opencodex/pulls/4050/reviews \
--jq '.[] | {user: .user.login, state, submitted_at, body}' \
|| trueLength of output: 1526 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Changed test range =='
awk 'NR >= 115 && NR <= 225 { printf "%d:%s\n", NR, $0 }' \
tests/providers/opencode-go-session-header.test.ts
echo '== PR diff for the test file =='
base="$(git merge-base HEAD origin/dev 2>/dev/null || true)"
if [ -n "$base" ]; then
git diff "$base" HEAD -- tests/providers/opencode-go-session-header.test.ts
else
echo 'Could not determine merge base with origin/dev.'
fi
echo '== TypeScript and lint configuration candidates =='
fd -HI '^tsconfig.*\.json$|^biome\.jsonc?$|^eslint\.config\.' . \
-E node_modules -E dist -E build \
| while IFS= read -r f; do
echo "--- $f"
sed -n '1,240p' "$f"
done
echo '== Package scripts =='
sed -n '1,130p' package.json
echo '== PR `#4050` comments and reviews with relevant terms =='
gh api --paginate repos/lidge-jun/opencodex/issues/4050/comments \
--jq '.[] | select((.body | test("duplicate|parse|docstring|coverage"; "i"))) | {user: .user.login, created_at, body}' \
|| true
gh api --paginate repos/lidge-jun/opencodex/pulls/4050/comments \
--jq '.[] | select((.body | test("duplicate|parse|docstring|coverage"; "i"))) | {user: .user.login, path, line, original_line, body}' \
|| true
gh api repos/lidge-jun/opencodex/pulls/4050/reviews \
--jq '.[] | {user: .user.login, state, submitted_at, body}' \
|| trueLength of output: 25148 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='cfff026d58c4d6e894214cbec5248d70d759ba7f'
echo '== Name and function-level diff =='
git diff --unified=20 "$base" HEAD -- \
src/server/claude-messages.ts \
src/server/responses/core.ts \
tests/providers/opencode-go-session-header.test.ts
echo '== Source outlines =='
ast-grep outline src/server/claude-messages.ts --items all || true
ast-grep outline src/server/responses/core.ts --items all || true
ast-grep outline tests/providers/opencode-go-session-header.test.ts --items all || true
echo '== Function declarations near changed lines =='
for f in src/server/claude-messages.ts src/server/responses/core.ts tests/providers/opencode-go-session-header.test.ts; do
echo "--- $f"
git diff --unified=0 "$base" HEAD -- "$f" \
| awk '
/^@@/ { print }
/^[+-][^+-]/ { print }
'
doneLength of output: 57524
The merge-risk paragraph is incorrect. It should not state that the regression suite cannot parse. Evidence at
The corrected paragraph is:
The advisory Docstring Coverage result is also not actionable as published. It reports only an aggregate: three analyzed functions, 66.67% coverage, and one unsupported item. It does not identify the functions, source lines, or the unsupported syntax. The repository has no docstring-coverage script or configuration that can reproduce its function-level mapping. The only plausible changed production function without a preceding JSDoc block is Therefore, I withdraw both implications that there is a known test parse failure and that the aggregate docstring percentage identifies a concrete missing contract. 🐇 You are interacting with an AI system. |
2 participants
Summary
Verification
bun run typecheck: passed.bun run privacy:scan: passed.cd docs-site && bun run build: passed (425 pages).git diff --checkand staged privacy/path review: passed.bun run prepushwas attempted twice, but its monolithic Bun test worker stalled; neither attempt is claimed green. The second attempt had no assertion failures before interruption. This matches the symptom class tracked in [Bug] CI: Linux test shards intermittently hang ~15 minutes and are killed, leaving an orphan bun process #1302, not a proven diagnosis of its underlying runtime cause.ee9a86c0950763c9fe064abe22c6219e039c016f, based ondevat7dc7dc99e65268bc8764e19840952256b030bce9. Rebase was clean; range-diff confirms the patch is unchanged. The checks and reproduction below were repeated on this head. Subsequentdevcommitcfff026d58c4d6e894214cbec5248d70d759ba7flanded after validation: the branch is one commit behind, within the repository's ten-commit readiness allowance. These local results do not claim to test that later commit.env -u OPENCODEX_CODEX_SHIM_AUTO_RESTORE OPENCODEX_BUN_PATH="$PWD/node_modules/bun/bin/bun.exe" bash scripts/ci/run-bun-test-batches.sh 1/1passed: 21,882 pass, 43 skip, 0 fail, 1,142 files in 96 batches, no runtime retries.env -u OPENCODEX_CODEX_SHIM_AUTO_RESTORE bun run test tests/storage/api-storage-policy-already-running.test.ts tests/storage/api-storage-policy-mutation-busy.test.ts tests/storage/api-storage-policy-put-race.test.ts tests/storage/api-storage-policy-run.test.ts tests/storage/api-storage-policy.test.ts tests/storage/api-storage.test.tspassed: 9 pass, 0 fail.env -u OPENCODEX_CODEX_SHIM_AUTO_RESTORE bun run test tests/server/api-usage.test.ts tests/ci-workflows/release-version-line.test.tspassed: 36 pass, 0 fail.OPENCODEX_CODEX_SHIM_AUTO_RESTORE=0; this conflicts with the shim auto-repair test. The test command unsets it without changing any persistent setting. The shim file then passed 9/9. An initial full run with that inherited flag failed the shim assertion and was interrupted after its worker stalled; it is not counted as a successful gate.Deterministic reproduction (no API key)
Requires Git, Node.js, and Bun. All upstream fetches in the selected tests are mocked. The script creates a disposable checkout and keeps its logs; it does not start a proxy or call Go. It runs exactly the same new regression tests against the unpatched base and the fixed production files.
The selected cases force the preliminary route to a non-Go Chat provider and actual random dispatch to Go, on both Go wire protocols. Recorded standalone output (setup and stack traces omitted):
Checklist
Co-authored-by: GPT-6 Astra noreply@openai.com
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
Improvements
Documentation