From d5d8a695470d93a2953b62374a46c463ccf5b7b8 Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Wed, 17 Jun 2026 14:53:25 -0700 Subject: [PATCH 01/11] feat(timeline): add rev CAS to CompositionDoc via a shared revisioned-store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the session store's in-process write serialization + rev compare-and-swap into a generic `createRevisionedStore` factory, and have both the session log and the CompositionDoc consume it so the two stores can't drift. The doc store previously did a lock-free read-apply-write, which silently loses updates the moment the agent and `clip ui` co-edit composition.json — the same bug class the session store already fixed. - src/storage/revisioned-store.ts: shared serialization + CAS retry + CAS write + parse-free overwrite (independent write-chain per store). - session/store.ts: refactored onto the factory; public API + behavior byte-identical (tests/session.test.ts unchanged and green). - timeline/composition.ts: add `rev` (default 0, back-compat) to CompositionSchema. - timeline/document-store.ts: mutateComposition now serialized + CAS; add writeCompositionIfUnchanged, overwriteComposition, CompositionConflictError. applyOp's return type is unchanged — the reversible op-log / {doc, opsApplied} change is the next PR. Co-Authored-By: Claude Opus 4.8 --- src/index.ts | 10 +++ src/session/store.ts | 95 ++++++-------------- src/storage/revisioned-store.ts | 154 ++++++++++++++++++++++++++++++++ src/timeline/composition.ts | 8 ++ src/timeline/document-store.ts | 83 +++++++++++++++-- tests/document-store.test.ts | 69 ++++++++++++++ 6 files changed, 345 insertions(+), 74 deletions(-) create mode 100644 src/storage/revisioned-store.ts diff --git a/src/index.ts b/src/index.ts index 2b793eb..2df29d1 100644 --- a/src/index.ts +++ b/src/index.ts @@ -78,6 +78,13 @@ export { SessionEntrySchema, SessionSchema, } from './session/types.js'; +export { + createRevisionedStore, + RevisionConflictError, + type Revisioned, + type RevisionedStore, + type RevisionedStoreConfig, +} from './storage/revisioned-store.js'; export { type CompileContext, CompileError, @@ -124,12 +131,15 @@ export { TransitionSchema, } from './timeline/composition.js'; export { + CompositionConflictError, CompositionCorruptError, compositionPath, mutateComposition, + overwriteComposition, readComposition, resetComposition, writeComposition, + writeCompositionIfUnchanged, } from './timeline/document-store.js'; export { buildMediaMap } from './timeline/media-registry.js'; export { diff --git a/src/session/store.ts b/src/session/store.ts index 1c13ab8..fef746c 100644 --- a/src/session/store.ts +++ b/src/session/store.ts @@ -1,6 +1,7 @@ import { randomBytes } from 'node:crypto'; import { type FileHandle, mkdir, open, readFile, rename, unlink } from 'node:fs/promises'; import { dirname, resolve } from 'node:path'; +import { createRevisionedStore } from '../storage/revisioned-store.js'; import { ensureWorkspace, getWorkspace } from '../workspace.js'; import { type Session, type SessionEntry, SessionSchema } from './types.js'; @@ -70,24 +71,23 @@ function freshEmpty(): Session { return { version: 1, rev: 0, entries: [] }; } -// ─── In-process single-writer serialization ────────────────────────────────── +// ─── Shared revisioned-store core (in-process serialization + CAS) ─────────── // -// Every mutation runs through this promise chain, so two concurrent callers in -// the SAME process (the embedded chat agent and a UI request both calling -// `appendOp`) can never interleave their read-modify-write and lose an entry. -// Reads are intentionally NOT serialized: atomic writes mean a reader always -// sees a complete file, so reads stay lock-free. -let writeChain: Promise = Promise.resolve(); - -function runExclusive(fn: () => Promise): Promise { - const run = writeChain.then(fn, fn); - // Keep the chain alive regardless of whether `fn` resolved or rejected. - writeChain = run.then( - () => undefined, - () => undefined, - ); - return run; -} +// The session log and the CompositionDoc share this machinery (see +// `storage/revisioned-store.ts`) so the two stores can't drift. Every mutation +// runs through a serialized chain, so two concurrent callers in the SAME process +// (the embedded chat agent and a UI request both calling `appendOp`) can never +// interleave their read-modify-write and lose an entry. Reads are intentionally +// NOT serialized: atomic writes mean a reader always sees a complete file. +const sessionStore = createRevisionedStore({ + read: readSession, + write: writeSession, + getRev: (session) => session.rev, + withRev: (session, rev) => ({ ...session, rev }), + readRevTolerant, + maxAttempts: MAX_MUTATE_ATTEMPTS, + onConflict: (expectedRev, actualRev) => new SessionConflictError(expectedRev, actualRev), +}); export async function readSession(): Promise { await ensureWorkspace(); @@ -211,19 +211,8 @@ async function fsyncDir(dir: string): Promise { * win undetected. A real cross-process lock is deferred until multi-process * editing is on the roadmap (the shipping surface is single-process). */ -export async function writeSessionIfUnchanged( - session: Session, - expectedRev: number, -): Promise { - return runExclusive(async () => { - const actualRev = await readDiskRev(); - if (actualRev !== expectedRev) { - throw new SessionConflictError(expectedRev, actualRev); - } - const next: Session = { ...session, rev: expectedRev + 1 }; - await writeSession(next); - return next; - }); +export function writeSessionIfUnchanged(session: Session, expectedRev: number): Promise { + return sessionStore.writeIfUnchanged(session, expectedRev); } /** @@ -247,37 +236,14 @@ export async function writeSessionIfUnchanged( export async function mutateSession( mutator: (session: Session) => T, ): Promise<{ result: T; session: Session }> { - return runExclusive(async () => { - for (let attempt = 1; attempt <= MAX_MUTATE_ATTEMPTS; attempt++) { - const current = await readSession(); - const baseRev = current.rev; - const working: Session = { version: 1, rev: baseRev, entries: [...current.entries] }; - - const result = mutator(working); - - // Re-read the on-disk rev immediately before committing. In-process the - // chain guarantees it's unchanged; a mismatch means another process wrote. - const diskRev = await readDiskRev(); - if (diskRev !== baseRev) { - if (attempt === MAX_MUTATE_ATTEMPTS) { - throw new SessionConflictError(baseRev, diskRev); - } - continue; - } - - working.rev = baseRev + 1; - await writeSession(working); - return { result, session: working }; - } - // Unreachable: the loop either returns or throws on the final attempt. - throw new SessionConflictError(-1, -1); + const { result, state } = await sessionStore.mutate((current) => { + // A private, mutable copy: the mutator appends to `entries` (or throws to + // abort) without aliasing the freshly-read state's backing array. + const working: Session = { version: 1, rev: current.rev, entries: [...current.entries] }; + const value = mutator(working); + return { next: working, result: value }; }); -} - -/** Read just the revision counter; a missing file reads as rev 0. Corrupt files - * still surface via `readSession`. */ -async function readDiskRev(): Promise { - return (await readSession()).rev; + return { result, session: state }; } /** @@ -288,13 +254,8 @@ async function readDiskRev(): Promise { * through `mutateSession` (whose first act is a strict `readSession`). `rev` * advances past the last good revision when one is readable, else restarts at 1. */ -export async function overwriteSession(entries: SessionEntry[]): Promise { - return runExclusive(async () => { - const baseRev = await readRevTolerant(); - const next: Session = { version: 1, rev: baseRev + 1, entries: [...entries] }; - await writeSession(next); - return next; - }); +export function overwriteSession(entries: SessionEntry[]): Promise { + return sessionStore.overwrite({ version: 1, rev: 0, entries: [...entries] }); } /** On-disk rev, treating a corrupt file as rev 0 (a deliberate overwrite is diff --git a/src/storage/revisioned-store.ts b/src/storage/revisioned-store.ts new file mode 100644 index 0000000..7bbcbd6 --- /dev/null +++ b/src/storage/revisioned-store.ts @@ -0,0 +1,154 @@ +/** + * Shared optimistic-concurrency core for the workspace's revisioned JSON + * documents (the session log and the CompositionDoc). Both files are a single + * source of truth that more than one writer can touch — the embedded chat agent + * and a `clip ui` request in the same process, or a CLI command run while the UI + * is open. Without serialization a lock-free read-modify-write loses updates (the + * original session bug); without a `rev` compare-and-swap a cross-process writer + * can still last-writer-win. + * + * This factory captures that machinery ONCE so the two stores can't drift: each + * store supplies how to `read`/`write` its state and read/set its `rev`, and gets + * back in-process serialization + a CAS retry loop + a CAS write + a + * parse-free overwrite. The previous design hand-rolled all of this inside the + * session store; the doc store had none of it. + * + * Each call returns an INDEPENDENT store (its own `runExclusive` chain), so + * session writes and composition writes serialize against themselves but not + * needlessly against each other — they are different files. + */ + +/** A monotonic revision counter on a persisted document. */ +export interface Revisioned { + rev: number; +} + +export interface RevisionedStoreConfig { + /** Strict read: returns the parsed state, throwing on a corrupt file. */ + read: () => Promise; + /** Atomic write of a committed state. */ + write: (state: S) => Promise; + getRev: (state: S) => number; + /** Return a copy of `state` with its `rev` set — never mutate the input. */ + withRev: (state: S, rev: number) => S; + /** + * Read just the rev, treating a corrupt file as rev 0 (used only by + * `overwrite`, which is about to discard whatever is there). Defaults to + * `getRev(read())`, i.e. corrupt files propagate — supply a tolerant reader + * when the store needs parse-free recovery. + */ + readRevTolerant?: () => Promise; + /** How many times `mutate` re-runs when a cross-process write is detected. */ + maxAttempts?: number; + /** Build the store's domain-specific conflict error (e.g. SessionConflictError). */ + onConflict?: (expectedRev: number, actualRev: number) => Error; +} + +export interface RevisionedStore { + /** Serialize `fn` after every prior in-process write to this store. */ + runExclusive: (fn: () => Promise) => Promise; + /** + * Serialized, atomic read → derive-next → write. `mutator` receives the + * freshly-read state and returns the `next` state plus a `result`; it must be + * free of external side effects (a detected cross-process race re-runs it). + * A throw from `mutator` aborts immediately and propagates. + */ + mutate: (mutator: (state: S) => { next: S; result: T }) => Promise<{ result: T; state: S }>; + /** CAS: write `state` only if the on-disk rev still equals `expectedRev`, then + * bump to `expectedRev + 1`; throws the conflict error otherwise. */ + writeIfUnchanged: (state: S, expectedRev: number) => Promise; + /** Replace the document WITHOUT trusting the current file to parse, advancing + * rev past the last readable revision (or restarting at 1 from a corrupt one). */ + overwrite: (state: S) => Promise; +} + +/** Default conflict error when a store does not supply its own. */ +export class RevisionConflictError extends Error { + readonly expectedRev: number; + readonly actualRev: number; + constructor(expectedRev: number, actualRev: number) { + super( + `Write rejected: expected rev ${expectedRev} but on-disk rev is ${actualRev}. ` + + `Another writer committed first — re-read and reapply.`, + ); + this.name = 'RevisionConflictError'; + this.expectedRev = expectedRev; + this.actualRev = actualRev; + } +} + +export function createRevisionedStore(cfg: RevisionedStoreConfig): RevisionedStore { + // Floor at 1 so `mutate` always makes at least one read-modify-write pass — a + // misconfigured `maxAttempts: 0` would otherwise skip the loop body entirely + // and surface a meaningless (-1, -1) conflict instead of applying the mutation. + const maxAttempts = Math.max(1, cfg.maxAttempts ?? 8); + const conflict = cfg.onConflict ?? ((e, a) => new RevisionConflictError(e, a)); + + // Every mutation runs through this promise chain, so two concurrent callers in + // the SAME process can never interleave their read-modify-write. Reads are + // intentionally NOT serialized: atomic writes mean a reader always sees a + // complete file, so reads stay lock-free. + let writeChain: Promise = Promise.resolve(); + + function runExclusive(fn: () => Promise): Promise { + const run = writeChain.then(fn, fn); + // Keep the chain alive regardless of whether `fn` resolved or rejected. + writeChain = run.then( + () => undefined, + () => undefined, + ); + return run; + } + + async function diskRev(): Promise { + return cfg.getRev(await cfg.read()); + } + + async function mutate( + mutator: (state: S) => { next: S; result: T }, + ): Promise<{ result: T; state: S }> { + return runExclusive(async () => { + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + const current = await cfg.read(); + const baseRev = cfg.getRev(current); + + const { next, result } = mutator(current); + + // Re-read the on-disk rev immediately before committing. In-process the + // chain guarantees it's unchanged; a mismatch means another process wrote. + const observed = await diskRev(); + if (observed !== baseRev) { + if (attempt === maxAttempts) throw conflict(baseRev, observed); + continue; + } + + const committed = cfg.withRev(next, baseRev + 1); + await cfg.write(committed); + return { result, state: committed }; + } + // Unreachable: the loop either returns or throws on the final attempt. + throw conflict(-1, -1); + }); + } + + async function writeIfUnchanged(state: S, expectedRev: number): Promise { + return runExclusive(async () => { + const actualRev = await diskRev(); + if (actualRev !== expectedRev) throw conflict(expectedRev, actualRev); + const next = cfg.withRev(state, expectedRev + 1); + await cfg.write(next); + return next; + }); + } + + async function overwrite(state: S): Promise { + return runExclusive(async () => { + const baseRev = cfg.readRevTolerant ? await cfg.readRevTolerant() : await diskRev(); + const next = cfg.withRev(state, baseRev + 1); + await cfg.write(next); + return next; + }); + } + + return { runExclusive, mutate, writeIfUnchanged, overwrite }; +} diff --git a/src/timeline/composition.ts b/src/timeline/composition.ts index deb3e8d..4579c26 100644 --- a/src/timeline/composition.ts +++ b/src/timeline/composition.ts @@ -176,6 +176,14 @@ export type Track = z.infer; export const CompositionSchema = z.object({ version: z.literal(COMPOSITION_VERSION), + /** + * Monotonic revision counter, bumped on every successful write — the + * compare-and-swap token for optimistic concurrency when the agent and the + * `clip ui` co-edit this document (mirrors `Session.rev`). Distinct from + * `version` (the format pin). Optional with a `0` default so documents written + * before this field existed — and hand-authored fixtures — still parse. + */ + rev: z.number().int().nonnegative().default(0), width: z.number().int().positive().default(1920), height: z.number().int().positive().default(1080), fps: z.number().positive().default(30), diff --git a/src/timeline/document-store.ts b/src/timeline/document-store.ts index 3697d63..7ddc463 100644 --- a/src/timeline/document-store.ts +++ b/src/timeline/document-store.ts @@ -1,6 +1,7 @@ import { readFile } from 'node:fs/promises'; import { resolve } from 'node:path'; import { atomicWriteFile } from '../session/store.js'; +import { createRevisionedStore } from '../storage/revisioned-store.js'; import { ensureWorkspace, getWorkspace } from '../workspace.js'; import { type Composition, CompositionSchema, emptyComposition } from './composition.js'; import { applyOps, type CompositionOp } from './ops.js'; @@ -26,6 +27,26 @@ export class CompositionCorruptError extends Error { } } +/** + * Thrown by `writeCompositionIfUnchanged` when the on-disk `rev` no longer + * matches the revision the caller based its edit on — another writer committed + * in between. Re-read and reapply rather than clobber (mirrors + * `SessionConflictError`). + */ +export class CompositionConflictError extends Error { + readonly expectedRev: number; + readonly actualRev: number; + constructor(expectedRev: number, actualRev: number) { + super( + `Composition write rejected: expected rev ${expectedRev} but on-disk rev is ${actualRev}. ` + + `Another writer committed first — re-read and reapply.`, + ); + this.name = 'CompositionConflictError'; + this.expectedRev = expectedRev; + this.actualRev = actualRev; + } +} + export async function readComposition(): Promise { await ensureWorkspace(); let raw: string; @@ -46,16 +67,64 @@ export async function writeComposition(comp: Composition): Promise { await atomicWriteFile(compositionPath(), `${JSON.stringify(comp, null, 2)}\n`); } +/** On-disk rev, treating a corrupt file as rev 0 — a deliberate overwrite is + * about to discard it anyway. Non-corruption read errors still propagate. */ +async function readCompositionRevTolerant(): Promise { + try { + return (await readComposition()).rev; + } catch (err) { + if (err instanceof CompositionCorruptError) return 0; + throw err; + } +} + +// Serialization + compare-and-swap, shared with the session store (see +// `storage/revisioned-store.ts`) so co-editing the doc (the agent + `clip ui`) +// can't silently lose updates. +const docStore = createRevisionedStore({ + read: readComposition, + write: writeComposition, + getRev: (comp) => comp.rev, + withRev: (comp, rev) => ({ ...comp, rev }), + readRevTolerant: readCompositionRevTolerant, + onConflict: (expectedRev, actualRev) => new CompositionConflictError(expectedRev, actualRev), +}); + /** - * Read → apply ops → write, returning the new composition. The CLI drives this - * sequentially (one process per command), so it needs no in-process lock; the - * single-writer concurrency the session store carries lands here when the UI and - * agent co-edit the doc (a later phase). Writes are already atomic. + * Read → apply ops → write, returning the new composition. Now serialized + rev + * compare-and-swap via the shared revisioned store, so the agent and the UI can + * co-edit composition.json without lost updates (writes were already atomic). + * `applyOp`'s return type is unchanged in this slice — the reversible op-log / + * `{ doc, opsApplied }` change lands in the next PR. */ export async function mutateComposition(ops: CompositionOp[]): Promise { - const next = applyOps(await readComposition(), ops); - await writeComposition(next); - return next; + const { state } = await docStore.mutate((current) => { + const next = applyOps(current, ops); + return { next, result: next }; + }); + return state; +} + +/** + * Compare-and-swap write: persist `comp` only if the on-disk `rev` still equals + * `expectedRev`, then bump to `expectedRev + 1`. Throws `CompositionConflictError` + * if another writer committed first — the optimistic-concurrency primitive the + * co-editing UI builds on. + */ +export function writeCompositionIfUnchanged( + comp: Composition, + expectedRev: number, +): Promise { + return docStore.writeIfUnchanged(comp, expectedRev); +} + +/** + * Recovery primitive: replace composition.json atomically and WITHOUT trusting + * the current file to parse (so it works precisely when the live file is the + * corrupt one being recovered), advancing `rev` past the last readable revision. + */ +export function overwriteComposition(comp: Composition = emptyComposition()): Promise { + return docStore.overwrite(comp); } export async function resetComposition( diff --git a/tests/document-store.test.ts b/tests/document-store.test.ts index 7aa9c69..1a6c647 100644 --- a/tests/document-store.test.ts +++ b/tests/document-store.test.ts @@ -4,12 +4,15 @@ import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { emptyComposition } from '../src/timeline/composition.js'; import { + CompositionConflictError, CompositionCorruptError, compositionPath, mutateComposition, + overwriteComposition, readComposition, resetComposition, writeComposition, + writeCompositionIfUnchanged, } from '../src/timeline/document-store.js'; import { mediaClip, videoTrack } from '../src/timeline/ops.js'; import type { MediaId } from '../src/timeline/schema.js'; @@ -72,3 +75,69 @@ describe('document-store', () => { expect(onDisk).toEqual(comp); }); }); + +describe('optimistic concurrency (rev)', () => { + it('loads a rev-less legacy composition as rev 0 (back-compat)', async () => { + // Documents written before `rev` existed must still parse. + await writeFile(compositionPath(), JSON.stringify({ version: 1, tracks: [] })); + expect((await readComposition()).rev).toBe(0); + }); + + it('bumps rev monotonically and persists it across mutations', async () => { + expect((await readComposition()).rev).toBe(0); + await mutateComposition([{ op: 'setCanvas', width: 1280 }]); + expect((await readComposition()).rev).toBe(1); + await mutateComposition([{ op: 'setCanvas', height: 720 }]); + expect((await readComposition()).rev).toBe(2); + }); + + it('writeCompositionIfUnchanged commits and bumps rev when the base is current', async () => { + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); // rev 1 + const current = await readComposition(); + const next = await writeCompositionIfUnchanged(current, current.rev); + expect(next.rev).toBe(current.rev + 1); + expect((await readComposition()).rev).toBe(current.rev + 1); + }); + + it('rejects a stale CAS write whose base rev was superseded, without clobbering', async () => { + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); // rev 1 + const stale = await readComposition(); // captured at rev 1 + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v1' }) }]); // winner → rev 2 + + await expect(writeCompositionIfUnchanged(stale, stale.rev)).rejects.toBeInstanceOf( + CompositionConflictError, + ); + // The winner survived; the rejected write did not overwrite it. + const final = await readComposition(); + expect(final.rev).toBe(2); + expect(final.tracks.map((t) => t.id)).toEqual(['v0', 'v1']); + }); + + it('serializes concurrent in-process mutations — no lost update', async () => { + // The lost-update bug the CAS store fixes: a lock-free read-apply-write let + // two concurrent callers clobber each other. Both ops must land, rev = 2. + await Promise.all([ + mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]), + mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v1' }) }]), + ]); + const comp = await readComposition(); + expect(comp.rev).toBe(2); + expect(new Set(comp.tracks.map((t) => t.id))).toEqual(new Set(['v0', 'v1'])); + }); +}); + +describe('overwriteComposition (recovery primitive)', () => { + it('replaces composition.json WITHOUT parsing it, even when corrupt', async () => { + await mutateComposition([{ op: 'setCanvas', width: 1280 }]); // rev 1 + await writeFile(compositionPath(), 'not valid json'); // live doc now corrupt + await expect(readComposition()).rejects.toBeInstanceOf(CompositionCorruptError); + + const next = await overwriteComposition(emptyComposition({ fps: 24 })); + expect(next.rev).toBe(1); // corrupt base → rev 0 → 1 + expect(next.fps).toBe(24); + + const reread = await readComposition(); + expect(reread.fps).toBe(24); + expect(reread.rev).toBe(1); + }); +}); From 452617805b5edb6bd53dd547c26efad170d400c7 Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Wed, 17 Jun 2026 15:23:27 -0700 Subject: [PATCH 02/11] feat(timeline): add invertOp (exact per-op inverses) + canonical doc order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Foundation for reversible editing (the op-log/undo lands next): a pure `invertOp(comp, op): CompositionOp[]` that, given the pre-state, returns the op(s) that undo `op` exactly. `applyOp`/`applyOps` keep their `(comp) => Composition` signature, so no existing caller changes. Two op-vocabulary additions are required for exact reversibility: - `addTrack` gains an optional `index` (insert at z-order position) so a `removeTrack` can be undone at the original index. - a new `clearTransform` op — the inverse of a `setTransform` that created a transform on a clip that had none. `applyOp` now also canonicalizes track order on every output — clips by (startSec, id), transitions by afterClipId — so two documents with identical content are deep-equal regardless of the op history that built them. Array order is semantically irrelevant (the compiler keys transitions by afterClipId and rejects overlapping clips), and canonical order is what lets undo land on a byte-identical document. Verified by a round-trip property — applyOps(applyOp(pre, op), invertOp(pre, op)) deep-equals pre — across every op kind plus a LIFO sequence-undo test, including clips carrying effects+transforms, multi-transition tracks, and equal-startSec siblings (41 tests). Co-Authored-By: Claude Opus 4.8 --- src/index.ts | 1 + src/timeline/ops.ts | 205 ++++++++++++++++- tests/composition-invert.test.ts | 376 +++++++++++++++++++++++++++++++ 3 files changed, 580 insertions(+), 2 deletions(-) create mode 100644 tests/composition-invert.test.ts diff --git a/src/index.ts b/src/index.ts index 2df29d1..5bd5206 100644 --- a/src/index.ts +++ b/src/index.ts @@ -150,6 +150,7 @@ export { CompositionOpError, type CompositionOpKind, colorClip, + invertOp, mediaClip, textClip, videoTrack, diff --git a/src/timeline/ops.ts b/src/timeline/ops.ts index 9807a5e..9a100a0 100644 --- a/src/timeline/ops.ts +++ b/src/timeline/ops.ts @@ -32,7 +32,7 @@ import type { MediaId } from './schema.js'; */ export type CompositionOp = | { op: 'setCanvas'; width?: number; height?: number; fps?: number; background?: string } - | { op: 'addTrack'; track: Track } + | { op: 'addTrack'; track: Track; index?: number } | { op: 'removeTrack'; trackId: string } | { op: 'addClip'; trackId: string; clip: Clip } | { op: 'removeClip'; clipId: string } @@ -43,6 +43,7 @@ export type CompositionOp = | { op: 'addEffect'; clipId: string; effect: Effect; index?: number } | { op: 'removeEffect'; clipId: string; index: number } | { op: 'setTransform'; clipId: string; transform: Partial } + | { op: 'clearTransform'; clipId: string } | { op: 'addTransition'; trackId: string; transition: Transition } | { op: 'removeTransition'; trackId: string; afterClipId: string }; @@ -101,7 +102,14 @@ export function applyOp(comp: Composition, op: CompositionOp): Composition { if (draft.tracks.some((t) => t.id === op.track.id)) { throw new CompositionOpError(`Track "${op.track.id}" already exists.`); } - draft.tracks.push(TrackSchema.parse(op.track)); + const track = TrackSchema.parse(op.track); + if (op.index === undefined) { + draft.tracks.push(track); + } else { + // Clamp into range and insert at z-order position (used by undo to + // restore a removed track at its original index). + draft.tracks.splice(Math.max(0, Math.min(op.index, draft.tracks.length)), 0, track); + } break; } case 'removeTrack': { @@ -199,6 +207,12 @@ export function applyOp(comp: Composition, op: CompositionOp): Composition { clip.transform = { ...base, ...op.transform }; break; } + case 'clearTransform': { + const { clip } = locateClip(draft, op.clipId); + // Back to "no transform" (the inverse of a setTransform that created one). + delete clip.transform; + break; + } case 'addTransition': { const track = requireTrack(draft, op.trackId); if (!track.clips.some((c) => c.id === op.transition.afterClipId)) { @@ -223,14 +237,201 @@ export function applyOp(comp: Composition, op: CompositionOp): Composition { } } + canonicalizeTracks(draft); return CompositionSchema.parse(draft); } +/** + * Put every track into a canonical array order — clips by (startSec, then id), + * transitions by afterClipId — so two documents with identical CONTENT always + * compare deep-equal regardless of the op history that built them. Array order is + * otherwise semantically irrelevant: the export compiler keys transitions by + * afterClipId and rejects overlapping (equal-start) clips, so nothing downstream + * depends on it. Canonicalizing here is what makes the op-log reversible — undo + * (apply an op's inverse) lands back on a byte-identical document instead of one + * that merely has the same clips in a different array slot. + */ +function canonicalizeTracks(comp: Composition): void { + const byString = (a: string, b: string): number => (a < b ? -1 : a > b ? 1 : 0); + for (const track of comp.tracks) { + track.clips.sort((a, b) => a.startSec - b.startSec || byString(a.id, b.id)); + track.transitions.sort((a, b) => byString(a.afterClipId, b.afterClipId)); + } +} + /** Fold a sequence of ops left-to-right. */ export function applyOps(comp: Composition, ops: CompositionOp[]): Composition { return ops.reduce(applyOp, comp); } +/** + * Compute the inverse of `op` against the PRE-state `comp`: the op(s) that, + * applied to the post-op document, restore `comp` exactly. Pure — it only reads + * `comp` (capturing whatever the op overwrites or drops) and returns new ops; it + * never mutates. This is what makes the doc op-log reversible: a writer records + * `invertOp(current, op)` alongside the op, and undo replays the inverse. + * + * Assumes `op` is applicable to `comp` (the caller applies it immediately after). + * It locates the entities it must capture and throws `CompositionOpError` if they + * are missing — the same failure `applyOp` would raise. For ops `applyOp` would + * reject on a *value* check (e.g. a degenerate trim), the inverse is harmless and + * simply discarded when `applyOp` throws. + */ +export function invertOp(comp: Composition, op: CompositionOp): CompositionOp[] { + switch (op.op) { + case 'setCanvas': + // Restore only the fields this op actually overwrote. + return [ + { + op: 'setCanvas', + ...(op.width !== undefined ? { width: comp.width } : {}), + ...(op.height !== undefined ? { height: comp.height } : {}), + ...(op.fps !== undefined ? { fps: comp.fps } : {}), + ...(op.background !== undefined ? { background: comp.background } : {}), + }, + ]; + case 'addTrack': + return [{ op: 'removeTrack', trackId: op.track.id }]; + case 'removeTrack': { + const index = comp.tracks.findIndex((t) => t.id === op.trackId); + const track = comp.tracks[index]; + if (!track) throw new CompositionOpError(`No track "${op.trackId}" to invert removal of.`); + // Re-insert the whole track (clips + transitions) at its original z-index. + return [{ op: 'addTrack', track: structuredClone(track), index }]; + } + case 'addClip': + return [{ op: 'removeClip', clipId: op.clip.id }]; + case 'removeClip': { + const { track, clip } = locateClip(comp, op.clipId); + const inverse: CompositionOp[] = [ + { op: 'addClip', trackId: track.id, clip: structuredClone(clip) }, + ]; + // removeClip also drops the transition that followed the clip — restore it. + for (const t of track.transitions) { + if (t.afterClipId === op.clipId) { + inverse.push({ op: 'addTransition', trackId: track.id, transition: structuredClone(t) }); + } + } + return inverse; + } + case 'moveClip': { + const { track, clip } = locateClip(comp, op.clipId); + const inverse: CompositionOp[] = [ + { op: 'moveClip', clipId: op.clipId, startSec: clip.startSec, toTrackId: track.id }, + ]; + // A cross-track move drops any transition that followed the clip on the + // source track; re-add it after the clip is back. + if (op.toTrackId !== undefined && op.toTrackId !== track.id) { + for (const t of track.transitions) { + if (t.afterClipId === op.clipId) { + inverse.push({ + op: 'addTransition', + trackId: track.id, + transition: structuredClone(t), + }); + } + } + } + return inverse; + } + case 'setTrim': { + const { clip } = locateClip(comp, op.clipId); + if (clip.kind !== 'media') return []; // applyOp will reject; nothing to invert. + return [ + { + op: 'setTrim', + clipId: op.clipId, + sourceInSec: clip.sourceInSec, + sourceOutSec: clip.sourceOutSec, + }, + ]; + } + case 'setDuration': { + const { clip } = locateClip(comp, op.clipId); + if (clip.kind === 'media') return []; // applyOp will reject. + return [{ op: 'setDuration', clipId: op.clipId, durationSec: clip.durationSec }]; + } + case 'splitClip': { + const { track, clip } = locateClip(comp, op.clipId); + const inverse: CompositionOp[] = []; + // On split, the transition after the original clip migrates to the new + // second half. Re-home it to the original, then remove the second half + // (which drops the migrated copy), then restore the original's extent. + const moved = track.transitions.find((t) => t.afterClipId === op.clipId); + if (moved) { + inverse.push({ + op: 'addTransition', + trackId: track.id, + transition: structuredClone(moved), + }); + } + inverse.push({ op: 'removeClip', clipId: op.newClipId }); + if (clip.kind === 'media') { + inverse.push({ + op: 'setTrim', + clipId: op.clipId, + sourceInSec: clip.sourceInSec, + sourceOutSec: clip.sourceOutSec, + }); + } else { + inverse.push({ op: 'setDuration', clipId: op.clipId, durationSec: clip.durationSec }); + } + return inverse; + } + case 'addEffect': { + const { clip } = locateClip(comp, op.clipId); + const at = + op.index === undefined + ? clip.effects.length + : Math.max(0, Math.min(op.index, clip.effects.length)); + return [{ op: 'removeEffect', clipId: op.clipId, index: at }]; + } + case 'removeEffect': { + const { clip } = locateClip(comp, op.clipId); + const effect = clip.effects[op.index]; + if (!effect) return []; // out of range — applyOp will reject. + return [ + { op: 'addEffect', clipId: op.clipId, effect: structuredClone(effect), index: op.index }, + ]; + } + case 'setTransform': { + const { clip } = locateClip(comp, op.clipId); + if (clip.transform === undefined) return [{ op: 'clearTransform', clipId: op.clipId }]; + return [ + { op: 'setTransform', clipId: op.clipId, transform: structuredClone(clip.transform) }, + ]; + } + case 'clearTransform': { + const { clip } = locateClip(comp, op.clipId); + if (clip.transform === undefined) return []; // already clear — no-op. + return [ + { op: 'setTransform', clipId: op.clipId, transform: structuredClone(clip.transform) }, + ]; + } + case 'addTransition': { + const track = requireTrack(comp, op.trackId); + const prior = track.transitions.find((t) => t.afterClipId === op.transition.afterClipId); + // addTransition replaces any same-anchor transition: restore the prior one + // if there was one, otherwise just remove what we added. + if (prior) + return [{ op: 'addTransition', trackId: op.trackId, transition: structuredClone(prior) }]; + return [ + { op: 'removeTransition', trackId: op.trackId, afterClipId: op.transition.afterClipId }, + ]; + } + case 'removeTransition': { + const track = requireTrack(comp, op.trackId); + const removed = track.transitions.find((t) => t.afterClipId === op.afterClipId); + if (!removed) return []; // removed nothing — no-op. + return [{ op: 'addTransition', trackId: op.trackId, transition: structuredClone(removed) }]; + } + default: { + const _exhaustive: never = op; + throw new CompositionOpError(`Cannot invert unknown op: ${JSON.stringify(_exhaustive)}`); + } + } +} + // ─── internals ─────────────────────────────────────────────────────────────── function locate(comp: Composition, clipId: string): boolean { diff --git a/tests/composition-invert.test.ts b/tests/composition-invert.test.ts new file mode 100644 index 0000000..c7f1ff9 --- /dev/null +++ b/tests/composition-invert.test.ts @@ -0,0 +1,376 @@ +import { describe, expect, it } from 'vitest'; +import { type Composition, emptyComposition } from '../src/timeline/composition.js'; +import { + applyOp, + applyOps, + audioTrack, + type CompositionOp, + invertOp, + mediaClip, + textClip, + videoTrack, +} from '../src/timeline/ops.js'; +import type { MediaId } from '../src/timeline/schema.js'; + +const M = 'm_aaaaaaaaaaaa' as MediaId; + +/** + * A document exercising the cases inverses must handle: a MIDDLE track (z-order), + * two media clips and a text clip, and a transition after the first clip. + */ +function richDoc(): Composition { + return applyOps(emptyComposition(), [ + { op: 'addTrack', track: videoTrack({ id: 'v0' }) }, + { op: 'addTrack', track: videoTrack({ id: 'v1' }) }, + { op: 'addTrack', track: audioTrack({ id: 'a0' }) }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c1', mediaId: M, sourceOutSec: 5, startSec: 0 }), + }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c2', mediaId: M, sourceOutSec: 4, startSec: 5 }), + }, + { + op: 'addClip', + trackId: 'v1', + clip: textClip({ id: 't1', text: 'hi', durationSec: 3, startSec: 0 }), + }, + { + op: 'addTransition', + trackId: 'v0', + transition: { afterClipId: 'c1', kind: 'fade', durationSec: 1 }, + }, + ]); +} + +/** The core property: applying an op then its inverse restores the pre-state exactly. */ +function expectRoundtrip(pre: Composition, op: CompositionOp): void { + const frozen = structuredClone(pre); + const after = applyOp(pre, op); + const inverse = invertOp(pre, op); + // invertOp must not mutate its input. + expect(pre).toEqual(frozen); + expect(applyOps(after, inverse)).toEqual(pre); +} + +describe('invertOp — per-op round-trip (apply then inverse = identity)', () => { + it('setCanvas (only overwritten fields restored)', () => { + expectRoundtrip(richDoc(), { op: 'setCanvas', width: 1080, fps: 24 }); + }); + + it('addTrack (append) → removeTrack', () => { + expectRoundtrip(richDoc(), { op: 'addTrack', track: videoTrack({ id: 'vNew' }) }); + }); + + it('addTrack (at index) → removeTrack', () => { + expectRoundtrip(richDoc(), { op: 'addTrack', track: videoTrack({ id: 'vNew' }), index: 1 }); + }); + + it('removeTrack of a MIDDLE track restores it at its original z-index', () => { + expectRoundtrip(richDoc(), { op: 'removeTrack', trackId: 'v1' }); + }); + + it('removeTrack of a track carrying clips + a transition', () => { + expectRoundtrip(richDoc(), { op: 'removeTrack', trackId: 'v0' }); + }); + + it('addClip → removeClip', () => { + expectRoundtrip(richDoc(), { + op: 'addClip', + trackId: 'v1', + clip: mediaClip({ id: 'cNew', mediaId: M, sourceOutSec: 2, startSec: 8 }), + }); + }); + + it('removeClip WITH a following transition restores both', () => { + expectRoundtrip(richDoc(), { op: 'removeClip', clipId: 'c1' }); + }); + + it('removeClip without a transition', () => { + expectRoundtrip(richDoc(), { op: 'removeClip', clipId: 'c2' }); + }); + + it('moveClip (same track, startSec only)', () => { + expectRoundtrip(richDoc(), { op: 'moveClip', clipId: 'c2', startSec: 7 }); + }); + + it('moveClip across tracks (drops + restores the source transition)', () => { + expectRoundtrip(richDoc(), { op: 'moveClip', clipId: 'c1', toTrackId: 'v1' }); + }); + + it('moveClip across tracks AND startSec', () => { + expectRoundtrip(richDoc(), { op: 'moveClip', clipId: 'c2', startSec: 1, toTrackId: 'v1' }); + }); + + it('setTrim', () => { + expectRoundtrip(richDoc(), { op: 'setTrim', clipId: 'c1', sourceInSec: 1, sourceOutSec: 4 }); + }); + + it('setDuration (text clip)', () => { + expectRoundtrip(richDoc(), { op: 'setDuration', clipId: 't1', durationSec: 6 }); + }); + + it('splitClip (media) WITH a following transition', () => { + expectRoundtrip(richDoc(), { op: 'splitClip', clipId: 'c1', atSec: 2, newClipId: 'c1b' }); + }); + + it('splitClip (media) without a transition', () => { + expectRoundtrip(richDoc(), { op: 'splitClip', clipId: 'c2', atSec: 6, newClipId: 'c2b' }); + }); + + it('splitClip (text)', () => { + expectRoundtrip(richDoc(), { op: 'splitClip', clipId: 't1', atSec: 1, newClipId: 't1b' }); + }); + + it('addEffect (append)', () => { + expectRoundtrip(richDoc(), { + op: 'addEffect', + clipId: 'c1', + effect: { type: 'speed', factor: 2 }, + }); + }); + + it('addEffect (at index, among existing effects)', () => { + const pre = applyOps(richDoc(), [ + { op: 'addEffect', clipId: 'c1', effect: { type: 'speed', factor: 2 } }, + { op: 'addEffect', clipId: 'c1', effect: { type: 'volume', gain: 0.5 } }, + ]); + expectRoundtrip(pre, { + op: 'addEffect', + clipId: 'c1', + effect: { type: 'fadeIn', durationSec: 1 }, + index: 1, + }); + }); + + it('removeEffect', () => { + const pre = applyOps(richDoc(), [ + { op: 'addEffect', clipId: 'c1', effect: { type: 'speed', factor: 2 } }, + { op: 'addEffect', clipId: 'c1', effect: { type: 'volume', gain: 0.5 } }, + ]); + expectRoundtrip(pre, { op: 'removeEffect', clipId: 'c1', index: 0 }); + }); + + it('setTransform on a clip with NO prior transform (inverse clears)', () => { + expectRoundtrip(richDoc(), { op: 'setTransform', clipId: 'c1', transform: { scale: 2 } }); + }); + + it('setTransform on a clip WITH a prior transform (inverse restores it)', () => { + const pre = applyOp(richDoc(), { + op: 'setTransform', + clipId: 'c1', + transform: { scale: 2, x: 0.25 }, + }); + expectRoundtrip(pre, { + op: 'setTransform', + clipId: 'c1', + transform: { x: 0.75, opacity: 0.5 }, + }); + }); + + it('clearTransform on a clip with a transform (inverse restores it)', () => { + const pre = applyOp(richDoc(), { op: 'setTransform', clipId: 'c1', transform: { scale: 3 } }); + expectRoundtrip(pre, { op: 'clearTransform', clipId: 'c1' }); + }); + + it('clearTransform on a clip with no transform (no-op round-trip)', () => { + expectRoundtrip(richDoc(), { op: 'clearTransform', clipId: 'c1' }); + }); + + it('addTransition (fresh)', () => { + expectRoundtrip(richDoc(), { + op: 'addTransition', + trackId: 'v0', + transition: { afterClipId: 'c2', kind: 'wipeleft', durationSec: 0.5 }, + }); + }); + + it('addTransition REPLACING an existing one restores the prior', () => { + expectRoundtrip(richDoc(), { + op: 'addTransition', + trackId: 'v0', + transition: { afterClipId: 'c1', kind: 'circleopen', durationSec: 2 }, + }); + }); + + it('removeTransition (existing)', () => { + expectRoundtrip(richDoc(), { op: 'removeTransition', trackId: 'v0', afterClipId: 'c1' }); + }); + + it('removeTransition (nonexistent — no-op round-trip)', () => { + expectRoundtrip(richDoc(), { op: 'removeTransition', trackId: 'v0', afterClipId: 'c2' }); + }); +}); + +describe('invertOp — LIFO sequence undo (mirrors the op-log undo path)', () => { + it('undoing a sequence of ops in reverse restores the start state', () => { + const start = richDoc(); + const ops: CompositionOp[] = [ + { op: 'setTransform', clipId: 'c1', transform: { scale: 2 } }, + { op: 'splitClip', clipId: 'c1', atSec: 2, newClipId: 'c1b' }, + { op: 'moveClip', clipId: 't1', toTrackId: 'v0' }, + { op: 'addEffect', clipId: 'c2', effect: { type: 'fadeOut', durationSec: 1 } }, + { op: 'removeTrack', trackId: 'a0' }, + ]; + + // Apply forward, capturing each op's inverse against the state it saw. + let state = start; + const inverses: CompositionOp[][] = []; + for (const op of ops) { + inverses.push(invertOp(state, op)); + state = applyOp(state, op); + } + + // Undo in reverse (LIFO), exactly as popping op-log entries would. + let undone = state; + for (let i = inverses.length - 1; i >= 0; i--) { + const inv = inverses[i]; + if (inv) undone = applyOps(undone, inv); + } + + expect(undone).toEqual(start); + }); +}); + +describe('new op-vocabulary arms', () => { + it('addTrack with index inserts at the z-order position', () => { + const comp = applyOp(richDoc(), { op: 'addTrack', track: videoTrack({ id: 'vX' }), index: 1 }); + expect(comp.tracks.map((t) => t.id)).toEqual(['v0', 'vX', 'v1', 'a0']); + }); + + it('addTrack index clamps out-of-range values', () => { + const comp = applyOp(richDoc(), { op: 'addTrack', track: videoTrack({ id: 'vX' }), index: 99 }); + expect(comp.tracks.map((t) => t.id)).toEqual(['v0', 'v1', 'a0', 'vX']); + }); + + it('clearTransform removes the transform field', () => { + const withTransform = applyOp(richDoc(), { + op: 'setTransform', + clipId: 'c1', + transform: { scale: 2 }, + }); + const cleared = applyOp(withTransform, { op: 'clearTransform', clipId: 'c1' }); + const clip = cleared.tracks[0]?.clips.find((c) => c.id === 'c1'); + expect(clip?.transform).toBeUndefined(); + }); + + it('clearTransform throws on a missing clip', () => { + expect(() => applyOp(richDoc(), { op: 'clearTransform', clipId: 'nope' })).toThrow(); + }); +}); + +describe('invertOp — clips carrying effects + a transform (no silent data loss)', () => { + // A clip with a transform and a non-trivial effect stack is where a sloppy + // inverse would quietly drop data; structuredClone + deep-equal must catch it. + function decoratedDoc(): Composition { + return applyOps(richDoc(), [ + { op: 'setTransform', clipId: 'c1', transform: { scale: 1.5, x: 0.4, opacity: 0.8 } }, + { op: 'addEffect', clipId: 'c1', effect: { type: 'speed', factor: 1.5 } }, + { op: 'addEffect', clipId: 'c1', effect: { type: 'fadeIn', durationSec: 0.5 } }, + ]); + } + + it('splitClip preserves effects + transform on both halves through the round-trip', () => { + expectRoundtrip(decoratedDoc(), { op: 'splitClip', clipId: 'c1', atSec: 2, newClipId: 'c1b' }); + }); + + it('moveClip across tracks preserves effects + transform + the dropped transition', () => { + expectRoundtrip(decoratedDoc(), { op: 'moveClip', clipId: 'c1', toTrackId: 'v1' }); + }); + + it('removeTrack restores a track whose clips carry effects + transforms', () => { + expectRoundtrip(decoratedDoc(), { op: 'removeTrack', trackId: 'v0' }); + }); +}); + +describe('invertOp — array-order edge cases (regressions)', () => { + // A track carrying MORE THAN ONE transition, where the one we disturb is not + // last in the array. A non-canonical inverse would re-append it and fail the + // deep-equal round-trip. + function multiTransitionDoc(): Composition { + return applyOps(emptyComposition(), [ + { op: 'addTrack', track: videoTrack({ id: 'v0' }) }, + { op: 'addTrack', track: videoTrack({ id: 'v1' }) }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c1', mediaId: M, sourceOutSec: 4, startSec: 0 }), + }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c2', mediaId: M, sourceOutSec: 4, startSec: 4 }), + }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c3', mediaId: M, sourceOutSec: 4, startSec: 8 }), + }, + { + op: 'addTransition', + trackId: 'v0', + transition: { afterClipId: 'c1', kind: 'fade', durationSec: 1 }, + }, + { + op: 'addTransition', + trackId: 'v0', + transition: { afterClipId: 'c2', kind: 'wipeleft', durationSec: 1 }, + }, + ]); + } + + it('removeClip of a clip whose transition is NOT last in the array', () => { + expectRoundtrip(multiTransitionDoc(), { op: 'removeClip', clipId: 'c1' }); + }); + + it('moveClip (cross-track) of a clip whose transition is not last', () => { + expectRoundtrip(multiTransitionDoc(), { op: 'moveClip', clipId: 'c1', toTrackId: 'v1' }); + }); + + it('splitClip of a clip whose transition is not last', () => { + expectRoundtrip(multiTransitionDoc(), { + op: 'splitClip', + clipId: 'c1', + atSec: 2, + newClipId: 'c1b', + }); + }); + + it('addTransition replacing one that is not last', () => { + expectRoundtrip(multiTransitionDoc(), { + op: 'addTransition', + trackId: 'v0', + transition: { afterClipId: 'c1', kind: 'circleopen', durationSec: 2 }, + }); + }); + + // Two clips sharing a startSec on one track — a startSec-only sort would flip + // their relative order when one is removed and re-added. + function equalStartDoc(): Composition { + return applyOps(emptyComposition(), [ + { op: 'addTrack', track: videoTrack({ id: 'v0' }) }, + { op: 'addTrack', track: videoTrack({ id: 'v1' }) }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'ca', mediaId: M, sourceOutSec: 3, startSec: 0 }), + }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'cb', mediaId: M, sourceOutSec: 3, startSec: 0 }), + }, + ]); + } + + it('removeClip of one of two equal-startSec siblings', () => { + expectRoundtrip(equalStartDoc(), { op: 'removeClip', clipId: 'ca' }); + }); + + it('moveClip (cross-track) of one of two equal-startSec siblings', () => { + expectRoundtrip(equalStartDoc(), { op: 'moveClip', clipId: 'ca', toTrackId: 'v1' }); + }); +}); From 0a21563230624a58e0db71b1313497ac66a04783 Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Wed, 17 Jun 2026 15:43:44 -0700 Subject: [PATCH 03/11] =?UTF-8?q?feat(timeline):=20op-log=20data=20layer?= =?UTF-8?q?=20=E2=80=94=20op=20schema,=20batch=20inverse,=20undo-stack=20a?= =?UTF-8?q?lgebra?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pure data layer for undo/redo (the I/O wiring into mutateComposition + the CLI lands next). No behavioural change yet — nothing reads or writes the log. - `CompositionOpSchema` (ops.ts): a runtime validator for a CompositionOp, used to validate ops read back from the persisted op-log. An exhaustive `Record` sample test parses every kind byte-stably so the schema can't drift from the hand-written union. Uses a defaults-free `PartialTransformSchema` for setTransform — `TransformSchema.partial()` would re-inflate omitted fields to defaults, turning a stored partial `{ scale: 2 }` into a full transform that overwrites x/y/opacity on redo. - `applyOpsTracked` (ops.ts): applies a batch, threading state so each op's inverse is captured at the pre-state it saw, returning the new doc plus a single inverse op-list that undoes the whole batch. - `doc-op-log.ts` (new): the undo-stack value type + pure algebra (record/undo/redo/canUndo/canRedo), `reconcile` (drops history on a doc/log rev mismatch — the crash-between-writes case — rather than risk a stale undo), and parse/serialize for the persisted `composition-ops.json`. Verified: applyOpsTracked round-trips (apply batch then inverse = identity); the stack algebra (truncate-redo-tail, cursor bounds); reconcile both ways; parse/serialize round-trip + malformed-op rejection. 488 tests. Co-Authored-By: Claude Opus 4.8 --- src/index.ts | 3 + src/timeline/doc-op-log.ts | 114 +++++++++++++++++++ src/timeline/ops.ts | 107 ++++++++++++++++++ tests/composition-op-schema.test.ts | 65 +++++++++++ tests/doc-op-log.test.ts | 169 ++++++++++++++++++++++++++++ 5 files changed, 458 insertions(+) create mode 100644 src/timeline/doc-op-log.ts create mode 100644 tests/composition-op-schema.test.ts create mode 100644 tests/doc-op-log.test.ts diff --git a/src/index.ts b/src/index.ts index 5bd5206..7fc7ccd 100644 --- a/src/index.ts +++ b/src/index.ts @@ -145,13 +145,16 @@ export { buildMediaMap } from './timeline/media-registry.js'; export { applyOp, applyOps, + applyOpsTracked, audioTrack, type CompositionOp, CompositionOpError, type CompositionOpKind, + CompositionOpSchema, colorClip, invertOp, mediaClip, + type TrackedApply, textClip, videoTrack, } from './timeline/ops.js'; diff --git a/src/timeline/doc-op-log.ts b/src/timeline/doc-op-log.ts new file mode 100644 index 0000000..17d329b --- /dev/null +++ b/src/timeline/doc-op-log.ts @@ -0,0 +1,114 @@ +import { z } from 'zod'; +import { type CompositionOp, CompositionOpSchema } from './ops.js'; + +/** + * The undo/redo history for a CompositionDoc — an append-only list of applied + * op batches plus a `cursor` that marks how many are currently in effect. + * Persisted next to the document in `composition-ops.json`, deliberately SEPARATE + * from `composition.json` so the render input stays a clean document. + * + * Model (a standard undo stack): + * - `entries[0..cursor-1]` are applied; `entries[cursor..]` are undone and + * available to redo. + * - undo: apply `entries[cursor-1].inverse`, cursor--. + * - redo: apply `entries[cursor].forward`, cursor++. + * - a fresh edit truncates the redo tail (`entries[cursor..]`) before appending. + * + * `rev` mirrors the document rev the log is consistent with — the coupling that + * lets `reconcile` detect (and conservatively discard) a log left inconsistent by + * a crash between the two files' writes. These functions are PURE: the I/O layer + * (document-store) reads/writes the file, mints entry ids, and applies the ops. + * + * Types are hand-written (`CompositionOp[]`, not the schema's inferred type) so + * callers apply entries directly; `DocOpLogSchema` validates the persisted form. + */ +export interface DocOpLogEntry { + id: string; + /** Human-readable summary of the batch (e.g. "addClip+addTransition"). */ + label: string; + /** The ops as originally applied — replayed on redo. */ + forward: CompositionOp[]; + /** The ops that undo this batch — replayed on undo. */ + inverse: CompositionOp[]; +} + +export interface DocOpLog { + version: 1; + rev: number; + cursor: number; + entries: DocOpLogEntry[]; +} + +const DocOpLogEntrySchema = z.object({ + id: z.string().min(1), + label: z.string(), + forward: z.array(CompositionOpSchema), + inverse: z.array(CompositionOpSchema), +}); + +export const DocOpLogSchema = z.object({ + version: z.literal(1), + rev: z.number().int().nonnegative().default(0), + cursor: z.number().int().nonnegative().default(0), + entries: z.array(DocOpLogEntrySchema).default([]), +}); + +export function emptyDocOpLog(rev = 0): DocOpLog { + return { version: 1, rev, cursor: 0, entries: [] }; +} + +/** Record a freshly-applied batch: drop any redo tail at the cursor, append the + * entry, and advance the cursor to the new end. */ +export function recordOps(log: DocOpLog, entry: DocOpLogEntry): DocOpLog { + const entries = log.entries.slice(0, log.cursor); + entries.push(entry); + return { ...log, entries, cursor: entries.length }; +} + +export function canUndo(log: DocOpLog): boolean { + return log.cursor > 0; +} + +export function canRedo(log: DocOpLog): boolean { + return log.cursor < log.entries.length; +} + +/** The entry an undo would reverse (apply its `inverse`) plus the log with the + * cursor moved back, or null if there is nothing to undo. */ +export function undo(log: DocOpLog): { entry: DocOpLogEntry; log: DocOpLog } | null { + if (!canUndo(log)) return null; + const entry = log.entries[log.cursor - 1]; + if (!entry) return null; + return { entry, log: { ...log, cursor: log.cursor - 1 } }; +} + +/** The entry a redo would re-apply (apply its `forward`) plus the log with the + * cursor moved forward, or null if there is nothing to redo. */ +export function redo(log: DocOpLog): { entry: DocOpLogEntry; log: DocOpLog } | null { + if (!canRedo(log)) return null; + const entry = log.entries[log.cursor]; + if (!entry) return null; + return { entry, log: { ...log, cursor: log.cursor + 1 } }; +} + +/** + * Bring the log into agreement with the authoritative document rev. If they + * match, the log is trusted as-is. If not, the only cause is a crash BETWEEN the + * two files' writes — the history can no longer be safely trusted (the last + * recorded change may or may not be reflected in the doc), so we conservatively + * DROP it, keeping the document intact. Losing undo history after a crash is an + * acceptable cost; replaying a stale inverse onto the wrong document is not. + */ +export function reconcile(log: DocOpLog, docRev: number): DocOpLog { + return log.rev === docRev ? log : emptyDocOpLog(docRev); +} + +/** Parse + validate a persisted log. Throws on a malformed file (the reader + * treats that the same as a desync: drop history, keep the doc). */ +export function parseDocOpLog(raw: unknown): DocOpLog { + return DocOpLogSchema.parse(raw) as unknown as DocOpLog; +} + +export function serializeDocOpLog(log: DocOpLog): string { + return `${JSON.stringify(log, null, 2)}\n`; +} diff --git a/src/timeline/ops.ts b/src/timeline/ops.ts index 9a100a0..56dcfe8 100644 --- a/src/timeline/ops.ts +++ b/src/timeline/ops.ts @@ -1,10 +1,13 @@ +import { z } from 'zod'; import { type Clip, + ClipSchema, type ColorClip, ColorClipSchema, type Composition, CompositionSchema, type Effect, + EffectSchema, type MediaClip, MediaClipSchema, makeClipId, @@ -49,6 +52,82 @@ export type CompositionOp = export type CompositionOpKind = CompositionOp['op']; +/** + * `Partial` for `setTransform` — every field optional and WITHOUT a + * default. `TransformSchema.partial()` would still re-inflate omitted fields to + * their defaults, so a stored partial op like `{ scale: 2 }` would round-trip + * into a full transform and silently change what the op overwrites on redo. + */ +const PartialTransformSchema = z.object({ + scale: z.number().positive().optional(), + x: z.number().optional(), + y: z.number().optional(), + rotationDeg: z.number().optional(), + opacity: z.number().min(0).max(1).optional(), +}); + +/** + * Runtime validator for a `CompositionOp`, used to validate ops read back from + * the persisted op-log. Kept structurally in lockstep with the hand-written + * `CompositionOp` union above; `tests/composition-op-schema.test.ts` parses an + * exhaustive sample of every op kind so the two cannot silently drift. + */ +export const CompositionOpSchema = z.discriminatedUnion('op', [ + z.object({ + op: z.literal('setCanvas'), + width: z.number().int().positive().optional(), + height: z.number().int().positive().optional(), + fps: z.number().positive().optional(), + background: z.string().optional(), + }), + z.object({ + op: z.literal('addTrack'), + track: TrackSchema, + index: z.number().int().nonnegative().optional(), + }), + z.object({ op: z.literal('removeTrack'), trackId: z.string() }), + z.object({ op: z.literal('addClip'), trackId: z.string(), clip: ClipSchema }), + z.object({ op: z.literal('removeClip'), clipId: z.string() }), + z.object({ + op: z.literal('moveClip'), + clipId: z.string(), + startSec: z.number().optional(), + toTrackId: z.string().optional(), + }), + z.object({ + op: z.literal('setTrim'), + clipId: z.string(), + sourceInSec: z.number().optional(), + sourceOutSec: z.number().optional(), + }), + z.object({ op: z.literal('setDuration'), clipId: z.string(), durationSec: z.number() }), + z.object({ + op: z.literal('splitClip'), + clipId: z.string(), + atSec: z.number(), + newClipId: z.string(), + }), + z.object({ + op: z.literal('addEffect'), + clipId: z.string(), + effect: EffectSchema, + index: z.number().int().nonnegative().optional(), + }), + z.object({ + op: z.literal('removeEffect'), + clipId: z.string(), + index: z.number().int().nonnegative(), + }), + z.object({ + op: z.literal('setTransform'), + clipId: z.string(), + transform: PartialTransformSchema, + }), + z.object({ op: z.literal('clearTransform'), clipId: z.string() }), + z.object({ op: z.literal('addTransition'), trackId: z.string(), transition: TransitionSchema }), + z.object({ op: z.literal('removeTransition'), trackId: z.string(), afterClipId: z.string() }), +]); + /** Thrown when an op references a missing entity or violates a clip invariant. */ export class CompositionOpError extends Error { constructor(message: string) { @@ -432,6 +511,34 @@ export function invertOp(comp: Composition, op: CompositionOp): CompositionOp[] } } +export interface TrackedApply { + doc: Composition; + /** The ops that, applied to `doc`, undo the WHOLE batch (each op's inverse, + * concatenated in reverse order). */ + inverse: CompositionOp[]; +} + +/** + * Apply a batch of ops, threading state so each op's inverse is captured at the + * exact pre-state it saw, and return the resulting doc plus a single inverse + * op-list that undoes the entire batch. This is the unit the op-log records per + * mutation: forward = the batch, inverse = what this returns. + */ +export function applyOpsTracked(comp: Composition, ops: CompositionOp[]): TrackedApply { + let doc = comp; + const perOpInverses: CompositionOp[][] = []; + for (const op of ops) { + perOpInverses.push(invertOp(doc, op)); + doc = applyOp(doc, op); + } + const inverse: CompositionOp[] = []; + for (let i = perOpInverses.length - 1; i >= 0; i--) { + const inv = perOpInverses[i]; + if (inv) inverse.push(...inv); + } + return { doc, inverse }; +} + // ─── internals ─────────────────────────────────────────────────────────────── function locate(comp: Composition, clipId: string): boolean { diff --git a/tests/composition-op-schema.test.ts b/tests/composition-op-schema.test.ts new file mode 100644 index 0000000..dcc2b3d --- /dev/null +++ b/tests/composition-op-schema.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it } from 'vitest'; +import { + type CompositionOp, + type CompositionOpKind, + CompositionOpSchema, + mediaClip, + videoTrack, +} from '../src/timeline/ops.js'; +import type { MediaId } from '../src/timeline/schema.js'; + +const M = 'm_aaaaaaaaaaaa' as MediaId; + +// One valid sample per op kind. The `Record` type makes +// this exhaustive at COMPILE time: add a new op kind and this map fails to type +// until a sample exists, which the parse test below then exercises — so the +// runtime schema can never silently fall behind the hand-written union. +const SAMPLES: Record = { + setCanvas: { op: 'setCanvas', width: 1280, height: 720, fps: 24, background: 'white' }, + addTrack: { op: 'addTrack', track: videoTrack({ id: 'v0' }), index: 1 }, + removeTrack: { op: 'removeTrack', trackId: 'v0' }, + addClip: { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c1', mediaId: M, sourceOutSec: 4, startSec: 0 }), + }, + removeClip: { op: 'removeClip', clipId: 'c1' }, + moveClip: { op: 'moveClip', clipId: 'c1', startSec: 2, toTrackId: 'v1' }, + setTrim: { op: 'setTrim', clipId: 'c1', sourceInSec: 1, sourceOutSec: 3 }, + setDuration: { op: 'setDuration', clipId: 't1', durationSec: 5 }, + splitClip: { op: 'splitClip', clipId: 'c1', atSec: 2, newClipId: 'c1b' }, + addEffect: { op: 'addEffect', clipId: 'c1', effect: { type: 'speed', factor: 2 }, index: 0 }, + removeEffect: { op: 'removeEffect', clipId: 'c1', index: 0 }, + setTransform: { op: 'setTransform', clipId: 'c1', transform: { scale: 2, x: 0.25 } }, + clearTransform: { op: 'clearTransform', clipId: 'c1' }, + addTransition: { + op: 'addTransition', + trackId: 'v0', + transition: { afterClipId: 'c1', kind: 'fade', durationSec: 1 }, + }, + removeTransition: { op: 'removeTransition', trackId: 'v0', afterClipId: 'c1' }, +}; + +describe('CompositionOpSchema', () => { + it.each(Object.entries(SAMPLES))('accepts a valid %s op byte-stably', (_kind, op) => { + expect(CompositionOpSchema.parse(op)).toEqual(op); + }); + + it('rejects an unknown op kind', () => { + expect(() => CompositionOpSchema.parse({ op: 'teleport', clipId: 'c1' })).toThrow(); + }); + + it('rejects a known op missing a required field', () => { + expect(() => CompositionOpSchema.parse({ op: 'removeClip' })).toThrow(); + }); + + it('rejects an op with a malformed nested payload', () => { + expect(() => + CompositionOpSchema.parse({ + op: 'addClip', + trackId: 'v0', + clip: { kind: 'media', id: 'c1', mediaId: 'not-a-media-id', sourceOutSec: 4, startSec: 0 }, + }), + ).toThrow(); + }); +}); diff --git a/tests/doc-op-log.test.ts b/tests/doc-op-log.test.ts new file mode 100644 index 0000000..8c7d7f3 --- /dev/null +++ b/tests/doc-op-log.test.ts @@ -0,0 +1,169 @@ +import { describe, expect, it } from 'vitest'; +import { emptyComposition } from '../src/timeline/composition.js'; +import { + canRedo, + canUndo, + type DocOpLogEntry, + emptyDocOpLog, + parseDocOpLog, + reconcile, + recordOps, + redo, + serializeDocOpLog, + undo, +} from '../src/timeline/doc-op-log.js'; +import { + applyOps, + applyOpsTracked, + type CompositionOp, + mediaClip, + videoTrack, +} from '../src/timeline/ops.js'; +import type { MediaId } from '../src/timeline/schema.js'; + +const M = 'm_aaaaaaaaaaaa' as MediaId; + +function entry(id: string, forward: CompositionOp[], inverse: CompositionOp[]): DocOpLogEntry { + return { id, label: forward.map((o) => o.op).join('+'), forward, inverse }; +} + +describe('applyOpsTracked', () => { + it('matches applyOps forward and returns an inverse that undoes the whole batch', () => { + const start = applyOps(emptyComposition(), [ + { op: 'addTrack', track: videoTrack({ id: 'v0' }) }, + ]); + const ops: CompositionOp[] = [ + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c1', mediaId: M, sourceOutSec: 4, startSec: 0 }), + }, + { op: 'splitClip', clipId: 'c1', atSec: 2, newClipId: 'c1b' }, + { op: 'setTransform', clipId: 'c1', transform: { scale: 2 } }, + ]; + const { doc, inverse } = applyOpsTracked(start, ops); + expect(doc).toEqual(applyOps(start, ops)); // same forward result + expect(applyOps(doc, inverse)).toEqual(start); // inverse undoes the batch + }); + + it('an empty batch is a no-op with an empty inverse', () => { + const start = emptyComposition(); + const { doc, inverse } = applyOpsTracked(start, []); + expect(doc).toEqual(start); + expect(inverse).toEqual([]); + }); +}); + +describe('doc-op-log algebra', () => { + it('recordOps appends entries and advances the cursor', () => { + let log = emptyDocOpLog(0); + log = recordOps( + log, + entry('dop_1', [{ op: 'setCanvas', width: 1 }], [{ op: 'setCanvas', width: 2 }]), + ); + log = recordOps( + log, + entry('dop_2', [{ op: 'setCanvas', width: 3 }], [{ op: 'setCanvas', width: 1 }]), + ); + expect(log.entries.map((e) => e.id)).toEqual(['dop_1', 'dop_2']); + expect(log.cursor).toBe(2); + expect(canUndo(log)).toBe(true); + expect(canRedo(log)).toBe(false); + }); + + it('undo moves the cursor back and returns the entry to reverse', () => { + let log = emptyDocOpLog(); + log = recordOps( + log, + entry('dop_1', [{ op: 'setCanvas', width: 1 }], [{ op: 'setCanvas', width: 9 }]), + ); + const u = undo(log); + expect(u?.entry.id).toBe('dop_1'); + expect(u?.log.cursor).toBe(0); + expect(canRedo(u?.log ?? log)).toBe(true); + }); + + it('redo returns the forward entry and restores the cursor', () => { + let log = emptyDocOpLog(); + log = recordOps( + log, + entry('dop_1', [{ op: 'setCanvas', width: 1 }], [{ op: 'setCanvas', width: 9 }]), + ); + const afterUndo = undo(log)?.log ?? log; + const r = redo(afterUndo); + expect(r?.entry.id).toBe('dop_1'); + expect(r?.log.cursor).toBe(1); + }); + + it('undo at the bottom and redo at the top return null', () => { + expect(undo(emptyDocOpLog())).toBeNull(); + const log = recordOps(emptyDocOpLog(), entry('dop_1', [], [])); + expect(redo(log)).toBeNull(); // cursor already at the end + }); + + it('a fresh edit after undo truncates the redo tail', () => { + let log = emptyDocOpLog(); + log = recordOps(log, entry('a', [{ op: 'setCanvas', width: 1 }], [])); + log = recordOps(log, entry('b', [{ op: 'setCanvas', width: 2 }], [])); + log = undo(log)?.log ?? log; // cursor 1; 'b' is now redoable + log = recordOps(log, entry('c', [{ op: 'setCanvas', width: 3 }], [])); // truncates 'b' + expect(log.entries.map((e) => e.id)).toEqual(['a', 'c']); + expect(log.cursor).toBe(2); + expect(canRedo(log)).toBe(false); + }); +}); + +describe('reconcile (crash-desync safety)', () => { + it('trusts the log when the revs match', () => { + const log = recordOps(emptyDocOpLog(5), entry('dop_1', [], [])); + expect(reconcile(log, 5)).toBe(log); + }); + + it('drops history (keeps an empty log at the doc rev) when revs disagree', () => { + const log = recordOps(emptyDocOpLog(5), entry('dop_1', [], [])); + const r = reconcile(log, 6); + expect(r.entries).toEqual([]); + expect(r.cursor).toBe(0); + expect(r.rev).toBe(6); + }); +}); + +describe('persistence (parse / serialize)', () => { + it('round-trips through serialize + parse', () => { + const log = recordOps( + emptyDocOpLog(3), + entry( + 'dop_1', + [ + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c1', mediaId: M, sourceOutSec: 4, startSec: 0 }), + }, + ], + [{ op: 'removeClip', clipId: 'c1' }], + ), + ); + expect(parseDocOpLog(JSON.parse(serializeDocOpLog(log)))).toEqual(log); + }); + + it('loads a rev-less / cursor-less legacy log with defaults', () => { + expect(parseDocOpLog({ version: 1, entries: [] })).toEqual({ + version: 1, + rev: 0, + cursor: 0, + entries: [], + }); + }); + + it('rejects a log carrying a malformed op', () => { + expect(() => + parseDocOpLog({ + version: 1, + rev: 0, + cursor: 0, + entries: [{ id: 'x', label: 'bad', forward: [{ op: 'nope' }], inverse: [] }], + }), + ).toThrow(); + }); +}); From dc097b4812c1c4e47a4942c98bdbc3cb87a947ce Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Wed, 17 Jun 2026 16:17:07 -0700 Subject: [PATCH 04/11] =?UTF-8?q?feat(timeline):=20persist=20the=20op-log?= =?UTF-8?q?=20=E2=80=94=20working=20undo/redo=20for=20the=20timeline?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires the op-log data layer into document-store so timeline edits are undoable end-to-end. The undo stack lives in composition-ops.json, separate from the composition.json render doc. - mutateComposition now records each batch (with its computed inverse) to the op-log and returns the new doc. Empty batches are a true no-op. - undoLastDocOp / redoDocOp apply an entry's inverse / forward ops and move the log cursor; clip timeline undo|redo|log expose them. - commitDocAndLog couples the doc + log writes in one serialized section, log-first so a crash leaves the log "ahead" (reconcile discards it) rather than the doc ahead (which could replay a stale inverse). doc.rev and log.rev advance together so reconcile can spot the crash window. Hardening from an adversarial review of the diff (all latent — no live caller yet, but fixed before the UI builds on these primitives): - overwriteComposition / resetComposition now reset the op-log inside one exclusive section and advance rev past the last readable one, so a recovery that lands on a colliding rev can't match a stale log and replay an inverse that throws; resetComposition no longer regresses rev to 0. - writeCompositionIfUnchanged resets undo history explicitly (a raw whole-doc CAS write has no recorded ops); undoable co-editing goes through mutateComposition. - readDocOpLog reads both files inside the exclusive section so an interleaved in-process commit can't make a consistent log momentarily read as empty. Co-Authored-By: Claude Opus 4.8 --- src/cli-timeline.ts | 48 +++++- src/index.ts | 4 + src/timeline/document-store.ts | 230 +++++++++++++++++++++++++---- tests/document-store-oplog.test.ts | 174 ++++++++++++++++++++++ 4 files changed, 430 insertions(+), 26 deletions(-) create mode 100644 tests/document-store-oplog.test.ts diff --git a/src/cli-timeline.ts b/src/cli-timeline.ts index 8e90bb7..18f6998 100644 --- a/src/cli-timeline.ts +++ b/src/cli-timeline.ts @@ -7,7 +7,14 @@ import { emptyComposition, makeClipId, } from './timeline/composition.js'; -import { mutateComposition, readComposition, resetComposition } from './timeline/document-store.js'; +import { + mutateComposition, + readComposition, + readDocOpLog, + redoDocOp, + resetComposition, + undoLastDocOp, +} from './timeline/document-store.js'; import { buildMediaMap } from './timeline/media-registry.js'; import type { CompositionOp } from './timeline/ops.js'; import { colorClip, mediaClip, textClip, videoTrack } from './timeline/ops.js'; @@ -27,6 +34,9 @@ const TIMELINE_HELP = `clip timeline — build and export a non-destructive comp clip timeline split clip timeline remove clip timeline show + clip timeline undo + clip timeline redo + clip timeline log clip timeline export [] `; @@ -241,6 +251,42 @@ export async function runTimeline(args: string[]): Promise { return; } + case 'undo': { + const { undone, doc, label } = await undoLastDocOp(); + out( + undone + ? { undone: true, label, rev: doc.rev } + : { undone: false, message: 'Nothing to undo.' }, + ); + return; + } + + case 'redo': { + const { redone, doc, label } = await redoDocOp(); + out( + redone + ? { redone: true, label, rev: doc.rev } + : { redone: false, message: 'Nothing to redo.' }, + ); + return; + } + + case 'log': { + const log = await readDocOpLog(); + out({ + rev: log.rev, + cursor: log.cursor, + canUndo: log.cursor > 0, + canRedo: log.cursor < log.entries.length, + entries: log.entries.map((e, i) => ({ + id: e.id, + label: e.label, + state: i < log.cursor ? 'applied' : 'undone', + })), + }); + return; + } + case 'export': { const [output] = positional; const comp = await readComposition(); diff --git a/src/index.ts b/src/index.ts index 7fc7ccd..8a2e645 100644 --- a/src/index.ts +++ b/src/index.ts @@ -133,11 +133,15 @@ export { export { CompositionConflictError, CompositionCorruptError, + compositionOpsPath, compositionPath, mutateComposition, overwriteComposition, readComposition, + readDocOpLog, + redoDocOp, resetComposition, + undoLastDocOp, writeComposition, writeCompositionIfUnchanged, } from './timeline/document-store.js'; diff --git a/src/timeline/document-store.ts b/src/timeline/document-store.ts index 7ddc463..7981645 100644 --- a/src/timeline/document-store.ts +++ b/src/timeline/document-store.ts @@ -1,17 +1,34 @@ +import { randomBytes } from 'node:crypto'; import { readFile } from 'node:fs/promises'; import { resolve } from 'node:path'; import { atomicWriteFile } from '../session/store.js'; import { createRevisionedStore } from '../storage/revisioned-store.js'; import { ensureWorkspace, getWorkspace } from '../workspace.js'; import { type Composition, CompositionSchema, emptyComposition } from './composition.js'; -import { applyOps, type CompositionOp } from './ops.js'; +import { + type DocOpLog, + type DocOpLogEntry, + emptyDocOpLog, + parseDocOpLog, + reconcile, + recordOps, + redo as redoLog, + serializeDocOpLog, + undo as undoLog, +} from './doc-op-log.js'; +import { applyOps, applyOpsTracked, type CompositionOp } from './ops.js'; const COMPOSITION_FILE = 'composition.json'; +const COMPOSITION_OPS_FILE = 'composition-ops.json'; export function compositionPath(): string { return resolve(getWorkspace(), COMPOSITION_FILE); } +export function compositionOpsPath(): string { + return resolve(getWorkspace(), COMPOSITION_OPS_FILE); +} + /** Thrown when composition.json exists but cannot be parsed/validated — surfaced, * never silently reset (same posture as the session store). */ export class CompositionCorruptError extends Error { @@ -90,46 +107,209 @@ const docStore = createRevisionedStore({ onConflict: (expectedRev, actualRev) => new CompositionConflictError(expectedRev, actualRev), }); +// ─── op-log (undo/redo) ────────────────────────────────────────────────────── +// +// The undo/redo history lives in a SEPARATE file from composition.json. Each +// mutation writes the log FIRST, then the doc: a crash in between leaves the log +// "ahead", which `reconcile` safely discards on the next read. The reverse +// ordering could leave the doc ahead and replay a stale inverse onto it, so it is +// never used. doc.rev and log.rev advance together so `reconcile` can spot the +// crash window. + +const MAX_COMMIT_ATTEMPTS = 8; + +function makeDocOpId(): string { + return `dop_${randomBytes(4).toString('hex')}`; +} + +function labelForOps(ops: CompositionOp[]): string { + return ops.map((o) => o.op).join('+') || 'edit'; +} + +/** Read the raw op-log file. Missing OR corrupt → an empty log: undo history is a + * convenience, never worth failing a read for. The caller reconciles it against + * the authoritative document rev. */ +async function readDocOpLogFile(): Promise { + await ensureWorkspace(); + let raw: string; + try { + raw = await readFile(compositionOpsPath(), 'utf-8'); + } catch (err: unknown) { + if (err instanceof Error && 'code' in err && err.code === 'ENOENT') return emptyDocOpLog(0); + throw err; + } + try { + return parseDocOpLog(JSON.parse(raw)); + } catch { + return emptyDocOpLog(0); + } +} + +async function writeDocOpLog(log: DocOpLog): Promise { + await atomicWriteFile(compositionOpsPath(), serializeDocOpLog(log)); +} + +/** The op-log reconciled against the current document rev — the trustworthy view + * of undo/redo state (drops history left inconsistent by a crash). Both reads run + * inside the store's exclusive section so an interleaved IN-PROCESS commit can't + * make a consistent log momentarily read as empty (cross-process reads stay + * best-effort, as everywhere in this store). */ +export async function readDocOpLog(): Promise { + return docStore.runExclusive(async () => { + const docRev = (await readComposition()).rev; + return reconcile(await readDocOpLogFile(), docRev); + }); +} + /** - * Read → apply ops → write, returning the new composition. Now serialized + rev - * compare-and-swap via the shared revisioned store, so the agent and the UI can - * co-edit composition.json without lost updates (writes were already atomic). - * `applyOp`'s return type is unchanged in this slice — the reversible op-log / - * `{ doc, opsApplied }` change lands in the next PR. + * Serialized commit of a coupled doc + op-log change. `derive` receives the + * current doc and the reconciled log and returns the next doc + next log (+ a + * caller result), or null for a no-op (e.g. undo with empty history). Writes the + * log first, then the doc, both advanced to the next rev. In-process this is + * exact (the revisioned store serializes writes); across processes a detected + * mid-flight write re-derives and ultimately throws `CompositionConflictError`. + * Cross-process safety is best-effort, and the window here is WIDER than the + * single-file session store's: two atomic writes (log then doc) sit between the + * rev re-check and the doc landing, so a colliding cross-process writer can still + * last-writer-win. A real cross-process lock is deferred (single-process is the + * shipping surface). + */ +async function commitDocAndLog( + derive: ( + doc: Composition, + log: DocOpLog, + ) => { doc: Composition; log: DocOpLog; result: T } | null, +): Promise<{ doc: Composition; result: T } | null> { + return docStore.runExclusive(async () => { + for (let attempt = 1; attempt <= MAX_COMMIT_ATTEMPTS; attempt++) { + const current = await readComposition(); + const log = reconcile(await readDocOpLogFile(), current.rev); + const derived = derive(current, log); + if (derived === null) return null; + + const observed = (await readComposition()).rev; + if (observed !== current.rev) { + if (attempt === MAX_COMMIT_ATTEMPTS) { + throw new CompositionConflictError(current.rev, observed); + } + continue; + } + + const nextRev = current.rev + 1; + const committedDoc = { ...derived.doc, rev: nextRev }; + const committedLog = { ...derived.log, rev: nextRev }; + await writeDocOpLog(committedLog); + await writeComposition(committedDoc); + return { doc: committedDoc, result: derived.result }; + } + throw new CompositionConflictError(-1, -1); + }); +} + +/** + * Read → apply ops → write, returning the new composition, and record the batch + * (with its computed inverse) to the op-log so the edit is undoable. Serialized + + * rev compare-and-swap via the shared revisioned store, so the agent and the UI + * can co-edit without lost updates. An empty batch is a true no-op. */ export async function mutateComposition(ops: CompositionOp[]): Promise { - const { state } = await docStore.mutate((current) => { - const next = applyOps(current, ops); - return { next, result: next }; + if (ops.length === 0) return readComposition(); + const committed = await commitDocAndLog((current, log) => { + const { doc, inverse } = applyOpsTracked(current, ops); + const entry: DocOpLogEntry = { + id: makeDocOpId(), + label: labelForOps(ops), + forward: ops, + inverse, + }; + return { doc, log: recordOps(log, entry), result: null }; }); - return state; + return committed ? committed.doc : readComposition(); +} + +/** Undo the most recent recorded edit (apply its inverse), moving the op-log + * cursor back. `undone: false` when there is nothing to undo. */ +export async function undoLastDocOp(): Promise<{ + undone: boolean; + doc: Composition; + label: string | null; +}> { + const committed = await commitDocAndLog((current, log) => { + const step = undoLog(log); + if (!step) return null; + return { doc: applyOps(current, step.entry.inverse), log: step.log, result: step.entry.label }; + }); + if (!committed) return { undone: false, doc: await readComposition(), label: null }; + return { undone: true, doc: committed.doc, label: committed.result }; +} + +/** Redo the most recently undone edit (re-apply its forward ops), moving the + * cursor forward. `redone: false` when there is nothing to redo. */ +export async function redoDocOp(): Promise<{ + redone: boolean; + doc: Composition; + label: string | null; +}> { + const committed = await commitDocAndLog((current, log) => { + const step = redoLog(log); + if (!step) return null; + return { doc: applyOps(current, step.entry.forward), log: step.log, result: step.entry.label }; + }); + if (!committed) return { redone: false, doc: await readComposition(), label: null }; + return { redone: true, doc: committed.doc, label: committed.result }; } /** - * Compare-and-swap write: persist `comp` only if the on-disk `rev` still equals - * `expectedRev`, then bump to `expectedRev + 1`. Throws `CompositionConflictError` - * if another writer committed first — the optimistic-concurrency primitive the - * co-editing UI builds on. + * Compare-and-swap write of a WHOLE document: persist `comp` only if the on-disk + * `rev` still equals `expectedRev`, then bump to `expectedRev + 1`. Throws + * `CompositionConflictError` if another writer committed first. + * + * This is a low-level CAS primitive that carries NO recorded ops, so it RESETS + * the undo history (the prior inverses no longer match the replaced document). + * Undoable co-editing must go through `mutateComposition` / `undoLastDocOp` / + * `redoDocOp`, which keep the op-log in lockstep. The reset is safe here without + * the in-section log-first dance because this always advances rev by exactly one, + * so a stale log can never collide with the new rev (unlike a recovery overwrite). */ -export function writeCompositionIfUnchanged( +export async function writeCompositionIfUnchanged( comp: Composition, expectedRev: number, ): Promise { - return docStore.writeIfUnchanged(comp, expectedRev); + const committed = await docStore.writeIfUnchanged(comp, expectedRev); + await writeDocOpLog(emptyDocOpLog(committed.rev)); + return committed; +} + +/** + * Replace the whole document and reset the op-log to empty at the new rev, both + * inside one exclusive section. Advances `rev` past the last readable revision so + * a fresh/recovered document NEVER reuses a rev — which also closes the window + * where a recovery that lands on a colliding rev could match a stale log and + * replay an inverse against the wrong document. Log-first ordering (the seeded + * log is empty anyway, so a crash resolves to the same empty history). + */ +async function overwriteDocAndResetLog(comp: Composition): Promise { + return docStore.runExclusive(async () => { + const baseRev = await readCompositionRevTolerant(); + const committed: Composition = { ...comp, rev: baseRev + 1 }; + await writeDocOpLog(emptyDocOpLog(committed.rev)); + await writeComposition(committed); + return committed; + }); } /** - * Recovery primitive: replace composition.json atomically and WITHOUT trusting - * the current file to parse (so it works precisely when the live file is the - * corrupt one being recovered), advancing `rev` past the last readable revision. + * Recovery primitive: replace composition.json WITHOUT trusting the current file + * to parse (so it works precisely when the live file is the corrupt one being + * recovered). Resets undo history — a recovered document has none — and advances + * `rev` so a stale log can never collide with the new revision. */ export function overwriteComposition(comp: Composition = emptyComposition()): Promise { - return docStore.overwrite(comp); + return overwriteDocAndResetLog(comp); } -export async function resetComposition( - comp: Composition = emptyComposition(), -): Promise { - await writeComposition(comp); - return comp; +/** Start a fresh timeline (`clip timeline new`): replace the document, clear undo + * history, and advance `rev` past the previous one (never reuse a rev). */ +export function resetComposition(comp: Composition = emptyComposition()): Promise { + return overwriteDocAndResetLog(comp); } diff --git a/tests/document-store-oplog.test.ts b/tests/document-store-oplog.test.ts new file mode 100644 index 0000000..96a8e9b --- /dev/null +++ b/tests/document-store-oplog.test.ts @@ -0,0 +1,174 @@ +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { emptyComposition } from '../src/timeline/composition.js'; +import { + compositionOpsPath, + compositionPath, + mutateComposition, + overwriteComposition, + readComposition, + readDocOpLog, + redoDocOp, + resetComposition, + undoLastDocOp, + writeCompositionIfUnchanged, +} from '../src/timeline/document-store.js'; +import { mediaClip, videoTrack } from '../src/timeline/ops.js'; +import type { MediaId } from '../src/timeline/schema.js'; + +const M = 'm_aaaaaaaaaaaa' as MediaId; + +let workspace: string; +let saved: string | undefined; + +beforeEach(async () => { + workspace = await mkdtemp(join(tmpdir(), 'mmc-oplog-test-')); + saved = process.env.MAKEMYCLIP_WORKSPACE; + process.env.MAKEMYCLIP_WORKSPACE = workspace; +}); + +afterEach(async () => { + if (saved === undefined) delete process.env.MAKEMYCLIP_WORKSPACE; + else process.env.MAKEMYCLIP_WORKSPACE = saved; + await rm(workspace, { recursive: true, force: true }); +}); + +describe('op-log integration — undo / redo', () => { + it('records each mutation and exposes undo/redo state', async () => { + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); + const log = await readDocOpLog(); + expect(log.entries).toHaveLength(1); + expect(log.cursor).toBe(1); + expect(log.entries[0]?.label).toBe('addTrack'); + }); + + it('undo reverts the document and redo re-applies it', async () => { + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); + const before = await readComposition(); + + expect((await undoLastDocOp()).undone).toBe(true); + expect((await readComposition()).tracks).toEqual([]); + + expect((await redoDocOp()).redone).toBe(true); + const after = await readComposition(); + expect(after.tracks.map((t) => t.id)).toEqual(['v0']); + // Same content as before the undo (rev advances — undo/redo are writes). + expect({ ...after, rev: 0 }).toEqual({ ...before, rev: 0 }); + }); + + it('undo at the bottom and redo at the top are no-ops', async () => { + expect((await undoLastDocOp()).undone).toBe(false); + await mutateComposition([{ op: 'setCanvas', width: 1280 }]); + expect((await redoDocOp()).redone).toBe(false); + }); + + it('a fresh edit after undo truncates the redo tail', async () => { + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v1' }) }]); + await undoLastDocOp(); // undo v1; it is now redoable + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v2' }) }]); // truncates v1 + + const log = await readDocOpLog(); + expect(log.entries.map((e) => e.label)).toEqual(['addTrack', 'addTrack']); + expect(log.cursor).toBe(2); + expect((await redoDocOp()).redone).toBe(false); + expect((await readComposition()).tracks.map((t) => t.id)).toEqual(['v0', 'v2']); + }); + + it('doc rev and log rev stay in lockstep across mutate / undo / redo', async () => { + await mutateComposition([{ op: 'setCanvas', width: 1280 }]); + expect((await readDocOpLog()).rev).toBe((await readComposition()).rev); + await mutateComposition([{ op: 'setCanvas', height: 720 }]); + expect((await readDocOpLog()).rev).toBe((await readComposition()).rev); + await undoLastDocOp(); + expect((await readDocOpLog()).rev).toBe((await readComposition()).rev); + await redoDocOp(); + expect((await readDocOpLog()).rev).toBe((await readComposition()).rev); + }); + + it('multi-step undo then redo returns to the exact prior state', async () => { + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); + await mutateComposition([ + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c1', mediaId: M, sourceOutSec: 4, startSec: 0 }), + }, + ]); + const final = await readComposition(); + + await undoLastDocOp(); + await undoLastDocOp(); + expect((await readComposition()).tracks).toEqual([]); + + await redoDocOp(); + await redoDocOp(); + expect({ ...(await readComposition()), rev: 0 }).toEqual({ ...final, rev: 0 }); + }); + + it('drops undo history when the op-log file is corrupt — the document survives', async () => { + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); + await writeFile(compositionOpsPath(), 'not valid json'); + + const log = await readDocOpLog(); + expect(log.entries).toEqual([]); // history dropped, no throw + expect((await readComposition()).tracks.map((t) => t.id)).toEqual(['v0']); // doc intact + expect((await undoLastDocOp()).undone).toBe(false); + }); + + it('resetComposition (clip timeline new) clears the op-log', async () => { + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); + await resetComposition(emptyComposition()); + expect((await readDocOpLog()).entries).toEqual([]); + expect((await undoLastDocOp()).undone).toBe(false); + }); + + it('an empty op batch is a true no-op (no rev bump, no log entry)', async () => { + await mutateComposition([{ op: 'setCanvas', width: 1280 }]); // rev 1 + const before = await readComposition(); + await mutateComposition([]); + expect((await readComposition()).rev).toBe(before.rev); + expect((await readDocOpLog()).entries).toHaveLength(1); + }); +}); + +describe('op-log lockstep across non-recording doc writers (regressions)', () => { + it('overwriteComposition over a corrupt doc resets history — undo does NOT replay a stale inverse', async () => { + // The colliding-rev case: mutate once (doc rev 1, log rev 1 with an addTrack + // entry), corrupt the doc, then recover. A naive overwrite resets the doc to + // rev 1 while leaving the log at rev 1, so reconcile MATCHES the stale log and + // undo applies removeTrack to a doc with no such track — which throws. + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); + await writeFile(compositionPath(), 'not valid json'); + await overwriteComposition(emptyComposition()); + + const log = await readDocOpLog(); + expect(log.entries).toEqual([]); + expect(log.rev).toBe((await readComposition()).rev); // lockstep + expect((await undoLastDocOp()).undone).toBe(false); // a clean no-op, not a throw + }); + + it('writeCompositionIfUnchanged resets undo history and keeps revs in lockstep', async () => { + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v1' }) }]); + const current = await readComposition(); + + await writeCompositionIfUnchanged({ ...current, background: '#111' }, current.rev); + + const log = await readDocOpLog(); + expect(log.entries).toEqual([]); // a raw CAS write is not recorded → history reset + expect(log.rev).toBe((await readComposition()).rev); // lockstep, not a silent desync + expect((await undoLastDocOp()).undone).toBe(false); + }); + + it('resetComposition advances rev past the previous one (never reuses a rev)', async () => { + await mutateComposition([{ op: 'setCanvas', width: 1280 }]); // rev 1 + await mutateComposition([{ op: 'setCanvas', height: 720 }]); // rev 2 + const reset = await resetComposition(emptyComposition()); + expect(reset.rev).toBeGreaterThan(2); + expect((await readComposition()).rev).toBe(reset.rev); + expect((await readDocOpLog()).rev).toBe(reset.rev); + }); +}); From b1e38b0a84966c06bab6b1cc4121b0cc34db4a5c Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Wed, 17 Jun 2026 16:35:46 -0700 Subject: [PATCH 05/11] =?UTF-8?q?feat(timeline):=20read-only=20introspecti?= =?UTF-8?q?on=20=E2=80=94=20show=20/=20at=20/=20frame=20(the=20agent's=20e?= =?UTF-8?q?yes)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three non-mutating views so an agent can see the document it edits: - clip timeline show (enriched): rev, exportability (dry-runs the compiler and reports exportable + blockers), and structured tracks/clips (id, kind, start, end, duration, label) + transitions — not just a clip count. - clip timeline at : every clip live at a timeline time, across all tracks, with the offset into each (clipsAtTime, half-open [startSec, end)). - clip timeline frame [output]: render ONE composited frame, through the REAL segment path (buildSegmentStep) so the preview can't diverge from export. Maps doc-local time to the post-speed segment timebase so speed clips land on the right source frame. - compile.ts: buildFrameAtPlan; the abutting check is extracted into a shared assertAbutting() used by BOTH export and frame, so a frame on an unexportable (overlapping/gapped) doc fails the same way export does instead of silently guessing a clip — an introspection tool must tell the truth about the doc. Verified: arg-array unit tests (offsets, speed timebase, boundary/gap/overlap throws) plus an end-to-end CLI render of a real 1920x1080 JPEG. The empty-string positional now errors instead of coercing to time 0. 511 tests. Co-Authored-By: Claude Opus 4.8 --- src/cli-timeline.ts | 98 +++++++++++++++++- src/index.ts | 2 + src/timeline/compile.ts | 90 +++++++++++++++-- src/timeline/composition.ts | 19 ++++ tests/timeline-introspect.test.ts | 160 ++++++++++++++++++++++++++++++ 5 files changed, 356 insertions(+), 13 deletions(-) create mode 100644 tests/timeline-introspect.test.ts diff --git a/src/cli-timeline.ts b/src/cli-timeline.ts index 18f6998..a4304ed 100644 --- a/src/cli-timeline.ts +++ b/src/cli-timeline.ts @@ -1,8 +1,12 @@ +import { resolve } from 'node:path'; import { appendOp } from './session/store.js'; -import { compileTimeline } from './timeline/compile.js'; +import { buildFrameAtPlan, CompileError, compileTimeline } from './timeline/compile.js'; import { + type Clip, type Composition, + clipDuration, clipEndSec, + clipsAtTime, compositionDuration, emptyComposition, makeClipId, @@ -34,6 +38,8 @@ const TIMELINE_HELP = `clip timeline — build and export a non-destructive comp clip timeline split clip timeline remove clip timeline show + clip timeline at + clip timeline frame [] clip timeline undo clip timeline redo clip timeline log @@ -71,6 +77,37 @@ function num(value: string | undefined, fallback: number): number { return n; } +/** A short, human-readable label for a clip (for `show` / `at`). */ +function clipLabel(clip: Clip): string { + switch (clip.kind) { + case 'media': + return clip.mediaId; + case 'text': + return clip.text.length > 30 ? `${clip.text.slice(0, 30)}…` : clip.text; + case 'color': + return clip.color; + } +} + +/** Dry-run the export compiler (pure — no FFmpeg runs) to report whether the + * document can export and, if not, the first blocker. */ +function checkExportable( + comp: Composition, + media: Awaited>, +): { exportable: boolean; blockers: string[] } { + try { + compileTimeline(comp, { + media, + dir: getWorkspace(), + output: resolve(getWorkspace(), '.probe.mp4'), + }); + return { exportable: true, blockers: [] }; + } catch (err) { + if (err instanceof CompileError) return { exportable: false, blockers: [err.message] }; + throw err; + } +} + const DEFAULT_TRACK = 'v0'; /** Ops to create the named video track if it doesn't exist yet. */ @@ -242,15 +279,70 @@ export async function runTimeline(args: string[]): Promise { case 'show': { const comp = await readComposition(); + const { exportable, blockers } = checkExportable(comp, await buildMediaMap()); out({ + rev: comp.rev, durationSec: compositionDuration(comp), canvas: { width: comp.width, height: comp.height, fps: comp.fps }, - tracks: comp.tracks.map((t) => ({ id: t.id, kind: t.kind, clips: t.clips.length })), - composition: comp, + exportable, + blockers, + tracks: comp.tracks.map((t) => ({ + id: t.id, + kind: t.kind, + muted: t.muted, + clips: t.clips.map((c) => ({ + id: c.id, + kind: c.kind, + startSec: c.startSec, + endSec: clipEndSec(c), + durationSec: clipDuration(c), + label: clipLabel(c), + })), + transitions: t.transitions.map((tr) => ({ + afterClipId: tr.afterClipId, + kind: tr.kind, + durationSec: tr.durationSec, + })), + })), }); return; } + case 'at': { + const [at] = positional; + if (!at) throw new Error('Usage: clip timeline at '); + const atSec = num(at, 0); + const comp = await readComposition(); + out({ + atSec, + clips: clipsAtTime(comp, atSec).map((h) => ({ + track: h.track.id, + clipId: h.clip.id, + kind: h.clip.kind, + localOffsetSec: h.localOffsetSec, + label: clipLabel(h.clip), + })), + }); + return; + } + + case 'frame': { + const [at, output] = positional; + if (!at) throw new Error('Usage: clip timeline frame []'); + const atSec = num(at, 0); + const comp = await readComposition(); + const media = await buildMediaMap(); + const finalOutput = output ? resolveInput(output) : newOutputPath('timeline-frame', 'jpg'); + const plan = buildFrameAtPlan( + comp, + { media, dir: getWorkspace(), output: finalOutput }, + atSec, + ); + const result = await runPlan(plan); + out({ frame: result.output, atSec }); + return; + } + case 'undo': { const { undone, doc, label } = await undoLastDocOp(); out( diff --git a/src/index.ts b/src/index.ts index 8a2e645..84792cb 100644 --- a/src/index.ts +++ b/src/index.ts @@ -86,6 +86,7 @@ export { type RevisionedStoreConfig, } from './storage/revisioned-store.js'; export { + buildFrameAtPlan, type CompileContext, CompileError, compileTimeline, @@ -106,6 +107,7 @@ export { CompositionSchema, clipDuration, clipEndSec, + clipsAtTime, compositionDuration, type Effect, EffectSchema, diff --git a/src/timeline/compile.ts b/src/timeline/compile.ts index b7ea478..b089611 100644 --- a/src/timeline/compile.ts +++ b/src/timeline/compile.ts @@ -1,4 +1,5 @@ import { resolve } from 'node:path'; +import { buildPreviewFrameArgs } from '../ffmpeg/args/preview.js'; import { buildTransitionArgs } from '../ffmpeg/args/transition.js'; import { quoteFilterArg } from '../ffmpeg/escape.js'; import { @@ -399,14 +400,13 @@ function buildHardCutArgs(a: string, b: string, output: string): string[] { * sums of durations, so allow a hair of accumulated rounding. */ const ABUT_EPSILON = 1e-4; -export function compileTimeline(comp: Composition, ctx: CompileContext): FfmpegPlan { - const track = selectVideoTrack(comp); - const clips = [...track.clips].sort((a, b) => a.startSec - b.startSec); - const transitionAfter = new Map(track.transitions.map((t) => [t.afterClipId, t])); - - // v1 renders a track as an abutting sequence. Reject gaps/overlaps with a clear - // error rather than silently collapsing them — the document is the source of - // truth, so export must not diverge from the timeline it describes. +/** + * v1 renders a track as an abutting sequence. Reject gaps/overlaps with a clear + * error rather than silently collapsing them — the document is the source of + * truth, so neither export NOR the frame preview may diverge from the timeline it + * describes. `clips` must be pre-sorted by `startSec`. + */ +function assertAbutting(clips: Clip[]): void { for (let i = 1; i < clips.length; i++) { const prev = clips[i - 1]; const cur = clips[i]; @@ -417,12 +417,82 @@ export function compileTimeline(comp: Composition, ctx: CompileContext): FfmpegP const kind = delta > 0 ? 'gap' : 'overlap'; throw new CompileError( `Timeline ${kind} between "${prev.id}" (ends ${prevEnd.toFixed(3)}s) and "${cur.id}" ` + - `(starts ${cur.startSec}s): export needs clips to abut. ` + + `(starts ${cur.startSec}s): the timeline needs clips to abut. ` + `${kind === 'gap' ? 'Close the gap (or add a filler clip)' : 'Remove the overlap'} — ` + - `positioned gaps/overlaps are not supported in export yet.`, + `positioned gaps/overlaps are not supported yet.`, ); } } +} + +/** + * Render ONE frame at timeline time `atSec` — the agent's "eyes" on the doc. + * Goes through the REAL segment path (`buildSegmentStep`) so the preview can't + * diverge from what export would produce, then extracts a still from that + * normalized segment. Two steps: encode the active clip's segment, then grab the + * frame at the mapped offset. Runs the SAME abutting check as export, so it fails + * (rather than guessing a clip) on an unexportable doc; throws `CompileError` past + * the end, in a gap, or on an overlap, and inherits the v1 single-video-track / + * no-compositing guards. + * + * Known v1 limits (flag, don't fix here): a frame inside a transition window + * shows the underlying clip, not the xfade blend; `-ss` is keyframe-accurate + * (off by up to a GOP) — fine for a thumbnail; and a long clip re-encodes a whole + * segment to grab one frame. + */ +export function buildFrameAtPlan( + comp: Composition, + ctx: CompileContext, + atSec: number, +): FfmpegPlan { + if (atSec < 0) throw new CompileError(`Frame time ${atSec}s is before the timeline start.`); + const track = selectVideoTrack(comp); + const clips = [...track.clips].sort((a, b) => a.startSec - b.startSec); + // Fail the way export does on an unexportable doc, so the preview tells the + // truth instead of silently picking one of several overlapping clips. + assertAbutting(clips); + const index = clips.findIndex((c) => atSec >= c.startSec && atSec < clipEndSec(c)); + const clip = clips[index]; + if (!clip) { + const last = clips[clips.length - 1]; + const end = last ? clipEndSec(last) : 0; + throw new CompileError( + `No clip at ${atSec}s — the timeline runs to ${end.toFixed(3)}s and ${atSec}s is ` + + `${atSec >= end ? 'past the end' : 'in a gap'}.`, + ); + } + assertCompilable(clip); + + // Encode the clip's segment exactly as export would, then extract the frame. + const segStep = buildSegmentStep(comp, clip, index, ctx); + + // Map doc-local time to the post-speed SEGMENT timebase. The ratio is 1 unless + // the clip carries a speed effect, which shortens the segment relative to the + // document extent — so a frame still lands on the source content the document + // places at `atSec`. + const clipDur = clipDuration(clip); + const outDur = outputDuration(clip); + const localDoc = atSec - clip.startSec; + const segTime = clipDur > 0 ? (localDoc * outDur) / clipDur : 0; + const lastFrame = Math.max(0, outDur - 1 / comp.fps); + const clamped = Math.max(0, Math.min(segTime, lastFrame)); + + const frameStep: FfmpegStep = { + label: `frame:${clip.id}@${atSec}`, + args: buildPreviewFrameArgs({ input: segStep.output, output: ctx.output, atSec: clamped }), + output: ctx.output, + textFiles: [], + }; + + return { steps: [segStep, frameStep], output: ctx.output, durationSec: 0 }; +} + +export function compileTimeline(comp: Composition, ctx: CompileContext): FfmpegPlan { + const track = selectVideoTrack(comp); + const clips = [...track.clips].sort((a, b) => a.startSec - b.startSec); + const transitionAfter = new Map(track.transitions.map((t) => [t.afterClipId, t])); + + assertAbutting(clips); const steps: FfmpegStep[] = []; diff --git a/src/timeline/composition.ts b/src/timeline/composition.ts index 4579c26..5d41752 100644 --- a/src/timeline/composition.ts +++ b/src/timeline/composition.ts @@ -246,3 +246,22 @@ export function findClip( } return null; } + +/** Every clip live at timeline time `atSec`, across all tracks, with its offset + * into the clip. Half-open `[startSec, clipEndSec)` so a clip that ends exactly + * where the next begins hands `atSec` to the later clip (abutting clips don't + * both claim the boundary). Pure read — the "what is at T" query. */ +export function clipsAtTime( + comp: Composition, + atSec: number, +): Array<{ track: Track; clip: Clip; localOffsetSec: number }> { + const hits: Array<{ track: Track; clip: Clip; localOffsetSec: number }> = []; + for (const track of comp.tracks) { + for (const clip of track.clips) { + if (atSec >= clip.startSec && atSec < clipEndSec(clip)) { + hits.push({ track, clip, localOffsetSec: atSec - clip.startSec }); + } + } + } + return hits; +} diff --git a/tests/timeline-introspect.test.ts b/tests/timeline-introspect.test.ts new file mode 100644 index 0000000..cb7bbbc --- /dev/null +++ b/tests/timeline-introspect.test.ts @@ -0,0 +1,160 @@ +import { describe, expect, it } from 'vitest'; +import { buildFrameAtPlan, CompileError, type MediaInfo } from '../src/timeline/compile.js'; +import { clipsAtTime, emptyComposition } from '../src/timeline/composition.js'; +import { applyOps, mediaClip, textClip, videoTrack } from '../src/timeline/ops.js'; +import type { MediaId } from '../src/timeline/schema.js'; + +const M = 'm_aaaaaaaaaaaa' as MediaId; +const MEDIA = new Map([[M, { path: '/in.mp4', hasAudio: true }]]); +const CTX = (output: string) => ({ media: MEDIA, dir: '/ws', output }); + +describe('clipsAtTime', () => { + const doc = applyOps(emptyComposition(), [ + { op: 'addTrack', track: videoTrack({ id: 'v0' }) }, + { op: 'addTrack', track: videoTrack({ id: 'v1' }) }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'a', mediaId: M, sourceOutSec: 5, startSec: 0 }), + }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'b', mediaId: M, sourceOutSec: 4, startSec: 5 }), + }, + { + op: 'addClip', + trackId: 'v1', + clip: textClip({ id: 't', text: 'hi', durationSec: 3, startSec: 0 }), + }, + ]); + + it('is half-open: the boundary belongs to the later clip', () => { + expect( + clipsAtTime(doc, 0) + .map((h) => h.clip.id) + .sort(), + ).toEqual(['a', 't']); + expect(clipsAtTime(doc, 4.999).map((h) => h.clip.id)).toContain('a'); + // At exactly 5, clip 'a' ([0,5)) ends and 'b' ([5,9)) begins → 'b', not 'a'. + expect(clipsAtTime(doc, 5).map((h) => h.clip.id)).not.toContain('a'); + expect(clipsAtTime(doc, 5).map((h) => h.clip.id)).toContain('b'); + }); + + it('returns every track live at the time, with the offset into each clip', () => { + const hits = clipsAtTime(doc, 2); + expect(hits.map((h) => h.clip.id).sort()).toEqual(['a', 't']); + expect(hits.find((h) => h.clip.id === 'a')?.localOffsetSec).toBe(2); + }); + + it('returns nothing in a gap or past the end', () => { + expect(clipsAtTime(doc, 9)).toEqual([]); // 'b' ends at 9 (half-open) + expect(clipsAtTime(doc, 100)).toEqual([]); + }); +}); + +describe('buildFrameAtPlan', () => { + function oneClip() { + return applyOps(emptyComposition(), [ + { op: 'addTrack', track: videoTrack({ id: 'v0' }) }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c1', mediaId: M, sourceOutSec: 8, startSec: 0 }), + }, + ]); + } + + it('encodes the clip segment then extracts the frame at the doc-local offset', () => { + const plan = buildFrameAtPlan(oneClip(), CTX('/ws/frame.jpg'), 3); + expect(plan.steps).toHaveLength(2); + const [seg, frame] = plan.steps; + expect(seg?.label).toBe('segment:c1'); + expect(frame?.label).toContain('frame:c1'); + expect(frame?.output).toBe('/ws/frame.jpg'); + // -ss -i … + expect(frame?.args.slice(0, 2)).toEqual(['-y', '-ss']); + expect(Number(frame?.args[2])).toBeCloseTo(3); + expect(frame?.args[4]).toBe(seg?.output); // reads the encoded segment + expect(frame?.args.at(-1)).toBe('/ws/frame.jpg'); + }); + + it('selects the second clip and offsets into it for a time in the second clip', () => { + const doc = applyOps(oneClip(), [ + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c2', mediaId: M, sourceOutSec: 4, startSec: 8 }), + }, + ]); + const plan = buildFrameAtPlan(doc, CTX('/ws/f.jpg'), 10); // 2s into c2 ([8,12)) + expect(plan.steps[0]?.label).toBe('segment:c2'); + expect(Number(plan.steps[1]?.args[2])).toBeCloseTo(2); + }); + + it('maps doc-local time to the post-speed segment timebase for a speed clip', () => { + // 8s source at 2x → segment is 4s; doc-local 4s maps to segment time 2s. + const doc = applyOps(emptyComposition(), [ + { op: 'addTrack', track: videoTrack({ id: 'v0' }) }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ + id: 'c1', + mediaId: M, + sourceOutSec: 8, + startSec: 0, + effects: [{ type: 'speed', factor: 2 }], + }), + }, + ]); + const plan = buildFrameAtPlan(doc, CTX('/ws/f.jpg'), 4); + expect(Number(plan.steps[1]?.args[2])).toBeCloseTo(2); + }); + + it('works on a text clip (no media required)', () => { + const doc = applyOps(emptyComposition(), [ + { op: 'addTrack', track: videoTrack({ id: 'v0' }) }, + { + op: 'addClip', + trackId: 'v0', + clip: textClip({ id: 't1', text: 'hi', durationSec: 4, startSec: 0 }), + }, + ]); + const plan = buildFrameAtPlan(doc, CTX('/ws/f.jpg'), 1); + expect(plan.steps[0]?.label).toBe('segment:t1'); + expect(Number(plan.steps[1]?.args[2])).toBeCloseTo(1); + }); + + it('throws past the end', () => { + expect(() => buildFrameAtPlan(oneClip(), CTX('/ws/f.jpg'), 9)).toThrow(CompileError); + }); + + it('throws in a gap', () => { + const doc = applyOps(oneClip(), [ + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c2', mediaId: M, sourceOutSec: 4, startSec: 12 }), + }, + ]); + // c1 is [0,8), c2 is [12,16); 10s is in the gap. + expect(() => buildFrameAtPlan(doc, CTX('/ws/f.jpg'), 10)).toThrow(CompileError); + }); + + it('throws before the start', () => { + expect(() => buildFrameAtPlan(oneClip(), CTX('/ws/f.jpg'), -1)).toThrow(CompileError); + }); + + it('throws on an overlapping (unexportable) timeline instead of guessing a clip', () => { + const doc = applyOps(oneClip(), [ + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'c2', mediaId: M, sourceOutSec: 6, startSec: 4 }), + }, + ]); + // c1 [0,8) and c2 [4,10) overlap on one track — export rejects this, so frame must too. + expect(() => buildFrameAtPlan(doc, CTX('/ws/f.jpg'), 5)).toThrow(CompileError); + }); +}); From ad3819c5f3db9beb55a0231cf37a24566b69ca9b Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Wed, 17 Jun 2026 19:28:51 -0700 Subject: [PATCH 06/11] feat(timeline): unify the agent + UI onto the op-aware, undoable edit path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both the chat agent and `clip ui` edited via legacy file-tools that bypassed the CompositionDoc (file-in/file-out, no undo). This routes them through the same non-destructive, undoable document the CLI already mutates, via a verb layer. - src/timeline/verbs.ts: CompositionVerbSchema (add_media/add_text/add_color/ trim/move/split/remove/transition/set_transform, each with .describe() for the agent) + lowerVerb/lowerVerbs, which isolate the impure parts (ingest, id minting, append-point) outside the pure reducer. - document-store.applyVerbs: lower → mutateComposition, applied + recorded as one undoable edit. - src/ui/timeline-tools.ts: op-aware agent tools (timeline_edit/show/undo/redo/ history); buildSystemPrompt rewritten to lead with them (legacy file-tools kept as a fallback for operations the engine doesn't cover yet). - server.ts: GET /api/timeline, POST /api/timeline/verbs. Hardening from an adversarial review of the diff: - intent-free trim/move/set_transform verbs lower to NOTHING (no more do-nothing ops polluting the undo stack / clobbering redo). - applyVerbs re-lowers under an expectedBaseRev CAS, so a concurrent in-process edit can't bake a stale append point into overlapping clips. - the verbs route maps conflicts/stale clip refs to 409/422, not 500. - add_* take an optional `id` so a batch can reference a clip it just created. - trackEnd/ensureTrack are now shared with the CLI (one definition). A pre-existing media-ingest path-confinement gap (orthogonal, would break CLI editing of files outside the workspace if naively fixed) is tracked separately. Co-Authored-By: Claude Opus 4.8 --- src/cli-timeline.ts | 20 +-- src/index.ts | 10 ++ src/timeline/document-store.ts | 44 ++++- src/timeline/verbs.ts | 285 +++++++++++++++++++++++++++++++++ src/ui/server.ts | 103 +++++++++--- src/ui/timeline-tools.ts | 124 ++++++++++++++ tests/timeline-verbs.test.ts | 187 +++++++++++++++++++++ 7 files changed, 730 insertions(+), 43 deletions(-) create mode 100644 src/timeline/verbs.ts create mode 100644 src/ui/timeline-tools.ts create mode 100644 tests/timeline-verbs.test.ts diff --git a/src/cli-timeline.ts b/src/cli-timeline.ts index a4304ed..5228de6 100644 --- a/src/cli-timeline.ts +++ b/src/cli-timeline.ts @@ -20,9 +20,9 @@ import { undoLastDocOp, } from './timeline/document-store.js'; import { buildMediaMap } from './timeline/media-registry.js'; -import type { CompositionOp } from './timeline/ops.js'; -import { colorClip, mediaClip, textClip, videoTrack } from './timeline/ops.js'; +import { colorClip, mediaClip, textClip } from './timeline/ops.js'; import { runPlan } from './timeline/run-plan.js'; +import { DEFAULT_VERB_TRACK as DEFAULT_TRACK, ensureTrack, trackEnd } from './timeline/verbs.js'; import { ingest } from './tools/ingest.js'; import { getWorkspace, newOutputPath, resolveInput } from './workspace.js'; @@ -108,22 +108,6 @@ function checkExportable( } } -const DEFAULT_TRACK = 'v0'; - -/** Ops to create the named video track if it doesn't exist yet. */ -function ensureTrack(comp: Composition, trackId: string): CompositionOp[] { - return comp.tracks.some((t) => t.id === trackId) - ? [] - : [{ op: 'addTrack', track: videoTrack({ id: trackId }) }]; -} - -/** End time of the last clip on a track (0 if empty/missing) — the append point. */ -function trackEnd(comp: Composition, trackId: string): number { - const track = comp.tracks.find((t) => t.id === trackId); - if (!track) return 0; - return track.clips.reduce((end, c) => Math.max(end, clipEndSec(c)), 0); -} - export async function runTimeline(args: string[]): Promise { const [sub, ...rest] = args; const { positional, flags } = parseArgs(rest); diff --git a/src/index.ts b/src/index.ts index 84792cb..b2be125 100644 --- a/src/index.ts +++ b/src/index.ts @@ -133,6 +133,7 @@ export { TransitionSchema, } from './timeline/composition.js'; export { + applyVerbs, CompositionConflictError, CompositionCorruptError, compositionOpsPath, @@ -176,6 +177,15 @@ export { TimecodeSchema, VideoStreamSchema, } from './timeline/schema.js'; +export { + type CompositionVerb, + type CompositionVerbKind, + CompositionVerbSchema, + DEFAULT_VERB_TRACK, + lowerVerb, + lowerVerbs, + type VerbContext, +} from './timeline/verbs.js'; export { AddAudioInput, type AddAudioInputType, diff --git a/src/timeline/document-store.ts b/src/timeline/document-store.ts index 7981645..19d1014 100644 --- a/src/timeline/document-store.ts +++ b/src/timeline/document-store.ts @@ -17,6 +17,7 @@ import { undo as undoLog, } from './doc-op-log.js'; import { applyOps, applyOpsTracked, type CompositionOp } from './ops.js'; +import { type CompositionVerb, lowerVerbs, type VerbContext } from './verbs.js'; const COMPOSITION_FILE = 'composition.json'; const COMPOSITION_OPS_FILE = 'composition-ops.json'; @@ -212,9 +213,19 @@ async function commitDocAndLog( * rev compare-and-swap via the shared revisioned store, so the agent and the UI * can co-edit without lost updates. An empty batch is a true no-op. */ -export async function mutateComposition(ops: CompositionOp[]): Promise { +export async function mutateComposition( + ops: CompositionOp[], + opts?: { expectedBaseRev?: number }, +): Promise { if (ops.length === 0) return readComposition(); const committed = await commitDocAndLog((current, log) => { + // Optimistic-concurrency guard for callers whose ops were lowered against a + // specific revision (e.g. `applyVerbs`, where a default append point is baked + // from a snapshot read): if the doc moved under us, reject so the caller can + // re-lower against the fresh state instead of committing stale positions. + if (opts?.expectedBaseRev !== undefined && current.rev !== opts.expectedBaseRev) { + throw new CompositionConflictError(opts.expectedBaseRev, current.rev); + } const { doc, inverse } = applyOpsTracked(current, ops); const entry: DocOpLogEntry = { id: makeDocOpId(), @@ -227,6 +238,37 @@ export async function mutateComposition(ops: CompositionOp[]): Promise { + // The default append point (`trackEnd`) is baked from the snapshot we lower + // against, so if another in-process writer commits between the read and the + // apply, re-lower against the fresh doc rather than land overlapping clips. A + // retry re-runs the impure lowering (it may re-`ingest`), which is acceptable + // on the rare conflict path. + for (let attempt = 1; attempt <= 4; attempt++) { + const current = await readComposition(); + const ops = await lowerVerbs(current, verbs, ctx); + try { + const doc = await mutateComposition(ops, { expectedBaseRev: current.rev }); + return { doc, ops }; + } catch (err) { + if (err instanceof CompositionConflictError && attempt < 4) continue; + throw err; + } + } + // Unreachable: the final attempt either returns or throws. + throw new CompositionConflictError(-1, -1); +} + /** Undo the most recent recorded edit (apply its inverse), moving the op-log * cursor back. `undone: false` when there is nothing to undo. */ export async function undoLastDocOp(): Promise<{ diff --git a/src/timeline/verbs.ts b/src/timeline/verbs.ts new file mode 100644 index 0000000..a94ab0f --- /dev/null +++ b/src/timeline/verbs.ts @@ -0,0 +1,285 @@ +import { z } from 'zod'; +import { + AnchorSchema, + type Composition, + clipEndSec, + makeClipId, + TransitionKindSchema, +} from './composition.js'; +import { applyOps, type CompositionOp, colorClip, mediaClip, textClip, videoTrack } from './ops.js'; +import type { MediaId } from './schema.js'; + +/** + * The VERB layer — a small, natural-language-shaped editing vocabulary that the + * agent and the `clip ui` emit, lowered to the wire-level `CompositionOp`s the + * reducer applies. Verbs exist so the impure parts of an edit — ingesting a file + * to learn its id/duration, minting clip ids, choosing an append point — live + * OUTSIDE the pure `applyOp` reducer, in `lowerVerb`. The agent and `clip ui` + * emit verbs through `applyVerbs`; the CLI builds the same ops directly through + * `mutateComposition` (and shares `trackEnd`/`ensureTrack` here). Either way every + * edit lands on one op-aware, undoable document — not the legacy file-tools that + * bypassed it. + * + * Per-field `.describe()` text is surfaced to the model by the AI SDK, so the + * verb schema doubles as the agent's tool documentation. + */ +export const CompositionVerbSchema = z.discriminatedUnion('verb', [ + z.object({ + verb: z.literal('add_media'), + id: z + .string() + .optional() + .describe( + 'Optional clip id — supply one to reference this clip in a later verb of the SAME batch.', + ), + path: z.string().describe('Absolute or workspace-relative path to a video/audio/image file.'), + track: z + .string() + .optional() + .describe('Track id to place it on (default: the main video track).'), + startSec: z + .number() + .nonnegative() + .optional() + .describe('Timeline start; default appends after the last clip.'), + sourceInSec: z + .number() + .nonnegative() + .optional() + .describe('Trim in-point within the source (default 0).'), + sourceOutSec: z + .number() + .positive() + .optional() + .describe('Trim out-point within the source (default the full duration).'), + }), + z.object({ + verb: z.literal('add_text'), + id: z + .string() + .optional() + .describe( + 'Optional clip id — supply one to reference this clip in a later verb of the SAME batch.', + ), + text: z.string().min(1).describe('The text to show.'), + durationSec: z.number().positive().describe('How long the text is on screen.'), + track: z.string().optional(), + startSec: z + .number() + .nonnegative() + .optional() + .describe('Timeline start; default appends after the last clip.'), + fontSize: z.number().int().positive().optional(), + color: z.string().optional(), + background: z + .string() + .nullable() + .optional() + .describe('Box color behind the text, or null for none.'), + anchor: AnchorSchema.optional(), + }), + z.object({ + verb: z.literal('add_color'), + id: z + .string() + .optional() + .describe( + 'Optional clip id — supply one to reference this clip in a later verb of the SAME batch.', + ), + durationSec: z.number().positive(), + color: z.string().optional().describe('Card color (default black).'), + track: z.string().optional(), + startSec: z.number().nonnegative().optional(), + }), + z.object({ + verb: z.literal('trim'), + clipId: z.string(), + sourceInSec: z.number().nonnegative().optional(), + sourceOutSec: z.number().positive().optional(), + }), + z.object({ + verb: z.literal('move'), + clipId: z.string(), + startSec: z.number().nonnegative().optional().describe('New timeline start.'), + toTrack: z.string().optional().describe('Move the clip to this track.'), + }), + z.object({ + verb: z.literal('split'), + clipId: z.string(), + atSec: z.number().describe('Timeline time to cut at (must fall inside the clip).'), + }), + z.object({ verb: z.literal('remove'), clipId: z.string() }), + z.object({ + verb: z.literal('transition'), + afterClipId: z + .string() + .describe('Add a transition after this clip, into the one that follows it.'), + kind: TransitionKindSchema.optional(), + durationSec: z.number().positive().max(10).optional(), + track: z.string().optional(), + }), + z.object({ + verb: z.literal('set_transform'), + clipId: z.string(), + scale: z.number().positive().optional(), + x: z.number().optional().describe('Horizontal center in [0,1].'), + y: z.number().optional().describe('Vertical center in [0,1].'), + rotationDeg: z.number().optional(), + opacity: z.number().min(0).max(1).optional(), + }), +]); +export type CompositionVerb = z.infer; +export type CompositionVerbKind = CompositionVerb['verb']; + +/** The impure capabilities `lowerVerb` needs — supplied by the I/O layer so the + * lowering stays unit-testable with stubs. */ +export interface VerbContext { + /** Register a media file and report its id + duration (wraps the ingest tool). */ + ingest: (path: string) => Promise<{ mediaId: MediaId; durationSec: number }>; + /** Track a clip is placed on when the verb doesn't say. */ + defaultTrack: string; +} + +const DEFAULT_TRACK = 'v0'; + +/** End time of the last clip on a track (0 if empty/missing) — the append point. + * Exported so the CLI shares one definition instead of a drifting copy. */ +export function trackEnd(comp: Composition, trackId: string): number { + const track = comp.tracks.find((t) => t.id === trackId); + if (!track) return 0; + return track.clips.reduce((end, c) => Math.max(end, clipEndSec(c)), 0); +} + +/** Ops to create the named video track if it doesn't exist yet. */ +export function ensureTrack(comp: Composition, trackId: string): CompositionOp[] { + return comp.tracks.some((t) => t.id === trackId) + ? [] + : [{ op: 'addTrack', track: videoTrack({ id: trackId }) }]; +} + +/** + * Lower one verb to the `CompositionOp`s that carry it out, against the CURRENT + * document. Impure (mints ids, may `ingest`), but it never mutates `comp` — the + * caller applies the returned ops through `mutateComposition` so the edit is + * validated, recorded, and undoable. + */ +export async function lowerVerb( + comp: Composition, + verb: CompositionVerb, + ctx: VerbContext, +): Promise { + switch (verb.verb) { + case 'add_media': { + const { mediaId, durationSec } = await ctx.ingest(verb.path); + const trackId = verb.track ?? ctx.defaultTrack; + const clip = mediaClip({ + id: verb.id ?? makeClipId(), + mediaId, + sourceInSec: verb.sourceInSec ?? 0, + sourceOutSec: verb.sourceOutSec ?? durationSec, + startSec: verb.startSec ?? trackEnd(comp, trackId), + }); + return [...ensureTrack(comp, trackId), { op: 'addClip', trackId, clip }]; + } + case 'add_text': { + const trackId = verb.track ?? ctx.defaultTrack; + const style: Record = {}; + if (verb.fontSize !== undefined) style.fontSize = verb.fontSize; + if (verb.color !== undefined) style.color = verb.color; + if (verb.background !== undefined) style.background = verb.background; + if (verb.anchor !== undefined) style.anchor = verb.anchor; + const clip = textClip({ + id: verb.id ?? makeClipId(), + text: verb.text, + durationSec: verb.durationSec, + startSec: verb.startSec ?? trackEnd(comp, trackId), + style, + }); + return [...ensureTrack(comp, trackId), { op: 'addClip', trackId, clip }]; + } + case 'add_color': { + const trackId = verb.track ?? ctx.defaultTrack; + const clip = colorClip({ + id: verb.id ?? makeClipId(), + color: verb.color, + durationSec: verb.durationSec, + startSec: verb.startSec ?? trackEnd(comp, trackId), + }); + return [...ensureTrack(comp, trackId), { op: 'addClip', trackId, clip }]; + } + case 'trim': { + // An intent-free verb lowers to nothing; `mutateComposition`'s empty-batch + // guard then makes it a true no-op (no rev bump, no spurious undo entry). + if (verb.sourceInSec === undefined && verb.sourceOutSec === undefined) return []; + return [ + { + op: 'setTrim', + clipId: verb.clipId, + ...(verb.sourceInSec !== undefined ? { sourceInSec: verb.sourceInSec } : {}), + ...(verb.sourceOutSec !== undefined ? { sourceOutSec: verb.sourceOutSec } : {}), + }, + ]; + } + case 'move': { + if (verb.startSec === undefined && verb.toTrack === undefined) return []; + return [ + { + op: 'moveClip', + clipId: verb.clipId, + ...(verb.startSec !== undefined ? { startSec: verb.startSec } : {}), + ...(verb.toTrack !== undefined ? { toTrackId: verb.toTrack } : {}), + }, + ]; + } + case 'split': + return [{ op: 'splitClip', clipId: verb.clipId, atSec: verb.atSec, newClipId: makeClipId() }]; + case 'remove': + return [{ op: 'removeClip', clipId: verb.clipId }]; + case 'transition': + return [ + { + op: 'addTransition', + trackId: verb.track ?? ctx.defaultTrack, + transition: { + afterClipId: verb.afterClipId, + kind: verb.kind ?? 'fade', + durationSec: verb.durationSec ?? 1, + }, + }, + ]; + case 'set_transform': { + const transform: Record = {}; + if (verb.scale !== undefined) transform.scale = verb.scale; + if (verb.x !== undefined) transform.x = verb.x; + if (verb.y !== undefined) transform.y = verb.y; + if (verb.rotationDeg !== undefined) transform.rotationDeg = verb.rotationDeg; + if (verb.opacity !== undefined) transform.opacity = verb.opacity; + if (Object.keys(transform).length === 0) return []; // nothing to set — no-op. + return [{ op: 'setTransform', clipId: verb.clipId, transform }]; + } + default: { + const _exhaustive: never = verb; + throw new Error(`Unknown verb: ${JSON.stringify(_exhaustive)}`); + } + } +} + +/** Lower a batch of verbs against the CURRENT doc, threading state so each verb + * sees the effect of the ones before it (e.g. a default append point shifts as + * earlier verbs add clips). Returns the flat op list to apply atomically. */ +export async function lowerVerbs( + comp: Composition, + verbs: CompositionVerb[], + ctx: VerbContext, +): Promise { + let doc = comp; + const all: CompositionOp[] = []; + for (const verb of verbs) { + const ops = await lowerVerb(doc, verb, ctx); + all.push(...ops); + doc = applyOps(doc, ops); + } + return all; +} + +export { DEFAULT_TRACK as DEFAULT_VERB_TRACK }; diff --git a/src/ui/server.ts b/src/ui/server.ts index 16726e8..5a48f3d 100644 --- a/src/ui/server.ts +++ b/src/ui/server.ts @@ -12,16 +12,24 @@ import { convertToModelMessages, stepCountIs, streamText, type UIMessage } from import getPort from 'get-port'; import { Hono } from 'hono'; import open from 'open'; -import { ZodError } from 'zod'; +import { ZodError, z } from 'zod'; import { probe } from '../ffmpeg/probe.js'; import { appendOp, readSession, SessionCorruptError, snapshotsDir } from '../session/store.js'; import type { SessionEntry } from '../session/types.js'; +import { + applyVerbs, + CompositionConflictError, + readComposition, +} from '../timeline/document-store.js'; +import { CompositionOpError } from '../timeline/ops.js'; +import { CompositionVerbSchema } from '../timeline/verbs.js'; import { ingest } from '../tools/ingest.js'; import { preview } from '../tools/preview.js'; import { snapshot } from '../tools/snapshot.js'; import { undo } from '../tools/undo.js'; import { ensureWorkspace, getWorkspace } from '../workspace.js'; import { buildChatTools } from './chat-tools.js'; +import { buildTimelineTools, makeVerbContext, summarizeComposition } from './timeline-tools.js'; import { isRegisteredTool, TOOL_REGISTRY } from './tool-registry.js'; const HERE = dirname(fileURLToPath(import.meta.url)); @@ -148,6 +156,45 @@ export async function startUiServer(options: UiServerOptions = {}): Promise { + return c.json({ document: await readComposition() }); + }); + + /** + * POST /api/timeline/verbs — apply editing verbs to the document as ONE + * undoable change (the same op-aware path the agent and CLI use). Body: + * `{ verbs: CompositionVerb[] }`. + */ + app.post('/api/timeline/verbs', async (c) => { + let body: unknown; + try { + body = await c.req.json(); + } catch { + return c.json({ error: 'Body must be JSON' }, 400); + } + try { + const verbs = z + .array(CompositionVerbSchema) + .min(1) + .parse((body as { verbs?: unknown }).verbs); + const { doc, ops } = await applyVerbs(verbs, makeVerbContext()); + return c.json({ applied: ops.length, document: doc }); + } catch (err) { + if (err instanceof ZodError) { + return c.json({ error: 'Validation failed', issues: err.issues }, 400); + } + // A lost write race is retriable (409); a stale/bad clip reference is a + // client condition (422) — neither is a server fault. + if (err instanceof CompositionConflictError) return c.json({ error: err.message }, 409); + if (err instanceof CompositionOpError) return c.json({ error: err.message }, 422); + const message = err instanceof Error ? err.message : String(err); + return c.json({ error: message }, 500); + } + }); + /** * Session safety endpoints. These intentionally live outside the * /api/tools/:name registry because snapshot/undo are meta-operations on @@ -263,28 +310,34 @@ export async function startUiServer(options: UiServerOptions = {}): Promise { - return readSession().then((session) => { - const workspace = getWorkspace(); - const recent = session.entries.slice(-20).map(summarizeEntryForChat).join('\n'); - const head = - session.entries.length > 20 - ? `\n(showing last 20 of ${session.entries.length}; older ops exist)\n` - : ''; - return [ - 'You are the assistant inside MakeMyClip Editor — an FFmpeg-backed video editor.', - 'You run editing operations by calling tools. Each tool call produces a new output file and appends one entry to the session log; the UI picks it up automatically.', - '', - 'Conventions:', - '- Refer to media by absolute file paths only — never relative.', - "- Chain ops by passing the previous op result's `path` as the next op's `input` (or `result.ref.path` for ingest).", - '- Prefer stream-copy ops (trim, split, concat) before re-encoding ops (transition, add_text, render) to keep results fast and lossless.', - '- When the user asks for something ambiguous, call `inspect`-style tools or read the session list to ground yourself before editing.', - '', - `Workspace: ${workspace}`, - `Session has ${session.entries.length} entries.${head}`, - recent ? `Recent ops:\n${recent}` : 'Session is empty.', - ].join('\n'); - }); + return Promise.all([readSession(), readComposition().catch(() => null)]).then( + ([session, comp]) => { + const workspace = getWorkspace(); + const recent = session.entries.slice(-12).map(summarizeEntryForChat).join('\n'); + return [ + 'You are the assistant inside MakeMyClip Editor — a local, FFmpeg-backed video editor.', + '', + 'You build a video by editing a TIMELINE DOCUMENT: a non-destructive list of tracks and clips. Every edit is recorded and undoable.', + '', + 'Primary tools — prefer these:', + '- `timeline_edit` — apply editing verbs (add_media, add_text, add_color, trim, move, split, remove, transition, set_transform) to the document. Batch related verbs into ONE call; they apply atomically as a single undoable change.', + '- `timeline_show` — read the current document (tracks, clips, timings). Call it to ground yourself BEFORE editing and to VERIFY AFTER an edit.', + '- `timeline_undo` / `timeline_redo` / `timeline_history` — move through the edit history.', + '', + 'Workflow: timeline_show → timeline_edit → timeline_show to confirm. Refer to clips by their `id` (from timeline_show); refer to media by absolute or workspace-relative path.', + '', + 'The legacy file-tools (chroma_key, stabilize, overlay, render, …) operate on standalone files, NOT the document. Use them only for operations the timeline engine does not cover yet — e.g. final render, chroma key, stabilization.', + '', + `Workspace: ${workspace}`, + comp + ? `Current document: ${JSON.stringify(summarizeComposition(comp))}` + : 'Current document: unreadable (corrupt?) — start over with `timeline_edit` after the user removes composition.json, or guide them to `clip timeline new`.', + recent ? `Recent file-tool ops:\n${recent}` : '', + ] + .filter(Boolean) + .join('\n'); + }, + ); } /** @@ -351,7 +404,9 @@ export async function startUiServer(options: UiServerOptions = {}): Promise { + const resolved = resolveInput(path); + const result = await ingest({ path: resolved }); + await appendOp({ + tool: 'ingest', + args: { path: resolved }, + result: result as unknown as Record, + }); + return { mediaId: result.mediaId, durationSec: result.ref.durationSec }; + }, + }; +} + +/** Compact, agent-readable view of the document — the textual "eyes". */ +export function summarizeComposition(comp: Composition): unknown { + return { + rev: comp.rev, + durationSec: compositionDuration(comp), + canvas: { width: comp.width, height: comp.height, fps: comp.fps }, + tracks: comp.tracks.map((t) => ({ + id: t.id, + kind: t.kind, + clips: t.clips.map((c) => ({ + id: c.id, + kind: c.kind, + startSec: c.startSec, + endSec: clipEndSec(c), + })), + })), + }; +} + +/** + * The op-aware timeline toolset for the chat agent: edits go through the verb + * layer → `mutateComposition`, so every agent edit lands on the SAME non- + * destructive, undoable document the human and CLI edit — not the legacy file + * tools that produced orphaned output files. Errors return as data so a failed + * call doesn't abort the streaming turn. + */ +export function buildTimelineTools(): Record { + const ctx = makeVerbContext(); + const asData = (fn: () => Promise) => async () => { + try { + return await fn(); + } catch (err) { + return { error: err instanceof Error ? err.message : String(err) }; + } + }; + + return { + timeline_edit: tool({ + description: + 'Edit the timeline document with one or more verbs (applied atomically as a single undoable change). This is the primary way to build a video: add clips, trim, move, split, transition, etc. Returns the updated document summary.', + inputSchema: z.object({ + verbs: z.array(CompositionVerbSchema).min(1).describe('Editing verbs to apply in order.'), + }), + execute: async ({ verbs }) => + asData(async () => { + const { doc, ops } = await applyVerbs(verbs, ctx); + return { applied: ops.length, document: summarizeComposition(doc) }; + })(), + }), + timeline_show: tool({ + description: + 'Read the current timeline document — tracks, clips, and timings. Call this to ground yourself before editing or to verify a change.', + inputSchema: z.object({}), + execute: async () => asData(async () => summarizeComposition(await readComposition()))(), + }), + timeline_undo: tool({ + description: 'Undo the most recent timeline edit.', + inputSchema: z.object({}), + execute: async () => + asData(async () => { + const { undone, label } = await undoLastDocOp(); + return undone ? { undone: true, label } : { undone: false, message: 'Nothing to undo.' }; + })(), + }), + timeline_redo: tool({ + description: 'Redo the most recently undone timeline edit.', + inputSchema: z.object({}), + execute: async () => + asData(async () => { + const { redone, label } = await redoDocOp(); + return redone ? { redone: true, label } : { redone: false, message: 'Nothing to redo.' }; + })(), + }), + timeline_history: tool({ + description: 'List the timeline edit history (what can be undone / redone).', + inputSchema: z.object({}), + execute: async () => + asData(async () => { + const log = await readDocOpLog(); + return { + canUndo: log.cursor > 0, + canRedo: log.cursor < log.entries.length, + entries: log.entries.map((e, i) => ({ + label: e.label, + state: i < log.cursor ? 'applied' : 'undone', + })), + }; + })(), + }), + }; +} diff --git a/tests/timeline-verbs.test.ts b/tests/timeline-verbs.test.ts new file mode 100644 index 0000000..f922bb9 --- /dev/null +++ b/tests/timeline-verbs.test.ts @@ -0,0 +1,187 @@ +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { emptyComposition } from '../src/timeline/composition.js'; +import { + applyVerbs, + CompositionConflictError, + mutateComposition, + readComposition, + readDocOpLog, + undoLastDocOp, +} from '../src/timeline/document-store.js'; +import { applyOps, type CompositionOp, videoTrack } from '../src/timeline/ops.js'; +import type { MediaId } from '../src/timeline/schema.js'; +import { lowerVerb, lowerVerbs, type VerbContext } from '../src/timeline/verbs.js'; + +const M = 'm_aaaaaaaaaaaa' as MediaId; +const ctx = (durationSec = 10): VerbContext => ({ + ingest: async () => ({ mediaId: M, durationSec }), + defaultTrack: 'v0', +}); + +type AddClipOp = Extract; +const addClips = (ops: CompositionOp[]): AddClipOp[] => + ops.filter((o): o is AddClipOp => o.op === 'addClip'); + +describe('lowerVerb', () => { + const empty = emptyComposition(); + + it('add_media ingests, creates the default track, and appends a media clip', async () => { + const ops = await lowerVerb(empty, { verb: 'add_media', path: '/a.mp4' }, ctx()); + expect(ops).toMatchObject([ + { op: 'addTrack', track: { id: 'v0' } }, + { + op: 'addClip', + trackId: 'v0', + clip: { kind: 'media', mediaId: M, sourceInSec: 0, sourceOutSec: 10, startSec: 0 }, + }, + ]); + }); + + it('add_media honors explicit start/trim/track and skips addTrack when it exists', async () => { + const doc = applyOps(empty, [{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); + const ops = await lowerVerb( + doc, + { + verb: 'add_media', + path: '/a.mp4', + track: 'v0', + startSec: 5, + sourceInSec: 1, + sourceOutSec: 4, + }, + ctx(), + ); + expect(ops).toMatchObject([ + { op: 'addClip', clip: { sourceInSec: 1, sourceOutSec: 4, startSec: 5 } }, + ]); + expect(ops.some((o) => o.op === 'addTrack')).toBe(false); + }); + + it('add_text lowers style and appends after the last clip by default', async () => { + const ops = await lowerVerb( + empty, + { verb: 'add_text', text: 'hi', durationSec: 3, color: 'red' }, + ctx(), + ); + expect(ops).toMatchObject([ + { op: 'addTrack', track: { id: 'v0' } }, + { + op: 'addClip', + clip: { kind: 'text', text: 'hi', durationSec: 3, style: { color: 'red' } }, + }, + ]); + }); + + it('edit verbs lower 1:1 to ops', async () => { + expect( + await lowerVerb( + empty, + { verb: 'trim', clipId: 'c1', sourceInSec: 1, sourceOutSec: 3 }, + ctx(), + ), + ).toEqual([{ op: 'setTrim', clipId: 'c1', sourceInSec: 1, sourceOutSec: 3 }]); + expect( + await lowerVerb(empty, { verb: 'move', clipId: 'c1', startSec: 2, toTrack: 'v1' }, ctx()), + ).toEqual([{ op: 'moveClip', clipId: 'c1', startSec: 2, toTrackId: 'v1' }]); + expect(await lowerVerb(empty, { verb: 'remove', clipId: 'c1' }, ctx())).toEqual([ + { op: 'removeClip', clipId: 'c1' }, + ]); + expect(await lowerVerb(empty, { verb: 'transition', afterClipId: 'c1' }, ctx())).toEqual([ + { + op: 'addTransition', + trackId: 'v0', + transition: { afterClipId: 'c1', kind: 'fade', durationSec: 1 }, + }, + ]); + expect( + await lowerVerb( + empty, + { verb: 'set_transform', clipId: 'c1', scale: 2, opacity: 0.5 }, + ctx(), + ), + ).toEqual([{ op: 'setTransform', clipId: 'c1', transform: { scale: 2, opacity: 0.5 } }]); + const split = await lowerVerb(empty, { verb: 'split', clipId: 'c1', atSec: 2 }, ctx()); + expect(split[0]).toMatchObject({ op: 'splitClip', clipId: 'c1', atSec: 2 }); + }); +}); + +describe('lowerVerbs (state threads between verbs)', () => { + it('the default append point shifts as earlier verbs add clips', async () => { + const ops = await lowerVerbs( + emptyComposition(), + [ + { verb: 'add_media', path: '/a.mp4' }, // [0,5) + { verb: 'add_color', durationSec: 2 }, // should append at 5 + ], + ctx(5), + ); + const color = addClips(ops).find((o) => o.clip.kind === 'color'); + expect(color?.clip.startSec).toBe(5); + }); +}); + +describe('intent-free verbs lower to no ops (no spurious undo entry)', () => { + it('trim/move with no value fields, and an empty set_transform, return []', async () => { + const c = ctx(); + expect(await lowerVerb(emptyComposition(), { verb: 'trim', clipId: 'c1' }, c)).toEqual([]); + expect(await lowerVerb(emptyComposition(), { verb: 'move', clipId: 'c1' }, c)).toEqual([]); + expect(await lowerVerb(emptyComposition(), { verb: 'set_transform', clipId: 'c1' }, c)).toEqual( + [], + ); + }); +}); + +describe('optional clip id (mid-batch references)', () => { + it('add_* honor a caller-supplied id so a later verb in the batch can target it', async () => { + const ops = await lowerVerbs( + emptyComposition(), + [ + { verb: 'add_color', id: 'card', durationSec: 2 }, + { verb: 'set_transform', clipId: 'card', opacity: 0.5 }, + ], + ctx(), + ); + expect(addClips(ops)[0]?.clip.id).toBe('card'); + expect(ops.some((o) => o.op === 'setTransform' && o.clipId === 'card')).toBe(true); + }); +}); + +describe('applyVerbs (op-aware + undoable)', () => { + let workspace: string; + let saved: string | undefined; + beforeEach(async () => { + workspace = await mkdtemp(join(tmpdir(), 'mmc-verbs-test-')); + saved = process.env.MAKEMYCLIP_WORKSPACE; + process.env.MAKEMYCLIP_WORKSPACE = workspace; + }); + afterEach(async () => { + if (saved === undefined) delete process.env.MAKEMYCLIP_WORKSPACE; + else process.env.MAKEMYCLIP_WORKSPACE = saved; + await rm(workspace, { recursive: true, force: true }); + }); + + it('applies verbs through mutateComposition and records ONE undoable entry', async () => { + const { doc, ops } = await applyVerbs([{ verb: 'add_media', path: '/a.mp4' }], ctx(6)); + expect(doc.tracks[0]?.clips[0]?.kind).toBe('media'); + expect(ops.length).toBeGreaterThan(0); + + const log = await readDocOpLog(); + expect(log.entries).toHaveLength(1); + + const undo = await undoLastDocOp(); + expect(undo.undone).toBe(true); + expect((await readComposition()).tracks).toEqual([]); + }); + + it('mutateComposition rejects a stale expectedBaseRev (the guard applyVerbs retries on)', async () => { + await mutateComposition([{ op: 'setCanvas', width: 1280 }]); // rev 1 + await expect( + mutateComposition([{ op: 'setCanvas', height: 720 }], { expectedBaseRev: 0 }), + ).rejects.toBeInstanceOf(CompositionConflictError); + const doc = await mutateComposition([{ op: 'setCanvas', height: 720 }], { expectedBaseRev: 1 }); + expect(doc.rev).toBe(2); + }); +}); From f4dc04ce1b1e03e3cc6b62dfbc5b0f0b5b3b96ce Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Wed, 17 Jun 2026 22:53:52 -0700 Subject: [PATCH 07/11] fix(ui): confine agent/UI media paths to the workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chat agent and the localhost `clip ui` routes dispatch model/HTTP- supplied file paths straight into FFmpeg — via every tool in the registry (POST /api/tools/:name + the agent's tool calls) and via the add_media verb (POST /api/timeline/verbs). resolveInput returns absolute paths as-is, so any of these untrusted surfaces could read an arbitrary file off disk: POST /api/tools/render {"input":"/etc/passwd"} re-encodes it into a workspace file the UI then serves back — an exfiltration channel. Violates AGENTS.md non-negotiable #3. Add resolveInWorkspace (relative resolves against the workspace; an absolute path must already sit inside it; ..-traversal and out-of-tree absolutes throw WorkspaceBoundaryError) and route EVERY path-bearing registry tool plus the add_media verb through it — not just ingest. Both server routes map the error to HTTP 403. The trusted CLI keeps resolveInput (a user-typed path is consent) and never dispatches through the registry, so editing files anywhere on disk still works. Containment is by resolved-path prefix and does not yet follow symlinks (documented residual: an in-workspace symlink can still point out — hardening via realpath is a follow-up). The boundary message is audience-neutral so it reads sensibly both as agent tool-result data and in the 403 body. --- src/ui/server.ts | 4 +- src/ui/timeline-tools.ts | 5 +- src/ui/tool-registry.ts | 110 +++++++++++++++++++++++++++++++-------- src/workspace.ts | 45 +++++++++++++++- tests/workspace.test.ts | 105 +++++++++++++++++++++++++++++++++++++ 5 files changed, 244 insertions(+), 25 deletions(-) create mode 100644 tests/workspace.test.ts diff --git a/src/ui/server.ts b/src/ui/server.ts index 5a48f3d..2254e59 100644 --- a/src/ui/server.ts +++ b/src/ui/server.ts @@ -27,7 +27,7 @@ import { ingest } from '../tools/ingest.js'; import { preview } from '../tools/preview.js'; import { snapshot } from '../tools/snapshot.js'; import { undo } from '../tools/undo.js'; -import { ensureWorkspace, getWorkspace } from '../workspace.js'; +import { ensureWorkspace, getWorkspace, WorkspaceBoundaryError } from '../workspace.js'; import { buildChatTools } from './chat-tools.js'; import { buildTimelineTools, makeVerbContext, summarizeComposition } from './timeline-tools.js'; import { isRegisteredTool, TOOL_REGISTRY } from './tool-registry.js'; @@ -151,6 +151,7 @@ export async function startUiServer(options: UiServerOptions = {}): Promise { - const resolved = resolveInput(path); + // Untrusted (agent/UI) input — confine to the workspace. + const resolved = resolveInWorkspace(path); const result = await ingest({ path: resolved }); await appendOp({ tool: 'ingest', diff --git a/src/ui/tool-registry.ts b/src/ui/tool-registry.ts index 4da2f42..c7e44ff 100644 --- a/src/ui/tool-registry.ts +++ b/src/ui/tool-registry.ts @@ -18,12 +18,98 @@ import { StabilizeInput, stabilize } from '../tools/stabilize.js'; import { TransitionInput, transition } from '../tools/transition.js'; import { TrimInput, trim } from '../tools/trim.js'; import { ZoomPanInput, zoomPan } from '../tools/zoom-pan.js'; +import { resolveInWorkspace } from '../workspace.js'; + +interface ToolEntry { + // biome-ignore lint/suspicious/noExplicitAny: each tool has a different schema; heterogeneous registry + schema: z.ZodType; + // biome-ignore lint/suspicious/noExplicitAny: schema validates at runtime; downstream casts + fn: (input: any) => Promise; + /** + * Input fields that carry a source file path (a string, or a string[] of + * paths). At the untrusted agent/UI boundary these are confined to the + * workspace before the handler runs — see `confineToolInput`. A handler whose + * inputs are all in-document/in-workspace by construction omits this. + */ + pathFields?: readonly string[]; +} + +/** + * Confine an untrusted tool input's source-path fields to the workspace. + * + * Both agent/UI dispatch surfaces — `POST /api/tools/:name` (unauthenticated + * localhost) and the chat agent's legacy tool calls — feed model/HTTP-supplied + * paths straight into FFmpeg, which reads whatever the OS will open. Without + * this, pointing any path-bearing tool (`render`, `trim`, `overlay`, …) at a + * file outside the workspace is an arbitrary-file read — and, since the result + * is re-encoded into a workspace file the UI serves back, an exfiltration + * channel. So every such field is routed through `resolveInWorkspace`, which + * throws `WorkspaceBoundaryError` on escape (AGENTS.md non-negotiable #3). The + * trusted CLI never dispatches through here — a user-typed path is consent. + * + * Only the listed fields are touched; the rest of the input passes through + * untouched. A wrong/absent field name would silently leave a path unconfined, + * so `pathFields` is kept in lockstep with each handler's `resolveInput` calls. + */ +function confineToolInput(input: unknown, pathFields: readonly string[]): unknown { + if (input === null || typeof input !== 'object') return input; + const confined: Record = { ...(input as Record) }; + for (const field of pathFields) { + const value = confined[field]; + if (typeof value === 'string') { + confined[field] = resolveInWorkspace(value); + } else if (Array.isArray(value)) { + confined[field] = value.map((item) => + typeof item === 'string' ? resolveInWorkspace(item) : item, + ); + } + } + return confined; +} + +/** + * Raw tool definitions. `pathFields` declares which inputs are source paths; + * the workspace-confining wrapper is applied below when building TOOL_REGISTRY. + */ +const RAW_TOOLS: Record = { + ingest: { schema: IngestInput, fn: ingest, pathFields: ['path'] }, + trim: { schema: TrimInput, fn: trim, pathFields: ['input'] }, + split: { schema: SplitInput, fn: split, pathFields: ['input'] }, + concat: { schema: ConcatInput, fn: concat, pathFields: ['inputs'] }, + add_text: { schema: AddTextInput, fn: addText, pathFields: ['input'] }, + add_audio: { schema: AddAudioInput, fn: addAudio, pathFields: ['input', 'audio'] }, + add_title_card: { schema: AddTitleCardInput, fn: addTitleCard, pathFields: ['input'] }, + transition: { schema: TransitionInput, fn: transition, pathFields: ['inputA', 'inputB'] }, + render: { schema: RenderInput, fn: render, pathFields: ['input'] }, + preview: { schema: PreviewInput, fn: preview, pathFields: ['input'] }, + adjust: { schema: AdjustInput, fn: adjust, pathFields: ['input'] }, + speed: { schema: SpeedInput, fn: speed, pathFields: ['input'] }, + overlay: { schema: OverlayInput, fn: overlay, pathFields: ['input', 'overlay'] }, + zoom_pan: { schema: ZoomPanInput, fn: zoomPan, pathFields: ['input'] }, + stabilize: { schema: StabilizeInput, fn: stabilize, pathFields: ['input'] }, + chroma_key: { schema: ChromaKeyInput, fn: chromaKey, pathFields: ['foreground', 'background'] }, + silence_remove: { schema: SilenceRemoveInput, fn: silenceRemove, pathFields: ['input'] }, + highlight_reel: { schema: HighlightReelInput, fn: highlightReel, pathFields: ['input'] }, + add_captions: { schema: AddCaptionsInput, fn: addCaptions, pathFields: ['input'] }, +}; + +function wrapWithConfinement(entry: ToolEntry): ToolEntry { + const { schema, fn, pathFields } = entry; + if (!pathFields) return { schema, fn }; + // async so a synchronous WorkspaceBoundaryError surfaces as a rejected promise + // (the dispatch sites await entry.fn and turn rejections into 403 / error-data). + return { schema, fn: async (input: unknown) => fn(confineToolInput(input, pathFields)) }; +} /** * Maps tool name → { schema, fn }. The UI uses this to: * - dispatch POST /api/tools/:name to the right function with Zod-validated input * - render forms for the schemas (eventually; for now forms are hand-built) * + * Every entry's path inputs are confined to the workspace (see `confineToolInput`) + * so the agent/UI tool surface cannot read files outside the sandbox; only the + * trusted CLI, which never dispatches through here, stays unconfined. + * * Composites (add_title_card, add_captions, highlight_reel, silence_remove, * chroma_key) are registered here once the UI has a hand-built form for * their schema — including the row-list pattern for structured-input @@ -40,27 +126,9 @@ export const TOOL_REGISTRY: Record< // biome-ignore lint/suspicious/noExplicitAny: schema validates at runtime; downstream casts fn: (input: any) => Promise; } -> = { - ingest: { schema: IngestInput, fn: ingest }, - trim: { schema: TrimInput, fn: trim }, - split: { schema: SplitInput, fn: split }, - concat: { schema: ConcatInput, fn: concat }, - add_text: { schema: AddTextInput, fn: addText }, - add_audio: { schema: AddAudioInput, fn: addAudio }, - add_title_card: { schema: AddTitleCardInput, fn: addTitleCard }, - transition: { schema: TransitionInput, fn: transition }, - render: { schema: RenderInput, fn: render }, - preview: { schema: PreviewInput, fn: preview }, - adjust: { schema: AdjustInput, fn: adjust }, - speed: { schema: SpeedInput, fn: speed }, - overlay: { schema: OverlayInput, fn: overlay }, - zoom_pan: { schema: ZoomPanInput, fn: zoomPan }, - stabilize: { schema: StabilizeInput, fn: stabilize }, - chroma_key: { schema: ChromaKeyInput, fn: chromaKey }, - silence_remove: { schema: SilenceRemoveInput, fn: silenceRemove }, - highlight_reel: { schema: HighlightReelInput, fn: highlightReel }, - add_captions: { schema: AddCaptionsInput, fn: addCaptions }, -}; +> = Object.fromEntries( + Object.entries(RAW_TOOLS).map(([name, entry]) => [name, wrapWithConfinement(entry)] as const), +); export function isRegisteredTool(name: string): name is keyof typeof TOOL_REGISTRY { return name in TOOL_REGISTRY; diff --git a/src/workspace.ts b/src/workspace.ts index ba34ac2..2192c04 100644 --- a/src/workspace.ts +++ b/src/workspace.ts @@ -1,7 +1,7 @@ import { randomBytes } from 'node:crypto'; import { mkdir } from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { isAbsolute, resolve } from 'node:path'; +import { isAbsolute, relative, resolve, sep } from 'node:path'; const WORKSPACE_ENV = 'MAKEMYCLIP_WORKSPACE'; @@ -19,6 +19,49 @@ export function resolveInput(path: string): string { return isAbsolute(path) ? path : resolve(process.cwd(), path); } +/** Thrown when an untrusted surface (agent verb/tool, `clip ui` route) asks to + * read a path outside the workspace — the AGENTS.md non-negotiable #3 boundary. */ +export class WorkspaceBoundaryError extends Error { + readonly path: string; + readonly workspace: string; + constructor(path: string, workspace: string) { + // Audience-neutral: served both to the agent (as tool-result data) and to a + // person via the 403 body, so it states the constraint without advising a + // surface only one of them has (no "use the CLI" — the agent cannot). + super( + `Refusing to read "${path}": it is outside the workspace (${workspace}). ` + + `Only files inside the workspace can be read here — move the file into the ` + + `workspace (its imports/ folder) and reference it from there.`, + ); + this.name = 'WorkspaceBoundaryError'; + this.path = path; + this.workspace = workspace; + } +} + +/** + * Resolve `path` and CONFINE it to the workspace — the trust boundary for + * untrusted input (the agent's verbs/tools and the localhost `clip ui` routes). + * Relative paths resolve against the workspace; absolute paths must already sit + * inside it. Throws `WorkspaceBoundaryError` on traversal or any path outside the + * tree. The trusted CLI keeps `resolveInput`, where a user-typed path is consent. + * + * Containment is by resolved-path prefix; it does NOT follow symlinks, so a + * symlink placed inside the workspace can still point out — hardening that is a + * follow-up, but `..`-traversal and absolute out-of-tree paths are rejected here. + */ +export function resolveInWorkspace(path: string): string { + const workspace = getWorkspace(); + const resolved = isAbsolute(path) ? resolve(path) : resolve(workspace, path); + const rel = relative(workspace, resolved); + // Outside the tree if the relative path is empty (the workspace dir itself), + // escapes via '..', or is absolute (e.g. a different Windows drive). + if (rel === '' || rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) { + throw new WorkspaceBoundaryError(path, workspace); + } + return resolved; +} + export function newOutputPath(prefix: string, ext: string): string { const id = randomBytes(4).toString('hex'); return resolve(getWorkspace(), `${prefix}-${id}.${ext}`); diff --git a/tests/workspace.test.ts b/tests/workspace.test.ts new file mode 100644 index 0000000..0132bc0 --- /dev/null +++ b/tests/workspace.test.ts @@ -0,0 +1,105 @@ +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { makeVerbContext } from '../src/ui/timeline-tools.js'; +import { TOOL_REGISTRY } from '../src/ui/tool-registry.js'; +import { resolveInput, resolveInWorkspace, WorkspaceBoundaryError } from '../src/workspace.js'; + +let workspace: string; +let saved: string | undefined; + +beforeEach(async () => { + workspace = await mkdtemp(join(tmpdir(), 'mmc-ws-test-')); + saved = process.env.MAKEMYCLIP_WORKSPACE; + process.env.MAKEMYCLIP_WORKSPACE = workspace; +}); + +afterEach(async () => { + if (saved === undefined) delete process.env.MAKEMYCLIP_WORKSPACE; + else process.env.MAKEMYCLIP_WORKSPACE = saved; + await rm(workspace, { recursive: true, force: true }); +}); + +describe('resolveInWorkspace (untrusted-input confinement)', () => { + it('accepts an absolute path inside the workspace', () => { + const p = resolve(workspace, 'imports/clip.mp4'); + expect(resolveInWorkspace(p)).toBe(p); + }); + + it('resolves a relative path against the workspace', () => { + expect(resolveInWorkspace('imports/clip.mp4')).toBe(resolve(workspace, 'imports/clip.mp4')); + }); + + it('rejects parent-traversal paths', () => { + expect(() => resolveInWorkspace('../escape.mp4')).toThrow(WorkspaceBoundaryError); + expect(() => resolveInWorkspace(resolve(workspace, '../sibling/secret.mov'))).toThrow( + WorkspaceBoundaryError, + ); + }); + + it('rejects an absolute path outside the workspace', () => { + expect(() => resolveInWorkspace('/etc/passwd')).toThrow(WorkspaceBoundaryError); + }); + + it('rejects the workspace directory itself (not a file)', () => { + expect(() => resolveInWorkspace(workspace)).toThrow(WorkspaceBoundaryError); + }); + + it('does not confuse a file named "..foo" with traversal', () => { + const p = resolve(workspace, '..foo.mp4'); + expect(resolveInWorkspace(p)).toBe(p); + }); + + it('the CLI resolver stays UNCONFINED (a user-typed path is consent)', () => { + expect(resolveInput('/etc/passwd')).toBe('/etc/passwd'); + }); +}); + +describe('the agent/UI ingest surfaces are gated', () => { + it('the registry-dispatched ingest tool rejects out-of-workspace paths before probing', async () => { + await expect(TOOL_REGISTRY.ingest.fn({ path: '/etc/passwd' })).rejects.toBeInstanceOf( + WorkspaceBoundaryError, + ); + }); + + it("makeVerbContext().ingest (the add_media verb's path) rejects out-of-workspace paths", async () => { + await expect(makeVerbContext().ingest('/etc/passwd')).rejects.toBeInstanceOf( + WorkspaceBoundaryError, + ); + }); +}); + +describe('every path-bearing registry tool is confined, not just ingest', () => { + // The other registry tools (render/trim/overlay/…) are reachable from the same + // untrusted surfaces (POST /api/tools/:name, the agent's tool calls). Each reads + // its source path(s) with FFmpeg, so each must reject out-of-workspace input — + // otherwise it's an arbitrary-file read (the rendered output is served back). + + it('a single-input tool (render) rejects an out-of-workspace input before probing', async () => { + await expect(TOOL_REGISTRY.render.fn({ input: '/etc/passwd' })).rejects.toBeInstanceOf( + WorkspaceBoundaryError, + ); + }); + + it('trim rejects an out-of-workspace input', async () => { + await expect( + TOOL_REGISTRY.trim.fn({ input: '/etc/passwd', startSec: 0, endSec: 1 }), + ).rejects.toBeInstanceOf(WorkspaceBoundaryError); + }); + + it('confines EVERY path field — a valid input with an escaping second path is rejected', async () => { + const inside = resolve(workspace, 'imports/clip.mp4'); + // add_audio confines both `input` and `audio`; the escape is in the 2nd field. + await expect( + TOOL_REGISTRY.add_audio.fn({ input: inside, audio: '/etc/passwd' }), + ).rejects.toBeInstanceOf(WorkspaceBoundaryError); + }); + + it('confines array-of-path inputs (concat) element-wise', async () => { + const inside = resolve(workspace, 'imports/a.mp4'); + await expect( + TOOL_REGISTRY.concat.fn({ inputs: [inside, '/etc/passwd'] }), + ).rejects.toBeInstanceOf(WorkspaceBoundaryError); + }); +}); From 4a2ff19a225e81dfc837f9db0a97eeb6f27526ca Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Thu, 18 Jun 2026 10:40:57 -0700 Subject: [PATCH 08/11] fix(timeline): follow symlinks when confining agent/UI paths to the workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveInWorkspace checked containment on the lexically-resolved path, so an in-workspace symlink pointing out of the tree was followed at FFmpeg-open time — the boundary it exists to enforce (AGENTS.md non-negotiable #3) leaked. Canonicalize both the workspace and the resolved path before comparing: walk the existing prefix with lstat, follow symlinks explicitly (so a DANGLING in-workspace link resolves toward its real, possibly out-of-tree, target instead of looking like an in-workspace leaf), and treat only a genuine ENOENT as a not-yet-created tail — EACCES / symlink loops fail closed. A symlinked workspace root (macOS /var -> /private/var) is collapsed too, so legitimate paths aren't rejected. The lexical resolved path is still returned, so an in-workspace symlink to an in-workspace target keeps working. Residual (documented): an open-time TOCTOU race needs O_NOFOLLOW at the open site, out of scope here. --- src/workspace.ts | 71 ++++++++++++++++++++++++++++++++++++++--- tests/workspace.test.ts | 41 +++++++++++++++++++++++- 2 files changed, 106 insertions(+), 6 deletions(-) diff --git a/src/workspace.ts b/src/workspace.ts index 2192c04..ed62e46 100644 --- a/src/workspace.ts +++ b/src/workspace.ts @@ -1,7 +1,8 @@ import { randomBytes } from 'node:crypto'; +import { lstatSync, readlinkSync, realpathSync } from 'node:fs'; import { mkdir } from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { isAbsolute, relative, resolve, sep } from 'node:path'; +import { basename, dirname, isAbsolute, relative, resolve, sep } from 'node:path'; const WORKSPACE_ENV = 'MAKEMYCLIP_WORKSPACE'; @@ -39,6 +40,49 @@ export class WorkspaceBoundaryError extends Error { } } +const MAX_SYMLINK_HOPS = 40; + +/** + * Canonicalize `p` for the workspace containment check: resolve symlinks on the + * existing prefix so the comparison runs on REAL targets, while tolerating a + * not-yet-created trailing path (a tool may be about to create it). Symlinks are + * followed explicitly — including a DANGLING one, whose (possibly out-of-tree) + * target is resolved so it can't be mistaken for an in-workspace leaf. Only a + * genuine `ENOENT` is treated as a missing tail; any other error (`EACCES`, + * `ELOOP`, a symlink cycle) throws so the caller can fail closed rather than + * decide containment on a path it could not fully resolve. + */ +function canonicalizeForCheck(p: string): string { + let current = resolve(p); + const tail: string[] = []; + let hops = 0; + for (;;) { + let stats: ReturnType; + try { + stats = lstatSync(current); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + const parent = dirname(current); + if (parent === current) return resolve(p); + tail.unshift(basename(current)); + current = parent; + continue; + } + if (stats.isSymbolicLink()) { + if (++hops > MAX_SYMLINK_HOPS) { + throw new Error(`Too many symlink hops resolving "${p}".`); + } + // Follow one hop toward the target (keeping the not-yet-resolved tail) so a + // dangling or outward link is canonicalized to its real destination. + current = resolve(dirname(current), readlinkSync(current)); + continue; + } + // A real, non-symlink entry: realpath collapses any symlinks in ITS prefix. + const real = realpathSync(current); + return tail.length > 0 ? resolve(real, ...tail) : real; + } +} + /** * Resolve `path` and CONFINE it to the workspace — the trust boundary for * untrusted input (the agent's verbs/tools and the localhost `clip ui` routes). @@ -46,14 +90,31 @@ export class WorkspaceBoundaryError extends Error { * inside it. Throws `WorkspaceBoundaryError` on traversal or any path outside the * tree. The trusted CLI keeps `resolveInput`, where a user-typed path is consent. * - * Containment is by resolved-path prefix; it does NOT follow symlinks, so a - * symlink placed inside the workspace can still point out — hardening that is a - * follow-up, but `..`-traversal and absolute out-of-tree paths are rejected here. + * Containment is checked on the REAL (symlink-collapsed) paths, so an in-workspace + * symlink — existing-target OR dangling — cannot smuggle a read outside the tree: + * each side is canonicalized via `canonicalizeForCheck` before comparing (a + * symlinked workspace root, e.g. macOS `/var` -> `/private/var`, is collapsed too + * so legit paths aren't falsely rejected). If a path can't be fully canonicalized + * (permission error, symlink loop) it fails closed. The lexical resolved path is + * returned, so an in-workspace symlink to an in-workspace target still works. + * + * Residual: an open-time TOCTOU race (the path is re-followed when FFmpeg opens + * it) is not closed here — that needs `O_NOFOLLOW` at the open site. */ export function resolveInWorkspace(path: string): string { const workspace = getWorkspace(); const resolved = isAbsolute(path) ? resolve(path) : resolve(workspace, path); - const rel = relative(workspace, resolved); + let realWorkspace: string; + let realResolved: string; + try { + realWorkspace = canonicalizeForCheck(workspace); + realResolved = canonicalizeForCheck(resolved); + } catch { + // Couldn't canonicalize (EACCES / symlink loop / …) — don't decide on a + // partially-resolved path; refuse. + throw new WorkspaceBoundaryError(path, workspace); + } + const rel = relative(realWorkspace, realResolved); // Outside the tree if the relative path is empty (the workspace dir itself), // escapes via '..', or is absolute (e.g. a different Windows drive). if (rel === '' || rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) { diff --git a/tests/workspace.test.ts b/tests/workspace.test.ts index 0132bc0..f2f80be 100644 --- a/tests/workspace.test.ts +++ b/tests/workspace.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm } from 'node:fs/promises'; +import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; @@ -103,3 +103,42 @@ describe('every path-bearing registry tool is confined, not just ingest', () => ).rejects.toBeInstanceOf(WorkspaceBoundaryError); }); }); + +describe('resolveInWorkspace follows symlinks (no in-workspace symlink escape)', () => { + it('rejects a path through an in-workspace symlink that points OUT of the tree', async () => { + const outside = await mkdtemp(join(tmpdir(), 'mmc-outside-')); + try { + await writeFile(join(outside, 'secret.txt'), 'secret'); + await mkdir(join(workspace, 'imports')); + // imports/escape -> , so imports/escape/secret.txt is really outside. + await symlink(outside, join(workspace, 'imports', 'escape')); + expect(() => resolveInWorkspace('imports/escape/secret.txt')).toThrow(WorkspaceBoundaryError); + // the symlinked directory itself resolves outside too + expect(() => resolveInWorkspace('imports/escape')).toThrow(WorkspaceBoundaryError); + } finally { + await rm(outside, { recursive: true, force: true }); + } + }); + + it('allows a path through an in-workspace symlink that stays inside the tree', async () => { + await mkdir(join(workspace, 'real')); + await symlink(join(workspace, 'real'), join(workspace, 'inside-link')); + // inside-link -> real (both in-workspace), so the read stays contained. + expect(resolveInWorkspace('inside-link/clip.mp4')).toBe( + resolve(workspace, 'inside-link/clip.mp4'), + ); + }); + + it('rejects a DANGLING in-workspace symlink whose target is outside (created later)', async () => { + const outside = await mkdtemp(join(tmpdir(), 'mmc-outside-')); + try { + await mkdir(join(workspace, 'imports')); + // The symlink exists but its target does not yet — the link must still be + // followed to its out-of-tree destination, not treated as an in-ws leaf. + await symlink(join(outside, 'not-there-yet.txt'), join(workspace, 'imports', 'dangling')); + expect(() => resolveInWorkspace('imports/dangling')).toThrow(WorkspaceBoundaryError); + } finally { + await rm(outside, { recursive: true, force: true }); + } + }); +}); From e3a322676392c0f392f73add8aae110bc36dbca8 Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Thu, 18 Jun 2026 10:40:57 -0700 Subject: [PATCH 09/11] fix(timeline): widen the applyVerbs retry to the commit-attempt cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit applyVerbs re-lowers and retries on a CompositionConflictError (the optimistic expectedBaseRev guard) but capped the loop at a hardcoded 4, while the inner commit loop allows MAX_COMMIT_ATTEMPTS (8). A burst of >4 concurrent edits against the same base rev therefore dropped its tail: the 5th+ writer exhausted its retries and lost the edit. Align the cap to MAX_COMMIT_ATTEMPTS so each concurrent edit re-lowers against fresh state and lands. Latent today (the shipping UI has no concurrent caller of the verbs path), but closed before the API gains one. expectedBaseRev stays load-bearing — the guard is what forces the re-lower. Adds a deterministic regression test that races a burst through a barrier so each writer needs an increasing attempt count. --- src/timeline/document-store.ts | 8 ++++--- tests/timeline-verbs.test.ts | 43 ++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/timeline/document-store.ts b/src/timeline/document-store.ts index 19d1014..d3f71c1 100644 --- a/src/timeline/document-store.ts +++ b/src/timeline/document-store.ts @@ -253,15 +253,17 @@ export async function applyVerbs( // against, so if another in-process writer commits between the read and the // apply, re-lower against the fresh doc rather than land overlapping clips. A // retry re-runs the impure lowering (it may re-`ingest`), which is acceptable - // on the rare conflict path. - for (let attempt = 1; attempt <= 4; attempt++) { + // on the rare conflict path. Retry to the same cap as the inner commit loop so + // a burst of N concurrent edits each re-lower and land instead of the (N-cap) + // tail silently losing to a `CompositionConflictError`. + for (let attempt = 1; attempt <= MAX_COMMIT_ATTEMPTS; attempt++) { const current = await readComposition(); const ops = await lowerVerbs(current, verbs, ctx); try { const doc = await mutateComposition(ops, { expectedBaseRev: current.rev }); return { doc, ops }; } catch (err) { - if (err instanceof CompositionConflictError && attempt < 4) continue; + if (err instanceof CompositionConflictError && attempt < MAX_COMMIT_ATTEMPTS) continue; throw err; } } diff --git a/tests/timeline-verbs.test.ts b/tests/timeline-verbs.test.ts index f922bb9..526b89a 100644 --- a/tests/timeline-verbs.test.ts +++ b/tests/timeline-verbs.test.ts @@ -184,4 +184,47 @@ describe('applyVerbs (op-aware + undoable)', () => { const doc = await mutateComposition([{ op: 'setCanvas', height: 720 }], { expectedBaseRev: 1 }); expect(doc.rev).toBe(2); }); + + it('a burst of concurrent applyVerbs all land — none lost to the retry cap', async () => { + // All N writers read the same base rev (held at a barrier in `ingest` until + // every writer has lowered), then race the commit. Each conflicts on the + // stale expectedBaseRev and re-lowers, so writer k commits on attempt k — + // forcing up to N attempts. N=6 exceeds the old hardcoded cap of 4 (which + // dropped the 5th/6th with a CompositionConflictError) and stays within + // MAX_COMMIT_ATTEMPTS, so the fix must land all six. + const N = 6; + const DUR = 5; + await mutateComposition([{ op: 'addTrack', track: videoTrack({ id: 'v0' }) }]); + + let arrived = 0; + let openGate!: () => void; + const gate = new Promise((resolve) => { + openGate = resolve; + }); + const barrierCtx: VerbContext = { + defaultTrack: 'v0', + ingest: async () => { + if (arrived < N) { + arrived++; + if (arrived === N) openGate(); + } + await gate; + return { mediaId: M, durationSec: DUR }; + }, + }; + + const results = await Promise.all( + Array.from({ length: N }, () => + applyVerbs([{ verb: 'add_media', path: '/a.mp4' }], barrierCtx), + ), + ); + expect(results).toHaveLength(N); + + const doc = await readComposition(); + const starts = (doc.tracks[0]?.clips ?? []).map((c) => c.startSec).sort((a, b) => a - b); + // All six landed, abutting with no overlap or gap (proves no edit was lost). + expect(starts).toEqual([0, 1, 2, 3, 4, 5].map((i) => i * DUR)); + // One undoable entry per landed edit, plus the seeding addTrack. + expect((await readDocOpLog()).entries).toHaveLength(N + 1); + }); }); From adced000afd33d8394c88ac131aa7c36dd692842 Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Thu, 18 Jun 2026 10:40:57 -0700 Subject: [PATCH 10/11] fix(timeline): drop per-clip fades only on real transition boundaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A per-clip fadeIn/fadeOut on the same cut as a transition double-darkened (the xfade already blends that window). Suppress the fade on a transition boundary so the xfade owns the blend — but only when the fold actually emits an xfade there. A transition keyed to the LAST clip (e.g. left dangling after the next clip was removed) blends nothing, so gate fadeOut suppression on a following clip existing; otherwise a fade-to-black at the timeline end was silently dropped to a hard cut. Outer-boundary fades and fades on plain cuts are untouched. Adds regression tests for the matched-boundary case and the dangling-transition (last-clip) case. --- src/timeline/compile.ts | 50 +++++++++++++++++++++------- tests/compile.test.ts | 72 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 11 deletions(-) diff --git a/src/timeline/compile.ts b/src/timeline/compile.ts index b089611..6392b16 100644 --- a/src/timeline/compile.ts +++ b/src/timeline/compile.ts @@ -30,10 +30,11 @@ import type { MediaId } from './schema.js'; * per-clip transform, chromaKey (which needs a background layer), or timeline * gaps/overlaps — throw a clear `CompileError` rather than emitting a wrong graph. * - * Known interaction (v1): a per-clip fadeOut/fadeIn placed on the SAME boundary - * as a transition double-darkens, since the xfade already supplies the blend over - * that window. Prefer one or the other on a given cut; fixing the overlap is a - * follow-up. + * Transition boundaries own their blend: a per-clip fadeOut/fadeIn on the SAME + * cut as a transition is dropped during segment normalization (see the + * `FadeSuppression` plumbing), so the xfade's blend isn't stacked over a fade + * over the same window. Fades on outer boundaries (open from black, close to + * black) and on plain cuts are untouched. */ export interface MediaInfo { @@ -151,9 +152,22 @@ interface FilterChains { audio: string[]; } +/** Whether a clip's leading/trailing per-clip fade should be dropped because a + * transition already owns the blend over that boundary (else they double-darken). */ +interface FadeSuppression { + in: boolean; + out: boolean; +} + /** Translate a clip's effect stack into video/audio filter fragments, applied - * after the geometry/source fragments. Order: color → speed → fades. */ -function effectFilters(clip: Clip, outDur: number): FilterChains { + * after the geometry/source fragments. Order: color → speed → fades. A fade on a + * boundary that carries a transition is skipped (`suppress`) so the xfade's blend + * isn't stacked over a per-clip fade on the same window. */ +function effectFilters( + clip: Clip, + outDur: number, + suppress: FadeSuppression = { in: false, out: false }, +): FilterChains { const video: string[] = []; const audio: string[] = []; @@ -181,11 +195,13 @@ function effectFilters(clip: Clip, outDur: number): FilterChains { for (const e of clip.effects) { // Clamp the fade to the clip so a fade longer than the clip still reaches // full black/silence within the available window instead of ramping partway. - if (e.type === 'fadeIn') { + // Skip the fade entirely when a transition owns this boundary (the xfade + // already blends that window — a per-clip fade on top double-darkens it). + if (e.type === 'fadeIn' && !suppress.in) { const d = Math.min(e.durationSec, outDur); video.push(`fade=t=in:st=0:d=${d}`); audio.push(`afade=t=in:st=0:d=${d}`); - } else if (e.type === 'fadeOut') { + } else if (e.type === 'fadeOut' && !suppress.out) { const d = Math.min(e.durationSec, outDur); const st = Math.max(0, outDur - d); video.push(`fade=t=out:st=${st}:d=${d}`); @@ -231,12 +247,13 @@ function buildSegmentStep( clip: Clip, index: number, ctx: CompileContext, + fadeSuppress: FadeSuppression = { in: false, out: false }, ): FfmpegStep { assertCompilable(clip); const outDur = outputDuration(clip); const { width, height, fps, background } = comp; const out = segPath(ctx.dir, index, clip.id); - const { video: fx, audio: afx } = effectFilters(clip, outDur); + const { video: fx, audio: afx } = effectFilters(clip, outDur, fadeSuppress); const textFiles: StepSideFile[] = []; const inputArgs: string[] = []; @@ -496,9 +513,20 @@ export function compileTimeline(comp: Composition, ctx: CompileContext): FfmpegP const steps: FfmpegStep[] = []; - // 1. Normalize every clip to a segment. + // 1. Normalize every clip to a segment. Drop a per-clip fade on a boundary that + // a transition already blends: fadeOut when a transition follows this clip, + // fadeIn when one precedes it (the preceding clip has a transition after it). + // The fold (step 2) only xfades a transition that has a FOLLOWING clip, so a + // transition keyed to the last clip (e.g. left dangling after the next clip was + // removed) blends nothing — keep that clip's fadeOut rather than silently + // dropping a fade-to-black to a hard cut. const segments = clips.map((clip, i) => { - const step = buildSegmentStep(comp, clip, i, ctx); + const prevClip = clips[i - 1]; + const fadeSuppress: FadeSuppression = { + in: prevClip ? transitionAfter.has(prevClip.id) : false, + out: i < clips.length - 1 && transitionAfter.has(clip.id), + }; + const step = buildSegmentStep(comp, clip, i, ctx, fadeSuppress); steps.push(step); return { clip, output: step.output, durationSec: outputDuration(clip) }; }); diff --git a/tests/compile.test.ts b/tests/compile.test.ts index c129d39..75522f0 100644 --- a/tests/compile.test.ts +++ b/tests/compile.test.ts @@ -205,6 +205,78 @@ describe('compileTimeline — fold (cuts & transitions)', () => { // 4 + 4 − 1 overlap = 7s expect(plan.durationSec).toBe(7); }); + + it('drops a per-clip fade on a transition boundary but keeps outer fades', () => { + const comp = oneTrack( + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'a', mediaId: M1, sourceOutSec: 4, startSec: 0 }), + }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'b', mediaId: M2, sourceOutSec: 4, startSec: 4 }), + }, + { op: 'addEffect', clipId: 'a', effect: { type: 'fadeIn', durationSec: 1 } }, + { op: 'addEffect', clipId: 'a', effect: { type: 'fadeOut', durationSec: 1 } }, + { op: 'addEffect', clipId: 'b', effect: { type: 'fadeIn', durationSec: 1 } }, + { op: 'addEffect', clipId: 'b', effect: { type: 'fadeOut', durationSec: 1 } }, + { + op: 'addTransition', + trackId: 'v0', + transition: { afterClipId: 'a', kind: 'dissolve', durationSec: 1 }, + }, + ); + const plan = compileTimeline(comp, ctx()); + const segA = plan.steps.find((s) => s.label === 'segment:a')?.args ?? []; + const fcA = segA[segA.indexOf('-filter_complex') + 1] ?? ''; + const segB = plan.steps.find((s) => s.label === 'segment:b')?.args ?? []; + const fcB = segB[segB.indexOf('-filter_complex') + 1] ?? ''; + + // Clip a opens from black (leading fadeIn kept) but its trailing fadeOut is + // dropped — the dissolve already blends that cut. + expect(fcA).toContain('fade=t=in:st=0:d=1'); + expect(fcA).not.toContain('fade=t=out'); + expect(fcA).not.toContain('afade=t=out'); + // Clip b's leading fadeIn is dropped (xfade owns it); its trailing fadeOut to + // black at the timeline end is kept. + expect(fcB).not.toContain('fade=t=in'); + expect(fcB).not.toContain('afade=t=in'); + expect(fcB).toContain('fade=t=out'); + // The transition fold itself is unaffected. + expect(plan.steps.some((s) => s.label === 'fold:xfade:1')).toBe(true); + }); + + it('keeps the last clip fadeOut when a transition dangles on it (no following clip to xfade)', () => { + // A transition keyed to the LAST clip blends nothing (the fold only xfades a + // transition with a following clip — e.g. one left dangling after the next + // clip was removed). The fade-to-black must survive, not drop to a hard cut. + const comp = oneTrack( + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'a', mediaId: M1, sourceOutSec: 4, startSec: 0 }), + }, + { + op: 'addClip', + trackId: 'v0', + clip: mediaClip({ id: 'b', mediaId: M2, sourceOutSec: 4, startSec: 4 }), + }, + { op: 'addEffect', clipId: 'b', effect: { type: 'fadeOut', durationSec: 1 } }, + { + op: 'addTransition', + trackId: 'v0', + transition: { afterClipId: 'b', kind: 'dissolve', durationSec: 1 }, + }, + ); + const plan = compileTimeline(comp, ctx()); + const segB = plan.steps.find((s) => s.label === 'segment:b')?.args ?? []; + const fcB = segB[segB.indexOf('-filter_complex') + 1] ?? ''; + expect(fcB).toContain('fade=t=out:st=3:d=1'); // preserved + // No xfade exists for a transition with nothing after it; the join is a cut. + expect(plan.steps.some((s) => s.label.startsWith('fold:xfade'))).toBe(false); + }); }); describe('compileTimeline — v1 guards (explicit, not silent-wrong)', () => { From 330615c382dc826e17b6d01ca30ed1118aef266a Mon Sep 17 00:00:00 2001 From: Slava Yultyyev Date: Thu, 18 Jun 2026 10:41:10 -0700 Subject: [PATCH 11/11] docs: document the timeline workflow, refresh counts, drop brand names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Document the `clip timeline` composition workflow (new → add-media → transition → show → export, with undo/redo and read-only show/at/frame) in README, SKILL.md, and the top-level `clip --help` (it was reachable but unlisted). - Reframe the source-of-truth narrative: the Composition document is canonical for assembled edits; session.json is the append-only op log of tool calls (was "the session is the source of truth", which contradicted the code and AGENTS.md non-negotiable #2). - Refresh the stale "356 tests" claim to 539 (README badge + FAQ, llms.txt). - Remove competitor brand names from the TransitionKind JSDoc (it ships in dist/index.d.ts); the README/llms comparison prose keeps its intentional naming. - Drop the hand-maintained, drifting SKILL.md frontmatter version field. --- README.md | 34 +++++++++++++++++++++++++--------- SKILL.md | 16 +++++++++++++++- llms.txt | 2 +- src/cli.ts | 5 +++++ src/ffmpeg/args/transition.ts | 6 +++--- 5 files changed, 49 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 65f5489..e2a8b08 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ The open-source, local-first AI video editor — chat, CLI, or browser timeline. License: MIT Node 24+ npm - Tests: 356 passing + Tests: 539 passing Telemetry: none

@@ -143,14 +143,14 @@ Legend: ✅ great fit · ⚠️ works but not the best · ❌ doesn't fit.
-Architecture — three surfaces, one registry, one append-only op log +Architecture — three surfaces, one composition document, one append-only op log ``` -Claude Code → skill triggers → npx -y @makemyclip/editor +Claude Code → skill triggers → npx -y @makemyclip/editor │ -Browser UI → /api/tools/:name → TOOL_REGISTRY (19 tools) +Browser UI → /api/tools/:name · /api/timeline/verbs → registry + verb layer │ -Chat panel → /api/chat → AI SDK + Anthropic + tool dispatch +Chat panel → /api/chat → AI SDK + Anthropic + timeline verbs │ ▼ Tool handlers (TypeScript) @@ -158,11 +158,27 @@ Chat panel → /api/chat → AI SDK + Anthropic + tool dispatch ▼ FFmpeg subprocess (args as array, no shell) │ - ▼ - session.json (append-only op log) + ┌───────────────────────┴───────────────────────┐ + ▼ ▼ + Composition document session.json + (non-destructive timeline, (append-only op log + op-log undo/redo) of every tool call) +``` + +Two layers of state, by design. The **composition document** is the source of truth for assembled edits: a non-destructive, multi-track timeline that `clip timeline`, the browser UI, and the chat agent all mutate through one op-aware path, with a coupled op log that powers undo/redo. **`session.json`** is an append-only op log of every tool invocation — `{ id, tool, args, result, timestamp }` — written through the same `appendOp()` path by the single-file tools (`clip trim`, `/api/tools/:name`, …); it is the audit trail and recovery layer. Both layers are written through one serialized path, so any combination of human + agent edits stays consistent. + +**Build a composition end-to-end** (CLI shown; the browser UI and chat agent drive the same document): + +```bash +clip timeline new # start an empty composition +clip timeline add-media intro.mp4 # ingest + append a clip +clip timeline add-media demo.mp4 +clip timeline transition fade 0.5 +clip timeline show # inspect tracks, clips, timings +clip timeline export final.mp4 # compile the document to a render ``` -The session is the source of truth: every editing operation appends one entry with `{ id, tool, args, result, timestamp }`. The CLI, browser UI, and chat panel all write through the same `appendOp()` path, so any combination of human + agent edits stays consistent. +Every edit is undoable (`clip timeline undo` / `redo`, `clip timeline log`), and `clip timeline at ` / `frame ` give an agent read-only eyes on the document. Run `clip timeline --help` for the full subcommand list. - **Language:** TypeScript (Node 24+) and React 19 (browser UI) - **Timeline schema:** [Zod](https://zod.dev/) (shared with the [MakeMyClip.com](https://makemyclip.com) web app) @@ -193,7 +209,7 @@ Only the chat panel inside `clip ui` needs `ANTHROPIC_API_KEY`. The CLI, the bro ### Capabilities & limits **Is it production-ready?** -This release is feature-complete for local editing — 19 tools, 356 tests passing, browser UI shipped, chat panel shipped. The API surface is still pre-1.0 (tool schemas may change in minor ways before 1.0). Use it for real work; pin a version in CI. +This release is feature-complete for local editing — 19 tools, 539 tests passing, browser UI shipped, chat panel shipped. The API surface is still pre-1.0 (tool schemas may change in minor ways before 1.0). Use it for real work; pin a version in CI. **What's the maximum video size or duration?** No hardcoded limit. FFmpeg streams the file rather than loading it into memory, and the browser UI streams uploads to disk — multi-GB recordings work fine. This release isn't tuned for projects with hundreds of clips or runtime above 30 minutes; for those, drive the CLI directly. diff --git a/SKILL.md b/SKILL.md index 8f45cb4..d269fa7 100644 --- a/SKILL.md +++ b/SKILL.md @@ -5,7 +5,6 @@ license: MIT compatibility: Requires Node 24+. FFmpeg is auto-downloaded via ffmpeg-static on first use; override with MAKEMYCLIP_FFMPEG_PATH. metadata: author: makemyclip - version: "0.0.1" homepage: https://github.com/MakeMyClip/editor runtime: node install: npx skills add MakeMyClip/editor @@ -299,6 +298,21 @@ Two-pass `vidstab`: pass 1 (`vidstabdetect`) analyzes motion and writes a transf Requires `vidstab`-enabled ffmpeg. The bundled `ffmpeg-static` includes it; the tool fails with a clear error otherwise. +### Build a multi-clip composition — the timeline (implemented) + +The single-file tools above each produce a new output file. To assemble several clips into one edit — with non-destructive trims, transitions, and **undo/redo** — drive the **timeline**: a composition document that is the source of truth for the assembled edit. The CLI, the browser UI, and this skill all mutate the same document. + +```bash +npx -y @makemyclip/editor timeline new # start an empty composition +npx -y @makemyclip/editor timeline add-media intro.mp4 # ingest + append a clip +npx -y @makemyclip/editor timeline add-media demo.mp4 +npx -y @makemyclip/editor timeline transition fade 0.5 +npx -y @makemyclip/editor timeline show # read tracks, clips, timings +npx -y @makemyclip/editor timeline export final.mp4 # compile the document to a render +``` + +Prefer the timeline over chaining single-file tools whenever the user is *assembling* a video (multiple clips, transitions, a final render): edits are non-destructive and reversible (`timeline undo` / `redo`, `timeline log`), and `timeline at ` / `timeline frame ` give you read-only eyes on the current document before you change it. Run `npx -y @makemyclip/editor timeline --help` for the full subcommand list. + ### Open the local UI (implemented) ```bash diff --git a/llms.txt b/llms.txt index 93a9864..0b3fe58 100644 --- a/llms.txt +++ b/llms.txt @@ -9,7 +9,7 @@ - **Backend:** FFmpeg (bundled via `ffmpeg-static`) - **Surfaces:** Claude Code skill · `clip` CLI · browser UI at `127.0.0.1:5573` - **AI integration:** Anthropic via Vercel AI SDK (optional; rest of editor works without API key) -- **Tests:** 356 passing +- **Tests:** 539 passing - **License:** MIT (code) + GPL (bundled FFmpeg binary, subprocess-isolated) ## How to install diff --git a/src/cli.ts b/src/cli.ts index 1934f36..03c98a4 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -60,6 +60,11 @@ Specialty: clip stabilize [] [] [] [] Two-pass vidstab. Defaults: shakiness=5, smoothing=10, accuracy=9, zoom=5. +Timeline (non-destructive composition — the source of truth for assembled edits): + clip timeline Build and export a multi-clip composition with + undo/redo. new → add-media → … → export. + clip timeline --help List the timeline subcommands. + UI: clip ui Start the local browser UI on http://127.0.0.1:5573. Renders the session log; click an op to play its output. diff --git a/src/ffmpeg/args/transition.ts b/src/ffmpeg/args/transition.ts index 4231d46..dc4b737 100644 --- a/src/ffmpeg/args/transition.ts +++ b/src/ffmpeg/args/transition.ts @@ -1,7 +1,7 @@ /** - * Subset of ffmpeg's `xfade` transition kinds. Chosen to match the common - * iMovie / CapCut palette without overwhelming the agent with 40+ obscure - * options. Each one maps 1:1 to an `xfade` `transition=` value. + * Subset of ffmpeg's `xfade` transition kinds. Chosen to cover the common + * consumer-editor transition palette without overwhelming the agent with 40+ + * obscure options. Each one maps 1:1 to an `xfade` `transition=` value. */ export type TransitionKind = | 'fade'