feat: carry operator-pinned reasoning effort with current config contracts - #3892
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThis change adds global and provider-specific reasoning-effort pins. It validates and sanitizes pin configuration, applies precedence and caps during Chat and Responses normalization, exposes management API updates with rollback handling, adds race-condition coverage, and documents the configuration. ChangesReasoning effort pinning
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)Runtime normalizationsequenceDiagram
participant ChatIngress
participant ResponsesNormalization
participant EffortPolicy
participant ProviderSerializer
ChatIngress->>EffortPolicy: Resolve provider and global pins
ResponsesNormalization->>EffortPolicy: Normalize the destination effort
EffortPolicy->>EffortPolicy: Apply caps and provider mapping
EffortPolicy->>ProviderSerializer: Serialize reasoning_effort
Management updatesequenceDiagram
participant ManagementClient
participant ProviderRoutes
participant ConfigValidation
participant ConfigPersistence
ManagementClient->>ProviderRoutes: Submit pin update
ProviderRoutes->>ConfigValidation: Validate merged configuration
ConfigValidation-->>ProviderRoutes: Return validation result
ProviderRoutes->>ConfigPersistence: Save with rollback state
ConfigPersistence-->>ManagementClient: Return updated configuration
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 58 / 80이 PR은 #3336(Liang-Psych)의 operator-owned reasoning effort pin을 현재 건드리는 면이 넓습니다. src/types/config.ts · src/types/provider.ts (pinned* 필드) - 공개 설정 표면이 늘어납니다. 분할 캠페인 중이라 필드 이름·위치( src/server/effort-policy.ts (resolvePinnedEffort / applyPinnedEffort) - cap 적용 순서가 “pin 후 cap”인지 코드·테스트가 같은 말을 하는지 확인하세요. 본문은 cap이 wire mapping 전에 온다고 합니다. src/server/chat-native.ts · responses/core.ts - native chat와 Responses 양쪽에 pin이 들어가는지, compaction 면제가 한쪽에만 빠지지 않는지 봐야 합니다. 원본 #3336 - 아직 OPEN입니다. 이 PR이 랜딩되면 원본에 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
1bd6ce3 to
e1d9af5
Compare
Allows users to configure an explicit reasoning effort tier (none..max) per model or provider-wide that is forcefully enforced on inbound requests, overriding caller choices or filling missing effort parameters. - types: add pinnedReasoningEffort and modelPinnedReasoningEfforts to OcxProviderConfig and modelPinnedEfforts to OcxConfig - policy: add resolvePinnedEffort and applyPinnedEffort in effort-policy - server: apply pinned reasoning effort override in handleResponses - api: expose and validate pinned reasoning efforts in PATCH /api/providers and PUT /api/effort-caps - tests: comprehensive regression coverage for policy, rewrites, and API (cherry picked from commit 6399c8e) Co-authored-by: Liang-Psych <19504206+Liang-Psych@users.noreply.github.com>
…support native chat path, and key-level merge in PUT\n\n- unify effort validation across policy and routes to use isDeclaredReasoningEffort\n- apply resolvePinnedEffort to handleNativeChatCompletions in chat-native.ts\n- support key-level partial merge and single-key deletions in PUT /api/effort-caps\n- add regression tests covering partial merge in PUT /api/effort-caps [skip ci] (cherry picked from commit db65ff9) Co-authored-by: Liang-Psych <19504206+Liang-Psych@users.noreply.github.com>
…e whitespace in model keys [skip ci] (cherry picked from commit a53597e) Co-authored-by: Liang-Psych <19504206+Liang-Psych@users.noreply.github.com>
…skip ci] Keep pins operator-owned, validate configuration and management writes, preserve omission/default semantics, and apply caps before wire mapping. Carry synthetic/combo effective effort into final destination normalization. Add persistence, DTO, wire and retry regression coverage in domain tests. Source review and centrally scheduled remote CI remain required; no local suite, typecheck, build or installation was run. Co-authored-by: Liang-Psych <19504206+Liang-Psych@users.noreply.github.com>
…ip ci] found by #3336 rebase security audit Re-read provider ownership after parsing the price request body, then merge and save against the current row. Cover replacement and deletion during a deferred body read without weakening existing pricing validation or tests. Co-authored-by: Liang-Psych <19504206+Liang-Psych@users.noreply.github.com>
e1d9af5 to
c145a70
Compare
Keep the pure policy factory unchanged. The two management cases now use a provider that matches defaultProvider under full candidate validation; all success, persistence, deletion and rejection assertions remain intact. Co-authored-by: Liang-Psych <19504206+Liang-Psych@users.noreply.github.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. |
Summary
nonemeans explicit effort omission/provider default, not guaranteed reasoning disablement. Compaction is exempt.Verification
eeb550181ea5167078b11c4f0445bc02d0ae6dac, based ondev@1e188b7874b1bfe6539e85fa91ba2d4ad12ca9eb.git diff --checkpassed. Replacement central CI run 34126879673,lane=all, has all 16 required per-chain jobs SUCCESS at this exact head. The full workflow was cancelled after excluded Windows/control jobs were stopped; its aggregate is not claimed green. Run 34126053072 passed gates but failed two pre-existing API fixtures with a missing default provider; that run was cancelled. The API-only fixture now satisfies the existing schema, with all assertions and production validation unchanged. Independent Averroes RCA/source review: PASS; Darwin and Epicurus rebound their PASS verdicts to this test-only follow-up head.git merge-base --is-ancestorbefore editing transferred core/config/native-chat files. C's subagent/health blocks remain untouched; only the separately granted effort-caps block/imports changed.git cherry-pick -x:6399c8eb05f9f5b2e16af1f499f3bd73a2983f04,db65ff9b3d5245f1efdea88109ae5be737317aa5,a53597edda1598c5d536a970c6fa724b231fba97. Every carry/adaptation commit preserves Liang-Psych credit. Registry-default additions were removed; these are operator settings only.found by #3336 rebase security audit: re-read provider ownership after asynchronous body parsing, then merge/save on the current row. Authentication, price validation and redaction stay unchanged.Manual chain (
stack: null):The original PR is superseded only after verified landing. No merge, release or publication performed.
Checklist
Co-authored-by: Liang-Psych 19504206+Liang-Psych@users.noreply.github.com
Maintainer integration decision (MAINTAINERS.md, dev-only admin integration; config/wire surface → security review attached in body): @lidge-jun integrates #3892 (carry of #3336 by @Liang-Psych, adapted to current config/wire contracts, plus the pricing-PUT race fix found by its rebase audit) into
dev. Exact-head evidence ateeb550181: Cross-platform CI run 34126879673 — 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/macos control deferred to the final train head. Prospective merge tree oforigin/dev@1e188b787+ head =402b8e750= tested tree. Independent CONFIG/SECURITY and WIRE reviews PASS (0 blockers) at this head. Local suites NOT RUN. Maintainer integration, not self-approval.Summary by CodeRabbit
New Features
nonesetting.Bug Fixes
Documentation