Skip to content

fix(kimi): repair 5h-policy test fixture + diagnostic logging for API failures - #15

Merged
linletian merged 6 commits into
mainfrom
fix/kimi-test-policy-and-diagnose-api-failures
Aug 2, 2026
Merged

fix(kimi): repair 5h-policy test fixture + diagnostic logging for API failures#15
linletian merged 6 commits into
mainfrom
fix/kimi-test-policy-and-diagnose-api-failures

Conversation

@linletian

Copy link
Copy Markdown
Owner

背景

app 调 Kimi API 经常失败,但 kimi CLI 的 /usage 始终正常。本分支不修根因,而是落地诊断日志,让下一次真实失败能一次定位到三个候选根因(凭证类型不匹配 / 字段类型严格校验 / UA 限流)中的哪一个。完整调查与处置表见 docs/kimi-api-failures-investigation.md

改动

Commit 内容
fix(kimi-test): align fixture with test name 旧 fixture 没有 limits 块,testUnparseableWeeklyResetTimeDoesNotDeclareFiveHourPolicy 必然失败(没真正测到名字声称的场景);补合法 5h limits 块使测试名实相符。附带 parser else 分支的保守策略注释,无行为变更
chore(kimi): surface response body on HTTP + parse failures NetworkClient 非 2xx 分支(所有 supplier)与 KimiSupplier parser catch 块,记录响应体前 512 字符 + URL + provider,privacy: .public 使 log show 可见。截断先 4 KB 字节窗口解码再按字符切 512,避免切断多字节 UTF-8。成功路径零改动
fix(kimi): drop publicError wrapper, call os.Logger directly os.Logger 强制字面量插值、OSLogMessage 不能转发,publicError 包装器编译不过;改为暴露 AppLogger.osLogger,调用点直调

风险

  • leak 面:失败响应体以 .public 进系统日志,对所有 supplier 生效(含本地读 SQLite 的 OpenCode)。已评估:各 supplier endpoint URL 均为静态、无 query 参数,API key 走 header;Kimi 响应体为 quota 数字无 PII。属诊断专用的有意豁免,根因修复后应回退或收敛到 Kimi 专用,详见文档 §9。
  • 未实机验证:log show 能否真的看到 body= 内容需在真实环境触发一次 Kimi 失败确认。合并前请按文档 §4.1 做一次实机验证(临时换错 Key 强制触发)。

验证

  • Release 构建通过(仅一条既有 SettingsViewModel.swift:157 unreachable-catch 警告,与本分支无关)
  • 实机验证 log show 输出(合并前必做,见文档 §4.1)

Old fixture had no limits block, so the parser always took the
missing-5h else branch and set the retainPrevious policy — the
XCTAssertNil could never pass. Add a valid 5h limits block so the
test actually exercises what its name claims: a bad weekly
resetTime must not turn on the 5h fallback policy.

Also document the conservative retain-previous choice (e462fa3) in
the parser's else branch, and add the Kimi API failures
investigation doc covering the diagnostic-logging plan.
Log the first 512 chars of the response body (plus URL and
provider) on non-2xx responses in NetworkClient and on Kimi parse
failures, via a new AppLogger.publicError that marks interpolations
privacy: .public so log show / Console.app reveal the payload
instead of <private>. Diagnostic-only; success paths unchanged.
See docs/kimi-api-failures-investigation.md.
os.Logger methods require a string-interpolation literal and reject
a forwarded OSLogMessage ('argument must be a string interpolation'),
so the AppLogger.publicError wrapper could not compile. Expose
AppLogger.osLogger instead and let diagnostic call sites invoke
logger.osLogger.error(...) directly; the contract comment moves to
the property. Doc updated to match (the literal requirement makes
the old 'overload not selected' worry in §4.1 moot).
@linletian

Copy link
Copy Markdown
Owner Author

评审总览

本分支实际包含两组不相关的改动

  1. Kimi 诊断日志 + 测试 fixture 修正(PR 描述里列的 3 个 commit)
  2. Copilot overage 卡在 100% 的生产逻辑修复6a85578 / 6d613ac,改了 CopilotResponseParser 的百分比公式 + RefreshServicedisplayUsage,加上 PRD / provider-interfaces 文档更新)

第 2 组占了 diff 的一半以上,且修改了会直接影响用户看到数字的生产代码路径,但 PR 描述的 commit 表格完全没提。调查文档质量很高(根因定位、字段对照表、兼容性分析都到位),fixture 修正也确实让测试名实相符。但下面第 1 条是建议合并前修掉的实质 bug


🔴 需要修复

1. RefreshService 的 fallback 分支仍在重复计 overage_count,与 parser 结论不一致

docs/copilot-overage-stuck-at-100-percent.md §5 明确写了修复策略是"有 credits_used 时直接用,无时按 remaining 形状分流"。Parser 照此实现了:

// CopilotResponseParser.swift
let totalUsage = remaining < 0 ? entitlement - remaining : entitlement + overageCount

RefreshService.swift:~830 只加了 credits_used 分支,else 分支原样保留了有 bug 的公式

} else if creditsUsed > 0 {
    displayUsage = String(creditsUsed)
    ...
} else {
    let used = max(0, entitlement - remaining) + overageCount   // ← 未修
}

testOverageDetectedWithoutCreditsUsed 那个 fixture(entitlement=100remaining=-10overage_count=10、无 credits_used)代入:

结果
Parser (100 - (-10)) / 100 * 100 = 110%
RefreshService max(0, 100-(-10)) + 10 = 120 / 100

即进度条显示 110%、面板文字显示 120 / 100自相矛盾,且正是文档 §5 认定要消除的 ±overage_count 双计。这个组合(新 API 形状 + 无 credits_used)不是假想场景 —— parser 侧专门为它写了测试用例,说明作者认为它可达。

建议 RefreshServiceelse 分支镜像 parser 的分流:

let used = remaining < 0 ? entitlement - remaining : max(0, entitlement - remaining) + overageCount

并在 RefreshServiceMappingTests 补一个"无 credits_used 的 overage"用例 —— 现有的 testCopilotOverageUsesCreditsUsedNotOverageCount 只覆盖了 credits_used 存在的路径,正好漏掉这个 bug。

2. NetworkClient.public body 日志:影响面判断有误 + 建议收窄

NetworkClient.swift 的 leak-surface 注释和文档 §9 都写了"OpenCode 从本地 SQLite 读,响应体可能含 user identifiers"来论证豁免范围。但实际上:

$ grep -rn "networkClient" APIUsageStatus/Suppliers/*.swift
DeepSeekSupplier / CopilotSupplier / MiniMaxSupplier / KimiSupplier

OpenCode 根本不走 NetworkClient,注释举的例子不成立。真实的影响面是 DeepSeek / Copilot / MiniMax / Kimi 四个远端 API 的失败响应体,而这里恰恰有一个注释没提到的风险:上游网关的 4xx 响应体有可能回显凭证片段或账号标识(GitHub 的错误体会带 message / documentation_url,MiniMax 的 base_resp 带账号上下文)。"Kimi 响应体只有 quota 数字"这个前提只对成功响应成立,对 401/403 的错误体没有验证过。

建议(任选):

  • 通用日志降级为 privacy: .private,只在 KimiSupplier 的 catch 块里保留 .public(本来这次要诊断的就是 Kimi)
  • 或者 #if DEBUG / 一个显式的诊断开关包起来,避免 Release 版长期带着这个面
  • 无论选哪个,请先把注释里 OpenCode 那句改掉,否则后续维护者会基于错误前提做决策

另外 PR 描述已经承诺"根因修复后应回退或收敛到 Kimi 专用"—— 建议直接开一个 issue 挂着,否则这种临时豁免最容易长期留存。

3. 验证状态与 PR 勾选项不符

我在本地跑了一次:

xcodebuild test -project APIUsageStatus.xcodeproj -scheme APIUsageStatus -destination 'platform=macOS'
→ 编译通过(无 error)
→ ** TEST FAILED **:Failed to load the test bundle
   ... mapped file has no Team ID and is not a platform binary

测试 target 能编译(说明文档 §10 修的那两处预存在编译错误确实解开了卡点),但测试 bundle 因签名问题在我这里加载不起来,所以"测试通过"这件事我无法证实。PR 的验证清单只勾了 Release 构建。既然这个分支包含改百分比公式的生产改动,建议在 PR 里贴一次真实的 Executed N tests 结果。


🟡 建议改进

4. creditsUsed > 0 当"字段是否存在"的哨兵值用

Parser 无论字段在不在都会写 rawData[":credits_used"] = String(Int(creditsUsed)),缺失时是字符串 "0"。所以 RefreshService 里的 creditsUsed > 0 无法区分"旧 API 没这个字段"和"新 API 但本月真的用了 0"。今天数值上无害(0 用量时两条公式同解),但这是个脆弱的约定。更直接的写法:

if let raw = response.value(forDimension: "\(key):credits_used"), let v = Int(raw), v > 0 { ... }

5. RefreshService 里那段注释与代码位置矛盾,且与项目分层约定相悖

注释写道:

The field name credits_used is Copilot-specific; the check below is intentionally a numeric guard rather than a provider == githubCopilot branch so that any future supplier that adopts the same key would get the same path for free.

但这段代码本身就在 if instance.provider == Provider.githubCopilot.rawValue 分支内部,其他 supplier 永远走不到,"future supplier gets it for free" 不成立。

更根本地,这与本项目"provider-specific 逻辑留在 Supplier/Parser 层、共享层保持 provider 无关"的约定是相反方向 —— 现在每加一个字段就要在 RefreshService 的 Copilot 分支里再加一层 if。理想解是让 parser 直接输出 provider-neutral 的 <key>:display_used / <key>:display_limitRefreshService 无脑消费。这次不必强求(既有的 provider 分支是预存在的),但请把这段有误导性的注释删掉或改对。

6. Parser 与 Service 对 credits_used 的使用不对称

  • Parser:只在 overage 分支credits_used 算百分比,非 overage 时仍用 100 - percent_remaining
  • Service:只要 credits_used > 0 就用它displayUsage

于是"新 API 形状 + 未超额"时,进度条来自 percent_remaining、文字来自 credits_used,两者可能对不上。既然 GitHub 已经证明了 percent_remaining 会被截断(这次 bug 的根因之一),建议只要 credits_used 存在就统一优先用它算百分比,两层同源。

7. Legacy fallback 公式静默丢掉了 remaining

: entitlement + overageCount      // 旧:entitlement - remaining + overageCount

只在 remaining == 0 时两者等价(testOverageData 的 fixture 正好是 remaining: 0,所以测试仍绿)。若旧 API 曾出现 remaining > 0overage_permitted && overage_count > 0,新公式会高估。保留 entitlement - remaining + overageCount 成本为零且在 remaining == 0 时完全同解,没必要改这一支。

8. 截断逻辑在两处重复

NetworkClientKimiSupplier 里各有一份 ~10 行的"4 KB 字节窗口 → 解码 → 512 字符"逻辑,连注释都近似重复。这种块级重复最容易日后只改一处而漂移。抽一个

extension Data { func utf8Preview(maxBytes: Int = 4096, maxChars: Int = 512) -> String }

顺带把它变成可单测的(现在这段逻辑一行测试都没有,多字节切断这个正确性论点纯靠注释背书)。

9. \(error, privacy: .public) 依赖 Error 的默认反射输出

直接插值 Error 值拿到的是编译器生成的 debug 描述,会随 enum 定义变化而变,且不保证包含 RefreshError.parsingError 的 message。建议显式 String(describing: error)error.localizedDescription,让日志格式稳定 —— 这次诊断的处置表(§6)就是靠 body 里能读到 Non-numeric value for limit 来区分假设 2 的。

10. 注释体量

新增了多处 8~15 行的注释块,内容与两份新文档高度重叠(历史沿革、GitHub 何时改了语义、为什么这样截断)。文档已经写得很详尽了,代码里保留一行结论 + 文档链接就够,长篇历史留在代码里反而会随文档更新而不同步。


⚪ Nits

  • docs/kimi-api-failures-investigation.md §1 是 (待用户补充...)、§5 是 <paste log here> —— 带占位符入库可以接受(这是待填的调查模板),但值得在 PR 里说明它是"活文档"
  • docs/copilot-overage-stuck-at-100-percent.md 缺文件末尾换行
  • Logger.swift 删掉的那个空行是无关噪声
  • 文档 §10 提到顺带修了 RefreshServiceMappingTests 的两处预存在编译错误(缺 } / 缺 throws)—— 这是整个 test target 在 main 上编译不过的原因,量级不小,建议提到 PR 描述里(若后续拆 PR,别把这两处丢了)
  • quota_remaining 缺失时写入 "0.0",与真实的 0.0 无法区分 —— 同 fix(menubar): render ∞ for unlimited Copilot plans #4 的哨兵值问题,但纯诊断字段,影响低
  • 文档 §9 判定"Kimi body 无 PII"是基于成功响应的字段。若这套诊断日志要长期留存,建议加一道 redaction(把 API key 字符串从 body 里抹掉)再打印

✅ 做得好的地方

  • testUnparseableWeeklyResetTimeDoesNotDeclareFiveHourPolicy 的 fixture 修正是真修:旧 fixture 完全没有 limits 块,测试根本没走到它名字声称的路径。补上合法 5h 窗口后测试名与实际断言终于对齐
  • 没有为了让 5h 策略"看起来更严"而反转 e462fa3 的保守选择,并且在文档 §7 备注里明确记录了"为什么回退了那个想法"—— 这种"记录被否决的方案"比记录采纳的方案更有价值
  • 截断策略(先 4 KB 字节窗口再按字符切)的推理是对的,Kimi 常返回中文错误串,按字节切会让 String(data:encoding:) 返回 nil,恰好在最需要 body 的时候失效
  • AppLogger.publicError 包装器编译不过 → 暴露 osLogger 让调用点直调,这个折返记录得很清楚,而且注释里指出"字面量要求意味着 .public 注解不可能被静默丢掉"是个准确的观察
  • 两份调查文档的字段对照表(旧契约 vs 2026-07-30 实测)质量很高,PRD / provider-interfaces/copilot.md / 风险表都同步更新了,不是只改代码

结论

Request changes —— 主要卡在 #1RefreshService fallback 分支的双计,会让面板数字和进度条自相矛盾)和 #2.public 影响面的判断依据不成立,注释需要改对,范围建议收窄)。

另外强烈建议把 Copilot overage 修复拆成独立 PR:它是改用户可见数字的生产逻辑,应该有自己的标题、描述和验证记录,而不是搭在一个标题写着 "kimi test policy + diagnostic logging" 的分支里。至少请更新 PR 描述,把那两个 Copilot commit 和它们的风险写进去。

@linletian linletian left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

评审结论:代码层面 LGTM(建议合并前完成实机验证)

关于本 PR 的真实范围 ⚠️

GitHub 的 Files 页目前显示的是陈旧 diff(936 行,含 Copilot overage 修复)。原因是 PR #14 在 2026-07-31 08:31 UTC 合并进 main 后,GitHub 尚未重算本 PR 的对比基准。本 PR 的真实增量只有 3 个 commit、6 个文件(+222/−5):

APIUsageStatus/Network/NetworkClient.swift        | +28/−2
APIUsageStatus/Suppliers/KimiResponseParser.swift | +4
APIUsageStatus/Suppliers/KimiSupplier.swift       | +25/−2
APIUsageStatus/Utilities/Logger.swift             | +18/−1
APIUsageStatusTests/KimiResponseParserTests.swift | +15/−2
docs/kimi-api-failures-investigation.md           | +130

Copilot 相关改动全部来自已合并的 #14,评审时请以 git diff origin/main...HEAD 为准。rebase 一下 main 或推送任意更新即可刷新 GitHub 视图。

测试验证 ✅

本地在本分支跑了完整测试套件:356 个用例全部通过,包括本次修复的 testUnparseableWeeklyResetTimeDoesNotDeclareFiveHourPolicy(0.004s passed)——fixture 补上合法 5h limits 块后,测试名实相符,坏 weekly resetTime 不再污染 5h policy 的断言真实生效。

一个环境发现(与本 PR 无关):直接按 AGENTS.md 的命令跑 xcodebuild test 会失败——test bundle 被 ad-hoc 签名,macOS 拒绝 dlopen(code signature ... not valid for use in process: mapped file has no Team ID)。失败发生在测试加载阶段、任何用例执行之前,属于工程签名配置(CODE_SIGN_STYLE: Automatic 且无 DEVELOPMENT_TEAM)的既有问题。加 CODE_SIGNING_ALLOWED=NO 后全部通过。建议后续在 AGENTS.md 的测试命令里补上这个参数。

代码评审

做得好的部分:

  • UTF-8 截断策略正确:先取 4 KB 字节窗口解码、再按字符切 512,避免了按字节切断多字节序列导致 String(data:encoding:) 返回 nil 的陷阱,且注释把"为什么"写清楚了。
  • 放弃 publicError(OSLogMessage) 包装器、改为暴露 AppLogger.osLogger 是正确的取舍——os.Logger 强制字面量插值,包装器确实无法编译;契约注释("诊断专用,never for tokens / secrets / PII")落在了属性上,后续调用方有据可依。
  • 两个诊断点(NetworkClient 非 2xx、KimiSupplier parse catch)都在失败路径且 rethrow 原错误,成功路径零改动、错误语义不变。KimiSupplier 的 catch 虽然是 catch-all,但 parser.parse 是同步方法只会抛 RefreshError,不会误吞 CancellationError,没有问题。

建议(不阻塞合并):

  1. 临时豁免要有回收机制 — NetworkClient 非 2xx 分支对所有 supplier 以 privacy: .public 记录响应体,docs/kimi-api-failures-investigation.md §9 已做了 leak 面评估并声明"根因修复后应回退或收敛到 Kimi 专用"。建议开一个 tracking issue 显式跟进这件事,避免临时诊断豁免被遗忘成永久状态。
  2. 文档待填项 — 调查文档 §1「现象」和 §5「复现数据」仍是占位符;按 §4.1 完成实机验证(临时换错 Key 强制触发一次失败,确认 log showbody= 不是 <private>)后一并补上,再合并。

总结

三个 commit 与 PR body 描述一致,改动聚焦、注释和文档质量高。代码与测试层面没有问题,合并条件按 PR 自己的约定:完成 §4.1 实机验证

PR #15 review follow-ups:

- NetworkClient's non-2xx branch logged response bodies at
  privacy: .public for ALL suppliers, justified by an OpenCode
  leak-surface note — but OpenCode never goes through NetworkClient
  (local SQLite). The real surface is DeepSeek/Copilot/MiniMax/Kimi
  error bodies, which may echo credential fragments or account
  identifiers. Bodies now default to .private; only endpoints with
  exposesFailureBodyInLog = true (currently just Kimi) stay .public.
- Extract the duplicated 4KB-window-then-512-chars truncation into
  Data.utf8Preview(maxBytes:maxChars:) with unit tests covering
  multi-byte slicing, invalid UTF-8, and boundary windows.
- Log errors via String(describing:) instead of relying on Error's
  reflection-based default interpolation, keeping the log format
  stable for the §6 disposition table.

Docs: kimi-api-failures-investigation.md §8/§9 corrected.

@linletian linletian left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #15 评审(自定义:跳过 PR 范围管理)

总体

整体方向可合,改动扎实。Copilot overage 修复有清晰的推导与双向 fixture(新/旧 API 形状),Data.utf8Preview 的多字节意识值得肯定,Endpoint.exposesFailureBodyInLog opt-in 设计 + 默认 .private 是合理的隐私收敛。文档尤其出色(copilot-overage-stuck-at-100-percent.md / kimi-api-failures-investigation.md)。


⚠️ 关键问题(合并前关注)

1. 诊断日志未实机验证
PR 描述自承"log show 能否真的看到 body= 内容需在真实环境触发一次 Kimi 失败确认"。logger.osLogger.error(...)privacy: .public 注解只是语法层面有效,运行时 log show 能否渲染真实值只能实机确认。文档 §4.1 给了脚本 — 不验证就合,等于诊断能力零增量。建议合并前先临时换错 Key 触发一次失败,跑一次 log show --predicate 'subsystem == "com.example.APIUsageStatus" AND category == "supplier"' --last 1m --info 确认 body 可见。

2. AppLogger.osLogger 暴露面太宽

var osLogger: os.Logger { logger }

私有 wrapper 不成立的注释做得很到位(os.Logger 要求字面量插值,无法转发 OSLogMessage),但把整个 os.Logger 暴露意味着将来任何 logger.osLogger.fault("token=\(token, privacy: .public)") 都能绕过"通用路径 body 默认 .private"的约束。建议在注释里加 TODO,或在合并后续 PR 收紧成 publicError(_:...) 一类的 spec wrapper。


✅ 优点

A. Copilot overage 修复扎实
CopilotResponseParser 三信号 OR 判定 + 双公式(credits_used 优先 / 形状分流兜底)实现干净。RefreshService.swiftused = max(0, entitlement - remaining) + overageCount 双重计数是 pre-existing bug,顺手修了 — 文档 §5 用 9055 vs 8054 的代入推导很有说服力。严格遵循 [[provider-specific-logic-boundaries]] — overage 识别完全在 parser 层。

B. Data.utf8Preview 的多字节意识

String(data: prefix(maxBytes), encoding: .utf8)  // 先按字节窗口解码
String(decoded.prefix(maxChars))                  // 再按字符切

先解后切的顺序避开了 "raw byte 切多字节 UTF-8 中段 → String(data:encoding:) 返回 nil" 的陷阱。6 个测试(ASCII、Chinese、超界 1 字节、字符边界、非法字节、空)corner case 都覆盖。Kimi 错误体常含中文,这步非常对。

C. Endpoint.exposesFailureBodyInLog opt-in + 默认 .private
通用路径默认 .private,只有 Kimi 显式 opt-in 后 .public — 把"上游 4xx body 可能回显凭证片段"的风险收敛到白名单。文档 §9 把每条字段的 leak 面逐项列出,值得效法。

D. testUnparseableWeeklyResetTimeDoesNotDeclareFiveHourPolicy 修复
名字与实际不符是 main 上长期潜伏的问题(旧 fixture 缺 limits,parser 路径根本走不到 5h branch,测试空转)。本次改成"合法 5h limits + 坏 weekly usage",并加注释澄清 e462fa3 选了 .retainPreviousIfResponseMissing(保守策略,不反转)。


🔍 关注点 / 改进建议

  1. displayUsage = String(creditsUsed) 精度 — parser 已 String(Int(...)) 截断 double,如果上游某天返回 8054.7 则显示为 8054。当前 quota 是整数场景无 bug,仅注意。

  2. quota_remaining 1 位小数格式化String(format: "%.1f", quotaRemaining) 若上游返回 -1054.5678 被截为 -1054.6。当前仅作副键 + 诊断,无业务消费,可接受。

  3. CopilotSupplier.fetchUsage 成功路径加 logger.debug("...rawData...")debug 级别默认不显示,合规;rawData 只含数字/布尔,无 token / 无 PII。保留可作长期诊断手段。

  4. Kimi 文档 §2 假设 1(凭证不匹配)是否可以合并前先 curl 验证 — 文档 §1 自承"调研时手头无 Key,OAuth token 实测 200"。如果合并诊断日志前先 curl 现有 Key 跑 /usages,若已 200 则诊断只服务"未来失败"(合并动机减弱);若 401,可直接转修凭证管理,不用走诊断分支。

  5. isNewOverage 信号聚合的可读性 — 三 OR 当前够用,以后若再加第四个信号建议拆 static func isOverage(...) 让测试更可读。不必强制。


合并 Checklist

  • 实机验证 log show 输出真实 body(文档 §4.1 步骤)
  • 确认 AppLogger.osLogger 暴露面的收敛策略(至少加 TODO)
  • d0adfa07 修了 main 上预存的测试编译错误,这意味着任何先于它合并的 PR 都跑不了测试 — 建议作者考虑是否拆为独立 PR 单独合并(同意作者文档 §10 的自我建议)
  • 可选:合并诊断日志前先 curl Kimi /usages 跑一次现有 Key,确认根因不在凭证

评分(剔除 scope 管理)

维度 评分 备注
代码正确性 ⭐⭐⭐⭐ Copilot overage 修复推导清晰,Kimi 诊断一处截断边界要 caveat
项目规范遵循 ⭐⭐⭐⭐ 严格遵守 [[provider-specific-logic-boundaries]],overage 识别全在 parser 层
隐私 / 安全 ⭐⭐⭐⭐ 默认 .private + opt-in 模式好,osLogger 直暴露面可收紧
测试覆盖 ⭐⭐⭐⭐½ Data.utf8Preview 6 fixture,copilot 新/旧 API 两个 fixture,完整
文档质量 ⭐⭐⭐⭐⭐ 两份 investigation 文档写到能复现 + 配处置表,极少 PR 能做到这个水平

The else branch still computed `max(0, entitlement - remaining) +
overageCount`, double-counting overage once `remaining` goes negative
(110% on the progress bar vs 120 / 100 in the panel text). Mirror
CopilotResponseParser's split: `remaining < 0` uses `entitlement -
remaining` (overage already embedded); `remaining >= 0` adds
overageCount (legacy shape clamps remaining to 0). Add
testCopilotOverageWithoutCreditsUsed which fails on the old formula
(120 != 110). Also drop the misleading 'future supplier gets it for
free' comment (code sits inside the githubCopilot branch), use an
optional-binding sentinel for credits_used, add a TODO to narrow
AppLogger.osLogger exposure, and fix missing trailing newline in the
copilot investigation doc.
@linletian

Copy link
Copy Markdown
Owner Author

评审意见回应与修复确认

针对 07-31 评审总览(Request changes 实质内容)

# 意见 状态
1 RefreshService fallback 双计 overage_count 已修复f571dea)。else 分支镜像 parser 分流:remaining < 0 ? entitlement - remaining : max(0, entitlement - remaining) + overageCount。补测试 testCopilotOverageWithoutCreditsUsed(entitlement=100 / remaining=-10 / overage_count=10 / 无 credits_used):修复前实测失败 ("120") is not equal to ("110"),修复后通过 —— 与评审推算完全一致
2 .public body 影响面判断有误 + 建议收窄 ✅ 已收窄(8cfc54e):Endpoint.exposesFailureBodyInLog opt-in,默认 .private,仅 Kimi 显式 opt-in;注释中 OpenCode 那句已改掉(OpenCode 不走 NetworkClient,实际影响面为 DeepSeek/Copilot/MiniMax/Kimi,且已按评审建议收敛)
3 测试无法实机验证 ✅ 用 CODE_SIGNING_ALLOWED=NO 跑通:363 个用例全部通过(含本次新增)
4 credits_used > 0 当哨兵值 ✅ 已改 Int? 可选绑定哨兵,区分"字段缺失"与"字段为 0"
5 误导性注释("future supplier gets it for free") ✅ 已删除,改为简洁说明 + 文档链接
6 Parser 与 Service 使用不对称 🟡 部分:本次修复让 fallback 与 parser 分流一致;非 overage 时 percent 仍走 percent_remaining(保持现状,属更大的设计改动,不阻塞本 PR)
7 Legacy fallback 丢 remaining ✅ 已保留 entitlement - remaining + overageCount 语义(remaining >= 0 分支),remaining == 0 时与原公式完全同解
8 截断逻辑重复 ✅ 已由 Data.utf8Preview 抽取 + 6 个单测(8cfc54e
9 Error 反射输出 ✅ KimiSupplier catch 已用 String(describing: error)
10 注释体量 ✅ RefreshService 注释已精简;文档链接指向两份 investigation 文档
nit copilot 文档缺末尾换行 ✅ 已补

针对 08-02 最新评审

  1. "RefreshService 双计顺手修了"——判断有误:实际未修(8cfc54e 前该公式原样存在)。本次 f571dea 已修复并补测试,上述 fix: ShellProcessRunner timeout throws timedOut instead of nonZeroExit #1 负验证即为证据。评审中"已修"的说法与代码事实不符,特此更正。
  2. AppLogger.osLogger 暴露面 ✅ 已加 TODO(pr15-followup)(spec wrapper / DEBUG gate 收敛策略),指向调查文档 §9。
  3. 实机验证 log show(§4.1) ⏳ 待人工:需临时换错 Key 触发一次 Kimi 失败确认 body=<private>,沙箱环境无法完成。
  4. d0adfa07 拆独立 PR ✅ 该 commit 已随 fix(copilot): handle new API shape (overage stuck at 100%) #14 合并进 main,无需操作。
  5. 可选 curl 验证 Kimi Key ⏳ 待人工。

评审中"已修"但实际未修的更正清单

  • 08-02 评审称 RefreshService 双计"顺手修了" → 实际未修,f571dea 补上
  • 其余评审点均已核实为代码事实(opt-in 设计、utf8Preview 抽取、6 个测试等均存在)

结论:所有 🔴 阻塞项已闭环,评审指出的实质 bug 已修复并附负验证。待人工项仅剩 §4.1 实机验证(合并前按 PR 自身约定执行)。

Resolve conflict in RefreshService.swift credits_used block: keep the
Int? sentinel (required by the else-if-let guard from f571dea) and fold
main's 0630250 'write contract' precondition into the comment, without
reviving the misleading 'future supplier gets it for free' claim that
review #5 flagged. Docs merged cleanly (the §7 contract bullet from
0630250 and the trailing newline from f571dea are the same change).
@linletian
linletian merged commit 5586320 into main Aug 2, 2026
linletian added a commit that referenced this pull request Aug 2, 2026
Post-merge review (PR #15) found two divergences from the strategy in
docs/copilot-overage-stuck-at-100-percent.md §7:

- RefreshService's fallback kept `max(0, entitlement - remaining) +
  overageCount` unconditionally, double-counting overage in the new
  API shape when `credits_used` is absent (panel text 120/100 vs the
  parser's 110% bar). Now mirrors the parser: `remaining < 0` ⇒
  `entitlement - remaining`, else add `overageCount`.
- Parser legacy branch dropped `- remaining` in 6a85578; restored
  (equivalent at remaining == 0, correct when > 0).

Also replace the misleading comment claiming the numeric guard is a
provider-agnostic extension point — the block sits inside the
githubCopilot branch, no other supplier can reach it.

Tests: testCopilotOverageFallbackWithoutCreditsUsedDoesNotDoubleCount,
testLegacyOverageWithPositiveRemainingSubtractsRemaining.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant