@qwen-code /triage

✅ Local verification report (maintainer)

Built and tested this PR locally to confirm the fix before merge.

Environment

• macOS (Darwin 25.5), Node v22.22.2
• PR head a3dfed9a9, base origin/main 2fd2104fa (1 commit on top)
• Method: isolated git worktree at the PR head with a clean npm ci + fresh core/acp-bridge build, then verified each claim independently.

Results

Mutation test (the key evidence). To confirm the new test actually guards the gap rather than passing vacuously, I reverted workspace-file-system.ts back to the main version (no maxEntries validation) while keeping the PR's new test. It fails immediately:

FAIL > list > rejects a non-positive-integer maxEntries with parse_error
  expect(isFsError(err)).toBe(true)  // got: false

With the unvalidated code, maxEntries: Infinity is silently accepted — entries.length >= Infinity never breaks, so list() resolves instead of throwing, and isFsError(err) is false. The test pins exactly that gap. It passes again once the guard is restored. The test covers [Infinity, NaN, 0, -1, 1.5, Number.MAX_SAFE_INTEGER + 1] — all six are rejected.

Correctness review

• The guard !Number.isSafeInteger(opts.maxEntries) || opts.maxEntries < 1 (only when maxEntries !== undefined) is a faithful copy of the established pattern in the same file — readText() guards line / limit / maxBytes with the identical !Number.isSafeInteger(x) || x < 1 shape. undefined still means "no cap" and is correctly left untouched.
• Error handling is consistent: the parse_error is thrown inside the existing try, so it flows through catch → recordAndWrap('list') and is audited/wrapped like every other error here. It sits after the trust gate and before any filesystem I/O (fail-fast).
• No caller is broken (the one real risk for this kind of change): the only two callers pass a finite cap — dispatch.ts → MAX_LIST + 1 and workspace-file-read.ts → MAX_LIST_ENTRIES + 1, both = 2001, which passes the guard. Nothing relies on Infinity / 0 as an "unlimited" sentinel.

Verdict: LGTM. Minimal, correct, mutation-verified, consistent with the sibling readText guards, and breaks no callers. Safe to merge from my side.

FAIL > list > rejects a non-positive-integer maxEntries with parse_error
  expect(isFsError(err)).toBe(true)  // 实际: false

Filled in the missing template sections: How to verify / Evidence / Tested on, the full Risk & Scope sub-items, Linked Issues, and the 中文说明. Ready for another look — thanks!

@qwen-code /triage

Thanks for the PR, @he-yufeng!

Template looks good ✓ — all required sections present, including the bilingual summary and reviewer test plan.

On direction: this is a clean follow-up to #5639, which added the same Number.isSafeInteger guard to readText()'s limit/line options. Closing the same validation gap for list()'s maxEntries is a natural consistency fix. No product-direction concerns — it's exactly the kind of small defensive-validation PR that keeps the serve layer predictable.

On approach: the scope is tight — 12 lines of guard + 19 lines of test, touching exactly one method. The implementation is a direct copy of the readText() pattern (Number.isSafeInteger + < 1 check → FsError('parse_error', ...)), which is the right call for consistency. No scope creep, no drive-by refactors.

Moving on to code review and tests. 🔍

— Qwen Code · qwen3.7-max

Code Review

The guard is a faithful mirror of the existing readText() validation at lines 386–401 — same Number.isSafeInteger + < 1 check, same FsError('parse_error', ...) shape, same error-message format. Placed right after assertTrustedForIntent, before the opendir call, which is the correct position (fail fast, before any I/O). No issues found.

One minor observation (not a blocker): the test asserts isFsError(err) + String(err).toContain('maxEntries') rather than checking err.kind === 'parse_error' directly (which some neighboring tests do). Both styles work — the String(err) approach is actually more thorough since it verifies the error message mentions the parameter name.

Test Results

With the guard (PR applied): 77/77 passed ✓

 ✓ src/serve/fs/workspace-file-system.test.ts (77 tests) 208ms

 Test Files  1 passed (1)
      Tests  77 passed (77)

Without the guard (source reverted, test kept): 1 failed, 76 passed — confirming the new test catches the gap.

 FAIL  src/serve/fs/workspace-file-system.test.ts > WorkspaceFileSystem - list > rejects a non-positive-integer maxEntries with parse_error
AssertionError: expected false to be true // Object.is equality
 ❯ src/serve/fs/workspace-file-system.test.ts:280:30
    280|       expect(isFsError(err)).toBe(true);

 Test Files  1 failed (1)
      Tests  1 failed | 76 passed (77)

Before/After (tmux): N/A — non–user-visible change in the serve layer (input validation), no TUI surface. Verified entirely by unit test.

— Qwen Code · qwen3.7-max

This is a textbook consistency fix. readText() got the same guard in #5639; list() was the obvious gap. The PR closes it with the minimum possible code, following the exact same pattern, and the test genuinely catches the bug (confirmed: 1 fail without the guard, 77 pass with it).

My independent proposal before reading the diff would have been: add Number.isSafeInteger + < 1 check on opts.maxEntries right after assertTrustedForIntent, throw FsError('parse_error', ...), test with Infinity/NaN/0/-1/float/unsafe-integer. That's exactly what the PR does.

Nothing to cut, nothing missing, nothing to bikeshed. Approving. ✅

— Qwen Code · qwen3.7-max

✅ Runtime-surface follow-up — drove the real qwen serve daemon

Complementary to my test/mutation report above (which was vitest + mutation based). This one runs the actual daemon and checks the change at the only runtime surface that reaches WorkspaceFileSystem.list() — the serve HTTP API.

Method: built the PR head, launched qwen serve --workspace <tmp> --port 4179 --no-web (loopback, bearer auth off by default), and hit GET /list against a workspace holding a 3-file dir and a 2001-file dir.

No-regression at the surface

The guard runs on every list call with the hardcoded MAX_LIST_ENTRIES + 1 = 2001, which is a valid safe integer, so it passes and listing is unaffected:

Reachability probe (the part worth adding)

I tried to feed a bad maxEntries from the client — the exact input the new guard exists to reject:

Every value is ignored — the query param never reaches list(). Both call sites (the GET /list route and the ACP qwen/file/list method) hardcode maxEntries server-side, and nowhere in the tree is ListOptions.maxEntries populated from client input. So the new parse_error guard cannot be reached from any external surface — it is pure defense-in-depth on the internal interface contract (faithfully mirroring the readText() guards), and the unit test is the right — and only — place to exercise it.

Verdict

PASS (runtime side) — confirms LGTM. Because the guard never fires on the real path, this PR is externally indistinguishable from base: same responses, breaks nothing. One scoping note for the merge log — the "entire directory is returned" scenario in the PR description is latent (no current caller passes a bad cap; both pass 2001), not a live bug. That makes this a consistency / future-proofing hardening, which is still a perfectly good reason to land it.

_{Runtime-surface verification on macOS — real qwen serve daemon (loopback, no auth), GET /list + client-maxEntries injection probe. Complements the static/test/mutation report above.}