Automated review by review-lead agent. 3/3 reviewers request changes. See aggregate verdict comment for details.

Review: chore(deps): bump vitest from 3.2.4 to 4.1.9 (#120)

Verdict: Comment

Dependabot major-version bump (vitest 3 → 4, a dev-only test dependency). The prior CR at 74ebe9c (lml2468) is now addressed by the follow-up commit. CI: check (22) build/test is green on vitest 4 — the decisive signal for a test-framework major bump. The two red checks (dependency-review, secret-scan) are the standing repo-level conditions, unrelated to this diff.

Verified correct

• The suite actually passes on v4. check (22) = SUCCESS at this head, so the full test run executes under vitest 4 — that's the thing that matters for a major test-tooling bump, and it's confirmed rather than assumed.
• The breaking change is handled, not papered over. vitest 4 changed restoreAllMocks to no longer reset automock state (only manual vi.spyOn spies). The follow-up commit (src/actions.test.ts:27-31, 61-74) re-seeds the ./inbound.js automocks to their factory defaults in beforeEach, with a comment naming the exact v4 behavior change. Without this, a persistent mockRejectedValue from one test would leak into later ones — a real cross-test-pollution bug the migration would otherwise introduce. This is the correct fix for the named behavior change, and it directly resolves the prior CR.
• Scope is contained. package.json (one line), package-lock.json (lockfile regen), and the one test-file compat fix. vitest is a devDependency — no runtime/production surface, no shipped-artifact change.
• Transitive bumps are consistent (chai 5→6, tinyrainbow 2→3, the @vitest/* family all to 4.1.9) — standard for a vitest major, all dev-tree.

Praise

• The author didn't just bump and hope — they hit the restoreAllMocks automock-reset behavior change (a genuinely easy-to-miss v4 breaking change), fixed it at the right place (beforeEach re-seed), and documented why in the comment. That's the difference between a green-by-luck bump and a verified migration.

Out of scope (informational)

• dependency-review / secret-scan reds are the standing repo-level conditions seen across every recent PR here; neither is introduced by a dev-dependency bump.

Clean, verified major-version dev-dep migration with the v4 breaking change correctly absorbed. Merge decision is the maintainer's (dependabot PR).

Summary: The PR is in scope for this repository and the Vitest 4 migration looks correct; no blocking issues found.

💬 Non-blocking

• 🟡 Warning: package.json:92 still advertises "node": ">=22", while the new locked Vite/Rolldown toolchain requires Node >=22.12.0 in package-lock.json:5425. CI uses node-version: 22, which currently resolves to a compatible latest Node 22, but contributors on Node 22.0-22.11 may hit engine warnings or runtime failures. Consider tightening the dev/runtime guidance to >=22.12.0 if the project intends local test/build support with this lockfile.

✅ Highlights

• src/actions.test.ts:59 correctly re-seeds the mocked uploadAndSendMedia and uploadMedia exports after vi.restoreAllMocks(), preventing one test’s persistent mock behavior from leaking into later tests under Vitest 4.
• The dependency bump is limited to test tooling and the adapted test behavior is covered by the existing action test suite.

Verification run:

• npm ci
• npm run type-check
• npx vitest run src/actions.test.ts
• npm test
• npm run build

Code Review — PR #120 (openclaw-channel-octo)

Scope: vitest major bump 3.2.4 → 4.1.9 (devDependency) + one accompanying test adaptation. Reviewed at head SHA 73a0e0b1ef4fc543d16999651ce57203e630c378.

1. Spec compliance

This is a Dependabot major-version bump. The diff is exactly what such a change should contain — nothing missing, nothing out of scope:

• package.json — vitest devDep ^3.0.0 → ^4.1.9. ✅
• package-lock.json — all changes are confined to vitest's transitive tree (esbuild, lightningcss*, loupe, deep-eql, check-error, etc.), all dev-time only. No production dependency (axios, crypto-js, ws, curve25519-js, md5-typescript) is touched. ✅
• src/actions.test.ts — minimal adaptation to a vitest 4 behavior change (details below). No source/runtime code modified. ✅

No over-build (no extra features/flags), no deviation. Spec: ✅

2. Code quality

The test adaptation is correct and necessary

vitest 4 changed vi.restoreAllMocks() semantics: it now only restores vi.spyOn spies and no longer resets automocked module functions to their factory defaults. In this file, ./inbound.js is automocked via vi.mock(...), and several tests set persistent mock behavior (uploadSpy.mockResolvedValue(...) at L914, mockRejectedValue(...) at L2133/L2154). Under vitest 4 that state would leak into subsequent tests.

The fix re-seeds both automocks to their factory defaults in beforeEach via mockReset().mockResolvedValue(...), with an added static import of the mocked symbols. This is the idiomatic vitest-4 remedy.

I verified the change is load-bearing, not superfluous: removing the re-seed block and running vitest run src/actions.test.ts under 4.1.9 produces 4 failures (should skip falsy values in attachments, should send media from all three sources, should collect args.url, should collect args.fileUrl) — exactly the cross-test leakage the comment describes. With the block in place, all pass.

Verification performed (clean checkout, npm ci)

• npm test → 35 files, 1300 tests passed on vitest 4.1.9.
• npm run type-check (tsc --noEmit) → clean.
• Lockfile resolves vitest@4.1.9; bumped packages are all dev-only transitive deps.

Notes (non-blocking)

• Four other test files (bug-repro, channel, reconnect-fixes, socket) also combine vi.mock(...) with restoreAllMocks, but the full suite passes — they don't depend on automock state being reset between tests, so they need no change. Worth keeping in mind if their mocks gain persistent return values later.
• vitest 4 pulls lightningcss (browser-mode tooling) into the dev tree; it is unused here and dev-only, so no runtime or supply-chain impact on the published plugin.

Quality: Approved.

3. Verdict

APPROVED. No P0/P1 findings. The bump is scoped to devDependencies, the single test change is the correct and minimal adaptation to vitest 4's restoreAllMocks change, and the entire suite + type-check pass on a clean install.

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

openclaw-channel-octo PR#120 Review Report

PR: #120
Title: chore(deps): bump vitest from 3.2.4 to 4.1.9
Head SHA: 73a0e0b
Reviewer: Octo-Q (automated review)

1. 验证结论

• ✅ Tests pass: 1300/1300 across 35 test files (vitest run — 4.13s)
• ✅ Type-check passes: tsc --noEmit clean
• ✅ Node engine compatibility: project requires >=22, vitest 4 requires ^20.0.0 || ^22.0.0 || >=24.0.0 — compatible
• ✅ Test adaptation correctly addresses vitest 4 breaking change
• ✅ No other test files affected by same breaking change (verified via grep)

2. 发现的问题（P0/P1/P2）

No P0/P1 findings.

P2 — form-data npm audit vulnerability (pre-existing)

• diff-scope: Pre-existing. form-data@4.0.5 is a transitive dep of axios@1.18.1, not vitest. Not introduced or amplified by this PR.
• CRLF injection advisory (GHSA-hmw2-7cc7-3qxx). Fixable via npm audit fix.
• Not blocking — unrelated to this PR's scope.

P2 — hosted-git-info@10.1.1 engine warning (cosmetic)

• diff-scope: Pre-existing. Requires ^22.22.2, environment has v22.22.0. Not introduced by this PR.
• No functional impact on tests or build.

3. 建议

No blocking suggestions. Optional improvements:

• Consider running npm audit fix in a separate PR to address the form-data vulnerability.

4. 额外发现

• Transitive dep chain impact: vitest 4 pulls in vite 8.1.0 (from 7.3.3), which replaces rollup with rolldown as the internal bundler. Since vite is only a transitive dep (not directly used by the project), this has no production impact.
• vitest 4 drops Node 18 support (now ^20.0.0 || ^22.0.0 || >=24.0.0). Project already requires >=22 — compatible.
• New transitive deps added: @emnapi/*, @napi-rs/wasm-runtime, @oxc-project/types, rolldown, obug, tslib. All are dev-only.
• Removed transitive deps: @esbuild/* platform binaries, rollup + @rollup/*, vite-node, tinypool, tinyspy, strip-literal, pathval, loupe, js-tokens, chai. These are replaced by vitest 4's internal rewrites.

5. 数据流回溯

src/actions.test.ts — mock reset in beforeEach:

• uploadAndSendMedia and uploadMedia are vi.fn() instances created in the vi.mock("./inbound.js", ...) factory (line 10-22).
• In vitest 3, vi.restoreAllMocks() also reset these factory-created mocks. In vitest 4, it only restores vi.spyOn() spies.
• The fix (lines 64-75) explicitly calls .mockReset().mockResolvedValue(...) on both mocks, re-seeding them to factory defaults.
• Data flow verified: vi.mocked(uploadAndSendMedia) → resolves to the vi.fn() created at line 14 → .mockReset() clears call history + return values → .mockResolvedValue(undefined) sets default behavior. Same pattern for uploadMedia with the matching factory-default return object.
• Import correctness (lines 26-27): Static import { uploadAndSendMedia, uploadMedia } from "./inbound.js" resolves to the mocked module because vi.mock() is hoisted before imports by vitest. Verified: the imported symbols are the same vi.fn() instances created in the factory.

Other test files using vi.restoreAllMocks() + factory mocks:

• channel.test.ts already avoids vi.restoreAllMocks() (line 678 comment: "do NOT call vi.restoreAllMocks() here") and uses vi.clearAllMocks() instead — unaffected.
• agent-tools.test.ts uses vi.clearAllMocks() (line 133) — unaffected.
• Other files with vi.restoreAllMocks() either don't use vi.fn() inside vi.mock() factories, or their factory mocks return static objects that aren't modified per-test — unaffected.

6. 盲点 checklist（R5）

• C1 双路径 parity: N/A — no symmetric paths (add/remove, subscribe/unsubscribe) touched.
• C2 control-flow ordering / 嵌套复用: N/A — no shared logic reordered or nested.
• C3 授权边界: N/A — no auth/permission changes.
• C4 授权生命周期 / 容器-成员状态级联: N/A — no auth changes.
• C5 build/note ≠ 运行期: Verified. Vite 8 replaces rollup with rolldown internally, but vite is only a transitive dep of vitest — not directly consumed by the project. tsc build and vitest run both pass. No relative-path/origin concerns (no browser extension, no CLI, no packaging).
• C6 治理/策略/安全文档自洽性: N/A — no doc changes.

7. 跨轮 blocker 复检（R6）

N/A — first review.

[Octo-Q] verdict: APPROVE

Clean major-version dependency bump. The vitest 3→4 breaking change (restoreAllMocks() semantics) is correctly addressed in actions.test.ts with explicit mock re-seeding. All 1300 tests pass. Type-check passes. No P0/P1 findings. No production code changes.