docs(04-01): complete audit-p1-polish-content-script plan

Plan 04-01 closure marker — 04-01-SUMMARY.md + STATE.md position advance (Plan 1 of 7 -> Plan 2 of 7; Plan 04-02 build hygiene queued NEXT in Wave 1) + ROADMAP plan-progress table flip ([ ] -> [x] 04-01-PLAN.md row). Plan delivered (per SUMMARY): - Audit P1 #11 fetch URL extraction fix (TWO sites; instanceof Request narrow) - Audit P1 #14 navigation URL tracking fix (module-level previousUrl) - Audit P1 #15 rrweb emit timestamp normalization (Date.now() Unix epoch) - 9 new vitest tests under tests/content/; baseline 171 -> 180/180 GREEN - tsc-clean preserved; Tier-1 hook-strings inventory unchanged at 12 - Audit P1 polish backlog CLOSED 3/3 Per-task commits (TDD pair): - 3dbc51c test(04-01): Wave 0 RED — content-script test scaffolds - 7da30af feat(04-01): Wave 1 GREEN — 3 surgical edits in src/content/index.ts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-21 14:34:03 +02:00
parent 7da30afa0a
commit f72bca5c46
3 changed files with 184 additions and 12 deletions
--- a/.planning/phases/04-harden-clean-up-optional/04-01-SUMMARY.md
+++ b/.planning/phases/04-harden-clean-up-optional/04-01-SUMMARY.md
@@ -0,0 +1,170 @@
+---
+phase: 04-harden-clean-up-optional
+plan: 01
+subsystem: content-script
+tags: [audit-p1-polish, p1-11-fetch-url, p1-14-nav-url, p1-15-rrweb-timestamps, tdd, content-script, charter-d-p4-02]
+
+# Dependency graph
+requires:
+  - phase: 03-spec-10-smoke-verification-dom-event-log-verification
+    provides: REQ-user-event-log validated (A30 harness; 5 UserEvent types — click + input + navigation + js_error + network_error captured under canonical assertion shape)
+  - phase: 01-stabilize-video-pipeline
+    provides: tests/background/start-video-capture-no-tab.test.ts scaffold pattern (Plan 01-09 — pure-function-with-vi.fn-stub mirror for tests/content/)
+provides:
+  - Audit P1 #11 fetch URL extraction fix — fetch(new Request(url)) now captures Request.url in events.json instead of literal '[object Request]'
+  - Audit P1 #14 navigation URL tracking fix — module-level `let previousUrl` tracker replaces stale `history.state?.url || 'unknown'` emit so meta.previousUrl carries the operator's prior URL verbatim
+  - Audit P1 #15 rrweb timestamp normalization — rrweb emit-side timestamps normalized to Date.now() Unix-epoch ms so cleanupOldEvents (now - event.timestamp) arithmetic is meaningful
+  - tests/content/ NEW directory mirroring src/content/ — establishes the content-script unit-test placement convention for Phase 4+
+  - 9 new vitest tests (171 baseline → 180 GREEN) pinning the three contracts above
+affects: [04-02 (CSP/dead-code hardening; same Phase 4 wave), 04-04 (A34 fetch+XHR empirical harness will exercise the P1 #11 fix end-to-end), 04-07 (Phase 4 closure aggregator)]
+
+# Tech tracking
+tech-stack:
+  added: []
+  patterns:
+    - "Content-script unit-test scaffold: vi.mock('rrweb') + globalThis stubs for window/document/history/Request before await import('../../src/content/index'); userEvents+rrwebEvents read back via GET_RRWEB_EVENTS chrome.runtime.onMessage handler. Established at tests/content/ for Phase 4+ content-script work."
+    - "Type-narrow over implicit-coercion at trust boundaries: `args[0] instanceof Request ? args[0].url : String(args[0])` replaces `args[0]?.toString()` at the fetch wrapper sites. Discipline applied at both ok-branch (line ~190) AND catch-branch (line ~210) — twin sites carry the same narrow."
+    - "Module-level state for closure-time inter-event tracking: `let previousUrl` mutated swap-then-emit inside handleNavigation. Mirrors the existing rrwebEvents + userEvents module-level pattern at lines 21+24."
+    - "Wrapper-side timestamp normalization at emit time: `event.timestamp = Date.now()` prepended in rrweb record() emit callback. Same Unix-epoch convention as the existing addUserEvent normalization at line 54."
+
+key-files:
+  created:
+    - "tests/content/fetch-interception.test.ts"
+    - "tests/content/navigation-tracking.test.ts"
+    - "tests/content/rrweb-timestamps.test.ts"
+  modified:
+    - "src/content/index.ts (3 surgical edits — module-level previousUrl declaration at line 29; handleNavigation rewrite at lines 106-122; fetch URL narrow at lines 190 + 210; rrweb emit timestamp normalization at line 315)"
+    - ".planning/config.json (use_worktrees: false — orchestrator-staged flip from earlier in this session; folded into Task 1 commit)"
+
+key-decisions:
+  - "Task 1 (RED) added 4 fetch tests instead of the plan's nominal 3: Tests A+C are paired with new Tests B+D so both fetch interception sites (ok-branch line ~190 + catch-branch line ~210) get an explicit Request-arg test against the same canonical narrow. Plan acceptance allows ≥3 it() in fetch-interception.test.ts (min_lines: 60); 4 strengthens the regression net."
+  - "Used `(typeof window !== 'undefined')` guard in the module-level previousUrl initializer so the content script remains importable under Node-env vitest (vitest.config.ts environment: 'node'). Production content-script realm always has window; the guard is a one-line defensive bootstrap mirroring the project's import-time discipline (same guard pattern Plan 01-03 established for chrome.* in src/offscreen/recorder.ts)."
+  - "Did NOT introduce a helper function for the Request-or-string narrow. The inline form `(args[0] instanceof Request ? args[0].url : String(args[0])) || 'unknown'` reads clearly at the two call sites and avoids a new module-scope name. Optionally extractable in a future plan if a third call site appears."
+
+patterns-established:
+  - "tests/content/ as the canonical content-script unit-test directory (parallel to existing tests/background/, tests/i18n/, tests/offscreen/, tests/build/)"
+  - "Reading content-script module state via the production GET_RRWEB_EVENTS chrome.runtime.onMessage path doubles as a contract pin for the archive export surface"
+  - "Audit P1 polish in src/content/index.ts is now closed end-to-end via a TDD pair (RED scaffolds + GREEN edits in cohesive sibling commits)"
+
+requirements-completed: []
+
+# Metrics
+duration: ~30 min
+completed: 2026-05-21
+---
+
+# Phase 4 Plan 01: audit-p1-polish-content-script Summary
+
+**Three audit P1 correctness fixes in src/content/index.ts — fetch(new Request(url)) URL extraction at both fetch-wrapper sites, module-level previousUrl tracker replacing the always-"unknown" navigation emit, and Unix-epoch rrweb emit timestamp normalization — gated by 9 new RED→GREEN unit tests under tests/content/.**
+
+## Performance
+
+- **Duration:** ~30 min (RED scaffold + GREEN edits + bundle gates + SUMMARY)
+- **Started:** 2026-05-21T11:55:00Z (after orchestrator respawn in foreground mode)
+- **Completed:** 2026-05-21T12:25:00Z
+- **Tasks:** 2 (Wave 0 RED + Wave 1 GREEN per plan's tdd type)
+- **Files modified:** 4 (3 new test files + 1 source file edit + 1 config flip)
+
+## Accomplishments
+
+- Audit P1 #11 (fetch URL extraction) — `fetch(new Request(url))` now records the canonical `request.url` in events.json instead of the literal string `'[object Request]'`. Twin fix at both ok-branch (line ~190) and catch-branch (line ~210).
+- Audit P1 #14 (navigation URL tracking) — module-level `let previousUrl` declared at line 29; handleNavigation rewritten to swap-then-emit so `meta.previousUrl` carries the operator's actual prior URL instead of the literal `'unknown'` produced by the stale `history.state?.url || 'unknown'` lookup.
+- Audit P1 #15 (rrweb emit timestamps) — `event.timestamp = Date.now()` prepended in the rrweb `record({ emit })` callback at line 315; cleanupOldEvents `(now - event.timestamp) < RRWEB_RETENTION_MS` arithmetic at line 33 is now meaningful (both sides Unix-epoch ms).
+- 9 new vitest tests across `tests/content/` pin all three contracts; baseline preserved (171 → 180/180 GREEN). tsc-clean preserved.
+
+## Task Commits
+
+Each task was committed atomically per the plan's TDD cycle:
+
+1. **Task 1: Wave 0 RED — content-script test scaffolds** — `3dbc51c` (test)
+   - 7 of 9 tests RED at commit (B+D fetch, A+B+C navigation, A+B rrweb); 2 tests pass-as-baseline (A+C fetch — String(string)===string identity).
+2. **Task 2: Wave 1 GREEN — 3 surgical edits in src/content/index.ts** — `7da30af` (feat)
+   - All 9 tests flip GREEN; full vitest baseline preserved at 180/180.
+
+**Plan metadata commit:** to follow this SUMMARY landing.
+
+## Files Created/Modified
+
+- `tests/content/fetch-interception.test.ts` (NEW; 4 tests pinning P1 #11 — fetch(stringUrl) ok-branch + fetch(new Request(url)) ok-branch + fetch(stringUrl) catch-branch + fetch(new Request(url)) catch-branch)
+- `tests/content/navigation-tracking.test.ts` (NEW; 3 tests pinning P1 #14 — popstate + hashchange + history.pushState wrap all read meta.previousUrl from module-level state)
+- `tests/content/rrweb-timestamps.test.ts` (NEW; 2 tests pinning P1 #15 — Test A normalization to >1e12 + Test B cleanupOldEvents arithmetic regression)
+- `src/content/index.ts` (3 surgical edits — see Decisions Made for line-level details)
+- `.planning/config.json` (use_worktrees flipped to false earlier in this session by the orchestrator; folded into Task 1 commit per orchestrator instruction)
+
+## Decisions Made
+
+**Inline narrow over extracted helper for P1 #11:** Considered factoring out `function extractFetchUrl(arg: unknown): string` at module scope; chose to keep the narrow inline at both call sites for the same reason the project consistently inlines `event.timestamp = Date.now();` in the existing addUserEvent path — the discipline reads at the call site, the call sites are two-and-only-two, and a future plan can refactor if a third caller appears.
+
+**typeof window guard for module-level previousUrl:** Production content-script realm always has `window`; the `(typeof window !== 'undefined')` guard at line 29 keeps the module importable under vitest's Node environment, mirroring the defensive bootstrap pattern Plan 01-03 established for chrome.* in src/offscreen/recorder.ts.
+
+**Test count bumped 8 → 9:** Plan nominal was 3+3+2=8 tests. Added a 4th fetch test (catch-branch + new Request) so both fetch-wrapper sites have an explicit Request-arg pin. Plan acceptance permits ≥3 it() per file; this strengthens the regression net for the catch-branch fix at line ~210 which would otherwise have a single regression-guard test (Test C with a string arg).
+
+## Deviations from Plan
+
+### Auto-fixed Issues
+
+**1. [Rule 3 - Blocking] vi.mock('rrweb') in fetch + navigation tests**
+- **Found during:** Task 1 (first npm test run)
+- **Issue:** Initial RED-test scaffold for fetch-interception + navigation-tracking did not stub rrweb. The content script's `import { record } from 'rrweb'` pulled rrweb's lib/*.js through vitest's CJS transform pipeline; rrweb 2.0.0-alpha.4 ships ESM-only, so the import crashed with "exports is not defined in ES module scope" before any test body ran.
+- **Fix:** Added `vi.mock('rrweb', () => ({ record: (_opts) => () => {} }))` at the top of both fetch-interception.test.ts and navigation-tracking.test.ts. The rrweb-timestamps test already mocked rrweb (it needs the captured emit callback), so this completes the pattern across all 3 files.
+- **Files modified:** tests/content/fetch-interception.test.ts, tests/content/navigation-tracking.test.ts
+- **Verification:** All 9 content tests imported cleanly post-fix; 7 RED + 2 baseline-pass as expected for Task 1's RED gate.
+- **Committed in:** `3dbc51c` (Task 1 commit; deviation absorbed into the same Wave 0 RED commit because the fix was in the test files themselves, pre-RED-gate).
+
+**2. [Rule 1 - Bug] Removed `static prototype: unknown` from FakeXHR stubs**
+- **Found during:** Task 1 (first npm test run — vite:oxc parse error)
+- **Issue:** Initial scaffold included `static prototype: unknown = { open: () => {}, send: () => {} };` on the FakeXHR class stub. The TC39 class fields proposal reserves `prototype` as a non-writable class property; oxc (Vite's parser) correctly rejects it with `Classes may not have a static property named 'prototype'`.
+- **Fix:** Removed the `static prototype` line from all 3 test files. The instance-level open/send/addEventListener methods on FakeXHR suffice — the content script only assigns to `XMLHttpRequest.prototype.open` and `XMLHttpRequest.prototype.send` (production prototype chain), which is satisfied by the auto-generated class prototype.
+- **Files modified:** tests/content/fetch-interception.test.ts, tests/content/navigation-tracking.test.ts, tests/content/rrweb-timestamps.test.ts
+- **Verification:** Files parsed clean post-fix.
+- **Committed in:** `3dbc51c` (Task 1 commit).
+
+---
+
+**Total deviations:** 2 auto-fixed (1 blocking, 1 bug — both in the new test files, both resolved pre-commit during the Task 1 RED iteration).
+**Impact on plan:** Zero scope creep; deviations were test-side mechanics that surfaced during the first RED-gate run and got rolled into the same Task 1 commit before the RED gate was claimed. Source-side edits (Task 2) landed exactly as RESEARCH §"Specifics" and 04-PATTERNS.md spelled out.
+
+## Issues Encountered
+
+**Pre-existing vitest flakes** — 3 unrelated tests intermittently fail across baseline runs (`tests/background/blob-url-download.test.ts` 5000ms timeout race, `tests/background/webm-remux.test.ts` ffprobe frame count, `tests/offscreen/webm-playback.test.ts` ffmpeg dry-run). These are documented Phase 4 deferred items per `03-VERIFICATION.md` "Forward-Looking Deferred Items" + 04-CONTEXT.md §"Flake stabilization" — owned by Plan 04-03 (A29 race rewrite + parallel-vitest race + 2 ffprobe flakes). Confirmed pre-existing via `git stash` baseline runs (171/171 only on lucky runs; 168/171 or 165/171 on flaky runs). Plan 04-01 introduces no new flakes — final clean baseline run was 180/180 GREEN.
+
+## Pre-Checkpoint Bundle Gates
+
+Per saved memory `feedback-pre-checkpoint-bundle-gates.md` (6/6 standard inventory):
+
+1. **Tier-1 FORBIDDEN_HOOK_STRINGS** — `tests/background/no-test-hooks-in-prod-bundle.test.ts` 13/13 GREEN; inventory unchanged at 12 strings (Plan 04-01 added no harness hooks; pure source-side polish).
+2. **SW CSP-safety grep** — 1 `new Function("")` in SW chunk `dist/assets/index.ts-8LkXuqac.js`. Pre-existing `vite-plugin-node-polyfills` setimmediate polyfill; documented at `.planning/phases/01-stabilize-video-pipeline/deferred-items.md`; Plan 04-02 owns the polyfill replacement. NOT a Plan 04-01 regression.
+3. **Node-globals grep** — `Buffer.copy / .isView / .length / .push / .shift` in SW chunk — all from JSZip internals (lowercase-b `buffer` field names + JSZip stream pipeline). Pre-existing across all Phase 1+2+3 builds.
+4. **DOM-globals grep** — `document.createElement / .createTextNode / .documentElement / .F` + `window.Math / .console / .localStorage / .process` in SW chunk — pre-existing shimmed-DOM references inside JSZip's text encoder fallback paths.
+5. **manifest.json** — present at `dist/manifest.json`; `name: "__MSG_extName__"` (chrome.i18n message resolution intact). Plan 04-01 did NOT touch `_locales/` so en↔ru parity is untouched.
+6. **Build itself** — `npm run build` exits 0 in 4.66s; all chunks ship.
+
+## Self-Check
+
+- **Files created:**
+  - tests/content/fetch-interception.test.ts → FOUND
+  - tests/content/navigation-tracking.test.ts → FOUND
+  - tests/content/rrweb-timestamps.test.ts → FOUND
+- **Files modified:**
+  - src/content/index.ts → FOUND (diff shows 3 surgical edits)
+  - .planning/config.json → FOUND (use_worktrees: false)
+- **Commits:**
+  - 3dbc51c → FOUND (test(04-01): Wave 0 RED)
+  - 7da30af → FOUND (feat(04-01): Wave 1 GREEN)
+- **Verification commands all green:**
+  - npm test -- tests/content/ --run → 9 passed (9)
+  - npm test -- --run → 180 passed (180) [confirmed twice; once on flaky run with 1 pre-existing failure, once 180/180 clean]
+  - npx tsc --noEmit → exit 0
+  - All 7 acceptance grep checks pass per Task 2 commit body
+
+## Self-Check: PASSED
+
+## Next Phase Readiness
+
+- Plan 04-02 (build hygiene: vite.config.ts polyfill replacement + generate-icons ESM/CJS + dead-code grep + deferred-items.md update) is **unblocked** and queued NEXT in Wave 1. It modifies `vite.config.ts` + `src/background/index.ts` + `generate-icons.{js,cjs}` + 2 build tests + the Phase 1 deferred-items.md; zero overlap with Plan 04-01's `src/content/index.ts` surface.
+- Plan 04-04 (A34 fetch+XHR network_error empirical harness; ROADMAP SC #2) will validate Plan 04-01's P1 #11 fix end-to-end against a real probe page in Wave 3.
+- Audit P1 polish backlog: **CLOSED 3/3** (items #11 + #14 + #15 all GREEN with regression coverage).
+
+---
+*Phase: 04-harden-clean-up-optional*
+*Completed: 2026-05-21*