Claude finished @Yeraze's task —— View job

PR Review: fix(traceroute): always send on channel 0 (Primary)

• Read changed files
• Review code quality and correctness
• Check test coverage
• Post findings

Summary

This is a clean, minimal fix for a real usability bug. The root cause and fix are both well understood, and the code is correct. A few minor observations below.

Code Quality

meshRequestRoutes.ts:29 — The fix is correct. Passing 0 as explicitChannel to resolveDestinationChannel causes it to return 0 immediately via the isValidChannelIndex fast path (line 52–54 of resolveDestinationChannel.ts), so the database lookup is skipped entirely. This is both correct and efficient.

One small observation: The redundant parentheses on resolveSourceManager(...) calls (lines 28, 63, 141, 221, 304) appear to be pre-existing style — this PR doesn't introduce them and they're harmless, but they look slightly unusual.

Potential Issues

Minor: resolveDestinationChannel is still called even though its result is predetermined. Passing 0 as the explicit channel causes resolveDestinationChannel to short-circuit immediately and return 0 without hitting the database. While this works correctly and efficiently, the intent could be made clearer by either:

1. Assigning const channel = 0; directly and skipping the call entirely, or
2. Adding a brief inline comment explaining why 0 is passed (the existing block comment at lines 24–28 does this well, though it could reference that the DB lookup is intentionally skipped).

Option 2 is arguably better because it keeps the call site consistent with the other endpoints and leverages the validation/clamping logic in resolveDestinationChannel as a safety net if the argument ever changes. The current approach is fine.

Test Coverage

The updated test at line 68–76 is good:

it('always sends traceroute on channel 0 (Primary) regardless of node stored channel', async () => {
  // node.channel is 2 from the beforeEach mock, but traceroutes must use
  // channel 0 so intermediate nodes can read the unencrypted packet (issue #3696).
  mockManager.sendTraceroute.mockResolvedValue(undefined);
  const res = await request(app).post('/traceroute').send({ destination: '!12345678' });
  expect(res.status).toBe(200);
  expect(res.body.success).toBe(true);
  expect(mockManager.sendTraceroute).toHaveBeenCalledWith(0x12345678, 0);
});

• The beforeEach sets node.channel: 2, so the test correctly proves the stored channel is ignored.
• The assertion toHaveBeenCalledWith(0x12345678, 0) directly verifies the bug fix.
• Test name is descriptive and references the issue.

One missing test case: There's no test verifying that the DB lookup (getNode) is not called during a traceroute, or alternatively a test passing a channel in the request body to confirm it's also ignored (unlike position requests). Both are low priority since the existing assertion already proves channel 0 is used regardless of the mock's stored value.

Security / Performance

No concerns. The fix reduces a DB call (the getNode lookup is skipped because isValidChannelIndex(0) returns true immediately), so it's a minor perf improvement too.

Verdict

Approve. The fix is correct, the test is solid, and the comment block in the route handler clearly explains the reasoning. No blocking issues.