Skip to content

fix(empty-completion): keep a discarded attempt's reasoning out of the retry's item - #7331

Closed
vadymhimself wants to merge 1 commit into
lidge-jun:devfrom
vadymhimself:fix/empty-completion-retry-boundary
Closed

vadymhimself wants to merge 1 commit into
lidge-jun:devfrom
vadymhimself:fix/empty-completion-retry-boundary

Conversation

@vadymhimself

@vadymhimself vadymhimself commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • The empty-completion guard holds pre-content events (reasoning) across its identical-turn retry and releases them together with the retry's output. Nothing separates the two attempts, so an unsigned reasoning delta from the retry is appended to the discarded attempt's still-open reasoning item: the client receives one reasoning item whose summary runs both attempts together ("discardedretry"). A signed Anthropic block is unaffected only because its pending signature closes the item first.
  • Each retry point (empty done, replay-safe pre-output error, pre-output EOF) now queues one assistant_boundary after the held events, only when something is held. The bridge (bridge/sse.ts, bridge/response-json.ts) and the direct encoders (protocols/encoders/adapter-events.ts) already treat that event as "close the open items"; isContentEvent already classifies it as non-content, so retry decisions are unchanged.
  • Only src/server/responses/empty-completion-guard.ts changes (+5).

Verification

  • New test the discarded attempt's reasoning stays out of the retry's reasoning item in tests/responses/empty-completion-guard.test.ts runs the guard through bridgeToResponsesSSE. Before the src change: 1 fail (one reasoning item "discardedretry" instead of ["discarded", "retry"]). Three existing event-sequence expectations in the same file now include the boundary; before the change they failed for exactly that missing event. After: bun test tests/responses/empty-completion-guard.test.ts tests/responses/empty-completion-hardening.test.ts tests/adapters/google/google-vertex-stream.test.ts -> 52 pass, 0 fail.
  • bun x tsc --noEmit -> exit 0. bun scripts/file-size-ratchet.ts -> passed.
  • bun run test:changed (Bun 1.4.3 via OCX_TEST_RUNNER_BUN) -> 16140 pass, 34 skip, 5 fail across 656 files, on a heavily loaded machine (two of the failures ran for 139-140 s). Re-run alone, claude-desktop-first-party-guards, settings-stream-mode and server-auth-scoped-quota pass (53 pass, 0 fail). The remaining failure, tests/cli/cli-connect-readiness.test.ts "a rejected preferred runtime falls back without rewriting the saved selection", also fails on an unmodified upstream/dev 8640c2f checkout on this machine (local-environment dependent) and is unrelated to the guard.
  • Full bun run test was not run locally: test:changed already covered the changed module's import graph; the remainder is left to CI.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (No docs change needed: stream item framing only.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (No secrets or auth behaviour touched.)

Source: carried from the vadymhimself fork build (empty-completion-guard.ts hunk of commit 8fb1bbb), re-authored smaller: the fork's bounded-buffer fallback and its Anthropic-lane anthropic_server_block / anthropicServerToolUse changes are not carried, and the boundary now also covers upstream's newer replay-safe error retry.

🤖 Generated with Claude Code

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • Required local validation passed; commands, results, and any full-suite exception are documented.

  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes
    • Reasoning from a retried response is now kept separate from reasoning generated by the discarded attempt, preventing the two from being combined in the response.
    • This separation is maintained when a retry follows a heartbeat or an attempt that ends without producing a final response.

…the retry's

The guard holds pre-content events across an empty-completion retry and
releases them with the retry's output. With nothing between the two attempts,
an unsigned reasoning delta from the retry is appended to the discarded
attempt's still-open reasoning item, so the client sees one reasoning item
whose text runs both attempts together ("discardedretry").

Each retry point now queues an assistant_boundary after the held events (only
when something is held), which the bridge and the direct encoders already
treat as "close the open items".

Co-Authored-By: Claude Code <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 18, 2026
@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ✅ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

This pull request has been marked Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

@github-actions
github-actions Bot marked this pull request as draft October 18, 2026 18:22
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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: a5450be9-41a3-4ab6-88ef-fa77c6a77f75
📥 Commits

Reviewing files that changed from the base of the PR and between 8640c2f and 911ee16.

📒 Files selected for processing: 2
  • src/server/responses/empty-completion-guard.ts
  • tests/responses/empty-completion-guard.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

The empty-completion guard now adds an assistant_boundary to buffered events before retrying after an empty successful completion, a replay-safe pre-output error, or pre-output EOF. Tests verify boundary placement and separate Responses reasoning items for discarded and retry attempts.

Changes: Retry Boundary Handling

Layer / File(s) Summary
Insert retry boundaries and verify output
src/server/responses/empty-completion-guard.ts, tests/responses/empty-completion-guard.test.ts
The guard appends an assistant_boundary to nonempty held events unless the last event is already a boundary. It adds the boundary before retries in three pre-output cases. Tests check boundary placement, including after a heartbeat and EOF, and verify that discarded-attempt and retry reasoning appear as separate Responses items.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 911ee

The retry boundary separates reasoning from discarded and retry attempts without changing retry eligibility. No actionable merge risk remains in the reviewed changes.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
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.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preventing reasoning from a discarded empty-completion attempt from being included in the retry's item.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

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

@github-actions
github-actions Bot marked this pull request as ready for review October 18, 2026 18:48
@Ingwannu

Copy link
Copy Markdown
Owner

Intake/scoped review at 911ee16f83953ffe6c33f501d7952d4b7279495a.

The retry boundary separates retained reasoning from the discarded attempt from the next attempt's reasoning item across clean-empty, retryable-terminal and pre-content EOF paths. The newly added reasoning-item assertion is more meaningful than only checking event strings; retain usage merging and the no-post-output-replay controls.

Current-head functional workflows are awaiting fork approval; that is pending execution, not a passing test result. No local suite was run for this branch in this intake. No workflow approval, new scan, live configuration change or merge was performed. Required review/readiness gates remain in force.

@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev as part of the replay/lifecycle bundle #7388 (squash c120eea), with your commits and Co-authored-by credit preserved. Closing as absorbed. Thank you!

@lidge-jun lidge-jun closed this Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants