fix(weixin): create the account credential file already private

[chinesepowered]

What this PR does

saveAccount in the WeChat channel stored the account credential by writing the file first and tightening its permissions afterwards. Between those two calls the file carried whatever the process umask allowed — 0644 under the usual 022 — so the API token was group- and world-readable for that window. This PR creates the file already private instead: the JSON is written to a temp file created with mode 0600 and renamed into place, which is the pattern the sibling qqbot channel already uses for its own state file.

Passing mode to writeFileSync on its own is not sufficient, and that is the part worth reviewing carefully. The mode is applied only when the file is created, so an account.json already on disk at 0644 from an older version keeps its permissions. Because rename carries the source mode onto the destination, existing installs are repaired on the next save rather than left as they were.

The temp file is opened with flag: 'wx' (O_CREAT|O_EXCL) under a name drawn from crypto.randomBytes, the form ChannelLoopStore uses. A fixed account.json.tmp would be pre-creatable by anyone able to write to the state directory, and a plain write follows a symlink found there: the token lands in the link target, the mode is silently dropped because the target already exists, and the rename then makes account.json a permanent symlink to it. The unguessable name makes that plant unlikely; O_EXCL makes it ineffective, so the defence does not rest on the name. O_EXCL also guarantees this call creates the file, which is what makes mode take effect at all — it is ignored for a path that already exists. A unique name additionally removes the stale-temp case, so no explicit chmod on the temp file is needed, and stops two concurrent saves interleaving into one file.

clearAccount sweeps account.json.*.tmp alongside the account file. A save killed between the write and the rename leaves a live token under a name that is never reused, so without the sweep the credential would outlive the logout meant to revoke it.

Why it's needed

AccountData.token is the WeChat API token, and the file lives under ~/.qwen/channels/weixin/. On a shared or multi-user host any local user could read it during the window. The window is short, but it is reached on every single save, and the fix costs nothing at runtime.

Making the write atomic is a second, smaller benefit: rename is atomic within a filesystem, so a concurrent reader now sees either the previous account or the new one, never a half-written file. The previous code truncated account.json in place, where an interrupted write left invalid JSON — loadAccount swallows the parse error and returns null, which reads to the user as being silently logged out.

Reviewer Test Plan

How to verify

npx vitest run --root packages/channels/weixin src/accounts.test.ts

18 pass. accounts.ts had no test file before this PR, so the suite also covers the existing loadAccount / clearAccount behaviour.

The tests are pinned to the fix rather than to incidental behaviour. Six mutants, each killed by a distinct, correctly-named test — reverting only accounts.ts and keeping the new test file:

mutant	result
A — revert accounts.ts to base	7 failed | 11 passed (18)
B — naive writeFileSync(p, json, { mode: 0o600 })	7 failed | 11 passed (18)
C — fixed ${p}.tmp name	3 failed | 15 passed (18)
D — delete the whole try/catch cleanup	2 failed | 16 passed (18)
E — drop flag: 'wx'	2 failed | 16 passed (18) — refuses to write through a symlink on the exact temp path
F — drop the clearAccount sweep	1 failed | 17 passed (18) — sweeps a temp file orphaned by a killed save

Mutant A fails:

× saveAccount > never creates the credential file at umask-default permissions
× saveAccount > refuses to write through a symlink on the exact temp path
× saveAccount > never leaves account.json as a symlink
× saveAccount > uses a different temp path on every save
× saveAccount > removes the tmp file and re-throws when the rename fails
× saveAccount > removes the partial tmp file and re-throws when the write fails
× clearAccount > sweeps a temp file orphaned by a killed save
  Tests  7 failed | 11 passed (18)

Worth noting why the first test is written with a spy rather than a statSync check: the 0644 window is transient, so a permission check after saveAccount returns cannot see it — the old code ends at 0600 too, it is just briefly readable on the way there. The test asserts the mode the file is created with, which is the property that actually matters.

The naive fix — replacing the chmodSync with writeFileSync(p, json, { mode: 0o600 }) — fails a different set, which is the regression the temp-and-rename approach avoids:

× saveAccount > narrows a world-readable file left by an older version
× saveAccount > refuses to write through a symlink on the exact temp path
× saveAccount > never leaves account.json as a symlink
× saveAccount > uses a different temp path on every save
× saveAccount > removes the tmp file and re-throws when the rename fails
× saveAccount > removes the partial tmp file and re-throws when the write fails
× clearAccount > sweeps a temp file orphaned by a killed save
  Tests  7 failed | 11 passed (18)

The write-failure mock creates a partial file before throwing, the way a real ENOSPC does. A mock that throws before creating anything makes the cleanup assertion vacuous — it passes with no cleanup code at all, which is why mutant D used to survive it.

Mode and symlink assertions are skipIf(process.platform === 'win32'), matching the core atomicFileWrite suite: Windows reports 0o666/0o444 and chmod moves only the read-only bit.

Full channel suite, tsc --noEmit -p packages/channels/weixin, eslint and prettier --check are all clean:

Test Files  5 passed (5)
     Tests  64 passed (64)

The underlying platform behaviour, if you want to confirm it independently:

$ node -e "…writeFileSync then chmodSync…"
process umask: 022
CURRENT  after writeFileSync: 0644   after chmodSync: 0600
FIXED    after writeFileSync: 0600
RE-WRITE existing 0644 file with {mode:0600} -> 0644   <-- mode ignored for existing files

Evidence (Before & After)

N/A — no user-visible or TUI change; the only observable difference is the file mode, covered by the tests above.

Tested on

OS	Status
🍏 macOS	✅
🪟 Windows	⚠️
🐧 Linux	⚠️

Environment (optional)

Unit tests only. macOS 15 (arm64), Node v22, vitest 3.2.4.

Risk & Scope

• Main risk or tradeoff: saveAccount now creates a uniquely-named temp file alongside the account file. It is removed on both the success and failure paths; a hard kill in between can still leave one behind, so clearAccount sweeps them at logout. It is created at 0600 and never reused, and the sweep matches on account.json.*.tmp only, so nothing else in the state directory (cursor.txt) is touched. Same trade channel-base already makes for its own atomic stores.
• POSIX permission bits are a no-op on Windows, so the mode assertions there are only meaningful in the sense that they do not fail; the atomicity benefit still applies. I marked Windows and Linux ⚠️ because I verified locally on macOS only — CI covers all three.
• Not validated / out of scope: the state directory is still created with default permissions by getStateDir(). The token itself is protected at 0600, so this is not a disclosure path, and changing directory modes is a separate concern from the file write this PR is about. Other channels were checked — qqbot already writes its state with { mode: 0o600 }; the writeFileSync calls in telegram, feishu, dingtalk and WeixinAdapter are downloaded media, not credentials.
• Breaking changes / migration notes: none. Existing account.json files are narrowed to 0600 automatically on the next save; no user action required.

Linked Issues

None.

中文说明

本 PR 做了什么

WeChat 渠道中的 saveAccount 在保存账号凭据时，先写文件、再收紧权限。在这两次调用之间，文件的权限取决于进程 umask —— 在常见的 022 下即为 0644，因此 API token 在该窗口内对同组用户和其他用户可读。本 PR 改为一开始就以私有权限创建文件：将 JSON 写入一个以 mode 0600 创建的临时文件，再 rename 到目标位置——这正是同级渠道 qqbot 保存自身状态文件时已经采用的模式。

仅向 writeFileSync 传入 mode 是不够的，这一点值得重点复审。该 mode 只在文件被创建时生效，因此磁盘上已存在的、由旧版本写入的 0644 account.json 会保留原权限。由于 rename 会把源文件的 mode 带到目标文件上，已有安装会在下一次保存时自动被修复，而不是维持原状。

临时文件以 flag: 'wx'（O_CREAT|O_EXCL）打开，名称取自 crypto.randomBytes，即 ChannelLoopStore 采用的形式。固定的 account.json.tmp 可被任何能写入该状态目录的人预先创建，而普通写入会跟随其中的符号链接：token 落入链接目标，mode 因目标已存在而被静默忽略，随后的 rename 更会让 account.json 永久变成指向它的符号链接。不可猜测的名称让预置难以命中，O_EXCL 则让预置彻底无效——因此该防护并不依赖名称本身。O_EXCL 同时保证本次调用就是创建该文件的调用，这正是 mode 得以生效的前提——对已存在的路径它会被忽略。唯一名称另外消除了「残留临时文件」的情形，因此无需再对临时文件显式 chmod，也避免两个并发保存交错写入同一文件。

clearAccount 在删除账号文件的同时清扫 account.json.*.tmp。保存过程若在写入与 rename 之间被强杀，会留下一个名称永不复用、内含有效 token 的文件；没有这次清扫，该凭据将比意在撤销它的登出活得更久。

为什么需要

AccountData.token 就是 WeChat API token，文件位于 ~/.qwen/channels/weixin/ 下。在共享或多用户主机上，任何本地用户都可能在该窗口内读到它。窗口虽短，但每一次保存都会经过，而修复在运行时没有任何代价。

让写入变为原子操作是第二个较小的收益：rename 在同一文件系统内是原子的，因此并发读取方现在只会看到旧账号或新账号，而不会看到写了一半的文件。此前的代码是就地截断 account.json，一旦写入被中断就会留下非法 JSON —— loadAccount 会吞掉解析错误并返回 null，在用户看来就是莫名其妙地退出登录了。

复审测试计划

如何验证

npx vitest run --root packages/channels/weixin src/accounts.test.ts

18 项通过。本 PR 之前 accounts.ts 没有测试文件，因此该套件同时覆盖了既有的 loadAccount / clearAccount 行为。

这些测试锚定在本次修复上，而非附带行为。共 6 个变异体，每个都被一项名称贴切且互不重叠的测试杀死：

变异体	结果
A —— 将 accounts.ts 还原至基线	7 failed | 11 passed (18)
B —— 朴素改法 writeFileSync(p, json, { mode: 0o600 })	7 failed | 11 passed (18)
C —— 固定的 ${p}.tmp 名称	3 failed | 15 passed (18)
D —— 删除整个 try/catch 清理	2 failed | 16 passed (18)
E —— 去掉 flag: 'wx'	2 failed | 16 passed (18)，含 refuses to write through a symlink on the exact temp path
F —— 去掉 clearAccount 的清扫	1 failed | 17 passed (18)，即 sweeps a temp file orphaned by a killed save

变异体 A 的失败项：

× saveAccount > never creates the credential file at umask-default permissions
× saveAccount > refuses to write through a symlink on the exact temp path
× saveAccount > never leaves account.json as a symlink
× saveAccount > uses a different temp path on every save
× saveAccount > removes the tmp file and re-throws when the rename fails
× saveAccount > removes the partial tmp file and re-throws when the write fails
× clearAccount > sweeps a temp file orphaned by a killed save
  Tests  7 failed | 11 passed (18)

写入失败的 mock 会先落下部分内容再抛出，与真实 ENOSPC 一致。若 mock 在创建任何文件之前就抛出，清理断言便是空断言——即使完全没有清理代码也会通过，这正是变异体 D 此前得以存活的原因。

涉及权限位与符号链接的断言均带 skipIf(process.platform === 'win32')，与 core 的 atomicFileWrite 套件一致：Windows 上 Node 报告 0o666/0o444，且 chmod 只改动只读位。

值得说明第一项测试为何用 spy 而不是 statSync 检查：0644 窗口是瞬时的，因此在 saveAccount 返回之后做权限检查根本看不到它 —— 旧代码最终同样是 0600，只是中途短暂可读。该测试断言的是文件被创建时所用的 mode，这才是真正关键的性质。

而朴素改法 —— 把 chmodSync 换成 writeFileSync(p, json, { mode: 0o600 }) —— 会让另一组测试失败，这正是「临时文件 + rename」方案所规避的回退：

× saveAccount > narrows a world-readable file left by an older version
× saveAccount > refuses to write through a symlink on the exact temp path
× saveAccount > never leaves account.json as a symlink
× saveAccount > uses a different temp path on every save
× saveAccount > removes the tmp file and re-throws when the rename fails
× saveAccount > removes the partial tmp file and re-throws when the write fails
× clearAccount > sweeps a temp file orphaned by a killed save
  Tests  7 failed | 11 passed (18)

The write-failure mock creates a partial file before throwing, the way a real ENOSPC does. A mock that throws before creating anything makes the cleanup assertion vacuous — it passes with no cleanup code at all, which is why mutant D used to survive it.

Mode and symlink assertions are skipIf(process.platform === 'win32'), matching the core atomicFileWrite suite: Windows reports 0o666/0o444 and chmod moves only the read-only bit.

渠道完整套件、tsc --noEmit -p packages/channels/weixin、eslint 与 prettier --check 均干净：

Test Files  5 passed (5)
     Tests  64 passed (64)

如需独立确认底层平台行为：

$ node -e "…writeFileSync 后 chmodSync…"
process umask: 022
CURRENT  after writeFileSync: 0644   after chmodSync: 0600
FIXED    after writeFileSync: 0600
RE-WRITE existing 0644 file with {mode:0600} -> 0644   <-- 对已存在文件，mode 被忽略

证据（前后对比）

N/A —— 无用户可见或 TUI 变化；唯一可观测的差异是文件权限位，已由上述测试覆盖。

测试平台

OS	状态
🍏 macOS	✅
🪟 Windows	⚠️
🐧 Linux	⚠️

环境（可选）

仅单元测试。macOS 15（arm64），Node v22，vitest 3.2.4。

风险与范围

• 主要风险或权衡：saveAccount 现在会在账号文件旁创建一个唯一命名的临时文件。成功与失败路径都会将其删除，但在写入与 rename 之间被强制杀死仍可能残留一个 —— 无害且以 0600 创建，不过由于名称不再复用，它会累积而非被下次保存覆盖。这属于文件残留而非权限暴露，也正是 channel-base 为其自身原子写入所做的同样取舍。
• POSIX 权限位在 Windows 上是空操作，因此那里的权限断言的意义仅在于「不会失败」；原子性收益仍然成立。我把 Windows 与 Linux 标为 ⚠️，因为本地只在 macOS 上验证过 —— CI 覆盖三个平台。
• 未验证 / 范围之外：状态目录仍由 getStateDir() 以默认权限创建。token 本身已受 0600 保护，因此这不构成泄露路径；修改目录权限与本 PR 关注的文件写入是彼此独立的问题。其他渠道已检查 —— qqbot 已经使用 { mode: 0o600 } 写入状态；telegram、feishu、dingtalk 与 WeixinAdapter 中的 writeFileSync 写的是下载的媒体文件，不是凭据。
• 破坏性变更 / 迁移说明：无。已有的 account.json 会在下一次保存时自动收紧为 0600，用户无需任何操作。

关联 Issue

无。

[qwen-code-ci-bot]

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

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

[qwen-code-ci-bot]

Thanks for the PR — and for the thorough rework since the last pass.

Template looks good ✓

Problem: real and demonstrated. The write-then-chmod TOCTOU is reproducible with a one-liner (node -e showing 0644 between the two syscalls), and every save hits it. Not theoretical.

Direction: squarely aligned — credential storage security is core to the project's trust model. The sibling qqbot channel already uses this exact temp-and-rename pattern, so this brings weixin in line with an established convention.

Size: not applicable (no core paths touched — packages/channels/weixin is outside Stage 0 scope). Production logic: 55 lines (51+4 in accounts.ts); test: 290 lines (new file).

Approach: the scope is tight and minimal — write to a 0600 temp file with O_CREAT|O_EXCL, rename into place, clean up on failure, sweep orphans on logout. The rework since the last review is strictly better: randomBytes replaces the guessable Date.now()/pid/Math.random() suffix, wx closes the symlink-follow hole by construction, and the now-unnecessary chmodSync on the temp was correctly removed (a unique name can't collide with a stale one). Nothing to cut.

Moving on to code review. 🔍

中文说明

感谢贡献——也感谢自上次审查以来的彻底重构。

模板完整 ✓

问题：真实且已演示。write-then-chmod TOCTOU 可用一行命令复现（node -e 展示两次系统调用之间的 0644），且每次保存都会经过。不是理论问题。

方向：完全对齐——凭据存储安全是项目信任模型的核心。同级 qqbot 渠道已使用完全相同的「临时文件 + rename」模式，本 PR 让 weixin 与既有惯例保持一致。

规模：不适用（未触及核心路径——packages/channels/weixin 不在 Stage 0 范围内）。生产逻辑：55 行（accounts.ts 中 51+4）；测试：290 行（新文件）。

方案：范围紧凑且最小化——以 O_CREAT|O_EXCL 写入 0600 临时文件、rename 到位、失败时清理、登出时清扫残留。自上次审查以来的重构严格更好：randomBytes 替代了可猜测的 Date.now()/pid/Math.random() 后缀，wx 从构造上关闭了符号链接跟随漏洞，不再需要的 chmodSync 被正确移除（唯一名称不会与残留文件碰撞）。没有可砍的部分。

进入代码审查 🔍

— Qwen Code · qwen3.8-max-preview

_{Reviewed at 0ab1c48aaffc004042afeef4c181fdc4bad5cfa2 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Code Review

Independent proposal: for a write-then-chmod TOCTOU on a credential file, I'd write to a uniquely-named temp file created at 0600 with O_CREAT|O_EXCL (blocks symlink plants), rename into place (atomic, carries the mode, repairs existing files), clean up the temp on failure, and sweep orphaned temps on logout.

Comparison with the diff: the PR does exactly this. randomBytes(6) for the name (matching ChannelLoopStore), flag: 'wx' for O_EXCL, mode: 0o600, renameSync for atomicity. No simpler path exists — writeFileSync with { mode } alone can't narrow an existing file, and a fixed temp name is pre-creatable. The implementation is the minimal correct fix.

No critical blockers. No convention violations. Specifics:

• The previous review's chmodSync(tmp, 0o600) is gone, and correctly so — it existed for the stale-tmp case (a crashed run's leftover keeps its own permissions because mode is ignored on an existing path), and a name drawn from randomBytes can never collide with one. Removing a line whose comment describes a case that no longer exists is the right call.
• flag: 'wx' (O_CREAT|O_EXCL) does double duty: it refuses a symlink at the final component instead of following it, and it guarantees this call creates the file — which is what makes mode take effect at all. Well-commented.
• renameSync carries the 0600 onto the destination, so existing 0644 files from older versions are repaired on next save. Correct.
• Error path: best-effort unlinkSync(tmp) in a nested try/catch, then rethrows. Cleanup failure doesn't mask the original error. Correct.
• clearAccount sweeps account.json.*.tmp via readdirSync — a killed save leaves a live token in a file whose name is never reused, so logout must remove it. The glob is tight enough to leave cursor.txt alone.
• The clearAccount refactor from accountPath() to inline getStateDir() + join avoids a double getStateDir() call (which would mkdirSync twice). Minor, but intentional.
• Test design is strong: the spy-based mode assertion is the only way to pin the transient creation window (post-hoc statSync can't see it), the symlink tests exercise both the temp-path and destination-path attack vectors, and the ENOSPC mock creates a partial file before throwing (a mock that throws first would make the cleanup assertion vacuous). 18 tests, each killing a named mutant.
• Follows project conventions: ESM, vitest, collocated test, license header, no any, kebab-case filenames.

Testing

N/A — no user-visible or TUI change. The only observable difference is the file permission bits on disk, covered by the unit tests.

CI Evidence

All checks completed on 0ab1c48:

Check	Conclusion
Test (ubuntu-latest, Node 22.x)	✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x)	✅ success
Classify PR	✅ success

_{One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。}

macOS and Windows test jobs are skipped (fork PR runner limitation). No failures.

中文说明

代码审查

独立方案： 针对凭据文件的 write-then-chmod TOCTOU，我会将数据写入以 O_CREAT|O_EXCL 创建的 0600 唯一命名临时文件（阻止符号链接预置），rename 到位（原子操作、携带 mode、修复已有文件），失败时清理临时文件，登出时清扫残留。

与 diff 对比： PR 的实现与此完全一致。randomBytes(6) 生成名称（与 ChannelLoopStore 一致），flag: 'wx' 实现 O_EXCL，mode: 0o600，renameSync 保证原子性。不存在更简路径。实现是最小的正确修复。

无关键阻塞项，无惯例违反。

测试

N/A——无用户可见或 TUI 变化。唯一可观测的差异是磁盘上的文件权限位，已由单元测试覆盖。

CI 证据

0ab1c48 上所有检查已完成：ubuntu 测试套件 ✅，web-shell E2E ✅，PR 分类 ✅。macOS 和 Windows 测试任务被跳过（fork PR 运行器限制）。无失败。

— Qwen Code · qwen3.8-max-preview

_{Reviewed at 0ab1c48aaffc004042afeef4c181fdc4bad5cfa2 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Confidence: 5/5 — clean across every stage; the rework made a strong PR stronger.

The previous pass reviewed e265343 and flagged two things: the predictable temp path and the untested error cleanup. Both are now addressed, and the fix is strictly better for it. randomBytes + O_EXCL closes the symlink-follow hole by construction rather than by obscurity, and removing the now-purposeless chmodSync shows the author understood why each line existed rather than just accumulating defenses.

The test suite is the standout. Eighteen tests, each pinned to a named mutant, covering attack vectors (symlink on temp path, symlink on destination, planted fixed-name temp), failure modes (ENOSPC mid-write, EXDEV on rename), and the subtle repair-on-save path that a naive { mode } fix would silently miss. The ENOSPC mock creates a partial file before throwing — the kind of detail that separates a real test from a vacuous one.

Two files, one fix, one test. No scope creep, no drive-by changes. If I had to maintain this in six months, I'd thank the author.

CI is green on 0ab1c48 — approving now.

中文说明

置信度：5/5 —— 每个阶段都干净；重构让一个强 PR 更强。

上一次审查在 e265343 上标记了两点：可预测的临时路径和未测试的错误清理。两者现已解决，修复因此严格更好。randomBytes + O_EXCL 从构造上而非依赖不可猜测性关闭了符号链接跟随漏洞，移除不再有用途的 chmodSync 表明作者理解每一行存在的原因，而非仅仅堆叠防御。

测试套件是亮点。十八项测试，每项锚定一个命名变异体，覆盖攻击向量（临时路径上的符号链接、目标路径上的符号链接、预置的固定名称临时文件）、失败模式（写入中途 ENOSPC、rename 时 EXDEV），以及朴素 { mode } 修复会静默遗漏的「保存时修复」路径。ENOSPC mock 在抛出前先创建部分文件——这正是区分真实测试与空断言的细节。

两个文件，一个修复，一个测试。无范围蔓延，无顺手改动。批准。

— Qwen Code · qwen3.8-max-preview

_{Reviewed at 0ab1c48aaffc004042afeef4c181fdc4bad5cfa2 · re-run with @qwen-code /triage}

[gwinthis]

Review + real-filesystem verification report (Linux, umask 022, tmux)

Verdict: correct, well-reasoned, and every claim in the PR description checks out empirically — including the subtle ones. One defense-in-depth suggestion on the directory mode.

Verification evidence (commit e265343)

Probe run against the PR implementation on a real filesystem under umask 022:

OLD pattern, mode after write (before chmod): 644  <- token readable window (premise demonstrated)
before save: account.json = 644 , stale tmp = 644  (simulating an older install + crashed run)
after save:  account.json = 600 (narrowed from 644)
round-trip token: t0k3n
tmp gone (renamed away) ✓

• Premise real: replaying the old write-then-chmod sequence shows the file at 644 between the calls — the window exists on every save, exactly as described.
• Repair-on-save works: a pre-existing 0644 account.json from an "older version" ends at 0600 after one save, via rename carrying the tmp's mode — the PR's most subtle claim, confirmed on disk.
• Stale-tmp hardening works: a crashed-run 0644 tmp is re-narrowed before rename (the explicit chmodSync covers the mode-only-on-create gap).
• Tests: 11/11 pass; reverting only accounts.ts fails exactly the two the author names — the suite pins the fix, and the pass-through writeFileSync spy is the right trick, since post-hoc permission checks cannot see the window.

Code review reasoning

1. The three-step tmp dance (writeFileSync(mode: 0600) → chmodSync → renameSync) closes both holes the naive mode: option leaves open, and each step's comment states the exact gap it covers. The error path unlinks the tmp best-effort and rethrows — correct.
2. The atomicity side-benefit is real: the old in-place truncate could leave invalid JSON that loadAccount swallows into null (silent logout); rename makes readers see old-or-new only. Same-directory rename, so no cross-device concern.
3. Matches the sibling qqbot channel's established pattern — consistency, not invention.

Suggestion (non-blocking)

getStateDir() creates ~/.qwen/channels/weixin with mkdirSync(dir, { recursive: true }) — umask default, measured at 755 in my probe. The 0600 file already protects the token, but a mode: 0o700 on the directory (the ~/.ssh convention) would add defense-in-depth and also stop other local users from even listing the credential file's existence, at zero cost. Existing dirs would need the same narrow-on-touch treatment this PR gave the file, so it may be cleaner as a tiny follow-up.

[qwen-code-ci-bot]

LGTM, looks ready to ship — CI landed green after the review. ✅

[qwen-code-ci-bot]

Reviewed — no blockers. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] The inline temp-file-and-rename re-implements a subset of atomicWriteFileSync from packages/core/src/utils/atomicFileWrite.ts, which already provides this pattern plus random tmp suffixes (crypto.randomBytes(6)), fsync before rename, EPERM/EACCES retry with exponential backoff, and EXDEV cross-filesystem fallback. The same manual pattern is also duplicated in group-history-store.ts and SessionRouter.ts in channel-base. — Concrete cost: the predictable ${p}.tmp path means two concurrent saveAccount calls (e.g. two CLI processes) write to the same tmp file, corrupting one another's data before rename; the shared utility avoids this with random suffixes. Without fsync, a crash mid-rename can leave the destination with stale data.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] The error path is untested — no test verifies that the tmp file is cleaned up or that the original error propagates when writeFileSync, chmodSync, or renameSync throws. — Concrete cost: if a future change accidentally removes unlinkSync(tmp) or the throw e, no test would catch it. A swallowed error would silently lose credential saves; missing cleanup would leave orphaned .tmp files after every failed save. The mocking infrastructure (vi.mock('node:fs', ...)) is already in place to add this.

Suggested change

	try {
	unlinkSync(tmp);
	} catch {
	/* best-effort cleanup */
	}
	throw e;
	try {
	unlinkSync(tmp);
	} catch {
	/* best-effort cleanup */
	}
	throw e;

Consider adding a test like:

it('cleans up the tmp file and re-throws on write failure', () => {
  vi.mocked(writeFileSync).mockImplementationOnce(() => {
    throw new Error('disk full');
  });
  expect(() => saveAccount(DATA)).toThrow('disk full');
  expect(existsSync(join(stateDir, 'account.json.tmp'))).toBe(false);
});

— qwen3.7-max via Qwen Code /review

[chinesepowered]

Thanks @gwinthis for the real-filesystem probe — confirming the repair-on-save path on disk is the claim I was least able to demonstrate from a unit test, and having it replayed independently under umask 022 is worth more than my node -e snippet.

Three review items, one taken as code, one declined with evidence, one deferred. Pushed as d56f725a6.

Taken: the predictable temp path

The ${p}.tmp suggestion is right, and I think the stronger reason is security rather than the concurrency one given. A fixed name in a world-writable-ish directory is pre-creatable: anyone who can write to the state dir can drop a symlink at account.json.tmp, and writeFileSync follows it — so the token lands wherever it points. That is a hole this PR opens, since before it there was no temp file at all, and it sits squarely inside what this PR is about. The concurrent-save corruption you describe is real too, just narrower.

So the temp name is now the unique-suffix form channel-base already uses for its own atomic stores (group-history-store.ts:259, SessionRouter.ts:765):

const tmp = `${p}.${Date.now()}-${process.pid}-${Math.random().toString(16).slice(2)}.tmp`;

The explicit chmodSync(tmp, 0o600) is gone with it, and I want to flag that rather than let it pass as a silent deletion. It existed for exactly one reason — a stale temp from a crashed run keeps its own permissions because mode is ignored when the path already exists — and a name that is never reused cannot collide with one. Leaving it in would have meant keeping a line whose comment described a case that can no longer happen, which is worse than not having it. The trade-off is that a crashed run now leaks one orphaned temp file instead of leaving one reusable one; that is litter, not exposure, and it's the same trade channel-base already makes.

Taken: the untested error path

Added, and I used the rename rather than the write, since that's the step that fails with the temp file actually on disk — a writeFileSync failure leaves nothing to clean up, so it can't tell whether unlinkSync still exists. Both are covered now:

• removes the tmp file and re-throws when the rename fails — renameSync throws EXDEV, asserts the error propagates and no .tmp survives.
• re-throws a write failure without leaving a credential file — ENOSPC on the write, asserts no account.json is created.

Declined: reusing atomicWriteFileSync

I looked at packages/core/src/utils/atomicFileWrite.ts and it is genuinely better than what's here — forceMode is documented for precisely this "credential file restored from backup at 0644" case, and noFollow closes the symlink hole by construction. I'd use it if I could. The blocker is a package boundary:

$ grep -rl "qwen-code-core" packages/channels/ --include="*.ts"
(no matches)

No file under packages/channels/** imports core. @qwen-code/channel-weixin depends only on @qwen-code/channel-base, as do qqbot and every other adapter; channel-base itself depends only on @agentclientprotocol/sdk. Adopting the utility means adding a channel→core dependency edge that does not currently exist anywhere in the tree — an architectural decision for a maintainer, not something to slip into a permissions fix. That is also why qqbot, group-history-store.ts and SessionRouter.ts each hand-roll the pattern: not oversight, but the boundary. Hoisting a shared atomic-write helper into channel-base and converting all four call sites is a real improvement and I'd be glad to open it as its own PR if a maintainer wants it.

Deferred: getStateDir() at 0700

@gwinthis — agreed on the merit, and ~/.ssh is the right precedent. I'm keeping it out of this PR deliberately: existing directories need the same narrow-on-touch treatment the file just got, so it is its own change with its own before/after tests, and this PR is already on its second review round. You suggested a follow-up yourself and I think that's right. Happy to open it.

Verification

packages/channels/weixin 14/14. Against main's accounts.ts with the new tests kept, 3 fail — never creates the credential file at umask-default permissions, uses a different temp path on every save, removes the tmp file and re-throws when the rename fails. Against the naive { mode }-only fix, a different 3 fail — narrows a world-readable file left by an older version, plus the same two — so the suite pins both the original bug and the wrong fix. eslint, prettier --check and tsc --noEmit -p packages/channels/weixin all clean.

[qwen-code-ci-bot]

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

中文说明

⚠️ 已从批准降级为评论：CI still running。 已审查。

— qwen3.7-max via Qwen Code /review

[wenshao]

Review — fix(weixin): create the account credential file already private

Reviewed at head d56f725a6 in an isolated worktree. I re-ran the suite and reproduced every mutation claim rather than taking the description's numbers at face value.

Verdict: the fix is correct and the tests are genuinely pinned — approve-with-nits. Nothing below blocks merge; item 1 is the one I'd most like addressed because the code comment leans on a defense that is weaker than it reads.

---

What I verified

Check	Result
npx vitest run --root packages/channels/weixin	67 passed (5 files)
npx vitest run --root packages/channels/weixin src/accounts.test.ts	14 passed
tsc --noEmit -p packages/channels/weixin / eslint / prettier --check	clean
Mutation A — revert only accounts.ts to base	3 failed | 11 passed (14) — never creates … umask-default permissions, uses a different temp path on every save, removes the tmp file and re-throws when the rename fails
Mutation B — naive writeFileSync(p, json, { mode: 0o600 })	3 failed | 11 passed (14) — narrows a world-readable file left by an older version, + the two temp-path tests
Mutation C — fixed ${p}.tmp name (i.e. commit e265343e1)	2 failed | 12 passed (14) — never writes through a planted account.json.tmp, uses a different temp path on every save
Mutation D — delete the whole try/catch cleanup	1 failed | 13 passed (14) — see nit 3

Each variant is killed by a distinct, correctly-named test. That is a well-built matrix.

The underlying platform claims also hold — I reproduced them directly:

flag 'w'  (current): wrote through a planted symlink -> target now = SECRET-TOKEN, target mode = 644
flag 'wx':           refused -> EEXIST | target still = ORIGINAL
after renameSync(symlink, dest): dest isSymbolicLink = true

What's good

• The diagnosis is right: write-then-chmod really does pass through 0644 under umask 022, and mode really is ignored for an existing file — so the obvious one-line fix would strand old 0644 files. Using rename to carry the mode across is the correct mechanism, not a stylistic preference.
• An unmentioned bonus: the old writeFileSync(p, …) would have followed a symlink planted at account.json itself. renameSync replaces the symlink instead. That's a second real hardening the description doesn't claim credit for.
• The atomicity argument is real, not decorative — truncate-in-place plus loadAccount's swallowed parse error (accounts.ts:45) genuinely surfaces as a silent logout.
• accounts.ts had no test file at all before this; the suite also backfills loadAccount / clearAccount.
• Comments explain why rather than restating the code, matching the surrounding style.

---

1. The temp write can still be redirected through a planted symlink (accounts.ts:52-69)

The comment makes the unguessable name the load-bearing defense. It is weaker than that framing implies, in two independent ways:

• writeFileSync defaults to flag 'w' (O_WRONLY|O_CREAT|O_TRUNC), which follows a symlink at the final component. My run above confirms it: the token lands in the symlink target and mode: 0o600 is silently dropped because the file "already exists" (target stayed 0644). renameSync then makes account.json itself a permanent symlink.
• Math.random() is V8's xorshift128+, not a CSPRNG, and Date.now()-pid are guessable. The sibling store in this repo that explicitly needs an unguessable temp name — ChannelLoopStore.ts:214 — uses crypto.randomBytes(6).toString('hex').

Both close deterministically for the cost of a flag and an import:

import { randomBytes } from 'node:crypto';

const tmp = `${p}.${randomBytes(6).toString('hex')}.tmp`;
// …
writeFileSync(tmp, JSON.stringify(data, null, 2), {
  encoding: 'utf-8',
  mode: 0o600,
  flag: 'wx', // O_CREAT|O_EXCL — refuses to open an existing path or a symlink
});

'wx' also makes the "unique name guarantees this call creates the file" comment enforced rather than asserted. To be fair on severity: reaching this needs the state dir to be attacker-writable, which the default ~/.qwen/channels/weixin (0755) is not for another unprivileged user — it's reachable mainly via WEIXIN_STATE_DIR pointed at a shared directory. So: hardening, not a blocker.

2. The cited precedent also hardens the directory (accounts.ts:26-34)

The PR declares directory mode out of scope, which is defensible — but all three channel-base atomic stores it points at do tighten it, and that is what actually removes the plant vector from item 1:

• ChannelLoopStore.ts:211-212 — mkdir(dir, { mode: 0o700 }) + chmod(dir, 0o700).catch(() => {})
• group-history-store.ts:225,239 — chmodPrivate(dir, 0o700)
• SessionRouter.ts:767-772 — mkdirSync(dir, { mode: 0o700 }) + guarded chmodSync

getStateDir() does plain mkdirSync(dir, { recursive: true }) → 0755 under umask 022, and monitor.ts:21 stores cursor.txt in the same directory. Two lines in getStateDir() would land it, and it's the same function the PR already touches conceptually. Your call whether it belongs in this PR or a follow-up.

3. re-throws a write failure without leaving a credential file doesn't cover the cleanup path (accounts.test.ts:157-166)

Mutation D — deleting the entire try/catch and keeping only write + rename — leaves this test green. Only removes the tmp file and re-throws when the rename fails catches it. The reason is that the mocked writeFileSync throws before creating anything, so expect(tmpFiles()).toEqual([]) is trivially satisfied. The existsSync(account.json) === false half is still meaningful; it's the tmp-cleanup half that's vacuous. Making the mock write a partial file and then throw (a realistic ENOSPC) would pin the write-path cleanup too.

4. The weixin package has no test script, so this suite never runs in CI

CI's gate is npm run test:ci → npm run test:ci --workspaces --if-present (ci.yml:364, and the macOS/Windows merge-queue jobs at :559/:614). packages/channels/weixin/package.json defines only build, so all five of its test files — including this one — are silently skipped. Confirmed: npm run test:ci --workspace=packages/channels/weixin --if-present exits 0 with no output. base, telegram, dingtalk and wecom all define test + test:ci.

This is a pre-existing repo gap, not something the PR introduced — but this PR's entire value rests on a regression gate that currently doesn't fire. Three lines make the evidence permanent:

"test": "vitest run",
"test:ci": "vitest run"

5. Reviewer Test Plan numbers are stale relative to the head commit

The body says 11 tests and quotes Tests 2 failed | 9 passed (11) for both mutations. Head is 14 tests, and both mutations reproduce as 3 failed | 11 passed (14). The three test names listed in each block are exactly right — only the counts drifted, describing e265343e1 rather than d56f725a6. Worth refreshing since the template is CI-enforced and reviewers key off those numbers.

6. Two smaller notes

• Windows. renameSync over a destination held open by another process fails EPERM, where truncate-in-place would have succeeded. The only caller (packages/cli/src/commands/channel/configure.ts:63) catches it and prints Login failed: …, which is a bit misleading — login succeeded, only persistence failed. Same trade the repo already accepts in three other stores, so I'd leave it; just flagging it since Windows is ⚠️ in the table.
• clearAccount doesn't sweep orphans. A hard kill between write and rename leaves a uniquely-named .tmp holding a live token at 0600. Because the name is never reused, logout (clearAccount) removes account.json and leaves the token behind indefinitely. That's slightly more than "litter" — a readdirSync(dir).filter(f => f.endsWith('.tmp')) sweep in clearAccount would close it.

---

中文说明

在隔离 worktree 中基于 head d56f725a6 复审。我没有直接采信描述中的数字，而是重跑了测试并逐一复现了所有变异验证。

结论：修复正确、测试锚定扎实——建议在处理若干小问题后合并。 下列各项均不阻塞合并；第 1 项最值得处理，因为代码注释所依赖的防护比它读起来要弱。

已验证内容

检查项	结果
npx vitest run --root packages/channels/weixin	67 通过（5 个文件）
src/accounts.test.ts	14 通过
tsc --noEmit / eslint / prettier --check	干净
变异 A —— 仅还原 accounts.ts	3 失败 | 11 通过（14）
变异 B —— 朴素改法 writeFileSync(p, json, { mode: 0o600 })	3 失败 | 11 通过（14）
变异 C —— 固定 ${p}.tmp 名称（即提交 e265343e1）	2 失败 | 12 通过（14）
变异 D —— 删除整个 try/catch 清理	1 失败 | 13 通过（14）

每个变体都被一项名称贴切且互不重叠的测试杀死，这是一套构造良好的测试矩阵。底层平台行为我也直接复现了：flag 'w' 会写穿预置的符号链接（目标权限保持 644），flag 'wx' 则以 EEXIST 拒绝；对符号链接执行 renameSync 后目标本身即为符号链接。

做得好的地方

• 诊断准确：在 umask 022 下「先写后 chmod」确实会途经 0644，且对已存在文件 mode 确实被忽略——因此显而易见的一行改法会把旧的 0644 文件留在原地。用 rename 携带权限位是正确机制，而非风格偏好。
• 一个描述里没提到的额外收益：旧代码的 writeFileSync(p, …) 会跟随预置在 account.json 本身的符号链接，而 renameSync 是直接替换它。
• 原子性论证是实打实的：就地截断加上 loadAccount（accounts.ts:45）吞掉解析错误，确实会表现为莫名退出登录。
• accounts.ts 此前完全没有测试，该套件同时补齐了 loadAccount / clearAccount。

1. 临时文件写入仍可能被预置的符号链接重定向（accounts.ts:52-69）

注释把「名称不可猜测」当作主要防线，但它在两个彼此独立的方面都更弱：

• writeFileSync 默认 flag 为 'w'（O_WRONLY|O_CREAT|O_TRUNC），会跟随末段符号链接。上面的实测确认：token 落到了链接目标里，而 mode: 0o600 因为「文件已存在」被静默忽略（目标仍为 0644）；随后的 renameSync 会让 account.json 本身永久变成一个符号链接。
• Math.random() 是 V8 的 xorshift128+，并非密码学安全随机源，Date.now() 与 pid 也可猜测。本仓库中明确需要不可猜测临时名的同类实现 —— ChannelLoopStore.ts:214 —— 用的是 crypto.randomBytes(6).toString('hex')。

改用 randomBytes 并加上 flag: 'wx'（O_CREAT|O_EXCL，拒绝已存在路径与符号链接）即可确定性地关闭这两点，同时让「唯一名称保证本次调用创建文件」这句注释从断言变成强制约束。就严重程度而言：利用它需要状态目录对攻击者可写，而默认的 ~/.qwen/channels/weixin（0755）对其他普通用户并不可写——主要经由 WEIXIN_STATE_DIR 指向共享目录才可达。因此属于加固项，而非阻塞项。

2. 所引用的先例同时收紧了目录权限（accounts.ts:26-34）

PR 声明目录权限不在范围内，这可以理解——但它所引用的三处 channel-base 原子写入实现都收紧了目录，而这恰恰是消除第 1 项攻击面的关键：ChannelLoopStore.ts:211-212、group-history-store.ts:225,239、SessionRouter.ts:767-772 均为 mkdir(dir, { mode: 0o700 }) 加尽力而为的 chmod 0o700。而 getStateDir() 只是 mkdirSync(dir, { recursive: true })，在 umask 022 下即 0755，且 monitor.ts:21 的 cursor.txt 也存放在同一目录。是否纳入本 PR 由你决定。

3. re-throws a write failure without leaving a credential file 并未覆盖清理路径（accounts.test.ts:157-166）

变异 D（删掉整个 try/catch，只保留 write + rename）后该测试仍为绿，只有 removes the tmp file and re-throws when the rename fails 能捕获。原因是被 mock 的 writeFileSync 在创建任何文件之前就抛出，因此 expect(tmpFiles()).toEqual([]) 天然成立。断言 account.json 不存在的那一半仍有意义，失效的是临时文件清理那一半。让 mock 先写入部分内容再抛出（模拟真实的 ENOSPC）即可把写入路径的清理也锚定住。

4. weixin 包没有 test 脚本，因此该套件在 CI 中根本不会运行

CI 的门禁是 npm run test:ci → npm run test:ci --workspaces --if-present（ci.yml:364，以及 :559/:614 的 macOS/Windows 合并队列任务）。packages/channels/weixin/package.json 只定义了 build，因此它的全部 5 个测试文件——包括本次新增的——都被静默跳过。已确认：npm run test:ci --workspace=packages/channels/weixin --if-present 无输出且退出码 0。base、telegram、dingtalk、wecom 都定义了 test 与 test:ci。

这是既有的仓库缺口而非本 PR 引入，但本 PR 的全部价值恰恰依赖一个当前并不会触发的回归门禁。补上 "test": "vitest run" 与 "test:ci": "vitest run" 即可让这些证据长期生效。

5. 复审测试计划中的数字相对 head 提交已过期

正文写的是 11 项测试，两处变异均引用 Tests 2 failed | 9 passed (11)。head 实际为 14 项，两处变异我复现的结果都是 3 failed | 11 passed (14)。两个代码块中列出的三个测试名称完全正确，只是计数停留在 e265343e1 而非 d56f725a6。鉴于模板受 CI 强制且复审者会依据这些数字判断，建议刷新。

6. 两点小注记

• Windows。 当目标文件被其他进程打开时 renameSync 会以 EPERM 失败，而就地截断本可成功。唯一调用方（packages/cli/src/commands/channel/configure.ts:63）会捕获并打印 Login failed: …，措辞略有误导——登录其实已成功，失败的只是持久化。这与仓库另外三处存储已接受的取舍一致，我倾向保持现状，只是鉴于表格中 Windows 标为 ⚠️ 而提示一下。
• clearAccount 不会清扫残留。 在写入与 rename 之间被强杀会留下一个唯一命名、权限 0600、内含有效 token 的 .tmp 文件。由于名称不再复用，退出登录（clearAccount）只会删除 account.json，token 则被长期留在磁盘上。这比「仅是文件残留」略重一些；在 clearAccount 中加一次 readdirSync(dir).filter(f => f.endsWith('.tmp')) 清扫即可关闭。

[chinesepowered]

Thanks — reproducing the mutation matrix instead of taking my numbers, and probing flag 'w' against a real symlink, is what turned item 1 from a style note into something I had to fix. Three taken, pushed as 0ab1c48aa. Two declined, with reasons.

1. Taken — and you're right that my comment was the actual defect

The comment made the unguessable name load-bearing, and your flag 'w' probe shows it isn't: the write follows the symlink, mode: 0o600 is dropped because the target "already exists", and the rename then makes account.json a permanent symlink. Both halves fixed — crypto.randomBytes(6) for the suffix, flag: 'wx' on the write.

What I'd add is that this was untestable before, which is why it read as stronger than it was. With a Math.random() suffix a test cannot plant on the temp path — so the defence could only ever be asserted in prose. Spying randomBytes makes it pinnable, so the claim is now enforced by a test rather than by a comment:

posixOnly('refuses to write through a symlink on the exact temp path', () => {
  const suffix = randomBytes(6).toString('hex');
  writeFileSync(victim, 'not the token', 'utf-8');
  vi.mocked(randomBytes).mockReturnValueOnce(Buffer.from(suffix, 'hex'));
  symlinkSync(victim, join(stateDir, `account.json.${suffix}.tmp`));

  expect(() => saveAccount(DATA)).toThrow(/EEXIST/);
  expect(readFileSync(victim, 'utf-8')).toBe('not the token');
});

Dropping flag: 'wx' fails it. That mutant was unkillable before this commit.

I also took your unclaimed bonus as a test rather than leaving it as description credit — never leaves account.json as a symlink plants on the destination and asserts lstatSync(...).isSymbolicLink() === false. It was true by accident of the rename; now it's pinned.

3. Taken — the assertion really was vacuous

You're right that the mock threw before creating anything, so expect(tmpFiles()).toEqual([]) was trivially satisfied. The mock now writes a partial file first, the way a real ENOSPC does:

vi.mocked(writeFileSync).mockImplementationOnce((path) => {
  appendFileSync(path as string, `{"token":"${DATA.token.slice(0, 8)}`);
  throw new Error('ENOSPC: no space left on device');
});

Mutant D now fails 2 instead of 1 — the write-path cleanup is covered.

6b. Taken — the orphan is this PR's doing, so it's this PR's to close

I'd filed this under "litter" in my own description, and re-reading it with your framing that's wrong: the file holds a live token, and because the name is never reused nothing else would ever remove it. Before this PR there was no temp file at all, so this is a hole my change opens — the same test I applied to the checkout regression on #7531. clearAccount now sweeps account.json.*.tmp. A test pins that cursor.txt in the same directory is untouched.

Full matrix at 0ab1c48aa — 18 tests, six mutants

mutant	yours at d56f725a6	now
A — revert accounts.ts to base	3 failed	7 failed | 11 passed
B — naive { mode: 0o600 }	3 failed	7 failed | 11 passed
C — fixed ${p}.tmp	2 failed	3 failed | 15 passed
D — delete the try/catch	1 failed	2 failed | 16 passed
E — drop flag: 'wx'	unkillable	2 failed | 16 passed
F — drop the clearAccount sweep	n/a	1 failed | 17 passed

packages/channels/weixin 71 passed (5 files); tsc --noEmit -p packages/channels/weixin, eslint, prettier --check clean. Description refreshed in both languages — counts, the new mechanism, and the matrix. Thank you for flagging the stale numbers (item 5); they described e265343e1.

4. Declined here — and your own finding is the reason it can't ride along

This is the item I most wanted to take, and I checked it properly before deciding. The blocker is that my own tests were not Windows-safe, and adding test:ci is exactly what would have exposed that: mode() asserts 600, but Windows reports 0o666/0o444 and chmod moves only the read-only bit. The repo's convention confirms it — atomicFileWrite.test.ts guards every mode assertion with it.skipIf(process.platform === 'win32'), 14 of them.

So I've fixed my half: all mode and symlink assertions are now posixOnly. What I can't verify from here is the other four weixin test files, which the same two lines would switch on across Linux, macOS and Windows merge-queue jobs simultaneously. You measured them green on one platform; I have only macOS. Turning on a workspace's CI gate is a repo-level change whose blast radius is four files I didn't write, and if one of them is Windows-fragile the failure lands on this PR as a mystery. That's the wrong PR to discover it in.

Happy to open it as a one-line follow-up the moment this lands — it's the change that makes all of this evidence permanent, and I agree it should happen.

2. Declined here — but the reason has changed

Previously I deferred the 0700 directory mode on scope. Your item 1 changes the argument: O_EXCL closes the plant vector deterministically, at the point of the write, without depending on the directory being private. So the directory mode is now genuine defence-in-depth rather than the thing holding the fix up — which makes it a clean follow-up rather than a dependency. @gwinthis suggested the same split. Still worth doing for cursor.txt, which has no protection of its own.

6a. Noted, not changed

Agreed on the Windows EPERM trade — same one channel-base accepts in three places. The misleading Login failed: message in configure.ts:63 is a real papercut but it's a different file and a different bug; I'd rather not fold it in.

中文说明

感谢复现整套变异矩阵而非采信我的数字，并用真实符号链接实测 flag 'w'——正是这一点把第 1 项从风格建议变成了我必须修的问题。已采纳三项，提交 0ab1c48aa。 另外两项谢绝，理由如下。

1. 已采纳——而且你说得对，真正的缺陷是我的注释

注释把「名称不可猜测」当作主要防线，而你的 flag 'w' 实测表明它不是：写入会跟随符号链接，mode: 0o600 因目标「已存在」而被丢弃，随后的 rename 更会让 account.json 永久变成符号链接。两方面均已修复——后缀改用 crypto.randomBytes(6)，写入加上 flag: 'wx'。

我想补充的是：这一点此前根本无法测试，这也是它读起来比实际更强的原因。使用 Math.random() 后缀时，测试无法在临时路径上预置任何东西——该防护只能停留在文字断言上。对 randomBytes 做 spy 后它变得可锚定，因此该主张现在由测试而非注释来保证。去掉 flag: 'wx' 即会让该测试失败；在本次提交之前，这个变异体是杀不死的。

你提到的那项「未被声称的额外收益」我也没有写进描述邀功，而是补成了测试——never leaves account.json as a symlink 在目标路径上预置符号链接并断言 lstatSync(...).isSymbolicLink() === false。它此前只是 rename 的意外结果，现在被明确锚定。

3. 已采纳——该断言确实是空的

你是对的：mock 在创建任何文件之前就抛出，因此 expect(tmpFiles()).toEqual([]) 天然成立。现在 mock 会先写入部分内容再抛出，与真实 ENOSPC 一致。变异体 D 现在失败 2 项而非 1 项——写入路径的清理已被覆盖。

6b. 已采纳——残留由本 PR 引入，就该由本 PR 关闭

我在自己的描述里把它归为「文件残留」，按你的框架重读后这是错的：该文件持有有效 token，且名称永不复用，因此不会有任何其他机制删除它。本 PR 之前根本没有临时文件，所以这是我的改动打开的洞——与我在 #7531 上对 checkout 回归采用的判据一致。clearAccount 现在清扫 account.json.*.tmp，并有测试锚定同目录下的 cursor.txt 不受影响。

0ab1c48aa 上的完整矩阵——18 项测试，6 个变异体

变异体 A：3 失败 → 7 失败 | 11 通过；B：3 失败 → 7 失败 | 11 通过；C：2 失败 → 3 失败 | 15 通过；D：1 失败 → 2 失败 | 16 通过；E（去掉 flag: 'wx'）：此前杀不死 → 2 失败 | 16 通过；F（去掉清扫）：1 失败 | 17 通过。

packages/channels/weixin 71 通过（5 个文件）；tsc --noEmit、eslint、prettier --check 均干净。描述已在中英两节刷新：计数、新机制与矩阵。感谢指出过期数字（第 5 项），它们描述的是 e265343e1。

4. 本 PR 谢绝——而理由恰恰来自你自己的发现

这是我最想采纳的一项，因此在决定前做了核查。障碍在于我自己的测试并非 Windows 安全，而添加 test:ci 正会暴露这一点：mode() 断言 600，但 Windows 上 Node 报告 0o666/0o444，chmod 只改动只读位。仓库惯例可以佐证——atomicFileWrite.test.ts 对每一处权限断言都加了 it.skipIf(process.platform === 'win32')，共 14 处。

因此我修好了属于我的那一半：所有权限与符号链接断言现在都是 posixOnly。我无法从本地验证的是另外 4 个 weixin 测试文件——同样那两行会让它们在 Linux、macOS 与 Windows 合并队列任务上同时启用。你在一个平台上测得全绿，而我只有 macOS。打开一个 workspace 的 CI 门禁属于仓库级改动，其影响面是 4 个并非我编写的文件；若其中之一在 Windows 上脆弱，失败会以「谜题」的形式落在本 PR 上。那不是发现此类问题的合适位置。

本 PR 合入后我很乐意立刻另开一行的后续 PR——它正是让上述全部证据长期生效的改动，我同意它应该做。

2. 本 PR 谢绝——但理由已经变了

此前我以「范围」为由推迟 0700 目录权限。你的第 1 项改变了这个论证：O_EXCL 在写入这一点上就确定性地关闭了预置攻击面，不再依赖目录是否私有。因此目录权限现在属于真正的纵深防御，而非修复所依赖的前提——这让它成为一个干净的后续项而非依赖项。@gwinthis 也建议同样拆分。就 cursor.txt（自身毫无保护）而言，它依然值得做。

6a. 已记录，未改动

同意 Windows 上 EPERM 的取舍——与 channel-base 三处已接受的一致。configure.ts:63 中误导性的 Login failed: 提示确是痛点，但那是另一个文件、另一个 bug，我不倾向把它并进来。

[qwen-code-ci-bot]

Reviewed. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

中文说明

已审查。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。

— qwen3.7-max via Qwen Code /review

[doudouOUC]

Reviewed — no blockers. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

— qwen3.7-max via Qwen Code /review

[wenshao]

Review (round 3) — fix(weixin): create the account credential file already private

Re-reviewed at head 0ab1c48aa in a fresh isolated worktree. I re-derived the whole six-mutant matrix from scratch rather than checking your table off, and re-tested the three items you took from round 2.

Verdict: all three round-2 items landed, and they landed correctly. Approve. Everything below is a nit. Two are new (items 1–2), one is description drift (item 3) — which I'd normally not re-raise, except that stale numbers were round-2 item 5 and one of the two stale spots is a Chinese risk bullet that now contradicts the shipped code.

---

Round-2 items — re-verified

Item	Status
1 — symlink on the temp path	Fixed. flag: 'wx' + randomBytes(6). Mutant E (drop wx) now fails 2, including refuses to write through a symlink on the exact temp path. Was unkillable at d56f725a6.
3 — vacuous cleanup assertion	Fixed. Partial-write mock. Mutant D went 1 → 2; removes the partial tmp file… now genuinely covers the write-path cleanup.
6b — orphaned temp holding a live token	Fixed at logout. Mutant F fails exactly sweeps a temp file orphaned by a killed save. See item 2 for a placement note.
2 — directory mode	Deferred, and I agree the reasoning changed: O_EXCL closes the plant vector at the write, so 0700 is now defence-in-depth rather than a dependency. Clean follow-up.
4 — no test:ci in the weixin workspace	Still open; decline substantiated below with a measurement.

Independent verification

Check	Result
npx vitest run --root packages/channels/weixin	71 passed (5 files)
src/accounts.test.ts	18 passed
tsc --noEmit -p packages/channels/weixin / eslint / prettier --check	clean
Mutant A — revert accounts.ts to base	7 failed | 11 passed ✓ matches
Mutant B — literal writeFileSync(p, json, { mode: 0o600 })	8 failed | 10 passed — table says 7, see 3b
Mutant C — fixed ${p}.tmp	3 failed | 15 passed ✓
Mutant D — delete the try/catch	2 failed | 16 passed ✓
Mutant E — drop flag: 'wx'	2 failed | 16 passed ✓
Mutant F — drop the clearAccount sweep	1 failed | 17 passed ✓

Five of six reproduce exactly. Each mutant is killed by a distinct, correctly-named test — no test is carrying two mutants, and no mutant is carried by a test whose name describes something else. That is a genuinely well-built matrix, and the randomBytes spy is the thing that made the wx claim testable at all; that was the right call.

I also audited accounts.test.ts for Windows-safety independently of your posixOnly pass: every unguarded test asserts either the options object handed to writeFileSync or ordinary file content, and none touches modes or symlinks. Your half is clean.

---

1. New: the readdirSync that feeds the sweep is the one call in clearAccount that isn't best-effort (accounts.ts:99)

Every unlinkSync in the sweep is wrapped in try/catch { /* best-effort cleanup */ }. The readdirSync(dir) that feeds them is not, so it throws out of clearAccount where the old body returned silently. Differential, HEAD vs base, on a state dir the process cannot read:

PROBE HEAD clearAccount -> EACCES: permission denied
PROBE BASE clearAccount -> no throw

configure.ts:26 calls clearAccount() outside any try/catch, so qwen channel configure-weixin clear goes from printing WeChat credentials cleared. to surfacing an unhandled EACCES. Reaching it needs an unreadable state dir — WEIXIN_STATE_DIR pointed somewhere root-owned, or ~/.qwen restored from a backup with wrong ownership — so it's rare, not unreachable. It's also the fail-open direction that matters least: the account file is deleted before the sweep runs, so the user's credential is already gone when the throw happens; only the "did it work?" signal is wrong.

One line, and it makes the whole function uniformly best-effort:

let entries: string[] = [];
try {
  entries = readdirSync(dir);
} catch {
  /* best-effort cleanup */
}
for (const f of entries) { … }

2. The sweep is attached to the one action a crashed save is least likely to be followed by (accounts.ts:99-107)

Logout is where you put it, and that's the right place for the revocation argument — the one you make in the comment. But the orphan is created by a killed save, and the natural next thing a user does after a login that died mid-write is run the login again. That path never sweeps:

PROBE leftovers after 3 saves: ["account.json.deadbeef0000.tmp"]
PROBE orphan mode: 600

Three successive saveAccount calls, and the live token from the killed save is still sitting there. clearAccount is only reachable via configure-weixin clear (configure.ts:25-29) — a command most users never run at all. So today the sweep closes the "token outlives its revocation" case, which is the sharper one, but leaves "token outlives the crash that produced it" wide open indefinitely.

Running the same sweep at the top of saveAccount would close both, costs one readdirSync per login, and needs no new logic — the loop is already written. Your call whether that's this PR or the same follow-up as the directory mode; I'd lean toward here, since the orphan is this PR's own artifact and you already applied that exact test to it in 6b.

3. Description drift — three spots, one of which now contradicts the code

Re-raising only because this was round-2 item 5 and 3c is a correctness problem rather than a stale count:

• (a) Full-suite count is still the e265343e1 number. Both language sections quote Tests 64 passed (64) (body lines 74 and 194). Actual at this head is 71 passed (71) — 64 was 53 + the original 11 accounts tests. Your own reply comment says 71, so it's just the body that didn't get the edit.
• (b) Mutant B is under-reported by one. The table and both code blocks say 7 failed | 11 passed. Running the literal writeFileSync(p, JSON.stringify(data, null, 2), { mode: 0o600 }) gives 8 failed | 10 passed — the listed block omits never creates the credential file at umask-default permissions, which the naive fix fails because it carries no flag: 'wx'. The conclusion is unaffected; the naive fix is rejected more thoroughly than you claim, not less. (The A and B blocks differ by exactly one swapped line, which is what it looks like happened.)
• (c) The Chinese Risk & Scope bullet still describes the pre-6b design. Body line 225 reads 无害且以 0600 创建，不过由于名称不再复用，它会累积而非被下次保存覆盖。这属于文件残留而非权限暴露 — harmless litter that accumulates, exactly the framing you retracted when you took 6b, and it never mentions the sweep. The English bullet was updated and the Chinese one wasn't, so the two halves of the description now disagree about what the code does. Relatedly, two English paragraphs ("The write-failure mock creates a partial file…", "Mode and symlink assertions are skipIf…") are duplicated untranslated inside 中文说明.

4. On item 4 — your decline holds up, and here is the follow-up's scope

You declined test:ci because you couldn't vouch for the four weixin test files you didn't write. That's the right instinct and I can now put a number on it. Re-running send.test.ts with the win32 path layer swapped in for the pass-through vi.mock('node:path'):

win32 path layer : 5 failed | 30 passed (35)
posix baseline   : 35 passed (35)

send.ts imports resolve/extname from node:path and builds its containment prefixes with a hardcoded + '/' (send.ts:163), while the suite mocks os.tmpdir() to /tmp and asserts POSIX literals. Caveat: this is a path-layer proxy, not a real Windows run — fs semantics differ too, so treat 5 as a lower bound on what needs guarding, not a verdict. But it does mean the two-line test:ci addition would have lit up the Windows merge-queue job with failures in a file this PR never touched, which is precisely the mystery failure you didn't want to land here. Follow-up now has a name and a size.

5. Test hygiene (very small)

beforeEach uses mockClear(), which does not drain a queued mockImplementationOnce/mockReturnValueOnce:

after mockClear -> ONCE     // leaked
after mockReset -> REAL

Every queued impl is consumed within its own test today, so nothing is wrong. But if one of those tests ever fails before consuming it, the next test silently inherits a throwing writeFileSync and the failure lands somewhere unrelated. mockReset() is a safe swap here — under vitest 3 it restores the implementation passed to vi.fn(actual.writeFileSync), verified above.

---

中文说明

在全新的隔离 worktree 中基于 head 0ab1c48aa 复审。我没有核对你的表格，而是从零重新推导了整套六变异体矩阵，并重新验证了第 2 轮中你采纳的三项。

结论：第 2 轮的三项均已落地且落地正确，同意合并。 以下全部为小问题。其中两项是新增的（第 1、2 项），一项是描述漂移（第 3 项）——本来不会重提，但过期数字正是第 2 轮的第 5 项，且两处过期之一是中文风险说明，它现在与已提交的代码相矛盾。

第 2 轮各项复核

项	状态
1 —— 临时路径符号链接	已修复。 flag: 'wx' + randomBytes(6)。变异体 E（去掉 wx）现在失败 2 项，含 refuses to write through a symlink on the exact temp path；在 d56f725a6 上该变异体杀不死。
3 —— 空的清理断言	已修复。 部分写入 mock。变异体 D 由 1 升至 2，写入路径的清理被真正覆盖。
6b —— 残留临时文件持有有效 token	登出路径已修复。 变异体 F 恰好失败 sweeps a temp file orphaned by a killed save。放置位置见第 2 项。
2 —— 目录权限	推迟，且我同意论证已改变：O_EXCL 在写入处即关闭预置攻击面，因此 0700 现在属于纵深防御而非依赖项，适合作为独立后续。
4 —— weixin workspace 没有 test:ci	仍未解决；下文用实测支持你的谢绝理由。

独立验证

检查项	结果
npx vitest run --root packages/channels/weixin	71 通过（5 个文件）
src/accounts.test.ts	18 通过
tsc --noEmit / eslint / prettier --check	干净
变异体 A —— 还原 accounts.ts 至基线	7 失败 | 11 通过 ✓ 一致
变异体 B —— 字面形式 writeFileSync(p, json, { mode: 0o600 })	8 失败 | 10 通过 —— 表格写的是 7，见 3b
变异体 C —— 固定 ${p}.tmp	3 失败 | 15 通过 ✓
变异体 D —— 删除 try/catch	2 失败 | 16 通过 ✓
变异体 E —— 去掉 flag: 'wx'	2 失败 | 16 通过 ✓
变异体 F —— 去掉 clearAccount 清扫	1 失败 | 17 通过 ✓

六个中有五个完全复现。每个变异体都由一项名称贴切且互不重叠的测试杀死——没有一项测试同时承担两个变异体，也没有哪个变异体是靠名不副实的测试兜住的。这是一套构造扎实的矩阵；而 randomBytes spy 正是让 wx 这一主张变得可测的关键，这个决定是对的。

我也独立于你的 posixOnly 改动审计了 accounts.test.ts 的 Windows 安全性：所有未加守卫的测试断言的要么是传给 writeFileSync 的选项对象，要么是普通文件内容，均未触及权限位或符号链接。属于你的那一半是干净的。

1. 新增：清扫所依赖的 readdirSync 是 clearAccount 中唯一不是尽力而为的调用（accounts.ts:99）

清扫里每个 unlinkSync 都包在 try/catch { /* best-effort cleanup */ } 中，而为它们提供输入的 readdirSync(dir) 没有，因此它会从 clearAccount 中抛出，而旧实现是静默返回。在进程无法读取的状态目录上做 HEAD 与基线的对比实测：

PROBE HEAD clearAccount -> EACCES: permission denied
PROBE BASE clearAccount -> no throw

configure.ts:26 调用 clearAccount() 时没有任何 try/catch，因此 qwen channel configure-weixin clear 会从打印 WeChat credentials cleared. 变成抛出未捕获的 EACCES。触发它需要状态目录不可读（WEIXIN_STATE_DIR 指向 root 所有的目录，或从备份恢复导致 ~/.qwen 属主错误），因此罕见但并非不可达。它失败的方向也是影响最小的那个：账号文件在清扫之前已被删除，因此抛出时用户凭据其实已经清掉，错的只是「是否成功」的反馈。

一行改动即可让整个函数统一为尽力而为：

let entries: string[] = [];
try {
  entries = readdirSync(dir);
} catch {
  /* best-effort cleanup */
}
for (const f of entries) { … }

2. 清扫挂在了「崩溃保存之后最不可能发生」的那个动作上（accounts.ts:99-107）

你把它放在登出，对撤销这条论证而言位置正确——那也正是注释所论证的。但残留文件是被强杀的保存产生的，而用户在登录写入中途崩溃后最自然的下一步是重新登录一次。那条路径完全不会清扫：

PROBE leftovers after 3 saves: ["account.json.deadbeef0000.tmp"]
PROBE orphan mode: 600

连续三次 saveAccount 之后，被强杀那次留下的有效 token 依然躺在那里。clearAccount 只能经由 configure-weixin clear 到达（configure.ts:25-29），而多数用户从不运行该命令。因此当前的清扫关闭了「token 比撤销活得更久」这个较尖锐的情形，却让「token 比产生它的那次崩溃活得更久」长期敞开。

在 saveAccount 开头执行同一次清扫即可同时关闭两者，代价是每次登录一次 readdirSync，且无需新逻辑——循环已经写好了。放本 PR 还是与目录权限一起做后续由你决定；我倾向放在这里，因为残留文件正是本 PR 自身的产物，而你在 6b 中已经对它套用过同一条判据。

3. 描述漂移——三处，其中一处已与代码矛盾

之所以重提，是因为这正是第 2 轮的第 5 项，且 3c 属于正确性问题而非过期计数：

• (a) 完整套件计数仍是 e265343e1 的数字。 中英两节都写 Tests 64 passed (64)（正文第 74、194 行）。本 head 实际为 71 通过（71）——64 是 53 加最初的 11 项。你自己的回复评论里写的是 71，因此只是正文漏改。
• (b) 变异体 B 少报了一项。 表格与两处代码块都写 7 failed | 11 passed。运行字面形式 writeFileSync(p, JSON.stringify(data, null, 2), { mode: 0o600 }) 得到 8 failed | 10 passed——所列清单漏掉了 never creates the credential file at umask-default permissions，而朴素改法确实会让它失败，因为它不带 flag: 'wx'。结论不受影响：朴素改法被否定得比你声称的更彻底，而非更弱。（A 与 B 两块正好差一行互换，看起来是这么来的。）
• (c) 中文的「风险与范围」仍在描述 6b 之前的设计。 正文第 225 行写着 无害且以 0600 创建，不过由于名称不再复用，它会累积而非被下次保存覆盖。这属于文件残留而非权限暴露——正是你采纳 6b 时收回的那套说法，且完全没提清扫。英文版改了而中文版没改，于是描述的两半现在对「代码做了什么」给出了互相矛盾的说法。相关地，有两段英文（The write-failure mock creates a partial file…、Mode and symlink assertions are skipIf…）未翻译地重复出现在 中文说明 内。

4. 关于第 4 项——你的谢绝站得住，这是后续的范围

你以「无法为并非自己编写的 4 个 weixin 测试文件背书」为由谢绝，这个直觉是对的，而我现在可以给出具体数字。把 send.test.ts 中透传的 vi.mock('node:path') 换成 win32 路径层后重跑：

win32 path layer : 5 failed | 30 passed (35)
posix baseline   : 35 passed (35)

send.ts 从 node:path 导入 resolve/extname，并用硬编码的 + '/' 拼接包含前缀（send.ts:163），而该套件把 os.tmpdir() mock 成 /tmp 并断言 POSIX 字面量。需要说明的是：这是路径层的替身实验而非真实 Windows 运行，文件系统语义也有差异，因此 5 应视为「需要加守卫的数量下限」而非定论。但它确实说明，那两行 test:ci 会让 Windows 合并队列任务在一个本 PR 从未触碰的文件上爆红——恰恰是你不希望在这里发现的那类谜题。后续 PR 现在有了明确的对象和量级。

5. 测试卫生（很小）

beforeEach 用的是 mockClear()，它不会清空排队中的 mockImplementationOnce/mockReturnValueOnce：

after mockClear -> ONCE     // 泄漏
after mockReset -> REAL

目前每个排队实现都在自己的测试内被消费，因此现状没有问题。但只要其中某项测试在消费之前失败，下一项测试就会静默继承一个会抛异常的 writeFileSync，failure 会落在毫不相关的地方。这里换成 mockReset() 是安全的——在 vitest 3 下它会恢复传给 vi.fn(actual.writeFileSync) 的那个实现，上面已实测确认。

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[qwen-code-ci-bot]

Released in v0.21.1.