fix(api): GET /stats returns 500 whenever sourceId is omitted

[rancur]

The bug

GET /api/stats returns HTTP 500 / STATS_FAILED for any caller that does not pass sourceId.

routes/dataExchangeRoutes.ts scopes three of its four queries with statsSourceId ?? ALL_SOURCES, but hands the fourth a bare statsSourceId:

const messageCount  = await databaseService.messages.getMessageCount(statsSourceId ?? ALL_SOURCES);
const nodeCount     = await databaseService.nodes.getNodeCount(statsSourceId ?? ALL_SOURCES);
const channelCount  = await databaseService.channels.getChannelCount(statsSourceId ?? ALL_SOURCES);
const messagesByDay = await databaseService.getMessagesByDayAsync(7, statsSourceId);   // <-- undefined

With sourceId omitted that argument is undefined, which BaseRepository.withSourceScope rejects by design:

withSourceScope: sourceId is required. Pass a concrete sourceId, or the
ALL_SOURCES sentinel for an intentional cross-source query. Omitting sourceId
used to silently return rows from every source (data leak).

The throw is caught by the route's catch and surfaced as a generic 500, so the endpoint fails with no hint of the cause. The comment immediately above the block states the intended behaviour — "intentional cross-source: stats totals span all sources when no sourceId is specified" — so this is a missed conversion, not a deliberate restriction. Only the ?sourceId=... path ever worked; the aggregate path was dead.

Why the type checker did not catch it

DatabaseService.getMessagesByDayAsync declared sourceId?: string, narrower than the SourceScope that its own messagesRepo.getMessagesByDay accepts. Passing the ALL_SOURCES symbol would not have compiled, so the correct call was not even expressible from the service. Widening the service signature to SourceScope fixes the leaky abstraction and keeps the service in step with the repository layer.

Changes

• routes/dataExchangeRoutes.ts — pass ALL_SOURCES when no sourceId is supplied, matching the three sibling calls.
• services/database.ts — widen getMessagesByDayAsync's sourceId to SourceScope.
• routes/dataExchangeRoutes.test.ts — regression test for the bare /stats call, asserting all four counts receive the sentinel. The existing test only exercised /stats?sourceId=..., which is why this went unnoticed.

Verification

• Reproduced on a live 4.13.0 deployment: GET /api/stats → 500 STATS_FAILED, GET /api/stats?sourceId=<id> → 200. After the change, the bare call returns 200 with correct cross-source totals (message/node/channel counts, and a non-empty messagesByDay).
• The new test fails without the route change (expected "vi.fn()" to be called with arguments: [ 7, Symbol(ALL_SOURCES) ]) and passes with it.
• npx vitest run src/server/routes/dataExchangeRoutes.test.ts → 7 passed.
• npx tsc --noEmit → clean.

No behaviour change for callers that already pass sourceId.

[Yeraze]

Review

Fixes a real, reachable bug with an appropriately minimal diff. I verified the description's claims against the tree rather than taking them on trust:

• ALL_SOURCES is already imported at dataExchangeRoutes.ts:4, so the route change compiles as-is
• SourceScope is already imported at services/database.ts:71 — no new import needed there either
• messagesRepo.getMessagesByDay already accepts SourceScope (messages.ts:468), so the "leaky abstraction" framing is accurate: the service signature genuinely was the narrower of the two
• SourceScope = string | typeof ALL_SOURCES (base.ts:36), so widening is strictly additive
• The route is the only non-test caller, so nothing else is affected

Diagnosis and fix both look right.

Two suggestions

1. Hoist the scope. The expression appears four times and the bug is precisely that one copy drifted. One binding makes that impossible:

const scope = statsSourceId ?? ALL_SOURCES;   // intentional cross-source
const messageCount  = await databaseService.messages.getMessageCount(scope);
const nodeCount     = await databaseService.nodes.getNodeCount(scope);
const channelCount  = await databaseService.channels.getChannelCount(scope);
const messagesByDay = await databaseService.getMessagesByDayAsync(7, scope);

2. Make the parameter required rather than just wider. The sibling repo methods take sourceId: SourceScope — required (messages.ts:240) — which is exactly why those three were correct by construction. Leaving getMessagesByDayAsync(days, sourceId?: SourceScope) optional keeps undefined expressible, so the identical bug can recur and will again surface as a runtime 500 rather than a compile error. Widening fixed expressibility of ALL_SOURCES; it didn't remove the hazard. Since the route is the only caller, tightening it is cheap.

Together these make the bug unrepresentable instead of fixed once.

One thing worth confirming

Because the bare path was dead, this makes cross-source aggregates reachable for the first time. requirePermission('dashboard', 'read') is invoked without sourceIdFrom, so it isn't source-scoped — a user whose dashboard grant is scoped to a single source will now receive totals spanning every source.

That intent is already established by the three sibling calls and the code comment, so this PR doesn't introduce it. But it's the change that makes it observable, and it deserves a conscious yes rather than arriving as a side effect.

Tests

Good. The regression test asserts all four counts receive the sentinel, and it failing without the route change with the exact expected diff is the right evidence.

On convention: the file uses the vi.mock('../../services/database.js') pattern rather than createRouteTestApp, which CLAUDE.md asks of changed route tests. I'd leave it here. This test asserts which argument reached the DB layer, which needs a mock; the harness runs real SQL and verifies outcomes, so it couldn't express this assertion as directly. Converting the file for a one-line fix would also be disproportionate.

Risk

Low. No behavior change for callers passing sourceId; the only newly-live path is the one that was returning 500.

Approve — suggestions 1 and 2 are worth folding in, but neither is blocking.

[Yeraze]

@rancur — I've pushed the two suggestions from my review directly to this branch as 0446a9f8, since maintainerCanModify is enabled. Flagging it so the commit isn't a surprise; revert or reshape it freely, it's your PR.

1. Hoisted the scope into a single statsScope binding. The bug you fixed was one of four copies of statsSourceId ?? ALL_SOURCES drifting from the other three — with one binding that can't recur.

2. Made sourceId required, on both getMessagesByDayAsync and the repository's getMessagesByDay. This is the part I think matters most: widening the type to SourceScope made ALL_SOURCES expressible, but leaving the parameter optional kept undefined expressible too — so the exact omission this PR fixes would still compile and still fail at runtime with a 500. Required makes it a compile error instead.

That also brings both in line with the sibling count methods (getMessageCount, getNodeCount, getChannelCount all take sourceId: SourceScope), which is precisely why those three were correct by construction and this one wasn't. The route is the only non-test caller of either method, so nothing existing is narrowed.

Verified on the branch: tsc --noEmit clean, lint:ci clean with no baseline growth, full suite success: true — 12,772 passed / 0 failed. (Lower total than main simply because this branch predates a few PRs merged today.) Your regression test still passes unchanged.

One thing from my review I did not act on, because it's a judgment call rather than a defect: this PR makes the cross-source aggregate path reachable for the first time, and requirePermission('dashboard', 'read') isn't source-scoped here — so a user whose dashboard grant covers one source will now see totals spanning all of them. Three of the four counts already intended that, so it isn't introduced here, but it's worth a deliberate yes from a maintainer rather than arriving as a side effect of the fix.