Ship readiness PR1: correctness + security hardening on Codex's cancellation pass#5
Open
correa-brian wants to merge 14 commits into
Open
Ship readiness PR1: correctness + security hardening on Codex's cancellation pass#5correa-brian wants to merge 14 commits into
correa-brian wants to merge 14 commits into
Conversation
Baseline commit for the HN-readiness branch. Captures the large Codex pass that added per-step live-status checks through image/video/text/transcript indexing paths, purge_indexed_data_for_file for partial-artifact cleanup, recover_interrupted_jobs Tauri command, isolated per-op connections for encrypted SQLCipher, rejection of audio files from the indexing path (Transcribe only), AudioPreview with real audio controls, transcribingPaths wiring through Grid/List/Preview, and hasActiveJobs including pending jobs. Subsequent commits on this branch apply the correctness + security fixes surfaced in the adversarial review of this pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to Codex's cancellation pass. Closes the correctness gaps surfaced by the adversarial review: B1: replace_text_chunks_for_file and replace_transcript_chunks_for_file wrap delete+insert in one BEGIN IMMEDIATE transaction so a cancel or crash between the two no longer leaves a file with zero indexed chunks. B2: introduce JobCancelled sentinel error + is_job_cancellation helper. Replace the fragile string-match on 'cancelled or removed from queue' across seven worker call sites with typed downcast. Filenames containing the old fragment can no longer trigger a spurious purge. B3: split recover API into recover_stale_jobs_at_startup (no grace, startup only) and recover_stale_running_jobs (clamped to 30s minimum). Tauri-exposed recover command now calls the graced variant so a rapid click cannot clobber a freshly claimed running job. B4: log DB update failures in the audio-rejection worker branch instead of dropping the Err silently. B7: drop redundant per-branch job-status checks inside index_image_files_batch. Keep one pre-encode check per file (cheap skip) and one batch-wide retain before bulk store. Trims ~2/3 of status queries on 100-image batches. F1: new src/lib/fileTypes.ts holds PURE_AUDIO_EXTENSIONS and TRANSCRIBABLE_EXTENSIONS; ContextMenu.tsx and PreviewContextMenu.tsx consume it instead of maintaining divergent local copies. Consolidation caught a real drift: PreviewContextMenu had been missing .aac and .wma in its transcribable set. Rust PURE_AUDIO_EXTENSIONS retains its const with a sync comment pointing at the TS source of truth. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
B5: extract sqlite-vec extension registration into a OnceLock-guarded helper called once per process. Removes four copies of the unsafe transmute block in database_service.rs. B6: replace execute_batch-with-format! PRAGMA calls with typed pragma_update for key, cipher_compatibility, journal_mode, and busy_timeout. The encryption key is alphanumeric so no injection was possible, but this is the idiomatic rusqlite form and removes the pattern from the codebase. Drive-by: generations_service::create_generation derived the primary key from chrono::Utc::now().timestamp_millis(). Two rapid calls in the same millisecond produced duplicate ids. Switched to uuid::Uuid::new_v4().simple() matching the pattern in job_queue_service, transcription_service, and watched_folder_service. The test suite now passes deterministically at --test-threads=1 (154/154). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
C1: production Tauri config now sets a default-src 'self' Content Security Policy with explicit allowances for the asset:/blob:/data: schemes we legitimately use for local image and media previews plus the IPC channels Tauri needs. Dev config stays permissive for Vite HMR. The asset protocol scope stays '**' intentionally. Cosmos lets users preview files anywhere on disk; restricting scope would break the core feature. The CSP carries the meaningful exfiltration-risk reduction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
P1: new fda_probe_service attempts to read ~/Library/Safari, an FDA-gated directory present on every macOS install. Returns Ok(true) when readable, Ok(false) on PermissionDenied, Err when the probe cannot be performed (unusual systems). Off macOS returns Ok(true) unconditionally. Exposed via the check_full_disk_access Tauri command. Frontend FullDiskAccessBanner hits the command on mount, renders a dismissable amber banner when FDA is missing, and links directly to the Privacy pane in System Settings via the x-apple.systempreferences: URL scheme through the opener plugin. Dismiss is session-scoped: the banner reappears next launch if FDA is still missing. Replaces the silent fail where indexing ~/Documents would just skip half the files without telling the user why. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
T1: new cancellation_consistency_tests.rs covers the invariants introduced by replace_text_chunks_for_file and replace_transcript_chunks_for_file. Includes a REGRESSION case where an invalid-embedding insert rolls back the transaction and leaves prior chunks intact — the exact scenario Codex's pass had widened by moving the delete after encoding. Also covers: transcript replace preserves document chunks at the same path, purge covers all tables, purge is idempotent on empty paths. T2: appended three tests to job_queue_tests.rs. Verifies that recover_stale_running_jobs clamps caller-supplied grace below 30s to the safety minimum (so a rapid UI click can't clobber a just-claimed running job), that recover_stale_jobs_at_startup is unconditional (safe because no workers are live), and that supplying a grace above the minimum still only resets jobs actually older than that grace. All eight new tests pass. Full suite: 154/154. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
README: add a Platform support table. macOS is signed/notarized/auto-updating; Windows and Linux build from source but are untested per release. Also adds a short note on how Cosmos surfaces the Full Disk Access prompt — only when it has reason to believe the permission is missing, never preemptively. SECURITY.md: new 'What is and isn't protected today' section owns the obfuscated-file key storage for what it is, names FileVault as the actual at-rest protection today, notes the in-memory key exposure all local apps share, and confirms no default network egress. Sets expectations honestly for HN security readers. TODOS.md: three new P2 items — failed-state UI for jobs (real gap), main.rs command-registration dedup macro (known drift), and cluster_members coverage in purge_indexed_data_for_file (wires up when Phase 2 clustering ships). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
VideoPreview was tracking click timestamps manually inside a useCallback to synthesize double-click detection. The callback's closure captured lastClickTime from state, but React's render cycle doesn't always flush fast enough between two rapid clicks for the second click to see the first click's timestamp update. Result: the play/pause toggle never fired. Switched to the browser's native onDoubleClick event. It fires after both click events have dispatched, with zero timing gymnastics. Single click still focuses the container and temporarily surfaces the controls overlay; double click toggles play/pause. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Left/Up goes to the previous file in the list you came from; Right/Down goes to the next. No modifier keys. Focus-aware: arrow keys inside inputs, textareas, selects, or contenteditable regions still do their normal thing. Implementation uses sessionStorage as the handoff: when PreviewContainer navigates a user into StudioEdit, it stashes the full filtered sibling list under 'studio.navigation' with the same normalization already applied to the path URL param. StudioEdit attaches a window-scoped keydown listener that reads the stash, finds the current file, and navigates to the neighbor (or no-ops at list boundaries). Gracefully no-ops when the stash is missing — arrow keys from entry points other than PreviewContainer (AppLayout search, Studio index, context-menu Send to Studio) just don't navigate until those sites opt in to writing the same key. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
StudioEdit calls setPointerCapture on pointerdown to drive its pan gesture. That capture diverts the subsequent pointerup and cancels the click event that would normally dispatch to the target. Result: the <video> element's onClick={togglePlayPause} never fires, which is why double-click didn't fix video playback — the click itself was being swallowed upstream. Same mechanism explains why clicking an image felt like dragging a canvas: the pan grabbed the pointer the instant you pressed down.
Introduced a 5px drag-threshold. On pointerdown we arm the gesture without capturing. On pointermove we only claim it as a drag (and call setPointerCapture) once movement exceeds the threshold. Below threshold, the click falls through to the target element as normal.
Standard drag-vs-click disambiguation; the rest of the codebase (ScrubBar, timeline) was already doing something similar. Same fix restores clicks on both images and videos in the preview.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hooked into the same window keydown listener that handles arrow-key navigation. Queries for the first <video> or <audio> element on the page and toggles paused state. Works for both StudioEdit's VideoPlayerWithTrim and the AudioPreview component. Respects the same input-focus guard so typing a space inside the search bar or any other input still inserts a space normally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
760 lines of unreachable code. The primary video player was replaced by VideoEditor/VideoPlayerWithTrim at some point; MediaPreview/VideoPreview stuck around but no call site imports it. Left a stale comment in VideoThumbnail.tsx referencing VideoPreview — cleaned that up too. Removing it eliminates a bug magnet: the earlier onDoubleClick fix in this PR landed here before I realized nothing renders it, and a future reader would hit the same trap trying to debug video playback by editing this file. Thumbnail coverage is unaffected — VideoThumbnail and NativeVideoThumbnail remain as they were. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two bugs compounded to produce the 'UNIQUE constraint failed on vec_images primary key' errors seen during video re-indexing: 1. purge_indexed_data_for_file (the cancel-safety cleanup) cleared rows from images, text_chunks, and vec_text_chunks — but never from vec_images. Orphan vec rows survived a cancelled indexing run. When the user re-indexed, SQLite recycled the old rowid for the fresh images row, and the new INSERT into vec_images collided with the orphan. 2. sqlite-vec's vec0 virtual tables do not honor ON CONFLICT REPLACE, so the INSERT OR REPLACE INTO vec_images(...) we relied on was effectively a plain INSERT. Once a collision happened it reported UNIQUE constraint failed, never actually replacing. Purge now explicitly deletes vec_images rows (joined to images via rowid) before dropping the images rows. All three insert paths — store_image_vector_with_drive, store_image_vectors_bulk, and the new replace_text_chunks_for_file helpers — now do DELETE-then-INSERT on the vec table to be tolerant of any stale rowid. Added reindex_after_purge_does_not_collide_on_recycled_rowid to lock the regression in. Full suite: 155/155. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The FDA banner's 'Open System Settings' button invokes openUrl with the x-apple.systempreferences: scheme. Tauri v2's opener plugin rejects any scheme not explicitly allowlisted in the capability — that's why the button was throwing 'Not allowed to open url' at runtime even though the CSP was fine. Added opener:allow-open-url scoped to three patterns: x-apple.systempreferences:*, https://*, and http://*. Keeps the default file path permission and prevents arbitrary custom-scheme escape hatches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
correa-brian
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First of two PRs closing the gaps surfaced by the adversarial review of Codex's recent job-cancellation pass. Scope: backend correctness, shared file-type helpers, Tauri CSP, Full Disk Access UX. PR2 (design/UX polish) will follow on a separate branch.
The base of this PR (
1bf5483) is Codex's pass as-is. Every subsequent commit is one of the fixes called out in~/.gstack/projects/golivecosmos-cosmos-oss/brian-main-hn-readiness-plan-*.md.What shipped
Cancel-safety (B1, B2).
replace_text_chunks_for_fileandreplace_transcript_chunks_for_filewrap delete + insert in oneBEGIN IMMEDIATEtransaction so a cancel or crash between the two cannot leave a file with zero indexed chunks. Fragile string-match on the error message (is_job_interruption_error) is gone — replaced with a typedJobCancelledsentinel and a downcast helper across seven worker call sites. Filenames containing the old fragment can no longer trigger a spurious purge.Recover-grace (B3).
recover_orphaned_jobssplit into two call-site-appropriate variants.recover_stale_jobs_at_startupruns unconditionally (no workers are live yet).recover_stale_running_jobs(grace)clamps caller-supplied grace to a 30s safety minimum so a rapid "recover interrupted" click from the UI can't clobber a just-claimed running job.Worker hygiene (B4, B7). Audio-rejection branch now logs DB update failures instead of dropping them silently.
index_image_files_batchtrims redundant per-branch status checks — keeps one pre-encode check per file and one batch-wide retain just before bulk store. ~2/3 fewer SQL round trips on 100-image batches.SQLCipher cleanup (B5, B6).
sqlite3_auto_extensionregistration moved into aOnceLock-guarded helper (theunsafe { transmute }is now in one place, not four). PRAGMA calls switched fromexecute_batch(&format!(...))to typedpragma_update.Shared file-type helpers (F1). New
src/lib/fileTypes.tsconsolidatesPURE_AUDIO_EXTENSIONSandTRANSCRIBABLE_EXTENSIONS. The consolidation caught a real drift:PreviewContextMenuwas missing.aacand.wmain its transcribable set. Rustindexing.rskeeps its const with a sync comment pointing at the TS source of truth.CSP (C1). Production Tauri config sets
default-src 'self'with explicit allowances forasset:/blob:/data:and the IPC channel. Asset protocol scope intentionally stays**— Cosmos' core value prop is previewing user files anywhere on disk. Dev config stays permissive for Vite HMR.Full Disk Access UX (P1). New macOS-gated probe attempts to read
~/Library/Safari. OnPermissionDenied, the frontend renders a dismissable amber banner with a deep link to the Privacy pane in System Settings. Replaces the silent "half my files weren't indexed" failure mode.Drive-by: flaky test fix.
generations_service::create_generationwas deriving the primary key fromtimestamp_millis(). Running the new tests changed timing enough to reliably collide. Switched touuid::Uuid::new_v4().simple()matching the pattern the rest of the codebase already uses. Full suite now passes deterministically at--test-threads=1(154/154).Docs.
README.mdgets a Platform Support table.docs/SECURITY.mdowns the obfuscated-file key storage for what it is (first-launch ergonomics, keychain migration on the roadmap) and names FileVault as the at-rest protection today.Tests
Eight new tests land in this PR, all passing:
cancellation_consistency_tests::replace_text_chunks_is_atomiccancellation_consistency_tests::replace_transcript_chunks_preserves_document_chunks_for_same_pathcancellation_consistency_tests::replace_with_invalid_embedding_leaves_prior_data_intact— REGRESSION for the delete-window bugcancellation_consistency_tests::purge_indexed_data_covers_text_and_vec_rowscancellation_consistency_tests::purge_indexed_data_is_idempotentjob_queue_tests::recover_stale_running_jobs_leaves_fresh_jobs_alonejob_queue_tests::recover_stale_jobs_at_startup_is_unconditionaljob_queue_tests::recover_stale_running_jobs_respects_grace_when_above_minimumFull suite: 154/154.
pnpm lintclean.Test plan
cargo test --manifest-path src-tauri/Cargo.toml -- --test-threads=1pnpm lintWhat's NOT in this PR
docs/SECURITY.md)TODOS.mdP1)TODOS.mdP2)🤖 Generated with Claude Code