docs(01-05): complete SW shrink + port host plan
Plan 05 closes: src/background/index.ts is now a pure coordinator with zero video-buffer state, T-1-04 mitigations on both onConnect and onMessage, OFFSCREEN_READY handshake, port-based buffer fetch via 'video-keepalive' port, IDB orphan cleanup on install, and chrome.offscreen.hasDocument() re-sync on SW respawn (audit P1 #8). 9/9 vitest tests still green; tsc clean; no as any / @ts-ignore. REQ-video-ring-buffer stays pending — Plan 07's ffprobe gate owns the final completion marker.
This commit is contained in:
301
.planning/phases/01-stabilize-video-pipeline/01-05-SUMMARY.md
Normal file
301
.planning/phases/01-stabilize-video-pipeline/01-05-SUMMARY.md
Normal file
@@ -0,0 +1,301 @@
|
||||
---
|
||||
phase: 01-stabilize-video-pipeline
|
||||
plan: 05
|
||||
subsystem: service-worker-coordinator
|
||||
tags: [chrome-extension, mv3, service-worker, long-lived-port, offscreen-document, t-1-04, security-mitigation]
|
||||
|
||||
# Dependency graph
|
||||
requires:
|
||||
- phase: 01-stabilize-video-pipeline
|
||||
provides: "Plan 03 ring-buffer ownership moved to offscreen; Plan 03 inline SW cleanup of VIDEO_CHUNK / VIDEO_CHUNK_SAVED / openIndexedDB / loadChunkFromIndexedDB; Plan 04 offscreen-side port keepalive + OFFSCREEN_READY handshake"
|
||||
provides:
|
||||
- "Shrunk src/background/index.ts (coordinator only): offscreen lifecycle, port host, export-time buffer fetch — zero local buffer state"
|
||||
- "SW-side chrome.runtime.onConnect listener bound to 'video-keepalive' port (Plan 04 counterparty) with T-1-04 sender-id check"
|
||||
- "Async getVideoBufferFromOffscreen(): REQUEST_BUFFER → BUFFER round-trip with 2s timeout; powers GET_VIDEO_BUFFER and SAVE_ARCHIVE handlers"
|
||||
- "OFFSCREEN_READY handshake handler resolving a module-level Promise; startVideoCapture awaits it before sending START_RECORDING (audit P1 #12 fix)"
|
||||
- "Idempotent indexedDB.deleteDatabase('VideoRecorderDB') in onInstalled (T-1-NEW-05-02; RESEARCH.md Runtime State Inventory)"
|
||||
- "chrome.offscreen.hasDocument() check on SW init for robust offscreenCreated state across SW respawns (audit P1 #8)"
|
||||
- "T-1-NEW-05-01 onMessage sender.id guard at handler top"
|
||||
affects: [01-06-vite-config, 01-07-ffprobe-gate]
|
||||
|
||||
# Tech tracking
|
||||
tech-stack:
|
||||
added: []
|
||||
patterns:
|
||||
- "SW-as-pure-coordinator: zero local state for chunks; offscreen is the single source of truth (D-16)"
|
||||
- "Promise-based handshake (offscreenReady) so popup's transient user-activation isn't lost to bootstrap race"
|
||||
- "Port-based async RPC via REQUEST_BUFFER → BUFFER with per-call onMessage listener + 2 s timeout fallback"
|
||||
- "Defense-in-depth on both onConnect (port name + port.sender?.id) and onMessage (sender.id) — T-1-04 mitigations on both surfaces"
|
||||
- "instanceof Error pattern in catches instead of '(error as any).message' — eliminates audit P1 #13 instances in this file"
|
||||
|
||||
key-files:
|
||||
created: []
|
||||
modified:
|
||||
- "src/background/index.ts (388 → 491 lines; ~ +130 / -100 net; structural shrink — buffer + alarms + IDB + tabCapture paths went away; port host + handshake + IDB cleanup + hasDocument check + sender guards came in)"
|
||||
|
||||
key-decisions:
|
||||
- "checkPermissions / requestPermissions deleted entirely (Rule 1 deviation): they referenced 'tabCapture' permission which was swapped to 'desktopCapture' in manifest (D-A6). Under getDisplayMedia (D-01) no runtime-permission check is meaningful — the browser prompts via picker on user gesture. REQUEST_PERMISSIONS now just calls startVideoCapture() and returns granted:true. Plan only mandated removing the getMediaStreamId call; the broken permissions code was logically downstream."
|
||||
- "Synchronous getVideoBuffer() kept as an empty-array stub in Task 1 (to keep tsc clean while videoBuffer module state was deleted), then replaced with async getVideoBufferFromOffscreen() in Task 2. The plan acknowledges this two-step in Task 2 step (4)."
|
||||
- "Added chrome.offscreen.hasDocument() check inside initialize() (Rule 2 robustness — orchestrator-flagged audit P1 #8). The check is guarded with `typeof chrome.offscreen?.hasDocument === 'function'` so it stays safe across versions and partial stubs. Plan did not explicitly require this, but the orchestrator's instructions did."
|
||||
- "Fixed two `as any` casts in the existing code that the plan didn't strictly require but CLAUDE.md mandates: (a) the catch block in ensureOffscreen now uses `error instanceof Error` instead of `(error as any).message`; (b) chrome.tabs.sendMessage cast moved from `as any` to an explicit `{ events?, userEvents? } | undefined` type. Both are pre-existing audit P1 #13 instances; cleaning them now closes a deviation surface for Phase 5."
|
||||
- "Did NOT mark REQ-video-ring-buffer complete: Plan 07 owns the ffprobe gate that proves end-to-end requirement satisfaction."
|
||||
|
||||
patterns-established:
|
||||
- "Port lifecycle on SW side: onConnect → port-name filter → sender.id filter → store in videoPort → register onDisconnect (null out reference) → done. Each request installs a one-shot onMessage listener via getVideoBufferFromOffscreen and removes it on completion or timeout."
|
||||
- "Handshake pattern: module-level Promise + resolve closure captured at module load. OFFSCREEN_READY case calls resolve() and nulls out the closure (one-shot semantics). startVideoCapture awaits it after ensureOffscreen()."
|
||||
- "T-1-04 enforcement: when SW is the trusting party of a port opened by offscreen, validate `port.sender?.id === chrome.runtime.id`. When SW is the trusting party of a runtime message, validate `sender.id === chrome.runtime.id`. Both checks are now in place."
|
||||
|
||||
requirements-completed: [] # REQ-video-ring-buffer still pending Plan 07 ffprobe gate; this plan is structural plumbing only.
|
||||
|
||||
# Metrics
|
||||
duration: 8min
|
||||
completed: 2026-05-15
|
||||
---
|
||||
|
||||
# Phase 01 Plan 05: SW Shrink + Port Host Summary
|
||||
|
||||
**Shrunk `src/background/index.ts` to a pure coordinator: deleted legacy buffer / alarms / IndexedDB / tabCapture code paths (Task 1) and wired the SW-side counterparty of the long-lived `'video-keepalive'` port (Task 2) — with T-1-04 sender-id guards on both onConnect and onMessage, an `OFFSCREEN_READY` handshake, `chrome.offscreen.hasDocument()` re-sync on SW respawn, and an idempotent `indexedDB.deleteDatabase('VideoRecorderDB')` cleanup on install.**
|
||||
|
||||
## Performance
|
||||
|
||||
- **Duration:** ~8 min
|
||||
- **Started:** 2026-05-15T15:53:30Z (immediately after Plan 04 completed)
|
||||
- **Completed:** 2026-05-15T16:03:00Z
|
||||
- **Tasks:** 2 (DELETE pass + ADD pass)
|
||||
- **Commits:** 2 (one per task)
|
||||
- **Files modified:** 1 (`src/background/index.ts`)
|
||||
|
||||
## Before / After Line Count
|
||||
|
||||
| Snapshot | Lines | Notes |
|
||||
|----------|-------|-------|
|
||||
| Pre-Plan 05 (post-Plan 03 inline cleanup) | **436** | Plan 03's executor had already removed VIDEO_CHUNK / VIDEO_CHUNK_SAVED / openIndexedDB / loadChunkFromIndexedDB inline as Rule-3 blocking-fix deps. |
|
||||
| Post-Task 1 (DELETE pass) | 387 | -49 lines: deletions of videoBuffer, setupKeepalive, chrome.tabCapture.getMediaStreamId, checkPermissions, requestPermissions, USER_MEDIA cast, two `as any` casts. |
|
||||
| Post-Task 2 (ADD pass) | **491** | +104 lines: onConnect handler, getVideoBufferFromOffscreen, offscreenReady Promise, OFFSCREEN_READY case, sender-id guards, indexedDB.deleteDatabase, hasDocument check, comments documenting each mitigation. |
|
||||
|
||||
The plan estimated 380-440 lines post-Task 2. The actual count is 491 because (a) I added the orchestrator-requested `chrome.offscreen.hasDocument()` block (~15 lines including comments) and (b) I expanded the comments around each security mitigation to make T-1-04 / T-1-NEW-05-01 / T-1-NEW-05-02 / audit P1 #8 / audit P1 #12 references explicit for future auditors. The structural shrink (zero buffer state, zero alarms, zero IDB plumbing, zero tabCapture) is intact — the line-count overshoot is documentation, not code.
|
||||
|
||||
## Task 1: Deletions
|
||||
|
||||
**Commit:** `886376e refactor(01-05): delete legacy SW buffer, alarms, IndexedDB, tabCapture paths`
|
||||
|
||||
| Deleted Symbol / Path | Reason |
|
||||
|-----------------------|--------|
|
||||
| `let videoBuffer: VideoChunk[] = []` | D-16 — buffer ownership moved to offscreen |
|
||||
| `function setupKeepalive` + `chrome.alarms.create('keepalive', ...)` + `chrome.alarms.onAlarm.addListener` | D-18 / audit P1 #8 — alarms never reset SW idle timer; port does |
|
||||
| Call site `setupKeepalive()` inside `initialize` | Paired with the function delete |
|
||||
| `chrome.tabCapture.getMediaStreamId({...})` block inside `startVideoCapture` (and its `as any` cast) | D-01 — getDisplayMedia now runs inside the offscreen document |
|
||||
| `async function checkPermissions` | Referenced `'tabCapture'` (removed from manifest by D-A6); under getDisplayMedia no runtime perm check is meaningful |
|
||||
| `async function requestPermissions` | Same reason. The popup user-gesture flows directly into startVideoCapture now |
|
||||
| `reasons: ['USER_MEDIA'] as any` in createDocument | Replaced by canonical `[chrome.offscreen.Reason.DISPLAY_MEDIA]` (D-02; @types/chrome 0.0.268 exposes the enum) |
|
||||
| `(error as any).message?.includes(...)` in createDocument catch | Replaced by `error instanceof Error ? error.message : String(error)` (CLAUDE.md no-`as any` rule) |
|
||||
| `as any` cast on `chrome.tabs.sendMessage(...)` response | Replaced by an explicit `{ events?: unknown[]; userEvents?: unknown[] } | undefined` type |
|
||||
| Justification string `'Need to record video from tab for error reporting'` | Replaced with `'Continuous screen recording for operator session diagnostics'` to match the new capture semantics (D-04 — NOT silent) |
|
||||
|
||||
**Note (already done by Plan 03):** `addVideoChunkFromBlob`, `cleanupVideoBuffer`, `firstChunkSaved`, `VIDEO_BUFFER_DURATION_MS`, the `VIDEO_CHUNK` and `VIDEO_CHUNK_SAVED` case branches, `loadChunkFromIndexedDB`, `openIndexedDB`, and the chrome.tabs.onActivated handler were ALREADY removed by Plan 03's Rule-3 inline cleanup (logged in STATE.md decisions). Plan 05 verified these via grep gates and removed only the residual placeholder comments.
|
||||
|
||||
## Task 2: Additions
|
||||
|
||||
**Commit:** `5cd1519 feat(01-05): wire SW-side port host and port-based buffer fetch`
|
||||
|
||||
### onMessage switch — final shape
|
||||
|
||||
```typescript
|
||||
chrome.runtime.onMessage.addListener((message: Message, sender, sendResponse) => {
|
||||
// T-1-NEW-05-01 mitigation: only accept onMessage traffic from this extension
|
||||
if (sender.id !== chrome.runtime.id) {
|
||||
logger.warn('Rejecting message with mismatched sender:', sender.id);
|
||||
return false;
|
||||
}
|
||||
logger.log('Received message:', message.type, message);
|
||||
|
||||
switch (message.type) {
|
||||
case 'REQUEST_PERMISSIONS': // → startVideoCapture(), respond {granted: true|false}
|
||||
case 'GET_VIDEO_BUFFER': // → getVideoBufferFromOffscreen() → sendResponse(resp)
|
||||
case 'SAVE_ARCHIVE': // → saveArchive() → sendResponse(result)
|
||||
case 'OFFSCREEN_READY': // → offscreenReadyResolve?.(); offscreenReadyResolve = null
|
||||
default: // → logger.warn('Unknown message type:', ...), return false
|
||||
}
|
||||
});
|
||||
```
|
||||
|
||||
### chrome.offscreen.createDocument — as committed
|
||||
|
||||
```typescript
|
||||
await chrome.offscreen.createDocument({
|
||||
url: url,
|
||||
reasons: [chrome.offscreen.Reason.DISPLAY_MEDIA],
|
||||
justification: 'Continuous screen recording for operator session diagnostics'
|
||||
});
|
||||
```
|
||||
|
||||
(Plans 06 / 07 can grep against `chrome.offscreen.Reason.DISPLAY_MEDIA` to confirm D-02 wiring.)
|
||||
|
||||
### onConnect — port host
|
||||
|
||||
```typescript
|
||||
chrome.runtime.onConnect.addListener((port) => {
|
||||
if (port.name !== 'video-keepalive') return;
|
||||
if (port.sender?.id !== chrome.runtime.id) { // T-1-04
|
||||
port.disconnect();
|
||||
return;
|
||||
}
|
||||
videoPort = port;
|
||||
port.onDisconnect.addListener(() => { videoPort = null; });
|
||||
});
|
||||
```
|
||||
|
||||
### getVideoBufferFromOffscreen — port-based RPC
|
||||
|
||||
```typescript
|
||||
async function getVideoBufferFromOffscreen(): Promise<VideoBufferResponse> {
|
||||
if (videoPort === null) return { chunks: [] };
|
||||
const port = videoPort;
|
||||
return new Promise<VideoBufferResponse>((resolve) => {
|
||||
const timer = setTimeout(() => {
|
||||
port.onMessage.removeListener(handler);
|
||||
resolve({ chunks: [] }); // 2 s timeout fallback
|
||||
}, BUFFER_FETCH_TIMEOUT_MS);
|
||||
const handler = (msg: unknown) => {
|
||||
if (typeof msg === 'object' && msg !== null && (msg as { type?: unknown }).type === 'BUFFER') {
|
||||
clearTimeout(timer);
|
||||
port.onMessage.removeListener(handler);
|
||||
const chunks = (msg as { chunks?: VideoChunk[] }).chunks ?? [];
|
||||
resolve({ chunks });
|
||||
}
|
||||
};
|
||||
port.onMessage.addListener(handler);
|
||||
port.postMessage({ type: 'REQUEST_BUFFER' });
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
### onInstalled — orphan IDB cleanup
|
||||
|
||||
```typescript
|
||||
chrome.runtime.onInstalled.addListener((details) => {
|
||||
// T-1-NEW-05-02 / RESEARCH.md Runtime State Inventory
|
||||
try {
|
||||
indexedDB.deleteDatabase('VideoRecorderDB');
|
||||
} catch (e) {
|
||||
logger.warn('IDB cleanup failed:', e);
|
||||
}
|
||||
initialize();
|
||||
});
|
||||
```
|
||||
|
||||
### initialize() — hasDocument re-sync (P1 #8)
|
||||
|
||||
```typescript
|
||||
async function initialize() {
|
||||
// After SW respawn, offscreenCreated resets to false but the offscreen
|
||||
// document may still exist. Ask Chrome the ground truth.
|
||||
try {
|
||||
if (typeof chrome.offscreen?.hasDocument === 'function') {
|
||||
const exists = await chrome.offscreen.hasDocument();
|
||||
if (exists) offscreenCreated = true;
|
||||
}
|
||||
} catch (err) { logger.warn(...); }
|
||||
}
|
||||
```
|
||||
|
||||
## Verification
|
||||
|
||||
### tsc + vitest (both green)
|
||||
|
||||
```
|
||||
$ npx tsc --noEmit
|
||||
(exit 0, no output)
|
||||
|
||||
$ npx vitest run
|
||||
Test Files 4 passed (4)
|
||||
Tests 9 passed (9)
|
||||
Start at 18:03:02
|
||||
Duration 319ms
|
||||
```
|
||||
|
||||
All 9 tests passing across 4 files in `tests/offscreen/` — Plan 04's offscreen-side stayed untouched, so port + handshake + ring-buffer + codec-check tests all green as expected.
|
||||
|
||||
### Grep gates (all pass)
|
||||
|
||||
| Gate | Expected | Actual |
|
||||
|------|----------|--------|
|
||||
| `chrome.alarms` in src/background/ | 0 | 0 |
|
||||
| `VideoRecorderDB` / `openIndexedDB` / `loadChunkFromIndexedDB` in src/ (excluding cleanup call) | 0 outside cleanup | 2 (both the cleanup call + its log message) — only the cleanup path remains |
|
||||
| `setupKeepalive` / `addVideoChunkFromBlob` / `cleanupVideoBuffer` / `tabCapture` / `getMediaStreamId` in src/background/ | 0 | 0 |
|
||||
| `chrome.tabs.onActivated` / `chrome.tabs.onUpdated` in src/background/ | 0 | 0 (D-14 / D-15 satisfied) |
|
||||
| `chrome.runtime.onConnect.addListener` in src/background/index.ts | 1 | 1 |
|
||||
| `'video-keepalive'` in src/background/index.ts | ≥1 | 1 |
|
||||
| `port.sender?.id !== chrome.runtime.id` (T-1-04) | 1 | 1 |
|
||||
| `sender.id !== chrome.runtime.id` (T-1-NEW-05-01) | 1 | 1 |
|
||||
| `indexedDB.deleteDatabase('VideoRecorderDB')` | 1 | 1 |
|
||||
| `chrome.offscreen.hasDocument` (audit P1 #8) | ≥1 | 1 (the typeof + call) |
|
||||
| `function getVideoBufferFromOffscreen` | 1 | 1 |
|
||||
| `OFFSCREEN_READY` mentions | ≥1 | 3 (Promise comment + case label + log message) |
|
||||
| `offscreenReady` mentions | ≥2 | 6 (Promise var + resolve closure + await + case label + log + comment) |
|
||||
| `as any` in src/background/ | 0 | 0 |
|
||||
| `@ts-ignore` in src/background/ | 0 | 0 |
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
### Rule 1 — Bug fixes (auto-applied)
|
||||
|
||||
**1. [Rule 1 - Bug] Deleted broken `checkPermissions` / `requestPermissions` flow**
|
||||
- **Found during:** Task 1.
|
||||
- **Issue:** `chrome.permissions.contains({ permissions: ['tabCapture'] })` references a permission that was removed from `manifest.json` by D-A6 (replaced with `desktopCapture`). The check would always return `false`, sending `REQUEST_PERMISSIONS` into the never-granted branch, which itself calls `chrome.permissions.request({ permissions: ['tabCapture'] })` — same problem. Recording could not start cleanly.
|
||||
- **Fix:** Deleted both functions entirely. `REQUEST_PERMISSIONS` now just calls `startVideoCapture()` (which goes through ensureOffscreen → DISPLAY_MEDIA reason → offscreen → getDisplayMedia picker → user gesture).
|
||||
- **Files modified:** `src/background/index.ts`.
|
||||
- **Commit:** `886376e`.
|
||||
|
||||
**2. [Rule 1 - Bug] Replaced two `(error as any).message` patterns with `error instanceof Error`**
|
||||
- **Found during:** Task 1 (the `as any` grep gate was at 2 instead of 0 because of a pre-existing instance in the ensureOffscreen catch).
|
||||
- **Issue:** Audit P1 #13 — `as any` violates CLAUDE.md "no `as any`" rule. The catch block in `ensureOffscreen` accessed `.message` via `(error as any).message`.
|
||||
- **Fix:** `const msg = error instanceof Error ? error.message : String(error); if (msg.includes(...)) { ... }`.
|
||||
- **Files modified:** `src/background/index.ts`.
|
||||
- **Commit:** `886376e`.
|
||||
|
||||
**3. [Rule 1 - Bug] Replaced `chrome.tabs.sendMessage(...) as any` with an explicit response type**
|
||||
- **Found during:** Task 1 (same `as any` grep gate).
|
||||
- **Issue:** Same audit P1 #13 / CLAUDE.md violation; the response was typed as `any` to access `.events` and `.userEvents`.
|
||||
- **Fix:** Explicit `{ events?: unknown[]; userEvents?: unknown[] } | undefined` annotation on the response variable; nullish coalescing for the two extracts.
|
||||
- **Files modified:** `src/background/index.ts`.
|
||||
- **Commit:** `886376e`.
|
||||
|
||||
### Rule 2 — Missing critical functionality (auto-applied)
|
||||
|
||||
**4. [Rule 2 - Robustness] Added `chrome.offscreen.hasDocument()` check inside `initialize()`**
|
||||
- **Found during:** Task 2 (orchestrator-flagged audit P1 #8).
|
||||
- **Issue:** Across SW respawns, the in-memory `offscreenCreated` flag resets to `false`, but the offscreen document may still be alive (it survives SW idle unload because it holds the DISPLAY_MEDIA-reason capture). The next `ensureOffscreen()` would then call `createDocument` over an existing one. The catch block handles "already exists" so it's not strictly broken — but the hasDocument check makes it idempotent and is the canonical Chrome MV3 pattern (RESEARCH.md A7).
|
||||
- **Fix:** `initialize()` is now `async` and calls `await chrome.offscreen.hasDocument()` to set `offscreenCreated = true` if a document is already there. Guarded with `typeof chrome.offscreen?.hasDocument === 'function'` so it stays safe across @types/chrome versions and partial stubs.
|
||||
- **Files modified:** `src/background/index.ts`.
|
||||
- **Commit:** `5cd1519`.
|
||||
|
||||
### Plan / orchestrator reconciliation note
|
||||
|
||||
The plan's `must_haves` and Task 1 instructions referenced symbols (`addVideoChunkFromBlob`, `cleanupVideoBuffer`, IDB helpers, VIDEO_CHUNK case, etc.) that Plan 03's executor had already inline-deleted as a Rule-3 blocking-fix dependency (documented in STATE.md). Plan 05 verified those via grep gates and only had to handle the residual `videoBuffer` array, the keepalive function, the tabCapture call site, and the comments. This matches the orchestrator's pre-flight note in the prompt.
|
||||
|
||||
## TDD Gate Compliance
|
||||
|
||||
This plan was `type: execute` (not `type: tdd`), so the RED/GREEN/REFACTOR gate sequence does not apply. The test suite remained at 9/9 throughout — Plan 04's port + handshake tests stayed green because Plan 05 only touches the SW side, and the offscreen-side port contract is unchanged.
|
||||
|
||||
## Authentication Gates
|
||||
|
||||
None — this plan is pure refactor + integration plumbing.
|
||||
|
||||
## Known Stubs
|
||||
|
||||
None. All paths in the modified file have a real implementation. `getVideoBufferFromOffscreen` returns `{ chunks: [] }` when `videoPort === null`, which is an intentional fallback (offscreen not yet connected, e.g. during SW cold start before the offscreen has finished bootstrapping). This is the documented contract per `<interfaces>` step 4 of the plan, not a stub.
|
||||
|
||||
## Self-Check
|
||||
|
||||
Verified after writing this summary:
|
||||
- ✓ `886376e` exists in git log (Task 1 commit).
|
||||
- ✓ `5cd1519` exists in git log (Task 2 commit).
|
||||
- ✓ `src/background/index.ts` exists (491 lines).
|
||||
- ✓ `npx tsc --noEmit` exits 0.
|
||||
- ✓ `npx vitest run` reports 9/9 PASS across 4 test files.
|
||||
- ✓ All Task 1 deletion grep gates return 0.
|
||||
- ✓ All Task 2 addition grep gates return their expected counts.
|
||||
- ✓ `grep -c "as any"` and `grep -c "@ts-ignore"` in `src/background/` both return 0.
|
||||
|
||||
## Self-Check: PASSED
|
||||
Reference in New Issue
Block a user