fix(responses): refuse ambiguous OpenCode Go resets

[luvs01]

Motivation

• Prevent ambiguous pre-header connection resets from causing automatic retries of non-idempotent OpenCode Go inference POSTs, which can duplicate provider-side work and consume quota.

Description

• Remove the destination-scoped 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.
• Drop the now-unused import and update the passthrough test to assert that no inference leg is declared replaySafe.

Testing

• bun test tests/responses/responses-passthrough-transient-policy.test.ts: 10 tests pass.

Summary by CodeRabbit

• Bug Fixes

  • Initial passthrough requests now consistently refuse ambiguous connection resets before a response begins, including requests sent to OpenCode Go destinations.
  • Retry behavior remains bounded by the configured attempt limit, helping prevent duplicate upstream sends.
  • Failed requests in this scenario return the appropriate refusal response instead of being replayed.

• Tests

  • Updated coverage to verify consistent handling across destinations and ensure only one physical send occurs before refusal.

[coderabbitai]

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 88093d22-6189-406c-819a-51983a1abe4a

📥 Commits

Reviewing files that changed from the base of the PR and between 92b74ec and faa2e53.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ff0d85d3-c985-4392-a6c1-c145cc55c6ec

📥 Commits

Reviewing files that changed from the base of the PR and between 62ac159 and 92b74ec.

📒 Files selected for processing (2)

• tests/responses/responses-passthrough-transient-policy.test.ts
• tests/responses/responses-send-budget-counts.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

The 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.

Changes

Passthrough retry policy

Layer / File(s)	Summary
Initial passthrough send policy src/server/responses/passthrough-dispatch.ts, tests/responses/responses-passthrough-transient-policy.test.ts, tests/responses/responses-send-budget-counts.test.ts	The dispatcher removes the OpenCode Go check and replaySafe option from the initial fetchWithTransientRetry call. The tests replace the source-text assertion with an execution test that expects HTTP 429, upstream_reset_replay_refused, and exactly one physical send after ECONNRESET.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: OpenCode Go now refuses ambiguous resets instead of automatically replaying them.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

✅ READY

• all PR quality gates passed.

Hygiene

✅ Deterministic PR hygiene checks passed.

[lidge-jun]

리뷰 · 우선순위 36 / 80

이 PR은 OpenCode Go로 요청을 처음 보낼 때, 답이 오기 전에 연결이 끊기면 같은 요청을 자동으로 다시 보내던 예외를 없앱니다.

이틀 전 병합된 #5223이 그 예외를 넣었습니다. OpenCode Go가 추론 요청을 붙잡았다가 끊으면, 프록시는 "이미 처리됐을 수 있으니 다시 보내지 않는다"는 429를 돌려줬습니다. 한 사례는 18초 대기 뒤 한 번만 보내고, 사용자 목표가 exceeded retry limit으로 끊겼습니다. #5223은 처음 보내는 그 한 번에만, 횟수 한도 안에서 다시 보내게 했습니다. 이 목적지는 구독 추론만 받아서, 다시 보내도 계산이 두 번 될 뿐이라는 이유였습니다.

이번 변경은 그 판단을 뒤집습니다. passthrough-dispatch.ts에서 replaySafe: isOpenCodeGoDestination(route.provider)와 안 쓰게 된 import를 뺐습니다. 답이 오기 전에 연결이 끊기면 다른 목적지와 같습니다. 운영자가 재전송을 켜 두었고 본문을 다시 보내도 되는 경우에만 한 번 더 보내고, 아니면 거절 429(upstream_reset_replay_refused)입니다. 답을 받은 뒤 다시 보내는 구간은 원래 이 예외가 없었습니다. 테스트는 이 파일 안에 replaySafe: 글자가 없는지만 보도록 바뀌었습니다.

라인 tests/responses/responses-passthrough-transient-policy.test.ts:187 - 테스트는 소스에서 replaySafe:가 0번인지만 셉니다. OpenCode Go로 답이 오기 전에 끊겼을 때 거절이 나오는지는 실행하지 않습니다. 그 글자 없이 같은 옵션을 넘기면 테스트는 통과합니다.

메인테이너의 판단이 필요한 지점

운영자가 재전송을 켜지 않으면, #5223이 막으려던 끊김이 다시 납니다. OpenCode Go가 답을 주기 전에 연결을 닫으면 자동 재시도가 없고, 클라이언트는 거절 429를 받습니다. 그 대가로, 이미 처리된 추론을 한 번 더 보내 할당량을 쓰는 일은 기본값에서 막힙니다. 기본을 거절로 둘지, 예전처럼 처음 한 번만 다시 보낼지는 여기서 정해야 합니다.

너의 추천

추론 요청은 같은 본문을 두 번 보내면 제공자 일이 두 번 될 수 있어서, 무조건 다시 보내는 예외를 빼는 쪽을 추천합니다. 베이스는 dev입니다. types.ts/config.ts 분할과 겹치지 않고, 이미 병합된 #5223의 반대라서 닫을 중복 PR은 없습니다. 병합 전에 "거절 429가 기본이어도 된다"만 확인하면 됩니다. 테스트는 글자 수 대신, 그 끊김이 거절로 끝나는 실행 한 건이 있으면 나중에 옵션이 다른 이름으로 돌아와도 잡힙니다.

이 댓글은 grok-bot이 작성했습니다

[abhisheksharma2411]

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:

• replaySafe stays 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.
• isOpenCodeGoDestination is still referenced at src/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.

[luvs01]

Review feedback applied on 92b74ecb: replaced the replaySafe: source-string count with an execution test - an OpenCode Go destination refuses an ambiguous pre-answer reset instead of replaying in responses-send-budget-counts.test.ts; it asserts the 429 upstream_reset_replay_refused with exactly one send on the wire. Tests: 26 pass across both files.

[abhisheksharma2411]

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.

[luvs01]

Consolidated into #5553 as a single related-function aggregate.

Source head: faa2e5336d0b1fdd2ccc317265df09845526f6a7. Replacement head: 67c4f579e4fb5201dcd36be225ee54306d78e63e.

Implementation 62ac159 and its execution-test follow-up 92b74ec match carried b8f9a45 and 808dd85 by stable patch ID with source authors retained. The dev merge adds no remerge-resolution delta. The final aggregate preserves all 27 prepared contribution files byte-for-byte after merging current dev. Its 604 distinct focused tests include one-send OpenCode Go refusal plus the combined fallback/account no-replay cases. Default refusal availability remains an explicit pre-merge policy question on #5553; full-suite completion, exact-head hosted CI and security review remain pending. No merge or release is claimed.

Closing this duplicate standalone review entry as part of the requested consolidation after verifying coverage. This is not a merge or release claim; remaining integration checks and reviews are tracked on the replacement. Original branches are retained.