Thanks for the PR!

Template looks good ✓

Problem: observed behavior mismatch — the code comment says "anything invalid falls back to the default," but Number.parseInt() prefix-parses malformed strings (-5 → -5, 1.5 → 1, 7junk → 7, 1e2 → 1). The before/after in the PR body demonstrates this concretely.

Direction: aligned — this is a small input-validation fix making the parser match its documented contract on a daemon query surface. No CHANGELOG reference needed for a parse-correctness fix.

Size: not applicable (no core paths touched — packages/cli/src/serve/routes/).

Approach: the scope is tight — one regex gate before the existing parseInt + clamp, plus four new test cases. Minimal and focused.

Moving on to code review. 🔍

— Qwen Code · qwen3.8-max-preview

_{Reviewed at 6066b03c698a32094a19807816a1c8973781993a · re-run with @qwen-code /triage}

Code Review

Independent proposal: given the problem (parseInt prefix-parses partial numeric strings), I'd add a /^\d+$/ regex guard before the existing parseInt + clamp — exactly what this PR does.

Findings: no blockers. The implementation is correct:

• typeof raw !== 'string' handles non-string inputs (arrays, undefined).
• /^\d+$/ rejects negatives, decimals, exponents, trailing junk, and empty strings — requires at least one digit.
• After the guard, parseInt is safe (input is guaranteed all-digits); the existing [1, 366] clamp is preserved unchanged.
• Edge case "0" → passes regex → parseInt → 0 → clamped to 1. Correct.
• Very long digit strings → parseInt may lose precision beyond MAX_SAFE_INTEGER, but the clamp to 366 makes this irrelevant.

Tests cover the four malformed categories (-5, 1.5, 7junk, 1e2) plus the existing valid/clamp cases remain. No scope creep, no unrelated changes.

Testing

Ubuntu unit tests and the Serve A/B integration both pass on the reviewed commit. Windows/macOS test jobs were skipped (infra scheduling, not PR-related). No failures.

Real-scenario testing: N/A — this is a query-parameter parser fix with no user-visible TUI surface; the route test and Serve A/B integration cover the behavior.

— Qwen Code · qwen3.8-max-preview

_{Reviewed at 6066b03c698a32094a19807816a1c8973781993a · re-run with @qwen-code /triage}

Confidence: 5/5 — clean across every stage; minimal fix that makes the parser match its documented contract, CI green, no reservations.

This is exactly the kind of PR that's easy to say yes to: one regex gate, four test cases, zero scope creep. The parseInt prefix-parsing gotcha is well-known and the fix is the obvious correct one. The before/after in the PR body demonstrates the problem concretely, and the route test pins the new behavior. If I had to maintain this in six months, I'd thank the author — the code reads clearly and the comment above the function now matches what it actually does.

Approving. ✅

— Qwen Code · qwen3.8-max-preview

_{Reviewed at 6066b03c698a32094a19807816a1c8973781993a · re-run with @qwen-code /triage}

🩺 serve daemon A/B

Built the PR base vs this PR head 6066b03, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

✅ No response changes against the PR base across 4 scenario(s).

— Qwen Code · serve A/B

Thanks for the review note on #7218 (comment). I added an explicit route-test assertion for ?heatmapDays=-5\ so the new invalid-value fallback behavior is pinned: negative strings now stay on the documented default path instead of slipping back to min-clamp behavior.

🔍 Local Verification Report — PR #7218

Scope: +16 lines, 2 files — parse heatmapDays strictly

Test Results

tmux CLI Startup

✅ CLI 正常启动（v0.20.0）

Architecture Review

论点: 输入解析应使用严格正则而非宽松 parseInt。

论据:

1. Before: Number.parseInt(raw, 10) 接受 -5→-5、1.5→1、7junk→7、1e2→1
2. After: /^\d+$/ 正则仅接受纯数字字符串，其他全部回退到默认值
3. 新增测试覆盖：负数、小数、尾部文本、科学记数法

论证: parseInt 的宽松解析是安全边界上的漏洞——7junk 不应该被解析为 7。严格正则 + 回退默认值是输入校验的正确模式。

Verdict

✅ 7/7 测试通过，CLI 正常启动。建议合并。

Verified locally: unit tests + CLI startup on macOS

Code Review — #7218 fix(cli): parse heatmapDays strictly

Verdict: LGTM / merge-ready. Correct, minimal, and it lands on the daemon's existing query-validation convention rather than inventing one. Notes below are optional polish — none blocking.

Local verification

Reverting only the source (not the test) is what proves the new assertions are load-bearing rather than vacuously green.

Old vs new, full matrix

"-5"          old=1    new=183   CHANGED
"1.5"         old=1    new=183   CHANGED
"7junk"       old=7    new=183   CHANGED
"1e2"         old=1    new=183   CHANGED
" 7" / "7 "   old=7    new=183   CHANGED   (not in PR body)
"+5"          old=5    new=183   CHANGED   (not in PR body)
"30.0"        old=30   new=183   CHANGED   (not in PR body)
"0x10"        old=1    new=183   CHANGED   (not in PR body)
"9"×400       old=183  new=366   CHANGED   (not in PR body — see note 2)
"abc" / ""    old=183  new=183
"0"           old=1    new=1                (clamp preserved ✓)
"99999"       old=366  new=366              (clamp preserved ✓)
"007"         old=7    new=7
["5","6"]     old=183  new=183              (Express array → default, unchanged)

All valid-unsigned-integer behavior is preserved exactly as the PR body claims.

1. The convention argument is stronger than the PR body makes it (context, not a change request)

The /^\d+$/-then-parseInt shape isn't ad hoc — it is the house pattern for daemon range params, in two places already:

• packages/cli/src/serve/server/request-helpers.ts:351 — parseMaxQueuedQuery
• packages/cli/src/serve/routes/workspace-file-read.ts:133 — parseIntInRange, whose docstring states it outright:

  "Strict on ^\d+$ so '', 'abc', '1.5', '-3' all reject — the daemon's other range parsers (parseMaxQueuedQuery) use the same regex shape, keeping query-validation behavior consistent."

Worth citing in the PR body, since it pre-empts the obvious reviewer alternative ("why not Number(raw) + Number.isInteger, which would map 1e2 → 100?"). Consistency with the two sibling parsers is a better tiebreaker than either parse semantic on its own.

2. [Low] Dropped Number.isFinite guard — one undocumented behavior delta

The old code's if (!Number.isFinite(value)) is gone. It was load-bearing in exactly one case: a ≥309-digit input overflows parseInt to Infinity.

"9".repeat(400)  →  old: 183 (default)   new: 366 (clamped to MAX)

Not a bug — still bounded by the clamp, and 366 is arguably more consistent (99999 → 366 too, so the old code was the odd one out). And it's not a perf lever: buildHeatmap (usage-dashboard-service.ts:119) iterates records, not days, so a larger window costs nothing extra.

Two small asks:

• Add this row to the PR body's before/after table — it's the one delta not listed.
• Consider one line for exact parity with the siblings, both of which retain a numeric guard (parseIntInRange uses Number.isSafeInteger, parseMaxQueuedQuery uses Number.isFinite):

  const value = Number.parseInt(raw, 10);
  if (!Number.isSafeInteger(value)) return DEFAULT_HEATMAP_DAYS;

  This PR is currently the only one of the three without such a guard.

3. [Low] SDK-side: a non-integer heatmapDays now silently widens the window

DaemonClient.usageDashboard (packages/sdk-typescript/src/daemon/DaemonClient.ts:971-976) types the option as heatmapDays?: number — not integer-constrained — and serializes with String(opts.heatmapDays). So an external SDK consumer computing e.g. 30.5 used to get a 30-day window and now silently gets the 183-day default: a wider window than requested, with no error. Same for useDaemonUsageDashboard in webui.

No in-repo caller is affected — I checked every useDaemonUsageDashboard / loadUsageDashboard call site and none passes heatmapDays at all (web-shell relies on the server default), which also explains the clean serve-A/B bot result. So this is doc-tightening, not a fix:

• Either params.set('heatmapDays', String(Math.floor(opts.heatmapDays))) in the SDK,
• or say "integer" in the jsdoc at DaemonClient.ts:970 and useDaemonUsageDashboard.ts:16-17 (both currently only say "clamped server-side to 1..366", which reads as though any number is accepted).

4. [Nit] Test hygiene — 4 assertions appended to one it()

All four new cases land in the existing it('clamps the heatmapDays query'). My A/B confirms the cost: with the fix reverted, only the -5 case reports (expected 1 to be 183) — the 1.5 / 7junk / 1e2 regressions are masked behind the first failure. it.each would give per-case signal for the same line count:

it.each([
  ['99999', 366], ['0', 1], ['abc', 183],
  ['-5', 183], ['1.5', 183], ['7junk', 183], ['1e2', 183],
])('heatmapDays=%s → %i', async (raw, expected) => {
  const res = await request(app).get(`/usage/dashboard?heatmapDays=${raw}`);
  expect(res.status).toBe(200);
  expect(res.body.heatmapDays).toBe(expected);
});

Also: the block asserts only .body.heatmapDays and never .status. If the route 500s, the failure surfaces as expected undefined to be 183 rather than pointing at the actual cause — the file's other tests do assert status.

5. Scope note (follow-up, not this PR)

This normalizes one of several loose parsers on the same surface. Still outstanding, same bug class:

• workspace-git-log.ts:95-96 — parsePagination uses bare parseInt for ?limit= / ?skip=, so ?limit=7junk → 7
• session.ts:257, workspace-status.ts:115, workspace-channel-observed-contacts.ts:30 — bare Number() (different profile: rejects 7junk, but accepts 1e2, " 7", 1.5)

Fine to leave; worth a tracking issue if the goal is uniform query validation across the daemon.

Security / performance

No impact either way. Both old and new clamp to [1, 366], so no unbounded window was ever reachable; buildHeatmap is O(records) not O(days); the route is an open read-only GET by existing design; and the regex ^\d+$ is linear with no backtracking risk. Multi-value (?heatmapDays=5&heatmapDays=6 → array) still falls to the default, unchanged.

@qwen-code /triage

✅ Qwen Triage finished — view run. See the stage comments in this thread for the result.

✅ Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。