Conversation
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe initial passthrough send no longer enables replay-safe retries for OpenCode Go destinations. Ambiguous pre-header resets now use the fail-closed policy applied to other destinations. Tests verify the refusal response and one physical send. ChangesPassthrough retry policy
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix 🚥 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. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
리뷰 · 우선순위 36 / 80이 PR은 OpenCode Go로 요청을 처음 보낼 때, 답이 오기 전에 연결이 끊기면 같은 요청을 자동으로 다시 보내던 예외를 없앱니다. 이틀 전 병합된 #5223이 그 예외를 넣었습니다. OpenCode Go가 추론 요청을 붙잡았다가 끊으면, 프록시는 "이미 처리됐을 수 있으니 다시 보내지 않는다"는 429를 돌려줬습니다. 한 사례는 18초 대기 뒤 한 번만 보내고, 사용자 목표가 이번 변경은 그 판단을 뒤집습니다. 라인 메인테이너의 판단이 필요한 지점 운영자가 재전송을 켜지 않으면, #5223이 막으려던 끊김이 다시 납니다. OpenCode Go가 답을 주기 전에 연결을 닫으면 자동 재시도가 없고, 클라이언트는 거절 429를 받습니다. 그 대가로, 이미 처리된 추론을 한 번 더 보내 할당량을 쓰는 일은 기본값에서 막힙니다. 기본을 거절로 둘지, 예전처럼 처음 한 번만 다시 보낼지는 여기서 정해야 합니다. 너의 추천 추론 요청은 같은 본문을 두 번 보내면 제공자 일이 두 번 될 수 있어서, 무조건 다시 보내는 예외를 빼는 쪽을 추천합니다. 베이스는 이 댓글은 grok-bot이 작성했습니다 |
abhisheksharma2411
left a comment
abhisheksharma2411
left a comment
There was a problem hiding this comment.
Correctness over availability is the right default here — an ambiguous pre-header reset on a non-idempotent inference POST can duplicate provider-side work and burn quota, and no amount of "the destination is flaky" justifies replaying it. I traced the mechanism rather than assuming it, and the removal does land where you say:
with the option gone, opts.replaySafe is undefined, so upstream-retry.ts:622's if (opts.replaySafe === true) falls to the else — which returns replayRefusalResponse() unless claimAmbiguousResend() grants the operator allowance. Fail-closed by default, operator policy still reachable, and the evidence-returning path (rather than throwing) is preserved so an outer catch can't turn it back into a replayable 502. That's exactly the shape the comment claims.
Also checked that this doesn't strand anything:
replaySafestays in use where it's genuinely true — the vision, web-search and image-bridge sidecars (src/vision/*,src/web-search/*,src/images/loop.ts:623). Those are idempotent side-calls, so the option keeps earning its place.isOpenCodeGoDestinationis still referenced atsrc/providers/key-failover.ts:595, so dropping the import here leaves no dead export.
One change I'd ask for: the new assertion is stronger than the invariant.
expect(occurrences(passthroughDispatchPacked, "replaySafe:")).toBe(0);dense() strips whitespace, so this forbids the literal substring replaySafe: anywhere in the module — including replaySafe: false. That's the explicit fail-closed declaration: the safest thing a future author could write, and the guard rejects it. Someone hardening this leg by stating the property outright instead of relying on undefined gets a red test with a message that reads as if they'd reintroduced the replay.
The invariant you actually want is "no inference leg is declared replay-safe", so I'd assert against the affirmative forms:
expect(occurrences(passthroughDispatchPacked, "replaySafe:true")).toBe(0);
expect(occurrences(passthroughDispatchPacked, "replaySafe:isOpenCodeGoDestination")).toBe(0);Same protection against the regression this PR is guarding, without outlawing the defensive spelling.
Related, and worth a sentence somewhere: the guard is file-scoped. If the opt-in ever moves behind a helper — a replayPolicyFor(route) that returns true for Go — passthrough-dispatch.ts stays clean and the test keeps passing while the behaviour comes back. Not a reason to change the test, just worth knowing what it does and doesn't cover.
One question rather than a finding. The comment you're removing described a live symptom: Go stalls-then-drops inference sends, surfacing as refused 429s, and the bounded replay was absorbing that. Refusing is the correct answer, but it's a user-visible one — those turns now fail where they previously recovered. Is there a tracking issue for the underlying Go behaviour, and is the operator-scoped ambiguous-resend policy documented anywhere a Go subscriber would find it? Otherwise this trades a silent duplication risk for a visible failure with no signposted remedy, which is still the right trade but a worse experience than it needs to be.
Nothing blocking beyond the assertion.
Per review on lidge-jun#5446: replace the replaySafe source-string count with an execution test that drops the connection before the answer on an opencode.ai/zen/go destination and asserts the 429 upstream_reset_replay_refused with exactly one send.
|
Review feedback applied on |
abhisheksharma2411
left a comment
abhisheksharma2411
left a comment
There was a problem hiding this comment.
This is better than what I suggested. I proposed tightening the source assertion to the affirmative forms (replaySafe:true, replaySafe:isOpenCodeGoDestination); you replaced it with an execution test instead, which closes both concerns rather than one:
const response = await handleResponses(responsesRequest("go/model-go"), config, logCtx);
expect(response.status).toBe(429);
expect((await response.json()).error.code).toBe("upstream_reset_replay_refused");
expect(sends).toBe(1);sends === 1 is the assertion that matters — it pins "exactly one send reached the wire", which is the actual property, not a spelling of it.
I checked the file-scoping concern I raised rather than assuming the new test covers it. It does. Reintroducing the opt-in inline and under its own name on the initial send:
claimAmbiguousResend: claimPreHeaderResend,
replaySafe: true,fails two tests in responses-send-budget-counts.test.ts (no replay or target hop after an ambiguous reset, both the combo=false and combo=true cases). A source-text guard in passthrough-dispatch.ts would only have caught it while the opt-in lived in that file; this catches it wherever it comes back from, which was the gap I flagged.
Verified at 92b74ecb: bun test --isolate tests/responses/responses-send-budget-counts.test.ts tests/responses/responses-passthrough-transient-policy.test.ts → 26 pass / 0 fail, matching your number.
The pointer comment left behind in the old location is a nice touch — it sends the next reader to the test that actually holds the contract instead of leaving a gap where an assertion used to be.
Nothing outstanding from me. The open question about the underlying Go stalls-then-drops behaviour is for the maintainers, not a blocker on this.
3 participants
Motivation
Description
replaySafe: isOpenCodeGoDestination(route.provider)opt-in from the passthrough initial send so ambiguous resets continue to produce the fail-closed refusal or follow the operator-scoped ambiguous-resend policy.replaySafe.Testing
bun test tests/responses/responses-passthrough-transient-policy.test.ts: 10 tests pass.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
Bug Fixes
Tests