Skip to content

fix(responses): strip internal summary:none marker on the wire - #6247

Merged
lidge-jun merged 4 commits into
devfrom
codex/rt6-l3-strip-summary-none
Sep 29, 2026
Merged

lidge-jun merged 4 commits into
devfrom
codex/rt6-l3-strip-summary-none

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Carry fix(responses): drop the internal summary:none marker before it reaches the upstream #6232: keep reasoning.summary: "none" as a parser-side hide marker, then remove it from the final upstream Responses request. Valid summary choices remain intact.
  • Move the regression into tests/responses/openai-responses-summary-none.test.ts to respect the passthrough test's file-size cap; register the new file in both layout manifests and its seed rule. Clarify the wire contract in structure/.

Co-authored-by: cshyang cshyang.chng@gmail.com

Verification

  • bun install --frozen-lockfile — passed.
  • bun test tests/responses/openai-responses-passthrough.test.ts tests/responses/openai-responses-summary-none.test.ts — 189 pass, 0 fail.
  • bun test tests/ci-workflows/file-size-ratchet.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts — 27 pass, 0 fail.
  • bun run typecheck — passed.
  • bun run structure:check — passed.
  • bun run privacy:scan — passed.
  • Full bun run test was not run locally because four RT6 worktrees share this machine; hosted CI owns the broad suite.

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
    • Fixed Responses request forwarding so the internal “none” reasoning-summary setting is not sent upstream.
    • Other reasoning settings, including effort and supported summary values, continue to be preserved.

Carry #6232 with its regression in a sibling file so the capped passthrough suite does not grow.

Co-authored-by: cshyang <cshyang.chng@gmail.com>
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 29, 2026 17:39
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 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-29T17:42:55.337305Z 4842a64 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 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 11 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: add83c55-9cb4-4906-a610-5041ff4a7327

📥 Commits

Reviewing files that changed from the base of the PR and between 4842a64 and 22728b9.

📒 Files selected for processing (7)
  • scripts/test-layout/layout.json
  • scripts/test-layout/seeds.json
  • src/adapters/openai-responses/passthrough.ts
  • src/adapters/openai-responses/reasoning.ts
  • structure/transports/responses-wire-shapes.md
  • tests/fixtures/test-layout-expected.json
  • tests/responses/openai-responses-summary-none.test.ts

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: 97a0f076-94a7-4092-a345-12428c29aa3f

📥 Commits

Reviewing files that changed from the base of the PR and between 73289d4 and 4842a64.

📒 Files selected for processing (7)
  • scripts/test-layout/layout.json
  • scripts/test-layout/seeds.json
  • src/adapters/openai-responses/passthrough.ts
  • src/adapters/openai-responses/reasoning.ts
  • structure/transports/responses-wire-shapes.md
  • tests/fixtures/test-layout-expected.json
  • tests/responses/openai-responses-summary-none.test.ts

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


📝 Walkthrough

Walkthrough

The Responses passthrough adapter removes reasoning.summary: "none" from outgoing request bodies. Other reasoning settings remain. Tests cover both Responses destinations and verify that valid summary values are preserved.

Changes

Responses summary marker handling

Layer / File(s) Summary
Strip the internal summary marker
src/adapters/openai-responses/reasoning.ts, src/adapters/openai-responses/passthrough.ts, structure/transports/responses-wire-shapes.md
The adapter removes reasoning.summary when its value is "none". It retains other reasoning properties and removes the reasoning object if no properties remain. The transport documentation describes this serialization behavior.
Test both Responses destinations
tests/responses/openai-responses-summary-none.test.ts, scripts/test-layout/layout.json, scripts/test-layout/seeds.json, tests/fixtures/test-layout-expected.json
Tests check that outgoing reasoning omits a "none"-only marker and preserves auto, concise, and detailed summaries with effort. Test layout mappings assign the new test to responses.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 4842a

No merge-blocking issue is established. The internal marker is omitted from outgoing Responses requests while valid reasoning settings remain intact.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 4842a

The change is narrow, and the reviewed request path shows no new endpoint or privilege. Tests cover the outgoing body for both destinations. Response-side privacy behavior has not been fully traced.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Input containing the internal marker can affect outbound reasoning serialization for either tested destination. The reviewed ranges do not establish a new route or credential authority.

Trust Boundaries and Controls

  • observed — The test confirms that the parsed request retains its hide-thinking flag before the marker is removed from the upstream body. It does not trace whether every downstream response path honors that flag.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (4 skipped: 4… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: removing the internal summary: "none" marker from outbound Responses requests while preserving it for parser-side behavior.
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (4 skipped: 4 unsupported.)

✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4842a640a4

ℹ️ 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".

Comment on lines +564 to +565
The passthrough adapter removes that internal `none` marker at final outbound serialization;
valid summary values remain on the upstream Responses wire.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Update public docs for the stripped summary marker

This new wire contract contradicts docs-site/src/content/docs/guides/grok-build.md:86-90, which still tells users that Responses passthrough forwards reasoning.summary as configured; the translated versions repeat that claim. After this change, an explicit "none" is accepted only as a proxy-side display marker and is omitted upstream, so update the English guide and its translations to distinguish the inbound proxy option from the outbound wire values.

AGENTS.md reference: AGENTS.md:L453-L454

Useful? React with 👍 / 👎.

@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 Sep 29, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 14 / 80

이 PR은 Claude 쪽에서 온 thinking.display: "omitted"가 내부용 표시인 reasoning.summary: "none"으로 바뀐 뒤, 그 값이 OpenAI Responses 업스트림 요청에까지 그대로 실려 나가던 문제를 고칩니다. 업스트림은 auto / concise / detailed만 받는데 "none"이 들어가면 400이 납니다. 고치는 방식은 파서가 hideThinkingSummary를 읽을 때는 마커를 그대로 두고, passthrough가 최종로 업스트림에 보내기 직전에 stripNoneReasoningSummary로만 빼는 것입니다. effort 같은 다른 reasoning 필드는 남기고, "none"만 있을 때는 reasoning 키 자체를 지웁니다. 회귀 테스트는 파일 크기 한도를 피하려고 passthrough 스위트가 아니라 openai-responses-summary-none.test.ts로 분리했고, layout·seeds·structure 문서도 맞춰 두었습니다. base는 dev입니다.

라인 - src/adapters/openai-responses/reasoning.ts stripNoneReasoningSummary: 구현 자체는 짧고 의도가 분명합니다. "none"만 제거하고 유효한 summary는 그대로 두는 동작이 테스트로도 확인됩니다.
라인 - src/adapters/openai-responses/passthrough.ts 호출 위치: strip이 buildRequest 앞쪽에서 한 번만 돌아갑니다. 지금 테스트가 최종 body를 검사하니 현재는 안전하지만, 나중에 그 아래에서 reasoning.summary를 다시 넣는 코드가 생기면 마커가 다시 샐 수 있습니다.
경로 - #6232: 같은 수정의 이전 PR이 아직 열려 있습니다. 그쪽은 테스트를 passthrough 파일에 넣었고, 이 PR(#6247)은 형제 파일로 옮기고 layout/seeds/structure까지 맞춘 이어받기입니다.

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

#6232를 이 PR에 흡수된 중복으로 보고 닫을지, 아니면 #6232 쪽을 먼저 정리한 뒤 이 브랜치를 다시 맞출지.

너의 추천

#6247을 기준으로 머지하고, 무효화된 #6232는 닫는 쪽을 추천합니다. 코드 수정·테스트 분리·layout 등록·wire 문서가 한 세트라 범위가 더 완전합니다.

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

@lidge-jun
lidge-jun merged commit 48470c3 into dev Sep 29, 2026
30 checks passed
@lidge-jun
lidge-jun deleted the codex/rt6-l3-strip-summary-none branch September 29, 2026 19:12
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