Fix RecordingManager: stale recorder cleanup + forceStop status ordering#8688
Merged
Jasonnnz merged 1 commit intoFeb 25, 2026
Merged
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20169c715b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
Author
|
Addressed in #8691 |
Jasonnnz
added a commit
that referenced
this pull request
Feb 25, 2026
* feat: add standalone screen recording core, UI, and IPC types Co-Authored-By: Claude <noreply@anthropic.com> * fix: address M1 review feedback — synchronous forceStop, start race guard, async HUD, picker close (#8686) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: stop stale recorder on start guard, fix forceStop status ordering (#8688) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: gate stale-start recorder cancel to avoid cross-session teardown (#8691) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: only skip stale recorder cancel when another session is confirmed recording (#8692) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: revert stale-start cancel to !state.isActive — state != .recording was a regression (#8694) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com>
Jasonnnz
added a commit
that referenced
this pull request
Feb 25, 2026
* feat: add standalone screen recording core, UI, and IPC types Co-Authored-By: Claude <noreply@anthropic.com> * fix: address M1 review feedback — synchronous forceStop, start race guard, async HUD, picker close (#8686) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: stop stale recorder on start guard, fix forceStop status ordering (#8688) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: gate stale-start recorder cancel to avoid cross-session teardown (#8691) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: only skip stale recorder cancel when another session is confirmed recording (#8692) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: revert stale-start cancel to !state.isActive — state != .recording was a regression (#8694) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com>
Jasonnnz
added a commit
that referenced
this pull request
Feb 25, 2026
* feat: add standalone screen recording core, UI, and IPC types Co-Authored-By: Claude <noreply@anthropic.com> * fix: address M1 review feedback — synchronous forceStop, start race guard, async HUD, picker close (#8686) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: stop stale recorder on start guard, fix forceStop status ordering (#8688) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: gate stale-start recorder cancel to avoid cross-session teardown (#8691) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: only skip stale recorder cancel when another session is confirmed recording (#8692) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: revert stale-start cancel to !state.isActive — state != .recording was a regression (#8694) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com>
Jasonnnz
added a commit
that referenced
this pull request
Feb 25, 2026
* feat: add standalone screen recording core, UI, and IPC types Co-Authored-By: Claude <noreply@anthropic.com> * fix: address M1 review feedback — synchronous forceStop, start race guard, async HUD, picker close (#8686) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: stop stale recorder on start guard, fix forceStop status ordering (#8688) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: gate stale-start recorder cancel to avoid cross-session teardown (#8691) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: only skip stale recorder cancel when another session is confirmed recording (#8692) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: revert stale-start cancel to !state.isActive — state != .recording was a regression (#8694) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com>
Jasonnnz
added a commit
that referenced
this pull request
Feb 25, 2026
* M1: Recording Core + UI (Routing Off) (#8684) * feat: add standalone screen recording core, UI, and IPC types Co-Authored-By: Claude <noreply@anthropic.com> * fix: address M1 review feedback — synchronous forceStop, start race guard, async HUD, picker close (#8686) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: stop stale recorder on start guard, fix forceStop status ordering (#8688) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: gate stale-start recorder cancel to avoid cross-session teardown (#8691) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: only skip stale recorder cancel when another session is confirmed recording (#8692) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: revert stale-start cancel to !state.isActive — state != .recording was a regression (#8694) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * M2: Daemon Recording Control + Deterministic Mapping (#8696) * feat: add daemon recording start/stop handlers with deterministic mapping Co-Authored-By: Claude <noreply@anthropic.com> * fix: prevent map leaks in recording handlers — duplicate start + missing socket (#8697) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * M3: Intent Routing + Strict Classification (Enable Flag) (#8699) * feat: add recording intent detection and standalone routing with feature flag Co-Authored-By: Claude <noreply@anthropic.com> * fix: address M3 PR #8699 review feedback (#8700) * fix: add isStopRecordingOnly guard, sessionId in message_complete, check stop return value Co-Authored-By: Claude <noreply@anthropic.com> * fix: handle polite filler words in isStopRecordingOnly and isRecordingOnly (#8701) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * M4: Finalization + Attachment Delivery (#8703) * feat: add recording finalization and attachment delivery for standalone recordings Co-Authored-By: Claude <noreply@anthropic.com> * fix: address M4 PR #8703 review feedback (#8709) * fix: use file-backed attachment storage, include attachment in message_complete, notify on failure Co-Authored-By: Claude <noreply@anthropic.com> * fix: make file-backed attachments retrievable (#8713) * fix: add file-backed attachment retrieval for recordings Co-Authored-By: Claude <noreply@anthropic.com> * fix: validate Range header bounds and handle suffix ranges (#8716) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * M5: Bundled Recording Skill (Activation Rules Only) (#8717) * feat: add bundled screen-recording skill with activation rules Co-Authored-By: Claude <noreply@anthropic.com> * fix: add macOS OS gate to screen-recording skill (#8718) * fix: restrict screen-recording skill to macOS via os metadata Co-Authored-By: Claude <noreply@anthropic.com> * fix: move os gate inside vellum metadata namespace (#8722) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * M6: Tests + Docs for standalone screen recording (#8727) * feat: add recording intent and handler tests Add comprehensive tests for recording-intent.ts (all 6 exported functions: detectRecordingIntent, isRecordingOnly, detectStopRecordingIntent, stripRecordingIntent, stripStopRecordingIntent, isStopRecordingOnly) and recording handler (start/stop/status flows with proper mocking). Co-Authored-By: Claude <noreply@anthropic.com> * docs: add standalone screen recording section to ARCHITECTURE.md Document the standalone recording lifecycle, key files, IPC messages, intent routing, file-backed attachments, and include a Mermaid sequence diagram showing the recording flow. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: prevent recording ownership overwrite and ensure sessionId on stop (#8828) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: move duplicate-start messaging to callers, return null from handleRecordingStart (#8829) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: type narrowing for nullable handleRecordingStart + add task_routed to else branch (#8830) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: add task_routed to stop-recording path in handleTaskSubmit (#8831) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: add global single-active recording guard and sessionId to stop-recording text delta (#8834) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: resolve handleRecordingStop globally for cross-conversation stop (#8836) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: always create new assistant message for recordings + notify on missing file (#8837) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * refactor: move Recording files and use raw content endpoint - Move Recording*.swift and ScreenRecorder.swift from ComputerUse/ to Recording/ - Switch InlineVideoAttachmentView to fetch raw bytes from /content endpoint instead of base64-decoding JSON, removing intermediate decode step - Stop excluding own app windows from screen capture filter Co-Authored-By: Claude <noreply@anthropic.com> * fix: fetch thumbnail for file-backed attachments - Extract thumbnail from content endpoint for lazy-load attachments that have no inline base64 data Co-Authored-By: Claude <noreply@anthropic.com> * Add microphone capture support to screen recording - Add includeMicrophone option to IPC RecordingOptions contract - Regenerate IPCContractGenerated.swift with new field - Add microphone toggle to RecordingSourcePickerView - Pass includeMicrophone through RecordingManager to ScreenRecorder - Add separate AAC mic track in AVAssetWriter (mono, 64kbps) - Register SCStream microphone output and handle in sample buffer - Check/request microphone permission before recording starts - Gate microphone APIs behind macOS 15 availability checks Co-Authored-By: Claude <noreply@anthropic.com> * fix: address PR review feedback for recording stop edge cases 1. misc.ts: Always call handleRecordingStop even when socket has no prior session binding. Previously, "stop recording" from task_submit with no prior session would skip the stop attempt entirely. 2. recording.ts: Don't clean up deterministic maps when no socket is found. Cleaning up orphaned the client-side recording (still running) while the daemon thought no recording was active, blocking future starts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: harden recording security and cleanup on disconnect 1. Require tracked recording IDs in recording_status — reject unknown session IDs to prevent forged status messages from attaching arbitrary files via the finalization path. 2. Restrict accepted file paths to the recordings directory to prevent path traversal attacks in crafted IPC messages. 3. Clean up recording state on socket disconnect to prevent stale entries from blocking future recordings when a client crashes or disconnects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: clean up recording maps on path rejection and support daemon restart - Extract cleanupMaps() helper to ensure map cleanup runs on all exit paths, including when path validation rejects a file outside the allowed recordings directory (previously skipped by break) - Fall back to attachToConversationId when in-memory maps are missing after daemon restart, preserving recording finalization across restarts - Remove dead else-if branch that was unreachable after early return Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: clean recording state by socket, not session ID, and disable mic on denied - Replace cleanupRecordingForSession(sessionId) with cleanupRecordingsOnDisconnect() which iterates all recording entries instead of a single session ID. Fixes the case where a recording started in conversation A is missed when the socket disconnects while bound to conversation B. - Move cleanup call outside the sessionId check so it runs regardless of session state. - When microphone permission is denied, rebuild options with includeMicrophone=false so recording starts without mic instead of potentially failing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: notify client when recording path fails security validation When a recording_status arrives with a file path outside the allowed recordings directory, send a user-visible error message before cleaning up state, so the user isn't left without feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: scope recording cleanup to disconnecting socket, not global cleanupRecordingsOnDisconnect now accepts the disconnecting socket and a conversation-to-socket lookup so it only clears recordings owned by conversations bound to that socket. This prevents an unrelated socket disconnect from dropping another session's active recording state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: notify client when recording stops without producing a file When recording_status arrives with status 'stopped' but no filePath, send a user-visible message explaining the recording produced no file instead of silently cleaning up. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: route recording notifications via reporting socket Use the socket that delivered recording_status as the primary recipient for completion/failure notifications. This ensures notifications reach the client even when the user has switched to a different conversation since starting the recording. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: regenerate ipc-contract-inventory.json after rebase Add recording_status, recording_start, and recording_stop wire types to the inventory after rebasing onto latest main. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use file_path column for file-backed attachment detection Use getFilePathForAttachment() instead of !dataBase64 to distinguish file-backed attachments from valid zero-byte inline uploads. The file_path column is the authoritative indicator of file-backed storage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add stop-acknowledgement timeout to prevent stale recording state (#9024) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: delete files only for confirmed orphans and suppress non-owner stop status (#9040) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: skip full video download for lazy attachment thumbnails (#9045) Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: replace dead-branch MIME type ternary with lookup map (#9049) * fix: replace dead-branch MIME type ternary with lookup map Co-Authored-By: Claude <noreply@anthropic.com> * fix: use Map for MIME types to avoid prototype pollution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: resolve symlinks in recording file path validation (#9046) * fix: resolve symlinks in recording file path validation Co-Authored-By: Claude <noreply@anthropic.com> * fix: also resolve allowedDir via realpathSync for APFS firmlink compatibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> * fix: delete orphaned conversation when recording start is rejected (#9048) * fix: delete orphaned conversation when recording start is rejected Co-Authored-By: Claude <noreply@anthropic.com> * fix: keep conversation on rejection, only unbind socket to avoid FK violations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Vellum Assistant <assistant@vellum.ai> Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses 2 review issues on PR #8686: (1) When start() detects stale state, cancel the now-active recorder to prevent untracked sessions, (2) Send status message before clearing attachToConversationId in forceStop() to preserve conversation association. Part of #8672.