Repository navigation
fix(cli): render a sub-minute duration that rounds to 60s as "1m" - #5287
Conversation
formatDuration formats durations under a minute with toFixed(1). Values from 59.95s up round to "60.0", so the tool printed "60.0s" (or "60s" with hideTrailingZeros) — not a valid sub-minute reading — instead of "1m". Detect when the rounded value reaches 60 and render it as the minute it rounds to, matching formatDuration(60000) === "1m".
wenshao
left a comment
There was a problem hiding this comment.
No review findings. The fix correctly handles the toFixed(1) rounding-up edge case at the 60-second boundary, and the test suite covers the boundary precisely (59949ms → "59.9s", 59950ms → "1m", 59999ms → "1m"). tsc and eslint both pass clean.
Note: the failing macOS CI check (Test (macos-latest, Node 22.x)) is a pre-existing flake in permissionMediator/session-reconcile tests — unrelated to this change; Ubuntu and Windows both pass.
— glm-5.2 via Qwen Code /review
|
@qwen-code /triage |
Maintainer verification — local merge build + before/after harness + macOS CI triageVerified the 3-way merge result ( 1. The fix —
|
| Input | before | after |
|---|---|---|
59949 (just under) |
59.9s |
59.9s (unchanged) |
59950 (rounds to 60.0) |
60.0s |
1m ✅ |
59999 (rounds to 60.0) |
60.0s |
1m ✅ |
59950 + hideTrailingZeros |
60s |
1m ✅ |
59000, 60000, 500, 0, -100, 90000 (1m30s), 3661000 (1h1m1s), 5000+htz |
— | all identical (no regression) |
3 values change — exactly the 59.95–59.999s window — everything else is byte-identical.
2. Build / tests / typecheck / lint (merge result)
| Check | Result |
|---|---|
formatters.test.ts |
✅ 33 passed (PR's new boundary case + 32 existing) |
tsc --noEmit (CLI) |
✅ the 2 changed files are clean |
eslint + prettier --check |
✅ clean |
| CI: Windows / Linux | ✅ pass |
| CI: macOS | ❌ failed → triaged below |
3. macOS CI failure triage — unrelated flake (this is why the PR shows BLOCKED)
The macOS job failed on exactly one test, 1 failed | 12059 passed:
CronScheduler > durable ownership > takes over via the lock probe after the owner releases the lock
- That test lives in
packages/core/src/services/cronScheduler.test.ts— a timing/concurrency lock-probe test, unrelated toformatters.ts. This PR touches only the CLI duration formatter and nothing incore. - It failed only on macOS; Windows and Linux passed the same commit — the signature of a flaky test, not a code regression.
- Locally on the merge result (macOS, same OS as the failing runner): the specific test passes 5/5 consecutive runs, and the full
cronScheduler.test.tssuite passes 89/89.
Conclusion: the macOS failure is a pre-existing flaky CronScheduler test, not caused by PR #5287. A CI re-run should clear it.
Verdict
The fix is correct, minimal, and regression-free — a small but real display inconsistency (60.0s should roll over to 1m). The only thing standing between this PR and merge is a flaky macOS CronScheduler test that has nothing to do with the change. ✅ Safe to merge once CI is re-run (or merged past the known flake).
Verified by maintainer @wenshao: 3-way merge build + formatters.test.ts (33/33) + a before/after formatDuration harness over a boundary corpus, and triage of the macOS CI failure (flaky cronScheduler lock-probe test: 5/5 + 89/89 locally on macOS). Env: Darwin arm64, Node v22.22.2.
中文版（点击展开）
维护者验证 —— 本地合并构建 + 前后对比 harness + macOS CI 排查
在 macOS（Darwin arm64，Node v22.22.2）上验证了三方合并结果（origin/main 06fdd59 + 本 PR cd18ebc）。合并干净、无漂移，合并后 diff 正好是 PR 的 +16/-0。对合并决策最关键的一点是那个 macOS CI 失败——它是一个与本 PR 无关的 flaky 测试（见 §3 排查）。
1. 修复 —— formatDuration 分钟边界（前后对比）
toFixed(1) 会把 59.95–59.999s 向上舍入成 "60.0"，这不是一个合法的"不到一分钟"的读数。修复把它渲染为 1m，与 formatDuration(60000) 一致。我用一组边界语料跑了两个版本：
| 输入 | before | after |
|---|---|---|
59949（刚好不到边界） |
59.9s |
59.9s（不变） |
59950（舍入到 60.0） |
60.0s |
1m ✅ |
59999（舍入到 60.0） |
60.0s |
1m ✅ |
59950 + hideTrailingZeros |
60s |
1m ✅ |
59000、60000、500、0、-100、90000(1m30s)、3661000(1h1m1s)、5000+htz |
— | 全部完全一致（无回归） |
只有 3 个值改变——正好是 59.95–59.999s 这个窗口——其余逐字节一致。
2. 构建 / 测试 / 类型检查 / lint（合并结果）
| 检查 | 结果 |
|---|---|
formatters.test.ts |
✅ 33 通过（PR 新增边界用例 + 既有 32 个） |
tsc --noEmit（CLI） |
✅ 2 个改动文件干净 |
eslint + prettier --check |
✅ 干净 |
| CI：Windows / Linux | ✅ 通过 |
| CI：macOS | ❌ 失败 → 见下方排查 |
3. macOS CI 失败排查 —— 无关 flake（这就是 PR 显示 BLOCKED 的原因）
macOS job 只挂了一个测试，1 failed | 12059 passed：
CronScheduler > durable ownership > takes over via the lock probe after the owner releases the lock
- 该测试在
packages/core/src/services/cronScheduler.test.ts——一个时序/并发的 lock-probe 测试，与formatters.ts无关。本 PR 只动了 CLI 的时长格式化，core里什么都没碰。 - 它只在 macOS 失败；Windows 和 Linux 在同一个 commit 上都通过了——这是 flaky 测试的特征，而非代码回归。
- 在合并结果上本地（macOS，与失败 runner 同系统）：该具体测试连续 5/5 通过，整个
cronScheduler.test.ts套件 89/89 通过。
结论：macOS 失败是一个既有的 flaky CronScheduler 测试，并非 PR #5287 引起。 重跑 CI 应能消除。
结论
修复正确、极简、无回归——一个虽小但真实的显示不一致（60.0s 应翻入 1m）。挡在这个 PR 和合并之间的唯一障碍，是一个与本改动无关的 macOS CronScheduler flaky 测试。✅ 重跑 CI 后即可合并（或在确认该已知 flake 的前提下直接合并）。
维护者 @wenshao 验证：三方合并构建 + formatters.test.ts（33/33）+ 一个针对边界语料的 formatDuration 前后对比 harness，并排查了 macOS CI 失败（flaky cronScheduler lock-probe 测试：本地 macOS 上 5/5 + 89/89）。环境：Darwin arm64，Node v22.22.2。
What this PR does
formatDurationrenders a duration under a minute with one decimal second (e.g.12.3s). When the value is at least 59.95s,toFixed(1)rounds it up to"60.0", so the function printed60.0s(or60swithhideTrailingZeros). This change detects that the rounded value reached 60 and renders it as1minstead, matchingformatDuration(60000) === "1m".Why it's needed
60.0sis not a valid sub-minute reading — once the displayed value rounds to a full minute it should roll over into the minute format, the same wayformatDuration(60000)already returns1m. It is a small but visible inconsistency in the elapsed-time indicators that use this helper.Reviewer Test Plan
How to verify
A new case asserts
formatDuration(59950) === "1m",formatDuration(59999) === "1m", thehideTrailingZerosvariant, and thatformatDuration(59949)still returns59.9s. The 60s case fails onmain(returns60.0s) and passes with this change; the existing 32 cases are unchanged.Evidence (Before & After)
N/A — a formatting helper covered by unit tests. Before:
formatDuration(59950) === "60.0s". After:"1m".Tested on
Environment (optional)
N/A — unit tests only.
Risk & Scope
formatDurationchanges; the 59.95–59.999s window now reads1m. No other input range is affected.formatDurationare untouched.Linked Issues
None.