213 lines
13 KiB
Markdown
213 lines
13 KiB
Markdown
---
|
|
phase: 01-stabilize-video-pipeline
|
|
fixed_at: 2026-05-16T07:25:00Z
|
|
review_source: .planning/phases/01-stabilize-video-pipeline/01-REVIEW.md
|
|
iteration: 1
|
|
findings_in_scope: 18
|
|
fixed: 5
|
|
skipped: 0
|
|
remaining: 13
|
|
status: partial
|
|
checkpoint: true
|
|
checkpoint_reason: context_budget_exhausted_after_critical_cluster
|
|
commit_range: 2e3f524..HEAD
|
|
---
|
|
|
|
# Phase 1: Code Review Fix Report (PARTIAL — CHECKPOINT)
|
|
|
|
**Fixed at:** 2026-05-16T07:25:00Z
|
|
**Source review:** [01-REVIEW.md](./01-REVIEW.md)
|
|
**Iteration:** 1
|
|
**Status:** partial — CHECKPOINT for orchestrator continuation
|
|
|
|
## 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)
|
|
|
|
**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<T = any>` to `Message<T = unknown>`. 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)
|
|
|
|
```
|
|
npx vitest run --reporter=dot
|
|
Test Files 8 passed (8)
|
|
Tests 30 passed (30)
|
|
Duration 2.67s
|
|
```
|
|
|
|
```
|
|
npx tsc --noEmit → exit 0 (no output)
|
|
```
|
|
|
|
```
|
|
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.
|
|
|
|
### Next Step
|
|
|
|
**Orchestrator action required**: spawn continuation instance with:
|
|
- Same `<config>` 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)."
|
|
|
|
After the continuation completes:
|
|
- Run `/gsd-verify-work 1` to gate Phase 1 requirements (NOT this fixer's
|
|
job per the `<no_phase_completion_marking>` directive in the prompt).
|
|
|
|
---
|
|
|
|
_Fixed: 2026-05-16T07:25:00Z_
|
|
_Fixer: Claude (gsd-code-fixer, partial — CHECKPOINT)_
|
|
_Iteration: 1_
|