.planning/debug/01-09-save-stops-recording.md

@@ -1,6 +1,6 @@
 ---
 slug: 01-09-save-stops-recording
-status: fixing
+status: awaiting_human_verify
 goal: find_and_fix
 trigger: Plan 01-13 Task 9 operator empirical UAT — clicking SAVE downloads archive but recording continues; toolbar press re-opens SAVE-only popup; operator confusion (4× toolbar presses → 2 zips downloaded, 1 misclick)
 tdd_mode: true
@@ -8,6 +8,9 @@ phase: 01-stabilize-video-pipeline
 plan: 01-09
 opened: 2026-05-19
 orchestrator_diagnosed: true
+red_commit: cd83eb0
+green_commit: 4f4c3e2
+a14_commit: 2b6c24b
 ---
 
 # Debug session 01-09-save-stops-recording — SAVE_ARCHIVE must auto-stop recording (SPEC one-shot intent)
@@ -129,13 +132,90 @@ atomically (no state-machine path desyncs them).
 Note: STOP_RECORDING is already in `MessageType` (src/shared/types.ts:14)
 — no type-system surface change needed.
 
-## RED → GREEN evidence (to be filled in)
+## RED → GREEN evidence
 
-- RED commit SHA: TBD
+- RED commit SHA: `cd83eb0` (4 tests, 3 RED + 1 regression-guard GREEN)
-- GREEN commit SHA: TBD
+- GREEN commit SHA: `4f4c3e2` (`src/background/index.ts` saveArchive
-- A14 commit SHA: TBD
+  finally block)
-- vitest count (target): >= 94 + 4 (new tests) = >= 98 GREEN
+- A14 commit SHA: `2b6c24b` (harness extension + A13 amendment)
-- `npm run test:uat` (target): 15/15 GREEN (was 14/14)
+- vitest count: **98/98 GREEN** (was 94/94 baseline; +4 from
+  `tests/background/save-archive-stops-recording.test.ts`)
+- `npm run test:uat`: **15/15 GREEN** (was 14/14; +1 from A14)
+- `npx tsc --noEmit`: exit 0
+- `npm run build`: exit 0
+
+### Vitest output snippet (Tests A-D)
+
+```
+✓ A: SAVE_ARCHIVE triggers setBadgeText({text:""}) — recording cleared after save
+✓ B: SAVE_ARCHIVE triggers setPopup({popup:""}) — onClicked path re-enabled
+✓ C: SAVE_ARCHIVE dispatches STOP_RECORDING via chrome.runtime.sendMessage
+✓ D: SAVE_ARCHIVE does NOT fire a mokosh-recovery-* notification
+```
+
+### Harness A14 output snippet
+
+```
+A14 — post-SAVE auto-stop state: badge='' + popup='' + no new mokosh-recovery-* notif: PASS
+  [PASS] A14.1: badge text is '' after SAVE_ARCHIVE auto-stop (setIdleMode)
+  [PASS] A14.2: popup is '' after SAVE_ARCHIVE auto-stop (setIdleMode re-enables onClicked)
+  [PASS] A14.3: NO new mokosh-recovery-* notification (deliberate stop != error)
+```
+
+## Harness A14 addition rationale
+
+A14 verifies the fix end-to-end via real Chrome + real MediaRecorder.
+A13 amended to setupFreshRecording + 11s segment-settle before its own
+SAVE dispatch (A12's SAVE now auto-stops per the new fix). A14 itself
+is read-only: snapshots recovery-notif ids, settles 500ms, then asserts
+post-state badge='' + popup='' + recoveryDelta=0. No new SAVE dispatch
+in A14 — chains off A13's SAVE for the event under test. Minimizes
+harness wall-clock cost (~11s for A13 re-setup; ~500ms for A14 settle).
+
+Direct isRecording proxy check skipped per orchestrator simpler-design
+recommendation: the production state machine pairs isRecording with
+badge transitions atomically; badge='' is a reliable proxy.
+
+## Noteworthy
+
+- **No PortMessage / Message type changes were needed.** `STOP_RECORDING`
+  was already in `src/shared/types.ts:14` MessageType union (from Plan
+  01-05's offscreen rebuild). The offscreen `STOP_RECORDING` case at
+  `src/offscreen/recorder.ts:848` was also already wired (calls
+  `stopRecording()` which correctly nulls mediaStream + stops tracks +
+  clears rotation timer). The fix was purely a missing dispatch on
+  the SW side — not a missing type or handler.
+
+- **`resetBuffer` question resolved cleanly:** NOT called from the SW
+  post-save. Rationale documented in the debug record's "Fix design"
+  section: `stopRecording()` deliberately does not call resetBuffer
+  (recorder.ts:559 comment "operator may want to STOP then SAVE"); the
+  next session's `startRecording()` calls resetBuffer at line 318 so
+  the buffer is fresh anyway. Calling it from the SW would be
+  defensive overkill.
+
+- **Empty-buffer-path trade-off documented inline in src/background/index.ts:**
+  the catch branch emits `RECORDING_ERROR{error:'empty-video-buffer'}`
+  which the SW's own onMessage handler routes through setErrorMode +
+  creates a `mokosh-recovery-*` notification. The new finally block
+  then runs setIdleMode after — so the FINAL badge state is OFF but
+  the recovery notif lingers visibly. The unit test mock for
+  `chrome.runtime.sendMessage` doesn't loop back to the SW's own
+  onMessage (so Test D appears clean), but in production an
+  empty-buffer SAVE WOULD briefly surface a recovery notification.
+  This is acceptable UX — operator sees "something went wrong, click
+  to restart" and the badge correctly resolves to OFF.
+
+## Follow-ups
+
+- Orchestrator runs pre-checkpoint bundle gates (SW CSP-safety, Node-globals
+  in SW chunk, DOM-globals not in SW chunk, Tier-1 hook-string grep over
+  dist/) BEFORE re-surfacing operator UAT. Per
+  `feedback-pre-checkpoint-bundle-gates.md` operator time = automation
+  cannot verify.
+- Operator UAT after this lands should confirm: SAVE click → zip lands +
+  badge clears + sharing banner closes + popup empties + subsequent
+  toolbar click starts a NEW recording session (clean state machine).
 
 ## Follow-ups