From 7c73b3caeebc2fd8db2f161f25d4f476ac2f78e2 Mon Sep 17 00:00:00 2001 From: Xuanru Li <157947275+xuanruli@users.noreply.github.com> Date: Tue, 13 Oct 2026 04:22:41 +0000 Subject: [PATCH] feat(cli): guard history turns with --exclusive, --who and --if-open (#5518) * feat(cli): guard history turns with --exclusive, --who and --if-open * fix(cli): make exclusive history turns atomic Re-read the open turn only after acquiring history ownership so concurrent exclusive begins cannot both claim the same project. Cover the race with a deterministic two-process regression test. * fix(cli): refuse a second exclusive turn while a preview is open The preview's begin window now rejects another agent's live turn, and an idle turn no longer blocks exclusive begin. The two-process test waits on pids collected from claim drafts instead of seeing both files at once. --- docs/packages/cli.mdx | 10 +- packages/cli/src/commands/history.test.ts | 125 ++++++++++++++++++ packages/cli/src/commands/history.ts | 104 ++++++++++++--- packages/cli/src/utils/historyOwner.ts | 96 ++++++++------ .../studio-server/src/history/ownerLock.ts | 2 +- .../src/history/projectHistory.ts | 30 ++++- packages/studio-server/src/routes/history.ts | 5 +- 7 files changed, 303 insertions(+), 69 deletions(-) diff --git a/docs/packages/cli.mdx b/docs/packages/cli.mdx index 32973d5f49..6b4485257a 100644 --- a/docs/packages/cli.mdx +++ b/docs/packages/cli.mdx @@ -1045,12 +1045,18 @@ npx hyperframes history undo --who claude # undo your newest turn | `restore [--who]` | Make the files what they were right after an entry, a time, or `start` | | `peek [path]` | List the files at an entry, a time, or `start` without changing anything; with a path, print that file | | `pin [--off]` | Keep an entry, and what it needs, when old history is trimmed. `--off` unpins it. | -| `begin --who --label` | Start a turn: your writes until `end` become one entry under your name | -| `end` | End your turn and print its entry | +| `begin --who --label` | Start a turn: your writes until `end` become one entry under your name. Another agent's open turn is ended first, or with `--exclusive` the command refuses instead. | +| `end` | End your turn and print its entry. `--who ` refuses unless the open turn is that name's; `--if-open` succeeds when no turn is open. | Every command also takes `--dir` and `--json`. A ref is an entry id or a unique prefix of one; a time is a local date or date-time such as `2026-09-24T14:30`. +Without a running preview, `history` keeps its data under `~/.cache/hyperframes/history`, or under +`HYPERFRAMES_HISTORY_ROOT` when that is set, which keeps an agent run or a test apart from your own +history. That path is resolved to an absolute one, and every agent on a project has to share it: the +lock lives there, while the turn marker lives in the project. A running preview server records to +its own location. + When a file in the entry has changed since, `undo` writes nothing, exits 2 and lists the newer entries. Run it again with `--just-this` to put back only that entry's files over the newer edits, or `--back-to-before` to go back to before diff --git a/packages/cli/src/commands/history.test.ts b/packages/cli/src/commands/history.test.ts index 4b448f94a4..e0994fc2a8 100644 --- a/packages/cli/src/commands/history.test.ts +++ b/packages/cli/src/commands/history.test.ts @@ -504,6 +504,131 @@ describe("hyperframes history, refusals", () => { expect(read("added.html")).toBe("new"); }); + it.each(["direct", "preview"])( + "begin --exclusive starts once another agent's turn has been idle (%s)", + async (mode) => { + const { dir, hf } = project(); + await hf(); + if (mode === "preview") await preview(dir); + historyDeps.turnIdleMs = 300; + await hf("begin", "--who", "codex", "--label", "Retitle"); + advance(1_000); + + const begun = await hf("begin", "--who", "claude", "--label", "Notes", "--exclusive"); + expect([begun.code, begun.err]).toEqual([0, ""]); + expect((await hf("end", "--who", "claude")).code).toBe(0); + }, + ); + + it("begin --exclusive through a preview refuses another agent's open turn", async () => { + const { dir, hf } = project(); + await hf(); + await preview(dir); + await hf("begin", "--who", "codex", "--label", "Retitle"); + const refused = await hf("begin", "--who", "claude", "--label", "Notes", "--exclusive"); + expect([refused.code, refused.err]).toEqual([ + 2, + "codex's turn is open; it has to end before yours begins", + ]); + }); + + it("begin --exclusive through a preview lets only one of two agents win", async () => { + const { dir, hf } = project(); + await hf(); + await preview(dir); + const begin = (who: string) => + withOwner(dir, async (owner) => { + await owner.begin({ kind: "agent", name: who }, who, true); + }).then( + () => "won", + (error: Error) => error.message, + ); + const results = await Promise.all([begin("codex"), begin("claude")]); + expect(results.filter((result) => result === "won")).toHaveLength(1); + expect(results.find((result) => result !== "won")).toMatch( + /^(codex|claude)'s turn is open; it has to end before yours begins$/, + ); + }); + + it("begin --exclusive refuses another agent's open turn and keeps it open", async () => { + const { write, hf, json } = project(); + await hf(); + await hf("begin", "--who", "codex", "--label", "Retitle"); + write("index.html", "A2"); + + const refused = await hf("begin", "--who", "claude", "--label", "Notes", "--exclusive"); + expect([refused.code, refused.err]).toEqual([ + 2, + "codex's turn is open; it has to end before yours begins", + ]); + expect((await hf("end", "--who", "codex")).code).toBe(0); + const entries = (await json()).entries as Array<{ who: { name: string }; files: string[] }>; + expect(entries.filter((entry) => entry.who.name === "codex")).toMatchObject([ + { files: ["index.html"] }, + ]); + }); + + it("serializes concurrent begin --exclusive claims under the history owner lock", async () => { + const { dir } = project(); + const historyRoot = tempDir("hf-history-cli-root-"); + const held = await openProjectHistory({ projectDir: dir, historyRoot }); + const home = join(historyRoot, held.projectId); + const claimers = new Set(); + const bothWaiting = new Promise((resolveWaiting) => { + const inspect = (_event: string, name: string | null) => { + const pid = name?.match(/^owner\.pid-(\d+)-/)?.[1]; + if (pid) claimers.add(pid); + if (claimers.size < 2) return; + watcher.close(); + resolveWaiting(); + }; + const watcher = watch(home, inspect); + onTestFinished(() => watcher.close()); + }); + const cli = resolve(fileURLToPath(import.meta.url), "..", "..", "cli.ts"); + const begin = (who: string) => { + const child = spawn( + "bun", + ["run", cli, "history", "begin", "--who", who, "--label", who, "--exclusive", "--dir", dir], + { + env: { + ...process.env, + HYPERFRAMES_HISTORY_ROOT: historyRoot, + HYPERFRAMES_SKIP_UPDATE_CHECK: "1", + HYPERFRAMES_NO_TELEMETRY: "1", + }, + }, + ); + onTestFinished(() => void child.kill()); + return new Promise<{ code: number | null; error: string }>((done) => { + let error = ""; + child.stderr.on("data", (chunk) => (error += chunk)); + child.on("exit", (code) => done({ code, error: error.trim() })); + }); + }; + const results = [begin("codex"), begin("claude")]; + await bothWaiting; + await held.close(); + + const settled = await Promise.all(results); + expect(settled.map((result) => result.code).sort()).toEqual([0, 2]); + expect(settled.find((result) => result.code === 2)?.error).toMatch( + /'s turn is open; it has to end before yours begins$/, + ); + }); + + it("end --who refuses another agent's turn, and --if-open succeeds with none open", async () => { + const { hf, json } = project(); + await hf(); + expect(await json("end", "--if-open")).toMatchObject({ entry: null, parts: [] }); + expect((await hf("end")).code).toBe(2); + + await hf("begin", "--who", "codex", "--label", "Retitle"); + const refused = await hf("end", "--who", "claude", "--if-open"); + expect([refused.code, refused.err]).toEqual([2, "The open turn is codex's, not claude's"]); + expect((await hf("end", "--who", "codex")).code).toBe(0); + }); + it("--json refusals go to stdout as {ok: false, error}", async () => { const { hf } = project(); await hf(); diff --git a/packages/cli/src/commands/history.ts b/packages/cli/src/commands/history.ts index b4ab0352a9..6b91dfbd19 100644 --- a/packages/cli/src/commands/history.ts +++ b/packages/cli/src/commands/history.ts @@ -77,6 +77,39 @@ function lastIndex(items: readonly T[], match: (item: T) => boolean): number } /** An entry id or a unique prefix of one. */ +async function latestWrite(owner: Owner, turn: Turn): Promise { + const entries = await owner.list(); + return entries.reduce((latest, entry) => { + const same = + entry.id === turn.id || + turn.parts.includes(entry.id) || + (entry.who.kind === turn.who.kind && + entry.who.name === turn.who.name && + entry.label === turn.label && + entry.startedAt === turn.startedAt); + return same ? Math.max(latest, entry.endedAt) : latest; + }, turn.lastWriteAt); +} + +async function releaseOpenTurn( + owner: Owner, + turn: Turn | null, + who: HistoryWho, + exclusive: boolean, + projectDir: string, +): Promise { + if (!turn) return; + if (owner.via !== "direct") { + if (!exclusive || turn.who.name === who.name) await endTurn(owner, turn, projectDir); + return; + } + const latest = await latestWrite(owner, turn); + const live = historyDeps.now() - latest <= historyDeps.turnIdleMs; + if (exclusive && live && turn.who.name !== who.name) + throw new Refusal(`${turn.who.name}'s turn is open; it has to end before yours begins`); + await endTurn(owner, turn, projectDir); +} + function entryOf(entries: readonly HistoryListItem[], ref: string): HistoryListItem { const found = entries.filter((entry) => entry.id.startsWith(ref)); if (found.length === 1) return found[0]!; @@ -123,6 +156,16 @@ function ago(at: number): string { return `${Math.round(seconds / 86_400)}d ago`; } +function endedTurnText(entry: HistoryEntry | null, parts: readonly string[]): string { + const earlier = parts.length - (entry ? 1 : 0); + const text = entry + ? line(entry) + : earlier + ? "Nothing changed since this turn's last command." + : "Nothing changed in this turn."; + return earlier ? `${text}\n(+${earlier} earlier part${earlier > 1 ? "s" : ""})` : text; +} + function line(entry: HistoryListItem | HistoryEntry): string { const paths = entry.files.map((file) => file.path); const files = paths.slice(0, 2).join(", ") + (paths.length > 2 ? ` +${paths.length - 2}` : ""); @@ -443,15 +486,27 @@ export default defineCommand({ }), ), begin: () => - sub<{ who: string; label: string; dir?: string; json: boolean }>( + sub<{ who: string; label: string; exclusive: boolean; dir?: string; json: boolean }>( "begin", "Start a turn: your writes until `end` become one entry under your name", - { who: { type: "string", required: true }, label: { type: "string", required: true } }, + { + who: { type: "string", required: true }, + label: { type: "string", required: true }, + exclusive: { + type: "boolean", + default: false, + description: "Refuse while another agent's turn is open, instead of ending it", + }, + }, (args) => withOwner("begin", args.dir, async (owner, turn, projectDir) => { - if (turn) await endTurn(owner, turn, projectDir); const who = whoOf(args.who); - const { id, startedAt } = await owner.begin(who, args.label); + await releaseOpenTurn(owner, turn, who, args.exclusive, projectDir); + const { id, startedAt } = await owner.begin( + who, + args.label, + args.exclusive && owner.via === "preview", + ); writeTurn(projectDir, { via: owner.via, id, @@ -465,22 +520,31 @@ export default defineCommand({ }), ), end: () => - sub<{ dir?: string; json: boolean }>("end", "End your turn and print its entry", {}, (args) => - withOwner("end", args.dir, async (owner, turn, projectDir) => { - if (!turn) throw new Refusal("No turn is open; start one with history begin"); - const { entry, parts } = await endTurn(owner, turn, projectDir); - const earlier = parts.length - (entry ? 1 : 0); - const text = entry - ? line(entry) - : earlier - ? "Nothing changed since this turn's last command." - : "Nothing changed in this turn."; - print( - args.json, - { entry: entry && publicEntry(entry), parts }, - earlier ? `${text}\n(+${earlier} earlier part${earlier > 1 ? "s" : ""})` : text, - ); - }), + sub<{ who?: string; ifOpen: boolean; dir?: string; json: boolean }>( + "end", + "End your turn and print its entry", + { + who: { type: "string", description: "Refuse unless the open turn is this agent's" }, + "if-open": { + type: "boolean", + default: false, + description: "Succeed with no entry when no turn is open", + }, + }, + (args) => + withOwner("end", args.dir, async (owner, turn, projectDir) => { + if (!turn && args.ifOpen) + return print(args.json, { entry: null, parts: [] }, "No turn is open."); + if (!turn) throw new Refusal("No turn is open; start one with history begin"); + if (args.who !== undefined && turn.who.name !== args.who) + throw new Refusal(`The open turn is ${turn.who.name}'s, not ${args.who}'s`); + const { entry, parts } = await endTurn(owner, turn, projectDir); + print( + args.json, + { entry: entry && publicEntry(entry), parts }, + endedTurnText(entry, parts), + ); + }), ), }, run: ({ args }) => guarded(listEntries)(args), diff --git a/packages/cli/src/utils/historyOwner.ts b/packages/cli/src/utils/historyOwner.ts index de2eb1039a..f012b6d6e9 100644 --- a/packages/cli/src/utils/historyOwner.ts +++ b/packages/cli/src/utils/historyOwner.ts @@ -1,6 +1,6 @@ import { randomUUID } from "node:crypto"; import { mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; -import { dirname, join } from "node:path"; +import { dirname, join, resolve } from "node:path"; import { DEFAULT_HISTORY_ROOT, HistoryBusyError, @@ -40,7 +40,11 @@ export interface Owner { undo(id: string, who: HistoryWho, mode?: UndoMode): Promise; restore(point: string, who: HistoryWho): Promise; pin(id: string, pinned: boolean): Promise; - begin(who: HistoryWho, label: string): Promise<{ id: string; startedAt: number }>; + begin( + who: HistoryWho, + label: string, + exclusive?: boolean, + ): Promise<{ id: string; startedAt: number }>; end(id: string): Promise; close(): Promise; } @@ -49,7 +53,7 @@ export type Reader = Pick; /** Swapped by tests. */ export const historyDeps = { - historyRoot: DEFAULT_HISTORY_ROOT, + historyRoot: resolve(process.env.HYPERFRAMES_HISTORY_ROOT || DEFAULT_HISTORY_ROOT), findServer: (projectDir: string) => findPreviewServerForProject(projectDir), /** A turn with no write for this long has ended, through a preview or not. */ turnIdleMs: MAX_WINDOW_IDLE_MS, @@ -144,8 +148,13 @@ function previewOwner(route: (path: string) => string): Owner { undo: (id, who, mode) => json("/undo", { entryId: id, who, mode }), restore: (point, who) => json("/restore", { point, who }), pin: async (id, pinned) => void (await json("/pin", { entryId: id, pinned })), - begin: async (who, label) => { - const opened = await json("/window", { who, label, idleMs: historyDeps.turnIdleMs }); + begin: async (who, label, exclusive) => { + const opened = await json("/window", { + who, + label, + idleMs: historyDeps.turnIdleMs, + ...(exclusive && { exclusive: true }), + }); return { id: opened.windowId, startedAt: opened.startedAt ?? historyDeps.now() }; }, // A preview restarted since begin committed the window on its way down. @@ -157,42 +166,48 @@ function previewOwner(route: (path: string) => string): Owner { }; } -async function directOwner(projectDir: string, turn: Turn | null): Promise { +async function directOwner(projectDir: string): Promise<{ owner: Owner; turn: Turn | null }> { + let turn: Turn | null = null; const history = await openProjectHistory({ projectDir, historyRoot: historyDeps.historyRoot, now: () => historyDeps.now(), pruneGoneProjectsBudgetMs: 1000, ownerWaitMs: historyDeps.ownerWaitMs, - // A turn begun through a preview that has since stopped is still the agent's, until its idle limit. - ...(turn && { - closedWindow: { - id: turn.id, - who: turn.who, - label: turn.label, - startedAt: turn.startedAt, - lastWriteAt: turn.lastWriteAt, - idleMs: historyDeps.turnIdleMs, - }, - }), + closedWindowAfterOwnership: () => { + turn = readTurn(projectDir); + return turn + ? { + id: turn.id, + who: turn.who, + label: turn.label, + startedAt: turn.startedAt, + lastWriteAt: turn.lastWriteAt, + idleMs: historyDeps.turnIdleMs, + } + : undefined; + }, }); return { - via: "direct", - list: async () => history.list(), - peek: async (point) => history.peek(point), - blob: (hash) => history.readBlob(hash), - undo: (id, who, mode) => history.undo(id, { who, ...(mode && { mode }) }), - restore: (point, who) => history.restore(point, who), - pin: async (id, pinned) => history.pin(id, pinned), - // Opening filed every earlier write; the turn's own writes are filed to it when the next open passes it. - begin: async () => ({ id: randomUUID(), startedAt: historyDeps.now() }), - end: async (id) => history.list().find((entry) => entry.id === id) ?? null, - close: () => history.close(), + turn, + owner: { + via: "direct", + list: async () => history.list(), + peek: async (point) => history.peek(point), + blob: (hash) => history.readBlob(hash), + undo: (id, who, mode) => history.undo(id, { who, ...(mode && { mode }) }), + restore: (point, who) => history.restore(point, who), + pin: async (id, pinned) => history.pin(id, pinned), + // Opening filed every earlier write; the turn's own writes are filed to it when the next open passes it. + begin: async () => ({ id: randomUUID(), startedAt: historyDeps.now() }), + end: async (id) => history.list().find((entry) => entry.id === id) ?? null, + close: () => history.close(), + }, }; } /** One owner: a preview that keeps this project's history, else the engine, which refuses a second opener. */ -async function connect(projectDir: string, turn: Turn | null): Promise { +async function connect(projectDir: string): Promise<{ owner: Owner; turn: Turn | null }> { const server = await historyDeps.findServer(projectDir); if (server) { const route = (path: string) => studioApiUrl(server, `history${path}`); @@ -201,9 +216,9 @@ async function connect(projectDir: string, turn: Turn | null): Promise { (response) => response.status, () => 404, ); - if (status !== 404) return previewOwner(route); + if (status !== 404) return { owner: previewOwner(route), turn: readTurn(projectDir) }; } - return directOwner(projectDir, turn); + return directOwner(projectDir); } /** Runs `task` with the project's history. ponytail: with no preview, a command mid-turn splits the turn in two. */ @@ -212,24 +227,23 @@ export async function withOwner( task: (owner: Owner, turn: Turn | null, projectDir: string) => Promise, ): Promise { const { dir: projectDir } = resolveProject(dir); - const turn = readTurn(projectDir); - const owner = await connect(projectDir, turn); + const { owner, turn } = await connect(projectDir); try { return await task(owner, turn, projectDir); } finally { // This open filed the turn so far under its id; the turn goes on under a fresh one, from its last write. - const direct = owner.via === "direct" && turn; - let kept: HistoryListItem | undefined; try { - kept = direct ? (await owner.list()).find((entry) => entry.id === turn.id) : undefined; + if (owner.via === "direct" && turn) { + const kept = (await owner.list()).find((entry) => entry.id === turn.id); + if (readTurn(projectDir)?.id === turn.id) { + const lastWriteAt = kept ? kept.endedAt : turn.lastWriteAt; + const parts = kept ? [...turn.parts, kept.id] : turn.parts; + writeTurn(projectDir, { ...turn, via: "direct", id: randomUUID(), lastWriteAt, parts }); + } + } } finally { await owner.close(); } - if (direct && readTurn(projectDir)?.id === turn.id) { - const lastWriteAt = kept ? kept.endedAt : turn.lastWriteAt; - const parts = kept ? [...turn.parts, kept.id] : turn.parts; - writeTurn(projectDir, { ...turn, via: "direct", id: randomUUID(), lastWriteAt, parts }); - } } } diff --git a/packages/studio-server/src/history/ownerLock.ts b/packages/studio-server/src/history/ownerLock.ts index eef000275d..4cf97986bc 100644 --- a/packages/studio-server/src/history/ownerLock.ts +++ b/packages/studio-server/src/history/ownerLock.ts @@ -32,7 +32,7 @@ function ownerOf(file: string): number | null { /** Takes `file` if nobody holds it: written aside and linked in, so a reader never sees it without its pid. */ function claim(file: string): boolean { - const draft = `${file}-${randomUUID()}.tmp`; + const draft = `${file}-${process.pid}-${randomUUID()}.tmp`; writeFileSync(draft, String(process.pid)); try { linkSync(draft, file); diff --git a/packages/studio-server/src/history/projectHistory.ts b/packages/studio-server/src/history/projectHistory.ts index 74b94c13fd..d1c8c6fbe6 100644 --- a/packages/studio-server/src/history/projectHistory.ts +++ b/packages/studio-server/src/history/projectHistory.ts @@ -82,6 +82,7 @@ export interface ProjectHistoryOptions { ownerWaitMs?: number; /** A CLI turn's window from an earlier open: writes since, within its idle limit, become the entry with its id. */ closedWindow?: ClosedWindow; + closedWindowAfterOwnership?: () => ClosedWindow | undefined; undoScope?: "own" | "everyone"; pruneGoneProjectsBudgetMs?: number; } @@ -135,7 +136,7 @@ export interface ProjectHistory { beginWindow( who: HistoryWho, label: string, - options?: { idleMs?: number }, + options?: { idleMs?: number; exclusive?: boolean }, ): Promise; /** * For a writer that records after writing (Studio): moves the uncommitted changes to `paths` into one entry of @@ -945,10 +946,25 @@ class Engine { }); } - beginWindow(who: HistoryWho, label: string, idleMs: number): Promise { + beginWindow( + who: HistoryWho, + label: string, + idleMs: number, + exclusive = false, + ): Promise { return this.queue(async () => { // Writes before the window opened are not this writer's, and are logged before it. await this.settle(); + const at = this.now(); + if (exclusive) { + const blocking = this.windows.find( + (open) => !sameWho(open.who, who) && takesWrite(open, at), + ); + if (blocking) + throw new Error(`${blocking.who.name}'s turn is open; it has to end before yours begins`); + for (const open of [...this.windows]) + if (!takesWrite(open, at) || sameWho(open.who, who)) await this.endWindow(open); + } const window = { ...this.newGroup(who, label), idleMs }; this.windows.push(window); this.touch(window, this.now()); @@ -1176,7 +1192,12 @@ class Engine { return { projectId: this.projectId, beginWindow: (who, label, options = {}) => - this.beginWindow(who, label, options.idleMs ?? this.options.maxGroupMs ?? 30_000), + this.beginWindow( + who, + label, + options.idleMs ?? this.options.maxGroupMs ?? 30_000, + options.exclusive === true, + ), claim: async (who, label, paths, options = {}) => { await Promise.all(paths.map((path) => this.adopting.get(this.logPath(path)))); return this.queue(() => this.claimNow(who, label, paths, options)); @@ -1288,7 +1309,8 @@ export async function openProjectHistory(options: ProjectHistoryOptions): Promis ); try { const blobs = await openBlobStore(join(options.historyRoot, projectId, "blobs")); - const engine = new Engine(options, projectId, blobs); + const closedWindow = options.closedWindowAfterOwnership?.() ?? options.closedWindow; + const engine = new Engine({ ...options, closedWindow }, projectId, blobs); await engine.queue(async () => { await engine.start(); engine.hearWrites(); diff --git a/packages/studio-server/src/routes/history.ts b/packages/studio-server/src/routes/history.ts index 62478607e0..e0bde16b06 100644 --- a/packages/studio-server/src/routes/history.ts +++ b/packages/studio-server/src/routes/history.ts @@ -141,7 +141,10 @@ export function registerHistoryRoutes(api: Hono, adapter: StudioApiAdapter): voi const window = await history.beginWindow( whoOf(body), text(body.label) ?? "Edited in Studio", - idleMs ? { idleMs } : undefined, + { + ...(idleMs ? { idleMs } : {}), + ...(body.exclusive === true && { exclusive: true }), + }, ); windows.set(window.id, { history, window }); // The window's id is its last entry's id, unless nothing was left after its last part.