refactor(bridge): split bridge.ts behind a facade - #4672
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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change extracts bridge functionality into shared utilities, a batch Responses JSON translator, and a streaming Responses SSE translator. It adds tool validation, usage normalization, budgeting, citations, terminal handling, heartbeats, cancellation, and extracted-module metadata. ChangesResponses bridge translation
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~90 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant Adapter as Adapter event iterable
participant Bridge as bridgeToResponsesSSE
participant Budget as Translator budget
participant Client as ReadableStream client
Adapter->>Bridge: Deliver adapter events
Bridge->>Budget: Charge retained and transient bytes
Bridge->>Client: Emit Responses SSE frames
Bridge->>Client: Emit terminal event and [DONE]
Merge Risk: ⚪ Minimal · up to The bridge extraction has no established current-head failure requiring a merge hold. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
chore
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba445bec4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export { buildResponseJSON } from "./bridge/response-json"; | ||
| export { bridgeToResponsesSSE } from "./bridge/sse"; |
There was a problem hiding this comment.
When bun test tests/lib/reasoning-replay-scope-source.test.ts runs, the test still reads only src/bridge.ts and expects two const replayCacheScope = options?.replayCacheScope; declarations. This facade now contains neither declaration, so .match() returns null and toHaveLength(2) fails. Update the test to inspect src/bridge/sse.ts and src/bridge/response-json.ts, or aggregate those leaves, so the required test suite can pass.
AGENTS.md reference: src/AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
리뷰 · 우선순위 73 / 80이 PR은 지금 왜 지금 점수이냐면, round5 로드맵( 다만 지금 GitHub 상태는 바로 머지하면 안 된다. #4671이 한 가지 설계 선택은 계획서와 다르다. CI는 gates·hygiene·docker smoke 등은 통과했고, 리뷰 시점 기준 라인 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
src/bridge.ts was 2,206 lines, and almost all of it was two functions: bridgeToResponsesSSE at 1,387 lines and buildResponseJSONWithBudget at 562. Both move whole; neither body changes. Four leaves under src/bridge/: errors.ts 34 formatErrorResponse internal.ts 174 shared helpers, the owned-budget state, output types response-json.ts 624 buildResponseJSON and buildResponseJSONWithBudget sse.ts 1,444 bridgeToResponsesSSE plus its two private helpers The facade is 7 lines of re-exports and keeps all six public names. Placement came from counting each helper's uses per region rather than guessing. sseEvent and responseError are read only inside the SSE function, so they travel with it. adapterFailureFromEvent is read from both the SSE and JSON paths, so it goes to internal.ts. The mutable module state needed care. `ownedBudgetAbandonedMs` is a module-level `let` mutated by setOwnedBudgetAbandonedMsForTests and read from inside the SSE function, which now lives in a different file. The declaration, its default constant and the setter all stay in internal.ts, and sse.ts imports the binding rather than copying the value, so the ES live binding still shows a test-set value. Copying it into a local or re-exporting a snapshot would have silently frozen the watchdog delay at ten minutes. tests/responses/responses-undeclared-tool-guard.test.ts repoints its comment reference for declaredToolNames to the leaf that holds it. Ratchet cap lowered from 2,206 to 7.
structure/manifest.json graces src/bridge.ts because no doc names that path; the leaves moved out of it inherit exactly that situation, and structure:check only saw the new directory once it was tracked. Regenerated structure/INDEX.md.
ba445be to
3c6cf21
Compare
…aves
tests/lib/reasoning-replay-scope-source.test.ts reads bridge source as text and
pins two declarations of `const replayCacheScope = options?.replayCacheScope;`.
After the facade split one lives in src/bridge/sse.ts and the other in
src/bridge/response-json.ts, so reading the facade matched nothing and the
assertion failed on null. Read both leaves and keep the count at 2.
This oracle was missed when the split was planned. The audit searched tests/ for
the literal `src/bridge.ts`, but this test composes the path from a relative
fragment: `repoPath("src", ...relative.split("/"))` called with `"bridge.ts"`.
A literal search cannot see that. The replacement check resolves every string
literal in a test that reads files, against the real src tree, which finds the
composed form too.
Maintainer integration recordIntegrating into Exact head verified: CI at that head: every non-skipped check reports SUCCESS, including One real failure was found and fixed on this branch, which is worth recording. The first push failed The planning audit missed it because it searched That is the failure mode this round was most worried about — an oracle that reads source as text and goes quiet when the content moves — and it is why the split PRs are verified against hosted CI rather than a local proof alone. Security review: not applicable. No authentication, credential, OAuth, workflow, release-automation or dependency-installation path is touched. Outstanding maintainer change requests: none. |
1 participant
Summary
Stacked on #4671.
src/bridge.tswas 2,206 lines, and almost all of it was two functions:bridgeToResponsesSSEat 1,387 lines andbuildResponseJSONWithBudgetat 562. Both move whole, neither body changes, and the facade becomes 7 lines of re-exports keeping all six public names.src/bridge/errors.tsformatErrorResponsesrc/bridge/internal.tssrc/bridge/response-json.tsbuildResponseJSONandbuildResponseJSONWithBudgetsrc/bridge/sse.tsbridgeToResponsesSSEand its two private helpersPlacement came from counting each helper's reads per region rather than guessing.
sseEventandresponseErrorare read only inside the SSE function, so they travel with it.adapterFailureFromEventis read from both the SSE and the JSON path, so it goes tointernal.ts.The mutable module state needed care.
ownedBudgetAbandonedMsis a module-levelletthatsetOwnedBudgetAbandonedMsForTestsmutates and that the SSE function reads when it arms the abandoned-budget watchdog. The declaration, its default constant and the setter all stay together ininternal.ts, andsse.tsimports the binding instead of copying the value, so the ES live binding still shows a test-set value. Copying it into a local or re-exporting a snapshot would have frozen the delay at ten minutes with every test still green.structure/manifest.jsongraces the newsrc/bridge/directory. The facade path is already graced because no doc names it, the leaves inherit exactly that situation, andstructure:checkonly sees the directory once it is tracked.structure/INDEX.mdregenerated.tests/responses/responses-undeclared-tool-guard.test.tsrepoints its comment reference fordeclaredToolNamesto the leaf that holds it.Ratchet cap lowered from 2,206 to 7.
Verification
Run in a worktree without
node_modules, so the hosted suite and typecheck are what this PR relies on for the rest.bun scripts/structure-ssot.ts—structure/ SSOT checks passedbun scripts/file-size-ratchet.ts—file-size ratchet passedsrcandgui/src: 8,344 specifiers, no new unresolved import against the pre-change baseline.origin/devwithBun.Transpiler().scan().exports:src/bridge.tsandsrc/adapters/openai-responses.tsboth identical.git show origin/dev:<path>, normalizing only the addedexportkeyword. 0 drift.bun x tsc --noEmit --strict --skipLibCheckover the facade and leaves reports three diagnostics naming these paths:TS2591forBuffertwice andTS2339forTimer.unref. Running the same command againstgit show origin/dev:src/bridge.tsreproduces all three, so they are the missing@types/nodein this worktree and not something the split introduced.Checklist
Roadmap and audit record:
devlog/_plan/260915_godfile_round5/.Summary by CodeRabbit
New Features
Documentation