fix(runtime): distinguish usage limits from auth errors - #2521
Conversation
ec8c6f3 to
4ffeea9
Compare
4ffeea9 to
b1e26b6
Compare
6598eb0 to
083ba9b
Compare
Preserve the original apache#2521 implementation history while accepting apache#2675 and current main as the canonical Runtime Host diagnostic path. The follow-up commit reapplies only the remaining classification and presentation changes.
083ba9b to
ce27694
Compare
|
After #2675 landed, I went back through this branch and rebuilt it on the current #2675 fixed the diagnostic path itself: it preserves an allowlisted/redacted/bounded provider summary at ModelAdapter, records the terminal message in the canonical RuntimeEvent, carries it through Runtime Host failed-Turn snapshots and recovery, and attaches the existing execution trace to Desktop error reports. I dropped this PR's older parallel Runtime Host, Headless, continuity-schema, and diagnostics changes instead of resolving those conflicts in favor of duplicated code. The part still missing was the meaning assigned before that path runs. Current This revision keeps Kimi's generic I added the observed Kimi envelope as a provider-boundary fixture and a TUI projection test. Local verification passed: Runtime 3,316 tests (3 skipped), CLI 352 tests, Desktop typecheck/checks and the targeted 10-test matrix, plus repository lint. I did not repeat a live request against an exhausted Kimi account, so that remains the real-account verification boundary. 中文对照#2675 合并后，我重新检查了这个分支，并基于当前 #2675 已经解决的是“诊断如何安全传到界面”：ModelAdapter 会保留经过白名单筛选、脱敏和长度限制的 provider 摘要；terminal RuntimeEvent 记录 canonical 失败消息；Runtime Host 将它带入失败 Turn snapshot 和恢复流程；Desktop 复制错误报告时会附上已有的 execution trace。因此我删除了本 PR 旧版本中并行实现的 Runtime Host、Headless、continuity schema 和 diagnostics 改动，没有为了消除冲突而保留重复代码。 还缺的是这条链路之前的语义分类。当前 这一版不猜测 Kimi 的通用 我加入了实际观察到的 Kimi envelope fixture 和一项 TUI 投影测试。本地验证通过：Runtime 3,316 项（3 项跳过）、CLI 352 项、Desktop typecheck/checks 与 10 项针对性矩阵，以及全仓 lint。没有再次用一个已耗尽额度的真实 Kimi 账户发请求，因此真实账户端到端验证仍是当前边界。 |
ce27694 to
f675515
Compare
|
One concrete before/after for the Kimi Code Plan case: Before this PR, the observed After this PR, Kimi's generic This deliberately does not classify the failure as 中文对照Kimi Code Plan 这个真实场景的前后对比如下。 本 PR 之前，实测 本 PR 之后，Kimi 的通用 这里不会因为 message 中出现 “usage limit” 就把错误分类成 |
f675515 to
ae1d994
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Codex automated review
I reviewed exact head ae1d994dbd83a375773e4702310c7180ad9712e1 across provider-owned structured evidence, retry metadata, ModelAdapter projection, TUI/Desktop presentation, recovery actions, and connection-test boundaries. The stronger structured classification and account-state retry policy are useful, but two active Desktop surfaces still reclassify the exact Kimi shape incorrectly; see the P2 inline findings.
This is a cohesive classification-and-presentation slice with green checks, so I do not recommend splitting it or deleting the focused provider fixtures. A follow-up should also consider reusing this authority in connection testing, which still maps every 403 to needs_reauth, but I am not assigning that untouched scope a separate P-level finding here.
Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.
ae1d994 to
1fd42a9
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for tightening the provider classification and addressing the earlier Desktop permission-label issue. I reviewed the current head, including the Runtime Host projection and connection-test paths. Two remaining gaps appear to share the same root cause: the structured provider meaning is established in ModelAdapter, but it is not preserved through every consumer, so downstream paths infer it again from HTTP status or event shape.
[P2] Preserve the provider summary through the real Runtime Host projection
ModelAdapter emits the observed Kimi failure with code: "permission_error" and a bounded provider summary. However, the canonical failed TurnSnapshot stores only failureClass and failureMessage, and RuntimeHostSessionProjector reconstructs the error with reason: root.failureClass but no code.
The real Desktop path therefore receives reason: "permission_error" and no code. sessionEventErrorMessage() skips the new event.code branch and falls back to the generic conversation-error copy, so the provider explanation still does not reach Desktop. The new Desktop test passes because it constructs an event containing code directly, which the Runtime Host terminal projection does not produce.
Could we carry explicit provider-summary provenance through the Runtime Host snapshot and projection, then cover the full ModelAdapter → failed TurnSnapshot → RuntimeHostSessionProjector → Desktop path? Desktop should render the upstream message directly only when that boundary has explicitly marked it as a safe, bounded provider summary.
[P2] Remove the parallel status-only classifier from connection testing
The connection-test path still maps every HTTP 401/403 to auth, and the connection-effect coordinator persists that result as needs_reauth. Desktop Settings and the command-palette presentation also continue to treat a 403 as an authentication problem.
With the same Kimi envelope covered by this PR, a normal model turn is now neutral and non-retryable, while Settings → Test connection still tells the user to re-authenticate. Could connection testing consume the same provider classification authority, with a regression proving that this Kimi 403 does not become needs_reauth?
The smallest complete shape I see is one structured provider-failure result—semantic class, retryability, stable code, safe summary, and provenance—produced by the runtime owner and carried through Runtime Host and connection testing. Presentation layers can then render that result without maintaining additional status or text classifiers.
Separately, the current head conflicts with main in packages/cli/src/__tests__/pi-transcript.test.ts. The existing green checks ran against the older base, so the affected checks will need to run again after the conflict is resolved.
Focused verification on the reviewed head:
- Runtime classification and ModelAdapter tests: 36 passed.
- Desktop provider-failure presentation tests: 5 passed, although the test described above does not exercise the real Runtime Host projection.
- The earlier
permission_error→ local permission-label issue is fixed and is not being repeated here.
中文对照
感谢收紧 provider 错误分类，也感谢修复之前 Desktop 将 permission_error 显示为本地权限请求的问题。我沿着当前 head 检查了 Runtime Host 投影和连接测试路径。目前剩余的两个缺口有同一个根因：ModelAdapter 已经建立了结构化的 provider 语义，但这个语义没有贯穿所有消费方，导致下游仍然根据 HTTP 状态码或事件形状重新推断。
[P2] 在真实 Runtime Host 投影中保留 provider 说明
ModelAdapter 会把实测的 Kimi 错误转换为 code: "permission_error"，并附带经过长度限制的安全 provider 摘要。但是，canonical failed TurnSnapshot 只保存 failureClass 和 failureMessage；RuntimeHostSessionProjector 重建错误事件时只写入 reason: root.failureClass，不会保留 code。
因此，真实 Desktop 路径收到的是 reason: "permission_error"，而不是带有 code 的事件。sessionEventErrorMessage() 会跳过新增的 event.code 分支，最终回退到通用错误文案，provider 的具体说明仍然无法抵达 Desktop。新增 Desktop 测试之所以通过，是因为它手工构造了带 code 的事件，但 Runtime Host 的 terminal projection 实际不会产生这种形状。
是否可以让 Runtime Host snapshot 和 projection 显式携带 provider summary 的来源标记，并通过完整的 ModelAdapter → failed TurnSnapshot → RuntimeHostSessionProjector → Desktop 链路覆盖回归？Desktop 只应在上游明确标记消息为安全、限长的 provider summary 时直接展示它。
[P2] 删除连接测试中的平行状态码分类
连接测试路径仍然把所有 HTTP 401/403 映射为 auth，connection-effect coordinator 随后会把结果持久化为 needs_reauth。Desktop Settings 和命令面板也仍然把 403 显示为鉴权问题。
对于本 PR 已覆盖的同一个 Kimi 错误，正常模型调用现在会得到 neutral、non-retryable 的结果，但 Settings → Test connection 仍然会要求用户重新认证。连接测试是否可以消费同一个 provider classification authority，并增加一条回归测试，证明这个 Kimi 403 不会变成 needs_reauth？
我认为最小完整的终态是由 runtime owner 产生一个统一的结构化 provider failure result，其中包含语义类别、是否可重试、稳定 code、安全摘要和 provenance；Runtime Host 与连接测试都传递或消费这个结果。这样展示层不再需要维护额外的状态码或文本分类器。
另外，当前 head 与 main 在 packages/cli/src/__tests__/pi-transcript.test.ts 存在冲突。现有绿灯基于旧 base，解决冲突后需要重新运行受影响的检查。
针对 reviewed head 的 focused verification：
- Runtime classification 与 ModelAdapter：36 项通过。
- Desktop provider-failure presentation：5 项通过，但上面提到的测试没有覆盖真实 Runtime Host 投影。
- 之前的
permission_error→ 本地权限标签问题已经修复，这里不再重复提出。
AI-assisted review disclosure: Codex coordinated the source inspection and focused verification on exact head 1fd42a9bc3aca92214d9397efeeeaeac3b37bac9, with Claude Opus 5 providing an additional adversarial pass. The human reviewer examined the evidence, independently evaluated the findings, and owns the final review judgment.
1fd42a9 to
2b90ae3
Compare
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWhat this PR solvesThe PR distinguishes authentication, permission, billing, usage-limit, rate-limit, and availability failures. Structured provider identifiers take precedence over HTTP status codes. Bare HTTP 403 responses remain unknown and non-retryable. Existing bare-429 behavior remains unchanged. The change prevents Kimi Source of truth and solution scopeThe PR extends the shared provider-error classification and ModelAdapter path. It does not create a parallel diagnostic path.
The solution is the smallest coherent change supported by the diff. The added fields, protocol validation, presentation mappings, and tests preserve classification and bounded-message metadata across connection tests and failed turns. No safe deletion or simplification is evident without weakening regression coverage or message-safety validation. Validation and risksTests cover structured provider identifiers, nested causes, Kimi plan-limit responses, permission errors, usage limits, billing, authentication, rate limits, availability, retry behavior, bare 401/403 responses, bounded-message propagation, Runtime Host protocol validation, failed-turn projection, TUI, Desktop, CLI, and connection-test presentation. Reported verification passed for Runtime, Runtime Host, Desktop, typecheck, formatting, diff checks, and reverse-mutation checks. No live request was made against an exhausted Kimi account. Unrelated Computer Use test-harness flakes were reported in CI and tracked separately in issue Complexity deltaThe PR adds:
The PR removes:
The shared authority replaces distributed status-based decisions. The new branches and fields preserve required behavior across several surfaces. The tests increase maintenance work but protect classification precedence, retry behavior, protocol validation, and safe presentation. Total maintenance complexity stays justified. Review-relevant risks
Required checks remain unverified from the available evidence. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe change adds shared provider-failure classification. Runtime, Runtime Host, Desktop, and CLI preserve structured failure details, bounded provider messages, localized explanations, and recovery actions. ChangesProvider failure flow
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to The PR improves provider-limit classification, but current failure paths can still misclassify some status-wrapped errors and replace localized CLI/Desktop guidance with less appropriate copy. Merge readiness remains moderate until these bounded correctness and presentation issues are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Provider
participant Runtime
participant RuntimeHost
participant Desktop
Provider->>Runtime: error response with status and provider code
Runtime->>Runtime: classify provider failure and bound message
Runtime->>RuntimeHost: failed turn or connection result
RuntimeHost->>Desktop: validated providerFailure metadata
Desktop->>Desktop: localize class or render bounded message
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
37713c9 to
622bc7e
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed exact head 622bc7e2ab3681b47280543b1343e3d3b9572ba6, including the full diff, green CI, previous reviews and replies, and the complete Runtime → Runtime Host → Desktop projection.
The problem definition is correct: structured provider identifiers are stronger evidence than ambiguous HTTP status codes, and account-state failures should not be retried as transient throttling. The earlier findings about preserving the bounded provider result through Runtime Host and removing the connection coordinator’s status-only classifier are fixed on this head.
Two P2 gaps remain, with the same root cause: provider failure meaning still has multiple authorities.
-
Desktop connection-test presentation still re-infers meaning from status and message text. A result with
statusCode: 403anderrorClass: "unknown"is still rendered as authentication failure by Settings and the command-palette path; reparsing the error message repeats the same mistake. The 429 path also returns before reading the structured response body, sousage_limit_reachedandinsufficient_quotaare collapsed into transient rate limiting. -
Durable provider diagnostics still let HTTP status override structured meaning.
permission_denied + 403becomesAuth,usage_limit_reached + 429becomesRateLimit, and the new classes fall toOtherwithout a status. That makes Runtime behavior and persisted execution telemetry disagree about the same failure.
The smallest complete fix is one structured provider-failure result—semantic class, retryability, status, stable code, bounded summary, and provenance—produced by the provider boundary and consumed without downstream reclassification. UI presentation and durable diagnostics should project that result rather than rebuilding taxonomy from HTTP/text.
I do not recommend splitting the PR. Once those remaining consumers use the same authority and focused regressions cover the real Settings and diagnostic paths, this should be ready.
Disclosure: Codex performed the read-only source, contract, lifecycle, test, CI, and prior-feedback analysis. The human contributor remains responsible for verifying the findings and the final merge decision.
中文
问题定义正确，但 provider failure 仍有多个 authority：Settings/命令面板继续把 neutral 403 显示成鉴权失败，429 跳过结构化额度信息；durable telemetry 又把 permission/usage 改写为 Auth/RateLimit。应由一个结构化结果贯穿所有消费者。
|
Addressed the latest provider-failure findings in b529c3f. A shared ProviderFailureResult is now the single structured authority from Runtime normalization through connection tests, Host protocol, durable telemetry, Settings, and the command palette. Structured provider meaning outranks HTTP fallback; neutral 403 is not relabeled as auth, usage_limit_reached + 429 remains UsageLimit, and only explicitly bounded provider summaries are displayed. Runtime, Runtime Host, and Desktop full suites pass sequentially; all-workspace typecheck, Biome, diff checks, and two reverse mutations pass. simplify-audit found no P0-P2 issues. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/runtime/src/__tests__/provider-error-classification.test.ts (1)
510-527: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test restates the
errorPresentationFromClassswitch.The four assertions duplicate the constant table in
provider-error-classification.tslines 704-730. It protects no observable behavior beyond the mapping itself, and it must be edited whenever the copy changes. The account-state coverage in the tests above already pins the classes.Disposition: optional. Delete this test, or fold the reason/message expectation into the surface test that renders it.
Based on path instructions: "Flag tests that duplicate existing coverage, assert implementation details, or do not protect observable behavior."
Source: Path instructions
packages/runtime/src/__tests__/provider-conformance.test.ts (1)
617-638: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the bare-401 case to pin the message policy.
This test asserts only
statusCodeanderrorClass. It does not assertresult.errorMessageorresult.providerFailure, so it does not catch that an empty body now yields'Provider request failed (status=401)'marked as a bounded provider message.Disposition: optional, and it becomes useful once the bounded-message policy is settled. Add an assertion that a body-less failure carries no
boundedProviderMessage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bad536a-3edc-48b3-bc6d-178ed5ebbd12
📒 Files selected for processing (36)
apps/desktop/src/main/__tests__/provider-failure-presentation.test.tsapps/desktop/src/main/__tests__/runtime-host-connections-ipc-main.test.tsapps/desktop/src/main/runtime-host-connections-ipc-main.tsapps/desktop/src/renderer/app-shell-copy.tsapps/desktop/src/renderer/locales/conversation-copy.tsapps/desktop/src/renderer/model-connection-errors.tsapps/desktop/src/renderer/session-error-presentation.tsapps/desktop/src/renderer/session-status-presentation.tsapps/desktop/src/renderer/settings/provider-panel-shared.tspackages/cli/src/__tests__/pi-transcript.test.tspackages/core/package.jsonpackages/core/src/events.tspackages/core/src/llm-connections.tspackages/core/src/provider-failure.tspackages/core/src/runtime-event.tspackages/runtime-host/src/__tests__/canonical-session-projection.test.tspackages/runtime-host/src/__tests__/connection-effect-coordinator.test.tspackages/runtime-host/src/__tests__/connection-effects-protocol.test.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/__tests__/session-projector.test.tspackages/runtime-host/src/adapter/session-projector.tspackages/runtime-host/src/protocol/connection-effects.tspackages/runtime-host/src/protocol/turn.tspackages/runtime-host/src/server/canonical-turn-snapshot.tspackages/runtime-host/src/server/connection-effect-coordinator.tspackages/runtime/src/__tests__/ai-sdk-flow.test.tspackages/runtime/src/__tests__/model-adapter.test.tspackages/runtime/src/__tests__/provider-conformance.test.tspackages/runtime/src/__tests__/provider-error-classification.test.tspackages/runtime/src/__tests__/provider-request-telemetry.test.tspackages/runtime/src/ai-sdk-flow.tspackages/runtime/src/connection-effect-outcome.tspackages/runtime/src/model-adapter.tspackages/runtime/src/model-protocol.tspackages/runtime/src/provider-error-classification.tspackages/runtime/src/test-connection.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the latest update. It fixes the earlier raw Kimi 403 exposure, routes adapter events through normalizeProviderFailure, and adds useful bounded-message coverage. One independent reviewer and a separate read-only DeepSeek V4 Flash high-effort pass both reproduced the cause-chain issue below; they also agreed that presentation should consume the Runtime-owned result without adding a second eligibility policy.
The problem definition is right: Runtime should be the single authority that turns provider/SDK failures into a safe, durable result. The current implementation is close, but providerFailureDiagnosticFacts() still selects one object from the cause chain instead of merging the strongest facts across it. A transport wrapper carrying HTTP 403 can therefore hide an inner structured usage_limit_reached, permission_denied, or context_length_exceeded code. That changes both the durable class and recovery behavior.
From first principles, collect a bounded evidence record across the cause chain, with structured provider semantics outranking wrapper transport status and status remaining a fallback. Then project one complete ProviderFailureResult—reason/class, retryability, bounded message, and optional code—and let CLI/Desktop render that contract directly. This removes the current duplicated presentation gates and is simpler than teaching each surface which subset of the result is safe.
I did not run a local test suite; this review is based on the latest head, focused static tracing, current green CI, one independent @reviewer pass, and a read-only ollama-cloud/deepseek-v4-flash high-effort review. AI-assisted review; I verified the reported paths and classifications.
中文评论
感谢更新。最新版本已经修复了此前 Kimi 403 原文暴露的问题，adapter event 也统一走 normalizeProviderFailure，并补了 bounded message 测试。
剩余核心问题是 cause chain 仍然只选择其中一个对象，而不是合并整条链上的最强证据。外层 403 transport wrapper 会遮住内层的 usage_limit_reached、permission_denied 或 context_length_exceeded，从而改变持久化分类和恢复策略。
更符合第一性原理的方案是：Runtime 在整条 cause chain 上生成一个有界 evidence record，结构化 provider 语义高于外层 HTTP 状态，状态只作为 fallback；随后投影完整的 ProviderFailureResult，CLI/Desktop 直接消费，不再各自增加安全显示资格判断。
本次未运行本地测试；结论来自最新 head 的静态追踪、当前绿色 CI、一个独立 reviewer 和一次只读的 DeepSeek V4 Flash high-effort 审查。AI 辅助审查；我已人工复核相关路径与分类结果。
f85faa3 to
94aac3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b583ce7-082b-4071-8fb3-e29b34fd144a
📒 Files selected for processing (17)
apps/desktop/src/main/__tests__/provider-failure-presentation.test.tsapps/desktop/src/renderer/model-connection-errors.tsapps/desktop/src/renderer/session-status-presentation.tspackages/cli/src/__tests__/pi-transcript.test.tspackages/cli/src/pi-transcript.tspackages/core/src/__tests__/runtime-event.test.tspackages/core/src/runtime-event.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/__tests__/session-projector.test.tspackages/runtime/src/__tests__/model-adapter-onerror.test.tspackages/runtime/src/__tests__/model-adapter.test.tspackages/runtime/src/__tests__/provider-conformance.test.tspackages/runtime/src/__tests__/provider-error-classification.test.tspackages/runtime/src/model-adapter.tspackages/runtime/src/network/__tests__/scoped-fetch-transport.test.tspackages/runtime/src/provider-error-classification.tspackages/runtime/src/test-connection.ts
💤 Files with no reviewable changes (1)
- packages/runtime/src/tests/model-adapter-onerror.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/core/src/runtime-event.ts
- packages/runtime-host/src/tests/protocol.test.ts
- apps/desktop/src/renderer/session-status-presentation.ts
- packages/runtime-host/src/tests/session-projector.test.ts
- packages/runtime/src/tests/model-adapter.test.ts
- packages/runtime/src/tests/provider-conformance.test.ts
- apps/desktop/src/main/tests/provider-failure-presentation.test.ts
- packages/runtime/src/test-connection.ts
- packages/runtime/src/model-adapter.ts
- apps/desktop/src/renderer/model-connection-errors.ts
- packages/runtime/src/provider-error-classification.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
Astro-Han
left a comment
There was a problem hiding this comment.
The direction is sound: a single structured provider-failure authority is the right seam, and the UI consumers now preserve the more specific account and request semantics. I found two protocol-boundary defects that need to be resolved before this is safe to merge. The smallest solution is to bump the compatibility epoch for the new strict wire shape and make the producer obey the decoder's message-provenance invariant.
AI-assisted review disclosure: Codex verified the final findings against the current head, protocol decoders, producers, consumers, and focused tests. Two independent reviewer-agent passes and an OpenCode Go DeepSeek V4 Flash (high) adversarial pass were used as inputs; unsupported claims were discarded. No local tests were run.
中文复核
整体方向正确：把 provider failure 收敛为单一结构化权威是合适的扩展点，新的 UI consumer 也确实保留了更细的账户和请求语义。但当前有两个协议边界问题：新增严格 wire 字段却没有提升兼容 epoch；producer 还会生成 decoder 明确拒绝的 message provenance 组合。最小修复是提升 epoch，并让 producer 与 decoder 的 provenance 约束一致。
本次为 AI 辅助审查：Codex 在最新 head 上核验协议 decoder、producer、consumer 与相关测试；另使用两次独立 reviewer 审查及一次 OpenCode Go DeepSeek V4 Flash（high）对抗审查，未被代码支持的指控已剔除。未运行本地测试。
|
/agentic_review |
Code Review by Qodo
1.
|
Astro-Han
left a comment
There was a problem hiding this comment.
The error taxonomy is much stronger on this head: compatibility epoch, bounded-message transport, cause-chain aggregation, structured account states, neutral 403 handling, and Desktop/TUI presentation are all aligned.
I’m leaving COMMENT for one remaining precedence bug. The implementation comments say a provider's structured context-overflow code is unconditional evidence and is checked before the text layer, but the actual branch runs abort wording and numeric 402/429/401 first. That can turn a real context_length_exceeded envelope into Abort or RateLimit and bypass compaction/recovery. I left the focused P2 inline.
AI-assisted review disclosure: two Codex reviewers independently reviewed exact head f29054b; Codex resolved their differing conclusions by tracing the actual classification order.
中文说明
当前错误分类体系的大部分边界已经统一。剩余一个优先级 P2：代码注释声明结构化 context-overflow code 是强证据、应早于文本判断，但实际 abort 文本和 402/429/401 状态先返回，可能把真正的 context overflow 误分成 Abort/RateLimit，从而跳过压缩恢复。建议调整判断顺序并补组合 envelope 回归测试。
|
This PR changes user-visible provider error presentation in Desktop and the TUI. Could you please add screenshots of at least one affected usage-limit or provider-permission state, showing the revised message and recovery action? A compact Desktop and TUI composite is welcome. Please sanitize provider and account details. Thanks! Posted by Codex on behalf of Astro-Han. |
|
Current-main integration update: rebased and advanced the provider-failure wire change to compatibility epoch 27 on head d81e93a. Local build, affected full suites, full typecheck, Biome, and diff checks pass. All review threads are resolved; ready for maintainer re-review once the new CI completes. |
|
CI retrigger request after the repository migration: the exact-head runs began while the repository was moving to apache/maka. Checkout failed with repository disabled / HTTP 403 before code execution, and the sibling runs were cancelled. I attempted failed-run reruns for CI, CLI packaging, dependency audit, Windows baseline, and Windows recovery, but the apache/maka API requires repository admin rights; this account has pull-only permission. Please rerun the exact-head workflows for d81e93a. The local current-main verification reported in the prior remediation comment remains green. No head change or merge action was made. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the compatibility-epoch collision is fixed on d81e93ac3b5b40f7d3e2dc9dbb4b76f5e3a70efb. I verified that this branch includes main’s epoch-26 change, advances the structured provider-failure wire contract to epoch 27, adds the corresponding > 26 regression, and integrates cleanly with current main. I found no P0–P2 code issues.
I’m leaving COMMENT rather than APPROVE only because the exact-head required checks did not execute: checkout failed during the repository migration with “Repository disabled” / HTTP 403, and the sibling workflows were cancelled before testing the code. I have now rerun the cancelled workflows for this same SHA. If they pass without a head change, this is ready for approval.
Small provenance cleanup: please extend the PR-body AI scope to mention that Codex also authored the epoch-27 compatibility remediation.
AI-assisted review disclosure: OpenAI Codex performed the exact-head rebase, protocol-epoch, review-thread, provenance, screenshot, and CI analysis; I verified the cited behavior and live GitHub state before posting.
中文说明
当前 head 已正确解决 compatibility epoch 冲突：分支包含 main 的 epoch 26，并将本 PR 的严格 wire 变化推进到 epoch 27；相对当前 main 的虚拟合并也保持为 27，没有发现 P0–P2 代码问题。
暂不批准的唯一原因是 exact-head CI 在仓库迁移期间 checkout 就因仓库 disabled / HTTP 403 失败，其余 workflow 在运行代码前被取消。我已经对同一 SHA 重跑；绿灯后即可批准。PR body 的 AI scope 也建议补充说明 Codex 完成了 epoch-27 compatibility remediation。
Astro-Han
left a comment
There was a problem hiding this comment.
Status update at the same head d81e93ac, no re-review of the code.
I said previously that the only thing keeping me from approving was the exact-head CI failing at checkout with a repository-disabled / HTTP 403 during the migration, and that I would approve once it went green. It has since re-run, and the outcome is different from what I expected: test and test_workspaces now report failure — a real result rather than an infrastructure error. Everything else on this head is green (typecheck, e2e, storybook, audit, astryx_surface, test_runtime_host, all five installed-CLI validation lanes, the Windows lanes).
So the blocker has changed rather than cleared, and it is no longer something a re-run fixes. My code findings are unchanged — I still found no P0–P2 in the diff — but please look at those two lanes; if they are failing for reasons unrelated to this branch, say so and I will approve on that basis.
d81e93a to
34745e3
Compare
|
Rebased onto latest main ( Rebuilt workspace deps and re-ran all affected suites on the rebased branch:
Note: an initial run showed stale- |
34745e3 to
79b1b3b
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
COMMENT. One P1, four P2s and a P3 inline, plus a process point I think matters more than any single finding.
The epoch is right: protocol/index.ts:75 is 28 against a base of 27, with a comment naming the reason. Watch the merge order, though — #3236, #3199, #3133 and this PR are all taking 28 from a base at 27. The = 28 line merges cleanly because every branch writes identical text and each PR's own lower-bound assertion still passes, so whichever lands second silently shares an epoch with the first. I filed #3313 about removing the class; until then this needs a human check at merge time. This branch's epoch has already collided with main twice (25→26, then 26→27), so a third is likely.
The PR body no longer describes the diff. It says "The production change stays at the shared ModelAdapter classification seam; the TUI change is a projection test, not a Kimi-specific branch," and that the Runtime Host changes "were removed because #2675 now owns those responsibilities." At 79b1b3b0e this PR changes production code in packages/core (two new fields on ErrorEvent/RuntimeEventErrorContent, a new provider-failure module and an export-map entry), five Runtime Host protocol and server files, the compatibility epoch, seven Desktop main and renderer files, and packages/cli/src/pi-transcript.ts (+72/−11, including a new hand-rolled zh/en copy table). The verification section still refers to "epoch-27 remediation" while the head is at 28. A committer reading the body will not be reviewing what the diff does, and under our baseline the reviewer of record is accountable for that gap. Please bring the body back in line before asking for the approving review.
Related, and a maintainer call rather than a finding: per AGENTS.md an intent is what one revert must undo, and this bundles a classifier rewrite, a core protocol field, a Host wire change plus epoch bump, a Desktop presentation refactor, and a new CLI localization mechanism. Reverting the classification change alone is not possible. Also, pi-transcript.ts:1751 is now the third copy of the error-class copy table, after errorPresentationFromClass and apps/desktop/src/renderer/locales/conversation-copy.ts. The earlier thread established that the CLI needs locale-aware copy; it did not establish that the CLI should own a private table, and a shared @maka/core copy module is the closer seam.
Two smaller things not worth their own threads:
- [P3] Reading the 429 body is now unconditional.
packages/runtime/src/test-connection.ts:438— the deleted 429 branch calledawait r.cancel()before returning;httpFailurenow always callsreadText, which can throwConnectionEffectInvalidResponseError('Provider response body exceeded its limit')or a fetch timeout (connection-effect-fetch.ts:123-131, 151-172). A provider returning an oversized or slow 429 body turns a clean rate-limit result into a throwninvalid_response. Cheap guard, easy to lose track of. - [P3] Dead branches.
session-status-presentation.ts:123-124—lower === 'provider_permission'andlower === 'usage_limit'are unreachable, becausedescribeSessionErrorReason(errorClass)at:114already returns for both exact strings; only a case-variant could reach them. The new'check_account'FailedTurnRecoveryActionvariant at:142is never read — the sole consumer (app-shell-turn-view-model.ts:192) uses.labeland discards.action. Andmodel-adapter.ts:932'snormalizeModelFailurenow has exactly one caller and is otherwise a duplicate ofnormalizeProviderFailure.
Things I checked and am not filing, so nobody re-raises them:
- Bare-429 retry behaviour is genuinely unchanged:
{ statusCode: 429 }givesRateLimitandretryable: false, and withretry-after: 30givesretryable: true, retryAfterMs: 30000. - Removing
errorMessagefromhttpFailureis a security improvement, not a regression —scoped-fetch-transport.test.ts:380shows the old code surfacing the raw provider body throughConnectionTestResult.errorMessage. - The Maka-authored
errorMessagestrings Desktop now ignores were already being swallowed bygeneralizedErrorMessage's fallback, so no user-visible copy was lost. The removed Chinese OAuth 429 copy is likewise not a regression;generalizedErrorMessageChinesealready collapsed it to the same string. - Cause-chain walking is depth-bounded at 4 and cycle-safe, and message and field truncation hold at 2048 and 256 bytes across multi-byte input.
- The new wire fields all pass through
requireShapedRecord/requireExactRecordcorrectly, with producer bounds strictly inside decoder bounds.
Two more test gaps beyond the one inline: provider-request-telemetry.test.ts:994 replaced the rate_limit_exceeded + 429 durable-telemetry fixture with usage_limit_reached rather than adding alongside it, so the previous durable expectation is no longer asserted anywhere in that file; and provider-error-classification.test.ts pins classifyError(providerError(403, …)) === 'AI_APICallError', which asserts a leaked SDK class name rather than the contract — if the intent is "a bare 403 is neutral", assert providerFailureResult(...).errorClass === 'RequestRejected'.
biome format is clean on every changed file the formatter actually sees. The one file that differs, apps/desktop/src/main/__tests__/provider-failure-presentation.test.ts, is excluded by formatter.includes: ["!apps/desktop/**"] in biome.jsonc, so CI will not see it either.
AI disclosure: this review was assisted by Claude (Opus), which performed the initial code search and cross-checking, including running parts of the classifier in isolation. Every finding published here I re-derived myself by reading the source at 79b1b3b0e, and where I could only confirm the structure rather than a witnessed failure I have said so in the finding.
Keep ambiguous 403 responses out of authentication classification, preserve the bounded provider explanation added by apache#2675, and use structured provider identifiers for stable account-state meanings. Generated-by: Maka
Generated-by: Maka
Mark ModelFailure messages taken from the provider-failure summary as bounded, display-safe provider wording and carry the marker through the durable error content so presentation layers never render unbounded transport text. Connection testing previously kept a parallel status-only classifier that mapped every 401/403 to auth. Route probe failures through the shared provider-failure authority instead: a Kimi 403 carrying a permission envelope stays neutral rather than demanding re-authentication. The durable diagnostic also prefers the status-bearing cause over an SDK wrapper whose transport code would otherwise shadow the real HTTP status. Generated-by: Maka
Failed turn snapshots now carry the stable failure code and the bounded provider-summary marker from the canonical terminal error fact, and the session projector forwards both to the projected error event. Desktop can therefore render the bounded provider wording without re-deriving meaning from HTTP status or message text. Generated-by: Maka
The error-toast fallback now requires the bounded provider-summary marker before showing a message raw. A bare code is no longer enough, since Node transport codes carry unbounded internal text. Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Codex
Generated-by: Codex
- Require positive provider provenance before certifying a chain link's message as a bounded provider message: a plain-object cause whose only message is its own .message is internal text, never provider wording. - Rank numeric status above free text in classification: a 429/500 whose body or JSON key names mention "aborted" keeps RateLimit / ProviderUnavailable, preserving retry-after handling. - Treat a text-derived Abort as non-retryable, matching the RetryError early return so the same class no longer carries opposite retry semantics. - Select the projected message and its paired code/status from the same cause-chain link instead of combining an inner link's message with an outer link's provider code. - Gate ErrorEvent.code through a closed vocabulary (semantic provider codes, numeric HTTP statuses, Maka-owned sentinels) so a provider's free-form token can never steer the Host's terminal-state taxonomy or the Desktop label/recovery matching. - Use Object.hasOwn for the provider-influenced notice-copy lookup. - Replace the epoch ladder assertion with handshake compatibility coverage and test both older- and newer-peer epoch rejection. Generated with AI assistance
14541bf to
e1e8ab4
Compare
|
Rebased onto current main; new head On the
PR body rewritten per your process point: it now describes the actual diff at this head (core 中文说明已 rebase 到最新 main（epoch 因 main 占用 28 而推进到 29，保留两边注释）。你 09:10 问的两个失败 lane 与本分支无关：唯一真实失败是 test_workspaces 里 ToolRuntime sandbox boundary 套件的一个用例，本 PR 未触碰该模块；同 head 其他 lane 重跑全绿，本地六个套件全绿；main 同期也在以不同用例间歇性变红。PR body 已按当前真实 diff 重写，AI scope 已补全 Codex 的 epoch-27 修复与后续 Maka 的 epoch 28→29 rebase。 |
Summary
After #2675, Maka already has one safe path for provider diagnostics: ModelAdapter keeps an allowlisted, redacted, bounded summary; the terminal RuntimeEvent remains canonical; Runtime Host projects the message through failed Turn snapshots and recovery; and Desktop can include the exact execution trace in copied reports.
This PR fills the remaining classification gap on top of that plumbing:
context_length_exceededenvelope reaches compaction/recovery instead of being misread as Abort or RateLimit;permission_errorremains unknown and non-retryable, so the bounded provider explanation reaches TUI/Desktop instead of being replaced withAuthentication failed;permission_denied,usage_limit_reached, andinsufficient_quotaproduce distinct non-retryable account meanings;Scope, accurately at the current head:
packages/core: two new fields onErrorEvent/RuntimeEventErrorContentfor the structured provider failure, a newprovider-failuremodule, and its export-map entry.packages/runtime: the classification precedence fix at the shared ModelAdapter seam, provider-failure provenance on connection-effect outcomes, and conformance/classification regression coverage.packages/runtime-host: Turn snapshots and connection effects carry the structured provider failure code and the bounded provider message (protocol, canonical turn snapshot, session projector); the compatibility epoch advances to 29 because older peers cannot safely preserve those fields.apps/desktop: main-process and renderer presentation of the account-limit vs. authentication vs. unknown states, including the allowance/plan/reset recovery guidance, in both locales.packages/cli:pi-transcriptrenders the bounded provider message through a small zh/en copy table.The branch was rebuilt on current
main. Older Runtime Host, Headless, continuity-schema, and diagnostics changes were removed earlier because #2675 now owns those responsibilities.Fixes #2516
Verification
npm --workspace @maka/core test— all passnpm --workspace @maka/storage test— all passnpm --workspace @maka/runtime test— all pass (provider classification, conformance, telemetry)npm --workspace @maka/runtime-host test— all pass (incl. protocol + handshake-compatibility epoch assertions at epoch 29)npm --workspace @maka/cli test— all pass (incl.pi-transcriptzh/en projection tests)apps/desktopmain-process suite — all pass (incl. provider-failure presentation matrix)npm run lint, Biome formatting, andgit diff --check origin/main...HEADI did not repeat a live request against an exhausted Kimi account. The provider-boundary tests replay the observed HTTP 403 envelope and exact message, and the TUI/Desktop tests verify the resulting Runtime Host error message is rendered verbatim.
Checklist
Does this PR entail a change in behavior?
中文说明
#2675 合并后，provider 诊断已经有了一条统一且安全的链路：ModelAdapter 保留经过白名单筛选、脱敏和长度限制的摘要；terminal RuntimeEvent 仍是 canonical fact；Runtime Host 将消息投影到失败 Turn snapshot 和恢复流程；Desktop 复制错误报告时可以附上精确的 execution trace。
本 PR 在这条链路之上补齐剩余的分类缺口：
context_length_exceededenvelope 会进入压缩/恢复流程，而不是被误分为 Abort/RateLimit；permission_error保持为不可重试的 unknown，因此 TUI/Desktop 会显示经过边界处理的 provider 说明，而不是替换成Authentication failed；permission_denied、usage_limit_reached、insufficient_quota等稳定标识会得到不同的、不可重试的账户状态语义；当前 head 的真实改动范围：
packages/core（ErrorEvent/RuntimeEventErrorContent 两个新字段、provider-failure 模块及 export-map 入口）、packages/runtime（ModelAdapter 分类优先级修正、connection-effect provenance、分类/conformance 回归）、packages/runtime-host（Turn snapshot 与 connection effects 携带结构化 provider failure code 和有界 provider 消息；兼容 epoch 推进到 29）、apps/desktop（账户限额/认证/未知三类状态的主进程与渲染层呈现，双语言恢复引导）、packages/cli（pi-transcript 的 zh/en 渲染）。没有再次消耗一个真实的 Kimi 限额账户发请求；provider 边界测试复用了实际观察到的 HTTP 403 envelope 和完整提示，TUI/Desktop 测试验证 Runtime Host 错误消息会原样显示。
Visual evidence
A sanitized structured usage-limit state in Desktop and TUI. The stable provider identifier is presented as an account limit—not authentication—and Desktop gives the corresponding allowance/plan/reset recovery guidance.
AI use
Tool(s) and scope: Maka authored the classification implementation, tests, verification, documentation, and visual evidence. OpenAI Codex authored the review remediation that keeps structured context-length failures out of generic retry (with its regression test) and the epoch-27 compatibility remediation. Maka authored the subsequent review remediations, the rebases that advanced the epoch to 28 and then 29 after main took each preceding value, this PR-body correction to match the actual diff, and the CI flake analysis.
Final squash trailers:
Generated-by: MakaGenerated-by: Codex