Plan 04-08's core thesis (HTMLVideoElement.captureStream bypasses canvas throttling per debug session-2 verdict) IS the correct path to close ROADMAP SC #1. But two blocking issues prevent reliable delivery: BLOCKER 1: Vite `?url` asset-emission analog mis-applied — mokosh-mark.svg is 877 bytes (inlined as data:image/svg+xml URI) so the Plan 01-10 "?url + crxjs auto-WAR" precedent is NOT a direct analog for the 1.9 MB WebM which will emit as a separate dist-test/assets/<hash>.webm file. WAR auto-generation for extracted assets is unverified in this codebase. Remediation: probe-then-decide OR Blob URL from ?raw ArrayBuffer. BLOCKER 2: installFakeDisplayMedia()'s eager-install-at-module-load contract is silently broken by the proposed async conversion. The race window opens because recorder.ts:48 resolves before the async install completes; recorder.startRecording → real getDisplayMedia → headless hang. Remediation: keep sync monkey-patch; defer the canplay wait into fakeGetDisplayMedia closure (lazy first-frame). WARNINGS surface unverified headless autoplay reliability, displaySurface monkey-patch portability to HTMLVideoElement tracks, spike probe-value gates not surfaced as automated verify, and ROADMAP.md flip without grep enforcement. Architectural alignment confirmed (segments: Blob[] preserved; IDB correctly rejected; D-P4-01 honored). iter-2 is a methodology-tightening pass, not re-architecture. Estimated ~150-300 lines of plan edits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
292 lines
28 KiB
Markdown
292 lines
28 KiB
Markdown
---
|
|
phase: 04-harden-clean-up-optional
|
|
plan: 08
|
|
checker_iteration: 1
|
|
checked_at: 2026-05-22T08:56:00Z
|
|
plan_commit: 504d9dc
|
|
verdict: ITERATE-NEEDED
|
|
severity_summary:
|
|
blocker: 2
|
|
warning: 5
|
|
cosmetic-advisory: 3
|
|
goal_backward_check: "Plan 04-08 IS architecturally aligned with closing ROADMAP SC #1 — the HTMLVideoElement.captureStream() methodology bypasses the canvas-throttling root cause identified in debug session-2. BUT two blocking issues prevent the plan from delivering reliably as written: (1) the Vite ?url asset-emission assumption is mis-extrapolated from a 877-byte-inlined SVG to a 1.9 MB extracted WebM (different bundling path; web_accessible_resources auto-WAR behavior is unverified for the extracted-asset case); (2) the conversion of installFakeDisplayMedia() to async forces module-load top-level await on `canplay` + `videoEl.play()`, which has unverified interactions with the existing recorder.ts:47 top-level await chain AND the offscreen document creation sequence. Both are fixable in iter-2 but they ARE blocking as currently specified."
|
|
recommendation: "Spawn planner iter-2 with the 2 BLOCKERs + 5 WARNINGs as concrete remediation items. After iter-2 lands, the plan SHOULD pass and close SC #1 under valid methodology."
|
|
---
|
|
|
|
# Plan 04-08 Pre-Execution Validation — Iter-1
|
|
|
|
**Plan under review:** `.planning/phases/04-harden-clean-up-optional/04-08-PLAN.md` @ commit `504d9dc`
|
|
|
|
**Phase goal recall:** Close ROADMAP SC #1 ("After running the extension idle for >5 minutes, then exporting, the archive still contains a non-empty video buffer") via methodology reframe, per debug session-2 REFUTED-architecture verdict (commit `4ea1bbb`). Plan 04-08 inserts as Wave 5.5 between Plans 04-06 and 04-07, replaces `canvas.captureStream(30)` with `HTMLVideoElement.captureStream(30)` in `installFakeDisplayMedia()`, re-runs the spike under valid methodology, then lands A33 + driveA33 + orchestrator wiring (33→34 GREEN).
|
|
|
|
**Authority artifacts read:**
|
|
|
|
- `.planning/phases/04-harden-clean-up-optional/04-08-PLAN.md` (804 lines)
|
|
- `.planning/phases/04-harden-clean-up-optional/04-04-PLAN.md` (476 lines)
|
|
- `.planning/phases/04-harden-clean-up-optional/04-04-SUMMARY.md` (267 lines incl. post-debug amendment)
|
|
- `.planning/debug/sw-offscreen-persistence-investigation-session-2.md` (97 lines)
|
|
- `.planning/phases/04-harden-clean-up-optional/04-CONTEXT.md` + `04-RESEARCH.md` + `04-PATTERNS.md`
|
|
- `.planning/PROJECT.md` + `.planning/ROADMAP.md`
|
|
- `src/test-hooks/offscreen-hooks.ts` (541 lines — current installFakeDisplayMedia + uninstallFakeDisplayMedia + 6 bridge ops)
|
|
- `src/offscreen/recorder.ts` lines 40-100 (top-level await + `let segments: Blob[] = []` at line 91)
|
|
- `tests/uat/spike-a33-sw-persistence.ts` (337 lines)
|
|
- `tests/uat/lib/harness-page-driver.ts` lines 1-100 + line 2464 (driveA32 location)
|
|
- `tests/uat/harness.test.ts` lines 95-138 (import block + FORBIDDEN_HOOK_STRINGS at lines 123-138) + lines 340-487 (wrapped-driver + drivers-array)
|
|
- `globals.d.ts` (37 lines)
|
|
- `manifest.json` + `dist/manifest.json` + `dist-test/manifest.json`
|
|
- `vite.config.ts` lines 1-50 + 145-170 + `vite.test.config.ts` (101 lines)
|
|
- `dist/assets/welcome-BXz6I5Ha.js` (verified SVG inlined as data:image/svg+xml URI)
|
|
- `tests/fixtures/last_30sec.webm` (1888636 bytes, ffprobe confirms codec=vp9 width=1142 height=1044)
|
|
|
|
---
|
|
|
|
## Goal-Backward Verification — Closes SC #1?
|
|
|
|
**Architectural claim:** Plan 04-08 replaces canvas.captureStream (throttled to 0 frames under headless 5-min idle per debug session-2) with HTMLVideoElement.captureStream backed by a bundled WebM source. The video file's real decoded frame timeline is NOT subject to invisible-canvas throttling — `videoEl.captureStream(30)` on a `<video>` element playing a bundled WebM produces real VP9 frames continuously across the 5-min idle.
|
|
|
|
**Authority for the claim:** debug session-2 verdict explicitly recommends "(b) replace the canvas-captureStream fake with a video-file-backed source (e.g., MediaStream from an HTMLVideoElement playing a bundled WebM) which doesn't suffer the invisible-canvas throttling" (line 69 of session-2 note). 7/7 spike runs across two debug sessions converge on the 8505-byte 0-frames failure under canvas; the videoEl path explicitly side-steps it per MDN HTMLMediaElement.captureStream + Chrome bug 653548 design doc.
|
|
|
|
**Verdict:** YES — Plan 04-08 IS architecturally the correct path to close SC #1. The methodology bypasses the documented root cause. The architectural invariant (`src/offscreen/recorder.ts:91 let segments: Blob[] = []` unchanged) is preserved verbatim. The IndexedDB plan-fix is correctly REJECTED per session-2 evidence.
|
|
|
|
**Caveat:** The plan's architectural correctness is necessary but not sufficient. Two BLOCKERs (below) prevent the plan from delivering reliably as currently written. They are fixable in iter-2.
|
|
|
|
---
|
|
|
|
## Findings by Severity
|
|
|
|
### BLOCKER
|
|
|
|
#### BLOCKER 1 — Vite `?url` asset-emission assumption is mis-extrapolated from Plan 01-10 SVG precedent
|
|
|
|
**Dimension:** key_links_planned / verification_derivation
|
|
|
|
**Plan claim** (line 42 must_have, line 132 context, line 543 Step 5, line 553 Step 6):
|
|
> "bundled into the test-config build via Vite ?url import (per Plan 01-10 mokosh-mark.svg precedent)"
|
|
> "the Vite `?url` import resolves the WebM via Vite's asset pipeline. ... `@crxjs/vite-plugin` auto-generates a `web_accessible_resources` entry for the offscreen-reachable asset (same pattern as Plan 01-10 mokosh-mark.svg per the existing welcome.ts precedent)."
|
|
|
|
**Empirical reality** (verified during this check):
|
|
|
|
- `src/shared/brand/mokosh-mark.svg` is **877 bytes** (below Vite's default `assetsInlineLimit: 4096`).
|
|
- The actual build output at `dist/assets/welcome-BXz6I5Ha.js` contains the SVG inlined as `data:image/svg+xml,%3csvg%20xmlns=...` URI — NOT as a separate `dist/assets/<hash>.svg` file.
|
|
- The current `dist/manifest.json` web_accessible_resources block contains ONLY `src/welcome/welcome.html` + 3 hashed JS chunks — NO SVG entry. The crxjs plugin did NOT auto-WAR the SVG because the SVG was never emitted as a file.
|
|
- `tests/fixtures/last_30sec.webm` is **1888636 bytes (1.9 MB)** — three orders of magnitude above the inline limit. Vite WILL emit it as a separate `dist-test/assets/<hash>.webm` file.
|
|
|
|
**Why this is a BLOCKER:**
|
|
|
|
The Plan 01-10 SVG precedent is fundamentally a different code path. The 1.9 MB WebM follows the *extracted-asset* path which has NOT been exercised in this codebase. Three specific unknowns:
|
|
|
|
1. **WAR auto-generation behavior for extracted assets is unverified.** `@crxjs/vite-plugin` is documented to auto-WAR assets transitively reachable from extension pages, but the offscreen document is NOT a typical extension page (it has its own document context loaded via `chrome.offscreen.createDocument`). The current `dist-test/manifest.json` is IDENTICAL to `dist/manifest.json` (both list the same WAR entries), suggesting the test-bundle's offscreen-hooks.ts module currently does not import any `?url` asset — Plan 04-08 would be the first test of this code path.
|
|
2. **`chrome-extension://<id>/assets/<hash>.webm` accessibility from the offscreen document via `<video>.src` is unverified.** Even with full extension privileges, an HTMLVideoElement loading a chrome-extension:// URL has Chrome-internal CORS + scheme rules. Operator-acceptance evidence in the codebase shows the offscreen has loaded chrome-extension:// resources for its own JS chunks, but never a binary media asset for `<video>.src`.
|
|
3. **Plan 04-08's contingency block is informal.** Step 5 says "Decision (executor verifies during Step 6 verify): if `@crxjs/vite-plugin` auto-WARs the asset (high confidence per Plan 01-10 precedent + Plan 01-12 RESEARCH §155), NO manifest.json edit is required. If WAR auto-generation fails ... add an explicit entry." This delegates a likely-failing path to executor improvisation, violating the spike-first contract that informed Plan 04-04 — the executor would be making an architectural decision under pressure.
|
|
|
|
**Remediation for iter-2** (concrete options):
|
|
|
|
- **Option A (preferred):** Pre-decide the WAR strategy. Spawn a one-shot probe BEFORE Task 1 lands the canvas-replacement code: write a 10-line synthetic offscreen-hooks edit that imports a tiny `tests/fixtures/probe.webm?url` (~ a few hundred bytes via ffmpeg `-c:v libvpx-vp9 -frames:v 1`), run `npm run build:test`, inspect `dist-test/manifest.json` for the WAR entry, then revert. Lock the empirical answer into Plan 04-08 BEFORE the methodology replacement.
|
|
- **Option B:** Pre-emptively add the explicit WAR entry in `manifest.json`:
|
|
```json
|
|
{
|
|
"matches": ["<all_urls>"],
|
|
"resources": ["assets/*.webm"]
|
|
}
|
|
```
|
|
Cost: adds an entry to the production manifest, even though the `*.webm` is test-only. But because `dist/` for production has zero webm assets (only `dist-test/` does), the entry would be inert in production — costs nothing at runtime, costs only the diff in the production manifest.
|
|
- **Option C:** Sidestep entirely by reading the fixture as a `?raw` ArrayBuffer or base64 string (NOT a URL) and constructing a Blob URL inside the offscreen at module load:
|
|
```typescript
|
|
import webmBase64 from '../../tests/uat/fixtures/synthetic-display-source.webm?raw'; // or ?inline
|
|
const webmBlob = new Blob([Uint8Array.from(atob(webmBase64), c => c.charCodeAt(0))], { type: 'video/webm' });
|
|
const videoEl = document.createElement('video');
|
|
videoEl.src = URL.createObjectURL(webmBlob);
|
|
```
|
|
This bypasses the chrome-extension://-scheme question entirely (blob: URLs work universally in same-origin contexts). Cost: ~2.5 MB string bloat in the dist-test bundle (1.9 MB base64-encoded ≈ +33%); tree-shaken in production via `__MOKOSH_UAT__`. The Plan 02-02 D-P2-01 lifecycle precedent already establishes the "Blob URL minted in offscreen" pattern.
|
|
|
|
**Recommended path:** Option A (probe-then-decide) OR Option C (sidestep). Option B works but increases the production manifest's surface area slightly.
|
|
|
|
---
|
|
|
|
#### BLOCKER 2 — `installFakeDisplayMedia()` async conversion is silently incompatible with the eager-install-at-module-load contract
|
|
|
|
**Dimension:** task_completeness / key_links_planned
|
|
|
|
**Plan claim** (lines 471-472 + 502-517):
|
|
> "Note: `installFakeDisplayMedia()` becomes async (it now `await`s the canplay event + .play()). The existing module-load eager call at the bottom of the file (lines 533-537) wraps in try/catch — update to either: (a) `void installFakeDisplayMedia().catch(...)` OR (b) leave the try/catch but accept that the wrapper now logs the rejection. Recommendation: option (a) for explicit fire-and-forget semantics; the bridge op `install-fake-display-media` (line 408-418) already awaits-and-responds properly."
|
|
|
|
**Empirical reality** (verified during this check):
|
|
|
|
- `src/test-hooks/offscreen-hooks.ts` lines 528-537 currently calls `installFakeDisplayMedia()` **eagerly** at module load (synchronous void call).
|
|
- `src/offscreen/recorder.ts:46-48` imports the test-hooks module via **top-level await**:
|
|
```typescript
|
|
if (__MOKOSH_UAT__) {
|
|
testHooks = await import('../test-hooks/offscreen-hooks');
|
|
}
|
|
```
|
|
- The current contract is: *by the time `recorder.ts:48` resolves, `navigator.mediaDevices.getDisplayMedia` is ALREADY monkey-patched on the offscreen document's `navigator` object.* This means when `recorder.ts:startRecording` is invoked later, `await navigator.mediaDevices.getDisplayMedia(...)` resolves with the synthetic stream immediately.
|
|
|
|
**Why the plan's recommended option (a) breaks this contract:**
|
|
|
|
If `installFakeDisplayMedia()` becomes async AND the module-load call switches to `void installFakeDisplayMedia().catch(...)` — fire-and-forget — there is a race window:
|
|
|
|
1. `recorder.ts:48` resolves (test-hooks dynamic import done).
|
|
2. `recorder.ts` continues to `bootstrap()` + later `startRecording()`.
|
|
3. The async `installFakeDisplayMedia()` is still awaiting `canplay` + `videoEl.play()` (potentially hundreds of ms on cold load).
|
|
4. `recorder.ts:startRecording` calls `await navigator.mediaDevices.getDisplayMedia(...)`. Because installFakeDisplayMedia hasn't yet executed line 261 (`navigator.mediaDevices.getDisplayMedia = fakeGetDisplayMedia`), the REAL `navigator.mediaDevices.getDisplayMedia` is invoked → Chrome screen-share picker pops → in headless mode this hangs forever OR rejects with NotAllowedError.
|
|
|
|
This race is structurally identical to the chicken-and-egg condition that the current eager-sync install was DESIGNED to prevent — and the in-line code comment at lines 530-532 documents this: "PROTOTYPE: install the fake getDisplayMedia eagerly so production recorder.startRecording will use the synthetic stream on its first call — **no chicken-and-egg with the bridge install op**."
|
|
|
|
**The plan's option (b)** (leave try/catch, accept rejection logging) keeps the surface signature the same but doesn't address the underlying race either — the eager call would resolve synchronously to a Promise that's still pending; the `try` block exits without waiting; the same race happens.
|
|
|
|
**Why this is a BLOCKER:**
|
|
|
|
This is the kind of subtle ordering bug that produces "spike re-run still produces small-byte WebM" symptoms WITHOUT being a methodology regression — it would falsely re-confirm the canvas-throttling hypothesis in iter-2 because the fake stream isn't installed in time and the recorder falls back to whatever happens (in headless: hang, then timeout, then empty buffer). This would consume an entire spike re-run (~6-7 min wall-clock + executor cycle) to surface, by which point context budget is degraded.
|
|
|
|
**Remediation for iter-2** (concrete options):
|
|
|
|
- **Option A (preferred):** Move the heavy async work (canplay wait + play()) into a NEW async wrapper, but keep the synchronous part (videoEl creation + DOM append + navigator.mediaDevices monkey-patch) eager. The mintStream factory becomes a lazy capture-stream that returns a stream whose first-frame readiness is awaited by the recorder if needed. Specifically:
|
|
```typescript
|
|
export function installFakeDisplayMedia(): void { // KEEP SYNC
|
|
if (fakeInstalled) return;
|
|
fakeInstalled = true;
|
|
// ... create videoEl + appendChild (sync) ...
|
|
// ... start play (kicked off; returns Promise; NOT awaited here) ...
|
|
void videoEl.play();
|
|
// Monkey-patch IMMEDIATELY (sync) — the fakeGetDisplayMedia closure
|
|
// awaits canplay inside itself, not at install time.
|
|
const fakeGetDisplayMedia = async (constraints?: DisplayMediaStreamOptions): Promise<MediaStream> => {
|
|
lastGetDisplayMediaConstraints = constraints ?? null;
|
|
// Wait for videoEl readiness ON THE CALL, not at install.
|
|
if (videoEl.readyState < 3) { // HAVE_FUTURE_DATA
|
|
await new Promise<void>((resolve, reject) => { /* canplay/error */ });
|
|
}
|
|
return mintStream();
|
|
};
|
|
(navigator.mediaDevices as ...).getDisplayMedia = fakeGetDisplayMedia;
|
|
}
|
|
```
|
|
Cost: 1 extra await inside fakeGetDisplayMedia; runs only on first call (cached readyState afterwards). Preserves the eager-monkey-patch contract.
|
|
- **Option B:** Make the module load itself synchronously block on the async install via top-level await in offscreen-hooks.ts (since the module is already loaded under top-level-await context per recorder.ts:47). I.e., flip the module-load call from `installFakeDisplayMedia()` (sync void) to `await installFakeDisplayMedia();` at line 534. This propagates the wait correctly through recorder.ts:48 — recorder.ts:bootstrap doesn't run until canplay + play resolve. Cost: ~100-500ms added to offscreen document init wall-clock. Plan must explicitly state this and acknowledge it.
|
|
- **Option C:** Add a `installPromise` module-level cell that the bridge handler can await. Make the bridge `install-fake-display-media` op publicly awaitable; have the recorder's startRecording explicitly await the install promise before calling getDisplayMedia. Cost: ~3 LOC change to recorder.ts (slight invariant shift — recorder.ts now knows about test-hooks install state).
|
|
|
|
**Recommended path:** Option A (lazy first-frame wait inside fakeGetDisplayMedia) — most surgical, preserves the existing contract, no recorder.ts changes needed.
|
|
|
|
---
|
|
|
|
### WARNING
|
|
|
|
#### WARNING 1 — `<video>` autoplay reliability in headless Chrome offscreen documents is unverified
|
|
|
|
**Dimension:** task_completeness
|
|
|
|
The plan relies on `videoEl.muted = true; videoEl.autoplay = true; await videoEl.play();` for headless playback. Chrome's autoplay policy (https://developer.chrome.com/blog/autoplay) allows muted-autoplay generally, BUT offscreen documents are a non-standard execution context (no visible viewport, no user gesture path). The plan's claim "Headless Chrome reliably honors muted-autoplay but the explicit call removes any policy edge" (line 471) is plausible but unsupported by any in-tree precedent — there are zero `<video>` elements in the existing codebase's test bundle.
|
|
|
|
**Remediation:** Iter-2 should add an explicit fallback: if `videoEl.play()` rejects with `NotAllowedError`, the plan should specify a documented recovery (e.g., dispatch a synthetic user-activation via Puppeteer CDP `Input.dispatchKeyEvent`, OR fall back to a frame-buffer canvas refresh driven by `requestVideoFrameCallback` on the videoEl). Currently the plan throws and dies — no Plan B documented.
|
|
|
|
#### WARNING 2 — `displaySurface` monkey-patch portability to HTMLVideoElement track is unverified
|
|
|
|
**Dimension:** task_completeness / key_links_planned
|
|
|
|
The existing `patchDisplaySurface(stream)` helper at offscreen-hooks.ts:210-222 wraps `videoTrack.getSettings()` to inject `displaySurface: 'monitor'`. The current implementation works for canvas-captureStream tracks because those tracks expose a writable `getSettings` reference. HTMLVideoElement.captureStream may produce tracks with a different prototype chain — specifically, the MediaStreamTrack returned by HTMLMediaElement.captureStream is sometimes a `CanvasCaptureMediaStreamTrack`-equivalent but more often a regular `MediaStreamTrack` with sealed property descriptors. The plan claims "patchDisplaySurface(stream) call is preserved" (must_haves.artifacts line 57) but does not verify the monkey-patch survives.
|
|
|
|
**Remediation:** Iter-2 should add a Task 1 read_first check + verify step that runs the monkey-patch end-to-end (call mintStream + `track.getSettings().displaySurface === 'monitor'`) before the spike re-run. If the patch fails, the production code's post-grant `displaySurface !== 'monitor'` gate at `src/offscreen/recorder.ts:294` will tear down the stream + throw `'wrong-display-surface'` — the spike would fail at Step 2 (assertA2 prime), not at Step 7 (videoSize check).
|
|
|
|
#### WARNING 3 — Plan 04-04's spike script (`tests/uat/spike-a33-sw-persistence.ts`) is referenced as a re-run target but its expected behavior under the new methodology is informally specified
|
|
|
|
**Dimension:** verification_derivation
|
|
|
|
Plan 04-08 Task 2 Step 1 says: "Run: `HEADLESS=1 SKIP_PROD_REBUILD=0 npx tsx tests/uat/spike-a33-sw-persistence.ts 2>&1 | tee /tmp/04-08-spike-rerun.log`" with expected outcome `SPIKE OUTCOME: PASSED` + `videoSize > 100_000`. But Step 2 also says "the spike already uses `__mokoshHarness.assertA2` for the prime step. No edit needed — but verify the script's PROBE 1 (POST-PRIME) returns count=0 AND PROBE 2 (PRE-KILL after 5min idle) returns count=3 AND PROBE 3 (POST-KILL) returns count=3." (line 603).
|
|
|
|
These three checks are correct and meaningful — they distinguish "frames present + persistence works" (success) from "frames present but lost" (would re-confirm architecture issue) from "frames absent" (would re-confirm canvas issue). But they are buried in prose, not surfaced as explicit `<verify>` automated commands. If iter-2 sets these as explicit grep checks against the spike log, the plan checker can auto-verify them in Plan 04-08-SUMMARY without an executor judgment call.
|
|
|
|
**Remediation:** Iter-2 should add the three probe-value asserts as explicit grep gates in Task 2's `<verify>` block:
|
|
```bash
|
|
grep -E 'SPIKE PROBE \[POST-PRIME\]: segments\.length=0' /tmp/04-08-spike-rerun.log
|
|
grep -E 'SPIKE PROBE \[PRE-KILL\]: segments\.length=(3|[3-9]|[1-9][0-9]+)' /tmp/04-08-spike-rerun.log
|
|
grep -E 'SPIKE PROBE \[POST-KILL\]: segments\.length=(3|[3-9]|[1-9][0-9]+)' /tmp/04-08-spike-rerun.log
|
|
```
|
|
|
|
#### WARNING 4 — Task 2 Step 7 ROADMAP.md flip is described inline without enforcement gate
|
|
|
|
**Dimension:** task_completeness
|
|
|
|
Plan 04-08 Task 2 Step 7 says (line 698): "ROADMAP.md: flip Phase 4 SC #1 row from 'STATUS 2026-05-21: OPEN' to 'STATUS 2026-05-22: CLOSED via Plan 04-08 — see .planning/phases/04-harden-clean-up-optional/04-08-SUMMARY.md'."
|
|
|
|
This is a critical state transition. The current ROADMAP.md (which I read at line 35-38) does NOT contain a "STATUS 2026-05-21: OPEN" row for Phase 4 SC #1 in machine-greppable form — it's prose. The executor would need to interpret where to add the row, increasing the risk of an ambiguous edit. The plan's `<verify>` block at line 705 does not enforce a post-edit grep gate on ROADMAP.md to confirm the flip landed.
|
|
|
|
**Remediation:** Iter-2 should specify the EXACT pre-edit line(s) to find + replace in ROADMAP.md (or document that the flip is a NEW row insertion), plus a `<verify>` grep gate like `grep -c 'CLOSED via Plan 04-08' .planning/ROADMAP.md` returns ≥ 1.
|
|
|
|
#### WARNING 5 — Plan 04-08 introduces no new FORBIDDEN_HOOK_STRINGS entries, but `synthetic-display-source` string appears in the test bundle without inventory acknowledgment
|
|
|
|
**Dimension:** context_compliance / scope_sanity
|
|
|
|
The plan correctly asserts (must_haves.truths line 49): "Tier-1 FORBIDDEN_HOOK_STRINGS inventory unchanged at 12 entries ... Plan 04-08 introduces NO new __MOKOSH_UAT__-gated symbols on production bundle." This is correct in the sense that no `__mokoshTest`-style symbol is introduced.
|
|
|
|
BUT the WebM filename (`synthetic-display-source.webm`) will appear in the test bundle as the resolved Vite hash URL — a new identifying string. The Tier-1 grep at `tests/uat/harness.test.ts:252` walks `dist/` (the *production* bundle), so a test-only string is correctly out of scope. However, the plan's Step 7 verify (line 559-560) says: `grep -c "synthetic-display-source" dist/ -r 2>/dev/null | grep -v ":0$" | wc -l | awk '$1 == 0 {exit 0} {exit 1}'` (proves zero hits in production dist/). This is correct but the test-bundle situation is not gated. If a future refactor accidentally inlines test-hooks into the production chunk, this filename leak would be the canary — but there's no test for it.
|
|
|
|
**Remediation:** Iter-2 OR a future plan should consider adding `synthetic-display-source` to a new Tier-2 inventory (test-bundle string blacklist for the production bundle). Not a blocker for Plan 04-08 — it inherits the existing Tier-1 invariant — but worth noting for downstream Plan 04-07 closure aggregator.
|
|
|
|
---
|
|
|
|
### cosmetic-advisory
|
|
|
|
#### advisory 1 — Plan references "Task 3 artifacts" in commit message but only has 2 tasks
|
|
|
|
**Plan line 702:** "Single atomic commit with all Task 2 + Task 3 artifacts: `feat(04-08): A33 SW state persistence harness assertion — methodology reframe (34/34 GREEN; ROADMAP SC #1 CLOSED)`."
|
|
|
|
But the plan only defines Task 1 + Task 2 — no Task 3. The commit message text is fine; the in-plan reference is a documentation slip. Cosmetic — does not affect execution.
|
|
|
|
#### advisory 2 — `architectural integrity preserved` truth references segments.length in must_haves but doesn't grep-pin the invariant in `<verify>`
|
|
|
|
**Plan line 51:** Truth #11 says `src/offscreen/recorder.ts:91 let segments: Blob[] = []` UNCHANGED. The verify block has no explicit gate like `grep -c '^let segments: Blob\[\] = \[\];' src/offscreen/recorder.ts` returns 1. The invariant is asserted but not enforced. Cosmetic because the plan makes no edits to recorder.ts in either task; accidental violation is unlikely.
|
|
|
|
**Remediation if desired:** Add to Task 1 `<verify>`: `grep -cE 'let segments: Blob\[\] = \[\];' src/offscreen/recorder.ts` ≥ 1.
|
|
|
|
#### advisory 3 — Plan does not specify what happens to the original `tests/fixtures/last_30sec.webm` source
|
|
|
|
**Plan line 386-388** describes copying `tests/fixtures/last_30sec.webm` → `tests/uat/fixtures/synthetic-display-source.webm`. The original file remains in place (`tests/fixtures/` is the historical regression fixture from Plan 01-07). This is correct, but the plan doesn't acknowledge the dual-location situation. Cosmetic.
|
|
|
|
**Remediation if desired:** Add a one-liner note in Task 1 Step 1: "tests/fixtures/last_30sec.webm remains in place (Plan 01-07 regression fixture; not affected)."
|
|
|
|
---
|
|
|
|
## Dimensional Pass/Fail Summary
|
|
|
|
| Dimension | Status | Notes |
|
|
|-----------|--------|-------|
|
|
| 1. Requirement Coverage | PASS | Phase requirements explicitly reference D-P4-01 (Full Phase 4 — ROADMAP SC #1) + RESEARCH Q2; tasks address SC #1 directly. |
|
|
| 2. Task Completeness | WARN | Task 1 + Task 2 both have all 4 elements; BLOCKER 2 + WARNING 1/2/4 surface specific incomplete-action sub-cases. |
|
|
| 3. Dependency Correctness | PASS | depends_on [01,02,03,04,05,06] is acyclic + correct (Wave 5.5 after Wave 5). |
|
|
| 4. Key Links Planned | FAIL | BLOCKER 1 — Vite `?url` → offscreen `<video>.src` chrome-extension:// access link is unverified. |
|
|
| 5. Scope Sanity | PASS | 2 tasks across 10 files_modified; well within 2-3 task / 5-8 file target despite the methodology breadth. |
|
|
| 6. Verification Derivation | WARN | must_haves are user-observable; WARNING 3 — spike probe values not surfaced as automated gates. |
|
|
| 7. Context Compliance (D-P4-01..05) | PASS | Honors D-P4-01 (full scope); does not contradict locked decisions; deferred IDB persistence work correctly excluded. |
|
|
| 7b. Scope Reduction Detection | PASS | No "v1"/"static for now"/"future enhancement" language; plan delivers SC #1 fully. |
|
|
| 7c. Architectural Tier Compliance | PASS | Test-only hook + bundled test asset are correctly in the test-hooks / fixture tiers per RESEARCH §"Architectural Responsibility Map". |
|
|
| 8. Nyquist Compliance | PASS | Both tasks have `<automated>` verify commands; Task 1 build-test verify + Task 2 UAT skip-mode verify; no watch-mode; sampling adequate. |
|
|
| 9. Cross-Plan Data Contracts | PASS | No competing transforms on shared data; offscreen RAM buffer pipeline unchanged. |
|
|
| 10. CLAUDE.md Compliance | N/A | No project CLAUDE.md present (verified via Read tool — file does not exist). Skip per protocol. |
|
|
| 11. Research Resolution | PASS | 04-RESEARCH.md Q2 was resolved + superseded by debug session-2; plan explicitly references the supersession. |
|
|
| 12. Pattern Compliance | WARN | Plan claims Plan 01-10 mokosh-mark.svg as direct analog — BLOCKER 1 surfaces the analog is NOT direct (877-byte inline vs 1.9 MB extract). |
|
|
|
|
---
|
|
|
|
## Recommendation for Orchestrator
|
|
|
|
**Verdict: ITERATE-NEEDED.**
|
|
|
|
Spawn planner iter-2 with the following concrete remediation directives:
|
|
|
|
1. **BLOCKER 1** — resolve the `?url` asset-emission path. Preferred: Option A (probe-then-decide spike) or Option C (Blob URL from `?raw` ArrayBuffer). Document the chosen path in plan + add an explicit `<verify>` gate that the resolved URL is reachable from the offscreen document.
|
|
2. **BLOCKER 2** — preserve the eager-install contract. Preferred: Option A (lazy first-frame wait inside fakeGetDisplayMedia closure). Task 1 Step 3 needs a re-draft that splits installFakeDisplayMedia into sync-monkey-patch + lazy-await sub-paths.
|
|
3. **WARNING 1** — document the headless autoplay fallback path in Task 1.
|
|
4. **WARNING 2** — add a Task 1 read_first + sub-verify for the patchDisplaySurface compatibility check.
|
|
5. **WARNING 3** — surface the 3 spike probe-value asserts as explicit grep gates in Task 2 `<verify>`.
|
|
6. **WARNING 4** — pre-specify the ROADMAP.md edit and add a grep gate for the flip.
|
|
7. Optionally address advisories 1-3 (cosmetic).
|
|
|
|
**Estimated iter-2 scope:** ~150-300 lines of plan edits (Task 1 Step 3 re-draft + Task 1 verify expansion + Task 2 verify expansion). Single iter-2 cycle should suffice if the planner addresses the BLOCKERs directly.
|
|
|
|
**Architectural alignment confirmed:** The plan's core thesis (HTMLVideoElement.captureStream bypasses canvas-throttling per debug session-2) is correct. iter-2 is a methodology-tightening pass, not a re-architecture pass.
|
|
|
|
---
|
|
|
|
*Plan-checker iter-1 completed 2026-05-22 by gsd-plan-checker.*
|
|
*Output committed atomically; orchestrator decides routing (planner iter-2 OR proceed with caveats acknowledged).*
|