---
phase: 09-timing-profiles
reviewed: 2026-04-13T12:00:00Z
depth: standard
files_reviewed: 5
files_reviewed_list:
  - src/profiles/profile-manager.ts
  - src/profiles/profile-resolver.ts
  - src/profiles/profile-types.ts
  - src/profiles/timing-presets.ts
  - src/server.ts
findings:
  critical: 1
  warning: 2
  info: 1
  total: 4
status: issues_found
---

# Phase 9: Code Review Report

**Reviewed:** 2026-04-13T12:00:00Z
**Depth:** standard
**Files Reviewed:** 5
**Status:** issues_found

## Summary

Reviewed the timing profiles feature: type definitions, persistence (ProfileManager), merge logic (profile-resolver), built-in presets, and MCP tool registration in server.ts. The type layer and persistence code are clean and well-structured. However, the integration in `start_capture` has two bugs that cause timing profile values to be silently ignored at runtime -- one critical (zod defaults defeating `??` override semantics) and one warning-level (screenshot profile merge discarding timing resolution). A separate warning-level issue makes timing profiles less useful since `interval_ms` is a required zod field even when a profile could supply it.

## Critical Issues

### CR-01: Zod boolean defaults defeat timing profile override semantics

**File:** `src/server.ts:141-150` and `src/server.ts:193-200`
**Issue:** The `start_capture` zod schema defines `delta_highlight`, `compress_idle`, and `gif_export` with `.default(false)`. When the user omits these fields, zod fills them with `false` before the handler runs. These `false` values are then passed to `resolveTimingProfile` at lines 193-200 as inline params. Since `resolveTimingProfile` uses nullish coalescing (`??`), `false` is treated as "user explicitly provided false" and the timing profile's value (e.g., `deltaHighlight: true` from the `debug-flicker` preset) is silently discarded. This means timing profile boolean flags can never take effect -- they are always overridden by the zod default `false`.

**Fix:** Change the boolean fields in the `start_capture` schema to `.optional()` instead of `.default(false)`, and apply defaults after profile resolution. Alternatively, only pass boolean fields to `resolveTimingProfile` when the user explicitly provided them (track which fields were user-supplied vs zod-defaulted). The simplest fix:
```typescript
// In start_capture inputSchema, change:
delta_highlight: z.boolean().default(false)  // BEFORE
delta_highlight: z.boolean().optional()       // AFTER
// Same for compress_idle and gif_export

// Then after all profile resolution, apply defaults:
resolvedArgs.delta_highlight ??= false;
resolvedArgs.compress_idle ??= false;
resolvedArgs.gif_export ??= false;
```

Note: The same issue applies to `jpeg_quality` (`.default(80)`) and `max_frames` (`.default(20)`) -- a timing profile setting `maxFrames: 6` (like `quick-glance`) would be overridden by the zod default of 20. These numeric defaults should also be changed to `.optional()` with post-resolution defaulting.

## Warnings

### WR-01: Screenshot profile merge discards timing profile resolution

**File:** `src/server.ts:238`
**Issue:** When both `timing_profile` and `screenshot_profile` are provided, line 202 correctly merges timing into `resolvedArgs`. But line 238 does `resolvedArgs = { ...args, ...resolved }` spreading the original `args` object (not `resolvedArgs`), which discards the timing profile values merged at line 202.

**Fix:** Change line 238 to spread `resolvedArgs` instead of `args`:
```typescript
// Line 238 BEFORE:
resolvedArgs = { ...args, ...resolved };

// AFTER:
resolvedArgs = { ...resolvedArgs, ...resolved };
```

### WR-02: `interval_ms` is required in zod schema, preventing timing-profile-only usage

**File:** `src/server.ts:50-52`
**Issue:** `interval_ms` is defined as a required field (no `.optional()` or `.default()`). When an agent wants to use a timing profile that supplies `intervalMs` (e.g., `quick-glance` with `intervalMs: 500`), the MCP SDK will reject the call if `interval_ms` is not also explicitly passed. This undermines the purpose of timing profiles for reducing parameter boilerplate.

**Fix:** Make `interval_ms` optional and validate it is present (from inline or profile) after resolution:
```typescript
// Schema change:
interval_ms: z.number().min(100).optional()
  .describe("Milliseconds between captures (required unless timing_profile provides it)"),

// After profile resolution, validate:
if (resolvedArgs.interval_ms === undefined) {
  return {
    content: [{ type: "text" as const, text: JSON.stringify({
      status: "error",
      error: "interval_ms is required (provide directly or via timing_profile)",
    })}],
    isError: true,
  };
}
```

## Info

### IN-01: Non-fractional time display assumption in `generateParameterSummary`

**File:** `src/profiles/timing-presets.ts:77-88`
**Issue:** The summary formatter divides `intervalMs` by 1000 and `durationMs` by 60000 without rounding. Values like `intervalMs: 1500` produce `"every 1.5s"` (fine), but `intervalMs: 1100` produces `"every 1.1s"` and `durationMs: 90000` produces `"for 1.5min"`. These are acceptable but could be confusing with less round values. Consider rounding to 1 decimal place for cleaner output.

**Fix:** Optional improvement -- add rounding for non-integer results:
```typescript
const seconds = profile.intervalMs / 1000;
const display = Number.isInteger(seconds) ? `${seconds}` : `${Math.round(seconds * 10) / 10}`;
```

---

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