From 87be9c5c781d05abed3ae2d519b9d6caa8374379 Mon Sep 17 00:00:00 2001 From: Tom Main Date: Thu, 24 Sep 2026 01:46:11 -0400 Subject: [PATCH 1/3] Fix the defects found reviewing PRs #3-#17 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) Claude-Session: https://claude.ai/code/session_01Vjn8uBEa4mJshaQU3pFuqX --- README.md | 9 +- src/client.test.ts | 13 +++ src/client.ts | 13 ++- src/http.ts | 25 +++-- src/tools/apply-scene.test.ts | 52 +++++++++- src/tools/apply-scene.ts | 62 +++++++++--- src/tools/confirm.test.ts | 31 ++++++ src/tools/confirm.ts | 37 +++++--- src/tools/lease.test.ts | 58 +++++++++--- src/tools/lease.ts | 36 +++++-- src/tools/preflight.test.ts | 20 ++++ src/tools/preflight.ts | 21 +++- src/tools/resources.test.ts | 13 +++ src/tools/resources.ts | 4 + src/tools/snapshots.test.ts | 66 ++++++++++++- src/tools/snapshots.ts | 163 ++++++++++++++++++++++++++------ src/tools/take-monitor.test.ts | 15 +++ src/tools/take-monitor.ts | 22 ++++- src/tools/takes.test.ts | 48 +++++++--- src/tools/takes.ts | 72 ++++++++++++-- src/tools/toolset-tools.test.ts | 2 + src/tools/toolset-tools.ts | 3 +- src/tools/trim.test.ts | 24 ++++- src/tools/trim.ts | 38 +++++++- src/tools/workflows.test.ts | 22 +++-- src/tools/workflows.ts | 8 +- test/e2e/http.e2e.test.ts | 8 ++ test/support/fake-obs-scene.ts | 7 +- test/support/fake-obs-state.ts | 22 +++++ 29 files changed, 778 insertions(+), 136 deletions(-) diff --git a/README.md b/README.md index 10a02ba..6c4345e 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ Two prompts appear as slash commands in clients that show MCP prompts: `record-d ### Confirming live actions -With `OBS_MCP_CONFIRM_LIVE=true`, `obs-stop-stream`, `obs-toggle-stream`, `obs-stop-record`, `obs-toggle-record`, and `obs-take-stop` ask before they run, as do `obs-call-request` and `obs-batch` when they carry one of those requests. Clients that support elicitation (Claude Code, VS Code, Cursor) show the user a confirmation. Other clients get an error asking the model to get the user's agreement and call again with `confirm: true`. These tools are also marked destructive, so clients that ask before destructive tools already prompt without this setting. +With `OBS_MCP_CONFIRM_LIVE=true`, `obs-stop-stream`, `obs-toggle-stream`, `obs-stop-record`, `obs-toggle-record`, `obs-take-stop`, `obs-stop-output`, and `obs-toggle-output` ask before they run, as do `obs-call-request` and `obs-batch` when they carry one of those requests. The generic output tools are included because the stream and the recording are outputs too. Clients that support elicitation (Claude Code, VS Code, Cursor) show the user a confirmation, and `confirm: true` from the model does not skip it. Other clients get an error asking the model to get the user's agreement and call again with `confirm: true`. These tools are also marked destructive, so clients that ask before destructive tools already prompt without this setting. ## Where it fits @@ -250,7 +250,7 @@ Without `apply: true` it returns the plan. When applying, it creates what is mis `obs-snapshot` saves the settings, mute, volume, and audio tracks of inputs, and the transform, visibility, lock, and order of scene items: by default the program scene and the inputs in it. `obs-restore` compares OBS with a snapshot and sends only what differs, in one request batch; `dryRun: true` lists the changes first. It cannot recreate a removed input or item, and leaves items added since in place; it reports both. `obs-capture-window` takes a snapshot automatically before it changes an existing capture. -Snapshots are saved in the server's state directory (the last 20, readable only by you), so `obs-restore` works after a restart and from another agent's session. Each server lists only the snapshots of the OBS instance it connects to. They contain input settings, such as file paths and browser source URLs. OBS's own scene collection file is not a substitute: OBS rewrites it while running. +Snapshots are saved in the server's state directory (the last 20 you take, plus the last 10 taken automatically before a change, readable only by you), so `obs-restore` works after a restart and from another agent's session. Each server lists only the snapshots of the OBS instance it connects to. They contain input settings, such as file paths and browser source URLs. OBS's own scene collection file is not a substitute: OBS rewrites it while running. ### Sharing one server @@ -258,12 +258,13 @@ By default each agent starts its own server over stdio, with its own OBS connect ```bash OBS_MCP_HTTP_PORT=8765 OBS_MCP_READ_OBS_CONFIG=true node /path/to/OBS-MCP/build/index.js -claude mcp add --transport http obs http://127.0.0.1:8765/mcp +claude mcp add --transport http obs 'http://127.0.0.1:8765/mcp?client=claude' +# In Codex's config, point it at http://127.0.0.1:8765/mcp?client=codex ``` The server listens only on 127.0.0.1 and rejects requests whose Host or Origin header is not localhost, which blocks DNS rebinding from web pages. Set `OBS_MCP_HTTP_TOKEN` to also require a bearer token. Each HTTP request is served statelessly, so `OBS_MCP_DYNAMIC_TOOLSETS` is ignored over HTTP. -While agents share a server, `obs-claim-control` gives one of them control of OBS for a set time (30 minutes by default). Until it calls `obs-release-control` or the time runs out, tools that change OBS refuse calls from other clients; reading OBS and stopping outputs always work. `obs-control-status` shows who holds control. `obs-take-start` claims control for the length of the take. Clients are told apart by the name they report (Claude Code and Codex report different names), so two sessions of the same client share control. +While agents share a server, `obs-claim-control` gives one of them control of OBS for a set time (30 minutes by default). Until it calls `obs-release-control` or the time runs out, tools that change OBS refuse calls from other clients; reading OBS and stopping outputs always work. `obs-control-status` shows who holds control. `obs-take-start` claims control for the length of the take. Clients are told apart by name: the `client` parameter of the URL they connect with, or else the name a 2026-07-28 client sends with each request. Clients using the 2025 protocol over HTTP send each request without a name, so give each agent its own `?client=` name. An unnamed client cannot claim control and is refused while another client holds it. Two agents given the same name share control. The names are not credentials; control is for agents cooperating on one machine, not for keeping out someone who can reach the server. ## Troubleshooting diff --git a/src/client.test.ts b/src/client.test.ts index 840b595..005efaf 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -333,6 +333,19 @@ describe("OBSWebSocketClient high-volume events", () => { await vi.waitFor(() => expect(server.eventSubscriptions(2)).toBe(EventSubscription.All)); }); + it("sends a subscription added while identifying once OBS has answered", async () => { + const server = await createServer(); + const client = createClient(server); + + const connecting = client.connect(); + await server.waitForFrame(({ frame }) => frame.op === OBS_OP.Identify); + const release = client.subscribeHighVolume(EventSubscription.InputVolumeMeters); + await connecting; + + await vi.waitFor(() => expect(server.eventSubscriptions()).toBe(EventSubscription.All | EventSubscription.InputVolumeMeters)); + release(); + }); + it("keeps unapplied output settings flagged when OBS acknowledges a Reidentify", async () => { const server = await createServer(); const client = createClient(server); diff --git a/src/client.ts b/src/client.ts index b27a8b9..fa0e01d 100644 --- a/src/client.ts +++ b/src/client.ts @@ -251,6 +251,8 @@ export class OBSWebSocketClient extends EventEmitter { private readonly pendingRequests = new Map(); // How many callers want each high-volume subscription bit. private readonly highVolumeCounts = new Map(); + // The subscriptions OBS was last told about. + private sentSubscriptions: number = EventSubscription.All; constructor(url = "ws://localhost:4455", password: string | null = null) { super(); @@ -323,6 +325,7 @@ export class OBSWebSocketClient extends EventEmitter { const socket = this.ws; if (eventSubscriptions === before || !this.isConnected() || !socket || socket.readyState !== WebSocket.OPEN) return; try { + this.sentSubscriptions = eventSubscriptions; socket.send(JSON.stringify({ op: OpCode.Reidentify, d: { eventSubscriptions } })); } catch (error) { logger.error(`Unable to change OBS event subscriptions: ${asError(error).message}`); @@ -595,8 +598,12 @@ export class OBSWebSocketClient extends EventEmitter { } // OBS answers Reidentify with Identified too; only a new session means // OBS may have restarted and rebuilt its outputs. - if (!this.identified) this.outputSettingsPending = false; - this.identified = true; + if (!this.identified) { + this.outputSettingsPending = false; + this.identified = true; + // A subscription added while Identify was in flight is not in what OBS was sent. + this.reidentifyIfChanged(this.sentSubscriptions); + } this.emit("identified", socket); break; case OpCode.RequestResponse: { @@ -679,7 +686,7 @@ export class OBSWebSocketClient extends EventEmitter { op: OpCode.Identify, d: { rpcVersion: hello.rpcVersion, - eventSubscriptions: this.eventSubscriptions(), + eventSubscriptions: this.sentSubscriptions = this.eventSubscriptions(), ...(authentication ? { authentication } : {}), }, }; diff --git a/src/http.ts b/src/http.ts index 4b08cc5..2779ee0 100644 --- a/src/http.ts +++ b/src/http.ts @@ -7,6 +7,7 @@ import crypto from "node:crypto"; import http from "node:http"; import type { AddressInfo } from "node:net"; import { Readable } from "node:stream"; +import { pipeline } from "node:stream/promises"; import { createMcpHandler, hostHeaderValidationResponse, @@ -64,9 +65,20 @@ async function send(res: http.ServerResponse, response: Response): Promise res.end(); return; } - const body = Readable.fromWeb(response.body as import("node:stream/web").ReadableStream); - res.on("close", () => body.destroy()); - body.pipe(res); + // pipeline destroys both sides on error or client disconnect instead of throwing an unhandled 'error'. + await pipeline(Readable.fromWeb(response.body as import("node:stream/web").ReadableStream), res).catch((error: unknown) => { + logger.debug(`MCP HTTP response stream ended early: ${error instanceof Error ? error.message : String(error)}`); + }); +} + +/** + * The name a client declares with `?client=` in the URL it connects to. + * 2025-era clients cannot otherwise be told apart over stateless HTTP, and + * the control lease needs a name. + */ +function declaredClient(url: URL): string | undefined { + const name = url.searchParams.get("client")?.trim(); + return name && /^[\w.@:-]{1,64}$/.test(name) ? name : undefined; } /** @@ -84,8 +96,8 @@ export async function serveHttp(factory: () => McpServer, options: HttpOptions): const server = http.createServer((req, res) => { void (async () => { try { - const path = new URL(req.url ?? "/", `http://${LOOPBACK}`).pathname; - if (path !== MCP_PATH) { + const url = new URL(req.url ?? "/", `http://${LOOPBACK}`); + if (url.pathname !== MCP_PATH) { res.writeHead(404, { "content-type": "text/plain" }).end(`Not found; the MCP endpoint is ${MCP_PATH}`); return; } @@ -99,7 +111,8 @@ export async function serveHttp(factory: () => McpServer, options: HttpOptions): res.writeHead(401, { "content-type": "text/plain", "www-authenticate": "Bearer" }).end("Missing or wrong bearer token"); return; } - await send(res, await handler.fetch(request)); + const clientId = declaredClient(url); + await send(res, await handler.fetch(request, clientId ? { authInfo: { token: options.token ?? "", clientId, scopes: [] } } : undefined)); } catch (error) { logger.error(`MCP HTTP request failed: ${error instanceof Error ? error.message : String(error)}`); if (!res.headersSent) res.writeHead(500, { "content-type": "text/plain" }); diff --git a/src/tools/apply-scene.test.ts b/src/tools/apply-scene.test.ts index 8581fde..ecd4fd9 100644 --- a/src/tools/apply-scene.test.ts +++ b/src/tools/apply-scene.test.ts @@ -85,8 +85,8 @@ describe("obs-apply-scene", () => { const batches = harness.fakeObs.history().filter(({ frame }) => frame.op === OBS_OP.RequestBatch).length; expect(batches).toBeGreaterThanOrEqual(2); expect(result.structuredContent).toMatchObject({ applied: true, converged: true, failures: [] }); - // A new scene has nothing to undo. - expect(result.structuredContent).not.toHaveProperty("snapshotId"); + // The scene is new, but it reuses existing inputs, whose state is saved. + expect(result.structuredContent).toHaveProperty("snapshotId"); }); it("changes only what differs in an existing scene, and saves a snapshot first", async () => { @@ -135,6 +135,54 @@ describe("obs-apply-scene", () => { expect(sent()).toEqual([]); }); + it("stacks the listed sources in front, in order, from any starting order, in one call", async () => { + state.inputs.Webcam = { kind: "image_source", settings: {} }; + state.inputs.Slides = { kind: "image_source", settings: {} }; + const all = ["Background", "Chrome", "Webcam", "Slides"]; + const permutations = (list: string[]): string[][] => list.length <= 1 ? [list] + : list.flatMap((head, index) => permutations([...list.slice(0, index), ...list.slice(index + 1)]).map((rest) => [head, ...rest])); + for (const order of permutations(all)) { + state.scenes.Demo = order.map((sourceName, index) => ({ sceneItemId: index + 1, sourceName, enabled: true, locked: false, transform: {} })); + + const result = await harness.call("obs-apply-scene", { + sceneName: "Demo", + sources: [{ name: "Slides" }, { name: "Chrome" }], + order: true, + apply: true, + }); + + const names = state.scenes.Demo.map(({ sourceName }) => sourceName); + expect(names.slice(-2), `from ${order.join(",")}`).toEqual(["Slides", "Chrome"]); + expect(result.isError, `from ${order.join(",")}: ${resultText(result)}`).toBeFalsy(); + } + }); + + it("refuses a source listed twice or named like a scene, before changing anything", async () => { + state.scenes["Be Right Back"] = []; + + const twice = await harness.call("obs-apply-scene", { sceneName: "Demo", sources: [{ name: "Chrome" }, { name: "Chrome" }], apply: true }); + const scene = await harness.call("obs-apply-scene", { sceneName: "Demo", sources: [{ name: "Be Right Back", kind: "image_source" }], apply: true }); + + expect(resultText(twice)).toContain("Chrome is listed more than once"); + expect(resultText(scene)).toContain("Be Right Back is a scene, not an input"); + expect(sent()).toEqual([]); + }); + + it("converges on decimal transforms, which OBS keeps as 32-bit floats", async () => { + const result = await harness.call("obs-apply-scene", { + sceneName: "Demo", + sources: [{ name: "Chrome", transform: { positionX: 100.1, scaleX: 0.3333, rotation: 12.34 } }], + apply: true, + }); + + expect(result.isError).toBeFalsy(); + expect(resultText(result)).toContain("OBS now matches the spec"); + expect(resultText(await harness.call("obs-apply-scene", { + sceneName: "Demo", + sources: [{ name: "Chrome", transform: { positionX: 100.1, scaleX: 0.3333, rotation: 12.34 } }], + }))).toBe("Scene Demo already matches"); + }); + it("does not warn about an audio file, which has no picture", async () => { const result = await harness.call("obs-apply-scene", { sceneName: "Demo", diff --git a/src/tools/apply-scene.ts b/src/tools/apply-scene.ts index 0caaff0..faabcce 100644 --- a/src/tools/apply-scene.ts +++ b/src/tools/apply-scene.ts @@ -7,7 +7,7 @@ import type { CallToolResult, McpServer } from "@modelcontextprotocol/server"; import { z } from "zod"; import type { BatchRequest, BatchResult, OBSWebSocketClient } from "../client.js"; import { withoutAudioOnly } from "./audio-only.js"; -import { readInputs, readItems, same, takeSnapshot, type InputState, type ItemState } from "./snapshots.js"; +import { readInputs, readItems, same, sameTransformValue, takeSnapshot, type InputState, type ItemState } from "./snapshots.js"; type JsonObject = Record; @@ -74,6 +74,7 @@ function toDb(multiplier: number | undefined): number | undefined { type Current = { sceneExists: boolean; + sceneNames: Set; inputKinds: Map; items: ItemState[]; inputs: InputState[]; @@ -85,8 +86,9 @@ async function readCurrent(client: OBSWebSocketClient, spec: SceneSpec): Promise client.sendRequest("GetSceneList") as Promise, client.sendRequest("GetInputList") as Promise, ]); - const sceneExists = (Array.isArray(sceneList.scenes) ? sceneList.scenes : []) - .some((scene) => isObject(scene) && scene.sceneName === spec.sceneName); + const sceneNames = new Set((Array.isArray(sceneList.scenes) ? sceneList.scenes : []) + .filter(isObject).map(({ sceneName }) => String(sceneName))); + const sceneExists = sceneNames.has(spec.sceneName); const inputKinds = new Map((Array.isArray(inputList.inputs) ? inputList.inputs : []) .filter(isObject) .map(({ inputName, inputKind }) => [String(inputName), String(inputKind)] as const)); @@ -97,7 +99,7 @@ async function readCurrent(client: OBSWebSocketClient, spec: SceneSpec): Promise ? client.sendRequest("GetVideoSettings") as Promise<{ baseWidth: number; baseHeight: number }> : Promise.resolve(null), ]); - return { sceneExists, inputKinds, items, inputs, video }; + return { sceneExists, sceneNames, inputKinds, items, inputs, video }; } /** @@ -114,6 +116,12 @@ export function planScene(spec: SceneSpec, current: Current): { actions: SceneAc if (!current.sceneExists) { actions.push({ target: `scene ${sceneName}`, change: "create", phase: "create", request: { requestType: "CreateScene", requestData: scene } }); } + const seen = new Set(); + for (const { name } of spec.sources) { + if (seen.has(name)) errors.push(`${name} is listed more than once`); + seen.add(name); + if (current.sceneNames.has(name) || name === sceneName) errors.push(`${name} is a scene, not an input; nest scenes with obs-create-scene-item`); + } for (const source of spec.sources) { const target = source.name; @@ -200,7 +208,7 @@ export function planScene(spec: SceneSpec, current: Current): { actions: SceneAc : {}), ...source.transform, }; - const changedFields = Object.keys(transform).filter((key) => !item || !same(transform[key], item.transform[key])); + const changedFields = Object.keys(transform).filter((key) => !item || !sameTransformValue(key, transform[key], item.transform[key])); if (changedFields.length > 0) { itemAction(source.fit && changedFields.includes("boundsType") ? "fit to the canvas" : `transform (${changedFields.join(", ")})`, "SetSceneItemTransform", { sceneItemTransform: transform }); } @@ -215,16 +223,19 @@ export function planScene(spec: SceneSpec, current: Current): { actions: SceneAc } if (spec.order) { - // Sources are listed back to front; items not in the spec stay behind them. - const base = spec.removeOthers ? 0 : others.length; + // Sources are listed back to front and end up in front of any others: + // the top items, bottom to top, must be the listed ones in listed order. const allExist = spec.sources.every(({ name }) => current.items.some(({ sourceName }) => sourceName === name)); - const outOfPlace = spec.sources.some(({ name }, index) => current.items.find(({ sourceName }) => sourceName === name)?.index !== base + index); + const stack = [...current.items].sort((a, b) => a.index - b.index).map(({ sourceName }) => sourceName); + const top = stack.slice(stack.length - spec.sources.length); if (!allExist) { actions.push({ target: `scene ${sceneName}`, change: "order the sources back to front as listed", phase: "change" }); - } else if (outOfPlace) { - spec.sources.forEach(({ name }, index) => { + } else if (spec.sources.some(({ name }, index) => top[index] !== name)) { + // Moving each to the top in listed order stacks them correctly whatever OBS shifts on the way. + const topIndex = (spec.removeOthers ? spec.sources.length : current.items.length) - 1; + spec.sources.forEach(({ name }) => { const item = current.items.find(({ sourceName }) => sourceName === name)!; - actions.push({ target: name, change: `order (to ${base + index})`, phase: "change", request: { requestType: "SetSceneItemIndex", requestData: { ...scene, sceneItemId: item.sceneItemId, sceneItemIndex: base + index } } }); + actions.push({ target: name, change: "order (to the front, as listed)", phase: "change", request: { requestType: "SetSceneItemIndex", requestData: { ...scene, sceneItemId: item.sceneItemId, sceneItemIndex: topIndex } } }); }); } } @@ -267,8 +278,22 @@ export async function applyScene(client: OBSWebSocketClient, spec: SceneSpec, ap }; } - const snapshot = before.sceneExists - ? await takeSnapshot(client, { scenes: [spec.sceneName], label: "before obs-apply-scene" }).catch(() => undefined) + // Save every existing input the spec touches, including ones shown only in + // other scenes, and this scene's items, so obs-restore can undo the change. + const touchedInputs = spec.sources.map(({ name }) => name).filter((name) => before.inputKinds.has(name)); + const sceneInputs = before.items.map(({ sourceName }) => sourceName).filter((name) => before.inputKinds.has(name)); + const snapshotInputs = [...new Set([...sceneInputs, ...touchedInputs])]; + let snapshotNote: string | undefined; + const snapshot = before.sceneExists || snapshotInputs.length > 0 + ? await takeSnapshot(client, { + scenes: before.sceneExists ? [spec.sceneName] : [], + inputs: snapshotInputs, + label: "before obs-apply-scene", + auto: true, + }).catch((error: unknown) => { + snapshotNote = `Could not save a snapshot first, so obs-restore cannot undo this: ${errorMessage(error)}`; + return undefined; + }) : undefined; // Pass 1 creates the scene, inputs, and items; pass 2 plans again with their IDs and changes the rest. @@ -277,7 +302,15 @@ export async function applyScene(client: OBSWebSocketClient, spec: SceneSpec, ap if (creations.length > 0) { const results = await client.sendBatch(creations.map(({ request }) => request!), { haltOnFailure: true }); problems.push(...failures(creations, results)); - if (problems.length > 0) return errorResult([`Stopped while creating:`, ...problems].join("\n")); + if (problems.length > 0) { + const created = creations.filter((_action, index) => results[index]?.ok); + return errorResult([ + "Stopped while creating:", + ...problems, + ...(created.length > 0 ? ["Created before stopping:", ...describe(created)] : []), + ...(snapshot ? [`Snapshot ${snapshot.id} holds the previous state of existing sources`] : []), + ].join("\n")); + } } const second = planScene(spec, await readCurrent(client, spec)); const changes = second.actions.filter((action) => action.request); @@ -292,6 +325,7 @@ export async function applyScene(client: OBSWebSocketClient, spec: SceneSpec, ap ...(remaining.length > 0 ? ["Still different afterwards:", ...describe(remaining)] : ["OBS now matches the spec"]), ...blank.map((name) => `Warning: ${name} renders at 0×0; a capture may need a window, display, or permission`), ...(snapshot ? [`Saved the previous state as snapshot ${snapshot.id}; obs-restore undoes changes to existing sources`] : []), + ...(snapshotNote ? [snapshotNote] : []), ]; return { content: [{ type: "text", text: lines.join("\n") }], diff --git a/src/tools/confirm.test.ts b/src/tools/confirm.test.ts index e20a17e..961b3cb 100644 --- a/src/tools/confirm.test.ts +++ b/src/tools/confirm.test.ts @@ -120,6 +120,37 @@ describe("live action confirmation", () => { expect(sent("StopStream")).toBe(false); }); + it("asks before stopping or toggling any output, which can be the stream or the recording", async () => { + const questions: string[] = []; + const { call } = await start({ confirmLive: true, onElicit: (message) => { + questions.push(message); + return { action: "decline" }; + } }); + + await call("obs-stop-output", { outputName: "simple_stream" }); + await call("obs-call-request", { requestType: "ToggleOutput", requestData: { outputName: "adv_file_output" } }); + + expect(questions).toEqual([ + "Stop the output simple_stream? If it is the stream or the recording, this ends it.", + "Toggle the output adv_file_output? If it is the stream or the recording, this can end it.", + ]); + expect(sent("StopOutput")).toBe(false); + }); + + it("asks the user even when the model passes confirm: true, if the client can ask", async () => { + const questions: string[] = []; + const { call } = await start({ confirmLive: true, onElicit: (message) => { + questions.push(message); + return { action: "decline" }; + } }); + + const result = await call("obs-stop-stream", { confirm: true }); + + expect(questions).toEqual(["Stop the live stream?"]); + expect(resultText(result)).toContain("Cancelled"); + expect(sent("StopStream")).toBe(false); + }); + it("confirms through the 2026-07-28 input_required round trip", async () => { vi.spyOn(console, "error").mockImplementation(() => undefined); const fakeObs = await FakeOBSServer.start(); diff --git a/src/tools/confirm.ts b/src/tools/confirm.ts index 71c4146..cf614b2 100644 --- a/src/tools/confirm.ts +++ b/src/tools/confirm.ts @@ -23,15 +23,23 @@ export const LIVE_ACTIONS: Readonly> = { }; /** OBS requests that end a live broadcast or a take, with the question to ask. */ -const LIVE_REQUESTS: Readonly> = { - StopStream: "Stop the live stream?", - ToggleStream: "Toggle the live stream? If it is live, this ends the broadcast.", - StopRecord: "Stop the recording?", - ToggleRecord: "Toggle recording? If it is recording, this ends the take.", +const LIVE_REQUESTS: Readonly) => string>> = { + StopStream: () => "Stop the live stream?", + ToggleStream: () => "Toggle the live stream? If it is live, this ends the broadcast.", + StopRecord: () => "Stop the recording?", + ToggleRecord: () => "Toggle recording? If it is recording, this ends the take.", + // The generic output requests reach the stream and recording outputs too (e.g. simple_stream, adv_file_output). + StopOutput: ({ outputName }) => `Stop the output ${String(outputName)}? If it is the stream or the recording, this ends it.`, + ToggleOutput: ({ outputName }) => `Toggle the output ${String(outputName)}? If it is the stream or the recording, this can end it.`, }; -function questionForRequests(requestTypes: unknown[]): string | undefined { - const questions = [...new Set(requestTypes.map((type) => LIVE_REQUESTS[String(type)]).filter(Boolean))]; +type RequestLike = { requestType?: unknown; requestData?: unknown }; + +function questionForRequests(requests: RequestLike[]): string | undefined { + const questions = [...new Set(requests.map(({ requestType, requestData }) => { + const ask = LIVE_REQUESTS[String(requestType)]; + return ask?.(typeof requestData === "object" && requestData !== null ? requestData as Record : {}); + }).filter((question): question is string => Boolean(question)))]; return questions.length > 0 ? questions.join(" ") : undefined; } @@ -40,10 +48,10 @@ function questionForRequests(requestTypes: unknown[]): string | undefined { * to ask, or undefined when these arguments need no confirmation. */ export const CONDITIONAL_LIVE_ACTIONS: Readonly) => string | undefined>> = { - "obs-call-request": ({ requestType }) => questionForRequests([requestType]), - "obs-batch": ({ requests }) => questionForRequests( - Array.isArray(requests) ? requests.map((request) => (request as { requestType?: unknown })?.requestType) : [], - ), + "obs-call-request": ({ requestType, requestData }) => questionForRequests([{ requestType, requestData }]), + "obs-batch": ({ requests }) => questionForRequests(Array.isArray(requests) ? requests as RequestLike[] : []), + "obs-stop-output": (args) => questionForRequests([{ requestType: "StopOutput", requestData: args }]), + "obs-toggle-output": (args) => questionForRequests([{ requestType: "ToggleOutput", requestData: args }]), }; const CONFIRM_KEY = "confirm"; @@ -85,8 +93,6 @@ function confirmation( confirmArg: boolean | undefined, ctx: RequestContext, ): "proceed" | ReturnType | CallToolResult { - if (confirmArg === true) return "proceed"; - const responses = ctx.mcpReq?.inputResponses; const answered = acceptedContent(responses, CONFIRM_KEY, CONFIRM_SCHEMA); if (answered) return answered.confirm ? "proceed" : textResult("Cancelled; nothing was changed"); @@ -108,6 +114,9 @@ function confirmation( }, }); } + // Only a client that cannot ask the user relies on the model passing confirm: true; + // one that can always asks, so the model cannot approve on the user's behalf. + if (confirmArg === true) return "proceed"; return textResult( `${question} This server requires confirmation for live actions and this client cannot ask. ` + "Ask the user, then call again with confirm: true.", @@ -134,7 +143,7 @@ export function withLiveConfirmation(server: McpServer): McpServer { const hadSchema = config.inputSchema !== undefined; const inputSchema = (config.inputSchema ?? z.object({})).extend({ confirm: z.boolean().optional() - .describe("Pass true only after the user has agreed; otherwise the server asks the user"), + .describe("For clients that cannot show the user a question: pass true only after the user has agreed. Ignored when the server can ask the user itself"), }); const confirmedCallback = async (args: Record, ctx: RequestContext) => { const { confirm, ...rest } = args; diff --git a/src/tools/lease.test.ts b/src/tools/lease.test.ts index 64fb39a..da9294e 100644 --- a/src/tools/lease.test.ts +++ b/src/tools/lease.test.ts @@ -11,7 +11,7 @@ import { Client, StreamableHTTPClientTransport, type CallToolResult } from "@mod import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { OBSWebSocketClient } from "../client.js"; import { FakeOBSServer } from "../../test/support/fake-obs-server.js"; -import { healthyObsState, servePreflightState } from "../../test/support/fake-obs-state.js"; +import { healthyObsState, servePreflightState, serveRecordOutput } from "../../test/support/fake-obs-state.js"; import { initialize } from "./index.js"; import { activeLease, claim } from "./lease.js"; @@ -21,12 +21,24 @@ let handler: McpHttpHandler; let directory: string; const clients: Client[] = []; -/** One MCP client, named as an agent would name itself, sharing the server with the others. */ -async function agent(name: string): Promise<(tool: string, args?: Record) => Promise> { - const client = new Client({ name, version: "1.0.0" }, { versionNegotiation: { mode: { pin: "2026-07-28" } } }); +/** + * One MCP client, named as an agent would name itself, sharing the server with + * the others. A 2025-era client sends each request to a fresh server with no + * name; `declared` is the name its URL gives (?client=), which serveHttp + * passes to the handler as authInfo. + */ +async function agent( + name: string, + { era = "modern", declared }: { era?: "modern" | "legacy"; declared?: string } = {}, +): Promise<(tool: string, args?: Record) => Promise> { + const client = new Client( + { name, version: "1.0.0" }, + { versionNegotiation: { mode: era === "modern" ? { pin: "2026-07-28" } : "legacy" } }, + ); + const authInfo = declared ? { authInfo: { token: "", clientId: declared, scopes: [] } } : undefined; await client.connect(new StreamableHTTPClientTransport( new URL("http://obs-mcp.test/mcp"), - { fetch: (input, init) => handler.fetch(new Request(input, init)) }, + { fetch: (input, init) => handler.fetch(new Request(input, init), authInfo) }, )); clients.push(client); return async (tool, args = {}) => await client.callTool({ name: tool, arguments: args }) as CallToolResult; @@ -43,17 +55,13 @@ beforeEach(async () => { servePreflightState(fakeObs, () => healthyObsState(directory)); fakeObs.respondWith("SetCurrentProgramScene", () => ({})); fakeObs.respondWith("CreateRecordChapter", () => ({})); - fakeObs.respondWith("StartRecord", () => { - setTimeout(() => fakeObs.sendEvent("RecordStateChanged", { outputActive: true, outputState: "OBS_WEBSOCKET_OUTPUT_STARTED", outputPath: join(directory, "take.mp4") }), 5); - return {}; - }); - fakeObs.respondWith("StopRecord", () => ({ outputPath: join(directory, "take.mp4") })); + serveRecordOutput(fakeObs, () => join(directory, "take.mp4")); obsClient = new OBSWebSocketClient(fakeObs.url); handler = createMcpHandler(() => { const server = new McpServer({ name: "obs-mcp-test", version: "0.0.0" }); initialize(server, obsClient, { sessionNotifications: false }); return server; - }, { legacy: "reject" }); + }); }); afterEach(async () => { @@ -97,6 +105,34 @@ describe("control lease", () => { expect((await claude("obs-stop-record")).isError).toBeFalsy(); }); + it("does not let unnamed 2025-era HTTP clients hold control or pass as the holder", async () => { + const a = await agent("agent-a", { era: "legacy" }); + const b = await agent("agent-b", { era: "legacy" }); + const named = await agent("codex", { era: "legacy", declared: "codex" }); + + expect(text(await a("obs-claim-control"))).toContain("did not identify itself"); + expect(text(await named("obs-claim-control"))).toMatch(/^codex has control/); + expect(text(await b("obs-set-current-scene", { sceneName: "X" }))).toContain("Not changed: codex has control"); + expect(text(await b("obs-release-control"))).toContain("Not released"); + }); + + it("lets the holder see and release its control, whichever way it identifies itself", async () => { + const named = await agent("codex", { era: "legacy", declared: "codex" }); + await named("obs-claim-control"); + + expect(text(await named("obs-control-status"))).toContain("(this client)"); + expect(text(await named("obs-release-control"))).toBe("Released control of OBS"); + }); + + it("says a take could not claim control for an unnamed client", async () => { + const unnamed = await agent("agent-a", { era: "legacy" }); + + const started = await unnamed("obs-take-start"); + + expect(text(started)).toContain("did not identify itself"); + await unnamed("obs-take-stop"); + }); + it("expires", () => { vi.useFakeTimers({ toFake: ["Date"] }); expect(claim(obsClient, "codex-mcp-client", 1, "").ok).toBe(true); diff --git a/src/tools/lease.ts b/src/tools/lease.ts index 37f0b05..acf5355 100644 --- a/src/tools/lease.ts +++ b/src/tools/lease.ts @@ -27,17 +27,25 @@ const ALWAYS_ALLOWED = new Set([ const UNIDENTIFIED = "an unidentified client"; -type RequestContext = { mcpReq?: { envelope?: Record } }; +type RequestContext = { + mcpReq?: { envelope?: Record }; + http?: { authInfo?: { clientId?: unknown } }; +}; function errorResult(text: string): CallToolResult { return { content: [{ type: "text", text }], isError: true }; } /** - * The calling client's name: from the request envelope on 2026-07-28, or - * from initialize on 2025-era sessions. Clients with the same name share a lease. + * The calling client's name, from the first of: the `client` parameter of + * the HTTP URL it connects with, the 2026-07-28 request envelope, or + * initialize on a 2025-era stdio session. A 2025-era client over HTTP sends + * each request to a fresh server without a name, so it is unidentified + * unless its URL names it. Clients with the same name share a lease. */ export function callerName(server: McpServer, ctx: unknown): string { + const declared = (ctx as RequestContext | undefined)?.http?.authInfo?.clientId; + if (typeof declared === "string" && declared) return declared; const envelope = (ctx as RequestContext | undefined)?.mcpReq?.envelope; const info = envelope?.[CLIENT_INFO_META_KEY] as { name?: unknown } | undefined; if (typeof info?.name === "string" && info.name) return info.name; @@ -55,7 +63,13 @@ export function activeLease(client: OBSWebSocketClient, now = Date.now()): Lease } /** Takes control for `holder`, or returns the lease that stands in the way. */ -export function claim(client: OBSWebSocketClient, holder: string, minutes: number, reason: string): { ok: true; lease: Lease } | { ok: false; lease: Lease } { +export function isIdentified(holder: string): boolean { + return holder !== UNIDENTIFIED; +} + +/** Takes control for `holder`, or returns the lease that stands in the way. An unidentified client cannot hold control. */ +export function claim(client: OBSWebSocketClient, holder: string, minutes: number, reason: string): { ok: true; lease: Lease } | { ok: false; lease?: Lease } { + if (!isIdentified(holder)) return { ok: false }; const current = activeLease(client); if (current && current.holder !== holder) return { ok: false, lease: current }; const now = Date.now(); @@ -97,8 +111,9 @@ export function withControlLease(server: McpServer, client: OBSWebSocketClient): const guarded = async (...callbackArgs: unknown[]) => { const lease = activeLease(client); if (lease) { + // An unidentified caller can never be the holder, so it is always refused. const caller = callerName(target, callbackArgs.at(-1)); - if (caller !== lease.holder) { + if (!isIdentified(caller) || caller !== lease.holder) { return errorResult(`Not changed: ${describe(lease)}. Ask them to call obs-release-control, ` + "or wait. Reading OBS and stopping outputs still work"); } @@ -131,7 +146,12 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void async ({ minutes, reason }, ctx): Promise => { const holder = callerName(server, ctx); const result = claim(client, holder, minutes, reason ?? ""); - if (!result.ok) return errorResult(`Not claimed: ${describe(result.lease)}`); + if (!result.ok) { + return errorResult(result.lease + ? `Not claimed: ${describe(result.lease)}` + : "Not claimed: this client did not identify itself. Over HTTP, connect with a name in the URL, " + + "e.g. http://127.0.0.1:/mcp?client=codex"); + } return { content: [{ type: "text", text: `${describe(result.lease)}. Release it with obs-release-control` }], structuredContent: { ...result.lease }, @@ -147,7 +167,7 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void inputSchema: z.object({}), annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: true, openWorldHint: false }, }, - async (ctx): Promise => { + async (_args, ctx): Promise => { const holder = callerName(server, ctx); const current = activeLease(client); if (release(client, holder)) return { content: [{ type: "text", text: "Released control of OBS" }] }; @@ -163,7 +183,7 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void inputSchema: z.object({}), annotations: READ_ONLY_TOOL, }, - async (ctx): Promise => { + async (_args, ctx): Promise => { const lease = activeLease(client); const caller = callerName(server, ctx); return { diff --git a/src/tools/preflight.test.ts b/src/tools/preflight.test.ts index 7652713..4563e36 100644 --- a/src/tools/preflight.test.ts +++ b/src/tools/preflight.test.ts @@ -132,6 +132,26 @@ describe("obs-preflight", () => { expect(byId("audio")?.message).not.toContain("Camera"); }); + it("follows the tracks Simple mode records, not just track 1", async () => { + state.profile["SimpleOutput/RecTracks"] = "3"; + state.inputs[0] = { inputName: "Chrome", inputKind: "screen_capture", muted: false, tracks: { 1: false, 2: true } }; + + const { byId } = await preflight({ expectSilent: true }); + + expect(byId("audio")).toMatchObject({ status: "fail", message: expect.stringContaining("Chrome (track 2)") }); + }); + + it("records only track 1 in Simple mode at stream quality", async () => { + state.profile["SimpleOutput/RecQuality"] = "Stream"; + state.profile["SimpleOutput/StreamEncoder"] = "apple_h264"; + state.profile["SimpleOutput/RecTracks"] = "3"; + state.inputs[0] = { inputName: "Chrome", inputKind: "screen_capture", muted: false, tracks: { 1: false, 2: true } }; + + const { byId } = await preflight({ expectSilent: true }); + + expect(byId("audio")?.status).toBe("pass"); + }); + it("ignores unmuted inputs whose recorded tracks are disabled", async () => { state.profile["Output/Mode"] = "Advanced"; state.profile["AdvOut/RecEncoder"] = "com.apple.videotoolbox.videoencoder.ave.avc"; diff --git a/src/tools/preflight.ts b/src/tools/preflight.ts index fd065d6..851e4aa 100644 --- a/src/tools/preflight.ts +++ b/src/tools/preflight.ts @@ -158,9 +158,26 @@ async function checkEncoder(client: OBSWebSocketClient): Promise { - if (await profileParameter(client, "Output", "Mode") !== "Advanced") return [1]; - const mask = Number(await profileParameter(client, "AdvOut", "RecTracks") ?? "1"); + const mode = await profileParameter(client, "Output", "Mode") ?? "Simple"; + let mask: number; + if (mode === "Advanced") { + const ffmpeg = await profileParameter(client, "AdvOut", "RecType") === "FFmpeg"; + mask = Number(await profileParameter(client, "AdvOut", ffmpeg ? "FFAudioMixes" : "RecTracks") ?? "1"); + } else { + const [quality, format] = await Promise.all([ + profileParameter(client, "SimpleOutput", "RecQuality"), + profileParameter(client, "SimpleOutput", "RecFormat2"), + ]); + if (quality === "Stream" || format === "flv") return [1]; + mask = Number(await profileParameter(client, "SimpleOutput", "RecTracks") ?? "1"); + } const tracks = [1, 2, 3, 4, 5, 6].filter((track) => (mask & (1 << (track - 1))) !== 0); return tracks.length > 0 ? tracks : [1]; } diff --git a/src/tools/resources.test.ts b/src/tools/resources.test.ts index c0546eb..9cf9b6c 100644 --- a/src/tools/resources.test.ts +++ b/src/tools/resources.test.ts @@ -99,6 +99,19 @@ describe("resources", () => { expect(updated).toHaveLength(2); }); + it("tells subscribers of a renamed scene's items that the old URI changed", async () => { + const updated: string[] = []; + harness.mcpClient.setNotificationHandler("notifications/resources/updated", async (notification) => { + updated.push(notification.params.uri); + }); + await harness.mcpClient.readResource({ uri: "obs://status" }); + await harness.mcpClient.subscribeResource({ uri: "obs://scene/Be%20Right%20Back/items" }); + + harness.fakeObs.sendEvent("SceneNameChanged", { oldSceneName: "Be Right Back", sceneName: "BRB" }); + + await vi.waitFor(() => expect(updated).toContain("obs://scene/Be%20Right%20Back/items")); + }); + it("serves the running take and notifies subscribers when it changes", async () => { const updated: string[] = []; harness.mcpClient.setNotificationHandler("notifications/resources/updated", async (notification) => { diff --git a/src/tools/resources.ts b/src/tools/resources.ts index 5ac91a0..07ba0c3 100644 --- a/src/tools/resources.ts +++ b/src/tools/resources.ts @@ -193,6 +193,10 @@ export function forwardResourceEvents( if (isObject(eventData) && typeof eventData.sceneName === "string") onUpdated(sceneItemsUri(eventData.sceneName)); }]), ...["SceneCreated", "SceneRemoved", "SceneNameChanged"].map((event): [string, () => void] => [event, onListChanged]), + // A renamed scene's items resource moves to a new URI; tell subscribers of the old one. + ["SceneNameChanged", (eventData) => { + if (isObject(eventData) && typeof eventData.oldSceneName === "string") onUpdated(sceneItemsUri(eventData.oldSceneName)); + }], ]; for (const [event, listener] of listeners) client.on(event, listener); const onTakeUpdate = () => onUpdated(TAKE_URI); diff --git a/src/tools/snapshots.test.ts b/src/tools/snapshots.test.ts index 46ea36b..8f054ea 100644 --- a/src/tools/snapshots.test.ts +++ b/src/tools/snapshots.test.ts @@ -3,7 +3,7 @@ * See NOTICE.md and Git history for authorship and change dates. * SPDX-License-Identifier: GPL-2.0-only */ -import { readFileSync, statSync, writeFileSync } from "node:fs"; +import { readdirSync, readFileSync, statSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { OBS_OP } from "../../test/support/fake-obs-server.js"; @@ -136,6 +136,70 @@ describe("obs-snapshot and obs-restore", () => { expect(resultText(list)).not.toContain("elsewhere"); }); + it("restores the saved order however the items were rearranged", async () => { + const extra = ["Slides", "Webcam"].map((sourceName, index) => ({ sceneItemId: 3 + index, sourceName, enabled: true, locked: false, transform: {} })); + items.push(...extra); + inputs.Slides = { kind: "image_source", settings: {} }; + inputs.Webcam = { kind: "image_source", settings: {} }; + const saved = items.map(({ sceneItemId }) => sceneItemId); + await harness.call("obs-snapshot"); + + const permutations = (list: number[]): number[][] => list.length <= 1 ? [list] + : list.flatMap((head, index) => permutations([...list.slice(0, index), ...list.slice(index + 1)]).map((rest) => [head, ...rest])); + for (const order of permutations(saved)) { + const byId = new Map(items.map((item) => [item.sceneItemId, item])); + items.splice(0, items.length, ...order.map((id) => byId.get(id)!)); + await harness.call("obs-restore"); + expect(items.map(({ sceneItemId }) => sceneItemId), `from ${order.join(",")}`).toEqual(saved); + } + }); + + it("keeps an item added since in its place while restoring the others' order", async () => { + await harness.call("obs-snapshot"); + items.reverse(); + items.unshift({ sceneItemId: 9, sourceName: "Webcam", enabled: true, locked: false, transform: {} }); + + await harness.call("obs-restore"); + + expect(items.map(({ sourceName }) => sourceName)).toEqual(["Webcam", "Background", "Chrome"]); + }); + + it("restores the scenes that still exist when another was deleted", async () => { + await harness.call("obs-snapshot", { scenes: ["Demo"], inputs: ["Chrome"] }); + const file = join(process.env.OBS_MCP_STATE_DIR!, "snapshots.json"); + const snapshots = JSON.parse(readFileSync(file, "utf8")) as { items: { sceneName: string }[] }[]; + snapshots[0]!.items.push({ ...snapshots[0]!.items[0]!, sceneName: "Deleted Scene" }); + writeFileSync(file, JSON.stringify(snapshots)); + items[1]!.enabled = false; + + const result = await harness.call("obs-restore"); + + expect(resultText(result)).toContain("scene Deleted Scene no longer exists"); + expect(items[1]!.enabled).toBe(true); + }); + + it("moves an unreadable snapshot file aside instead of overwriting it", async () => { + const directory = process.env.OBS_MCP_STATE_DIR!; + writeFileSync(join(directory, "snapshots.json"), "{ not json"); + + await harness.call("obs-snapshot"); + + expect(readdirSync(directory).some((name) => name.startsWith("snapshots.json.unreadable-"))).toBe(true); + expect(JSON.parse(readFileSync(join(directory, "snapshots.json"), "utf8"))).toHaveLength(1); + }); + + it("does not let automatic snapshots push out ones a user took", async () => { + await harness.call("obs-snapshot", { label: "mine" }); + for (let index = 0; index < 12; index += 1) { + await harness.call("obs-apply-scene", { sceneName: "Demo", sources: [{ name: "Chrome", volumeDb: -index - 1 }], apply: true }); + } + + const list = resultText(await harness.call("obs-snapshot", { list: true })); + + expect(list).toContain('"mine"'); + expect(list.split("\n").filter((line) => line.includes("before obs-apply-scene"))).toHaveLength(10); + }); + it("refuses to restore before any snapshot", async () => { const result = await harness.call("obs-restore"); diff --git a/src/tools/snapshots.ts b/src/tools/snapshots.ts index a27868e..b2c321c 100644 --- a/src/tools/snapshots.ts +++ b/src/tools/snapshots.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: GPL-2.0-only */ import crypto from "node:crypto"; -import { mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; +import { closeSync, mkdirSync, openSync, readFileSync, renameSync, rmSync, statSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import type { CallToolResult, McpServer } from "@modelcontextprotocol/server"; import { z } from "zod"; @@ -39,6 +39,8 @@ export type Snapshot = { id: string; /** The OBS WebSocket URL the snapshot was taken from. */ obsUrl?: string; + /** Taken automatically before a change, not by a user. */ + auto?: boolean; label?: string; takenAt: string; inputs: InputState[]; @@ -48,8 +50,11 @@ export type Snapshot = { /** A change obs-restore would make, and the request that makes it. */ export type RestoreChange = { target: string; field: string; request: BatchRequest }; -/** Kept on disk across every OBS instance, newest last. */ +/** Kept on disk across every OBS instance, newest last: snapshots users take, and automatic ones. */ const MAX_SNAPSHOTS = 20; +const MAX_AUTO_SNAPSHOTS = 10; +const LOCK_WAIT_MS = 2_000; +const LOCK_STALE_MS = 10_000; const SNAPSHOT_FILE = "snapshots.json"; /** The transform fields SetSceneItemTransform accepts; the rest are computed by OBS. */ @@ -72,6 +77,29 @@ function errorResult(text: string): CallToolResult { } /** Deep equality for JSON values, with a tolerance for floating-point numbers. */ +/** + * OBS keeps transforms as 32-bit floats, so 100.1 reads back as 100.09999847. + * Two values match when they are the same 32-bit float. + */ +export function sameFloat32(a: unknown, b: unknown): boolean { + if (typeof a === "number" && typeof b === "number") return Math.fround(a) === Math.fround(b) || Math.abs(a - b) < 1e-6; + return same(a, b); +} + +/** OBS rounds scene item positions to half a pixel (204.37 reads back as 204.5, observed on OBS 32.2.2). */ +const POSITION_FIELDS = new Set(["positionX", "positionY"]); + +/** Whether a transform field OBS read back matches the value asked for. */ +export function sameTransformValue(key: string, a: unknown, b: unknown): boolean { + if (POSITION_FIELDS.has(key) && typeof a === "number" && typeof b === "number") return Math.abs(a - b) <= 0.5; + return sameFloat32(a, b); +} + +export function sameTransform(a: JsonObject, b: JsonObject): boolean { + const keys = new Set([...Object.keys(a), ...Object.keys(b)]); + return [...keys].every((key) => sameTransformValue(key, a[key], b[key])); +} + export function same(a: unknown, b: unknown): boolean { if (typeof a === "number" && typeof b === "number") return Math.abs(a - b) < 1e-6; if (Array.isArray(a) && Array.isArray(b)) return a.length === b.length && a.every((value, index) => same(value, b[index])); @@ -96,21 +124,81 @@ function snapshotFile(): string { } function loadAll(): Snapshot[] { + const path = snapshotFile(); + let text: string; try { - const parsed: unknown = JSON.parse(readFileSync(snapshotFile(), "utf8")); - return Array.isArray(parsed) ? parsed.filter((entry): entry is Snapshot => isObject(entry) && typeof entry.id === "string") : []; + text = readFileSync(path, "utf8"); } catch { return []; } + try { + const parsed: unknown = JSON.parse(text); + if (!Array.isArray(parsed)) throw new Error("not a list"); + return parsed.filter((entry): entry is Snapshot => isObject(entry) && typeof entry.id === "string"); + } catch (error) { + // Keep the unreadable file instead of letting the next save replace the whole history. + const backup = `${path}.unreadable-${Date.now()}`; + try { + renameSync(path, backup); + logger.error(`${path} could not be read (${error instanceof Error ? error.message : String(error)}); moved it to ${backup}`); + } catch { + // Another server moved it first. + } + return []; + } +} + +/** Automatic snapshots (taken before a change) are capped separately, so they never push out ones a user took. */ +function prune(list: Snapshot[]): Snapshot[] { + const autoKept = new Set(list.filter(({ auto }) => auto).slice(-MAX_AUTO_SNAPSHOTS)); + const userKept = new Set(list.filter(({ auto }) => !auto).slice(-MAX_SNAPSHOTS)); + return list.filter((snapshot) => autoKept.has(snapshot) || userKept.has(snapshot)); +} + +function sleepSync(ms: number): void { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +/** + * Runs `change` with the snapshot file locked, so two server processes saving + * at once cannot lose each other's snapshot. A lock left by a crashed process + * is broken after LOCK_STALE_MS. + */ +function withFileLock(change: () => void): void { + const lock = `${snapshotFile()}.lock`; + mkdirSync(stateDirectory(), { recursive: true, mode: 0o700 }); + const deadline = Date.now() + LOCK_WAIT_MS; + for (;;) { + try { + closeSync(openSync(lock, "wx", 0o600)); + break; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; + try { + if (Date.now() - statSync(lock).mtimeMs > LOCK_STALE_MS) rmSync(lock, { force: true }); + } catch { + // Released meanwhile. + } + if (Date.now() > deadline) throw new Error(`the snapshot file is locked by another process (${lock})`); + sleepSync(25); + } + } + try { + change(); + } finally { + rmSync(lock, { force: true }); + } } -function saveAll(list: Snapshot[]): void { +function saveSnapshot(snapshot: Snapshot): void { const path = snapshotFile(); try { - mkdirSync(stateDirectory(), { recursive: true, mode: 0o700 }); - const temporary = `${path}.${process.pid}.tmp`; - writeFileSync(temporary, `${JSON.stringify(list.slice(-MAX_SNAPSHOTS), null, 2)}\n`, { mode: 0o600 }); - renameSync(temporary, path); + withFileLock(() => { + const list = prune([...loadAll(), snapshot]); + const temporary = `${path}.${process.pid}.tmp`; + writeFileSync(temporary, `${JSON.stringify(list, null, 2)}\n`, { mode: 0o600 }); + renameSync(temporary, path); + }); } catch (error) { logger.error(`Could not save snapshots to ${path}: ${error instanceof Error ? error.message : String(error)}`); } @@ -183,7 +271,7 @@ export async function readInputs(client: OBSWebSocketClient, inputNames: string[ */ export async function takeSnapshot( client: OBSWebSocketClient, - options: { scenes?: string[]; inputs?: string[]; label?: string }, + options: { scenes?: string[]; inputs?: string[]; label?: string; auto?: boolean }, ): Promise { let scenes = options.scenes; if (!scenes && !options.inputs) { @@ -203,12 +291,13 @@ export async function takeSnapshot( const snapshot: Snapshot = { id: crypto.randomUUID().slice(0, 8), obsUrl: client.getConnectionStatus().url, + ...(options.auto ? { auto: true } : {}), ...(options.label ? { label: options.label } : {}), takenAt: new Date().toISOString(), inputs: await readInputs(client, inputNames), items, }; - saveAll([...loadAll(), snapshot]); + saveSnapshot(snapshot); return snapshot; } @@ -218,12 +307,17 @@ export async function planRestore( snapshot: Snapshot, ): Promise<{ changes: RestoreChange[]; missing: string[] }> { const sceneNames = [...new Set(snapshot.items.map(({ sceneName }) => sceneName))]; - const [currentItems, currentInputs] = await Promise.all([ - readItems(client, sceneNames).catch(() => [] as ItemState[]), + // Read each scene on its own, so one deleted scene does not hide the others. + const [perScene, currentInputs] = await Promise.all([ + Promise.all(sceneNames.map((sceneName) => readItems(client, [sceneName]).then((items) => ({ sceneName, items }), () => ({ sceneName, items: null })))), readInputs(client, snapshot.inputs.map(({ inputName }) => inputName)), ]); + const readableScenes = perScene.filter(({ items }) => items !== null).map(({ sceneName }) => sceneName); + const currentItems = perScene.flatMap(({ items }) => items ?? []); const changes: RestoreChange[] = []; - const missing: string[] = []; + const missing: string[] = perScene + .filter(({ items }) => items === null) + .map(({ sceneName }) => `scene ${sceneName} no longer exists; its items were not restored`); for (const saved of snapshot.inputs) { const current = currentInputs.find(({ inputName }) => inputName === saved.inputName); @@ -248,7 +342,7 @@ export async function planRestore( } } - for (const saved of snapshot.items) { + for (const saved of snapshot.items.filter(({ sceneName }) => readableScenes.includes(sceneName))) { const current = currentItems.find(({ sceneName, sceneItemId }) => sceneName === saved.sceneName && sceneItemId === saved.sceneItemId); const target = `${saved.sceneName} › ${saved.sourceName} (#${saved.sceneItemId})`; if (!current) { @@ -256,7 +350,7 @@ export async function planRestore( continue; } const selector = { sceneName: saved.sceneName, sceneItemId: saved.sceneItemId }; - if (!same(saved.transform, current.transform)) { + if (!sameTransform(saved.transform, current.transform)) { changes.push({ target, field: "transform", request: { requestType: "SetSceneItemTransform", requestData: { ...selector, sceneItemTransform: saved.transform } } }); } if (saved.enabled !== current.enabled) { @@ -267,22 +361,33 @@ export async function planRestore( } } - // Reorder last, bottom to top, so earlier moves do not shift later ones. - const moved = snapshot.items - .filter((saved) => { - const current = currentItems.find(({ sceneName, sceneItemId }) => sceneName === saved.sceneName && sceneItemId === saved.sceneItemId); - return current && current.index !== saved.index; - }) - .sort((a, b) => a.index - b.index); - for (const saved of moved) { - changes.push({ - target: `${saved.sceneName} › ${saved.sourceName} (#${saved.sceneItemId})`, - field: `order (to ${saved.index})`, - request: { requestType: "SetSceneItemIndex", requestData: { sceneName: saved.sceneName, sceneItemId: saved.sceneItemId, sceneItemIndex: saved.index } }, + // Order last. The saved items that survive go back into the positions they + // occupy now, in their saved order, so items added since keep their place. + // Moving every item of the scene to its final index, bottom to top, gives + // that order whatever moves OBS makes along the way. + for (const sceneName of readableScenes) { + const now = currentItems.filter((item) => item.sceneName === sceneName).sort((a, b) => a.index - b.index); + const survivors = snapshot.items + .filter((saved) => saved.sceneName === sceneName && now.some(({ sceneItemId }) => sceneItemId === saved.sceneItemId)) + .sort((a, b) => a.index - b.index); + const survivorIds = new Set(survivors.map(({ sceneItemId }) => sceneItemId)); + let next = 0; + const target = now.map((item) => { + if (!survivorIds.has(item.sceneItemId)) return item; + const wanted = survivors[next++]!; + return now.find(({ sceneItemId }) => sceneItemId === wanted.sceneItemId)!; + }); + if (target.every((item, index) => item.sceneItemId === now[index]!.sceneItemId)) continue; + target.forEach((item, index) => { + changes.push({ + target: `${sceneName} › ${item.sourceName} (#${item.sceneItemId})`, + field: `order (to ${index})`, + request: { requestType: "SetSceneItemIndex", requestData: { sceneName, sceneItemId: item.sceneItemId, sceneItemIndex: index } }, + }); }); } - for (const sceneName of sceneNames) { + for (const sceneName of readableScenes) { const savedIds = new Set(snapshot.items.filter((item) => item.sceneName === sceneName).map(({ sceneItemId }) => sceneItemId)); for (const item of currentItems.filter((current) => current.sceneName === sceneName && !savedIds.has(current.sceneItemId))) { missing.push(`${sceneName} › ${item.sourceName} (#${item.sceneItemId}) was added since; it is left in place`); diff --git a/src/tools/take-monitor.test.ts b/src/tools/take-monitor.test.ts index fe3f6f2..c367a5a 100644 --- a/src/tools/take-monitor.test.ts +++ b/src/tools/take-monitor.test.ts @@ -194,6 +194,21 @@ describe("TakeMonitor", () => { expect(second.summary().warnings).toEqual([]); }); + it("leaves paused time out of take times, and ignores audio while paused", async () => { + const take = await startMonitor(); + fakeObs.sendEvent("RecordStateChanged", { outputActive: true, outputState: "OBS_WEBSOCKET_OUTPUT_PAUSED" }); + await vi.waitFor(() => expect(take.isPaused()).toBe(true)); + meters([{ inputName: "Chrome", peak: 0.5 }]); + await new Promise((resolve) => setTimeout(resolve, 300)); + fakeObs.sendEvent("RecordStateChanged", { outputActive: true, outputState: "OBS_WEBSOCKET_OUTPUT_RESUMED" }); + await vi.waitFor(() => expect(take.isPaused()).toBe(false)); + + const summary = await take.stop(); + + expect(summary.durationSeconds).toBeLessThan(0.25); + expect(summary.warnings.filter(({ kind }) => kind === "audio")).toEqual([]); + }); + it("writes a take log next to the recording with chapters and without -Infinity", async () => { const take = await startMonitor(); take.mark("Intro"); diff --git a/src/tools/take-monitor.ts b/src/tools/take-monitor.ts index e5c7c3d..dbabe23 100644 --- a/src/tools/take-monitor.ts +++ b/src/tools/take-monitor.ts @@ -123,6 +123,8 @@ export class TakeMonitor { private pictureChecks: "on" | "off" = "off"; private programScene: string | null = null; private recordingPath: string | null = null; + private pausedMs = 0; + private pausedSince: number | null = null; private readonly timers: NodeJS.Timeout[] = []; private readonly busy = new Set(); private releaseMeters: (() => void) | null = null; @@ -141,8 +143,15 @@ export class TakeMonitor { } /** Seconds since the monitor started. */ + /** Seconds of recorded time since the take started: paused time is left out, as it is from the file. */ now(): number { - return round(((this.stoppedAtMs ?? Date.now()) - this.startedAtMs) / 1000); + const end = this.stoppedAtMs ?? Date.now(); + const paused = this.pausedMs + (this.pausedSince === null ? 0 : end - this.pausedSince); + return round((end - this.startedAtMs - paused) / 1000); + } + + isPaused(): boolean { + return this.pausedSince !== null; } async start(): Promise { @@ -161,6 +170,12 @@ export class TakeMonitor { }); this.listen("RecordStateChanged", (data) => { if (typeof data.outputPath === "string" && data.outputPath) this.recordingPath = data.outputPath; + if (data.outputState === "OBS_WEBSOCKET_OUTPUT_PAUSED" && this.pausedSince === null) { + this.pausedSince = Date.now(); + } else if (data.outputState === "OBS_WEBSOCKET_OUTPUT_RESUMED" && this.pausedSince !== null) { + this.pausedMs += Date.now() - this.pausedSince; + this.pausedSince = null; + } if (data.outputActive === false && !this.stopExpected && data.outputState === "OBS_WEBSOCKET_OUTPUT_STOPPED") { this.warn("output", "The recording stopped before it was asked to"); } @@ -310,7 +325,8 @@ export class TakeMonitor { } private onMeters(data: JsonObject): void { - if (this.stoppedAtMs !== null || !Array.isArray(data.inputs)) return; + // While paused nothing is written to the file, so nothing heard then is in the recording. + if (this.stoppedAtMs !== null || this.isPaused() || !Array.isArray(data.inputs)) return; for (const input of data.inputs) { if (!isObject(input) || typeof input.inputName !== "string" || !Array.isArray(input.inputLevelsMul)) continue; // Each channel is [magnitude, peak, input peak]; peak is after volume and mute. @@ -372,7 +388,7 @@ export class TakeMonitor { } private async samplePicture(): Promise { - if (!this.programScene) return; + if (!this.programScene || this.isPaused()) return; const response = await this.client.sendRequest("GetSourceScreenshot", { sourceName: this.programScene, imageFormat: "ppm", diff --git a/src/tools/takes.test.ts b/src/tools/takes.test.ts index 219085c..364ab8e 100644 --- a/src/tools/takes.test.ts +++ b/src/tools/takes.test.ts @@ -9,9 +9,9 @@ import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { EventSubscription } from "../client.js"; import { FakeOBSRequestError } from "../../test/support/fake-obs-server.js"; -import { healthyObsState, servePreflightState, type FakeObsState } from "../../test/support/fake-obs-state.js"; +import { healthyObsState, servePreflightState, serveRecordOutput, type FakeObsState } from "../../test/support/fake-obs-state.js"; import { resultText, startMcpHarness, type HarnessOptions, type McpHarness } from "../../test/support/mcp-harness.js"; -import { currentTake } from "./takes.js"; +import { currentTake, stopTake } from "./takes.js"; let harness: McpHarness; let recordDirectory: string; @@ -26,15 +26,7 @@ async function startHarness(options: HarnessOptions = {}): Promise { harness = await startMcpHarness({ platform: "macos", ...options }); servePreflightState(harness.fakeObs, () => state); const { fakeObs } = harness; - fakeObs.respondWith("StartRecord", () => { - setTimeout(() => fakeObs.sendEvent("RecordStateChanged", { - outputActive: true, - outputState: "OBS_WEBSOCKET_OUTPUT_STARTED", - outputPath: clipPath, - }), 5); - return {}; - }); - fakeObs.respondWith("StopRecord", () => ({ outputPath: clipPath })); + serveRecordOutput(fakeObs, () => clipPath); fakeObs.respondWith("CreateRecordChapter", () => ({})); fakeObs.respondWith("SetCurrentProfile", () => ({})); fakeObs.respondWith("SetProfileParameter", () => ({})); @@ -49,8 +41,9 @@ beforeEach(() => { }); afterEach(async () => { + // Stop directly: a test may have declined the confirmation obs-take-stop asks for. const take = currentTake(harness.obsClient); - if (take) await harness.call("obs-take-stop", { confirm: true }); + if (take) await stopTake(harness.obsClient, take); await harness.close(); rmSync(recordDirectory, { recursive: true, force: true }); vi.restoreAllMocks(); @@ -91,6 +84,35 @@ describe("recording takes", () => { expect(sent("StartRecord")).toBe(1); }); + it("refuses a second take started while the first is still starting", async () => { + await startHarness(); + + const [first, second] = await Promise.all([harness.call("obs-take-start"), harness.call("obs-take-start")]); + + const texts = [resultText(first), resultText(second)]; + expect(texts.filter((text) => text.includes("is recording"))).toHaveLength(1); + expect(texts.some((text) => /being started|already recording/.test(text))).toBe(true); + expect(sent("StartRecord")).toBe(1); + }); + + it("waits for OBS to finish the file before reporting it", async () => { + await startHarness(); + // StopRecord answers before the file is finished; only the STOPPED event carries the final path here. + harness.fakeObs.respondWith("StopRecord", () => { + setTimeout(() => harness.fakeObs.sendEvent("RecordStateChanged", { + outputActive: false, + outputState: "OBS_WEBSOCKET_OUTPUT_STOPPED", + outputPath: clipPath, + }), 50); + return {}; + }); + await harness.call("obs-take-start"); + + const stop = await harness.call("obs-take-stop"); + + expect(resultText(stop)).toContain(`to ${clipPath}`); + }); + it("does not start when preflight fails", async () => { await startHarness(); state.recordDirectory = join(recordDirectory, "missing"); @@ -113,6 +135,8 @@ describe("recording takes", () => { const readOnly = await harness.call("obs-batch", { requests: [{ requestType: "GetStats" }] }); expect(resultText(profile)).toMatch(/is recording, and changing this now would switch profiles under it/); + expect(resultText(await harness.call("obs-set-output-settings", { outputName: "adv_file_output", outputSettings: {} }))) + .toContain("would change an output's settings"); expect(resultText(batch)).toContain("would send SetProfileParameter"); expect(readOnly.isError).toBeFalsy(); expect(sent("SetCurrentProfile")).toBe(0); diff --git a/src/tools/takes.ts b/src/tools/takes.ts index 15e2a43..4f305e4 100644 --- a/src/tools/takes.ts +++ b/src/tools/takes.ts @@ -59,18 +59,25 @@ export async function inspectRecording(outputPath: string, measureLoudness: bool const streams = Array.isArray(probe.streams) ? probe.streams.filter(isObject) : []; const format = isObject(probe.format) ? probe.format : {}; + const duration = Number(format.duration); + if (!Number.isFinite(duration)) return { skipped: "ffprobe could not read the file's length; it may be incomplete" }; const check: ClipCheck = { - durationSeconds: Number(format.duration), + durationSeconds: duration, audioStreams: streams.filter((stream) => stream.codec_type === "audio").length, }; if (!measureLoudness || check.audioStreams === 0) return check; + // Every audio track, since a recording can write several: the loudest one counts. try { - const { stderr } = await execFileAsync("ffmpeg", [ - "-hide_banner", "-nostats", "-i", outputPath, "-map", "0:a:0", "-af", "volumedetect", "-f", "null", "-", - ], { timeout: 60_000 }); - const match = /max_volume:\s*(-?[\d.]+|-inf) dB/.exec(stderr); - if (match?.[1]) check.maxVolumeDb = match[1] === "-inf" ? -Infinity : Number(match[1]); + for (let track = 0; track < (check.audioStreams ?? 0); track += 1) { + const { stderr } = await execFileAsync("ffmpeg", [ + "-hide_banner", "-nostats", "-i", outputPath, "-map", `0:a:${track}`, "-af", "volumedetect", "-f", "null", "-", + ], { timeout: 60_000 }); + const match = /max_volume:\s*(-?[\d.]+|-inf) dB/.exec(stderr); + if (!match?.[1]) continue; + const level = match[1] === "-inf" ? -Infinity : Number(match[1]); + check.maxVolumeDb = Math.max(check.maxVolumeDb ?? -Infinity, level); + } } catch (error) { check.skipped = `ffmpeg loudness check failed: ${errorMessage(error)}`; } @@ -92,6 +99,8 @@ export type Take = { }; const takes = new WeakMap(); +/** Clients with a take being started: preflight and the start take time, and a second start must not slip in. */ +const starting = new WeakSet(); export function currentTake(client: OBSWebSocketClient): Take | undefined { return takes.get(client); @@ -112,6 +121,20 @@ export async function startTake( const refuse = (result: CallToolResult) => ({ ok: false as const, result }); const running = currentTake(client); if (running) return refuse(errorResult(`Take ${running.id} is already recording; stop it with obs-take-stop first`)); + if (starting.has(client)) return refuse(errorResult("Another take is being started; wait for it, then stop it with obs-take-stop")); + starting.add(client); + try { + return await startTakeReserved(client, options); + } finally { + starting.delete(client); + } +} + +async function startTakeReserved( + client: OBSWebSocketClient, + options: StartTakeOptions, +): Promise<{ ok: true; take: Take } | { ok: false; result: CallToolResult }> { + const refuse = (result: CallToolResult) => ({ ok: false as const, result }); const preflight = await runPreflight(client, { minFreeDiskMb: options.minFreeDiskMb, expectSilent: options.expectSilent }); if (!preflight.ready) { @@ -153,6 +176,26 @@ export async function markTake(client: OBSWebSocketClient, take: Take, name: str } } +const STOP_TIMEOUT_MS = 10_000; + +/** Resolves with OBS's RecordStateChanged STOPPED event, or null after the timeout or cancel. */ +function recordStopped(client: OBSWebSocketClient, timeoutMs: number): { done: Promise<{ outputPath?: string } | null>; cancel(): void } { + let finish: (value: { outputPath?: string } | null) => void = () => undefined; + const done = new Promise<{ outputPath?: string } | null>((resolve) => { finish = resolve; }); + const listener = (data: unknown) => { + if (!isObject(data) || data.outputState !== "OBS_WEBSOCKET_OUTPUT_STOPPED") return; + end(typeof data.outputPath === "string" ? { outputPath: data.outputPath } : {}); + }; + const timer = setTimeout(() => end(null), timeoutMs); + const end = (value: { outputPath?: string } | null) => { + clearTimeout(timer); + client.off("RecordStateChanged", listener); + finish(value); + }; + client.on("RecordStateChanged", listener); + return { done, cancel: () => end(null) }; +} + /** * Stops the recording, stops the monitor, checks the file, and writes the * take log. Never throws, so callers can stop from a finally block. @@ -165,13 +208,19 @@ export async function stopTake( take.monitor.expectStop(); let outputPath: string | undefined; let stopError: string | undefined; + // StopRecord answers before OBS has finished writing the file; wait for the + // STOPPED state before inspecting it. + const finished = recordStopped(client, STOP_TIMEOUT_MS); try { const stopped: unknown = await client.sendRequest("StopRecord"); if (isObject(stopped) && typeof stopped.outputPath === "string") outputPath = stopped.outputPath; } catch (error) { // The recording may have stopped already; OBS reported the file when it did. stopError = errorMessage(error); + finished.cancel(); } + const stoppedState = await finished.done; + outputPath ??= stoppedState?.outputPath; const summary = await take.monitor.stop(); if (takes.get(client) === take) takes.delete(client); outputPath ??= take.monitor.outputPath() ?? undefined; @@ -256,6 +305,7 @@ export const LOCKED_TOOLS: Readonly> = { "obs-set-record-directory": "change the recording directory", "obs-set-video-settings": "change the canvas and frame rate", "obs-set-stream-service-settings": "change the stream service", + "obs-set-output-settings": "change an output's settings", }; const LOCKED_REQUESTS = new Set([ @@ -268,6 +318,7 @@ const LOCKED_REQUESTS = new Set([ "SetRecordDirectory", "SetVideoSettings", "SetStreamServiceSettings", + "SetOutputSettings", ]); function lockedAction(name: string, args: unknown): string | undefined { @@ -344,14 +395,19 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void const { take } = started; // Other clients sharing this server cannot change OBS until the take stops. const holder = callerName(server, ctx); - if (claim(client, holder, 240, `recording take ${take.id}`).ok) take.leaseHolder = holder; + const claimed = claim(client, holder, 240, `recording take ${take.id}`); + if (claimed.ok) take.leaseHolder = holder; const status = take.monitor.summary(); return { content: [{ type: "text", text: `Take ${take.id} is recording${take.locked ? " (profile and output changes locked)" : ""}. ` + `Watching audio${status.pictureChecks === "on" ? ", frames, and picture" : " and frames"}; ` - + "stop it with obs-take-stop", + + "stop it with obs-take-stop" + + (claimed.ok ? "" : claimed.lease + ? `. ${claimed.lease.holder} already has control of OBS` + : ". Other clients sharing this server can still change OBS: this client did not identify itself " + + "(over HTTP, add ?client= to the URL)"), }], structuredContent: { takeId: take.id, locked: take.locked, pictureChecks: status.pictureChecks, chapterNotes: take.chapterNotes }, }; diff --git a/src/tools/toolset-tools.test.ts b/src/tools/toolset-tools.test.ts index 7f17061..96c8be6 100644 --- a/src/tools/toolset-tools.test.ts +++ b/src/tools/toolset-tools.test.ts @@ -72,6 +72,8 @@ describe("dynamic tool groups", () => { const names = await toolNames(); expect(names).toContain("obs-get-record-status"); expect(names).not.toContain("obs-start-record"); + const { tools } = await harness!.mcpClient.listTools(); + expect(tools.filter(({ annotations }) => annotations?.readOnlyHint !== true).map(({ name }) => name)).toEqual([]); }); it("adds no group tools without OBS_MCP_DYNAMIC_TOOLSETS", async () => { diff --git a/src/tools/toolset-tools.ts b/src/tools/toolset-tools.ts index 7975e59..9b67785 100644 --- a/src/tools/toolset-tools.ts +++ b/src/tools/toolset-tools.ts @@ -83,7 +83,8 @@ export function initialize(server: McpServer, registry: readonly RegisteredTool[ ? "Turn on groups of OBS tools for this session; the client reloads its tool list. See obs-list-toolsets" : "Turn off groups of OBS tools for this session to keep the tool list short", inputSchema: z.object({ groups: GroupList.describe("Groups from obs-list-toolsets") }), - annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: true, openWorldHint: false }, + // Read-only toward OBS: they only change which tools this session lists. + annotations: { readOnlyHint: true, destructiveHint: false, idempotentHint: true, openWorldHint: false }, }, async ({ groups }): Promise => { const changed = setGroups(registry, groups, enabled); diff --git a/src/tools/trim.test.ts b/src/tools/trim.test.ts index 2411962..2529fad 100644 --- a/src/tools/trim.test.ts +++ b/src/tools/trim.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: GPL-2.0-only */ import { execFileSync } from "node:child_process"; -import { existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; @@ -151,6 +151,28 @@ describe.skipIf(!ffmpegAvailable)("obs-trim-take and obs-contact-sheet", () => { expect(tone).toMatchObject({ cuts: [], audioConsidered: true }); }, 30_000); + it("keeps chapter names with control characters from breaking the chapter list", async () => { + const tricky = join(directory, "tricky.mp4"); + const metadata = join(directory, "tricky.txt"); + // A title holding \r would otherwise end the line and start a forged chapter. + writeFileSync(metadata, ";FFMETADATA1\n[CHAPTER]\nTIMEBASE=1/1000\nSTART=0\nEND=11500\ntitle=Intro\\\r[CHAPTER]\n" + + "[CHAPTER]\nTIMEBASE=1/1000\nSTART=11500\nEND=14000\ntitle=Answer\n"); + execFileSync("ffmpeg", ["-v", "error", "-y", "-i", clip, "-f", "ffmetadata", "-i", metadata, "-map", "0", "-map_chapters", "1", "-c", "copy", tricky]); + writeFileSync(join(directory, "tricky.take.json"), "{}"); + + const result = await harness.call("obs-trim-take", { path: "tricky.mp4", apply: true, contactSheet: false }); + + expect(result.structuredContent).toMatchObject({ outputChapters: 2 }); + }, 30_000); + + it("refuses an output path that is a dangling symlink", async () => { + symlinkSync(join(tmpdir(), "obs-mcp-nowhere", "escape.mp4"), join(directory, "link.mp4")); + + const result = await harness.call("obs-trim-take", { path: "take.mp4", apply: true, outputPath: "link.mp4", contactSheet: false }); + + expect(resultText(result)).toContain("already exists"); + }, 30_000); + it("refuses files outside the OBS recording directory", async () => { const outside = mkdtempSync(join(tmpdir(), "obs-mcp-outside-")); try { diff --git a/src/tools/trim.ts b/src/tools/trim.ts index 2a70830..3457642 100644 --- a/src/tools/trim.ts +++ b/src/tools/trim.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: GPL-2.0-only */ import { execFile, spawn } from "node:child_process"; -import { existsSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { lstatSync, mkdtempSync, readFileSync, realpathSync, renameSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { basename, dirname, extname, isAbsolute, join, relative } from "node:path"; import { promisify } from "node:util"; @@ -52,6 +52,15 @@ function errorResult(text: string): CallToolResult { const round = (seconds: number) => Math.round(seconds * 100) / 100; +function pathTaken(path: string): boolean { + try { + lstatSync(path); + return true; + } catch { + return false; + } +} + function mergeSpans(spans: Span[]): Span[] { const sorted = [...spans].sort((a, b) => a.start - b.start); const merged: Span[] = []; @@ -195,7 +204,11 @@ function stillSpans(path: string, info: Probe, minIdleSeconds: number, silenceAr "-map", "0:v:0", "-vf", `fps=${SAMPLE_FPS},scale=${SAMPLE_WIDTH}:${height},format=gray`, "-f", "rawvideo", "pipe:1", ...silenceArgs, ], { stdio: ["ignore", "pipe", "pipe"] }); - const timer = setTimeout(() => child.kill("SIGKILL"), 300_000); + let timedOut = false; + const timer = setTimeout(() => { + timedOut = true; + child.kill("SIGKILL"); + }, 300_000); const tracker = new StillTracker(minIdleSeconds); let stderr = ""; let pending: Buffer = Buffer.alloc(0); @@ -219,6 +232,10 @@ function stillSpans(path: string, info: Probe, minIdleSeconds: number, silenceAr }); child.on("close", (code) => { clearTimeout(timer); + if (timedOut) { + reject(new Error("Finding still stretches took longer than 5 minutes; try a shorter recording")); + return; + } if (code !== 0) { reject(new Error(`ffmpeg exited with ${code}: ${stderr.trim().split("\n").slice(-2).join(" ")}`)); return; @@ -259,7 +276,9 @@ async function videoEncoderArgs(): Promise { } function ffmetadata(chapters: Chapter[], duration: number): string { - const escape = (text: string) => text.replace(/([=;#\\\n])/g, "\\$1"); + // ffmetadata ends a line at \r as well as \n: drop control characters, then escape its special characters. + // eslint-disable-next-line no-control-regex + const escape = (text: string) => text.replace(/[\u0000-\u001f\u007f]/g, " ").replace(/([=;#\\])/g, "\\$1"); const lines = [";FFMETADATA1"]; chapters.forEach((chapter, index) => { const end = chapters[index + 1]?.at ?? duration; @@ -405,8 +424,17 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void false, ); if (outputPath === path) return errorResult("The trimmed copy cannot replace the original"); - if (existsSync(outputPath)) return errorResult(`${outputPath} already exists; choose another outputPath`); - await exportTrimmed(path, outputPath, keep, info.audioStreams, newChapters, plan.trimmedSeconds); + // lstat also sees a dangling symlink, which ffmpeg would write through, possibly outside the directory. + if (pathTaken(outputPath)) return errorResult(`${outputPath} already exists; choose another outputPath`); + // Export under a temporary name so a failed or interrupted export leaves nothing that looks finished. + const partial = join(dirname(outputPath), `.${basename(outputPath)}.partial${extname(outputPath)}`); + try { + await exportTrimmed(path, partial, keep, info.audioStreams, newChapters, plan.trimmedSeconds); + if (pathTaken(outputPath)) return errorResult(`${outputPath} appeared while exporting; the export is discarded`); + renameSync(partial, outputPath); + } finally { + rmSync(partial, { force: true }); + } const result = await probe(outputPath); lines.push(`Exported ${outputPath}: ${round(result.duration)}s, ${result.chapters.length} chapter(s)`); diff --git a/src/tools/workflows.test.ts b/src/tools/workflows.test.ts index 6ff4d82..45fed5e 100644 --- a/src/tools/workflows.test.ts +++ b/src/tools/workflows.test.ts @@ -9,7 +9,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { FakeOBSRequestError, OBS_OP } from "../../test/support/fake-obs-server.js"; -import { healthyObsState, servePreflightState, type FakeObsState } from "../../test/support/fake-obs-state.js"; +import { healthyObsState, servePreflightState, serveRecordOutput, type FakeObsState } from "../../test/support/fake-obs-state.js"; import { resultText, startMcpHarness, type McpHarness } from "../../test/support/mcp-harness.js"; function hasFfmpeg(): boolean { @@ -50,15 +50,7 @@ async function startHarness(platform = "macos"): Promise { harness = await startMcpHarness({ platform }); servePreflightState(harness.fakeObs, () => state); const { fakeObs } = harness; - fakeObs.respondWith("StartRecord", () => { - setTimeout(() => fakeObs.sendEvent("RecordStateChanged", { - outputActive: true, - outputState: "OBS_WEBSOCKET_OUTPUT_STARTED", - outputPath: clipPath, - }), 5); - return {}; - }); - fakeObs.respondWith("StopRecord", () => ({ outputPath: clipPath })); + serveRecordOutput(fakeObs, () => clipPath); fakeObs.respondWith("CreateRecordChapter", () => ({})); } @@ -191,6 +183,16 @@ describe("obs-record-clip", () => { }); }); + it("refuses a chapter at or after the end, which would keep it recording", async () => { + await startHarness(); + + const result = await harness.call("obs-record-clip", { durationSeconds: 5, chapters: [{ atSeconds: 3600, name: "Late" }] }); + + expect(result.isError).toBe(true); + expect(resultText(result)).toContain('"Late" start at or after the 5s clip ends'); + expect(requestTypes()).not.toContain("StartRecord"); + }); + it("rejects clips longer than a client timeout allows", async () => { await startHarness(); diff --git a/src/tools/workflows.ts b/src/tools/workflows.ts index eb46037..16abf84 100644 --- a/src/tools/workflows.ts +++ b/src/tools/workflows.ts @@ -92,7 +92,7 @@ async function captureWindow( } // Changing an existing capture can break a working setup; save it first so obs-restore can undo this. const snapshot = existing - ? await takeSnapshot(client, { inputs: [args.inputName], scenes: [args.sceneName], label: "before obs-capture-window" }) + ? await takeSnapshot(client, { inputs: [args.inputName], scenes: [args.sceneName], label: "before obs-capture-window", auto: true }) .catch(() => undefined) : undefined; const type = args.window ? SCK_WINDOW_CAPTURE : SCK_APPLICATION_CAPTURE; @@ -201,6 +201,12 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: false, openWorldHint: false }, }, async (args) => { + // A chapter past the end would keep recording until it, beyond what the client waits for. + const late = args.chapters.filter(({ atSeconds }) => atSeconds >= args.durationSeconds); + if (late.length > 0) { + return errorResult(`Not recording: chapter(s) ${late.map(({ name }) => `"${name}"`).join(", ")} start at or after the ` + + `${args.durationSeconds}s clip ends`); + } try { return await recordClip(client, args); } catch (error) { diff --git a/test/e2e/http.e2e.test.ts b/test/e2e/http.e2e.test.ts index a387d81..172ae26 100644 --- a/test/e2e/http.e2e.test.ts +++ b/test/e2e/http.e2e.test.ts @@ -80,6 +80,14 @@ describe("compiled HTTP server", () => { const codex = await agent(url, "codex-mcp-client"); const claude = await agent(url, "claude-code"); + // A 2025-era client over HTTP is named by its URL. + const legacy = new Client({ name: "legacy", version: "1.0.0" }, { versionNegotiation: { mode: "legacy" } }); + await legacy.connect(new StreamableHTTPClientTransport(new URL(`${url}?client=codex-legacy`))); + clients.push(legacy); + expect(text(await legacy.callTool({ name: "obs-control-status", arguments: {} }) as CallToolResult)) + .toContain("any client may change OBS"); + expect(text(await legacy.callTool({ name: "obs-claim-control", arguments: {} }) as CallToolResult)).toMatch(/^codex-legacy has control/); + await legacy.callTool({ name: "obs-release-control", arguments: {} }); await vi.waitFor(async () => expect(text(await claude("obs-get-status"))).toContain('"connected": true'), { timeout: 3_000 }); await codex("obs-claim-control", { reason: "recording" }); diff --git a/test/support/fake-obs-scene.ts b/test/support/fake-obs-scene.ts index 7df975e..2cbfef7 100644 --- a/test/support/fake-obs-scene.ts +++ b/test/support/fake-obs-scene.ts @@ -178,7 +178,12 @@ export function serveSceneState(fakeObs: FakeOBSServer, state: FakeSceneState): })()); fakeObs.respondWith("SetSceneItemTransform", (data) => done(() => { const target = item(data); - target.transform = { ...target.transform, ...(data.sceneItemTransform as JsonObject) }; + // OBS stores transforms as 32-bit floats and rounds positions to half a pixel. + const stored = Object.fromEntries(Object.entries(data.sceneItemTransform as JsonObject).map(([key, value]) => { + if (typeof value !== "number" || Number.isInteger(value)) return [key, value]; + return [key, key === "positionX" || key === "positionY" ? Math.round(value * 2) / 2 : Math.fround(value)]; + })); + target.transform = { ...target.transform, ...stored }; })()); fakeObs.respondWith("SetSceneItemEnabled", (data) => done(() => { item(data).enabled = data.sceneItemEnabled as boolean; })()); fakeObs.respondWith("SetSceneItemLocked", (data) => done(() => { item(data).locked = data.sceneItemLocked as boolean; })()); diff --git a/test/support/fake-obs-state.ts b/test/support/fake-obs-state.ts index ad7f28f..2dd2a3c 100644 --- a/test/support/fake-obs-state.ts +++ b/test/support/fake-obs-state.ts @@ -84,3 +84,25 @@ export function servePreflightState(fakeObs: FakeOBSServer, getState: () => Fake })), })); } + +/** + * Answers StartRecord and StopRecord the way OBS does: each returns at once, + * and the RecordStateChanged event that confirms it follows shortly after. + */ +export function serveRecordOutput(fakeObs: FakeOBSServer, outputPath: () => string): void { + const later = (outputState: string, outputActive: boolean) => setTimeout(() => { + try { + fakeObs.sendEvent("RecordStateChanged", { outputActive, outputState, outputPath: outputPath() }); + } catch { + // The connection closed first. + } + }, 5); + fakeObs.respondWith("StartRecord", () => { + later("OBS_WEBSOCKET_OUTPUT_STARTED", true); + return {}; + }); + fakeObs.respondWith("StopRecord", () => { + later("OBS_WEBSOCKET_OUTPUT_STOPPED", false); + return { outputPath: outputPath() }; + }); +} From 1e9b8602e7aeb047c53f5e485edb61b97ba98ac0 Mon Sep 17 00:00:00 2001 From: Tom Main Date: Thu, 24 Sep 2026 01:50:38 -0400 Subject: [PATCH 2/3] Address Codex review: temporary export name, half-pixel positions - 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) Claude-Session: https://claude.ai/code/session_01Vjn8uBEa4mJshaQU3pFuqX --- src/tools/snapshots.test.ts | 10 ++++++++++ src/tools/snapshots.ts | 9 +++++++-- src/tools/trim.test.ts | 10 ++++++++++ src/tools/trim.ts | 5 ++++- 4 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/tools/snapshots.test.ts b/src/tools/snapshots.test.ts index 8f054ea..90cf76a 100644 --- a/src/tools/snapshots.test.ts +++ b/src/tools/snapshots.test.ts @@ -200,6 +200,16 @@ describe("obs-snapshot and obs-restore", () => { expect(list.split("\n").filter((line) => line.includes("before obs-apply-scene"))).toHaveLength(10); }); + it("restores a half-pixel move, which OBS keeps", async () => { + await harness.call("obs-snapshot"); + items[1]!.transform = { ...items[1]!.transform, positionX: 204.5 }; + + const result = await harness.call("obs-restore"); + + expect(resultText(result)).toContain("Chrome (#2): transform"); + expect(items[1]!.transform.positionX).toBe(204); + }); + it("refuses to restore before any snapshot", async () => { const result = await harness.call("obs-restore"); diff --git a/src/tools/snapshots.ts b/src/tools/snapshots.ts index b2c321c..fcb8595 100644 --- a/src/tools/snapshots.ts +++ b/src/tools/snapshots.ts @@ -88,10 +88,15 @@ export function sameFloat32(a: unknown, b: unknown): boolean { /** OBS rounds scene item positions to half a pixel (204.37 reads back as 204.5, observed on OBS 32.2.2). */ const POSITION_FIELDS = new Set(["positionX", "positionY"]); +const toHalfPixel = (value: number) => Math.round(value * 2) / 2; -/** Whether a transform field OBS read back matches the value asked for. */ +/** + * Whether two transform values are the same once OBS stores them: positions + * on the same half pixel, other numbers as the same 32-bit float. A real + * half-pixel move (204 to 204.5) still counts as a difference. + */ export function sameTransformValue(key: string, a: unknown, b: unknown): boolean { - if (POSITION_FIELDS.has(key) && typeof a === "number" && typeof b === "number") return Math.abs(a - b) <= 0.5; + if (POSITION_FIELDS.has(key) && typeof a === "number" && typeof b === "number") return toHalfPixel(a) === toHalfPixel(b); return sameFloat32(a, b); } diff --git a/src/tools/trim.test.ts b/src/tools/trim.test.ts index 2529fad..bcb0fe0 100644 --- a/src/tools/trim.test.ts +++ b/src/tools/trim.test.ts @@ -173,6 +173,16 @@ describe.skipIf(!ffmpegAvailable)("obs-trim-take and obs-contact-sheet", () => { expect(resultText(result)).toContain("already exists"); }, 30_000); + it("never deletes a recording whose name looks like a temporary export", async () => { + // Exported to fresh.mp4, the old temporary name was exactly this file. + const lookalike = join(directory, ".fresh.mp4.partial.mp4"); + execFileSync("cp", [clip, lookalike]); + + await harness.call("obs-trim-take", { path: ".fresh.mp4.partial.mp4", apply: true, outputPath: "fresh.mp4", contactSheet: false }); + + expect(existsSync(lookalike)).toBe(true); + }, 30_000); + it("refuses files outside the OBS recording directory", async () => { const outside = mkdtempSync(join(tmpdir(), "obs-mcp-outside-")); try { diff --git a/src/tools/trim.ts b/src/tools/trim.ts index 3457642..5b9f55e 100644 --- a/src/tools/trim.ts +++ b/src/tools/trim.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: GPL-2.0-only */ import { execFile, spawn } from "node:child_process"; +import crypto from "node:crypto"; import { lstatSync, mkdtempSync, readFileSync, realpathSync, renameSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { basename, dirname, extname, isAbsolute, join, relative } from "node:path"; @@ -427,7 +428,9 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void // lstat also sees a dangling symlink, which ffmpeg would write through, possibly outside the directory. if (pathTaken(outputPath)) return errorResult(`${outputPath} already exists; choose another outputPath`); // Export under a temporary name so a failed or interrupted export leaves nothing that looks finished. - const partial = join(dirname(outputPath), `.${basename(outputPath)}.partial${extname(outputPath)}`); + // A unique name, so it can never be the recording being trimmed, which the finally block would delete. + const partial = join(dirname(outputPath), `.${basename(outputPath)}.${process.pid}-${crypto.randomUUID().slice(0, 8)}.partial${extname(outputPath)}`); + if (partial === path || pathTaken(partial)) return errorResult("Could not choose a temporary name for the export; try again"); try { await exportTrimmed(path, partial, keep, info.audioStreams, newChapters, plan.trimmedSeconds); if (pathTaken(outputPath)) return errorResult(`${outputPath} appeared while exporting; the export is discarded`); From d8cdc8388158f0b52e2914beecbc119bd004df1c Mon Sep 17 00:00:00 2001 From: Tom Main Date: Thu, 24 Sep 2026 02:01:11 -0400 Subject: [PATCH 3/3] Finish the review remediation: trim cancellation, reconnect backoff - 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) Claude-Session: https://claude.ai/code/session_01Vjn8uBEa4mJshaQU3pFuqX --- README.md | 2 +- src/server.ts | 4 +++- src/tools/trim.test.ts | 7 ++++++- src/tools/trim.ts | 42 +++++++++++++++++++++------------------ test/e2e/http.e2e.test.ts | 1 + 5 files changed, 34 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 6c4345e..4946a35 100644 --- a/README.md +++ b/README.md @@ -262,7 +262,7 @@ claude mcp add --transport http obs 'http://127.0.0.1:8765/mcp?client=claude' # In Codex's config, point it at http://127.0.0.1:8765/mcp?client=codex ``` -The server listens only on 127.0.0.1 and rejects requests whose Host or Origin header is not localhost, which blocks DNS rebinding from web pages. Set `OBS_MCP_HTTP_TOKEN` to also require a bearer token. Each HTTP request is served statelessly, so `OBS_MCP_DYNAMIC_TOOLSETS` is ignored over HTTP. +The server listens only on 127.0.0.1 and rejects requests whose Host or Origin header is not localhost, which blocks DNS rebinding from web pages. Without `OBS_MCP_HTTP_TOKEN`, any program running on the machine can control OBS through the server; set it to require a bearer token. Each HTTP request is served statelessly, so `OBS_MCP_DYNAMIC_TOOLSETS` is ignored over HTTP. While agents share a server, `obs-claim-control` gives one of them control of OBS for a set time (30 minutes by default). Until it calls `obs-release-control` or the time runs out, tools that change OBS refuse calls from other clients; reading OBS and stopping outputs always work. `obs-control-status` shows who holds control. `obs-take-start` claims control for the length of the take. Clients are told apart by name: the `client` parameter of the URL they connect with, or else the name a 2026-07-28 client sends with each request. Clients using the 2025 protocol over HTTP send each request without a name, so give each agent its own `?client=` name. An unnamed client cannot claim control and is refused while another client holds it. Two agents given the same name share control. The names are not credentials; control is for agents cooperating on one machine, not for keeping out someone who can reach the server. diff --git a/src/server.ts b/src/server.ts index b9d6325..786ab62 100644 --- a/src/server.ts +++ b/src/server.ts @@ -91,7 +91,9 @@ async function attemptOBSConnection(): Promise { obsClient.on("disconnected", () => { obsConnected = false; - if (!shuttingDown) { + // A failed attempt schedules its own retry once it has counted the failure; + // scheduling here too would use the count from before it. + if (!shuttingDown && !connectionAttempt) { logger.log("OBS WebSocket disconnected; scheduling a reconnect."); scheduleReconnect(); } diff --git a/src/tools/trim.test.ts b/src/tools/trim.test.ts index bcb0fe0..37f3769 100644 --- a/src/tools/trim.test.ts +++ b/src/tools/trim.test.ts @@ -10,7 +10,7 @@ import { join } from "node:path"; import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { healthyObsState, servePreflightState } from "../../test/support/fake-obs-state.js"; import { resultText, startMcpHarness, type McpHarness } from "../../test/support/mcp-harness.js"; -import { planCuts, remapTime } from "./trim.js"; +import { contactSheet, planCuts, remapTime } from "./trim.js"; function hasFfmpeg(): boolean { try { @@ -183,6 +183,11 @@ describe.skipIf(!ffmpegAvailable)("obs-trim-take and obs-contact-sheet", () => { expect(existsSync(lookalike)).toBe(true); }, 30_000); + it("passes the tool call's cancellation to ffmpeg", async () => { + // The trim tools hand ctx.mcpReq.signal, which the SDK aborts when the client cancels, to every child process. + await expect(contactSheet(clip, [1], 1, 80, AbortSignal.abort())).rejects.toThrow(/abort/i); + }); + it("refuses files outside the OBS recording directory", async () => { const outside = mkdtempSync(join(tmpdir(), "obs-mcp-outside-")); try { diff --git a/src/tools/trim.ts b/src/tools/trim.ts index 5b9f55e..703fe8b 100644 --- a/src/tools/trim.ts +++ b/src/tools/trim.ts @@ -163,10 +163,10 @@ function parseSilence(stderr: string, duration: number): Span[] { type Probe = { duration: number; audioStreams: number; chapters: Chapter[]; width?: number; height?: number }; -async function probe(path: string): Promise { +async function probe(path: string, signal?: AbortSignal): Promise { const { stdout } = await execFileAsync("ffprobe", [ "-v", "error", "-print_format", "json", "-show_format", "-show_streams", "-show_chapters", path, - ], { timeout: 15_000 }); + ], { timeout: 15_000, signal }); const data = JSON.parse(stdout) as JsonObject; const streams = Array.isArray(data.streams) ? data.streams.filter(isObject) : []; const video = streams.find((stream) => stream.codec_type === "video"); @@ -195,7 +195,7 @@ function takeLogChapters(path: string): Chapter[] { } /** Streams SAMPLE_FPS grayscale frames and returns the stretches where the picture did not change. */ -function stillSpans(path: string, info: Probe, minIdleSeconds: number, silenceArgs: string[]): Promise<{ spans: Span[]; stderr: string }> { +function stillSpans(path: string, info: Probe, minIdleSeconds: number, silenceArgs: string[], signal?: AbortSignal): Promise<{ spans: Span[]; stderr: string }> { const aspect = info.width && info.height ? info.height / info.width : 9 / 16; const height = Math.max(2, Math.round((SAMPLE_WIDTH * aspect) / 2) * 2); const frameBytes = SAMPLE_WIDTH * height; @@ -204,7 +204,7 @@ function stillSpans(path: string, info: Probe, minIdleSeconds: number, silenceAr "-hide_banner", "-nostats", "-i", path, "-map", "0:v:0", "-vf", `fps=${SAMPLE_FPS},scale=${SAMPLE_WIDTH}:${height},format=gray`, "-f", "rawvideo", "pipe:1", ...silenceArgs, - ], { stdio: ["ignore", "pipe", "pipe"] }); + ], { stdio: ["ignore", "pipe", "pipe"], ...(signal ? { signal } : {}) }); let timedOut = false; const timer = setTimeout(() => { timedOut = true; @@ -246,13 +246,13 @@ function stillSpans(path: string, info: Probe, minIdleSeconds: number, silenceAr }); } -async function detectIdle(path: string, info: Probe, minIdleSeconds: number): Promise<{ idle: Span[]; audible: boolean }> { +async function detectIdle(path: string, info: Probe, minIdleSeconds: number, signal?: AbortSignal): Promise<{ idle: Span[]; audible: boolean }> { // Silence spans only matter if the recording has sound; they keep narration from being cut. const withAudio = info.audioStreams > 0; const silenceArgs = withAudio ? ["-map", "0:a:0", "-af", `silencedetect=n=-50dB:d=${minIdleSeconds}`, "-f", "null", "-"] : []; - const { spans: still, stderr } = await stillSpans(path, info, minIdleSeconds, silenceArgs); + const { spans: still, stderr } = await stillSpans(path, info, minIdleSeconds, silenceArgs, signal); if (!withAudio) return { idle: still, audible: false }; const silent = parseSilence(stderr, info.duration); const whollySilent = silent.length === 1 && silent[0]!.start <= 0.1 && silent[0]!.end >= info.duration - 0.1; @@ -295,6 +295,7 @@ async function exportTrimmed( audioStreams: number, chapters: Chapter[], duration: number, + signal?: AbortSignal, ): Promise { const work = mkdtempSync(join(tmpdir(), "obs-mcp-trim-")); try { @@ -311,28 +312,28 @@ async function exportTrimmed( "-hide_banner", "-nostats", "-y", "-i", path, "-f", "ffmetadata", "-i", metadata, "-filter_complex", filter, "-map", "[v]", ...(withAudio ? ["-map", "[a]", "-c:a", "aac", "-b:a", "160k"] : []), "-map_metadata", "1", "-map_chapters", "1", ...(await videoEncoderArgs()), "-movflags", "+faststart", outputPath, - ], { timeout: 600_000, maxBuffer: 32 * 1024 * 1024 }); + ], { timeout: 600_000, maxBuffer: 32 * 1024 * 1024, signal }); } finally { rmSync(work, { recursive: true, force: true }); } } /** Grabs one frame per time and tiles them into one JPEG. */ -export async function contactSheet(path: string, times: number[], columns: number, width: number): Promise { +export async function contactSheet(path: string, times: number[], columns: number, width: number, signal?: AbortSignal): Promise { const work = mkdtempSync(join(tmpdir(), "obs-mcp-sheet-")); try { for (const [index, time] of times.entries()) { await execFileAsync("ffmpeg", [ "-hide_banner", "-v", "error", "-y", "-ss", String(Math.max(0, time)), "-i", path, "-frames:v", "1", "-vf", `scale=${width}:-2`, "-q:v", "4", join(work, `${String(index).padStart(3, "0")}.jpg`), - ], { timeout: 30_000 }); + ], { timeout: 30_000, signal }); } const rows = Math.ceil(times.length / columns); const sheet = join(work, "sheet.jpg"); await execFileAsync("ffmpeg", [ "-hide_banner", "-v", "error", "-y", "-framerate", "1", "-i", join(work, "%03d.jpg"), "-vf", `tile=${Math.min(columns, times.length)}x${rows}:padding=4:color=white`, "-frames:v", "1", "-q:v", "4", sheet, - ], { timeout: 30_000 }); + ], { timeout: 30_000, signal }); return readFileSync(sheet); } finally { rmSync(work, { recursive: true, force: true }); @@ -383,13 +384,15 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void }), annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: true, openWorldHint: false }, }, - async (args): Promise => { + async (args, ctx): Promise => { + // Cancelling the tool call stops ffmpeg instead of leaving it running for minutes. + const signal = (ctx as { mcpReq?: { signal?: AbortSignal } } | undefined)?.mcpReq?.signal; try { const path = await recordingFile(client, args.path); - const info = await probe(path); + const info = await probe(path, signal); if (!Number.isFinite(info.duration)) return errorResult(`ffprobe could not read the length of ${path}`); const chapters = info.chapters.length > 0 ? info.chapters : takeLogChapters(path); - const { idle, audible } = await detectIdle(path, info, args.minIdleSeconds); + const { idle, audible } = await detectIdle(path, info, args.minIdleSeconds, signal); const { cuts, keep } = planCuts({ ...args, duration: info.duration, idle, chapters }); const removed = round(cuts.reduce((total, { start, end }) => total + end - start, 0)); const newChapters = chapters.map(({ at, name }) => ({ at: remapTime(at, cuts), name })); @@ -432,19 +435,19 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void const partial = join(dirname(outputPath), `.${basename(outputPath)}.${process.pid}-${crypto.randomUUID().slice(0, 8)}.partial${extname(outputPath)}`); if (partial === path || pathTaken(partial)) return errorResult("Could not choose a temporary name for the export; try again"); try { - await exportTrimmed(path, partial, keep, info.audioStreams, newChapters, plan.trimmedSeconds); + await exportTrimmed(path, partial, keep, info.audioStreams, newChapters, plan.trimmedSeconds, signal); if (pathTaken(outputPath)) return errorResult(`${outputPath} appeared while exporting; the export is discarded`); renameSync(partial, outputPath); } finally { rmSync(partial, { force: true }); } - const result = await probe(outputPath); + const result = await probe(outputPath, signal); lines.push(`Exported ${outputPath}: ${round(result.duration)}s, ${result.chapters.length} chapter(s)`); const content: CallToolResult["content"] = []; if (args.contactSheet && seams.length > 0) { const times = seams.flatMap((seam) => [Math.max(0, seam - 0.5), seam + 0.2]).slice(0, MAX_SHEET_FRAMES); - const sheet = await contactSheet(outputPath, times, 4, 320); + const sheet = await contactSheet(outputPath, times, 4, 320, signal); lines.push(`Contact sheet: frames 0.5s before and 0.2s after each seam, in pairs, left to right: ${times.map((t) => `${round(t)}s`).join(", ")}`); content.push({ type: "image", data: sheet.toString("base64"), mimeType: "image/jpeg" }); } @@ -476,10 +479,11 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void }), annotations: { readOnlyHint: true, destructiveHint: false, idempotentHint: true, openWorldHint: false }, }, - async (args): Promise => { + async (args, ctx): Promise => { + const signal = (ctx as { mcpReq?: { signal?: AbortSignal } } | undefined)?.mcpReq?.signal; try { const path = await recordingFile(client, args.path); - const info = await probe(path); + const info = await probe(path, signal); const chapters = info.chapters.length > 0 ? info.chapters : takeLogChapters(path); const times = (args.times ?? (chapters.length > 0 ? chapters.map(({ at }) => at + 1) @@ -487,7 +491,7 @@ export function initialize(server: McpServer, client: OBSWebSocketClient): void .filter((time) => time < info.duration) .slice(0, MAX_SHEET_FRAMES); if (times.length === 0) return errorResult("No requested time falls inside the recording"); - const sheet = await contactSheet(path, times, args.columns, args.width); + const sheet = await contactSheet(path, times, args.columns, args.width, signal); const legend = times.map((time, index) => { const chapter = !args.times ? chapters[index]?.name : undefined; return `${index + 1}. ${round(time)}s${chapter ? ` ${chapter}` : ""}`; diff --git a/test/e2e/http.e2e.test.ts b/test/e2e/http.e2e.test.ts index 172ae26..ecbc61c 100644 --- a/test/e2e/http.e2e.test.ts +++ b/test/e2e/http.e2e.test.ts @@ -106,6 +106,7 @@ describe("compiled HTTP server", () => { const port = new URL(url).port; expect(await rawPost(url, { host: "evil.example", authorization: "Bearer local-secret" })).toBe(403); + expect(await rawPost(url, { host: `127.0.0.1:${port}`, origin: "https://evil.example", authorization: "Bearer local-secret" })).toBe(403); expect(await rawPost(url, { host: `127.0.0.1:${port}` })).toBe(401); expect(await rawPost(url, { host: `127.0.0.1:${port}`, authorization: "Bearer wrong-secret!" })).toBe(401); expect(await rawPost(url.replace("/mcp", "/other"), { host: `127.0.0.1:${port}` })).toBe(404);