Skip to content

Commit 9bbb1b4

Browse files
steipetesteipete
committed
fix(gateway): guard worktree name reuse by owner and env-scope group transactions
- managedWorktrees.create rejects a caller-chosen name whose live or restorable record belongs to a different owner, so write-scoped sessions.create cannot bind a session into another session's or a manual checkout - session group catalog writes run their SQLite transaction on the same env-scoped handle as their statements, keeping OPENCLAW_STATE_DIR overrides atomic and away from the default state DB Part of #103431
1 parent a6afa58 commit 9bbb1b4

3 files changed

Lines changed: 138 additions & 72 deletions

File tree

‎src/agents/worktrees/service.test.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,34 @@ describe("ManagedWorktreeService", () => {
157157
expect(await git(created.path, "rev-parse", "HEAD")).toBe(baseCommit);
158158
});
159159

160+
it("rejects name reuse across owners instead of adopting a foreign worktree", async () => {
161+
await service.create({
162+
repoRoot: repo,
163+
name: "shared-name",
164+
ownerKind: "session",
165+
ownerId: "agent:main:dashboard:one",
166+
});
167+
await expect(
168+
service.create({
169+
repoRoot: repo,
170+
name: "shared-name",
171+
ownerKind: "session",
172+
ownerId: "agent:main:dashboard:two",
173+
}),
174+
).rejects.toThrow(/already in use by session/);
175+
await expect(service.create({ repoRoot: repo, name: "shared-name" })).rejects.toThrow(
176+
/already in use by session/,
177+
);
178+
// The rightful owner still reuses its record.
179+
const reused = await service.create({
180+
repoRoot: repo,
181+
name: "shared-name",
182+
ownerKind: "session",
183+
ownerId: "agent:main:dashboard:one",
184+
});
185+
expect(reused.ownerId).toBe("agent:main:dashboard:one");
186+
});
187+
160188
it("does not remove a concurrent successful create during remote fallback", async () => {
161189
await addRemote(root, repo);
162190

‎src/agents/worktrees/service.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,16 @@ function generateName(): string {
7575
return `wt-${randomBytes(4).toString("hex")}`;
7676
}
7777

78+
function recordOwnerMatches(
79+
record: ManagedWorktreeRecord,
80+
params: Pick<CreateManagedWorktreeParams, "ownerKind" | "ownerId">,
81+
): boolean {
82+
return (
83+
record.ownerKind === (params.ownerKind ?? "manual") &&
84+
(record.ownerId ?? undefined) === (params.ownerId ?? undefined)
85+
);
86+
}
87+
7888
async function resolveRepository(repoRoot: string): Promise<{
7989
repoRoot: string;
8090
sourceRoot: string;
@@ -360,13 +370,26 @@ export class ManagedWorktreeService {
360370
const root = path.join(resolveStateDir(this.env), "worktrees", repository.fingerprint);
361371
const worktreePath = path.join(root, name);
362372
const existing = findRegistryWorktreeByPath(this.env, worktreePath);
373+
// Name reuse only ever adopts the caller's own record. Without this guard a
374+
// caller-chosen name could bind a new owner to another session's or a
375+
// manual checkout and run inside it.
376+
if (existing?.name === name && !existing.removedAt && !recordOwnerMatches(existing, params)) {
377+
throw new Error(
378+
`worktree name is already in use by ${existing.ownerKind}${existing.ownerId ? ` ${existing.ownerId}` : ""}: ${name}`,
379+
);
380+
}
363381
if (existing?.name === name && existing.removedAt === undefined) {
364382
if (await pathExists(existing.path)) {
365383
return existing;
366384
}
367385
updateRegistryWorktree(this.env, existing.id, { removedAt: this.now() });
368386
}
369387
if (existing?.name === name && existing.removedAt !== undefined && existing.snapshotRef) {
388+
if (!recordOwnerMatches(existing, params)) {
389+
throw new Error(
390+
`worktree name is already in use by ${existing.ownerKind}${existing.ownerId ? ` ${existing.ownerId}` : ""}: ${name}`,
391+
);
392+
}
370393
return await this.restore({ id: existing.id });
371394
}
372395
await fs.mkdir(root, { recursive: true });

‎src/gateway/session-groups.ts‎

Lines changed: 87 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@ import {
1313
runOpenClawStateWriteTransaction,
1414
} from "../state/openclaw-state-db.js";
1515

16+
// Write transactions must run on the same env-scoped handle as their
17+
// statements; a bare transaction would open the default state DB while the
18+
// SQL hits the override, losing atomicity under OPENCLAW_STATE_DIR overrides.
19+
1620
export type SessionGroupRecord = { name: string; position: number };
1721

1822
type SessionGroupsDatabase = Pick<OpenClawStateKyselyDatabase, "session_groups">;
@@ -58,28 +62,30 @@ export function putSessionGroups(
5862
env: NodeJS.ProcessEnv = process.env,
5963
): SessionGroupRecord[] {
6064
const normalized = normalizeGroupNames(names);
61-
const db = dbFor(env);
62-
const kysely = kyselyFor(db);
6365
const now = Date.now();
64-
runOpenClawStateWriteTransaction(() => {
65-
const existing = new Map(
66-
executeSqliteQuerySync(
67-
db,
68-
kysely.selectFrom("session_groups").select(["name", "created_at"]),
69-
).rows.map((row) => [row.name, row.created_at]),
70-
);
71-
executeSqliteQuerySync(db, kysely.deleteFrom("session_groups"));
72-
normalized.forEach((name, position) => {
73-
executeSqliteQuerySync(
74-
db,
75-
kysely.insertInto("session_groups").values({
76-
name,
77-
position,
78-
created_at: existing.get(name) ?? now,
79-
}),
66+
runOpenClawStateWriteTransaction(
67+
({ db }) => {
68+
const kysely = kyselyFor(db);
69+
const existing = new Map(
70+
executeSqliteQuerySync(
71+
db,
72+
kysely.selectFrom("session_groups").select(["name", "created_at"]),
73+
).rows.map((row) => [row.name, row.created_at]),
8074
);
81-
});
82-
});
75+
executeSqliteQuerySync(db, kysely.deleteFrom("session_groups"));
76+
normalized.forEach((name, position) => {
77+
executeSqliteQuerySync(
78+
db,
79+
kysely.insertInto("session_groups").values({
80+
name,
81+
position,
82+
created_at: existing.get(name) ?? now,
83+
}),
84+
);
85+
});
86+
},
87+
{ env },
88+
);
8389
return normalized.map((name, position) => ({ name, position }));
8490
}
8591

@@ -95,57 +101,61 @@ export function ensureSessionGroupRegistered(
95101
if (!normalized) {
96102
return;
97103
}
98-
const db = dbFor(env);
99-
const kysely = kyselyFor(db);
100-
runOpenClawStateWriteTransaction(() => {
101-
const existing = executeSqliteQuerySync(
102-
db,
103-
kysely.selectFrom("session_groups").select("name").where("name", "=", normalized).limit(1),
104-
).rows[0];
105-
if (existing) {
106-
return;
107-
}
108-
const maxRow = executeSqliteQuerySync(
109-
db,
110-
kysely.selectFrom("session_groups").select("position").orderBy("position", "desc").limit(1),
111-
).rows[0];
112-
executeSqliteQuerySync(
113-
db,
114-
kysely.insertInto("session_groups").values({
115-
name: normalized,
116-
position: (maxRow?.position ?? -1) + 1,
117-
created_at: Date.now(),
118-
}),
119-
);
120-
});
104+
runOpenClawStateWriteTransaction(
105+
({ db }) => {
106+
const kysely = kyselyFor(db);
107+
const existing = executeSqliteQuerySync(
108+
db,
109+
kysely.selectFrom("session_groups").select("name").where("name", "=", normalized).limit(1),
110+
).rows[0];
111+
if (existing) {
112+
return;
113+
}
114+
const maxRow = executeSqliteQuerySync(
115+
db,
116+
kysely.selectFrom("session_groups").select("position").orderBy("position", "desc").limit(1),
117+
).rows[0];
118+
executeSqliteQuerySync(
119+
db,
120+
kysely.insertInto("session_groups").values({
121+
name: normalized,
122+
position: (maxRow?.position ?? -1) + 1,
123+
created_at: Date.now(),
124+
}),
125+
);
126+
},
127+
{ env },
128+
);
121129
}
122130

123131
function renameCatalogEntry(from: string, to: string, env: NodeJS.ProcessEnv): void {
124-
const db = dbFor(env);
125-
const kysely = kyselyFor(db);
126-
runOpenClawStateWriteTransaction(() => {
127-
const source = executeSqliteQuerySync(
128-
db,
129-
kysely.selectFrom("session_groups").selectAll().where("name", "=", from).limit(1),
130-
).rows[0];
131-
const targetExists = executeSqliteQuerySync(
132-
db,
133-
kysely.selectFrom("session_groups").select("name").where("name", "=", to).limit(1),
134-
).rows[0];
135-
executeSqliteQuerySync(db, kysely.deleteFrom("session_groups").where("name", "=", from));
136-
if (targetExists) {
137-
// Rename into an existing group merges memberships; keep the target row.
138-
return;
139-
}
140-
executeSqliteQuerySync(
141-
db,
142-
kysely.insertInto("session_groups").values({
143-
name: to,
144-
position: source?.position ?? 0,
145-
created_at: source?.created_at ?? Date.now(),
146-
}),
147-
);
148-
});
132+
runOpenClawStateWriteTransaction(
133+
({ db }) => {
134+
const kysely = kyselyFor(db);
135+
const source = executeSqliteQuerySync(
136+
db,
137+
kysely.selectFrom("session_groups").selectAll().where("name", "=", from).limit(1),
138+
).rows[0];
139+
const targetExists = executeSqliteQuerySync(
140+
db,
141+
kysely.selectFrom("session_groups").select("name").where("name", "=", to).limit(1),
142+
).rows[0];
143+
executeSqliteQuerySync(db, kysely.deleteFrom("session_groups").where("name", "=", from));
144+
if (targetExists) {
145+
// Rename into an existing group merges memberships; keep the target row.
146+
return;
147+
}
148+
executeSqliteQuerySync(
149+
db,
150+
kysely.insertInto("session_groups").values({
151+
name: to,
152+
position: source?.position ?? 0,
153+
created_at: source?.created_at ?? Date.now(),
154+
}),
155+
);
156+
},
157+
{ env },
158+
);
149159
}
150160

151161
/**
@@ -211,10 +221,15 @@ export async function deleteSessionGroup(params: {
211221
if (!name) {
212222
throw new Error("group delete requires a non-empty name");
213223
}
214-
const db = dbFor(env);
215-
runOpenClawStateWriteTransaction(() => {
216-
executeSqliteQuerySync(db, kyselyFor(db).deleteFrom("session_groups").where("name", "=", name));
217-
});
224+
runOpenClawStateWriteTransaction(
225+
({ db }) => {
226+
executeSqliteQuerySync(
227+
db,
228+
kyselyFor(db).deleteFrom("session_groups").where("name", "=", name),
229+
);
230+
},
231+
{ env },
232+
);
218233
const updatedSessions = await updateMemberCategories(params.cfg, name, undefined, env);
219234
return { groups: listSessionGroups(env), updatedSessions };
220235
}

0 commit comments

Comments
 (0)