---
phase: 03-diagnostic-features
reviewed: 2026-04-12T00:00:00Z
depth: standard
files_reviewed: 7
files_reviewed_list:
  - src/processing/pixel-compare.ts
  - src/processing/delta-highlighter.ts
  - src/processing/idle-compressor.ts
  - src/processing/gif-exporter.ts
  - src/processing/grid-compiler.ts
  - src/server.ts
  - src/types.ts
findings:
  critical: 0
  warning: 4
  info: 3
  total: 7
status: issues_found
---

# Phase 03: Code Review Report

**Reviewed:** 2026-04-12T00:00:00Z
**Depth:** standard
**Files Reviewed:** 7
**Status:** issues_found

## Summary

Reviewed the Phase 03 diagnostic features: pixel comparison, delta highlighting, idle frame compression, GIF export, grid compiler integration, server tool/resource registration, and updated types. The code is generally well-structured with good defensive checks (zero-frame guards, dimension mismatch handling). No critical security issues found. Four warnings identified around missing dimension validation, missing error handling, logic correctness in idle compression, and duplicated processing pipelines. Three informational items around code duplication and a minor unused schema.

## Warnings

### WR-01: Missing dimension mismatch guard in pixel-compare compareFrames

**File:** `src/processing/pixel-compare.ts:48-64`
**Issue:** `compareFrames` accesses `a.data` and `b.data` at offsets derived from `a.width * a.height`, but never validates that `a` and `b` have the same dimensions. If frames have different dimensions, the function will silently read wrong data or overrun the buffer of the smaller frame, producing corrupt results or a crash. Both callers (delta-highlighter and idle-compressor) resize before calling, but `compareFrames` is a public export and has no self-protection.
**Fix:** Add a dimension check at the start of `compareFrames`:
```typescript
if (a.width !== b.width || a.height !== b.height || a.channels !== b.channels) {
  throw new Error(
    `Frame dimension mismatch: ${a.width}x${a.height}x${a.channels} vs ${b.width}x${b.height}x${b.channels}`
  );
}
```

### WR-02: Idle compressor logic emits non-idle single frames incorrectly on transitions

**File:** `src/processing/idle-compressor.ts:59-71`
**Issue:** When a non-idle frame is detected at index `i`, the code checks `if (i - 1 > idleRunStart)` to decide whether the preceding run was idle. If `i - 1 === idleRunStart` (i.e., the previous single frame was the start of a potential run), it pushes `frames[idleRunStart]` as a single non-idle frame, then sets `idleRunStart = i`. This is correct for truly non-idle frames, but consider the sequence: frame 0 (different from frame 1), frame 1 (different from frame 2), frame 2 (same as frame 3), frame 3. When `i=1` and the comparison says "not idle", the code pushes `frames[0]` and sets `idleRunStart=1`. When `i=2` and the comparison says "not idle" (frame 1 vs frame 2 differ), it pushes `frames[1]` and sets `idleRunStart=2`. Then the flush at line 77 sees `frames.length - 1 (3) > idleRunStart (2)`, so it creates a collapsed frame for indices 2-3. This is correct.

However, the real issue is: when `i - 1 > idleRunStart` is true (we had an idle run), the code creates a collapsed frame for `idleRunStart` to `i-1`, then sets `idleRunStart = i`, but the frame at index `i` (the one that broke the idle run) is never separately evaluated as a potential start of a new idle run with the *next* frame. The current frame `i` becomes `idleRunStart` but its comparison with `i+1` happens on the next loop iteration, which is correct. After closer analysis, the logic is sound but hard to follow. Downgrading concern -- the actual bug risk is in the flush logic at line 77: the condition `frames.length - 1 > idleRunStart` treats any remaining multi-frame tail as an idle run, even if the last pair was not idle. The last comparison result is not checked.

**Fix:** The flush at line 77 should check whether the trailing frames are actually idle. Currently, if the last two frames are different, `idleRunStart` points to the second-to-last frame, and the flush creates a collapsed frame for a non-idle pair. Track a boolean `lastWasIdle` and use it in the flush:
```typescript
// After the loop, check if trailing frames are actually an idle run
if (frames.length - 1 > idleRunStart && lastWasIdle) {
  result.push(await createCollapsedFrame(frames, idleRunStart, frames.length - 1));
} else if (frames.length - 1 > idleRunStart && !lastWasIdle) {
  // Push remaining frames individually
  for (let j = idleRunStart; j < frames.length; j++) {
    result.push(frames[j]);
  }
} else {
  result.push(frames[idleRunStart]);
}
```

### WR-03: GIF resource handler has no error handling for processing pipeline

**File:** `src/server.ts:598-608`
**Issue:** The GIF resource handler at lines 598-608 dynamically imports and calls `compressIdleFrames`, `applyDeltaHighlights`, and `exportGif` without any try/catch. If any of these fail (e.g., a frame has corrupt data, or a dependency is missing), the error will propagate as an unhandled rejection, potentially crashing the MCP server or returning an opaque internal error. Compare with the grid compiler at lines 36-52 which wraps each step in try/catch.
**Fix:** Wrap the processing pipeline in try/catch, consistent with grid-compiler:
```typescript
try {
  let processedFrames = [...session.frames];
  if (session.config.compressIdle) {
    const { compressIdleFrames } = await import("./processing/idle-compressor.js");
    processedFrames = await compressIdleFrames(processedFrames);
  }
  if (session.config.deltaHighlight) {
    const { applyDeltaHighlights } = await import("./processing/delta-highlighter.js");
    processedFrames = await applyDeltaHighlights(processedFrames);
  }
  const { exportGif } = await import("./processing/gif-exporter.js");
  const gifBuffer = await exportGif(processedFrames);
  return {
    contents: [{ uri: uri.href, mimeType: "image/gif" as const, blob: gifBuffer.toString("base64") }],
  };
} catch (err) {
  logger.error(`GIF export failed: ${err instanceof Error ? err.message : String(err)}`);
  return {
    contents: [{ uri: uri.href, mimeType: "text/plain" as const, text: "GIF export failed" }],
  };
}
```

### WR-04: Grid compiler passes zero frames to sharp composite on empty input

**File:** `src/processing/grid-compiler.ts:54-56`
**Issue:** If `processedFrames` is empty after idle compression (or if the input was empty), `Math.ceil(Math.sqrt(0))` is 0, making `cols = 0`. Then `rows = Math.ceil(0 / 0) = NaN`, and `cellWidth = Math.floor(1568 / 0) = Infinity`. This will crash sharp when creating the canvas. The `get_capture_status` handler in server.ts guards against zero frames at line 396, but `compileGrid` itself has no guard and is a public export.
**Fix:** Add an early return at the top of `compileGrid`:
```typescript
if (frames.length === 0) {
  throw new Error("Cannot compile grid with zero frames");
}
```

## Info

### IN-01: Duplicated processing pipeline between grid resource and GIF resource

**File:** `src/server.ts:598-606` and `src/processing/grid-compiler.ts:34-52`
**Issue:** The idle compression + delta highlighting pipeline is duplicated: once inside `compileGrid` (lines 34-52) and once inline in the GIF resource handler (server.ts lines 598-606). If pipeline ordering or options change, both must be updated in sync.
**Fix:** Extract a shared `processFramePipeline(frames, config)` function that both `compileGrid` and the GIF handler can call.

### IN-02: StartCaptureInputSchema in types.ts is not used by server.ts

**File:** `src/types.ts:79-180`
**Issue:** `StartCaptureInputSchema` and `GetCaptureStatusInputSchema` are defined in `types.ts` but `server.ts` defines its own inline schemas in `registerTool` calls. The schemas are kept in sync manually, which is duplication-prone.
**Fix:** Consider importing and using the schemas from `types.ts` in the `registerTool` calls, or remove the schemas from `types.ts` if they are only used for type inference.

### IN-03: Commented-out code style -- empty catch blocks in grid-compiler

**File:** `src/processing/grid-compiler.ts:41,49,99`
**Issue:** The catch blocks at lines 41, 49, and 99 swallow errors with only a debug log. While this is intentional (graceful degradation when optional modules are unavailable), the pattern of catching all errors means genuine bugs (e.g., sharp out of memory) will be silently swallowed too.
**Fix:** Consider catching only `MODULE_NOT_FOUND` / import errors specifically, and re-throwing unexpected errors.

---

_Reviewed: 2026-04-12T00:00:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_