---
phase: 11-frame-repository-and-subset-grid-compilation
reviewed: 2026-04-13T00:00:00Z
depth: standard
files_reviewed: 5
files_reviewed_list:
  - src/repository/repository-types.ts
  - src/repository/frame-query.ts
  - src/repository/repository-manager.ts
  - src/repository/repository-scheduler.ts
  - src/server.ts
findings:
  critical: 1
  warning: 4
  info: 2
  total: 7
status: issues_found
---

# Phase 11: Code Review Report

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

## Summary

The frame repository system introduces disk-backed capture sessions with query-based frame selection and subset grid compilation. The architecture is solid: clean separation between types, query logic, persistence, and scheduling. Path traversal defenses (UUID validation, session name sanitization) are well-implemented. The main concerns are an unhandled promise rejection in the scheduler, a silent data-loss pattern in `detectQueryMode`, error swallowing in `persistManifest`, and missing `validateSessionId` on a public method.

## Critical Issues

### CR-01: Unhandled promise rejection if scheduler tick throws after resolve

**File:** `src/repository/repository-scheduler.ts:149`
**Issue:** `scheduleTick` is an async function called via `setTimeout(scheduleTick, delay)`. If the `await repositoryManager.completeSession()` call on lines 93 or 137 throws, or if `captureFrame()` throws an error that is not an `Error` instance (bypassing the catch), the rejected promise from `scheduleTick()` is never caught. `setTimeout` discards the return value of an async function, so any rejection becomes an unhandled promise rejection that can crash the Node.js process (depending on `--unhandled-rejections` mode).
**Fix:** Wrap the entire `scheduleTick` body in a try/catch, or chain `.catch()` on the setTimeout callback:
```typescript
setTimeout(() => {
  scheduleTick().catch((err) => {
    logger.error(`Scheduler tick failed: ${err instanceof Error ? err.message : String(err)}`);
    repositoryManager.errorSession(manifest.sessionId, err instanceof Error ? err.message : String(err))
      .catch(() => {});
    resolve();
  });
}, delay);
```
The same pattern applies to the initial call on line 153 (`scheduleTick()` without `.catch()`).

## Warnings

### WR-01: detectQueryMode silently ignores partial params, returning "all" mode

**File:** `src/repository/frame-query.ts:51-53`
**Issue:** When a user provides only `to_ms` without `from_ms`, or only `to_index` without `from_index`, the function falls through to `{ mode: "all" }` and silently ignores the user's filter intent. The comment on line 51 acknowledges this ("treat as 'all' with those ignored") but this is a data correctness issue: a user who passes `to_ms: 5000` expects only frames up to 5 seconds, but gets all frames instead.
**Fix:** Either reject partial params as ambiguous errors, or treat missing `from_ms`/`from_index` as 0:
```typescript
// Option A: reject
if (hasToMs && !hasFromMs) {
  return { error: "to_ms requires from_ms" };
}
if (hasToIndex && !hasFromIndex) {
  return { error: "to_index requires from_index" };
}

// Option B: default from to 0
if (hasToMs) {
  return { mode: "time_range", from_ms: params.from_ms ?? 0, to_ms: params.to_ms! };
}
```

### WR-02: persistManifest silently swallows write errors

**File:** `src/repository/repository-manager.ts:328-344`
**Issue:** The `persistManifest` method chains onto `persistQueue` using `.then()`, but errors from `writeFile` inside the chained function are not caught. If `writeFile` throws (e.g., disk full, permissions), the `persistQueue` promise chain breaks -- subsequent calls to `persistManifest` will never execute because the chain is rejected and `.then()` on a rejected promise skips the callback. This silently stops all future manifest writes.
**Fix:** Add error handling inside the queue chain:
```typescript
private async persistManifest(sessionId: string): Promise<void> {
  this.persistQueue = this.persistQueue.then(async () => {
    const manifest = this.sessions?.get(sessionId);
    if (!manifest) return;
    const manifestPath = path.join(this.basePath, sessionId, "manifest.json");
    await writeFile(manifestPath, JSON.stringify(manifest, null, 2), "utf-8");
  }).catch((err) => {
    logger.error(`Failed to persist manifest for ${sessionId}: ${err instanceof Error ? err.message : String(err)}`);
  });
  return this.persistQueue;
}
```

### WR-03: getSession does not validate sessionId, bypassing path traversal guard

**File:** `src/repository/repository-manager.ts:216-221`
**Issue:** The `getSession` method does a direct `Map.get()` without calling `validateSessionId()`. While the map lookup itself is safe (it won't find a non-UUID key if only UUIDs are stored), this breaks defense-in-depth. Other methods like `appendFrame`, `completeSession`, `errorSession`, and `deleteSession` all call `validateSessionId` first. If `getSession` is ever used to construct a file path downstream (as `get_capture_status` on server.ts line 477 does -- though it then only reads manifest data, not files), the missing validation creates a gap.
**Fix:**
```typescript
async getSession(sessionId: string): Promise<RepositoryManifest | undefined> {
  await this.ensureLoaded();
  validateSessionId(sessionId);
  return this.sessions!.get(sessionId);
}
```

### WR-04: Scheduler skipped-frame mutation not persisted to disk

**File:** `src/repository/repository-scheduler.ts:123-128`
**Issue:** When a frame capture fails, the scheduler pushes to `manifest.skippedFrames` directly but never calls `repositoryManager.persistManifest()` (or any persist method). The skipped frame data exists only in memory. If the process crashes, all skipped frame records are lost. The `completeSession` call at the end persists the final state, but if the session errors out due to consecutive failures (line 103-110), `errorSession` is called which persists the manifest -- so skipped frames are preserved in that path. However, for sessions that complete normally, skipped frames from earlier in the session are at risk if the process crashes mid-capture.
**Fix:** Call `persistManifest` (or a dedicated method) after recording skipped frames, or persist during the next successful `appendFrame` call by ensuring the manifest includes the updated `skippedFrames` array (which it does since it's the same object reference -- so this is actually persisted on the next successful frame write). This is lower severity since the manifest object is shared by reference, but the gap exists between skip and next successful write.

## Info

### IN-01: Substantial code duplication between start_capture and start_repository_capture

**File:** `src/server.ts:1276-1489`
**Issue:** The `start_repository_capture` tool handler duplicates approximately 150 lines of profile resolution, target validation, window lookup, and target factory logic from `start_capture` (lines 170-452). Any bug fix or feature addition to one must be mirrored in the other.
**Fix:** Extract the shared logic (profile resolution, target validation, target factory) into a helper function like `resolveAndBuildTarget(args)` that both handlers call.

### IN-02: changeSummary missing framesWithChanges field

**File:** `src/server.ts:1719-1724`
**Issue:** The `ChangeSummary` interface in `repository-types.ts:59` declares a `framesWithChanges` field, but the `changeSummary` object constructed in `list_repository_frames` (line 1719) does not include it. This is a minor inconsistency -- the interface is not actually used as a type annotation for this object, so there is no compile error, but consumers expecting the documented shape will not find the field.
**Fix:** Either add `framesWithChanges` to the constructed object (requires pixel comparison which may be expensive) or remove it from the `ChangeSummary` interface if it is not intended for timing-only summaries.

---

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