fix(kiro): gate request diagnostics behind the debug check (carry #3837) - #3867
Conversation
… [skip ci] Carries PR #3840 (head 9a21452, 5 commits squashed onto current dev): gpt-6-astra, Grok and MAI models on GitHub Copilot default to the Responses wire instead of Chat Completions, which returned unsupported_api_for_model. Docs scope the per-wire model lists and the discovery fallback test covers legacy overrides. (carried from #3840) Co-authored-by: chilung-cgu <215337896+chilung-cgu@users.noreply.github.com>
`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>
|
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. |
리뷰 · 우선순위 54 / 80이 PR은 기여자 #3837(luvs01)을 메인테이너 릴리스 트레인 레이어 3( 추가로 원본 #3837 리뷰에서 지적된 테스트 고립도 고쳤습니다. 예전에 공유 setup이 현재
테스트 고립 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
2 participants
Summary
Carries #3837 by @luvs01: Kiro request diagnostic payloads are no longer encoded when debug is off. Adds the test-isolation fix requested in the maintainer review (discussion_r3945935220): the test now snapshots and restores
OCX_DEBUG,OCX_DEBUG_FRAMESand the runtime debug override, so an inheritedOCX_DEBUG=1cannot make the encoder-spy assertion fail.(carried/reimplemented from #3837; Co-authored-by trailer in the commit)
Verification
tests/providers/kiro/kiro-stream.test.tsencoder-spy test with isolated debug settings 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 3 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.