Skip to content

fix(streaming): repair sparse Responses snapshots - #894

Closed
0xWinner98 wants to merge 4 commits into
lidge-jun:devfrom
0xWinner98:agent/responses-snapshot-repair
Closed

fix(streaming): repair sparse Responses snapshots#894
0xWinner98 wants to merge 4 commits into
lidge-jun:devfrom
0xWinner98:agent/responses-snapshot-repair

Conversation

@0xWinner98

@0xWinner98 0xWinner98 commented Aug 2, 2026

Copy link
Copy Markdown

Summary

  • add a disabled-by-default provider option, responsesSnapshotRepair;
  • backfill missing canonical fields in sparse Responses lifecycle snapshots, output items, nested reasoning summaries, content parts, and text events;
  • reconstruct a missing terminal output only from bounded, contiguous response.output_item.done indexes, apply the same integrity guard to raw continuation persistence, and preserve the original request's tool metadata;
  • repair client-facing SSE and non-streaming JSON while keeping raw upstream bytes authoritative for inspection and persistence;
  • account retained reconstruction bytes against the translator budget and dispose all rewrite state on every relay exit;
  • allow an explicit Darwin eager-relay path to apply the rewrite inline after the single reader.

Why

Some Responses-compatible gateways return successful SSE streams whose lifecycle objects omit fields required by current Codex clients. OpenCodex relays those snapshots byte-for-byte, so the upstream and proxy can finish with HTTP 200 while the client fails to commit or render a final assistant message.

The repair is provider-local and opt-in. Providers that already emit canonical Responses payloads retain the existing passthrough behavior.

Root cause

The wire payloads are valid JSON but structurally sparse. The Responses passthrough path had no compatibility layer for missing snapshot fields, and Darwin's explicit eager single-reader relay did not previously admit a client-facing payload rewrite.

The new rewrite only fills absent or structurally invalid fields. It never replaces a valid upstream value. Inspection, terminal accounting, and persistence continue to consume the raw upstream payload; only the client-facing SSE or JSON response is repaired. Reconstruction is bounded by item count, byte count, and the existing translator memory budget.

Scope

This PR intentionally excludes chat-completions reasoning-tag splitting and other provider-specific behavior.

Related issue

Closes #893

Test plan

  • bun test tests/responses-snapshot-repair.test.ts tests/sse-payload-rewrite.test.ts tests/config.test.ts tests/bun-stream-caps.test.ts tests/relay-eager.test.ts tests/passthrough-abort.test.ts — 198 passed, 0 failed
  • bun test tests/sse-inspector-bounds.test.ts tests/responses-snapshot-repair.test.ts tests/responses-state.test.ts tests/relay-eager.test.ts — 155 passed, 0 failed
  • bun run typecheck
  • bun run privacy:scan
  • cd docs-site && bun run build — 206 pages built successfully
  • bun run prepush reached 6,981 passed / 8 skipped; the only 7 failures reproduce unchanged on clean dev because this host resolves reserved/example test hostnames into the benchmark address range

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@0xWinner98, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 25 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 41659531-7259-4eba-b17f-44b5942b7d4f

📥 Commits

Reviewing files that changed from the base of the PR and between 4fcdcd1 and fdbb3b7.

📒 Files selected for processing (5)
  • src/server/relay.ts
  • src/server/responses-snapshot-repair.ts
  • tests/responses-snapshot-repair.test.ts
  • tests/responses-state.test.ts
  • tests/sse-inspector-bounds.test.ts
📝 Walkthrough

Walkthrough

This change adds opt-in Responses snapshot repair for SSE and JSON payloads. It repairs sparse lifecycle fields, composes with existing rewrites, preserves raw inspection data, adds relay cleanup, and enables explicit Darwin eager rewriting.

Changes

Responses snapshot repair

Layer / File(s) Summary
Snapshot repair implementation
src/server/responses-snapshot-repair.ts, tests/responses-snapshot-repair.test.ts
Repairs missing response, output, content, reasoning, status, tool, and logprob fields. Reconstructs completed output items and enforces retention limits.
Rewrite configuration and composition
src/types.ts, src/config.ts, src/server/responses/core.ts, tests/config.test.ts, tests/passthrough-abort.test.ts, docs-site/src/content/docs/*/reference/configuration/providers.md
Adds disabled-by-default provider configuration. Composes snapshot repair with existing rewrites for SSE and JSON paths. Updates provider documentation and examples.
Relay lifecycle and resource cleanup
src/server/sse-payload-rewrite.ts, src/server/relay-eager.ts, tests/relay-eager.test.ts, tests/sse-payload-rewrite.test.ts
Tracks client terminal delivery, handles relay failures, cancels upstream streams, and disposes payload rewrites across termination paths.
Platform relay policy
src/lib/bun-stream-caps.ts, tests/bun-stream-caps.test.ts, devlog/_plan/260731_macos_rss_retention/*
Allows explicit Darwin eager rewriting while retaining tee behavior for Darwin auto mode and unsupported behavior on other platforms.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Upstream
  participant ResponsesCore
  participant Relay
  participant SnapshotRepair
  participant Client
  participant Inspection
  Upstream->>ResponsesCore: Responses SSE or JSON
  ResponsesCore->>Inspection: Raw upstream payload
  ResponsesCore->>Relay: Composed payload rewrite
  Relay->>SnapshotRepair: Client-facing payload
  SnapshotRepair->>Client: Repaired response
  Relay->>Upstream: Cancel on client delivery failure
Loading

Possibly related PRs

Suggested reviewers: lidge-jun, ingwannu

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation satisfies [#893] with opt-in client-only snapshot repair, preserved valid values, rewrite composition, bounded retention, and raw inspection.
Out of Scope Changes check ✅ Passed The relay, Darwin routing, teardown, documentation, and tests directly support the linked issue requirements and do not introduce unrelated scope.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the main change: repairing sparse Responses snapshots during streaming.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions github-actions Bot added the bug Something isn't working label Aug 2, 2026
@0xWinner98

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/bun-stream-caps.test.ts (1)

121-132: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Add a regression test for "darwin + rewrite + auto" staying on tee.

The suite tests "darwin + no rewrite + auto" (Lines 121-123, expects null) and "darwin + rewrite + config-eager" (Lines 125-128, expects the eager decision), but no test covers "darwin + rewrite + auto." This is the exact safety boundary the selectEagerPath change in src/lib/bun-stream-caps.ts (Lines 107-108) is meant to preserve: Darwin must remain on tee for rewrite traffic unless the operator explicitly sets streamMode: "eager-relay".

Without this test, a future change that widens the decision.reason === "config-eager" check in selectEagerPath could silently make Darwin's default (auto) mode use the eager relay for rewrite traffic, changing default behavior for all Darwin operators who enable responsesSnapshotRepair or responsesItemIdRepair.

✅ Proposed additional test
   test("darwin + rewrite + config-eager → eager single-reader relay", () => {
     expect(selectEagerPath("darwin", true, "eager-relay", "1.3.14", null))
       .toEqual({ useEagerRelay: true, reason: "config-eager" });
   });
+
+  test("darwin + rewrite + auto known-bad runtime → tee with no eager decision", () => {
+    expect(selectEagerPath("darwin", true, "auto", "1.3.14", null)).toBeNull();
+  });

Based on path instructions, "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."

🤖 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 `@tests/bun-stream-caps.test.ts` around lines 121 - 132, Add a focused
regression test beside the existing selectEagerPath tests covering Darwin with
rewrite enabled and stream mode "auto"; assert that selectEagerPath returns
null, preserving tee behavior unless "eager-relay" is explicitly configured. Use
the existing test structure and symbol selectEagerPath.

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.

Inline comments:
In `@src/lib/bun-stream-caps.ts`:
- Around line 95-98: Correct the doc comment for selectEagerPath to state that
Windows rewrite traffic is intentionally excluded here because
isWin32EagerRewrite separately forces the eager relay path, avoiding tee and its
JS-pull wrapper for the Bun#32111 crash case. Keep the existing Darwin behavior
and null-return description accurate.

In `@src/server/responses-snapshot-repair.ts`:
- Around line 66-84: Update the tool_choice repair condition in the response
snapshot repair function to backfill any structurally invalid value, not just
null or undefined. Reject non-string and non-object values, and validate object
values include the required type shape before assigning "auto"; preserve valid
tool_choice values and the existing changed/response return behavior.

---

Outside diff comments:
In `@tests/bun-stream-caps.test.ts`:
- Around line 121-132: Add a focused regression test beside the existing
selectEagerPath tests covering Darwin with rewrite enabled and stream mode
"auto"; assert that selectEagerPath returns null, preserving tee behavior unless
"eager-relay" is explicitly configured. Use the existing test structure and
symbol selectEagerPath.
🪄 Autofix (Beta)

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: 766b150b-3fd5-4eb8-83b9-940bab477e3c

📥 Commits

Reviewing files that changed from the base of the PR and between f9b9440 and 2670f24.

📒 Files selected for processing (10)
  • src/config.ts
  • src/lib/bun-stream-caps.ts
  • src/server/responses-snapshot-repair.ts
  • src/server/responses/core.ts
  • src/types.ts
  • tests/bun-stream-caps.test.ts
  • tests/config.test.ts
  • tests/passthrough-abort.test.ts
  • tests/relay-eager.test.ts
  • tests/responses-snapshot-repair.test.ts

Comment thread src/lib/bun-stream-caps.ts Outdated
Comment thread src/server/responses-snapshot-repair.ts
@0xWinner98

0xWinner98 commented Aug 2, 2026

Copy link
Copy Markdown
Author

Updated in d219dbc after CodeRabbit, independent red-team, and Claude Opus review. The revision adds item-status and nested-summary repair, request-faithful tool metadata, bounded output reconstruction, translator-budget accounting and teardown, client-only SSE/JSON repair, and corrected platform routing documentation. Validation: focused 196/196; typecheck, privacy scan, diff check, and docs build pass. The full prepush run reached 6,976 pass / 8 skip; its 7 failures are the same host-DNS fixture failures reproduced on clean dev. @coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

@0xWinner98 I will review the updates in commit d219dbc4, including the repair scope, stream routing, raw-byte preservation, budget teardown, and regression coverage.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

@0xWinner98 I will review the updates in commit d219dbc4. I will check the repair behavior, stream routing, raw-byte preservation, translator-budget teardown, and regression coverage.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 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 `@devlog/_plan/260731_macos_rss_retention/060_impl_roadmap.md`:
- Line 37: Update the roadmap’s stale rewrite restrictions: revise or mark
superseded the statements at lines 87-90 that require !needsClientRewrite and
prohibit rewrite-path widening, and remove or update the future-work note at
lines 98-99 about rewrite-traffic eager migration. Keep the roadmap consistent
with 100_darwin_eager_optin.md, where Darwin explicit eager-relay includes
inline client rewrites while auto remains tee.

In `@src/server/responses-snapshot-repair.ts`:
- Around line 80-90: Update the message branch in repairOutputItem so role is
preserved when it is a valid string and replaced with "assistant" only when
structurally invalid. Adjust the changed calculation and repaired object
construction accordingly, matching the forward-compatible behavior used by
isStructurallyValidToolChoice and status handling.

In `@src/server/sse-payload-rewrite.ts`:
- Around line 66-78: Update the exported SsePayloadRewrite type documentation to
state that its dispose method releases retained state and may throw the first
child-disposal error after attempting all child disposals. Leave the existing
disposal implementation and caller behavior unchanged.

In `@tests/responses-snapshot-repair.test.ts`:
- Around line 168-177: Add a blank-string status case to the response snapshot
repair test covering createResponsesSnapshotPayloadRewrite, and assert the
intended behavior explicitly: either rewrite status: "" to the event status
"completed" to match structural string validation, or assert that it remains
blank if pass-through is deliberate. Keep the existing null and numeric cases
unchanged.
- Around line 310-325: Update the loop bound in the test using
MAX_COMPLETED_OUTPUT_ITEMS imported from ../src/server/relay instead of the
literal 256. Ensure the loop still emits one more output item than the cap so
reconstructionTainted is triggered and terminal.response.output remains empty.
🪄 Autofix (Beta)

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: c4695ddc-a16b-4891-8889-c7adae5601c4

📥 Commits

Reviewing files that changed from the base of the PR and between 2670f24 and d219dbc.

📒 Files selected for processing (17)
  • devlog/_plan/260731_macos_rss_retention/060_impl_roadmap.md
  • devlog/_plan/260731_macos_rss_retention/100_darwin_eager_optin.md
  • docs-site/src/content/docs/ja/reference/configuration/providers.md
  • docs-site/src/content/docs/ko/reference/configuration/providers.md
  • docs-site/src/content/docs/reference/configuration/providers.md
  • docs-site/src/content/docs/ru/reference/configuration/providers.md
  • docs-site/src/content/docs/zh-cn/reference/configuration/providers.md
  • src/lib/bun-stream-caps.ts
  • src/server/relay-eager.ts
  • src/server/responses-snapshot-repair.ts
  • src/server/responses/core.ts
  • src/server/sse-payload-rewrite.ts
  • src/types.ts
  • tests/bun-stream-caps.test.ts
  • tests/relay-eager.test.ts
  • tests/responses-snapshot-repair.test.ts
  • tests/sse-payload-rewrite.test.ts

Comment thread devlog/_plan/260731_macos_rss_retention/060_impl_roadmap.md
Comment thread src/server/responses-snapshot-repair.ts
Comment thread src/server/sse-payload-rewrite.ts
Comment thread tests/responses-snapshot-repair.test.ts
Comment thread tests/responses-snapshot-repair.test.ts
@0xWinner98

Copy link
Copy Markdown
Author

Final review fixes are in 4fcdcd1. Focused tests: 197/197; independent review: NO FINDINGS; typecheck, privacy scan, diff check pass; full prepush: 6,977 pass / 8 skip with the same 7 host-DNS fixture failures as clean dev. Please verify the five addressed comments, including the deliberate canonical assistant role repair. @coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== PR metadata and review comments ==='
gh pr view 894 --repo lidge-jun/opencodex --json number,headRefOid,commits,comments,reviews 2>&1 || true
gh api "repos/lidge-jun/opencodex/pulls/894/comments?per_page=100" 2>&1 || true
gh api "repos/lidge-jun/opencodex/issues/894/comments?per_page=100" 2>&1 || true

echo '=== Commit and PR diff overview ==='
git status --short
git show --no-ext-diff --format=fuller --stat 4fcdcd12
git diff --no-ext-diff --unified=80 4fcdcd12^ 4fcdcd12 -- \
  src/server/responses-snapshot-repair.ts \
  src/server/responses/core.ts \
  src/server/relay-eager.ts \
  src/server/sse-payload-rewrite.ts \
  tests/responses-snapshot-repair.test.ts \
  tests/relay-eager.test.ts \
  tests/sse-payload-rewrite.test.ts

echo '=== Relevant symbol maps ==='
ast-grep outline src/server/responses-snapshot-repair.ts --items all --view expanded
ast-grep outline src/server/responses/core.ts --items all --view expanded
ast-grep outline src/server/relay-eager.ts --items all --view expanded
ast-grep outline tests/responses-snapshot-repair.test.ts --items all --view expanded

echo '=== Targeted references ==='
rg -n -C 4 -e 'assistant' -e 'responsesSnapshotRepair' -e 'repairResponsesSnapshotJson' \
  -e 'createResponsesSnapshotPayloadRewrite' -e 'output_item\.done' \
  -e 'TranslatorBudget|dispose' \
  src/server/responses-snapshot-repair.ts src/server/responses/core.ts \
  src/server/relay-eager.ts src/server/sse-payload-rewrite.ts \
  tests/responses-snapshot-repair.test.ts tests/relay-eager.test.ts tests/sse-payload-rewrite.test.ts

Length of output: 202055


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== Current commit and exact changed lines ==='
git rev-parse HEAD
git diff --no-ext-diff --unified=12 4fcdcd12^ 4fcdcd12

echo '=== Snapshot-repair implementation ==='
nl -ba src/server/responses-snapshot-repair.ts | sed -n '60,325p'

echo '=== Focused snapshot-repair tests ==='
nl -ba tests/responses-snapshot-repair.test.ts | sed -n '105,345p'

echo '=== Output-message schema and all role definitions ==='
rg -n -C 8 --type ts 'assistantMessageItemSchema|z\.literal\("assistant"\)|role:\s*z\.' src/responses src 2>/dev/null || true

echo '=== Completed-item cap definition and consumers ==='
rg -n -C 5 'MAX_COMPLETED_OUTPUT_ITEMS|MAX_COMPLETED_OUTPUT_ITEM_SOURCE_BYTES' src tests

echo '=== Rewrite disposal implementation and tests ==='
nl -ba src/server/sse-payload-rewrite.ts | sed -n '1,220p'
nl -ba tests/sse-payload-rewrite.test.ts | sed -n '96,150p'
nl -ba src/server/relay-eager.ts | sed -n '250,310p'

echo '=== Darwin eager-route implementation, tests, and documentation ==='
rg -n -C 8 'selectEagerPath|isWin32EagerRewrite|darwin|eager-relay|inline client rewrites|no-rewrite' \
  src/lib/bun-stream-caps.ts src/server/responses/core.ts \
  tests/bun-stream-caps.test.ts tests/passthrough-abort.test.ts \
  devlog/_plan/260731_macos_rss_retention/060_impl_roadmap.md \
  devlog/_plan/260731_macos_rss_retention/100_darwin_eager_optin.md

Length of output: 14544


🏁 Script executed:

#!/bin/bash
set -u

numbered_range() {
  file="$1"
  start="$2"
  end="$3"
  echo "=== $file:$start-$end ==="
  awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d  %s\n", NR, $0 }' "$file" || true
}

numbered_range src/server/responses-snapshot-repair.ts 60 320
numbered_range tests/responses-snapshot-repair.test.ts 105 345
numbered_range src/responses/schema.ts 25 65
numbered_range src/server/sse-payload-rewrite.ts 1 215
numbered_range tests/sse-payload-rewrite.test.ts 96 150
numbered_range src/server/relay-eager.ts 250 310
numbered_range src/lib/bun-stream-caps.ts 1 180
numbered_range tests/bun-stream-caps.test.ts 1 180
numbered_range devlog/_plan/260731_macos_rss_retention/060_impl_roadmap.md 65 105
numbered_range devlog/_plan/260731_macos_rss_retention/100_darwin_eager_optin.md 1 220

echo '=== Cap definitions and all consumers ==='
rg -n -C 4 'MAX_COMPLETED_OUTPUT_ITEMS|MAX_COMPLETED_OUTPUT_ITEM_SOURCE_BYTES' src tests || true

echo '=== All rewrite-disposal callers ==='
rg -n -C 4 'disposeRewrite|\.dispose\?\.\(\)|rewrite\.dispose' src tests || true

echo '=== Eager-route selection and test assertions ==='
rg -n -C 6 'selectEagerPath|isWin32EagerRewrite|config-eager|darwin' \
  src/lib/bun-stream-caps.ts src/server/responses/core.ts \
  tests/bun-stream-caps.test.ts tests/passthrough-abort.test.ts || true

Length of output: 104931


@0xWinner98 Verified commit 4fcdcd12. I found no new review findings.

The five items are resolved:

  1. Blank lifecycle statuses
    src/server/responses-snapshot-repair.ts Lines 93-95 and Lines 136-139 now repair empty or whitespace-only status strings.
    tests/responses-snapshot-repair.test.ts Lines 113-176 cover empty statuses and preserve non-empty future values.

  2. Canonical assistant output role
    src/responses/schema.ts Lines 46-50 define output-message role as the literal "assistant".
    src/server/responses-snapshot-repair.ts Lines 87-90 correctly normalizes non-assistant output-message roles.
    tests/responses-snapshot-repair.test.ts Lines 197-205 cover role: "user" repair. The prior concern does not apply.

  3. Completed-item cap test
    tests/responses-snapshot-repair.test.ts Lines 320-334 now uses MAX_COMPLETED_OUTPUT_ITEMS.
    The loop emits MAX_COMPLETED_OUTPUT_ITEMS + 1 items. This preserves the intended tainted-reconstruction case.

  4. Composed rewrite disposal
    src/server/sse-payload-rewrite.ts Lines 69-82 make disposal idempotent, attempt each child disposer, and rethrow the first error after cleanup.
    src/server/sse-payload-rewrite.ts Lines 147-152 and src/server/relay-eager.ts Lines 298-299 guard teardown errors.
    tests/sse-payload-rewrite.test.ts Lines 106-147 cover these behaviors.

  5. Darwin eager-relay documentation and policy
    devlog/_plan/260731_macos_rss_retention/060_impl_roadmap.md Lines 85-100 now matches the implementation.
    src/lib/bun-stream-caps.ts Lines 100-115 allow Darwin client rewrites only with explicit streamMode: "eager-relay".
    tests/bun-stream-caps.test.ts Lines 125-131 cover explicit eager rewrite and auto-mode tee behavior.

I did not independently run the test suite in this sandbox. The source-level review is complete.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-snapshot-repair.ts (1)

206-239: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reject gapped retained indexes before reconstruction.

At src/server/responses-snapshot-repair.ts:227-253, output_index accepts any non-negative integer. If completed items use indexes 0 and 2, sorting emits them at array positions 0 and 1, which produces an incomplete snapshot that appears valid. Require retained indexes to equal 0..n-1; otherwise set reconstructionTainted and suppress reconstruction. Add a regression in tests/responses-snapshot-repair.test.ts.

🤖 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-snapshot-repair.ts` around lines 206 - 239, The
reconstruction flow must reject gaps in retained output indexes instead of
compacting them into contiguous array positions. Update retainCompletedItem
and/or the surrounding response rewrite logic in responses-snapshot-repair.ts to
validate that retained indexes are exactly 0 through n-1; when a gap or invalid
sequence is detected, set reconstructionTainted and suppress reconstruction. Add
a regression test in responses-snapshot-repair.test.ts covering completed
indexes 0 and 2.

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-snapshot-repair.ts`:
- Around line 206-239: The reconstruction flow must reject gaps in retained
output indexes instead of compacting them into contiguous array positions.
Update retainCompletedItem and/or the surrounding response rewrite logic in
responses-snapshot-repair.ts to validate that retained indexes are exactly 0
through n-1; when a gap or invalid sequence is detected, set
reconstructionTainted and suppress reconstruction. Add a regression test in
responses-snapshot-repair.test.ts covering completed indexes 0 and 2.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 008c65a9-3ee3-40f2-8495-d2c0d8c58795

📥 Commits

Reviewing files that changed from the base of the PR and between d219dbc and 4fcdcd1.

📒 Files selected for processing (4)
  • devlog/_plan/260731_macos_rss_retention/060_impl_roadmap.md
  • src/server/responses-snapshot-repair.ts
  • src/server/sse-payload-rewrite.ts
  • tests/responses-snapshot-repair.test.ts

@0xWinner98

Copy link
Copy Markdown
Author

The gapped-index finding is fixed in fdbb3b7 across both client rewrite and raw inspector/persistence paths. Indexes must be exactly 0..n-1 before reconstruction; gaps suppress synthesis, out-of-order contiguous indexes sort correctly, duplicate indexes remain latest-wins, and authoritative non-empty output still wins. Validation: focused 198/198; persistence/inspector 155/155; independent review NO FINDINGS; typecheck, privacy scan, diff check pass; full prepush 6,981 pass / 8 skip with the same 7 host-DNS fixture failures as clean dev. @coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== Commit and changed files ==='
git rev-parse HEAD
git show --no-ext-diff --format=fuller --stat fdbb3b77
git diff --no-ext-diff --unified=30 fdbb3b77^ fdbb3b77 -- \
  src tests

echo '=== PR comments for finding context ==='
gh api "repos/lidge-jun/opencodex/pulls/894/comments?per_page=100" \
  --jq '.[] | {path, line, body, user: .user.login}' || true

echo '=== Candidate files and symbols ==='
fd -t f . src tests | rg 'responses|snapshot|inspect|persist|relay'
rg -n -C 5 \
  -e 'MAX_COMPLETED_OUTPUT_ITEMS' \
  -e 'output_item\.done' \
  -e 'completedOutput' \
  -e 'reconstruct' \
  -e 'index' \
  -e 'repairResponsesSnapshotJson' \
  -e 'createResponsesSnapshotPayloadRewrite' \
  src tests

echo '=== AST outlines ==='
for file in \
  src/server/responses-snapshot-repair.ts \
  src/server/responses/core.ts \
  tests/responses-snapshot-repair.test.ts
do
  if [ -f "$file" ]; then
    echo "--- $file ---"
    ast-grep outline "$file" --items all --view expanded
  fi
done

echo '=== Exact current implementations around reconstruction ==='
for file in \
  src/server/responses-snapshot-repair.ts \
  src/server/responses/core.ts \
  tests/responses-snapshot-repair.test.ts
do
  if [ -f "$file" ]; then
    echo "--- $file ---"
    nl -ba "$file" | sed -n '1,460p'
  fi
done

Length of output: 194604


🏁 Script executed:

#!/bin/bash
set -euo pipefail

show_range() {
  file="$1"
  start="$2"
  end="$3"
  printf '\n=== %s, Lines %s-%s ===\n' "$file" "$start" "$end"
  awk -v start="$start" -v end="$end" \
    'NR >= start && NR <= end { printf "%6d  %s\n", NR, $0 }' "$file"
}

echo '=== Revision identity ==='
git rev-parse HEAD
git show --no-ext-diff --format=fuller --stat fdbb3b77

echo '=== Exact fix diff ==='
git diff --no-ext-diff --unified=20 fdbb3b77^ fdbb3b77 -- \
  src/server/responses-snapshot-repair.ts \
  src/server/responses-inspector.ts \
  src/server/responses-state.ts \
  tests/responses-snapshot-repair.test.ts \
  tests/responses-state.test.ts

echo '=== Locate raw inspector implementation and relevant test names ==='
rg -n -C 3 \
  -e 'createSseInspector' \
  -e 'onCompletedResponse' \
  -e 'reconstructionTainted' \
  -e 'orderedItems' \
  -e 'output_index keeps' \
  -e 'gapped|gap|out-of-order|authoritative' \
  src/server tests/responses-snapshot-repair.test.ts tests/responses-state.test.ts

show_range src/server/responses-snapshot-repair.ts 149 307
show_range tests/responses-snapshot-repair.test.ts 1 460
show_range tests/responses-state.test.ts 250 550

for file in $(fd -t f . src/server | rg 'responses.*(inspect|state)|inspect.*responses'); do
  echo "=== outline: $file ==="
  ast-grep outline "$file" --items all --view expanded
done

Length of output: 82534


🏁 Script executed:

#!/bin/bash
set -euo pipefail

show_range() {
  file="$1"
  start="$2"
  end="$3"
  printf '\n=== %s, Lines %s-%s ===\n' "$file" "$start" "$end"
  awk -v start="$start" -v end="$end" \
    'NR >= start && NR <= end { printf "%6d  %s\n", NR, $0 }' "$file"
}

show_range src/server/relay.ts 559 735
show_range tests/sse-inspector-bounds.test.ts 1 260

echo '=== Index-specific regression tests ==='
rg -n -C 6 \
  -e 'gap|gapped|contiguous|out-of-order|duplicate|authoritative' \
  tests/sse-inspector-bounds.test.ts \
  tests/responses-state.test.ts \
  tests/responses-snapshot-repair.test.ts

echo '=== Commit test diff ==='
git diff --no-ext-diff --unified=12 fdbb3b77^ fdbb3b77 -- \
  tests/sse-inspector-bounds.test.ts \
  tests/responses-state.test.ts \
  tests/responses-snapshot-repair.test.ts

Length of output: 42545


@0xWinner98 Verified commit fdbb3b77. I found no new review findings.

The gapped-index finding is resolved.

  • Client rewrite path: src/server/responses-snapshot-repair.ts Lines 252-260 sorts retained items and reconstructs only when indexes are exactly 0..n-1. A gap leaves the terminal output empty.
  • Raw inspector and persistence path: src/server/relay.ts Lines 714-722 applies the same sorted-contiguous check. A gap clears retained state and prevents onCompletedResponse from persisting a partial replay.
  • Out-of-order contiguous indexes: Both paths sort by output_index before reconstruction. tests/sse-inspector-bounds.test.ts Lines 245-258 verifies index-order output.
  • Duplicate indexes: src/server/relay.ts Lines 622-634 replaces the prior retained item at the same index. tests/sse-inspector-bounds.test.ts Lines 260-269 verifies latest-wins behavior.
  • Authoritative terminal output: src/server/relay.ts Lines 703-724 keeps a non-empty terminal response.output authoritative, even after reconstruction taint. tests/responses-state.test.ts Lines 326-360 and tests/sse-inspector-bounds.test.ts Lines 306-323 cover this behavior.
  • Client regression coverage: tests/responses-snapshot-repair.test.ts Lines 240-255 verifies that indexes [0, 2] do not synthesize output.
  • Raw-path regression coverage: tests/sse-inspector-bounds.test.ts Lines 235-243 verifies that the same indexes do not invoke the persistence callback.

I did not run the test suite in this sandbox. The source-level review is complete.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@lidge-jun

Copy link
Copy Markdown
Owner

Closing unmerged, with three concrete reasons:

  1. It rewrites another unit's locked design record. The diff edits devlog/_plan/260731_macos_rss_retention/060_impl_roadmap.md and 100_darwin_eager_optin.md to supersede decisions that unit explicitly locked ("Design decisions locked for 100"), including one deferred as "future unit" — widening Darwin eager-relay to client-rewrite traffic. A drive-by PR cannot re-open another workstream's locked gate matrix; that needs a maintainer discussion first.
  2. It overlaps relay paths being hardened right now. relay.ts / relay-eager.ts / sse-payload-rewrite.ts / responses/core.ts are all in flight in fix: bound the six remaining retained-state leaks (#820 wave 2) #892 (retained-state bounds). Stacking a 1.7k-line opt-in rewrite feature on the same files this week is how regressions ship.
  3. The core idea is fine. A provider-local, disabled-by-default responsesSnapshotRepair with bounded reconstruction is a reasonable compat feature. If you resubmit it as a narrow PR — config option + responses-snapshot-repair.ts + tests, no devlog edits, no eager-relay gate change — after fix: bound the six remaining retained-state leaks (#820 wave 2) #892 lands, we will review it on its merits.

Thanks for the thorough engineering; the process boundaries are the blocker, not the code quality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants