@qwen-code /triage

Verification report — real-execution A/B

I checked out this branch and exercised the real WorkspaceFileSystem service against a real temp file (the actual code path the PR changes), ran an A/B (PR vs. the same code with the new check removed), and traced every caller of readText to confirm reachability and regression-safety.

TL;DR: verified correct, consistent with the existing line guard, and regression-free. Safe to merge.

Method

• git worktree on the PR head; npm ci; vitest drives the real createWorkspaceFileSystemFactory over a real mkdtemp workspace (no mocks).
• A/B: ran a probe that records readText's actual outcome for valid + the 7 invalid limit values, once on the PR source and once with the 9-line check reverted.
• (No TUI/tmux here — this is a server-side service-boundary validation, so "real test" = driving the real service on real files.)

Results

1. A/B on a real 5-line file (L1..L5) — the new check is exactly what stops the silent, inconsistent truncation the PR describes:

Base degrades into 4 different behaviors (all / empty / truncated / newline-dropping) depending on the bad value — the PR makes all of them a consistent parse_error, while valid limits are untouched.

2. The test is load-bearing. Reverting the 9-line check makes the PR's own new test fail (expected false to be true — readText returns content instead of throwing). With the check, the full file is 76/76 green; eslint, prettier --check, and tsc --noEmit (cli workspace) all pass.

3. Reachability — the check is correctly scoped as service-boundary defense-in-depth. All external entry points already validate limit before readText, so this introduces no external behavior change:

• GET /file route (workspace-file-read.ts:232) — parseIntInRange(req.query['limit'], 1, MAX_FILE_LINE_LIMIT) → 400 parse_error.
• ACP HTTP dispatch (acpHttp/dispatch.ts:1556) — parseOptionalSafeIntegerInRange(...) → RPC error.
• ACP bridge adapter (bridge-file-system-adapter.ts) — drops null and non-positive limit.

The new guard protects direct/internal service callers (the scenario the PR body calls out), which is exactly what the new unit test exercises.

4. No regression for the one internal limit: Infinity caller. workspace-file-system.ts:1156 (inside edit()) passes limit: Number.POSITIVE_INFINITY to this.deps.lowFs.readTextFile(...) — the low-level read, not the guarded service readText — so the new check does not touch it.

5. Consistency. The new block mirrors the existing line guard verbatim (!Number.isSafeInteger(x) || x < 1), lives in the same try (so it gets the same fs.denied audit-on-error treatment), and like line applies no service-level upper bound (the route owns the policy cap). The existing comment above the line check already documents the exact Infinity/float/NaN/0 degradation rationale this extends to limit.

Verdict

Approve. Correct, minimal, well-tested, no regression, no external behavior change, and consistent with the established pattern. The only nit (optional): the new FsError message says limit must be a positive integer — fine as-is; the route's richer "in [1, N]" message stays at the HTTP layer where the cap applies.

Thanks for the PR!

Template looks good ✓ — all required sections present and filled in.

On direction: this is a clean defensive-boundary fix. Issue #5623 identifies a real inconsistency — readText validates line but not limit at the same service boundary, and the degradation behaviors (empty string, full file, float-truncated, newline-dropping) are genuinely surprising. Tightening the contract at the shared boundary is exactly what the service layer should do. No CHANGELOG precedent needed; this is internal API hygiene, not a user-facing feature.

On approach: the scope is minimal — 9 lines of validation mirroring the existing line guard verbatim, plus a focused test covering the 7 edge values from the issue. No scope creep, no drive-by refactors. The two checks could theoretically share a helper, but at two call sites that's premature abstraction. This is the right shape.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Independent proposal: Given the issue — readText validates line but not limit — I would add the same Number.isSafeInteger(x) || x < 1 guard for limit, placed right after the line check in the same try block, throwing FsError('parse_error', ...). Test would cover the same 7 bad values (Infinity, -Infinity, MAX_SAFE_INTEGER+1, 0, -1, 1.5, NaN).

Comparison with diff: the PR does exactly this. The guard is a verbatim mirror of the line check — same condition, same error type, same placement. No surprises.

Correctness: sound. Number.isSafeInteger correctly rejects NaN, Infinity, -Infinity, floats, and unsafe integers in one check. The < 1 catches 0 and negatives. The opts.limit !== undefined guard correctly skips validation when limit is not provided.

Reuse check: the line guard above uses the identical pattern. A shared helper (assertPositiveSafeInteger) could DRY these two blocks, but at two call sites this isn't worth the abstraction — AGENTS.md's "simplicity first" principle supports keeping them inline.

Test quality: good. The test iterates all 7 bad values from the issue and asserts both isFsError and kind === 'parse_error'. Valid limit values remain covered by the existing tests above in the same file.

No blockers found.

Test Results

Unit tests (run from worktree on PR branch):

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

 Test Files  1 passed (1)
      Tests  76 passed (76)
   Duration  5.46s

Typecheck: tsc --noEmit -p packages/cli/tsconfig.json — clean, no errors.

CI status: all green — Test (windows, macos, ubuntu Node 22.x), Lint, CodeQL, Integration Tests (No-AK Smoke), Classify PR all pass.

Real-Scenario Testing

N/A — this is a non-user-visible service-boundary validation. The change is internal API contract enforcement; there is no CLI behavior to exercise via tmux. The unit tests drive the real WorkspaceFileSystem service against real temp files, which is the appropriate test level for this change.

— Qwen Code · qwen3.7-max

Final Decision

This PR is exactly what it claims to be: a 9-line service-boundary guard that makes limit validation consistent with the already-existing line validation in the same method. The diff is minimal, the test covers all edge cases from the linked issue, and there's zero scope creep.

Going back to my independent proposal from Stage 2 — the PR matches it exactly. I can't find a simpler path. The implementation mirrors the existing pattern verbatim, lives in the right place (same try block, same error handling), and doesn't touch any callers (because all external entry points already validate limit upstream).

The only question worth asking is whether to extract a shared assertPositiveSafeInteger helper for the two near-identical blocks. But at two call sites, that's premature — and the PR author correctly chose to match the existing pattern rather than refactor it.

76/76 tests pass, typecheck clean, CI all green across Windows, macOS, and Linux. The collaborator verification report (A/B testing on real files) independently confirmed the same findings.

Approving. ✅

— Qwen Code · qwen3.7-max