fix(responses): strip internal summary:none marker on the wire - #6247
Conversation
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>
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. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Responses passthrough adapter removes ChangesResponses summary marker handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No merge-blocking issue is established. The internal marker is omitted from outgoing Responses requests while valid reasoning settings remain intact. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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 💡
🧪 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 |
There was a problem hiding this comment.
💡 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".
| The passthrough adapter removes that internal `none` marker at final outbound serialization; | ||
| valid summary values remain on the upstream Responses wire. |
There was a problem hiding this comment.
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 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 14 / 80이 PR은 Claude 쪽에서 온 라인 - 메인테이너의 판단이 필요한 지점 #6232를 이 PR에 흡수된 중복으로 보고 닫을지, 아니면 #6232 쪽을 먼저 정리한 뒤 이 브랜치를 다시 맞출지. 너의 추천 #6247을 기준으로 머지하고, 무효화된 #6232는 닫는 쪽을 추천합니다. 코드 수정·테스트 분리·layout 등록·wire 문서가 한 세트라 범위가 더 완전합니다. 이 댓글은 grok-bot이 작성했습니다 |
Summary
reasoning.summary: "none"as a parser-side hide marker, then remove it from the final upstream Responses request. Valid summary choices remain intact.tests/responses/openai-responses-summary-none.test.tsto respect the passthrough test's file-size cap; register the new file in both layout manifests and its seed rule. Clarify the wire contract instructure/.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.bun run testwas not run locally because four RT6 worktrees share this machine; hosted CI owns the broad suite.Checklist
Summary by CodeRabbit