feat(storage): let the user choose where projects are stored - #80
Conversation
|
Reviewed the full diff against main (9f88aae). Holding merge for two P0 data-safety issues: P0-1 Relocation skips the SQLite authority store
P0-2 Media is copied from the wrong source in default mode
P1
Details and the remaining P2s (isolated keystore not relocated, non-atomic pointer write, no data-merge on clearing, models field carrying a path, missing SQLite/isolated-pointer/media test coverage): see the full review. The zero-regression default path and the pointer/no-keystore cycle-break design are sound — the mechanics of the copy are what need rework. |
Projects, version history and media live in a hidden application data
directory, so uninstalling or reinstalling the app sits on top of the user's
work, and a user who wants that work on an external or synced drive has no
way to say so. Add a "project storage folder" setting next to the existing
media directory one.
Resolution order, highest first:
OPENCHATCUT_DATA_DIR pins the root from a launch script and refuses any
change from the settings panel
pointer file ~/.openchatcut/data-dir.json, written by the panel
default today's behaviour, unchanged
The chosen path cannot be recorded in the settings keystore: an isolated dev
profile keeps its keystore inside the storage root, so reading it would mean
already knowing the root. Hence the small pointer file at a fixed location
that never moves. A damaged pointer degrades to the default instead of
blocking startup, since the profile resolves synchronously at module load
and has nowhere to report an error.
Saving copies the known store entries into the new root and never deletes
the source: relocating must not be the step that loses the projects. An
entry that already exists in a non-empty destination is skipped and logged,
never overwritten. Regenerated files (logs, caches) stay behind. The active
profile is resolved at startup, so the move only takes effect on the next
launch, which the save response now reports through restartRequired.
Tests: new server/data-dir.verify.ts (expansion, pointer round trip and
degradation, writability probe, relocation safety), storage-root cases in
server/runtime-profile.verify.ts, and a storage/projects probe block in
server/key-probes.verify.ts.
server/key-probes.verify.ts also becomes hermetic. Its local-directory
assertions read the runtime profile, which resolves once at module load out
of HOME and the profile environment, so the developer's own storage root, or
an exported OPENCHATCUT_DEV_PROFILE_ID, could decide their outcome. The
suite now pins both to a throwaway home before importing the module under
test. Reproduced before the fix: with a storage folder configured, assertion
8 failed on a path outside the checkout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…files Addresses the review on 0xsline#80. P0-1 · relocation skipped the SQLite authority store. RELOCATED_ENTRIES only listed the legacy JSON entries, so a migrated user landed on an empty root with projects, chat, versions and agent runs all apparently gone. A plain copy of a live WAL-mode database is unsafe anyway, so relocation now refuses outright while sqliteStoreEnabled(), with an explicit message. A quiesced snapshot taken under the store lock is the right fix and belongs in its own change, not behind a settings field that looks harmless. P0-2 · media was copied from the wrong source. In the default profile uploads live outside the root (checkout public/media/uploads, or userData when packaged), so copying <root>/media moved an empty directory and every /media/uploads/... reference went offline after the restart. Media is now out of the root-entry list entirely and copied through syncUploadDirectories() from the resolved uploadDir(profile) into relocatedUploadDir(target). P1 · the machine-wide pointer file leaked into isolated dev profiles, which breaks the isolation rule: a checkout could read and write the developer's real storage root. Isolated profiles now accept OPENCHATCUT_DATA_DIR only, and the settings endpoint refuses to change the storage directory while one is active. Tests: relocation refusal under SQLite (nothing copied at all), media excluded from root entries, relocatedUploadDir target, and an isolated profile ignoring the pointer file. runtime-profile / data-dir / key-probes verifies, tsc and oxlint pass, including with a hostile HOME + DATA_DIR + DEV_PROFILE_ID exported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ec2518f to
c9ef4ba
Compare
|
Thanks for the review, it was precise and both P0s were real. Rebased on P0-1 · SQLite · relocation now refuses while P0-2 · media · P1 · isolated profiles · you were right that this broke the isolation rule. The pointer file is now honored for the ordinary profile only; isolated profiles accept Still open, and I would rather say so than paper over it: the copy still runs outside the store lock, so a write landing during the copy stays in the old root. With SQLite refused and the JSON store writing per-file atomically the blast radius is "the last few seconds of autosave", not corruption, but it is not zero. The clean fix is to relocate at startup before anything opens the store, so there are no concurrent writers by construction. Happy to do that here if you prefer it in one go, or as a follow-up. Verification: The P2s you listed (isolated keystore, atomic pointer write, merge-on-clear, |
Two findings from an adversarial pass over the previous fix.
Clearing the storage field skipped everything. expandDataDir('') returns null,
so the relocation block was bypassed entirely: no SQLite refusal, no copy. A
user who had migrated to SQLite and moved their storage, then cleared the field
to "go back to default", restarted on an empty or stale root with projects,
chat and versions apparently gone. Exactly the case the SQLite refusal exists
to prevent, reached through the other door. Clearing is now treated as what it
is: a relocation to defaultRootDir(profile), with the same refusal and the same
copy.
Entry copies are now atomic. cp() wrote straight into the destination, so an
interrupted relocation (disk full, power loss, quit) left a half-copied store
that the non-empty guard then treated as complete forever, and the next attempt
skipped it. Each entry is now copied to a staging sibling and renamed into
place, so it exists whole or not at all; a leftover staging directory from a
crashed attempt is discarded on the retry.
Tests cover both: clearing while relocated, and a retry after a simulated crash
that must adopt the real store and leave nothing of the partial one behind.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Follow-up: I put the reworked diff through an adversarial pass before asking you to look again, and it found a hole I had left open. Pushed as Clearing the field bypassed the SQLite refusal. Entry copies were not atomic. Both are covered by tests: clearing while relocated, and a retry after a simulated crash that must adopt the real store and leave nothing of the partial one behind. Verification unchanged in method: The store-lock point from my previous comment still stands as the remaining known gap, and I still think relocating at startup before anything opens the store is the honest fix for it. Say the word and I will do it here rather than as a follow-up. |
…files Addresses the review on #80. P0-1 · relocation skipped the SQLite authority store. RELOCATED_ENTRIES only listed the legacy JSON entries, so a migrated user landed on an empty root with projects, chat, versions and agent runs all apparently gone. A plain copy of a live WAL-mode database is unsafe anyway, so relocation now refuses outright while sqliteStoreEnabled(), with an explicit message. A quiesced snapshot taken under the store lock is the right fix and belongs in its own change, not behind a settings field that looks harmless. P0-2 · media was copied from the wrong source. In the default profile uploads live outside the root (checkout public/media/uploads, or userData when packaged), so copying <root>/media moved an empty directory and every /media/uploads/... reference went offline after the restart. Media is now out of the root-entry list entirely and copied through syncUploadDirectories() from the resolved uploadDir(profile) into relocatedUploadDir(target). P1 · the machine-wide pointer file leaked into isolated dev profiles, which breaks the isolation rule: a checkout could read and write the developer's real storage root. Isolated profiles now accept OPENCHATCUT_DATA_DIR only, and the settings endpoint refuses to change the storage directory while one is active. Tests: relocation refusal under SQLite (nothing copied at all), media excluded from root entries, relocatedUploadDir target, and an isolated profile ignoring the pointer file. runtime-profile / data-dir / key-probes verifies, tsc and oxlint pass, including with a hostile HOME + DATA_DIR + DEV_PROFILE_ID exported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merged into main (1186471 + rework commits + one follow-up fix d4052ab). The remaining clear-direction media target issue from the re-review is fixed: clearing the field now copies media back to the checkout-relative DEFAULT_UPLOAD_DIR (matching the no-dataDir branch of resolveRuntimeProfile) instead of ~/.openchatcut/media/uploads, extracted as relocatedMediaDestination with verify coverage for both directions. tsc/lint and data-dir/runtime-profile/key-probes verifiers pass. Store-lock-atomicity stays deferred as the agreed P2 (legacy-JSON only, SQLite relocation refuses outright). |
…files Addresses the review on 0xsline#80. P0-1 · relocation skipped the SQLite authority store. RELOCATED_ENTRIES only listed the legacy JSON entries, so a migrated user landed on an empty root with projects, chat, versions and agent runs all apparently gone. A plain copy of a live WAL-mode database is unsafe anyway, so relocation now refuses outright while sqliteStoreEnabled(), with an explicit message. A quiesced snapshot taken under the store lock is the right fix and belongs in its own change, not behind a settings field that looks harmless. P0-2 · media was copied from the wrong source. In the default profile uploads live outside the root (checkout public/media/uploads, or userData when packaged), so copying <root>/media moved an empty directory and every /media/uploads/... reference went offline after the restart. Media is now out of the root-entry list entirely and copied through syncUploadDirectories() from the resolved uploadDir(profile) into relocatedUploadDir(target). P1 · the machine-wide pointer file leaked into isolated dev profiles, which breaks the isolation rule: a checkout could read and write the developer's real storage root. Isolated profiles now accept OPENCHATCUT_DATA_DIR only, and the settings endpoint refuses to change the storage directory while one is active. Tests: relocation refusal under SQLite (nothing copied at all), media excluded from root entries, relocatedUploadDir target, and an isolated profile ignoring the pointer file. runtime-profile / data-dir / key-probes verifies, tsc and oxlint pass, including with a hostile HOME + DATA_DIR + DEV_PROFILE_ID exported.
Why
Projects, version history and media live in a hidden application data directory. Uninstalling or reinstalling the app sits on top of the user's work, and a user who wants that work on an external drive or a synced folder has no way to say so. This adds a project storage folder setting, next to the media directory one that already exists.
I hit this while using OpenChatCut for real editing work: I wanted the saved projects in my own backed-up folder, not under
~/.openchatcut.How it resolves
OPENCHATCUT_DATA_DIR~/.openchatcut/data-dir.jsonThe chosen path cannot be stored in the settings keystore: an isolated dev profile keeps its keystore inside the storage root, so reading it would mean already knowing the root. Hence the small pointer file at a fixed location that never moves. A damaged pointer degrades to the default rather than blocking startup, because the profile resolves synchronously at module load and has nowhere to report an error.
Relocation safety
Saving copies the known store entries (
project-store-v1,media, tombstones, generation operations) into the new root and never deletes the source: relocating must not be the step that loses someone's projects. An entry that already exists in a non-empty destination is skipped and logged, never overwritten. Regenerated files (logs, caches) stay behind.The active profile is resolved at startup, so the move only applies on the next launch. The save response reports that with
restartRequired, and the panel says so instead of the generic "saved" message.Test suite hardening included
server/key-probes.verify.tswas not hermetic. Its local-directory assertions read the runtime profile, which resolves once at module load out ofHOMEand the profile environment, so a developer's own storage root, or an exportedOPENCHATCUT_DEV_PROFILE_ID, could decide their outcome. Assertion 8 failed for me on a path outside the checkout. The suite now pinsHOMEand the profile environment to a throwaway directory before importing the module under test, so the existing assertions keep their meaning on any machine.Verification
server/data-dir.verify.ts(new): path expansion, pointer round trip and its degradation, writability probe, relocation safety, idempotent root creation. Registered inverify:runtime-profile.server/runtime-profile.verify.ts: environment variable wins over the pointer,~/expansion, relative path throws, damaged pointer degrades, isolated dev profile follows the chosen root,defaultRootDir()still answers "where does clearing the field lead?".server/key-probes.verify.ts: newstorage/projectsprobe block (unset, relative, writable, recorded pointer, pinned by environment). Local disk only, no network request.tsc -b,oxlint,verify:i18n,verify:runtime-profile,verify:affected: all pass. The three suites above also pass with a hostile environment exported (OPENCHATCUT_DEV_PROFILE_ID+OPENCHATCUT_DATA_DIRset to unrelated values).Happy to split the
key-probes.verify.tsisolation into its own PR if you would rather review it separately.