chore(api): remove 4.13 grace-period shims — v1 root paths + /api/upgrade 410s (closes #4117)

[Yeraze]

Summary

Removes the two deprecation shims that shipped in 4.13 with a documented one-release grace period. Docs and the announcement blog promised both would be removed in 4.14; this does that cleanly.

Part 1 — v1 root-path API shim (removed)

• Deleted src/server/routes/v1/deprecatedShim.ts (the deprecationShim middleware that stamped the Warning: 299 header) and its test.
• Removed the legacy root mounts in v1/index.ts (/api/v1/{nodes,messages,channels,telemetry,traceroutes,packets,network,status} + root position-history). These now 404.
• Simplified the getScopedSourceId helpers across the sub-routers (and the inline resolver in status.ts) to read the :sourceId path param only — the ?sourceId= / body fallbacks existed solely to serve the removed root mounts.
• Kept the canonical /api/v1/sources/{sourceId}/... mounts, attachSource, the default alias, and the global /api/v1/solar + /api/v1/channel-database endpoints.
• openapi.yaml: dropped the deprecated /nodes root path block and all Warning: 299 framing.

Part 2 — /api/upgrade/* 410 FEATURE_RETIRED endpoints (removed)

• Deleted src/server/routes/upgradeRoutes.ts (the 410 stubs from feat!: retire in-app auto-upgrade, replace with update notifications #4108) and its server.ts mount. These paths now 404 like any unknown route (Auto-Upgrade Retirement Plan, Phase 3).

Docs

• REST_API.md, API_REFERENCE.md, development/api-reference.md: warning banners rewritten to past tense ("removed in 4.14 — now return 404"); migration table retained for reference.
• Historical blog post left as-is (it documents what 4.13 did). docs/api/API.md already marked outdated (no change). FAQ auto-upgrade stubs left (they reference the retirement, not the 410 endpoints).
• Docs build (npm run docs:build, vitepress) is green.

Tests

• Converted the legacy-shape tests (v1-api.test.ts, messages.search.test.ts, messages.permissions.test.ts) to the per-source shape.
• Added assertions: GET /api/v1/nodes (root) → 404, GET /api/v1/messages (root) → 404, canonical /api/v1/sources/default/nodes still works; /api/upgrade/* → 404 (not 410).

Acceptance criteria

• GET /api/v1/nodes (root shape) → 404; GET /api/v1/sources/default/nodes still works.
• deprecatedShim.ts deleted; no Warning: 299 emitted anywhere (no code sets a Warning header; remaining mentions are past-tense docs/comments + the historical blog).
• /api/upgrade/trigger → 404 (not 410).
• Docs updated to past tense; docs build green.
• Server typecheck clean; npm run lint:ci exits 0.
• Full Vitest suite: 9525 passed; the only remaining failures are pre-existing/environmental in this worktree (missing ts-overlapping-marker-spiderfier-leaflet dep + the [BUG] MeshCore: Virtual Node functionality still does not work correctly #4094 fork-dependent meshcoreCompanionCodec test) — all confirmed to fail identically on the clean baseline, and they pass in CI. My changes introduce zero new failures.

Note on the default alias

While converting tests I found a pre-existing latent issue (not introduced here, out of scope): attachSource normalizes req.params.sourceId to the resolved source id, but Express re-derives req.params for the mergeParams sub-router, so handlers reading req.params.sourceId (the codebase convention, incl. actions.ts) see the raw URL literal "default" rather than the resolved id. Route resolution for default still works (returns 200); deeper DB scoping by the literal "default" is the latent bug. Kept helpers param-only per the issue's guidance; used concrete source ids where a test asserts real scoping. Flagging for a follow-up.

⚠️ MERGE TIMING

This breaks the documented grace period if it ships in a 4.13.x release. It should only be merged once 4.14 is the active target. Do not merge before then.

🤖 Generated with Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: chore(api): remove 4.13 grace-period shims

Tasks

• Gather context and understand the PR
• Read changed files
• Review code quality, bugs, security, test coverage
• Post final review

---

Overall Assessment

This is a clean, well-scoped removal PR. The diff is net-negative (−211 lines), the intent is clear, and the approach is sound. A few observations below, ranging from blocking to informational.

---

Issues Worth Addressing

1. Duplicate test in v1-api.test.ts (minor bug)

v1-api.test.ts:1394–1405 has an exact duplicate of the ?before=2500 position-history test at lines 1381–1393. Both describe blocks have the same title 'should filter positions by before parameter' and identical bodies. One of them should be removed or renamed (e.g. to test an after or since parameter).

2. void getScopedSourceId(req) in network.ts:65 is a smell

network.ts:65:

void getScopedSourceId(req);

The call is discarded to silence a lint warning, with a TODO referencing getDirectNeighborStatsAsync not yet accepting sourceId. This pattern is fine as a placeholder, but it means the /direct-neighbors endpoint silently ignores the :sourceId path param — a caller with a scoped token expecting source isolation will get cross-source data without any indication. Consider returning a 501 Not Implemented or a response note until the repo is extended, rather than silently ignoring the param.

3. nodes.ts single-node lookup fetches all nodes (pre-existing, worth flagging)

nodes.ts:143–144:

const sourceNodes = (await databaseService.nodes.getAllNodes(sourceId ?? ALL_SOURCES)) as unknown as DbNode[];
const node = sourceNodes.find(n => n.nodeId === nodeId);

Fetching all nodes and filtering in-process is inefficient when only one node is needed. This is pre-existing code, not introduced here, but since this file was touched it's a good opportunity to note it. If there's a getNode(nodeId, sourceId) repo method, it should be used instead.

---

Positive Observations

Good: getScopedSourceId simplification is correct. Removing the ?sourceId= / body fallbacks that existed solely for the removed root mounts keeps each helper focused. The remaining one-liner return typeof req.params.sourceId === 'string' ? req.params.sourceId : undefined; is uniform across all sub-routers.

Good: Upgrade routes test conversion to createRouteTestApp. upgradeRoutes.test.ts correctly uses the real harness (per CLAUDE.md requirements) instead of the deprecated monkey-patch pattern.

Good: messages.permissions.test.ts now uses real SQL. The conversion from vi.mock('../../services/database.js', ...) to createRouteTestApp with vi.spyOn passthroughs is exactly the pattern described in CLAUDE.md. The source-isolation assertions (sourceA grants → 200, sourceB → count 0) provide meaningful regression coverage.

Good: The default alias latent issue is documented but left out of scope. The note in the PR description about req.params.sourceId being re-derived by Express after attachSource normalizes it is accurate and appropriate to flag for a follow-up rather than blocking this cleanup PR.

Good: Test coverage of the removal. The V1 legacy root paths removed in 4.14 describe block explicitly asserts 404 on /api/v1/nodes, /api/v1/messages, and confirms the canonical /api/v1/sources/default/nodes still returns 200 with no Warning header.

Good: Server typecheck and lint:ci are confirmed green.

---

Security / Permissions

No regressions observed. Permission checks remain in place on all retained routes. The removal of root mounts closes the surface area where a caller could bypass the per-source attachSource middleware by hitting the old shape.

---

Summary

Two items worth fixing before merge:

1. Remove the duplicate position-history test (v1-api.test.ts ~line 1394).
2. Decide whether /direct-neighbors should explicitly signal that sourceId is not yet honored, or leave the TODO as-is with a comment (not silently void-discarding the parsed param).

Neither is a blocker in the strictest sense, but the duplicate test is a clear bug-in-tests and should be cleaned up. The void discard is a pre-existing pattern and acceptable to leave as a follow-up.

Merge timing note is well-called — this should only land once 4.14 is the active target.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: chore(api): remove 4.13 grace-period shims

Tasks

• Gather context and understand the PR
• Read changed files
• Review code quality, bugs, security, test coverage
• Post final review

---

Overall Assessment

This is a clean, well-executed removal PR. The diff is net-negative, the scope is precisely bounded, and all the acceptance criteria are met. The previous review's two flagged items (duplicate test, void getScopedSourceId) were already present on the baseline — neither was introduced by this PR. Below I distinguish between confirmed pre-existing issues and anything newly worth flagging.

---

Issues Found

1. Duplicate test — still present (pre-existing, flagged in July review)

v1-api.test.ts:1409 and v1-api.test.ts:1422 are identical: both are it('should filter positions by before parameter', ...) hitting ?before=2500 with identical assertions. This was flagged in the July 18 review and remains. Since the file was substantially modified in this PR, it's a good time to clean it up.

One of the two should either be removed or repurposed (e.g., test ?after= or ?since= parameter to complement the before coverage — the ?since=1500 mock-call assertion at line 1435 doesn't verify the filtered response shape, so that's a reasonable gap to fill).

2. void getScopedSourceId(req) in network.ts:66 (pre-existing, flagged in July review)

Still present. The call is discarded to silence a lint warning, with a TODO(#2773 follow-up) comment. The practical effect is that /api/v1/sources/:sourceId/network/direct-neighbors silently returns cross-source data regardless of which :sourceId was requested. This is not a regression introduced here, and the TODO is clear, but it's worth noting since network.ts was touched in this PR.

3. Two new doc files bundled into a cleanup PR

docs/internal/dev-notes/MCP_SERVER_PLAN.md (+203 lines) and docs/internal/dev-notes/RETICULUM_SOURCE_RESEARCH.md (+162 lines) appear in the diff but have nothing to do with removing the 4.13 shims. These are design notes for future features (MCP server integration, Reticulum source support). They're low-risk content-wise, but they make the commit history harder to bisect — a reviewer or future git log --follow on these files will find them orphaned in a "remove deprecation shims" commit. Consider splitting them into a separate commit (or a separate PR) so the shim-removal commit stays focused.

---

Positive Observations

getScopedSourceId simplification is correct. Every sub-router helper is now a clean one-liner delegating to resolvedSourceIdFromPath(req), which correctly prefers req.source.id (set by attachSource) over the raw path param — handling the default alias correctly across mergeParams sub-routers.

sourceParam.ts comment on the default alias re-derivation is accurate and helpful (resolvedSourceIdFromPath lines 160–175). The comment precisely explains the Express mergeParams gotcha and why req.source must be the authoritative carrier.

attachSource middleware is correct. The default alias resolution, permission enforcement, admin bypass, and req.params.sourceId normalization are all sound. The latent bug (Express re-deriving req.params in sub-routers so downstream handlers still see "default") is correctly documented in resolvedSourceIdFromPath, and req.source.id is the right fix — already used by all the simplified getScopedSourceId helpers.

Test conversion to createRouteTestApp is correct. messages.permissions.test.ts now uses real SQL for permission enforcement rather than a hand-rolled lambda — this is the pattern required by CLAUDE.md and catches the regression class documented in issue #3745. The source-isolation assertions (sourceA → 200, sourceB → count 0) are meaningful.

upgradeRoutes.test.ts correctly asserts 404 (not 410). The test harness mounts no /api/upgrade router, so any request falls through to a generic 404. The expect(res.body?.code).not.toBe('FEATURE_RETIRED') assertion is a nice double-check.

Legacy 404 coverage is solid. The V1 legacy root paths removed in 4.14 describe block confirms /api/v1/nodes → 404, /api/v1/messages → 404, and /api/v1/sources/default/nodes → 200 with no Warning header — exactly the right three cases.

openapi.yaml cleanup is correct — the deprecated /nodes root path block and all Warning: 299 framing are removed, leaving only the canonical /sources/{sourceId}/... paths.

---

Security / Permissions

No regressions. Removing the root mounts eliminates the surface where callers could attempt to bypass attachSource by hitting the old shape. All retained routes still enforce requireAPIToken() → attachSource() → permission check.

---

Summary

Blocking:

• None strictly blocking — both flagged items below are pre-existing.

Should fix before merge (or in immediate follow-up):

1. Duplicate test (v1-api.test.ts lines 1409 and 1422) — identical describe title and body; one should be removed or repurposed.
2. Unrelated doc files (MCP_SERVER_PLAN.md, RETICULUM_SOURCE_RESEARCH.md) bundled into this commit — consider splitting to keep commit history clean.

Acknowledged / follow-up issues:

• void getScopedSourceId(req) in network.ts:66 — pre-existing, TODO is clear, out of scope here.
• default alias req.params re-derivation latent bug — correctly documented, appropriately deferred.

The merge timing note stands: this should only land once 4.14 is the active target.

[Yeraze]

Rebased onto main (was 246 commits behind, last touched 2026-07-18) now that 4.14 is open.

Conflicts resolved (13 files). All the same shape: main introduced a resolvedSourceIdFromPath(req) helper while this PR removes the legacy ?sourceId= fallback. Resolution kept both — main's helper, this PR's removal:

// was (main):  resolvedSourceIdFromPath(req) ?? req.query.sourceId  [?? req.body.sourceId]
// now:         resolvedSourceIdFromPath(req)

Applied to channels, messages, network, nodes, packets, positionHistory, status, telemetry, traceroutes. In v1-api.test.ts the conflict was imports: kept main's TxDisabledError, dropped ALL_SOURCES (zero remaining uses after the root-path tests this PR deletes).

One real fix needed on top of the rebase. Two of main's newer TX_DISABLED tests still posted to the root path this PR removes:

• should return 409 TX_DISABLED… → got 404, because /api/v1/messages is gone
• should send short messages directly… → got 409, collateral damage: the 404 short-circuited before reaching the manager, so the first test's mockRejectedValueOnce was never consumed and leaked into the next test

Repointed the first to /api/v1/sources/test-source/messages, which fixes both.

Full suite after rebase: 13,090 passed, 0 failed. lint:ci and tsc clean.