From f2eae920c61f0434b37b331cc597a4faa0f90830 Mon Sep 17 00:00:00 2001 From: David Brailovsky Date: Tue, 1 Sep 2026 14:58:49 +0200 Subject: [PATCH] fix(agent): validate fork entries against named branch ancestry Reject off-branch and unknown fork points, handle null tips and before-root placement, and make SQLite validate from the named branch tip. --- packages/agent/src/harness/session/fork.ts | 23 +-- .../testing/conformance/session-repo.ts | 131 +++++++++++------- packages/agent/src/harness/session/types.ts | 2 +- .../sqlite-node/src/sqlite/repo.ts | 5 +- .../sqlite-node/src/sqlite/storage.ts | 5 +- 5 files changed, 100 insertions(+), 66 deletions(-) diff --git a/packages/agent/src/harness/session/fork.ts b/packages/agent/src/harness/session/fork.ts index 76d0bb897..3581a2bfd 100644 --- a/packages/agent/src/harness/session/fork.ts +++ b/packages/agent/src/harness/session/fork.ts @@ -103,21 +103,26 @@ function selectForkContents( } else { const sourceTip = sourceTips.find((stored) => stored.address.key === options.branch); if (sourceTip === undefined) throw new Error(`Unknown source branch: ${options.branch}`); - const requested = options.entryId ?? sourceTip.value; - let tipId = requested; - if (requested !== null) { - const target = sourceEntries.get(requested); - if (target === undefined) throw new Error(`Unknown fork entry: ${requested}`); - if (options.position === "before") tipId = target.parentId; - } - let entryId = tipId; + const requested = options.entryId ?? sourceTip.value; + let found = requested === null; + let tipId: string | null = null; + let entryId = sourceTip.value; while (entryId !== null) { const entry = sourceEntries.get(entryId); if (entry === undefined) throw new Error(`Corrupt source branch: missing parent ${entryId}`); - entryIds.add(entryId); + if (entry.id === requested) { + found = true; + tipId = options.position === "before" ? entry.parentId : entry.id; + if (options.position !== "before") entryIds.add(entry.id); + } else if (found) { + entryIds.add(entry.id); + } entryId = entry.parentId; } + if (!found) { + throw new Error(`Fork entry ${requested} is not on source branch ${JSON.stringify(options.branch)}`); + } destinationTips.set(options.branch, tipId); } return { entryIds, destinationTips }; diff --git a/packages/agent/src/harness/session/testing/conformance/session-repo.ts b/packages/agent/src/harness/session/testing/conformance/session-repo.ts index 8e396963f..00b637a3f 100644 --- a/packages/agent/src/harness/session/testing/conformance/session-repo.ts +++ b/packages/agent/src/harness/session/testing/conformance/session-repo.ts @@ -36,6 +36,7 @@ const SIBLING_ID = "00000000-0000-7000-8000-000000000003"; const USAGE_ID = "00000000-0000-7000-8000-000000000004"; const OPERATION_ID = "00000000-0000-7000-8000-000000000005"; const PENDING_ID = "00000000-0000-7000-8000-000000000006"; +const UNKNOWN_ID = "00000000-0000-7000-8000-000000000007"; const idleLaneState = { currentOperationId: null, lastOperationId: null, @@ -476,59 +477,87 @@ export function createSessionRepoForkBehaviorConformance { - const source = await repo.create({ id: "source" }, BACKGROUND_CONTEXT); - await source.mutate( - (mutator) => - mutator.commit( - [ - insertEntry({ - id: ROOT_ID, - parentId: null, - type: "custom", - customType: "root", - }), - insertEntry({ - id: CHILD_ID, - parentId: ROOT_ID, - type: "custom", - customType: "child", - }), - setValue(branchTip("main"), CHILD_ID), - ], - BACKGROUND_CONTEXT, - ), - BACKGROUND_CONTEXT, - ); - - const before = await repo.fork( - source.metadata, - { id: "before", scope: "branch", branch: "main", entryId: CHILD_ID, position: "before" }, - BACKGROUND_CONTEXT, - ); - strictEqual(await getBranchTip(before), ROOT_ID); - deepStrictEqual( - (await before.findEntries({ order: "asc" }, BACKGROUND_CONTEXT)).map(({ id }) => id), - [ROOT_ID], - ); - await rejects( - repo.fork( - source.metadata, - { id: "failed", scope: "branch", branch: "main", entryId: SIBLING_ID }, + createCase(factory, "forks", "enforces branch ancestry for at and before placement", async ({ repo }) => { + const source = await repo.create({ id: "source" }, BACKGROUND_CONTEXT); + await source.mutate( + (mutator) => + mutator.commit( + [ + insertEntry({ id: ROOT_ID, parentId: null, type: "custom", customType: "root" }), + insertEntry({ id: CHILD_ID, parentId: ROOT_ID, type: "custom", customType: "child" }), + insertEntry({ id: SIBLING_ID, parentId: ROOT_ID, type: "custom", customType: "sibling" }), + setValue(branchTip("main"), CHILD_ID), + setValue(laneConfig("main"), configuration), + setValue(laneState("main"), idleLaneState), + setValue(branchTip("empty"), null), + setValue(laneConfig("empty"), configuration), + setValue(laneState("empty"), idleLaneState), + ], BACKGROUND_CONTEXT, ), - ); - deepStrictEqual((await repo.list(undefined, BACKGROUND_CONTEXT)).map(({ id }) => id).sort(), [ - "before", - "source", - ]); - await Promise.all([source.close(BACKGROUND_CONTEXT), before.close(BACKGROUND_CONTEXT)]); - }, - ), + BACKGROUND_CONTEXT, + ); + + const before = await repo.fork( + source.metadata, + { id: "before", scope: "branch", branch: "main", entryId: CHILD_ID, position: "before" }, + BACKGROUND_CONTEXT, + ); + strictEqual(await getBranchTip(before), ROOT_ID); + deepStrictEqual( + (await before.findEntries({ order: "asc" }, BACKGROUND_CONTEXT)).map(({ id }) => id), + [ROOT_ID], + ); + + const mid = await repo.fork( + source.metadata, + { id: "mid", scope: "branch", branch: "main", entryId: ROOT_ID, position: "at" }, + BACKGROUND_CONTEXT, + ); + strictEqual(await getBranchTip(mid), ROOT_ID); + deepStrictEqual( + (await mid.findEntries({ order: "asc" }, BACKGROUND_CONTEXT)).map(({ id }) => id), + [ROOT_ID], + ); + + const beforeRoot = await repo.fork( + source.metadata, + { id: "before-root", scope: "branch", branch: "main", entryId: ROOT_ID, position: "before" }, + BACKGROUND_CONTEXT, + ); + strictEqual(await getBranchTip(beforeRoot), null); + deepStrictEqual(await beforeRoot.findEntries({ order: "asc" }, BACKGROUND_CONTEXT), []); + + const empty = await repo.fork( + source.metadata, + { id: "empty", scope: "branch", branch: "empty" }, + BACKGROUND_CONTEXT, + ); + strictEqual(await getBranchTip(empty, "empty"), null); + deepStrictEqual(await empty.findEntries({ order: "asc" }, BACKGROUND_CONTEXT), []); + + for (const [id, branch, entryId] of [ + ["off-branch", "main", SIBLING_ID], + ["unknown", "main", UNKNOWN_ID], + ["null-tip", "empty", ROOT_ID], + ] as const) { + await rejects(repo.fork(source.metadata, { id, scope: "branch", branch, entryId }, BACKGROUND_CONTEXT)); + } + deepStrictEqual((await repo.list(undefined, BACKGROUND_CONTEXT)).map(({ id }) => id).sort(), [ + "before", + "before-root", + "empty", + "mid", + "source", + ]); + await Promise.all([ + source.close(BACKGROUND_CONTEXT), + before.close(BACKGROUND_CONTEXT), + beforeRoot.close(BACKGROUND_CONTEXT), + empty.close(BACKGROUND_CONTEXT), + mid.close(BACKGROUND_CONTEXT), + ]); + }), createCase(factory, "forks", "forks a closed source session", async ({ repo }) => { const source = await repo.create({ id: "source" }, BACKGROUND_CONTEXT); await source.mutate( diff --git a/packages/agent/src/harness/session/types.ts b/packages/agent/src/harness/session/types.ts index 2c8fee6ec..bb54b17cd 100644 --- a/packages/agent/src/harness/session/types.ts +++ b/packages/agent/src/harness/session/types.ts @@ -563,7 +563,7 @@ export type ForkOptions = scope: "branch"; /** Source Branch to copy under the same name in the destination. */ branch: string; - /** Entry to fork from. Defaults to the source Branch's current tip. */ + /** Entry on the source Branch's current tip ancestry. Defaults to the current tip. */ entryId?: string; /** * Whether the fork includes the selected entry or stops at its parent. diff --git a/packages/session-backends/sqlite-node/src/sqlite/repo.ts b/packages/session-backends/sqlite-node/src/sqlite/repo.ts index 5168e9d48..07404927c 100644 --- a/packages/session-backends/sqlite-node/src/sqlite/repo.ts +++ b/packages/session-backends/sqlite-node/src/sqlite/repo.ts @@ -114,8 +114,9 @@ function readForkSourceEntries( (stored) => stored.address.namespace === sourceAddress.namespace && stored.address.key === sourceAddress.key, ) as StoredValue | undefined; if (sourceTip === undefined) throw new Error(`Unknown source branch: ${options.branch}`); - const requested = options.entryId ?? sourceTip.value; - return requested === null ? [] : scanBranchEntries(db, sessionId, { start: requested, order: "oldestFirst" }); + return sourceTip.value === null + ? [] + : scanBranchEntries(db, sessionId, { start: sourceTip.value, order: "oldestFirst" }); } function createSqliteForkSnapshot( diff --git a/packages/session-backends/sqlite-node/src/sqlite/storage.ts b/packages/session-backends/sqlite-node/src/sqlite/storage.ts index a9c24e0d7..197e88443 100644 --- a/packages/session-backends/sqlite-node/src/sqlite/storage.ts +++ b/packages/session-backends/sqlite-node/src/sqlite/storage.ts @@ -155,10 +155,9 @@ export class SqliteStorage implements Storage { (stored) => stored.address.namespace === sourceAddress.namespace && stored.address.key === sourceAddress.key, ) as StoredValue | undefined; if (sourceTip === undefined) throw new Error(`Unknown source branch: ${options.branch}`); - const requested = options.entryId ?? sourceTip.value; - return requested === null + return sourceTip.value === null ? [] - : scanBranchEntries(this.db, this.sessionId, { start: requested, order: "oldestFirst" }); + : scanBranchEntries(this.db, this.sessionId, { start: sourceTip.value, order: "oldestFirst" }); } private applyCommit(writes: Write[]): CommitResult {