fix(cli): render a sub-minute duration that rounds to 60s as "1m" by he-yufeng · Pull Request #5287 · QwenLM/qwen-code

he-yufeng · 2026-06-18T06:03:17Z

What this PR does

formatDuration renders 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 printed 60.0s (or 60s with hideTrailingZeros). This change detects that the rounded value reached 60 and renders it as 1m instead, matching formatDuration(60000) === "1m".

Why it's needed

60.0s is 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 way formatDuration(60000) already returns 1m. It is a small but visible inconsistency in the elapsed-time indicators that use this helper.

Reviewer Test Plan

How to verify

npx vitest run packages/cli/src/ui/utils/formatters.test.ts -t formatDuration

A new case asserts formatDuration(59950) === "1m", formatDuration(59999) === "1m", the hideTrailingZeros variant, and that formatDuration(59949) still returns 59.9s. The 60s case fails on main (returns 60.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

OS	Status
🍏 macOS	N/A
🪟 Windows	✅
🐧 Linux	⚠️

Environment (optional)

N/A — unit tests only.

Risk & Scope

Main risk or tradeoff: only the sub-minute branch of formatDuration changes; the 59.95–59.999s window now reads 1m. No other input range is affected.
Not validated / out of scope: callers of formatDuration are untouched.
Breaking changes / migration notes: none.

Linked Issues

None.

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

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

wenshao · 2026-06-18T19:51:05Z

@qwen-code /triage

qwen-code-ci-bot · 2026-06-18T19:54:37Z

Thanks for the PR!

Template looks good ✓

On direction: this is a clear-cut display bug — 60.0s is not a valid reading and should roll over to 1m. Solidly within scope, no concerns.

On approach: the scope is exactly right — 4 lines of fix plus 6 lines of tests, nothing extraneous. The fix is placed at the natural spot (right after toFixed in the sub-minute branch) and handles the edge case cleanly.

Moving on to code review. 🔍

中文说明

感谢贡献！

模板完整 ✓

方向：这是一个明确的显示 bug——60.0s 不是有效的读数，应该进位到 1m。完全在范围内，没有问题。

方案：范围恰到好处——4 行修复加 6 行测试，没有多余内容。修复放在了自然的位置（toFixed 之后），干净地处理了边界情况。

进入代码审查 🔍

— Qwen Code · qwen3.7-max

wenshao · 2026-06-18T19:58:02Z

Maintainer verification — local merge build + before/after harness + macOS CI triage

Verified the 3-way merge result (origin/main 06fdd59 + this PR cd18ebc) on macOS (Darwin arm64, Node v22.22.2). Merge is clean, no drift, and the resulting diff is exactly the PR's +16/-0. The headline for the merge decision is the macOS CI failure — it is an unrelated flaky test, not this PR (triage in §3).

1. The fix — `formatDuration` minute-boundary (before/after)

toFixed(1) rounds 59.95–59.999s up to "60.0", which isn't a valid sub-minute reading. The fix renders it as 1m, matching formatDuration(60000). Ran both versions over a boundary corpus:

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 to formatters.ts. This PR touches only the CLI duration formatter and nothing in core.
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.ts suite 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。}

qwen-code-ci-bot · 2026-06-18T20:00:16Z

Code Review

The fix is clean and minimal — 4 lines in the right place. parseFloat(formatted) >= 60 catches the only edge case where toFixed(1) rounds a sub-minute value up to 60.0. No correctness bugs, no security concerns, no AGENTS.md violations. The comment explains the why without being verbose.

Independent proposal comparison: I would have done the same thing — a guard after toFixed checking the rounded value against the minute boundary. The PR's approach matches exactly.

Test Results

# Before (main, no fix) — test fails
 ✓ src/ui/utils/formatters.test.ts (33 tests | 17 skipped)
   Tests  1 failed | 15 passed | 17 skipped

   Expected: "1m"
   Received: "60.0s"

   ❯ expect(formatDuration(59950)).toBe('1m');

# After (with fix) — all pass
 ✓ src/ui/utils/formatters.test.ts (33 tests | 17 skipped)
   Tests  16 passed | 17 skipped

The new test correctly fails on main and passes with the fix. All 15 existing formatDuration tests remain green — no regressions.

This is a pure formatting utility (formatDuration is a helper function, not user-facing TUI), so tmux testing doesn't apply — unit tests are the right verification method here.

中文说明

代码审查

修复干净且最小化——4 行代码放在了正确的位置。parseFloat(formatted) >= 60 捕获了 toFixed(1) 将亚分钟值向上取整到 60.0 的唯一边界情况。没有正确性 bug，没有安全问题，没有违反 AGENTS.md。注释解释了原因，不多不少。

与独立方案对比： 我的做法会一样——在 toFixed 之后加一个守卫，检查取整后的值是否跨越分钟边界。PR 的方案完全一致。

测试结果

修复前 (main)： 新测试失败，期望 "1m" 实际 "60.0s"，其余 15 个测试通过
修复后： 全部 16 个测试通过，无回归

这是一个纯格式化工具函数（formatDuration 是辅助函数，不是面向用户的 TUI），tmux 测试不适用——单元测试是正确的验证方式。

— Qwen Code · qwen3.7-max

qwen-code-ci-bot · 2026-06-18T20:00:21Z

This is exactly what a good small fix looks like. A real display bug, a minimal diff, well-placed guard, and tests that prove both the failure mode and the fix. No scope creep, no drive-by refactors — just the fix.

The before/after is unambiguous: formatDuration(59950) returns "60.0s" on main and "1m" with the fix, and the existing 15 tests are untouched. The parseFloat approach is a tiny bit indirect (could just check totalSeconds >= 59.95 before toFixed), but it's arguably clearer to keep the check adjacent to the rounding that causes the bug. Not worth changing.

LGTM. Approving. ✅

中文说明

这就是一个好的小修复应该有的样子。一个真实的显示 bug，最小的 diff，放在正确位置的守卫，以及同时证明失败模式和修复的测试。没有范围蔓延，没有顺手重构——只有修复本身。

修复前后对比明确：formatDuration(59950) 在 main 上返回 "60.0s"，修复后返回 "1m"，现有的 15 个测试不受影响。parseFloat 的方式稍微间接了一点（可以在 toFixed 之前直接检查 totalSeconds >= 59.95），但把检查放在导致 bug 的取整旁边可能更清晰。不值得改。

LGTM，批准。✅

— Qwen Code · qwen3.7-max

qwen-code-ci-bot

LGTM, looks ready to ship. ✅

wenshao reviewed Jun 18, 2026

View reviewed changes

qwen-code-ci-bot approved these changes Jun 18, 2026

View reviewed changes

wenshao approved these changes Jun 18, 2026

View reviewed changes

wenshao enabled auto-merge (squash) June 18, 2026 20:23

wenshao merged commit 62c05c8 into QwenLM:main Jun 18, 2026
38 of 39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(cli): render a sub-minute duration that rounds to 60s as "1m"#5287

fix(cli): render a sub-minute duration that rounds to 60s as "1m"#5287
wenshao merged 1 commit into
QwenLM:mainfrom
he-yufeng:fix/format-duration-minute-rollover

he-yufeng commented Jun 18, 2026

Uh oh!

wenshao left a comment

Uh oh!

wenshao commented Jun 18, 2026

Uh oh!

qwen-code-ci-bot commented Jun 18, 2026

Uh oh!

wenshao commented Jun 18, 2026

维护者验证 —— 本地合并构建 + 前后对比 harness + macOS CI 排查

1. 修复 —— `formatDuration` 分钟边界（前后对比）

2. 构建 / 测试 / 类型检查 / lint（合并结果）

3. macOS CI 失败排查 —— 无关 flake（这就是 PR 显示 `BLOCKED` 的原因）

结论

Uh oh!

qwen-code-ci-bot commented Jun 18, 2026

代码审查

测试结果

Uh oh!

qwen-code-ci-bot commented Jun 18, 2026

Uh oh!

qwen-code-ci-bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

he-yufeng commented Jun 18, 2026

What this PR does

Why it's needed

Reviewer Test Plan

How to verify

Evidence (Before & After)

Tested on

Environment (optional)

Risk & Scope

Linked Issues

Uh oh!

wenshao left a comment

Choose a reason for hiding this comment

Uh oh!

wenshao commented Jun 18, 2026

Uh oh!

qwen-code-ci-bot commented Jun 18, 2026

Uh oh!

wenshao commented Jun 18, 2026

Maintainer verification — local merge build + before/after harness + macOS CI triage

1. The fix — formatDuration minute-boundary (before/after)

2. Build / tests / typecheck / lint (merge result)

3. macOS CI failure triage — unrelated flake (this is why the PR shows BLOCKED)

Verdict

维护者验证 —— 本地合并构建 + 前后对比 harness + macOS CI 排查

1. 修复 —— formatDuration 分钟边界（前后对比）

2. 构建 / 测试 / 类型检查 / lint（合并结果）

3. macOS CI 失败排查 —— 无关 flake（这就是 PR 显示 BLOCKED 的原因）

结论

Uh oh!

qwen-code-ci-bot commented Jun 18, 2026

Code Review

Test Results

代码审查

测试结果

Uh oh!

qwen-code-ci-bot commented Jun 18, 2026

Uh oh!

qwen-code-ci-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1. The fix — `formatDuration` minute-boundary (before/after)

3. macOS CI failure triage — unrelated flake (this is why the PR shows `BLOCKED`)

1. 修复 —— `formatDuration` 分钟边界（前后对比）

3. macOS CI 失败排查 —— 无关 flake（这就是 PR 显示 `BLOCKED` 的原因）