Skip to content

fix(web): keep synthetic session keys verbatim so marks survive persistence #353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
onychen wants to merge 1 commit into from
+152 −14
Closed

fix(web): keep synthetic session keys verbatim so marks survive persistence #353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 115 additions & 5 deletions tests/web/pi-adapter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ import {
mkdtemp,
readFile,
realpath,
rename,
rm,
writeFile,
} from "node:fs/promises";
import { tmpdir } from "node:os";
import { join, resolve } from "node:path";
import { basename, join, resolve } from "node:path";
import test from "node:test";
import { SessionManager } from "@earendil-works/pi-coding-agent";
import { PiWebAdapter } from "../../web/adapter/pi-adapter.ts";
Expand Down Expand Up @@ -224,9 +225,118 @@ test("first archive mutation preserves previously persisted archive metadata", a
const persisted = JSON.parse(
await readFile(join(sessionDirectory, "archived-sessions.json"), "utf8"),
) as string[];
assert.deepEqual(
new Set(persisted),
new Set([existing, resolve(currentPath)]),
assert.deepEqual(new Set(persisted), new Set([existing, currentPath]));
} finally {
await rm(root, { recursive: true, force: true });
}
});

test("removing the current workspace keeps its unpersisted session ungrouped after it persists", async () => {
const root = await mkdtemp(join(tmpdir(), "openpi-web-remove-workspace-"));
const sessionDirectory = join(root, "sessions");
const stash = join(root, "stash");
try {
await Promise.all([
mkdir(sessionDirectory, { recursive: true }),
mkdir(stash, { recursive: true }),
]);
const persisted = SessionManager.create(root, sessionDirectory);
persistSession(persisted, "persisted", 1);
const persistedPath = persisted.getSessionFile();
assert.ok(persistedPath);
const persistedId = persisted.getSessionId();
const stashedPath = join(stash, basename(persistedPath));
await rename(persistedPath, stashedPath);
let currentFile: string | null = null;
const current = {
getSessionId: () => persistedId,
getSessionFile: () => currentFile,
getBranch: () => [],
getSessionName: () => undefined,
} as unknown as SessionManager;
const adapter = new PiWebAdapter(
runtimeFor(root, sessionDirectory, current),
);

await adapter.removeWorkspace(root);

const state = JSON.parse(
await readFile(join(sessionDirectory, "workspace-state.json"), "utf8"),
) as { ungroupedSessions: string[] };
assert.deepEqual(state.ungroupedSessions, [`current:${persistedId}`]);
assert.equal(
(await adapter.getSnapshot()).sessions.find(
(session) => session.id === persistedId,
)?.ungrouped,
true,
);

await rename(stashedPath, persistedPath);
currentFile = persistedPath;

const snapshot = await adapter.getSnapshot();
assert.equal(
snapshot.sessions.find((session) => session.id === persistedId)
?.ungrouped,
true,
);
assert.equal(
(await adapter.requireSession(persistedPath)).cwd,
resolve(root),
);
} finally {
await rm(root, { recursive: true, force: true });
}
});

test("archiving an unpersisted session preserves the mark after it persists", async () => {
const root = await mkdtemp(join(tmpdir(), "openpi-web-archive-persist-"));
const sessionDirectory = join(root, "sessions");
const stash = join(root, "stash");
try {
await Promise.all([
mkdir(sessionDirectory, { recursive: true }),
mkdir(stash, { recursive: true }),
]);
const persisted = SessionManager.create(root, sessionDirectory);
persistSession(persisted, "persisted", 1);
const persistedPath = persisted.getSessionFile();
assert.ok(persistedPath);
const persistedId = persisted.getSessionId();
const stashedPath = join(stash, basename(persistedPath));
await rename(persistedPath, stashedPath);
let currentFile: string | null = null;
const current = {
getSessionId: () => persistedId,
getSessionFile: () => currentFile,
getBranch: () => [],
getSessionName: () => undefined,
} as unknown as SessionManager;
const adapter = new PiWebAdapter(
runtimeFor(root, sessionDirectory, current),
);

await adapter.archiveSession(`current:${persistedId}`);

const marks = JSON.parse(
await readFile(join(sessionDirectory, "archived-sessions.json"), "utf8"),
) as string[];
assert.deepEqual(marks, [`current:${persistedId}`]);
assert.equal(
(await adapter.getSnapshot()).sessions.find(
(session) => session.id === persistedId,
)?.archived,
true,
);

await rename(stashedPath, persistedPath);
currentFile = persistedPath;

assert.equal(
(await adapter.getSnapshot()).sessions.find(
(session) => session.id === persistedId,
)?.archived,
true,
);
} finally {
await rm(root, { recursive: true, force: true });
Expand Down Expand Up @@ -375,7 +485,7 @@ test("archive transactions roll back a failed mutation before the next mutation
const persisted = JSON.parse(
await readFile(join(sessionDirectory, "archived-sessions.json"), "utf8"),
) as string[];
assert.deepEqual(persisted, [resolve(currentPath)]);
assert.deepEqual(persisted, [currentPath]);
assert.equal(
(await adapter.getSnapshot()).sessions.find(
(session) => session.id === current.getSessionId(),
Expand Down
46 changes: 37 additions & 9 deletions web/adapter/pi-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,18 @@ type WorkspaceStateSnapshot = {
restoreInitialWorkspace: boolean;
};

// An unpersisted active session is keyed by the synthetic `current:<sessionId>`
// marker instead of a file path. Synthetic keys must round-trip verbatim:
// resolving one against the process cwd both pollutes persisted state and
// breaks lookups once the session gains a real file.
function isSyntheticSessionKey(path: string) {
return path.startsWith("current:");
}

function sessionKey(path: string) {
return isSyntheticSessionKey(path) ? path : resolve(path);
}

export class PiWebAdapter {
private readonly runtime: WebRuntimeController;
private readonly importedWorkspaces = new Set<string>();
Expand Down Expand Up @@ -88,7 +100,7 @@ export class PiWebAdapter {
this.hiddenWorkspaces.add(resolve(path));
}
for (const path of state.ungroupedSessions) {
this.ungroupedSessions.add(resolve(path));
this.ungroupedSessions.add(sessionKey(path));
}
for (const [path, name] of Object.entries(state.workspaceNames)) {
this.workspaceNames.set(resolve(path), name as string);
Expand Down Expand Up @@ -144,7 +156,7 @@ export class PiWebAdapter {
) {
throw new Error("Archive metadata must be an array of paths");
}
for (const path of parsed) this.archivedSessions.add(resolve(path));
for (const path of parsed) this.archivedSessions.add(sessionKey(path));
} catch (error) {
if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error;
}
Expand Down Expand Up @@ -280,7 +292,7 @@ export class PiWebAdapter {
await this.ensureArchivesLoaded();
await this.enqueueArchiveMutation(async (draft) => {
const session = await this.requireSession(path);
draft.add(resolve(session.path));
draft.add(sessionKey(session.path));
});
}

Expand All @@ -295,14 +307,14 @@ export class PiWebAdapter {
const sessions = await SessionManager.listAll(this.runtime.sessionDirectory);
for (const session of sessions) {
if (resolve(session.cwd) === canonical) {
draft.ungroupedSessions.add(resolve(session.path));
draft.ungroupedSessions.add(sessionKey(session.path));
}
}
if (resolve(this.runtime.cwd) === canonical) {
const currentPath =
this.runtime.sessionManager.getSessionFile() ??
`current:${this.runtime.sessionManager.getSessionId()}`;
draft.ungroupedSessions.add(resolve(currentPath));
draft.ungroupedSessions.add(sessionKey(currentPath));
}
draft.importedWorkspaces.delete(canonical);
draft.workspaceNames.delete(canonical);
Expand Down Expand Up @@ -351,8 +363,12 @@ export class PiWebAdapter {
session.firstMessage,
WEB_MAX_SESSION_PREVIEW,
),
...(this.archivedSessions.has(resolve(session.path)) ? { archived: true } : {}),
...(this.ungroupedSessions.has(resolve(session.path)) ? { ungrouped: true } : {}),
...(this.hasSessionMark(this.archivedSessions, session.path, session.id)
? { archived: true }
: {}),
...(this.hasSessionMark(this.ungroupedSessions, session.path, session.id)
? { ungrouped: true }
: {}),
}));
if (
this.runtime.workspaceSelected === true &&
Expand Down Expand Up @@ -390,10 +406,10 @@ export class PiWebAdapter {
created: now,
messageCount,
firstMessage: boundedText(firstUser, WEB_MAX_SESSION_PREVIEW),
...(this.archivedSessions.has(resolve(currentPath))
...(this.hasSessionMark(this.archivedSessions, currentPath, currentId)
? { archived: true }
: {}),
...(this.ungroupedSessions.has(resolve(currentPath))
...(this.hasSessionMark(this.ungroupedSessions, currentPath, currentId)
? { ungrouped: true }
: {}),
});
Expand Down Expand Up @@ -598,6 +614,18 @@ export class PiWebAdapter {
snapshot.truncation.truncated = true;
}

// Marks recorded while a session was unpersisted are stored under the
// synthetic `current:<id>` key; match either the resolved file path (marks
// recorded after persistence) or the synthetic key so marks survive the
// session gaining a real file.
private hasSessionMark(
marks: ReadonlySet<string>,
path: string,
id: string,
) {
return marks.has(sessionKey(path)) || marks.has(`current:${id}`);
}

private captureWorkspaceState(): WorkspaceStateSnapshot {
return {
importedWorkspaces: new Set(this.importedWorkspaces),
Expand Down
Loading