@qwen-code /triage

Thanks for the PR!

Template looks good ✓

On direction: this is a straightforward bug fix for a real user-facing issue — web UI file links with zero-based line/column values crash the VS Code openFile handler by producing negative Position values. Linked issue #5710 clearly describes the problem. Well within scope for the VS Code companion. CHANGELOG has no direct reference but the area (IDE integration / file operations) is core functionality.

On approach: scope is tight and appropriate. The fix extracts a small toZeroBasedEditorPosition helper that clamps the result of parseInt - 1 to zero, replacing the two inline expressions. The regression test covers the exact failure mode. No scope creep, no unrelated changes. One small observation: the helper name is quite long for what it does — toEditorPosition or clampPosition would be shorter — but that's a style nit, not worth blocking on.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

The diff is minimal and well-focused — exactly what a bug fix should look like.

Production code (FileMessageHandler.ts, +9/-2): adds toZeroBasedEditorPosition() which combines parseInt, the one-based-to-zero-based subtraction, and a Math.max(0, ...) clamp. The two inline expressions that previously did parseInt(lineStr, 10) - 1 are replaced by calls to this helper. Clean consolidation, no over-engineering.

Test code (FileMessageHandler.test.ts, +71/-0): extends the vscodeMock with Position, Selection, Range, and TextEditorRevealType classes — the VS Code APIs the openFile path actually uses. The mock Position constructor throws on negative values (matching real VS Code behavior), which makes the regression test genuinely enforce the invariant. The new test sends src/app.ts:0:0 and asserts no error + correct editor position.

Reuse check: the parseInt - 1 pattern only appears at this one call site. No duplication to flag.

No correctness issues, no security concerns, no AGENTS.md violations.

Testing

Unit Tests

All 8 tests pass, including the new regression test:

 ✓ src/webview/handlers/FileMessageHandler.test.ts (8 tests) 15ms
   ✓ openFile clamps zero line and column values to the file start

 Test Files  1 passed (1)
      Tests  8 passed (8)

Regression Verification (reverted fix + re-ran test)

To confirm the test actually catches the bug, I reverted the production code change and re-ran:

 FAIL  src/webview/handlers/FileMessageHandler.test.ts
 ✗ openFile clamps zero line and column values to the file start

 Expected spy "showErrorMessage" to not be called, but it was called 1 time:
   1st call: ["Failed to open file: Position line and character must be non-negative"]

 Test Files  1 failed (1)
      Tests  1 failed | 7 passed (8)

Without the clamp, parseInt("0", 10) - 1 = -1 triggers the negative Position error — exactly the bug described in #5710. The test correctly catches it.

Tmux / CLI Testing

Not applicable — this is a VS Code extension internal handler, not a CLI-invokable feature. The behavior only manifests through the VS Code companion webview messaging API.

Type Check

The check-types failures (@qwen-code/webui module not found in sessionGrouping.ts and utils.ts) are pre-existing and unrelated to this PR — the webui workspace package wasn't built in the test environment.

— Qwen Code · qwen3.7-max

Reflection

This is exactly the kind of PR that's easy to approve: a small, focused fix for a real bug, with a test that actually catches the failure mode.

My independent proposal was Math.max(0, line - 1) inline — the PR's approach is better because the extracted helper also consolidates the parseInt call, so the two call sites become trivially readable. I verified the regression test genuinely enforces the invariant: reverting the production code makes the test fail with the exact error from issue #5710.

The mock additions (Position, Selection, Range throwing on negative values) are a nice touch — they make the test a real safety net rather than a rubber stamp. Every change in the diff serves the stated goal. No scope creep, no drive-by refactors.

The only nit I had was the helper method name being slightly long, but that's genuinely not worth a round trip. Ships it.

— Qwen Code · qwen3.7-max

✅ Local runtime verification — recommend merge

I verified this PR on Linux by driving the real FileMessageHandler.handle({ type: 'openFile' }) end-to-end against a faithful mock vscode (the same one this PR adds, where new vscode.Position(line, character) throws on negatives — exactly like the real VS Code API). VS Code extension code can only execute against a mocked vscode surface, so vitest is the package's real runtime; I ran it in tmux as an A/B (reverting only the source for BEFORE) and added an adversarial 7-scenario matrix well beyond the PR's single test.

Note: a standalone esbuild bundle of the handler isn't viable here — its import graph reaches @qwen-code/qwen-code-core (tree-sitter .wasm), so the mock-vscode vitest path is the faithful way to exercise the compiled handler.

Behavioral evidence — real handler A/B (observed)

Each row drives handle({ openFile, path }) and records what the editor actually did:

This reproduces #5710 exactly: any zero-containing suffix (a web-UI link can emit a zero-based line) made BEFORE compute a negative position → vscode.Position(-1, …) throws → the handler surfaces a "Failed to open file" error and never places the cursor. AFTER, Math.max(0, parseInt(v,10) - 1) clamps each axis to the file start, so the file opens and the cursor lands at (0,0). Every valid one-based link and the plain path behave identically to before.

Tests

• Full suite on AFTER: vitest run FileMessageHandler.test.ts → 8/8 pass.
• Mutation / teeth check: revert only FileMessageHandler.ts to the parent while keeping the new test → the new openFile clamps zero line and column values to the file start test fails with the precise bug (Position line and character must be non-negative → Failed to open file); the other 7 tests still pass. The new test genuinely guards the fix.

Risk

Low / surgical. Only the previously-broken zero-valued suffixes change behavior (now open at file start instead of erroring) — intended per #5710. Valid one-based :line:column links and suffix-less paths are byte-for-byte unchanged. As the PR notes, negative suffixes like :-1 aren't parsed as line/column by the existing regex, so they're unaffected.

Verdict: ✅ behaves exactly as described. Recommend merge.

I think the clamp is fine as defensive handling, but we should clarify the source of :0.

The handler only gets -1 after it receives a :0 suffix and subtracts 1. I don't see MarkdownRenderer generating 0 by itself unless the original link already contains #L0 / :0.

One related source is read_file.offset: it is documented as 0-based, and toolLocations() returns it as line. If that line is later rendered via FileLink, it gets appended as :<line> and interpreted by openFile as 1-based. That means clamping fixes 0 -> -1, but not the broader off-by-one case for offset > 0.

Could we either add a test through the real producer of :0, or normalize ToolLocation.line before it reaches FileLink?

✅ 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 826b8cca0 — MERGEABLE, FileMessageHandler.ts has no drift vs current origin/main.
• Method: isolated git worktree at the PR head with a clean npm ci, then verified each claim independently.

Results

Mutation test (the key evidence). I reverted FileMessageHandler.ts to main while keeping the new test — it fails with exactly the #5710 bug:

× openFile clamps zero line and column values to the file start
  [FileMessageHandler] Failed to open file: Error: Position line and character must be non-negative

src/app.ts:0:0 made the old code compute parseInt('0') - 1 = -1, and new vscode.Position(-1, -1) throws, so the handler fell into the catch and surfaced showErrorMessage(...). The new test pins exactly that (asserts no error + selection/range at {0,0}); it passes once the clamp is restored.

Correctness review

• New toZeroBasedEditorPosition(value) returns 0 for empty/undefined, else Math.max(0, parseInt(value, 10) - 1). So the 1-based → 0-based conversion can no longer go negative: :0 (and any value the regex captures) clamps to the file start. Applied to both line and column. The regex (?::(\d+))? guarantees the captured groups are digit-only, so parseInt is safe, and the if (lineStr) guard keeps navigation opt-in. Minimal and on-point for bug(vscode): openFile accepts zero line and column values #5710.

Verdict: LGTM. Correct, minimal, mutation-verified (reproduces #5710 exactly); merges cleanly, types/lint/format clean. Safe to merge from my side.

× openFile clamps zero line and column values to the file start
  [FileMessageHandler] Failed to open file: Error: Position line and character must be non-negative

@yiliang114 Thanks — I traced the whole chain against the code. Short version: the data-model inconsistency you spotted is real, but the off-by-one you describe isn't reachable today, and read_file isn't the producer of :0. The clamp is a correct defensive fix and is safe to merge.

What's correct in your analysis

The 0-based vs 1-based mismatch is genuinely there in the data:

• read_file.offset is 0-based — getDescription() renders it as offset + 1 (packages/core/src/tools/read-file.ts).
• toolLocations() returns it raw: return [{ path, line: this.params.offset }] (read-file.ts:97), no +1.
• The ACP emitter also doesn't convert: .map((loc) => ({ path: loc.path, line: loc.line ?? null })) (ToolCallEmitter.ts:309).
• FileLink expects 1-based (line prop doc says "starting from 1") and appends :${line}; openFile then subtracts 1.

So ToolLocation.line being 0-based while FileLink is 1-based is a real latent inconsistency.

Why the off-by-one isn't actually reachable (and read_file isn't the :0 source)

Your inference is hedged on "if that line is later rendered via FileLink" — and it currently isn't:

• routing.ts routes read_file (kind read) to ReadToolCall.
• Every FileLink in ReadToolCall.tsx (lines 155 / 183 / 209 / 239) passes path only — no line, and ReadToolCall doesn't use LocationsList (the component that does forward loc.line).

So read_file's 0-based offset is dropped at render time and never reaches FileLink.line. It produces neither a clickable :0 nor an offset > 0 off-by-one. (edit.toolLocations() returns no line; grep/ripGrep don't override toolLocations() at all — so the 0-based read_file offset is the only value that could be inconsistent, and it isn't rendered.)

The actual source of :0

The other source you mentioned is the real one: a Markdown link with #L0 / #0. MarkdownRenderer's normalizeExplicitFileLink uses ^L?(\d+)(?:-\d+)?$, so #L0 → base:0. A model-authored #L5 (humans write 1-based) → :5 → -1 → line 5, which is correct; only the degenerate #L0 / :0 hit Position(-1). The clamp is exactly the right guard for that, and it only affects :0/negative inputs (:1+ unchanged), so it masks nothing.

Suggestion

• Merge this PR as-is — the defensive clamp is correct.
• The 0-based/1-based inconsistency is worth a separate follow-up: normalize in read-file.ts toolLocations() (line: offset + 1) or in the emitter, so it stays correct if ReadToolCall ever renders a line. Not a blocker here since that path is unwired.
• On testing: a test "through the real producer" would go through the MarkdownRenderer #L0 path, not read_file; the handler-level unit test added here is still a valid boundary guard.

(Static dataflow verification — routing + component source make "the read_file line never reaches FileLink" deterministic, so I didn't need to drive the webview.)