Skip to content

fix(responses): drop the internal summary:none marker before it reaches the upstream - #6232

Closed
cshyang wants to merge 1 commit into
lidge-jun:devfrom
cshyang:fix/strip-none-reasoning-summary-on-wire
Closed

cshyang wants to merge 1 commit into
lidge-jun:devfrom
cshyang:fix/strip-none-reasoning-summary-on-wire

Conversation

@cshyang

@cshyang cshyang commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Verification

  • bun test tests/responses/openai-responses-passthrough.test.ts -t "omitted thinking display": passes with the change, fails without it (summary: "none" stays on the wire).
  • bun test tests/responses/openai-responses-passthrough.test.ts: 181 pass, 5 fail. The same 5 "byte accounting" cases fail on unmodified dev in my environment, so they are not from this change.
  • bun run typecheck, bun run structure:check, bun run privacy:scan: pass.
  • Manual check on a real install: ocx claude -p ... --model <native gpt-6-astra slot> returned 400 before the patch and 200 with "ok" after.
  • Not run: the full bun run test (the WebSocket steering suites time out in my environment). CI has not run on this branch yet.

Checklist

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

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • Required local validation passed; commands, results, and any full-suite exception are documented.

  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes
    • OpenAI Responses requests no longer send the unsupported reasoning.summary: "none" marker. Other reasoning settings, such as effort, are preserved.

@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
@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ⬜ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@github-actions
github-actions Bot marked this pull request as draft September 29, 2026 08:54
@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.

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: 2ec8e642-ebc5-4e94-9d08-762a4696ec87

📥 Commits

Reviewing files that changed from the base of the PR and between 86ac102 and 2ce99ad.

📒 Files selected for processing (3)
  • src/adapters/openai-responses/passthrough.ts
  • src/adapters/openai-responses/reasoning.ts
  • tests/responses/openai-responses-passthrough.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 now removes reasoning.summary when its value is "none". Other reasoning settings, including effort, remain in the outbound body. Tests cover translated Claude requests and outbound request variants.

Changes

Responses passthrough

Layer / File(s) Summary
Outbound reasoning summary filtering
src/adapters/openai-responses/reasoning.ts, src/adapters/openai-responses/passthrough.ts, tests/responses/openai-responses-passthrough.test.ts
The new helper removes reasoning.summary when it is "none" and removes the reasoning object if no settings remain. The passthrough applies the helper after removing unsupported reasoning-summary delivery. Tests verify that outbound bodies omit the marker while retaining effort, and that a summary-only marker leaves no reasoning object.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 2ce99

Claude’s omitted-thinking behavior is preserved while the unsupported marker is removed from outbound requests; no actionable merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 2ce99

The change removes an internal reasoning marker from outgoing requests without changing request access, credentials, or routing. No new security attack path was identified, although end-to-end coverage is limited.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected exposure is the reasoning field sent through the existing passthrough to its configured upstream; the inspected change adds no ingress route or credential forwarding.

Trust Boundaries and Controls

  • observed — A client-supplied omitted-display setting becomes an internal summary marker. The parser consumes it for client-facing behavior, while the outbound adapter removes it before constructing the upstream POST body.
🚥 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. 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 reasoning.summary: "none" marker before sending requests upstream. This matches the changes in reasoning.ts, `passt…
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • 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.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 65 / 80

Claude Code가 생각 요약을 숨기라고 하면(thinking.display: "omitted"), 번역기가 안쪽 표시용으로 reasoning.summary: "none"을 붙입니다. 파서는 그걸로 hideThinkingSummary를 켭니다. 그런데 그 값이 OpenAI Responses 쪽으로도 그대로 나가면, 업스트림이 none을 거절해서(허용은 auto/concise/detailed) 매 턴이 HTTP 400으로 끝납니다. native GPT로 보낸 Claude 요청이 전부 깨지던 회귀입니다(#5962 / #5984 쪽, thinking: disabled를 다루던 #1426 / #1428과 같은 급).

이 PR은 번역·파서는 그대로 두고, 와이어로 나가기 직전에만 summary: "none"을 빼는 stripNoneReasoningSummary를 넣습니다. 남은 칸이 없으면 reasoning 자체도 지웁니다. passthrough에서 다른 summary 정리 옆에 붙였고, Codex forward와 api.openai.com 둘 다 잡는 회귀 테스트가 있습니다. base는 dev이고, tip 대비 1커밋 ahead·0 behind입니다. 작성자가 해당 테스트·typecheck·실제 설치에서 400→200도 확인했습니다. 아직 draft이고 readiness 체크리스트는 0/4입니다.

라인 - src/server/responses/native-steering-policy.ts createSteeringSettingsNormalizer: 여기는 stripDisabledReasoningSummaries / stripUnsupportedReasoningSummaryDelivery만 돌리고 stripNoneReasoningSummary는 없습니다. 그런데 native-steering-settings.ts는 steering 값으로 reasoning.summary: "none"을 허용합니다. mid-stream으로 그 값이 오면, 같은 400이 다시 날 수 있습니다. 이번 테스트는 첫 buildRequest만 봅니다.

라인 - src/adapters/openai-responses/passthrough.ts: stripNoneReasoningSummary는 mid-pipeline(대략 stripUnsupportedReasoningSummaryDelivery 옆)에만 있습니다. 최종 직렬화의 stripDisabledReasoningSummaries 묶음에는 없습니다. 지금 mid 이후가 summary: "none"을 다시 넣지는 않아서 이번 버그 경로에는 충분해 보이지만, steering이 쓰는 최종 sanitizer 묶음과 어긋납니다.

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

steering에서도 summary: "none"을 내부 hide 마커로 받을지, 아니면 wire enum만 허용하고 none은 거절할지. 받을 거면 createSteeringSettingsNormalizer에도 같은 strip이 필요합니다. 이 수정과 겹치는 열린 중복 PR은 없습니다. types.ts/config.ts 분할 이슈도 아닙니다.

너의 추천

방향은 맞습니다. 머지 전에 steering normalizer에도 stripNoneReasoningSummary를 같은 순서로 넣으세요. 가능하면 passthrough 최종 sanitizer 묶음에도 한 번 더 두어, wire 정리가 한곳에 모이게 하세요. 체크리스트(특히 로컬 검증·ready) 채운 뒤 draft를 풀면 됩니다. preview deploy 이야기는 이 변경과 무관합니다.

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

lidge-jun added a commit that referenced this pull request Sep 29, 2026
Carries #6232 by @cshyang: the internal reasoning summary "none" marker is stripped at final outbound serialization so native Responses upstreams do not reject it.

Co-authored-by: cshyang <cshyang.chng@gmail.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev via the credited carry #6247 (Co-authored-by trailer retained in the squash commit). Thank you, @cshyang.

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