feat(fastwire): let a relay declare its service_tier echo non-authoritative - #6112
Conversation
|
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 configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (21)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughAdds the optional ChangesResponse Tier Authority
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ProviderConfig
participant CoreNormalize
participant FastWire
participant UsageLog
participant CostEstimation
ProviderConfig->>CoreNormalize: response-tier authority setting
CoreNormalize->>FastWire: tier observation context
FastWire->>UsageLog: tier outcome with authority and response tier
UsageLog->>CostEstimation: normalized tier outcome
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue remains before merge; normal checks still apply. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The change makes Fast outcomes and price estimates depend on a provider setting rather than the upstream response alone. The setting does not enable Fast or change what is sent upstream, but an assumed outcome is not proof of the service actually delivered. No new security-boundary bypass was established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 11 files. (10 skipped: 10 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 |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95c59fc8da
ℹ️ 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".
| config.fastMode, | ||
| callerTier, | ||
| isCanonicalOpenAiForwardProvider(route.provider) ? false : undefined, | ||
| responseTierAuthorityForProvider(route.provider), |
There was a problem hiding this comment.
When /v1/chat/completions targets an eligible openai-chat provider, handleChatCompletions exits through handleNativeChatCompletions before this normalizer runs. That path builds its tier decision directly in buildOpenAIChatPassthroughRequest and never calls responseTierAuthorityForProvider or creates tier metadata, so responseTierAuthoritative: false is ignored: the response echo is not retained as assumed evidence and persisted attempts omit the authority flag. Thread this policy through the native Chat request/response path and add focused streaming and non-streaming coverage.
AGENTS.md reference: AGENTS.md:L447-L451
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Traced this and it holds in the narrow sense: the native Chat passthrough never consults the declaration. It also never observes the echo. buildOpenAIChatPassthroughRequest returns no tierLog (src/adapters/openai-chat/passthrough.ts), and neither the JSON path (src/server/chat-native.ts) nor the SSE path (src/server/chat-native-sse.ts) records service_tier into logCtx.responseServiceTier or an attempt outcome. So on that path an echo can't confirm or deny Fast, and pricing falls back to the requested tier (serviceTierContext in src/usage/cost.ts), which is what responseTierAuthoritative: false asks for anyway.
Nothing is mis-recorded there today. Adding response-tier observation to native Chat would be a new feature and is out of scope for this carry. I've noted in the PR description that the flag governs the Responses-path observation, and that native Chat records no response-tier evidence.
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 60 / 80이 PR은 중계 서버가 돌려준 지금 코드는 중계 응답의 공급자 설정에 열려 있는 #5497을 지금 라인 - 라인 - 메인테이너의 판단이 필요한 지점 네이티브 Chat 시도에도 같은 #5497은 아직 열려 있고 지금 이 설정을 끈 공급자는 응답이 너의 추천 바탕은 이 댓글은 grok-bot이 작성했습니다 |
…tative A relay can echo service_tier default (or priority) without that echo proving what the upstream scheduled. Providers may now set the optional boolean responseTierAuthoritative: false so the echo stays in logs as evidence only: an eligible Fast request is recorded as applied/assumed, and pricing never treats the echo, or a confirmed label, as response confirmation. Omitting the field keeps the legacy authoritative reading, canonical ChatGPT forwarding stays observational, and the outbound tier decision and local downgrades are unchanged. Carries #5497. Co-authored-by: BigHulk <happyhls@gmail.com>
95c59fc to
2e83368
Compare
|
Maintainer integration into
|
1 participant
Summary
Some OpenAI-compatible relays echo
service_tierin their responses without that echo proving what the upstream actually scheduled. Today such an echo decides the Fast outcome: a relay that answers"default"to a priority request marks itresponse-declined, and a"priority"echo is recorded, and priced, as confirmed.A provider can now declare
responseTierAuthoritative: false. For that provider the echo stays in logs as evidence only: an eligible Fast request is recorded asapplied/assumed, and pricing uses the requested-tier estimate. Neither the raw echo nor aconfirmedlabel counts as response confirmation.true.service_tier, the tier decision, and local unsupported-route downgrades are unchanged. The field cannot enable Fast."false",0,nulland objects), round-trips through the provider editor without exposing the API key, survives a model rename, and is kept on persisted attempt and final usage records. Old records without it read as before.This carries #5497 by @hulkbig onto current
dev, adapted to the moved code. The only behaviour added beyond the original is that a persistedconfirmedlabel with the flagfalseis priced as a requested-tier estimate.Co-authored-by: BigHulk happyhls@gmail.com
Verification
tests/routing/fastwire-response-authority.test.ts(27 cases: JSON and SSE relay sends,defaultandpriorityechoes withfalse, absent andtruelegacy, canonical forward withtrue, malformed config, editor round trip, persisted attempt and final entries, old records, cost withfalse+confirmed, unsupported Fast route downgrade). Red-green: with the originalcost.tsandcore-normalize.ts10/27 fail; with the change 27/27 pass.95c59fc8da:bun testover the new file plusfastwire-observability,model-rename-migration,provider-config-validation,usage-cost,cost-cap-unknown-evidence,test-layout,test-layout-toolingandfile-size-ratchet— 261 pass, 0 fail.bun run typecheck,bun run structure:check,bun run privacy:scan,bun run skill:surface:check,git diff --check— passed. docs-sitebun install --frozen-lockfile && bun run buildpassed.bun run test:changedselects most of the suite here (core-normalize,usage/logandusage/costare imported widely). In a separate/private/tmpcheckout it hit the local 900 s wrapper limit while seven release lanes shared one test lock: 28,409 passed and 19 failed across six service and native-toggle files. Rerunning those files plus the interruptedcli-status-jsongave 348 pass and 3 fail. The three failures are the launcher shutdown tests, which fail because this machine's real opencodex proxy is running on port 10100, so the test's launcher starts as a second instance. The other 16 passed in isolation. Full-suite coverage is left to hosted CI.Checklist
Summary by CodeRabbit