fix(responses): bound the streaming citation marker span (carry #3843) - #3868
Conversation
`debugProviderDiagnostic` already returns early when provider debug is off, but its argument object is built by the caller first. The Kiro request path therefore ran `new TextEncoder().encode(body).length` over the entire serialized request body on every request, including when diagnostics were disabled, and then discarded the result inside the callee. Wrap the diagnostic call in `isDebugEnabled()` so the details are only constructed when they can actually be emitted. `src/adapters/openai-chat.ts` already guards its diagnostics the same way. The regression asserts that building a request performs no `TextEncoder` encode over the serialized payload while diagnostics are off; it fails without the guard and passes with it. (cherry picked from commit d5d711a) Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…ate reads [skip ci] Resolves the maintainer objection on #3837 (discussion_r3945935220): the shared setup cleared only OCX_DEBUG_FRAMES, so an inherited OCX_DEBUG=1 or a runtime debug override made the encoder-spy test fail legitimately. Snapshot OCX_DEBUG, OCX_DEBUG_FRAMES and the runtime override in beforeEach, clear them, and restore the exact previous values in afterEach. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
(cherry picked from commit 8ef77f7) Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…er marker in the same delta [skip ci] Resolves the unresolved major finding on #3843 (discussion_r3946034145): lastIndexOf selected the later START, its END made the whole-string strip pair the first START with that END, and the malformed text vanished. Walk START-delimited segments independently so a superseded or over-bound span is emitted verbatim and only a bounded trailing span is held for the next delta. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. |
![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: 00b74c7200
ℹ️ 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".
| // Superseded by a later START, or over the bound: ordinary text, emitted verbatim | ||
| // so neither the retained text nor the per-delta rescan grows without limit. | ||
| out += segment; |
There was a problem hiding this comment.
When an over-limit unterminated START is followed by a valid marker, this branch emits the malformed segment verbatim, but src/bridge.ts:615 still constructs every terminal event by running the raw accumulated text through stripCitationMarkers(). That whole-string parser pairs the malformed START with the later marker's END, so the deltas contain the retained text while response.output_text.done, content_part.done, and output_item.done omit it; clients can therefore replace visible output or save a truncated transcript. Build the terminal text from the same segmented filtering result (or give the whole-string stripper identical semantics) and cover this case through bridgeToResponsesSSE, not only the standalone filter.
AGENTS.md reference: AGENTS.md:L367-L370
Useful? React with 👍 / 👎.
리뷰 · 우선순위 61 / 80이 PR은 기여자 #3843(luvs01)을 릴리스 트레인 레이어 4( 새 구현은 테스트 세 개가 계약을 잠급니다. (1) 미종료 스팬에 x를 5000자 붙이면 전부 방출되고 동작 미묘한 점 하나: START가 전혀 없을 때 예전 코드는
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…treaming filter After #3868 the streaming filter keeps a malformed START verbatim when a later START opens a real span, but stripCitationMarkers still paired the first START with that later span's END and deleted everything between. bridge.ts re-strips the accumulated text for output_text.done and output_item.done, so the terminal text disagreed with the concatenated deltas. Walk START-delimited segments in the whole-string path too, and share the 4096 span bound with the whole-string path (an over-bound span that is terminated late is malformed text in both), and assert delta-vs-whole equality across several chunkings. Found by the lane A fresh-base composition audit on dev d00615d. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…treaming filter After #3868 the streaming filter keeps a malformed START verbatim when a later START opens a real span, but stripCitationMarkers still paired the first START with that later span's END and deleted everything between. bridge.ts re-strips the accumulated text for output_text.done and output_item.done, so the terminal text disagreed with the concatenated deltas. Walk START-delimited segments in the whole-string path too, and share the 4096 span bound with the whole-string path (an over-bound span that is terminated late is malformed text in both), and assert delta-vs-whole equality across several chunkings. Found by the lane A fresh-base composition audit on dev d00615d. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…treaming filter After #3868 the streaming filter keeps a malformed START verbatim when a later START opens a real span, but stripCitationMarkers still paired the first START with that later span's END and deleted everything between. bridge.ts re-strips the accumulated text for output_text.done and output_item.done, so the terminal text disagreed with the concatenated deltas. Walk START-delimited segments in the whole-string path too, and share the 4096 span bound with the whole-string path (an over-bound span that is terminated late is malformed text in both), and assert delta-vs-whole equality across several chunkings. Found by the lane A fresh-base composition audit on dev d00615d. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…treaming filter After #3868 the streaming filter keeps a malformed START verbatim when a later START opens a real span, but stripCitationMarkers still paired the first START with that later span's END and deleted everything between. bridge.ts re-strips the accumulated text for output_text.done and output_item.done, so the terminal text disagreed with the concatenated deltas. Walk START-delimited segments in the whole-string path too, and share the 4096 span bound with the whole-string path (an over-bound span that is terminated late is malformed text in both), and assert delta-vs-whole equality across several chunkings. Found by the lane A fresh-base composition audit on dev d00615d. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
2 participants
Summary
Carries #3843 by @luvs01: the streaming citation-marker filter retains at most 4096 characters for an unterminated START. Adds the fix for the unresolved major finding (discussion_r3946034145): START-delimited segments are now walked independently, so an oversized malformed span followed by a valid marker in the same delta is emitted verbatim instead of being paired with the later END and dropped.
(carried/reimplemented from #3843; Co-authored-by trailer in the commit)
Verification
tests/responses/citation-markers.test.ts(existing bound/recovery tests plus the new same-delta case) runs in the chain-top CI.Checklist
Manual review chain (integrate bottom-up; stack: null, no native stack)
codex/rt-m1-3532devcodex/rt-m2-3840codex/rt-m3-3837codex/rt-m4-3843codex/rt-m5-3845codex/rt-m6-2033Verification policy (maintainer instruction, this train): local test suite / typecheck / build were NOT RUN; branches pushed with
--no-verify. Lower layers carry[skip ci]; the full Cross-platform CI (lane=all, Windows shards included) runs once at the chain top head and is the exact-head evidence for the cumulative tree.Layer 4 of 6. Review this PR's diff only.
Maintainer integration decision (MAINTAINERS.md, dev-only admin integration): @lidge-jun integrates this manual chain into
devbottom-up. Exact chain-top evidence: Cross-platform CI run 34106345180 at head6eadb1658(lane=all: Linux 4/4, macOS 2/2 + control, Windows 6/6, gates, storage policy, api usage, keyring ×3, npm-global ×3, docker smoke, aggregateci= success). Tested tree7621cac89equals the prospective merge tree oforigin/dev@ece556a6e+ chain top. Independent chain review PASS; #3845 security review PASS (see #3869). Local suites NOT RUN by maintainer instruction. This is maintainer integration, not self-approval. Lower-layer PR runs are skipped/cancelled by design ([skip ci]); they are not passing evidence on their own.