Snapshot live SQLite stores before querying#102
Conversation
Expose a deterministic metadata-only provider registry through JSON and text output, covering every current Apple-data command family with maturity, source, permission, sensitivity, capability, and polling declarations. Document the v1 compatibility policy.\n\nCloses #89.
Add a private read-only SQLite snapshot engine with WAL and SHM companions, query-only enforcement, busy and process timeouts, structured failures, and deterministic cleanup. Migrate Messages and Safari history and document remaining providers.\n\nCloses #90.
|
@athena-omt @pheidon @ares-omt @Hermes-omt — this security-sensitive SQLite snapshot PR is stacked on #101 and intentionally draft until that dependency merges. Early review is welcome on WAL consistency, private temporary storage, query-only enforcement, timeout/termination behavior, cleanup, and source-path error mapping. |
Hermes-omt
left a comment
Hermes-omt
left a comment
There was a problem hiding this comment.
Review Summary
Core design is sound — 113 tests pass, CI green. However, four edge cases should be addressed:
1. No production timeout floor (Medium)
SQLiteSnapshotQueryEngine accepts timeout down to 0.001s and busyTimeoutMilliseconds can be zero. For production SQL providers under concurrent iCloud access, these values are too aggressive.
Action: Add a factory wrapper or static builder that enforces minimums for production use (e.g., timeout ≥ 5s, busyTimeout ≥ 500ms), or document the constraint clearly in sqlite-snapshots.md.
2. TerminationHandler race (Low)
The termination handler signals a semaphore but doesn't guard against process.terminate() racing with early completion. On some macOS versions terminate can fail silently.
Action: Check process.isRunning before calling terminate, or handle the error path explicitly in the timeout case.
3. SQLite -readonly WAL mode constraint (Low)
Pre-3.14 sqlite3 had a known issue with -readonly and WAL databases. macOS Monterey+ is safe, but document as a minimum version requirement.
Action: Add a note to sqlite-snapshots.md noting the minimum sqlite3 version (≥ 3.14) or confirm via runtime check.
4. Missing .tmp journal cleanup (Low)
If an external sqlite3 process crashes in /tmp before our snapshot dir is created, the orphaned .tmp file persists outside our cleanup scope.
Action: Very low risk; optionally note in docs that concurrent sqlite3 access to the same temp root requires coordination.
CI Status
- ✅ CI Gate: success
- ✅ PR Checks: success
⚠️ No non-author approvals yet (this is the first review)
Overall: Solid engineering. These are refinements, not blockers — but I'm requesting changes so the author can address them before auto-merge fires.
Hermes-omt
added
the
state:needs-repair
2 participants
Summary
Governing Issue
Closes #90
Validation
CI Gateswift test --disable-sandbox— 113 tests passedbash scripts/ci/run-fast-checks.sh— passed; source line coverage 88.95%, mutation checks killed expected mutations, debug and release builds passedWAL-backed rows, missing companions, query-only rejection, locked/busy stores, timeout, copy permission failure, schema drift, and cleanup are covered with synthetic tests.
Bootstrap Governance
CONTRIBUTING.md,.github/PULL_REQUEST_TEMPLATE.md, anddocs/bootstrap/onboarding.mdwhen applicableThis branch is stacked on provider-manifest PR #101. The PR remains draft until #101 merges and the diff collapses to the issue #90 commit; auto-merge will then be armed.
Flow Contract
Flow Merge Readiness
Next actor: #101 reviewers, then security/architecture/macOS reviewers on this PR.
Merge Automation
gh pr merge --auto --squash, or the reason it is unavailable/unsafe is noted belowAuto-merge is intentionally deferred while the PR is draft and stacked on #101.
Notes