Claude finished @Yeraze's task —— View job

PR Review: refactor(server): extract scripts, data-exchange & server-info route groups

Tasks:

• Gather context and understand request
• Read changed files
• Review code quality and best practices
• Check for bugs/security issues
• Evaluate test coverage
• Post final review

Overall Assessment

This is a clean, behaviour-preserving refactor that follows the pattern established by earlier batches in this series. The extraction is mechanically sound and the handler bodies are moved verbatim. The three test files provide good coverage of the validation paths. A few small observations below.

Code Quality & Best Practices

scriptRoutes.ts

1. Module-level side effect on import (scriptRoutes.ts:16): getEnvironmentConfig() is called at the top level, outside any function. This is a pattern already used elsewhere in the codebase, but it means the config is baked in at module load time and can't be replaced by test mocks applied after the module is first imported. The test file works around this with vi.hoisted / top-level vi.mock, so it's not broken, but it's worth noting.

2. Duplicate directory-existence check in scriptsEndpoint (scriptRoutes.ts:160-163): getScriptsDirectory() already creates the directory if it doesn't exist (scriptRoutes.ts:35-38), so the existsSync guard inside scriptsEndpoint can never be true. This was present before the refactor so it's pre-existing tech debt, but worth flagging.

3. interface ParamSpec declared inside a loop body (scriptRoutes.ts:312-315): The interface is re-declared on every iteration of the pattern-matching loop. TypeScript erases interfaces at compile time so this is harmless, but it's unusual and could be hoisted to the module level or removed in favour of an inline type.

4. Dynamic import() for child_process / util (scriptRoutes.ts:473-475): These are Node built-ins and will always be available. The dynamic imports are carried over from the original code but add unnecessary async overhead; static import at the top of the file would be cleaner.

5. scriptsEndpoint typed with any (scriptRoutes.ts:156): (_req: any, res: any). The surrounding handlers use the typed Request/Response from the existing imports — this one should too.

dataExchangeRoutes.ts — looks clean. No issues.

serverInfoRoutes.ts — looks clean. No issues.

Potential Bugs

6. __dirname path depth comment vs. reality (scriptRoutes.ts:27-29): The comment says "project root is three levels up (../../../) vs. two from server.ts" and the code uses ../../../. At runtime __dirname for src/server/routes/scriptRoutes.ts (after compile to dist/server/routes/) resolves to dist/server/routes/, so path.resolve(__dirname, '../../../') → project root. This appears correct, but the comment says "vs. two from server.ts" — server.ts lives in src/server/ (compiled to dist/server/) so it only needs ../../. The description in the PR matches this. ✅ No bug, just worth double-checking the dev path actually resolves to the same data/scripts as it did from server.ts.

7. AbortController created but signal not always cleared (scriptRoutes.ts:703-704): In the /http/test handler, timeoutId is cleared in the success and fetch-error paths, but the AbortController itself is not checked. If safeFetch throws a non-AbortError synchronously before clearTimeout, the timeout will still fire and abort nothing. This is an existing behaviour but is worth noting.

8. scripts/export — scripts.length logged, not the actual count written (scriptRoutes.ts:850): After filtering out missing/invalid paths, archive.finalize() is called and then logger.info(\✅ Exported ${scripts.length} script(s)`)` logs the requested count, not the count of files actually added to the archive. Pre-existing behaviour.

Security

9. Path traversal in DELETE /scripts/:filename (scriptRoutes.ts:888): path.basename(filename) correctly strips path separators, and there's no further startsWith containment check like the one in /scripts/import (scriptRoutes.ts:787-789). For a DELETE, the impact is limited because getScriptsDirectory() + path.join(scriptsDir, sanitizedFilename) can't escape the directory after path.basename, but adding the same defence-in-depth check as import for consistency would be tidy.

10. req.body spread into process.env-derived object (scriptRoutes.ts:479): scriptEnv starts with { ...process.env } and is then written to with values from req.body (scriptEnv.MESSAGE = testMessage, etc.). Since these keys are fixed literals, there's no injection path, but if mockNode is user-supplied, the destructured mockNode?.nodeNum etc. are still controlled values that go into the environment of a child process. Existing behaviour; the existing prototype-pollution guard at line 431 already covers the PARAM_* case.

Performance

11. scriptsEndpoint re-reads the directory on every call (scriptRoutes.ts:167): fs.readdirSync is synchronous and blocks the event loop. For a small scripts directory this is fine, but since the endpoint is rate-limited and the directory is user-managed, it's unlikely to be a problem in practice.

12. fs.openSync / fs.readSync / fs.closeSync inside a map (scriptRoutes.ts:193-197): Low-level sync I/O in a map callback for each script. Acceptable given the small scale of the scripts directory; no action needed.

Test Coverage

Tests are well-structured and cover the key validation and happy paths. A few gaps:

• DELETE /scripts/:filename — "happy path" (actually deleting a file): Not tested. Only the system-script guard is exercised. Mocking fs.unlinkSync and fs.existsSync would cover the success path and the 404 case.
• GET /stats — no-sourceId variant: The test at line 40 passes ?sourceId=src-1 but there's no test asserting that omitting sourceId passes undefined through to the db calls (line 53 covers the error path but without a sourceId query param, so it incidentally tests this).
• scriptsEndpoint export shape: The only test is expect(typeof scriptsEndpoint).toBe('function'). There's no test that exercises the listing behaviour (e.g. with a mocked fs). This is reasonable for a refactor PR that's not changing behaviour.

Minor Nits

• scriptRoutes.ts:156: scriptsEndpoint uses any types; prefer Request/Response from the existing imports.
• scriptRoutes.ts:407: for (let i = 0; ...) shadows the outer let i = 0 at line 363. This is valid JS/TS (block scoping), but renaming the inner loop variable would aid readability.
• dataExchangeRoutes.ts:2: requireAdmin is imported but requirePermission is unused — actually requireAdmin is used for /export and /import and requirePermission is used for /stats, so both imports are needed. ✅

Summary: The refactor achieves its stated goal cleanly. All observations above are either pre-existing behaviours carried over verbatim or minor polish items. No blockers identified.