@qwen-code /triage

@qwen-code /triage

✅ Maintainer verification — real qwen channel test under tmux

Verified this PR locally by driving the actual qwen channel status / stop commands against malformed pidfiles in a real terminal (tmux), in addition to the unit suite. Verdict: a correct and worthwhile security/robustness fix — good to merge. This adds 🐧 Linux coverage (the platform where the bad signal semantics actually bite).

Environment

• Isolated git worktree at the PR head (1939cd4c, on current upstream main), fresh npm ci (no symlinked node_modules), real esbuild bundle.
• Linux · Node v22.22.2 · bundled qwen 0.18.3.
• Scope is clean: exactly 2 files, +109 / −19, single commit.

1. Static checks (reproducing the PR test plan)

2. Why this matters (the bug)

process.kill(pid, sig) has special semantics for non-positive pids: pid 0 targets the caller's entire process group, and pid -1 targets every process the user may signal. The pre-PR code trusted whatever pid sat in ~/.qwen/channels/service.pid, so a malformed/leftover pidfile with pid: 0 would (a) be reported as a running service, and (b) make qwen channel stop signal its own process group — a self-kill / DoS.

3. Real command A/B in tmux

Drove the real binary against planted pidfiles (sandboxed HOME). status cases use signal 0 (probe only — safe). The stop case is contained in a fresh setsid process group with a sentinel sleep, so a stray kill(0) is observed without touching anything outside it.

[A] qwen channel status:

The already-handled cases (string/missing pid, corrupt JSON) behave identically — no regression — while pid 0 / pid -1 are the cases the fix closes. Valid running services are still detected and their pidfile preserved.

[B] qwen channel stop with pid: 0 (contained in a setsid group + sentinel):

OLD (pre-PR):  Stopping channel service (PID 0)...
               SENTINEL: KILLED -> kill(0) signaled the whole process group (pid-0 DANGER)

NEW (this PR): No channel service is running.   (stop_exit=0)
               SENTINEL: ALIVE  -> no kill(0); process group untouched (SAFE)

The pre-PR binary printed Stopping … (PID 0)… and then process.kill(0, 'SIGTERM') took down the entire process group (the sentinel died); the PR binary treats the malformed file as "not running" and never signals. Exactly issue #5337's concern, demonstrated safely. (I deliberately did not run stop with pid: -1, since kill(-1, …) is not bounded by a process group — setsid couldn't contain it. The guard makes that path safe too.)

Observations (non-blocking)

1. Scope is intentionally narrow: the guard rejects invalid pids (≤ 0, non-integer, non-safe-integer, NaN, wrong shape). A valid-but-recycled pid (e.g. a stale file whose pid was reused, or pid: 1) is still trusted — but that's PID-reuse, a separate problem this PR doesn't claim to solve. Fine to leave out of scope.
2. startedAt validation is lenient (!Number.isNaN(Date.parse(...))), but pid — the security-relevant field — is strictly validated, and writeServiceInfo only ever emits ISO strings, so real files pass. No concern.
3. channels: [] passes isServiceInfo ([].every(...) is true). Reasonable — a structurally valid record with zero channels.
4. Cross-platform: the dangerous kill(0|-1) semantics are POSIX; the isValidPid guard is platform-agnostic and correct everywhere. Verified on Linux, where the risk is real.

旧版（PR 前）: Stopping channel service (PID 0)...
              哨兵：被杀 -> kill(0) 向整个进程组发了信号（pid-0 危险）

新版（本 PR）: No channel service is running.   (stop_exit=0)
              哨兵：存活 -> 没有 kill(0)；进程组未受影响（安全）

@qwen-code /triage

Thanks for the PR, @tt-a1i!

Template looks good — all required sections filled in ✓

On direction: this is a real safety issue. process.kill(0, sig) targeting the caller's entire process group is a well-known POSIX footgun, and the pidfile code was trusting its input blindly. A planted or corrupted pid: 0 file causing a local DoS via self-signal is exactly the kind of thing worth fixing before someone gets burned. Channel/daemon PID management is squarely within the CLI's core responsibility.

On approach: the scope is tight — 2 files, +109/−19, single-purpose. Three layers of defense (validate on read → guard isProcessAlive → guard signalService/waitForExit) is the right depth. The unlinkPidFile helper consolidation is a natural byproduct, not scope creep. Nothing to cut.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code review: Clean implementation. My independent proposal before reading the diff was: add a isValidPid guard (positive safe integer), parse JSON as unknown with a type guard, and add defensive guards on isProcessAlive/signalService/waitForExit. The PR matches this exactly, with the bonus of consolidating the four scattered try { unlinkSync } blocks into unlinkPidFile.

• isValidPid correctly uses Number.isSafeInteger + pid > 0 — covers 0, -1, floats, NaN, strings, Infinity.
• isServiceInfo type guard is thorough: checks object shape, pid validity, startedAt parseability, and channels array element types.
• The guard placement in isProcessAlive means waitForExit is transitively protected without needing its own explicit check — good layering.
• No correctness bugs, security holes, or regressions found.

No blockers.

Testing:

Unit tests: 16/16 pass (12 original + 4 new). ESLint and Prettier clean.

Real-scenario test on Linux (Node v22.22.3) — planted malformed pidfiles and drove the real qwen channel status vs the PR code:

Before (installed qwen, pre-PR)

Planted service.pid with {"pid":0,...}:

$ HOME=/tmp/triage-pidfile-NhXr9t qwen channel status
Channel service: running (PID 0)
Uptime:          20h 30m

Channel        Sessions
-----------------------
test           0

Pidfile not cleaned up. Bug confirmed — pid:0 treated as a running service, and process.kill(0, 0) would probe the caller's entire process group.

After (this PR, via npm run dev)

Same pid:0 pidfile:

$ HOME=/tmp/triage-pidfile-NhXr9t npm run dev -- channel status
No channel service is running.

Pidfile cleaned up (deleted). Fix confirmed.

Same test with pid:-1:

$ echo '{"pid":-1,...}' > service.pid
$ HOME=/tmp/triage-pidfile-NhXr9t npm run dev -- channel status
No channel service is running.

Pidfile cleaned up. The isValidPid guard rejects -1 before it ever reaches process.kill.

— Qwen Code · qwen3.7-max

Stepping back: this is a textbook defensive fix. The problem is real (POSIX kill(0) semantics are a well-documented footgun), the solution is minimal (3 guard points + 1 helper extraction, no scope creep), and the tests actually prove the fix works — not just "code doesn't crash" but "pid:0 is rejected before reaching process.kill."

My independent proposal from Stage 2 was essentially identical to what the PR does. The unlinkPidFile consolidation was a nice touch I hadn't explicitly planned but is obviously correct.

The tmux A/B test provides clear evidence: the installed build incorrectly reports running (PID 0) and leaves the pidfile in place, while the PR build correctly says "No channel service is running" and cleans up the malformed file. Same result for pid:-1.

Ships cleanly. Approving.

— Qwen Code · qwen3.7-max

@qwen-code /triage