Repository navigation
Fix the defects found reviewing PRs #3–#17 - #18
Merged
Merged
Conversation
Control and HTTP: - The control lease did nothing between 2025-era HTTP clients: each request reaches a fresh server without a name, so every client was the same "unidentified" caller. A client can now name itself with ?client= in its URL; an unnamed client cannot claim control and is refused while another client holds it. - obs-release-control and obs-control-status read their arguments as the request context, so a holder could not release control on stdio sessions. - HTTP responses are piped with stream.pipeline, so a failed stream cannot crash the process. Takes and recording: - Two takes started at once both succeeded and one monitor ran forever; a start now reserves the slot first. - StopRecord answers before OBS finishes the file (obs-websocket's own TODO); stopping now waits for the STOPPED state before inspecting it. - Simple mode records SimpleOutput/RecTracks, and Advanced FFmpeg recording FFAudioMixes; silence checks now follow them and measure every audio track. - Paused time is left out of take times, and audio heard while paused is not reported. - The take lock also refuses SetOutputSettings. - obs-record-clip refuses chapters at or after the clip's end, which kept it recording past its length. - Live-action confirmation covers obs-stop-output/obs-toggle-output and StopOutput/ToggleOutput, and ignores confirm: true when the client can ask. Trim: - Control characters in chapter names no longer break or forge chapters in the exported file. - Exports go to a temporary name and are renamed on success, so a failure leaves no partial copy; an existing or dangling-symlink output is refused. Scenes and snapshots: - obs-restore restored the wrong order when several items moved (184 of 870 arrangements), and pushed items added since to the top. - obs-apply-scene order: true did not converge when an unlisted item sat above listed ones; it now moves each listed source to the front in order. - Transforms compare as OBS stores them: 32-bit floats, and positions rounded to half a pixel (observed live). - obs-apply-scene refuses duplicate names and names of scenes, snapshots every existing input it touches, and reports partial creation. - A deleted scene no longer stops restoring the others. - Snapshot writes take a lock file, an unreadable file is moved aside, and automatic snapshots are capped separately from a user's. Also: tool-group tools are read-only toward OBS, a subscription added during the handshake is sent once OBS answers, and renaming a scene notifies the old items URI. Every new test fails against the previous code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vjn8uBEa4mJshaQU3pFuqX
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 87be9c5c78
ℹ️ 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".
- The temporary export name was derived from the output name only, so a recording named like it (.clip.partial.mp4 exported to clip.mp4) was the temporary file and was deleted afterwards. The name is now unique. - Positions compared within half a pixel, so a real half-pixel move was treated as no change and never restored. Both values are now rounded to the half pixel and compared exactly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vjn8uBEa4mJshaQU3pFuqX
- obs-trim-take and obs-contact-sheet pass the tool call's abort signal (ctx.mcpReq.signal) to ffprobe and ffmpeg, so a cancelled call stops them instead of leaving them running for up to 10 minutes. - A failed connection attempt no longer also schedules a retry from the disconnect handler with the previous attempt count; the attempt schedules its own once it has counted the failure. - The HTTP e2e test checks that a foreign Origin header is refused, and the README says that without OBS_MCP_HTTP_TOKEN any local program can control OBS through the server. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vjn8uBEa4mJshaQU3pFuqX
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.
Review: three fresh-context read-only reviewers looked at the diff
2f99df0..ca16917, one each on transport and security, the recording pipeline, and scene state. They all ran on the same model as the session. I checked every finding against the code; the obs-websocket and OBS sources on GitHub settled the two the reviewers couldn't verify. This PR fixes all of them. Every new test fails against the previous code: I swapped in each of the 11 fixed files frommainand ran its tests.Control and HTTP
?client=in the URL (passed asauthInfo, which reaches tools asctx.http.authInfoin both protocol eras; tested). Unnamed clients cannot claim control and are refused while another client holds it.obs-release-controlandobs-control-statusread their arguments as the request context. On stdio a holder could not release control.stream.pipeline, so a stream error cannot crash the process.Takes and recording
obs-take-startcalls both succeeded and orphaned a monitor. The slot is now reserved before the first await.StopRecordanswers before the file is finished. This is obs-websocket's own TODO inRequestHandler_Record.cpp. The stop now waits forRecordStateChangedSTOPPED, up to 10 s, before inspecting.SimpleOutput/RecTracks, except at stream quality or FLV. Advanced FFmpeg recording usesFFAudioMixes. Loudness is measured on every audio stream.SetOutputSettings.obs-record-cliprefuses chapters at or after the end.obs-stop-output/obs-toggle-outputandStopOutput/ToggleOutput.confirm: trueis ignored when the client can ask the user itself.Trim
Scenes and snapshots
obs-apply-scenewithorder: truedid not converge when an unlisted item sat above listed ones. It now moves each listed source to the front, in order. A new test covers every starting order of 4 items.obs-apply-scenerefuses duplicate names and scene names, snapshots every existing input it touches, and lists what was created when a creation fails partway.Smaller
Tests
npm run checkpasses: 233 unit, 14 e2e.?client=.Live (OBS 32.2.2)
obs-apply-scenewith decimal transforms converges in one call, and the automatic snapshot restored the original.Deliberately left: the reconnect backoff is one step behind, which is harmless.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Vjn8uBEa4mJshaQU3pFuqX