Skip to content

Commit 1f023a1

Browse files
committed
fix: verify selected folder identity
1 parent 5b273ce commit 1f023a1

7 files changed

Lines changed: 160 additions & 15 deletions

File tree

AI.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ The core domain in `packages/core/src/domain/` covers the entities the plan call
8787
- the shared shell wraps routes in `ToastProvider`; route code should use `useToast()` for short-lived feedback and keep blocking or recoverable errors in `InlineAlert`
8888
- the shared shell exposes explicit project actions next to the save-state indicator: `Save now` / `Retry save` routes through the active `ProjectStoreAdapter.save()` plus `markSaving()` / `markSaved()` / `markSaveFailed()`, records a launcher recent from successful browser/folder save metadata, `Switch project` navigates to `/projects`, and `Close project` returns to the launcher immediately only for clean saved projects; dirty projects and unsaved in-memory imports/demos must confirm `Close without saving` before `closeProject()` runs
8989
- `StorageSettings` is the active-project storage-location surface, not a read-only trust report: a browser-local or unsaved web project can `Save to local folder`, while a folder-backed web project can `Change folder` or `Use browser storage`; each successful transition serializes the bundle with the intended trust, saves through the installed `window.__gph_store` adapter, calls `markSaved()` with returned metadata, and replaces the same-key launcher recent
90-
- storage-location transitions deliberately pass a null expected revision because the current revision belongs to the old target rather than the newly selected backend; folder selection checks `listFolderProjects()` and refuses to overwrite a same-ID `.pms.json` unless it is the already-active folder target, then calls the adapter's optional `restorePreviousFolder()` rollback after collisions, scan errors, failed writes, or an active-project switch so an unsuccessful transition cannot leave future folder operations bound to the unaccepted directory
90+
- storage-location transitions deliberately pass a null expected revision because the current revision belongs to the old target rather than the newly selected backend; folder selection checks `listFolderProjects()` and refuses to overwrite a same-ID `.pms.json` unless the adapter proves the selected directory is the same filesystem entry as the previous active binding through optional `isSelectedFolderSameAsPrevious()` semantics—display names and saved path strings are never folder identities. A confirmed re-selection of the current folder is a no-op rather than a blind rewrite, while collisions, scan errors, failed writes, and active-project switches call `restorePreviousFolder()` so an unsuccessful transition cannot leave future folder operations bound to the unaccepted directory
9191
- switching storage locations is copy-first and non-destructive: the old folder file or browser recovery copy remains in place, changing back to browser storage does not clear the shared browser folder handle needed by other folder-backed recents, and an `AbortError` from the native directory picker is a normal dismissal that does not mark the project save as failed
9292
- manual storage transitions serialize one target-trust snapshot before writing, then compare that snapshot with the latest active bundle normalized to the same target trust; the returned metadata still activates the new location, but a divergent latest bundle is immediately left dirty via `markUnsaved()` so edits made during the in-flight write are not falsely reported as saved and can auto-save next
9393
- web and desktop auto-save success and failure completions must verify that `latest.storageTrust` still matches the target they wrote before applying returned metadata or save-error state; this prevents an in-flight save to the previous location from reverting the active trust/path/revision or marking the newly active target as failed after a storage transition. Folder-to-folder moves are also disabled while the current folder project is dirty, because choosing a new browser folder handle changes the adapter target immediately.
@@ -726,6 +726,10 @@ The core domain in `packages/core/src/domain/` covers the entities the plan call
726726
- extending `ProjectStoreAdapter` with optional `restorePreviousFolder()` rollback semantics for consumers that validate a directory after the native picker returns
727727
- checkpointing the prior web `FileSystemDirectoryHandle` before each accepted pick and restoring both the active in-memory handle and best-effort IndexedDB binding when Settings rejects a same-ID collision
728728
- invoking rollback before showing the collision warning and after any scan/save failure or active-project switch, with web-adapter coverage proving scans return to the accepted folder plus Settings coverage for collision, picker cancellation, and failed-write behavior
729+
- addressed the follow-up Greptile same-name folder identity finding by:
730+
- removing the `StorageSettings` path-suffix/display-name heuristic that could mistake two distinct directories with the same basename for one folder target
731+
- adding optional `ProjectStoreAdapter.isSelectedFolderSameAsPrevious()` semantics and implementing the web comparison with `FileSystemHandle.isSameEntry()`; unavailable or failed identity checks conservatively remain collisions
732+
- covering same-name distinct handle comparisons in the web adapter plus both Settings outcomes: distinct directories retain the collision warning and restore the accepted binding, while a confirmed re-selection of the current directory performs no write
729733

730734
## Open follow-on planning
731735

Readme.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ Important behavior:
8383

8484
- New folder-backed projects write their initial `.pm-suite/<project-id>.pms.json` immediately.
8585
- An open browser-local project can move to a chosen folder from **Settings -> Storage** without being recreated; a folder-backed project can change folders or switch back to browser storage from the same panel.
86-
- Changing storage locations writes and activates the new copy first. The previous browser or folder copy is retained as a recovery point, and a collision or failed move restores the previously accepted folder binding instead of leaving later saves pointed at a rejected directory.
86+
- Changing storage locations writes and activates the new copy first. The previous browser or folder copy is retained as a recovery point, and a collision or failed move restores the previously accepted folder binding instead of leaving later saves pointed at a rejected directory. Folder identity is checked by the browser's filesystem handles, so two different folders with the same name cannot bypass the same-project collision guard; reselecting the actual current folder is recognized without rewriting its file.
8787
- If an edit arrives while that location change is still writing, Grillo activates the new destination but keeps the newer edit marked unsaved so the next auto-save includes it.
8888
- Auto-save results belong to the storage target that started them. If the user changes targets while a save is in flight, its later success or failure cannot overwrite the new target's saved state.
8989
- Folder-backed browser saves keep a browser-local recovery copy so reloads are recoverable when the browser cannot restore folder access.

apps/web/src/platform/storage/web-storage.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ function createFakeFolder(name: string, seed: Record<string, string> = {}): Fake
4545
name,
4646
queryPermission: async () => "granted",
4747
requestPermission: async () => "granted",
48+
isSameEntry: async (other: FileSystemHandle) => other === handle,
4849
getDirectoryHandle: async (dirname: string, options?: FileSystemGetDirectoryOptions) => {
4950
if (dirname === ".pm-suite") return projectDir;
5051
if (options?.create) throw new Error("Only .pm-suite is supported by this test handle");
@@ -147,6 +148,28 @@ describe("WebStorageAdapter folder mode", () => {
147148
await expect(adapter.listFolderProjects?.()).resolves.toEqual(["accepted.pms.json"]);
148149
});
149150

151+
it("compares folder picks by handle identity instead of their display names", async () => {
152+
const accepted = createFakeFolder("Shared Name");
153+
const different = createFakeFolder("Shared Name");
154+
Object.defineProperty(window, "showDirectoryPicker", {
155+
configurable: true,
156+
value: vi.fn()
157+
.mockResolvedValueOnce(accepted.handle)
158+
.mockResolvedValueOnce(accepted.handle)
159+
.mockResolvedValueOnce(different.handle)
160+
});
161+
const adapter = await getAdapter();
162+
163+
await adapter.chooseFolder?.();
164+
await expect(adapter.isSelectedFolderSameAsPrevious?.()).resolves.toBe(false);
165+
166+
await adapter.chooseFolder?.();
167+
await expect(adapter.isSelectedFolderSameAsPrevious?.()).resolves.toBe(true);
168+
169+
await adapter.chooseFolder?.();
170+
await expect(adapter.isSelectedFolderSameAsPrevious?.()).resolves.toBe(false);
171+
});
172+
150173
it("rejects a stale folder save after the project file changes externally", async () => {
151174
const bundle = buildProjectFromTemplate("software-project", "Original");
152175
const originalJson = exportProjectJson(bundle);

apps/web/src/platform/storage/web-storage.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -375,6 +375,24 @@ class WebLocalStorageAdapter implements ProjectStoreAdapter {
375375
// durable handle persistence remains best-effort, matching chooseFolder().
376376
}
377377
}
378+
async isSelectedFolderSameAsPrevious(): Promise<boolean> {
379+
const selectedHandle = activeFolderHandle;
380+
const previousHandle = this.previousFolderHandle;
381+
if (!selectedHandle || !previousHandle) return false;
382+
const isSameEntry = (
383+
selectedHandle as FileSystemDirectoryHandle & {
384+
isSameEntry?: (other: FileSystemHandle) => Promise<boolean>;
385+
}
386+
).isSameEntry;
387+
if (!isSameEntry) return false;
388+
try {
389+
return await isSameEntry.call(selectedHandle, previousHandle);
390+
} catch {
391+
// Display names are not identities. If the browser cannot compare handles,
392+
// callers must conservatively treat an existing same-ID file as a collision.
393+
return false;
394+
}
395+
}
378396
async getCurrentFolderDisplay(): Promise<string | null> {
379397
const handle = await readStoredFolderHandle();
380398
return handle?.name ?? null;

packages/core/src/storage/store.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ export type ProjectStoreAdapter = {
6565
chooseFolder?(): Promise<string | null>;
6666
/** Optional rollback for the most recent successful folder pick when validation rejects it. */
6767
restorePreviousFolder?(): Promise<void>;
68+
/** Optional identity check for the latest folder pick against the binding it replaced. */
69+
isSelectedFolderSameAsPrevious?(): Promise<boolean>;
6870
/** Optional human-readable display name for the currently selected folder. */
6971
getCurrentFolderDisplay?(): Promise<string | null>;
7072
/** Optional reset that makes subsequent explicitly browser-local saves ignore a selected folder. */

packages/ui/src/views/settings/SettingsView.test.tsx

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,104 @@ describe("SettingsView", () => {
399399
expect(useProjectStore.getState()).toMatchObject({ storageTrust: "browser", saveError: null });
400400
});
401401

402+
it("does not trust a matching folder name when a different folder contains the same project id", async () => {
403+
const bundle = useProjectStore.getState().bundle!;
404+
const storagePath = `Shared Name/.pm-suite/${bundle.project.id}.pms.json`;
405+
useProjectStore.setState({
406+
storagePath,
407+
storageTrust: "folder",
408+
isDirty: false,
409+
saveStatus: "saved"
410+
});
411+
const save = vi.fn();
412+
const restorePreviousFolder = vi.fn(async () => undefined);
413+
const isSelectedFolderSameAsPrevious = vi.fn(async () => false);
414+
const adapter: ProjectStoreAdapter = {
415+
capabilities: { folderBacked: true, fileWatch: false, attachments: true },
416+
list: async () => [],
417+
has: async () => true,
418+
load: async () => null,
419+
save,
420+
delete: async () => undefined,
421+
chooseFolder: async () => "Shared Name",
422+
restorePreviousFolder,
423+
isSelectedFolderSameAsPrevious,
424+
listFolderProjects: async () => [`${bundle.project.id}.pms.json`]
425+
};
426+
(window as typeof window & { __gph_store?: ProjectStoreAdapter }).__gph_store = adapter;
427+
428+
render(
429+
<ThemeProvider>
430+
<MemoryRouter>
431+
<SettingsView />
432+
</MemoryRouter>
433+
</ThemeProvider>
434+
);
435+
436+
await userEvent.click(screen.getByRole("tab", { name: "Storage" }));
437+
const panel = screen.getByRole("tabpanel", { name: "Storage" });
438+
await userEvent.click(within(panel).getByRole("button", { name: "Change folder" }));
439+
440+
expect(await within(panel).findByText(/already contains .*\.pms\.json/i)).toBeInTheDocument();
441+
expect(isSelectedFolderSameAsPrevious).toHaveBeenCalledOnce();
442+
expect(save).not.toHaveBeenCalled();
443+
expect(restorePreviousFolder).toHaveBeenCalledOnce();
444+
expect(useProjectStore.getState()).toMatchObject({
445+
storagePath,
446+
storageTrust: "folder",
447+
saveStatus: "saved"
448+
});
449+
});
450+
451+
it("treats a confirmed re-selection of the active folder as a no-op", async () => {
452+
const bundle = useProjectStore.getState().bundle!;
453+
const storagePath = `Client Work/.pm-suite/${bundle.project.id}.pms.json`;
454+
useProjectStore.setState({
455+
storagePath,
456+
storageTrust: "folder",
457+
isDirty: false,
458+
saveStatus: "saved"
459+
});
460+
const save = vi.fn();
461+
const restorePreviousFolder = vi.fn(async () => undefined);
462+
const isSelectedFolderSameAsPrevious = vi.fn(async () => true);
463+
const adapter: ProjectStoreAdapter = {
464+
capabilities: { folderBacked: true, fileWatch: false, attachments: true },
465+
list: async () => [],
466+
has: async () => true,
467+
load: async () => null,
468+
save,
469+
delete: async () => undefined,
470+
chooseFolder: async () => "Client Work",
471+
restorePreviousFolder,
472+
isSelectedFolderSameAsPrevious,
473+
listFolderProjects: async () => [`${bundle.project.id}.pms.json`]
474+
};
475+
(window as typeof window & { __gph_store?: ProjectStoreAdapter }).__gph_store = adapter;
476+
477+
render(
478+
<ThemeProvider>
479+
<MemoryRouter>
480+
<SettingsView />
481+
</MemoryRouter>
482+
</ThemeProvider>
483+
);
484+
485+
await userEvent.click(screen.getByRole("tab", { name: "Storage" }));
486+
const panel = screen.getByRole("tabpanel", { name: "Storage" });
487+
await userEvent.click(within(panel).getByRole("button", { name: "Change folder" }));
488+
489+
expect(await within(panel).findByText(/already saved in Client Work\. No files were changed/i)).toBeInTheDocument();
490+
expect(isSelectedFolderSameAsPrevious).toHaveBeenCalledOnce();
491+
expect(save).not.toHaveBeenCalled();
492+
expect(restorePreviousFolder).not.toHaveBeenCalled();
493+
expect(useProjectStore.getState()).toMatchObject({
494+
storagePath,
495+
storageTrust: "folder",
496+
saveStatus: "saved"
497+
});
498+
});
499+
402500
it("restores the previous folder when saving to a newly selected folder fails", async () => {
403501
const restorePreviousFolder = vi.fn(async () => undefined);
404502
const adapter: ProjectStoreAdapter = {

packages/ui/src/views/settings/StorageSettings.tsx

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -36,17 +36,6 @@ function isFolderPickerDismissal(error: unknown): boolean {
3636
return error instanceof DOMException && error.name === "AbortError";
3737
}
3838

39-
function isCurrentFolderTarget(
40-
storagePath: string | null,
41-
folderName: string,
42-
projectKey: string
43-
): boolean {
44-
if (!storagePath) return false;
45-
const normalized = storagePath.replace(/\\/g, "/");
46-
const expected = `${folderName}/.pm-suite/${projectKey}.pms.json`;
47-
return normalized === expected || normalized.endsWith(`/${expected}`);
48-
}
49-
5039
export function StorageSettings() {
5140
const bundle = useProjectStore((state) => state.bundle);
5241
const storagePath = useProjectStore((state) => state.storagePath);
@@ -172,8 +161,19 @@ export function StorageSettings() {
172161
const projectAlreadyExists = folderFiles?.some(
173162
(filename) => filename.toLowerCase() === `${key}.pms.json`.toLowerCase()
174163
);
175-
const currentTarget = current.storageTrust === "folder" && isCurrentFolderTarget(current.storagePath, folderName, key);
176-
if (projectAlreadyExists && !currentTarget) {
164+
const currentTarget = Boolean(
165+
projectAlreadyExists
166+
&& current.storageTrust === "folder"
167+
&& await adapter.isSelectedFolderSameAsPrevious?.()
168+
);
169+
if (projectAlreadyExists && currentTarget) {
170+
setFeedback({
171+
message: `This project is already saved in ${folderName}. No files were changed.`,
172+
tone: "info"
173+
});
174+
return;
175+
}
176+
if (projectAlreadyExists) {
177177
await restoreRejectedFolder();
178178
setFeedback({
179179
message: `That folder already contains ${key}.pms.json. Grillo did not overwrite it; open that project from the workspace launcher or choose a different folder.`,

0 commit comments

Comments
 (0)