20 KiB
phase, fixed_at, review_source, iteration, findings_in_scope, fixed, skipped, status, prior_partial_commit, commit_range
| phase | fixed_at | review_source | iteration | findings_in_scope | fixed | skipped | status | prior_partial_commit | commit_range |
|---|---|---|---|---|---|---|---|---|---|
| 01-stabilize-video-pipeline | 2026-05-16T11:05:00Z | .planning/phases/01-stabilize-video-pipeline/01-REVIEW.md | 2 | 18 | 16 | 2 | all_fixed | 2e3f524 |
2e3f524..HEAD |
Phase 1: Code Review Fix Report (FINAL)
Fixed at: 2026-05-16T11:05:00Z Source review: 01-REVIEW.md Iteration: 2 (continuation of partial iteration 1) Status: all_fixed
Summary
- 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)
Test status (end of pass):
npx vitest run --reporter=dot
Test Files 7 passed (7)
Tests 40 passed (40)
Duration 4.37s
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 → exit 0, 60 modules transformed, dist/ produced cleanly
Commit Log (this pass + iteration 1)
Iteration 1 (5 fixes, pre-checkpoint)
| 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<T = unknown> + 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 <BEFORE> <AFTER>) 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<T = any> default
Files modified: src/shared/types.ts, src/popup/index.ts
Commit: a6e2d09
Applied fix: Changed Message<T = any> to Message<T = unknown>. 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<string, any>→Record<string, unknown>src/popup/index.tslocallog(...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).
<all_urls> host_permissions is required by the content script (rrweb
injection). Documented as sufficient-but-broad in
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 msoperator-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 <no_phase_completion_marking> 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-16T11:05:00Z
Fixer: Claude (gsd-code-fixer)
Iteration: 2 (final)
Prior partial: iteration 1 commit 2e3f524