Skip to content

fix(responses): make whole-string citation stripping agree with the streaming filter - #3882

Merged
lidge-jun merged 1 commit into
devfrom
codex/rt-m7-citation-terminal
Sep 7, 2026
Merged

fix(responses): make whole-string citation stripping agree with the streaming filter#3882
lidge-jun merged 1 commit into
devfrom
codex/rt-m7-citation-terminal

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Summary

After #3868 the streaming citation 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 the text between. src/bridge.ts:615 re-strips the accumulated message for output_text.done / output_item.done, so the terminal text disagreed with the concatenated deltas. This makes the whole-string path walk START-delimited segments exactly like the streaming filter.

Found by the lane A fresh-base composition audit on dev@d00615d56 (anchors: src/responses/citation-markers.ts:48, src/bridge.ts:615). Follow-up to the #3843 carry; Co-authored-by trailer kept.

Verification

  • New regressions in tests/responses/citation-markers.test.ts: malformed START before a later valid span is kept in whole-string mode; concatenated streaming output equals whole-string stripping for four inputs × four chunkings (1, 7, 4097, whole).
  • Cross-platform CI (lane=all) at this head: recorded below before merge.
  • Local checks NOT RUN by maintainer instruction; pushed with --no-verify.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes
    • Improved citation marker handling for malformed, nested, or incorrectly ordered markers.
    • Preserved answer text when citation spans are incomplete, exceed the supported length, or contain unexpected marker sequences.
    • Ensured citation cleanup behaves consistently across complete responses and streaming output.
    • Fixed cases where valid citation spans could incorrectly remove surrounding answer text.
    • Added coverage for malformed markers, boundary-length spans, and streaming responses assembled from arbitrary chunks.

Independent review (astra explorer, read-only) — PASS at amended head. The shared 4096 bound closes the chunk-dependence gap the first round found: the whole-string path and the streaming filter now classify every START-delimited segment identically (in-bound END = span removed; over-bound or superseded = verbatim). Hand-traced all existing tests plus the late-END and at-bound equivalence inputs across chunk sizes 1/7/4097/whole. Callers bridge.ts:615 and :1707 change only in the intended way.


Maintainer integration decision (MAINTAINERS.md, dev-only admin integration): @lidge-jun integrates #3882 (+ docs follow-up #3888) into dev. Exact-head evidence for #3882 at e96b5c52e: Cross-platform CI run 34114667385 — Linux test 1/4–4/4, macOS 1/2, 2/2, gates, storage policy, api usage, keyring ×3, npm-global ×3, docker smoke = success; Windows shards and macos control deferred to the final release-train head by maintainer policy. #3888 is docs-only on top of #3882. Independent review PASS at the amended head (see body). Local suites NOT RUN. Maintainer integration, not self-approval.

@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 7, 2026 10:19
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-07T10:23:06.392127Z 922dc58 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 8aa3e90a-6b48-4f1c-bebc-b7fdd8e2858f

📥 Commits

Reviewing files that changed from the base of the PR and between 922dc58 and 1586d9e.

📒 Files selected for processing (2)
  • src/responses/citation-markers.ts
  • tests/responses/citation-markers.test.ts

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


📝 Walkthrough

Walkthrough

stripCitationMarkers now processes each START-delimited segment independently. Malformed segments remain unchanged, valid citation spans are removed, and streaming output matches whole-string output across multiple chunk sizes.

Changes

Citation marker alignment

Layer / File(s) Summary
START-delimited stripping and streaming parity
src/responses/citation-markers.ts, tests/responses/citation-markers.test.ts
stripCitationMarkers removes only valid spans within each START-delimited segment. Over-bound and malformed segments remain verbatim. The shared span limit aligns whole-string and streaming behavior. Tests cover nested START markers, boundary lengths, late END markers, and multiple chunk sizes.

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

Merge Risk: ⚪ Minimal · up to e96b5

Citation stripping now preserves malformed marker text while removing only valid bounded citation spans, with streaming and whole-string output aligned. The covered boundary and chunking cases leave no active merge-readiness risk.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.
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: aligning whole-string citation stripping with the streaming filter. It matches the PR objectives and changed files.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/rt-m7-citation-terminal

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.

@lidge-jun
lidge-jun force-pushed the codex/rt-m7-citation-terminal branch 2 times, most recently from 5d2d3cb to 1586d9e Compare September 7, 2026 10:21
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 7, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 73 / 80

이 PR은 방금 dev에 올라간 #3868(인용 마커 스트리밍 필터) 바로 위의 후속 정정입니다. 지금 HEAD d00615d56createCitationMarkerFilter는 START 구간을 앞에서부터 하나씩 걷고, 과대·앞선 malformed START는 뒤쪽 END와 짝짓지 않은 채 그대로 내보냅니다. 그런데 같은 파일의 stripCitationMarkers(통째 문자열 경로)는 아직 예전처럼 첫 START와 나중에 나오는 END를 한 쌍으로 묶습니다. src/bridge.ts는 델타를 필터로 내보낸 뒤 output_text.done / output_item.done에서 누적 문장을 다시 stripCitationMarkers로 깎습니다(대략 615·1707줄 근처). 그래서 스트리밍으로 이미 보낸 글자와, 끝나면 다시 깎인 최종 글자가 어긋날 수 있습니다. 사용자는 중간에는 답이 보이다가 마지막에 한 덩어리가 사라지거나 바뀌는 식으로 느낄 수 있습니다.

이 패치는 통째 경로도 스트리밍과 같은 규칙으로 맞춥니다. START부터 다음 START(또는 끝)까지를 한 구간으로 보고, 그 안에 END가 있고 길이가 상한 안이면 그 스팬만 지웁니다. END가 없거나 상한을 넘으면 malformed 일반 텍스트로 남깁니다. 상한 이름도 MAX_STREAMING_MARKER_SPAN_LENGTH에서 MAX_CITATION_SPAN_LENGTH로 바꿔 두 경로가 같은 4096을 씁니다. 스트리밍 쪽도 늦은 END가 상한을 넘긴 스팬을 지우지 않도록 end !== -1 && end + 1 <= MAX_CITATION_SPAN_LENGTH로 맞춰 두었습니다. 그래서 “델타를 어떻게 잘랐는지”와 무관하게 같은 입력이 같은 결과를 냅니다.

테스트가 계약을 직접 잠급니다. 과대 malformed 뒤에 정상 스팬이 오면 malformed는 남고 정상만 빠집니다. 이어 붙은 START끼리도 앞 조각이 뒤 END에 먹히지 않습니다. 그리고 여러 입력을 청크 크기 1·7·4097·통째로 잘라 스트리밍 drain 결과와 stripCitationMarkers가 항상 같다는 걸 확인합니다. 상한을 딱 채운 스팬과, 상한을 넘긴 뒤 늦게 END가 오는 경우도 넣었습니다. types.ts/config.ts 대분할과 겹치지 않고, 파일도 src/responses/citation-markers.ts와 테스트 두 곳뿐입니다. #3868이 이미 dev에 있으니 이 PR은 리베이스 부담이 거의 없고, 지금 합치면 방금 연 구멍(#3843 계열의 통째·스트리밍 불일치)을 바로 닫습니다.

로컬 검사는 메인테이너 지시로 NOT RUN이고 CI는 머지 전 기록 예정입니다. 독립 astra 리뷰는 PASS로 적혀 있습니다. 범위가 작고 회귀 축이 분명해서, CI만 초록이면 머지 가치가 큽니다.

src/responses/citation-markers.ts stripCitationMarkers - START 구간 walk + 공유 상한. 예전 첫 START↔나중 END 짝짓기를 없애 스트리밍과 같은 분류를 만듭니다.
src/responses/citation-markers.ts MAX_CITATION_SPAN_LENGTH - 4096을 통째·스트리밍이 같이 씁니다. 늦은 END가 과대 스팬을 지우지 않게 맞춘 핵심입니다.
src/bridge.ts (대략 615·1707) - 호출부는 그대로지만, done 재스트립이 델타 합과 같아지도록 이 PR이 전제합니다. 호출 시그니처 변경은 없습니다.
tests/responses/citation-markers.test.ts - malformed 보존 + 청크 크기별 동등성. done != concatenated deltas 회귀를 직접 잠급니다.

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

  • 원격 lane=all(또는 최소 citation 테스트 포함 CI)이 초록인지 합치기 직전에만 확인할지
  • 상한 4096을 #3868과 동일하게 유지할지, 통째 경로만 더 작게 줄일지(권장은 동일 유지)
  • 기여자 fix(responses): bound the streaming citation marker span #3843 계열이 이미 #3868로 닫혔는지, 이 후속만 따로 릴리스 노트에 한 줄 남길지

너의 추천
CI 초록 확인 후 dev에 바로 머지하세요. #3868 직후 불일치를 막는 작은 정정이고, 테스트가 그 계약을 잠급니다. types/config 분할과 무관하니 닫지 말고 합치세요.

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

@lidge-jun
lidge-jun force-pushed the codex/rt-m7-citation-terminal branch from 1586d9e to 85a3fee Compare September 7, 2026 10:52
…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>
@lidge-jun
lidge-jun force-pushed the codex/rt-m7-citation-terminal branch from 85a3fee to e96b5c5 Compare September 7, 2026 11:03
@lidge-jun
lidge-jun merged commit 6389787 into dev Sep 7, 2026
26 of 47 checks passed
@lidge-jun
lidge-jun deleted the codex/rt-m7-citation-terminal branch September 7, 2026 11:20
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.

1 participant