diff --git a/.planning/phases/01-stabilize-video-pipeline/01-REVIEW.md b/.planning/phases/01-stabilize-video-pipeline/01-REVIEW.md new file mode 100644 index 0000000..9273316 --- /dev/null +++ b/.planning/phases/01-stabilize-video-pipeline/01-REVIEW.md @@ -0,0 +1,393 @@ +--- +phase: 01-stabilize-video-pipeline +reviewed: 2026-05-15T22:30:00Z +depth: deep +files_reviewed: 19 +files_reviewed_list: + - manifest.json + - package.json + - smoke.sh + - src/background/index.ts + - src/offscreen/index.html + - src/offscreen/recorder.ts + - src/shared/binary.ts + - src/shared/logger.ts + - src/shared/types.ts + - tests/offscreen/codec-check.test.ts + - tests/offscreen/handshake.test.ts + - tests/offscreen/port-serialization.test.ts + - tests/offscreen/port.test.ts + - tests/offscreen/ring-buffer.test.ts + - tests/offscreen/segment-keyframes.test.ts + - tests/offscreen/segment-rotation.test.ts + - tests/offscreen/webm-playback.test.ts + - vite.config.ts + - vitest.config.ts +findings: + critical: 3 + warning: 9 + info: 6 + total: 18 +status: issues_found +--- + +# Phase 1: Code Review Report + +**Reviewed:** 2026-05-15T22:30:00Z +**Depth:** deep +**Files Reviewed:** 19 +**Status:** issues_found + +## Summary + +Phase 1 delivers the video pipeline stabilization: D-12 (base64 wire format) and D-13 (restart-segments lifecycle) both landed cleanly. The TDD discipline is visible in the test artifacts (port-serialization, segment-keyframes, webm-playback have explicit RED-then-GREEN structure). Type safety in the Phase 1 src/ files is clean — zero `as any` and zero `@ts-ignore` in `src/background/index.ts`, `src/offscreen/recorder.ts`, `src/shared/*.ts`. Storage constraint CON-buffer-storage is intact: only `indexedDB.deleteDatabase('VideoRecorderDB')` appears (idempotent cleanup), no writes to `chrome.storage`/IDB/localStorage from the new pipeline. + +However, deep cross-file analysis surfaces **three Critical correctness defects**: + +1. A **port reconnect race** in `src/offscreen/recorder.ts` that drops in-flight `REQUEST_BUFFER` responses to a stale port reference (data-loss path). +2. A **port-keepalive ineffectiveness bug** on the SW side — the SW never registers an `onMessage` listener on the port, so Chrome may not credit the PING traffic toward idle-timer reset under all conditions, and the `BUFFER` response path coexists with a SW-registered listener that conflicts. +3. A **handshake race window** that re-opens after a Service Worker respawn: the OFFSCREEN_READY promise is resolved at most once, but the SW may respawn while the offscreen document persists — the second SW lifetime can no longer await readiness and will deadlock on `offscreenReady`. + +Plus 9 Warnings (lifecycle correctness gaps, smoke-script bash hygiene, codec-check side effect, base64 encode/decode performance and edge cases) and 6 Info-level items (style divergence, pre-existing P5-deferred audit items, dead-code branches). + +## Critical Issues + +### CR-01: Port reconnect race drops BUFFER response under reconnect-during-encode + +**File:** `src/offscreen/recorder.ts:335-388` (combined with `src/background/index.ts:111-154`) +**Issue:** +`encodeAndSendBuffer()` reads `keepalivePort` after an `await Promise.all(...)`. If the port disconnects during the `~tens-of-ms` base64 encode of three 10s segments, the `onDisconnect` handler nulls `keepalivePort` AND synchronously calls `connectPort()`, which **reassigns `keepalivePort` to a fresh port instance**. The post-await check `if (keepalivePort === null) { ... drop ... }` therefore evaluates `false` on the reconnected case, and the BUFFER response is posted on the **new** port — but the SW-side `getVideoBufferFromOffscreen` is still listening on the **old** port (captured in `const port = videoPort`, line 110). Result: silent timeout (BUFFER_FETCH_TIMEOUT_MS = 2_000), SAVE_ARCHIVE produces a zip with `segments: []`. This is a data-loss path that masquerades as a benign timeout in logs. + +Concretely the race is: SW sends REQUEST_BUFFER → offscreen begins encode → port hits its 290 s pre-emptive disconnect → offscreen receives BUFFER on new port → SW timed out and gave up on old port → ZIP has no video. + +**Fix:** +Capture the port reference *before* the await and post on that captured handle (or refuse to post if it's no longer the live `keepalivePort`): + +```ts +async function encodeAndSendBuffer(): Promise { + const portAtRequest = keepalivePort; // capture identity + if (portAtRequest === null) return; + // ... build allSegments / baseTimestamp ... + const encodeResults = await Promise.all(/* ... */); + // After await: refuse to post on a new port that the SW isn't listening on. + if (keepalivePort !== portAtRequest) { + logger.warn('port replaced during encode; dropping BUFFER response'); + return; + } + portAtRequest.postMessage({ type: 'BUFFER', segments: transferred }); +} +``` +The SW side already times out cleanly (2 s budget); the correct fix is to let the SW time out so the next save attempt re-issues REQUEST_BUFFER on the new port. Don't post stale data onto a stranger port. + +--- + +### CR-02: SW never registers `onMessage` on the incoming port — BUFFER response can race a torn-down listener and PING is silently discarded + +**File:** `src/background/index.ts:78-96` +**Issue:** +The SW-side `chrome.runtime.onConnect` handler captures `videoPort = port` and attaches only `onDisconnect`. The `port.onMessage.addListener(handler)` for BUFFER is added **per-request** inside `getVideoBufferFromOffscreen()` (line 152), then removed on timeout/response. Two consequences: + +1. **PING traffic is discarded**: When the per-request handler is *not* installed (which is the normal idle state of the port), PINGs from offscreen have no listener. Per the Chrome 110+ keepalive guarantee, the SW idle-timer reset triggers on inbound port traffic regardless of whether a listener is attached — but in practice "no listener" combined with the message-not-being-consumed has caused subtle behaviour differences in the field. The comment at line 95 ("Inbound traffic is mostly PING (ignored) ...") acknowledges this but doesn't actually attach a sink. Either install a no-op `onMessage` listener at port-acceptance time (preferred), or document explicitly that PING relies on the kernel-level port-message side effect. +2. **Race window on BUFFER**: If the offscreen recorder happens to send a BUFFER for ANY reason (e.g., a stale `REQUEST_BUFFER` from a previous SW lifetime that's now being processed) before `getVideoBufferFromOffscreen` registers its per-call handler, the message is dropped. The current architecture treats this as fine — but it means the SW depends on strict ping-pong ordering with no transport-level guarantee. + +**Fix:** +Add a permanent port-message router at acceptance time and dispatch BUFFER to subscribers: + +```ts +chrome.runtime.onConnect.addListener((port) => { + if (port.name !== 'video-keepalive') return; + if (port.sender?.id !== chrome.runtime.id) { + logger.warn('Rejecting port', port.sender?.id); + port.disconnect(); + return; + } + videoPort = port; + port.onMessage.addListener((msg) => { + if (typeof msg === 'object' && msg !== null + && (msg as {type?: unknown}).type === 'PING') { + return; // explicit sink — keeps SW alive even with no pending fetch + } + // BUFFER routing handled by per-request listener; unknown traffic dropped. + }); + port.onDisconnect.addListener(() => { videoPort = null; }); +}); +``` + +--- + +### CR-03: Handshake deadlock after Service Worker respawn + +**File:** `src/background/index.ts:32-35, 158-202, 487-491` +**Issue:** +`offscreenReady` is a one-shot promise resolved on the first `OFFSCREEN_READY` message (line 489 nulls the resolver). If the Service Worker is evicted (Chrome MV3 ~30 s idle) while the offscreen document is still alive, the next SW lifetime will: + +1. Create a fresh `offscreenReady` Promise at module load (line 33). +2. Detect the existing offscreen via `chrome.offscreen.hasDocument()` (line 516) — `offscreenCreated = true`. +3. The next REQUEST_PERMISSIONS triggers `startVideoCapture()` which awaits `offscreenReady` on line 180. +4. But the offscreen document has **already sent its OFFSCREEN_READY exactly once** (line 452 in recorder.ts) at its original bootstrap — it does NOT re-emit on SW respawn. +5. `offscreenReady` therefore hangs **forever**, and `startVideoCapture()` never returns, until the user closes Chrome. + +This is masked in dev because each fresh dev-load tears down everything together. The audit P1 #8 mitigation at lines 514-524 handles the offscreen-exists case for `ensureOffscreen()` but not the readiness handshake. + +**Fix:** +Either (a) have offscreen re-emit OFFSCREEN_READY periodically OR on inbound SW message, OR (b) on SW init, if `chrome.offscreen.hasDocument()` returns true, immediately resolve `offscreenReady` (the offscreen has already been ready in a prior life): + +```ts +async function initialize() { + // ... + if (typeof chrome.offscreen?.hasDocument === 'function') { + const exists = await chrome.offscreen.hasDocument(); + if (exists) { + offscreenCreated = true; + // Offscreen already bootstrapped in a prior SW lifetime — don't wait + // for an OFFSCREEN_READY that will never come. + offscreenReadyResolve?.(); + offscreenReadyResolve = null; + logger.log('Existing offscreen — resolving readiness immediately'); + } + } +} +``` +Option (b) is simpler and sufficient. Add a regression test that imports `src/background/index.ts` with a `chrome.offscreen.hasDocument` stub returning `true` and asserts that `offscreenReady` resolves without needing an OFFSCREEN_READY message. + +## Warnings + +### WR-01: `getDisplayMedia` rejection from user-cancel surfaces as RECORDING_ERROR with raw DOMException message + +**File:** `src/offscreen/recorder.ts:146-151` +**Issue:** +When the operator dismisses the picker, Chrome rejects `getDisplayMedia` with `NotAllowedError`. The catch block at line 146 forwards the raw `error.message` (e.g., "Permission denied by user") into a runtime broadcast. Per CLAUDE.md (Python guidelines also apply philosophically: "Sanitize error messages: log details server-side, return generic messages to clients"), the runtime message should be a stable, programmatically actionable code; raw browser error strings change between Chrome versions and locales. + +**Fix:** +Classify the failure mode (`'user-cancelled'`, `'codec-unsupported'`, `'permission-denied'`, `'unknown'`) and emit a code; log the raw string at warn level only: + +```ts +} catch (error) { + const code = classifyDisplayMediaError(error); // → 'user-cancelled' | ... + logger.error('startRecording failed:', error); + chrome.runtime.sendMessage({ type: 'RECORDING_ERROR', error: code }); + throw error; +} +``` + +--- + +### WR-02: `assertCodecSupported()` has a side-effect on a side-effect (`chrome.runtime.sendMessage` from a pure-named function) + +**File:** `src/offscreen/recorder.ts:102-113` +**Issue:** +A function named `assertCodecSupported` reads as a pure predicate that throws. It actually broadcasts a `RECORDING_ERROR` over `chrome.runtime.sendMessage` before throwing. This couples the assertion to extension runtime, breaks single responsibility, and forces the test (codec-check.test.ts) to stub `chrome.runtime.sendMessage` even when only the assertion logic is under test. Also, `startRecording()` then **re-emits** `RECORDING_ERROR` for the same failure in its catch block (line 149), so the popup receives **two** RECORDING_ERROR messages for one underlying problem. + +**Fix:** +Either rename to `assertCodecSupportedAndNotify`, or remove the sendMessage from the assertion and rely on the caller's catch block. Recommended: extract notification to the caller. + +```ts +export function assertCodecSupported(): void { + if (!isCodecSupported()) { + throw new Error(`vp9 unsupported. UA=${navigator?.userAgent ?? ''}`); + } +} +// startRecording handles the notify-on-throw via its existing catch. +``` +This also fixes the double-emit. Update `codec-check.test.ts` GREEN block accordingly. + +--- + +### WR-03: `mergeVideoSegments` sort uses non-stable timestamp resolution; equal timestamps may reorder + +**File:** `src/background/index.ts:254` + `src/offscreen/recorder.ts:352, 366` +**Issue:** +The offscreen recorder uses `baseTimestamp + idx` (`Date.now() + idx`) to tag segments — millisecond resolution + index offset. Two consecutive REQUEST_BUFFER calls within the same millisecond would both produce timestamps starting at the same `Date.now()`. While the merging code now operates on a single REQUEST_BUFFER batch (so collisions don't happen in practice), the contract is fragile: a future change that calls `encodeAndSendBuffer` twice (e.g., diagnostic prefetch + actual save) would produce duplicate timestamps. `Array.prototype.sort` is stable per ES2019 spec, so the in-batch order is preserved, but cross-batch order is not guaranteed by anything visible. Document this or move to a strictly monotonic counter. + +**Fix:** +Replace `Date.now() + idx` with a module-level monotonic counter: + +```ts +let segmentSeq = 0; +// ... +return { data, type: segment.type || 'video/webm', timestamp: ++segmentSeq }; +``` + +--- + +### WR-04: Smoke script — `set -euo pipefail` interacts badly with `read -r -d ''` and `grep ... || true` patterns + +**File:** `smoke.sh:92, 109, 161, 197` +**Issue:** +Several locations rely on `|| true` to swallow non-zero exits, which is correct under Google's shell style but fragile when piping. Specifically: +- Line 109: `python3 -c '...' 2>/dev/null || printf '%s' "${SMOKE_HTML}"` — the fallback path emits **unencoded** HTML into a `data:` URL. If python3 is missing, the URL will contain literal `<`, `>`, spaces, and quotes — Chrome may fail to parse the URL silently, and the operator will see a blank tab. +- Line 161: `find ... | sort -rn | head -1 | cut -d' ' -f2- || true` — `|| true` on the last segment of a pipe under `pipefail` masks errors mid-pipe. +- Line 197: `ffprobe ... && GATE=0 || GATE=$?` — this is the classic anti-pattern: `GATE=0` is itself a command that returns 0, so `|| GATE=$?` is unreachable. `GATE` is always 0 here, even on ffprobe failure. **This means the acceptance gate is broken** — but only structurally: the script proceeds to `if [[ ${GATE} -eq 0 ]]` and prints "PASSED" regardless of ffprobe. + +Wait — let me re-read. `ffprobe ... && GATE=0 || GATE=$?` — if ffprobe exits 0, then `&&` runs `GATE=0` which returns 0, so `||` short-circuits. If ffprobe exits non-zero, `&&` is skipped, so `||` runs `GATE=$?`. But `$?` here is the exit code of the `&&` chain, which is ffprobe's exit. **Actually correct** because the `&&` short-circuits. False alarm on this one. + +The python3 fallback is a real defect though. + +**Fix:** +1. Require python3 explicitly in pre-flight: +```bash +command -v python3 >/dev/null || { red "FAIL: python3 not installed (needed for URL encoding)"; exit 1; } +``` +Then remove the `|| printf '%s' "${SMOKE_HTML}"` fallback. +2. Consider using `pipefail` semantics consistently; the `|| true` at line 161 should be inside the subshell, not after the pipe. + +--- + +### WR-05: Smoke script writes operator's `~/Downloads` directory and globs all `session_report_*.zip` — risk of false-positive detection + +**File:** `smoke.sh:40, 74, 158, 161, 174-180` +**Issue:** +The script polls `~/Downloads` for *any* file matching `session_report_*.zip`. If the operator was running the extension prior to the smoke test, or is running it in another browser window, an unrelated session_report could trigger the polling success and the script will ffprobe **the wrong file**. The `BEFORE_LIST` count (line 74) mitigates this for *count* but not for *identity* — if one pre-existing zip and one new zip both exist, `LATEST` (line 161) picks by mtime, which is correct, but the count comparison `NEW_COUNT > BEFORE_LIST` is satisfied by any operation that adds a matching zip. + +**Fix:** +Snapshot the *full list* before, diff *after*: +```bash +BEFORE_ZIPS=$(find "${DOWNLOADS_DIR}" -maxdepth 1 -name 'session_report_*.zip' -printf '%p\n' | sort) +# ... later ... +AFTER_ZIPS=$(find ...) +NEW_ZIP=$(comm -13 <(printf '%s\n' "${BEFORE_ZIPS}") <(printf '%s\n' "${AFTER_ZIPS}") | head -1) +``` +Additionally consider redirecting the smoke download to `/tmp/mokosh-smoke-downloads` via `chrome.downloads` API or a custom `--download-directory` flag — but that requires Chrome side changes. + +--- + +### WR-06: `blobToBase64` ⇒ O(n²) string concatenation for large segments + +**File:** `src/shared/binary.ts:48-52` +**Issue:** +Per-byte `binary += String.fromCharCode(bytes[i])` for a ~1.5 MB segment produces ~1.5M string concatenations. V8 optimizes ConsString chains, but the worst case is allocation-heavy and pauses the event loop visibly. The phase context notes "~1.5 MB segments" — that's ~1.5M iterations per `Promise.all` call (3 segments × ~1.5MB = ~4.5M iterations on each REQUEST_BUFFER). The 2-second SW timeout (BUFFER_FETCH_TIMEOUT_MS) leaves headroom in benchmarks today, but this is one of the dimensions that grows with future quality bumps (e.g., raising VIDEO_BITRATE or SEGMENT_DURATION_MS). + +This is **out of v1 review scope (performance)** per ``, BUT it ALSO has a correctness edge: `String.fromCharCode(...subarray)` via `.apply()` blows the argument-length limit (~64 KiB) on some engines. The current per-byte loop avoids that. So the comment at lines 45-47 is correct — the trade-off is intentional. Documenting this is fine; no behavioural fix required for v1. Flagged for awareness if Phase 2 quality model includes higher bitrate. + +**Fix:** +None needed for v1. For Phase 2, consider chunked apply with 32 KiB stride: +```ts +const CHUNK = 0x8000; +const parts: string[] = []; +for (let i = 0; i < bytes.length; i += CHUNK) { + parts.push(String.fromCharCode.apply(null, bytes.subarray(i, i + CHUNK) as any)); +} +return btoa(parts.join('')); +``` + +--- + +### WR-07: `base64ToBlob` does not validate input — malformed base64 throws cryptic `InvalidCharacterError` + +**File:** `src/shared/binary.ts:67-74` +**Issue:** +If a malformed base64 string slips through (e.g., a port message corrupted in transit, though TLS-equivalent inside Chrome makes this near-impossible), `atob(b64)` throws `InvalidCharacterError` from the SW caller. The caller (`src/background/index.ts:141-147`) does have a try/catch, so a segment is dropped on failure — fine. But the `b64` parameter has no input validation: empty string `atob('') === ''` returns an empty Blob silently. Empty Blob has size 0, but currently no caller filters zero-size segments out of the WebM concat result, which would produce a slightly malformed multi-EBML file. + +**Fix:** +Add a defensive shortcut for empty input: +```ts +export function base64ToBlob(b64: string, mimeType: string): Blob { + if (b64.length === 0) { + return new Blob([], { type: mimeType }); // explicit, not via atob('') + } + // ... +} +``` +And in `getVideoBufferFromOffscreen` filter out empty segments before pushing to `mergeVideoSegments`. + +--- + +### WR-08: `downloadArchive` builds a Data URL with `btoa(per-byte binary string)` — re-implements the same pattern with the same potential issue + +**File:** `src/background/index.ts:350-358` +**Issue:** +This duplicates the body of `blobToBase64` inline instead of importing it. Same O(n²) concern as WR-06, applied to the **entire** zip (which is the SUM of all segments, ~5-10 MB after JSZip compression on three ~1.5MB segments). At 10 MB this is on the edge — ~10M iterations of string concatenation. **More importantly: code duplication.** The `src/shared/binary.ts` helper exists; this caller should use it. + +**Fix:** +```ts +import { blobToBase64 } from '../shared/binary'; +// ... +const base64 = await blobToBase64(archiveBlob); +const url = `data:application/zip;base64,${base64}`; +``` + +--- + +### WR-09: `mergeVideoSegments` accepts `VideoSegment[]` but offscreen-side `encodeAndSendBuffer` may inject an *unfinalized* in-flight segment + +**File:** `src/offscreen/recorder.ts:340-348` and `src/background/index.ts:248-274` +**Issue:** +`encodeAndSendBuffer` includes the currently-recording chunks as an additional segment (lines 342-347). This in-flight Blob lacks the WebM-finalization that `MediaRecorder.stop()` performs (Matroska SegmentSize, Cues). The phase context (debug session webm-playback-freeze) explicitly notes that this finalization is what fixes the "File ended prematurely" ffmpeg error. So including an unfinalized in-flight segment **could re-introduce the prematurely-ended-file symptom** for the LAST segment in the merged file. + +The trade-off comment at line 340 acknowledges this exists to avoid an empty-buffer UX issue if SAVE_ARCHIVE fires within 10 s of START_RECORDING. But the contract is: Chrome's multi-EBML-header concat works because each segment is *self-contained*. An unfinalized segment is **not** self-contained. + +**Fix (options):** +(a) Exclude the in-flight segment unless `segments.length === 0` (preserve UX for first-10-s case, otherwise rely on finalized data only). +(b) Force a synchronous rotation at REQUEST_BUFFER time: call `rotateSegment()`, await its onstop, then encode `getSegments()`. The added latency is bounded. + +Option (a) recommended for least change: +```ts +const allSegments: Blob[] = + finalized.length === 0 && inFlight !== null + ? [inFlight] + : finalized; +``` + +## Info + +### IN-01: `extensionVersion: '1.0.0'` hardcoded in metadata — pre-existing P5-deferred audit item + +**File:** `src/background/index.ts:324` + `manifest.json:4` +**Issue:** +Per Phase 1 context note (9), this is a known pre-existing audit item deferred to Phase 5. Surfaced here for tracking continuity. The hardcoded `'1.0.0'` will silently lie about the running extension version once `manifest.json` bumps. Currently both are `1.0.0` so the bug is dormant. +**Fix (Phase 5):** `extensionVersion: chrome.runtime.getManifest().version` + +--- + +### IN-02: `Logger` and `ContentLogger` use `...args: any[]` while `OffscreenLogger` uses `...args: unknown[]` + +**File:** `src/shared/logger.ts:9, 14-22, 35, 40-50, 63, 68-78` +**Issue:** +Style divergence is explicitly called out in the comment at lines 54-55 — referred to as plan 01-03's `style_divergence_note`. Not actionable for Phase 1; flag for Phase 5 cleanup. The risk: a wholesale ts-strict tightening pass will flag `Logger` / `ContentLogger` and not `OffscreenLogger`, increasing review noise. + +**Fix (Phase 5):** Migrate `Logger` and `ContentLogger` to `...args: unknown[]` once the consumers' call sites are audited for type safety. + +--- + +### IN-03: `tests/offscreen/ring-buffer.test.ts` is a vestigial 30-line breadcrumb — consider deleting + +**File:** `tests/offscreen/ring-buffer.test.ts:1-29` +**Issue:** +The file is intentionally kept as a "breadcrumb" pointing readers to `segment-rotation.test.ts`. This is fine for one phase but accumulates archaeology over time. A `git mv` rename in the same commit (with git's rename detection) would preserve the breadcrumb in `git log --follow` without leaving a 30-line tombstone in the working tree. + +**Fix (optional):** Delete the file; rely on `git log -- tests/offscreen/segment-rotation.test.ts` or a `MIGRATIONS.md` doc to surface the rename. + +--- + +### IN-04: `tests/offscreen/webm-playback.test.ts` defines `decodeDryRun` but never calls it; uses `void decodeDryRun` to silence noUnusedLocals + +**File:** `tests/offscreen/webm-playback.test.ts:65-105, 172-178` +**Issue:** +`decodeDryRun` (using `execFileSync`) is kept "for documentation value" but never executed. This is dead code in production-test files. The `void decodeDryRun` pattern at line 178 is a tsc-appeasement hack. The comment block at lines 172-176 articulates the intent, but the cleaner answer is to extract the comparison into a JSDoc block, delete the unused function, and keep only `decodeDryRunStrict`. + +**Fix:** Delete `decodeDryRun` and the `void` suppression. + +--- + +### IN-05: `Message` interface uses `any` type parameter default + +**File:** `src/shared/types.ts:18-22` +**Issue:** +`export interface Message { type: MessageType; data?: T; tabId?: number; }` — the `any` default leaks through everywhere a `Message` is referenced without an explicit type argument (most call sites do). Recommend `unknown` default with mandatory narrowing at callers. +**Fix (Phase 5):** `Message`. Audit downstream `(msg as Message).data` accesses for explicit narrowing. + +--- + +### IN-06: Manifest permissions — `activeTab` + `` host_permissions is broader than strictly required for `getDisplayMedia` flow + +**File:** `manifest.json:6-15` +**Issue:** +After the D-01 amendment (chrome.tabCapture → getDisplayMedia), the `activeTab` permission is no longer used by the video path. It remains in use by `chrome.tabs.captureVisibleTab` (line 228 of background/index.ts) for the screenshot. That's legitimate. However `host_permissions: [""]` is required only by the content script (rrweb injection). This is documented sufficient-but-broad in CONTEXT.md. + +No defect — flagged for principle-of-least-privilege awareness for any future scoping work. + +--- + +_Reviewed: 2026-05-15T22:30:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: deep_