feat: add cipher performance profiling framework - #647

tests/unit/performance/profiler.test.ts (1)

66-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing direct unit tests for measureMemoryUsage.

measureMemoryUsage in lib/performance/profiler.ts (lines 116-161) has nontrivial platform-specific branches (Chrome performance.memory, Node process.memoryUsage, GC forcing), but no describe('measureMemoryUsage', ...) block exists here, unlike the sibling measureExecutionTime coverage above. The profile() tests only check that metrics.memoryUsage exists, not that the underlying measurement logic behaves correctly for each branch.

Add direct tests mocking process.memoryUsage and performance.memory to cover both runtime branches and the zero-iterations case.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/performance/profiler.test.ts` around lines 66 - 98, Add a
dedicated describe('measureMemoryUsage') test block alongside
describe('measureExecutionTime') in the profiler tests. Mock process.memoryUsage
and performance.memory to exercise both Node and Chrome memory-measurement
branches, assert the returned measurement counts and values, and include a
zero-iterations case that returns an empty array.

lib/performance/profiler.ts (1)

143-150: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

GC-forcing relies on exception-based feature detection, duplicated in two places.

Both measureMemoryUsage (lines 143-150) and profile (lines 322-329) read global as { gc?: () => void } without first checking whether global exists. In a real browser without a global polyfill, this throws a ReferenceError on every call, which the surrounding try/catch silently swallows. Functionally this is safe, but using exceptions as control flow inside a hot loop (default 100 iterations) adds unnecessary overhead in browser runs, and the logic is duplicated verbatim.

Use typeof global !== 'undefined' instead, which never throws for an undeclared identifier, and extract the check into one shared private helper used by both call sites.

♻️ Proposed refactor

+  private static tryForceGc(): void {
+    if (typeof global !== 'undefined' && typeof (global as { gc?: () => void }).gc === 'function') {
+      ;(global as { gc: () => void }).gc()
+    }
+  }

Then replace both occurrences:

-      try {
-        const globalWithGc = global as { gc?: () => void }
-        if (typeof globalWithGc.gc === 'function') {
-          globalWithGc.gc()
-        }
-      } catch {
-        // Ignore if gc is not available
-      }
+      this.tryForceGc()

Also applies to: 322-329

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/performance/profiler.ts` around lines 143 - 150, Extract the duplicated
GC detection and invocation from measureMemoryUsage and profile into one shared
private helper. In that helper, guard access with typeof global !== 'undefined'
before casting and invoking global.gc, preserving the no-op behavior when GC is
unavailable; replace both existing try/catch blocks with calls to the helper.

lib/performance/ci.ts (3)

29-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Loose outputFormat type lets an unvalidated string reach the reporter.

outputFormat is typed as 'text' | 'json' | 'markdown' | string (Line 33). Adding | string to a literal union widens it for editor autocomplete purposes and, combined with the unchecked cast at Line 86 (opts.outputFormat as 'text' | 'json' | 'markdown'), lets any string value pass through to PerformanceReporter.exportReportToFile without validation. If a caller passes an unsupported format string, this fails silently downstream instead of at the CI options boundary.

Narrow the type to the three literals (drop | string) and validate before casting, defaulting to 'text' when the value doesn't match a known format.

Also applies to: 84-88

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/performance/ci.ts` around lines 29 - 36, The CIOptions interface has an
overly permissive type for outputFormat by including the bare string type
alongside the three literal formats, which allows any string value to pass
through without validation. Narrow the outputFormat type to only the three
literal values (remove the | string union) in the CIOptions interface
definition. Then, in the code around line 86 where the cast occurs (before
passing to PerformanceReporter.exportReportToFile), add validation logic that
checks if the outputFormat value matches one of the known formats ('text',
'json', or 'markdown'), defaulting to 'text' if it does not, before performing
the cast to the narrowed literal type.

---

42-104: 📐 Maintainability & Code Quality | 🔵 Trivial

Document that profiles without baselines bypass regression enforcement.

batchCompare (called at Line 55) silently drops any profile without a stored baseline, so newly added ciphers produce totalChecked: 0/success: true with no regression signal at all. This matches the test at tests/unit/performance/ci.test.ts Lines 122-128, so it's by design, not a bug. Consider calling out in getRecommendedCIConfig (or wiring validateBaselinesExist into the recommended pipeline) that a CI step should also assert baseline coverage, so a newly introduced cipher can't slip through both checks unnoticed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/performance/ci.ts` around lines 42 - 104, Add documentation in
getRecommendedCIConfig (or by integrating validateBaselinesExist into the
recommended pipeline) to explain that batchCompare silently excludes profiles
without stored baselines from regression checks. This ensures users understand
that newly added profiles bypass regression enforcement entirely and will report
success with totalChecked: 0 unless baseline coverage is explicitly validated as
part of the CI flow.

---

303-311: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Use the existing bulk saveBaselines API instead of looping per profile.

Each call to BaselineManager.saveBaseline inside this loop performs a full loadBaselines + full saveBaselines (which serializes and rewrites the entire baseline file/localStorage entry) for every single profile. For n profiles, that's n full read/write cycles of the whole baseline store instead of one.

BaselineManager.saveBaselines(map) already accepts a batch, so this can be reduced to a single load, in-memory merge, and single save.

♻️ Proposed refactor

   static createCIBaseline(
     profiles: PerformanceProfile[],
     version: string,
     commitHash: string,
   ): void {
-    for (const profile of profiles) {
-      BaselineManager.saveBaseline(profile, version, commitHash)
-    }
+    const baselines = BaselineManager.loadBaselines()
+    for (const profile of profiles) {
+      baselines.set(profile.cipherId, BaselineManager.createBaseline(profile, version, commitHash))
+    }
+    BaselineManager.saveBaselines(baselines)
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/performance/ci.ts` around lines 303 - 311, Replace the for loop in
createCIBaseline that calls BaselineManager.saveBaseline for each profile with a
single call to BaselineManager.saveBaselines. Build a map from the profiles
array that matches the format saveBaselines expects, passing the version and
commitHash parameters appropriately so that all profiles are persisted in a
single load-merge-save operation instead of n separate operations.

tests/unit/performance/reporter.test.ts (1)

242-257: 📐 Maintainability & Code Quality | 🔵 Trivial

Test performs real filesystem writes with no cleanup and weak assertions.

This test writes to /tmp/test-report.txt on the real filesystem and leaves the artifact behind. Its assertion logic is trivial either way: if exportReportToFile succeeds, nothing is asserted about file content; if it throws, the only check is that an error message is defined, which passes for any error. Mock fs (for example, with vi.mock('fs', ...)) to test the write path deterministically and remove the real I/O side effect.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/performance/reporter.test.ts` around lines 242 - 257, Replace the
real filesystem call in the “should not throw when process exists but fs fails
gracefully” test with a mocked fs.writeFile implementation, such as via vi.mock,
and assert the expected write behavior deterministically. Remove the try/catch
and /tmp/test-report.txt dependency so the test validates
PerformanceReporter.exportReportToFile without leaving filesystem artifacts.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes