---
phase: 02-window-and-region-targeting
reviewed: 2026-04-12T00:00:00Z
depth: standard
files_reviewed: 7
files_reviewed_list:
  - src/capture/scheduler.ts
  - src/capture/targets/region-target.ts
  - src/capture/targets/window-region-target.ts
  - src/capture/targets/window-target.ts
  - src/capture/targets/window-utils.ts
  - src/server.ts
  - src/types.ts
findings:
  critical: 0
  warning: 4
  info: 2
  total: 6
status: issues_found
---

# Phase 02: Code Review Report

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

## Summary

Reviewed window targeting, region targeting, window-region targeting, window utilities, scheduler, server tool registration, and type definitions. The capture target implementations are well-structured with good error handling for null monitors, minimized windows, and empty captures. The scheduler's self-correcting drift compensation logic is clean. The main concerns are around missing bounds-clamping on crop operations (which could cause native library crashes), a division-by-zero edge case in grid dimension calculation, and duplicated schema definitions between server.ts and types.ts.

## Warnings

### WR-01: Crop dimensions can exceed image bounds in RegionTarget

**File:** `src/capture/targets/region-target.ts:41`
**Issue:** The code validates that `relX` and `relY` are within the image, but does not clamp `this.width` and `this.height` to fit within the remaining image area. If the requested region extends beyond the monitor's captured image (e.g., region x=100, width=2000 on a 1920px monitor), `cropSync` receives out-of-bounds dimensions. Depending on the native library behavior, this could silently produce corrupt output or throw an unhandled native error/crash.
**Fix:** Clamp crop dimensions to available image area before calling `cropSync`:
```typescript
const clampedWidth = Math.min(this.width, image.width - relX);
const clampedHeight = Math.min(this.height, image.height - relY);
const cropped = image.cropSync(relX, relY, clampedWidth, clampedHeight);
```

### WR-02: Crop dimensions can exceed image bounds in WindowRegionTarget

**File:** `src/capture/targets/window-region-target.ts:51-56`
**Issue:** Same issue as WR-01 but for window-region captures. The origin point is validated against image bounds, but `regionWidth` and `regionHeight` are passed directly to `cropSync` without checking whether they fit within the captured window image dimensions.
**Fix:** Clamp crop dimensions before calling `cropSync`:
```typescript
const clampedW = Math.min(this.regionWidth, image.width - this.regionX);
const clampedH = Math.min(this.regionHeight, image.height - this.regionY);
const cropped = image.cropSync(this.regionX, this.regionY, clampedW, clampedH);
```

### WR-03: Division by zero when completed session has 0 frames

**File:** `src/server.ts:376-377`
**Issue:** When a session completes with 0 captured frames (all frames skipped due to errors, reaching maxFrames via skipped frame counting), `Math.ceil(Math.sqrt(0))` produces `0` for `cols`, then `Math.ceil(0 / 0)` produces `NaN` for `rows`. These invalid grid dimensions are returned to the caller and would also cause errors in the grid compiler if the resource is read. The scheduler marks a session as "complete" even if zero frames were successfully captured (all skipped but under the consecutive-failure threshold).
**Fix:** Guard against zero frames before computing grid dimensions:
```typescript
if (session.frames.length === 0) {
  return {
    content: [
      {
        type: "text" as const,
        text: JSON.stringify({
          status: "complete",
          sessionId: session.id,
          frameCount: 0,
          timestamps: [],
          capturedDurationMs: 0,
          skippedFrames: session.skippedFrames?.length ?? 0,
        }),
      },
    ],
  };
}
const cols = Math.ceil(Math.sqrt(session.frames.length));
const rows = Math.ceil(session.frames.length / cols);
```

### WR-04: Negative coordinate values not validated for region targets

**File:** `src/server.ts:84-105`
**Issue:** The Zod schemas for `x` and `y` (screen-region) and `region_x` / `region_y` (window-region) accept negative numbers. While `width`/`height` have `.min(1)`, the coordinate fields have no minimum constraint. Negative coordinates passed to `cropSync` would cause undefined behavior in the native library or an unhandled native crash.
**Fix:** Add `.min(0)` to the coordinate schemas:
```typescript
x: z.number().min(0).optional().describe("Region left X in screen pixels..."),
y: z.number().min(0).optional().describe("Region top Y in screen pixels..."),
region_x: z.number().min(0).optional().describe("Sub-region left X relative to window..."),
region_y: z.number().min(0).optional().describe("Sub-region top Y relative to window..."),
```
Apply the same fix in `src/types.ts` lines 112-147 to keep the schemas consistent.

## Info

### IN-01: Duplicated Zod schemas between server.ts and types.ts

**File:** `src/server.ts:45-131` and `src/types.ts:75-160`
**Issue:** The `start_capture` input schema is defined inline in `server.ts` (lines 45-131) and also as `StartCaptureInputSchema` in `types.ts` (lines 75-160). The types.ts version is only used in tests. Any schema change must be made in two places, risking divergence. The `GetCaptureStatusInputSchema` in types.ts is similarly duplicated with the inline schema in server.ts line 329-332.
**Fix:** Import and reuse the schema from `types.ts` in `server.ts`, or remove the unused copy from `types.ts`. Note: the MCP SDK `registerTool` API may require inline object schemas rather than a pre-built `z.object()`, in which case keep the authoritative schema in `types.ts` and use `.shape` to extract individual field schemas for reuse.

### IN-02: Non-null assertion on optional skippedFrames

**File:** `src/capture/scheduler.ts:93,105,135`
**Issue:** `session.skippedFrames` is typed as `SkippedFrame[] | undefined` (optional in the `CaptureSession` interface), but accessed with `!` non-null assertions throughout the scheduler. The scheduler does initialize `session.skippedFrames = []` at line 28, so this is safe at runtime, but the pattern is fragile -- if `startScheduler` is ever called without that initialization, these would throw.
**Fix:** Either make `skippedFrames` required (non-optional) in the `CaptureSession` interface, or initialize it in `SessionManager.create()` instead of the scheduler to centralize the guarantee.

---

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