Phase 3 server optimization: storage refactor, lint gates, benchmark CI - #21
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the “Phase 3 server optimization” work by replacing the legacy DB entrypoint with a modular FileStore storage package, splitting scan/vulnerability server code into clearer submodules, and tightening repo governance via stricter lint/maint checks and benchmark CI triggers.
Changes:
- Introduces
web/src/server/storage/*as the new FileStore-backedstoragefacade (query engine, snapshot codec/IO, lock + upsert pipeline). - Refactors scan + vulnerability server routes into submodules (hot-indexed scan status reads, trend/listing/patch helpers, presenter/query helpers).
- Enforces stricter repo guardrails: lint must be zero-warning, max-lines rules, maint check script, and benchmark regression workflow triggers + deterministic benchmark inputs.
Reviewed changes
Copilot reviewed 88 out of 89 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/server/storage/* | New modular FileStore storage layer (snapshot codec/IO, query engine, repository facade, vulnerability upsert pipeline). |
| web/src/server/routes/scan/* + scan-progress-bus.ts | Hot-indexed scan status reads + metrics logging; progress bus now remembers latest events. |
| web/src/server/routes/vulnerabilities/* | Extracted vulnerability listing, trend, patch delta helpers; routes migrated to storage. |
| web/src/server/vulnerability-presenter.ts + vulnerability-query.ts | Centralized serialization + where-filter builder utilities. |
| web/src/server/health-score-core.ts + health-score.ts | Extracted health scoring core and migrated persistence reads to storage. |
| web/benchmark-scan-workspace.mjs + web/benchmarks/scan-baseline.json | Deterministic benchmark fixtures (seed/workspaceRoot) and refreshed baseline output format. |
| .github/workflows/benchmark-regression.yml | Runs benchmark regression on PR/push touching server/benchmark paths (plus schedule/dispatch). |
| eslint.config.mjs + package.json scripts | Lint gates strengthened; maint hotspot checks added. |
| web/src/common/hooks/* + components bridge deletions | Removes hook atom re-exports and deletes one-line component bridge re-export files; updates imports accordingly. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function clearInflightReferences(taskId: string): void { | ||
| inflightScans.delete(taskId) | ||
| } | ||
|
|
||
| export function tryGetInflightTaskId( | ||
| body: ScanBody, | ||
| engineMode: 'baseline' | 'agentic_beta', | ||
| ): { | ||
| fingerprint: string | ||
| existingTaskId: string | undefined | ||
| } { | ||
| const fingerprint = computeScanFingerprint( | ||
| body.files, | ||
| body.depth, | ||
| body.forceRescan ?? false, | ||
| engineMode, | ||
| ) | ||
|
|
||
| return { | ||
| fingerprint, | ||
| existingTaskId: inflightScans.get(fingerprint), | ||
| } | ||
| } | ||
|
|
||
| export function registerInflightTask(taskId: string, fingerprint: string): void { | ||
| inflightScans.set(taskId, taskId) | ||
| inflightScans.set(fingerprint, taskId) | ||
| } |
There was a problem hiding this comment.
clearInflightReferences only deletes the taskId key, but registerInflightTask also stores a fingerprint -> taskId mapping. After completion/failure, the fingerprint mapping remains until TTL expiry, causing avoidable reads and dedupe lookups against non-running tasks. Consider tracking taskId -> fingerprint (reverse index) so both keys can be cleared, or only storing the fingerprint mapping and removing the unused taskId -> taskId entry.
| for (const item of indexed) { | ||
| const source = item.vuln.source === 'dast' ? 'dast' : 'sast' | ||
| const normalizedPath = normalizeStableFingerprintPath(item.vuln.filePath) | ||
| const normalizedType = item.vuln.type.trim().toLowerCase() | ||
| const normalizedSnippet = normalizeStableFingerprintSnippet(item.vuln.codeSnippet) | ||
| const baseKey = `${normalizedPath}::${normalizedType}::${normalizedSnippet}` | ||
| const nextIndex = (counters.get(baseKey) ?? 0) + 1 | ||
| counters.set(baseKey, nextIndex) | ||
| const stableFingerprint = | ||
| typeof item.vuln.stableFingerprint === 'string' && | ||
| item.vuln.stableFingerprint.trim().length > 0 | ||
| ? item.vuln.stableFingerprint | ||
| : createStableFingerprint({ | ||
| filePath: item.vuln.filePath, | ||
| type: item.vuln.type, | ||
| codeSnippet: item.vuln.codeSnippet, | ||
| index: nextIndex, | ||
| }) |
There was a problem hiding this comment.
normalizeVulnerabilityInputsForUpsert uses normalizeStableFingerprintPath/Snippet to build baseKey for the per-(path,type,snippet) counter, but createStableFingerprint() applies a different normalization internally. If the grouping logic doesn’t exactly match the fingerprint normalization, the computed index can differ and produce different stableFingerprint values for the same underlying issue across scans (breaking dedupe/trend/relocation). Consider reusing/exporting the exact normalization used by createStableFingerprint (or introducing a shared createStableFingerprintBaseKey() helper) so baseKey and fingerprint generation are consistent.
| const whereOr = vulns.map((item) => ({ | ||
| filePath: item.filePath, | ||
| line: item.line, | ||
| column: item.column, | ||
| codeHash: item.codeHash, | ||
| type: item.type, | ||
| })) | ||
|
|
||
| const rows = (await client.vulnerability.findMany({ | ||
| where: { OR: whereOr }, | ||
| select: { |
There was a problem hiding this comment.
buildExactIdByKey builds a large where: { OR: ... } array from every input vulnerability. With the FileStore query engine this becomes O(existingRows × inputCount) matching work and can get very expensive on large workspaces. Consider avoiding a giant OR by narrowing with { filePath: { in: [...] } } first, or fetching a smaller candidate set and indexing it in memory by the idempotent key.
| const latestEventByTaskId = new Map<string, ScanProgressEvent>() | ||
| let latestUpdatedAtMs = 0 | ||
| let latestTaskId: string | null = null | ||
|
|
||
| function cloneProgressEvent(event: ScanProgressEvent): ScanProgressEvent { | ||
| return { | ||
| ...event, | ||
| } | ||
| } | ||
|
|
||
| export function rememberScanProgress(event: ScanProgressEvent): void { | ||
| latestEventByTaskId.set(event.id, cloneProgressEvent(event)) | ||
| const updatedAtMs = Date.parse(event.updatedAt) | ||
| if (!Number.isNaN(updatedAtMs) && updatedAtMs >= latestUpdatedAtMs) { | ||
| latestUpdatedAtMs = updatedAtMs | ||
| latestTaskId = event.id | ||
| } | ||
| } |
There was a problem hiding this comment.
latestEventByTaskId (and the latestTaskId tracking) is never pruned in production code. Since emitScanProgress remembers every task’s last event, a long-running server can accumulate an unbounded number of completed task entries. Consider adding a bounded size/TTL cleanup (e.g., cap map size, or delete entries once status is completed/failed and older than N minutes).
Summary
web/src/serverfrom legacy db entry to modularstoragepackage and split server pipelinesmaint:checkguardrails for max-lines/hotspots.kiro/steering/*andAGENTS.mdwith the new architecture and governanceValidation