diff --git a/.planning/phases/01-stabilize-video-pipeline/01-REVIEW-FIX.md b/.planning/phases/01-stabilize-video-pipeline/01-REVIEW-FIX.md index da09dfe..a2be708 100644 --- a/.planning/phases/01-stabilize-video-pipeline/01-REVIEW-FIX.md +++ b/.planning/phases/01-stabilize-video-pipeline/01-REVIEW-FIX.md @@ -1,182 +1,37 @@ --- phase: 01-stabilize-video-pipeline -fixed_at: 2026-05-16T07:25:00Z +fixed_at: 2026-05-16T11:05:00Z review_source: .planning/phases/01-stabilize-video-pipeline/01-REVIEW.md -iteration: 1 +iteration: 2 findings_in_scope: 18 -fixed: 5 -skipped: 0 -remaining: 13 -status: partial -checkpoint: true -checkpoint_reason: context_budget_exhausted_after_critical_cluster +fixed: 16 +skipped: 2 +status: all_fixed +prior_partial_commit: 2e3f524 commit_range: 2e3f524..HEAD --- -# Phase 1: Code Review Fix Report (PARTIAL — CHECKPOINT) +# Phase 1: Code Review Fix Report (FINAL) -**Fixed at:** 2026-05-16T07:25:00Z +**Fixed at:** 2026-05-16T11:05:00Z **Source review:** [01-REVIEW.md](./01-REVIEW.md) -**Iteration:** 1 -**Status:** partial — CHECKPOINT for orchestrator continuation +**Iteration:** 2 (continuation of partial iteration 1) +**Status:** all_fixed ## Summary -- Findings in scope: 18 (3 Critical + 9 Warning + 6 Info) -- Fixed this pass: **5** (3 Critical + 2 Warning, all delivered as one - cohesive commit because they share the `encodeAndSendBuffer` code path) -- Skipped: 0 -- Remaining: **13** (7 Warning + 6 Info) — see "Remaining Work" below -- Sweep items added: 0 (no spare context budget for the defensive sweep - on this pass — see "Sweep Status" below) +- Findings in scope: **18** (3 Critical + 9 Warning + 6 Info) +- Fixed: **16** +- Skipped (pre-approved documented skips, NOT context cuts): **2** (WR-06 + IN-06) +- Sweep targets addressed: **8** (1, 2, 3, 4, 5, 7 active; 6 covered by tests added with WR-07; 8 no-op after inspection) -**Commits this pass:** - -| Hash | Subject | -|-----------|-------------------------------------------------------------------------------| -| `2e3f524` | fix(01-review): CR-01+CR-02+CR-03+WR-03+WR-09 critical port + handshake race | - -## Fixed Issues - -### CR-01: Port reconnect race drops BUFFER response under reconnect-during-encode - -**Files modified:** `src/offscreen/recorder.ts` -**Commit:** `2e3f524` -**Applied fix:** Captured `portAtRequest = keepalivePort` before the await in -`encodeAndSendBuffer`. After `blobToBase64` completes, refuse to post if -`keepalivePort !== portAtRequest` (i.e. a reconnect replaced the port mid-encode). -The SW already times out cleanly after 2 s, so the next SAVE_ARCHIVE -re-issues REQUEST_BUFFER on the fresh port — stale data never leaks to a -stranger port. - -### CR-02: SW never registers `onMessage` on the incoming port — PING silently discarded - -**Files modified:** `src/background/index.ts` -**Commit:** `2e3f524` -**Applied fix:** Added a permanent `port.onMessage.addListener` in the SW -`onConnect` handler that explicitly drains PING and silently drops unknown -traffic. The per-request BUFFER listener installed by `getVideoBufferFromOffscreen` -still wins (it's added later in the listener chain when SAVE_ARCHIVE fires). -This guarantees the SW idle-timer reset is consumed by a real handler under -all Chrome 110+ behaviour variants. - -### CR-03: Handshake deadlock after Service Worker respawn - -**Files modified:** `src/background/index.ts` -**Commit:** `2e3f524` -**Applied fix:** When `chrome.offscreen.hasDocument()` returns true on SW init, -immediately call `offscreenReadyResolve?.()` and null the resolver. The -offscreen MUST have completed its bootstrap before becoming observable via -`hasDocument()` — waiting for an OFFSCREEN_READY that will never come is the -deadlock. This is REVIEW.md option (b) (resolve-on-hasDocument-true). - -### WR-03: `mergeVideoSegments` sort uses non-stable timestamp resolution - -**Files modified:** `src/offscreen/recorder.ts` -**Commit:** `2e3f524` -**Applied fix:** Added a module-level `let segmentSeq = 0;` counter and -replaced `baseTimestamp + idx` with `++segmentSeq`. Switched the encode -loop from `Promise.all(map)` to a sequential `for` loop so the counter -increments are deterministic (Promise.all would interleave shared-state -mutations). Throughput impact: ~150 ms vs ~50 ms for 3 segments — -still well under the 2 s SW timeout budget. - -### WR-09: `mergeVideoSegments` accepts unfinalized in-flight segment - -**Files modified:** `src/offscreen/recorder.ts` -**Commit:** `2e3f524` -**Applied fix:** Restructured the segment-selection logic in -`encodeAndSendBuffer` to include the in-flight Blob ONLY when -`finalized.length === 0` (the first-10-s UX trade-off documented at the -original comment). Once any rotation has finalized a segment, the -unfinalized in-flight tail is dropped — its missing Matroska -SegmentSize/Cues was exactly the "File ended prematurely" symptom from -the webm-playback-freeze debug session. - -## Remaining Work — CHECKPOINT for orchestrator continuation - -The orchestrator must spawn a continuation to complete the remaining 13 -findings plus the defensive sweep. Listing them with file:line and the -intended fix so the next instance can resume without re-reading REVIEW.md. - -### Warnings (7 remaining) - -| ID | File:Line | Intended fix | -|-------|----------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| WR-01 | `src/offscreen/recorder.ts:146-151` | Classify getDisplayMedia rejection into a stable error code (`'user-cancelled'`, `'codec-unsupported'`, `'permission-denied'`, `'unknown'`); broadcast the code, log the raw DOMException at warn level. | -| WR-02 | `src/offscreen/recorder.ts:102-113` | Remove `chrome.runtime.sendMessage` from `assertCodecSupported`; let startRecording's catch block handle the notify (currently double-emits). Update `tests/offscreen/codec-check.test.ts` GREEN block. | -| WR-04 | `smoke.sh:109` | Require `python3` in pre-flight; remove the `\|\| printf '%s' "${SMOKE_HTML}"` fallback that emits raw HTML into the data URL. (The ffprobe-gate || true pattern at line 197 is a false alarm — keep.) | -| WR-05 | `smoke.sh:74, 158, 161, 174-180` | Snapshot the BEFORE list as full filenames (sorted); diff with comm -13 against AFTER list to identify the genuinely-new zip by identity, not just count. | -| WR-06 | `src/shared/binary.ts:48-52` | DEFERRED to Phase 2 per REVIEW.md own analysis — flagged for awareness, no v1 fix required. Document this skip in REVIEW-FIX.md. | -| WR-07 | `src/shared/binary.ts:67-74` | Add `if (b64.length === 0) return new Blob([], { type: mimeType });` early-return; in SW `getVideoBufferFromOffscreen`, filter out empty segments before pushing to `mergeVideoSegments`. | -| WR-08 | `src/background/index.ts:350-358` | Replace inline base64 encoding in `downloadArchive` with `await blobToBase64(archiveBlob)` from `src/shared/binary.ts`. | - -### Info (6 remaining) - -| ID | File:Line | Intended fix | -|-------|--------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| IN-01 | `src/background/index.ts:324` + `manifest.json:4` | Replace hardcoded `extensionVersion: '1.0.0'` with `chrome.runtime.getManifest().version`. REVIEW.md flags as Phase 5 but USER said FULL SCOPE — apply now. | -| IN-02 | `src/shared/logger.ts:9, 14-22, 35, 40-50` | Migrate `Logger` and `ContentLogger` from `...args: any[]` to `...args: unknown[]`. Audit call sites in `src/background/index.ts` and content script for `console.log(...args)` semantic preservation. | -| IN-03 | `tests/offscreen/ring-buffer.test.ts` | Delete the vestigial breadcrumb file. REVIEW.md says "optional" — USER said FULL SCOPE → delete. | -| IN-04 | `tests/offscreen/webm-playback.test.ts:65-105, 172-178`| Delete `decodeDryRun` and the `void decodeDryRun` suppression; keep `decodeDryRunStrict` and move documentation into a JSDoc block. | -| IN-05 | `src/shared/types.ts:18-22` | Change `Message` to `Message`. Audit downstream `(msg as Message).data` accesses for explicit narrowing. May require updating multiple call sites in `src/background/index.ts`. | -| IN-06 | `manifest.json:6-15` | No defect — REVIEW.md confirms permissions are necessary. Document as "skipped: documented sufficient-but-broad scope per CONTEXT.md". | - -### Sweep Status - -The defensive_sweep_directive in the prompt asked for a second pass -covering missing error handling, race conditions, type-safety holes, -edge cases not tested, etc. **NOT performed this pass** due to context -budget. - -Suggested sweep targets for the continuation instance (priorities ordered -by impact based on what was observed during the CR fix pass): - -1. **Race between `rotateSegment()` and `STOP_RECORDING`**: `stopRecording()` - clears `rotationTimerId` and calls `videoRecorder.stop()`, but - `onSegmentStopped` then unconditionally calls `startNewSegment()` if - `mediaStream !== null`. After STOP_RECORDING, the stream is NOT - nulled (only `onUserStoppedSharing` nulls it), so a new segment - starts AFTER the operator manually stopped. Likely defect. -2. **`encodeAndSendBuffer` re-entrance**: Two back-to-back REQUEST_BUFFER - messages would race — `++segmentSeq` shared state, two outstanding - awaits. Each call captures `portAtRequest` independently so they - don't cross-corrupt, but timestamps interleave nondeterministically - between the two BUFFER responses. Test coverage missing. -3. **`startNewSegment()` MediaRecorder constructor throw**: If the codec - becomes unavailable mid-session (very rare but possible on some - GPU/driver hot-swaps), the `new MediaRecorder(...)` throws inside - `startNewSegment` and the rotation chain dies silently — - `onSegmentStopped` is never called again, the recorder is stuck. -4. **`onUserStoppedSharing` re-entrance**: registered with `{ once: true }` - on EACH track. If video and audio tracks both end (audio is currently - never enabled, but D-13 allows the option), the handler runs twice. - `resetBuffer()` is idempotent so this is benign today, but the - `chrome.runtime.sendMessage({ ...error: 'user-stopped-sharing' })` - call fires twice — same double-emit pattern as WR-02. -5. **`getVideoBufferFromOffscreen` resolver leak**: If the per-request - handler is removed AND a stale BUFFER arrives, the permanent - onMessage sink (CR-02 fix) catches it. But if the SW respawns mid- - `getVideoBufferFromOffscreen`, the `videoPort` reference goes stale - and the request hangs until the 2 s timeout. Test coverage missing. -6. **`base64ToBlob` adversarial input** (extends WR-07): chars outside - base64 alphabet → `atob` throws `InvalidCharacterError`. Caller - does have try/catch but the error path doesn't classify the failure. -7. **`smoke.sh` Bash style sweep**: `set -euo pipefail` interactions with - `2>/dev/null || true` patterns; missing `--` separators on `rm`, - `find`, `cp`; unquoted globs in `find ... -name '...'`. Per Google - shell style guide. -8. **Build-config hygiene**: `vite.config.ts` and `vitest.config.ts` not - inspected this pass. Quick scan for unused plugins, missing strict - options, alignment with `tsconfig.json`. - -### Test Status (end of this pass) +**Test status (end of pass):** ``` npx vitest run --reporter=dot - Test Files 8 passed (8) - Tests 30 passed (30) - Duration 2.67s + Test Files 7 passed (7) + Tests 40 passed (40) + Duration 4.37s ``` ``` @@ -188,25 +43,390 @@ grep -RIn "as any\|@ts-ignore" src/offscreen/ src/background/index.ts src/shared → no matches (gate clean) ``` -`npm run build` NOT yet exercised this pass — orchestrator continuation -should run it after all remaining fixes land. +``` +npm run build → exit 0, 60 modules transformed, dist/ produced cleanly +``` -### Next Step +## Commit Log (this pass + iteration 1) -**Orchestrator action required**: spawn continuation instance with: -- Same `` block. -- Add to the continuation prompt: "Start from commit `2e3f524`. Remaining - work is documented in `.planning/phases/01-stabilize-video-pipeline/01-REVIEW-FIX.md` - under 'Remaining Work'. Apply all 7 Warnings + 6 Info + the sweep, then - rewrite this REVIEW-FIX.md as the final report (replace the partial - marker, status: all_fixed, list every commit hash)." +### Iteration 1 (5 fixes, pre-checkpoint) -After the continuation completes: -- Run `/gsd-verify-work 1` to gate Phase 1 requirements (NOT this fixer's - job per the `` directive in the prompt). +| Hash | Subject | +|-----------|-------------------------------------------------------------------------------| +| `2e3f524` | fix(01-review): CR-01+CR-02+CR-03+WR-03+WR-09 critical port + handshake race | + +### Iteration 2 (12 fixes + sweep, this pass) + +| Hash | Subject | +|------------|---------------------------------------------------------------------------------------------------------------------| +| `650c546` | fix(01-review): WR-01+WR-02 stable capture error codes + pure assertCodecSupported | +| `349ae88` | fix(01-review): WR-04+WR-05 smoke.sh require python3 + identity-based zip detection | +| `e9aae09` | fix(01-review): WR-07 base64ToBlob empty-input shortcut + SW-side empty-segment filter | +| `f8a9c10` | fix(01-review): WR-08 downloadArchive use shared blobToBase64 helper | +| `6286957` | fix(01-review): IN-01 read extensionVersion from chrome.runtime.getManifest() | +| `b0631a4` | fix(01-review): IN-02 migrate Logger and ContentLogger to unknown[] args | +| `cb23143` | fix(01-review): IN-03 delete vestigial ring-buffer.test.ts breadcrumb | +| `680eee3` | fix(01-review): IN-04 delete decodeDryRun helper, retain only spawnSync-based decodeDryRunStrict | +| `a6e2d09` | fix(01-review): IN-05 Message + sweep any[] in RrwebEventsResponse/UserEvent.meta/popup log | +| `08a79a6` | fix(01-review): sweep #1 stopRecording nulls mediaStream first to prevent rotation race | +| `7c91f52` | fix(01-review): sweep #2+#3+#4 recorder lifecycle hardening (re-entrance + start throw + dual-track teardown) | +| `034155b` | fix(01-review): sweep #5 surface port-replaced-during-fetch diagnostic on buffer timeout | + +## Fixed Issues + +### Critical (3/3 fixed in iteration 1) + +#### CR-01: Port reconnect race drops BUFFER response under reconnect-during-encode + +**Files modified:** `src/offscreen/recorder.ts` +**Commit:** `2e3f524` +**Applied fix:** Captured `portAtRequest = keepalivePort` before the await in +`encodeAndSendBuffer`. After `blobToBase64` completes, refuse to post if +`keepalivePort !== portAtRequest` (i.e. a reconnect replaced the port mid-encode). +The SW already times out cleanly after 2 s, so the next SAVE_ARCHIVE re-issues +REQUEST_BUFFER on the fresh port — stale data never leaks to a stranger port. + +#### CR-02: SW never registers `onMessage` on the incoming port — PING silently discarded + +**Files modified:** `src/background/index.ts` +**Commit:** `2e3f524` +**Applied fix:** Added a permanent `port.onMessage.addListener` in the SW +`onConnect` handler that explicitly drains PING and silently drops unknown +traffic. The per-request BUFFER listener installed by `getVideoBufferFromOffscreen` +still wins (it's added later in the listener chain when SAVE_ARCHIVE fires). +Guarantees the SW idle-timer reset is consumed by a real handler under all +Chrome 110+ behaviour variants. + +#### CR-03: Handshake deadlock after Service Worker respawn + +**Files modified:** `src/background/index.ts` +**Commit:** `2e3f524` +**Applied fix:** When `chrome.offscreen.hasDocument()` returns true on SW init, +immediately call `offscreenReadyResolve?.()` and null the resolver. The offscreen +MUST have completed its bootstrap before becoming observable via `hasDocument()` +— waiting for an OFFSCREEN_READY that will never come is the deadlock. + +### Warnings (8/9 fixed; WR-06 pre-approved skip) + +#### WR-01: getDisplayMedia rejection surfaces raw DOMException message + +**Files modified:** `src/offscreen/recorder.ts`, `tests/offscreen/codec-check.test.ts` +**Commit:** `650c546` +**Applied fix:** Added `classifyCaptureError(error: unknown): CaptureErrorCode` +that maps DOMException names (`NotAllowedError`, `SecurityError`, `NotFoundError`, +`AbortError`) and our own codec-error to a stable string union +(`'user-cancelled' | 'permission-denied' | 'codec-unsupported' | 'no-source-selected' | 'capture-failed' | 'unknown'`). +`startRecording` and `startNewSegment` (sweep #3) both emit the code via +RECORDING_ERROR instead of the raw `error.message`. Raw message stays in logs at +warn level for forensic value. Added 7 unit tests pinning the classifier. + +#### WR-02: `assertCodecSupported()` has a side-effect + +**Files modified:** `src/offscreen/recorder.ts`, `tests/offscreen/codec-check.test.ts` +**Commit:** `650c546` +**Applied fix:** Removed the `chrome.runtime.sendMessage` from inside +`assertCodecSupported`. The function is now a pure predicate that throws. +`startRecording`'s catch block (which routes through `classifyCaptureError`) is +now the single source of truth for the broadcast. Fixes the double-emit pattern +where popup received two RECORDING_ERROR messages for one underlying problem. +Updated `codec-check.test.ts` to assert the no-side-effect contract. + +#### WR-03: `mergeVideoSegments` sort uses non-stable timestamp resolution + +**Files modified:** `src/offscreen/recorder.ts` +**Commit:** `2e3f524` +**Applied fix:** Added a module-level `let segmentSeq = 0;` counter and replaced +`baseTimestamp + idx` with `++segmentSeq`. Switched the encode loop from +`Promise.all(map)` to a sequential `for` loop so the counter increments are +deterministic. + +#### WR-04: smoke.sh python3 fallback emits raw HTML into data URL + +**Files modified:** `smoke.sh` +**Commit:** `349ae88` +**Applied fix:** Required python3 in pre-flight checks. Removed the +`|| printf '%s' "${SMOKE_HTML}"` fallback. Now smoke.sh fails loudly if python3 +is missing instead of producing a Chrome-unparseable data: URL silently. Also +applied bash style sweep (sweep #7): added `--` separators to `rm`, `mkdir`, +`stat`, `cp`, `ls`, `dirname`, `unzip` calls per Google shell style guide +defensive-against-leading-dash filenames. + +#### WR-05: smoke.sh detects new zip by count, not identity + +**Files modified:** `smoke.sh` +**Commit:** `349ae88` +**Applied fix:** Snapshot the full `BEFORE_ZIPS` list pre-recording, compute +`NEW_ZIPS=$(comm -13 )` in the polling loop, then pick the +latest among genuinely-new zips by mtime. The previous count-only comparison +false-positived when ANY session_report appeared in Downloads from an unrelated +extension session. + +#### WR-06: blobToBase64 O(n²) string concat — DEFERRED to Phase 2 + +**Status:** SKIPPED (pre-approved per REVIEW.md own analysis) +**Reason:** REVIEW.md explicitly classifies this as "out of v1 review scope +(performance)" and notes the per-byte concat is the portable way to avoid +`String.fromCharCode(...subarray)` apply-spread argument-length limit (~64 KiB). +The trade-off comment in `src/shared/binary.ts:45-47` documents the choice. The +review itself recommends: "None needed for v1. For Phase 2, consider chunked +apply with 32 KiB stride." Flagged for Phase 2 quality-bump review. + +#### WR-07: base64ToBlob does not validate input + +**Files modified:** `src/shared/binary.ts`, `src/background/index.ts`, `tests/offscreen/port-serialization.test.ts` +**Commit:** `e9aae09` +**Applied fix:** Added defensive `if (b64.length === 0) return new Blob([], { type: mimeType });` +early-return in `base64ToBlob`. In SW's `getVideoBufferFromOffscreen`, filter +empty wire segments before decode (single-pass `.filter()` then `.flatMap()` +chain — no `continue` statement per project style) AND filter zero-byte +post-decode blobs (defense-in-depth against decode edge cases). Added 3 unit +tests pinning the empty-input + adversarial-input contract (covers sweep #6). + +#### WR-08: downloadArchive duplicates blobToBase64 inline + +**Files modified:** `src/background/index.ts` +**Commit:** `f8a9c10` +**Applied fix:** Added `blobToBase64` to the existing import from +`../shared/binary`, replaced the inline `arrayBuffer → Uint8Array → for-loop → +btoa` chain with `const base64 = await blobToBase64(archiveBlob);`. Future +performance work (e.g. chunked apply for WR-06 Phase 2) now propagates to both +call sites automatically. + +#### WR-09: mergeVideoSegments accepts unfinalized in-flight segment + +**Files modified:** `src/offscreen/recorder.ts` +**Commit:** `2e3f524` +**Applied fix:** Include in-flight Blob ONLY when `finalized.length === 0` (the +first-10-s UX trade-off documented at the original comment). Once any rotation +has finalized a segment, the unfinalized in-flight tail is dropped. + +### Info (5/6 fixed; IN-06 pre-approved skip) + +#### IN-01: Hardcoded extensionVersion '1.0.0' + +**Files modified:** `src/background/index.ts` +**Commit:** `6286957` +**Applied fix:** Replaced `extensionVersion: '1.0.0'` with +`extensionVersion: chrome.runtime.getManifest().version`. SessionMetadata now +reflects the running manifest version, eliminating the dormant lie when the +manifest bumps. + +#### IN-02: Logger/ContentLogger use `...args: any[]` + +**Files modified:** `src/shared/logger.ts` +**Commit:** `b0631a4` +**Applied fix:** Migrated `Logger` and `ContentLogger` from `...args: any[]` to +`...args: unknown[]`. The `private logWithLevel` and all three public methods +(log/warn/error) on each class updated. console.log accepts unknown values +natively (its variadic signature is `(...data: any[]) => void`). Style +divergence note from plan 01-03 is now retired — all three loggers share the +same signature. + +#### IN-03: Vestigial ring-buffer.test.ts breadcrumb + +**Files modified:** `tests/offscreen/ring-buffer.test.ts` (deleted) +**Commit:** `cb23143` +**Applied fix:** `git rm tests/offscreen/ring-buffer.test.ts`. The successor +test suite `tests/offscreen/segment-rotation.test.ts` covers the D-13 invariants. +The retirement remains auditable via `git log -- tests/offscreen/ring-buffer.test.ts`. +Test count dropped from 41 → 40 (the breadcrumb's single passing assertion was +just `expect(true).toBe(true)`). + +#### IN-04: webm-playback.test.ts dead `decodeDryRun` helper + +**Files modified:** `tests/offscreen/webm-playback.test.ts` +**Commit:** `680eee3` +**Applied fix:** Deleted the `decodeDryRun(execFileSync)` helper and the +`void decodeDryRun` noUnusedLocals appeasement hack. Moved the documentation +that motivated keeping both helpers into a JSDoc block above +`decodeDryRunStrict`. The retained spawnSync-based path is the actual code path +used by the assertions; the execFileSync variant existed only as a documentation +foil. Also deduplicated the `import { spawnSync } from 'node:child_process'` +which was added mid-file. + +#### IN-05: Message default + +**Files modified:** `src/shared/types.ts`, `src/popup/index.ts` +**Commit:** `a6e2d09` +**Applied fix:** Changed `Message` to `Message`. Audit of +downstream call sites found NO direct `.data` accesses — both the SW (`src/background/index.ts`) +and the offscreen (`src/offscreen/recorder.ts`) destructure via `msg.type` switch +without reading `.data`. tsc remained at exit 0 with no widening required. +Companion sweep applied: +- `RrwebEventsResponse.events: any[]` → `unknown[]` +- `UserEvent.meta?: Record` → `Record` +- `src/popup/index.ts` local `log(...args: any[])` → `unknown[]` (aligns with IN-02) + +#### IN-06: Manifest permissions — broader than strictly required + +**Status:** SKIPPED (pre-approved per REVIEW.md own analysis) +**Reason:** REVIEW.md explicitly states "No defect — flagged for principle-of- +least-privilege awareness for any future scoping work." `activeTab` remains in +use by `chrome.tabs.captureVisibleTab` for the screenshot path (legitimate). +`` host_permissions is required by the content script (rrweb +injection). Documented as sufficient-but-broad in +[CONTEXT.md](./01-CONTEXT.md) D-A6 / D-05. + +## Sweep Targets (8 addressed) + +### #1: Race between rotateSegment() and STOP_RECORDING + +**Files modified:** `src/offscreen/recorder.ts`, `tests/offscreen/segment-rotation.test.ts` +**Commit:** `08a79a6` +**Applied fix:** `stopRecording()` now nulls `mediaStream` FIRST (before +`videoRecorder.stop()`), so the subsequent async `onSegmentStopped` callback's +`if (mediaStream !== null) startNewSegment()` gate sees the stopped state and +exits cleanly. Also stops the captured stream's tracks to release the Chrome +"Sharing your screen" indicator (secondary issue). Added regression test pinning +`resetBuffer` clears all segments and subsequent pushes start from empty buffer. +Note: deliberately does NOT call `resetBuffer()` in `stopRecording()` — the +operator may STOP then SAVE the buffered footage. + +### #2: encodeAndSendBuffer re-entrance + +**Files modified:** `src/offscreen/recorder.ts` +**Commit:** `7c91f52` +**Applied fix:** Added `encodeInFlight: boolean` guard around `encodeAndSendBuffer`. +The async work is moved into `doEncodeAndSendBuffer(portAtRequest)` and wrapped +in a try/finally that clears the flag. A concurrent call drops with a warn log; +the SW timeout fires cleanly and the next saveArchive retries on the +post-completion state. Prevents three issues: (a) interleaved `segmentSeq` +increments creating non-contiguous timestamps, (b) overlapping content if SW +combined two BUFFERs, (c) unnecessary base64 CPU during the encode window. + +### #3: startNewSegment MediaRecorder constructor throw + +**Files modified:** `src/offscreen/recorder.ts` +**Commit:** `7c91f52` +**Applied fix:** Wrapped the `new MediaRecorder(...)` + `.start()` block in a +try/catch. On throw (codec hot-swap, GPU/driver change), classify via +`classifyCaptureError` (re-using WR-01's classifier), emit RECORDING_ERROR with +the code, and tear down session state (null mediaStream, null videoRecorder, +clear rotation timer, stop all stream tracks). Previously the rotation chain +would die silently — popup green, nothing recording. Now the operator gets +actionable feedback. + +### #4: onUserStoppedSharing re-entrance with `{ once: true }` per track + +**Files modified:** `src/offscreen/recorder.ts` +**Commit:** `7c91f52` +**Applied fix:** Added `teardownInProgress: boolean` guard. The handler is +registered with `{ once: true }` on EACH track of the stream; with multiple +tracks (video + audio under D-13 — audio currently disabled but the +registration walks `getTracks()` defensively), both tracks could fire `ended` +in close succession and double-emit RECORDING_ERROR. The guard gates the +broadcast + cleanup; reset via `queueMicrotask` so same-tick re-entrance is +still gated but future startRecording → onUserStoppedSharing cycles work +correctly. + +### #5: getVideoBufferFromOffscreen resolver leak + +**Files modified:** `src/background/index.ts` +**Commit:** `034155b` +**Applied fix:** Added diagnostic logging in the buffer-fetch timeout callback: +`port_replaced_during_fetch: videoPort !== port`. Surfaces the case where the +captured `port` reference went stale (offscreen reconnected mid-fetch) — the +operator's log now shows the reconnect timing was the proximate cause of the +silent timeout. The 2 s timeout itself is the cleanup mechanism (not a true +leak); this fix turns a previously-invisible failure into a debuggable one. + +### #6: base64ToBlob adversarial input (extends WR-07) + +**Files modified:** `tests/offscreen/port-serialization.test.ts` (test-only) +**Commit:** `e9aae09` (folded into WR-07) +**Applied fix:** Added test asserting `base64ToBlob('!!!not-base64!!!', mime)` +throws synchronously. The caller (`getVideoBufferFromOffscreen`) has a try/catch +that drops the bad segment without aborting the batch — that contract is now +empirically pinned. No production code change needed beyond WR-07's filter; the +error classification at the caller would be over-engineering for a single +log-and-skip path. + +### #7: smoke.sh Google Bash style sweep + +**Files modified:** `smoke.sh` +**Commit:** `349ae88` (folded into WR-04 + WR-05) +**Applied fix:** Added `--` separators to user-path-taking commands per Google +shell style guide §"Special considerations" (defense against filenames starting +with `-`): `rm -rf --`, `mkdir -p --`, `cp --`, `stat --`, `ls --`, `dirname --`, +`unzip -p --`. Documented the `&& GATE=0 || GATE=$?` chain (REVIEW.md's WR-04 +false-alarm note) as correct in inline comment. No further `set -euo pipefail` +interactions found; the existing `|| true` patterns are properly scoped. + +### #8: build-config hygiene (vite.config.ts / vitest.config.ts) + +**Status:** NO ACTION (clean after inspection) +**Reason:** Both configs are minimal. `vite.config.ts` has crxjs with +`injectCss: false` (intentional per D-08 — no content CSS injection) and +explicit `offscreen: 'src/offscreen/index.html'` rollup input (necessary for +crxjs to discover the offscreen entry). `vitest.config.ts` has +`environment: 'node'` (correct — no jsdom needed), `typecheck: { enabled: false }` +(tsc runs separately via `npm run build`). No unused plugins, no missing +strict options, alignment with tsconfig.json is correct. Build verified clean: +`npm run build` exits 0 with 60 modules transformed. + +## Test Status (end of pass) + +``` +$ npx vitest run --reporter=dot + + Test Files 7 passed (7) + Tests 40 passed (40) + Duration 4.37s +``` + +**Test count change across iteration 2:** +- Baseline at iteration 2 start: 30 tests, 8 files +- IN-03 deletion: -1 test, -1 file (ring-buffer.test.ts breadcrumb) +- WR-01 classifier additions: +7 tests (in codec-check.test.ts) +- WR-07 adversarial-input additions: +3 tests (in port-serialization.test.ts) +- Sweep #1 segment-rotation additions: +1 test (in segment-rotation.test.ts) +- **Final: 40 tests, 7 files** + +``` +$ npx tsc --noEmit +exit 0 (no output) +``` + +``` +$ grep -RIn "as any\|@ts-ignore" src/offscreen/ src/background/index.ts src/shared/ +(no matches — type-safety gate clean) +``` + +``` +$ npm run build +✓ 60 modules transformed. +✓ built in 3.17s (exit 0) +``` + +## Constraints Respected + +- **D-12 (base64 wire format):** Untouched. All WR-07 / WR-08 / sweep #5 + changes preserve the base64 round-trip contract pinned by + `tests/offscreen/port-serialization.test.ts`. +- **D-13 (restart-segments lifecycle):** Untouched. Sweep #1 / #3 hardening + preserves the segment rotation cadence and the + `MAX_SEGMENTS × SEGMENT_DURATION_MS = 30_000 ms` operator-facing promise. +- **Project memory `feedback-gsd-ceremony-for-fixes`:** Followed. No hot-edits + outside the REVIEW.md scope; the type-safety companion fixes (popup/types) + were scoped expansion of IN-02/IN-05 per the user's "full scope" + "many more" + directive, NOT mid-fix incidental edits. +- **Project memory `feedback-no-unilateral-scope-reduction`:** Followed. All 18 + findings + all 8 sweep targets attempted. The 2 skips (WR-06, IN-06) are + documented in REVIEW.md as pre-approved no-defect / Phase-2-deferral, NOT + unilateral context cuts. + +## Next Step + +Phase 1 closure marking was already done in iteration 1's predecessors +(`7df72aa`, `1d06d9d`). Per the `` directive, this +fix pass does NOT touch REQ-*/ROADMAP.md/STATE.md. + +**Orchestrator action:** commit this REVIEW-FIX.md, then run `/gsd-verify-work 1` +to gate Phase 1 requirements against the post-fix branch state. --- -_Fixed: 2026-05-16T07:25:00Z_ -_Fixer: Claude (gsd-code-fixer, partial — CHECKPOINT)_ -_Iteration: 1_ +_Fixed: 2026-05-16T11:05:00Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 2 (final)_ +_Prior partial: iteration 1 commit `2e3f524`_